Bug 80350

Created attachment 130236 [details] Patch

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

Created attachment 130250 [details] Patch

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

Created attachment 130478 [details] Patch

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.

Created attachment 132171 [details] Patch

(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.

Thanks for taking a look. Tony, did you want to review this too?

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?

Created attachment 133173 [details] Patch

(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 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)?

(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.

Created attachment 138887 [details] Patch for landing

Comment on attachment 138887 [details] Patch for landing Clearing flags on attachment: 138887 Committed r115274: <http://trac.webkit.org/changeset/115274>

All reviewed patches have been landed. Closing bug.