Bug 119584 – [Gtk] URL printing code in DumpRenderTree doesn't match WTR or Mac DRT

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Alexey Proskuryakov]

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.

[Simon Pena]

I can take a look at this

[Simon Pena]

Created attachment 208644 [details]
Patch

[Alexey Proskuryakov]

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.

[Mario Sanchez Prada]

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.

[Simon Pena]

(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

[Simon Pena]

(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

[Simon Pena]

Created attachment 208722 [details]
Patch

[Simon Pena]

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.

[Simon Pena]

Created attachment 208732 [details]
Patch

Updated patch to remove a no longer used method

[Simon Pena]

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.

[Simon Pena]

(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.

[Simon Pena]

(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?

[Gustavo Noronha (kov)]

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.

[Simon Pena]

Created attachment 208799 [details]
Patch

[Simon Pena]

(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.

[Gustavo Noronha (kov)]

Comment on attachment 208799 [details]
Patch

Thanks!

[Simon Pena]

Comment on attachment 208799 [details]
Patch

Clearing flags on attachment: 208799

Committed r154098: <http://trac.webkit.org/changeset/154098>

[Simon Pena]

All reviewed patches have been landed.  Closing bug.