Gtk DumpRenderTree has a function named pathFromSoupURI() that's meant to print file paths without exposing full local paths, which are machine specific. It needs to match other platforms, however there are some incompatibilities: - what should be printed is a URL relative to main frame URL, or just a file name if the resource is in a different directory; - instead of a check for file URLs, there is a negative check for http and ftp ones.
I can take a look at this
Created attachment 208644 [details] Patch
Comment on attachment 208644 [details] Patch I'm not sure if this fully matches other platforms yet. You could get paths with multiple components on other platforms, while here it's still %s/%s. It may be possible to model the code more closely after what WebKitTestRunner does.
Comment on attachment 208644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208644&action=review > Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1149 > + soup_uri_free(mainFrameUri); You can use GOwnPtr<SoupURI> for mainFrameUri, as provided by WebCore/platform/network/soup/GOwnPtrSoup.h, and get rid of this explicit free.
(In reply to comment #3) > (From update of attachment 208644 [details]) > I'm not sure if this fully matches other platforms yet. You could get paths with multiple components on other platforms, while here it's still %s/%s. > > It may be possible to model the code more closely after what WebKitTestRunner does. I will do that, thanks
(In reply to comment #4) > (From update of attachment 208644 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208644&action=review > > > Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1149 > > + soup_uri_free(mainFrameUri); > > You can use GOwnPtr<SoupURI> for mainFrameUri, as provided by WebCore/platform/network/soup/GOwnPtrSoup.h, and get rid of this explicit free. Thanks, I will change that in the next version
Created attachment 208722 [details] Patch
With these changes, 11 of the 15 failing tests in the GTK bots get fixed. Of the 4 remaining, 3 seem to be errors hiding in our previous implementation, while the other one is unrelated.
Created attachment 208732 [details] Patch Updated patch to remove a no longer used method
Comment on attachment 208732 [details] Patch Sorry for the noise, I accidentally r- the patch. Getting it back to r? The three tests that still fail after this patch is in are related to the AuthenticationChallenge: the password is not displayed. I am investigating that as well. If I fix it before this patch is reviewed and the changes make sense here, I will incorporate them too. If not, I will file a new bug for them.
(In reply to comment #10) > (From update of attachment 208732 [details]) > Sorry for the noise, I accidentally r- the patch. Getting it back to r? > > The three tests that still fail after this patch is in are related to the AuthenticationChallenge: the password is not displayed. I am investigating that as well. If I fix it before this patch is reviewed and the changes make sense here, I will incorporate them too. If not, I will file a new bug for them. The issue is that soup_uri_to_string doesn't include the password in the output.
(In reply to comment #11) > (In reply to comment #10) > > (From update of attachment 208732 [details] [details]) > > Sorry for the noise, I accidentally r- the patch. Getting it back to r? > > > > The three tests that still fail after this patch is in are related to the AuthenticationChallenge: the password is not displayed. I am investigating that as well. If I fix it before this patch is reviewed and the changes make sense here, I will incorporate them too. If not, I will file a new bug for them. > > The issue is that soup_uri_to_string doesn't include the password in the output. Regarding this, there is a soupURIToStringPreservingPassword method in TestRunnerGtk, so I guess it could be refactored so it is shared here as well, right?
Comment on attachment 208732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208732&action=review r- because of the suggested changes, and I think it would be great to include the fix for the password you mentioned, too > Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:83 > +static const char divider = '/'; I don't think it's necessary to use a const name just for a single usage, I'd use '/' directly, not like this is making the code more readable. > Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1151 > if (g_str_equal(uri->scheme, "http") || g_str_equal(uri->scheme, "ftp")) { This should be a check for !file scheme instead of http || ftp, according to ap. It's what EFL is doing as well, we should probably follow.
Created attachment 208799 [details] Patch
(In reply to comment #13) > (From update of attachment 208732 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208732&action=review > > r- because of the suggested changes, and I think it would be great to include the fix for the password you mentioned, too > > > Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:83 > > +static const char divider = '/'; > > I don't think it's necessary to use a const name just for a single usage, I'd use '/' directly, not like this is making the code more readable. > > > Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1151 > > if (g_str_equal(uri->scheme, "http") || g_str_equal(uri->scheme, "ftp")) { > > This should be a check for !file scheme instead of http || ftp, according to ap. It's what EFL is doing as well, we should probably follow. Thanks for your review, Gustavo. I addressed your comments and added the fix for the password as well.
Comment on attachment 208799 [details] Patch Thanks!
Comment on attachment 208799 [details] Patch Clearing flags on attachment: 208799 Committed r154098: <http://trac.webkit.org/changeset/154098>
All reviewed patches have been landed. Closing bug.