[webkit-reviews] review granted: [Bug 194977] Introduce ContentChangeObserver class : [Attachment 362829] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted  review:
Bug 194977: Introduce ContentChangeObserver class
https://bugs.webkit.org/show_bug.cgi?id=194977

Attachment 362829: Patch

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




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

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

> Source/WebCore/page/DOMTimer.cpp:242
> +	   LOG_WITH_STREAM(ContentObservation, stream << "DOMTimer::install:
registed this timer: (" << timer << ") and observe when it fires.");

registed. Maybe log the timer ID?

> Source/WebCore/page/DOMTimer.cpp:360
> +    if (is<Document>(context) && downcast<Document>(context).page()) {
> +	   page = downcast<Document>(context).page();
> +	   auto& contentChangeObserver = page->contentChangeObserver();
> +	   isObservingLastTimer =
contentChangeObserver.countOfObservedDOMTimers() == 1;
> +	   shouldBeginObservingChanges =
contentChangeObserver.containsObservedDOMTimer(*this);
>      }

If this were a lambda the code would read more nicely.

> Source/WebCore/page/DOMTimer.cpp:391
>	       LOG(ContentObservation, "DOMTimer::fired: in determined
state.");

Log the timer ID?

> Source/WebCore/page/DOMTimer.cpp:395
>	       LOG(ContentObservation, "DOMTimer::fired: wait until next style
recalc fires.");

Ditto.

> Source/WebCore/page/DOMWindow.cpp:1698
> +	   LOG_WITH_STREAM(ContentObservation, stream <<
"DOMWindow::clearTimeout: remove registered timer (" << timer << ")");

Log the ID

> Source/WebCore/page/DOMWindow.cpp:1704
> +    handleObservedTimerCancelIfNeeded();

The lambda doesn't really add anything here.

> Source/WebCore/page/ios/EventHandlerIOS.mm:495
> +    document.updateStyleIfNeeded();

Would be nice to add a comment to say why we have do this here and again just
below.