Closed Bug 947988 Opened 10 years ago Closed 10 years ago

Australis: Take switch-to-metro-specific code out of CustomizableUI.jsm

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

(Whiteboard: [beta28] p=0 [Australis:P5][qa-])

Attachments

(1 file)

This should never have gotten review, but now that it is in there, we should get it out ASAP:

hg.mozilla.org/projects/ux/annotate/e28d7509c27f/browser/components/customizableui/src/CustomizableUI.jsm#l1054

http://hg.mozilla.org/projects/ux/annotate/e28d7509c27f/browser/components/customizableui/src/CustomizableUI.jsm#l1751
Whiteboard: [beta28] p=0
Mike, how does this look? Note that while I did some quick testing by disabling the ifdef here, I can't properly test because I don't have a win8 machine. Try push to be sure: https://tbpl.mozilla.org/?tree=Try&rev=c8a415346b81
Attachment #8344768 - Flags: review?(mconley)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(In reply to Brian R. Bondy [:bbondy] from comment #2)
> See also bug 945842

I'm not sure how that is relevant to this bug/patch.
It's relevant because one patch will need to be rebased for the other depending on which one lands first.
(In reply to Brian R. Bondy [:bbondy] from comment #4)
> It's relevant because one patch will need to be rebased for the other
> depending on which one lands first.

Version control is sometimes sad. :-(
(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Brian R. Bondy [:bbondy] from comment #4)
> > It's relevant because one patch will need to be rebased for the other
> > depending on which one lands first.
> 
> Version control is sometimes sad. :-(

Also I should mention that it's hard to see what is relevant to this patch because Comment 0 simply states it shouldn't have gotten a review, without giving an explanation of why, making it hard for one to learn from a mistake made.
Comment on attachment 8344768 [details] [diff] [review]
take Australis switch-to-metro code out of CustomizableUI.jsm,

Review of attachment 8344768 [details] [diff] [review]:
-----------------------------------------------------------------

Yep, much better. Thanks!
Attachment #8344768 - Flags: review?(mconley) → review+
(In reply to Brian R. Bondy [:bbondy] from comment #6)
> (In reply to :Gijs Kruitbosch from comment #5)
> > (In reply to Brian R. Bondy [:bbondy] from comment #4)
> > > It's relevant because one patch will need to be rebased for the other
> > > depending on which one lands first.
> > 
> > Version control is sometimes sad. :-(
> 
> Also I should mention that it's hard to see what is relevant to this patch
> because Comment 0 simply states it shouldn't have gotten a review, without
> giving an explanation of why, making it hard for one to learn from a mistake
> made.

You're right, and I'm sorry for being snarky and not explaining anything. I shouldn't have done that.

Basically, the problem is that CustomizableUI.jsm inasmuch as possible shouldn't have code specific to a single widget. Unfortunately because of the way default areas are assigned it's not possible to tell the widgets a default area in CustomizableWidgets (as you would do for other widgets) because of when they are constructed. I'm actually not sure why they can't be constructed after the areas, in which case we could move the last explicit mentions of the buttons or any logic pertaining to them (the gPalette check for the switch-to-metro button, and the charset check for the charset menu button) outside of this file too.

In any case, the point is, CustomizableUI is meant to be generic stuff managing all the widgets, including add-ons. Add-ons shouldn't need to modify it specifically for their purposes, and neither should we. For each widget that's created, options should be passed in that will cause the right thing to happen.

Instead of doing that, the original patches for this feature modified core code in CustomizableUI to special-case the widget in several places and override the default behaviour. The patch here moved the relevant code to the CustomizableWidgets file, where the widget-specific behaviour for default widgets is arranged (for widgets that don't warrant their own files/arch, anyway, like the downloads button, URL bar, etc.). This way, the widget passes in the correct information into CustomizableUI and things Just Work.

Part of the problem here is lacking documentation about CustomizableUI and widgets. I'm working on that in bug 942393.
remote:   https://hg.mozilla.org/integration/fx-team/rev/3ccff2141bb0
Whiteboard: [beta28] p=0 → [beta28] p=0 [Australis:P5][fixed-in-fx-team]
I see, thanks for the explanation!
https://hg.mozilla.org/mozilla-central/rev/3ccff2141bb0
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [beta28] p=0 [Australis:P5][fixed-in-fx-team] → [beta28] p=0 [Australis:P5]
Target Milestone: --- → Firefox 29
No longer blocks: metrov1backlog
Whiteboard: [beta28] p=0 [Australis:P5] → [beta28] p=0 [Australis:P5][qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: