Unifies ScrollView and RenderLayer to use non-shifted [minPos, maxPos] scroll offset

Unifies ScrollView and RenderLayer to use non-shifted [minPos, maxPos] scroll offset

For pages with negative overflow (due to RTL or writing mode), there are
two sets of scroll offset defined. One that ranges from [0, maxPos - minPos]
and the other one ranges from [minPos, maxPos]. To reduce confusion, this
patch unifies ScrollView and RenderLayer to use the second definition of
scroll offset.

BUG=232063
TEST=manual
R=jamesr@chromium.org,jchaffraix@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=150124

[trchen, 2013-05-02 00:17:44]

https://codereview.chromium.org/14807003/diff/1/LayoutTests/fast/multicol/shr...
File LayoutTests/fast/multicol/shrink-to-column-height-for-pagination.html
(right):

https://codereview.chromium.org/14807003/diff/1/LayoutTests/fast/multicol/shr...
LayoutTests/fast/multicol/shrink-to-column-height-for-pagination.html:10: }
I'm hesitant about what do with this test.

The green box in this test will be paginated to the second page. Not sure if it
is the correct behavior.

If not, the test never worked before. The error was hidden by a scrolling bug.
We shouldn't change the test but instead mark it with ImageFailure.

If yes, the fix is to scroll the window to the second page to match expectation.
The setTimeout here is a horrible hack to wait for the FrameView to update
scroll range before window.scrollTo can do anything.

[jamesr, 2013-05-03 05:59:22]

https://codereview.chromium.org/14807003/diff/1/Source/core/platform/Scrollba...
File Source/core/platform/Scrollbar.cpp (right):

https://codereview.chromium.org/14807003/diff/1/Source/core/platform/Scrollba...
Source/core/platform/Scrollbar.cpp:292: float maxPos = (m_orientation ==
HorizontalScrollbar) ? m_scrollableArea->maximumScrollPosition().x() :
m_scrollableArea->maximumScrollPosition().y();
I think I've seen these lines before - can we share this code somehow?

[trchen, 2013-05-03 23:19:12]

Okay. I added some convenience functions to ScrollablaArea to get scroll
parameters for a certain orientation.

https://codereview.chromium.org/14807003/diff/6001/Source/core/platform/mac/S...
File Source/core/platform/mac/ScrollAnimatorMac.mm (right):

https://codereview.chromium.org/14807003/diff/6001/Source/core/platform/mac/S...
Source/core/platform/mac/ScrollAnimatorMac.mm:670: float newPos =
std::max<float>(std::min<float>(currentPos + (step * multiplier),
m_scrollableArea->maximumScrollPosition(orientation)),
m_scrollableArea->minimumScrollPosition(orientation));
Should we add a float version of ScrollableArea::clampScrollPosition()?

https://codereview.chromium.org/14807003/diff/6001/Source/core/platform/mac/S...
Source/core/platform/mac/ScrollAnimatorMac.mm:700: float newY =
max<float>(min<float>(position.y(), maxPos.y()), minPos.y());
ditto

[jamesr, 2013-05-07 03:33:31]

lgtm

https://codereview.chromium.org/14807003/diff/6001/Source/WebKit/chromium/tes...
File Source/WebKit/chromium/tests/ScrollAnimatorNoneTest.cpp (right):

https://codereview.chromium.org/14807003/diff/6001/Source/WebKit/chromium/tes...
Source/WebKit/chromium/tests/ScrollAnimatorNoneTest.cpp:154: //   
EXPECT_CALL(scrollableArea,
scrollSize(_)).Times(AtLeast(1)).WillRepeatedly(Return(1000));
delete this line if you don't want it, don't just comment it out

https://codereview.chromium.org/14807003/diff/6001/Source/core/platform/Scrol...
File Source/core/platform/Scrollbar.cpp (right):

https://codereview.chromium.org/14807003/diff/6001/Source/core/platform/Scrol...
Source/core/platform/Scrollbar.cpp:291: 
nit: don't need this blank line

[trchen, 2013-05-07 03:39:58]

Thanks! I'm doing a final check to make sure scrolling still works in both
threaded and non-threaded mode, and I'll come up with a unit test by tomorrow.

What do you think of the pagination test? Should I change it as in the patch, or
should I just mark it as expected failure? See my comments in Patch Set 1.

On 2013/05/07 03:33:31, jamesr wrote:
> lgtm
>
https://codereview.chromium.org/14807003/diff/6001/Source/WebKit/chromium/tes...
> File Source/WebKit/chromium/tests/ScrollAnimatorNoneTest.cpp (right):
> 
>
https://codereview.chromium.org/14807003/diff/6001/Source/WebKit/chromium/tes...
> Source/WebKit/chromium/tests/ScrollAnimatorNoneTest.cpp:154: //   
> EXPECT_CALL(scrollableArea,
> scrollSize(_)).Times(AtLeast(1)).WillRepeatedly(Return(1000));
> delete this line if you don't want it, don't just comment it out
> 
>
https://codereview.chromium.org/14807003/diff/6001/Source/core/platform/Scrol...
> File Source/core/platform/Scrollbar.cpp (right):
> 
>
https://codereview.chromium.org/14807003/diff/6001/Source/core/platform/Scrol...
> Source/core/platform/Scrollbar.cpp:291: 
> nit: don't need this blank line

[trchen, 2013-05-09 22:28:55]

Hello James,
I made more changes to RenderLayer and RenderLayerBacking. Please let me know if
you still feel the patch is good to land. Thanks!

As for tests, I just realized that currently we don't have effective way to test
threaded scrolling. I tested manually to make sure both frames and
overflow:scroll scrolling works on linux in all 3 modes. (SW mode, composting
mode, threaded-scrolling mode)

[Julien - ping for review, 2013-05-10 01:03:34]

LGTM

RenderListBox wasn't updated as part of this change (which means we run into the
risk of having 2 different ways to compute the scroll offset). It shouldn't
block the refactoring though.

https://codereview.chromium.org/14807003/diff/33001/Source/core/platform/mac/...
File Source/core/platform/mac/ScrollAnimatorMac.mm (right):

https://codereview.chromium.org/14807003/diff/33001/Source/core/platform/mac/...
Source/core/platform/mac/ScrollAnimatorMac.mm:669: float newPos =
std::max<float>(std::min<float>(currentPos + (step * multiplier),
m_scrollableArea->maximumScrollPosition(orientation)),
m_scrollableArea->minimumScrollPosition(orientation));
Do we really need the extra precision here or can we use
ScrollableArea::clampScrollPosition? (this can be done in a follow-up change to
alleviate some potential regressions)

https://codereview.chromium.org/14807003/diff/33001/Source/core/platform/mac/...
Source/core/platform/mac/ScrollAnimatorMac.mm:699: float newY =
max<float>(min<float>(position.y(), maxPos.y()), minPos.y());
Same question here.

https://codereview.chromium.org/14807003/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/14807003/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:2208: void
RenderLayer::setScrollOffset(const IntPoint& newScrollOffset)
It's a shame that we are not consistent in the code. RenderLayer says that an
offset is an IntSize but not ScrollableArea :(

https://codereview.chromium.org/14807003/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:2600: if (!box)
Only boxes can have scrollbars. Can't we just ASSERT(box) here? If not, I would
rather return 0 as that's what scrollOrigin is in this case.

https://codereview.chromium.org/14807003/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerBacking.cpp (right):

https://codereview.chromium.org/14807003/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBacking.cpp:681: if
(style->shouldPlaceBlockDirectionScrollbarOnLogicalLeft())
This function is a misnomer as it isn't really logical.

[trchen, 2013-05-10 01:27:52]

On 2013/05/10 01:03:34, Julien Chaffraix wrote:
> LGTM
> 
> RenderListBox wasn't updated as part of this change (which means we run into
the
> risk of having 2 different ways to compute the scroll offset). It shouldn't
> block the refactoring though.

So far I have no problem dealing with #item-based coordinates in RenderListBox.
The idea is the scrollOffset can be neutral to units, whether it is pixel-based
or even real world units.

I agree that it is inconsistent. I was surprised when Xianzhu told me about it.
We might want to change it in the future if later it becomes a problem.

>
https://codereview.chromium.org/14807003/diff/33001/Source/core/platform/mac/...
> File Source/core/platform/mac/ScrollAnimatorMac.mm (right):
> 
>
https://codereview.chromium.org/14807003/diff/33001/Source/core/platform/mac/...
> Source/core/platform/mac/ScrollAnimatorMac.mm:669: float newPos =
> std::max<float>(std::min<float>(currentPos + (step * multiplier),
> m_scrollableArea->maximumScrollPosition(orientation)),
> m_scrollableArea->minimumScrollPosition(orientation));
> Do we really need the extra precision here or can we use
> ScrollableArea::clampScrollPosition? (this can be done in a follow-up change
to
> alleviate some potential regressions)
> 
>
https://codereview.chromium.org/14807003/diff/33001/Source/core/platform/mac/...
> Source/core/platform/mac/ScrollAnimatorMac.mm:699: float newY =
> max<float>(min<float>(position.y(), maxPos.y()), minPos.y());
> Same question here.

I think the solution is to do some rewrite to ScrollAnimatorMac. It can use
whatever precision it wants internally, but only clamp and round the offset when
assigning to the ScrollableArea.

>
https://codereview.chromium.org/14807003/diff/33001/Source/core/rendering/Ren...
> File Source/core/rendering/RenderLayer.cpp (right):
> 
>
https://codereview.chromium.org/14807003/diff/33001/Source/core/rendering/Ren...
> Source/core/rendering/RenderLayer.cpp:2208: void
> RenderLayer::setScrollOffset(const IntPoint& newScrollOffset)
> It's a shame that we are not consistent in the code. RenderLayer says that an
> offset is an IntSize but not ScrollableArea :(

Xianzhu and I have a design proposal at graphics-dev@: http://goo.gl/0TbOv
My plan is to use IntSize scrollOffset, and deprecates IntPoint scrollPosition.

The reason is that scroll offset isn't really a point on any plane. It is a
translation offset from its neutral position when doing painting.

>
https://codereview.chromium.org/14807003/diff/33001/Source/core/rendering/Ren...
> Source/core/rendering/RenderLayer.cpp:2600: if (!box)
> Only boxes can have scrollbars. Can't we just ASSERT(box) here? If not, I
would
> rather return 0 as that's what scrollOrigin is in this case.

If that's the case, should we also return 0 for minimumScrollPosition?
Anyway, I plan to remove scrollOrigin all at once in a future patch.
Please refer to the same design doc: http://goo.gl/0TbOv

>
https://codereview.chromium.org/14807003/diff/33001/Source/core/rendering/Ren...
> File Source/core/rendering/RenderLayerBacking.cpp (right):
> 
>
https://codereview.chromium.org/14807003/diff/33001/Source/core/rendering/Ren...
> Source/core/rendering/RenderLayerBacking.cpp:681: if
> (style->shouldPlaceBlockDirectionScrollbarOnLogicalLeft())
> This function is a misnomer as it isn't really logical.

Well this flags is only enabled on horizontal blocks. Still kind of true. :>
Joking aside, I think we should copy IE's behavior to always place scrollbars to
the overflow direction. For example, put horizontal scrollbar on the top if
writing mode is horizontal-bt. I believe it will be less error-prone since the
logical (start, begin) will be always (0, 0).

[commit-bot: I haz the power, 2013-05-10 01:29:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/14807003/33001

[commit-bot: I haz the power, 2013-05-10 01:29:21]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 1659.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
888b4b59e8951570e4c70a89893913c7f59c75c5..1a37fd5603ed75e1d6356ee549d68b81043d6951
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1659,3 +1659,7 @@ crbug.com/238858 [ Win7 Debug ]
virtual/gpu/fast/hidpi/video-controls-in-hidpi.h
 crbug.com/239077 fast/canvas/webgl/index-validation-with-resized-buffer.html  [
Crash ]
 
 crbug.com/238813 [ Mac Debug ] inspector/debugger/pause-in-internal-script.html
[ Pass Crash ]
+
+# Transient. PNGs need to be uploaded separately.
+crbug.com/232063
virtual/gpu/compositedscrolling/scrollbars/scrollbars-on-positioned-content.html
[ ImageOnlyFailure ]
+crbug.com/232063
virtual/gpu/compositedscrolling/scrollbars/custom-scrollbar-with-incomplete-style.html
[ ImageOnlyFailure ]

[commit-bot: I haz the power, 2013-05-10 01:31:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/14807003/46001

[commit-bot: I haz the power, 2013-05-10 07:50:01]

Retried try job too often on mac_layout for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[commit-bot: I haz the power, 2013-05-10 08:14:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/14807003/46001

[commit-bot: I haz the power, 2013-05-10 13:43:33]

Retried try job too often on win_layout for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-05-10 19:18:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/14807003/46001

[commit-bot: I haz the power, 2013-05-10 19:19:36]

Change committed as 150124