1175794 - Refactor pageInfo.js to receive a media element at a time from messages and continually update UI

Status: RESOLVED FIXED

(Firefox :: Page Info Window, defect)

Comment 1

[Florian Quèze [:florian]]

[Tracking Requested - why for this release]: I suspect bug 1040947 (which landed for 42) is causing regressions:
- with e10s off, the content script is running on the same process as the UI, and will now freeze the whole UI until it is done traversing the full DOM of the page.
- with e10s on, the UI will be responsive, but nothing will be displayed in the media tab until the content script is done traversing the DOM. On very large page, this could take several seconds.

The goal of this bug is to fix for of these issues.

Comment 2

[Kate Glazko]

Tracking in 42 because regression, causes UI freezes.

Comment 3

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: bug1175794-make-mediatab-fetching-use-task-v1

Refactor page info media fetching to be a task that uses setTimeout so content process doesn't get blocked.

Remove use of variables on this for pageInfoListener. Instead pass variables that you need and return variables that you want to send back to pageinfo.js

Comment 4

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: bug1175794-make-mediatab-fetching-use-task-and-messages-v2

This is the previous patch with some changes.
But now we send media data back every 500 elements you loop through
The media tab will continuously update as more media data elements are sent.

I was wondering if this would be the better way to do it compared to the previous patch?

Comment 5

[Florian Quèze [:florian]]

(In reply to Jimmy Wang (:jimicy) - works on e10s stuff from comment #3)
> Created attachment 8643107 [details] [diff] [review]

> Remove use of variables on this for pageInfoListener. Instead pass variables
> that you need and return variables that you want to send back to pageinfo.js

The patch would be significantly easier to review if these changes were in their own patch.

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8643107 [details] [diff] [review]
bug1175794-make-mediatab-fetching-use-task-v1

Review of attachment 8643107 [details] [diff] [review]:
-----------------------------------------------------------------

Removing the review flag until the patch is split in a way that makes it easy to review.

::: browser/base/content/content.js
@@ +855,3 @@
>  
>      let frameOuterWindowID = message.data.frameOuterWindowID;
> +    let aWindow;

The 'a' prefix means 'argument', it's used for parameter names; please don't use it for local variables.

@@ +874,5 @@
>      sendAsyncMessage("PageInfo:data", pageInfoData);
>  
> +    // Separate step so page info dialog isn't blank while waiting for media tab data.
> +    this.getMediaInfo(aDocument, aWindow, strings).then(function(result) {
> +      sendAsyncMessage("PageInfo:mediaData", {imageViewRows: result});

With this patch, the media tab will stay blank until we are done walking the DOM, so your second attachment is really needed.

@@ +926,5 @@
>  
>      return docInfo;
>    },
>  
> +  getFeedsInfo: function(aDocument, strings) {

Please use a consistent naming scheme for parameters. They should either all have an 'a' prefix, or none of them should have it (which seems to be the style of the rest of this file).

@@ +1000,5 @@
> +        }
> +
> +        numOfNodes++;
> +        if (numOfNodes % 500 === 0) {
> +          // setTimeOut every 500 elements so you don't keep blocking the content process.

setTimeout, not setTimeOut
I don't think any other comment here is using "you"; change it to "we"?

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 8643112 [details] [diff] [review]
bug1175794-make-mediatab-fetching-use-task-and-messages-v2

Review of attachment 8643112 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/content.js
@@ +1011,5 @@
>            yield new Promise(resolve => setTimeout(resolve, 10));
>          }
>        }
>      }
> +    //return the remaining image view rows

What about doing the sendAsyncMessage("PageInfo:mediaData", ... call here?

::: browser/base/content/pageinfo/pageInfo.js
@@ +389,5 @@
>    });
>  
>    // Get the media elements from content script to setup the media tab.
>    mm.addMessageListener("PageInfo:mediaData", function onmessage(message){
> +    makeMediaTab(message.data.imageViewRows);

I think this makeMediaTab function should be renamed to something more descriptive of what the function does, as with the current name it seems like the function can't be called more than once without overwriting the effect of the previous call. Maybe 'insertMediaItems'? Or maybe 'addImages' to reflect that it's mostly just a loop calling addImage.

@@ +392,5 @@
>    mm.addMessageListener("PageInfo:mediaData", function onmessage(message){
> +    makeMediaTab(message.data.imageViewRows);
> +  });
> +
> +  mm.addMessageListener("PageInfo:finishedMediaTabSetup", function onmessage(message){

I'm wondering if it wouldn't be easier to have only a single message name rather than having a separate message to indicate the end.

If you want to use a single message name, the way to do it is to append a null element at the end of the array of the last message. Then the makeMediaTab function will detect this value, and return true to indicate it is done, and that the caller should remove the message listener.

@@ +397,1 @@
>      mm.removeMessageListener("PageInfo:mediaData", onmessage);

This is not removing the right 'onmessage' function.

Comment 8

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: 1-Bug1175794-make-getMediaInfo-self-contained

Comment 9

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: 2-Bug1175794-remove-use-of-variables-on-this-for-pageinfo

Comment 10

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: 3-bug1175794-minor-cleanup-for-pageinfo

windowInfo.isTopWindow = window == window.top;
> +    var titleFormat = windowInfo.isTopWindow ? "pageInfo.page.title"
> +                                             : "pageInfo.frame.title";

I switched the order. Because the previous check in the original page info was
> var titleFormat = gWindow != gWindow.top ? "pageInfo.frame.title"
>                                          : "pageInfo.page.title";

Comment 11

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: 4-bug1175794-mediatab-data-fetching-use-task-setTimeout-and-messages

Comment 12

[Florian Quèze [:florian]]

Comment on attachment 8644779 [details] [diff] [review]
1-Bug1175794-make-getMediaInfo-self-contained

Review of attachment 8644779 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/content.js
@@ +978,3 @@
>    },
>  
> +  processFrames: function (document, frameList, strings)

nit: no space between 'function' and '('.

@@ +998,4 @@
>    {
>      // Check for images defined in CSS (e.g. background, borders), any node may have multiple.
>      let computedStyle = elem.ownerDocument.defaultView.getComputedStyle(elem, "");
> +    let mediaElement = false;

nit: false doesn't seem the right value to initialize this variable, as it's not used as a boolean. Maybe set it to null instead?

Comment 13

[Florian Quèze [:florian]]

Comment on attachment 8644791 [details] [diff] [review]
4-bug1175794-mediatab-data-fetching-use-task-setTimeout-and-messages

Review of attachment 8644791 [details] [diff] [review]:
-----------------------------------------------------------------

All these mm changes don't really seem related to this bug. Are you trying to fix bug 1191869 (the description of which I don't really understand) at the same time?

::: browser/base/content/content.js
@@ +990,5 @@
>  
>          if (mediaNode) {
>            imageViewRows.push(mediaNode);
> +
> +          numOfMediaNodes++;

I don't think you need to keep this count. imageViewRows.length would give you the same value.

@@ +991,5 @@
>          if (mediaNode) {
>            imageViewRows.push(mediaNode);
> +
> +          numOfMediaNodes++;
> +          // Send imageViewsRows every 10 media nodes so media tab isn't blank.

Is having 2 different magic constants here really needed?

I was thinking we could just send whatever is already stored in imageViewsRows just before we do a setTimeout. Or is there a significant delay to process 500 DOM nodes?

@@ +1000,5 @@
> +          }
> +        }
> +
> +        numOfNodes++;
> +        if (numOfNodes % 500 === 0) {

== is enough.

::: browser/base/content/pageinfo/pageInfo.js
@@ +152,5 @@
>  
>  // mmm, yummy. global variables.
>  var gDocInfo = null;
>  var gImageElement = null;
> +var mm;

Do we really need this global variable? :-( If we do, it should be named gMm for consistency I guess.

Do we need to set it to null at some point? Wouldn't keeping a reference to the message manager from the Page Info window prevent it from being cleaned up when the related tab is closed?

@@ +404,5 @@
> +  }
> +
> +  // The page info media fetching has been completed.
> +  if (message.data.isComplete) {
> +    mm.removeMessageListener("PageInfo:mediaData", setupPageInfoMediaTab);

Would this be a good place to set the global mm variable to null? (obviously the unload code would then need a nullcheck).

Comment 14

[Jimmy Wang (:jimicy) - works on e10s stuff]

(In reply to Florian Quèze [:florian] [:flo] from comment #13)
> Comment on attachment 8644791 [details] [diff] [review]
> 4-bug1175794-mediatab-data-fetching-use-task-setTimeout-and-messages
> 
> Review of attachment 8644791 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> All these mm changes don't really seem related to this bug. Are you trying
> to fix bug 1191869 (the description of which I don't really understand) at
> the same time?

The mm changes aren't related to bug 1191869.
The page info dialog could be closed before the message listeners have a chance to get removed.
I'm not sure if I needed to remove them. Since the pageinfo.js script would be unloaded as well.
Maybe the previous way we did it without the global mm was better. If you could advise.

bug 1191869 is when you close a page info dialog, but the content process is still fetching the images. If you open a new page info dialog, the new page info dialog would receive these messages intended for the previous pageinfo. This only happened because of the dump statements which made it slow enough to happen. It's pretty hard to reproduce normally.

The solution would be to let the content process know that the pageinfo was closed and to stop doing the content image fetching for that pageinfo.

> I was thinking we could just send whatever is already stored in
> imageViewsRows just before we do a setTimeout. Or is there a significant
> delay to process 500 DOM nodes?

This ended up with sending a lot of empty imageViewRows for some sites.

Comment 15

[Florian Quèze [:florian]]

(In reply to Jimmy Wang (:jimicy) - works on e10s stuff from comment #14)
> (In reply to Florian Quèze [:florian] [:flo] from comment #13)
> > Comment on attachment 8644791 [details] [diff] [review]
> > 4-bug1175794-mediatab-data-fetching-use-task-setTimeout-and-messages
> > 
> > Review of attachment 8644791 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > All these mm changes don't really seem related to this bug. Are you trying
> > to fix bug 1191869 (the description of which I don't really understand) at
> > the same time?
> 
> The mm changes aren't related to bug 1191869.
> The page info dialog could be closed before the message listeners have a
> chance to get removed.
> I'm not sure if I needed to remove them. Since the pageinfo.js script would
> be unloaded as well.
> Maybe the previous way we did it without the global mm was better. If you
> could advise.

I think removing them makes sense. Could we keep the old way, and have the listener remove itself if window.closed (or something similar) is true?

> > I was thinking we could just send whatever is already stored in
> > imageViewsRows just before we do a setTimeout. Or is there a significant
> > delay to process 500 DOM nodes?
> 
> This ended up with sending a lot of empty imageViewRows for some sites.

You can skip sending the message if it's an empty one.

Comment 16

[Jimmy Wang (:jimicy) - works on e10s stuff]

(In reply to Florian Quèze [:florian] [:flo] from comment #15)
> I think removing them makes sense. Could we keep the old way, and have the
> listener remove itself if window.closed (or something similar) is true?

How can I check that the window is closed in the listener?
Correct me if I'm wrong, but I don't think there's anything on the xul window to check that.
Also if the window is closed, does the code in the listener still execute? Wouldn't pageinfo.js be unloaded?

The only way I can see how I would remove the listener is just removing it like I am right now in onUnloadPageInfo().

Would I be able to get the mm in onUnloadPageInfo()?
let mm = window.opener.gBrowser.selectedBrowser.messageManager;
instead of doing a global for mm. There's only one message manager for the selected browser so wouldn't it be the same as the above message manager?

Could you provide more detail on what you would like to see? Cause I'm confused how I would do it.
Otherwise, I'll go ahead and do the global mm and set mm to null when it is not needed.

Comment 17

[Florian Quèze [:florian]]

(In reply to Jimmy Wang (:jimicy) - works on e10s stuff from comment #16)
> (In reply to Florian Quèze [:florian] [:flo] from comment #15)
> > I think removing them makes sense. Could we keep the old way, and have the
> > listener remove itself if window.closed (or something similar) is true?
> 
> How can I check that the window is closed in the listener?
> Correct me if I'm wrong, but I don't think there's anything on the xul
> window to check that.

Like I said, window.closed

You can check it by running this in the Browser Console:

let win = Services.wm.getMostRecentWindow("Browser:page-info"); setTimeout(function() {alert(win.closed);}, 3000);
(close the Page Info dialog less than 3s after running it, and it will show true in the alert).

> Also if the window is closed, does the code in the listener still execute?
> Wouldn't pageinfo.js be unloaded?

Event listeners attached to DOM nodes would no longer execute (the DOM nodes don't exist anymore), but I think message listeners would still execute, and could prevent some of the window from being garbage collected.

> The only way I can see how I would remove the listener is just removing it
> like I am right now in onUnloadPageInfo().

What I'm suggesting is:

  mm.addMessageListener("PageInfo:mediaData", function onmessage(message) {
    if (window.closed) {
      mm.removeMessageListener("PageInfo:mediaData", onmessage);
      return;
    }
    ...
  });

Comment 18

[Florian Quèze [:florian]]

Comment on attachment 8644791 [details] [diff] [review]
4-bug1175794-mediatab-data-fetching-use-task-setTimeout-and-messages

Review of attachment 8644791 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/pageinfo/pageInfo.js
@@ +397,5 @@
> +  onLoadPermission(uri);
> +  securityOnLoad(uri, windowInfo);
> +}
> +
> +function setupPageInfoMediaTab(message) {

setupPageInfoMediaTab is not a good name for something we expect to be called more than once. Should this just be the insertMediaItems function where we could add a few lines at the top and bottom?

Comment 19

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: pageinfo-close-window.mp4

Something strange I noticed.
This is for closing pageinfo before all the media messages are all sent back and a final close message is sent back.

So for the first instance.
If I use Command+w to close pageinfo then dump(window.closed) will be run in the message listener callback to print true.

However if I close pageinfo by clicking the close button then the dump(window.closed) doesn't get run in the callback.

Is there a problem with how the current code works?

> function setupPageInfoMediaTab(message) {
>   if (message.data.imageViewRows.length) {
>     insertMediaItems(message.data.imageViewRows);
>   }
> 
>   dump(window.closed + "\n");
> 
>   // The page info media fetching has been completed.
>   if (message.data.isComplete) {
>     dump("isClosed----------------------------------------------------\n");
>     mm.removeMessageListener("PageInfo:mediaData", setupPageInfoMediaTab);
>     onFinished.forEach(function(func) { func(pageInfoData); });
>   }
> }

Comment 20

[Florian Quèze [:florian]]

Please attach a patch if you need help debugging. By the way, your video shows there are some JavaScript error being reported to your terminal. If insertMediaItems throws, your dump will never be reached.

Comment 21

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: bug1175794-pageinfo-message-debug

Hi Florian,
I attached a patch you can apply on top.

The error you saw is this
JavaScript error: chrome://browser/content/pageinfo/pageInfo.js, line 628: TypeError: gImageView.tree is null

but this isn't related to the message listener callback and doesn't stop the code executing.

I commented out this code so the error doesn't happen in the patch.
/*  if (message.data.imageViewRows.length) {
    insertMediaItems(message.data.imageViewRows);
  }*/

If you could take a look at this as well, I'd really appreciate it.

Comment 22

[Florian Quèze [:florian]]

(In reply to Jimmy Wang (:jimicy) - works on e10s stuff from comment #21)

The reason why you sometimes don't see your dump(window.closed) being printed is that you still have this code in the unload event listener:
  mm.removeMessageListener("PageInfo:mediaData", setupPageInfoMediaTab);

> The error you saw is this
> JavaScript error: chrome://browser/content/pageinfo/pageInfo.js, line 628:
> TypeError: gImageView.tree is null
> 
> but this isn't related to the message listener callback and doesn't stop the
> code executing.
> 
> I commented out this code so the error doesn't happen in the patch.
> /*  if (message.data.imageViewRows.length) {
>     insertMediaItems(message.data.imageViewRows);
>   }*/
> 
> If you could take a look at this as well, I'd really appreciate it.

I haven't been able to reproduce this exact error, but I got:
JavaScript error: chrome://browser/content/pageinfo/pageInfo.js, line 72: TypeError: this.tree is null
when I uncommented the insertMediaItems call.

It makes sense that the tree no longer works/exists when the window is closed.

Comment 23

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: 4-bug1175794-mediatab-data-fetching-use-task-setTimeout-and-messages-v2

Removed the global mm. Changed back to an anomymous function for the message listeners.

Still need the isComplete on the message for "PageInfo:mediaData" to execute functions after all the images have been added to pageinfo.

I want to stick with sending imageViewRows every 10 media nodes instead of every 500 elements looped since you would usually be sending 1 element per message.

> +          // Send imageViewsRows every 10 media nodes so media tab isn't blank.
> +          if (imageViewRows.length % 10 === 0) {
> +            sendAsyncMessage("PageInfo:mediaData",
> +                             {imageViewRows: imageViewRows, isComplete: false});
> +            imageViewRows = []; //clear the sent imageViewRows
> +          }

Comment 24

[Florian Quèze [:florian]]

(In reply to Jimmy Wang (:jimicy) - works on e10s stuff from comment #23)

> I want to stick with sending imageViewRows every 10 media nodes instead of
> every 500 elements looped since you would usually be sending 1 element per
> message.

Why would that be a problem?

Comment 25

[Jimmy Wang (:jimicy) - works on e10s stuff]

(In reply to Florian Quèze [:florian] [:flo] from comment #24)
> (In reply to Jimmy Wang (:jimicy) - works on e10s stuff from comment #23)
> 
> > I want to stick with sending imageViewRows every 10 media nodes instead of
> > every 500 elements looped since you would usually be sending 1 element per
> > message.
> 
> Why would that be a problem?

If we had a page with a lot of media elements. Wouldn't sending 1 element per message be slower than sending 10 elements at a time in a message?

Comment 26

[Florian Quèze [:florian]]

(In reply to Jimmy Wang (:jimicy) - works on e10s stuff from comment #25)
> (In reply to Florian Quèze [:florian] [:flo] from comment #24)
> > (In reply to Jimmy Wang (:jimicy) - works on e10s stuff from comment #23)
> > 
> > > I want to stick with sending imageViewRows every 10 media nodes instead of
> > > every 500 elements looped since you would usually be sending 1 element per
> > > message.
> > 
> > Why would that be a problem?
> 
> If we had a page with a lot of media elements. Wouldn't sending 1 element
> per message be slower than sending 10 elements at a time in a message?

It would certainly use more CPU, but the kind of performance that matters here is perceived performance (ie. is the UI responsive). Before the changes to make Page Info e10s friendly, the Media tab was updated each time we found a media element.

And we would still be sending at most one message per 500 dom nodes. So the case that you describe where we would be sending plenty of 1-element messages is a large page with plenty of images, but not more than one image out of 500 DOM nodes. That's a lot of non-image nodes.

Comment 27

[Florian Quèze [:florian]]

Comment on attachment 8647333 [details] [diff] [review]
4-bug1175794-mediatab-data-fetching-use-task-setTimeout-and-messages-v2

Review of attachment 8647333 [details] [diff] [review]:
-----------------------------------------------------------------

Looks pretty good; I still have a few nits.

::: browser/base/content/content.js
@@ +980,2 @@
>    {
> +    let numOfNodes = 0;

nit: is this variable name similar to anything we already have in this file? If not, I think I would prefer 'nodeCount' (to avoid abbreviations when we don't need them).

@@ +990,5 @@
>          if (mediaNode) {
>            imageViewRows.push(mediaNode);
> +
> +          // Send imageViewsRows every 10 media nodes so media tab isn't blank.
> +          if (imageViewRows.length % 10 === 0) {

I still think this would be changed to:
 if (imageViewRows.length) and the whole block moved inside the block executed every 500 nodes.

@@ +998,5 @@
> +          }
> +        }
> +
> +        numOfNodes++;
> +        if (numOfNodes % 500 === 0) {

nit: ==

Also, I think these 2 lines can be merged to
if (++numOfNodes % 500 == 0)

::: browser/base/content/pageinfo/pageInfo.js
@@ +389,5 @@
>    });
>  
>    // Get the media elements from content script to setup the media tab.
> +  mm.addMessageListener("PageInfo:mediaData", function onmessage(message) {
> +    // Pageinfo window was closed.

nit: 'Page Info' not 'Pageinfo'; let's be consistent :).

Comment 28

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: 4-bug1175794-mediatab-data-fetching-use-task-setTimeout-and-messages-v3

I see your point about UI responsiveness.
I made a new change to just send back a mediaNode everytime it is found. Then you don't have to check every 500 elements if there is a mediaNode to send back.

Let me know what you think!

Comment 29

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: 1-Bug1175794-make-getMediaInfo-self-contained-v2

Minor fixes from your suggestion.

::: browser/base/content/content.js
@@ +978,3 @@
>    },
>  
> +  processFrames: function(document, frameList, strings)

Removed space between 'function' and '('.

@@ +998,4 @@
>    {
>      // Check for images defined in CSS (e.g. background, borders), any node may have multiple.
>      let computedStyle = elem.ownerDocument.defaultView.getComputedStyle(elem, "");
> +    let mediaElement = null;
Set mediaElement to null instead of false

Comment 30

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: 2-Bug1175794-remove-use-of-variables-on-this-for-pageinfo-v2

Did not modify this patch.
Made change in 1-Bug1175794-make-getMediaInfo-self-contained-v2
Putting this up so interdiffs work properly.

Comment 31

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: 3-bug1175794-minor-cleanup-for-pageinfo-v2

Did not modify this patch.
Made change in 1-Bug1175794-make-getMediaInfo-self-contained-v2
Putting this up so interdiffs work properly.

Comment 32

[Florian Quèze [:florian]]

Comment on attachment 8647348 [details] [diff] [review]
4-bug1175794-mediatab-data-fetching-use-task-setTimeout-and-messages-v3

Review of attachment 8647348 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Jimmy Wang (:jimicy) - works on e10s stuff from comment #28)

> I made a new change to just send back a mediaNode everytime it is found.
> Then you don't have to check every 500 elements if there is a mediaNode to
> send back.

I _think_ that's OK. But we may have to revisit if the flood of messages is somehow causing performance issues (eg. on a large page with a CSS rule like "* { background-image: url(...); }"). The reason I suggested doing it every 500 DOM nodes is that it matched the previous behavior in terms of responsiveness: the non-e10s Page Info was inserting the media elements one at a time in its array, but the UI would only be refreshed on the screen when we stopped doing JS computations (that's why we had a setTimeout call; to let the displayed UI refresh itself).

::: browser/base/content/content.js
@@ +990,5 @@
> +          sendAsyncMessage("PageInfo:mediaData",
> +                           {imageViewRow: mediaNode, isComplete: false});
> +        }
> +
> +        if (++nodeCount% 500 === 0) {

nit: space before '%' and '==' rather than '==='

::: browser/base/content/pageinfo/pageInfo.js
@@ +402,5 @@
> +      onFinished.forEach(function(func) { func(pageInfoData); });
> +      return;
> +    }
> +
> +    addImage.apply(null, message.data.imageViewRow);

why not just addImage(message.data.imageViewRow); ?

Comment 33

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: 4-bug1175794-mediatab-data-fetching-use-task-setTimeout-and-messages-v4

Minor fixes from suggestions

Comment 34

[Florian Quèze [:florian]]

Comment on attachment 8647398 [details] [diff] [review]
4-bug1175794-mediatab-data-fetching-use-task-setTimeout-and-messages-v4

Review of attachment 8647398 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.

Note: I haven't had time to test the patch locally myself.

Comment 35

[Jimmy Wang (:jimicy) - works on e10s stuff]

Try push looks good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=35671219cc85

Comment 36

[Jimmy Wang (:jimicy) - works on e10s stuff]

Please merge these patches in this order.
1-Bug1175794-make-getMediaInfo-self-contained-v2
2-Bug1175794-remove-use-of-variables-on-this-for-pageinfo-v2
3-bug1175794-minor-cleanup-for-pageinfo-v2
4-bug1175794-mediatab-data-fetching-use-task-setTimeout-and-messages-v4

Thank you

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/240c0bb2f67c
https://hg.mozilla.org/integration/fx-team/rev/d6d3b2e8dfc7
https://hg.mozilla.org/integration/fx-team/rev/47b1d38a23f5
https://hg.mozilla.org/integration/fx-team/rev/40b5df5cdc59

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/240c0bb2f67c
https://hg.mozilla.org/mozilla-central/rev/d6d3b2e8dfc7
https://hg.mozilla.org/mozilla-central/rev/47b1d38a23f5
https://hg.mozilla.org/mozilla-central/rev/40b5df5cdc59

Comment 39

[Florian Quèze [:florian]]

Comment on attachment 8647398 [details] [diff] [review]
4-bug1175794-mediatab-data-fetching-use-task-setTimeout-and-messages-v4

Approval Request Comment
[Feature/regressing bug #]: regression from bug 1040947
[User impact if declined]: see comment 1
[Describe test coverage new/current, TreeHerder]: baked on central for a few days
[Risks and why]: seems OK to me for aurora, it's cleanup/polishing of a patch that landed on central at the end of the 42 cycle.
[String/UUID change made/needed]: none.

Note: this approval request is for the 4 patches that landed here, not just this specific attachment.

Comment 40

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8647398 [details] [diff] [review]
4-bug1175794-mediatab-data-fetching-use-task-setTimeout-and-messages-v4

Regression, taking it.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5fef83dfe7b0
https://hg.mozilla.org/releases/mozilla-aurora/rev/23d7dfbc4e1d
https://hg.mozilla.org/releases/mozilla-aurora/rev/1c4c3d6c5fd3
https://hg.mozilla.org/releases/mozilla-aurora/rev/c8ca46b051f0