[webkit-reviews] review granted: [Bug 196280] Web Inspector: Crash when interacting with Template Content in Console : [Attachment 375273] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Yury Semikhatsky
<yurys at chromium.org>'s request for review:
Bug 196280: Web Inspector: Crash when interacting with Template Content in
Console
https://bugs.webkit.org/show_bug.cgi?id=196280

Attachment 375273: Patch

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




--- Comment #12 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 375273
  --> https://bugs.webkit.org/attachment.cgi?id=375273
Patch

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

> LayoutTests/inspector/dom/inspect-template-node-expected.txt:1
> +Test that document inside a template node can be passed to inspect()
function in the console and refernced as $0.

Typo: "refernced" => "referenced"

> LayoutTests/inspector/dom/inspect-template-node-expected.txt:7
> +PASS: evaluate an element in a template
> +PASS: resolved js object id to DOM node id
> +PASS: set $0 to the template element
> +PASS: evaluate $0
> +PASS: $0 value is correct

Normally these lines would start with capital letters and end in a period. We
aren't entirely consistent... but a capital letter would make the output read a
bit better!

> LayoutTests/inspector/dom/inspect-template-node.html:9
> +    function assertResponse(message, response) {

I'd probably change the order of arguments here. (response, message), so the
string is at the end like most other functions.

> LayoutTests/inspector/dom/inspect-template-node.html:10
> +	   InspectorProtocol.checkForError(response)

Style: semicolon

> LayoutTests/inspector/dom/inspect-template-node.html:23
> +	   InspectorProtocol.sendCommand("DOM.setInspectedNode", {nodeId},
function (response) {

Style: All of these `function (response) {` should not have a space. But you
could just arrow function them `(response) => {`