1258987 - Drop broadcaster/command for tools entries

Status: RESOLVED FIXED

(DevTools :: General, defect, P3)

Description

[Alexandre Poirot [:ochameau] PTO, back August 11th]

Bug 1248603 stopped generating useless broadcaster/command xul elements for top level menuitems. We can also simply per-tool entries, while ensuring we no longer use "oncommand" strings:
http://hg.mozilla.org/mozilla-central/annotate/3b8417be6c87/devtools/client/framework/devtools-browser.js#l647
  cmd.setAttribute("oncommand",
    gDevToolsBrowser.selectToolCommand(gBrowser, "' + id + '");')

So that we no longer depends on any global from browser.xul.

Comment 1

[Alexandre Poirot [:ochameau] PTO, back August 11th]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=21957c219574

Comment 2

[Alexandre Poirot [:ochameau] PTO, back August 11th]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b22fce248e7d

Comment 3

[Alexandre Poirot [:ochameau] PTO, back August 11th]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2de933d3de6

Comment 4

[Alexandre Poirot [:ochameau] PTO, back August 11th]

Attachment: patch v1

Comment 5

[Alexandre Poirot [:ochameau] PTO, back August 11th]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1585dcd23f6

Comment 6

[Alexandre Poirot [:ochameau] PTO, back August 11th]

Attachment: patch v2

I'm wondering if that patch fixes the error you reported on irc?

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8737783 [details] [diff] [review]
patch v2

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

Looks good to me.  Doesn't fix my error, but we now have bug 1261920 for that.

::: devtools/client/framework/browser-menus.js
@@ +298,5 @@
> +        let key = createKey({
> +          doc,
> +          id: item.key.id,
> +          shortcut: shortcut,
> +          keytext: shortcut.startsWith("VK_") ? l10n(l10nKey + ".keytext") : null,

Do you need to check here `shortcut.startsWith("VK_")`?  You check it again in `createKey`.

Comment 8

[Alexandre Poirot [:ochameau] PTO, back August 11th]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> Doesn't fix my error, but we now have bug 1261920 for that.

Yes, it is a very different issue related to customizable widgets.

> ::: devtools/client/framework/browser-menus.js
> @@ +298,5 @@
> > +        let key = createKey({
> > +          doc,
> > +          id: item.key.id,
> > +          shortcut: shortcut,
> > +          keytext: shortcut.startsWith("VK_") ? l10n(l10nKey + ".keytext") : null,
> 
> Do you need to check here `shortcut.startsWith("VK_")`?  You check it again
> in `createKey`.

Unfortunately, yes. The ".keytext" localization only exists for VK_ keys.
It would throw if we try to access it for non-vk_ keys.

Comment 9

[Alexandre Poirot [:ochameau] PTO, back August 11th]

addon-compat:
Removing xul:command and xul:broadcaster for per-tool entries in the Web Developer menu.
Bug 1248603 already did that for global entries. So all entries are now going to miss these useless xul elements.

Comment 10

[Alexandre Poirot [:ochameau] PTO, back August 11th]

https://hg.mozilla.org/integration/fx-team/rev/4b00370192a495bf15b4110a2dd6c8fd8084e0c9
Bug 1258987 - Stop generating useless xul:command and xul:broadcaster. r=jryans

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4b00370192a4