Closed
Bug 733339
Opened 9 years ago
Closed 9 years ago
Legacy style sheet switching functions depend on all gPageStyleMenu methods having a global equivalent by the same name
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 13
People
(Reporter: dao, Assigned: dao)
References
Details
Attachments
(4 files)
7.60 KB,
patch
|
Dolske
:
review+
|
Details | Diff | Splinter Review |
3.63 KB,
patch
|
Dolske
:
review+
|
Details | Diff | Splinter Review |
4.10 KB,
patch
|
Dolske
:
review+
|
Details | Diff | Splinter Review |
2.04 KB,
patch
|
Dolske
:
review+
|
Details | Diff | Splinter Review |
e.g. stylesheetSwitchAll() depends on stylesheetSwitchFrame being global, since 'this' is the window object. This is going to be unexpected when someone expands gPageStyleMenu without making new stuff global as well.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #603219 -
Flags: review?(dolske)
Assignee | ||
Comment 2•9 years ago
|
||
this is followup API cleanup that part 1 made possible
Attachment #603221 -
Flags: review?(dolske)
Assignee | ||
Comment 3•9 years ago
|
||
more API cleanup. I think gPageStyleMenu is pretty sane at this point.
Attachment #603222 -
Flags: review?(dolske)
Assignee | ||
Comment 4•9 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #3) > Created attachment 603222 [details] [diff] [review] > part 3: make the content window passed to gPageStyleMenu.switchStyleSheet > optional Note that this makes switchStyleSheet consistent with the other non-internal methods. The reason why it required the content window is that the global stylesheetSwitchAll function was recursive.
Assignee | ||
Comment 5•9 years ago
|
||
this is really internal but some add-ons (ab)use it for stuff that has nothing to do with the page style menu
Attachment #603226 -
Flags: review?(dolske)
Assignee | ||
Updated•9 years ago
|
Summary: Legacy style sheet switching functions depend on all functions being global → Legacy style sheet switching functions depend on all gPageStyleMenu methods having a global equivalent by the same name
Comment 6•9 years ago
|
||
Comment on attachment 603219 [details] [diff] [review] part 1: remove unneeded legacy functions and bind remaining ones to gPageStyleMenu Review of attachment 603219 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +6233,5 @@ > }, > }; > > /* Legacy global page-style functions */ > +var getAllStyleSheets = gPageStyleMenu.getAllStyleSheets.bind(gPageStyleMenu); Is the bind needed? Oh my, it is. I am dumb and my patch that added these was only working by accident. :(
Attachment #603219 -
Flags: review?(dolske) → review+
Comment 7•9 years ago
|
||
Comment on attachment 603221 [details] [diff] [review] part 2: replace gPageStyleMenu.setStyleDisabled with gPageStyleMenu.disableStyle Review of attachment 603221 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-menubar.inc @@ +291,5 @@ > </menu> > <menu id="pageStyleMenu" label="&pageStyleMenu.label;" > accesskey="&pageStyleMenu.accesskey;" observes="isImage"> > <menupopup onpopupshowing="gPageStyleMenu.fillPopup(this);" > + oncommand="gPageStyleMenu.switchStyleSheet(window.content, event.target.getAttribute('data'));"> Might be nice to get rid of the reliance on event propagation, and just have the various oncommands do this work individually or with a helper. Fine either way though.
Attachment #603221 -
Flags: review?(dolske) → review+
Updated•9 years ago
|
Attachment #603222 -
Flags: review?(dolske) → review+
Comment 8•9 years ago
|
||
Comment on attachment 603226 [details] [diff] [review] part 4: mark getAllStyleSheets as internal while keeping the legacy function Review of attachment 603226 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for splitting this into pieces.
Attachment #603226 -
Flags: review?(dolske) → review+
Assignee | ||
Comment 9•9 years ago
|
||
> Might be nice to get rid of the reliance on event propagation, and just have > the various oncommands do this work individually or with a helper. Fine > either way though. filed bug 734706
Assignee | ||
Comment 10•9 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/5f8a4ddce900
Target Milestone: --- → Firefox 13
Assignee | ||
Comment 11•9 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/cb70875d31bc http://hg.mozilla.org/integration/mozilla-inbound/rev/f397bf34d922 http://hg.mozilla.org/integration/mozilla-inbound/rev/f5f3a550257b
Comment 12•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f5f3a550257b https://hg.mozilla.org/mozilla-central/rev/f397bf34d922 https://hg.mozilla.org/mozilla-central/rev/cb70875d31bc https://hg.mozilla.org/mozilla-central/rev/5f8a4ddce900
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•