[Web Timing] Add a vendor-prefixed Performance Timeline API

[James Simonsen]

[Web Timing] Add a vendor-prefixed Performance Timeline API

[James Simonsen]

Created attachment 130236 [details]
Patch

[Build Bot]

Comment on attachment 130236 [details]
Patch

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

[James Simonsen]

Created attachment 130250 [details]
Patch

[Early Warning System Bot]

Comment on attachment 130250 [details]
Patch

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

[James Simonsen]

Created attachment 130478 [details]
Patch

[Hajime Morrita]

Comment on attachment 130478 [details]
Patch

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

Basically looks good.

> Source/WebCore/page/Performance.cpp:76
> +PassRefPtr<PerformanceEntryList> Performance::webkitGetEntriesByType(const String& entryType)

Could you omit the name of unused parameters?
Some compiler configuration complains it.

> Source/WebCore/page/Performance.cpp:82
> +PassRefPtr<PerformanceEntryList> Performance::webkitGetEntriesByName(const String& name, const String& entryType)

Ditto for param name.

[James Simonsen]

Created attachment 132171 [details]
Patch

[James Simonsen]

(In reply to comment #6)
> (From update of attachment 130478 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130478&action=review
> 
> Basically looks good.
> 
> > Source/WebCore/page/Performance.cpp:76
> > +PassRefPtr<PerformanceEntryList> Performance::webkitGetEntriesByType(const String& entryType)
> 
> Could you omit the name of unused parameters?
> Some compiler configuration complains it.

Done.

> > Source/WebCore/page/Performance.cpp:82
> > +PassRefPtr<PerformanceEntryList> Performance::webkitGetEntriesByName(const String& name, const String& entryType)
> 
> Ditto for param name.

Done.

[James Simonsen]

Thanks for taking a look.

Tony, did you want to review this too?

[Tony Gentilcore]

Comment on attachment 132171 [details]
Patch

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

The patch looks correct to me. Is the webkit-dev thread resolved now?

> Source/WebCore/page/PerformanceEntryList.cpp:34
> +#if ENABLE(WEB_TIMING)

Should we consider separating out the flags for NAVIGATION_TIMING and PERFORMANCE_TIMELINE?

[James Simonsen]

Created attachment 133173 [details]
Patch

[James Simonsen]

(In reply to comment #10)
> (From update of attachment 132171 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132171&action=review
> 
> The patch looks correct to me. Is the webkit-dev thread resolved now?

I've pinged it. It's somewhat separate from Resource Timing, but I mentioned it anyway.

> 
> > Source/WebCore/page/PerformanceEntryList.cpp:34
> > +#if ENABLE(WEB_TIMING)
> 
> Should we consider separating out the flags for NAVIGATION_TIMING and PERFORMANCE_TIMELINE?

I've got 3 flags now:

1. WEB_TIMING: Enables window.performance and Navigation Timing.
2. PERFORMANCE_TIMELINE: Requires WEB_TIMING. Adds Performance Timeline. Inferred that Navigation Timing is included in the timeline, because WEB_TIMING is true.
3. RESOURCE_TIMING: Requires PERFORMANCE_TIMELINE. Adds Resource Timing to the timeline.

[Tony Gentilcore]

Comment on attachment 133173 [details]
Patch

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

Looks really good. With the webkit-dev thread satisfied and the spec at last call, I agree it is time to get this moving.

> Source/WebCore/ChangeLog:14
> +        Test: fast/dom/Window/window-properties-performance.html

This doesn't really test it yet since it is disabled by default.

Perhaps the log could be more clear about that and the fact that this is off-by-default.

> Source/WebCore/page/PerformanceEntry.h:57
> +    double m_duration;

Is it worth making this completely immutable (all data members const)?

[James Simonsen]

(In reply to comment #13)
> > Source/WebCore/page/PerformanceEntry.h:57
> > +    double m_duration;
> 
> Is it worth making this completely immutable (all data members const)?

Yeah, sounds good.

[James Simonsen]

Created attachment 138887 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 138887 [details]
Patch for landing

Clearing flags on attachment: 138887

Committed r115274: <http://trac.webkit.org/changeset/115274>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.