Drop broadcaster/command for tools entries

RESOLVED FIXED in Firefox 48

Status

defect
P3
normal
RESOLVED FIXED
3 years ago
10 months ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

({addon-compat})

unspecified
Firefox 48
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [btpp-backlog])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 4

3 years ago
Posted patch patch v1 (obsolete) — Splinter Review
(Assignee)

Updated

3 years ago
Assignee: nobody → poirot.alex
Priority: -- → P3
Whiteboard: [btpp-backlog]
(Assignee)

Comment 6

3 years ago
Posted patch patch v2Splinter Review
I'm wondering if that patch fixes the error you reported on irc?
Attachment #8737783 - Flags: review?(jryans)
(Assignee)

Updated

3 years ago
Attachment #8734555 - Attachment is obsolete: true
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`.
Attachment #8737783 - Flags: review?(jryans) → review+
(Assignee)

Comment 8

3 years ago
(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.
(Assignee)

Comment 9

3 years ago
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.
Keywords: addon-compat
(Assignee)

Comment 10

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

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4b00370192a4
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48

Updated

10 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.