366887 - Improve stylesheet switcher using DOM style set methods

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The idea is to use the API introduced in bug 200930 to improve our stylesheet switcher.

I have a partial patch for this, but then I got stuck because I didn't have time to sort out why our code is doing what it's doing (as commented in the patch).

The patch basically makes stylesheetInFrame a one-liner and removes stylesheetSwitchFrame completely.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Partial fix

Comment 2

[[e-mail]]

Attachment: Proposed patch

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 251408 [details] [diff] [review]
Proposed patch

I'd like to see documentation for the args to the functions.  That would also make it a lot easier for me to review the code.

We should probably file a separate bug on Firefox too.

Comment 4

[[e-mail]]

Attachment: With comments

Somehow adding the comments makes the diff less readable :-/

Trying IanN for review this time as CTho doesn't understand the code either.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 251917 [details] [diff] [review]
With comments

>Index: navigator.js
>+  for (var i = 0; i < styleSheetSets.length; i++) {

This loop body accesses styleSheetSets[i] a lot.  Maybe just get it once and put it in a local var?

>+ * Adds all available stylesheet sets to the View > Use Style submenu
>+ *
>+ * If any frame has no preferred stylesheet set then the "Default style"
>+ * menuitem should be shown checked, unless any frame has a selected
>+ * stylesheet set in which case it should be shown unchecked. Otherwise
>+ * unless the "None" style is currently selected then if a stylesheet
>+ * set is selected in all frames then its menuitem should be checked.

This comment is really confusing.  It sounds like there is a list of mutually exclusive possibilities with resulting outcomes; I'd just format it as a list or decision tree...  Also, generally starting with the strongest condition makes the logic clearer.  So in this case, perhaps start with "If the 'None' style is currently selected it remains selected" or something?

I should note that the comment for stylesheetFillFrame managed to be a lot more readable, somehow....  Maybe this comment should just reference that one (and say "call stylesheetFillFrame on all the frames in this frameset")?

Other that that, looks great.  Thanks for the comments!

sr=bzbarsky, and if you care you could try to treat it as an r= too.  ;)

Comment 6

[Ian Neal]

Comment on attachment 251917 [details] [diff] [review]
With comments

The logic looks ok but I've not used this functionality enough to know if it is behaving as it should.

Comment 7

[[e-mail]]

Fix checked in assuming ½ each of the review from the previous two comments ;-)