Use float for scroll offset.

Use float for scroll offset.

Previously, scroll offsets were stored in a mix of float, double,
and int.

BUG=470718
R=skobes@chromium.org,bokan@chromium.org,aelias@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/80e6cf8d1fa064239b919bd111dc9304209dc9d3
Cr-Commit-Position: refs/heads/master@{#423772}

[szager1, 2016-10-01 17:58:20]

Description was changed from

==========
Use float for scroll offset.

Previously, scroll offsets were stored in a mix of float, double,
and int.

BUG=470718
R=skobes@chromium.org,bokan@chromium.org,aelias@chromium.org
==========

to

==========
Use float for scroll offset.

Previously, scroll offsets were stored in a mix of float, double,
and int.

BUG=470718
R=skobes@chromium.org,bokan@chromium.org,aelias@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[szager1, 2016-10-01 17:58:21]

Description was changed from

==========
Use float for scroll offset.

Previously, scroll offsets were stored in a mix of float, double,
and int.

BUG=470718
R=skobes@chromium.org,bokan@chromium.org,aelias@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Use float for scroll offset.

Previously, scroll offsets were stored in a mix of float, double,
and int.

BUG=470718
R=skobes@chromium.org,bokan@chromium.org,aelias@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[szager1, 2016-10-01 18:00:18]

The CQ bit was checked by szager@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-01 18:00:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[szager1, 2016-10-01 18:09:40]

Description was changed from

==========
Use float for scroll offset.

Previously, scroll offsets were stored in a mix of float, double,
and int.

BUG=470718
R=skobes@chromium.org,bokan@chromium.org,aelias@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Use float for scroll offset.

Previously, scroll offsets were stored in a mix of float, double,
and int.

BUG=470718
R=skobes@chromium.org,bokan@chromium.org,aelias@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[szager1, 2016-10-01 18:09:40]

szager@chromium.org changed reviewers:
+ danakj@chromium.org, pdr@chromium.org

[szager1, 2016-10-01 18:10:14]

+danakj for ui/gfx
+pdr for WebKit/Source/platform, WebKit/Source/web, WebKit/public

[commit-bot: I haz the power, 2016-10-01 19:28:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-01 19:28:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bokan, 2016-10-02 19:47:51]

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/TreeScope.cpp (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/TreeScope.cpp:223:
pointInDocument.moveBy(FloatPoint(frameView->scrollOffset()));
use move() instead, it takes a FloatSize

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/MouseRelatedEvent.cpp (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/MouseRelatedEvent.cpp:77: LayoutPoint
scrollOffset;
Make this LayoutSize to avoid casts below.

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/DOMVisualViewport.h (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/DOMVisualViewport.h:60: float scrollLeft();
Perhaps we should have a ScrollUnit typedef?

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.cpp:3058:
rect.moveBy(LayoutPoint(FloatPoint(-scrollOffset())));
Use move() (here and the changes below)

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.cpp:3537: ScrollOffset oldOffset
= m_scrollOffset;
No need for the oldOffset var here, just use it inline below.

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:52:
ret.moveBy(LayoutPoint(FloatPoint((layoutViewport().scrollOffset()))));
ret.move()

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:219: //
ScrollableArea's offset may have the fractional part truncated off.
Please add a TODO to revisit this now that the offsets in ScrollableArea will be
floats.

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/VisualViewport.h (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/VisualViewport.h:97: FloatPoint location()
const { return FloatPoint(m_offset); }
This should return a ScrollOffset but if you want to leave it to a future patch
that's fine.

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/VisualViewport.h:202: float scrollLeft();
these should stay double since they're Web exposed APIs and JS numbers/WebIDL is
a double type.

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.h:550: void
scrollToPosition(const FloatPoint&,
Why FloatPoint instead of ScrollOffset?

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/HistoryItem.h (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/HistoryItem.h:118: ScrollOffset
m_scrollOffset;
Changing this makes me a bit nervous because, IIRC, this gets serialized to disk
and we have to remain compatible with old versions, at least for a while. Could
we just floor and keep this as an int? Probably not a huge deal if history
navigation is off by a fraction...

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:316: FloatPoint
absolutePosition() const {
Since scrollOrigin is a ScrollableArea concept, I think this method should be
declared there as well.

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:1185: // so we
need to use the ScrollableArea version. The FrameView method should go away soon
anyway.
Remove comment

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scroll/ScrollTypes.h (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollTypes.h:204: double
unusedScrollDeltaX,
Shouldn't we keep these as float?

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.h (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollableArea.h:198: virtual IntSize
scrollOffsetInt() const = 0;
Is there a reason this Position->Offset renaming is in this patch rather than
the other one? (It's fine if it was just easier to split the patches, just
making sure I'm understanding)

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/FindInPageCoordinates.cpp (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/FindInPageCoordinates.cpp:95:
normalizedRect.moveBy(
move()

https://codereview.chromium.org/2387883002/diff/1/ui/gfx/geometry/scroll_offs...
File ui/gfx/geometry/scroll_offset.h (right):

https://codereview.chromium.org/2387883002/diff/1/ui/gfx/geometry/scroll_offs...
ui/gfx/geometry/scroll_offset.h:75: float y_;
So did we decide that we don't care about the monstrously sized pages? Anyway,
nice to be aligning CC and Blink finally :)

[dmazzoni, 2016-10-03 03:46:38]

dmazzoni@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[dmazzoni, 2016-10-03 03:46:39]

lgtm for modules/accessibility

[danakj, 2016-10-03 21:24:41]

cc and ui/gfx LGTM

https://codereview.chromium.org/2387883002/diff/1/ui/gfx/geometry/scroll_offs...
File ui/gfx/geometry/scroll_offset.h (right):

https://codereview.chromium.org/2387883002/diff/1/ui/gfx/geometry/scroll_offs...
ui/gfx/geometry/scroll_offset.h:75: float y_;
On 2016/10/02 19:47:51, bokan wrote:
> So did we decide that we don't care about the monstrously sized pages? Anyway,
> nice to be aligning CC and Blink finally :)

See the bug for how we got here :)

[szager1, 2016-10-05 00:02:02]

The CQ bit was checked by szager@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 00:02:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 02:55:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-05 02:55:24]

Dry run: This issue passed the CQ dry run.

[szager1, 2016-10-05 07:42:23]

The CQ bit was checked by szager@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 07:42:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[szager1, 2016-10-05 07:43:36]

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/TreeScope.cpp (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/TreeScope.cpp:223:
pointInDocument.moveBy(FloatPoint(frameView->scrollOffset()));
On 2016/10/02 19:47:50, bokan wrote:
> use move() instead, it takes a FloatSize

Done.

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/MouseRelatedEvent.cpp (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/MouseRelatedEvent.cpp:77: LayoutPoint
scrollOffset;
On 2016/10/02 19:47:50, bokan wrote:
> Make this LayoutSize to avoid casts below.

Done.

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/DOMVisualViewport.h (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/DOMVisualViewport.h:60: float scrollLeft();
On 2016/10/02 19:47:50, bokan wrote:
> Perhaps we should have a ScrollUnit typedef?

I'll add the typedef, but I'm not going to try and replace all floats with
ScrollUnit in this patch.  This patch is big enough already.

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.cpp:3058:
rect.moveBy(LayoutPoint(FloatPoint(-scrollOffset())));
On 2016/10/02 19:47:50, bokan wrote:
> Use move() (here and the changes below)

Done.

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.cpp:3537: ScrollOffset oldOffset
= m_scrollOffset;
On 2016/10/02 19:47:50, bokan wrote:
> No need for the oldOffset var here, just use it inline below.

Done.

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:52:
ret.moveBy(LayoutPoint(FloatPoint((layoutViewport().scrollOffset()))));
On 2016/10/02 19:47:51, bokan wrote:
> ret.move()

Done.

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/RootFrameViewport.cpp:219: //
ScrollableArea's offset may have the fractional part truncated off.
On 2016/10/02 19:47:50, bokan wrote:
> Please add a TODO to revisit this now that the offsets in ScrollableArea will
be
> floats.

Done.

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/VisualViewport.h (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/VisualViewport.h:97: FloatPoint location()
const { return FloatPoint(m_offset); }
On 2016/10/02 19:47:51, bokan wrote:
> This should return a ScrollOffset but if you want to leave it to a future
patch
> that's fine.

No time like the present!

I got rid of this method, since it is redundant.

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/VisualViewport.h:202: float scrollLeft();
On 2016/10/02 19:47:51, bokan wrote:
> these should stay double since they're Web exposed APIs and JS numbers/WebIDL
is
> a double type.

Done.

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.h:550: void
scrollToPosition(const FloatPoint&,
On 2016/10/02 19:47:51, bokan wrote:
> Why FloatPoint instead of ScrollOffset?

The convention I'm using is:

'position' is a point in the coordinate system of the overflow rect.
'offset' is a distance from the scroll origin.

Using a Point type is intuitive for something called 'position', and it also
helps prevent (via static type checking) inadvertently using the wrong quantity
in code.

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/HistoryItem.h (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/HistoryItem.h:118: ScrollOffset
m_scrollOffset;
On 2016/10/02 19:47:51, bokan wrote:
> Changing this makes me a bit nervous because, IIRC, this gets serialized to
disk
> and we have to remain compatible with old versions, at least for a while.
Could
> we just floor and keep this as an int? Probably not a huge deal if history
> navigation is off by a fraction...

The offsets are truncated by WebHistoryItem::scrollOffset(), which (still)
returns a WebPoint.

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:316: FloatPoint
absolutePosition() const {
On 2016/10/02 19:47:51, bokan wrote:
> Since scrollOrigin is a ScrollableArea concept, I think this method should be
> declared there as well.

Done.

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:1185: // so we
need to use the ScrollableArea version. The FrameView method should go away soon
anyway.
On 2016/10/02 19:47:51, bokan wrote:
> Remove comment

Done.

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scroll/ScrollTypes.h (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollTypes.h:204: double
unusedScrollDeltaX,
On 2016/10/02 19:47:51, bokan wrote:
> Shouldn't we keep these as float?

Whoops, fixed.

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.h (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollableArea.h:198: virtual IntSize
scrollOffsetInt() const = 0;
On 2016/10/02 19:47:51, bokan wrote:
> Is there a reason this Position->Offset renaming is in this patch rather than
> the other one? (It's fine if it was just easier to split the patches, just
> making sure I'm understanding)

The other patch had functional changes, and it was too confusing to combine
renames and functional changes in the same patch.

This patch enforces the convention that 'offset' is a distance from the scroll
origin and has type ScrollOffset; and 'position' is a point in the coordinate
system of the overflow rect and has type FloatPoint.  I think it's clearer to do
the renaming and type changes in a single patch, to demonstrate the new
convention.

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/FindInPageCoordinates.cpp (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/FindInPageCoordinates.cpp:95:
normalizedRect.moveBy(
On 2016/10/02 19:47:51, bokan wrote:
> move()

Done.

[commit-bot: I haz the power, 2016-10-05 10:53:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-05 10:54:00]

Dry run: This issue passed the CQ dry run.

[skobes, 2016-10-05 21:19:28]

https://codereview.chromium.org/2387883002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollTypes.h (right):

https://codereview.chromium.org/2387883002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollTypes.h:34: typedef float
ScrollUnit;
I'm not sure we get much value from this.  Even if we convert everything now I
think we will accumulate an arbitrary mix of ScrollUnit and bare float over
time.

https://codereview.chromium.org/2387883002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollTypes.h:35: typedef FloatSize
ScrollOffset;
Let's add a comment here about what ScrollOffset means.  (Can be a pointer to
the README.)

https://codereview.chromium.org/2387883002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.h (right):

https://codereview.chromium.org/2387883002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollableArea.h:81: virtual void
scrollBy(const ScrollOffset&,
This is technically a scroll delta, not a scroll "offset"... maybe we should use
a different type or just a FloatSize?

https://codereview.chromium.org/2387883002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollableArea.h:216: FloatPoint
absolutePosition() const {
Suggest calling this "scrollPosition" or "absoluteScrollPosition".

https://codereview.chromium.org/2387883002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebLayer.h (right):

https://codereview.chromium.org/2387883002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebLayer.h:162: virtual void
setScrollOffset(WebFloatSize) = 0;
Shouldn't these be "position"?  WebLayer should use blink terminology, not cc
terminology.

https://codereview.chromium.org/2387883002/diff/60001/ui/gfx/geometry/scroll_...
File ui/gfx/geometry/scroll_offset.h (right):

https://codereview.chromium.org/2387883002/diff/60001/ui/gfx/geometry/scroll_...
ui/gfx/geometry/scroll_offset.h:17: class GFX_EXPORT ScrollOffset {
Do we eventually want to switch cc from "offset" -> "position"?  Maybe add a
TODO here?

[szager1, 2016-10-06 00:11:57]

PTAL, I updated README.md and added some pictures to the docs/ director.

https://codereview.chromium.org/2387883002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollTypes.h (right):

https://codereview.chromium.org/2387883002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollTypes.h:34: typedef float
ScrollUnit;
On 2016/10/05 21:19:28, skobes wrote:
> I'm not sure we get much value from this.  Even if we convert everything now I
> think we will accumulate an arbitrary mix of ScrollUnit and bare float over
> time.

Yeah, I tend to agree.  I'll back this bit out and leave it for a follow-up that
may never happen.

https://codereview.chromium.org/2387883002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollTypes.h:35: typedef FloatSize
ScrollOffset;
On 2016/10/05 21:19:28, skobes wrote:
> Let's add a comment here about what ScrollOffset means.  (Can be a pointer to
> the README.)

Done.

https://codereview.chromium.org/2387883002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.h (right):

https://codereview.chromium.org/2387883002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollableArea.h:81: virtual void
scrollBy(const ScrollOffset&,
On 2016/10/05 21:19:28, skobes wrote:
> This is technically a scroll delta, not a scroll "offset"... maybe we should
use
> a different type or just a FloatSize?

In my Mental Model™, a delta and an offset are the same thing: they are both a
measure of displacement.  IMO, using the variable name "delta" where appropriate
is enough.

WDYT?

https://codereview.chromium.org/2387883002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollableArea.h:216: FloatPoint
absolutePosition() const {
On 2016/10/05 21:19:28, skobes wrote:
> Suggest calling this "scrollPosition" or "absoluteScrollPosition".

Switched to scrollPosition, and moved it closer to the declaration of
scrollOffset(), with a comment describing the difference.

https://codereview.chromium.org/2387883002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebLayer.h (right):

https://codereview.chromium.org/2387883002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebLayer.h:162: virtual void
setScrollOffset(WebFloatSize) = 0;
On 2016/10/05 21:19:28, skobes wrote:
> Shouldn't these be "position"?  WebLayer should use blink terminology, not cc
> terminology.

Good point, fixed.

https://codereview.chromium.org/2387883002/diff/60001/ui/gfx/geometry/scroll_...
File ui/gfx/geometry/scroll_offset.h (right):

https://codereview.chromium.org/2387883002/diff/60001/ui/gfx/geometry/scroll_...
ui/gfx/geometry/scroll_offset.h:17: class GFX_EXPORT ScrollOffset {
On 2016/10/05 21:19:28, skobes wrote:
> Do we eventually want to switch cc from "offset" -> "position"?  Maybe add a
> TODO here?

I talked to some blink paint and cc people about this, and everyone seemed OK
with the idea that cc and blink have different meanings for 'offset'.  I'll add
a TODO, but I don't think it's a priority.

[skobes, 2016-10-06 20:06:54]

https://codereview.chromium.org/2387883002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.h (right):

https://codereview.chromium.org/2387883002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollableArea.h:81: virtual void
scrollBy(const ScrollOffset&,
On 2016/10/06 00:11:57, szager1 wrote:
> On 2016/10/05 21:19:28, skobes wrote:
> > This is technically a scroll delta, not a scroll "offset"... maybe we should
> use
> > a different type or just a FloatSize?
> 
> In my Mental Model™, a delta and an offset are the same thing: they are both a
> measure of displacement.  IMO, using the variable name "delta" where
appropriate
> is enough.
> 
> WDYT?

ok

https://codereview.chromium.org/2387883002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/README.md (right):

https://codereview.chromium.org/2387883002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/README.md:43: Unfortunately, the geometric
abstraction of origin, offset, and position breaks down slighlty
"slightly"

https://codereview.chromium.org/2387883002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/README.md:75: _______________________
I still can't wrap my head around this picture.

Why does the overflow rect overlap the right-side scrollbar?  If this is the
start-of-flow position the right edge of the content area should be visible,
with the scrollbar extruding to the right.  Then the content rect would be
aligned with the scroll origin just like it is in the left-side scrollbar
diagram.

[szager1, 2016-10-06 21:07:03]

https://codereview.chromium.org/2387883002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/README.md (right):

https://codereview.chromium.org/2387883002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/README.md:43: Unfortunately, the geometric
abstraction of origin, offset, and position breaks down slighlty
On 2016/10/06 20:06:53, skobes wrote:
> "slightly"

Done.

https://codereview.chromium.org/2387883002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/README.md:75: _______________________
On 2016/10/06 20:06:53, skobes wrote:
> I still can't wrap my head around this picture.
> 
> Why does the overflow rect overlap the right-side scrollbar?  If this is the
> start-of-flow position the right edge of the content area should be visible,
> with the scrollbar extruding to the right.  Then the content rect would be
> aligned with the scroll origin just like it is in the left-side scrollbar
> diagram.

The overflow rect is determined independently of whether the container has a
vertical scrollbar.  So, its dimensions are the same whether or not there's a
vertical scrollbar.  This is why we need
LayoutBox::originAdjustmentForScrollbars.

The downside is that the mental model breaks down: the overflow rect extends to
the 15 pixels underneath the vertical scrollbar.  The upside is that scrollbar
existence (especially overflow:auto scrollbar is existence) is volatile, but we
avoid ever mucking with the location of the overflow rect once it has been
established during layout.

[szager1, 2016-10-06 22:00:14]

After some offline discussion with skobes@, I modified the README.md to reflect
reality.

PTAL

[skobes, 2016-10-06 22:10:40]

lgtm :)

[szager1, 2016-10-06 22:11:25]

The CQ bit was checked by szager@chromium.org

[szager1, 2016-10-06 22:11:26]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2387883002/#ps100001
(title: "Fix README.md")

[commit-bot: I haz the power, 2016-10-06 22:12:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 22:25:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-06 22:25:51]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[szager1, 2016-10-06 22:31:26]

pdr@, I'm waiting for your OWNERS powers

[bokan, 2016-10-06 22:32:17]

lgtm (but you still need pdr for platform)

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.h:550: void
scrollToPosition(const FloatPoint&,
On 2016/10/05 07:43:36, szager1 wrote:
> On 2016/10/02 19:47:51, bokan wrote:
> > Why FloatPoint instead of ScrollOffset?
> 
> The convention I'm using is:
> 
> 'position' is a point in the coordinate system of the overflow rect.
> 'offset' is a distance from the scroll origin.
> 
> Using a Point type is intuitive for something called 'position', and it also
> helps prevent (via static type checking) inadvertently using the wrong
quantity
> in code.

Acknowledged.

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/HistoryItem.h (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/HistoryItem.h:118: ScrollOffset
m_scrollOffset;
On 2016/10/05 07:43:36, szager1 wrote:
> On 2016/10/02 19:47:51, bokan wrote:
> > Changing this makes me a bit nervous because, IIRC, this gets serialized to
> disk
> > and we have to remain compatible with old versions, at least for a while.
> Could
> > we just floor and keep this as an int? Probably not a huge deal if history
> > navigation is off by a fraction...
> 
> The offsets are truncated by WebHistoryItem::scrollOffset(), which (still)
> returns a WebPoint.

Acknowledged.

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scroll/ScrollableArea.h (right):

https://codereview.chromium.org/2387883002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollableArea.h:198: virtual IntSize
scrollOffsetInt() const = 0;
On 2016/10/05 07:43:36, szager1 wrote:
> On 2016/10/02 19:47:51, bokan wrote:
> > Is there a reason this Position->Offset renaming is in this patch rather
than
> > the other one? (It's fine if it was just easier to split the patches, just
> > making sure I'm understanding)
> 
> The other patch had functional changes, and it was too confusing to combine
> renames and functional changes in the same patch.
> 
> This patch enforces the convention that 'offset' is a distance from the scroll
> origin and has type ScrollOffset; and 'position' is a point in the coordinate
> system of the overflow rect and has type FloatPoint.  I think it's clearer to
do
> the renaming and type changes in a single patch, to demonstrate the new
> convention.

Acknowledged.

[pdr., 2016-10-06 22:37:24]

On 2016/10/06 at 22:31:26, szager wrote:
> pdr@, I'm waiting for your OWNERS powers

I'd prefer if those docs were .svg, but lets roll with .png for now.

[pdr., 2016-10-06 22:37:37]

LGTM

[szager1, 2016-10-07 01:12:56]

The CQ bit was checked by szager@chromium.org

[commit-bot: I haz the power, 2016-10-07 01:13:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 01:22:51]

Description was changed from

==========
Use float for scroll offset.

Previously, scroll offsets were stored in a mix of float, double,
and int.

BUG=470718
R=skobes@chromium.org,bokan@chromium.org,aelias@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Use float for scroll offset.

Previously, scroll offsets were stored in a mix of float, double,
and int.

BUG=470718
R=skobes@chromium.org,bokan@chromium.org,aelias@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-10-07 01:22:54]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-07 01:25:00]

Description was changed from

==========
Use float for scroll offset.

Previously, scroll offsets were stored in a mix of float, double,
and int.

BUG=470718
R=skobes@chromium.org,bokan@chromium.org,aelias@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Use float for scroll offset.

Previously, scroll offsets were stored in a mix of float, double,
and int.

BUG=470718
R=skobes@chromium.org,bokan@chromium.org,aelias@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/80e6cf8d1fa064239b919bd111dc9304209dc9d3
Cr-Commit-Position: refs/heads/master@{#423772}
==========

[commit-bot: I haz the power, 2016-10-07 01:25:02]

Patchset 6 (id:??) landed as
https://crrev.com/80e6cf8d1fa064239b919bd111dc9304209dc9d3
Cr-Commit-Position: refs/heads/master@{#423772}