1248348 - Dynamically register into browser UI instead of hardcoding devtools specifics into browser/

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[Alexandre Poirot [:ochameau]]

We moved almost all devtools codebase to devtools/, but we still have various hardcoded things into browser/:
 * menuitems
 * shortcuts
 * context menu
 * top level window (browser.xul) watching

This has multiple issues:
 * we still have to modify /browser/ whereas 99% of the codebase lives in /devtools/
 * this prevent reloading devtools easily as all this is directly hardcoded in file packaged in firefox. All this hardcoded things uses Loader.jsm and ends up using the very first instance of it.

Instead, we should be registering all that dynamically. We could use our existing helpers, or use something from WebExtensions, SDK, ...

It would also open ways to completely disable devtools and ship them easily as an addon. If we wanted to. But that's not the goal of this bug.

Comment 1

[Alexandre Poirot [:ochameau]]

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

Comment 2

[Alexandre Poirot [:ochameau]]

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

Comment 3

[Philip Chee]

(In reply to Alexandre Poirot [:ochameau] from comment #0)
> We moved almost all devtools codebase to devtools/, but we still have
> various hardcoded things into browser/:
>  * menuitems
>  * shortcuts
>  * context menu
>  * top level window (browser.xul) watching
> 
> This has multiple issues:
>  * we still have to modify /browser/ whereas 99% of the codebase lives in
> /devtools/
>  * this prevent reloading devtools easily as all this is directly hardcoded
> in file packaged in firefox. All this hardcoded things uses Loader.jsm and
> ends up using the very first instance of it.

Our strategy in SeaMonkey is to isolate all DevTools functionality into an overlay:
http://mxr.mozilla.org/comm-central/find?text=&string=webDeveloperOverlay

> Instead, we should be registering all that dynamically. We could use our
> existing helpers, or use something from WebExtensions, SDK, ...
> 
> It would also open ways to completely disable devtools and ship them easily
> as an addon. If we wanted to. But that's not the goal of this bug.

You can do that if you move all the items from browser.xul into an overlay and put the overlay into the addon.

Comment 4

[Alexandre Poirot [:ochameau]]

Overlay are not unloadable. We can't use that as we have to remove and readd the entries in order to possibly change them in between. That's the whole point of the reload addon.

Having said that, I'm open to help you support seamonkey through this change.
I'm convince this change is going to help anyway.

Comment 5

[Alexandre Poirot [:ochameau]]

By the way, I was about to ask you about "appmenu_webDeveloper_popup". Is this something used in Seamonkey? Or is this just an historical relic?

It is being used over here:
http://mxr.mozilla.org/mozilla-central/source/devtools/client/framework/devtools-browser.js#528
But there is no other occurence of this thing anywhere else.
You recently touched that code in bug 1223344.

Comment 6

[Alexandre Poirot [:ochameau]]

Please also have a look at bug 1248603 where I'm about to change menuitem, key, broadcaster, ...

Comment 7

[Philip Chee]

(In reply to Alexandre Poirot [:ochameau] from comment #6)
> Please also have a look at bug 1248603 where I'm about to change menuitem,
> key, broadcaster, ...

In general I think your points are good. Only problem is that some of our webdev access keys are different due to clashes with existing menu items. And I've removed one or two menu items (Toolkit Error Console) that are duplicated by existing menu items elsewhere.

(In reply to Alexandre Poirot [:ochameau] from comment #5)
> By the way, I was about to ask you about "appmenu_webDeveloper_popup". Is
> this something used in Seamonkey? Or is this just an historical relic?
> 
> It is being used over here:
> http://mxr.mozilla.org/mozilla-central/source/devtools/client/framework/
> devtools-browser.js#528
> But there is no other occurence of this thing anywhere else.
> You recently touched that code in bug 1223344.

Firefox used to have a "app menu" widget which more or less mirrored the existing menubar menuitems and sub-menus. I *think* that's gone away and replaced by the "hamburger" menu.

Also my reviewer wants to get rid of the broadcasters because (he thinks) the only reason for their existence was the app menu items that duplicate the menu items in the menu toolbar. e.g. Tools -> Webdev vs AppMenu -> Tools -> Webdev

There is a addon to restore the app menu:
Screenshot: https://addons.mozilla.org/en-us/firefox/addon/personal-menu/

Comment 8

[Alexandre Poirot [:ochameau]]

(In reply to Philip Chee from comment #7)
> (In reply to Alexandre Poirot [:ochameau] from comment #6)
> > Please also have a look at bug 1248603 where I'm about to change menuitem,
> > key, broadcaster, ...
> 
> In general I think your points are good. Only problem is that some of our
> webdev access keys are different due to clashes with existing menu items.
> And I've removed one or two menu items (Toolkit Error Console) that are
> duplicated by existing menu items elsewhere.

Ah. Hum. What about overriding menus.properties to overload the accesskeys?
  chrome://devtools/locale/menus.properties
  I introdude this new properties file in bug 1248603.
If that doesn't work, we can surely find some other way to allow you to customize accesskey.
Do you think that's the only thing you would need regarding menuitems and shortcuts?

> Firefox used to have a "app menu" widget which more or less mirrored the
> existing menubar menuitems and sub-menus. I *think* that's gone away and
> replaced by the "hamburger" menu.

Ah ok. Makes sense. I'll try to remove that then. The personal-menu addon doesn't seem to reintroduce this appmenu_webDeveloper_popup element anyway.

> Also my reviewer wants to get rid of the broadcasters because (he thinks)
> the only reason for their existence was the app menu items that duplicate
> the menu items in the menu toolbar. e.g. Tools -> Webdev vs AppMenu -> Tools
> -> Webdev

I kept them for compatiblity with addons. Otherwise I may not even have used xul:command.

Comment 9

[Philip Chee]

(In reply to Alexandre Poirot [:ochameau] from comment #8)

> Ah. Hum. What about overriding menus.properties to overload the accesskeys?
>   chrome://devtools/locale/menus.properties
>   I introdude this new properties file in bug 1248603.

That's a good idea. Temporary clashing access keys shouldn't block anything.

> If that doesn't work, we can surely find some other way to allow you to
> customize accesskey.
> Do you think that's the only thing you would need regarding menuitems and
> shortcuts?

Comment 10

[Patrick Brosset <:pbro>]

I talked to Alex just now and found that:
1) This bug is no longer a priority.
2) We ended up completing this work. The only dependent bug about context menu has been fixed by  https://bugzilla.mozilla.org/show_bug.cgi?id=1372520

So I'm going to close this bug now.