[webkit-reviews] review granted: [Bug 171394] Zero out PerformanceResourceTiming properties for cached cross-origin responses without Timing-Allow-Origin : [Attachment 308496] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 171394: Zero out PerformanceResourceTiming properties for cached
cross-origin responses without Timing-Allow-Origin
https://bugs.webkit.org/show_bug.cgi?id=171394

Attachment 308496: [PATCH] Proposed Fix

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




--- Comment #22 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 308496
  --> https://bugs.webkit.org/attachment.cgi?id=308496
[PATCH] Proposed Fix

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

> LayoutTests/imported/w3c/ChangeLog:11
> +	   requests include timing data or not.

What is the issue with WPT and the disk cache?
Some fetch API tests are doing disk cache related stuff.
Memory cache is not caching fetch/xhr but we should be able to exercise this on
other request types.

>
LayoutTests/imported/w3c/web-platform-tests/resource-timing/rt-revalidate-reque
sts.html:9
> +<script src="resources/rt-utilities.js?pipe=sub"></script>

Nit: rename rt-utilities.js as rt-utilities.sub.js to remove pipe=sub.

>
LayoutTests/imported/w3c/web-platform-tests/resource-timing/rt-revalidate-reque
sts.html:23
> +    const path = "resource-timing/resources/rt-revalidation-response.py";

Is there any reason to use let above and const here?

>
LayoutTests/imported/w3c/web-platform-tests/resource-timing/rt-revalidate-reque
sts.html:115
> +	   assertDisallowedTimingData(entry);

Could call assertAlways from assertAllowedTimingData/assertDisallowedTimingData
and have this as a oneliner.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:781
> +		   ResourceTiming resourceTiming =
ResourceTiming::fromCache(url, request.initiatorName(), loadTiming,
resource->response(), origin);

The resource has an origin and should know whether it is cross origin or not.
I wonder whether we should not punt on the CachedResource to know whether it
should allow resource timing.
That would mean moving passesTimingAllowCheck to CachedResource if it is not
used elsewhere.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:783
> +		       ASSERT(request.origin());

Seems fine but what is the exact purpose here?
If we were to catch this, wouldn't we want to check that directly when entering
requestResource?