Closed Bug 1654762 Opened 2 months ago Closed 2 months ago

Use traits instead of actorHasMethod in storage/ui module

Categories

(DevTools :: General, task, P3)

task

Tracking

(firefox80 fixed)

RESOLVED FIXED
Firefox 80
Tracking Status
firefox80 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(Blocks 4 open bugs)

Details

Attachments

(2 files, 1 obsolete file)

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

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

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.

Assignee: nobody → jdescottes
Status: NEW → ASSIGNED

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

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.

Summary: Remove usage of actorHasMethod in storage/ui module → Use traits instead of actorHasMethod in storage/ui module
Blocks: 1654996
Blocks: 1654998

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.

Attachment #9165672 - Attachment is obsolete: true

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.

Blocks: 1655001
Pushed by jdescottes@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 80
You need to log in before you can comment on or make changes to this bug.