733339 - Legacy style sheet switching functions depend on all gPageStyleMenu methods having a global equivalent by the same name

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [::dao]]

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.

Comment 1

[Dão Gottwald [::dao]]

Attachment: part 1: remove unneeded legacy functions and bind remaining ones to gPageStyleMenu

Comment 2

[Dão Gottwald [::dao]]

Attachment: part 2: replace gPageStyleMenu.setStyleDisabled with gPageStyleMenu.disableStyle

this is followup API cleanup that part 1 made possible

Comment 3

[Dão Gottwald [::dao]]

Attachment: part 3: make the content window passed to gPageStyleMenu.switchStyleSheet optional

more API cleanup. I think gPageStyleMenu is pretty sane at this point.

Comment 4

[Dão Gottwald [::dao]]

(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.

Comment 5

[Dão Gottwald [::dao]]

Attachment: part 4: mark getAllStyleSheets as internal while keeping the legacy function

this is really internal but some add-ons (ab)use it for stuff that has nothing to do with the page style menu

Comment 6

[Justin Dolske [:Dolske]]

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. :(

Comment 7

[Justin Dolske [:Dolske]]

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.

Comment 8

[Justin Dolske [:Dolske]]

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.

Comment 9

[Dão Gottwald [::dao]]

> 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

Comment 10

[Dão Gottwald [::dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/5f8a4ddce900

Comment 11

[Dão Gottwald [::dao]]

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

[Daniel Holbert [:dholbert]]

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