[webkit-reviews] review granted: [Bug 136273] overflow:scroll elements should not latch to the body if the body is overflow:hidden : [Attachment 237233] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 27 11:39:03 PDT 2014


Darin Adler <darin at apple.com> has granted Beth Dakin <bdakin at apple.com>'s
request for review:
Bug 136273: overflow:scroll elements should not latch to the body if the body
is overflow:hidden
https://bugs.webkit.org/show_bug.cgi?id=136273

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

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


> Source/WebCore/page/FrameView.cpp:3268
> +bool FrameView::isScrollable(Scrollability defineScrollable)

I would call this definitionOfScrollable or scrollabilityDefinition or some
other noun phrase. “define scrollable” is a verb phrase.

> Source/WebCore/page/FrameView.cpp:3310
> +    return parentView->enclosingLayer() ?
parentView->enclosingLayer()->hasScrollableOrRubberbandableAncestor() : false;

Could the local variable be the layer rather than the parentView? The point of
the local variable is to make the null check possible, and apparently the
parent view can’t be null.

I prefer && for these rather than ? : false.

    RenderLayer* layer = parentFrameView()->renderView()->enclosingLayer();
    return layer && layer->hasScrollableOrRubberbandableAncestor();

> Source/WebCore/page/FrameView.h:405
> +    bool isScrollable(Scrollability defineScrollable =
Scrollability::Scrollable);

Should omit the argument name here. Unless the new one makes it really easy to
understand.

> Source/WebCore/page/FrameView.h:406
> +    virtual bool hasScrollableOrRubberbandableAncestor() override;

Can this be private instead of public?

> Source/WebCore/rendering/RenderLayer.cpp:3068
> +bool RenderLayer::hasScrollableOrRubberbandableAncestor()

Seems messy for this function to special-case RenderView and RenderBox rather
than using a virtual function to deal with that.

> Source/WebCore/rendering/RenderLayer.cpp:3078
> +	       return nextLayer;

Should say return true here. This is just a roundabout way to write it!

> Source/WebCore/rendering/RenderLayer.h:442
> +    virtual bool hasScrollableOrRubberbandableAncestor() override;

Does this need to be public?

> Source/WebCore/rendering/RenderListBox.cpp:792
> +    return enclosingLayer() ?
enclosingLayer()->hasScrollableOrRubberbandableAncestor() : false;

I prefer && instead of ? : false.


More information about the webkit-reviews mailing list