[Webkit-unassigned] [Bug 103867] [Resource Timing] Record redirectStart & redirectEnd time and expose in resource timing entries

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=103867





--- Comment #9 from James Simonsen <simonjam at chromium.org>  2013-01-15 17:01:08 PST ---
(From update of attachment 182559)
View in context: https://bugs.webkit.org/attachment.cgi?id=182559&action=review

> Source/WebCore/loader/SubresourceLoader.cpp:149
> +        m_resource->addRedirect();

We also need to check if this is a cross-origin redirect. If it is, then we must reset everything, including the start value stored in m_initiatorMap. See processing model step 19a.

> Source/WebCore/loader/cache/CachedResource.cpp:878
> +    m_lastRedirectReportTime = monotonicallyIncreasingTime();

This is supposed to be the equivalent to responseEnd, which is when the network stack receives the last byte. You should pull that value from the redirected response instead of using the current time.

> Source/WebCore/loader/cache/CachedResource.h:261
> +    double lastRedirectReportTime() const { return m_lastRedirectReportTime; }

Have you considered storing this in m_initiatorMap instead? It seems nice to keep all of the Resource Timing info together. Perhaps we should rename m_initiatorMap to better reflect that it's storing all of the Resource Timing info.

> Source/WebCore/page/PerformanceResourceTiming.cpp:120
> +    if (m_lastRedirectReportTime && !m_shouldReportDetails)

I'm going to go back on what I said in the last code review. Let's move passesTimingAllowOrigin() back to Performance.cpp and pass in m_shouldReportDetails as a bool. Then, we should just pass in the correct values for startTime and redirectEnd in the constructor. I think it'll be easier to read if all of that logic is in one place instead of sprinkled throughout these functions.

> Source/WebCore/page/PerformanceResourceTiming.cpp:134
> +    return (PerformanceEntry::startTime());

This shouldn't be inside (). Not sure why the old code was that way too.

> Source/WebCore/page/PerformanceResourceTiming.h:52
> +    static PassRefPtr<PerformanceResourceTiming> create(const AtomicString& initiatorType, const ResourceRequest& request, const ResourceResponse& response, double initiationTime, double lastRedirectReportTime, double finishTime, Document* requestingDocument)

It's really confusing that sometimes lastRedirectReportTime comes before finishTime in the args list and sometimes it's after it. Please pick one and be consistent in all the functions.

> LayoutTests/http/tests/w3c/webperf/submission/Intel/resource-timing/test_resource_timing_cross_origin_redirect_with_timing_allow_origin.html:25
> +                test_noless_than(entry.redirectEnd, entry.redirectStart, "redirectEnd should be no less than redirectStart in timing allowed cross-origin redirect!");

Isn't this the same as test_greater_or_equals? Why do we even have test_noless_than?

> LayoutTests/http/tests/w3c/webperf/submission/Intel/resource-timing/test_resource_timing_same_origin_redirect.html:15
> +            setup({explicit_done: true});

I don't think you need this if everything is tested during the load event. Same for the other tests.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.