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

[bugzilla-daemon at webkit.org]

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 164608: Patch
https://bugs.webkit.org/attachment.cgi?id=164608&action=review

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


> Source/WebCore/WebCore.exp.in:2235
> +__ZNK7WebCore9DOMWindow8documentEv

Please keep the entries sorted.

> Source/WebCore/inspector/test-front-end/protocol-test.html:2
> +Copyright (C) 2012 Samsung Electronics. All rights reserved.

To avoid running the protocol-test.html as a layout test you can put it under
resources/ folder. So it would be something like
LayoutTests/http/tests/inspector-protocol/resources/protocol-test.html

> Source/WebCore/inspector/test-front-end/protocol-test.html:30
> +window.addEventListener("message", function(event) {

This code can be moved to InspectorTest.js

Also we may need to send more messages from the inspected page to the front-end
but I believe this function can be changed then.

> Source/WebCore/testing/Internals.cpp:1147
> +    m_frontendWindow->resetUnlessSuspendedForPageCache();

Is it really needed? I'd expect that m_frontendWindow->close should be enough.

> Source/WebCore/testing/Internals.cpp:1148
> +    m_frontendWindow->close(m_frontendWindow->scriptExecutionContext());

Also you should clear m_frontendWindow reference.