Bug 42704 – WebKitTestRunner needs to print history delegate information

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Sam Weinig]

WebKitTestRunner needs to print history delegate information

[Sam Weinig]

<rdar://problem/8213829>

[Mikhail Pozdnyakov]

Created attachment 167995 [details]
WIP patch

WIP as the given patch does no provide expected result for:
http/tests/globalhistory/history-delegate-basic-302-redirect.html
http/tests/globalhistory/history-delegate-basic-refresh-redirect.html

The reason for it is that client redirection data is logged earlier than expected because History Client is
dumping directly from WebKitTestRunner UI process. 

Alternative solution would be history bundle client creation, but I would like to know reviewer's opinions first whether we really need it
because adding of a history bundle client looks like a big change affecting public APIs and work of WebFrameLoaderClient.

[Kenneth Rohde Christiansen]

Comment on attachment 167995 [details]
WIP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167995&action=review

> Tools/WebKitTestRunner/TestController.cpp:531
> +    // Reset logging from History Client.
> +    m_shouldLogHistoryClient = false;

isn't it normally called something like m_shouldDump..

> Tools/WebKitTestRunner/TestController.cpp:1078
> +    // URL
> +    WKRetainPtr<WKURLRef> urlWK(AdoptWK, WKNavigationDataCopyURL(navigationData));
> +    WKRetainPtr<WKStringRef> urlStringWK(AdoptWK, WKURLCopyString(urlWK.get()));
> +    // Title
> +    WKRetainPtr<WKStringRef> titleWK(AdoptWK, WKNavigationDataCopyTitle(navigationData));
> +    // HTTP method
> +    WKRetainPtr<WKURLRequestRef> requestWK(AdoptWK, WKNavigationDataCopyOriginalRequest(navigationData));
> +    WKRetainPtr<WKStringRef> methodWK(AdoptWK, WKURLRequestCopyHTTPMethod(requestWK.get()));
> +    // Client redirect source
> +    WKRetainPtr<WKStringRef> clientRedirectSourceWK(AdoptWK, WKNavigationDataCopyClientRedirectSource(navigationData));
> +    bool wasRedirected = !WKStringIsEmpty(clientRedirectSourceWK.get());
> +    // Was failure
> +    WKRetainPtr<WKURLResponseRef> responseWK(AdoptWK, WKNavigationDataCopyResponse(navigationData));
> +    bool wasFailure = WKNavigationDataHasSubstituteData(navigationData) || WKURLResponseHTTPStatusCode(responseWK.get()) >= 400;

Please add newline before the comment. That might even make the comments unneeded as it makes it easier to read the code which is quite obvious.

Comments also ends with a punctuation mark (such as dot, question mark, etc etc)

> Tools/WebKitTestRunner/TestController.cpp:1080
> +    printf("WebView navigated to url \"%s\" with title \"%s\" with HTTP equivalent method \"%s\".  The navigation was %s and was %s%s.\n",

Shouldnt you write to the string builder?

[Mikhail Pozdnyakov]

>Tools/WebKitTestRunner/TestController.cpp:1080
> > +    printf("WebView navigated to url \"%s\" with title \"%s\" with HTTP equivalent method \"%s\".  The navigation was %s and was %s%s.\n",
> 
> Shouldnt you write to the string builder?
No, because I'm writing from TestController which is UI process directly (not from injected bundle). We have already dumping like this for example in runBeforeUnloadConfirmPanel() function and some other places, but this dumping is off course not with accordance with dumping from injected bundle, that is why 
 
http/tests/globalhistory/history-delegate-basic-302-redirect.html
http/tests/globalhistory/history-delegate-basic-refresh-redirect.html

cannot pass. 

So the question is: do we want the data to be dumped in expected order (and if so we need to create history bundle client which looks like a really big work), or we are glad with the existing output and will just rebaseline 2 mentioned tests.

I personally do not mind creating history bundle client in case it is needed not only for passing of those 2 tests but for something more meaningful.

[Andreas Kling]

Comment on attachment 167995 [details]
WIP patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.

[Mark Rowe (bdash)]

The ordering issue is simple enough to solve: use TestInvocation::outputText rather than printf to log the messages.

I'm going to post a tweaked version of the patch for review that a) fixes that issue, and b) excludes the new API on WKNavigationData for now, since it's not necessary to match the WebKit1 output.

[Mark Rowe (bdash)]

Created attachment 228019 [details]
Patch

[Sam Weinig]

Comment on attachment 228019 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=228019&action=review

> Tools/WebKitTestRunner/TestController.cpp:1324
> +    WKRetainPtr<WKURLRef> urlWK(AdoptWK, WKNavigationDataCopyURL(navigationData));
> +    WKRetainPtr<WKStringRef> urlStringWK(AdoptWK, WKURLCopyString(urlWK.get()));

I prefer the WKRetainPtr<WKURLRef> urlWK = adoptWK() style, as it matches the normal RetainPtr.

[Mark Rowe (bdash)]

Committed r166396: <http://trac.webkit.org/changeset/166396>