1654762 - Use traits instead of actorHasMethod in storage/ui module

Status: RESOLVED FIXED

(DevTools :: General, task, P3)

Description

[Julian Descottes [:jdescottes]]

actorHasMethod has too many pitfalls to be reliably used.

https://searchfox.org/mozilla-central/rev/dcd9c2d2bc19d96d487825eb70c2333a4d60994e/devtools/client/storage/ui.js#783-798

        this.actorSupportsAddItem = await target.actorHasMethod(
          type,
          "addItem"
        );
        this.actorSupportsRemoveItem = await target.actorHasMethod(
          type,
          "removeItem"
        );
        this.actorSupportsRemoveAll = await target.actorHasMethod(
          type,
          "removeAll"
        );
        this.actorSupportsRemoveAllSessionCookies = await target.actorHasMethod(
          type,
          "removeAllSessionCookies"
        );

Here we should introduce traits instead because different types of storage actors will implement different set of APIs. See https://searchfox.org/mozilla-central/source/devtools/shared/specs/storage.js

Comment 1

[Julian Descottes [:jdescottes]]

Note that we will need to wait until the traits added here reach the release version to fully remove the method.

Comment 2

[Julian Descottes [:jdescottes]]

Let's try to get that in Firefox 80 since we will need to wait for the backward compat window before we can remove the methods.

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1654762 - Use traits to detect DevTools storage actor supported methods

This is the only meaningful callsite for actorHasMethod.
This method has many bugs and we want to replace it with traits

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1654762 - Check supportsXXX traits directly on individual storage fronts

Depends on D84689

This is optional but the current implementation barely works.
We store the supportsXXX flags directly on the UI module instance, whereas the values differ for each type.

It more or less works because the values are recomputed every time we select a catefory, but it fails in several edge cases:

• trying to right click on a category while another category is selected
• toolbar is not updated correctly, meaning the addItem icon is always displayed even when it is not possible

I can perfectly move that to another bug if that's preferable.

Comment 5

[Phabricator Automation]

Comment on attachment 9165672 [details]
Bug 1654762 - Check supportsXXX traits directly on individual storage fronts

Revision D84692 was moved to bug 1654998. Setting attachment 9165672 [details] to obsolete.

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1654762 - Wait for store-objects-updated event in Storage Inspector init

Depends on D84689

The test failures are coming from a race condition which became visible with the previous patch in the stack.
The storage inspector head.js waits for an event emitted by the UI when opening the panel.
But now that we don't have to wait for the 4 actorHasMethod calls, the event is emitted before the test can listen for it.

One option is to let onTargetAvailable wait for the event instead, so that the Storage panel is only initialized when the store-objects-updated was fired.
This way, the test harness no longer has to wait for this event.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/feb3ad4b4a25
Use traits to detect DevTools storage actor supported methods r=nchevobbe,davidwalsh
https://hg.mozilla.org/integration/autoland/rev/dfa63a8802e7
Wait for store-objects-updated event in Storage Inspector init r=nchevobbe

Comment 8

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/feb3ad4b4a25
https://hg.mozilla.org/mozilla-central/rev/dfa63a8802e7