[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 bugzilla-daemon at webkit.org
Fri Apr 28 08:38:46 PDT 2017

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

Attachment 308496: [PATCH] Proposed Fix


--- 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.

> +<script src="resources/rt-utilities.js?pipe=sub"></script>

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

> +    const path = "resource-timing/resources/rt-revalidation-response.py";

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

> +	   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

More information about the webkit-reviews mailing list