[webkit-reviews] review granted: [Bug 226678] Move Timing-Allow-Origin checks to the network process : [Attachment 430974] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 226678: Move Timing-Allow-Origin checks to the network process
https://bugs.webkit.org/show_bug.cgi?id=226678

Attachment 430974: Patch

https://bugs.webkit.org/attachment.cgi?id=430974&action=review




--- Comment #11 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 430974
  --> https://bugs.webkit.org/attachment.cgi?id=430974
Patch

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

> Source/WebCore/platform/network/TimingAllowOrigin.cpp:42
> +    const String& timingAllowOriginString =
response.httpHeaderField(HTTPHeaderName::TimingAllowOrigin);

auto& ?

> Source/WebCore/platform/network/TimingAllowOrigin.cpp:43
> +    const String& securityOrigin = initiatorSecurityOrigin.toString();

ditto.

> Source/WebCore/platform/network/TimingAllowOrigin.cpp:44
> +    for (auto& originWithSpace : timingAllowOriginString.split(',')) {

Wouldn't it be more efficient to iterate over
StringView(timingAllowOriginString).split(',') ?

> Source/WebCore/platform/network/TimingAllowOrigin.cpp:45
> +	   auto origin =
stripLeadingAndTrailingHTTPSpaces(StringView(originWithSpace));

Since you want StringViews anyway?

> Source/WebCore/platform/network/TimingAllowOrigin.h:33
> +WEBCORE_EXPORT bool passesTimingAllowOriginCheck(const ResourceResponse&,
const WebCore::SecurityOrigin& initiatorSecurityOrigin);

WebCore:: is unnecessary.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:525
> +void ResourceHandle::platformLoadResourceSynchronously(NetworkingContext*
context, const ResourceRequest& request,  StoredCredentialsPolicy
storedCredentialsPolicy, SecurityOrigin* sourceOrigin, ResourceError& error,
ResourceResponse& response, Vector<uint8_t>& data)

extra space before StoredCredentialsPolicy

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:540
> +    RefPtr<ResourceHandle> handle = adoptRef(new ResourceHandle(context,
request, &client, defersLoading, shouldContentSniff,
shouldContentEncodingSniff, sourceOrigin, false));

A comment by the 'false' to clarify what it means or an enum class would be
nice

>
LayoutTests/imported/w3c/web-platform-tests/resource-timing/buffer-full-inspect
-buffer-during-callback-expected.txt:2
> +Harness Error (TIMEOUT), message = null

Please skip in TestExpectations.

>
LayoutTests/imported/w3c/web-platform-tests/resource-timing/buffer-full-set-to-
current-buffer-expected.txt:2
> +Harness Error (TIMEOUT), message = null

Please skip in TestExpectations.

>
LayoutTests/imported/w3c/web-platform-tests/resource-timing/document-domain-no-
impact-opener-expected.txt:2
> +Harness Error (TIMEOUT), message = null

Please skip test in TestExpectations to avoid slowing runs.