[webkit-reviews] review granted: [Bug 202704] Web Inspector: notify inspector when provisional page is created, committed and destroyed : [Attachment 381421] 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 202704: Web Inspector: notify inspector when provisional page is created,
committed and destroyed
https://bugs.webkit.org/show_bug.cgi?id=202704

Attachment 381421: Patch

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




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

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

r=me, awesome work!

> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:135
> +    m_frontendDispatcher->didCommitProvisionalTarget(oldTargetID,
buildTargetInfoObject(*target));

Would we ever expect the `type` to change?  If not, couldn't we simplify this
to just send the `oldTargetID` and `newTargetID` and have the frontend reuse
the same `WI.Target` by changing it's `_identifier`?

> Source/JavaScriptCore/inspector/protocol/Target.json:13
> +		   { "name": "isProvisional", "type": "boolean", "optional":
true, "description": "Whether this is a provisional page target." }

This doesn't explain what a "provisional" target is.  I'd expect some
explanation of what that is.

> Source/JavaScriptCore/inspector/protocol/Target.json:43
> +		   { "name": "previousTargetId", "type": "string",
"description": "ID of the target committed target is swapped with." },

This is an awkward sentence description.  Please rephrase it.

> Source/JavaScriptCore/inspector/protocol/Target.json:44
> +		   { "name": "targetInfo", "$ref": "TargetInfo", "description":
"Committed target info." }

These types of descriptions aren't very useful.  I'd either add more
information or remove it.

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:125
> +	   this.dispatchEventToListeners(WI.TargetManager.Event.TargetCreated,
{targetInfo});

Rather than pass the `targetInfo`, can we instead pass the new `WI.Target`? 
That way, we could use it in non-test code as well (if needed).  We could pass
`isProvisional` as a separate argument in the object since it's not saved on
`WI.Target`.

NIT: you can drop the `WI` from `WI.TargetManager` since it's in the same
class.

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:131
> +	   this._provisionalTargetIds.delete(targetInfo.targetId);

We should assert that it was previously in the `Set`

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:133
> +	   this._swappedTargetIds.add(previousTargetId);

We should assert that it wasn't already in the `Set`

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:135
> +	  
this.dispatchEventToListeners(WI.TargetManager.Event.DidCommitProvisionalTarget
, {previousTargetId, targetInfo});

Ditto (125)

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:142
> +	  
this.dispatchEventToListeners(WI.TargetManager.Event.TargetDestroyed,
{targetId});

To test even more correctness, we could keep a copy of the `WI.Target` and pass
it in here instead of `targetId` so the test can confirm that `isDestroyed` is
the right value.

Although we may not always have a `WI.Target`, such as in the case of
`didCommitProvisionalTarget`, so maybe not.

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:145
> +    _targetCreated(parentTarget, targetInfo)

We should name this function and `_targetDestroyed` to be more active instead
of looking/sounding like an event listener callback:
 - `_targetCreated` => `_createTarget` (merging with the existing
`_createTarget`)
 - `_targetDestroyed` => `_destroyTarget`
This way, it also won't be as confusing trying to differentiate between
`targetCreated` and `_targetCreated`.

Also, this function and `_destroyTarget` should be moved into the `// Private`
section below.

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:152
> +	  
console.assert(!this._provisionalTargetIds.has(targetInfo.targetId));

I would actually move this assertion before the `if`, because we wouldn't want
the same provisional `targetId` to be added twice.

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:168
> +	       return;
>	   let target = this._targets.get(targetId);

Style: missing newline

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:177
> +	       return;
>	   let target = this._targets.get(targetId);

Style: missing newline

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:310
> +    // Events below are triggered only by corresponding protocol events.

This comment isn't really necessary.

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:313
> +    TargetCreated: Symbol("target-manager-target-created"),
> +    DidCommitProvisionalTarget:
Symbol("target-manager-provisional-target-committed"),
> +    TargetDestroyed: Symbol("target-manager-target-detroyed"),

NIT: I know that the other events are `Symbol`, but we really don't need to do
that, so please just make these strings.

> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:324
> +	       // Command may be sent after target is already destroyed on
backend,
> +	       // ignore such errors.

We can simplify this to be on one line
```
    // Ignore errors if the target was destroyed after the command was
dispatched.
```

> Source/WebInspectorUI/UserInterface/Protocol/Target.js:165
> +    destroy() { this._isDestroyed = true; }

Style: we only put functions onto a single line if they're simple getters. 
Since this is a function, please put the body on separate lines and move it to
be below all of the rest of the getters (I'd put this between `addScript` and
`hasDomain`).

> Source/WebInspectorUI/UserInterface/Views/QuickConsole.js:115
> +    _pageTargetTransisioned()

Typo: `_pageTargetTransisioned` => `_pageTargetTransitioned`

> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.cpp:37
> +Ref<WebAuthenticationPanel> WebAuthenticationPanel::create(const
AuthenticatorManager& manager, const WTF::String& rpId)

This should definitely be in a separate change.  Also, I think the more
"correct" solution would be to add an `#include <wtf/text/WTFString.h>` in the
.cpp and `#include <wtf/Forward.h>` in the .h

> Source/WebKit/UIProcess/WebPageInspectorController.cpp:191
> +    // FIXME(webkit.org/b/202937): do not destroy targets that belong to the
committed page.

NIT: our usual style is `// FIXME: <https://webkit.org/b/202937> do not destroy
targets that belong to the committed page`

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:6271
> +				37B47E2C1D64DB76005F4EFF /* objcSPI.h */,

Where did this come from?

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:-9412
> -				7AA746D523593D8100095050 /* SecItemSPI.h in
Headers */,

Ditto (6271)

>
LayoutTests/http/tests/inspector/target/target-events-for-provisional-page.html
:21
> +		   let targetInfo = event.data.targetInfo;

NIT: you can destructure this as `let {targetInfo} = event.data;` as you do
below

>
LayoutTests/http/tests/inspector/target/target-events-for-provisional-page.html
:35
> +		   let targetId = event.data.targetId;

Ditto (21)

>
LayoutTests/http/tests/inspector/target/target-events-for-provisional-page.html
:39
> +		   // Wait for page reload event to avoid race between test
results flushing and the test completion.
> +		   reloadPromise.then(resolve);

Nice catch!

NIT: I'd add a newline before the comment.