Bug 42704 - WebKitTestRunner needs to print history delegate information
Summary: WebKitTestRunner needs to print history delegate information
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Mark Rowe (bdash)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-07-20 15:19 PDT by Sam Weinig
Modified: 2014-03-27 21:07 PDT (History)
3 users (show)

See Also:


Attachments
WIP patch (18.92 KB, patch)
2012-10-10 07:13 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
Patch (12.40 KB, patch)
2014-03-27 19:50 PDT, Mark Rowe (bdash)
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2010-07-20 15:19:14 PDT
WebKitTestRunner needs to print history delegate information
Comment 1 Sam Weinig 2010-07-20 15:30:21 PDT
<rdar://problem/8213829>
Comment 2 Mikhail Pozdnyakov 2012-10-10 07:13:04 PDT
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.
Comment 3 Kenneth Rohde Christiansen 2012-10-10 14:43:02 PDT
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?
Comment 4 Mikhail Pozdnyakov 2012-10-11 07:38:30 PDT
>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.
Comment 5 Andreas Kling 2014-02-05 11:00:24 PST
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.
Comment 6 Mark Rowe (bdash) 2014-03-27 17:56:44 PDT
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.
Comment 7 Mark Rowe (bdash) 2014-03-27 19:50:12 PDT
Created attachment 228019 [details]
Patch
Comment 8 Sam Weinig 2014-03-27 20:10:48 PDT
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.
Comment 9 Mark Rowe (bdash) 2014-03-27 21:07:24 PDT
Committed r166396: <http://trac.webkit.org/changeset/166396>