[webkit-reviews] review granted: [Bug 184363] Add support for Navigation Timing Level 2 : [Attachment 428956] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 184363: Add support for Navigation Timing Level 2
https://bugs.webkit.org/show_bug.cgi?id=184363

Attachment 428956: Patch

https://bugs.webkit.org/attachment.cgi?id=428956&action=review




--- Comment #17 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 428956
  --> https://bugs.webkit.org/attachment.cgi?id=428956
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_redirec
t_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_redirec
t_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_redirec
t_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/supportedEntry
Types.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/supportedEntry
Types.any.worker-expected.txt:3
> +FAIL supportedEntryTypes caches result assert_true: expected true got false

Same issue.