Bug 184363 - Add support for Navigation Timing Level 2
Summary: Add support for Navigation Timing Level 2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Safari 11
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar, WPTImpact
Depends on:
Blocks:
 
Reported: 2018-04-06 12:38 PDT by cvazac
Modified: 2021-05-19 17:33 PDT (History)
29 users (show)

See Also:


Attachments
Patch (163.14 KB, patch)
2021-01-12 12:23 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (185.51 KB, patch)
2021-01-12 17:43 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (203.66 KB, patch)
2021-05-15 00:24 PDT, Alex Christensen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (204.43 KB, patch)
2021-05-15 00:30 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (211.55 KB, patch)
2021-05-15 10:24 PDT, Alex Christensen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (213.81 KB, patch)
2021-05-15 10:58 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (215.92 KB, patch)
2021-05-15 15:14 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (220.51 KB, patch)
2021-05-18 11:09 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (220.76 KB, patch)
2021-05-18 15:05 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (220.76 KB, patch)
2021-05-18 15:06 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (218.77 KB, patch)
2021-05-19 10:11 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (216.37 KB, patch)
2021-05-19 12:30 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description cvazac 2018-04-06 12:38:33 PDT
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
Comment 1 Clint Stevenson 2019-02-07 14:43:05 PST
Seconded. We do have PerformanceTiming, but that's been deprecated since (2014?):
https://w3c.github.io/navigation-timing/#the-performancetiming-interface
Comment 2 Radar WebKit Bug Importer 2020-11-09 10:12:01 PST
<rdar://problem/71197716>
Comment 3 Alex Christensen 2021-01-12 12:23:49 PST
Created attachment 417477 [details]
Patch
Comment 4 EWS Watchlist 2021-01-12 12:24:42 PST
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
Comment 5 Alex Christensen 2021-01-12 17:43:56 PST
Created attachment 417502 [details]
Patch
Comment 6 Ryosuke Niwa 2021-01-12 20:02:41 PST
Looks like the test failures are legitimate.
Comment 7 Darin Adler 2021-01-13 10:40:24 PST
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").
Comment 8 Jakub G (dailymotion) 2021-02-15 03:35:40 PST
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
Comment 9 Alex Christensen 2021-05-15 00:24:34 PDT
Created attachment 428722 [details]
Patch
Comment 10 Alex Christensen 2021-05-15 00:30:34 PDT
Created attachment 428723 [details]
Patch
Comment 11 Alex Christensen 2021-05-15 10:24:51 PDT
Created attachment 428729 [details]
Patch
Comment 12 Alex Christensen 2021-05-15 10:58:19 PDT
Created attachment 428731 [details]
Patch
Comment 13 Alex Christensen 2021-05-15 15:14:45 PDT
Created attachment 428745 [details]
Patch
Comment 14 Ryosuke Niwa 2021-05-17 18:25:26 PDT
Comment on attachment 428745 [details]
Patch

Can we add a runtime flag for this?
Comment 15 Alex Christensen 2021-05-18 11:09:47 PDT
Created attachment 428956 [details]
Patch
Comment 16 Alex Christensen 2021-05-18 11:40:14 PDT
Good idea, Ryosuke.  I verified the experimental feature is off by default and can be turned on.  I turned it on for tests.
Comment 17 Ryosuke Niwa 2021-05-18 13:08:44 PDT
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 18 Alex Christensen 2021-05-18 13:39:30 PDT
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 19 Alex Christensen 2021-05-18 14:40:33 PDT
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.
Comment 20 Alex Christensen 2021-05-18 15:05:20 PDT
Created attachment 428984 [details]
Patch
Comment 21 Alex Christensen 2021-05-18 15:06:21 PDT
Created attachment 428985 [details]
Patch
Comment 22 Sam Weinig 2021-05-18 16:14:56 PDT
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.
Comment 23 Alex Christensen 2021-05-19 09:51:46 PDT
Aha!
It might be worth renaming RuntimeEnabledFeatures to DeprecatedRuntimeEnabledFeatures
Comment 24 Alex Christensen 2021-05-19 10:11:11 PDT
Created attachment 429059 [details]
Patch
Comment 25 Sam Weinig 2021-05-19 10:25:40 PDT
(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).
Comment 26 Alex Christensen 2021-05-19 12:30:34 PDT
Created attachment 429085 [details]
Patch
Comment 27 Alex Christensen 2021-05-19 17:33:13 PDT
r277767