1476097 - Use the Menu API's edit menu for all context menu textboxes within the toolbox

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P2)

Description

[Brian Grinstead [:bgrins]]

Right now, we use editMenuCommands and editMenuKeys, along with some XUL markup for the devtools "Edit" menu: https://searchfox.org/mozilla-central/rev/b0275bc977ad7fda615ef34b822bba938f2b16fd/devtools/client/framework/toolbox.xul#30-50.

This gets wired up through the toolbox here with associated functions https://searchfox.org/mozilla-central/rev/b0275bc977ad7fda615ef34b822bba938f2b16fd/devtools/client/framework/toolbox.js#481-494.

We're changing this for the Browser Console in Bug 1456852. In addition to moving the edit menu handling out of the webconsole and up to the toolbox, we should check to make sure we create and update the necessary command elements so that the "Edit" menu for the application menu in the browser gets set properly when showing.

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: Bug 1476097 - Use the Menu API's edit menu for all context menu textboxes within the toolbox; r=bgrins

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 5

[Francesco Lodolo [:flod]]

Comment on attachment 9010950 [details]
Bug 1476097 - Use the Menu API's edit menu for all context menu textboxes within the toolbox; r=bgrins

Francesco Lodolo [:flod] has approved the revision.

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

The following PR for devtools-html/devtools-core is also needed
https://github.com/devtools-html/devtools-core/pull/1094

This package is duping menu API impl:
https://github.com/devtools-html/devtools-core/blob/4a6a6decb7a866c5d0a1b3ff66b57edd66827c3f/packages/devtools-modules/src/menu/index.js

Two debugger tests will also need some changes I guess.

devtools/client/debugger/new/test/mochitest/browser_dbg-breakpoints-actions.js
devtools/client/debugger/new/test/mochitest/browser_dbg-breakpoints-cond.js

Honza

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 9010950 [details]
Bug 1476097 - Use the Menu API's edit menu for all context menu textboxes within the toolbox; r=bgrins

Brian Grinstead [:bgrins] has approved the revision.

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

So, I am indeed seeing perf regression:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=18915d8a2376985a1e3b031f08b9b436fe53ffee&newProject=try&newRevision=a7376a24f883d7414b9de14b92e7cf98a27d0a9e&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&framework=12

console.autocomplete 5.51%

(not sure why console.autocompletion)

I tried to use MozXULElement, but putting the following statement into Toolbox constructor:
(somewhere here: https://searchfox.org/mozilla-central/rev/65f9687eb192f8317b4e02b0b791932eff6237cc/devtools/client/framework/toolbox.js#102)

... produces an error:
ReferenceError: MozXULElement is not defined

How should I use it?

Honza

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Jan Honza Odvarko [:Honza] from comment #8)
> So, I am indeed seeing perf regression:
> https://treeherder.mozilla.org/perf.html#/
> comparesubtest?originalProject=try&originalRevision=18915d8a2376985a1e3b031f0
> 8b9b436fe53ffee&newProject=try&newRevision=a7376a24f883d7414b9de14b92e7cf98a2
> 7d0a9e&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignatur
> e=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&framework=12
> 
> console.autocomplete 5.51%
> 
> (not sure why console.autocompletion)

Huh, I'd like to know why this is happening. I would have expected maybe some toolbox open regressions based on eager Fluent loading (i.e. Bug 1441035). If this is truly linux only and only on autocompletion then I'm not super concerned about it, but we should check with Nicolas before landing anything there.

> I tried to use MozXULElement, but putting the following statement into
> Toolbox constructor:
> (somewhere here:
> https://searchfox.org/mozilla-central/rev/
> 65f9687eb192f8317b4e02b0b791932eff6237cc/devtools/client/framework/toolbox.
> js#102)
> 
> ... produces an error:
> ReferenceError: MozXULElement is not defined
> 
> How should I use it?

Is this for using MozXULElement.insertFTLIfNeeded? MozXULElement is attached to the window itself (https://searchfox.org/mozilla-central/source/toolkit/components/processsingleton/CustomElementsListener.jsm#19) so it should work if you do `contentWindow.MozXULElement`. Let me know if it doesn't.

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Brian Grinstead [:bgrins] from comment #9)
> Is this for using MozXULElement.insertFTLIfNeeded? MozXULElement is attached
> to the window itself
> (https://searchfox.org/mozilla-central/source/toolkit/components/
> processsingleton/CustomElementsListener.jsm#19) so it should work if you do
> `contentWindow.MozXULElement`. Let me know if it doesn't.

So, inserting:
`contentWindow.MozXULElement.insertFTLIfNeeded("toolkit/main-window/editmenu.ftl");`

into the Toolbox constructor generates:

TypeError: contentWindow.MozXULElement is undefined; can't access its "insertFTLIfNeeded" property

Honza

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

I'm not sure why this is happening as well. 
The autocomplete test only measure time spent showing and closing the autocomplete popup (e.g. no toolbox opening), so I'm curious what's going on here.
The autocomplete popup does not seem modified here, so the only way of knowing why the patch regresses the autocomplete test is to record a profile and compare it to another profile without the patch.

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Jan Honza Odvarko [:Honza] from comment #10)
> (In reply to Brian Grinstead [:bgrins] from comment #9)
> > Is this for using MozXULElement.insertFTLIfNeeded? MozXULElement is attached
> > to the window itself
> > (https://searchfox.org/mozilla-central/source/toolkit/components/
> > processsingleton/CustomElementsListener.jsm#19) so it should work if you do
> > `contentWindow.MozXULElement`. Let me know if it doesn't.
> 
> So, inserting:
> `contentWindow.MozXULElement.insertFTLIfNeeded("toolkit/main-window/editmenu.
> ftl");`
> 
> into the Toolbox constructor generates:
> 
> TypeError: contentWindow.MozXULElement is undefined; can't access its
> "insertFTLIfNeeded" property

I checked, and the contentWindow's location has set yet at this point (so the CE listener hasn't loaded the script, and even if it had it would be the wrong document at this point).

We'll want to put it after toolbox.xul has been loaded. I'm not sure exactly where would be best, but `this.win.MozXULElement` is accessible inside onceDOMReady, for instance: https://searchfox.org/mozilla-central/rev/c9272ef398954288525e37196eada1e5a93d93bf/devtools/client/framework/toolbox.js#443

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Brian Grinstead [:bgrins] from comment #12)
> We'll want to put it after toolbox.xul has been loaded. I'm not sure exactly
> where would be best, but `this.win.MozXULElement` is accessible inside
> onceDOMReady, for instance:
> https://searchfox.org/mozilla-central/rev/
> c9272ef398954288525e37196eada1e5a93d93bf/devtools/client/framework/toolbox.
> js#443

@Brian, the patch is updated. Strings (*.ftl file) is now loaded lazily
when the menu is opened for the first time. Please have a look at it

There is no perf regression now:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=9a181702ac16f5e0899a795c525e4e05cc675ade&newProject=try&newRevision=5060a71aec85d95b429e7856776517dd453a2c93&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&framework=12

Honza

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/796f3bfbaa2d
Use the Menu API's edit menu for all context menu textboxes within the toolbox; r=bgrins,flod

Comment 15

[Eliza Balazs [:ebalazs_]]

Backed out changeset 796f3bfbaa2d (Bug 1476097) for perma dt failures in devtools/client/debugger/new/test/mochitest/browser_dbg-breakpoints-actions.js CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=796f3bfbaa2db4fba707e3bb2c3c1160e46c354b&selectedJob=208604940

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=208604940&repo=autoland&lineNumber=1761

Backout push: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=513d18d84b8270c12da05ec90c2886f601f885cb

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2812f44f383e
Use the Menu API's edit menu for all context menu textboxes within the toolbox; r=bgrins,flod

Comment 17

[Jan Honza Odvarko [:Honza] (always need-info? me)]

devtools-core module updated:
https://github.com/devtools-html/devtools-core/commit/d167402099026959d3475514571be5ac94328466

...and Try is green.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a71a3953c55ef8ee284d3675fc000145cdf859f8

Trying to re-land.

Honza

Comment 18

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2812f44f383e