[e10s] Opening page info from a remote tab is sluggish

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: mconley, Assigned: jimicy)

Tracking

(Depends on 1 bug, Blocks 2 bugs)

30 Branch
Firefox 42
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(e10sm8+, firefox42 fixed)

Details

Attachments

(3 attachments, 21 obsolete attachments)

62.51 KB, patch
mconley
: review+
Details | Diff | Splinter Review
5.46 KB, patch
florian
: review+
Details | Diff | Splinter Review
62.82 KB, patch
jimicy
: checkin+
Details | Diff | Splinter Review
STR:

1) Open a new e10s window, and browse to some page.
2) Ensure there is an underline under the tab title to indicate that you're looking at a remote tab.
3) Right click on the content, and choose "Page Info"

The Page Info dialog comes up, but it's pretty sluggish to finish loading.

Comment 1

5 years ago
See this too. Paage Source Dialog is also rather sluggish
Reporter

Updated

5 years ago
Duplicate of this bug: 1080152

Comment 3

5 years ago
One more observation:
If I open page info, General, Permissions and Security show up immediately, Media shows up 1 to 2 seconds after the other sections of page info.
Reporter

Updated

5 years ago
Duplicate of this bug: 1100111
Reporter

Updated

5 years ago
Duplicate of this bug: 1112327
This site really horks the page info window: http://beta.unity3d.com/jonas/DT2/
Reporter

Updated

4 years ago
Blocks: 1109869
According to the Browser Console, the Page Info dialog uses an enormous number of unsafe CPOWs. I suspect it's attempting to walk the DOM tree of the contentDocument CPOW to get information about various scripts, images, etc.

Re-nomming, since this seems like a pretty bad usage of unsafe CPOWs.
Blocks: 1162718
Assignee: nobody → jimmyw22
Posted patch metaView-e10s-on-pageInfo (obsolete) — Splinter Review
Work in progress:
move unsafe CPOW operations on the content page to get the meta info and image content nodes to a content script pageContent.js

Next steps:
Remove excess logic and refactor code.

Need to have discussion on how the current pageInfo.js file is getting args.
How will the browser pass the selected image element to the content script, pageContent.js

So when you do
go to twitter.com -> right click -> select view image info -> this sets args on the pageInfo.js

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/pageinfo/pageInfo.js#451

args being passed from browser.js when you do view image info
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2406
Depends on: 1170291
Posted patch metaView-e10s-on-pageInfo (obsolete) — Splinter Review
fixed broken tests
browser/base/content/test/general/browser_bug460146.js
browser/base/content/test/general/browser_pageInfo.js
browser/base/content/test/plugins/browser_pageInfo_plugins.js
Attachment #8613665 - Attachment is obsolete: true
Also fixes
https://bugzilla.mozilla.org/show_bug.cgi?id=1171031

Functionality of pageInfo should be the same. Moved most of the Unsafe CPOW operations on getting meta info and image elements for media tab to a content script.

TODOs for other bugs
Remove unsafe CPOW usage in feeds.js, permission.js and security.js
Attachment #8614792 - Attachment is obsolete: true
Attachment #8616042 - Flags: review?(florian)
Attachment #8616042 - Flags: review?(florian)
Status: NEW → ASSIGNED
moved my pageContent.js script to content.js

Also fixes
https://bugzilla.mozilla.org/show_bug.cgi?id=1171031

Functionality of pageInfo should be the same. Moved most of the Unsafe CPOW operations on getting meta info and image elements for media tab to a content script.

TODOs for other bugs
Remove unsafe CPOW usage in feeds.js, permission.js and security.js
Attachment #8616042 - Attachment is obsolete: true
Attachment #8616729 - Flags: review?(florian)
Duplicate of this bug: 1171031
Duplicate of this bug: 866413
Comment on attachment 8616729 [details] [diff] [review]
gImageElement-metaView-e10s-on-pageInfo

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

Not a full review, I figured I had already included enough comments that I would need to look at the whole patch again once you have a new version. This seems to be going in a reasonable direction. I'm not sure how much you are willing to refactor/cleanup the existing code, vs just making it work OK with e10s (eg. my suggestion of using Tasks may be significantly outside the scope of this bug). For future patches, please attach a diff with 8 lines of context.

::: browser/base/content/content.js
@@ +737,5 @@
> +
> +let pageInfoListener = {
> +
> +  init: function(chromeGlobal) {
> +    chromeGlobal.addMessageListener("pageinfo:args", this, false, true);

"pageinfo:args" isn't a descriptive message. I don't understand what 'args' means ;-).

@@ +740,5 @@
> +  init: function(chromeGlobal) {
> +    chromeGlobal.addMessageListener("pageinfo:args", this, false, true);
> +  },
> +
> +  //spec says looks for a receiveMessage property.

not a useful comment.

@@ +743,5 @@
> +
> +  //spec says looks for a receiveMessage property.
> +  receiveMessage: function(message) {
> +    this.gDocument = content.document;
> +    this.gWindow = content.window;

How does this work for frames? ("This Frame -> View Frame Info" from the context menu on an iframe.)

@@ +746,5 @@
> +    this.gDocument = content.document;
> +    this.gWindow = content.window;
> +    this.imageRows = [];
> +    this.gFrameList = [];
> +    this.gStrings = message.data.gStrings;

please remove all the g prefixes here.

@@ +779,5 @@
> +    return docInfo;
> +  },
> +
> +  getImageViewInfo: function() {
> +    this.makeTabs();

This doesn't look like a very useful function if all it does is calling another function.

@@ +821,5 @@
> +    }
> +  },
> +
> +  /**
> +   * This function's previous purpose in pageInfo.js was to get the first 500 media elements.

Not 500 media elements. It was processing 500 elements from the DOM (any element, including the completely uninteresting ones like <br>), and then posing to let the UI update.

@@ +824,5 @@
> +  /**
> +   * This function's previous purpose in pageInfo.js was to get the first 500 media elements.
> +   * Then do setTimeOut so the UI doesn't hang and the function was continue to fetch media elements.
> +   * We sendAsyncMessage after it processes all the elements. So setTimeOut doesn't do anything.
> +   * #TODO: refactor pageInfo.js to receive 500 media elements at a time from messages and continually update UI

Do you intend to do this in the next version of this patch, or do you expect to do it in a follow-up bug?

Now that we are on the child process, we may not need the setTimeout calls anymore.

This code would likely be way more readable if we made it use Tasks. I wonder if we could send a message immediately whenever we find an interesting element?

@@ +922,5 @@
> +   * Set up a JSON object with all the instanceOf and other infomation that
> +   * makePreview in pageInfo.js uses to display the preview.
> +   */
> +
> +  makeElem: function (url, type, alt, elem, isBG)

I think you can come up with a better name for this function.

@@ +927,5 @@
> +  {
> +    // Interface for image loading content
> +    const nsIImageLoadingContent = Components.interfaces.nsIImageLoadingContent;
> +
> +    let item = elem;

Why aren't you naming the parameter 'item' directly?

@@ +928,5 @@
> +    // Interface for image loading content
> +    const nsIImageLoadingContent = Components.interfaces.nsIImageLoadingContent;
> +
> +    let item = elem;
> +    let elem2 = {};

Naming this "result" would be more readable to me.

@@ +1067,5 @@
> +
> +    text = text.replace(middleRE, " ");
> +    return text.replace(endRE, "");
> +  }
> +}

nit: missing ';'

::: browser/base/content/pageinfo/pageInfo.js
@@ +57,5 @@
>    },
>  
> +  addRows: function(rows)
> +  {
> +    for (var i = 0; i < rows.length; i++)

Is 'rows' a JS array? If so, you can simplify to:
  for (let row of rows)
    this.addRow(row);

@@ +148,4 @@
>  // mmm, yummy. global variables.
>  var gWindow = null;
>  var gDocument = null;
> +var docInfo = null;

this wants to have a 'g' prefix like the other global variables around.

@@ +148,5 @@
>  // mmm, yummy. global variables.
>  var gWindow = null;
>  var gDocument = null;
> +var docInfo = null;
> +var mm = null;

Do you actually need this mm global variable?

@@ +169,4 @@
>  // one nsITreeView for each tree in the window
>  var gMetaView = new pageInfoTreeView('metatree', COPYCOL_META_CONTENT);
>  var gImageView = new pageInfoTreeView('imagetree', COPYCOL_IMAGE);
> +var gViewInfo; //stores the pageInfo for metatree and imagetree from pageContent script

add a line break before this line, and put the comment above the variable declaration.

@@ +348,4 @@
>      gDocument = gWindow.document;
>    }
>  
> +  mm = window.opener.gBrowser.selectedBrowser.messageManager;

This looks like 'selectedBrowser' should be a temporary variable used by both this and the instruction 3 lines above.

@@ +361,4 @@
>              .notifyObservers(window, "page-info-dialog-loaded", null);
>  }
>  
> +function loadPageInfo(args)

Is this args parameter actually used?

@@ +365,3 @@
>  {
> +  //look for pageInfoListener in content.js. Sends message to listener with gString args
> +  mm.sendAsyncMessage("pageinfo:args", {gStrings: gStrings});

can mm be a local variable defined here?

@@ +378,2 @@
>  
> +    document.getElementById("main-window").setAttribute("relatedUrl", docInfo.location);

Isn't the location of the document already available immediately in the parent process without waiting for a message from the child?

@@ +380,5 @@
> +
> +    makeGeneralTab();
> +    makeMediaTab();
> +
> +    gMetaView.addRows(gViewInfo.gMetaView.rows);

The gViewInfo.gMetaView naming is awkward. The 'g' prefix means 'global', and gMetaView is obviously not global here.

@@ +464,4 @@
>    gImageElement = args && args.imageElement;
>  
>    /* Load the page info */
> +  loadPageInfo(args);

Looks like this change can be reverted.

@@ +576,2 @@
>  
> +  for (var i=0; i<imageRows.length; i++) {

for (let image of gViewInfo.gImageView) {

@@ +578,5 @@
> +    let url  = imageRows[i][0];
> +    let type = imageRows[i][1];
> +    let alt  = imageRows[i][2];
> +    let elem = imageRows[i][3];
> +    let isBg = imageRows[i][4];

let [url, type, alt, elem, isBg] = image;

@@ +579,5 @@
> +    let type = imageRows[i][1];
> +    let alt  = imageRows[i][2];
> +    let elem = imageRows[i][3];
> +    let isBg = imageRows[i][4];
> +    addImage(url, type, alt, elem, isBg);

Or you can even do addImage.apply(null, image);

@@ +584,2 @@
>    }
> +  onFinished.push(selectImage);

Is there a reason for pushing selectImage into onFinished, rather than just calling it directly?

@@ +779,4 @@
>      permissionManager.remove(uri.host, "image");
>  }
>  
> +//Call this function whenever you select a row on the media tab.

nit: space after '//'. I'm not sure this comment actually helps by the way. Who is expected to call this?

@@ +899,5 @@
>      var width = 0, height = 0;
>  
> +    if ((item.HTMLLinkElement || item.HTMLInputElement ||
> +         item.HTMLImageElement ||
> +         item.SVGImageElement ||

nit: The wrapping is strange there, can you put both of these tests on the same line to match what was done on line 901?

@@ +1043,3 @@
>  //******** Other Misc Stuff
>  // Modified from the Links Panel v2.3, http://segment7.net/mozilla/links/links.html
>  // parse a node to extract the contents of the node

If you are removing the getValueText function, you should also remove this 2 lines of comment that were above it.

@@ +1130,5 @@
>  
>    var tree = document.getElementById("imagetree");
>    for (var i = 0; i < tree.view.rowCount; i++) {
> +    //if the image element is the same as the image selected from the right click "view image info"
> +    //make sure the selected image is focusable (able to be selected)

nit: space after '//', start sentences with an uppercase letter and finish with a period.

@@ +1131,5 @@
>    var tree = document.getElementById("imagetree");
>    for (var i = 0; i < tree.view.rowCount; i++) {
> +    //if the image element is the same as the image selected from the right click "view image info"
> +    //make sure the selected image is focusable (able to be selected)
> +    if (gImageElement && gImageElement.currentSrc == gImageView.data[i][COL_IMAGE_ADDRESS] &&

wrap this line after &&
Attachment #8616729 - Flags: review?(florian) → feedback+
Duplicate of this bug: 1162718
You're right it didn't work for frames before. Fixed to work with iframes. I pass the Frame's OuterWindowID down from nsContextMenu.js and then use it to get the frame's document and window.
site to test on: http://nunzioweb.com/iframes-example.htm

@@ +378,2 @@
> +    document.getElementById("main-window").setAttribute("relatedUrl", docInfo.location);

Isn't the location of the document already available immediately in the parent process without waiting for a message from the child

It's waiting for docInfo's value. Which is set from the message from the child.
Renamed to gDocInfo (since it's global)

>      var width = 0, height = 0;
>
> +    if ((item.HTMLLinkElement || item.HTMLInputElement ||
> +         item.HTMLImageElement ||
> +         item.SVGImageElement ||

nit: The wrapping is strange there, can you put both of these tests on the same line to match what was done on line 901?

line 903:

> if ((item.HTMLLinkElement || item.HTMLInputElement || item.HTMLImageElement || item.SVGImageElement ||
>         (item.HTMLObjectElement && mimeType && mimeType.startsWith("image/")) || isBG) &&
>         isProtocolAllowed) {

Not sure if this is what you meant by put these tests on the same line. If I put them all on 1 line it'll be too long.

You're right we don't need setTimeOut.
As of right now I'm just trying to get to work the same way and remove unsafe CPOW operations.
If I do make it use Tasks, it will be in a follow-up bug. If you don't mind reviewing that?
Attachment #8616729 - Attachment is obsolete: true
Attachment #8620435 - Flags: review?(florian)
Attachment #8620435 - Attachment is obsolete: true
Attachment #8620435 - Flags: review?(florian)
Attachment #8621084 - Flags: review?(florian)
8 line diff version of the patch.
Attachment #8621084 - Attachment is obsolete: true
Attachment #8621084 - Flags: review?(florian)
Attachment #8621100 - Flags: review?(florian)
Comment on attachment 8621100 [details] [diff] [review]
bug-1040947-metaView-mediaView-e10s-on-pageInfo

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

Looks better, but there are still a couple things to address. I read the code in content.js quickly as I assumed most of it had been moved from pageInfo.js and we want to make only minimal changes to it.

::: browser/base/content/browser.js
@@ +2382,5 @@
>  }
>  
>  // doc - document to use for source, or null for this window's document
>  // initialTab - name of the initial tab to display, or null for the first tab
>  // imageElement - image to load in the Media Tab of the Page Info window; can be null/omitted

Please update the comment here to document frameOuterWindowID

@@ +2384,5 @@
>  // doc - document to use for source, or null for this window's document
>  // initialTab - name of the initial tab to display, or null for the first tab
>  // imageElement - image to load in the Media Tab of the Page Info window; can be null/omitted
> +function BrowserPageInfo(doc, initialTab, imageElement, frameOuterWindowID) {
> +  var args = {doc: doc, initialTab: initialTab, imageElement: imageElement, frameOuterWindowID: frameOuterWindowID};

Please wrap this long line.

::: browser/base/content/content.js
@@ +762,5 @@
> +
> +  receiveMessage: function(message) {
> +    this.imageViewRows = [];
> +    this.frameList = [];
> +    this.strings = message.data.gStrings;

This should be named strings rather than gStrings.

@@ +764,5 @@
> +    this.imageViewRows = [];
> +    this.frameList = [];
> +    this.strings = message.data.gStrings;
> +
> +    let frameOuterWindowID = message.data.frameOuterWindowID

missing ';'

@@ +766,5 @@
> +    this.strings = message.data.gStrings;
> +
> +    let frameOuterWindowID = message.data.frameOuterWindowID
> +
> +    // If inside iframe then get the iframe's window and document.

I mentioned iframe as an example in my previous review, but this applies to frames in general, so saying "If inside a frame" would be more correct than "If inside iframe".

@@ +768,5 @@
> +    let frameOuterWindowID = message.data.frameOuterWindowID
> +
> +    // If inside iframe then get the iframe's window and document.
> +    if (frameOuterWindowID) {
> +      let wMediator = Cc["@mozilla.org/appshell/window-mediator;1"].getService(Ci.nsIWindowMediator);

Services.wm

@@ +838,5 @@
> +      this.frameList.shift();
> +      setTimeout(iterator => this.doGrab(iterator), 10, iterator);
> +    }
> +    else {
> +      //send the message after all the media elements have been walked through

nit: space after //, uppercase first letter, period ;).

@@ +839,5 @@
> +      setTimeout(iterator => this.doGrab(iterator), 10, iterator);
> +    }
> +    else {
> +      //send the message after all the media elements have been walked through
> +      let pageInfoData = {metaViewRows: this.getMetaInfo(), imageViewRows: this.imageViewRows, docInfo: this.getDocumentInfo()};

nit: wrap this long line.

@@ +845,5 @@
> +    }
> +  },
> +
> +  /**
> +   * This function's previous purpose in pageInfo.js was to get the first 500 media elements.

My previous review comment here still applies.

::: browser/base/content/pageinfo/pageInfo.js
@@ +58,5 @@
>  
> +  addRows: function(rows)
> +  {
> +    for (let row of rows)
> +      this.addRow(row);

This will call N times this.rowCountChanged and this.selection.select. Can we do better?

Maybe instead you could use .concat to modify this.data, and make the addRow method call addRows?

Is addRow still used after this patch?

@@ +168,5 @@
>  var gMetaView = new pageInfoTreeView('metatree', COPYCOL_META_CONTENT);
>  var gImageView = new pageInfoTreeView('imagetree', COPYCOL_IMAGE);
>  
> +// Stores the pageInfoData sent back from content script.
> +var pageInfoData;

If this is staying a global variable, please name it with a 'g' prefix to be consistent with the other global variables here.

Does this actually need to be a global variable? Or could it be given as a parameter to makeGeneralTab and makeMediaTab?

@@ +169,5 @@
>  var gImageView = new pageInfoTreeView('imagetree', COPYCOL_IMAGE);
>  
> +// Stores the pageInfoData sent back from content script.
> +var pageInfoData;
> +var gDocInfo;

Same question here, does it need to be a global variable?

@@ +309,3 @@
>  // These functions are called once when all the elements in all of the target
>  // document (and all of its subframes, if any) have been processed
> +// Used by browser_bug460146.js to excute a test function after images have loaded on media tab

Please wrap before 80 chars.

@@ +366,2 @@
>  
> +  //look for pageInfoListener in content.js. Sends message to listener with gString as an argument

nit(copied from previous review): space after '//', start sentences with an uppercase letter and finish with a period.

@@ +366,3 @@
>  
> +  //look for pageInfoListener in content.js. Sends message to listener with gString as an argument
> +  mm.sendAsyncMessage("PageInfo:getData", {gStrings: gStrings, frameOuterWindowID: frameOuterWindowID});

Please wrap before 80 chars. Note: we don't apply this rule very strictly when finding a reasonable place to break a line is difficult and wrapping would reduce readability, but here it's easy: either before "{gStrings" or before "frameOuterWindowId:".

@@ +369,3 @@
>  
> +  //setup the page info dialog once you get json info for content (tab page) from content script
> +  mm.addMessageListener("PageInfo:data", function onmessage(message) {

Do we need to check here that |message| is actually the answer to our PageInfo:getData message? I wonder what will happen if a user opens Page Info several times quickly. Won't we show the information related to the last page in all the dialogs?

@@ +380,5 @@
> +    document.getElementById("main-window").setAttribute("relatedUrl", gDocInfo.location);
> +
> +    makeGeneralTab();
> +    makeMediaTab();
> +

nit: Please remove this empty line that doesn't help with readability.

@@ +461,5 @@
>      gWindow = gDocument.defaultView;
>    }
>  
>    gImageElement = args && args.imageElement;
> +  // frameOuterWindowID is passed if you view frame info inside an iframe. If not passed then it's false.

"If not passed then it's false." I don't see any code implementing this, so I think you can remove this sentence. It seems the value will be undefined when it's not passed, which is the standard value for an optional parameter without default value.

@@ +529,3 @@
>    document.getElementById("encodingtext").value = encoding;
>  
> +  //get the number of metaNode elements. number of rows of meta info to display

I don't understand this comment; rephrase? :-)

@@ +529,4 @@
>    document.getElementById("encodingtext").value = encoding;
>  
> +  //get the number of metaNode elements. number of rows of meta info to display
> +  let metaViewRows = pageInfoData.metaViewRows

missing ';'

@@ +572,5 @@
> +/**
> + * Make the media tab.
> + * Loop through array of mediaRows (images, video, swf, etc) sent back from content script.
> + * call addImage passing in the args to add the media rows to the view on Media Tab
> + * Once the imageRows have been added to gImageView. Loop through onFinished and execute the functions on it.

I can't parse this comment. The point of a comment here should be to help the reader understand faster what the code does. In this specific case, reading the code (it's a short function!) is faster for me than reading the current comment. You probably want to simplify what the comment says :-).

@@ +584,3 @@
>    }
> +  selectImage();
> +  onFinished.forEach(function(func) { func(); });

I think this would make more sense if we called it from the function that's calling makeMediaTab();

Assuming you take into account my suggestion of changing pageInfoData and gDocInfo from global variables into parameters, these functions probably want to receive these parameters too.

If you do this, please remember to update the documentation above 'var onFinished = [ ];', thanks!

@@ +894,5 @@
>      newImage.id = "thepreviewimage";
>      var physWidth = 0, physHeight = 0;
>      var width = 0, height = 0;
>  
> +    if ((item.HTMLLinkElement || item.HTMLInputElement || item.HTMLImageElement || item.SVGImageElement ||

This would look better like this:
    if ((item.HTMLLinkElement || item.HTMLInputElement ||
         item.HTMLImageElement || item.SVGImageElement ||

Sorry for being ambiguous in my previous comment.

@@ +896,5 @@
>      var width = 0, height = 0;
>  
> +    if ((item.HTMLLinkElement || item.HTMLInputElement || item.HTMLImageElement || item.SVGImageElement ||
> +         (item.HTMLObjectElement && mimeType && mimeType.startsWith("image/")) || isBG) &&
> +         isProtocolAllowed) {

Wrapping before isProtocolAllowed is misleading here, because the && at the end is barely noticeable in a block where all lines end with ||.

@@ +1122,5 @@
>  
>    var tree = document.getElementById("imagetree");
>    for (var i = 0; i < tree.view.rowCount; i++) {
> +    // If the image element is the same as the image selected from the right click "view image info".
> +    // Make sure the selected image is focusable (able to be selected).

This comment is also hard to read for me. I don't see any code doing "Make sure the selected image is focusable (able to be selected)".

@@ +1123,5 @@
>    var tree = document.getElementById("imagetree");
>    for (var i = 0; i < tree.view.rowCount; i++) {
> +    // If the image element is the same as the image selected from the right click "view image info".
> +    // Make sure the selected image is focusable (able to be selected).
> +    if (gImageElement &&

You don't need this check, there's already an early return a couple lines above.
Attachment #8621100 - Flags: review?(florian) → feedback+
Posted patch e10s-on-pageInfo (obsolete) — Splinter Review
This patch includes https://bugzilla.mozilla.org/show_bug.cgi?id=1170291 's fixes merged in. Since too many parts overlap and doing them separately would be more work.

Removed comments that didn't make sense. And simplified comments
---------------------------------------------------
@@ +340,5 @@
>               window.arguments.length >= 1 &&
>               window.arguments[0];
>  
>    if (!args || !args.doc) {
> +    gDocument = window.opener.gBrowser.selectedBrowser.contentWindowAsCPOW.document;

Why is this still needed? I thought the goal of this second patch was to get rid of the remaining CPOWs?

You're right. Would this be a good solution to remove use of gDocument?
I have to modify `contentAreaUtil.js` to account for a property on a json for the doc instead of actually using the document itself to check `PrivateBrowsingUtils.isContentWindowPrivate(...)`

>+++ b/browser/base/content/content.js
> +    let defaultView = {};
> +    defaultView.isContentWindowPrivate = PrivateBrowsingUtils.isContentWindowPrivate(content);
> +    docInfo.defaultView = defaultView;

>+++ b/toolkit/content/contentAreaUtils.js
> +  let isPrivate;
> +  if (typeof persistArgs.initiatingWindow.isContentWindowPrivate !== 'undefined') {
> +    isPrivate = persistArgs.initiatingWindow.isContentWindowPrivate;
> +    console.log(isPrivate);
> +  }
> +  else {
> +    isPrivate = PrivateBrowsingUtils.isContentWindowPrivate(persistArgs.initiatingWindow);
> +  }

---------------------------------------------------

@@ +825,5 @@
> +
> +    let documentURIObject = {};
> +    documentURIObject.spec = this.document.documentURIObject.spec;
> +    documentURIObject.originCharset = this.document.documentURIObject.originCharset;
> +    docInfo.documentURIObject = documentURIObject;

Can we transfer URI objects directly? If not, could we reconstruct it in the parent process so that it behaves like a normal URI object?

::: browser/base/content/pageinfo/permissions.js
@@ +29,5 @@
>  };
>  
>  function onLoadPermission()
>  {
> +  var uri = BrowserUtils.makeURI(gDocInfo.documentURIObject.spec, gDocInfo.documentURIObject.originCharset, null);

don't you just want:
  var uri = gDocInfo.documentURIObject;
?


@@ +133,5 @@
>      var eTLDService = Components.classes["@mozilla.org/network/effective-tld-service;1"].
>                        getService(Components.interfaces.nsIEffectiveTLDService);
>  
>      var eTLD;
> +    var uri = BrowserUtils.makeURI(gDocInfo.documentURIObject.spec, gDocInfo.documentURIObject.originCharset, null);

same comment here (and at the next few occurrences), isn't gDocInfo.documentURIObject enough?

We can't transfer URI objects directly. We can reconstruct a URI object in the parent process by calling
var uri = BrowserUtils.makeURI(documentURIObject.spec, documentURIObject.originCharset, null);
and this operation doesn't use CPOWs. Which is why I have to do it this way instead of `var uri = gDocInfo.documentURIObject;`

---------------------------------------------------

> +++ b/browser/base/content/pageinfo/pageInfo.js
> +    makeGeneralTab(pageInfoData.metaViewRows, docInfo);
> +    makeMediaTab(pageInfoData.imageViewRows);
> +    initFeedTab(pageInfoData.feeds);
> +    onLoadPermission(docInfo.documentURIObject);
> +    securityOnLoad(docInfo.documentURIObject, windowInfo);

Removed global usage of pageInfoData, docInfo. They're passed as parameters. Removed their g from variable name since not global.

@@ +584,3 @@
>    }
> +  selectImage();
> -  onFinished.forEach(function(func) { func(); });

onFinished is now called in function loadPageInfo(frameOuterWindowID).

Assuming you take into account my suggestion of changing pageInfoData and gDocInfo from global variables into parameters, these functions probably want to receive these parameters too.
If you do this, please remember to update the documentation above 'var onFinished = [ ];', thanks!

None of the tests/addons functions being pushed to onFinished need pageInfoData or other variables I'm removing from global. So I'm not updating the documentation about 'var onFinished = [ ];'
---------------------------------------------------
> +++ b/browser/base/content/pageinfo/pageInfo.js
>    addRows: function(rows)
>    {
> +    this.data = this.data.concat(rows);
> +    this.rowCountChanged(this.rows, rows.length);
> +    this.rows = this.data.length;
> +    if (this.selection.count == 0 && this.rowCount && !gImageElement) {
> +      this.selection.select(0);
> +    }
>    },

I fixed addRows to concat the rows being added and not call addRow and only call rowCountChanged(...) once.

Is addRow still used after this patch?
Yes, by addImage(url, type, alt, elem, isBg). The reason this is still in pageInfo.js is because gImageElement is a CPOW and comes by NSContextMenu. As well as this function needs the gImageView on pageInfo.js
---------------------------------------------------
> +  /**
> +   * This function's previous purpose in pageInfo.js was to get loop through first 500 elements.
> +   * Then do setTimeout so the UI doesn't hang. The iterator filter will continue to look for media elements.
> +   * We sendAsyncMessage after it processes all the elements. So setTimeout doesn't do anything.
> +   * #TODO: refactor pageInfo.js to receive a media element at a time from messages and continually update UI
> +   */

Changed comment for doGrab to be this. Let me know if this is a good enough comment.
---------------------------------------------------
@@ +369,3 @@
>  
> +  //setup the page info dialog once you get json info for content (tab page) from content script
> +  mm.addMessageListener("PageInfo:data", function onmessage(message) {

Do we need to check here that |message| is actually the answer to our PageInfo:getData message? I wonder what will happen if a user opens Page Info several times quickly. Won't we show the information related to the last page in all the dialogs?

The message listeners are connect to your currently selected tab since `selectedBrowser`
`let mm = window.opener.gBrowser.selectedBrowser.messageManager;`
This means the content script for each tab can only send to their appropriate tab. No one else will listen and get stale data of the old tab.

One thing I noticed you can do is. If you open up a page. And use the shortcut cmd+i on mac, ctrl+i on windows to open up the page info really quickly. You can get multiple page info dialogs for the same page. This probably has something to do with pageInfo dialog not being loaded yet so the event handler lets you open another one instead of resetting the current one.
Attachment #8621100 - Attachment is obsolete: true
Attachment #8623177 - Flags: review?(florian)
Comment on attachment 8623177 [details] [diff] [review]
e10s-on-pageInfo

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

Thanks for the updated patch.

When qfold'ing, don't forget to update the commit message, or you get the 2 messages, like in this patch.

(In reply to Jimmy Wang (:jimicy) - I'm an intern from comment #21)

> You're right. Would this be a good solution to remove use of gDocument?

Is gDocument still used anywhere?

> I have to modify `contentAreaUtil.js` to account for a property on a json
> for the doc instead of actually using the document itself to check
> `PrivateBrowsingUtils.isContentWindowPrivate(...)`

I don't understand why this needs to be modified in contentAreaUtil.js rather than handled in Page Info itself.

> @@ +584,3 @@
> >    }
> > +  selectImage();
> > -  onFinished.forEach(function(func) { func(); });
> 
> onFinished is now called in function loadPageInfo(frameOuterWindowID).
> 
> Assuming you take into account my suggestion of changing pageInfoData and
> gDocInfo from global variables into parameters, these functions probably
> want to receive these parameters too.
> If you do this, please remember to update the documentation above 'var
> onFinished = [ ];', thanks!
> 
> None of the tests/addons functions being pushed to onFinished need
> pageInfoData or other variables I'm removing from global.

Is none of the data stored there potentially useful for add-ons?

> Yes, by addImage(url, type, alt, elem, isBg). The reason this is still in
> pageInfo.js is because gImageElement is a CPOW and comes by NSContextMenu.
> As well as this function needs the gImageView on pageInfo.js

Is there anything we can do to no longer need this CPOW?

::: browser/base/content/browser.js
@@ +2389,5 @@
>  // initialTab - name of the initial tab to display, or null for the first tab
>  // imageElement - image to load in the Media Tab of the Page Info window; can be null/omitted
> +// frameOuterWindowID - the id of the frame that the context menu opened in; can be null/omitted
> +function BrowserPageInfo(doc, initialTab, imageElement, frameOuterWindowID) {
> +  var args = {doc: doc, initialTab: initialTab, imageElement: imageElement, 

nit: trailing whitespace

::: browser/base/content/content.js
@@ +835,5 @@
> +
> +  getFeedsInfo: function() {
> +    let feeds = [];
> +    // Get the feeds from the page.
> +    var linkNodes = this.document.getElementsByTagName("link");

Looks like all the 'var' in this function can be replaced with 'let'.

@@ +863,5 @@
> +    return feeds;
> +  },
> +
> +  //******** Generic Build-a-tab
> +  // Assumes the views are empty. Only called once to build the tabs, and

"Generic Build-a-tab" and "Assumes the views are empty" seem obsolete comments for something in a content script.

@@ +896,5 @@
> +    }
> +    else {
> +      // Send the message after all the media elements have been walked through.
> +      let pageInfoData = {metaViewRows: this.getMetaInfo(), imageViewRows: this.imageViewRows,
> +                          docInfo: this.getDocumentInfo(), feeds: this.getFeedsInfo(), 

trailing whitespace

@@ +906,5 @@
> +  /**
> +   * This function's previous purpose in pageInfo.js was to get loop through first 500 elements.
> +   * Then do setTimeout so the UI doesn't hang. The iterator filter will continue to look for media elements.
> +   * We sendAsyncMessage after it processes all the elements. So setTimeout doesn't do anything.
> +   * #TODO: refactor pageInfo.js to receive a media element at a time from messages and continually update UI

Could you please file a bug for this, and include the bug number in the TODO comment?

@@ +1004,5 @@
> +   * Set up a JSON element object with all the instanceOf and other infomation that
> +   * makePreview in pageInfo.js uses to figure out how to display the preview.
> +   */
> +
> +  serializeElementInfo: function (url, type, alt, item, isBG)

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

::: browser/base/content/nsContextMenu.js
@@ +1050,5 @@
>                                         referrerURI: gContextMenuContentData.documentURIObject });
>    },
>  
>    viewFrameInfo: function() {
> +    BrowserPageInfo(this.target.ownerDocument, null, null, this.frameOuterWindowID);

nit: wrap this long line before this.frameOuterWindowID

::: browser/base/content/pageinfo/pageInfo.js
@@ +357,2 @@
>  
> +  const feedTypes = {

Shouldn't this go into the gStrings object?

@@ +365,3 @@
>  
> +  // Look for pageInfoListener in content.js. Sends message to listener with arguments.
> +  mm.sendAsyncMessage("PageInfo:getData", {strings: gStrings, 

trailing whitespace

@@ +377,3 @@
>  
> +    var titleFormat = windowInfo.isTopWindow ? "pageInfo.frame.title"
> +                                                     : "pageInfo.page.title";

nit: align ':' with '?'

@@ +387,5 @@
> +    onLoadPermission(docInfo.documentURIObject);
> +    securityOnLoad(docInfo.documentURIObject, windowInfo);
> +
> +    // Loop through onFinished and execute the functions on it.
> +    onFinished.forEach(function(func) { func(); });

Did you not like my suggestion of giving pageInfoData as a parameter to the functions here?

@@ +463,1 @@
>    gImageElement = args && args.imageElement;

It seems all we are ever doing with the gImageElement CPOW is looking at its currentSrc attribute, in a loop. Shouldn't we instead store only that url in our global variable?

By the way, before the patch we were comparing the element rather than just the url. I think with the patch applied, if the web page shows the same image twice but at different sizes, clicking "View Image Info" won't always show the size of the element the user has clicked on. Is there a reasonably easy way to fix this?

@@ +539,5 @@
>        metaTagsCaption.label = gBundle.getFormattedString("generalMetaTags", [length]);
>      var metaTree = document.getElementById("metatree");
>      metaTree.view = gMetaView;
>  
> +    // Add the metaViewRows onto the general tab's meta info content panel.

'content panel' is misleading here. Did you mean 'tree'?

@@ +880,5 @@
>      var width = 0, height = 0;
>  
> +    if ((item.HTMLLinkElement || item.HTMLInputElement || 
> +         item.HTMLImageElement || item.SVGImageElement || 
> +        (item.HTMLObjectElement && mimeType && mimeType.startsWith("image/")) 

missing one space on the indent of this line.

trailing whitespace on these 3 lines

@@ +881,5 @@
>  
> +    if ((item.HTMLLinkElement || item.HTMLInputElement || 
> +         item.HTMLImageElement || item.SVGImageElement || 
> +        (item.HTMLObjectElement && mimeType && mimeType.startsWith("image/")) 
> +         || isBG) && isProtocolAllowed) {

operators go at the end of the previous line when breaking long lines

@@ +1104,5 @@
>      return;
>  
>    var tree = document.getElementById("imagetree");
>    for (var i = 0; i < tree.view.rowCount; i++) {
> +    // If the image row element is the image selected from the right click "view image info".

the "View Image Info" context menu item

also, break this long line in 2 :)

::: browser/base/content/pageinfo/permissions.js
@@ +27,5 @@
>      }
>    }
>  };
>  
> +function onLoadPermission(documentURIObject)

the parameter here should just be "uri" (do the makeURI call in the caller). Also, makeURI is defined in the global scope in pageInfo.js so you don't need "BrowserUtils."

::: browser/base/content/pageinfo/security.js
@@ +5,5 @@
>  
>  Components.utils.import("resource://gre/modules/BrowserUtils.jsm");
>  
>  var security = {
> +  init: function(documentURIObject, windowInfo) {

this should just be "uri"

@@ +30,2 @@
>        return null;
> +    }

Please don't add these {} here. New code tends to have them, but here let's preserve consistency with the surrounding code.

@@ +32,2 @@
>  
> +    var hostName = this.windowInfo.hostName

missing ';'.

@@ +177,5 @@
>    _cert : null
>  };
>  
> +function securityOnLoad(documentURIObject, windowInfo) {
> +  security.init(documentURIObject, windowInfo);

uri

::: toolkit/content/contentAreaUtils.js
@@ +415,5 @@
>    var targetFileURL = makeFileURI(persistArgs.targetFile);
>  
> +  let isPrivate;
> +  if (typeof persistArgs.initiatingWindow.isContentWindowPrivate !== 'undefined') {
> +    isPrivate = persistArgs.initiatingWindow.isContentWindowPrivate;

As mentioned above, it's not clear to me why we need changes in this file.
Attachment #8623177 - Flags: review?(florian) → feedback+
Posted patch bug1040947-e10s-on-pageInfo (obsolete) — Splinter Review
Attachment #8623177 - Attachment is obsolete: true
(In reply to Florian Quèze [:florian] [:flo] from comment #22)
> Comment on attachment 8623177 [details] [diff] [review]
> e10s-on-pageInfo

Is gDocument still used anywhere?
only for the saveURL() and internalSave() calling contentAreaUtil.js function.

> I have to modify `contentAreaUtil.js` to account for a property on a json
> for the doc instead of actually using the document itself to check
> `PrivateBrowsingUtils.isContentWindowPrivate(...)`

I don't understand why this needs to be modified in contentAreaUtil.js rather than handled in Page Info itself.

::: toolkit/content/contentAreaUtils.js
@@ +415,5 @@
>    var targetFileURL = makeFileURI(persistArgs.targetFile);
>  
> +  let isPrivate;
> +  if (typeof persistArgs.initiatingWindow.isContentWindowPrivate !== 'undefined') {
> +    isPrivate = persistArgs.initiatingWindow.isContentWindowPrivate;

As mentioned above, it's not clear to me why we need changes in this file.

So my reasoning for modifying it in contentAreaUtil is that internalSave() is in contentAreaUtil. And it passes gDocument (before a CPOW) to continueSave() which passes it to internalPersist(persistArgs) who then checks

PrivateBrowsingUtils.isContentWindowPrivate(persistArgs.initiatingWindow);

I don't see how I could handle this in PageInfo when all the contentAreaUtil functions expect to pass a gDocument down and then finally checks it in internalPersist(persistArgs). That is why I modified it to check for a property from my json object document if one is passed.

content document is only on the content script. But you can't saveURL() in a content script since it touches the file system. So you would have to modify the saveURL() function in the parent process to accept a json gDoc with the info you need.

---------------------------------------------------

@@ +387,5 @@
> +    onLoadPermission(docInfo.documentURIObject);
> +    securityOnLoad(docInfo.documentURIObject, windowInfo);
> +
> +    // Loop through onFinished and execute the functions on it.
> +    onFinished.forEach(function(func) { func(pageInfoData); });

Did you not like my suggestion of giving pageInfoData as a parameter to the functions here?
The reason I didn't pass them in was because none of the functions used pageInfoData. But you're right it might be useful for future functions being pushed.

---------------------------------------------------
@@ +896,5 @@
>      var width = 0, height = 0;
>  
> +    if ((item.HTMLLinkElement || item.HTMLInputElement || item.HTMLImageElement || item.SVGImageElement ||
> +         (item.HTMLObjectElement && mimeType && mimeType.startsWith("image/")) || isBG) &&
> +         isProtocolAllowed) {

Wrapping before isProtocolAllowed is misleading here, because the && at the end is barely noticeable in a block where all lines end with ||.

@@ +881,5 @@
>  
> +    if ((item.HTMLLinkElement || item.HTMLInputElement || 
> +         item.HTMLImageElement || item.SVGImageElement || 
> +        (item.HTMLObjectElement && mimeType && mimeType.startsWith("image/")) 
> +         || isBG) && isProtocolAllowed) {

missing one space on the indent of this line.
  [I indented the line with the parathensis ((item.HTMLObjectElement...]

trailing whitespace on these 3 lines
operators go at the end of the previous line when breaking long lines

So taking those 4 feedbacks in. Does this look ok?

> +    if ((item.HTMLLinkElement || item.HTMLInputElement ||
> +         item.HTMLImageElement || item.SVGImageElement ||
> +         (item.HTMLObjectElement && mimeType && mimeType.startsWith("image/")) ||
> +         isBG) && isProtocolAllowed) {

---------------------------------------------------

> Yes, by addImage(url, type, alt, elem, isBg). The reason this is still in
> pageInfo.js is because gImageElement is a CPOW and comes by NSContextMenu.
> As well as this function needs the gImageView on pageInfo.js

Is there anything we can do to no longer need this CPOW?
We would have to change how NSContextMenu uses CPOWs. Since almost all it's operations are using unsafe CPOWs.
Since context menu is in the parent process, but it needs to know about the content process in order
to change context menu options. I decided if I were to change this, it would make more sense having another bug to fix CPOW usage in NSContextMenu since it's not an easy fix.

---------------------------------------------------

@@ +463,1 @@
>    gImageElement = args && args.imageElement;

It seems all we are ever doing with the gImageElement CPOW is looking at its currentSrc attribute, in a loop. Shouldn't we instead store only that url in our global variable?

By the way, before the patch we were comparing the element rather than just the url. I think with the patch applied, if the web page shows the same image twice but at different sizes, clicking "View Image Info" won't always show the size of the element the user has clicked on. Is there a reasonably easy way to fix this?

You're right about this. My new patch checks for width and height. I also assign gImageElement a json so we don't need to use unsafe CPOW operations everytime to compare currentSRC or other attributes.

>  function loadTab(args)
>  {
> -  if (args && args.doc) {
> -    gDocument = args.doc;
> -    gWindow = gDocument.defaultView;
> +  let imageElement = args.imageElement;
> +  if (args && imageElement) {
> +    gImageElement = {currentSrc: imageElement.currentSrc,
> +                     width: imageElement.width, height: imageElement.height};
>    }

> @@ -632,92 +608,27 @@ function addImage(url, type, alt, elem,
> +    if (gImageElement && url == gImageElement.currentSrc &&
> +        gImageElement.width == elem.width &&
> +        gImageElement.height == elem.height) {
>        gImageView.data[i][COL_IMAGE_NODE] = elem;
Attachment #8624025 - Flags: review?(florian)
Comment on attachment 8624025 [details] [diff] [review]
bug1040947-e10s-on-pageInfo

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

Getting really close to r+ :-)

::: browser/base/content/pageinfo/pageInfo.js
@@ +301,1 @@
>  var onLoadRegistry = [ ];

The comment before onLoadRegistry says "The global variables gDocument and gWindow are set." Looks like you need to update it.

@@ +371,5 @@
> +    let pageInfoData = message.data;
> +    let docInfo = pageInfoData.docInfo;
> +    let windowInfo = pageInfoData.windowInfo;
> +    let uri = makeURI(docInfo.documentURIObject.spec,
> +                      docInfo.documentURIObject.originCharset, null);

I don't think the trailing null parameter is useful.

@@ +610,5 @@
>                  .addObserver(imagePermissionObserver, "perm-changed", false);
>      }
>    }
>    else {
> +    // When you have the same image on the page, but in different sizes.

I don't think you intended to put this comment at the top of this block, as it doesn't seem related to what the code does. An image being displayed twice at the same size will still cause the count column to be incremented.

@@ +1113,5 @@
>  
>    var tree = document.getElementById("imagetree");
>    for (var i = 0; i < tree.view.rowCount; i++) {
> +    // If the image row element is the image selected from the
> +    // right click "view image info".

As said in my previous comment, 'the right click "view image info"' isn't clear. I suggested another wording.

@@ +1117,5 @@
> +    // right click "view image info".
> +    let image = gImageView.data[i][COL_IMAGE_NODE];
> +    if (gImageElement.currentSrc == gImageView.data[i][COL_IMAGE_ADDRESS] &&
> +        gImageElement.width == image.width &&
> +        gImageElement.height == image.height &&

I wonder if we should also check the image text (the value coming from: "imageText = item.title || item.alt" in the content script).

::: toolkit/content/contentAreaUtils.js
@@ +413,5 @@
>  
>    // Find the URI associated with the target file
>    var targetFileURL = makeFileURI(persistArgs.targetFile);
>  
> +  let isPrivate;

This seems too magical. What about we add an isPrivate parameter to saveURL, and saveInternal, and look at the initiatingWindow value only if isPrivate is undefined?
Attachment #8624025 - Flags: review?(florian) → feedback+
Attachment #8624025 - Attachment is obsolete: true
As said in my previous comment, 'the right click "view image info"' isn't clear. I suggested another wording.

How about this reworded comment? Is this more clear?

>    var tree = document.getElementById("imagetree");
>    for (var i = 0; i < tree.view.rowCount; i++) {
> -    if (gImageElement == gImageView.data[i][COL_IMAGE_NODE] &&
> -        !gImageView.data[i][COL_IMAGE_BG]) {
> +    // Check that the image is the same image you "view image info" on from the context menu.

I wonder if we should also check the image text (the value coming from: "imageText = item.title || item.alt" in the content script).

Yes you're right. I added a check for that. When you "view image info" on a repeated image with a meta tag, it will check that it is same image.

> +    // Check that the image is the same image you "view image info" on from the context menu.
> +    let image = gImageView.data[i][COL_IMAGE_NODE];
> +    if (!gImageView.data[i][COL_IMAGE_BG] &&
> +        gImageElement.currentSrc == gImageView.data[i][COL_IMAGE_ADDRESS] &&
> +        gImageElement.width == image.width &&
> +        gImageElement.height == image.height &&
> +        gImageElement.imageText == image.imageText) {

This seems too magical. What about we add an isPrivate parameter to saveURL, and saveInternal, and look at the initiatingWindow value only if isPrivate is undefined?

I pass a isContentWindowPrivate variable through now all the way down to internalPersist(persistArgs). Which then checks if it has been defined, if not then it will use the window passed like normal.

> @@ -809,24 +722,25 @@ function saveMedia()
> +      saveURL(url, null, titleKey, false, false, makeURI(item.baseURI),
> +              gDocument, gDocument.isContentWindowPrivate);

> @@ -809,24 +722,25 @@ function saveMedia()
>          var saveAnImage = function(aURIString, aChosenData, aBaseURI) {
>            internalSave(aURIString, null, null, null, null, false, "SaveImageTitle",
> +                       aChosenData, aBaseURI, gDocument, gDocument.isContentWindowPrivate);

> +++ b/toolkit/content/contentAreaUtils.js
>  function saveURL(aURL, aFileName, aFilePickerTitleKey, aShouldBypassCache,
> -                 aSkipPrompt, aReferrer, aSourceDocument)
> +                 aSkipPrompt, aReferrer, aSourceDocument, isContentWindowPrivate)
>  {

> +++ b/toolkit/content/contentAreaUtils.js
> @@ -409,17 +410,23 @@ function internalPersist(persistArgs)
> +  let isPrivate;
> +  if (typeof persistArgs.isContentWindowPrivate !=='undefined') {
> +    isPrivate = persistArgs.isContentWindowPrivate;
> +  }
> +  else {
> +    isPrivate = PrivateBrowsingUtils.isContentWindowPrivate(persistArgs.initiatingWindow);
> +  }
Posted patch bug1040947-e10s-on-pageInfo (obsolete) — Splinter Review
Attachment #8626295 - Attachment is obsolete: true
Attachment #8626317 - Flags: review?(florian)
separate patch to not clutter previous patch diff with removing trailing spaces
Comment on attachment 8626317 [details] [diff] [review]
bug1040947-e10s-on-pageInfo

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

I haven't looked at the whole patch this time, I only looked at the changes that are related to the comments I gave in comment 25. I hope I didn't miss anything (interdiff didn't work between attachment 8624025 [details] [diff] [review] and this new patch).

::: browser/base/content/pageinfo/pageInfo.js
@@ +371,5 @@
> +    let docInfo = pageInfoData.docInfo;
> +    let windowInfo = pageInfoData.windowInfo;
> +    let uri = makeURI(docInfo.documentURIObject.spec,
> +                      docInfo.documentURIObject.originCharset);
> +    gDocument = docInfo;

I would suggest renaming this global variable (eg. to gDocInfo), as its content is no longer a DOM document, and any add-on previously relying on this variable is now broken, so it's better if their code fails with a clear error message saying the variable they use no longer exists.

@@ +1117,5 @@
>      return;
>  
>    var tree = document.getElementById("imagetree");
>    for (var i = 0; i < tree.view.rowCount; i++) {
> +    // Check that the image is the same image you "view image info" on from the context menu.

'the same image you "view image info" on' still isn't easy to read for me. Was there something you disliked in the wording I suggested in comment 22:
If the image row element is the image selected from the "View Image Info" context menu item.

::: toolkit/content/contentAreaUtils.js
@@ +271,4 @@
>   *        default downloads folder without prompting.
>   * @param aCacheKey [optional]
>   *        If set will be passed to saveURI.  See nsIWebBrowserPersist for
>   *        allowed values.

You need to document the new parameter here.

@@ +275,5 @@
>   */
>  function internalSave(aURL, aDocument, aDefaultFileName, aContentDisposition,
>                        aContentType, aShouldBypassCache, aFilePickerTitleKey,
>                        aChosenData, aReferrer, aInitiatingDocument, aSkipPrompt,
> +                      aCacheKey, isContentWindowPrivate)

The other parameters here all start with an a ('a' means 'argument' here), so the new one should be consistent. Same comment applies for the saveURL function above.

@@ +418,5 @@
> +  let isPrivate;
> +  if (typeof persistArgs.isContentWindowPrivate !=='undefined') {
> +    isPrivate = persistArgs.isContentWindowPrivate;
> +  }
> +  else {

Can this be simplified to:
let isPrivate = persistArgs.isContentWindowPrivate;
if (isPrivate === undefined)
  isPrivate = PrivateBrowsingUtils...
Attachment #8626317 - Flags: review?(florian) → feedback+
Posted patch bug1040947-e10s-on-pageInfo (obsolete) — Splinter Review
'the same image you "view image info" on' still isn't easy to read for me. Was there something you disliked in the wording I suggested in comment 22:
If the image row element is the image selected from the "View Image Info" context menu item.

Sorry, I missed that comment suggestion. I have changed all instances about "view image info" to match

--- a/browser/base/content/pageinfo/pageInfo.js
+++ b/browser/base/content/pageinfo/pageInfo.js

> +  // Image Element is the "View Image Info" context menu item.
> +  // Image Element will not be a background image because you can't "view image info" on one.
> +  let imageElement = args.imageElement;
> +  if (args && imageElement) {
> +    gImageElement = {currentSrc: imageElement.currentSrc,
> +                     width: imageElement.width, height: imageElement.height,
> +                     imageText: imageElement.title || imageElement.alt};

> +    // When you have the same image on the page, but in different sizes.
> +    // Set the image to be the same as the "View Image Info" context menu item.
> +    if (!gImageView.data[i][COL_IMAGE_BG] &&
> +        gImageElement && url === gImageElement.currentSrc &&
> +        gImageElement.width === elem.width &&
> +        gImageElement.height === elem.height &&
> +        gImageElement.imageText === elem.imageText) {
>        gImageView.data[i][COL_IMAGE_NODE] = elem;
> +    }

>    for (var i = 0; i < tree.view.rowCount; i++) {
> +    // If the image row element is the image selected from the "View Image Info" context menu item.
> +    let image = gImageView.data[i][COL_IMAGE_NODE];
> +    if (!gImageView.data[i][COL_IMAGE_BG] &&
> +        gImageElement.currentSrc === gImageView.data[i][COL_IMAGE_ADDRESS] &&
> +        gImageElement.width === image.width &&
> +        gImageElement.height === image.height &&
> +        gImageElement.imageText === image.imageText) {
>        tree.view.selection.select(i);
>        tree.treeBoxObject.ensureRowIsVisible(i);
>        tree.focus();
>        return;
>      }
Attachment #8626317 - Attachment is obsolete: true
Posted patch bug1040947-e10s-on-pageInfo (obsolete) — Splinter Review
Attachment #8628878 - Attachment is obsolete: true
Attachment #8626318 - Attachment is obsolete: true
Attachment #8628886 - Flags: review?(florian)
Comment on attachment 8628886 [details] [diff] [review]
bug1040947-e10s-on-pageInfo

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

::: browser/base/content/pageinfo/pageInfo.js
@@ +458,5 @@
>  }
>  
>  function loadTab(args)
>  {
> +  // Image Element is the "View Image Info" context menu item.

No, the image element isn't the XUL <menuitem>

@@ +459,5 @@
>  
>  function loadTab(args)
>  {
> +  // Image Element is the "View Image Info" context menu item.
> +  // Image Element will not be a background image because you can't "view image info" on one.

Suggestion:
// If the "View Image Info" context menu item was used, the related image
// element is provided as an argument. This can't be a background image.

@@ +460,5 @@
>  function loadTab(args)
>  {
> +  // Image Element is the "View Image Info" context menu item.
> +  // Image Element will not be a background image because you can't "view image info" on one.
> +  let imageElement = args.imageElement;

Change this to args && args.imageElement;

@@ +461,5 @@
>  {
> +  // Image Element is the "View Image Info" context menu item.
> +  // Image Element will not be a background image because you can't "view image info" on one.
> +  let imageElement = args.imageElement;
> +  if (args && imageElement) {

If args is null, you've already caused an exception at the previous line.

@@ +615,5 @@
>    else {
>      var i = gImageHash[url][type][alt];
>      gImageView.data[i][COL_IMAGE_COUNT]++;
> +    // When you have the same image on the page, but in different sizes.
> +    // Set the image to be the same as the "View Image Info" context menu item.

// The same image can occur several times on the page at different sizes.
// If the "View Image Info" context menu item was used, ensure we select
// the correct element.

@@ +617,5 @@
>      gImageView.data[i][COL_IMAGE_COUNT]++;
> +    // When you have the same image on the page, but in different sizes.
> +    // Set the image to be the same as the "View Image Info" context menu item.
> +    if (!gImageView.data[i][COL_IMAGE_BG] &&
> +        gImageElement && url === gImageElement.currentSrc &&

Why the ===?

@@ +1121,5 @@
>    for (var i = 0; i < tree.view.rowCount; i++) {
> +    // If the image row element is the image selected from the "View Image Info" context menu item.
> +    let image = gImageView.data[i][COL_IMAGE_NODE];
> +    if (!gImageView.data[i][COL_IMAGE_BG] &&
> +        gImageElement.currentSrc === gImageView.data[i][COL_IMAGE_ADDRESS] &&

Why do you need === rather than ==?

::: toolkit/content/contentAreaUtils.js
@@ +267,2 @@
>   * @param aInitiatingDocument
>   *        The document from which the save was initiated.

So, I think you are changing this function to support receiving here values that aren't real document objects. I think you should detail what the required properties for aInitiatingDocument are in that case.

@@ +272,5 @@
>   * @param aCacheKey [optional]
>   *        If set will be passed to saveURI.  See nsIWebBrowserPersist for
>   *        allowed values.
> + * @param aIsContentWindowPrivate [optional]
> + *        Whether the content window is private or not.

This sentence is just a plain English version of the parameter name, it doesn't help a developer reading the comment to understand when that parameter should be provided and why.

I don't fully remember (I'm hoping you remember better than I do), but what you probably want to say is that when this parameter is provided, aInitiatingDocument doesn't have to be a real DOM document object.

@@ +273,5 @@
>   *        If set will be passed to saveURI.  See nsIWebBrowserPersist for
>   *        allowed values.
> + * @param aIsContentWindowPrivate [optional]
> + *        Whether the content window is private or not.
> + *        Stored on persistArgs and passed to internalPersist(persistArgs).

This doesn't seem relevant (it's the kind of information you typically get by reading the code, not by reading the documentation).

@@ +399,5 @@
>   * @param persistArgs.initiatingWindow
>   *        The window from which the save operation was initiated.
> + * @param persistArgs.isContentWindowPrivate
> + *        Whether the content window is private or not.
> + *        If present then isPrivate is set to this value.

If present then isPrivate is set to this value without touching persistArgs.initiatingWindow
Attachment #8628886 - Flags: review?(florian) → feedback+
Posted patch bug1040947-e10s-on-pageInfo (obsolete) — Splinter Review
Replaced the "view image info" type of comments with your suggestions. Thanks!

@@ -634,90 +610,29 @@ function addImage(url, type, alt, elem,
>      gImageView.data[i][COL_IMAGE_COUNT]++;
> +    // The same image can occur several times on the page at different sizes.
> +    // If the "View Image Info" context menu item was used, ensure we select
> +    // the correct element.
> +    if (!gImageView.data[i][COL_IMAGE_BG] &&
> +        gImageElement && url == gImageElement.currentSrc &&

Why the ===?
You're right I don't really need it in this case.
But I figured it would just be better pratice to use ===.
I have changed it back to ==

>    for (var i = 0; i < tree.view.rowCount; i++) {
> +    // If the image row element is the image selected from the "View Image Info" context menu item.
> +    let image = gImageView.data[i][COL_IMAGE_NODE];
> +    if (!gImageView.data[i][COL_IMAGE_BG] &&
> +        gImageElement.currentSrc == gImageView.data[i][COL_IMAGE_ADDRESS] &&

Why do you need === rather than ==?
Same reasioning here. I have changed it back to ==

::: toolkit/content/contentAreaUtils.js

>   * @param aInitiatingDocument
>   *        The document from which the save was initiated.

So, I think you are changing this function to support receiving here values that aren't real document objects. I think you should detail what the required properties for aInitiatingDocument are in that case.

aInitiatingDocument doesn't need any properties. Since we pass aIsContentWindowPrivate which was the only thing aInitiatingDocument was being used for check for. I have left this comment as it is.

> + * @param aIsContentWindowPrivate [optional]
> + *        This parameter is provided when the aInitiatingDocument is not a
> + *        real document object. Stores whether aInitiatingDocument.defaultView
> + *        was private or not.
>   */

Is this comment better?
Attachment #8628886 - Attachment is obsolete: true
Attachment #8630481 - Flags: review?(florian)
(In reply to Jimmy Wang (:jimicy) - I'm an intern from comment #35)
> Created attachment 8630481 [details] [diff] [review]
> bug1040947-e10s-on-pageInfo

In the future, please put version numbers in your attachment names. That makes using Bugzilla's interdiff feature way easier.

> > +    if (!gImageView.data[i][COL_IMAGE_BG] &&
> > +        gImageElement && url == gImageElement.currentSrc &&
> 
> Why the ===?
> You're right I don't really need it in this case.
> But I figured it would just be better pratice to use ===.
> I have changed it back to ==

Also, please use the "reply" link when replying to a bugzilla comment, so that what's quoted from the comment you are replying to is immediately distinguishable from the new text you wrote.


Is there a reason why these === shouldn't also be == for the width, height and imageText values too?
Comment on attachment 8630481 [details] [diff] [review]
bug1040947-e10s-on-pageInfo

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

::: browser/base/content/pageinfo/pageInfo.js
@@ +729,5 @@
>        else if (item instanceof HTMLAudioElement)
>          titleKey = "SaveAudioTitle";
>  
> +      saveURL(url, null, titleKey, false, false, makeURI(item.baseURI),
> +              gDocInfo, gDocInfo.isContentWindowPrivate);

I don't think you actually want to give gDocInfo to the saveURL method. An empty object would give the same result. So I'm wondering if we shouldn't just make that parameter optional.

@@ +736,5 @@
>      selectSaveFolder(function(aDirectory) {
>        if (aDirectory) {
>          var saveAnImage = function(aURIString, aChosenData, aBaseURI) {
>            internalSave(aURIString, null, null, null, null, false, "SaveImageTitle",
> +                       aChosenData, aBaseURI, gDocInfo, gDocInfo.isContentWindowPrivate);

Same here.

::: toolkit/content/contentAreaUtils.js
@@ +360,5 @@
>        targetFile        : file,
>        sourceCacheKey    : aCacheKey,
>        sourcePostData    : nonCPOWDocument ? getPostData(aDocument) : null,
>        bypassCache       : aShouldBypassCache,
> +      initiatingWindow  : aInitiatingDocument.defaultView,

If we make aInitiatingDocument optional, then this line needs to become:
 aInitiatingDocument ? aInitiatingDocument.defaultView : null
or just:
 aInitiatingDocument && aInitiatingDocument.defaultView

@@ +396,5 @@
>   *        the same content type as the source document. Currently only
>   *        "text/plain" is meaningful.
>   * @param persistArgs.bypassCache
>   *        If true, the document will always be refetched from the server
>   * @param persistArgs.initiatingWindow

We should mark this [optional]. And I guess the comment should say that if this is omitted then isContentWindowPrivate has to be provided.

@@ +398,5 @@
>   * @param persistArgs.bypassCache
>   *        If true, the document will always be refetched from the server
>   * @param persistArgs.initiatingWindow
>   *        The window from which the save operation was initiated.
> + * @param persistArgs.isContentWindowPrivate

[optional]
Attachment #8630481 - Attachment is obsolete: true
Attachment #8630481 - Flags: review?(florian)
Attachment #8630501 - Attachment is obsolete: true
Attachment #8630519 - Flags: review?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #37)
Comment on attachment 8630481 [details] [diff] [review]
bug1040947-e10s-on-pageInfo
 
Review of attachment 8630481 [details] [diff] [review]:
-----------------------------------------------------------------

> In the future, please put version numbers in your attachment names. That makes using Bugzilla's interdiff feature way easier. Also, please use the "reply" link when replying to a bugzilla comment, so that what's quoted from the comment you are replying to is immediately distinguishable from the new text you wrote.

Thanks for the suggestions! Will do for the future.

> Is there a reason why these === shouldn't also be == for the width, height and imageText values too?

Just thought === was better pratice like before. New patch has changed it back to ==.

Made gDocInfo an optional parameter. Updated documentation to reflect that

> + * @param aInitiatingDocument [optional]
>   *        The document from which the save was initiated.
> + *        If this is omitted then isContentWindowPrivate has to be provided.

> + * @param aIsContentWindowPrivate [optional]
> + *        This parameter is provided when the aInitiatingDocument is not a
> + *        real document object. Stores whether aInitiatingDocument.defaultView
> + *        was private or not.

> + * @param persistArgs.initiatingWindow [optional]
>   *        The window from which the save operation was initiated.
> + *        If this is omitted then isContentWindowPrivate has to be provided.
> + * @param persistArgs.isContentWindowPrivate [optional]
> + *        If present then isPrivate is set to this value without touching
> + *        persistArgs.initiatingWindow.
Comment on attachment 8630519 [details] [diff] [review]
bug1040947-e10s-on-pageInfo-v3

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

This time I actually tried the patch locally, and noticed that the whole Page Info dialog flickers, because the content isn't filled before we are done walking the DOM to collect media elements. On small pages, the Page Info dialog is blank for a split second. On a larger page it can take much longer (eg on http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js the dialog stays blank for about 5s on my fast macbook).

What we need to do to address this is to send a first message from the content script once we have collected all the general information (the info we need to fill the general, security, feeds, etc... tabs), and then a second message to fill the Media tab once we are done walking the DOM. Sorry for not catching this earlier.

::: browser/base/content/content.js
@@ +760,5 @@
> +  },
> +
> +  getWindowInfo: function() {
> +    let windowInfo = {};
> +    windowInfo.isTopWindow = this.window != this.window.top;

This should be ==, not !=

@@ +856,5 @@
> +      // Send the message after all the media elements have been walked through.
> +      let pageInfoData = {metaViewRows: this.getMetaInfo(), imageViewRows: this.imageViewRows,
> +                          docInfo: this.getDocumentInfo(), feeds: this.getFeedsInfo(),
> +                          windowInfo: this.getWindowInfo()};
> +      sendAsyncMessage("PageInfo:data", pageInfoData);

I think at this point we are done, so we should drop references to all the stuff we have kept in memory:
    this.imageViewRows = null;
    this.frameList = null;
    this.strings = null;
    this.window = null;
    this.document = null;

@@ +863,5 @@
> +
> +  /**
> +   * This function's previous purpose in pageInfo.js was to get loop through first 500 elements.
> +   * Then do setTimeout so the UI doesn't hang. The iterator filter will continue to look for media elements.
> +   * We sendAsyncMessage after it processes all the elements. So setTimeout doesn't do anything.

I don't understand what "So setTimeout doesn't do anything" means here.

::: browser/base/content/pageinfo/pageInfo.js
@@ +469,1 @@
>    }

I wonder if we need an else to set gImageElement back to null when no image element was specified. Is there some other code reseting gImageElement when the Page Info dialog is reloaded?

::: browser/base/content/pageinfo/security.js
@@ +25,5 @@
>      const nsISSLStatus = Components.interfaces.nsISSLStatus;
>  
>      // We don't have separate info for a frame, return null until further notice
>      // (see bug 138479)
> +    if (this.windowInfo.isTopWindow)

This should be: if (!this...)

::: toolkit/content/contentAreaUtils.js
@@ +268,2 @@
>   *        The document from which the save was initiated.
> + *        If this is omitted then isContentWindowPrivate has to be provided.

aIsContentWindowPrivate

@@ +402,4 @@
>   *        The window from which the save operation was initiated.
> + *        If this is omitted then isContentWindowPrivate has to be provided.
> + * @param persistArgs.isContentWindowPrivate [optional]
> + *        If present then isPrivate is set to this value without touching

I know I proposed this wording, but after reading it again, I think I would prefer "using" instead of "touching", as 'touching' may imply that we modify it.
Attachment #8630519 - Flags: review?(florian) → feedback+
Attachment #8630519 - Attachment is obsolete: true
Attachment #8631255 - Flags: review?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #41)
 Comment on attachment 8630519 [details] [diff] [review]
 bug1040947-e10s-on-pageInfo-v3
 
 Review of attachment 8630519 [details] [diff] [review]:
 -----------------------------------------------------------------

In my new patch
attachment 8631255 [details] [diff] [review]

+++ b/browser/base/content/content.js
> +  receiveMessage: function(message) {
> +    this.imageViewRows = [];
> +    this.frameList = [];
> +    this.strings = message.data.strings;
> +
> +    let frameOuterWindowID = message.data.frameOuterWindowID;
> +
> +    // If inside frame then get the frame's window and document.
> +    if (frameOuterWindowID) {
> +      this.window = Services.wm.getOuterWindowWithId(frameOuterWindowID);
> +      this.document = this.window.document;
> +    }
> +    else {
> +      this.document = content.document;
> +      this.window = content.window;
> +    }
> +
> +    let pageInfoData = {metaViewRows: this.getMetaInfo(), docInfo: this.getDocumentInfo(),
> +                        feeds: this.getFeedsInfo(), windowInfo: this.getWindowInfo()};
> +    sendAsyncMessage("PageInfo:data", pageInfoData);
> +
> +    //Separate step so page info dialog isn't blank while waiting for this to finish.
> +    this.getMediaInfo();
> +
> +    // Send the message after all the media elements have been walked through.
> +    let pageInfoMediaData = {imageViewRows: this.imageViewRows};
> +
> +    this.imageViewRows = null;
> +    this.frameList = null;
> +    this.strings = null;
> +    this.window = null;
> +    this.document = null;
> +
> +    sendAsyncMessage("PageInfo:mediaData", pageInfoMediaData);
> +  },

I removed the uses of setTimeOut because it didn't do anything. And removed the associated comments which didn't make sense.
By doing that I was able to have the async message for media elements be sent in receiveMessage(...).
I think this is more clear and explicit to see what is being sent. Instead of having to go and trace your way to processFrames()
to see that, that's where the async message is happening.
Comment on attachment 8631255 [details] [diff] [review]
bug1040947-e10s-on-pageInfo-v4

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

With your patch applied, on this page: https://www.capitainetrain.com/signin the Media tab never appears, and I have this JavaScript error: chrome://browser/content/content.js, line 1012: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [imgIRequest.image]

Could you please investigate?

I'm not very excited by the setTimeout removals, as we are now potentially blocking the content process for a long while on very large pages. I think it's ok for nightly, but not shippable. I'm r+'ing in the interest of moving forward and looking at smaller patches for what remains, but please ensure bug 1175794 gets attention soon.

::: browser/base/content/content.js
@@ +745,5 @@
> +    let pageInfoData = {metaViewRows: this.getMetaInfo(), docInfo: this.getDocumentInfo(),
> +                        feeds: this.getFeedsInfo(), windowInfo: this.getWindowInfo()};
> +    sendAsyncMessage("PageInfo:data", pageInfoData);
> +
> +    //Separate step so page info dialog isn't blank while waiting for this to finish.

nit: space after '//'.

@@ +876,5 @@
> +   * from messages and continually update UI.
> +   */
> +  doGrab: function(iterator)
> +  {
> +    for (let i = 0; i < 500; ++i)

This could as well become:
   while (true)

@@ +884,5 @@
> +        return;
> +      }
> +    }
> +
> +    this.doGrab(iterator);

and then this line can be removed.
Attachment #8631255 - Flags: review?(florian) → review+
Attachment #8631255 - Attachment is obsolete: true
Attachment #8634470 - Flags: review?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #44)
> Comment on attachment 8631255 [details] [diff] [review]
> bug1040947-e10s-on-pageInfo-v4
> 
> Review of attachment 8631255 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> With your patch applied, on this page: https://www.capitainetrain.com/signin
> the Media tab never appears, and I have this JavaScript error:
> chrome://browser/content/content.js, line 1012: NS_ERROR_FAILURE: Component
> returned failure code: 0x80004005 (NS_ERROR_FAILURE) [imgIRequest.image]
> 
> Could you please investigate?
> 
> I'm not very excited by the setTimeout removals, as we are now potentially
> blocking the content process for a long while on very large pages. I think
> it's ok for nightly, but not shippable. I'm r+'ing in the interest of moving
> forward and looking at smaller patches for what remains, but please ensure
> bug 1175794 gets attention soon.

I took a look. So it seems that imgIRequest.image also fails in firefox developer edition with e10s
http://i.imgur.com/qtyDugx.png
I was only able to get this error message. By using the debugger and manually call the function myself.

The only difference is that before moving pageinfo into a content.js is that this failure would not produce an error message. However it was erroring out, so all code after it didn't execute. I wasn't able to log [imgIRequest.image] at all. Since it fails silently. PageInfo media tab could still get created.

I did a try and catch in my new patch so the code continues to run.

In my new patch I also get this error
>JavaScript error: chrome://browser/content/pageinfo/pageInfo.js, line 1042: TypeError: /^Content-Type:\s*(.*?)\s*(?:\;|$)/im.exec(...) is null

However if you look in the image I linked, this error also happens for the same image. It's just that the code before would error out and nothing after it would execute. Excuting it manually in the debugger in firefox developer edition e10s, yields the same error.

Would you happen to know what this line is trying to do?
mimeType = getContentTypeFromHeaders(cacheEntry);

In this situation, what would you advise I do? Since the original code also has this bug.
I can trace through it and figure out the intended behaviour for this line.
So, the reason why we have issues with this specific image is that the server is returning:
 "204 No Content" and the Content-Type header is not included. Before the patch, the error occurred only when the user selected the row in the media tab and we tried to make the preview. I don't know why the error wasn't reported, but as you noticed it stopped code execution, so there was indeed an error.


You can fix the getContentTypeFromHeaders function by adding a nullcheck.
  return (/^Content-Type:\s*(.*?)\s*(?:\;|$)/mi
          .exec(cacheEntryDescriptor.getMetaDataElement("response-head")))[1];
Should become something like:
  let headers = cacheEntryDescriptor.getMetaDataElement("response-head");
  let type = /^Content-Type:\s*(.*?)\s*(?:\;|$)/mi.exec();
  return type && type[1];


For the imgIRequest.image exception, it makes sense that we can't access the image if the server returned nothing.

The imgIRequest interface exposes an imageStatus attribute (http://mxr.mozilla.org/mozilla-central/source/image/imgIRequest.idl?rev=20748e1e2276#60). I wonder if we could fix the bug by doing this:
let image = (imageRequest.imageStatus != Ci.imgIRequest.STATUS_NONE) && imageRequest.image;


Looking at this code again, it's not entirely clear to me why the pageInfo.js code (especially the makePreview function) does computations related to the mimeType (isn't this something that could be done completely by the content script?). I also wonder if getContentTypeFromHeaders is something that should live in the content script.

I'm not sure why we check |if (item.imageRequestInfo.hasImageRequest)|, couldn't we just omit imageRequestInfo completely in the data we transfer when it's null? ie we could check |if (item.imageRequestInfo)|.

|let foo = Boolean(bar);| can be simplified to |let foo = !!bar;|
Duplicate of this bug: 1170291
No longer blocks: 1175794
Posted patch Folded patch (obsolete) — Splinter Review
Attachment #8634470 - Attachment is obsolete: true
Attachment #8634470 - Flags: review?(florian)
Attachment #8635566 - Flags: review?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #47)
 
>> Looking at this code again, it's not entirely clear to me why the
>> pageInfo.js code (especially the makePreview function) does computations
>> related to the mimeType (isn't this something that could be done completely
>> by the content script?). I also wonder if getContentTypeFromHeaders is
>> something that should live in the content script.

I have moved most of the mimeType operations to content.js

in pageinfo.js
> var mimeType = item.mimeType || this.getContentTypeFromHeaders(cacheEntry);

this.getContentTypeFromHeaders(cacheEntry) has to stay in the parent script. Since cacheEntry comes from
openCacheEntry() which uses diskStorage.asyncOpenURI and can't be done from a content script.

>> I'm not sure why we check |if (item.imageRequestInfo.hasImageRequest)|,
>> couldn't we just omit imageRequestInfo completely in the data we transfer
>> when it's null? ie we could check |if (item.imageRequestInfo)|.
>>
>> |let foo = Boolean(bar);| can be simplified to |let foo = !!bar;|

I no longer need imageRequest. numFrames is also now set from the content script.

I think this is good to get merged in. Let me know what you think.
I have also made bug 1175794 [Refactor pageInfo.js with the Tasks idea you suggested] a meta bug for
Bug 866413 - Electrolysis: Make Page Information dialog work
Attachment #8631255 - Attachment is obsolete: false
Implemented the minor fixes that florian suggested for the r+ patch.
Alternate version of patch bug1040947-e10s-on-pageInfo-v6 that instead of doing it as one big patch. Has that's patches changes as a separate patch on top of the old bug1040947-e10s-on-pageInfo-v4 patch.
Comment on attachment 8638686 [details] [diff] [review]
Follow-up fixes

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

This patch is what the new patch bug1040947-e10s-on-pageInfo-v6 changes from bug1040947-e10s-on-pageInfo-v5.
It moves the code for figuring out the mimetype to content.js
Adds a check for imageRequest error so that image is set to false if there isn't an image.
Attachment #8638686 - Flags: review?(mconley)
Reporter

Updated

4 years ago
Attachment #8635566 - Flags: review?(florian)
Reporter

Updated

4 years ago
Attachment #8628891 - Attachment is obsolete: true
Reporter

Updated

4 years ago
Attachment #8638685 - Attachment is obsolete: true
Reporter

Updated

4 years ago
Attachment #8638685 - Attachment description: bug1040947-e10s-on-pageInfo-v4-with-florian-suggestions → Base patch with suggestions (r+'d by florian)
Attachment #8638685 - Attachment is obsolete: false
Attachment #8638685 - Flags: review+
Reporter

Updated

4 years ago
Attachment #8631255 - Attachment is obsolete: true
Reporter

Updated

4 years ago
Attachment #8635566 - Attachment description: bug1040947-e10s-on-pageInfo-v6 → Folded patch
Reporter

Updated

4 years ago
Attachment #8638686 - Attachment description: bug104094-e10s-on-pageInfo-v6-ontop-patch → Follow-up fixes
Comment on attachment 8638686 [details] [diff] [review]
Follow-up fixes

Looks good. Sorry I didn't find time to review attachment 8635566 [details] [diff] [review] ten days ago. With the changes in a separate attachment, it's easier to review quickly :-) (I had tried looking at the interdiff at the time, but bugzilla's interdiff failed on that attachment for some reason).
Attachment #8638686 - Flags: review?(mconley) → review+
Comment on attachment 8638685 [details] [diff] [review]
Base patch with suggestions (r+'d by florian)

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

So it looks like florian already r+'d your interdiff, so you should be good to go.

I do think there's follow-up work here. See below. Can you please file a bug(s) for those?

Also, if there's no bug already for the last paragraph of comment 17 (about using Tasks instead), please file one, and maybe get started on it. I'm a bit worried that this will regress performance in single-process Firefox by not going back to the event loop as often as we used to.

::: browser/base/content/browser.js
@@ +2418,3 @@
>    var windows = Services.wm.getEnumerator("Browser:page-info");
>  
>    var documentURL = doc ? doc.location : window.gBrowser.selectedBrowser.contentDocumentAsCPOW.location;

While you're here, might as well use window.gBrowser.selectedBrowser.currentURI.spec instead of contentDocumentAsCPOW.

::: browser/base/content/content.js
@@ +38,5 @@
>    Cu.import("resource://gre/modules/PageMenu.jsm", tmp);
>    return new tmp.PageMenuChild();
>  });
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "Feeds", "resource:///modules/Feeds.jsm");

Let's put this up with the other lazy module getters, and keep the formatting.

@@ +842,5 @@
>    if (disable)
>      sendAsyncMessage("ContextMenu:SetAsDesktopBackground:Result", { disable });
>  });
> +
> +let pageInfoListener = {

These global things seem to be capitalized like: PageInfoListener. Might as well keep that convention.

@@ +844,5 @@
>  });
> +
> +let pageInfoListener = {
> +
> +  init: function(chromeGlobal) {

I don't see much value in passing in the chromeGlobal like this - might as well just do:

init: function() {
  addMessageListener("PageInfo:getData", this);
},

@@ +845,5 @@
> +
> +let pageInfoListener = {
> +
> +  init: function(chromeGlobal) {
> +    chromeGlobal.addMessageListener("PageInfo:getData", this, false, true);

There's no 4th argument to addMessageListener. Third defaults to false anyway, so this should be:

addMessageListener("PageInfo:getData", this);

@@ +849,5 @@
> +    chromeGlobal.addMessageListener("PageInfo:getData", this, false, true);
> +  },
> +
> +  receiveMessage: function(message) {
> +    this.imageViewRows = [];

So, I know florian already reviewed this, but I've got some more fixes for this base patch.

I see what you're doing here - you're attaching these expandos to the PageInfoListener object so that the various methods on this object don't need to pass things around...

But I think I'd prefer it if they passed things around. Maintaining state like imageViewRows and frameList, even if it's just for the duration of this message handler, is where I've seen bugs creep in.

So what I'd rather we do here is have getMediaInfo return results, like:

let doc = ... // resolve to the right doc
let {frameList, imageViewRows, strings} = this.getMediaInfo(doc);

We don't need window, since we can imply the window from the document via doc.defaultView.

And then we don't have to maintain the imageViewRows, frameList, strings, window or document variables.
Folded patch of
Base patch with suggestions (r+'d by florian)
Follow-up fixes (r+'d by mconley)

try push green for the 2 patches on top of each other
https://treeherder.mozilla.org/#/jobs?repo=try&revision=18e709db0de0

Feel free to merge either this folded patch or the 2 patches separately.
Would recommend the folded patch since the 2 separate patches had a merge conflict as someone had touched one of the files recently.
Attachment #8635566 - Attachment is obsolete: true
Attachment #8640094 - Flags: checkin+
In the future, please try to use commit messages that say what the patch is actually doing instead of restating the problem being solved.
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions

Updated

4 years ago
Duplicate of this bug: 1188875
https://hg.mozilla.org/mozilla-central/rev/b32f21db299c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42

Updated

4 years ago
Depends on: 1207168

Updated

4 years ago
Depends on: 1218144
Depends on: 1241147
You need to log in before you can comment on or make changes to this bug.