1279784 - Remove hardcoded navigator:browser in devtools

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement)

Description

[Philipp Kewisch [:Fallen][📱📆]]

devtools uses navigator:browser in the client, it would be great to be able to replace this in a way that Thunderbird can change it.

The following patch does so, with the exception of tests, and the occurrence in devtools-bootstrap.js which also hardcodes a bunch of other Firefox-isms and may not need replacing as we can live without hot reload in Thunderbird.

On the Thunderbird side of things I've made sure to set DebuggerServer.chromeWindowType and gDevTools.chromeWindowType just before the devtools command line handler in its own clh, which works quite well.

Comment 1

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v1

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

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8762376 [details] [diff] [review]
Fix - v1

Review of attachment 8762376 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good overall, thanks for working on it!

I wasn't sure if you'd be able to change the value in time, but it sounds like you've confirmed you can.

Sorry for the review delay!

::: devtools/client/framework/devtools-browser.js
@@ +736,5 @@
>  Services.obs.addObserver(gDevToolsBrowser, "browser-delayed-startup-finished", false);
>  
>  // Fake end of browser window load event for all already opened windows
>  // that is already fully loaded.
> +let enumerator = Services.wm.getEnumerator(gDevTools.chromeWindowType);

This path seems specific to browser windows, right?  I assume Thunderbird windows don't have gBrowserInit?  I guess it's okay to change since it tests for gBrowserInit, but it just seems unlikely to work elsewhere.

::: devtools/client/webconsole/hudservice.js
@@ +1,1 @@
>  /* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */

Feel free to remove these two editor config lines, we don't add them to new files.

::: devtools/client/webide/content/webide.js
@@ +239,5 @@
>    openInBrowser: function (url) {
>      // Open a URL in a Firefox window
> +    let mainWindow = Services.wm.getMostRecentWindow(gDevTools.chromeWindowType);
> +    if (mainWindow) {
> +      let gBrowser = mainWindow.gBrowser;

Is this safe for non-Firefox windows?  Should it test for gBrowser / use a different method instead?  (Seems quite unlikely that you would use WebIDE outside Firefox, but anyway...)

Comment 3

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #2)
> Comment on attachment 8762376 [details] [diff] [review]
> Fix - v1
> 
> Review of attachment 8762376 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good overall, thanks for working on it!
> 
> I wasn't sure if you'd be able to change the value in time, but it sounds
> like you've confirmed you can.
I can, thanks to command line handlers. I've installed a commandline handler that runs before the devtools commandline handler which takes care of the initialization.


> Sorry for the review delay!
No worries, I didn't expect a quick review with the work week in between.


> 
> ::: devtools/client/framework/devtools-browser.js
> @@ +736,5 @@
> >  Services.obs.addObserver(gDevToolsBrowser, "browser-delayed-startup-finished", false);
> >  
> >  // Fake end of browser window load event for all already opened windows
> >  // that is already fully loaded.
> > +let enumerator = Services.wm.getEnumerator(gDevTools.chromeWindowType);
> 
> This path seems specific to browser windows, right?  I assume Thunderbird
> windows don't have gBrowserInit?  I guess it's okay to change since it tests
> for gBrowserInit, but it just seems unlikely to work elsewhere.
I think Seamonkey includes devtools-browser and has gBrowserInit. I am aware some of the changes may only work in Firefox, but I thought it would be good to change it for consistency.


> Feel free to remove these two editor config lines, we don't add them to new
> files.
Done

> 
> ::: devtools/client/webide/content/webide.js
> @@ +239,5 @@
> >    openInBrowser: function (url) {
> >      // Open a URL in a Firefox window
> > +    let mainWindow = Services.wm.getMostRecentWindow(gDevTools.chromeWindowType);
> > +    if (mainWindow) {
> > +      let gBrowser = mainWindow.gBrowser;
> 
> Is this safe for non-Firefox windows?  Should it test for gBrowser / use a
> different method instead?  (Seems quite unlikely that you would use WebIDE
> outside Firefox, but anyway...)
Thunderbird won't be using WebIDE. Nevertheless, I've changed this to check for gBrowser.

Comment 4

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v2

Comment 5

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v3

Actually, I've reverted the webide changes, because this will be taken care of in bug 1279833 anyway.

Comment 6

[Carsten Book [:Tomcat]]

seems there are conflicts

applying devtools-chromeWindowType.diff
patching file devtools/client/netmonitor/netmonitor-view.js
Hunk #1 FAILED at 22
1 out of 2 hunks FAILED -- saving rejects to file devtools/client/netmonitor/netmonitor-view.js.rej
patching file devtools/client/shared/AppCacheUtils.jsm
Hunk #1 FAILED at 25
1 out of 2 hunks FAILED -- saving rejects to file devtools/client/shared/AppCacheUtils.jsm.rej
patching file devtools/client/shared/widgets/MdnDocsWidget.js
Hunk #1 FAILED at 21
1 out of 2 hunks FAILED -- saving rejects to file devtools/client/shared/widgets/MdnDocsWidget.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh devtools-chromeWindowType.diff

Comment 7

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v4

Sorry about that, here is an updated patch. Just some minor conflicts due to added imports.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/6b8a1c9c0251
Remove hardcoded navigator:browser in devtools. r=jryans

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6b8a1c9c0251