[webkit-reviews] review granted: [Bug 217338] ChromeClient::needsImmediateRenderingUpdate() only exists to work around a WebKit1 bug : [Attachment 410563] Patch

[bugzilla-daemon at webkit.org]

Said Abou-Hallawa <sabouhallawa at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 217338: ChromeClient::needsImmediateRenderingUpdate() only exists to work
around a WebKit1 bug
https://bugs.webkit.org/show_bug.cgi?id=217338

Attachment 410563: Patch

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




--- Comment #2 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
Comment on attachment 410563
  --> https://bugs.webkit.org/attachment.cgi?id=410563
Patch

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

> Source/WebCore/ChangeLog:9
> +	   because the RunLoopObserver was always invalidate at the end of the
callback.

invalidating? or calling invalidate()?

> Source/WebKitLegacy/mac/WebView/WebViewData.h:125
> +    bool m_insideCallback { false };
> +    bool m_rescheduledInsideCallback { false };

I think using two booleans can be a little bit difficult to understand. Can't
we use enum for the states of these two members?

    enum class SchedulingState { None, InsideCallback, AlreadyRescheduled } //
Maybe the names need to be changed.

> Source/WebKitLegacy/mac/WebView/WebViewData.mm:129
> +	   SetForScope<bool> insideCallbackScope(m_insideCallback, true);
> +	   m_rescheduledInsideCallback = false;

This can be changed to 
    SetForScope<SchedulingState> change(m_schedulingState,
SchedulingState::InsideCallback);