[webkit-reviews] review denied: [Bug 64143] REGRESSION (r60268): Page-Up or Page-Down scrolls a scrollable textarea and the next embedding scrollable area at the same time : [Attachment 121936] rebased patch for review

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Kentaro Hara
<haraken at chromium.org>'s request for review:
Bug 64143: REGRESSION (r60268): Page-Up or Page-Down scrolls a scrollable
textarea and the next embedding scrollable area at the same time
https://bugs.webkit.org/show_bug.cgi?id=64143

Attachment 121936: rebased patch for review
https://bugs.webkit.org/attachment.cgi?id=121936&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=121936&action=review


> Source/WebCore/ChangeLog:11
> +	   strategies, AlignCursorOnScrollOfScrollableElement, which does not
scroll a parent layer

I don't think "of" is a good preposition here. How about "inside"? i.e.
AlignCursorOnScrollInsideScrollableElement
Or maybe AlignCursorOnScrollInInnerMostScrollableElement since you're only
scrolling in the inner most one if you have multiple scrollable elements?

> Source/WebCore/ChangeLog:13
> +

Could you also explain how you're implement this new behavior using
alignCenterIfChildIsNotScrollable?

> Source/WebCore/editing/FrameSelection.cpp:445
> +ScrollAlignment FrameSelection::getScrollAlignment(CursorAlignOnScroll
align, bool shouldCenterAlignWhenSelectionIsRevealed)

We use "get" prefix iff it has an out argument.
Also, this function can be static local.

> Source/WebCore/editing/FrameSelection.cpp:456
> +    } else {

No else since the previous "if" block all ends with "return".

> Source/WebCore/rendering/RenderLayer.cpp:1586
> +    if (childScrollable && (ScrollAlignment::getVisibleBehavior(alignX) ==
alignCenterIfChildIsNotScrollable ||
ScrollAlignment::getVisibleBehavior(alignX) == alignTopIfChildIsNotScrollable))


Can we add a helper inline function?

>
LayoutTests/platform/chromium-linux/fast/layers/scroll-rect-to-visible-expected
.txt:2
> +layer at (0,0) size 785x664
> +  RenderView at (0,0) size 785x600

Where is scroll-rect-to-visible.html added? r- because you seem to be missing
the test for this expectation.