[Resource Timing] Implement Resource Timing interface

• Component: New Bugs
• Status: RESOLVED FIXED

[James Simonsen]

[Resource Timing] Implement Resource Timing interface

[James Simonsen]

Created attachment 94141 [details]
Patch

[Eric Seidel (no email)]

8 months since this was posted.  No comment.  Do we really wan this? or can this bug be closed.

[James Simonsen]

I think we can move forward again.

We had the W3C security and privacy teams review it. I think Tony was going to report the results to the old thread on webkit-dev. Maybe he already did.

I can post a link in the metabug too.

FYI, I'm planning to finish up the implementation this quarter.

[James Simonsen]

Created attachment 139121 [details]
Patch

[Early Warning System Bot]

Comment on attachment 139121 [details]
Patch

Attachment 139121 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12560070

[Philippe Normand]

Comment on attachment 139121 [details]
Patch

Attachment 139121 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12549076

[James Simonsen]

Created attachment 139126 [details]
Patch

[Early Warning System Bot]

Comment on attachment 139126 [details]
Patch

Attachment 139126 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12554084

[James Simonsen]

Created attachment 139128 [details]
Patch

[Tony Gentilcore]

Comment on attachment 139128 [details]
Patch

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

Setting r- for now until we figure out the comment about initiator type/time.

Everything else is nit-picky.

> Source/WebCore/ChangeLog:6
> +        This patch implements the Resource Timing interface. It doesn't do anything

I recommend a link to the spec in the ChangeLog.

> Source/WebCore/page/Performance.cpp:63
> +    return frame()->document()->scriptExecutionContext();

Are we guaranteed a Frame here?

Also, Document is a ScriptExecutionContext, so is the scriptExecutionContext() call doing anything useful?

> Source/WebCore/page/Performance.cpp:94
> +    for (Vector<RefPtr<PerformanceEntry> >::const_iterator resource = m_resourceTimingBuffer.begin(); resource != m_resourceTimingBuffer.end(); ++resource)

Vector does have an append() that takes a Vector and appends all. Would it be cleaner to avoid the iterator here and modify PerformanceEntryList to take advantage of that? We seem to append all more than once.

> Source/WebCore/page/Performance.cpp:138
> +{

Add FIXME

> Source/WebCore/page/Performance.h:43
> +#include "ResourceResponse.h"

Can these both be forward declared?

> Source/WebCore/page/PerformanceResourceTiming.cpp:50
> +    case ResourceRequest::InitiatorIsCSS:

There was a recent thread about these. Are they being reworked in the spec?

> Source/WebCore/page/PerformanceResourceTiming.cpp:68
> +    case ResourceRequest::InitiatorIsUnknown:

Are there known unknowns? If so, this shouldn't hit the ASSERT_NOT_REACHED().

> Source/WebCore/page/PerformanceResourceTiming.h:36
> +#include "Performance.h"

Unused?

> Source/WebCore/page/PerformanceResourceTiming.h:38
> +#include "ResourceLoadTiming.h"

I always forget whether it is okay to forward declare something used as a RefPtr. You have it one way for Document and another way for ResourceLoadTiming. So either this can be forward declared or else we are pulling in Document.h transitively and it should be explicit.

> Source/WebCore/page/PerformanceResourceTiming.h:39
> +#include "ResourceRequest.h"

Forward declare like ResourceResponse?

> Source/WebCore/page/PerformanceResourceTiming.h:41
> +#include <wtf/RefCounted.h>

Should be wtf/RefPtr.h, right? And also add wtf/text/WTFString.h.

> Source/WebCore/platform/network/ResourceRequestBase.h:153
> +        double initiatedTime() const { return m_initiatedTime; }

The devtools timeline seems to already know initiatedTime and initiatorType. DevTools and ResourceTiming should probably share the same data store for this information.

Have you looked into how that works and whether any of these ResourceRequest modifications are absolutely necessary?

> Source/WebCore/platform/network/ResourceRequestBase.h:214
> +        double m_initiatedTime;

Worth enforcing const here?

[James Simonsen]

Created attachment 146451 [details]
Patch

[James Simonsen]

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

>> Source/WebCore/ChangeLog:6
>> +        This patch implements the Resource Timing interface. It doesn't do anything
> 
> I recommend a link to the spec in the ChangeLog.

Done.

>> Source/WebCore/page/Performance.cpp:63
>> +    return frame()->document()->scriptExecutionContext();
> 
> Are we guaranteed a Frame here?
> 
> Also, Document is a ScriptExecutionContext, so is the scriptExecutionContext() call doing anything useful?

Fixed both.

>> Source/WebCore/page/Performance.cpp:94
>> +    for (Vector<RefPtr<PerformanceEntry> >::const_iterator resource = m_resourceTimingBuffer.begin(); resource != m_resourceTimingBuffer.end(); ++resource)
> 
> Vector does have an append() that takes a Vector and appends all. Would it be cleaner to avoid the iterator here and modify PerformanceEntryList to take advantage of that? We seem to append all more than once.

Good idea. Done.

>> Source/WebCore/page/Performance.cpp:138
>> +{
> 
> Add FIXME

Done.

>> Source/WebCore/page/Performance.h:43
>> +#include "ResourceResponse.h"
> 
> Can these both be forward declared?

Yep.

>> Source/WebCore/page/PerformanceResourceTiming.cpp:50
>> +    case ResourceRequest::InitiatorIsCSS:
> 
> There was a recent thread about these. Are they being reworked in the spec?

They are. Should we match the current spec or what it's going to be? I could file a bug for the latter.

>> Source/WebCore/page/PerformanceResourceTiming.cpp:68
>> +    case ResourceRequest::InitiatorIsUnknown:
> 
> Are there known unknowns? If so, this shouldn't hit the ASSERT_NOT_REACHED().

No, AFAICT it shouldn't be reached.

>> Source/WebCore/page/PerformanceResourceTiming.h:36
>> +#include "Performance.h"
> 
> Unused?

Yep.

>> Source/WebCore/page/PerformanceResourceTiming.h:38
>> +#include "ResourceLoadTiming.h"
> 
> I always forget whether it is okay to forward declare something used as a RefPtr. You have it one way for Document and another way for ResourceLoadTiming. So either this can be forward declared or else we are pulling in Document.h transitively and it should be explicit.

Forward declaration seems fine so I made both that. I put a destructor in the .cpp where both are fully defined to be sure it'll work.

>> Source/WebCore/page/PerformanceResourceTiming.h:39
>> +#include "ResourceRequest.h"
> 
> Forward declare like ResourceResponse?

Done.

>> Source/WebCore/page/PerformanceResourceTiming.h:41
>> +#include <wtf/RefCounted.h>
> 
> Should be wtf/RefPtr.h, right? And also add wtf/text/WTFString.h.

Done.

>> Source/WebCore/platform/network/ResourceRequestBase.h:153
>> +        double initiatedTime() const { return m_initiatedTime; }
> 
> The devtools timeline seems to already know initiatedTime and initiatorType. DevTools and ResourceTiming should probably share the same data store for this information.
> 
> Have you looked into how that works and whether any of these ResourceRequest modifications are absolutely necessary?

I removed initiatedTime.

Inspector has two relevant fields to initiatorType: initiator and type. Initiator comes from inspecting state (is the parser running, is there a JS callstack). Type comes from the CachedResourceType. That's insufficient for our needs, because it can't identify the source element. So, I think we still need to add initiatorType.

Not sure if Inspector type should switch to initiatorType. It'll probably become harder once that's a node name though.

>> Source/WebCore/platform/network/ResourceRequestBase.h:214
>> +        double m_initiatedTime;
> 
> Worth enforcing const here?

No longer there.

[Early Warning System Bot]

Comment on attachment 146451 [details]
Patch

Attachment 146451 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12908555

[Early Warning System Bot]

Comment on attachment 146451 [details]
Patch

Attachment 146451 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12908559

[James Simonsen]

Created attachment 146466 [details]
Patch

[Early Warning System Bot]

Comment on attachment 146466 [details]
Patch

Attachment 146466 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12921405

[Early Warning System Bot]

Comment on attachment 146466 [details]
Patch

Attachment 146466 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12910537

[Gyuyoung Kim]

Comment on attachment 146466 [details]
Patch

Attachment 146466 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12908610

[James Simonsen]

Created attachment 147155 [details]
Patch

[James Simonsen]

Created attachment 147721 [details]
Patch

[Tony Gentilcore]

Comment on attachment 147721 [details]
Patch

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

> Source/WebCore/ChangeLog:12611
> [phone]  James Simonsen  <[e-mail]>

Merge problem

> Source/WebCore/page/PerformanceResourceTiming.cpp:107
> +        connectStart = m_timing->dnsEnd;

A lot of these conditions remind me of similar ones in Navigation Timing that should probably stay in sync in the future.

I don't have a specific suggestion, but Is there any reasonable way to factor out any commonality between the two?

[James Simonsen]

Created attachment 148852 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 148852 [details]
Patch for landing

Clearing flags on attachment: 148852

Committed r120962: <http://trac.webkit.org/changeset/120962>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.