[webkit-reviews] review granted: [Bug 122764] Web Inspector: Avoid using Runtime.executionContextCreated to figure out the iframe's contentDocument node. : [Attachment 384918] 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 122764: Web Inspector: Avoid using Runtime.executionContextCreated to
figure out the iframe's contentDocument node.
https://bugs.webkit.org/show_bug.cgi?id=122764

Attachment 384918: Patch

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




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

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

r=me, with some style comments

> Source/WebCore/inspector/InspectorInstrumentation.cpp:720
> +    if (PageRuntimeAgent* pageRuntimeAgent =
instrumentingAgents.pageRuntimeAgent())

NIT: `auto`

> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:22
> +	       await InspectorProtocol.awaitCommand({method:
"Runtime.enable"});

Style: please add a newline after

> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:23
> +	       let [eventPayload] = await Promise.all([

Style: please use a more descriptive name, like `executionContextCreatedEvent`

> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:27
> +		       params: {expression: "createFrame()"}

Style: we usually use backticks (`) for expressions that will be evaluated in
the inspected page, so it's more distinct and can be more easily identified
when scanning a file

> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:33
> +	       let resultPayload = await InspectorProtocol.awaitCommand({

Style: please use a more descriptive name, like `evaluateResult`

> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:37
> +		       expression: "document.body.textContent"

Ditto (27)

> LayoutTests/inspector/runtime/execution-context-in-scriptless-page.html:41
> +	       ProtocolTest.expectEqual(resultPayload.result.value, "No
JavaScript.", "Can evaluate in the subframe using new context.");

Style: our usual style is to make the expectation message an imperative
sentence, like "Should be able to evaluate in subframe."