[webkit-reviews] review granted: [Bug 131869] Latched scrolling may interact badly with custom programmatic scrolling : [Attachment 229714] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Brent Fulgham <bfulgham at webkit.org>'s
request for review:
Bug 131869: Latched scrolling may interact badly with custom programmatic
scrolling
https://bugs.webkit.org/show_bug.cgi?id=131869

Attachment 229714: Patch
https://bugs.webkit.org/attachment.cgi?id=229714&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=229714&action=review


> Source/WebCore/dom/Element.cpp:802
> +    if (RenderBox* rend = renderBox()) {

rend is such an unpleasant local variable name; I suggest renderer instead

> Source/WebCore/dom/Element.cpp:804
> +	   if (ScrollableArea* scrollableArea =
static_cast<ScrollableArea*>(rend->layer()))

This typecast is a bad idea. A RenderLayer is a ScrollableArea, so there is no
reason to cast, but further, no reason to narrow the type like this. It should
just say:

    if (auto* layer = renderer->layer())
	layer->setScrolledProgrammatically(true);

> Source/WebCore/dom/Element.cpp:805
> +	       scrollableArea->setScrolledProgrammatically(true);

This won’t ever get reset to false? It’s a permanent state of affairs?

> Source/WebCore/page/EventHandler.h:301
> +protected:
> +    void clearLatchedState();

should be private not protected; no classes derive from EventHandler, so
there’s no difference between protected and private

Also, should always prefer private to protected unless there is some reason a
member needs to be exposed to derived classes.