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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 13

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(4 files)

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.
this is followup API cleanup that part 1 made possible
Attachment #603221 - Flags: review?(dolske)
more API cleanup. I think gPageStyleMenu is pretty sane at this point.
Attachment #603222 - Flags: review?(dolske)
(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.
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)
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 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 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+
Attachment #603222 - Flags: review?(dolske) → review+
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+
> 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
You need to log in before you can comment on or make changes to this bug.