Closed
Bug 1248603
Opened 8 years ago
Closed 8 years ago
Add top level menuitems dynamically
Categories
(DevTools :: General, defect, P3)
DevTools
General
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.
Assignee | ||
Comment 1•8 years ago
|
||
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.
Assignee | ||
Comment 2•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f9dbb0c9a70
Assignee | ||
Comment 3•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=962c1d178a3b
Assignee | ||
Comment 4•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4724ce86906
Assignee | ||
Comment 5•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d98a961384f
Assignee | ||
Comment 6•8 years ago
|
||
Assignee | ||
Comment 7•8 years ago
|
||
Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6989b13865cb
Assignee | ||
Comment 8•8 years ago
|
||
Preliminary patch that is going to help.
Assignee | ||
Updated•8 years ago
|
Attachment #8720006 -
Attachment is obsolete: true
Attachment #8722929 -
Attachment is obsolete: true
Assignee | ||
Comment 9•8 years ago
|
||
The main part of the patch. I still need to verify xul actual content before/after these patches.
Assignee | ||
Comment 10•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b3069485624
Assignee | ||
Comment 11•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Attachment #8723604 -
Attachment is obsolete: true
Assignee | ||
Comment 12•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Attachment #8723605 -
Attachment is obsolete: true
Assignee | ||
Comment 13•8 years ago
|
||
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.
Assignee | ||
Comment 14•8 years ago
|
||
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.
Updated•8 years ago
|
Flags: needinfo?(philip.chee)
Assignee | ||
Comment 15•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=569901fa5c7d
Assignee | ||
Comment 16•8 years ago
|
||
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)
Assignee | ||
Comment 17•8 years ago
|
||
Do not forget to look at bug comments, I explained various things/tests I made.
Attachment #8726946 -
Flags: feedback?(jryans)
Assignee | ||
Updated•8 years ago
|
Attachment #8724771 -
Attachment is obsolete: true
Assignee | ||
Comment 18•8 years ago
|
||
Attachment #8726947 -
Flags: feedback?(jryans)
Assignee | ||
Updated•8 years ago
|
Attachment #8724773 -
Attachment is obsolete: true
Assignee | ||
Comment 19•8 years ago
|
||
Attachment #8726948 -
Flags: feedback?(jryans)
Assignee | ||
Updated•8 years ago
|
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 #8726948 -
Flags: feedback?(jryans) → feedback+
Assignee | ||
Updated•8 years ago
|
Attachment #8724767 -
Attachment is obsolete: true
Assignee | ||
Comment 24•8 years ago
|
||
(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/.
Assignee | ||
Comment 25•8 years ago
|
||
(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.
Assignee | ||
Comment 26•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d14a2d5efd2e
Assignee | ||
Comment 27•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=07b5a1eb5a6a
Assignee | ||
Comment 28•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b21cf66c0637
Assignee | ||
Comment 29•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8726944 -
Attachment is obsolete: true
Assignee | ||
Comment 30•8 years ago
|
||
Attachment #8731700 -
Flags: review?(jryans)
Assignee | ||
Updated•8 years ago
|
Attachment #8726946 -
Attachment is obsolete: true
Assignee | ||
Comment 31•8 years ago
|
||
Attachment #8731701 -
Flags: review?(jryans)
Assignee | ||
Updated•8 years ago
|
Attachment #8726947 -
Attachment is obsolete: true
Assignee | ||
Comment 32•8 years ago
|
||
Attachment #8731702 -
Flags: review?(jryans)
Assignee | ||
Updated•8 years ago
|
Attachment #8726948 -
Attachment is obsolete: true
Assignee | ||
Comment 33•8 years ago
|
||
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)
Assignee | ||
Comment 34•8 years ago
|
||
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)
Attachment #8731699 -
Flags: review?(jryans) → review+
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)
Attachment #8731701 -
Flags: review?(jryans) → review+
Attachment #8731702 -
Flags: review?(jryans) → review+
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 38•8 years ago
|
||
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 39•8 years ago
|
||
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 40•8 years ago
|
||
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.
Assignee | ||
Comment 41•8 years ago
|
||
(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.
Assignee | ||
Comment 42•8 years ago
|
||
(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.
Assignee | ||
Comment 43•8 years ago
|
||
Arf the comment was sent while I was editing it... More reply and patches to come.
Comment 44•8 years ago
|
||
(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 .
Assignee | ||
Updated•8 years ago
|
Attachment #8731699 -
Attachment is obsolete: true
Assignee | ||
Comment 46•8 years ago
|
||
Rebased, applying it before "Register devtools menuitems dynamically" patch.
Attachment #8732919 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8731703 -
Attachment is obsolete: true
Assignee | ||
Comment 47•8 years ago
|
||
(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)
Assignee | ||
Updated•8 years ago
|
Attachment #8731700 -
Attachment is obsolete: true
Assignee | ||
Comment 48•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8731701 -
Attachment is obsolete: true
Assignee | ||
Comment 49•8 years ago
|
||
(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
Assignee | ||
Comment 50•8 years ago
|
||
(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 51•8 years ago
|
||
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 52•8 years ago
|
||
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+
Assignee | ||
Comment 53•8 years ago
|
||
Just posted about getting rid of the old XUL Error Console: https://groups.google.com/forum/#!topic/mozilla.dev.developer-tools/XYPqQ58ndX4
Comment 54•8 years ago
|
||
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)
Assignee | ||
Comment 55•8 years ago
|
||
(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.
Assignee | ||
Comment 56•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c70499331d52
Assignee | ||
Comment 57•8 years ago
|
||
Interdiff on top of attachment 8732922 [details] [diff] [review].
Assignee | ||
Comment 58•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8732922 -
Attachment is obsolete: true
Assignee | ||
Comment 59•8 years ago
|
||
The removal of xul:command forces me to update code in order to toggle the menuitems instead.
Attachment #8733462 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 60•8 years ago
|
||
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 61•8 years ago
|
||
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 62•8 years ago
|
||
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+
Assignee | ||
Comment 63•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=97d3de3c5f6c
Assignee | ||
Comment 64•8 years ago
|
||
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_
Assignee | ||
Updated•8 years ago
|
Keywords: addon-compat
Assignee | ||
Comment 65•8 years ago
|
||
Attachment #8733829 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8733457 -
Attachment is obsolete: true
Attachment #8733461 -
Attachment is obsolete: true
Assignee | ||
Comment 66•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8733462 -
Attachment is obsolete: true
Assignee | ||
Comment 67•8 years ago
|
||
Forgot the command=";" trick for additionnal keys (F12 for toolbox).
Attachment #8733891 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8733829 -
Attachment is obsolete: true
Assignee | ||
Comment 68•8 years ago
|
||
A few more tweaks are needed to accommodate the disparition of command and broadcasters.
Attachment #8733892 -
Flags: review?(jryans)
Assignee | ||
Updated•8 years ago
|
Attachment #8733830 -
Attachment is obsolete: true
Attachment #8733830 -
Flags: review?(jryans)
Assignee | ||
Comment 69•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=82a220d48add
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+
Assignee | ||
Comment 71•8 years ago
|
||
Merging the two patches while addressing toggleMenuItem and fixing eslint.
Attachment #8733997 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8733891 -
Attachment is obsolete: true
Attachment #8733892 -
Attachment is obsolete: true
Assignee | ||
Comment 72•8 years ago
|
||
Had an issue during the rebase, missing a part.
Attachment #8734012 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8733997 -
Attachment is obsolete: true
Assignee | ||
Comment 73•8 years ago
|
||
(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.
Assignee | ||
Comment 74•8 years ago
|
||
Rebased on top of Responsive Design s/Tool/Mode/ change. Ready to land once trees reopen...
Attachment #8736747 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8734012 -
Attachment is obsolete: true
Assignee | ||
Comment 75•8 years ago
|
||
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
Comment 76•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d03f089bbab8 https://hg.mozilla.org/mozilla-central/rev/836da2d40b19 https://hg.mozilla.org/mozilla-central/rev/0b016f319532 https://hg.mozilla.org/mozilla-central/rev/9b9b8514c6b0 https://hg.mozilla.org/mozilla-central/rev/97e1c3fdf6f2
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•