[webkit-reviews] review granted: [Bug 61339] Can't scroll scaled page that has overflow:hidden on its root : [Attachment 94548] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 61339: Can't scroll scaled page that has overflow:hidden on its root
https://bugs.webkit.org/show_bug.cgi?id=61339

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

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

I’m going to say r=me but this probably needs more refinement.

>> Source/WebCore/page/FrameView.cpp:523
>> +	    case OHIDDEN: {
> 
> A case label should not be indented, but line up with its switch statement. 
[whitespace/indent] [4]

No need to add braces.

> Source/WebCore/page/FrameView.cpp:527
> +	       if (m_frame->pageScaleFactor() != 1)
> +		   hMode = ScrollbarAuto;
> +	       else
> +		   hMode = ScrollbarAlwaysOff;

I think you want to check if the scale factor is > 1. I don’t think that != is
quite right.

Also, I think this logic is right for the top level frame, but not sure it’s
helpful for nested frames.

> LayoutTests/ChangeLog:19
> +2011-05-23  Sam Weinig  <sam at webkit.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Need a short description and bug URL (OOPS!)
> +
> +	   *
fast/events/scroll-in-scaled-page-with-overflow-hidden-expected.txt: Added.
> +	   * fast/events/scroll-in-scaled-page-with-overflow-hidden.html:
Added.
> +
> +2011-05-23  Sam Weinig  <sam at webkit.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Can't scroll scaled page that has overflow:hidden on its root
> +	   https://bugs.webkit.org/show_bug.cgi?id=61339
> +
> +	   *
fast/events/scroll-in-scaled-page-with-overflow-hidden-expected.txt: Added.
> +	   * fast/events/scroll-in-scaled-page-with-overflow-hidden.html:
Added.
> +

Double change log is not good.