Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2472583003:
Navigation Timing Level 2 (Closed)
DescriptionImplement Navigation Timing Level 2.
Design doc link:
https://docs.google.com/document/d/1DskJn3cKo8RlHxAKJqOSwYlN5mGLOHf3uWo5lwrUWu0/edit#
BUG=504237
Committed: https://crrev.com/fbec30ffe38c608db610239c168df667cab6a454
Cr-Commit-Position: refs/heads/master@{#433765}
Patch Set 1 : First working version #
Total comments: 24
Patch Set 2 : Resolved comments #
Total comments: 13
Patch Set 3 : Resolved comments and also exposed Navigation Timing entry to performance.getEntriesByType and perf… #Patch Set 4 : Added feature flag to getEntriesByName #
Total comments: 24
Patch Set 5 : resolved comments #Patch Set 6 : added two TODOs and attached crbugs #
Total comments: 7
Patch Set 7 : added layout test #
Total comments: 10
Patch Set 8 : resolved comments #Patch Set 9 : added TAO(timing-allow-origin) check and RA(redirect allow) check #
Total comments: 32
Patch Set 10 : addressed comments #
Total comments: 37
Patch Set 11 : addressed more comments #Patch Set 12 : added one TODO #Patch Set 13 : fixed layout tests #Patch Set 14 : addressed layout test failures #
Total comments: 4
Patch Set 15 : addressed comments #Patch Set 16 : don't provide nav timing info when visiting internal pages such as about:blank #Patch Set 17 : revert to previous version #Patch Set 18 : replaced old layout test with http layout test #
Total comments: 2
Patch Set 19 : addressed comments #Patch Set 20 : synced cl #
Total comments: 5
Patch Set 21 : added runtime flag in idl and got rid of webexposed changes #Patch Set 22 : fixed layout tests failure #Dependent Patchsets: Messages
Total messages: 146 (80 generated)
sunjian@chromium.org changed reviewers: + panicker@chromium.org
Description was changed from ========== First working version of NavigationTiming BUG= ========== to ========== Navigation Timing Level 2 BUG= ==========
https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1403: // This is unfortunately that we have to cast in here You shouldn't need a cast here. https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1405: // Also check nullptr Add DCHECK here: Performance* performance = DOMWindowPerformance::performance(..); DCHECK(performance); performance->addNavigationTiming(..); https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:58: probably needs a buffer size? (only have 1 entry) https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:349: void PerformanceBase::addNavigationTiming(LocalFrame* frame) { Add a check for the runtimeflag: if (!RuntimeEnabledFeatures::yourflagname()) return https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:355: if (!documentLoader) replace DCHECK with ASSERT and remove this if https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:362: if (!documentTiming || !documentLoadTiming) (same comment) https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:367: // Right now NavigationType doesn't seem to have a Prerender type yet Prefix with: // TODO(sunjian): Right now .... https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:374: // TODO(sunjian) for now just use encodedDataLength as transferSize, but will for now just set it to 0 https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:397: m_navigationTimingBuffer.append(entry); assert that there's at most 1 entry in buffer? https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/timing/PerformanceBase.h (right): https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceBase.h:125: static const DocumentLoader* documentLoader(LocalFrame*); why are these public? Prefer moving them to anonymous namepace in .cpp https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:40: static double monotonicTimeToDOMHighResTimeStamp(double timeOrigin, Isn't this the same code as ResouceTiming? violates DRY (Don't Repeat Yourself) https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp (right): https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:51: PerformanceResourceTiming::PerformanceResourceTiming( why are 2 ctors necessary?
https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1403: // This is unfortunately that we have to cast in here On 2016/11/02 20:12:37, Shubhie wrote: > You shouldn't need a cast here. Done. https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1405: // Also check nullptr On 2016/11/02 20:12:37, Shubhie wrote: > Add DCHECK here: > Performance* performance = DOMWindowPerformance::performance(..); > DCHECK(performance); > performance->addNavigationTiming(..); Done. https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:58: On 2016/11/02 20:12:37, Shubhie wrote: > probably needs a buffer size? > (only have 1 entry) Since it's only one element, probably don't need a buffer size or a vector. https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:349: void PerformanceBase::addNavigationTiming(LocalFrame* frame) { On 2016/11/02 20:12:37, Shubhie wrote: > Add a check for the runtimeflag: > if (!RuntimeEnabledFeatures::yourflagname()) > return Done. https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:355: if (!documentLoader) On 2016/11/02 20:12:37, Shubhie wrote: > replace DCHECK with ASSERT and remove this if Done. https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:362: if (!documentTiming || !documentLoadTiming) On 2016/11/02 20:12:37, Shubhie wrote: > (same comment) Done. https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:367: // Right now NavigationType doesn't seem to have a Prerender type yet On 2016/11/02 20:12:37, Shubhie wrote: > Prefix with: > // TODO(sunjian): Right now .... Done. https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:374: // TODO(sunjian) for now just use encodedDataLength as transferSize, but will On 2016/11/02 20:12:37, Shubhie wrote: > for now just set it to 0 Done. https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:397: m_navigationTimingBuffer.append(entry); On 2016/11/02 20:12:37, Shubhie wrote: > assert that there's at most 1 entry in buffer? Done. https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/timing/PerformanceBase.h (right): https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceBase.h:125: static const DocumentLoader* documentLoader(LocalFrame*); On 2016/11/02 20:12:37, Shubhie wrote: > why are these public? > Prefer moving them to anonymous namepace in .cpp Done. https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:40: static double monotonicTimeToDOMHighResTimeStamp(double timeOrigin, On 2016/11/02 20:12:37, Shubhie wrote: > Isn't this the same code as ResouceTiming? > violates DRY (Don't Repeat Yourself) Will work on moving this logic into PerformanceBase. Added a TODO and will resolve it in the later updates. https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp (right): https://codereview.chromium.org/2472583003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:51: PerformanceResourceTiming::PerformanceResourceTiming( On 2016/11/02 20:12:37, Shubhie wrote: > why are 2 ctors necessary? Because in NavigationTiming's case, there isn't a ResourceTimingInfo, which is needed in the private constructor. So we need to have another constructor for Navigation.
Looking pretty good. https://codereview.chromium.org/2472583003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2472583003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:107: m_userTiming(nullptr), initialize m_navigationTiming to nullptr https://codereview.chromium.org/2472583003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:369: // TODO(sunjian) for now just set it to 0 "TODO(sunjian): Implement transfer size." https://codereview.chromium.org/2472583003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2472583003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:40: // TODO(sunjian): Move this logic into PerformanceBase later? drop the "later?" https://codereview.chromium.org/2472583003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:181: builder.addNumber("uploadEventStart", unloadEventStart()); s/upload/unload/ https://codereview.chromium.org/2472583003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h (right): https://codereview.chromium.org/2472583003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:43: class DocumentTiming; Could you add a basic layout test? You can follow this template: https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/perf... https://codereview.chromium.org/2472583003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2472583003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:264: PerformanceNavigationTiming status=experimental this should be "test" to start with
panicker@chromium.org changed reviewers: + kinuko@chromium.org, yoav@yoav.ws
Yoav, Kinuko - review requested :)
https://codereview.chromium.org/2472583003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2472583003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:161: // Methods that are overriden from PerformanceResourceTiming Let's not implement methods that need TAO check yet. Add them as follow-up?
https://codereview.chromium.org/2472583003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2472583003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:369: // TODO(sunjian) for now just set it to 0 On 2016/11/04 18:55:32, Shubhie wrote: > "TODO(sunjian): Implement transfer size." Done. https://codereview.chromium.org/2472583003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2472583003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:40: // TODO(sunjian): Move this logic into PerformanceBase later? On 2016/11/04 18:55:32, Shubhie wrote: > drop the "later?" Done. https://codereview.chromium.org/2472583003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:161: // Methods that are overriden from PerformanceResourceTiming On 2016/11/04 19:46:35, Shubhie wrote: > Let's not implement methods that need TAO check yet. Add them as follow-up? Then we might not be able to implement any timing-related methods (which comprise most of the methods). From my understanding, all timing-related metrics should pass the allow-timing check for them to be exposed not just the ones that are overridden in here. How about we leave them as this and come back adding the allow-timing check later for all of them? https://codereview.chromium.org/2472583003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:181: builder.addNumber("uploadEventStart", unloadEventStart()); On 2016/11/04 18:55:32, Shubhie wrote: > s/upload/unload/ Done. https://codereview.chromium.org/2472583003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h (right): https://codereview.chromium.org/2472583003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:43: class DocumentTiming; On 2016/11/04 18:55:32, Shubhie wrote: > Could you add a basic layout test? > You can follow this template: > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/perf... Will add tests in the follow-up cl. Added a TODO https://codereview.chromium.org/2472583003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2472583003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:264: PerformanceNavigationTiming status=experimental On 2016/11/04 18:55:32, Shubhie wrote: > this should be "test" to start with Done.
kinuko@chromium.org changed reviewers: + ksakamoto@chromium.org
+ksakamoto@ who knows the timing code very well Haven't looked into design details yet but made some comments. Could you add a link to the bug and issue description with a link to your impl plan doc? https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1408: DCHECK(performance); nit: this DCHECK is redundant; if it's null we crash at the next line and it'll give us necessary information to debug https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:78: } Not really sure if adding these helper methods are super useful. These all seem to be called only once and we're DCHECK'ing on the results, then why we need these extra null checks? https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:94: return "navigate"; Do these need to use AtomicString? Calling this seems to always construct an atomic string which involves hash lookup. I think we could use a simple enum here and return a String only when PerformanceNavigationTiming.type is accessed. https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:97: } nit: } // namespace https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:163: } nit: no { } for single-line body in blink https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:381: // TODO(sunjian) Implement transfer size. please link to a bug https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h (right): https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:51: static PerformanceNavigationTiming* create( This create method may not be really useful as it just returns (GC'able) rawptr? (Lots of blink code has create method but they were more useful in the era when we used to use blink-specific smart ptrs) https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:107: explicit PerformanceNavigationTiming(double timeOrigin, no need of explicit https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp (right): https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:65: double startTime) Could we use delegated constructors to reduce duplicated code/initialization?
Love to see a layout test :) NT1 tests in third_party/WebKit/LayoutTests/http/tests/w3c/webperf/approved/navigation-timing/ could be good examples. https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/timing/PerformanceEntry.h (right): https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceEntry.h:65: Navigation = 1 << 7, It looks like the bit 1 is unused. Can you insert Navigation after Invalid as 1 << 0? https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h (right): https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:2: * Copyright (C) 2016 Google Inc. All rights reserved. Please use the short license block (here, .cpp and .idl). http://dev.chromium.org/blink/coding-style#TOC-License https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:267: PerformanceNavigationTiming status=test Can we rename it to PerformanceNavigationTiming2, since we already have NavigationTiming1 shipped.
https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1408: DCHECK(performance); On 2016/11/06 15:16:43, kinuko wrote: > nit: this DCHECK is redundant; if it's null we crash at the next line and it'll > give us necessary information to debug I guess it's for documentation? https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:78: } On 2016/11/06 15:16:43, kinuko wrote: > Not really sure if adding these helper methods are super useful. These all seem > to be called only once and we're DCHECK'ing on the results, then why we need > these extra null checks? Done. https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:94: return "navigate"; On 2016/11/06 15:16:43, kinuko wrote: > Do these need to use AtomicString? Calling this seems to always construct an > atomic string which involves hash lookup. I think we could use a simple enum > here and return a String only when PerformanceNavigationTiming.type is accessed. Done. https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:97: } On 2016/11/06 15:16:43, kinuko wrote: > nit: } // namespace No need anymore. https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:163: } On 2016/11/06 15:16:43, kinuko wrote: > nit: no { } for single-line body in blink Done. https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:381: // TODO(sunjian) Implement transfer size. On 2016/11/06 15:16:43, kinuko wrote: > please link to a bug Done. https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/timing/PerformanceEntry.h (right): https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceEntry.h:65: Navigation = 1 << 7, On 2016/11/07 07:53:18, Kunihiko Sakamoto wrote: > It looks like the bit 1 is unused. > Can you insert Navigation after Invalid as 1 << 0? Done. https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h (right): https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:2: * Copyright (C) 2016 Google Inc. All rights reserved. On 2016/11/07 07:53:18, Kunihiko Sakamoto wrote: > Please use the short license block (here, .cpp and .idl). > http://dev.chromium.org/blink/coding-style#TOC-License Done. https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:51: static PerformanceNavigationTiming* create( On 2016/11/06 15:16:43, kinuko wrote: > This create method may not be really useful as it just returns (GC'able) rawptr? > (Lots of blink code has create method but they were more useful in the era when > we used to use blink-specific smart ptrs) Done. https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:107: explicit PerformanceNavigationTiming(double timeOrigin, On 2016/11/06 15:16:43, kinuko wrote: > no need of explicit Done. https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp (right): https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:65: double startTime) On 2016/11/06 15:16:43, kinuko wrote: > Could we use delegated constructors to reduce duplicated code/initialization? Done. https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2472583003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:267: PerformanceNavigationTiming status=test On 2016/11/07 07:53:18, Kunihiko Sakamoto wrote: > Can we rename it to PerformanceNavigationTiming2, since we already have > NavigationTiming1 shipped. Done.
https://codereview.chromium.org/2472583003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2472583003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:322: const DocumentTiming docTiming = frame->document()->timing(); We don't usually abbreviate variable names in blink, prefer doc -> document https://codereview.chromium.org/2472583003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2472583003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:268: PerformanceNavigationTiming2 status=test status=test is not super useful esp. without test code. You probably want status=experimental instead (therefore developers can try out the feature with experimental flag)? Oh, btw-- have you already sent intent to implement email?
On 2016/11/08 13:39:32, kinuko wrote: > https://codereview.chromium.org/2472583003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): > > https://codereview.chromium.org/2472583003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/timing/PerformanceBase.cpp:322: const > DocumentTiming docTiming = frame->document()->timing(); > We don't usually abbreviate variable names in blink, prefer doc -> document > > https://codereview.chromium.org/2472583003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): > > https://codereview.chromium.org/2472583003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:268: > PerformanceNavigationTiming2 status=test > status=test is not super useful esp. without test code. You probably want > status=experimental instead (therefore developers can try out the feature with > experimental flag)? > > Oh, btw-- have you already sent intent to implement email? Also: could you please add a link to the bug (BUG= line) and also a link to your impl plan doc in the issue description?
Kinuko - thanks again for reviewing this CL (very appreciated!) https://codereview.chromium.org/2472583003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2472583003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:160: if (entryType.isNull() || type == PerformanceEntry::Navigation) { these should be "else if" to avoid needless execution ? https://codereview.chromium.org/2472583003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2472583003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:268: PerformanceNavigationTiming2 status=test On 2016/11/08 13:39:32, kinuko wrote: > status=test is not super useful esp. without test code. You probably want > status=experimental instead (therefore developers can try out the feature with > experimental flag)? > > Oh, btw-- have you already sent intent to implement email? Jian is adding Layout tests. We want to flip to "experimental" when the implementation is complete (i.e. major TODOs are resolved) Also drafted and just sending Intent: https://docs.google.com/document/d/1z0gzmvJKtoA3pjXz9UXy2KvtnNire0eV9mzb5usEg...
https://codereview.chromium.org/2472583003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2472583003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:160: if (entryType.isNull() || type == PerformanceEntry::Navigation) { On 2016/11/08 18:27:06, Shubhie wrote: > these should be "else if" to avoid needless execution ? I think they intended to collect all entries with the given name if type is not provided. https://codereview.chromium.org/2472583003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:322: const DocumentTiming docTiming = frame->document()->timing(); On 2016/11/08 13:39:32, kinuko wrote: > We don't usually abbreviate variable names in blink, prefer doc -> document Done. https://codereview.chromium.org/2472583003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2472583003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:268: PerformanceNavigationTiming2 status=test On 2016/11/08 13:39:32, kinuko wrote: > status=test is not super useful esp. without test code. You probably want > status=experimental instead (therefore developers can try out the feature with > experimental flag)? > > Oh, btw-- have you already sent intent to implement email? Acknowledged.
The CQ bit was checked by sunjian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Thanks for adding a layout test. Could you add another test case that include redirects? https://codereview.chromium.org/2472583003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2472583003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:87: return monotonicTimeToDOMHighResTimeStamp(m_timeOrigin, m_unloadEventStart); Don't we need origin flags check here? https://codereview.chromium.org/2472583003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:148: return monotonicTimeToDOMHighResTimeStamp(m_timeOrigin, m_redirectStart); This and redirectEnd() should return zero if any of redirects are cross-origin. https://codereview.chromium.org/2472583003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h (right): https://codereview.chromium.org/2472583003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:20: // TODO(sunjian): Add layout test Now we have a layout test. Remove this? https://codereview.chromium.org/2472583003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.idl (right): https://codereview.chromium.org/2472583003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.idl:5: // https://www.w3.org/TR/navigation-timing-2/#dfn-current-document Let's link to the IDL definition. #dfn-current-document -> #sec-PerformanceNavigationTiming
On 2016/11/09 10:03:08, Kunihiko Sakamoto wrote: > Thanks for adding a layout test. > Could you add another test case that include redirects? > Hey Kunihiko, You mean actual redirect tests (not just a test case for presence of field) - right? the detailed tests (including redirect) should be web-platform-tests (not layout tests at this point) And cover the redirect cases here: https://github.com/w3c/web-platform-tests/tree/master/navigation-timing Do you agree? We'll address this in a followup CL. I filed: https://bugs.chromium.org/p/chromium/issues/detail?id=663894 If you were suggesting a minor test case coverage please clarify, thanks!
https://codereview.chromium.org/2472583003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2472583003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:87: return monotonicTimeToDOMHighResTimeStamp(m_timeOrigin, m_unloadEventStart); On 2016/11/09 10:03:08, Kunihiko Sakamoto wrote: > Don't we need origin flags check here? Do you mean timing-allow-origin check? Already created a bug for that and will address that in the follow-up cl. https://codereview.chromium.org/2472583003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:148: return monotonicTimeToDOMHighResTimeStamp(m_timeOrigin, m_redirectStart); On 2016/11/09 10:03:08, Kunihiko Sakamoto wrote: > This and redirectEnd() should return zero if any of redirects are cross-origin. Same as above. The bug is crbug/663219 https://codereview.chromium.org/2472583003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h (right): https://codereview.chromium.org/2472583003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:20: // TODO(sunjian): Add layout test On 2016/11/09 10:03:08, Kunihiko Sakamoto wrote: > Now we have a layout test. Remove this? Done. https://codereview.chromium.org/2472583003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.idl (right): https://codereview.chromium.org/2472583003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.idl:5: // https://www.w3.org/TR/navigation-timing-2/#dfn-current-document On 2016/11/09 10:03:08, Kunihiko Sakamoto wrote: > Let's link to the IDL definition. > #dfn-current-document -> #sec-PerformanceNavigationTiming Done.
LGTM https://codereview.chromium.org/2472583003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2472583003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:87: return monotonicTimeToDOMHighResTimeStamp(m_timeOrigin, m_unloadEventStart); On 2016/11/09 22:45:04, sunjian wrote: > On 2016/11/09 10:03:08, Kunihiko Sakamoto wrote: > > Don't we need origin flags check here? > > Do you mean timing-allow-origin check? Already created a bug for that and will > address that in the follow-up cl. He means the redirect-check, which you've add now (great!) https://codereview.chromium.org/2472583003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:148: return monotonicTimeToDOMHighResTimeStamp(m_timeOrigin, m_redirectStart); On 2016/11/09 22:45:04, sunjian wrote: > On 2016/11/09 10:03:08, Kunihiko Sakamoto wrote: > > This and redirectEnd() should return zero if any of redirects are > cross-origin. > > Same as above. The bug is crbug/663219 Same comment. (TAO is a separate check, that is not needed for this CL)
Covering redirect cases in web-platform-tests SGTM. Please update the CL description as kinuko suggested. https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.h (right): https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.h:52: class DocumentTiming; Are these forward declarations necessary? https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:59: hasCrossOriginRedirect, I think this should be !hasCrossOriginRedirect. https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h (right): https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:87: bool m_hasCrossOriginRedirect; Can we omit this field by using PerformanceResourceTiming::m_allowTimingDetails?
Apologies for the delay in review (travel...) I haven't yet finished fully reviewing this entire CL, but initial comments below https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1408: DCHECK(performance); FWIW, I like the DCHECK here as it helps: a) make the assumption that performance is not null here explicit b) debugging such cases using release builds with dcheck on, in case the assumption somehow breaks (e.g. in new and incomplete unit tests) https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:36: #include "core/dom/Element.h" I believe DocumentTiming.h and Element.h are not needed here https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:63: m_navigationTiming(nullptr), Is the nullptr initialization needed? (both here and in line below) I think Member automatically initializes to nullptr when no other pointer is given https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:85: if (RuntimeEnabledFeatures::performanceNavigationTiming2Enabled()) We should append m_navigationTiming only when it's not nullptr, as scripts can call getEntries before the load event was dispatched and m_navigationTiming created. This check here would also make it unnecessary to check the runtime flag, as when it's false, m_navigationTiming is guaranteed to be null. https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:117: if (RuntimeEnabledFeatures::performanceNavigationTiming2Enabled()) same as above. Only add when non-null and no need to check flag. https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:162: m_navigationTiming->name() == name) A check for nav timing not being null would prevent a crash here.
Some more comments... thanks for adding tests! https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1408: DCHECK(performance); On 2016/11/10 14:58:58, Yoav Weiss wrote: > FWIW, I like the DCHECK here as it helps: > a) make the assumption that performance is not null here explicit > b) debugging such cases using release builds with dcheck on, in case the > assumption somehow breaks (e.g. in new and incomplete unit tests) Well, I've also heard people say multiple reasons not to have redundant DCHECKs, but this is not on our style guide and apparently different people have different opinions-- and I'm fine with these DCHECKs too :) https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:64: m_userTiming(nullptr), it's not related to this change but ditto for this (no need to initialize it with nullptr for Member) https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:318: DCHECK(frame); We have null-checks for frame() at the callsite, are you sure this DCHECK always holds? https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:324: ResourceResponse finalResponse = documentLoader->response(); This creates a copy, looks like just using a const ref should be fine? https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:141: // Methods that are overriden from PerformanceResourceTiming This comment should be rather in the header, no need to be in .cpp https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h (right): https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:60: nit: have a comment here for overriding methods // PerformanceResourceTiming overrides:
https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:318: DCHECK(frame); On 2016/11/10 17:55:33, kinuko wrote: > We have null-checks for frame() at the callsite, are you sure this DCHECK always > holds? Yep, if frame is NULL we should not call addNavigationTiming, so add "if (frame)" at the call-site. https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h (right): https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:87: bool m_hasCrossOriginRedirect; On 2016/11/10 03:31:49, Kunihiko Sakamoto wrote: > Can we omit this field by using PerformanceResourceTiming::m_allowTimingDetails? This is a bit different from allowTimingDetails which comes from the TAO opt-in for given resource, we don't need TAO opt-in on the document resource. "m_allowRedirectDetails" is also not the right counterpart, here we want an indicator for "cross-origin redirects were involved when navigating to the document" I think.
https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1408: DCHECK(performance); On 2016/11/10 14:58:58, Yoav Weiss wrote: > FWIW, I like the DCHECK here as it helps: > a) make the assumption that performance is not null here explicit > b) debugging such cases using release builds with dcheck on, in case the > assumption somehow breaks (e.g. in new and incomplete unit tests) Acknowledged. https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1408: DCHECK(performance); On 2016/11/10 17:55:33, kinuko wrote: > On 2016/11/10 14:58:58, Yoav Weiss wrote: > > FWIW, I like the DCHECK here as it helps: > > a) make the assumption that performance is not null here explicit > > b) debugging such cases using release builds with dcheck on, in case the > > assumption somehow breaks (e.g. in new and incomplete unit tests) > > Well, I've also heard people say multiple reasons not to have redundant DCHECKs, > but this is not on our style guide and apparently different people have > different opinions-- and I'm fine with these DCHECKs too :) Acknowledged. https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:36: #include "core/dom/Element.h" On 2016/11/10 14:58:58, Yoav Weiss wrote: > I believe DocumentTiming.h and Element.h are not needed here I think i can be onboard with getting rid of "core/dom/Element.h" cause it doesn't seem to be used at all. But "core/dom/DocumentTiming.h" is actually being used. I tried getting rid of "core/dom/DocumentTiming.h" and it does compile successfully cause "core/dom/Document.h" included "core/dom/DocumentTiming.h" already. But that is implicit right? https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:63: m_navigationTiming(nullptr), On 2016/11/10 14:58:58, Yoav Weiss wrote: > Is the nullptr initialization needed? (both here and in line below) > I think Member automatically initializes to nullptr when no other pointer is > given I will get rid of m_navigationTiming's redundant initialization only this since the line below it is not related to this cl. https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:64: m_userTiming(nullptr), On 2016/11/10 17:55:33, kinuko wrote: > it's not related to this change but ditto for this (no need to initialize it > with nullptr for Member) Acknowledged. https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:85: if (RuntimeEnabledFeatures::performanceNavigationTiming2Enabled()) On 2016/11/10 14:58:58, Yoav Weiss wrote: > We should append m_navigationTiming only when it's not nullptr, as scripts can > call getEntries before the load event was dispatched and m_navigationTiming > created. > This check here would also make it unnecessary to check the runtime flag, as > when it's false, m_navigationTiming is guaranteed to be null. Done. https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:117: if (RuntimeEnabledFeatures::performanceNavigationTiming2Enabled()) On 2016/11/10 14:58:58, Yoav Weiss wrote: > same as above. Only add when non-null and no need to check flag. Done. https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:162: m_navigationTiming->name() == name) On 2016/11/10 14:58:58, Yoav Weiss wrote: > A check for nav timing not being null would prevent a crash here. Done. https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:318: DCHECK(frame); On 2016/11/11 17:14:02, Shubhie wrote: > On 2016/11/10 17:55:33, kinuko wrote: > > We have null-checks for frame() at the callsite, are you sure this DCHECK > always > > holds? > > Yep, if frame is NULL we should not call addNavigationTiming, so add "if > (frame)" at the call-site. > Done. https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:324: ResourceResponse finalResponse = documentLoader->response(); On 2016/11/10 17:55:33, kinuko wrote: > This creates a copy, looks like just using a const ref should be fine? Done. https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.h (right): https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.h:52: class DocumentTiming; On 2016/11/10 03:31:49, Kunihiko Sakamoto wrote: > Are these forward declarations necessary? Done. https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:59: hasCrossOriginRedirect, On 2016/11/10 03:31:49, Kunihiko Sakamoto wrote: > I think this should be !hasCrossOriginRedirect. Done. https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:141: // Methods that are overriden from PerformanceResourceTiming On 2016/11/10 17:55:33, kinuko wrote: > This comment should be rather in the header, no need to be in .cpp Done. https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h (right): https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:60: On 2016/11/10 17:55:33, kinuko wrote: > nit: have a comment here for overriding methods > > // PerformanceResourceTiming overrides: Done. https://codereview.chromium.org/2472583003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:87: bool m_hasCrossOriginRedirect; On 2016/11/11 17:14:02, Shubhie wrote: > On 2016/11/10 03:31:49, Kunihiko Sakamoto wrote: > > Can we omit this field by using > PerformanceResourceTiming::m_allowTimingDetails? > > This is a bit different from allowTimingDetails which comes from the TAO opt-in > for given resource, we don't need TAO opt-in on the document resource. > "m_allowRedirectDetails" is also not the right counterpart, here we want an > indicator for "cross-origin redirects were involved when navigating to the > document" I think. > > Acknowledged.
The CQ bit was checked by sunjian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Navigation Timing Level 2 BUG= ========== to ========== Implement Navigation Timing Level 2. Design doc link: https://docs.google.com/document/d/1DskJn3cKo8RlHxAKJqOSwYlN5mGLOHf3uWo5lwrUW... issue link: https://bugs.chromium.org/p/chromium/issues/detail?id=504237&q=owner%3Ame&col... BUG=504237 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Implement Navigation Timing Level 2. Design doc link: https://docs.google.com/document/d/1DskJn3cKo8RlHxAKJqOSwYlN5mGLOHf3uWo5lwrUW... issue link: https://bugs.chromium.org/p/chromium/issues/detail?id=504237&q=owner%3Ame&col... BUG=504237 ========== to ========== Implement Navigation Timing Level 2. Design doc link: https://docs.google.com/document/d/1DskJn3cKo8RlHxAKJqOSwYlN5mGLOHf3uWo5lwrUW... BUG=504237 ==========
A few more comments. Also - test failures seem relevant (at least the webexposed/global-interface-listing.html ones) https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:35: #include "core/dom/DocumentTiming.h" Is there anything in the style guide on such cases, and if we need an explicit h file declaration in the cpp? https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:317: const DocumentLoadTiming documentLoadTiming = documentLoader->timing(); const DocumentLoadTiming& https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:318: const DocumentTiming documentTiming = frame->document()->timing(); const DocumentTiming& https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:10: #include "core/loader/DocumentLoader.h" Are these h file declarations needed here? https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:51: timing, We can DCHECK that timing is not null here, to make that fact explicit. Looking at PerformanceResourceTiming and https://bugs.chromium.org/p/chromium/issues/detail?id=251100 looks like m_timing can be null inside PerformanceResourceTiming when AppCache is involved, but we should make it explicit that we're not adding a new case here. https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:58: true /*allowTimingDetails*/, This should be an enum https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h (right): https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:9: #include "core/frame/DOMWindowProperty.h" Is this h file needed here? https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:10: #include "core/loader/FrameLoaderTypes.h" Is this h file needed here? https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:18: class DocumentTiming; Are there forward declarations needed? https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.h (right): https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceResourceTiming.h:99: bool allowRedirectDetails, Can these booleans be enums to avoid the "true /* explanation */" pattern?
Also, can you include a link to the I2S thread at this issue's description?
Overall the CL looks good. Thanks for you continuous work on this :) A few more comments... https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:39: #include "core/loader/DocumentLoadTiming.h" Same for LocalFrame.h and DocumentLoadTiming.h. They're not really needed here. Do we need to make their inclusion explicit? (personally I think we shouldn't, and AFAIR including unnecessary h files can slow down our build) https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:314: return; Can we add a `DCHECK(frame)` here? https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:318: const DocumentTiming documentTiming = frame->document()->timing(); Are you sure frame->document() is not null? If so, we should DCHECK it here. Otherwise, we should guard against it. https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:21: return PerformanceBase::clampTimeResolution(seconds - timeOrigin) * 1000.0; Should we make sure here that the result is never negative?
https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1407: Performance* performance = DOMWindowPerformance::performance(*this); these 2 lines can go inside the if () below.. https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:35: #include "core/dom/DocumentTiming.h" On 2016/11/12 17:13:47, Yoav Weiss wrote: > Is there anything in the style guide on such cases, and if we need an explicit h > file declaration in the cpp? I think it's entirely reasonable to include headers that are in use in a file, instead of depending on transitive includes: it serves as documentation in code search and prevents surprises when refactoring. Not seeing anything in chromium style, but google3 had made a big push on "include-what-you-use" http://include-what-you-use.org/ https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:58: true /*allowTimingDetails*/, On 2016/11/12 17:13:47, Yoav Weiss wrote: > This should be an enum What are the values of the enum? Let's move improvements to the existing code to separate CLs.
https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:146: if (m_hasCrossOriginRedirect) Add a TODO & bug to follow up on whether this should be non-zero with opt-in.
https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:58: true /*allowTimingDetails*/, On 2016/11/14 17:38:52, Shubhie wrote: > On 2016/11/12 17:13:47, Yoav Weiss wrote: > > This should be an enum > > What are the values of the enum? Let's move improvements to the existing code to > separate CLs. I think this enum should be defined as part of the new constructor we defined in PerformanceResourceTiming. With that said, I'm fine with putting this off to a followup CL.
https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1407: Performance* performance = DOMWindowPerformance::performance(*this); On 2016/11/14 17:38:52, Shubhie wrote: > these 2 lines can go inside the if () below.. Done. https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:35: #include "core/dom/DocumentTiming.h" On 2016/11/14 17:38:52, Shubhie wrote: > On 2016/11/12 17:13:47, Yoav Weiss wrote: > > Is there anything in the style guide on such cases, and if we need an explicit > h > > file declaration in the cpp? > > I think it's entirely reasonable to include headers that are in use in a file, > instead of depending on transitive includes: it serves as documentation in code > search and prevents surprises when refactoring. > Not seeing anything in chromium style, but google3 had made a big push on > "include-what-you-use" > http://include-what-you-use.org/ Acknowledged. https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:39: #include "core/loader/DocumentLoadTiming.h" On 2016/11/13 13:48:02, Yoav Weiss wrote: > Same for LocalFrame.h and DocumentLoadTiming.h. > They're not really needed here. Do we need to make their inclusion explicit? > (personally I think we shouldn't, and AFAIR including unnecessary h files can > slow down our build) I actually agree more with Shubhie on this. https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:314: return; On 2016/11/13 13:48:02, Yoav Weiss wrote: > Can we add a `DCHECK(frame)` here? Done. https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:317: const DocumentLoadTiming documentLoadTiming = documentLoader->timing(); On 2016/11/12 17:13:47, Yoav Weiss wrote: > const DocumentLoadTiming& Done. https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:318: const DocumentTiming documentTiming = frame->document()->timing(); On 2016/11/12 17:13:47, Yoav Weiss wrote: > const DocumentTiming& Done. https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:318: const DocumentTiming documentTiming = frame->document()->timing(); On 2016/11/12 17:13:47, Yoav Weiss wrote: > const DocumentTiming& Done. https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:318: const DocumentTiming documentTiming = frame->document()->timing(); On 2016/11/13 13:48:02, Yoav Weiss wrote: > Are you sure frame->document() is not null? > > If so, we should DCHECK it here. Otherwise, we should guard against it. Done. https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:10: #include "core/loader/DocumentLoader.h" On 2016/11/12 17:13:47, Yoav Weiss wrote: > Are these h file declarations needed here? Done. https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:21: return PerformanceBase::clampTimeResolution(seconds - timeOrigin) * 1000.0; On 2016/11/13 13:48:02, Yoav Weiss wrote: > Should we make sure here that the result is never negative? Done. https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:51: timing, On 2016/11/12 17:13:47, Yoav Weiss wrote: > We can DCHECK that timing is not null here, to make that fact explicit. Looking > at PerformanceResourceTiming and > https://bugs.chromium.org/p/chromium/issues/detail?id=251100 looks like m_timing > can be null inside PerformanceResourceTiming when AppCache is involved, but we > should make it explicit that we're not adding a new case here. But timing could possibly be nullptr, right? And PerformanceResourceTiming doesn't take timing as nullptr. https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:58: true /*allowTimingDetails*/, On 2016/11/14 20:59:50, Yoav Weiss wrote: > On 2016/11/14 17:38:52, Shubhie wrote: > > On 2016/11/12 17:13:47, Yoav Weiss wrote: > > > This should be an enum > > > > What are the values of the enum? Let's move improvements to the existing code > to > > separate CLs. > > I think this enum should be defined as part of the new constructor we defined in > PerformanceResourceTiming. > With that said, I'm fine with putting this off to a followup CL. Added a TODO and will come back to make the improvements later. https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:146: if (m_hasCrossOriginRedirect) On 2016/11/14 19:31:47, Shubhie wrote: > Add a TODO & bug to follow up on whether this should be non-zero with opt-in. Done. https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h (right): https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:9: #include "core/frame/DOMWindowProperty.h" On 2016/11/12 17:13:48, Yoav Weiss wrote: > Is this h file needed here? Done. https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:10: #include "core/loader/FrameLoaderTypes.h" On 2016/11/12 17:13:48, Yoav Weiss wrote: > Is this h file needed here? This one is needed to provide NavigationType used in this .h file. https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:18: class DocumentTiming; On 2016/11/12 17:13:47, Yoav Weiss wrote: > Are there forward declarations needed? Done. https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.h (right): https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceResourceTiming.h:99: bool allowRedirectDetails, On 2016/11/12 17:13:48, Yoav Weiss wrote: > Can these booleans be enums to avoid the "true /* explanation */" pattern? Added a TODO in the previous related comment.
The CQ bit was checked by sunjian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:51: timing, On 2016/11/14 22:12:28, sunjian wrote: > On 2016/11/12 17:13:47, Yoav Weiss wrote: > > We can DCHECK that timing is not null here, to make that fact explicit. > Looking > > at PerformanceResourceTiming and > > https://bugs.chromium.org/p/chromium/issues/detail?id=251100 looks like > m_timing > > can be null inside PerformanceResourceTiming when AppCache is involved, but we > > should make it explicit that we're not adding a new case here. > > But timing could possibly be nullptr, right? And PerformanceResourceTiming > doesn't take timing as nullptr. Is there a scenario where the document's ResourceResponse has a nullptr m_resourceLoadTiming upon the document's onload? (when this is called) I don't think that's the case, but if it is, we should somehow address it, as this will expose that state to the Web.
On 2016/11/14 23:02:52, Yoav Weiss wrote: > https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp > (right): > > https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:51: > timing, > On 2016/11/14 22:12:28, sunjian wrote: > > On 2016/11/12 17:13:47, Yoav Weiss wrote: > > > We can DCHECK that timing is not null here, to make that fact explicit. > > Looking > > > at PerformanceResourceTiming and > > > https://bugs.chromium.org/p/chromium/issues/detail?id=251100 looks like > > m_timing > > > can be null inside PerformanceResourceTiming when AppCache is involved, but > we > > > should make it explicit that we're not adding a new case here. > > > > But timing could possibly be nullptr, right? And PerformanceResourceTiming > > doesn't take timing as nullptr. > > Is there a scenario where the document's ResourceResponse has a nullptr > m_resourceLoadTiming upon the document's onload? (when this is called) > I don't think that's the case, but if it is, we should somehow address it, as > this will expose that state to the Web. Personally, i think the reason they did a checking of nullptr for resourceLoadTiming in PerformanceResourceTiming is because it might be nullptr. If resourceLoadTiming being nullptr is not a legal state upon the document's onload, then the checking should happen at the lower level (such as ResourceResponse::resourceLoadTiming()) instead of getting reflected in nav timing or resource timing, right? That way we only need one checking and also makes upper level classes more generic? Totally open to discussions.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2016/11/15 00:01:14, sunjian wrote: > On 2016/11/14 23:02:52, Yoav Weiss wrote: > > > https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp > > (right): > > > > > https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:51: > > timing, > > On 2016/11/14 22:12:28, sunjian wrote: > > > On 2016/11/12 17:13:47, Yoav Weiss wrote: > > > > We can DCHECK that timing is not null here, to make that fact explicit. > > > Looking > > > > at PerformanceResourceTiming and > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=251100 looks like > > > m_timing > > > > can be null inside PerformanceResourceTiming when AppCache is involved, > but > > we > > > > should make it explicit that we're not adding a new case here. > > > > > > But timing could possibly be nullptr, right? And PerformanceResourceTiming > > > doesn't take timing as nullptr. > > > > Is there a scenario where the document's ResourceResponse has a nullptr > > m_resourceLoadTiming upon the document's onload? (when this is called) > > I don't think that's the case, but if it is, we should somehow address it, as > > this will expose that state to the Web. > > Personally, i think the reason they did a checking of nullptr for > resourceLoadTiming in PerformanceResourceTiming > is because it might be nullptr. If resourceLoadTiming being nullptr is not a > legal state upon the document's onload, > then the checking should happen at the lower level (such as > ResourceResponse::resourceLoadTiming()) instead of getting reflected in nav > timing or resource timing, right? > That way we only need one checking and also makes upper level classes more > generic? Totally open to discussions. I guess i misunderstood the intent for the DCHECK. Will add a TODO and come back to do more investigation.
On 2016/11/15 00:31:30, sunjian wrote: > On 2016/11/15 00:01:14, sunjian wrote: > > On 2016/11/14 23:02:52, Yoav Weiss wrote: > > > > > > https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp > > > (right): > > > > > > > > > https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:51: > > > timing, > > > On 2016/11/14 22:12:28, sunjian wrote: > > > > On 2016/11/12 17:13:47, Yoav Weiss wrote: > > > > > We can DCHECK that timing is not null here, to make that fact explicit. > > > > Looking > > > > > at PerformanceResourceTiming and > > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=251100 looks like > > > > m_timing > > > > > can be null inside PerformanceResourceTiming when AppCache is involved, > > but > > > we > > > > > should make it explicit that we're not adding a new case here. > > > > > > > > But timing could possibly be nullptr, right? And PerformanceResourceTiming > > > > doesn't take timing as nullptr. > > > > > > Is there a scenario where the document's ResourceResponse has a nullptr > > > m_resourceLoadTiming upon the document's onload? (when this is called) > > > I don't think that's the case, but if it is, we should somehow address it, > as > > > this will expose that state to the Web. > > > > Personally, i think the reason they did a checking of nullptr for > > resourceLoadTiming in PerformanceResourceTiming > > is because it might be nullptr. If resourceLoadTiming being nullptr is not a > > legal state upon the document's onload, > > then the checking should happen at the lower level (such as > > ResourceResponse::resourceLoadTiming()) instead of getting reflected in nav > > timing or resource timing, right? > > That way we only need one checking and also makes upper level classes more > > generic? Totally open to discussions. > > I guess i misunderstood the intent for the DCHECK. Will add a TODO and come back > to > do more investigation. Tl;dr is that we aren't sure if there could be some (caching?) scenario at document onload where timing is null. However if Yoav is sure of it then we should just add the DCHECK here as suggested. Otherwise we should investigate and consider adding it as a follow-up.
The CQ bit was checked by sunjian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/11/15 00:42:02, Shubhie wrote: > On 2016/11/15 00:31:30, sunjian wrote: > > On 2016/11/15 00:01:14, sunjian wrote: > > > On 2016/11/14 23:02:52, Yoav Weiss wrote: > > > > > > > > > > https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... > > > > File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2472583003/diff/180001/third_party/WebKit/Sou... > > > > third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:51: > > > > timing, > > > > On 2016/11/14 22:12:28, sunjian wrote: > > > > > On 2016/11/12 17:13:47, Yoav Weiss wrote: > > > > > > We can DCHECK that timing is not null here, to make that fact > explicit. > > > > > Looking > > > > > > at PerformanceResourceTiming and > > > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=251100 looks > like > > > > > m_timing > > > > > > can be null inside PerformanceResourceTiming when AppCache is > involved, > > > but > > > > we > > > > > > should make it explicit that we're not adding a new case here. > > > > > > > > > > But timing could possibly be nullptr, right? And > PerformanceResourceTiming > > > > > doesn't take timing as nullptr. > > > > > > > > Is there a scenario where the document's ResourceResponse has a nullptr > > > > m_resourceLoadTiming upon the document's onload? (when this is called) > > > > I don't think that's the case, but if it is, we should somehow address it, > > as > > > > this will expose that state to the Web. > > > > > > Personally, i think the reason they did a checking of nullptr for > > > resourceLoadTiming in PerformanceResourceTiming > > > is because it might be nullptr. If resourceLoadTiming being nullptr is not a > > > legal state upon the document's onload, > > > then the checking should happen at the lower level (such as > > > ResourceResponse::resourceLoadTiming()) instead of getting reflected in nav > > > timing or resource timing, right? > > > That way we only need one checking and also makes upper level classes more > > > generic? Totally open to discussions. > > > > I guess i misunderstood the intent for the DCHECK. Will add a TODO and come > back > > to > > do more investigation. > > Tl;dr is that we aren't sure if there could be some (caching?) scenario at > document onload where timing is null. However if Yoav is sure of it then we > should just add the DCHECK here as suggested. > Otherwise we should investigate and consider adding it as a follow-up. I'm not 100% sure that timing is never null here (just not aware of any scenario where it is). TODO and further investigation sounds good.
LGTM % webexposed/global-interface-listing.html failure, which seems relevant
The CQ bit was checked by sunjian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by sunjian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
https://codereview.chromium.org/2472583003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2472583003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:52: // null, if it can't, add DCHECK I think this can be nullptr at least for non-HTTP site, e.g. for about:blank. nit: can bu -> can be https://codereview.chromium.org/2472583003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:61: // for this. nit/optional: is this formatting done by clang formatter? For the comment that could span multiple lines I see following style more often and I slightly prefer that decodedBodyLength, didReuseConnection, // TODO(sunjian): ..... true /* allowTimingDetails */, ....
The CQ bit was checked by sunjian@chromium.org to run a CQ dry run
https://codereview.chromium.org/2472583003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2472583003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:52: // null, if it can't, add DCHECK On 2016/11/16 01:05:13, kinuko wrote: > I think this can be nullptr at least for non-HTTP site, e.g. for about:blank. > > nit: can bu -> can be > Done. https://codereview.chromium.org/2472583003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:61: // for this. On 2016/11/16 01:05:13, kinuko wrote: > nit/optional: is this formatting done by clang formatter? For the comment that > could span multiple lines I see following style more often and I slightly prefer > that > > decodedBodyLength, > didReuseConnection, > // TODO(sunjian): ..... > true /* allowTimingDetails */, > .... I used git cl format to indent the lines.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/16 02:22:37, sunjian wrote: > https://codereview.chromium.org/2472583003/diff/260001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp > (right): > > https://codereview.chromium.org/2472583003/diff/260001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:52: // > null, if it can't, add DCHECK > On 2016/11/16 01:05:13, kinuko wrote: > > I think this can be nullptr at least for non-HTTP site, e.g. for about:blank. So what do we want to do this with these nullptr cases?
On 2016/11/16 03:03:44, kinuko wrote: > On 2016/11/16 02:22:37, sunjian wrote: > > > https://codereview.chromium.org/2472583003/diff/260001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp > > (right): > > > > > https://codereview.chromium.org/2472583003/diff/260001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:52: // > > null, if it can't, add DCHECK > > On 2016/11/16 01:05:13, kinuko wrote: > > > I think this can be nullptr at least for non-HTTP site, e.g. for > about:blank. > > So what do we want to do this with these nullptr cases? (Btw I actually confirmed that it can be null, so this TODO feels a bit irrelevant now)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/11/16 03:04:53, kinuko wrote: > On 2016/11/16 03:03:44, kinuko wrote: > > On 2016/11/16 02:22:37, sunjian wrote: > > > > > > https://codereview.chromium.org/2472583003/diff/260001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp > > > (right): > > > > > > > > > https://codereview.chromium.org/2472583003/diff/260001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:52: // > > > null, if it can't, add DCHECK > > > On 2016/11/16 01:05:13, kinuko wrote: > > > > I think this can be nullptr at least for non-HTTP site, e.g. for > > about:blank. > > > > So what do we want to do this with these nullptr cases? > > (Btw I actually confirmed that it can be null, so this TODO feels a bit > irrelevant now) Thanks for confirming that it can be null. I think we should not report nav timing at all for such cases, rather than report a partial one. As long as this is limited to internal pages ("about:" "chrome:", etc), I don't think it's a big deal
The CQ bit was checked by sunjian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by sunjian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/11/16 07:51:33, Yoav Weiss wrote: > On 2016/11/16 03:04:53, kinuko wrote: > > On 2016/11/16 03:03:44, kinuko wrote: > > > On 2016/11/16 02:22:37, sunjian wrote: > > > > > > > > > > https://codereview.chromium.org/2472583003/diff/260001/third_party/WebKit/Sou... > > > > File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2472583003/diff/260001/third_party/WebKit/Sou... > > > > third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:52: > // > > > > null, if it can't, add DCHECK > > > > On 2016/11/16 01:05:13, kinuko wrote: > > > > > I think this can be nullptr at least for non-HTTP site, e.g. for > > > about:blank. > > > > > > So what do we want to do this with these nullptr cases? > > > > (Btw I actually confirmed that it can be null, so this TODO feels a bit > > irrelevant now) > > Thanks for confirming that it can be null. I think we should not report nav > timing at all for such cases, rather than report a partial one. > As long as this is limited to internal pages ("about:" "chrome:", etc), I don't > think it's a big deal So it broke my layout tests cause it's expected that there must be one instance of nav timing. I think i can add an additional passing case in the layout test for when there isn't any nav timing instance, but then it brings extra complexity for identifying the internal navigation cases (such as "about:" and "chrome"). I am not even sure this is implementable in the current layout test. I can do more investigation into that if we are certain that not providing nav timing instance when visiting internal pages is the right thing to do here.
The CQ bit was checked by sunjian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/17 01:06:29, sunjian wrote: > On 2016/11/16 07:51:33, Yoav Weiss wrote: > > On 2016/11/16 03:04:53, kinuko wrote: > > > On 2016/11/16 03:03:44, kinuko wrote: > > > > On 2016/11/16 02:22:37, sunjian wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2472583003/diff/260001/third_party/WebKit/Sou... > > > > > File > third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2472583003/diff/260001/third_party/WebKit/Sou... > > > > > > third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:52: > > // > > > > > null, if it can't, add DCHECK > > > > > On 2016/11/16 01:05:13, kinuko wrote: > > > > > > I think this can be nullptr at least for non-HTTP site, e.g. for > > > > about:blank. > > > > > > > > So what do we want to do this with these nullptr cases? > > > > > > (Btw I actually confirmed that it can be null, so this TODO feels a bit > > > irrelevant now) > > > > Thanks for confirming that it can be null. I think we should not report nav > > timing at all for such cases, rather than report a partial one. > > As long as this is limited to internal pages ("about:" "chrome:", etc), I > don't > > think it's a big deal > > So it broke my layout tests cause it's expected that there must be one instance > of nav timing. > I think i can add an additional passing case in the layout test for when there > isn't any nav timing instance, > but then it brings extra complexity for identifying the internal navigation > cases (such as "about:" and "chrome"). > I am not even sure this is implementable in the current layout test. I can do > more investigation into that if we are > certain that not providing nav timing instance when visiting internal pages is > the right thing to do here. We can make it an layout/http test.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_compile_dbg on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_cronet on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) cast_shell_android on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by sunjian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL. Did what Yoav suggested not exposing nav timing instance when ResourceLoadTiming doesn't exist. Moved layout test into layout/http.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Thanks! lgtm for the part I care about (probably Yoav wants to take another look) https://codereview.chromium.org/2472583003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2472583003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:327: // such as about:blank Could we rephrase this comment to make it clearer why we check resourceLoadTiming here, like: "Don't create a navigation timing instance when resourceLoadTiming is null, which could happen when visiting non-http sites such as about:blank or in some error cases." (Also: please end the comment with period)
On 2016/11/18 06:53:38, kinuko wrote: > Thanks! lgtm for the part I care about (probably Yoav wants to take another > look) > > https://codereview.chromium.org/2472583003/diff/340001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): > > https://codereview.chromium.org/2472583003/diff/340001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/timing/PerformanceBase.cpp:327: // such as > about:blank > Could we rephrase this comment to make it clearer why we check > resourceLoadTiming here, like: "Don't create a navigation timing instance when > resourceLoadTiming is null, which could happen when visiting non-http sites such > as about:blank or in some error cases." > > (Also: please end the comment with period) LGTM % kinuko's comment
The CQ bit was checked by sunjian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from panicker@chromium.org Link to the patchset: https://codereview.chromium.org/2472583003/#ps340001 (title: "replaced old layout test with http layout test")
The CQ bit was unchecked by sunjian@chromium.org
The CQ bit was checked by sunjian@chromium.org
The CQ bit was unchecked by sunjian@chromium.org
https://codereview.chromium.org/2472583003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2472583003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:327: // such as about:blank On 2016/11/18 06:53:38, kinuko wrote: > Could we rephrase this comment to make it clearer why we check > resourceLoadTiming here, like: "Don't create a navigation timing instance when > resourceLoadTiming is null, which could happen when visiting non-http sites such > as about:blank or in some error cases." > > (Also: please end the comment with period) Done.
The CQ bit was checked by sunjian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, yoav@yoav.ws, panicker@chromium.org Link to the patchset: https://codereview.chromium.org/2472583003/#ps360001 (title: "addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by sunjian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, yoav@yoav.ws, panicker@chromium.org Link to the patchset: https://codereview.chromium.org/2472583003/#ps380001 (title: "synced cl")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by sunjian@chromium.org
The CQ bit was checked by sunjian@chromium.org
The CQ bit was unchecked by sunjian@chromium.org
The CQ bit was checked by sunjian@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/18 21:11:06, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... There's something weird going on here with lacking approvals for LayoutTests: https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux... I thought this was a wildcard / any committer. Any ideas?
On 2016/11/18 21:46:04, Shubhie wrote: > On 2016/11/18 21:11:06, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > There's something weird going on here with lacking approvals for LayoutTests: > https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux... > > I thought this was a wildcard / any committer. Any ideas? Okay, looks like "webexposed" tests require API_OWNERS approval.
panicker@chromium.org changed reviewers: + foolip@chromium.org, rbyers@chromium.org
Rick, Philip - could one of you please approve for API_OWNERS for the updates made to the "webexposed" LayoutTests. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was unchecked by sunjian@chromium.org
On 2016/11/18 22:04:41, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Friendly morning ping for Rick / Philip. Please stamp for API_OWNERS?
Description was changed from ========== Implement Navigation Timing Level 2. Design doc link: https://docs.google.com/document/d/1DskJn3cKo8RlHxAKJqOSwYlN5mGLOHf3uWo5lwrUW... BUG=504237 ========== to ========== Implement Navigation Timing Level 2. Design doc link: https://docs.google.com/document/d/1DskJn3cKo8RlHxAKJqOSwYlN5mGLOHf3uWo5lwrUW... Intent to ship: https://groups.google.com/a/chromium.org/d/msg/blink-dev/AT7aqhPcNdw/10jQ2B0H... BUG=504237 ==========
Description was changed from ========== Implement Navigation Timing Level 2. Design doc link: https://docs.google.com/document/d/1DskJn3cKo8RlHxAKJqOSwYlN5mGLOHf3uWo5lwrUW... Intent to ship: https://groups.google.com/a/chromium.org/d/msg/blink-dev/AT7aqhPcNdw/10jQ2B0H... BUG=504237 ========== to ========== Implement Navigation Timing Level 2. Design doc link: https://docs.google.com/document/d/1DskJn3cKo8RlHxAKJqOSwYlN5mGLOHf3uWo5lwrUW... BUG=504237 ==========
Due to the approved intent-to-ship (https://groups.google.com/a/chromium.org/d/msg/blink-dev/AT7aqhPcNdw/10jQ2B0H...) I assumed this CL was intending to ship the new API. But it looks like that's not yet the plan, right? https://codereview.chromium.org/2472583003/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2472583003/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:313: if (!RuntimeEnabledFeatures::performanceNavigationTiming2Enabled()) So the intent here is to implement the new API behind a flag but not ship anything new yet, right? If that's the case then you should add [RuntimeEnabled=PerformanceNavigationTiming2] to the PerformanceNavigationTiming interface definition. That'll keep feature detects like "if (window.PerformanceNavigationTiming)" from succeeding before you flip the RuntimeEnabledFeature. It'll also avoid any changes to virtual/stable/webexposed test output :-) https://codereview.chromium.org/2472583003/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.idl (right): https://codereview.chromium.org/2472583003/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.idl:5: // https://www.w3.org/TR/navigation-timing-2/#sec-PerformanceNavigationTiming nit: please use the living link: https://w3c.github.io/navigation-timing/#sec-PerformanceNavigationTiming We're working on some tooling to auto-detect IDL/spec mismatches and generally we care most about the tip-of-tree version of specs.
The CQ bit was checked by sunjian@chromium.org to run a CQ dry run
That's right we have a few more TODOs remaining, including addition of tests. A future CL will flip the flag. https://codereview.chromium.org/2472583003/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2472583003/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:313: if (!RuntimeEnabledFeatures::performanceNavigationTiming2Enabled()) On 2016/11/21 19:05:01, Rick Byers wrote: > So the intent here is to implement the new API behind a flag but not ship > anything new yet, right? If that's the case then you should add > [RuntimeEnabled=PerformanceNavigationTiming2] to the PerformanceNavigationTiming > interface definition. That'll keep feature detects like "if > (window.PerformanceNavigationTiming)" from succeeding before you flip the > RuntimeEnabledFeature. It'll also avoid any changes to > virtual/stable/webexposed test output :-) Good catch (Jian is updating) Although just want to mention that we still need this check here. The way this inherits from PerformanceEntry, only the additional fields added in subclass are behind the flag in IDL, and without this check, one can still get to the base fields in PerformanceEntry with something like getEntriesByType("navigation") (this was a surprise to me).
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2472583003/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2472583003/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:313: if (!RuntimeEnabledFeatures::performanceNavigationTiming2Enabled()) On 2016/11/21 19:05:01, Rick Byers wrote: > So the intent here is to implement the new API behind a flag but not ship > anything new yet, right? If that's the case then you should add > [RuntimeEnabled=PerformanceNavigationTiming2] to the PerformanceNavigationTiming > interface definition. That'll keep feature detects like "if > (window.PerformanceNavigationTiming)" from succeeding before you flip the > RuntimeEnabledFeature. It'll also avoid any changes to > virtual/stable/webexposed test output :-) Done. https://codereview.chromium.org/2472583003/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.idl (right): https://codereview.chromium.org/2472583003/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.idl:5: // https://www.w3.org/TR/navigation-timing-2/#sec-PerformanceNavigationTiming On 2016/11/21 19:05:01, Rick Byers wrote: > nit: please use the living link: > https://w3c.github.io/navigation-timing/#sec-PerformanceNavigationTiming > We're working on some tooling to auto-detect IDL/spec mismatches and generally > we care most about the tip-of-tree version of specs. Done.
The CQ bit was checked by sunjian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/21 19:24:39, Shubhie wrote: > That's right we have a few more TODOs remaining, including addition of tests. A > future CL will flip the flag. > > https://codereview.chromium.org/2472583003/diff/380001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): > > https://codereview.chromium.org/2472583003/diff/380001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/timing/PerformanceBase.cpp:313: if > (!RuntimeEnabledFeatures::performanceNavigationTiming2Enabled()) > On 2016/11/21 19:05:01, Rick Byers wrote: > > So the intent here is to implement the new API behind a flag but not ship > > anything new yet, right? If that's the case then you should add > > [RuntimeEnabled=PerformanceNavigationTiming2] to the > PerformanceNavigationTiming > > interface definition. That'll keep feature detects like "if > > (window.PerformanceNavigationTiming)" from succeeding before you flip the > > RuntimeEnabledFeature. It'll also avoid any changes to > > virtual/stable/webexposed test output :-) > > Good catch (Jian is updating) > Although just want to mention that we still need this check here. > The way this inherits from PerformanceEntry, only the additional fields added in > subclass are behind the flag in IDL, and without this check, one can still get > to the base fields in PerformanceEntry with something like > getEntriesByType("navigation") (this was a surprise to me). Yep, makes sense. LGTM (not that you need my stamp anymore now that this CL doesn't ship anything). I reviewed just the webexposed/ changes, IDL files and runtimeenabledfeatures.in.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sunjian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yoav@yoav.ws, panicker@chromium.org, kinuko@chromium.org Link to the patchset: https://codereview.chromium.org/2472583003/#ps420001 (title: "fixed layout tests failure")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 420001, "attempt_start_ts": 1479774612916610,
"parent_rev": "7811a564615fce1a594137c0ef932c7399d520a3", "commit_rev":
"afb39a1e2a04ff28975c3b7ace30b4d33b3d23ee"}
Message was sent while issue was closed.
Description was changed from ========== Implement Navigation Timing Level 2. Design doc link: https://docs.google.com/document/d/1DskJn3cKo8RlHxAKJqOSwYlN5mGLOHf3uWo5lwrUW... BUG=504237 ========== to ========== Implement Navigation Timing Level 2. Design doc link: https://docs.google.com/document/d/1DskJn3cKo8RlHxAKJqOSwYlN5mGLOHf3uWo5lwrUW... BUG=504237 ==========
Message was sent while issue was closed.
Committed patchset #22 (id:420001)
Message was sent while issue was closed.
Description was changed from ========== Implement Navigation Timing Level 2. Design doc link: https://docs.google.com/document/d/1DskJn3cKo8RlHxAKJqOSwYlN5mGLOHf3uWo5lwrUW... BUG=504237 ========== to ========== Implement Navigation Timing Level 2. Design doc link: https://docs.google.com/document/d/1DskJn3cKo8RlHxAKJqOSwYlN5mGLOHf3uWo5lwrUW... BUG=504237 Committed: https://crrev.com/fbec30ffe38c608db610239c168df667cab6a454 Cr-Commit-Position: refs/heads/master@{#433765} ==========
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/fbec30ffe38c608db610239c168df667cab6a454 Cr-Commit-Position: refs/heads/master@{#433765} |
