[Note]: - This is a follow up bug filed from Bug 1245074 Comment 45 [Affected versions]: - 47.0a2 (2016-04-05) [Affected platforms]: - Ubuntu 12.04 x64, - Ubuntu 14.04 x86, - Ubuntu 14.04 x64, - Mac OS X 10.11, - Windows 10 x64. [Steps to reproduce]: 1. Launch Firefox with a clean profile. 2. Click on Menu panel (Hamburger button). 3. Select Customize. 4. Check the state of the "Restore Defaults" button. 5. Click the "Restore Defaults" button. [Expected result]: - 'Restore Defaults' button is disabled. [Actual result]: - 'Restore Defaults' button is enabled by default (step 3). - Clicking the button causes a position switch between the Hello (Loop) button and the WebIDE button (step 5). [Regression range]: Pretty much the same as in Bug 1245074 Comment 0: - Last good revision: 0c12d4229be0068eb1b9beb5064b800f12143f20 - First bad revision: 3ba655f6bc67660a2dcfc4c2a5b3d0d17714f53d - Pushlog: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=0c12d4229be0068eb1b9beb5064b800f12143f20&tochange=3ba655f6bc67660a2dcfc4c2a5b3d0d17714f53d - Regressed by 3ba655f6bc67 Shane Caraveo — Bug 1215694 move pocket to a system addon [Additional notes]: - This is reproducible on Fx47 only.

(In reply to Iulia Cristescu, QA [:IuliaC] from comment #0) > [Regression range]: > Pretty much the same as in Bug 1245074 Comment 0: > - Last good revision: 0c12d4229be0068eb1b9beb5064b800f12143f20 > - First bad revision: 3ba655f6bc67660a2dcfc4c2a5b3d0d17714f53d > - Pushlog: > https://hg.mozilla.org/integration/fx-team/ > pushloghtml?fromchange=0c12d4229be0068eb1b9beb5064b800f12143f20&tochange=3ba6 > 55f6bc67660a2dcfc4c2a5b3d0d17714f53d > - Regressed by 3ba655f6bc67 Shane Caraveo — Bug 1215694 move pocket to a > system addon > > [Additional notes]: > - This is reproducible on Fx47 only. Did you actually check this regression range? Because if this only reproduces on aurora, how did you get the fx-team range?

(In reply to :Gijs Kruitbosch from comment #1) > Did you actually check this regression range? Because if this only > reproduces on aurora, how did you get the fx-team range? I haven't yet double-checked the regression range, because I've considered this issue a lingering effect from what the patch pushed for Bug 1245074 should have also fixed. I'll take another look at this as soon as possible. I'm leaving the ni? in place as a reminder.

(In reply to Iulia Cristescu, QA [:IuliaC] from comment #2) > (In reply to :Gijs Kruitbosch from comment #1) > > Did you actually check this regression range? Because if this only > > reproduces on aurora, how did you get the fx-team range? > > I haven't yet double-checked the regression range, because I've considered > this issue a lingering effect from what the patch pushed for Bug 1245074 > should have also fixed. > > I'll take another look at this as soon as possible. I'm leaving the ni? in > place as a reminder. Bug 1245074 was related to how the pocket button was inserted. I'm fairly sure this bug is related to how the hello button is inserted, so I expect it to have a different explanation (namely: moving Loop to a system add-on). This is also why I moved it to Loop::Client already.

Both Loop and the webide button are using the CUI createwidget api, they will be appended at the end in the order they are called. Seems like webide is beating loop, but the order is the other way around in navbarPlacements (CustomizableUI.jsm), thus reset defaults is enabled.

This needs to be fixed by Loop. Loop is still in the set of defaults in CustomizableUI.jsm, and they should make sure to insert in the right place.

(In reply to :Gijs Kruitbosch from comment #5) > This needs to be fixed by Loop. Loop is still in the set of defaults in > CustomizableUI.jsm, and they should make sure to insert in the right place. Given Shane's comments, I'm not convinced this is possible at the moment. For instance, we could move our default to after the webide button, but isn't that then going to affect all existing users? (or at least put everyone in a non-default configuration). Even if we can do that acceptably, can we always guarantee that Loop is always installed / started before or after Pocket? What happens when we move to restartless system add-ons? If I understand right, we need the core bug 1023319 fixing first, then we might be able to fix this.

(In reply to Mark Banner (:standard8) from comment #6) > For instance, we could move our default to after the webide button, but > isn't that then going to affect all existing users? (or at least put > everyone in a non-default configuration). > > Even if we can do that acceptably, can we always guarantee that Loop is > always installed / started before or after Pocket? What happens when we move > to restartless system add-ons? > > If I understand right, we need the core bug 1023319 fixing first, then we > might be able to fix this. None of the above. Pocket has issues here because their default is not in CustomizableUI's list at all. You're having issues because both you and webide have defaults in CustomizableUI but at least one and maybe both of you are inserting into the toolbar manually. Whoever is doing that (and that might be both of you) needs to ensure that the way they do it ensures that they match up with the placements of items in the nav-bar, which by default is controlled by the items which are in the default placement list, in CUI.jsm . If the result doesn't match that list, that means one or both of you is inserting yourself without regard for the position the button takes in the navbar. Until the default placement information for either of these buttons leaves CustomizableUI.jsm, this doesn't need bug 1023319 to be fixed.

(In reply to :Gijs Kruitbosch from comment #7) > (In reply to Mark Banner (:standard8) from comment #6) > > For instance, we could move our default to after the webide button, but > > isn't that then going to affect all existing users? (or at least put > > everyone in a non-default configuration). > > > > Even if we can do that acceptably, can we always guarantee that Loop is > > always installed / started before or after Pocket? What happens when we move > > to restartless system add-ons? > > > > If I understand right, we need the core bug 1023319 fixing first, then we > > might be able to fix this. > > None of the above. Pocket has issues here because their default is not in > CustomizableUI's list at all. > > You're having issues because both you and webide have defaults in > CustomizableUI but at least one and maybe both of you are inserting into the > toolbar manually. Neither are really doing anything wrong or manual here, both are using CUI.createWidget API which will always append to the end of the toolbar, there is no control for position here, even with the entries in the list. Switching the order around in the list would probably fix it for new profiles the vast majority of the time, but there is no way to guarantee order in this situation.

(In reply to Shane Caraveo (:mixedpuppy) from comment #8) > Neither are really doing anything wrong or manual here https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/devtools-browser.js#339

(In reply to Shane Caraveo (:mixedpuppy) from comment #8) > CUI.createWidget API which will always append to the end of the toolbar, I am fairly sure that this is not the case and that we have tests that verify this is not the case. If placements actually look like: ["a", "b", "c"] and "a" and "c" exist, and you createWidget({id: "b", ...}), either nothing gets inserted or (and this is what I believe is the case), "b" gets inserted in the correct place. If that isn't the case we should fix it, but it would be surprising - we would have run into this a long time ago. Trivial test you can run in the browser console: CustomizableUI.addWidgetToArea("some-funky-test-button", "nav-bar", 3) CustomizableUI.getWidgetIdsInArea("nav-bar"); CustomizableUI.createWidget({id: "some-funky-test-button", label: "Hi"}); CustomizableUI.getWidgetIdsInArea("nav-bar"); and you'll see space somewhere in the middle of the nav-bar, and you'll see the two getWidgetIdsInArea calls have the same return values.

@Gijs Apologies, you're right. I tested slightly different, by adding "some-funky-test-button" to the navbarPlacents list, then called (via browser console) CustomizableUI.createWidget({id: "some-funky-test-button", label: "Hi", defaultArea: CustomizableUI.AREA_NAVBAR}); It does properly place the button and it is set properly for the defaults (ie. restore defaults button is correct). I am however now confused, I even commented specifically about createWidget appending in the patch on bug 1245074 here: https://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizableUI.jsm#252

(In reply to Shane Caraveo (:mixedpuppy) from comment #11) > @Gijs Apologies, you're right. I tested slightly different, by adding > "some-funky-test-button" to the navbarPlacents list, then called (via > browser console) > > CustomizableUI.createWidget({id: "some-funky-test-button", label: "Hi", > defaultArea: CustomizableUI.AREA_NAVBAR}); > > It does properly place the button and it is set properly for the defaults > (ie. restore defaults button is correct). > > I am however now confused, I even commented specifically about createWidget > appending in the patch on bug 1245074 here: > https://mxr.mozilla.org/mozilla-central/source/browser/components/ > customizableui/CustomizableUI.jsm#252 I'm not sure I understand precisely what you are confused about, but hopefully this helps. I'm CC'ing :ochameau because he's working on some of the devtools thingies. If we wanted to, we could change where in the navbarPlacements we put pocket-button, and it should (I haven't tested it) Just Work, assuming pocket does not at some point call addWidgetToArea and moves the button itself (which I think it used to do in some cases? I don't remember if we got rid of that, we went through a lot of iterations...). Before we added the pocket item back to CustomizableUI.jsm, calling createWidget with defaultArea set would append to that area, because no position for the button would have been set. There isn't currently a way to define such a position in the widget spec consumers pass to CustomizableUI - you can only specify the default area. That wasn't really a problem because most add-ons append their buttons to the end of the nav-bar anyway. You could move it "manually" (ie using the CUI APIs) to some other position, of course, but you couldn't change the default. Either way, for non-builtin buttons, we'd remove the buttons from whichever area (and whatever position) they were placed in when the user uses "restore defaults", so the lack of API for add-ons is minimal in this sense. They just have to do slightly more manual work, in a sense. The final piece of the puzzle here is that default areas are only 1 part of the problem. CUI knows the defaults for any area, like the navbar, and the current set of items that are in there in the Firefox profile that it's running, if that's different. At the point where you add a new (external, not builtin) item, all it can really do is append (see previous paragraph), in part because if the area has already been customized (ie the placements array is no longer the default one), there's no telling where the "right" place for the widget is. Appending is what add-ons used to do in this case, so we do the same. When CUI was written the assumption was that all browser-builtin widgets would always and forever live in CustomizableWidgets.jsm or be a XUL thing in browser.xul, and that the default placements would live in CustomizableUI.jsm. Besides hardcoding it this way, there isn't really a way to ensure add-ons don't change the default placements, because of XUL/XPCOM-based add-ons being able to do pretty much anything. Some people within Mozilla who shall remain nameless were very insistent that add-ons shouldn't be able to change the default placements, and that 'restore defaults' should always mean "get rid of all the add-on buttons". So that got implemented. Things in CUI.jsm / CustomizableWidgets.jsm are "special", and everyone else is SOL. Devedition already kind of messed with that because now the defaults are different depending on which release version you're running. System add-ons then break with those assumptions completely. That's where we are now. The stuff from bug 1023319 will help for system add-ons. It won't really help for devtools and devedition, because they're not system add-ons. I think CUI probably exposes enough API surface for them to be able to get it right if they keep the default placement stuff in CUI.jsm (x-ref discussion in bug 1261920); if not we can discuss making changes, but I'm not sure off-hand what else would be necessary.

I've been using CustomizableUI just because it was already using that. Otherwise I would have tried WebExtension's.browserAction. (ok devtools are not an addon, but that doesn't mean we can't use WebExtension APIs!) Let me try to think about tomorrow. Stop using XUL and chrome. What about using WebExtensions and browserAction instead. Asking for being a default could be an attribute in the addon manifest. The position could also be requested in there. Then it would be much more easy to think that bug 1023319 is going to be able to assert that only official addon will do so. No cryptic JS parsing. Just a JSON assertion. As simple as that. Now get back to the real world. 1/ How WebExtension could be the only one to be able to do that? It could have a private key, a simple JS object that it hands over CUI.jsm and CUI.jsm would expect to receive this key to allow default/position parameters. This trick comes from the AddonSDK. It works well with WeakMaps: let secrets = new WeakMap(); let key = {}; secrets.set(key, privateData); // Here is a possible sketch. it introduces weird dependency, but it should work? CUI.setWidgetDefault(id, area, position, key) { if (!WebExt.validateKey(key)) throw new Error("Bad key"); ... } 2/ Wait. Loop, Pocket. Devtools. They aren't WebExtensions. Any old school bootstrapless addon can also be a WebExtension one. I've been starting to convert Loop to WebExtension, making it use browserAction for ex. It is as easy as that: Components.utils.import("resource://gre/modules/Extension.jsm"); var extension; function startup(data) { extension = new Extension(data); extension.startup(); // Do whatever you want as in regular bootstrap.js } function shutdown(data, reason) { extension.shutdown(); } And just put a manifest.json file in your addon to declare all WebExtension stuff. But note that. I don't see why we put so much effort in fighting XUL/chrome addons, they can just add toolbarbutton anywhere, anytime. I see much more value in making WebExtension great rather than spending much time to limit and control chrome and xul things.

(In reply to Alexandre Poirot [:ochameau] from comment #13) Bug 1023319 already defines a solution similar to the one you describe using the add-on manager that works for any add-on. > 2/ Wait. Loop, Pocket. Devtools. They aren't WebExtensions. > > Any old school bootstrapless addon can also be a WebExtension one. The problem is that devtools isn't an add-on at all, and nor is devedition.

(In reply to :Gijs Kruitbosch from comment #14) > (In reply to Alexandre Poirot [:ochameau] from comment #13) > > Bug 1023319 already defines a solution similar to the one you describe using > the add-on manager that works for any add-on. You have to trust the JS analyser to detect any fraud. Will it be done for real? Will it report another set of false alarm again to the addon developers?? Parsing json manifests sounds much more realistic. > > > 2/ Wait. Loop, Pocket. Devtools. They aren't WebExtensions. > > > > Any old school bootstrapless addon can also be a WebExtension one. > > The problem is that devtools isn't an add-on at all, and nor is devedition. Devtools could possibly be an addon. We are already using an addon to make them reloadable. This is not a goal. But if that helps in anyway...

(In reply to Alexandre Poirot [:ochameau] from comment #15) > (In reply to :Gijs Kruitbosch from comment #14) > > (In reply to Alexandre Poirot [:ochameau] from comment #13) > > > > Bug 1023319 already defines a solution similar to the one you describe using > > the add-on manager that works for any add-on. > > You have to trust the JS analyser to detect any fraud. Umm, no? The system works the same way as what you describe, but it's the add-on manager (rather than the webextensions implementation) that passes a unique key to each add-on in bootstrap.js . You can then check with the add-on manager if the add-on in question is a system add-on and the key is correct. > Will it be done for real? Will it report another set of false alarm again to > the addon developers?? I don't understand these questions.

Here's the updated regression range for this issue: * Last good revision: 0c12d4229be0068eb1b9beb5064b800f12143f20 * First bad revision: 3ba655f6bc67660a2dcfc4c2a5b3d0d17714f53d * Pushlog: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=0c12d4229be0068eb1b9beb5064b800f12143f20&tochange=3ba655f6bc67660a2dcfc4c2a5b3d0d17714f53d * Looks like the following bug has the changes which introduced the regression: https://bugzilla.mozilla.org/show_bug.cgi?id=1215694

Because this is the result of multiple system add-ons and we think needs customizable UI changes, we believe this needs to be fixed in core Firefox. Or, at least we are not able to prioritize this or move it forward in Hello, so it would just linger at P4 if it's in Hello's component.

This bug seems to have taken a meandering path with lots of tangents. Comment 7 is the core issue, and it's simple enough to verify in a browser console: >> CustomizableUI.getWidgetIdsInArea("nav-bar") Array [ "urlbar-container", "search-container", "developer-button", "bookmarks-menu-button", "downloads-button", "home-button", "webide-button", "loop-button", "pocket-button" ] >> xxx = Cu.import("resource:///modules/CustomizableUI.jsm", {}); >> xxx.gAreas.get("nav-bar").get("defaultPlacements") Array [ "urlbar-container", "search-container", "developer-button", "bookmarks-menu-button", "downloads-button", "home-button", "loop-button", "webide-button", "pocket-button" ] Note that the actual (live) set ends with webide,loop,pocket. The default set ends with loop,webide,pocket. They're different, and so the "Restore Defaults" button is enabled. (And, indeed, entering customize mode to swap loop and webide will disable the button. The fix here is either to change the expected order of the buttons (defaultPlacement), or to change the actual order of the buttons (to match defaultPlacement). This should be a trivial change to CustomizableUI or the system-addon/devtools code. https://dxr.mozilla.org/mozilla-central/rev/e5a10bc7dac4ee2453d8319165c1f6578203eac7/browser/components/customizableui/CustomizableUI.jsm#233 seems to handle setting the expected default placements. Ian, can you take care of sorting out what which placement needs changed and make it so?

(In reply to Justin Dolske [:Dolske] from comment #19) > Note that the actual (live) set ends with webide,loop,pocket. The default > set ends with loop,webide,pocket. They're different, and so the "Restore > Defaults" button is enabled. (And, indeed, entering customize mode to swap > loop and webide will disable the button. > > > The fix here is either to change the expected order of the buttons > (defaultPlacement), or to change the actual order of the buttons (to match > defaultPlacement). This should be a trivial change to CustomizableUI or the > system-addon/devtools code. I'm not convinced this is the full fix. It'd likely help for the majority of the time, but when we make system add-ons restartless, and they can remove and re-insert the icons on upgrade, I suspect this would likely break again. We're also likely to be affected if anything causes a change in the add-on startup order, again getting us back to where we started.

For 48 we can make the change suggested in Comment 19, but it's just codifying the side effect of undefined behavior, we should expect that it will break in the future. Dan or Mark: could you make that change?

(In reply to Mark Banner (:standard8) from comment #20) > I'm not convinced this is the full fix. Why not? > It'd likely help for the majority of > the time, but when we make system add-ons restartless, and they can remove > and re-insert the icons on upgrade, I suspect this would likely break again. > > We're also likely to be affected if anything causes a change in the add-on > startup order, again getting us back to where we started. I really don't understand this. The placements are defined. Any consumer, at whatever point in time it inserts itself, can ask for them from CUI and respond accordingly. If the placements change as a result of a call by a consumer calling addWidgetToArea or similar methods, that's because they don't read the placements and just insert themselves willy-nilly (usually at the very end of the toolbar). There is no "undefined" behaviour involved, and the startup order of the consumers should not make any difference if consumers are responsible. There is no way for CUI to be responsible "for them", because it has no way of knowing whether the consumer actually *wants* to move the button with respect to the current set of placements or not.

I tried this scenario and while this is a visual glitch which we should be fixing in 48/49, I would not consider this a release blocking issue. Ian, Mark and Overholt agree with this assessment. It's a wontfix for Fx47.

Bug 1287827 has removed Hello from Firefox, I tested the restore default after removal and it seemed to work fine, hence closing this bug off.