Bug 80350 - [Web Timing] Add a vendor-prefixed Performance Timeline API
Summary: [Web Timing] Add a vendor-prefixed Performance Timeline API
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:
Blocks: 84881 61152
  Show dependency treegraph
 
Reported: 2012-03-05 16:47 PST by James Simonsen
Modified: 2012-04-25 18:17 PDT (History)
8 users (show)

See Also:


Attachments
Patch (35.58 KB, patch)
2012-03-05 17:00 PST, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (35.69 KB, patch)
2012-03-05 17:45 PST, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (36.22 KB, patch)
2012-03-06 16:59 PST, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (36.21 KB, patch)
2012-03-15 18:27 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (35.19 KB, patch)
2012-03-21 20:00 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch for landing (35.17 KB, patch)
2012-04-25 16:02 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 2012-03-05 16:47:49 PST
[Web Timing] Add a vendor-prefixed Performance Timeline API
Comment 1 James Simonsen 2012-03-05 17:00:06 PST
Created attachment 130236 [details]
Patch
Comment 2 Build Bot 2012-03-05 17:35:23 PST
Comment on attachment 130236 [details]
Patch

Attachment 130236 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11833072
Comment 3 James Simonsen 2012-03-05 17:45:54 PST
Created attachment 130250 [details]
Patch
Comment 4 Early Warning System Bot 2012-03-05 19:54:09 PST
Comment on attachment 130250 [details]
Patch

Attachment 130250 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11833128
Comment 5 James Simonsen 2012-03-06 16:59:14 PST
Created attachment 130478 [details]
Patch
Comment 6 Hajime Morrita 2012-03-15 04:42:43 PDT
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.
Comment 7 James Simonsen 2012-03-15 18:27:09 PDT
Created attachment 132171 [details]
Patch
Comment 8 James Simonsen 2012-03-15 18:28:28 PDT
(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.
Comment 9 James Simonsen 2012-03-15 18:29:28 PDT
Thanks for taking a look.

Tony, did you want to review this too?
Comment 10 Tony Gentilcore 2012-03-21 03:40:29 PDT
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?
Comment 11 James Simonsen 2012-03-21 20:00:05 PDT
Created attachment 133173 [details]
Patch
Comment 12 James Simonsen 2012-03-21 20:52:29 PDT
(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.
Comment 13 Tony Gentilcore 2012-04-25 14:58:28 PDT
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)?
Comment 14 James Simonsen 2012-04-25 16:01:41 PDT
(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.
Comment 15 James Simonsen 2012-04-25 16:02:43 PDT
Created attachment 138887 [details]
Patch for landing
Comment 16 WebKit Review Bot 2012-04-25 18:16:59 PDT
Comment on attachment 138887 [details]
Patch for landing

Clearing flags on attachment: 138887

Committed r115274: <http://trac.webkit.org/changeset/115274>
Comment 17 WebKit Review Bot 2012-04-25 18:17:06 PDT
All reviewed patches have been landed.  Closing bug.