Closed Bug 1161838 Opened 6 years ago Closed 6 years ago

Widgets added to the navbar using the pref-based facility after customizing the navbar end up at the end of the navbar regardless of desired position

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 40
Iteration:
40.3 - 11 May
Tracking Status
firefox38 --- wontfix
firefox38.0.5 --- verified
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: adw, Assigned: Gijs)

References

Details

Attachments

(1 file, 1 obsolete file)

Something like that anyway.  I haven't looked into why too much yet.
Assignee: nobody → gijskruitbosch+bugs
Blocks: 1155521
Status: NEW → ASSIGNED
Iteration: --- → 40.3 - 11 May
Points: --- → 3
See Also: 1155521
Flags: qe-verify+
Flags: in-testsuite+
Flags: firefox-backlog+
Attached file MozReview Request: bz://1161838/Gijs (obsolete) —
/r/8205 - Bug 1161838 - fix positioning of newly added widgets, r?jaws

Pull down this commit:

hg pull -r 139f3a379625519905136138bf58dd1dacd08f92 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8602163 - Flags: review?(jaws)
https://reviewboard.mozilla.org/r/8203/#review6967

::: browser/components/customizableui/test/browser_1161838_inserted_new_default_buttons.js:67
(Diff revision 1)
> +  debugger;

Forgot about this. Let's pretend it's not there...
Comment on attachment 8602163 [details]
MozReview Request: bz://1161838/Gijs

https://reviewboard.mozilla.org/r/8203/#review6969

conditional r+ assuming nothing too drastic has to be changed to fix the issues brought up below.

::: browser/components/customizableui/test/browser_1161838_inserted_new_default_buttons.js:10
(Diff revision 1)
> +  let CustomizableUIBSPass = Cu.import("resource:///modules/CustomizableUI.jsm", {});

I don't see anything here checking what version we're on. I think this comment is in the wrong place.

::: browser/components/customizableui/test/browser_1161838_inserted_new_default_buttons.js:32
(Diff revision 1)
> +                                                                CustomizableUI.SOURCE_BUILTIN);

(Kinda stinks how easy it is for an add-on to subvert our SOURCE_BUILTIN checks)

::: browser/components/customizableui/test/browser_1161838_inserted_new_default_buttons.js:47
(Diff revision 1)
> +    CustomizableUIBSPass.gSavedState = {

Yuck! This test has two code paths for either setting the saved state or for modifying it if it exists. Can we clear the saved state at the beginning of the test or do something to guarantee that it gets set through normal code paths (such as opening Customization mode, tweaking something, exiting, reopening, restoring defaults, exiting)?

::: browser/components/customizableui/test/browser_1161838_inserted_new_default_buttons.js:71
(Diff revision 1)
> +  info(savedPlacements.join(', '));

nit: info("Saved placements: " + savedPlacements.join(", "));

::: browser/components/customizableui/test/browser_1161838_inserted_new_default_buttons.js:64
(Diff revision 1)
> +    let placements = [...gFuturePlacements.get(CustomizableUI.AREA_NAVBAR)];

Instead of doing a .has() call three lines above and then a .get() call here, can we just do a .get() call three lines above and then reuse that variable here?

::: browser/components/customizableui/test/browser_1161838_inserted_new_default_buttons.js:76
(Diff revision 1)
> +  if (haveNavbarPlacements) {

Can this one also have a ok(haveNavbarPlacements, "...") call before the if() like is done about 15 line earlier?

::: browser/components/customizableui/CustomizableUI.jsm:363
(Diff revision 1)
> +          (defaultWidgetIndex = defaultPlacements.indexOf(widget.id)) === -1) {

Are you sure you want this line to do as you have written it? defaultWidgetIndex here will be converted from a Number (-1) to a Boolean (true or false) depending on the indexOf() === -1 comparison.

A bit later on defaultWidgetIndex is used as a Number type in the for-loop (let i = defaultWidgetIndex; i > 0; i--).

At any response, we shouldn't be using a variable named 'Index' as an Boolean that then gets coerced to a Number.
Attachment #8602163 - Flags: review?(jaws) → review+
https://reviewboard.mozilla.org/r/8203/#review6971

> Are you sure you want this line to do as you have written it? defaultWidgetIndex here will be converted from a Number (-1) to a Boolean (true or false) depending on the indexOf() === -1 comparison.
> 
> A bit later on defaultWidgetIndex is used as a Number type in the for-loop (let i = defaultWidgetIndex; i > 0; i--).
> 
> At any response, we shouldn't be using a variable named 'Index' as an Boolean that then gets coerced to a Number.

This doesn't make sense to me. defaultWidgetIndex will be a number. It's a number after the comparison with -1 still, just like it would be if we did, say:

foo === -1

and that doesn't change anything about foo.

> I don't see anything here checking what version we're on. I think this comment is in the wrong place.

Yeah, the comment should have been removed.

> Yuck! This test has two code paths for either setting the saved state or for modifying it if it exists. Can we clear the saved state at the beginning of the test or do something to guarantee that it gets set through normal code paths (such as opening Customization mode, tweaking something, exiting, reopening, restoring defaults, exiting)?

I would prefer to just change gDirty and force a save. Cheating, but a lot faster and less prone to intermittent test failures.

> Can this one also have a ok(haveNavbarPlacements, "...") call before the if() like is done about 15 line earlier?

Nothing changes haveNavbarPlacements, so there was no point.
(In reply to :Gijs Kruitbosch from comment #4)
> https://reviewboard.mozilla.org/r/8203/#review6971
> 
> > Are you sure you want this line to do as you have written it? defaultWidgetIndex here will be converted from a Number (-1) to a Boolean (true or false) depending on the indexOf() === -1 comparison.
> > 
> > A bit later on defaultWidgetIndex is used as a Number type in the for-loop (let i = defaultWidgetIndex; i > 0; i--).
> > 
> > At any response, we shouldn't be using a variable named 'Index' as an Boolean that then gets coerced to a Number.
> 
> This doesn't make sense to me. defaultWidgetIndex will be a number. It's a
> number after the comparison with -1 still, just like it would be if we did,
> say:
> 
> foo === -1
> 
> and that doesn't change anything about foo.
> 

The line in question is:
> defaultWidgetIndex = defaultPlacements.indexOf(widget.id)) === -1

This assigns the result of the === comparison to defaultWidgetIndex. So it will either be `true` or `false` which will later be used as `1` or `0`, respectively.
Gijs pointed out over IRC that I missed the second closing parenthesis here. Nothing to see here, move along :)
Attachment #8602163 - Flags: review+ → review?(jaws)
Comment on attachment 8602163 [details]
MozReview Request: bz://1161838/Gijs

/r/8205 - Bug 1161838 - fix positioning of newly added widgets, r?jaws

Pull down this commit:

hg pull -r 631e4fbfc45429fa2f7dac217913eec5da88bcb0 https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8602163 [details]
MozReview Request: bz://1161838/Gijs

https://reviewboard.mozilla.org/r/8203/#review6993

Ship It!

::: browser/components/customizableui/CustomizableUI.jsm:366
(Diff revisions 1 - 2)
> +      if (defaultWidgetIndex === -1) {

Thanks, this is easier to read.
Attachment #8602163 - Flags: review?(jaws) → review+
Comment on attachment 8602163 [details]
MozReview Request: bz://1161838/Gijs

Approval Request Comment
[Feature/regressing bug #]: pocket: inserting pocket toolbar button wouldn't happen in the right place without this
[User impact if declined]: ditto
[Describe test coverage new/current, TreeHerder]: has a test
[Risks and why]: medium-low. Uplifting a change to our core customization code to our beta branch is a little scary. On the other hand, all we're doing is improving how we handle newly added default widgets, and it has a unit test, and there's still some weeks of test coverage left. The one thing I can think of that would be quite useful is if, after landing, someone from QA would check how upgrading from a pre-loop/hello copy of Firefox (33 or something) with a customized navbar to 38-with-loop-and-pocket would work. In principle it should work fine, but it'd be great to verify that this is the case.
[String/UUID change made/needed]: nope
Attachment #8602163 - Flags: approval-mozilla-beta?
Attachment #8602163 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/79ce48075b62
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
QA Contact: andrei.vaida
Comment on attachment 8602163 [details]
MozReview Request: bz://1161838/Gijs

Required for 38.0.5 Pocket launch.
Attachment #8602163 - Flags: approval-mozilla-beta?
Attachment #8602163 - Flags: approval-mozilla-beta+
Attachment #8602163 - Flags: approval-mozilla-aurora?
Attachment #8602163 - Flags: approval-mozilla-aurora+
Verified fixed across the following platforms: 
* Windows 7 x64: updates successfully applied to 38.0.5 beta 1 (Build ID: 20150511143336) from 33.0b2 (en-US, ru and de builds) with a customized navbar: both buttons are properly displayed
* Mac OSX 10.9.5: updates successfully applied to 38.0.5 beta 1 from 33.0b4 (en-US, ja and de builds) with a customized navbar; both buttons are properly displayed on en-US and de; the only issue here is that Pocket is not enabled on ja builds (bug 1164407)
* Ubuntu 14.04 x32: updates successfully applied to 38.0.5 beta 1 from 33.0b5 (en-US, es-ES and ru builds) with a customized navbar - both buttons are properly displayed
Flags: qe-verify+
No longer depends on: 1163512
Attachment #8602163 - Attachment is obsolete: true
Attachment #8620235 - Flags: review+
You need to log in before you can comment on or make changes to this bug.