|Summary:||WebKitTestRunner needs to print history delegate information|
|Product:||WebKit||Reporter:||Sam Weinig <sam>|
|Component:||WebKit2||Assignee:||Mark Rowe (bdash) <mrowe>|
|Severity:||Normal||CC:||ap, kenneth, mikhail.pozdnyakov|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.5|
Description Sam Weinig 2010-07-20 15:19:14 PDT
WebKitTestRunner needs to print history delegate information
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 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.