[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
ContentChangeObserver::adjustObservedState
https://bugs.webkit.org/show_bug.cgi?id=195244
Attachment 363422: Patch
https://bugs.webkit.org/attachment.cgi?id=363422&action=review
--- Comment #3 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 363422
--> https://bugs.webkit.org/attachment.cgi?id=363422
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=363422&action=review
>
LayoutTests/fast/events/touch/ios/visibility-change-happens-at-the-second-timer
.html:3
> +<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
>
LayoutTests/fast/events/touch/ios/visibility-change-happens-at-the-second-timer
.html:50
> + 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:
Event::InstalledDOMTimer.
More information about the webkit-reviews
mailing list