[webkit-reviews] review denied: [Bug 200117] Web Inspector: Storage: disable related agents when the tab is closed : [Attachment 374883] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 200117: Web Inspector: Storage: disable related agents when the tab is
closed
https://bugs.webkit.org/show_bug.cgi?id=200117

Attachment 374883: Patch

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




--- Comment #3 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 374883
  --> https://bugs.webkit.org/attachment.cgi?id=374883
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?