This is quite similar to bug 1261920. STR: 1. Start Firefox 2. A browser window appears, no errors so far 3. Open an additional browser window, errors appear: console.error: CustomizableUI: Key element with id 'key_devToolboxMenuItem' for widget 'developer-button' not found! console.error: CustomizableUI: Key element with id 'key_webide' for widget 'webide-button' not found!

:ochameau, do you have any ideas about this? I know you've worked in this area with the similar bug 1261920 in the past.

One thing to add to the STR, open WebIDE at least once. So the story is that <xul:key> are added dynamically now, via browser-menus.js. But it is done too late regarding CustomizableUI. CustomizableUI is already loading on browser.xul's load event and expect to find any related <xul:key>. Whereas browser-menus's entry point which is "addMenus" is only called on browser-delayed-startup-finished. We delay as far as we can to prevent impacting firefox startup, but we may be more agressive for newly opened windows.

Load devtools in two steps. Watch for browser.xul's DOMContentLoaded that, to register keys before CustomizableUI. I'm not a big fan of this as we aren't that lazy anymore and complexity gDevToolsBrowser... But I don't see how to address this differently?

You'll get these errors when detaching tabs into their own process/windows as well. STR: * launch the latest version of m-a ** build used: fx50.0a2, buildId: 20160824004001, changeset: a72bfbdf5c9b * open a new tab so there's two tabs opened * detach one of the tabs into it's own separate process/window You'll receive the following errors in the browser console: * CustomizableUI:Key element with id 'key_devToolboxMenuItem' for widget 'developer-button' not found! CustomizableUI.jsm:1370 * CustomizableUI:Key element with id 'key_webide' for widget 'webide-button' not found! CustomizableUI.jsm:1370

Comment on attachment 8784373 [details] [diff] [review] patch v1 Tests look green on opt. Do you thin that's a sensible approach? I wish we could keep things simple and lazy...

Thanks kamil for your report, this patch should fix both STRs.

Comment on attachment 8784373 [details] [diff] [review] patch v1 Review of attachment 8784373 [details] [diff] [review]: ----------------------------------------------------------------- Seems to work well, thanks for the fix. It seems reasonable enough to me. You may want to do some Talos runs as well, to be sure it's not regressing browser startup.

Here is my try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=82a7837b76a2&selectedJob=26324861 The base changeset, from mozilla-central: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=01748a2b1a463f24efd9cd8abad9ccfd76b037b8&filter-searchStr=talos&selectedJob=4739416 And if I understand talos and the related tools correctly, here is the results: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=01748a2b1a46&newProject=try&newRevision=f8cd7ed8b05f&framework=1 Which reports scary things regarding fileio but not that much around startup itself. There is tabpaint opt e10s on linux with a 7% regression but some others better improvements on others like tp5o responsiveness opt e10s. Hard to tell, I've just started some new runs on the base build. fileio regression makes sense if we start pulling many dependencies via addMenus... which doesn't surprise me.

Looking at the results, it does seem there are some regressions. Can we try to be more lazy inside addMenus code path at least?

Hum. I looked at what modules we are pulling earlier and it looks like there is only two: resource://devtools/client/framework/browser-menus.js resource://devtools/client/menus.js Which is the bare minimum for executing addMenus. That's for the commonjs modules, but we are also loading this properties file: chrome://devtools/locale/menus.properties I'm wondering if just these 3 resources being loaded earlier are the reason of the possible reported regressions ?!?

Another patch that has nothing to do with this bug, but just to see the influence of loading a bunch of modules during firefox startup. This patch stops loading a bunch of SDK modules related to unload modules and tons of helpers pulled by JSON viewer xpcom...

https://treeherder.mozilla.org/perf.html#/compare?originalProject=autoland&originalRevision=7293879d87d7&newProject=try&newRevision=4c8154e148f9&framework=1 It looks like loading less modules during very early devtools startup, from devtools-startup.js, so very early during firefox startup, has an effect on sessionrestore, ts_paint and tp5o. Between 2.7% to 6.5% win. I don't know yet about fileio because it needs talos run on windows, which I juts spawn on my try build.

Hum. fileio test seems to be misleading. It also report a regression with the second patch which clearly optimize things for most of the other tests. So I'm wondering if we should just ignore fileio? But still, the offending patch seems to regress "tabpaint opt e10s" by 7%, whereas my second optimizing patch doesn't affect this particular test. (so I can't trade this regression with an optimization)

:jmaher, any thoughts on how to best interpret the data here?

for fileio- there is bug 1274018 for that which documents the misleading nature of it, so please ignore that. As for the tabpaint, is there more data you need?

Just pushed two new try with patches being rebased. It no longer report any regression for the main patch (attachment 8784373 [details] [diff] [review]). And still confirm a win with the second patch that has nothing to do with this bug. https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx-team&originalRevision=cfdb7af3af2e&newProject=try&newRevision=e5e8ad106067&framework=1&filter=tabpaint&showOnlyImportant=0 https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx-team&originalRevision=cfdb7af3af2e&newProject=try&newRevision=3332f63169be&framework=1 Joel, could you confirm that I ran every important test on talos convering firefox startup? And that the test results looks green to you? I'm a bit confused the first run reported tabpaint regression and this one doesn't. Also, what do you think about the second patch, there is around 2 to 4% wins in various tests. Is that significant? Is it worth spending time to get such win?

yes, i see all the tests ran for linux64/win7. We have winxp/win8/osx10.10 to potentially run tests on, but these two platforms is a great representations of issues to see. I do see a nice set of wins in the second patch, since it is on many tests and between the two platforms, I would be excited to see this land- it might not be worth 20+ hours of work to make it work without failure.

Comment on attachment 8785984 [details] [diff] [review] Stop loading various useless SDK modules on early devtools startup. Opened bug 1302996 to followup on this.

Comment on attachment 8791559 [details] Bug 1297535 - Register devtools menu and keys on DOMContentLoaded to happen before CustomizableUI. https://reviewboard.mozilla.org/r/78962/#review77588 ::: devtools/client/framework/devtools-browser.js:146 (Diff revision 1) > } > } > break; > + case "domwindowopened": > + let win = subject.QueryInterface(Ci.nsIDOMEventTarget); > + win.addEventListener("DOMContentLoaded", this); Assuming this doesn't need to be uplifted beyond 50, we should be able to use the new 'once' option on addEventListener to avoid removing it later on: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#Parameters ::: devtools/client/framework/devtools-browser.js:419 (Diff revision 1) > + * > + * @param {ChromeWindow} window > + * The window to which devtools should be hooked to. > + */ > + _onBrowserWindowLoaded: function (win) { > + if (!win.gBrowser) { In registerBrowserWindow we had a guard against ever adding the menus twice to the same document. I don't think it's possible here but can you think of a case where it could happen?

(In reply to Brian Grinstead [:bgrins] from comment #25) > Comment on attachment 8791559 [details] > Bug 1297535 - Register devtools menu and keys on DOMContentLoaded to happen > before CustomizableUI. > > https://reviewboard.mozilla.org/r/78962/#review77588 > > ::: devtools/client/framework/devtools-browser.js:146 > (Diff revision 1) > > } > > } > > break; > > + case "domwindowopened": > > + let win = subject.QueryInterface(Ci.nsIDOMEventTarget); > > + win.addEventListener("DOMContentLoaded", this); > > Assuming this doesn't need to be uplifted beyond 50, we should be able to > use the new 'once' option on addEventListener to avoid removing it later on: > https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/ > addEventListener#Parameters done. > > ::: devtools/client/framework/devtools-browser.js:419 > (Diff revision 1) > > + * > > + * @param {ChromeWindow} window > > + * The window to which devtools should be hooked to. > > + */ > > + _onBrowserWindowLoaded: function (win) { > > + if (!win.gBrowser) { > > In registerBrowserWindow we had a guard against ever adding the menus twice > to the same document. > > I don't think it's possible here but can you think of a case where it could > happen? No, and we would have to introduce a specific guard as we can't reuse the existing one from registerBrowserWindow. The two calls are from this module and are exclusive.

Comment on attachment 8791559 [details] Bug 1297535 - Register devtools menu and keys on DOMContentLoaded to happen before CustomizableUI. https://reviewboard.mozilla.org/r/78962/#review78188

Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/da90ec959a3c Register devtools menu and keys on DOMContentLoaded to happen before CustomizableUI. r=bgrins

Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/29f6034a4ecb Backed out changeset da90ec959a3c for memory leaks in browser_885530_showInPrivateBrowsing.js

Comment on attachment 8794803 [details] fix bug 1297535 I'll merge that into the first changeset, I had to ignore non firefox windows. The DOMContentLoaded codepath was trigerred by some random window spawn by tests and ended up leaking. I tried to put helpful comment, but do not hesitate to suggest new ones or new phrasing.

Comment on attachment 8794803 [details] fix bug 1297535 https://reviewboard.mozilla.org/r/81096/#review80464 ::: devtools/client/framework/devtools-browser.js:421 (Diff revision 1) > * The window to which devtools should be hooked to. > */ > _onBrowserWindowLoaded: function (win) { > - if (!win.gBrowser) { > + // This method is called for all top level window, only consider firefox > + // windows > + if (!win.location.href.endsWith("browser.xul")) { Should we also check `!win.gBrowser || !win.location.href.endsWith("browser.xul")`? Not sure if there's some weird case where some other window will end with browser.xul

Comment on attachment 8794803 [details] fix bug 1297535 https://reviewboard.mozilla.org/r/81096/#review80468 Please re-check try before landing, the original push had a lot of bustage. I just sent up a new push

(In reply to Brian Grinstead [:bgrins] from comment #37) > Comment on attachment 8794803 [details] > fix bug 1297535 > > https://reviewboard.mozilla.org/r/81096/#review80464 > > ::: devtools/client/framework/devtools-browser.js:421 > (Diff revision 1) > > * The window to which devtools should be hooked to. > > */ > > _onBrowserWindowLoaded: function (win) { > > - if (!win.gBrowser) { > > + // This method is called for all top level window, only consider firefox > > + // windows > > + if (!win.location.href.endsWith("browser.xul")) { > > Should we also check `!win.gBrowser || > !win.location.href.endsWith("browser.xul")`? Not sure if there's some weird > case where some other window will end with browser.xul Added that test, merged the two changesets and pushed to try.

Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d271311721bb Register devtools menu and keys on DOMContentLoaded to happen before CustomizableUI. r=bgrins

It looks like the previous patch is still regression Firefox startup (bug 1307365, I requested a backout there). Here is an alternative one, using a completely different path. I just avoid hitting the precise code from CustomizableUI that throws errors and break. (Note that is wasn't just about an error in the console, the tooltip for WebIDE is also wrong on the new window) By removing the "shortcutId" we prevent this issue, but loss the key shortcut displayed in the tooltip, by handling the localization in devtools, we can keep displaying it.

Comment on attachment 8797807 [details] [diff] [review] Localize WebIDE widget from devtools to prevent exception in CustomizableUI. I think :jryans would be a better reviewer for this

Comment on attachment 8797807 [details] [diff] [review] Localize WebIDE widget from devtools to prevent exception in CustomizableUI. Review of attachment 8797807 [details] [diff] [review]: ----------------------------------------------------------------- Seems good for WebIDE, but what about key_devToolboxMenuItem? Don't you need a change for it as well?

Right, the same happens if you move the developer menu from the hamburger popup to the navbar, I'll also tweak it then.

Sorry for not using mozreview, but with this previous changeset that isn't backedout but which is meant to be backed out... I can't make mozreview accept another mozreview request for this bug :/ So, I removed all l10n keys from browser/ regarding customizableUI widgets (also the labels) and did that for both items (webide and devtools).

Comment on attachment 8799352 [details] [diff] [review] Localize devtools widgets from devtools to prevent exception in CustomizableUI. Review of attachment 8799352 [details] [diff] [review]: ----------------------------------------------------------------- (Not sure I follow the issues you're having with MozReview, we should talk about it! Maybe it's related to the single patch queue approach you are using? You should be able to `git mozreview push X` to push specific commits no matter what, I would think.) Seems to fix the issue, but I have some questions about the shortcut text. ::: devtools/client/locales/en-US/menus.properties @@ +59,5 @@ > webide.key = VK_F8 > webide.keytext = F8 > +# LOCALIZATION NOTE (webide.widget.tooltiptext): This is the label for the icon > +# in the navigation bar that is added only after you already opened WebIDE. > +webide.widget.tooltiptext = Open WebIDE (Shift+F8) This is currently shown (at least on macOS) with a shift arrow icon. Is there any way to continue computing this as before? @@ +69,5 @@ > +devtools.widget.label = Developer > +# LOCALIZATION NOTE (devtools.widget.tooltiptext): This is the label for the icon > +# when you customize and move the Developer item from the hamburger menu to the > +# navigation bar. > +devtools.widget.tooltiptext = Open Web developer tools (Ctrl+Shift+I) Hmm, so here's a place where this plan is complicated since we don't use the same shortcut on all OSes (toggle toolbox is Cmd-Opt-I on Mac). It currently gets replaced with the modifier key symbols, like "Open Web developer tools (⌥⌘I)". At the same time, if you actually do use this item in the navigation bar, clicking it doesn't open the tools, it shows the Developer menu. So, why do we list the toggle tools shortcut here at all...? I guess that is the standard practice though...? It looks like other submenu widgets like history and bookmarks list the main shortcut for their category as the tooltip text of the menu's button like we are doing now.

Backed out the patch for developer's request, https://bugzilla.mozilla.org/show_bug.cgi?id=1307365#c8

Backout by ihsiao@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/2976adccde9b Backed out changeset d271311721bb for developer's request

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #48) > Comment on attachment 8799352 [details] [diff] [review] > Localize devtools widgets from devtools to prevent exception in > CustomizableUI. > > Review of attachment 8799352 [details] [diff] [review]: > ----------------------------------------------------------------- > > (Not sure I follow the issues you're having with MozReview, we should talk > about it! Maybe it's related to the single patch queue approach you are > using? You should be able to `git mozreview push X` to push specific > commits no matter what, I would think.) It says "Parent review request is submitted or discarded". I tried erasing the mozreview id that is in the commit message, but it doesn't change anything. It isn't related to the single patch queue, I'm now cherry-picking patches in a dedicated branch before pushing to mozreview. Here the existing mozreview was still with the previous "to be backed out" changeset. I didn't wanted to push to that branch as I want only my new changeset to be visible. Also I wanted to just have a new mozreview in order to keep the previous review as-is for the archive. It looks like we can't have more than one mozreview per bug? > ::: devtools/client/locales/en-US/menus.properties > @@ +59,5 @@ > > webide.key = VK_F8 > > webide.keytext = F8 > > +# LOCALIZATION NOTE (webide.widget.tooltiptext): This is the label for the icon > > +# in the navigation bar that is added only after you already opened WebIDE. > > +webide.widget.tooltiptext = Open WebIDE (Shift+F8) > > This is currently shown (at least on macOS) with a shift arrow icon. Is > there any way to continue computing this as before? Shift arrow icon?? What is that? In the navigation bar? We should have the worl icon with a pencil on windows/linux. And a regular tooltip when hovering with this text. > @@ +69,5 @@ > > +devtools.widget.label = Developer > > +# LOCALIZATION NOTE (devtools.widget.tooltiptext): This is the label for the icon > > +# when you customize and move the Developer item from the hamburger menu to the > > +# navigation bar. > > +devtools.widget.tooltiptext = Open Web developer tools (Ctrl+Shift+I) > > Hmm, so here's a place where this plan is complicated since we don't use the > same shortcut on all OSes (toggle toolbox is Cmd-Opt-I on Mac). It > currently gets replaced with the modifier key symbols, like "Open Web > developer tools (⌥⌘I)". We already have KeyShortcuts.stringify. We are already using it for just one key shortcut (minimize the toolbox, which is a hidden feature, still). It stringify to the electron syntax so it isn't as nice nor localized. > At the same time, if you actually do use this item in the navigation bar, > clicking it doesn't open the tools, it shows the Developer menu. So, why do > we list the toggle tools shortcut here at all...? Yes it doesn't sound very consistent. Especially given that the first item of the menu is that precise command (toggle tools). So it should be somewhat easy to discover this shortcut. > I guess that is the standard practice though...? It looks like other > submenu widgets like history and bookmarks list the main shortcut for their > category as the tooltip text of the menu's button like we are doing now. Listing Bookmarks is somewhar similar, you get a different behavior between clicking on the icon and hitting the shortcut. (open menu versus a window) I really don't want to spend time optimizing something that can't be much faster (previous patch). So I would like to figure out something else. This new patch or yet another approach. I see two tweaks to the current patch: * drop the shortcut on developer button. * display the shortcut by passing it through KeyShortcuts.stringify * display the shortcut by passing it through KeyShortcuts.stringify and improve it to have a localized string.

(In reply to Alexandre Poirot [:ochameau] from comment #51) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #48) > > Comment on attachment 8799352 [details] [diff] [review] > > Localize devtools widgets from devtools to prevent exception in > > CustomizableUI. > > > > Review of attachment 8799352 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > (Not sure I follow the issues you're having with MozReview, we should talk > > about it! Maybe it's related to the single patch queue approach you are > > using? You should be able to `git mozreview push X` to push specific > > commits no matter what, I would think.) > > It says "Parent review request is submitted or discarded". > I tried erasing the mozreview id that is in the commit message, but it > doesn't change anything. > It isn't related to the single patch queue, I'm now cherry-picking patches > in a dedicated branch before pushing to mozreview. > Here the existing mozreview was still with the previous "to be backed out" > changeset. I didn't wanted to push to that branch as I want only my new > changeset to be visible. Also I wanted to just have a new mozreview in order > to keep the previous review as-is for the archive. > It looks like we can't have more than one mozreview per bug? There is an assumption of one active MozReview session per bug currently, so it either creates one if there's none or updates with new patches if one exists. To start a totally separate review session, I guess the simplest path would be to go to the old session and discard it first, like when rebasing. It's not deleted, it just gets "closed", and there are still comments in the bug that link to it for history. > > ::: devtools/client/locales/en-US/menus.properties > > @@ +59,5 @@ > > > webide.key = VK_F8 > > > webide.keytext = F8 > > > +# LOCALIZATION NOTE (webide.widget.tooltiptext): This is the label for the icon > > > +# in the navigation bar that is added only after you already opened WebIDE. > > > +webide.widget.tooltiptext = Open WebIDE (Shift+F8) > > > > This is currently shown (at least on macOS) with a shift arrow icon. Is > > there any way to continue computing this as before? > > Shift arrow icon?? What is that? In the navigation bar? > We should have the worl icon with a pencil on windows/linux. And a regular > tooltip when hovering with this text. Sorry, I was talking about in the tooltip text, to represent the key combo. Instead of the word "Shift", there's an up arrow icon. > > @@ +69,5 @@ > > > +devtools.widget.label = Developer > > > +# LOCALIZATION NOTE (devtools.widget.tooltiptext): This is the label for the icon > > > +# when you customize and move the Developer item from the hamburger menu to the > > > +# navigation bar. > > > +devtools.widget.tooltiptext = Open Web developer tools (Ctrl+Shift+I) > > > > Hmm, so here's a place where this plan is complicated since we don't use the > > same shortcut on all OSes (toggle toolbox is Cmd-Opt-I on Mac). It > > currently gets replaced with the modifier key symbols, like "Open Web > > developer tools (⌥⌘I)". > > We already have KeyShortcuts.stringify. We are already using it for just one > key shortcut (minimize the toolbox, which is a hidden feature, still). It > stringify to the electron syntax so it isn't as nice nor localized. > > > At the same time, if you actually do use this item in the navigation bar, > > clicking it doesn't open the tools, it shows the Developer menu. So, why do > > we list the toggle tools shortcut here at all...? > > Yes it doesn't sound very consistent. Especially given that the first item > of the menu is that precise command (toggle tools). So it should be somewhat > easy to discover this shortcut. > > > I guess that is the standard practice though...? It looks like other > > submenu widgets like history and bookmarks list the main shortcut for their > > category as the tooltip text of the menu's button like we are doing now. > > Listing Bookmarks is somewhar similar, you get a different behavior between > clicking on the icon and hitting the shortcut. (open menu versus a window) > > > I really don't want to spend time optimizing something that can't be much > faster (previous patch). > So I would like to figure out something else. This new patch or yet another > approach. > I see two tweaks to the current patch: > * drop the shortcut on developer button. > * display the shortcut by passing it through KeyShortcuts.stringify > * display the shortcut by passing it through KeyShortcuts.stringify and > improve it to have a localized string. I guess let's try to display the shortcut for now, to avoid a behavior change here. Was the shortcut not localized before?

On Windows and linux customizable ui shortcuts in tooltips are localized. It isn't using icons which is a nice way to not translate strings...

This started happening a lot to me very recently. @ochameau, have there been any recent changes to webide?

I imagine that's because you switched to DevEdition, or moved one of the devtools icon into the customizable navbar (devtools wrench or webide) and you are opening more than one top level window.

The leave-open keyword is there and there is no activity for 6 months. :ochameau, maybe it's time to close this bug?