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

RESOLVED FIXED in Firefox 55

Status

enhancement
P1
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: pbro, Assigned: pbro)

Tracking

unspecified
Firefox 55
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [nosdk] [qf-])

Attachments

(1 attachment)

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
Priority: -- → P2
Whiteboard: [qf]
Whiteboard: [qf] → [qf-]
The 3 usages in RDM were cleaned up as part of bug 1353045.
Depends on: 1353045
Comment hidden (mozreview-request)
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Duplicate of this bug: 1353043

Comment 4

2 years ago
mozreview-review
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.
Attachment #8864019 - Flags: review?(zer0) → review+
Comment hidden (mozreview-request)
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 hidden (mozreview-request)

Comment 8

2 years ago
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4c2036f901c1
Get rid of all sdk/tabs and sdk/tabs/utils usage; r=zer0
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
Flags: needinfo?(pbrosset)

Comment 10

2 years ago
Backout by ihsiao@mozilla.com:
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 hidden (mozreview-request)

Comment 12

2 years ago
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b8e521edbe83
Get rid of all sdk/tabs and sdk/tabs/utils usage; r=zer0
Flags: needinfo?(pbrosset)

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b8e521edbe83
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Flags: qe-verify-
Priority: P2 → P1
Whiteboard: [qf-] → [nosdk] [qf-]

Updated

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