Closed Bug 1248603 Opened 4 years ago Closed 4 years ago

Add top level menuitems dynamically

Categories

(DevTools :: General, defect, P3)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: ochameau, Assigned: ochameau, NeedInfo)

References

(Depends on 1 open bug)

Details

(Keywords: addon-compat, Whiteboard: [btpp-backlog])

Attachments

(5 files, 26 obsolete files)

3.68 KB, patch
jryans
: review+
Details | Diff | Splinter Review
20.77 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
4.43 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
846 bytes, patch
flod
: review+
Details | Diff | Splinter Review
53.78 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
See bug 1248348.

Today, we hardcode <xul:menuitem>, <xul:broadcaster>, <xul:key> and <xul:command> for all top level items in /browser/:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-menubar.inc#522

Whereas we do all that dynamically for all panel-related menuitems and shortcuts (like WebConsole, Debugger, Inspector, ...)

We can cleanup /browser/ from devtools specific by also doing that dynamically.
It would allow to work on menuitems and key shortcuts via the hot reload addon.
Attached patch wip (obsolete) — Splinter Review
This is the most challenging patch for dynamic registration (set of patch blocking bug 1248348).

I would like to verify any DOM difference before/after my patch
as various theme addons are using overlays against these xul elements.
Blocks: 1250832
Attached patch patch v1 (obsolete) — Splinter Review
Preliminary patch that is going to help.
Attachment #8720006 - Attachment is obsolete: true
Attachment #8722929 - Attachment is obsolete: true
Attached patch patch v2 (obsolete) — Splinter Review
The main part of the patch.
I still need to verify xul actual content before/after these patches.
Ok. So these patches should be good to review if try is green.

This first patch moves all menuitem/key/broadcaster/command things to a dedicated module.
It helps reducing the side of gDevToolsBrowser/devtools-browser.js
and may help sharing some code between per-tool and top-level items.
Attachment #8723604 - Attachment is obsolete: true
Attached patch patch v3 (obsolete) — Splinter Review
Main patch. I verified it. I compared <command>, <broadcaster>, <key> and <menuitems>
before and after this patch.
Here is the only differences left:
* <xul:command> do not have any inlined JS in "oncommand".
  Instead they all have oncommand=";", a xul workaround to ensure receiving
  a "command" DOM Event.
* the <command> "Tools:DevToolbarFocus" no longer exists, it has been merged into "Tools:DevToolbar"
  but we still support two distinct behavior between toggle the toolbar from menus and the shortcut.
  (The shortcut will not toggle the toolbar if the toolbar isn't focus, instead it will focus it.
   If you press the shortcut again, it will close the toolbar)
* the xul:key DevToolbar now maps to Tools:DevToolbar command
* <xul:menuitem> for "Connect..." has an empty accesskey="" (see next patch)
* in all these xul elements the webconsole item is now after the inspector (depends on 1247203)
  we now order correctly the tools.
* Note that PageSource is special. We only add its menuitem dynamically, everything else is still
  hardcoded in browser/

Otherwise, speaking about the patch itself:
* I had to move the localization from dtd to properties.
  Hopefully that won't be an issue?
* there is now a special file, a bit like definitions.js:
  devtools/client/menus.js
  which defines the menus and shortcuts.
Attachment #8723605 - Attachment is obsolete: true
The dtd had an accesskey for "Connect..." but it wasn't used in the XUL element.
I remove it since it conflicts with Browser Toolbox one.
The change made to DevToolbar/DevToolbarFocus forces us to tweak toolbar tests.
We need to toggle the toolbar via the menuitem instead of command in order
to see it being toggled immediately, otherwise it is just focused.
Flags: needinfo?(philip.chee)
Depends on: 1247203
Rebased patches.

These patches depends on bug 1247203, which also depends on bug 1233463.
If you want to actually apply these patches, here is a branch where you will see these patches and many others,
but you should easily be able to cherry-pick all the required patches.
  https://github.com/ochameau/mozilla-central/commits/wip
And a try run:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=e32e03e8d665

Given how how my patches are interleaved I'm just f? here, but do not hesitate to r+ patches you consider somewhat ready to proceed ;)
I'm off all next week, so no rush. I would just let you a change to look at these patches
before you go to your workweek and become busy.
Attachment #8726944 - Flags: feedback?(jryans)
Do not forget to look at bug comments, I explained various things/tests I made.
Attachment #8726946 - Flags: feedback?(jryans)
Attachment #8724771 - Attachment is obsolete: true
Attachment #8726947 - Flags: feedback?(jryans)
Attachment #8724773 - Attachment is obsolete: true
Attachment #8726948 - Flags: feedback?(jryans)
Attachment #8725247 - Attachment is obsolete: true
Filter on TEAPOT-SPLINES.
Priority: -- → P3
Whiteboard: [btpp-backlog]
Comment on attachment 8726944 [details] [diff] [review]
Factor out menu and shortcut creation to dedicated module - v3

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

::: devtools/client/framework/browser-menus.js
@@ +104,5 @@
> +    menuitem: menuitem
> +  };
> +}
> +
> +function insertToolMenuElements(doc, toolDefinition, prevDef) {

Add some comments to explain what these locations are that the elements are being added to.
Attachment #8726944 - Flags: feedback?(jryans) → feedback+
Comment on attachment 8726946 [details] [diff] [review]
Register devtools menuitems dynamically.

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

::: browser/base/content/browser-menubar.inc
@@ -547,5 @@
> -                            accesskey="&eyedropper.accesskey;"/>
> -                  <menuitem id="menu_scratchpad"
> -                            observes="devtoolsMenuBroadcaster_Scratchpad"
> -                            accesskey="&scratchpad.accesskey;"/>
> -                  <menuitem id="menu_pageSource"

Can we live this item in place statically?  View Source does not depend on code in /devtools, so it seems strange for our code to manage it.

::: devtools/client/framework/browser-menus.js
@@ +42,5 @@
> + *          If true, consider the shortcut as a characther one,
> + *          otherwise a non-character one like F12.
> + *
> + * @return XULKeyElement
> + **/

Nit: I think the typical doc comment style is two stars to open, one star for remaining lines:

/**
 * 
 */

@@ +293,5 @@
> + *
> + * @param {XULDocument} doc
> + *        The document to which keys and menus are to be added.
> + **/
> +function addToplevelItems(doc) {

Nit: TopLevel

::: devtools/client/framework/devtools-browser.js
@@ +342,3 @@
>     */
>    _registerBrowserWindow: function(win) {
> +    if (gDevToolsBrowser._trackedBrowserWindows.has(win))

Nit: braces
Attachment #8726946 - Flags: feedback?(jryans) → feedback+
Comment on attachment 8726947 [details] [diff] [review]
Removed unused connect access key.

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

::: devtools/client/locales/en-US/menus.properties
@@ +5,5 @@
>  devToolsCmd.key = VK_F12
>  devToolsCmd.keytext = F12
>  
>  devtoolsConnect.label = Connect…
> +devtoolsConnect.accesskey =

Shouldn't the string be removed entirely?
Attachment #8726947 - Flags: feedback?(jryans) → feedback+
Attachment #8724767 - Attachment is obsolete: true
(In reply to J. Ryan Stinnett [:jryans] (use ni?) (at work week until Mar. 18) from comment #22)
> Comment on attachment 8726946 [details] [diff] [review]
> ::: browser/base/content/browser-menubar.inc
> > -                  <menuitem id="menu_pageSource"
> 
> Can we live this item in place statically?  View Source does not depend on
> code in /devtools, so it seems strange for our code to manage it.

It would make the whole menu insertion even more complex as this item is in middle of all other devtools items :/
It's in between scratchpad and javascript console.
So, I could do that, but is it really worth? Note that I only manage the menuitem. The key and command are still hardcoded in browser/.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) (at work week until Mar. 18) from comment #23)
> Comment on attachment 8726947 [details] [diff] [review]
> ::: devtools/client/locales/en-US/menus.properties
> @@ +5,5 @@
> >  devToolsCmd.key = VK_F12
> >  devToolsCmd.keytext = F12
> >  
> >  devtoolsConnect.label = Connect…
> > +devtoolsConnect.accesskey =
> 
> Shouldn't the string be removed entirely?

If I do, l10n/GetStringFromName functions are going to throw for a missing key. I would like to keep it that way so that localizers can easily re-add an accesskey if they want, and, above all, keep throwing in case we miss a localization key so that we know something is missing.
Do not hesitate to review these patches next week.
I addressed the previous feedback comments and replieds in previous comments about the one I didn't.
Attachment #8731699 - Flags: review?(jryans)
Attachment #8726944 - Attachment is obsolete: true
Attachment #8731700 - Flags: review?(jryans)
Attachment #8726946 - Attachment is obsolete: true
Attachment #8731701 - Flags: review?(jryans)
Attachment #8726947 - Attachment is obsolete: true
Attachment #8726948 - Attachment is obsolete: true
New patch in this serie.
It looks like with my discussion with philip that this appmenu thing if totally gone.
It used to be the big orange special system menu in windows and may be other OSs
that aggregated all the menus into one.
Attachment #8731703 - Flags: review?(jryans)
Also note that if you want to apply/test these patches, you may have to apply bug 1247203 first (all patches are already checked in except the last one)
Comment on attachment 8731699 [details] [diff] [review]
Factor out menu and shortcut creation to dedicated module - v4

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

::: devtools/client/framework/browser-menus.js
@@ +9,5 @@
> + *
> + * Menu and shortcut definitions are fetched from:
> + * - devtools/client/menus for top level entires
> + * - devtools/client/definitions for tool-specifics entries
> + **/

Nit: only one star

@@ +258,5 @@
> + * Add menus and shortcuts to a browser document
> + *
> + * @param {XULDocument} doc
> + *        The document to which keys and menus are to be added.
> + **/

Nit: only one star
Comment on attachment 8731700 [details] [diff] [review]
Register devtools menuitems dynamically - v2

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

I still feel like the view source menu item should be left in place statically no matter is going on with DevTools.

But, let's ask :Gijs!
Attachment #8731700 - Flags: review?(jryans) → feedback?(gijskruitbosch+bugs)
Comment on attachment 8731703 [details] [diff] [review]
Remove support of the appmenu from devtools - v1

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

This seems fine to me, but I don't really know the history here.  Let's check with :Gijs.
Attachment #8731703 - Flags: review?(jryans) → review?(gijskruitbosch+bugs)
Comment on attachment 8731700 [details] [diff] [review]
Register devtools menuitems dynamically - v2

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

I concur that we should keep page source available statically.

(In reply to Alexandre Poirot [:ochameau] from comment #24)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) (at work week until Mar.
> 18) from comment #22)
> > Comment on attachment 8726946 [details] [diff] [review]
> > ::: browser/base/content/browser-menubar.inc
> > > -                  <menuitem id="menu_pageSource"
> > 
> > Can we live this item in place statically?  View Source does not depend on
> > code in /devtools, so it seems strange for our code to manage it.
> 
> It would make the whole menu insertion even more complex as this item is in
> middle of all other devtools items :/
> It's in between scratchpad and javascript console.

There's nothing that says it should be there - stick it at the bottom, and put 'get more tools' and its separator after it?

If that's problematic, it used to live in the View menu, and I don't know why it even moved. I still look for it there every now and again. You could have a copy in the tools menu if that makes it easier.

> So, I could do that, but is it really worth? Note that I only manage the
> menuitem. The key and command are still hardcoded in browser/.

That would mean the shortcut wouldn't be discoverable and we're splitting the implementation and localization. Doesn't seem like a good idea to me...

Note that:

> It would allow to work on menuitems and key shortcuts via the hot reload
> addon.

didn't use to be true, at least - removing key registrations did not make them available for new keys, IME. It's possible this has changed since I last tried to do it, or that the problem is/was OS-specific.

If that's still the case, I'm not sure to what degree this is really useful. :-\

(In reply to Alexandre Poirot [:ochameau] from comment #13)
> Created attachment 8724773 [details] [diff] [review]
> Removed unused connect access key.
> 
> The dtd had an accesskey for "Connect..." but it wasn't used in the XUL
> element.
> I remove it since it conflicts with Browser Toolbox one.

If I open the menu in 46, and press "c", the connect window opens. Either way, you shouldn't leave the string empty if it's unused. In fact, shouldn't change strings at all without changing the string identifier...

(In reply to Alexandre Poirot [:ochameau] from comment #12)
> * I had to move the localization from dtd to properties.
>   Hopefully that won't be an issue?

It isn't per se, though we're creating work for localizers. It'd be good if we were sure we gained something for that work.


FWIW, one of the things that I'd like to do at some point is taking the frontend hookup code of devtools out of the way of ts_paint/tpaint. I don't know if this helps or not, because devtools has all its own extra-special module loading infrastructure that is completely opaque to me so I can't tell how the dependencies work now... but it would be good to keep in mind.

::: devtools/client/framework/browser-menus.js
@@ +28,5 @@
> + *        The document to which keys are to be added.
> + * @param {String} property
> + *        Prefix of the properties entry to look for key shortcut in
> + *        localization file. We will look for {property}.key and
> + *        {property}.keytext for non-character shortcuts like F12.

This docstring does not match the function definition.

"dtd" seems misleading considering it's not actually from a dtd file... maybe just call it stringId or something like this if you want to be more specific than "property"?

@@ +305,5 @@
> + * @param {XULDocument} doc
> + *        The document to which keys and menus are to be added.
> + */
> +function addTopLevelItems(doc) {
> +  let frags = {

None of the lines where you use this comes close to 80 characters, so just call the variable "fragments".

@@ +322,5 @@
> +    } else {
> +      let { id, observes, dtd } = item;
> +      let broadcasterId = "devtoolsMenuBroadcaster_" + observes;
> +
> +      // Create a <menuitem>

Seems like this could be a helper method as well, and could be reused by the other code in browser-menus.js that also creates these items.

@@ +335,5 @@
> +        menuitem.setAttribute("autocheck", "false");
> +      }
> +      frags.menuItems.appendChild(menuitem);
> +
> +      if (item.onlyMenuItem) {

This makes people defining menus make something explicit that the code should just detect. Can just do:

if (!doc.getElementById(broadcasterId)) {
  createBroadcaster(fragments, item, dtd);
}

which is much more obvious code - as it is, I'm wondering, why would one of the menus have an item with no command element or broadcaster, and which one is that, etc.

... but then, if we leave page source out of this, I guess this is all moot anyway.

@@ +339,5 @@
> +      if (item.onlyMenuItem) {
> +        continue;
> +      }
> +
> +      // Create a <broadcaster>

This too should be a helper, to avoid having such long methods which are hard to read.

@@ +353,5 @@
> +      if (typeof(item.command) == "function") {
> +        // Do not use intermediate <command> and call JS function
> +        broadcaster.oncommand = item.command;
> +        // For unknown reason, the menuitem isn't able to fire its related
> +        // broadcaster.

??

What were you trying to do and what is the problem here? If the broadcaster's oncommand attribute has no effect, why bother setting it on the broadcaster at all?

@@ +359,5 @@
> +      } else if (typeof(item.command) == "string") {
> +        // Refer to an existing <command>
> +        broadcaster.setAttribute("command", item.command);
> +      } else if (item.command) {
> +        // Create a <command>

And this should be a helper.

@@ +365,5 @@
> +        let c = doc.createElement("command");
> +        c.id = "Tools:" + command.id;
> +        broadcaster.setAttribute("command", c.id);
> +
> +        c.setAttribute("oncommand", ";"); // needed. See bug 371900

Rather than saying "needed", please actually explain what the issue is.

@@ +370,5 @@
> +        c.addEventListener("command", command.oncommand);
> +
> +        // For unknown reason, the menuitem isn't able to fire its related
> +        // command
> +        menuitem.addEventListener("command", command.oncommand);

It sounds like the command and oncommand attributes of the broadcaster are just not being propagated at all. Did you check? Did you try using a C++ level debugger to see what was going on?

The level of "for some reason this doesn't work so I'm hacking it" in this file is a bit disconcerting.

Looking briefly at the documentation for broadcasters, from purely the code in this patch, it seems like many (all?) of the cases that this code uses broadcasters for could just be handled by directly linking the menuitems to commands, see https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/Broadcasters_and_Observers .

::: devtools/client/menus.js
@@ +11,5 @@
> + */
> +
> +const {Cu} = require("chrome");
> +const Services = require("Services");
> +const isMac = Services.appinfo.OS.includes("Darwin");

== "Darwin" - no need for includes()

@@ +21,5 @@
> +    observes: "DevToolbox",
> +    dtd: "devToolboxMenuItem",
> +    command: {
> +      id: "DevToolbox",
> +      oncommand: function(event) {

oncommand(event) here and elsewhere.

@@ +87,5 @@
> +    disabled: true,
> +    command: {
> +      id: "BrowserToolbox",
> +      oncommand: function() {
> +        let { BrowserToolboxProcess } = Cu.import("resource://devtools/client/framework/ToolboxProcess.jsm", {});

Instead of all of these methods calling Cu.import on a new object every time, it seems like this module could just use XPCOMUtils.defineLazyModuleGetter and then the methods could just use the globals.
Attachment #8731700 - Flags: feedback?(gijskruitbosch+bugs) → feedback-
Comment on attachment 8731703 [details] [diff] [review]
Remove support of the appmenu from devtools - v1

Yeah, all of this is extremely dead, AIUI. Can just land this separately?
Attachment #8731703 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8731700 [details] [diff] [review]
Register devtools menuitems dynamically - v2

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

::: browser/base/content/browser-sets.inc
@@ -103,5 @@
> -    <command id="Tools:BrowserContentToolbox" oncommand="gDevToolsBrowser.openContentProcessToolbox();" disabled="true" hidden="true"/>
> -    <command id="Tools:BrowserConsole" oncommand="HUDService.openBrowserConsoleOrFocus();"/>
> -    <command id="Tools:Scratchpad" oncommand="Scratchpad.openScratchpad();"/>
> -    <command id="Tools:ResponsiveUI" oncommand="ResponsiveUI.toggle();"/>
> -    <command id="Tools:Eyedropper" oncommand="openEyedropper();"/>

Oh, and I almost forgot: this and other global things here should be removed as part of this patch if, like this one, there are no other callers.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #35)
> ::: devtools/client/framework/browser-menus.js
> > + **/
> 
> Nit: only one star

Oh I addressed these two, but mixed things up in the next patch. I'll fix that.
(In reply to :Gijs Kruitbosch from comment #38)
> Comment on attachment 8731700 [details] [diff] [review]
> > It would make the whole menu insertion even more complex as this item is in
> > middle of all other devtools items :/
> > It's in between scratchpad and javascript console.
> 
> There's nothing that says it should be there - stick it at the bottom, and
> put 'get more tools' and its separator after it?

Ok. Often we are told to not change anything regarding user facing behavior... so I tried hard to keep everything as-is.
I moved back the menuitem to /browser/. Note that I only moved the <menuitem>, I was already using key and broadcaster from /browser/.

> > It would allow to work on menuitems and key shortcuts via the hot reload
> > addon.
> 
> didn't use to be true, at least - removing key registrations did not make
> them available for new keys, IME. It's possible this has changed since I
> last tried to do it, or that the problem is/was OS-specific.

It works on windows, which is the main OS I'm targeting for the reload addon.

> (In reply to Alexandre Poirot [:ochameau] from comment #13)
> > Created attachment 8724773 [details] [diff] [review]
> > Removed unused connect access key.
> > 
> > The dtd had an accesskey for "Connect..." but it wasn't used in the XUL
> > element.
> > I remove it since it conflicts with Browser Toolbox one.
> 
> If I open the menu in 46, and press "c", the connect window opens. Either
> way, you shouldn't leave the string empty if it's unused. In fact, shouldn't
> change strings at all without changing the string identifier...

Ok. I'll update the patch to change its accesskey to "c", but note it also conflicts with the "Error console". But it's not as terrible as conflicting with the Browser toolbox, which is much more common.

> (In reply to Alexandre Poirot [:ochameau] from comment #12)
> > * I had to move the localization from dtd to properties.
> >   Hopefully that won't be an issue?
> 
> It isn't per se, though we're creating work for localizers. It'd be good if
> we were sure we gained something for that work.

Removing all devtools junks out of /browser/ is, for me, already a great win!!
Being able to reload devtools is a second win.
If that's really terrible for localizers, is there anything I can do to help?
Better/more inline docs? Some flags on this bug? A message somewhere?

> FWIW, one of the things that I'd like to do at some point is taking the
> frontend hookup code of devtools out of the way of ts_paint/tpaint. I don't
> know if this helps or not, because devtools has all its own extra-special
> module loading infrastructure that is completely opaque to me so I can't
> tell how the dependencies work now... but it would be good to keep in mind.

You should talk to us about such things. We can surely tweak things. I have a clear picture of devtools startup these days... We recently change devtools tartup in bug 1248348.
It is no longer loaded arbitrarily in middle of gBrowserInit._delayedStartup.
Instead it is loaded on browser-delayed-startup-finished, which is slightly after.
Is it still happening before ts_paint/tpaint? If yes, How can I easily see that without running talos, like just running firefox?

> ::: devtools/client/framework/browser-menus.js
> @@ +28,5 @@
> > + *        The document to which keys are to be added.
 > > + * @param {String} property
> > + *        Prefix of the properties entry to look for key shortcut in
> > + *        localization file. We will look for {property}.key and
> > + *        {property}.keytext for non-character shortcuts like F12.
> 
> This docstring does not match the function definition.
> 
> "dtd" seems misleading considering it's not actually from a dtd file...
> maybe just call it stringId or something like this if you want to be more
> specific than "property"?
> 
> @@ +305,5 @@
> > + * @param {XULDocument} doc
> > + *        The document to which keys and menus are to be added.
> > + */
> > +function addTopLevelItems(doc) {
> > +  let frags = {
> 
> None of the lines where you use this comes close to 80 characters, so just
> call the variable "fragments".
> 
> @@ +322,5 @@
> > +    } else {
> > +      let { id, observes, dtd } = item;
> > +      let broadcasterId = "devtoolsMenuBroadcaster_" + observes;
> > +
> > +      // Create a <menuitem>
> 
> Seems like this could be a helper method as well, and could be reused by the
> other code in browser-menus.js that also creates these items.
> 
> @@ +335,5 @@
> > +        menuitem.setAttribute("autocheck", "false");
> > +      }
> > +      frags.menuItems.appendChild(menuitem);
> > +
> > +      if (item.onlyMenuItem) {
> 
> This makes people defining menus make something explicit that the code
> should just detect. Can just do:
> 
> if (!doc.getElementById(broadcasterId)) {
>   createBroadcaster(fragments, item, dtd);
> }
> 
> which is much more obvious code - as it is, I'm wondering, why would one of
> the menus have an item with no command element or broadcaster, and which one
> is that, etc.
> 
> ... but then, if we leave page source out of this, I guess this is all moot
> anyway.
> 
> @@ +339,5 @@
> > +      if (item.onlyMenuItem) {
> > +        continue;
> > +      }
> > +
> > +      // Create a <broadcaster>
> 
> This too should be a helper, to avoid having such long methods which are
> hard to read.
> 
> @@ +353,5 @@
> > +      if (typeof(item.command) == "function") {
> > +        // Do not use intermediate <command> and call JS function
> > +        broadcaster.oncommand = item.command;
> > +        // For unknown reason, the menuitem isn't able to fire its related
> > +        // broadcaster.
> 
> ??
> 
> What were you trying to do and what is the problem here? If the
> broadcaster's oncommand attribute has no effect, why bother setting it on
> the broadcaster at all?
> 
> @@ +359,5 @@
> > +      } else if (typeof(item.command) == "string") {
> > +        // Refer to an existing <command>
> > +        broadcaster.setAttribute("command", item.command);
> > +      } else if (item.command) {
> > +        // Create a <command>
> 
> And this should be a helper.
> 
> @@ +365,5 @@
> > +        let c = doc.createElement("command");
> > +        c.id = "Tools:" + command.id;
> > +        broadcaster.setAttribute("command", c.id);
> > +
> > +        c.setAttribute("oncommand", ";"); // needed. See bug 371900
> 
> Rather than saying "needed", please actually explain what the issue is.
> 
> @@ +370,5 @@
> > +        c.addEventListener("command", command.oncommand);
> > +
> > +        // For unknown reason, the menuitem isn't able to fire its related
> > +        // command
> > +        menuitem.addEventListener("command", command.oncommand);
> 
> It sounds like the command and oncommand attributes of the broadcaster are
> just not being propagated at all. Did you check? Did you try using a C++
> level debugger to see what was going on?
> 
> The level of "for some reason this doesn't work so I'm hacking it" in this
> file is a bit disconcerting.
> 
> Looking briefly at the documentation for broadcasters, from purely the code
> in this patch, it seems like many (all?) of the cases that this code uses
> broadcasters for could just be handled by directly linking the menuitems to
> commands, see
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/
> Broadcasters_and_Observers .
> 
> ::: devtools/client/menus.js
> @@ +11,5 @@
> > + */
> > +
> > +const {Cu} = require("chrome");
> > +const Services = require("Services");
> > +const isMac = Services.appinfo.OS.includes("Darwin");
> 
> == "Darwin" - no need for includes()
> 
> @@ +21,5 @@
> > +    observes: "DevToolbox",
> > +    dtd: "devToolboxMenuItem",
> > +    command: {
> > +      id: "DevToolbox",
> > +      oncommand: function(event) {
> 
> oncommand(event) here and elsewhere.
> 
> @@ +87,5 @@
> > +    disabled: true,
> > +    command: {
> > +      id: "BrowserToolbox",
> > +      oncommand: function() {
> > +        let { BrowserToolboxProcess } = Cu.import("resource://devtools/client/framework/ToolboxProcess.jsm", {});
> 
> Instead of all of these methods calling Cu.import on a new object every
> time, it seems like this module could just use
> XPCOMUtils.defineLazyModuleGetter and then the methods could just use the
> globals.
Arf the comment was sent while I was editing it... More reply and patches to come.
(In reply to Alexandre Poirot [:ochameau] from comment #42)
> (In reply to :Gijs Kruitbosch from comment #38)
> > Comment on attachment 8731700 [details] [diff] [review]
> > > It would make the whole menu insertion even more complex as this item is in
> > > middle of all other devtools items :/
> > > It's in between scratchpad and javascript console.
> > 
> > There's nothing that says it should be there - stick it at the bottom, and
> > put 'get more tools' and its separator after it?
> 
> Ok. Often we are told to not change anything regarding user facing
> behavior... so I tried hard to keep everything as-is.
> I moved back the menuitem to /browser/. Note that I only moved the
> <menuitem>, I was already using key and broadcaster from /browser/.

Well, this menu is only used by devtools, really, right? So it seems OK for devtools to change it. :-)

... and potentially add-ons? For which this dynamic insertion will already be problematic (ie you will break XUL overlays that rely on any of the items we're now inserting dynamically, irrespective of how you order them). The bug probably needs addon-compat and dev-doc-needed flags.

> > > It would allow to work on menuitems and key shortcuts via the hot reload
> > > addon.
> > 
> > didn't use to be true, at least - removing key registrations did not make
> > them available for new keys, IME. It's possible this has changed since I
> > last tried to do it, or that the problem is/was OS-specific.
> 
> It works on windows, which is the main OS I'm targeting for the reload addon.

When you say "works", have you tested what I suggested, ie does removing <key> elements stop that shortcut from working, and if you then re-add such a key element with the same shortcut but a different behaviour, does the new <key> get used?

> > (In reply to Alexandre Poirot [:ochameau] from comment #13)
> > > Created attachment 8724773 [details] [diff] [review]
> > > Removed unused connect access key.
> > > 
> > > The dtd had an accesskey for "Connect..." but it wasn't used in the XUL
> > > element.
> > > I remove it since it conflicts with Browser Toolbox one.
> > 
> > If I open the menu in 46, and press "c", the connect window opens. Either
> > way, you shouldn't leave the string empty if it's unused. In fact, shouldn't
> > change strings at all without changing the string identifier...
> 
> Ok. I'll update the patch to change its accesskey to "c",

Note that you will need to update the string identifier in that case.

> but note it also
> conflicts with the "Error console". But it's not as terrible as conflicting
> with the Browser toolbox, which is much more common.

Is that menuitem still in use? I don't see it and didn't think it was still available. I'd be in favour of unshipping it, especially now that it's easier to integrate devtools with other gecko things, but I don't know how other people feel about that.

> If that's really terrible for localizers, is there anything I can do to help?
> Better/more inline docs? Some flags on this bug? A message somewhere?

No, just a bullet we/they will have to bite, unfortunately. Part of this is that our l10n story is still pretty bad. It might be helpful to ping :flod about this.

> > FWIW, one of the things that I'd like to do at some point is taking the
> > frontend hookup code of devtools out of the way of ts_paint/tpaint. I don't
> > know if this helps or not, because devtools has all its own extra-special
> > module loading infrastructure that is completely opaque to me so I can't
> > tell how the dependencies work now... but it would be good to keep in mind.
> 
> You should talk to us about such things. We can surely tweak things. I have
> a clear picture of devtools startup these days... We recently change
> devtools tartup in bug 1248348.
> It is no longer loaded arbitrarily in middle of gBrowserInit._delayedStartup.
> Instead it is loaded on browser-delayed-startup-finished, which is slightly
> after.
> Is it still happening before ts_paint/tpaint?

I don't know but I expect so.

> If yes, How can I easily see
> that without running talos, like just running firefox?

Running talos is easy, check ./mach help talos and/or ./mach talos --help .
Attachment #8731699 - Attachment is obsolete: true
Rebased, applying it before "Register devtools menuitems dynamically" patch.
Attachment #8732919 - Flags: review+
Attachment #8731703 - Attachment is obsolete: true
(In reply to :Gijs Kruitbosch from comment #38)
> Comment on attachment 8731700 [details] [diff] [review]
> ::: devtools/client/framework/browser-menus.js
> @@ +28,5 @@
> > + *        The document to which keys are to be added.
> > + * @param {String} property
> > + *        Prefix of the properties entry to look for key shortcut in
> > + *        localization file. We will look for {property}.key and
> > + *        {property}.keytext for non-character shortcuts like F12.
> 
> This docstring does not match the function definition.
> 
> "dtd" seems misleading considering it's not actually from a dtd file...
> maybe just call it stringId or something like this if you want to be more
> specific than "property"?

Went for "l10nKey".

> @@ +305,5 @@
> > + * @param {XULDocument} doc
> > + *        The document to which keys and menus are to be added.
> > + */
> > +function addTopLevelItems(doc) {
> > +  let frags = {
> 
> None of the lines where you use this comes close to 80 characters, so just
> call the variable "fragments".

Done.

> @@ +322,5 @@
> > +    } else {
> > +      let { id, observes, dtd } = item;
> > +      let broadcasterId = "devtoolsMenuBroadcaster_" + observes;
> > +
> > +      // Create a <menuitem>
> 
> Seems like this could be a helper method as well, and could be reused by the
> other code in browser-menus.js that also creates these items.

Done.

> @@ +335,5 @@
> > +        menuitem.setAttribute("autocheck", "false");
> > +      }
> > +      frags.menuItems.appendChild(menuitem);
> > +
> > +      if (item.onlyMenuItem) {
> 
> This makes people defining menus make something explicit that the code
> should just detect. Can just do:
> [...]
> 
> ... but then, if we leave page source out of this, I guess this is all moot
> anyway.

Removed by letting page source in /browser/.

> 
> @@ +339,5 @@
> > +      if (item.onlyMenuItem) {
> > +        continue;
> > +      }
> > +
> > +      // Create a <broadcaster>
> 
> This too should be a helper, to avoid having such long methods which are
> hard to read.

Done.

> @@ +353,5 @@
> > +      if (typeof(item.command) == "function") {
> > +        // Do not use intermediate <command> and call JS function
> > +        broadcaster.oncommand = item.command;
> > +        // For unknown reason, the menuitem isn't able to fire its related
> > +        // broadcaster.
> 
> ??
> 
> What were you trying to do and what is the problem here? If the
> broadcaster's oncommand attribute has no effect, why bother setting it on
> the broadcaster at all?

broadcaster.oncommand is called when hitting the key shortcut,
but not when clicking on the menuitem.

>  menuitem.addEventListener("command", item.command);

allows to address just that.
And so we have to keep broadcaster.oncommand for the key shortcut to keep working.

I can update the comment by removing the "unknown reason" to make it less depressing.
I can also open a XUL bug, or try to look for an existing one if you want.

> @@ +359,5 @@
> > +      } else if (typeof(item.command) == "string") {
> > +        // Refer to an existing <command>
> > +        broadcaster.setAttribute("command", item.command);
> > +      } else if (item.command) {
> > +        // Create a <command>
> 
> And this should be a helper.

Done.

> @@ +365,5 @@
> > +        let c = doc.createElement("command");
> > +        c.id = "Tools:" + command.id;
> > +        broadcaster.setAttribute("command", c.id);
> > +
> > +        c.setAttribute("oncommand", ";"); // needed. See bug 371900
> 
> Rather than saying "needed", please actually explain what the issue is.

Done.

> @@ +370,5 @@
> > +        c.addEventListener("command", command.oncommand);
> > +
> > +        // For unknown reason, the menuitem isn't able to fire its related
> > +        // command
> > +        menuitem.addEventListener("command", command.oncommand);
> 
> It sounds like the command and oncommand attributes of the broadcaster are
> just not being propagated at all. Did you check? Did you try using a C++
> level debugger to see what was going on?
> 
> The level of "for some reason this doesn't work so I'm hacking it" in this
> file is a bit disconcerting.

I'm no longer trying to debug XUL. Last time I tried to fix something it got rejected.
I consider XUL as a dead piece of meat that we have to workaround until we finaly get rid of.
Sorry if that ends up with depressing comments in code.

> Looking briefly at the documentation for broadcasters, from purely the code
> in this patch, it seems like many (all?) of the cases that this code uses
> broadcasters for could just be handled by directly linking the menuitems to
> commands, see
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/
> Broadcasters_and_Observers.

As for Page Source, I'm trying to keep everything in-place.
I'm expecting addons or seamonkey to be displeased by broadcaster removal:
  https://mxr.mozilla.org/addons/search?string=devtoolsMenuBroadcaster_
As you already highlighted, all overlays are going to break, but there isn't only overlay usages.
If you think I shouldn't care about compatiblity and just clean up things, I may keep just the keys and menuitems.

Hopefully, we could switch to some Add-on API like WebExtension to have something simplier.
Sorry again for the lack of efforts around XUL, I'm not sure it is worthwhile to spend much time around it.

> ::: devtools/client/menus.js

All comments about this file adressed.
Attachment #8732922 - Flags: review?(gijskruitbosch+bugs)
Attachment #8731700 - Attachment is obsolete: true
Ok so instead of removing the accesskey I'm explicitely setting it to "c".
The error console is disabled by default, you have to toggle this pref to see it:
  devtools.errorconsole.enabled
I can start a thread to see if we can just remove it.
I was surprised to see some other mozilla product, seamonkey or something else
still using it actively :/

Do I really need to update the key, even if we just switched the dtd into properties?
Attachment #8732923 - Flags: review?(gijskruitbosch+bugs)
Attachment #8731701 - Attachment is obsolete: true
(In reply to :Gijs Kruitbosch from comment #44)
> When you say "works", have you tested what I suggested, ie does removing
> <key> elements stop that shortcut from working, and if you then re-add such
> a key element with the same shortcut but a different behaviour, does the new
> <key> get used?

Yes. "works" means it works great.
Previous key is no longer working and new one works.
But note that we were already registering tool-specific menus and keys dynamically
and already have a workaround over here:
  http://mxr.mozilla.org/mozilla-central/source/devtools/client/framework/devtools-browser.js#371
(In reply to :Gijs Kruitbosch from comment #40)
> Comment on attachment 8731700 [details] [diff] [review]
> Register devtools menuitems dynamically - v2
> 
> Review of attachment 8731700 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser-sets.inc
> @@ -103,5 @@
> > -    <command id="Tools:BrowserContentToolbox" oncommand="gDevToolsBrowser.openContentProcessToolbox();" disabled="true" hidden="true"/>
> > -    <command id="Tools:BrowserConsole" oncommand="HUDService.openBrowserConsoleOrFocus();"/>
> > -    <command id="Tools:Scratchpad" oncommand="Scratchpad.openScratchpad();"/>
> > -    <command id="Tools:ResponsiveUI" oncommand="ResponsiveUI.toggle();"/>
> > -    <command id="Tools:Eyedropper" oncommand="openEyedropper();"/>
> 
> Oh, and I almost forgot: this and other global things here should be removed
> as part of this patch if, like this one, there are no other callers.

This is part of my overall plan, but in a followup (bug 1250832) as it is surely going to break various addons. I think I'll remove all the one with no occurence on mxr addons, and let some others like gDevTools that are extensively used.
Comment on attachment 8732923 [details] [diff] [review]
Update connect access key to prevent conflict with Browser toolbox - v1

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

I think this is OK because we're not changing the meaning of the label - just picking a non-conflicting access key in English (oops). Francesco, can you confirm we don't need to change the string id for this?
Attachment #8732923 - Flags: review?(gijskruitbosch+bugs) → review?(francesco.lodolo)
Comment on attachment 8732923 [details] [diff] [review]
Update connect access key to prevent conflict with Browser toolbox - v1

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

Yes, this is absolutely fine.
Attachment #8732923 - Flags: review?(francesco.lodolo) → review+
Just posted about getting rid of the old XUL Error Console:
  https://groups.google.com/forum/#!topic/mozilla.dev.developer-tools/XYPqQ58ndX4
Comment on attachment 8732922 [details] [diff] [review]
Register devtools menuitems dynamically - v3

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

In future, please use mozreview. Now I have no interdiff... (not much use doing it now as it still won't have interdiff with the earlier patches).


> If you think I shouldn't care about compatiblity and just clean up things, I may keep just the keys and menuitems.

Personally, I would be fine with doing that. I expect it might simplify some of this patch.

::: devtools/client/framework/browser-menus.js
@@ +18,5 @@
>  loader.lazyRequireGetter(this, "gDevTools", "devtools/client/framework/devtools", true);
>  
> +// Keep list of inserted DOM Elements in order to remove them on unload
> +// Maps browser xul document => list of DOM Elements
> +const FragmentsCache = new WeakMap();

This doesn't work as described, does it? The keys will be weakly-refererenced, but the values in the map will be strongly-referenced and they have a strong ref to their documents, so using a weakmap instead of a map will not in fact help with GC'ing anything. Plus, you don't nullcheck the return value of .get() when accessing items in the weak map, so you're also assuming all the documents in it never go away (and that you'll only ever be passed documents that are in the map).

Why do we need this at all? Your previous patch didn't have it. Why do we need to remove the nodes from the dead document? Is it because we're keeping references to them, and they have function references to us, and we're kept alive because we're not tied to the window's lifetime, and so the whole thing stays alive? In that case, I'm not sure to what extent removing the elements fixes much. Switching away from the added event listeners and function references might obviate the need for this though.

@@ +214,5 @@
>  
> +  let bc = createBroadcaster({
> +    doc,
> +    id: "devtoolsMenuBroadcaster_" + id,
> +    label: toolDefinition.menuLabel || toolDefinition.label

Shouldn't the access key be on the broadcaster as well? Splitting the location of the label and the access key is confusing...

@@ +404,5 @@
> +      if (typeof(item.command) == "function") {
> +        // Do not use intermediate <command> and call JS function
> +        broadcaster.oncommand = item.command;
> +        // For unknown reason, the menuitem isn't able to fire its related
> +        // broadcaster.

Actually, I expect I know why. Use broadcaster.setAttribute("oncommand", item.command) instead of using the property. Yes, that means you'll need to use strings everywhere. The previous code did that, too, so I don't think it should be very hard...

As it is, I suspect your workaround won't work properly when we copy the menuitems into the subview panel for the developer tools button, ie I expect the copies will miss an event handler and do nothing.

@@ +422,5 @@
> +        broadcaster.setAttribute("command", c.id);
> +
> +        // For unknown reason, the menuitem isn't able to fire its related
> +        // command
> +        menuitem.addEventListener("command", command.oncommand);

Again, refactor to use strings and you won't need this workaround.

@@ +445,5 @@
> +  }
> +
> +  // Cache all nodes before insertion to be able to remove them on unload
> +  let nodes = [];
> +  for(let node of fragments.commands.children) {

nit: space after "for"
Attachment #8732922 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #54)
> ::: devtools/client/framework/browser-menus.js
> @@ +18,5 @@
> >  loader.lazyRequireGetter(this, "gDevTools", "devtools/client/framework/devtools", true);
> >  
> > +// Keep list of inserted DOM Elements in order to remove them on unload
> > +// Maps browser xul document => list of DOM Elements
> > +const FragmentsCache = new WeakMap();
> 
> This doesn't work as described, does it? The keys will be
> weakly-refererenced, but the values in the map will be strongly-referenced
> and they have a strong ref to their documents, so using a weakmap instead of
> a map will not in fact help with GC'ing anything.

Oh right. Yes this is wrong. I'll use a Map and remove the entry once items are removed.
removeMenus is called whenever a top window is closed and also when devtools are unloaded.

> Why do we need this at all?

This is one key goal of that patch: be able to reload devtools without restarting Firefox.
We have a helper addon that allows to do that.
So we have to remove all menus and keys when the devtools are unloaded.

> Switching away from the added event listeners and
> function references might obviate the need for this though.

This is not the primary goal, but it helps.

> @@ +214,5 @@
> >  
> > +  let bc = createBroadcaster({
> > +    doc,
> > +    id: "devtoolsMenuBroadcaster_" + id,
> > +    label: toolDefinition.menuLabel || toolDefinition.label
> 
> Shouldn't the access key be on the broadcaster as well? Splitting the
> location of the label and the access key is confusing...

This is the existing code:
  http://mxr.mozilla.org/mozilla-central/source/devtools/client/framework/devtools-browser.js#665
And label used to be on the broadcasters for other menu entries:
  http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sets.inc#199
I imagine it would be better to just get rid of broadcasters.
Would you mind doing this in followup?

> @@ +404,5 @@
> > +      if (typeof(item.command) == "function") {
> > +        // Do not use intermediate <command> and call JS function
> > +        broadcaster.oncommand = item.command;
> > +        // For unknown reason, the menuitem isn't able to fire its related
> > +        // broadcaster.
> 
> Actually, I expect I know why. Use broadcaster.setAttribute("oncommand",
> item.command) instead of using the property. Yes, that means you'll need to
> use strings everywhere. The previous code did that, too, so I don't think it
> should be very hard...
> 
> As it is, I suspect your workaround won't work properly when we copy the
> menuitems into the subview panel for the developer tools button, ie I expect
> the copies will miss an event handler and do nothing.

... I didn't knew about this. I imagine you are refering to the devtools entry in the hamburger menu?
Yes. It doesn't work.

I wish we could get away from command strings as it relies on globals being available in browser.xul scope. I'll try to use strings and open a followup to try to get away from them.
Attached patch interdiff (obsolete) — Splinter Review
Interdiff on top of attachment 8732922 [details] [diff] [review].
Ok. So I went ahead and simplified the code by creating only menuitem and key elements.
I'm not using oncommand strings as it makes everything more complex.
(It forces to expose a global method and figure out on which menu/key we received an event from)

I haven't cleaned the existing codepaths, for per-tool menuitems and keys.
But I'm happy to do that in a followup bug.

See the interdiff in previous comment.
Attachment #8733461 - Flags: review?(gijskruitbosch+bugs)
Attachment #8732922 - Attachment is obsolete: true
The removal of xul:command forces me to update code in order to toggle the menuitems instead.
Attachment #8733462 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8733461 [details] [diff] [review]
Register devtools menuitems dynamically - v4

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

::: devtools/client/framework/browser-menus.js
@@ +149,5 @@
> +    command.setAttribute("oncommand", oncommand);
> +  } else if (typeof(oncommand) == "function") {
> +    // Bug 371900: command event is fired only if "oncommand" attribute is set.
> +    command.setAttribute("oncommand", ";");
> +    command.addEventListener("command", oncommand);

But I can remove this code. This shouldn't be used anymore.

@@ +398,5 @@
> +      if (item.key && l10nKey) {
> +        // Create a <key>
> +        let key = createKey(doc, l10nKey, null, item.key);
> +        key.setAttribute("oncommand", ";");
> +        key.addEventListener("command", item.oncommand);

I should have add a comment there. <key> have the same issue than <command>...
Comment on attachment 8733462 [details] [diff] [review]
Toggle menuitem directly instead of now gone xul:command - v1

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

rs=me - a green try would be some comfort. Note that the utility function seems to still be called "toggleCmd" ? Might be worth updating that. Also not sure that I'm the best reviewer for this, seems like it's all devtools code...
Attachment #8733462 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8733461 [details] [diff] [review]
Register devtools menuitems dynamically - v4

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

You'll want to merge this with the other patch I just rs'd, because otherwise the tree will be in a half-broken state between these patches (as devtools-browser.js references the commands that are gone after this patch).

With both of our comments addressed, r=me.

::: devtools/client/framework/browser-menus.js
@@ +75,5 @@
> + * @param {String} id
> + *        Element id.
> + * @param {String} label
> + *        Menu label.
> + * @param {String} broadcasterId

This is optional now.

@@ +91,5 @@
> +  menuitem.id = id;
> +  if (label) {
> +    menuitem.setAttribute("label", label);
> +  }
> +  menuitem.setAttribute("observes", broadcasterId);

This should be conditional - you don't pass a broadcasterId in the new code, just in the old code that's now using this method.

@@ +404,5 @@
> +        keys.appendChild(key);
> +      }
> +      if (item.additionalKeys) {
> +        // Create additional <key>
> +        item.additionalKeys.forEach(key => {

Personally I would just:

for (let key of item.additionalKeys)

but I guess that's just me? :-)

@@ +405,5 @@
> +      }
> +      if (item.additionalKeys) {
> +        // Create additional <key>
> +        item.additionalKeys.forEach(key => {
> +          let k = createKey(doc, key.l10nKey, null, key);

either way, let's name this "keyElement" or "node" or something slightly more descriptive than "k".
Attachment #8733461 - Flags: review?(gijskruitbosch+bugs) → review+
Blocks: 1258985
This bug is going to change devtools xul elements that used to be hardcoded in browser.xul.
All xul:command, xul:broadcaster are gone, we no longer generate them.
Second and even more important, this is no longer hardcoded, so we still generate xul:key and xul:menuitem, but on runtime, after browser-delayed-startup-finished event.

It basically means that all addons using overlays to shuffle around devtools may break.
(Note that you can still move the web developer menu. The runtime code is going to add menuitems whereever it ends up being. So overlay against "menuWebDeveloperPopup" are still going to work)

And the addons querying for xul:command by IDs will also need to be refering to menuitem instead.

If that can help, here is a list of usages that are going to break:
  https://mxr.mozilla.org/addons/search?string=devtoolsMenuBroadcaster_
Keywords: addon-compat
Blocks: 1258987
Attachment #8733457 - Attachment is obsolete: true
Attachment #8733461 - Attachment is obsolete: true
To be merged into attachment 8733829 [details] [diff] [review].
:jryans, could you review this, :gijs isn't confident reviewing this very devtools code.
So I ended up not creating command and broadcaster elements.
So that we have to toggle menuitems directly.
Attachment #8733830 - Flags: review?(jryans)
Attachment #8733462 - Attachment is obsolete: true
Forgot the command=";" trick for additionnal keys (F12 for toolbox).
Attachment #8733891 - Flags: review+
Attachment #8733829 - Attachment is obsolete: true
A few more tweaks are needed to accommodate the disparition of command and broadcasters.
Attachment #8733892 - Flags: review?(jryans)
Attachment #8733830 - Attachment is obsolete: true
Attachment #8733830 - Flags: review?(jryans)
Comment on attachment 8733892 [details] [diff] [review]
Toggle menuitem directly instead of now gone xul:command - v3

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

::: devtools/client/framework/devtools-browser.js
@@ +78,5 @@
>     */
>    updateCommandAvailability: function(win) {
>      let doc = win.document;
>  
> +    function toggleMenuitem(id, isEnabled) {

Nit: toggleMenuItem

::: devtools/client/menus.js
@@ +47,5 @@
> + * Detect the presence of a Firebug.
> + */
> +function isFirebugInstalled() {
> +  let bootstrappedAddons = Services.prefs.getCharPref("extensions.bootstrappedAddons");
> +  return bootstrappedAddons.indexOf("firebug@software.joehewitt.com") != -1;

Seems like we should really be asking the add-on manager here, but anyway, I know you are just moving the previous code.
Attachment #8733892 - Flags: review?(jryans) → review+
Merging the two patches while addressing toggleMenuItem and fixing eslint.
Attachment #8733997 - Flags: review+
Attachment #8733891 - Attachment is obsolete: true
Attachment #8733892 - Attachment is obsolete: true
Had an issue during the rebase, missing a part.
Attachment #8734012 - Flags: review+
Attachment #8733997 - Attachment is obsolete: true
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #70)
> ::: devtools/client/menus.js
> @@ +47,5 @@
> > + * Detect the presence of a Firebug.
> > + */
> > +function isFirebugInstalled() {
> > +  let bootstrappedAddons = Services.prefs.getCharPref("extensions.bootstrappedAddons");
> > +  return bootstrappedAddons.indexOf("firebug@software.joehewitt.com") != -1;
> 
> Seems like we should really be asking the add-on manager here, but anyway, I
> know you are just moving the previous code.

I've kept this code as-is as it has to be synchronous :/
It may have been the original reason we didn't used the AddonManager?


Otherwise, this bug depends on bug 1247203 which isn't ready yet. I'll land this one as soon as I get a green try.
Blocks: 1261092
Rebased on top of Responsive Design s/Tool/Mode/ change.
Ready to land once trees reopen...
Attachment #8736747 - Flags: review+
Attachment #8734012 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/d03f089bbab80cd36c4b0e82cbbaa1eebdb52528
Bug 1248603 - Factor out menu and shortcut creation to dedicated module. r=jryans

https://hg.mozilla.org/integration/fx-team/rev/836da2d40b19d6503e640897e7834ed36a8e8dab
Bug 1248603 - Remove support of the appmenu from devtools. r=jryans

https://hg.mozilla.org/integration/fx-team/rev/0b016f319532937405bf971cdb1e426e81bc35f5
Bug 1248603 - Toggle developer toolbar via the menuitem in tests. r=jryans

https://hg.mozilla.org/integration/fx-team/rev/9b9b8514c6b0479d470b0c30f84b6e9552591da5
Bug 1248603 - Register devtools menuitems dynamically. r=gijs,jryans

https://hg.mozilla.org/integration/fx-team/rev/97e1c3fdf6f22c4a643b378eec2b775d9d95bb3d
Bug 1248603 - Update connect access key to prevent conflict with Browser toolbox. r=flod
Depends on: 1265599
Depends on: 1353548
Depends on: 1418377
See Also: → 1428191
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.