Bug 203427

Created attachment 381949 [details] Patch

Comment on attachment 381949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381949&action=review > Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:724 > + globalObjectCleared(target) Why are we passing in a target when: 1) We assume it will always be the main target 2) It is never used > Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:947 > + console.assert(target === frame.mainResource.target); Same as above. Why bother passing in the target when we are just asserting that it is always `frame.mainResource.target`? > Source/WebInspectorUI/UserInterface/Protocol/PageTarget.js:28 > + constructor(parentTarget, targetId, name, connection) `parentTarget` should be the last parameter, since it is optional.

Comment on attachment 381949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381949&action=review >> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:724 >> + globalObjectCleared(target) > > Why are we passing in a target when: > > 1) We assume it will always be the main target > 2) It is never used This is preparation for the next step (see https://webkit.org/b/202219) where this method could be called for provisional target as well, that will change the assumption (1) to a condition. For now it's used in the assert to validate the assumptions but I can remove that if you feel like there is a problem. >> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:947 >> + console.assert(target === frame.mainResource.target); > > Same as above. Why bother passing in the target when we are just asserting that it is always `frame.mainResource.target`? This will be used in the next changes which would enable agents on the provisional targets early. The target is going to be used to find correct map of frames. Basically NetworkManager will have a map target->{frameId->frame}. In general most manager methods should have target as a parameter to keep per target state. >> Source/WebInspectorUI/UserInterface/Protocol/PageTarget.js:28 >> + constructor(parentTarget, targetId, name, connection) > > `parentTarget` should be the last parameter, since it is optional. Well, it is nullable but not optional, I'd like all callers to pass it explicitly, so that relations between the targets are clear from the code.

Comment on attachment 381949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381949&action=review >>> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:724 >>> + globalObjectCleared(target) >> >> Why are we passing in a target when: >> >> 1) We assume it will always be the main target >> 2) It is never used > > This is preparation for the next step (see https://webkit.org/b/202219) where this method could be called for provisional target as well, that will change the assumption (1) to a condition. For now it's used in the assert to validate the assumptions but I can remove that if you feel like there is a problem. If this isn't directly needed for this patch, please do not include it in this patch. That makes it harder to bisect/rollout regressions, and we generally don't like doing that. >>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:947 >>> + console.assert(target === frame.mainResource.target); >> >> Same as above. Why bother passing in the target when we are just asserting that it is always `frame.mainResource.target`? > > This will be used in the next changes which would enable agents on the provisional targets early. The target is going to be used to find correct map of frames. Basically NetworkManager will have a map target->{frameId->frame}. In general most manager methods should have target as a parameter to keep per target state. If this isn't directly needed for this patch, please do not include it in this patch. That makes it harder to bisect/rollout regressions, and we generally don't like doing that. > Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:149 > + _connectToTarget(parentTarget, targetInfo) This function should be moved to the "Private" section below. I believe I asked you to do this in the last patch/review as well. > Source/WebInspectorUI/UserInterface/Models/Script.js:254 > + _uniqueNumbersForTarget() NIT: I'd rename this to be more explicit, like `_uniqueDisplayNameNumbersForRootTarget()`. > Source/WebInspectorUI/UserInterface/Models/Script.js:256 > + if (!WI.Script._rootTargetToUniqueNumbers) NIT: I'd rename this to something more straightforward, like `_uniqueDisplayNameNumbersForRootTargetMap`. > Source/WebInspectorUI/UserInterface/Models/Script.js:259 > + let rootTarget = this._target.rootTarget; It is possible for a `WI.Script` to not have a `_target`, so we should either assert that we can't reach this code or use a special key if `this instanceof WI.LocalScript`. > Source/WebInspectorUI/UserInterface/Protocol/Connection.js:-287 > - constructor(parentTarget, workerId) Please update any construction sites to not pass any arguments. > Source/WebInspectorUI/UserInterface/Protocol/Connection.js:-310 > - constructor(parentTarget, targetId) Please update any construction sites to not pass any arguments. > Source/WebInspectorUI/UserInterface/Protocol/MultiplexingBackendTarget.js:35 > + super(null, "multi", WI.UIString("Web Page"), WI.TargetType.WebPage, InspectorBackend.backendConnection); Style: we prefer creating constants for things like this so we can know at the callsite what the primitive is expected to be used for (assuming that it isn'd derivable from the value itself): ``` const parentTarget = null; const targetId = "multi" super(parentTarget, targetId, WI.UIString("Web Page"), WI.TargetType.WebPage, InspectorBackend.backendConnection); ``` > Source/WebInspectorUI/UserInterface/Protocol/Target.js:32 > + this._parentTarget = parentTarget; Please add a `console.assert(parentTarget === null || parentTarget instanceof WI.Target);` if you want to enforce "nullable, not optional". > Source/WebInspectorUI/UserInterface/Protocol/Target.js:157 > + if (this._type === InspectorBackend.Enum.Target.TargetInfoType.Page) This should use `WI.TargetType.Page`.

Comment on attachment 381949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381949&action=review >>>> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:724 >>>> + globalObjectCleared(target) >>> >>> Why are we passing in a target when: >>> >>> 1) We assume it will always be the main target >>> 2) It is never used >> >> This is preparation for the next step (see https://webkit.org/b/202219) where this method could be called for provisional target as well, that will change the assumption (1) to a condition. For now it's used in the assert to validate the assumptions but I can remove that if you feel like there is a problem. > > If this isn't directly needed for this patch, please do not include it in this patch. That makes it harder to bisect/rollout regressions, and we generally don't like doing that. Removed. >>>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:947 >>>> + console.assert(target === frame.mainResource.target); >>> >>> Same as above. Why bother passing in the target when we are just asserting that it is always `frame.mainResource.target`? >> >> This will be used in the next changes which would enable agents on the provisional targets early. The target is going to be used to find correct map of frames. Basically NetworkManager will have a map target->{frameId->frame}. In general most manager methods should have target as a parameter to keep per target state. > > If this isn't directly needed for this patch, please do not include it in this patch. That makes it harder to bisect/rollout regressions, and we generally don't like doing that. Removed. >> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:149 >> + _connectToTarget(parentTarget, targetInfo) > > This function should be moved to the "Private" section below. I believe I asked you to do this in the last patch/review as well. Done. I realized that I didn't include it to the original change which is why I'm updating it here :-) >> Source/WebInspectorUI/UserInterface/Models/Script.js:254 >> + _uniqueNumbersForTarget() > > NIT: I'd rename this to be more explicit, like `_uniqueDisplayNameNumbersForRootTarget()`. Done. >> Source/WebInspectorUI/UserInterface/Models/Script.js:256 >> + if (!WI.Script._rootTargetToUniqueNumbers) > > NIT: I'd rename this to something more straightforward, like `_uniqueDisplayNameNumbersForRootTargetMap`. Done. >> Source/WebInspectorUI/UserInterface/Models/Script.js:259 >> + let rootTarget = this._target.rootTarget; > > It is possible for a `WI.Script` to not have a `_target`, so we should either assert that we can't reach this code or use a special key if `this instanceof WI.LocalScript`. Done. >> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:-287 >> - constructor(parentTarget, workerId) > > Please update any construction sites to not pass any arguments. Done. >> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:-310 >> - constructor(parentTarget, targetId) > > Please update any construction sites to not pass any arguments. Done. >> Source/WebInspectorUI/UserInterface/Protocol/MultiplexingBackendTarget.js:35 >> + super(null, "multi", WI.UIString("Web Page"), WI.TargetType.WebPage, InspectorBackend.backendConnection); > > Style: we prefer creating constants for things like this so we can know at the callsite what the primitive is expected to be used for (assuming that it isn'd derivable from the value itself): > ``` > const parentTarget = null; > const targetId = "multi" > super(parentTarget, targetId, WI.UIString("Web Page"), WI.TargetType.WebPage, InspectorBackend.backendConnection); > ``` Done. >> Source/WebInspectorUI/UserInterface/Protocol/Target.js:32 >> + this._parentTarget = parentTarget; > > Please add a `console.assert(parentTarget === null || parentTarget instanceof WI.Target);` if you want to enforce "nullable, not optional". Done. >> Source/WebInspectorUI/UserInterface/Protocol/Target.js:157 >> + if (this._type === InspectorBackend.Enum.Target.TargetInfoType.Page) > > This should use `WI.TargetType.Page`. Done.

Created attachment 381976 [details] Patch

Comment on attachment 381976 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381976&action=review > Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:-728 > - WI.Script.resetUniqueDisplayNameNumbers(); We actually still probably want to do something like this (but specific to the current `WI.Target`), such as if the developer refreshes the page, which would create a new global object but would _not_ create a new `WI.Target`. > Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:175 > + let connection = new InspectorBackend.TargetConnection(); Style: we don't add `()` if there are no arguments for a constructor > Source/WebInspectorUI/UserInterface/Controllers/WorkerManager.js:48 > + let connection = new InspectorBackend.WorkerConnection(); Style: we don't add `()` if there are no arguments for a constructor > Source/WebInspectorUI/UserInterface/Models/Script.js:260 > + console.assert(this._target); Do we expect this to ever be called by a `WI.Script` that does _not_ have a `_target`? AFAIK, the only situation of this happening is for the bootstrap script (see `WI.NetworkManager.prototype.async createBootstrapScript`), which shouldn't ever make it to this function anyways. Typically, we either have an assertion or a fallback path, not both (the only exception to this is for early-returns, or cases where the lack of the value would cause an exception). > Source/WebInspectorUI/UserInterface/Models/Script.js:261 > + if (this._target) { Style: we don't add `{` and `}` if the body of the `if` is only one line > Source/WebInspectorUI/UserInterface/Models/Script.js:265 > + WI.Script._keyForNoTarget = "no-target"; `WeakMap`s require that the key be an object, so you can't just use a string. In the past, I've used something like `{noTarget: true}` as the key.

Comment on attachment 381976 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381976&action=review >> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:-728 >> - WI.Script.resetUniqueDisplayNameNumbers(); > > We actually still probably want to do something like this (but specific to the current `WI.Target`), such as if the developer refreshes the page, which would create a new global object but would _not_ create a new `WI.Target`. Good point. Done. >> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:175 >> + let connection = new InspectorBackend.TargetConnection(); > > Style: we don't add `()` if there are no arguments for a constructor Done. >> Source/WebInspectorUI/UserInterface/Controllers/WorkerManager.js:48 >> + let connection = new InspectorBackend.WorkerConnection(); > > Style: we don't add `()` if there are no arguments for a constructor Done. >> Source/WebInspectorUI/UserInterface/Models/Script.js:260 >> + console.assert(this._target); > > Do we expect this to ever be called by a `WI.Script` that does _not_ have a `_target`? AFAIK, the only situation of this happening is for the bootstrap script (see `WI.NetworkManager.prototype.async createBootstrapScript`), which shouldn't ever make it to this function anyways. Typically, we either have an assertion or a fallback path, not both (the only exception to this is for early-returns, or cases where the lack of the value would cause an exception). I wasn't sure if scripts with no target could make it to here. Left only the assertion and check that target is present before calling rootTarget to avoid an exception. 'undedined' key should give a reasonable fallback should the assertion ever fail. >> Source/WebInspectorUI/UserInterface/Models/Script.js:261 >> + if (this._target) { > > Style: we don't add `{` and `}` if the body of the `if` is only one line Done. I added them here for consistency with the other branch, which is actually a requirement in many style guides (no {} around single line statements unless another branch has {}). >> Source/WebInspectorUI/UserInterface/Models/Script.js:265 >> + WI.Script._keyForNoTarget = "no-target"; > > `WeakMap`s require that the key be an object, so you can't just use a string. In the past, I've used something like `{noTarget: true}` as the key. Done.

Created attachment 382002 [details] Patch

Comment on attachment 382002 [details] 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?

Comment on attachment 382002 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382002&action=review >> 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. I'd love to keep it inside the constructor, the problem is parentTarget is not known there yet because TargetConnection.target is not set yet. This is a chicken and egg problem as Target and TargetConnection reference each other and we cannot pass Target to the TargetConnection constructor. >> Source/WebInspectorUI/UserInterface/Controllers/WorkerManager.js:47 >> + console.assert(target.hasCommand("Worker.sendMessageToWorker")); > > Ditto (TargetManager.js:174) See above. >> Source/WebInspectorUI/UserInterface/Models/Script.js:63 >> + this._uniqueDisplayNameNumber = this.constructor._nextUniqueConsoleDisplayNameNumber(); > > Shouldn't this be `this._nextUniqueConsoleDisplayNameNumber()`? That's what it is currently, did you mean _nextUniqueDisplayNameNumber() ? >> 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. Done. I was worried about possible exceptions here in case we hit this assert. >> 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). Updated the other place as well, though it makes less sense there. I extracted it in a var here for documentation purposes to make it clear that target id is worker id in this case. >> Source/WebInspectorUI/UserInterface/Protocol/DirectBackendTarget.js:35 >> + 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`. Done. >> 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. Done. >> 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? It always has MultiplexingBackendTarget as its parent.

Comment on attachment 382002 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382002&action=review >>> Source/WebInspectorUI/UserInterface/Models/Script.js:63 >>> + this._uniqueDisplayNameNumber = this.constructor._nextUniqueConsoleDisplayNameNumber(); >> >> Shouldn't this be `this._nextUniqueConsoleDisplayNameNumber()`? > > That's what it is currently, did you mean _nextUniqueDisplayNameNumber() ? I see what you meant. Fixed.

Created attachment 382360 [details] Patch for landing

Comment on attachment 382360 [details] Patch for landing Clearing flags on attachment: 382360 Committed r251816: <https://trac.webkit.org/changeset/251816>

All reviewed patches have been landed. Closing bug.

<rdar://problem/56761709>