Closed
Bug 1161838
Opened 9 years ago
Closed 9 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)
Firefox
Toolbars and Customization
Tracking
()
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 | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Flags: qe-verify+
Flags: in-testsuite+
Flags: firefox-backlog+
Assignee | ||
Comment 1•9 years ago
|
||
/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)
Assignee | ||
Comment 2•9 years ago
|
||
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 3•9 years ago
|
||
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+
Assignee | ||
Comment 4•9 years ago
|
||
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.
Comment 5•9 years ago
|
||
(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.
Comment 6•9 years ago
|
||
Gijs pointed out over IRC that I missed the second closing parenthesis here. Nothing to see here, move along :)
Assignee | ||
Updated•9 years ago
|
Attachment #8602163 -
Flags: review+ → review?(jaws)
Assignee | ||
Comment 7•9 years ago
|
||
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 8•9 years ago
|
||
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+
Assignee | ||
Comment 10•9 years ago
|
||
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: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Updated•9 years ago
|
QA Contact: andrei.vaida
Comment 12•9 years ago
|
||
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+
Comment 13•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/b7f3f7aa3515 https://hg.mozilla.org/releases/mozilla-release/rev/2eeb61f35995
status-firefox38.0.5:
--- → fixed
status-firefox39:
--- → fixed
Updated•9 years ago
|
status-firefox38:
--- → wontfix
Comment 14•9 years ago
|
||
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+
Assignee | ||
Comment 15•9 years ago
|
||
Attachment #8602163 -
Attachment is obsolete: true
Attachment #8620235 -
Flags: review+
Assignee | ||
Comment 16•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•