Bug 200117 – Web Inspector: Storage: disable related agents when the tab is closed

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

.

[Devin Rousso]

Created attachment 374883 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Joseph Pecoraro]

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?

[Devin Rousso]

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?

[Devin Rousso]

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

[Devin Rousso]

Created attachment 375347 [details]
Patch

[Devin Rousso]

Created attachment 375368 [details]
Patch

[Joseph Pecoraro]

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!

[Devin Rousso]

Created attachment 375444 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 375444 [details]
Patch

Clearing flags on attachment: 375444

Committed r248179: <https://trac.webkit.org/changeset/248179>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53878741>