Closed
Bug 652510
Opened 14 years ago
Closed 11 years ago
Make the Page Style menu ready for e10s
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 30
Tracking | Status | |
---|---|---|
firefox30 | --- | disabled |
People
(Reporter: dao, Assigned: billm)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 4 obsolete files)
10.04 KB,
patch
|
ttaubert
:
review+
|
Details | Diff | Splinter Review |
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.
Attachment #528076 -
Flags: review?(gavin.sharp)
Reporter | ||
Comment 1•14 years ago
|
||
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...
Reporter | ||
Comment 2•14 years ago
|
||
switched to the window's messageManager for loading content.js, since we want it for every browser
Attachment #528076 -
Attachment is obsolete: true
Attachment #528076 -
Flags: review?(gavin.sharp)
Attachment #536834 -
Flags: review?(gavin.sharp)
Comment 3•14 years ago
|
||
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?
Reporter | ||
Comment 4•14 years ago
|
||
Probably accessible/tests/mochitest/tree/test_tabbrowser.xul and accessible/tests/mochitest/relations/test_tabbrowser.xul.
Comment 5•13 years ago
|
||
Comment on attachment 536834 [details] [diff] [review]
patch v2
Cancelling e10s review request since we're not going to be pursuing e10s on desktop.
Attachment #536834 -
Flags: review?(gavin.sharp)
Reporter | ||
Updated•13 years ago
|
Assignee: dao → nobody
Status: ASSIGNED → NEW
Assignee | ||
Comment 6•11 years ago
|
||
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.
Assignee: nobody → wmccloskey
Attachment #536834 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8387372 -
Flags: review?(ttaubert)
Assignee | ||
Comment 7•11 years ago
|
||
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•11 years ago
|
||
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.
Attachment #8387372 -
Flags: review?(ttaubert) → feedback+
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #8387372 -
Attachment is obsolete: true
Attachment #8387889 -
Flags: review?(ttaubert)
Assignee | ||
Comment 10•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
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.
Attachment #8387889 -
Flags: review?(ttaubert) → feedback+
Comment 13•11 years ago
|
||
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.
Attachment #8387889 -
Flags: feedback+
Comment 14•11 years ago
|
||
Sorry for the double post, Bugzilla was giving me internal errors.
Assignee | ||
Comment 15•11 years ago
|
||
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).
Attachment #8387889 -
Attachment is obsolete: true
Attachment #8388928 -
Flags: review?(ttaubert)
Comment 16•11 years ago
|
||
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.
Attachment #8388928 -
Flags: review?(ttaubert) → review+
Assignee | ||
Comment 17•11 years ago
|
||
Comment 18•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Comment 19•11 years ago
|
||
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? :-)
Flags: needinfo?(wmccloskey)
Assignee | ||
Comment 20•11 years ago
|
||
I'm afraid I don't see this either. Can you try to find a more specific STR?
Flags: needinfo?(wmccloskey)
Updated•11 years ago
|
QA Whiteboard: [qa-]
Comment 21•11 years ago
|
||
Due to bug 1001996 and Bug 1002000 at least,
I think Bug 652510 should be backed out when Aurora30.0a1 become Beta30.0
Flags: needinfo?(wmccloskey)
Reporter | ||
Comment 22•11 years ago
|
||
(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.
Assignee | ||
Comment 23•11 years ago
|
||
I backed this out of 30.
https://hg.mozilla.org/releases/mozilla-beta/rev/30a4e7382829
Flags: needinfo?(wmccloskey)
Comment 24•11 years ago
|
||
Updated•11 years ago
|
status-firefox30:
--- → disabled
You need to log in
before you can comment on or make changes to this bug.
Description
•