see patch
Created attachment 60240 [details] Patch
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.
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.
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.
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.
Attachment 60351 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3412059
Attachment 60351 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/3389113
Attachment 60351 [details] did not build on win: Build output: http://webkit-commit-queue.appspot.com/results/3410088
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.
Created attachment 69766 [details] Patch
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.
(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).
(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.
Created attachment 69809 [details] Patch
@Darin: Can you have a look a the filename and say if it's ok to you?
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.
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.
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 commit-queue@webkit.org... 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
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.
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 commit-queue@webkit.org... 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
Created attachment 69911 [details] Patch
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 commit-queue@webkit.org... 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
Comment on attachment 69911 [details] Patch Clearing flags on attachment: 69911 Manually committed r69183: <http://trac.webkit.org/changeset/69183
All reviewed patches have been landed. Closing bug.
*** Bug 47189 has been marked as a duplicate of this bug. ***