Bug 84883 – [Resource Timing] Expose timing information

• Component: Platform
• Status: RESOLVED CONFIGURATION CHANGED

[James Simonsen]

This is the main work to plumb the resource timing information to the Performance Timeline.

[James Simonsen]

Created attachment 152361 [details]
Patch

[WebKit Review Bot]

Comment on attachment 152361 [details]
Patch

Attachment 152361 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13238299

New failing tests:
http/tests/w3c/webperf/submission/resource-timing/html/test_resource_dynamic_insertion.html
http/tests/w3c/webperf/submission/resource-timing/html/test_resource_initiator_types.html
http/tests/w3c/webperf/submission/resource-timing/html/test_resource_redirects.html
http/tests/w3c/webperf/submission/resource-timing/html/test_resource_script_types.html
http/tests/w3c/webperf/submission/resource-timing/html/test_resource_attribute_order.html

[WebKit Review Bot]

Created attachment 152397 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[WebKit Review Bot]

Comment on attachment 152361 [details]
Patch

Attachment 152361 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13242317

New failing tests:
http/tests/w3c/webperf/submission/resource-timing/html/test_resource_dynamic_insertion.html
http/tests/w3c/webperf/submission/resource-timing/html/test_resource_initiator_types.html
http/tests/w3c/webperf/submission/resource-timing/html/test_resource_redirects.html
http/tests/w3c/webperf/submission/resource-timing/html/test_resource_script_types.html
http/tests/w3c/webperf/submission/resource-timing/html/test_resource_attribute_order.html

[WebKit Review Bot]

Created attachment 152401 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[James Simonsen]

Created attachment 152584 [details]
Patch

[James Simonsen]

Created attachment 166597 [details]
Patch

[Build Bot]

Comment on attachment 166597 [details]
Patch

Attachment 166597 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14132058

[Build Bot]

Comment on attachment 166597 [details]
Patch

Attachment 166597 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14137019

[James Simonsen]

Created attachment 166714 [details]
Patch

[Build Bot]

Comment on attachment 166714 [details]
Patch

Attachment 166714 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14130381

[Build Bot]

Comment on attachment 166714 [details]
Patch

Attachment 166714 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14126421

[James Simonsen]

Created attachment 166726 [details]
Patch

[Build Bot]

Comment on attachment 166726 [details]
Patch

Attachment 166726 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14131346

[James Simonsen]

Created attachment 166768 [details]
Patch

[Tony Gentilcore]

Comment on attachment 166768 [details]
Patch

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

Overall this patch looks good to me. Just a couple of nits and test suggestions.

I feel comfortable approving the tests and performance code, but the loader changes are fundamental enough that I'd prefer it if Nate, Adam or someone would take a look.

> Source/WebCore/css/CSSLoadInitiator.h:40
> +namespace WebCore {

Line break after namespace

> Source/WebCore/loader/ThreadableLoader.h:-44
> -    class ResourceError;

In such a large patch, I recommend resisting the urge to do unrelated style fixes.

> LayoutTests/ChangeLog:34
> +        * http/tests/w3c/webperf/submission/resource-timing/html/test_resource_script_types.html: Added.

Please add a test for reparenting nodes across documents.

> LayoutTests/http/tests/w3c/webperf/resources/all_resource_types.htm:39
> +        </ol>

Some more resource types to consider testing:
<applet archive>
<audio src>
<body background>
<embed src>
<input type=image src>
<object data>
<track src>
<video poster>
<video src>

Some things that might need a new test:
<frame src>
<html manifest>

> LayoutTests/http/tests/w3c/webperf/submission/resource-timing/html/test_resource_attribute_order-expected.txt:10
> +FAIL fetchStart should be non-zero assert_true: fetchStart should be non-zero expected true got false

Is this really expected right now?

[Adam Barth]

Comment on attachment 166768 [details]
Patch

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

> Source/WebCore/css/CSSLoadInitiator.h:51
> +    Document* m_document;

What ensures that this doesn't point to garbage memory?

[Adam Barth]

Comment on attachment 166768 [details]
Patch

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

> Source/WebCore/loader/LoadInitiator.h:43
> +    virtual ~LoadInitiator() { }

You've added this class as a base class to Node, which means you've added a new vtable to Node, growing its size by a pointer.

[James Simonsen]

Comment on attachment 166768 [details]
Patch

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

>> Source/WebCore/css/CSSLoadInitiator.h:40
>> +namespace WebCore {
> 
> Line break after namespace

Done.

>> Source/WebCore/css/CSSLoadInitiator.h:51
>> +    Document* m_document;
> 
> What ensures that this doesn't point to garbage memory?

Switched to a RefPtr.

Since LoadInitiators are temporaries, I figured it'd never point to garbage. But, there's probably some code path where requesting a resource destroys the document, so better safe than sorry.

>> Source/WebCore/loader/LoadInitiator.h:43
>> +    virtual ~LoadInitiator() { }
> 
> You've added this class as a base class to Node, which means you've added a new vtable to Node, growing its size by a pointer.

Good point. Changed the design so it no longer uses inheritance.

>> LayoutTests/ChangeLog:34
>> +        * http/tests/w3c/webperf/submission/resource-timing/html/test_resource_script_types.html: Added.
> 
> Please add a test for reparenting nodes across documents.

Good idea. Done.

>> LayoutTests/http/tests/w3c/webperf/resources/all_resource_types.htm:39
>> +        </ol>
> 
> Some more resource types to consider testing:
> <applet archive>
> <audio src>
> <body background>
> <embed src>
> <input type=image src>
> <object data>
> <track src>
> <video poster>
> <video src>
> 
> Some things that might need a new test:
> <frame src>
> <html manifest>

<body background>
<embed src>
<input type=image src>
<object data>
<video poster>
<frame src>

Done.

<applet archive>

This parameter is passed to the plugin to be loaded. WebKit has no visibility.

<audio src>
<track src>
<video src>

The spec intentionally omits media types until we resolve how to handle streaming, etc.

<html manifest>

This is passed to the embedder to be loaded. WebKit has no visibility. We might want to add hooks back into WebKit later to fix this. I think it should be a separate bug though.

>> LayoutTests/http/tests/w3c/webperf/submission/resource-timing/html/test_resource_attribute_order-expected.txt:10
>> +FAIL fetchStart should be non-zero assert_true: fetchStart should be non-zero expected true got false
> 
> Is this really expected right now?

Nope, fixed. But do note that DRT doesn't populate these so this test isn't especially useful anyway. It passes in Chrome too though.

[James Simonsen]

Created attachment 167880 [details]
Patch

[Tony Gentilcore]

Comment on attachment 167880 [details]
Patch

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

lgtm!

However, I'd feel better if abarth@ or japhet@ reviewed since this still touches stuff in loader.

> Source/WebCore/loader/DocumentLoadTiming.h:74
> +    double rawNavigationStart() const { return m_navigationStart; }

Too bad about this method. Is there a more descriptive term than "raw"? Better yet, is there a way to avoid exposing it here altogether?

> Source/WebCore/page/PerformanceResourceTiming.cpp:50
> +    if (!seconds)

Is this test necessary?

> LayoutTests/platform/chromium/TestExpectations:3794
> +# webkit.org/b/61138 http/tests/w3c/webperf/submission/resource-timing [ Skip ]

Don't forget to uncomment this line. Also, is it necessary for other platforms?

[James Simonsen]

Created attachment 168811 [details]
Patch

[James Simonsen]

(In reply to comment #21)
> (From update of attachment 167880 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167880&action=review
> 
> lgtm!
> 
> However, I'd feel better if abarth@ or japhet@ reviewed since this still touches stuff in loader.

Nate or Adam, any chance you could take a look at this (again)?

> 
> > Source/WebCore/loader/DocumentLoadTiming.h:74
> > +    double rawNavigationStart() const { return m_navigationStart; }
> 
> Too bad about this method. Is there a more descriptive term than "raw"? Better yet, is there a way to avoid exposing it here altogether?

Renamed to monotonicNavigationStart. Do you like that better? I don't think I can get rid of it entirely, but I'm open to other names.

> 
> > Source/WebCore/page/PerformanceResourceTiming.cpp:50
> > +    if (!seconds)
> 
> Is this test necessary?

Good point. Not anymore. The new monotonicTimeToZeroBasedDocumentTime will behave correctly.
> 
> > LayoutTests/platform/chromium/TestExpectations:3794
> > +# webkit.org/b/61138 http/tests/w3c/webperf/submission/resource-timing [ Skip ]
> 
> Don't forget to uncomment this line. Also, is it necessary for other platforms?

Done and done. The Apple ports skip the entire w3c directory, so no need to update those.

[Eric Seidel (no email)]

Attachment 168811 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1
LayoutTests/http/tests/w3c/webperf/resources/blank_image.png:0:  Have to enable auto props in the subversion config file (/Users/eseidel/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/Users/eseidel/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 94 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 168811 [details]
Patch

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

> Source/WebCore/css/CSSLoadInitiator.h:47
> +    CSSLoadInitiator(Document*);

Please mark these sort of one-argument constructors with the "explicit" keyword.

> Source/WebCore/css/CSSLoadInitiator.h:50
> +    virtual const WTF::AtomicString& initiatorName() const;

The "WTF::" prefix isn't needed here.

> Source/WebCore/dom/NodeLoadInitiator.h:51
> +    NodeLoadInitiator(Node*);
> +    virtual ~NodeLoadInitiator();
> +
> +    virtual const WTF::AtomicString& initiatorName() const;
> +    virtual Document* initiatorDocument() const;

Similar nits here.

> Source/WebCore/html/HTMLBodyElement.cpp:79
> +            style->setProperty(CSSProperty(CSSPropertyBackgroundImage, imageValue));

imageValue -> imageValue.release()

> Source/WebCore/html/parser/CSSPreloadScanner.h:38
> +class CSSPreloadScanner : private LoadInitiator {

We don't usually use private inheritance, but I guess it makes sense here...

> Source/WebCore/html/parser/CSSPreloadScanner.h:41
>      CSSPreloadScanner(Document*);

Unrelated: This constructor should be marked explicit.

> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:149
> +    Document* m_document;

How do we know this document will alive as long as this task is alive?

> Source/WebCore/loader/LoadInitiatorHandle.cpp:46
> +    , m_time(monotonicallyIncreasingTime())

So, the copy constructor gets a new time value?  That's confusing.  Can we make this operation explicit rather than implicit?

> Source/WebCore/loader/MainResourceLoader.cpp:534
> +        m_frame->ownerElement()->document()->domWindow()->performance()->addResourceTiming(NodeLoadInitiator(m_frame->ownerElement()), originalRequest(), m_response, documentLoader()->timing()->monotonicNavigationStart(), finishTime);

Why does this get added to the parent document's performance object?  It seems entirely possible that the navigation was initiated from within this frame.

> Source/WebCore/loader/TextTrackLoader.cpp:45
> -    
> +

I ran out of steam reviewing this CL around here.

[Adam Barth]

> 203.61 KB

This patch is a bit big.  :(

There are a bunch of reasons we'd like to know how a load was initiated.  We already have this concept in FrameLoadRequest, for example.

It's not clear to me that it makes sense to store this information in CachedResource, however, since CachedResources are shared by multiple requestors.  Consider an image that is being loaded by two different documents at the same time.  Which one of them is the anointed requestor?

The underlying issue seems to be that the CachedResourceLoader doesn't have a notion of a "request" object to wrap the lower level ResourceRequest object in the same way that FrameLoadRequests wraps ResourceRequest.

[James Simonsen]

(In reply to comment #26)
> > 203.61 KB
> 
> This patch is a bit big.  :(

Yeah, sorry. I broke up this feature as best I could. This one, unfortunately, is the biggest single chunk.

> There are a bunch of reasons we'd like to know how a load was initiated.  We already have this concept in FrameLoadRequest, for example.
> 
> It's not clear to me that it makes sense to store this information in CachedResource, however, since CachedResources are shared by multiple requestors.  Consider an image that is being loaded by two different documents at the same time.  Which one of them is the anointed requestor?

In the Resource Timing spec, it's the first one to request to it, even if the first document cancels the request or the document itself goes away.

> The underlying issue seems to be that the CachedResourceLoader doesn't have a notion of a "request" object to wrap the lower level ResourceRequest object in the same way that FrameLoadRequests wraps ResourceRequest.

That's fine with me too. I'd be happy to create a CachedResourceRequest to wrap this state. Is that what you want?

Should we try to merge it with FrameLoadRequest too?

[Adam Barth]

> In the Resource Timing spec, it's the first one to request to it, even if the first document cancels the request or the document itself goes away.

Does that make sense for other (future) uses of this information?  Having the initiator around is going to tempt future code into use it.  We should be sure we're doing something sensible.

> That's fine with me too. I'd be happy to create a CachedResourceRequest to wrap this state. Is that what you want?

I haven't thought through the design enough to know what the best approach is.  I'm just sharing what I was thinking when reviewing your patch.

> Should we try to merge it with FrameLoadRequest too?

I don't think that makes sense.  FrameLoadRequest's primary job is to hold m_frameName, which doesn't make sense for subresource loads.

I think the underlying issue here is that CachedResourceLoader::requestResource takes a lot of arguments instead of a CachedResourceRequest object.  I would probably start by creating the request object to hold that information and see where that approach leads.

I would also try to avoid adding a LoadInitiatorHandle member variable to CachedResource because the cached resource is shared between many initiators.  It's going to confuse future code if people think that there's a unique initiator for each CachedResource.

(Also, all the spurious whitespace changes in your patch make it harder to see what's going on.)

[James Simonsen]

(In reply to comment #28)
> I think the underlying issue here is that CachedResourceLoader::requestResource takes a lot of arguments instead of a CachedResourceRequest object.  I would probably start by creating the request object to hold that information and see where that approach leads.

Okay, I can do that and post a patch. It seems like a pretty straightforward refactoring. It shouldn't affect the behavior at all.

> I would also try to avoid adding a LoadInitiatorHandle member variable to CachedResource because the cached resource is shared between many initiators.  It's going to confuse future code if people think that there's a unique initiator for each CachedResource.

Well there is a unique initiator for the purposes of Resource Timing: it's always the first one. Are you implying that Resource Timing's definition doesn't belong in loader/? Or is "initiator" not a clear enough term to describe the first element making the request? Maybe I should call the argument passed in to requestResource() "requester" and the first requester becomes the "initiator."

Another ideas is to make PerformanceResourceTiming into an observer and have it store the LoadInitiatorHandles itself. Then we don't need CachedResource to hang on to its initiator.

[Adam Barth]

See bug 92761 for another use case for figuring out who initiated a load.

[Joseph Pecoraro]

This bug seems stale. Initiator information is working in the current implementation of Resource Timing, so this bug seems stale. Lets close.