Improve stylesheet switcher using DOM style set methods

RESOLVED FIXED

Status

SeaMonkey
General
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: bz, Assigned: neil@parkwaycc.co.uk)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Depends on: 200930
Created attachment 251341 [details] [diff] [review]
Partial fix
(Assignee)

Comment 2

11 years ago
Created attachment 251408 [details] [diff] [review]
Proposed patch
Assignee: general → neil
Attachment #251341 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #251408 - Flags: superreview?(bzbarsky)
Attachment #251408 - Flags: review?(cst)
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.
Attachment #251408 - Flags: superreview?(bzbarsky) → superreview-
(Assignee)

Comment 4

11 years ago
Created attachment 251917 [details] [diff] [review]
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.
Attachment #251408 - Attachment is obsolete: true
Attachment #251917 - Flags: superreview?(bzbarsky)
Attachment #251917 - Flags: review?(iann_bugzilla)
Attachment #251408 - Flags: review?(cst)
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.  ;)
Attachment #251917 - Flags: superreview?(bzbarsky) → superreview+

Comment 6

11 years ago
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.
(Assignee)

Comment 7

11 years ago
Fix checked in assuming ½ each of the review from the previous two comments ;-)
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Attachment #251917 - Flags: review?(iann_bugzilla) → review+
You need to log in before you can comment on or make changes to this bug.