Bug 184363 – Add support for Navigation Timing Level 2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

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

[Clint Stevenson]

Seconded. We do have PerformanceTiming, but that's been deprecated since (2014?):
https://w3c.github.io/navigation-timing/#the-performancetiming-interface

[Radar WebKit Bug Importer]

<rdar://problem/71197716>

[Alex Christensen]

Created attachment 417477 [details]
Patch

[EWS Watchlist]

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

[Alex Christensen]

Created attachment 417502 [details]
Patch

[Ryosuke Niwa]

Looks like the test failures are legitimate.

[Darin Adler]

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").

[Jakub G (dailymotion)]

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

[Alex Christensen]

Created attachment 428722 [details]
Patch

[Alex Christensen]

Created attachment 428723 [details]
Patch

[Alex Christensen]

Created attachment 428729 [details]
Patch

[Alex Christensen]

Created attachment 428731 [details]
Patch

[Alex Christensen]

Created attachment 428745 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 428745 [details]
Patch

Can we add a runtime flag for this?

[Alex Christensen]

Created attachment 428956 [details]
Patch

[Alex Christensen]

Good idea, Ryosuke.  I verified the experimental feature is off by default and can be turned on.  I turned it on for tests.

[Ryosuke Niwa]

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.

[Alex Christensen]

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.

[Alex Christensen]

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.

[Alex Christensen]

Created attachment 428984 [details]
Patch

[Alex Christensen]

Created attachment 428985 [details]
Patch

[Sam Weinig]

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.

[Alex Christensen]

Aha!
It might be worth renaming RuntimeEnabledFeatures to DeprecatedRuntimeEnabledFeatures

[Alex Christensen]

Created attachment 429059 [details]
Patch

[Sam Weinig]

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

[Alex Christensen]

Created attachment 429085 [details]
Patch

[Alex Christensen]

r277767