Bug 62095 – Disable WebProcess side display throttling when in a user scroll

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Disable WebProcess side display throttling when in a user scroll

[Sam Weinig]

Created attachment 96032 [details]
Patch

[Sam Weinig]

<rdar://problem/9517175>

[Darin Adler]

Comment on attachment 96032 [details]
Patch

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

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:778
> +    m_page->drawingArea()->enableDisplayThrottling();

What if this rubber-banding is ending after some other scrolling has begun? Can that happen? Does starting a new scroll terminate rubber-banding already in effect? If not, then I’m not sure it’s correct to unconditionally enable. Should this be a count instead of a single flag? A count would be more risky, though, because if it got off by one you could be left in the wrong state permanently.

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:792
> +    m_page->drawingArea()->enableDisplayThrottling();

Same question as for rubber banding. Could we have started some other kind of scroll before the animated scroll completed?

> Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.h:122
> +    // Whether we should throttle displays to a set update rate on the WebProcess
> +    // side.

Awkward line break. I suggest merging this comment into one line.

> Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.h:123
> +    bool m_throttleDisplay;

For boolean data members we try to avoid verb phrases. I suggest either m_shouldThrottleDisplay or m_isThrottlingDisplay.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1072
> +#if PLATFORM(MAC)

Why put this inside an #if? I’d think we’d want this on any platform where enableDisableDisplayThrottling has an effect.

[Sam Weinig]

Committed r88127: <http://trac.webkit.org/changeset/88127>

[Sam Weinig]

(In reply to comment #3)
> (From update of attachment 96032 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96032&action=review
> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:778
> > +    m_page->drawingArea()->enableDisplayThrottling();
> 
> What if this rubber-banding is ending after some other scrolling has begun? Can that happen? Does starting a new scroll terminate rubber-banding already in effect? If not, then I’m not sure it’s correct to unconditionally enable. Should this be a count instead of a single flag? A count would be more risky, though, because if it got off by one you could be left in the wrong state permanently.
> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:792
> > +    m_page->drawingArea()->enableDisplayThrottling();
> 
> Same question as for rubber banding. Could we have started some other kind of scroll before the animated scroll completed?

In very rare cases, I believe we can get out of order calls to enable/disable, but the affect is marginal, and will be rectified by a subsequent scroll.

> 
> > Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.h:122
> > +    // Whether we should throttle displays to a set update rate on the WebProcess
> > +    // side.
> 
> Awkward line break. I suggest merging this comment into one line.

Fixed.

> 
> > Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.h:123
> > +    bool m_throttleDisplay;
> 
> For boolean data members we try to avoid verb phrases. I suggest either m_shouldThrottleDisplay or m_isThrottlingDisplay.

Fixed.

> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1072
> > +#if PLATFORM(MAC)
> 
> Why put this inside an #if? I’d think we’d want this on any platform where enableDisableDisplayThrottling has an effect.

This only works, for platforms that support momentum scrolling and thus have scroll phases, and right now that is just the mac.