Bug 119584 - [Gtk] URL printing code in DumpRenderTree doesn't match WTR or Mac DRT
Summary: [Gtk] URL printing code in DumpRenderTree doesn't match WTR or Mac DRT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Pena
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-08 11:25 PDT by Alexey Proskuryakov
Modified: 2013-08-15 06:14 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.33 KB, patch)
2013-08-13 09:44 PDT, Simon Pena
no flags Details | Formatted Diff | Diff
Patch (13.60 KB, patch)
2013-08-14 06:52 PDT, Simon Pena
no flags Details | Formatted Diff | Diff
Patch (14.57 KB, patch)
2013-08-14 08:46 PDT, Simon Pena
no flags Details | Formatted Diff | Diff
Patch (18.54 KB, patch)
2013-08-15 02:42 PDT, Simon Pena
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2013-08-08 11:25:50 PDT
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.
Comment 1 Simon Pena 2013-08-13 05:47:54 PDT
I can take a look at this
Comment 2 Simon Pena 2013-08-13 09:44:34 PDT
Created attachment 208644 [details]
Patch
Comment 3 Alexey Proskuryakov 2013-08-13 10:03:25 PDT
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 4 Mario Sanchez Prada 2013-08-13 13:59:43 PDT
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.
Comment 5 Simon Pena 2013-08-14 01:57:19 PDT
(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
Comment 6 Simon Pena 2013-08-14 02:57:37 PDT
(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
Comment 7 Simon Pena 2013-08-14 06:52:16 PDT
Created attachment 208722 [details]
Patch
Comment 8 Simon Pena 2013-08-14 06:54:32 PDT
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.
Comment 9 Simon Pena 2013-08-14 08:46:03 PDT
Created attachment 208732 [details]
Patch

Updated patch to remove a no longer used method
Comment 10 Simon Pena 2013-08-14 09:32:23 PDT
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.
Comment 11 Simon Pena 2013-08-14 09:45:21 PDT
(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.
Comment 12 Simon Pena 2013-08-14 09:50:27 PDT
(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 13 Gustavo Noronha (kov) 2013-08-14 17:42:19 PDT
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.
Comment 14 Simon Pena 2013-08-15 02:42:08 PDT
Created attachment 208799 [details]
Patch
Comment 15 Simon Pena 2013-08-15 02:42:57 PDT
(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 16 Gustavo Noronha (kov) 2013-08-15 06:03:56 PDT
Comment on attachment 208799 [details]
Patch

Thanks!
Comment 17 Simon Pena 2013-08-15 06:14:32 PDT
Comment on attachment 208799 [details]
Patch

Clearing flags on attachment: 208799

Committed r154098: <http://trac.webkit.org/changeset/154098>
Comment 18 Simon Pena 2013-08-15 06:14:40 PDT
All reviewed patches have been landed.  Closing bug.