[webkit-reviews] review granted: [Bug 56067] Overlay scrollers in overflow areas need send notifications at appropriate times (showing up, resizing) : [Attachment 85271] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Beth Dakin <bdakin at apple.com>'s
request for review:
Bug 56067: Overlay scrollers in overflow areas need send notifications at
appropriate times (showing up, resizing)
https://bugs.webkit.org/show_bug.cgi?id=56067

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

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

> Source/WebCore/page/EventHandler.cpp:1873
> +	   RenderLayer* layerForLastNode = m_lastNodeUnderMouse &&
m_lastNodeUnderMouse->renderer()
> +					   ?
m_lastNodeUnderMouse->renderer()->enclosingLayer() : 0;
> +	   RenderLayer* layerForNodeUnderMouse = m_nodeUnderMouse &&
m_nodeUnderMouse->renderer()
> +					   ?
m_nodeUnderMouse->renderer()->enclosingLayer() : 0;

I’d like to use a helper function to get the layer for a node so we don’t have
these unpleasant ? : expressions. That helper could also be used in
EventHandler::mouseMoved.

> Source/WebCore/page/EventHandler.cpp:1877
> +	       // The mouse has moved between frames

We normally use periods in this kind of comment.

> Source/WebCore/page/FocusController.cpp:414
> +	   HashSet<ScrollableArea*>* scrollableAreas =
m_page->scrollableAreaSet();

Missing check for null.

> Source/WebCore/page/FrameView.cpp:2061
> +    HashSet<ScrollableArea*>* scrollableAreas = page->scrollableAreaSet();

Missing check for null.

> Source/WebCore/page/Page.cpp:898
> +	   m_scrollableAreaSet = new ScrollableAreaSet;

Please use adoptPtr here.

> Source/WebCore/page/Page.h:286
> +	   bool pageContainsScrollableArea(ScrollableArea*) const;

Don’t need the word page in this function name. The class is Page.

> Source/WebCore/page/Page.h:287
> +	   ScrollableAreaSet* scrollableAreaSet() const { return
m_scrollableAreaSet.get(); }

This should probably return a const ScrollableAreaSet* since you only use it to
iterate all the areas.