Closed Bug 1378821 Opened 4 years ago Closed 4 years ago

Stop using sdk/window/utils in DevTools

Categories

(DevTools :: General, enhancement, P1)

enhancement

Tracking

(firefox56 fixed)

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: zer0, Assigned: Honza)

References

Details

(Whiteboard: [nosdk])

Attachments

(1 file)

Used in:

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

devtools/shared/gcli/commands/rulers.js
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Flags: qe-verify-
Priority: -- → P3
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".
Priority: P3 → P1
Whiteboard: [nosdk]
Target Milestone: --- → Firefox 56
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
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6a297d875e0e
Stop using sdk/window/utils in DevTools; r=zer0
https://hg.mozilla.org/mozilla-central/rev/6a297d875e0e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.