Closed Bug 1261920 Opened 5 years ago Closed 5 years ago

Key element with id 'key_devToolboxMenuItem' for widget 'developer-button' not found!

Categories

(DevTools :: Framework, defect, P1)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: jryans, Assigned: ochameau)

Details

(Whiteboard: [btpp-fix-now])

Attachments

(1 file, 3 obsolete files)

When I start Firefox with my usual dev profile, which has the Developer menu moved into the main toolbar, I see the following in my terminal:

console.error: CustomizableUI:
  Key element with id 'key_devToolboxMenuItem' for widget 'developer-button' not found!

:ochameau, this seems related to your recent work.
Flags: needinfo?(poirot.alex)
This is the CustomizableWidget, still hardcoded in /browser/ which gets registered before the key has a chance to be registered.
Flags: needinfo?(poirot.alex)
Attached patch patch v1 (obsolete) — Splinter Review
This move the developer button creation to /devtools/.
It is similar to WebIDE button.
There is still various reference in /browser/,
I may have to use CustomizableUI.moveWidgetWithinArea to dynamically set
the position of the widget.
Assignee: nobody → poirot.alex
Attached patch patch v2 (obsolete) — Splinter Review
Removed hardcoded developer-button from CustomizableUI.jsm
and instead use CustomizableUI.moveWidgetWithinArea.
I imagine we could do the same for WebIDE widget.
There is still a bunch of developer-button references in tests.
I imagine as soon as tests keep green, we are good?

Also in CSS and localization, but I'm not sure it is worth moving?

And some in xul, that I think are uncessary. I'll try to remove them.
Note that the regression seems to be light. We can't display the shortcut in the tooltip of the developer button. And we get this exception printed.
Component: Developer Tools → Developer Tools: Framework
Priority: -- → P1
Whiteboard: [btpp-fix-now]
Attached patch patch v3 (obsolete) — Splinter Review
Also removed reference from browser.xul which seems useless
as we add developer-button to the customize panel dynamically.

I verified this patch against nightly, and nightly build as dev-edition,
to ensure that the developer button appears in the navbar at the right position
in dev edition and in the menu at the right position in nightly.

Now there is still various references in tests, but I looks like CustomizeUI tests
rather than devtools ones so I've let them in /browser/

https://treeherder.mozilla.org/#/jobs?repo=try&revision=83fcf6885a1e
Attachment #8738584 - Flags: review?(jryans)
Attachment #8737960 - Attachment is obsolete: true
Attachment #8738094 - Attachment is obsolete: true
Comment on attachment 8738584 [details] [diff] [review]
patch v3

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

This works well for me, and the DevTools changes make sense to me.

However, I don't know the CustomizableUI code well at all, so let's get another from someone on the browser team.

::: devtools/client/framework/devtools-browser.js
@@ +286,5 @@
>  
>    /**
> +   * Install Developer widget
> +   */
> +  installDeveloperWidget: function () {

Nit: function()
Attachment #8738584 - Flags: review?(jryans)
Attachment #8738584 - Flags: review?(gijskruitbosch+bugs)
Attachment #8738584 - Flags: review+
Comment on attachment 8738584 [details] [diff] [review]
patch v3

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

r=me with the somewhat substantial adjustments I'm suggesting, assuming they work and automated tests pass without other changes. If not, please ask for review again.

::: browser/base/content/browser.xul
@@ -663,2 @@
>               defaultset="urlbar-container,search-container,bookmarks-menu-button,downloads-button,home-button,loop-button"
> -#endif

You should keep this in sync with CustomizableUI.jsm, so if you're keeping that, keep this too.

::: browser/components/customizableui/CustomizableUI.jsm
@@ -242,5 @@
>      ];
>  
> -    if (AppConstants.MOZ_DEV_EDITION) {
> -      navbarPlacements.splice(2, 0, "developer-button");
> -    }

You should probably keep these to avoid restore defaults kicking the button out of either of the places.

::: devtools/client/framework/devtools-browser.js
@@ +350,5 @@
> +      let btn = widgets.indexOf("add-ons-button");
> +      if (btn > -1) {
> +        CustomizableUI.moveWidgetWithinArea(id, btn + 1);
> +      }
> +    }

If you keep the default position in CustomizableUI, I think you don't need any of this manual insertion code. If you do need to keep it for some reason (please explain!) then this is wrong and I need to review it again. It won't obey user customizations the way it's currently been written, and if you remove the search container or add-ons button from their respective places, it won't insert your button. Both of those obviously need addressing.
Attachment #8738584 - Flags: review?(gijskruitbosch+bugs) → review+
Please ensure that trypushes run mochitest-bc and mochitest-e10s-bc because otherwise you won't notice CustomizableUI test failures until you land. Run ./mach mochitest browser/components/customizableui to check them locally.
(In reply to :Gijs Kruitbosch from comment #10)
> Please ensure that trypushes run mochitest-bc and mochitest-e10s-bc because
> otherwise you won't notice CustomizableUI test failures until you land. Run
> ./mach mochitest browser/components/customizableui to check them locally.

Oh right... many tests are broken, I was surprised to see them green!

(In reply to :Gijs Kruitbosch from comment #9)
> You should probably keep these to avoid restore defaults kicking the button
> out of either of the places.
> 
> ::: devtools/client/framework/devtools-browser.js
> @@ +350,5 @@
> > +      let btn = widgets.indexOf("add-ons-button");
> > +      if (btn > -1) {
> > +        CustomizableUI.moveWidgetWithinArea(id, btn + 1);
> > +      }
> > +    }
> 
> If you keep the default position in CustomizableUI, I think you don't need
> any of this manual insertion code. If you do need to keep it for some reason
> (please explain!) then this is wrong and I need to review it again. It won't
> obey user customizations the way it's currently been written, and if you
> remove the search container or add-ons button from their respective places,
> it won't insert your button. Both of those obviously need addressing.

I would really appreciate to remove all devtools junks out of /browser/.
So if we could keep removing all this, it would be great.
I'm trying to do what pocket does. It looks like I can save user placement easily.
By simply checking for "developer-button" existance in the `widgets` array.
Then I can accomodate my code when relatives elements aren't there, but it should be rare edge cases.
(In reply to Alexandre Poirot [:ochameau] from comment #11)
> (In reply to :Gijs Kruitbosch from comment #10)
> > Please ensure that trypushes run mochitest-bc and mochitest-e10s-bc because
> > otherwise you won't notice CustomizableUI test failures until you land. Run
> > ./mach mochitest browser/components/customizableui to check them locally.
> 
> Oh right... many tests are broken, I was surprised to see them green!
> 
> (In reply to :Gijs Kruitbosch from comment #9)
> > You should probably keep these to avoid restore defaults kicking the button
> > out of either of the places.
> > 
> > ::: devtools/client/framework/devtools-browser.js
> > @@ +350,5 @@
> > > +      let btn = widgets.indexOf("add-ons-button");
> > > +      if (btn > -1) {
> > > +        CustomizableUI.moveWidgetWithinArea(id, btn + 1);
> > > +      }
> > > +    }
> > 
> > If you keep the default position in CustomizableUI, I think you don't need
> > any of this manual insertion code. If you do need to keep it for some reason
> > (please explain!) then this is wrong and I need to review it again. It won't
> > obey user customizations the way it's currently been written, and if you
> > remove the search container or add-ons button from their respective places,
> > it won't insert your button. Both of those obviously need addressing.
> 
> I would really appreciate to remove all devtools junks out of /browser/.
> So if we could keep removing all this, it would be great.
> I'm trying to do what pocket does. It looks like I can save user placement
> easily.

I think you underestimate how hard this is to get right. See e.g. bug 1262447, bug 1167096, bug 1245074, ...

> By simply checking for "developer-button" existance in the `widgets` array.
> Then I can accomodate my code when relatives elements aren't there, but it
> should be rare edge cases.

People can and do customize the toolbar...
FWIW, pocket is still having issues because the placement got removed from CustomizableUI.jsm. Anything not in there will be removed if the user uses "restore defaults", which isn't expected for builtin buttons. There are some ideas on how to solve this for system add-ons, but devtools isn't a system add-on and so at this stage I have no idea how to fix that.
https://hg.mozilla.org/releases/mozilla-beta/rev/78c4ca9fd5a2
OMG... this is so sad.

Yes I obviously underestimate how bad CustomizateUI is regarding addons.
I imagine support for non-mozilla addons is even worse, I thought it was good enough for simple things.

Ok, ok I'll just keep these things in /browser/.
Hopefully it will improve over time and then we can rip this out.
(In reply to Alexandre Poirot [:ochameau] from comment #14)
> Yes I obviously underestimate how bad CustomizateUI is regarding addons.
> I imagine support for non-mozilla addons is even worse, I thought it was
> good enough for simple things.

No, the problem is it works really well for external code, but nobody thought that "builtin" Firefox things would ever live somewhere else. Good luck coming up with a better idea if you want separate behaviour for add-ons and "built in" widgets, and given XUL add-ons that live in the same namespace.
Attached patch patch v4Splinter Review
Patch that passed this green try.
Revert the requested changes from comment 9.
That makes tests happy again.

I'll followup if CUI address pocket/loop integration.
Attachment #8740413 - Flags: review+
Attachment #8738584 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/5abc57693914
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.