652510 - Make the Page Style menu ready for e10s

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

Basically this just adds a central content script, adds a few messages and moves half of the page style menu code to the content script.

Comment 1

[Dão Gottwald [:dao]]

Comment on attachment 528076 [details] [diff] [review]
patch

>-function stylesheetFillPopup(menuPopup) {
>+function stylesheetFillPopup(menuPopup, aAdditionalCallback) {
>+  var manager = gBrowser.selectedBrowser.messageManager;
>+
>+  function messageListener(msg) {
>+    removeListener();
>+
>+    stylesheetFillPopup._callback(menuPopup,
>+                                  msg.json.styleSheets,
>+                                  msg.json.authorStyleDisabled);
>+
>+    if (aAdditionalCallback)
>+      aAdditionalCallback();

Note that I the additional callback is only used in browser_page_style_menu.js.

>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml

>             // NB: this appendChild call causes us to run constructors for the
>             // browser element, which fires off a bunch of notifications. Some
>             // of those notifications can cause code to run that inspects our
>             // state, so it is important that the tab element is fully
>             // initialized by this point.
>             this.mPanelContainer.appendChild(notificationbox);
>+            if (b.messageManager)
>+              b.messageManager.loadFrameScript("chrome://browser/content/content.js", true);

>       <constructor>
>         <![CDATA[
>           this.mCurrentBrowser = this.mPanelContainer.childNodes[0].firstChild.firstChild;
>           this.mCurrentTab = this.tabContainer.firstChild;
>           document.addEventListener("keypress", this, false);
>+          if (this.mCurrentBrowser.messageManager)
>+            this.mCurrentBrowser.messageManager.loadFrameScript("chrome://browser/content/content.js", true);

The "if (...messageManager)" checks are for the fake tabbrowsers in a11y tests...

Comment 2

[Dão Gottwald [:dao]]

Attachment: patch v2

switched to the window's messageManager for loading content.js, since we want it for every browser

Comment 3

[:Felipe Gomes (needinfo for replies!)]

The electrolysis project branch is now open for this type of patches, if you want to land there instead of dealing with m-c. I'm merging m-c -> e10s daily and will merge e10s -> m-c often.

Please add the MPL header to the scripts with # before landing

> The "if (...messageManager)" checks are for the fake tabbrowsers in a11y
> tests...

do you recall what were these tests? I know this is not needed for this patch anymore but is it possible to fix the test instead of adding the check?

Comment 4

[Dão Gottwald [:dao]]

Probably accessible/tests/mochitest/tree/test_tabbrowser.xul and accessible/tests/mochitest/relations/test_tabbrowser.xul.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 536834 [details] [diff] [review]
patch v2

Cancelling e10s review request since we're not going to be pursuing e10s on desktop.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: page-style

I was going to use messages to do this, but it turns out that we expose getAllStyleSheets to add-ons, and it's actually used by a few. We already use CPOWs for similar menus like the context menu and the history menu, so I don't think it's so bad to use them here too.

Would you mind reviewing this Tim? It's pretty similar to some stuff we've done in the session store code.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Oh, I forgot to say that getAllStyleSheets ignores the window that you pass in. It just uses the top window of the currently selected browser. All the add-ons that use this function just pass in gBrowser.contentWindow, so it seems fine to do that.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8387372 [details] [diff] [review]
page-style

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

Looks good in general, just a few style nits and questions.

::: browser/base/content/browser.js
@@ +5223,5 @@
> +      styleSheetInfo = handler.getStyleSheetInfo();
> +    } catch (ex) {
> +      // In case the child died or timed out.
> +      styleSheetInfo = {styleSheets: [], authorStyleDisabled: false, preferredStyleSheetSet: true};
> +    }

Would be nice to move this to a separate function like this.getStyleSheetInfo() as well.

@@ +5271,3 @@
>    switchStyleSheet: function (title, contentWindow) {
> +    let mm = gBrowser.selectedBrowser.messageManager;
> +    mm.sendAsyncMessage("PageStyle:Switch", {title: title});

Should this ignore the passed in contentWindow as well?

::: browser/base/content/content.js
@@ +297,5 @@
> +    addMessageListener("PageStyle:Disable", this);
> +
> +    // Send a CPOW to the parent so that it can synchronously request
> +    // the list of style sheets.
> +    sendSyncMessage("PageStyle:SetSyncHandler", {}, {syncHandler: this});

Should we be worried about exposing everything here?

@@ +316,5 @@
> +  // Called synchronously via CPOW from the parent.
> +  getAllStyleSheets: function(frameset) {
> +    if (!frameset) {
> +      frameset = content;
> +    }

Maybe: getAllStyleSheets: function(frameset = content) { ?

@@ +318,5 @@
> +    if (!frameset) {
> +      frameset = content;
> +    }
> +
> +    var styleSheetsArray = Array.slice(frameset.document.styleSheets);

let styleSheetsArray = [...frameset.document.styleSheets];

@@ +319,5 @@
> +      frameset = content;
> +    }
> +
> +    var styleSheetsArray = Array.slice(frameset.document.styleSheets);
> +    for (let i = 0; i < frameset.frames.length; i++) {

for (let frame of frameset.frames) {

@@ +321,5 @@
> +
> +    var styleSheetsArray = Array.slice(frameset.document.styleSheets);
> +    for (let i = 0; i < frameset.frames.length; i++) {
> +      let frameSheets = this.getAllStyleSheets(frameset.frames[i]);
> +      styleSheetsArray = styleSheetsArray.concat(frameSheets);

styleSheetsArray.push(...frameSheets);

@@ +324,5 @@
> +      let frameSheets = this.getAllStyleSheets(frameset.frames[i]);
> +      styleSheetsArray = styleSheetsArray.concat(frameSheets);
> +    }
> +    return styleSheetsArray;
> +  },

I wonder...

return Array.reduce(frameset.frames, (acc, frame) => {
  acc.push(...this.getAllStyleSheets(frame));
}, [...frameset.document.styleSheets]);

... is that better? Because that's pretty much what we do here.

@@ +328,5 @@
> +  },
> +
> +  receiveMessage: function(msg) {
> +    let markupDocumentViewer =
> +      docShell.contentViewer.QueryInterface(Ci.nsIMarkupDocumentViewer);

This could be a getter and re-used in getStyleSheetInfo().

@@ +347,5 @@
> +    if (!title || this._stylesheetInFrame(frameset, title))
> +      this._stylesheetSwitchFrame(frameset, title);
> +
> +    for (let i = 0; i < frameset.frames.length; i++)
> +      this._stylesheetSwitchAll(frameset.frames[i], title);

for (let frame of frameset.frames) {

And please add a comment that were recursing into subframes here.

@@ +354,5 @@
> +  _stylesheetSwitchFrame: function (frame, title) {
> +    var docStyleSheets = frame.document.styleSheets;
> +
> +    for (let i = 0; i < docStyleSheets.length; ++i) {
> +      let docStyleSheet = docStyleSheets[i];

for (let docStyleSheet of frame.document.styleSheets) {

@@ +365,5 @@
> +  },
> +
> +  _stylesheetInFrame: function (frame, title) {
> +    return Array.some(frame.document.styleSheets,
> +                      function (stylesheet) stylesheet.title == title);

Nit: this could probably be a one-liner using an arrow-function... :)

@@ +372,5 @@
> +  _filterStyleSheets: function(styleSheets) {
> +    let result = [];
> +
> +    for (let i = 0; i < styleSheets.length; ++i) {
> +      let currentStyleSheet = styleSheets[i];

Nit: for (let currentStyleSheet of styleSheets) {

@@ +379,5 @@
> +        continue;
> +
> +      // Skip any stylesheets that don't match the screen media type.
> +      if (currentStyleSheet.media.length > 0) {
> +        let media = currentStyleSheet.media.mediaText.split(", ");

Should this be .split(/\s*,\s/*)?

@@ +382,5 @@
> +      if (currentStyleSheet.media.length > 0) {
> +        let media = currentStyleSheet.media.mediaText.split(", ");
> +        if (media.indexOf("screen") == -1 &&
> +            media.indexOf("all") == -1)
> +          continue;

Why did you switch from window.matchMedia() to white-listing screen and all here? I'm not sure how this affects behavior.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: page-style v2

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I made the changes you requested except as explained below.

> @@ +5271,3 @@
> >    switchStyleSheet: function (title, contentWindow) {
> > +    let mm = gBrowser.selectedBrowser.messageManager;
> > +    mm.sendAsyncMessage("PageStyle:Switch", {title: title});
> 
> Should this ignore the passed in contentWindow as well?

It looks like that's unused by add-ons. I took out the extra parameter.

> ::: browser/base/content/content.js
> @@ +297,5 @@
> > +    addMessageListener("PageStyle:Disable", this);
> > +
> > +    // Send a CPOW to the parent so that it can synchronously request
> > +    // the list of style sheets.
> > +    sendSyncMessage("PageStyle:SetSyncHandler", {}, {syncHandler: this});
> 
> Should we be worried about exposing everything here?

You mean exposing the extra methods to the parent? I guess it doesn't seem to important. Are you worried about exposing stuff to add-ons? I guess I'll defer to your judgment; I don't have much experience with that stuff.

> @@ +318,5 @@
> > +    if (!frameset) {
> > +      frameset = content;
> > +    }
> > +
> > +    var styleSheetsArray = Array.slice(frameset.document.styleSheets);
> 
> let styleSheetsArray = [...frameset.document.styleSheets];

I wasn't able to make these sorts of changes. I sort of remember we had a similar problem in session restore with style sheets. document.styleSheets is an nsIDOMStyleSheetList. There's some support in nsDOMClassInfo.cpp for making them look "array-like", but it's not enough to support these fancy new JS features.

I did change the stuff that only involved real arrays.

> @@ +324,5 @@
> > +      let frameSheets = this.getAllStyleSheets(frameset.frames[i]);
> > +      styleSheetsArray = styleSheetsArray.concat(frameSheets);
> > +    }
> > +    return styleSheetsArray;
> > +  },
> 
> I wonder...
> 
> return Array.reduce(frameset.frames, (acc, frame) => {
>   acc.push(...this.getAllStyleSheets(frame));
> }, [...frameset.document.styleSheets]);
> 
> ... is that better? Because that's pretty much what we do here.

You are a wizard! But given the problems I had above, I didn't attempt this.

> @@ +382,5 @@
> > +      if (currentStyleSheet.media.length > 0) {
> > +        let media = currentStyleSheet.media.mediaText.split(", ");
> > +        if (media.indexOf("screen") == -1 &&
> > +            media.indexOf("all") == -1)
> > +          continue;
> 
> Why did you switch from window.matchMedia() to white-listing screen and all
> here? I'm not sure how this affects behavior.

Sorry, I copied that code from Dão's patch and it looks like things have changed since then. I've fixed the problem.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Bill McCloskey (:billm) from comment #10)
> > > +    sendSyncMessage("PageStyle:SetSyncHandler", {}, {syncHandler: this});
> > 
> > Should we be worried about exposing everything here?
> 
> You mean exposing the extra methods to the parent? I guess it doesn't seem
> to important. Are you worried about exposing stuff to add-ons? I guess I'll
> defer to your judgment; I don't have much experience with that stuff.

I agree this doesn't really look like a supported API. If someone uses this they shouldn't be surprised if it breaks.

> > > +    var styleSheetsArray = Array.slice(frameset.document.styleSheets);
> > 
> > let styleSheetsArray = [...frameset.document.styleSheets];
> 
> I wasn't able to make these sorts of changes. I sort of remember we had a
> similar problem in session restore with style sheets. document.styleSheets
> is an nsIDOMStyleSheetList. There's some support in nsDOMClassInfo.cpp for
> making them look "array-like", but it's not enough to support these fancy
> new JS features.

Aww :( I just found bug 738196.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8387889 [details] [diff] [review]
page-style v2

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

::: browser/base/content/browser.js
@@ +5212,5 @@
> +      return handler.getAllStyleSheets();
> +    //} catch (ex) {
> +      // In case the child died or timed out.
> +      //return [];
> +    //}

Left-over code? Do we need that in case the child dies?

@@ +5222,5 @@
> +      return handler.getStyleSheetInfo();
> +    //} catch (ex) {
> +      // In case the child died or timed out.
> +      //return {styleSheets: [], authorStyleDisabled: false, preferredStyleSheetSet: true};
> +    //}

Same here.

@@ +5286,3 @@
>  var stylesheetFillPopup = gPageStyleMenu.fillPopup.bind(gPageStyleMenu);
>  function stylesheetSwitchAll(contentWindow, title) {
>    gPageStyleMenu.switchStyleSheet(title, contentWindow);

Please remove the second parameter here as well and add a comment that explains why we're ignoring it.

::: browser/base/content/content.js
@@ +320,5 @@
> +    var styleSheetsArray = Array.slice(frameset.document.styleSheets);
> +    for (let i = 0; i < frameset.frames.length; i++) {
> +      let frameSheets = this.getAllStyleSheets(frameset.frames[i]);
> +      styleSheetsArray.push(...frameSheets);
> +    }

You could still use Array.reduce() here, just pass Array.slice(frameset.document.styleSheets) as the initial acc.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8387889 [details] [diff] [review]
page-style v2

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

::: browser/base/content/browser.js
@@ +5212,5 @@
> +      return handler.getAllStyleSheets();
> +    //} catch (ex) {
> +      // In case the child died or timed out.
> +      //return [];
> +    //}

Left-over code? Do we need that in case the child dies?

@@ +5222,5 @@
> +      return handler.getStyleSheetInfo();
> +    //} catch (ex) {
> +      // In case the child died or timed out.
> +      //return {styleSheets: [], authorStyleDisabled: false, preferredStyleSheetSet: true};
> +    //}

Same here.

@@ +5286,3 @@
>  var stylesheetFillPopup = gPageStyleMenu.fillPopup.bind(gPageStyleMenu);
>  function stylesheetSwitchAll(contentWindow, title) {
>    gPageStyleMenu.switchStyleSheet(title, contentWindow);

Please remove the second parameter here as well and add a comment that explains why we're ignoring it.

::: browser/base/content/content.js
@@ +320,5 @@
> +    var styleSheetsArray = Array.slice(frameset.document.styleSheets);
> +    for (let i = 0; i < frameset.frames.length; i++) {
> +      let frameSheets = this.getAllStyleSheets(frameset.frames[i]);
> +      styleSheetsArray.push(...frameSheets);
> +    }

You could still use Array.reduce() here, just pass Array.slice(frameset.document.styleSheets) as the initial acc.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Sorry for the double post, Bugzilla was giving me internal errors.

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: page-style v3

The commented out code was for debugging. I wanted to see non-CPOW exceptions coming from the code in content.js. I wish there were a way to intercept only exceptions created when the process crashes, but I think we just throw strings in that case.

I figured out some code using Array.map instead of Array.reduce. I think it's a little easier to follow. I had hoped to use a list comprehension, but that also runs into a problem where frameset.frames doesn't support the new iterator protocol (just like frameset.document.styleSheets).

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8388928 [details] [diff] [review]
page-style v3

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

(In reply to Bill McCloskey (:billm) from comment #15)
> The commented out code was for debugging. I wanted to see non-CPOW
> exceptions coming from the code in content.js. I wish there were a way to
> intercept only exceptions created when the process crashes, but I think we
> just throw strings in that case.

Ah, okay. Too bad.

> I figured out some code using Array.map instead of Array.reduce. I think
> it's a little easier to follow. I had hoped to use a list comprehension, but
> that also runs into a problem where frameset.frames doesn't support the new
> iterator protocol (just like frameset.document.styleSheets).

I like it, looks a lot clearer.

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f2f5c15e065

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4f2f5c15e065

Comment 19

[:Gijs (he/him)]

When dragging items about in customize mode, I'm seeing lots of:

System JS : ERROR chrome://browser/content/browser.js:14764 - TypeError: value is not a non-null object


which is this line in my build:

      this._pageStyleSyncHandlers.set(msg.target.permanentKey, msg.objects.syncHandler);

Tim suggests it might be to do with this bug. It looks like msg.target.permanentKey is undefined... Tim couldn't reproduce, though... Bill, can you? :-)

Comment 20

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'm afraid I don't see this either. Can you try to find a more specific STR?

Comment 21

[Alice0775 White]

Due to bug 1001996 and Bug 1002000 at least, 
I think Bug 652510 should be backed out when Aurora30.0a1 become Beta30.0

Comment 22

[Dão Gottwald [:dao]]

(In reply to Alice0775 White from comment #21)
> Due to bug 1001996 and Bug 1002000 at least, 
> I think Bug 652510 should be backed out when Aurora30.0a1 become Beta30.0

I agree, let's back this out from 30 and fix the regressions in 31.

Comment 23

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I backed this out of 30.
https://hg.mozilla.org/releases/mozilla-beta/rev/30a4e7382829

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/30a4e7382829