While working on https://bugs.webkit.org/show_bug.cgi?id=175569, I found that there is not yet support for PerformanceNavigationTiming, aka performance.getEntriesByType('navigation'). Spec (editors draft): https://w3c.github.io/navigation-timing WPT results, see ("nav2*"): https://wpt.fyi/navigation-timing
Seconded. We do have PerformanceTiming, but that's been deprecated since (2014?): https://w3c.github.io/navigation-timing/#the-performancetiming-interface
<rdar://problem/71197716>
Created attachment 417477 [details] Patch
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Created attachment 417502 [details] Patch
Looks like the test failures are legitimate.
Comment on attachment 417502 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417502&action=review > Source/WebCore/ChangeLog:11 > + I noted that exposing transfer size is prolematic, and some of the tests for reading transfer size still fail. Noticed a typo/spelling error ("prolematic").
Note (orthogonal but somehow related): When shipping this, it would be great to also enable `serverTiming` which as far as I can tell is still disabled, both on `resource`-s and `navigation`-s. https://bugs.webkit.org/show_bug.cgi?id=190609
Created attachment 428722 [details] Patch
Created attachment 428723 [details] Patch
Created attachment 428729 [details] Patch
Created attachment 428731 [details] Patch
Created attachment 428745 [details] Patch
Comment on attachment 428745 [details] Patch Can we add a runtime flag for this?
Created attachment 428956 [details] Patch
Good idea, Ryosuke. I verified the experimental feature is off by default and can be turned on. I turned it on for tests.
Comment on attachment 428956 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428956&action=review > Source/WebCore/dom/Document.cpp:1391 > m_documentTiming.domComplete = MonotonicTime::now(); How about calling MonotonicTime::now() above switch so that we can have the identical value for m_documentTiming.domComplete and m_documentTiming.domInteractive in fall-through case? > Source/WebCore/loader/DocumentLoader.cpp:423 > + if (auto* document = this->document()) { Use makeRefPtr? > Source/WebCore/loader/DocumentLoader.cpp:424 > + if (auto* domWindow = document->domWindow()) { Ditto; also, why not check RuntimeEnabledFeatures here at the same time: if (auto domWindow = makeRefPtr(document->domWindow()); RuntimeEnabledFeatures::sharedFeatures().performanceNavigationTimingAPIEnabled()) > Source/WebCore/page/Performance.cpp:232 > + m_navigationTiming = PerformanceNavigationTiming::create(m_timeOrigin, resource, timing, metrics, document.timing(), document.securityOrigin(), documentLoader.triggeringAction().type()); Can we assert that the runtime flag is enabled here? > Source/WebCore/page/PerformanceNavigationTiming.cpp:65 > +double PerformanceNavigationTiming::millisecondsSinceOrigin(MonotonicTime time, Optional<MonotonicTime> timeOrigin) const > +{ > + if (!time) > + return 0; > + return Performance::reduceTimeResolution(time - (timeOrigin ? *timeOrigin : m_timeOrigin)).milliseconds(); > +} Can we just use Performance::relativeTimeFromTimeOriginInReducedResolution? We can make Performance inherit from CanMakeWeak. > Source/WebCore/page/PerformanceResourceTiming.cpp:105 > - if (!m_shouldReportDetails) > + if (!m_resourceTiming.allowTimingDetails()) Do we also need to check redirect conditions here? > LayoutTests/imported/w3c/web-platform-tests/navigation-timing/nav2_test_redirect_chain_xserver_partial_opt_in-expected.txt:9 > - > +FAIL Navigation Timing 2 WPT TypeError: undefined is not an object (evaluating 'frame_performance.getEntriesByType("navigation")[0].type') Is this FAIL expected? > LayoutTests/imported/w3c/web-platform-tests/navigation-timing/nav2_test_redirect_server-expected.txt:8 > +FAIL Navigation Timing 2 WPT Error: assert_true: Expected startTime to be no greater than redirectStart. expected true got false Hm... maybe this test needs to be updated upstream? > LayoutTests/imported/w3c/web-platform-tests/navigation-timing/nav2_test_redirect_xserver-expected.txt:9 > - > +FAIL Navigation Timing 2 WPT TypeError: undefined is not an object (evaluating 'performanceNamespace.getEntriesByType("navigation")[0].type') Is this failure expected?? > LayoutTests/imported/w3c/web-platform-tests/navigation-timing/nav2_test_unique_nav_instances-expected.txt:6 > +FAIL Each window has a unique nav timing 2 instance. assert_equals: Only one nav timing instance exists. expected 1 but got 0 Looks like we're failing to insert navigation entry in iframe? That seems to be the common theme with these failing tests. > LayoutTests/imported/w3c/web-platform-tests/navigation-timing/secure_connection_start_non_zero.https-expected.txt:6 > -FAIL Test that secureConnectionStart is not zero undefined is not an object (evaluating 'entry.secureConnectionStart') > +FAIL Test that secureConnectionStart is not zero assert_greater_than: secureConnectionStart is non-zero expected a number greater than 0 but got 0 Why is this failing? > LayoutTests/imported/w3c/web-platform-tests/performance-timeline/supportedEntryTypes.any-expected.txt:3 > PASS supportedEntryTypes exists and returns entries in alphabetical order > +FAIL supportedEntryTypes caches result assert_true: expected true got false Huh, I think we need [CachedAttribute] on supportedEntryTypes. > LayoutTests/imported/w3c/web-platform-tests/performance-timeline/supportedEntryTypes.any.worker-expected.txt:3 > +FAIL supportedEntryTypes caches result assert_true: expected true got false Same issue.
Comment on attachment 428956 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428956&action=review >> Source/WebCore/dom/Document.cpp:1391 >> m_documentTiming.domComplete = MonotonicTime::now(); > > How about calling MonotonicTime::now() above switch so that we can have the identical value for > m_documentTiming.domComplete and m_documentTiming.domInteractive in fall-through case? Great idea. >> Source/WebCore/loader/DocumentLoader.cpp:424 >> + if (auto* domWindow = document->domWindow()) { > > Ditto; also, why not check RuntimeEnabledFeatures here at the same time: > if (auto domWindow = makeRefPtr(document->domWindow()); RuntimeEnabledFeatures::sharedFeatures().performanceNavigationTimingAPIEnabled()) That doesn't null check domWindow. I'd need a "domWindow && " after the semicolon >> Source/WebCore/page/Performance.cpp:232 >> + m_navigationTiming = PerformanceNavigationTiming::create(m_timeOrigin, resource, timing, metrics, document.timing(), document.securityOrigin(), documentLoader.triggeringAction().type()); > > Can we assert that the runtime flag is enabled here? Good idea. >> Source/WebCore/page/PerformanceNavigationTiming.cpp:65 >> +} > > Can we just use Performance::relativeTimeFromTimeOriginInReducedResolution? > We can make Performance inherit from CanMakeWeak. No, that uses Performance::m_timeOrigin, not PerformanceNavigationTiming::m_timeOrigin. I need to redo all our time origins I think. >> Source/WebCore/page/PerformanceResourceTiming.cpp:105 >> + if (!m_resourceTiming.allowTimingDetails()) > > Do we also need to check redirect conditions here? possibly. That should be its own change, though. >> LayoutTests/imported/w3c/web-platform-tests/navigation-timing/nav2_test_redirect_chain_xserver_partial_opt_in-expected.txt:9 >> +FAIL Navigation Timing 2 WPT TypeError: undefined is not an object (evaluating 'frame_performance.getEntriesByType("navigation")[0].type') > > Is this FAIL expected? Yes. As explained in the change log, this uses www.localhost, which our test infrastructure doesn't support. >> LayoutTests/imported/w3c/web-platform-tests/navigation-timing/nav2_test_redirect_xserver-expected.txt:9 >> +FAIL Navigation Timing 2 WPT TypeError: undefined is not an object (evaluating 'performanceNamespace.getEntriesByType("navigation")[0].type') > > Is this failure expected?? ditto >> LayoutTests/imported/w3c/web-platform-tests/navigation-timing/nav2_test_unique_nav_instances-expected.txt:6 >> +FAIL Each window has a unique nav timing 2 instance. assert_equals: Only one nav timing instance exists. expected 1 but got 0 > > Looks like we're failing to insert navigation entry in iframe? > That seems to be the common theme with these failing tests. That's the part of this that is yet to be implemented. >> LayoutTests/imported/w3c/web-platform-tests/navigation-timing/secure_connection_start_non_zero.https-expected.txt:6 >> +FAIL Test that secureConnectionStart is not zero assert_greater_than: secureConnectionStart is non-zero expected a number greater than 0 but got 0 > > Why is this failing? This is flaky. It assumes there's no reused connection at the beginning of the test, which isn't a good assumption in our test infrastructure. I have an idea how to do a bunch of "Connection: close" responses before this to clear out any cached connections there may be, but I'm planning to upstream that separately if it does work. I wanted to leave all the tests as they are upstream in this import/implementation. >> LayoutTests/imported/w3c/web-platform-tests/performance-timeline/supportedEntryTypes.any-expected.txt:3 >> +FAIL supportedEntryTypes caches result assert_true: expected true got false > > Huh, I think we need [CachedAttribute] on supportedEntryTypes. And we need to change our bindings generator to do it right on a static attribute like this.
Comment on attachment 428956 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428956&action=review >>> Source/WebCore/dom/Document.cpp:1391 >>> m_documentTiming.domComplete = MonotonicTime::now(); >> >> How about calling MonotonicTime::now() above switch so that we can have the identical value for >> m_documentTiming.domComplete and m_documentTiming.domInteractive in fall-through case? > > Great idea. I think that may increase the calls to MonotonicTime::now, which would mildly hurt performance. I'm going to keep this as it is since it is quite rare for both calls to be hit.
Created attachment 428984 [details] Patch
Created attachment 428985 [details] Patch
Comment on attachment 428985 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428985&action=review > Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml:789 > + webcoreBinding: RuntimeEnabledFeatures If this can be made a Setting, please use that instead. I am working on removing RuntimeEnabledFeatures and adding more just increases that work. If Setting won't work for this, that's ok, just please let me know what issue you ran into so I can try to fix that. > Tools/DumpRenderTree/TestOptions.cpp:95 > + { "PerformanceNavigationTimingAPIEnabled", true }, This is not necessary. All Experimental features are on by default in the tests. (except in Windows DRT, so you may have to add it below to keep the windows bots happy). > Tools/WebKitTestRunner/TestOptions.cpp:105 > + { "PerformanceNavigationTimingAPIEnabled", true }, This is not necessary. All Experimental features are on by default in the tests.
Aha! It might be worth renaming RuntimeEnabledFeatures to DeprecatedRuntimeEnabledFeatures
Created attachment 429059 [details] Patch
(In reply to Alex Christensen from comment #23) > Aha! > It might be worth renaming RuntimeEnabledFeatures to > DeprecatedRuntimeEnabledFeatures Good point. Will do (and merge with the other similar named one).
Created attachment 429085 [details] Patch
r277767