[webkit-reviews] review denied: [Bug 191052] Web Inspector: Separate target initialization from frontend initialization : [Attachment 353347] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has denied Joseph Pecoraro <joepeck at webkit.org>'s
request for review:
Bug 191052: Web Inspector: Separate target initialization from frontend
initialization
https://bugs.webkit.org/show_bug.cgi?id=191052

Attachment 353347: [PATCH] Proposed Fix

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




--- Comment #10 from Brian Burg <bburg at apple.com> ---
Comment on attachment 353347
  --> https://bugs.webkit.org/attachment.cgi?id=353347
[PATCH] Proposed Fix

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

Looks almost ready, setting r- due to EWS failures.

> Source/WebInspectorUI/ChangeLog:78
> +	   only needs to be performed once on a target that supports it.

👍🏻

> Source/WebInspectorUI/ChangeLog:85
> +	   each of the managers to do initializtion work for this target.

Nit: initialization

>> Source/WebInspectorUI/UserInterface/Base/Main.js:485
>> +WI.performOneTimeInitializationWithTarget = function(target)
> 
> This name implies that each one-time-initialization is per-target.  Are these
initialize functions per-target, or across all targets?  If the latter, I think
something like `performOneTimeInitializationsUsingTarget` may be more accurate.

I had the same comment. It seems to be once-per-frontend-app, except the
ensureDocument call would be per-target?

> Source/WebInspectorUI/UserInterface/Base/Main.js:497
> +    // FIXME: This should move to DOMManager.initializeTarget when adding
better DOM multi-target support since it is really per-target.

I'd prefer we just move this code over now. The initializeTarget method can be
filled in more later.

>> Source/WebInspectorUI/UserInterface/Base/Main.js:668
>> +	   
WI.CSSCompletions.initializeCSSCompletions(WI.assumingMainTarget());
> 
> I realize that this was here already, but is it actually needed?

Ditto, isn't it redundant with the call in
WI.performOneTimeInitializationWithTarget()?

> Source/WebInspectorUI/UserInterface/Controllers/ApplicationCacheManager.js:44
> +    initializeTarget(target)

I really like this pattern.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:131
> +	       targetData.pauseIfNeeded();

This reads poorly, what is the type of targetData?

> Source/WebInspectorUI/UserInterface/Protocol/Target.js:66
> +	   // initialization messages will have their responses come in before
a potential bulk

Nit: This sentence is ungrammatical.

> Source/WebInspectorUI/UserInterface/Protocol/Target.js:70
> +	   setTimeout(() => {

Seems weird to use two setTimeouts back to back. Should this use Promise.all?

> Source/WebInspectorUI/UserInterface/Protocol/WorkerTarget.js:32
> +	   this._executionContext = new WI.ExecutionContext(this,
WI.RuntimeManager.TopLevelContextExecutionIdentifier, this.displayName, false,
null);

This is a lot nicer now!