[webkit-reviews] review granted: [Bug 195244] [ContentChangeObserver] Introduce ContentChangeObserver::adjustObservedState : [Attachment 363422] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Mar 2 13:27:28 PST 2019

Simon Fraser (smfr) <simon.fraser at apple.com> has granted zalan
<zalan at apple.com>'s request for review:
Bug 195244: [ContentChangeObserver] Introduce

Attachment 363422: Patch


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

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

> +<title>This test that we trigger hover when the content change starts as
async but it turns into a sync style recalc.</title>

This tests that

> +	   document.body.offsetHeight;

Is this necessary?

> Source/WebCore/page/ios/ContentChangeObserver.cpp:143
>  void ContentChangeObserver::didContentVisibilityChange()

contentVisibilityDidChange? didContentVisibilityChange sounds like a question.

> Source/WebCore/page/ios/ContentChangeObserver.cpp:197
> +    if (event == Event::DidContentObservationStart) {

Use a switch so that you get warnings from the complier?

> Source/WebCore/page/ios/ContentChangeObserver.cpp:219
> +void ContentChangeObserver::signalContentChangeIfNeeded()

I think "notify" would be a better term to use.

> Source/WebCore/page/ios/ContentChangeObserver.h:93
> +    void addObservedDOMTimer(const DOMTimer&);
>      void removeObservedDOMTimer(const DOMTimer&);

Normally we'd say "startObservingFoo/stopObservingFoo".

> Source/WebCore/page/ios/ContentChangeObserver.h:115
> +    enum class Event {

Event is a bit of an overloaded term. Maybe OK here.

> Source/WebCore/page/ios/ContentChangeObserver.h:119
> +	   DidContentObservationStart,
> +	   DidInstallDOMTimer,
> +	   DidRemoveDOMTimer,
> +	   DidContentVisibilityChange

I think you can remove the "Did" from all these. Maybe use the past tense:

More information about the webkit-reviews mailing list