[webkit-reviews] review granted: [Bug 203427] Web Inspector: track WI.Script unique display name numbers per Page target : [Attachment 382002] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Yury Semikhatsky
<yurys at chromium.org>'s request for review:
Bug 203427: Web Inspector: track WI.Script unique display name numbers per Page
target
https://bugs.webkit.org/show_bug.cgi?id=203427

Attachment 382002: Patch

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




--- Comment #10 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 382002
  --> https://bugs.webkit.org/attachment.cgi?id=382002
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:174
> +	  
console.assert(parentTarget.hasCommand("Target.sendMessageToTarget"));

NIT: it would probably be better to keep this inside the constructor of
`InspectorBackend.TargetConnection` (even if the constructor is empty
otherwise, we do this in a few places in Web Inspector code), but I'm fine with
this either way since we only create it here.

> Source/WebInspectorUI/UserInterface/Controllers/WorkerManager.js:47
> +	   console.assert(target.hasCommand("Worker.sendMessageToWorker"));

Ditto (TargetManager.js:174)

> Source/WebInspectorUI/UserInterface/Models/Script.js:63
> +	       this._uniqueDisplayNameNumber =
this.constructor._nextUniqueConsoleDisplayNameNumber();

Shouldn't this be `this._nextUniqueConsoleDisplayNameNumber()`?

> Source/WebInspectorUI/UserInterface/Models/Script.js:269
> +	   if (this._target)

If you're asserting that it exists on the line above, you don't need to add an
`if` for it.

> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:289
> +	   let workerId = this.target.identifier;

I'd either inline this (to match
`InspectorBackend.TargetConnection.prototype.sendMessageToBackend`) or change
`InspectorBackend.TargetConnection.prototype.sendMessageToBackend` to also have
a variable (for consistency).

> Source/WebInspectorUI/UserInterface/Protocol/DirectBackendTarget.js:35
> +	   const parentTarget = null;
> +	   const targetId = "direct";

Style: we try to match the order of variable declarations with the order in
which they're used as arguments. so please move these above the `let`.

> Source/WebInspectorUI/UserInterface/Protocol/Target.js:32
> +	   console.assert(parentTarget === null || parentTarget instanceof
WI.Target);

Style: we usually put assertions before the `super()` call, so that we can
check the type/value before anything has had a chance to modify it.

> Source/WebInspectorUI/UserInterface/Protocol/Target.js:158
> +	   if (this._type === WI.TargetType.Page)

Do we ever have a situation where a page target has a parent target?  If so,
isn't that a bug?