[webkit-reviews] review granted: [Bug 207153] Timestamps should be the same for all rendering update steps : [Attachment 397439] Patch

Fri Apr 24 09:51:02 PDT 2020

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 207153: Timestamps should be the same for all rendering update steps
https://bugs.webkit.org/show_bug.cgi?id=207153

Attachment 397439: Patch

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

--- Comment #19 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 397439
  --> https://bugs.webkit.org/attachment.cgi?id=397439
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397439&action=review

> Source/WebCore/page/DOMWindow.h:355
>      WEBCORE_EXPORT double nowTimestamp() const;
> +    void freezeNowTimestamp();
> +    void unfreezeNowTimestamp();
> +    double frozenNowTimestamp() const;

It should would be nice of nowTimestamp() and frozenNowTimestamp() returned
Seconds.

> Source/WebCore/page/DOMWindow.h:475
> +    Optional<double> m_frozenNowTimestamp;

Seconds

> Source/WebCore/page/IntersectionObserver.cpp:258
> +	   timestamp = std::round(1000 * window->frozenNowTimestamp());

We need a Seconds -> DOMHighResTimeStamp helper somewhere.

> Source/WebCore/page/Page.cpp:1330
> +    Vector<WeakPtr<Document>> initialDocuments;
> +    forEachDocument([&initialDocuments] (Document& document) {
> +	   document.domWindow()->freezeNowTimestamp();
> +	   initialDocuments.append(makeWeakPtr(&document));
> +    });

I think it's OK to do it like this for now, but longer term I'd like to see the
frozen "now" time be generated in this function, and passed down into all the
functions that need it. That would require having each document compute its own
frozen now time based on its time origin, but it would be nicer than storing
temporary state on DOMWindow.

> Source/WebCore/page/Page.cpp:1349
> -	   DOMHighResTimeStamp timestamp =
document.domWindow()->nowTimestamp();
> +	   DOMHighResTimeStamp timestamp =
document.domWindow()->frozenNowTimestamp();

Wait, do we need some seconds/milliseconds conversion here? The lack of types
makes it hard to know.

>
LayoutTests/intersection-observer/intersection-observer-callback-timestamp.html
:1
> +<!DOCTYPE HTML>

This should be a WPT if there's no test that detected our buggy behavior.

>
LayoutTests/intersection-observer/intersection-observer-callback-timestamp.html
:6
> +<style>

<style> should go in the <head>