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

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Patrick Brosset <:pbro>]

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.

Comment 1

[Julian Descottes [:jdescottes]]

We might be running into similar issues as with Bug 1394750 here.

Comment 2

[Julian Descottes [:jdescottes]]

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?

Comment 3

[Julian Descottes [:jdescottes]]

(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)

Comment 4

[Julian Descottes [:jdescottes]]

: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.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(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.

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

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?

Comment 7

[Julian Descottes [:jdescottes]]

Proposal looks great Luca, thanks!
Assigning to :daisuke, let's try to get this in before the soft freeze!

Comment 8

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1455573 - Part 1: Introduce extensionId to the tool definition for web extension.

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

Comment 9

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1455573 - Part 2: Correspond to the extension tool tab.

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

Comment 10

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1455573 - Part 3: Save the reordering preference when destroying.

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

Comment 11

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1455573 - Part 4: Add test for reordering with an extensions.

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

Comment 12

[Daisuke Akatsuka (:daisuke)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c90cdf0d7c08ff29a2c2b99f77e860255684c887

Comment 13

[Julian Descottes [:jdescottes]]

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

Comment 14

[Julian Descottes [:jdescottes]]

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 :)

Comment 15

[Julian Descottes [:jdescottes]]

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.

Comment 16

[Julian Descottes [:jdescottes]]

Comment on attachment 8970494 [details]
Bug 1455573 - Part 3: Save the reordering preference when destroying.

https://reviewboard.mozilla.org/r/239260/#review244974

Comment 17

[Daisuke Akatsuka (:daisuke)]

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 18

[Daisuke Akatsuka (:daisuke)]

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 19

[Daisuke Akatsuka (:daisuke)]

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

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

Comment 20

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8970493 [details]
Bug 1455573 - Part 2: Correspond to the extension tool tab.

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

Comment 21

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8970494 [details]
Bug 1455573 - Part 3: Save the reordering preference when destroying.

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

Comment 22

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8970495 [details]
Bug 1455573 - Part 4: Add test for reordering with an extensions.

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

Comment 23

[Daisuke Akatsuka (:daisuke)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=160f449af9e0d2614790515d268ac20b244727c0

Comment 24

[Luca Greco [:rpl] [:luca] [:lgreco]]

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

Comment 25

[Luca Greco [:rpl] [:luca] [:lgreco]]

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 26

[Daisuke Akatsuka (:daisuke)]

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 27

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8970495 [details]
Bug 1455573 - Part 4: Add test for reordering with an extensions.

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

Comment 28

[Daisuke Akatsuka (:daisuke)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b2c6d1b49206c135350fa0d11181e2077ab3acd

Comment 29

[Julian Descottes [:jdescottes]]

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.

Comment 30

[Julian Descottes [:jdescottes]]

(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 31

[Daisuke Akatsuka (:daisuke)]

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 32

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8970493 [details]
Bug 1455573 - Part 2: Correspond to the extension tool tab.

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

Comment 33

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8970494 [details]
Bug 1455573 - Part 3: Save the reordering preference when destroying.

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

Comment 34

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8970495 [details]
Bug 1455573 - Part 4: Add test for reordering with an extensions.

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

Comment 35

[Luca Greco [:rpl] [:luca] [:lgreco]]

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.

Comment 36

[Daisuke Akatsuka (:daisuke)]

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

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

Comment 37

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8970493 [details]
Bug 1455573 - Part 2: Correspond to the extension tool tab.

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

Comment 38

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8970494 [details]
Bug 1455573 - Part 3: Save the reordering preference when destroying.

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

Comment 39

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8970495 [details]
Bug 1455573 - Part 4: Add test for reordering with an extensions.

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

Comment 40

[Daisuke Akatsuka (:daisuke)]

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

Comment 41

[Pulsebot]

Pushed by [e-mail]:
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

Comment 42

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/6b3c256ecab9
https://hg.mozilla.org/mozilla-central/rev/7b406eceeadd
https://hg.mozilla.org/mozilla-central/rev/a266c1d71a16
https://hg.mozilla.org/mozilla-central/rev/c7604ce41bc5