Bug 200117 - Web Inspector: Storage: disable related agents when the tab is closed
Summary: Web Inspector: Storage: disable related agents when the tab is closed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: 200118
  Show dependency treegraph
 
Reported: 2019-07-24 21:39 PDT by Devin Rousso
Modified: 2019-08-02 14:06 PDT (History)
11 users (show)

See Also:


Attachments
Patch (40.12 KB, patch)
2019-07-25 00:19 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (42.43 KB, patch)
2019-08-01 14:32 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (43.81 KB, patch)
2019-08-01 17:00 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (43.75 KB, patch)
2019-08-02 13:07 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-07-24 21:39:48 PDT
.
Comment 1 Devin Rousso 2019-07-25 00:19:50 PDT
Created attachment 374883 [details]
Patch
Comment 2 Build Bot 2019-07-25 00:22:13 PDT Comment hidden (obsolete)
Comment 3 Joseph Pecoraro 2019-07-29 12:44:45 PDT
Comment on attachment 374883 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=374883&action=review

In general this seems fine to me. It is tricky to get right (the entire state of the world needs to be torn down on disable and rebuilt on enable) but it looks like you've done this properly.

• I dislike the "_initialize" naming pattern. I realize it existed before for most of these managers, but it confuses me. "reset" seems clearer but we can create a better name.
• In most cases I feel the _initialize (reset) action needs to happen early in enable (or not at all given the last state would have been construction or disable), so that anything performed later on will take effect and not accidentally be cleared by the _initialize (reset) at the end of enable. Some of these methods dispatch an event so I could see value in eliminating an unnecessary dispatch.
• Great cleanup re-ordering the methods a bit. Even though it makes the diff a little harder to read it'll be cleaner in the future.

In an ITMLKit world the DOMStorage agent can get activated via `activateExtraDomains`, which in this case does not do what you want:

```
    activateExtraDomains(domains)
    {
        ...
        for (let domain of domains) {
            let agent = InspectorBackend.activateDomain(domain);
            if (agent.enable)
                agent.enable();
            for (let target of WI.targets)
                target.activateExtraDomain(domain);
        }
        ...
    }
```

In this case the `agent.enable()` is now wrong. Seems like you might want something:

    • Find Manager associated with Domain
        => Ask manager to activateExtraDomain
            => Manager may enable the Domain if needed.

I'm going to r- for the ITMLKit issue right now.

> Source/WebInspectorUI/UserInterface/Controllers/ApplicationCacheManager.js:91
> +        this._initialize();

I find `_initialize` here to be a very weird name.

I think of "initialization" work as "work performed on something once, particularly early on". Here "_initialize" is being called whenever the manager is enabled/disabled.

I think this is more akin to `reset()` which other managers have, or "reinitialize" which is not used anywhere in our code-base. But we should definitely be able to come up with a better name for this.

> Source/WebInspectorUI/UserInterface/Controllers/ApplicationCacheManager.js:128
> +        var frame = WI.networkManager.frameForIdentifier(frameId);

Nit: Might as well upgrade these to `let` while moving them.

> Source/WebInspectorUI/UserInterface/Controllers/DatabaseManager.js:100
> +        let inspect = () => {

A more readable name might be:

    inspect(id)
    tryInspect(id)
    attemptToInspect(id)

Where you pass the database identifier instead of capture it.

> Source/WebInspectorUI/UserInterface/Controllers/DatabaseManager.js:116
> +        if (!this._enabled) {
> +            let listener = this.addEventListener(DatabaseManager.Event.DatabaseWasAdded, (event) => {
> +                if (inspect())
> +                    this.removeEventListener(DatabaseManager.Event.DatabaseWasAdded, listener);
> +            });
> +            this.enable();
>              return;
> -        this.dispatchEventToListeners(WI.DatabaseManager.Event.DatabaseWasInspected, {database});
> +        }

This seems cryptic enough to deserve a comment.

Does this mean that `console.inspect(db)` automatically enables the DatabaseManager and ends up showing the Storage Tab?
Comment 4 Devin Rousso 2019-08-01 12:53:12 PDT
Comment on attachment 374883 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=374883&action=review

>> Source/WebInspectorUI/UserInterface/Controllers/DatabaseManager.js:116
>> +        }
> 
> This seems cryptic enough to deserve a comment.
> 
> Does this mean that `console.inspect(db)` automatically enables the DatabaseManager and ends up showing the Storage Tab?

This actually should be put back the way it was, as there's no way for us to to reach this without the `Database` agent being enabled in the first place.

My goal with this was to make it so that when the user calls `inspect(database)`, it would forcibly show the Storage tab, but I'm not sure that's the right way to go.  Other API (`console.record` or `console.profile`) don't forcibly show the related tab when called from the page, but this is slightly different in that it's part of the CommandLineAPI.  Perhaps we should make it so that `inspect` forcibly enables the tab?
Comment 5 Devin Rousso 2019-08-01 14:31:36 PDT
(In reply to Joseph Pecoraro from comment #3)
> In this case the `agent.enable()` is now wrong. Seems like you might want
> something:
> 
>     • Find Manager associated with Domain
>         => Ask manager to activateExtraDomain
>             => Manager may enable the Domain if needed.
I think all we actually need to do is _not_ call `enable` if the domain is controlled by a manager.  In that case, the tab shouldn't have been created yet as the frontend agent shouldn't exist, so the tab shouldn't be `isTabAllowed`. Domains controlled by a manager are enabled/disabled based on whether the tab is open (doesn't have to be selected/visible) or not, meaning that it's already handled by the above case.
Comment 6 Devin Rousso 2019-08-01 14:32:13 PDT
Created attachment 375347 [details]
Patch
Comment 7 Devin Rousso 2019-08-01 17:00:43 PDT
Created attachment 375368 [details]
Patch
Comment 8 Joseph Pecoraro 2019-08-02 12:51:29 PDT
Comment on attachment 375368 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375368&action=review

> Source/WebInspectorUI/UserInterface/Controllers/AppController.js:81
> +            if (manager)
> +                manager.activateExtraDomain(domain);

Can't wait to remove this code!

> Source/WebInspectorUI/UserInterface/Controllers/ApplicationCacheManager.js:124
> +    networkStateUpdated(isNowOnline)

Maybe we should have a new section:

    // Protocol Callbacks

Or just:

    // ApplicationCacheObserver

And we can remove all these inner methods.

> Source/WebInspectorUI/UserInterface/Controllers/DOMStorageManager.js:105
>      itemsCleared(storageId)

Yeah, if we had:

    // DOMStorageObserver

Here that would save so much space!

> Source/WebInspectorUI/UserInterface/Views/StorageTabContentView.js:51
> -        return !!window.DOMStorageAgent || !!window.DatabaseAgent || !!window.IndexedDBAgent;
> +        return !!(InspectorBackend.domains.ApplicationCache || InspectorBackend.domains.DOMStorage || InspectorBackend.domains.Database || InspectorBackend.domains.IndexedDB);

We probably can't do this change until other work, since this should always be true...

Once we do the BackendCommands rework we can do this!
Comment 9 Devin Rousso 2019-08-02 13:07:41 PDT
Created attachment 375444 [details]
Patch
Comment 10 WebKit Commit Bot 2019-08-02 14:05:45 PDT
Comment on attachment 375444 [details]
Patch

Clearing flags on attachment: 375444

Committed r248179: <https://trac.webkit.org/changeset/248179>
Comment 11 WebKit Commit Bot 2019-08-02 14:05:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2019-08-02 14:06:20 PDT
<rdar://problem/53878741>