Bug 61152 - [Resource Timing] Implement Resource Timing interface
Summary: [Resource Timing] Implement Resource Timing interface
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Simonsen
URL:
Keywords:
Depends on: 85025 80350
Blocks: 61138 84883
  Show dependency treegraph
 
Reported: 2011-05-19 16:04 PDT by James Simonsen
Modified: 2012-06-21 14:16 PDT (History)
17 users (show)

See Also:


Attachments
Patch (42.43 KB, patch)
2011-05-19 16:19 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (43.89 KB, patch)
2012-04-26 19:33 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (46.95 KB, patch)
2012-04-26 20:18 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (47.70 KB, patch)
2012-04-26 20:51 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (51.33 KB, patch)
2012-06-07 19:04 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (49.28 KB, patch)
2012-06-07 20:53 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (47.48 KB, patch)
2012-06-12 14:02 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (42.45 KB, patch)
2012-06-14 21:03 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch for landing (39.93 KB, patch)
2012-06-21 11:48 PDT, James Simonsen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Simonsen 2011-05-19 16:04:12 PDT
[Resource Timing] Implement Resource Timing interface
Comment 1 James Simonsen 2011-05-19 16:19:02 PDT
Created attachment 94141 [details]
Patch
Comment 2 Eric Seidel 2012-01-30 15:16:00 PST
8 months since this was posted.  No comment.  Do we really wan this? or can this bug be closed.
Comment 3 James Simonsen 2012-01-30 15:29:23 PST
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.
Comment 4 James Simonsen 2012-04-26 19:33:51 PDT
Created attachment 139121 [details]
Patch
Comment 5 Early Warning System Bot 2012-04-26 19:59:08 PDT
Comment on attachment 139121 [details]
Patch

Attachment 139121 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12560070
Comment 6 Philippe Normand 2012-04-26 20:01:49 PDT
Comment on attachment 139121 [details]
Patch

Attachment 139121 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12549076
Comment 7 James Simonsen 2012-04-26 20:18:00 PDT
Created attachment 139126 [details]
Patch
Comment 8 Early Warning System Bot 2012-04-26 20:43:57 PDT
Comment on attachment 139126 [details]
Patch

Attachment 139126 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12554084
Comment 9 James Simonsen 2012-04-26 20:51:24 PDT
Created attachment 139128 [details]
Patch
Comment 10 Tony Gentilcore 2012-06-01 10:06:50 PDT
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?
Comment 11 James Simonsen 2012-06-07 19:04:59 PDT
Created attachment 146451 [details]
Patch
Comment 12 James Simonsen 2012-06-07 19:06:54 PDT
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.
Comment 13 Early Warning System Bot 2012-06-07 20:08:01 PDT
Comment on attachment 146451 [details]
Patch

Attachment 146451 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12908555
Comment 14 Early Warning System Bot 2012-06-07 20:19:18 PDT
Comment on attachment 146451 [details]
Patch

Attachment 146451 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12908559
Comment 15 James Simonsen 2012-06-07 20:53:29 PDT
Created attachment 146466 [details]
Patch
Comment 16 Early Warning System Bot 2012-06-07 21:24:37 PDT
Comment on attachment 146466 [details]
Patch

Attachment 146466 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12921405
Comment 17 Early Warning System Bot 2012-06-07 22:09:41 PDT
Comment on attachment 146466 [details]
Patch

Attachment 146466 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12910537
Comment 18 Gyuyoung Kim 2012-06-08 00:24:55 PDT
Comment on attachment 146466 [details]
Patch

Attachment 146466 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12908610
Comment 19 James Simonsen 2012-06-12 14:02:06 PDT
Created attachment 147155 [details]
Patch
Comment 20 James Simonsen 2012-06-14 21:03:38 PDT
Created attachment 147721 [details]
Patch
Comment 21 Tony Gentilcore 2012-06-14 21:21:23 PDT
Comment on attachment 147721 [details]
Patch

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

> Source/WebCore/ChangeLog:12611
> +2012-04-25  James Simonsen  <simonjam@chromium.org>

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?
Comment 22 James Simonsen 2012-06-21 11:48:11 PDT
Created attachment 148852 [details]
Patch for landing
Comment 23 WebKit Review Bot 2012-06-21 14:15:45 PDT
Comment on attachment 148852 [details]
Patch for landing

Clearing flags on attachment: 148852

Committed r120962: <http://trac.webkit.org/changeset/120962>
Comment 24 WebKit Review Bot 2012-06-21 14:16:08 PDT
All reviewed patches have been landed.  Closing bug.