[webkit-reviews] review denied: [Bug 55482] Web Inspector: introduce protocol test for RuntimeAgent. : [Attachment 84249] [patch] initial version

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Ilya Tikhonovsky
<loislo at chromium.org>'s request for review:
Bug 55482: Web Inspector: introduce protocol test for RuntimeAgent.
https://bugs.webkit.org/show_bug.cgi?id=55482

Attachment 84249: [patch] initial version
https://bugs.webkit.org/attachment.cgi?id=84249&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=84249&action=review

> LayoutTests/http/tests/inspector/inspector-test.js:226
> +InspectorTest.dumpStepResult = function(step, expression, result)

Please use InspectorTest.runTestSuite instead (look up for the syntax in other
tests).

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:5
> +was called: "RuntimeAgent.evaluate('true', 'test', false,
InspectorTest.callback)"

Given that this is a protocol test, it would be nice to dump both: protocol
request and response:

{
    request : {
    }

    response : {
    }
}

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:8
> +    objectId : null

We should stop sending objectId and hasChildren for primitive values.

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:19
> +    objectId : {

Form valid JSON (with "," - need to patch addObject).

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:20
> +	   injectedScriptId : 1

This can become the source of flake. However, if it does not, lets leave it
like this.
Btw, addObject allows printing types of members with given names instead of
their values.

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:21
> +	   id : 1

This can become the source of flake

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:22
> +	   groupName : "test"

I think we can safely remove groupName from objectId. We would need an id ->
groupName map in injected script for getProperties that would be cleared
whenever id is cleared.

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:34
> +    $ : true

Two problems here:
1) it does not list getElementBy*
2) We should really send these in array of strings. That was we would be able
to send "__proto__" suggestion. Should be a separate patch though.

Also, it is worth sending getCompletions('windo') and passing 'false' in
includeCommandLineAPI.

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:68
> +result = true

result should be success: true, right?

> LayoutTests/inspector/protocol/runtime-agent-expected.txt:124
> +was called: "RuntimeAgent.getProperties(testObjectId, false, false,
InspectorTest.callback)"

is this a dupe?