Closed Bug 1455573 Opened 2 years ago Closed 2 years ago

The custom order for toolbox tabs created by WebExtensions is not persisted

Categories

(DevTools :: Framework, defect)

defect
Not set

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: pbro, Assigned: daisuke)

References

Details

Attachments

(4 files)

Steps:
- Install a WebExtension that creates a DevTools panel (like the React DevTools https://addons.mozilla.org/en-US/firefox/collections/mozilla/webdeveloper/)
- Open DevTools
- Drag the React tab to a different location
- Close and re-open DevTools

Expected:
The React tab should appear where I moved it

Actual:
It appears as the last tab instead.
We might be running into similar issues as with Bug 1394750 here.
See Also: → 1394750
Testing quickly the order being saved when reordering a webextensions panel:

inspector,
webconsole,
webext-devtools-panel-_ff0cf743-dcf3-4097-ae4c-d872c18f7b4e_-11-0,
jsdebugger,
styleeditor,
netmonitor,
performance,
memory,
storage

After reopening the toolbox the id for the webextensions panel was webext-devtools-panel-_ff0cf743-dcf3-4097-ae4c-d872c18f7b4e_-13-0 

Luca: Can we rely on the pattern `webext-devtools-panel-_${uuid}_-${increment}` that seems to be used here for webextension panel ids ? We could remove the variable part from ids that match this pattern and use the rest to persist the order of webextensions panels?
Flags: needinfo?(lgreco)
(to be clear, in my example above, only "-11-0" was changed to "-13-0" when closing and reopening the toolbox, so I hope we can consider the rest of the ID as stable)
:mikeratcliffe pointed at existing logic in devtools to translate webextension ids:
https://searchfox.org/mozilla-central/source/devtools/client/framework/devtools.js#518

The current implementation became invalid after Bug 1394750 however. We should agree on an API between devtools and webextensions so that our code to read and translate ids remains in sync.
(In reply to Julian Descottes [:jdescottes][:julian] from comment #4)
> :mikeratcliffe pointed at existing logic in devtools to translate
> webextension ids:
> https://searchfox.org/mozilla-central/source/devtools/client/framework/
> devtools.js#518
> 
> The current implementation became invalid after Bug 1394750 however. We
> should agree on an API between devtools and webextensions so that our code
> to read and translate ids remains in sync.

We really need something that is the same across all users installations, identifiable and unique so that we can use them for telemetry.
As briefly discussed over IRC, we can add some additional field to the tool definition registered to the toolbox:

- https://searchfox.org/mozilla-central/rev/11a2ae294f50049e12515b5821f5a396d951aacb/browser/components/extensions/parent/ext-devtools-panels.js#82-86

In particular we may want to add the following information to the tool definition:

- extensionId, by retrieving it from `this.context.extension.id`
- extensionPanelURL by retrieving it from `this.panelOptions.url` (may also be useful for both the "tabs order" and the "telemetry pings", because the tool definition url is actually a xul file, "chrome://browser/content/webext-panels.xul", shared with other extensions ui pages, like the sidebar and the extension popups) 

and then we can use them for the persisted "tabs order", and in the telemetry to identify the extension and the particular panel accessed by the user.

How that sounds?
Flags: needinfo?(lgreco)
Proposal looks great Luca, thanks!
Assigning to :daisuke, let's try to get this in before the soft freeze!
Assignee: nobody → dakatsuka
Status: NEW → ASSIGNED
Comment on attachment 8970493 [details]
Bug 1455573 - Part 2: Correspond to the extension tool tab.

https://reviewboard.mozilla.org/r/239258/#review244950

One issue and a code style nit, but the approach looks good to me!

::: devtools/client/framework/toolbox-tabs-order-manager.js:120
(Diff revision 1)
>        return;
>      }
>  
>      if (this.isOrderUpdated) {
> +      const tabs = [...this.toolboxTabsElement.querySelectorAll(".devtools-tab")];
>        const ids =

Can we split this with intermediary variables, it's a bit hard to follow at the moment: we could first define const overflowedTabs, const overflowedTabIds etc...

::: devtools/client/framework/toolbox-tabs-order-manager.js:126
(Diff revision 1)
> -        [...this.toolboxTabsElement.querySelectorAll(".devtools-tab")]
> -          .map(tabElement => tabElement.dataset.id)
> -          .concat(this.overflowedTabIds);
> +        tabs.map(tab => tab.dataset.extensionId || tab.dataset.id)
> +            // Append overflowed tools.
> +            .concat(this.currentPanelDefinitions.filter(
> +              definition => {
> +                return !tabs.some(tab => tab.dataset.id === definition.id ||
> +                                         tab.dataset.extensionId === definition.id);

this second check should be:

(tab.dataset.extensionId && (tab.dataset.extensionId === definition.extensionId))

This comparison should only be done for tabs that have an extensionId, and we should compare it to extensionId, not to id.

STRs that currently fail:
- install https://addons.mozilla.org/en-US/firefox/addon/devtools-highlighter/
- reorder highlighter to be in before-last position
- select a tool on the left (eg inspector)
- resize toolbox to put highlighter and a few other tabs in overflowed tools
- reorder inspector to be in second position
- resize toolbox again to show everything: highlighter is in last position again
Attachment #8970493 - Flags: review?(jdescottes) → review+
Comment on attachment 8970492 [details]
Bug 1455573 - Part 1: Introduce extensionId to the tool definition for web extension.

Luca will be a much better reviewer here :)
Attachment #8970492 - Flags: review?(jdescottes) → review?(lgreco)
Comment on attachment 8970495 [details]
Bug 1455573 - Part 4: Add test for reordering with an extensions.

https://reviewboard.mozilla.org/r/239262/#review244968

This looks good, thanks Daisuke!

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_with_extension.js:105
(Diff revision 1)
> +  ok(!toolbox.doc.getElementById("tools-chevron-menu-button"),
> +     "The size of the screen being too small");
> +
> +  for (const id of TEST_STARTING_ORDER) {
> +    ok(getElementByToolIdOrExtensionId(toolbox, id),
> +       `Tab element shoud exist for ${ id }`);

nit shoud -> should

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_with_extension.js:109
(Diff revision 1)
> +    ok(getElementByToolIdOrExtensionId(toolbox, id),
> +       `Tab element shoud exist for ${ id }`);
> +  }
> +}
> +
> +function assertTabsOrder(toolbox, expectedOrder) {

Can this method be shared with the other drag and drop test, by moving is to head.js? 

(methods in head.js need to have a detailed jsdoc though, so this can be handled in a follow up)

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_with_extension.js:117
(Diff revision 1)
> +  const tabEls = toolbox.doc.querySelectorAll(".devtools-tab");
> +
> +  for (let i = 0; i < expectedOrder.length; i++) {
> +    const isOrdered = tabEls[i].dataset.id === expectedOrder[i] ||
> +                      tabEls[i].dataset.extensionId === expectedOrder[i];
> +    ok(isOrdered, `The tab[${ expectedOrder[i] }] should be existente at [${ i }]`);

nit: should be existent at -> should exist at, or should be at

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_with_extension.js:133
(Diff revision 1)
> +  info("Check the order in DevTools preference for tabs order");
> +  is(Services.prefs.getCharPref("devtools.toolbox.tabsOrder"), expectedOrder.join(","),
> +     "The preference should be correct");
> +}
> +
> +async function dnd(toolbox, dragTarget, dropTarget) {

Same comment about sharing the method.

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_with_extension.js:153
(Diff revision 1)
> +  EventUtils.synthesizeMouseAtCenter(dropTargetEl,
> +                                     { type: "mouseup" },
> +                                     dropTargetEl.ownerGlobal);
> +}
> +
> +async function resizeWindow(toolbox, width, height) {

Same comment about sharing the method.
Attachment #8970495 - Flags: review?(jdescottes) → review+
Comment on attachment 8970494 [details]
Bug 1455573 - Part 3: Save the reordering preference when destroying.

https://reviewboard.mozilla.org/r/239260/#review244974
Attachment #8970494 - Flags: review?(jdescottes) → review+
Comment on attachment 8970493 [details]
Bug 1455573 - Part 2: Correspond to the extension tool tab.

https://reviewboard.mozilla.org/r/239258/#review244950

> this second check should be:
> 
> (tab.dataset.extensionId && (tab.dataset.extensionId === definition.extensionId))
> 
> This comparison should only be done for tabs that have an extensionId, and we should compare it to extensionId, not to id.
> 
> STRs that currently fail:
> - install https://addons.mozilla.org/en-US/firefox/addon/devtools-highlighter/
> - reorder highlighter to be in before-last position
> - select a tool on the left (eg inspector)
> - resize toolbox to put highlighter and a few other tabs in overflowed tools
> - reorder inspector to be in second position
> - resize toolbox again to show everything: highlighter is in last position again

Thank you very much, Julian!

Ah, though I rethinked about the second check, it seems we can drop this. Because we check only whether the tab was overflowed in here, we can check with just the id.

And, sorry, I could not reproduce the issue,,
If possible, could you send the value of preference `devtools.toolbox.tabsOrder` of when reordered?
Comment on attachment 8970495 [details]
Bug 1455573 - Part 4: Add test for reordering with an extensions.

https://reviewboard.mozilla.org/r/239262/#review244968

> Can this method be shared with the other drag and drop test, by moving is to head.js? 
> 
> (methods in head.js need to have a detailed jsdoc though, so this can be handled in a follow up)

Yes, indeed!
Comment on attachment 8970492 [details]
Bug 1455573 - Part 1: Introduce extensionId to the tool definition for web extension.

https://reviewboard.mozilla.org/r/239256/#review245242
Attachment #8970492 - Flags: review?(lgreco) → review+
Comment on attachment 8970495 [details]
Bug 1455573 - Part 4: Add test for reordering with an extensions.

https://reviewboard.mozilla.org/r/239262/#review245180

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_with_extension.js:50
(Diff revision 1)
> +  });
> +
> +  await extension.startup();
> +
> +  const tab = await addTab("about:blank");
> +  const onReady = extension.awaitMessage("ready");

Nit, extension.awaitMessage is going to work fine by calling it after openToolboxForTab and so here we can just do something like:

```
const toolbox = await openToolboxForTab(tab, "webconsole", Toolbox.HostType.Bottom);
await extension.awaitMessage("ready");
```

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_with_extension.js:50
(Diff revision 1)
> +  });
> +
> +  await extension.startup();
> +
> +  const tab = await addTab("about:blank");
> +  const onReady = extension.awaitMessage("ready");

Nit, How about using a message string like "devtools-page-ready" or "devtools-page-loaded"?
so that it is immediatelly clear what we are we waiting for.

::: devtools/client/framework/test/browser_toolbox_toolbar_reorder_with_extension.js:85
(Diff revision 1)
> +  await gDevTools.closeToolbox(target);
> +  await target.destroy();
> +  assertPreferenceOrder(["storage", "inspector", "webconsole", "jsdebugger",
> +                         "styleeditor", "performance", "memory", "netmonitor"]);
> +
> +  Services.prefs.setCharPref("devtools.toolbox.tabsOrder", originalPreference);

If I'm not reading it wrong, if this test fails then this preference is not going to be set to its originalPreference, it would be safer to do this into a callback passed to `registerCleanupFunction`.
Comment on attachment 8970495 [details]
Bug 1455573 - Part 4: Add test for reordering with an extensions.

https://reviewboard.mozilla.org/r/239262/#review245180

> Nit, extension.awaitMessage is going to work fine by calling it after openToolboxForTab and so here we can just do something like:
> 
> ```
> const toolbox = await openToolboxForTab(tab, "webconsole", Toolbox.HostType.Bottom);
> await extension.awaitMessage("ready");
> ```

Thank you for the review, Luca!

I see, will address it.

> Nit, How about using a message string like "devtools-page-ready" or "devtools-page-loaded"?
> so that it is immediatelly clear what we are we waiting for.

Yep!

> If I'm not reading it wrong, if this test fails then this preference is not going to be set to its originalPreference, it would be safer to do this into a callback passed to `registerCleanupFunction`.

Oh, it's very reasonable.
I'll restore not only the preference but also window size.
Thanks again!
Comment on attachment 8970493 [details]
Bug 1455573 - Part 2: Correspond to the extension tool tab.

https://reviewboard.mozilla.org/r/239258/#review245314

::: devtools/client/framework/toolbox-tabs-order-manager.js:121
(Diff revision 2)
> -          .map(tabElement => tabElement.dataset.id)
> -          .concat(this.overflowedTabIds);
> -      const pref = ids.join(",");
> +      const overflowedTabIds =
> +        this.currentPanelDefinitions
> +            .filter(definition => !tabs.some(tab => tab.dataset.id === definition.id))
> +            .map(definition => definition.extensionId || definition.id);

Ah good point! Indeed we don't need to compare extension ids in this filter.

Maybe we can add a comment here to explain that overflowed tabs cannot be reordered so we just need to read the current list of ids and concatenate it with the visible tab ids.
(In reply to Daisuke Akatsuka (:daisuke) from comment #17)
> Comment on attachment 8970493 [details]
> Bug 1455573 - Part 2: Correspond to the extension tool tab.
> 
> https://reviewboard.mozilla.org/r/239258/#review244950
> 
> > this second check should be:
> > 
> > (tab.dataset.extensionId && (tab.dataset.extensionId === definition.extensionId))
> > 
> > This comparison should only be done for tabs that have an extensionId, and we should compare it to extensionId, not to id.
> > 
> > STRs that currently fail:
> > - install https://addons.mozilla.org/en-US/firefox/addon/devtools-highlighter/
> > - reorder highlighter to be in before-last position
> > - select a tool on the left (eg inspector)
> > - resize toolbox to put highlighter and a few other tabs in overflowed tools
> > - reorder inspector to be in second position
> > - resize toolbox again to show everything: highlighter is in last position again
> 
> Thank you very much, Julian!
> 
> Ah, though I rethinked about the second check, it seems we can drop this.
> Because we check only whether the tab was overflowed in here, we can check
> with just the id.
> 
> And, sorry, I could not reproduce the issue,,
> If possible, could you send the value of preference
> `devtools.toolbox.tabsOrder` of when reordered?

And regarding the issue, it only occurred after fixing `tab.dataset.extensionId === definition.id` to `tab.dataset.extensionId === definition.extensionId` which seemed to be the intent. But as you said the check was not necessary at all.
Comment on attachment 8970493 [details]
Bug 1455573 - Part 2: Correspond to the extension tool tab.

https://reviewboard.mozilla.org/r/239258/#review245314

> Ah good point! Indeed we don't need to compare extension ids in this filter.
> 
> Maybe we can add a comment here to explain that overflowed tabs cannot be reordered so we just need to read the current list of ids and concatenate it with the visible tab ids.

Thanks, I'll add a comment like that!
Comment on attachment 8970495 [details]
Bug 1455573 - Part 4: Add test for reordering with an extensions.

https://reviewboard.mozilla.org/r/239262/#review245824

Thanks Daisuke! the new browser_toolbox_toolbar_reorder_with_extension.js test file looks good to me.
Attachment #8970495 - Flags: review?(lgreco) → review+
Thanks, Luca!

I updated the patches since need to re-base.
I will make them to land after checking the try.
Thanks!

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=14ca1ccaf1bd1d7f55a04822d32cc1f70be3101a
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6b3c256ecab9
Part 1: Introduce extensionId to the tool definition for web extension. r=rpl
https://hg.mozilla.org/integration/autoland/rev/7b406eceeadd
Part 2: Correspond to the extension tool tab. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/a266c1d71a16
Part 3: Save the reordering preference when destroying. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/c7604ce41bc5
Part 4: Add test for reordering with an extensions. r=jdescottes,rpl
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.