[Webkit-unassigned] [Bug 136273] overflow:scroll elements should not latch to the body if the body is overflow:hidden

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=136273





--- Comment #6 from Beth Dakin <bdakin at apple.com>  2014-08-27 12:24:34 PST ---
(In reply to comment #4)
> (From update of attachment 237233 [details])
> 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.
> 

Changed this to definitionOfScrollable

> > 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.
> 

Actually, I think we do need null checks here! Eek! Added those.

> I prefer && for these rather than ? : false.
> 
>     RenderLayer* layer = parentFrameView()->renderView()->enclosingLayer();
>     return layer && layer->hasScrollableOrRubberbandableAncestor();
> 

I changed this to use &&.

> > 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.
> 

I think that the new one (definitionOfScrollable) is helpful, so I left it.

> > 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.
> 

I added a virtual function on RenderBox, overridden by RenderView.

> > Source/WebCore/rendering/RenderLayer.cpp:3078
> > +            return nextLayer;
> 
> Should say return true here. This is just a roundabout way to write it!
> 

Silly! The code is not here anymore now that I added the virtual function on RenderBox.

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

Changed to &&.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.