<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - Decode data URLs in web process"
href="https://bugs.webkit.org/show_bug.cgi?id=148128">bug 148128</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #259618 Flags</td>
<td>review?
</td>
<td>review+
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Decode data URLs in web process"
href="https://bugs.webkit.org/show_bug.cgi?id=148128#c35">Comment # 35</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Decode data URLs in web process"
href="https://bugs.webkit.org/show_bug.cgi?id=148128">bug 148128</a>
from <span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=259618&action=diff" name="attach_259618" title="patch">attachment 259618</a> <a href="attachment.cgi?id=259618&action=edit" title="patch">[details]</a></span>
patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=259618&action=review">https://bugs.webkit.org/attachment.cgi?id=259618&action=review</a>
Looks good to me.
<span class="quote">> Source/WebCore/loader/ResourceLoader.cpp:210
> + if (url.protocolIsData()) {</span >
I think the code inside this if statement is long enough that it could be clearer if factored into a separate private named member function, perhaps named loadDataURL or startLoadingDataURL or something.
<span class="quote">> Source/WebCore/loader/ResourceLoader.cpp:222
> + ResourceResponse dataResponse { url, result.mimeType, dataSize, result.charset };</span >
If we had the right kind of constructor for ResourceResponse we could use WTF::move on all the arguments here to avoid a little bit of reference count churn.
<span class="quote">> Source/WebCore/platform/network/DataURLDecoder.cpp:53
> + DecodeTask(const String& urlString, StringView encodedData, bool isBase64, DecodeCompletionHandler completionHandler)
> + : urlString(urlString)
> + , encodedData(encodedData)
> + , isBase64(isBase64)
> + , completionHandler(completionHandler)
> + { }</span >
I don’t think we need this constructor. We should just be able to write:
std::make_unique<DecodeTask>({ ... });
below without using a constructor.
<span class="quote">> Source/WebCore/platform/network/DataURLDecoder.cpp:68
> + ASSERT(urlString.startsWith(dataString));</span >
This check needs to ignore ASCII case.
<span class="quote">> Source/WebCore/platform/network/DataURLDecoder.cpp:72
> + if (headerEnd == notFound)
> + return nullptr;</span >
Do we have a test case that covers this?
<span class="quote">> Source/WebCore/platform/network/DataURLDecoder.cpp:87
> + mimeType = "text/plain";</span >
I think ASCIILiteral("text/plain") will be slightly more efficient.
<span class="quote">> Source/WebCore/platform/network/DataURLDecoder.cpp:89
> + charset = "US-ASCII";</span >
I think ASCIILiteral("US-ASCII") will be slightly more efficient.
<span class="quote">> Source/WebCore/platform/network/DataURLDecoder.cpp:92
> + decodeTask->result.mimeType = mimeType;</span >
Please consider using WTF::move here to get rid of a tiny bit of reference count churn.
<span class="quote">> Source/WebCore/platform/network/DataURLDecoder.cpp:93
> + decodeTask->result.charset = charset;</span >
Please consider using WTF::move here to get rid of a tiny bit of reference count churn.
<span class="quote">> Source/WebCore/platform/network/DataURLDecoder.cpp:104
> + if (!base64URLDecode(task.encodedData.toStringWithoutCopying(), buffer)) {
> + // Didn't work, try unescaping and decoding as base64.
> + auto unescapedString = decodeURLEscapeSequences(task.encodedData.toStringWithoutCopying());</span >
This toStringWithoutCopying thing seems a bit ugly, and could be avoided if the functions took StringView instead of const String& arguments.
<span class="quote">> Source/WebCore/platform/network/DataURLDecoder.cpp:116
> + auto buffer = decodeURLEscapeSequencesAsData(task.encodedData.toStringWithoutCopying(), encoding);</span >
Same thought about toStringWithoutCopying.
<span class="quote">> Source/WebCore/platform/network/DataURLDecoder.cpp:117
> + task.result.data = SharedBuffer::create(buffer.data(), buffer.size());</span >
Why not use SharedBuffer::adoptVector here?
Stray space here, after the "=".
<span class="quote">> Source/WebCore/platform/network/DataURLDecoder.cpp:147
> + decodeTask->completionHandler(decodeTask->result);</span >
Please consider using WTF::move here to get rid of a tiny bit of reference count churn.
<span class="quote">> Source/WebCore/platform/network/DataURLDecoder.h:49
> +void decode(const URL&, DecodeCompletionHandler);</span >
Don’t functions sometimes have state and are thus expensive to copy? If so, it might be nicer to take ownership of the completion handler rather than passing it by value.
<span class="quote">> Source/WebCore/platform/text/DecodeEscapeSequences.h:158
> +inline Vector<char> decodeURLEscapeSequencesAsData(const String& string, const TextEncoding& encoding)</span >
I think it would be better to have this take a StringView instead of a String. Should be a simple matter of changing the URLEscapeSequence functions to work on StringView.
<span class="quote">> Source/WebCore/platform/text/DecodeEscapeSequences.h:170
> + encodedRunPosition = length;</span >
I’m not sure this line of code is needed. We intentionally made encodedRunPosition a very large value so it doesn’t need to be explicitly checked for. Using StringView::substring with it should work because it clamps its arguments down to the length of the string. It’s even possible, depending on how URLEscapeSequence::findEndOfRun is written, that we could remove the if statement entirely.
Maybe you don’t want to write code that relies on that. Maybe some day we’ll change the return values to be Optional<size_t> instead to make things like that more clear.
<span class="quote">> Source/WebCore/platform/text/DecodeEscapeSequences.h:181
> + auto stringFragment = StringView(string).substring(decodedPosition, encodedRunPosition - decodedPosition);
> + auto encodedStringFragment = encoding.encode(StringView(string).substring(decodedPosition, encodedRunPosition - decodedPosition), URLEncodedEntitiesForUnencodables);</span >
Looks like when refactoring this you forgot to use the stringFragment local. I suggest either getting rid of it or using it.
<span class="quote">> Source/WebCore/platform/text/DecodeEscapeSequences.h:184
> + if (encodedRunPosition == length)</span >
If you make the change I suggested above this would be encodedRunPosition == notFound.
<span class="quote">> Source/WebCore/platform/text/DecodeEscapeSequences.h:185
> + return result;</span >
Should this be doing some kind of “shrink to fit” thing?
<span class="quote">> Source/WebCore/platform/text/DecodeEscapeSequences.h:195
> + ASSERT_NOT_REACHED();
> + return { };</span >
I’m surprised this is needed. Don’t our compilers recognize the infinite loop?
<span class="quote">> Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:155
> + resourceLoader->start();
> m_webResourceLoaders.set(identifier, WebResourceLoader::create(resourceLoader));</span >
I know it’s only one or two lines of code, but I think a helper function might be nice for the m_webResourceLoaders thing that’s now repeated four times.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>