[webkit-reviews] review granted: [Bug 96474] Add status events on <link rel=prerender> elements. : [Attachment 178031] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Gavin Peters <gavinp at chromium.org>'s
request for review:
Bug 96474: Add status events on <link rel=prerender> elements.
https://bugs.webkit.org/show_bug.cgi?id=96474

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=178031&action=review


The bulk of this change is fine, but your unit tests are talking to WebCore
directly.  We try to avoid that whenever possible.  Would you be willing to
re-work you tests to use the WebKit API?

> Source/WebCore/platform/PrerenderClient.h:39
> +#if ENABLE(LINK_PRERENDER)

Usually we put these ifdefs around the whole header, but I guess this saves
ifdefs elsewhere?

> Source/WebKit/chromium/tests/PrerenderingTest.cpp:59
> +    TestPrerendererClient(WebView* webView)

explicit

> Source/WebKit/chromium/tests/PrerenderingTest.cpp:107
> +WebView* staticWebViewForTest()
> +{
> +    static WebView* webView = 0;
> +    if (!webView) {
> +	   webView = FrameTestHelpers::createWebViewAndLoad("about:blank");
> +	   webView->setFocus(true);
> +    }
> +    return webView;
> +}

Please don't use a static WebView.  That can cause state leaks between tests. 
If you copied this pattern from another test, can we remove this pattern from
that test in a followup patch?

> Source/WebKit/chromium/tests/PrerenderingTest.cpp:113
> +TestPrerendererClient* staticPrerendererClientForTest()
> +{
> +    static TestPrerendererClient* prerendererClient = new
TestPrerendererClient(staticWebViewForTest());
> +    return prerendererClient;
> +}

Please do not use statics.

> Source/WebKit/chromium/tests/PrerenderingTest.cpp:322
> +    RefPtr<WebCore::PrerenderHandle> handle = prerenderer()->render(&client,
toKURL("http://www.foo.com"));

We shouldn't talk directly to WebCore in unit tests.  We should use the WebKit
API to do all of our testing.

> Source/WebKit/chromium/tests/PrerenderingTest.cpp:401
> +    prerenderer()->suspend(WebCore::ActiveDOMObject::PageWillBeSuspended);

This isn't the right way to test this stuff.  You should test it via the API. 
There won't be any way to test suspect today in Chromium because Chromium never
calls suspend.