[webkit-reviews] review canceled: [Bug 191345] Web Inspector: Start moving toward better multi-target support : [Attachment 354057] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has canceled Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 191345: Web Inspector: Start moving toward better multi-target support
https://bugs.webkit.org/show_bug.cgi?id=191345

Attachment 354057: [PATCH] Proposed Fix

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




--- Comment #3 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 354057
  --> https://bugs.webkit.org/attachment.cgi?id=354057
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:692
>> +	    if (!InspectorBackend.domains.Debugger.setPauseOnAssertions) {
> 
> Why not use `target.DebuggerAgent` here?

This is using `setPauseOnAssertions` as an agnostic feature check for "we are
after iOS 10 or not". I think its more of an agnostic feature check then a
per-target check. But I'll still switch to the target version here.

>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:66
>> +		    target.NetworkAgent.setResourceCachingDisabled(true);
> 
> You could just call this function with the `WI.Setting`'s value instead of
using `true`:
> 
>     if (target.NetworkAgent.setResourceCachingDisabled)
>	 
target.NetworkAgent.setResourceCachingDisabled(WI.settings.resourceCachingDisab
led.value);

Done. Some of these we probably just wanted to avoid a message to the backend
if we know we were matching the default value, like sending false when the
backend should be false.

>> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:726
>> +	    if (!window.TimelineAgent)
> 
> Shouldn't this follow the `domains` approach?
> 
>     if (!InspectorBackend.domains.Timeline)

Ultimately for multi-target support this code will need to attempt to enable
auto capture on one or more targets. So this will have to be revisited later.
So I used `window.TargetAgent` to make it clear that we should revisit it.

>> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:1095
>> +		target.TimelineAgent.setInstruments([...instrumentSet]);
> 
> `Array.from` should be faster/simpler than constructing an array using spread
> <https://jsperf.com/set-iterator-vs-foreach/4>

Done.

>> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:178
>> +	    if (!InspectorBackend.domains.DOM.hasEvent("pseudoElementAdded"))
> 
> While we're doing this change, is there no other protocol check we can
perform for iOS9?  Something in one of the "always available" domains?	Perhaps
that `RuntimeAgent.enableTypeProfiler` exists?

This is a difference between 9.0 and 9.3 and later... So our options are
limited (none in the big 3 Runtime/Debugger/Console). I'm going to keep this
because it was the most accurate at the time.

>> Source/WebInspectorUI/UserInterface/Protocol/Target.js:75
>> +		if (this.PageAgent.setShowPaintRects &&
WI.settings.showPaintRects.value)
> 
> Ditto (NetworkManager.js:65)

I think here we are avoiding sending `false` to the backend when we know the
backend should already be starting out `false`.

>> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:176
>> +			result.release();
> 
> Shouldn't we still release in the case that the `result.type !== "function"`?

Agreed, I cleaned this up to:

    remoteObject.getProperty("constructor", (error, result, wasThrown) => {
	if (error)
	    return;
	if (result.type === "function")
	   
remoteObject.target.DebuggerAgent.getFunctionDetails(result.objectId,
didGetFunctionDetails);
	result.release();
    });

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:104
>> +	    if (window.PageAgent && PageAgent.setShowRulers)
> 
> Ditto (TimelineManager.js:726)
> 
>     if (InspectorBackend.domains.Page.setShowRulers)

Same thing, regarding multi-target support.

>> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:131
>> +	    if (window.HeapAgent && HeapAgent.gc)
> 
> Ditto (TimelineManager.js:726)
> 
>     if (InspectorBackend.domains.Heap.gc)

Same thing regarding multi-target support.