[webkit-reviews] review granted: [Bug 131869] Latched scrolling may interact badly with custom programmatic scrolling : [Attachment 229714] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat Apr 19 14:53:35 PDT 2014
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.
More information about the webkit-reviews
mailing list