[webkit-reviews] review denied: [Bug 22769] Scrolling allowed when overflow:hidden (seen on Acid2) : [Attachment 209756] patch 1 - for review

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Antonio Gomes <tonikitoo at webkit.org>'s
request for review:
Bug 22769: Scrolling allowed when overflow:hidden (seen on Acid2)
https://bugs.webkit.org/show_bug.cgi?id=22769

Attachment 209756: patch 1 - for review
https://bugs.webkit.org/attachment.cgi?id=209756&action=review

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


Generally looks OK to me. There are some small things that need to change,
though.

> Source/WebCore/rendering/RenderBox.cpp:820
> +    bool isDocumentNode = node() == &document();

The usual way to write this is:

    node()->isDocumentNode()

I don’t think we need a local variable for this.

> Source/WebCore/rendering/RenderBox.cpp:822
> +	   return &frame() && view().frameView().isScrollable();

The whole point of having frame() return a reference is that it’s guaranteed
never to be zero. So it’s nonsense to check &frame() to see if it’s zero!

> Source/WebCore/rendering/RenderBox.cpp:-828
> -    // Check for a box that represents the top level of a web page.
> -    // This can be scrolled by calling Chrome::scrollRectIntoView.
> -    // This only has an effect on the Mac platform in applications
> -    // that put web views into scrolling containers, such as Mac OS X Mail.
> -    // The code for this is in RenderLayer::scrollRectToVisible.

Could you explain more about why it’s OK to remove this comment?

> Source/WebCore/rendering/RenderBox.cpp:-832
> -    return page && &page->mainFrame() == &frame() &&
view().frameView().isScrollable();

Why is it OK to remove the mainFrame check?

> Source/WebCore/rendering/RenderBox.cpp:828
> +    ASSERT(node() != &document());

I think it’s silly to re-assert this here. It was checked at the top of the
function.