[webkit-reviews] review denied: [Bug 90675] Web Inspector: implement testing harness for pure protocol tests. : [Attachment 160949] Patch

Wed Aug 29 01:09:08 PDT 2012

Yury Semikhatsky <yurys at chromium.org> has denied Vivek Galatage
<vivekgalatage at gmail.com>'s request for review:
Bug 90675: Web Inspector: implement testing harness for pure protocol tests.
https://bugs.webkit.org/show_bug.cgi?id=90675

Attachment 160949: Patch
https://bugs.webkit.org/attachment.cgi?id=160949&action=review

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=160949&action=review

> Source/WebCore/testing/Internals.cpp:118
> +    void attachWindow() { }

Although C++ doesn't require this, please preserve 'virtual' modifier on
overriden virtual methods like we do in other places and also add OVERRIDE
modifier to them.

> Source/WebCore/testing/Internals.cpp:1126
> +    static EmptyFrameLoaderClient* frameLoaderClient = adoptPtr(new
EmptyFrameLoaderClient()).leakPtr();

I see that it is implemented this way in other places too but it would be nice
to find an owner for the object instead of leaking it in a static variable. The
owner would have to implemented FrameDestructionObserver I believe and would
likely add more complexity to the code but I really don't like the fact that we
need to create a leaking client here.

>
LayoutTests/http/tests/inspector-protocol/css-getSupportedCSSProperties.html:1
> +<html>

This test should be under LayoutTests/inspector-protocol since it doesn't
depend on running web server.

>
LayoutTests/http/tests/inspector-protocol/css-getSupportedCSSProperties.html:9
> +	   if (p1.name == p2.name)

== -> ===

> LayoutTests/http/tests/inspector-protocol/protocol-test.js:27
> +    WebInspector = {};

Wrong alignment, should start from the the beginning of the line. See
LayoutTests/http/tests/inspector/inspector-test.js for example.

> LayoutTests/http/tests/inspector-protocol/protocol-test.js:46
> +    var message = "{\"method\": \"" + method + "\",\"params\":{" +
parameters.join() + "},\"id\":"+ (this._id++) +"}";

I would create a JSON object and use JSON.stringify instead.

> LayoutTests/http/tests/inspector-protocol/protocol-test.js:65
> +InspectorTest.setEventHandler = function(handler)

The method is unused. Please remove it and other unused.

> LayoutTests/http/tests/inspector-protocol/protocol-test.js:83
> +    this._listeners[method] = handler;    

This looks like unnecessary complication. Why not use direct mapping requestId
-> handler? Also can we reuse InspectorBackend.js + InspectorBackendCommands.js
that we currently use in the front-end to parse and dispatch protocol
messsages? The API should be generic enough to use it in the protocol test
harness.