[webkit-reviews] review denied: [Bug 90675] Web Inspector: implement testing harness for pure protocol tests. : [Attachment 163579] 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 163579: Patch
https://bugs.webkit.org/attachment.cgi?id=163579&action=review

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


> Source/WebCore/testing/Internals.cpp:1130
> +    m_frontendWindow = adoptPtr(window->open(url, "", "", window,
window).get());

No need to call .get() and then adoptPtr simply m_frontendWindow =
window->open(url, "", "", window, window);

> Source/WebCore/testing/Internals.cpp:1138
> +   
frontendPage->inspectorController()->setInspectorFrontendClient(m_frontendClien
t.release());

You should use a local variable to store the client here instead of
m_frontendClient field since you clear m_frontendClient at this line.

> Source/WebCore/testing/Internals.cpp:1144
> +    frontendPage->mainFrame()->script()->evaluate(ScriptSourceCode(script));


Now that we create full-fledged window for the front-end, we could try to use
window.postMessage to communicate with it.

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

You should also call m_frontendWindow.clear().

> Source/WebCore/testing/Internals.h:232
> +    OwnPtr<DOMWindow> m_frontendWindow;

It must be RefPtr. We should probably fix OwnPtr to never take a pointer to a
reference counted object.