Bug 41462 – Move parseDataUrl() from CURL into ResourceHandle

• Component: Platform
• Status: RESOLVED FIXED

[Patrick R. Gansterer]

see patch

[Patrick R. Gansterer]

Created attachment 60240 [details]
Patch

[WebKit Review Bot]

Attachment 60240 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/network/ResourceHandle.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 60240 [details]
Patch

I’m not so comfortable with putting this function into the cross platform file because it's used by two back ends. There are many other back ends that don't use it. Should the other back ends be using it too? Is there any other place we could put it?

Naming the function "parse" seems wrong. Parsing is part of what it does, but it actually performs the loading as well. I think the name should be loadDataURL or handleDataURL. The word "parse" gives the sense of something that would output a parsed result, not something that would make actual loading-related calls to the client.

The parseDataURL function should not have a return value. Callers don't look at it.

It should be parseDataURL, not parseDataUrl.

Calling latin1() twice on the same string seems like a mistake. That ends up allocating a CString twice.

Does latin1() handle non-ASCII characters in the way that's correct for this function? Failing as it should? Is there a test case covering that?

I'm surprised that the reinterpret_cast works reliably. What makes the endianness always work out correctly?

The assertion about the URL's scheme should be a protocolIs assertion, not a custom startsWith call.

What data URL test cases do we have?

The ResourceResponse be populated with more, including the URL and the content length as expectedContentLength.

[Patrick R. Gansterer]

Created attachment 60351 [details]
Patch

(In reply to comment #3)
> (From update of attachment 60240 [details])
> I’m not so comfortable with putting this function into the cross platform file because it's used by two back ends. There are many other back ends that don't use it. Should the other back ends be using it too? Is there any other place we could put it?
It can be used by other backends too.
Maybe it can be used at http://trac.webkit.org/browser/trunk/WebCore/page/Page.cpp#L574 too?

> Naming the function "parse" seems wrong. Parsing is part of what it does, but it actually performs the loading as well. I think the name should be loadDataURL or handleDataURL. The word "parse" gives the sense of something that would output a parsed result, not something that would make actual loading-related calls to the client.
I didn't liked the name too. ;-)

> Calling latin1() twice on the same string seems like a mistake. That ends up allocating a CString twice.
I created an overload for base64Decode at bug 41510.

> What data URL test cases do we have?
DataURLs are tested indirect, because other test use it (e.g. embedding images).

Addressd the other points too.

[WebKit Review Bot]

Attachment 60351 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/network/ResourceHandle.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 60351 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3412059

[WebKit Review Bot]

Attachment 60351 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3389113

[WebKit Review Bot]

Attachment 60351 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3410088

[Martin Robinson]

Comment on attachment 60351 [details]
Patch

I do not think this is so useful as parseDataUrl in the soup backend is soon going to diverge greatly from the one in the cURL backend (see https://bugs.webkit.org/show_bug.cgi?id=44261 ). Feel free to CC me on a patch renaming this poorly-named method though.

[Patrick R. Gansterer]

Created attachment 69766 [details]
Patch

[WebKit Review Bot]

Attachment 69766 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/network/ResourceHandle.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Patrick R. Gansterer]

(In reply to comment #9)
> (From update of attachment 60351 [details])
> I do not think this is so useful as parseDataUrl in the soup backend is soon going to diverge greatly from the one in the cURL backend (see https://bugs.webkit.org/show_bug.cgi?id=44261 ). Feel free to CC me on a patch renaming this poorly-named method though.
The main reason for this change is that I need the same code in ResourceHandleWin.cpp, so I moved it to ResourceHandle.cpp. Even if you won't use it, ResourceHandleWin will use it (see bug 47165).

[Martin Robinson]

(In reply to comment #12)
> The main reason for this change is that I need the same code in ResourceHandleWin.cpp, so I moved it to ResourceHandle.cpp. Even if you won't use it, ResourceHandleWin will use it (see bug 47165).

Oh! Sharing here seems more appropriate than copy and paste. It probably makes sense to do this as one patch though. I think Darin is right that this should go somewhere else than the cross platform file though.

[Patrick R. Gansterer]

Created attachment 69809 [details]
Patch

[Patrick R. Gansterer]

@Darin: Can you have a look a the filename and say if it's ok to you?

[Darin Adler]

Comment on attachment 69809 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69809&action=review

I didn’t set commit-queue+ yet because I have some comments you may wish to address.

> WebCore/platform/network/DataURL.cpp:82
> +        if (data.length() > 0) {

Is this check really needed? Doesn’t TextEncoding::encode already handle a length of 0 correctly?

> WebCore/platform/network/DataURL.h:33
> +void handleDataURL(ResourceHandle* handle);

No need for the argument name “handle” here.

> WebCore/platform/network/win/ResourceHandleWin.cpp:353
> +    if (firstRequest().url().protocolIs("data")) {

It seems a shame that we have protocolIs("data") in so many places. Maybe protocolIsData should be added to KURL.h.

[Patrick R. Gansterer]

Created attachment 69853 [details]
Patch

(In reply to comment #16)
> Is this check really needed? Doesn’t TextEncoding::encode already handle a length of 0 correctly?
I removed the check.

> > WebCore/platform/network/DataURL.h:33
> > +void handleDataURL(ResourceHandle* handle);
> 
> No need for the argument name “handle” here.
Removed.

> > WebCore/platform/network/win/ResourceHandleWin.cpp:353
> > +    if (firstRequest().url().protocolIs("data")) {
> 
> It seems a shame that we have protocolIs("data") in so many places. Maybe protocolIsData should be added to KURL.h.
I've created bug 47219 for this.

[WebKit Commit Bot]

Comment on attachment 69853 [details]
Patch

Rejecting patch 69853 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 69853]" exit_code: 2
Cleaning working directory
Updating working directory
Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=69853&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=41462&ctype=xml
Processing 1 patch from 1 bug.
Processing patch 69853 from bug 41462.
Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/4247051

[Patrick R. Gansterer]

Comment on attachment 69853 [details]
Patch

(In reply to comment #18)
> (From update of attachment 69853 [details])
> Rejecting patch 69853 from commit-queue.
Patch works on my machine. Try again.

[WebKit Commit Bot]

Comment on attachment 69853 [details]
Patch

Rejecting patch 69853 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 69853]" exit_code: 2
Cleaning working directory
Updating working directory
Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=69853&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=41462&ctype=xml
Processing 1 patch from 1 bug.
Processing patch 69853 from bug 41462.
Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/4173092

[Patrick R. Gansterer]

Created attachment 69911 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 69911 [details]
Patch

Rejecting patch 69911 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 69911]" exit_code: 2
Cleaning working directory
Updating working directory
Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=69911&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=41462&ctype=xml
Processing 1 patch from 1 bug.
Processing patch 69911 from bug 41462.
Failed to run "[u'/Users/abarth/git/webkit-queue/WebKitTools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/4142110

[Patrick R. Gansterer]

Comment on attachment 69911 [details]
Patch

Clearing flags on attachment: 69911

Manually committed r69183: <http://trac.webkit.org/changeset/69183

[Patrick R. Gansterer]

All reviewed patches have been landed.  Closing bug.

[Kwang Yul Seo]

*** Bug 47189 has been marked as a duplicate of this bug. ***