1353042 - Get rid of sdk/tabs and sdk/tabs/utils

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P1)

Description

[Patrick Brosset <:pbro>]

In bug 1350645 we're trying to stop using certain SDK APIs, 2 of those are sdk/tabs and sdk/tabs/utils.

Here are the occurrences in our codebase:

devtools/client/responsive.html/manager.js
devtools/shared/gcli/command-state.js
devtools/client/debugger/test/mochitest/browser_dbg_event-listeners-04.js
devtools/client/performance/test/helpers/tab-utils.js
devtools/client/responsive.html/test/browser/browser_menu_item_01.js
devtools/client/responsive.html/test/browser/head.js
devtools/shared/tests/mochitest/test_devtools_extensions.html

Comment 1

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

The 3 usages in RDM were cleaned up as part of bug 1353045.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: Bug 1353042 - Get rid of all sdk/tabs and sdk/tabs/utils usage;

Removed obsolete browser_dbg_event-listeners-04.js test (it is for the
old debugger, and the new debugger doesn't even support event listeners
yet, so no use keeping this test around).

Removed getBrowserForTab SDK dependency from command-state.js.

Changed the implementation of addTab/removeTab in perf test's tab-utils
helper.

Review commit: https://reviewboard.mozilla.org/r/135732/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135732/

Comment 4

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8864019 [details]
Bug 1353042 - Get rid of all sdk/tabs and sdk/tabs/utils usage;

https://reviewboard.mozilla.org/r/135732/#review141450

::: devtools/client/performance/test/helpers/tab-utils.js:31
(Diff revision 1)
>    let id = getRandomInt(0, Number.MAX_SAFE_INTEGER - 1);
>    url += `#${id}`;
>  
>    dump(`Adding tab with url: ${url}.\n`);
>  
>    return new Promise(resolve => {

You could probably replace all this block, from line 31 to 43, with a call to `BrowserTestUtils.openNewForegroundTab`. Something like:

```js
let { gBrowser } = win || window;

return BrowserTestUtils.openNewForegroundTab(gBrowser, url, !options.dontWaitForTabReady);
```

It should be equivalent.

::: devtools/client/performance/test/helpers/tab-utils.js:52
(Diff revision 1)
>   * Removes a browser tab from the specified window and waits for it to close.
>   */
>  exports.removeTab = function (tab, options = {}) {
> -  dump(`Removing tab: ${tabUtils.getURI(tab)}.\n`);
> +  dump(`Removing tab: ${tab.linkedBrowser.currentURI.spec}.\n`);
>  
>    return new Promise(resolve => {

You could probably use the `BrowserTestUtils.removeTab` here, since the code seems safer; however it has a slightly different logic from ours, so instead of returns its promise, we probably need something like:

```js
return new Promise(resolve => {
  // we can simplify using `ownerGlobal` instead
  let { gBrowser } = tab.ownerGlobal;
  
  // BrowserTestUtils.removeTab doesn't pass the `tab` 
  BrowserTestUtils.removeTab(tab).then(()=> resolve(tab));
  
  if (options.dontWaitForTabClose) {
    resolve(tab);
  }
});
```

::: devtools/shared/tests/mochitest/test_devtools_extensions.html:54
(Diff revision 1)
>            include: TEST_URL,
>            contentScriptWhen: 'ready',
>            contentScript: 'null;'
>          });
>  
> -        tabs.open({
> +        let { gBrowser } = getMostRecentBrowserWindow();

I think this test might benefit from the usage of `BrowserTestUtils.withNewTab` method instead of manually open the tab and close it: it's better for racing condition (I solved a couple of them in a test with that), plus you won't have to use `getMostRecentBrowserWindow` because it's implicit.

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8864019 [details]
Bug 1353042 - Get rid of all sdk/tabs and sdk/tabs/utils usage;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135732/diff/1-2/

Comment 6

[Patrick Brosset <:pbro>]

This fell off my radar somehow.
Here's a new try build to make sure this patch still works: https://treeherder.mozilla.org/#/jobs?repo=try&revision=097baa232895ddfd70111dcb4b097a523a773536&group_state=expanded

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8864019 [details]
Bug 1353042 - Get rid of all sdk/tabs and sdk/tabs/utils usage;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135732/diff/2-3/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c2036f901c1
Get rid of all sdk/tabs and sdk/tabs/utils usage; r=zer0

Comment 9

[Iris Hsiao [:ihsiao]]

Backed out for build bustage in Symlink target path does not exist: doc_event-listeners-04.html like https://treeherder.mozilla.org/logviewer.html#?job_id=101151277&repo=autoland&lineNumber=19714

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f499580389b
Backed out changeset 4c2036f901c1 for build bustage in Symlink target path does not exist: doc_event-listeners-04.html

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8864019 [details]
Bug 1353042 - Get rid of all sdk/tabs and sdk/tabs/utils usage;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135732/diff/3-4/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8e521edbe83
Get rid of all sdk/tabs and sdk/tabs/utils usage; r=zer0

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b8e521edbe83