Stop using sdk/window/utils in DevTools

RESOLVED FIXED in Firefox 56

Status

P1
normal
RESOLVED FIXED
a year ago
6 months ago

People

(Reporter: zer0, Assigned: Honza)

Tracking

unspecified
Firefox 56
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [nosdk])

Attachments

(1 attachment)

Used in:

devtools/client/shared/doorhanger.js
devtools/shared/worker/worker.js
Used also in:

devtools/shared/gcli/commands/rulers.js
Comment hidden (mozreview-request)
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Flags: qe-verify-
Priority: -- → P3
Comment hidden (mozreview-request)
(Reporter)

Comment 4

a year ago
mozreview-review
Comment on attachment 8884227 [details]
Bug 1378821 - Stop using sdk/window/utils in DevTools;

https://reviewboard.mozilla.org/r/155180/#review160240

r+ but the comments addressed, since this code could behave differently from the original.

::: devtools/client/shared/doorhanger.js:153
(Diff revision 2)
>  
>    return promise;
>  }
>  
>  function getGBrowser() {
> -  return getMostRecentBrowserWindow().gBrowser;
> +  return Services.wm.getMostRecentWindow(gDevTools.chromeWindowType).gBrowser;

I'm not sure I would add the whole framework/devtools module, and the gDevTools just for a string here. It's okay to use if we already require that, but I think here we can just use `Services.wm.getMostRecentWindow("navigator:browser")` as in most of the firefox's codebase seems to be done – I was looking if Services or wm exposed this string as constant, but basically it's plain almost everywhere.

Unless we don't want "navigator:browser" all the times: `chromeWindowType` seems to be a property that other gecko apps can override: http://searchfox.org/mozilla-central/source/devtools/client/framework/devtools.js#70-72

So, what we want here exactly? If we want to have the previous behavior, it should be "navigator:browser", otherwise it means the previous behavior was wrong all long, and we should take this occasion to fix it, and use `chromeWindowType`.

Here it seems to me that we want to have the Global Browser, so I suggest to use:
```js
Services.wm.getMostRecentWindow("navigator:browser")
```

::: devtools/shared/worker/worker.js:146
(Diff revision 2)
>                   "used in production.");
> -    // Fetch via window/utils here as we don't want to include
> -    // this module normally.
> -    let { getMostRecentBrowserWindow } = require("sdk/window/utils");
> -    let { URL, Blob } = getMostRecentBrowserWindow();
> +    // Fetch modules here as we don't want to include it normally.
> +    const Services = require("Services");
> +    const { gDevTools } = require("devtools/client/framework/devtools");
> +
> +    let { URL, Blob } = Services.wm.getMostRecentWindow(gDevTools.chromeWindowType);

Same here. If we want to address the `chromeWindowType`, that *might* not be the browser window, it's OK (however, in that case I'm suggesting to implement in `gDevTools` a method to do so, like `gDevTools.getMostRecentChromeWindow()`); otherwise if we want the browser window, we should use directly "navigator:browser".
Comment hidden (mozreview-request)
Priority: P3 → P1
Whiteboard: [nosdk]
Target Milestone: --- → Firefox 56
(Reporter)

Comment 6

a year ago
mozreview-review
Comment on attachment 8884227 [details]
Bug 1378821 - Stop using sdk/window/utils in DevTools;

https://reviewboard.mozilla.org/r/155180/#review163488

Looks good to me! I just noticed that we have an occurrence here to: http://searchfox.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/damp.js#7 maybe we want to address that in this patch in the same way you've done for the other files (for sd/window/utils, it just uses `getMostRecentBrowserWindow`).
Attachment #8884227 - Flags: review?(zer0) → review+
I added r+ since the changes to damp.js would be the same of the others, so no review required. If you do not address the issue in this patch for some reasons, we should file a follow up bug.
(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #6)
> Comment on attachment 8884227 [details]
> Bug 1378821 - Stop using sdk/window/utils in DevTools;
> 
> https://reviewboard.mozilla.org/r/155180/#review163488
> 
> Looks good to me! I just noticed that we have an occurrence here to:
> http://searchfox.org/mozilla-central/source/testing/talos/talos/tests/
> devtools/addon/content/damp.js#7 maybe we want to address that in this patch
> in the same way you've done for the other files (for sd/window/utils, it
> just uses `getMostRecentBrowserWindow`).

There's a different bug on file for getting rid of SDK usage in DAMP: Bug 1364596
(In reply to Brian Grinstead [:bgrins] from comment #8)
> There's a different bug on file for getting rid of SDK usage in DAMP: Bug
> 1364596
Thanks, let's solve the issue in this bug.

Honza

Comment 10

a year ago
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6a297d875e0e
Stop using sdk/window/utils in DevTools; r=zer0

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6a297d875e0e
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.