Compute snap offsets (both repeat and element based)

Compute snap offsets according to CSS Scroll Snap Spec Level 1

Add logic to compute snap offsets (those defined on snap container and snap
areas) according to the published spec [1].

SnapCoordinator maintains the mapping between snap containers and the snap
areas. This mapping is stored in |LayoutBoxRareData| and updated on style
change, attach, and detach events via hooks in |LayoutBox::styleWillChange| ,
|LayoutBox::insertedIntoTree| and |LayoutBox::willBeDestoryed|.

This implementation is the naive approach as described in [2]. It does not
attempt to track which snap areas have moved in relation to their container to
minimize  snap offsets re-computation. It simply re-computes all snap points
every time they are needed.

At the moment the computed snap offsets are not used for anything except to pass
the tests. In future,  |SnapCoordinator| will compute them as part of the
document lifecycle and send necessary updates to the Compositor Worker instance
which is going to be responsible for snapping during scrolls.

[1] https://www.w3.org/TR/2015/WD-css-snappoints-1-20150326/
[2] https://docs.google.com/document/d/1BO0EgcMe3DTsO1uwM_psZeJ_Gz5OZR-oYrpJBKAJLoU/edit?pli=1

Committed: https://crrev.com/3194544827727439d99dc72bec19d59a6004db0d
Cr-Commit-Position: refs/heads/master@{#391562}
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq

[majidvp, 2015-07-07 19:06:09]

majidvp@chromium.org changed reviewers:
+ vollick@chromium.org

[majidvp, 2015-07-08 18:57:05]

vollick@ please take a look and let me know if the overall design of this makes
sense to you.

[Ian Vollick, 2015-07-08 19:16:51]

Seems pretty straightforward, though I'd like to be sure we have a consistent
story for both snap points and position sticky.

flackr, would this approach work for you, too?

[flackr, 2015-07-21 17:38:46]

On 2015/07/08 19:16:51, vollick (ooo until July 27) wrote:
> Seems pretty straightforward, though I'd like to be sure we have a consistent
> story for both snap points and position sticky.
> 
> flackr, would this approach work for you, too?

Yes, I have a similar approach for sticky up at
https://codereview.chromium.org/1248743002/. I'd prefer to move the integration
points (attach / detach / style change / bounds change) to observers in the
manner of the proposed LayoutObserver and StyleObserver though so that we don't
have to check for a bunch of different coordinators at each code location.

[majidvp, 2015-09-14 18:16:54]

On 2015/07/21 17:38:46, flackr wrote:
> On 2015/07/08 19:16:51, vollick (ooo until July 27) wrote:
> > Seems pretty straightforward, though I'd like to be sure we have a
consistent
> > story for both snap points and position sticky.
> > 
> > flackr, would this approach work for you, too?
> 
> Yes, I have a similar approach for sticky up at
> https://codereview.chromium.org/1248743002/. I'd prefer to move the
integration
> points (attach / detach / style change / bounds change) to observers in the
> manner of the proposed LayoutObserver and StyleObserver though so that we
don't
> have to check for a bunch of different coordinators at each code location.


FYI, here is a patch for a simple style observer based on Rob's initial work: 
https://codereview.chromium.org/1323243002/

However based on current on-going discussion with style experts it appears that
such
generic observer interfaces are not desired in style engine. Based on our
offline
discussions it does not seem appropriate to block snap-points implementation on 
having style/layout observer hooks. So I am going to ask for review on the
current
implementation as discussed.

[majidvp, 2015-09-14 18:24:32]

majidvp@chromium.org changed reviewers:
+ esprehn@chromium.org, ojan@chromium.org, timloh@chromium.org

[majidvp, 2015-09-14 18:24:33]

esprehn@, timloh@, ojan@: can one of you please take a look at this CL and
let me know if there is any major issues with the way I am using style changes
to trigger snap-points computation work.

[majidvp, 2015-09-21 12:42:40]

majidvp@chromium.org changed reviewers:
- ojan@chromium.org, timloh@chromium.org

[majidvp, 2015-09-21 12:43:50]

On 2015/09/14 18:24:33, majidvp wrote:
> esprehn@, timloh@, ojan@: can one of you please take a look at this CL and
> let me know if there is any major issues with the way I am using style changes
> to trigger snap-points computation work.

esprehn@: friendly ping.

[esprehn, 2015-09-24 17:37:41]

https://codereview.chromium.org/1188563005/diff/320001/Source/core/dom/Contai...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/1188563005/diff/320001/Source/core/dom/Contai...
Source/core/dom/ContainerNode.cpp:877: if (document().frame() &&
document().frame()->snapCoordinator())
ditto checking for frame() doesn't make sense, this is also not the right layer
for this. This should be in Element, not ContainerNode.

https://codereview.chromium.org/1188563005/diff/320001/Source/core/dom/Contai...
Source/core/dom/ContainerNode.cpp:886:
document().frame()->snapCoordinator()->detach(*this);
frame() checks here don't make sense, we must have a frame if we're inside
detach. I also don't like the generic "detach" callbacks out to code like this.
This code should read like:

if (isSnapContainer())
  document().frameView()->removeSnapContainer(*this);
if (isSnapElement())
  document().frameView()->removeSnapElement(*this);

this code should also be in Element, not ContainerNode.

https://codereview.chromium.org/1188563005/diff/320001/Source/core/frame/Loca...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1188563005/diff/320001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:858: m_snapCoordinator =
SnapCoordinator::create(*this);
This should be on FrameView.

https://codereview.chromium.org/1188563005/diff/320001/Source/core/layout/Lay...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1188563005/diff/320001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.cpp:1971: snapCoordinator->styleChanged(node(),
oldStyle);
This doesn't belong here, I think you want it in Element::recalcOwnStyle?

https://codereview.chromium.org/1188563005/diff/320001/Source/core/page/scrol...
File Source/core/page/scrolling/SnapCoordinator.cpp (right):

https://codereview.chromium.org/1188563005/diff/320001/Source/core/page/scrol...
Source/core/page/scrolling/SnapCoordinator.cpp:35: SnapElementMap::AddResult
addResult = m_elementMap.add(&container, nullptr);
This should just be in ElementRareData.

https://codereview.chromium.org/1188563005/diff/320001/Source/core/page/scrol...
Source/core/page/scrolling/SnapCoordinator.cpp:62:
m_elementMap.get(container)->remove(&element);
We don't make maps of Elements, this is what ElementRareData is for.

https://codereview.chromium.org/1188563005/diff/320001/Source/core/page/scrol...
Source/core/page/scrolling/SnapCoordinator.cpp:108: if (node->isContainerNode())
{
Style only exists for Text and Element, doing this for Text doesn't make any
sense, so everything that comes through here will be Element.

The signature of this function should be Element&

https://codereview.chromium.org/1188563005/diff/320001/Source/core/page/scrol...
Source/core/page/scrolling/SnapCoordinator.cpp:121: return node.computedStyle()
&& node.computedStyle()->scrollSnapType() != ScrollSnapTypeNone;
computedStyle contains branches, we don't generally call it like this. Save it
on the stack. This will also always be an Element. None of this code needs to
operate on ContainerNode

https://codereview.chromium.org/1188563005/diff/320001/Source/core/page/scrol...
Source/core/page/scrolling/SnapCoordinator.cpp:182: Vector<double>
SnapCoordinator::snapOffsets(const ContainerNode& snapContainer,
ScrollbarOrientation orientation)
Element, no ContainerNodes

https://codereview.chromium.org/1188563005/diff/320001/Source/core/page/scrol...
Source/core/page/scrolling/SnapCoordinator.cpp:188: //
ASSERT(snapPoints.hasRepeat || snapPoints.usesElements);
Please remove, we don't check in commented out code.

https://codereview.chromium.org/1188563005/diff/320001/Source/core/page/scrol...
Source/core/page/scrolling/SnapCoordinator.cpp:212: for (size_t i = 0; i <
snapCoordinates.size(); ++i) {
range loop

https://codereview.chromium.org/1188563005/diff/320001/Source/core/page/scrol...
File Source/core/page/scrolling/SnapCoordinator.h (right):

https://codereview.chromium.org/1188563005/diff/320001/Source/core/page/scrol...
Source/core/page/scrolling/SnapCoordinator.h:37: void styleChanged(const Node*,
const ComputedStyle*);
I don't want general "styleChanged" hooks, please put the actual comparison in
Element.cpp's recalcOwnStyle

https://codereview.chromium.org/1188563005/diff/320001/Source/core/page/scrol...
Source/core/page/scrolling/SnapCoordinator.h:40: void attach(const
ContainerNode&);
Expose a real API, don't generically hook attach/detach. Also this should take
Element&, not ContainerNode.

https://codereview.chromium.org/1188563005/diff/320001/Source/core/page/scrol...
Source/core/page/scrolling/SnapCoordinator.h:63: RawPtrWillBeMember<LocalFrame>
m_frame;
You never actually use Frame, why does it have this member?

This thing belongs on FrameView or Document, not on the LocalFrame.

[majidvp, 2015-10-15 21:47:03]

Thanks esprehn@ for review. I have updated the patch and it should address all
comments. PTAL.

https://codereview.chromium.org/1188563005/diff/320001/Source/core/dom/Contai...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/1188563005/diff/320001/Source/core/dom/Contai...
Source/core/dom/ContainerNode.cpp:877: if (document().frame() &&
document().frame()->snapCoordinator())
On 2015/09/24 17:37:41, esprehn wrote:
> ditto checking for frame() doesn't make sense, this is also not the right
layer
> for this. This should be in Element, not ContainerNode.

Done.

https://codereview.chromium.org/1188563005/diff/320001/Source/core/dom/Contai...
Source/core/dom/ContainerNode.cpp:886:
document().frame()->snapCoordinator()->detach(*this);
On 2015/09/24 17:37:41, esprehn wrote:
> frame() checks here don't make sense, we must have a frame if we're inside
> detach. I also don't like the generic "detach" callbacks out to code like
this.
> This code should read like:
> 
> if (isSnapContainer())
>   document().frameView()->removeSnapContainer(*this);
> if (isSnapElement())
>   document().frameView()->removeSnapElement(*this);
> 
> this code should also be in Element, not ContainerNode.

Done.

https://codereview.chromium.org/1188563005/diff/320001/Source/core/frame/Loca...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1188563005/diff/320001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:858: m_snapCoordinator =
SnapCoordinator::create(*this);
On 2015/09/24 17:37:41, esprehn wrote:
> This should be on FrameView.

Following your other comment I moved it to document which I think is a more
appropriate owner. wdyt?

https://codereview.chromium.org/1188563005/diff/320001/Source/core/layout/Lay...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1188563005/diff/320001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.cpp:1971: snapCoordinator->styleChanged(node(),
oldStyle);
On 2015/09/24 17:37:41, esprehn wrote:
> This doesn't belong here, I think you want it in Element::recalcOwnStyle?

Done.

https://codereview.chromium.org/1188563005/diff/320001/Source/core/page/scrol...
File Source/core/page/scrolling/SnapCoordinator.cpp (right):

https://codereview.chromium.org/1188563005/diff/320001/Source/core/page/scrol...
Source/core/page/scrolling/SnapCoordinator.cpp:35: SnapElementMap::AddResult
addResult = m_elementMap.add(&container, nullptr);
On 2015/09/24 17:37:41, esprehn wrote:
> This should just be in ElementRareData.

It was suggested to me to use Map of Elements for now and once feature is mature
enough and design becomes stable then move them to ElementRareData. wdyt?

https://codereview.chromium.org/1188563005/diff/320001/Source/core/page/scrol...
Source/core/page/scrolling/SnapCoordinator.cpp:108: if (node->isContainerNode())
{
On 2015/09/24 17:37:41, esprehn wrote:
> Style only exists for Text and Element, doing this for Text doesn't make any
> sense, so everything that comes through here will be Element.
> 
> The signature of this function should be Element&

Done.

https://codereview.chromium.org/1188563005/diff/320001/Source/core/page/scrol...
Source/core/page/scrolling/SnapCoordinator.cpp:121: return node.computedStyle()
&& node.computedStyle()->scrollSnapType() != ScrollSnapTypeNone;
On 2015/09/24 17:37:41, esprehn wrote:
> computedStyle contains branches, we don't generally call it like this. Save it
> on the stack. This will also always be an Element. None of this code needs to
> operate on ContainerNode

Done.

https://codereview.chromium.org/1188563005/diff/320001/Source/core/page/scrol...
Source/core/page/scrolling/SnapCoordinator.cpp:182: Vector<double>
SnapCoordinator::snapOffsets(const ContainerNode& snapContainer,
ScrollbarOrientation orientation)
On 2015/09/24 17:37:41, esprehn wrote:
> Element, no ContainerNodes

Done.

https://codereview.chromium.org/1188563005/diff/320001/Source/core/page/scrol...
Source/core/page/scrolling/SnapCoordinator.cpp:188: //
ASSERT(snapPoints.hasRepeat || snapPoints.usesElements);
On 2015/09/24 17:37:41, esprehn wrote:
> Please remove, we don't check in commented out code.

Done.

https://codereview.chromium.org/1188563005/diff/320001/Source/core/page/scrol...
Source/core/page/scrolling/SnapCoordinator.cpp:212: for (size_t i = 0; i <
snapCoordinates.size(); ++i) {
On 2015/09/24 17:37:41, esprehn wrote:
> range loop

Done.

https://codereview.chromium.org/1188563005/diff/320001/Source/core/page/scrol...
File Source/core/page/scrolling/SnapCoordinator.h (right):

https://codereview.chromium.org/1188563005/diff/320001/Source/core/page/scrol...
Source/core/page/scrolling/SnapCoordinator.h:37: void styleChanged(const Node*,
const ComputedStyle*);
On 2015/09/24 17:37:41, esprehn wrote:
> I don't want general "styleChanged" hooks, please put the actual comparison in
> Element.cpp's recalcOwnStyle

Done. Now only exposing snap{Element,Container}DidChange.

https://codereview.chromium.org/1188563005/diff/320001/Source/core/page/scrol...
Source/core/page/scrolling/SnapCoordinator.h:40: void attach(const
ContainerNode&);
On 2015/09/24 17:37:41, esprehn wrote:
> Expose a real API, don't generically hook attach/detach. Also this should take
> Element&, not ContainerNode.

Done. Now only exposing snap{Element,Container}DidChange.

https://codereview.chromium.org/1188563005/diff/320001/Source/core/page/scrol...
Source/core/page/scrolling/SnapCoordinator.h:63: RawPtrWillBeMember<LocalFrame>
m_frame;
On 2015/09/24 17:37:41, esprehn wrote:
> You never actually use Frame, why does it have this member?
> 
> This thing belongs on FrameView or Document, not on the LocalFrame.

Done. Moved it to document.

[majidvp, 2015-10-20 17:52:41]

majidvp@chromium.org changed reviewers:
- vollick@chromium.org

[majidvp, 2015-10-20 17:52:42]

esprehn: ping

[esprehn, 2015-10-20 21:03:35]

I don't think you need most of this, you just want to store the snap data in
ElementRareData or LayoutBoxRareData, this seems to be a very style specific
concept, do you actually want it to be handled in LayoutBox::styleDidChange (and
LayoutBox::willBeDestroyed), and store it in LayoutBoxRareData?

I'm not sure which is better, but you shouldn't have a separate map either way.

https://codereview.chromium.org/1188563005/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1188563005/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:1852: Vector<LengthPoint>
newSnapCoordinate = newStyle ? newStyle->scrollSnapCoordinate() :  emptyVector;
This means making copies all the time, I think you want const refs to the
vectors

https://codereview.chromium.org/1188563005/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp (right):

https://codereview.chromium.org/1188563005/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:34: if
(addResult.isNewEntry)
This should just be in the ElementRareData, you don't need a separate map

https://codereview.chromium.org/1188563005/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:91: bool
isSnapContainer(const ComputedStyle* style)
static

https://codereview.chromium.org/1188563005/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:96: bool
isSnapElement(const ComputedStyle* style)
static

https://codereview.chromium.org/1188563005/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:105: Node*
curNode = curBox->node();
you're walking the dom here, this should just be
NodeRenderingTraversal::parent() loop, not enclosingBox

https://codereview.chromium.org/1188563005/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:125:
Vector<LengthPoint> snapCoordinates =
element.computedStyle()->scrollSnapCoordinate();
const ref? this is more copies

[majidvp, 2015-10-22 21:03:03]

> I don't think you need most of this, you just want to store the snap data in
> ElementRareData or LayoutBoxRareData, this seems to be a very style specific
> concept, do you actually want it to be handled in LayoutBox::styleDidChange
(and
> LayoutBox::willBeDestroyed), and store it in LayoutBoxRareData?
> 
> I'm not sure which is better, but you shouldn't have a separate map either
way.

I can move it to either ElementRareData or LayoutBoxRareData.

Given that we walk up the containing block chain (as opposed to DOM) to
compute the snap container this may be more related to layout tree than DOM
tree. So LayoutBox may be more appropriate. wdyt?

https://codereview.chromium.org/1188563005/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp (right):

https://codereview.chromium.org/1188563005/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:91: bool
isSnapContainer(const ComputedStyle* style)
On 2015/10/20 21:03:35, esprehn wrote:
> static

Acknowledged.

https://codereview.chromium.org/1188563005/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:96: bool
isSnapElement(const ComputedStyle* style)
On 2015/10/20 21:03:35, esprehn wrote:
> static

Acknowledged.

https://codereview.chromium.org/1188563005/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:105: Node*
curNode = curBox->node();
On 2015/10/20 21:03:35, esprehn wrote:
> you're walking the dom here, this should just be
> NodeRenderingTraversal::parent() loop, not enclosingBox

We are not walking the DOM tree. The aim here is to walk the containing
block chain because the snap point should be contributed to the nearest 
scroll container.

For example, an absolutely positioned element should not contribute snap
point to its first scrollable ancestor in DOM but should contribute to 
its ancestor in the containing block chain.

Note that this is very similar to how we compute scroll chain:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/1188563005/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:125:
Vector<LengthPoint> snapCoordinates =
element.computedStyle()->scrollSnapCoordinate();
On 2015/10/20 21:03:35, esprehn wrote:
> const ref? this is more copies

Acknowledged.

[majidvp, 2015-11-23 15:43:57]

esprehn@: I got rid of the maps and moved the storage of mapping to
LayoutBoxRareData. Addressed other feedback as well.

https://codereview.chromium.org/1188563005/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp (right):

https://codereview.chromium.org/1188563005/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:34: if
(addResult.isNewEntry)
On 2015/10/20 21:03:35, esprehn wrote:
> This should just be in the ElementRareData, you don't need a separate map

Moved to LayoutBoxRareData as discussed above.

https://codereview.chromium.org/1188563005/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:91: bool
isSnapContainer(const ComputedStyle* style)
On 2015/10/22 21:03:03, majidvp wrote:
> On 2015/10/20 21:03:35, esprehn wrote:
> > static
> 
> Acknowledged.

Done.

https://codereview.chromium.org/1188563005/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:96: bool
isSnapElement(const ComputedStyle* style)
On 2015/10/22 21:03:03, majidvp wrote:
> On 2015/10/20 21:03:35, esprehn wrote:
> > static
> 
> Acknowledged.

Done.

https://codereview.chromium.org/1188563005/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:125:
Vector<LengthPoint> snapCoordinates =
element.computedStyle()->scrollSnapCoordinate();
On 2015/10/22 21:03:03, majidvp wrote:
> On 2015/10/20 21:03:35, esprehn wrote:
> > const ref? this is more copies
> 
> Acknowledged.

Done.

[majidvp, 2015-11-23 15:50:14]

Description was changed from

==========
Computation of snap offset (both repeat and element based)

Snap coordinator maintains a map between scroll containers and the snap
elements. This map is updated when elements is attached, detached,  or its
style changes. This is done by adding hooks in |LayoutObject::styleDidChange|,
|ContainerNode::attach|, and |ContainerNode::detach|.


This implementation is the naive approach as described in [1]. It does not
attempt to track what snap points have moved in relation to their container
to minimize  snap offsets re-computation. It simply re-computes all snap points
every time they are needed.

At the moment SnapCoordinator is not hooked up to anything 
but I expect it to be triggered on compositor worker precommit phase.

[1]
https://docs.google.com/document/d/1BO0EgcMe3DTsO1uwM_psZeJ_Gz5OZR-oYrpJBKAJL...
==========

to

==========
Computation of snap offset (both repeat and element based)

Snap coordinator maintains the mapping between snap containers and the snap
areas. This mapping is stored in |LayoutBoxRareData| and updated on  style
change, attach, and detach events.

This is done by adding hooks in |LayoutBox::styleWillChange|
, |LayoutBox::insertedIntoTree| and |LayoutBox::willBeDestoryed|.


This implementation is the naive approach as described in [1]. It does not
attempt to track what snap points have moved in relation to their container to
minimize  snap offsets re-computation. It simply re-computes all snap points
every time they are needed.

At the moment SnapCoordinator is not hooked up to anything  but I expect it to
be triggered at the end of the commit to recompute and send necessary updates
to the Compositor Worker instance.


[1]
https://docs.google.com/document/d/1BO0EgcMe3DTsO1uwM_psZeJ_Gz5OZR-oYrpJBKAJL...
==========

[majidvp, 2015-11-25 20:43:01]

@esprehn: friendly ping

[esprehn, 2015-12-01 17:48:07]

On 2015/11/25 at 20:43:01, majidvp wrote:
> @esprehn: friendly ping

Looking now, sorry for the turkey day delay. :)

[esprehn, 2015-12-01 18:31:12]

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:272:
updateScrollSnapMappingAfterStyleChange(&newStyle, oldStyle);
newStyle should be a reference, don't do & here.

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:356: const
Vector<LengthPoint>& newSnapCoordinate = newStyle ?
newStyle->scrollSnapCoordinate() :  emptyVector;
newStyle can't be null, you don't need the null check.

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:374:
this->setSnapContainer(nullptr);
Why do you need this-> ?

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp (right):

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:49: 
Remove empty line here

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:55:
m_snapContainers.add(&snapContainer);
remove this map, it's not used outside debug code anyway, for debug just walk
the entire layout tree

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:73: LayoutBox*
curBox = snapArea.enclosingBox();
box, you're also mixing concepts here of containing blocks and boxes, does the
spec do that on purpose?

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:75: if
(isSnapContainer(curBox->style())) {
style() can't be null, you don't need the null check, or the isSnapContainer
method, just do ->scrollSnapType() != ScrollSnapTypeNone

this function can be:

LayoutBox* box = snapArea.enclosingBox();
while (box && box->style()->scrollSnapType() == ScrollSnapTypeNone)
  box = box->containingBlock();
return box;

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:112:
LayoutUnit clientSize = (orientation == HorizontalScrollbar) ?
snapContainer->clientWidth(): snapContainer->clientHeight();
missing space before :

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:113:
LayoutUnit scrollSize = (orientation == HorizontalScrollbar) ?
snapContainer->scrollWidth(): snapContainer->scrollHeight();
ditto

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:120: repeat =
std::max(repeat, LayoutUnit(1));
we often write:

std::max<LayoutUnit>(a, b)

instead of doing LayoutUnit on one side

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:151: for
(auto& container : m_snapContainers)
nothing uses this set except your debug code, why do you keep track of it?

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.h (right):

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.h:8: #include
"core/dom/Element.h"
can you forward declare more of this?

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp
(right):

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:48:
layoutAndStyle();
document.updateLayout()

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:53:
document.documentElement()->setInnerHTML(String::fromUTF8(htmlContent),
ASSERT_NO_EXCEPTION);
you don't need to call fromUTF8

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:58:
document.view()->updateAllLifecyclePhases();
document.updateLayout(), you don't need this helper either

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:63: return
document.getElementById("snap-container");
return a reference, your code assumes this is never null anyway

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:83:
snapContainer()->setAttribute("style", "scroll-snap-points-x: repeat(20%);
scroll-snap-points-y: repeat(400px);", exceptionState);
styleAttr, you don't need the exceptionState, or you can write:

setInlineStyleProperty(CSSPropertyScrollSnapPointsX, 20,
CSSPrimitiveValue::UnitType::Percentage, false)

but that's less webby, and more internal api

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:102:
snapContainer()->setAttribute("style", "scroll-snap-points-x: none;
scroll-snap-points-y: none;", exceptionState);
styleAttr

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:224:
snapElement->parentNode()->removeChild(snapElement, exceptionState);
snapElement->remove()

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:232:
container->setInnerHTML(String::fromUTF8("<div style='scroll-snap-coordinate:
20px 22px;'><div style='width:2000px; height:2000px;'></div></div>"),
ASSERT_NO_EXCEPTION);
no need to call fromUTF8, just do setInnerHTML("...")

[majidvp, 2015-12-03 17:54:17]

Thanks for review Elliot and happy thanks giving. :)

I have tried to address the feedback. The main change have to do with how the
scroll container is computed matching the latest spec language.

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:272:
updateScrollSnapMappingAfterStyleChange(&newStyle, oldStyle);
On 2015/12/01 18:31:11, esprehn wrote:
> newStyle should be a reference, don't do & here.

Good point. I have now changed the code to make |addScrollSnapMapping|
and |clearScrollSnapMapping| more symmetrical and both using 
|updateScrollSnapMappingAfterStyleChange|. This is cleaner and 
addresses an small issue I noticed too.

To do this, I am using the fact that newStyle is a pointer and can be null.

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:356: const
Vector<LengthPoint>& newSnapCoordinate = newStyle ?
newStyle->scrollSnapCoordinate() :  emptyVector;
On 2015/12/01 18:31:11, esprehn wrote:
> newStyle can't be null, you don't need the null check.

See above.

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:374:
this->setSnapContainer(nullptr);
On 2015/12/01 18:31:11, esprehn wrote:
> Why do you need this-> ?

Done.

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp (right):

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:49: 
On 2015/12/01 18:31:11, esprehn wrote:
> Remove empty line here

Done.

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:55:
m_snapContainers.add(&snapContainer);
On 2015/12/01 18:31:11, esprehn wrote:
> remove this map, it's not used outside debug code anyway, for debug just walk
> the entire layout tree

At the moment it is for debug only but I am planning to use it in follow up
patches. In particular, after layout we need to traverse over all snap
containers and recompute their snap points if necessary and send updates to
CW. I suspect walking the whole layout tree to do this is going to be
expensive given that snap containers are going to be sparse which is why
I am keeping a set.

If it turns out there is a better way to do this, then I can get rid of it.

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:73: LayoutBox*
curBox = snapArea.enclosingBox();
On 2015/12/01 18:31:11, esprehn wrote:
> box, you're also mixing concepts here of containing blocks and boxes, does the
> spec do that on purpose?

The spec talks about "the nearest ancestor (on the element’s containing block
chain) scroll container".

I used enclosingBox() to make this scroll ancestor chain match the logic in
|EventHandler:: recomputeScrollChain| but I realized now that
snapArea.enclosingBox() is essentially a no-op here given that snapArea is a
box. Removing this.

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:75: if
(isSnapContainer(curBox->style())) {
On 2015/12/01 18:31:11, esprehn wrote:
> style() can't be null, you don't need the null check, or the isSnapContainer
> method, just do ->scrollSnapType() != ScrollSnapTypeNone
> 
> this function can be:
> 
> LayoutBox* box = snapArea.enclosingBox();
> while (box && box->style()->scrollSnapType() == ScrollSnapTypeNone)
>   box = box->containingBlock();
> return box;

I have modified the logic to match your recommendation and also the latest spec.
The snap elements are attributed to nearest scrolling ancestor regardless of its
scroll-snap-type value. The scroll-snap-type value is only considered when
computing the actual snap offsets.

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:112:
LayoutUnit clientSize = (orientation == HorizontalScrollbar) ?
snapContainer->clientWidth(): snapContainer->clientHeight();
On 2015/12/01 18:31:11, esprehn wrote:
> missing space before :

Done.

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:113:
LayoutUnit scrollSize = (orientation == HorizontalScrollbar) ?
snapContainer->scrollWidth(): snapContainer->scrollHeight();
On 2015/12/01 18:31:11, esprehn wrote:
> ditto

Done.

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:120: repeat =
std::max(repeat, LayoutUnit(1));
On 2015/12/01 18:31:11, esprehn wrote:
> we often write:
> 
> std::max<LayoutUnit>(a, b)
> 
> instead of doing LayoutUnit on one side

Updated to match expected style.

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:151: for
(auto& container : m_snapContainers)
On 2015/12/01 18:31:11, esprehn wrote:
> nothing uses this set except your debug code, why do you keep track of it?

Explained above that I expect to use this in upcoming patches.

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.h (right):

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.h:8: #include
"core/dom/Element.h"
On 2015/12/01 18:31:12, esprehn wrote:
> can you forward declare more of this?

Done.

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp
(right):

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:48:
layoutAndStyle();
On 2015/12/01 18:31:12, esprehn wrote:
> document.updateLayout()

Done.

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:53:
document.documentElement()->setInnerHTML(String::fromUTF8(htmlContent),
ASSERT_NO_EXCEPTION);
On 2015/12/01 18:31:12, esprehn wrote:
> you don't need to call fromUTF8

Done.

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:58:
document.view()->updateAllLifecyclePhases();
On 2015/12/01 18:31:12, esprehn wrote:
> document.updateLayout(), you don't need this helper either

Done.

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:63: return
document.getElementById("snap-container");
On 2015/12/01 18:31:12, esprehn wrote:
> return a reference, your code assumes this is never null anyway

Done.

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:83:
snapContainer()->setAttribute("style", "scroll-snap-points-x: repeat(20%);
scroll-snap-points-y: repeat(400px);", exceptionState);
On 2015/12/01 18:31:12, esprehn wrote:
> styleAttr, you don't need the exceptionState, or you can write:
> 
> setInlineStyleProperty(CSSPropertyScrollSnapPointsX, 20,
> CSSPrimitiveValue::UnitType::Percentage, false)
> 
> but that's less webby, and more internal api

Didn't know about styleAttr. Neat. Done.

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:102:
snapContainer()->setAttribute("style", "scroll-snap-points-x: none;
scroll-snap-points-y: none;", exceptionState);
On 2015/12/01 18:31:12, esprehn wrote:
> styleAttr

Done.

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:224:
snapElement->parentNode()->removeChild(snapElement, exceptionState);
On 2015/12/01 18:31:12, esprehn wrote:
> snapElement->remove()

Done.

https://codereview.chromium.org/1188563005/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:232:
container->setInnerHTML(String::fromUTF8("<div style='scroll-snap-coordinate:
20px 22px;'><div style='width:2000px; height:2000px;'></div></div>"),
ASSERT_NO_EXCEPTION);
On 2015/12/01 18:31:12, esprehn wrote:
> no need to call fromUTF8, just do setInnerHTML("...")

Done.

[majidvp, 2015-12-08 20:09:13]

esprehn@: Did these fixes address your review feedback?

[majidvp, 2015-12-14 22:19:43]

On 2015/12/08 20:09:13, majidvp wrote:
> esprehn@: Did these fixes address your review feedback?

esprehn@: ping

[esprehn, 2015-12-16 02:06:10]

Your Oilpan code isn't right for LayoutBox.

https://codereview.chromium.org/1188563005/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp (right):

https://codereview.chromium.org/1188563005/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:34:
snapArea.setSnapContainer(nullptr);
early return.

if (snapCoordinates.isEmpty()) {
    snapArea.setSnapContainer(nullptr);
    return;
}

if (LayoutBox* snapContainer = findSnapContainer(snapArea)) {
    snapArea.setSnapContainer(snapContainer);
    return;
}

// TODO(...)

then you don't need to indent the entire function or nest the cases.

https://codereview.chromium.org/1188563005/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:36: LayoutBox*
snapContainer = findSnapContainer(snapArea);
this should have been in the if () so you could do else if () { } else { }

https://codereview.chromium.org/1188563005/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:67: LayoutBox*
SnapCoordinator::findSnapContainer(const LayoutBox& snapArea)
this doesn't access any state inside this coordinator, move it to be a static
method in the .cpp

https://codereview.chromium.org/1188563005/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.h (right):

https://codereview.chromium.org/1188563005/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.h:44:
WillBeHeapHashSet<RawPtrWillBeWeakMember<const LayoutBox>> m_snapContainers;
this code doesn't make sense, LayoutObject isn't garbage collected, you can't
have a weak reference to it. This is just a HashSet<LayoutBox*> without Oilpan,
and that's what you need to write here, with Oilpan enabled this should just
fail to compile.

[majidvp, 2015-12-17 19:22:34]

Description was changed from

==========
Computation of snap offset (both repeat and element based)

Snap coordinator maintains the mapping between snap containers and the snap
areas. This mapping is stored in |LayoutBoxRareData| and updated on  style
change, attach, and detach events.

This is done by adding hooks in |LayoutBox::styleWillChange|
, |LayoutBox::insertedIntoTree| and |LayoutBox::willBeDestoryed|.


This implementation is the naive approach as described in [1]. It does not
attempt to track what snap points have moved in relation to their container to
minimize  snap offsets re-computation. It simply re-computes all snap points
every time they are needed.

At the moment SnapCoordinator is not hooked up to anything  but I expect it to
be triggered at the end of the commit to recompute and send necessary updates
to the Compositor Worker instance.


[1]
https://docs.google.com/document/d/1BO0EgcMe3DTsO1uwM_psZeJ_Gz5OZR-oYrpJBKAJL...
==========

to

==========
Computation of snap offset (both repeat and element based)

Snap coordinator maintains the mapping between snap containers and the snap
areas. This mapping is stored in |LayoutBoxRareData| and updated on  style
change, attach, and detach events.

This is done by adding hooks in |LayoutBox::styleWillChange|
, |LayoutBox::insertedIntoTree| and |LayoutBox::willBeDestoryed|.


This implementation is the naive approach as described in [1]. It does not
attempt to track what snap points have moved in relation to their container to
minimize  snap offsets re-computation. It simply re-computes all snap points
every time they are needed.

At the moment SnapCoordinator is not hooked up to anything  but I expect it to
be triggered at the end of the commit to recompute and send necessary updates
to the Compositor Worker instance.


[1]
https://docs.google.com/document/d/1BO0EgcMe3DTsO1uwM_psZeJ_Gz5OZR-oYrpJBKAJL...

BUG=311234
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_oilpan_rel
==========

[majidvp, 2015-12-17 23:51:21]

https://codereview.chromium.org/1188563005/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp (right):

https://codereview.chromium.org/1188563005/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:34:
snapArea.setSnapContainer(nullptr);
On 2015/12/16 02:06:10, esprehn wrote:
> early return.
> 
> if (snapCoordinates.isEmpty()) {
>     snapArea.setSnapContainer(nullptr);
>     return;
> }
> 
> if (LayoutBox* snapContainer = findSnapContainer(snapArea)) {
>     snapArea.setSnapContainer(snapContainer);
>     return;
> }
> 
> // TODO(...)
> 
> then you don't need to indent the entire function or nest the cases.

Done.

https://codereview.chromium.org/1188563005/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:36: LayoutBox*
snapContainer = findSnapContainer(snapArea);
On 2015/12/16 02:06:10, esprehn wrote:
> this should have been in the if () so you could do else if () { } else { }

Done.

https://codereview.chromium.org/1188563005/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:67: LayoutBox*
SnapCoordinator::findSnapContainer(const LayoutBox& snapArea)
On 2015/12/16 02:06:10, esprehn wrote:
> this doesn't access any state inside this coordinator, move it to be a static
> method in the .cpp

Done.

https://codereview.chromium.org/1188563005/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.h (right):

https://codereview.chromium.org/1188563005/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.h:44:
WillBeHeapHashSet<RawPtrWillBeWeakMember<const LayoutBox>> m_snapContainers;
On 2015/12/16 02:06:10, esprehn wrote:
> this code doesn't make sense, LayoutObject isn't garbage collected, you can't
> have a weak reference to it. This is just a HashSet<LayoutBox*> without
Oilpan,
> and that's what you need to write here, with Oilpan enabled this should just
> fail to compile.

Fixed. I missed the fact that LayoutBox is not yet garbage collected. It was
failing
with oilpan as you predicted! I fixed here and also in LayoutBox where I was
using
a similar HashSet for snapAreas.

[majidvp, 2016-01-04 19:08:37]

esprehn: ping

[majidvp, 2016-01-20 21:15:59]

On 2016/01/04 19:08:37, majidvp wrote:
> esprehn: ping

esprehn@: PTAL.

[Ian Vollick, 2016-03-02 16:36:12]

On 2016/01/20 21:15:59, majidvp wrote:
> On 2016/01/04 19:08:37, majidvp wrote:
> > esprehn: ping
> 
> esprehn@: PTAL.

It looks like Elliott is busy. Let's set another reviewer for now. If he has
concerns or wants to pick this up again, we can easily switch back.

[majidvp, 2016-03-02 16:46:28]

Description was changed from

==========
Computation of snap offset (both repeat and element based)

Snap coordinator maintains the mapping between snap containers and the snap
areas. This mapping is stored in |LayoutBoxRareData| and updated on  style
change, attach, and detach events.

This is done by adding hooks in |LayoutBox::styleWillChange|
, |LayoutBox::insertedIntoTree| and |LayoutBox::willBeDestoryed|.


This implementation is the naive approach as described in [1]. It does not
attempt to track what snap points have moved in relation to their container to
minimize  snap offsets re-computation. It simply re-computes all snap points
every time they are needed.

At the moment SnapCoordinator is not hooked up to anything  but I expect it to
be triggered at the end of the commit to recompute and send necessary updates
to the Compositor Worker instance.


[1]
https://docs.google.com/document/d/1BO0EgcMe3DTsO1uwM_psZeJ_Gz5OZR-oYrpJBKAJL...

BUG=311234
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_oilpan_rel
==========

to

==========
Computation of snap offset (both repeat and element based)

Snap coordinator maintains the mapping between snap containers and the snap
areas. This mapping is stored in |LayoutBoxRareData| and updated on  style
change, attach, and detach events.

This is done by adding hooks in |LayoutBox::styleWillChange|
, |LayoutBox::insertedIntoTree| and |LayoutBox::willBeDestoryed|.


This implementation is the naive approach as described in [1]. It does not
attempt to track what snap points have moved in relation to their container to
minimize  snap offsets re-computation. It simply re-computes all snap points
every time they are needed.

At the moment SnapCoordinator is not hooked up to anything  but I expect it to
be triggered at the end of the commit to recompute and send necessary updates
to the Compositor Worker instance.


[1]
https://docs.google.com/document/d/1BO0EgcMe3DTsO1uwM_psZeJ_Gz5OZR-oYrpJBKAJL...

BUG=311234
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_oilpan_rel
==========

[majidvp, 2016-03-02 16:49:50]

majidvp@chromium.org changed reviewers:
+ eae@chromium.org

[majidvp, 2016-03-02 16:49:51]

eae@: can you please take a look at this patch.

P.S.: I am going to do a rebase to make sure everything is up to date but I
don't imagine it to make a lot changes.

[eae, 2016-03-02 19:23:11]

I'll try to get to this later today.

[eae, 2016-03-03 00:50:28]

eae@chromium.org changed reviewers:
+ skobes@chromium.org

[eae, 2016-03-03 00:50:30]

I'd like skobes to review the scrolling aspects of this change.

[skobes, 2016-03-07 03:42:27]

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:173: for (LayoutObject* curr
= parent(); curr && !curr-isLayoutView(); curr = curr->parent()) {
This change looks unintended.

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:387: ScrollSnapType
oldSnapType = oldStyle ? oldStyle->getScrollSnapType() :  ScrollSnapTypeNone;
You have an extra space after :

(here and throughout this function)

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:4718:
ensureRareData().m_snapContainer = container;
If container is null, we don't need m_rareData.  Perhaps this should be

if (m_rareData || container)
    m_rareData->m_snapContainer = container;

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp (right):

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:78: //
Translate local snap coordinates into snap container space
Since you are adjusting the output of localToAncestorPoint by the scroll offset,
clarify that this is actually translating into the coordinate space of the snap
container's scrolling contents.

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:94:
Vector<double> SnapCoordinator::snapOffsets(const Element& element,
ScrollbarOrientation orientation)
Where will this be called from?

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:108:
LayoutUnit clientSize = (orientation == HorizontalScrollbar) ?
snapContainer->clientWidth() : snapContainer->clientHeight();
I think this has some issues when the snap container is the viewport.

The scroll-snap-type style is set on the viewport-defining element, but the snap
container should probably be the LayoutView.  I would suggest propagating the
style to the document in Document::inheritHtmlAndBodyElementStyles, and changing
LayoutBox::updateScrollSnapMappingAfterStyleChange to treat the
viewport-defining element as if it were ScrollSnapTypeNone, similar to how
LayoutBlock::allowsOverflowClip suppresses the overflow clip.

You will also need to get clientSize and scrollSize from the FrameView when root
layer scrolling is disabled.

This is assuming you want to snap the layout viewport as opposed to the visual
(pinch-zoom) viewport?

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.h (right):

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.h:18: // Snap
Coordinator keeps track of snap containers and all of their associated
This comment should either describe what snap containers and snap elements are,
or link to something that does.

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.h:43:
HashSet<const LayoutBox*> m_snapContainers;
Why do we need this exactly?  Is it just for showSnapAreaMap?  If so, guard with
#ifndef NDEBUG.

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp
(right):

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:27: "   
::-webkit-scrollbar { display: none; }"
It would be good to also test with scrollbars present.

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:256:
"<body style='scroll-snap-type: mandatory; height:1000px; width:1000px;
overflow:scroll;'>"
Setting overflow:scroll on <body> probably masks some of the aforementioned
viewport issues.

[majidvp, 2016-05-03 17:23:36]

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:173: for (LayoutObject* curr
= parent(); curr && !curr-isLayoutView(); curr = curr->parent()) {
On 2016/03/07 03:42:27, skobes (OOO) wrote:
> This change looks unintended.

Ops! It compiles with no warning and does not trip any test :'(

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:387: ScrollSnapType
oldSnapType = oldStyle ? oldStyle->getScrollSnapType() :  ScrollSnapTypeNone;
On 2016/03/07 03:42:27, skobes (OOO) wrote:
> You have an extra space after :
> 
> (here and throughout this function)

Done.

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:4718:
ensureRareData().m_snapContainer = container;
On 2016/03/07 03:42:27, skobes (OOO) wrote:
> If container is null, we don't need m_rareData.  Perhaps this should be
> 
> if (m_rareData || container)
>     m_rareData->m_snapContainer = container;

Good point. Although we still want to clear existing container value. 
I update the logic to exit early in case that old and new are the same.
This also ensures that we never create m_rareDate unnecessarily.

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp (right):

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:78: //
Translate local snap coordinates into snap container space
On 2016/03/07 03:42:27, skobes (OOO) wrote:
> Since you are adjusting the output of localToAncestorPoint by the scroll
offset,
> clarify that this is actually translating into the coordinate space of the
snap
> container's scrolling contents.

Done.

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:94:
Vector<double> SnapCoordinator::snapOffsets(const Element& element,
ScrollbarOrientation orientation)
On 2016/03/07 03:42:27, skobes (OOO) wrote:
> Where will this be called from?

At the moment this is only used in tests. But plan is to use this to compute
snap destination coordinate and pass them to the compositor-worker instance
which is responsible to handle snapping for composited scrolling.

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.h (right):

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.h:18: // Snap
Coordinator keeps track of snap containers and all of their associated
On 2016/03/07 03:42:27, skobes (OOO) wrote:
> This comment should either describe what snap containers and snap elements
are,
> or link to something that does.

Done.

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.h:43:
HashSet<const LayoutBox*> m_snapContainers;
On 2016/03/07 03:42:27, skobes (OOO) wrote:
> Why do we need this exactly?  Is it just for showSnapAreaMap?  If so, guard
with
> #ifndef NDEBUG.
Here is the rseponse I gave to esprehn@ on this issue earlier.
If you prefer, I can move this to debug-only until it actually used.

On 2015/12/01 18:31:11, esprehn wrote:
> remove this map, it's not used outside debug code anyway, for debug just walk
> the entire layout tree

At the moment it is for debug only but I am planning to use it in follow up
patches. In particular, after layout we need to traverse over all snap
containers and recompute their snap points if necessary and send updates to
CW. I suspect walking the whole layout tree to do this is going to be
expensive given that snap containers are going to be sparse which is why
I am keeping a set.

If it turns out there is a better way to do this, then I can get rid of it.

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp
(right):

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:27: "   
::-webkit-scrollbar { display: none; }"
On 2016/03/07 03:42:27, skobes wrote:
> It would be good to also test with scrollbars present.

Add the scrollbars back. Their only affect is the change in
client size of snapport. So I had to take the scrollbar size
into account in calculations.

https://codereview.chromium.org/1188563005/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinatorTest.cpp:256:
"<body style='scroll-snap-type: mandatory; height:1000px; width:1000px;
overflow:scroll;'>"
On 2016/03/07 03:42:27, skobes (OOO) wrote:
> Setting overflow:scroll on <body> probably masks some of the aforementioned
> viewport issues.

Added two other tests where body, document are viewport defining and with 
body overflowing in addition to the viewport.

Also made the test parametric ensuring it is executed with and without 
root-layer-scrolls flag.

[majidvp, 2016-05-03 18:27:09]

@skobes: PTAL :)

[skobes, 2016-05-03 19:45:30]

lgtm w/ nit

https://codereview.chromium.org/1188563005/diff/580001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp (right):

https://codereview.chromium.org/1188563005/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:123:
WTFLogAlways("repeat: %f, scrollSize: %f, clientSize:%f", repeat.toFloat(),
scrollSize.toFloat(), clientSize.toFloat());
remove this

[majidvp, 2016-05-04 15:38:44]

Description was changed from

==========
Computation of snap offset (both repeat and element based)

Snap coordinator maintains the mapping between snap containers and the snap
areas. This mapping is stored in |LayoutBoxRareData| and updated on  style
change, attach, and detach events.

This is done by adding hooks in |LayoutBox::styleWillChange|
, |LayoutBox::insertedIntoTree| and |LayoutBox::willBeDestoryed|.


This implementation is the naive approach as described in [1]. It does not
attempt to track what snap points have moved in relation to their container to
minimize  snap offsets re-computation. It simply re-computes all snap points
every time they are needed.

At the moment SnapCoordinator is not hooked up to anything  but I expect it to
be triggered at the end of the commit to recompute and send necessary updates
to the Compositor Worker instance.


[1]
https://docs.google.com/document/d/1BO0EgcMe3DTsO1uwM_psZeJ_Gz5OZR-oYrpJBKAJL...

BUG=311234
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_oilpan_rel
==========

to

==========
Compute snap offsets according to CSS Scroll Snap Spec Level 1

Add logic to compute snap offsets (those defined on snap container and snap
areas) according to the published spec [1].

SnapCoordinator maintains the mapping between snap containers and the snap
areas. This mapping is stored in |LayoutBoxRareData| and updated on style
change, attach, and detach events via hooks in |LayoutBox::styleWillChange| ,
|LayoutBox::insertedIntoTree| and |LayoutBox::willBeDestoryed|.


This implementation is the naive approach as described in [2]. It does not
attempt to track which snap areas have moved in relation to their container to
minimize  snap offsets re-computation. It simply re-computes all snap points
every time they are needed.

At the moment the computed snap offsets are not used for anything except to pass
the tests. In future,  |SnapCoordinator| will compute them as part of the
document lifecycle and send necessary updates to the Compositor Worker instance
which is going to be responsible for snapping during scrolls.


[1] https://www.w3.org/TR/2015/WD-css-snappoints-1-20150326/
[2]
https://docs.google.com/document/d/1BO0EgcMe3DTsO1uwM_psZeJ_Gz5OZR-oYrpJBKAJL...
==========

[majidvp, 2016-05-04 15:40:49]

Thanks skobes@

eae@: PTAL

https://codereview.chromium.org/1188563005/diff/580001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp (right):

https://codereview.chromium.org/1188563005/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/SnapCoordinator.cpp:123:
WTFLogAlways("repeat: %f, scrollSize: %f, clientSize:%f", repeat.toFloat(),
scrollSize.toFloat(), clientSize.toFloat());
On 2016/05/03 19:45:30, skobes wrote:
> remove this

Done.

[eae, 2016-05-04 15:51:36]

LGTM

[majidvp, 2016-05-04 15:52:31]

The CQ bit was checked by majidvp@chromium.org

[majidvp, 2016-05-04 15:52:31]

The patchset sent to the CQ was uploaded after l-g-t-m from skobes@chromium.org
Link to the patchset: https://codereview.chromium.org/1188563005/#ps600001
(title: "Remove missed logging")

[commit-bot: I haz the power, 2016-05-04 15:52:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1188563005/600001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1188563005/600001

[commit-bot: I haz the power, 2016-05-04 18:14:14]

Description was changed from

==========
Compute snap offsets according to CSS Scroll Snap Spec Level 1

Add logic to compute snap offsets (those defined on snap container and snap
areas) according to the published spec [1].

SnapCoordinator maintains the mapping between snap containers and the snap
areas. This mapping is stored in |LayoutBoxRareData| and updated on style
change, attach, and detach events via hooks in |LayoutBox::styleWillChange| ,
|LayoutBox::insertedIntoTree| and |LayoutBox::willBeDestoryed|.


This implementation is the naive approach as described in [2]. It does not
attempt to track which snap areas have moved in relation to their container to
minimize  snap offsets re-computation. It simply re-computes all snap points
every time they are needed.

At the moment the computed snap offsets are not used for anything except to pass
the tests. In future,  |SnapCoordinator| will compute them as part of the
document lifecycle and send necessary updates to the Compositor Worker instance
which is going to be responsible for snapping during scrolls.


[1] https://www.w3.org/TR/2015/WD-css-snappoints-1-20150326/
[2]
https://docs.google.com/document/d/1BO0EgcMe3DTsO1uwM_psZeJ_Gz5OZR-oYrpJBKAJL...
==========

to

==========
Compute snap offsets according to CSS Scroll Snap Spec Level 1

Add logic to compute snap offsets (those defined on snap container and snap
areas) according to the published spec [1].

SnapCoordinator maintains the mapping between snap containers and the snap
areas. This mapping is stored in |LayoutBoxRareData| and updated on style
change, attach, and detach events via hooks in |LayoutBox::styleWillChange| ,
|LayoutBox::insertedIntoTree| and |LayoutBox::willBeDestoryed|.


This implementation is the naive approach as described in [2]. It does not
attempt to track which snap areas have moved in relation to their container to
minimize  snap offsets re-computation. It simply re-computes all snap points
every time they are needed.

At the moment the computed snap offsets are not used for anything except to pass
the tests. In future,  |SnapCoordinator| will compute them as part of the
document lifecycle and send necessary updates to the Compositor Worker instance
which is going to be responsible for snapping during scrolls.


[1] https://www.w3.org/TR/2015/WD-css-snappoints-1-20150326/
[2]
https://docs.google.com/document/d/1BO0EgcMe3DTsO1uwM_psZeJ_Gz5OZR-oYrpJBKAJL...
==========

[commit-bot: I haz the power, 2016-05-04 18:14:15]

Committed patchset #31 (id:600001)

[commit-bot: I haz the power, 2016-05-04 18:15:33]

Description was changed from

==========
Compute snap offsets according to CSS Scroll Snap Spec Level 1

Add logic to compute snap offsets (those defined on snap container and snap
areas) according to the published spec [1].

SnapCoordinator maintains the mapping between snap containers and the snap
areas. This mapping is stored in |LayoutBoxRareData| and updated on style
change, attach, and detach events via hooks in |LayoutBox::styleWillChange| ,
|LayoutBox::insertedIntoTree| and |LayoutBox::willBeDestoryed|.


This implementation is the naive approach as described in [2]. It does not
attempt to track which snap areas have moved in relation to their container to
minimize  snap offsets re-computation. It simply re-computes all snap points
every time they are needed.

At the moment the computed snap offsets are not used for anything except to pass
the tests. In future,  |SnapCoordinator| will compute them as part of the
document lifecycle and send necessary updates to the Compositor Worker instance
which is going to be responsible for snapping during scrolls.


[1] https://www.w3.org/TR/2015/WD-css-snappoints-1-20150326/
[2]
https://docs.google.com/document/d/1BO0EgcMe3DTsO1uwM_psZeJ_Gz5OZR-oYrpJBKAJL...
==========

to

==========
Compute snap offsets according to CSS Scroll Snap Spec Level 1

Add logic to compute snap offsets (those defined on snap container and snap
areas) according to the published spec [1].

SnapCoordinator maintains the mapping between snap containers and the snap
areas. This mapping is stored in |LayoutBoxRareData| and updated on style
change, attach, and detach events via hooks in |LayoutBox::styleWillChange| ,
|LayoutBox::insertedIntoTree| and |LayoutBox::willBeDestoryed|.

This implementation is the naive approach as described in [2]. It does not
attempt to track which snap areas have moved in relation to their container to
minimize  snap offsets re-computation. It simply re-computes all snap points
every time they are needed.

At the moment the computed snap offsets are not used for anything except to pass
the tests. In future,  |SnapCoordinator| will compute them as part of the
document lifecycle and send necessary updates to the Compositor Worker instance
which is going to be responsible for snapping during scrolls.

[1] https://www.w3.org/TR/2015/WD-css-snappoints-1-20150326/
[2]
https://docs.google.com/document/d/1BO0EgcMe3DTsO1uwM_psZeJ_Gz5OZR-oYrpJBKAJL...

Committed: https://crrev.com/3194544827727439d99dc72bec19d59a6004db0d
Cr-Commit-Position: refs/heads/master@{#391562}
==========

[commit-bot: I haz the power, 2016-05-04 18:15:35]

Patchset 31 (id:??) landed as
https://crrev.com/3194544827727439d99dc72bec19d59a6004db0d
Cr-Commit-Position: refs/heads/master@{#391562}

[Mathieu, 2016-05-04 19:42:01]

A revert of this CL (patchset #31 id:600001) has been created in
https://codereview.chromium.org/1949843004/ by mathp@chromium.org.

The reason for reverting is: Some snap tests are failing:

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Android%20%28N....

[majidvp, 2016-05-10 21:08:31]

Patchset #31 (id:600001) has been deleted

[majidvp, 2016-06-29 14:14:48]

Description was changed from

==========
Compute snap offsets according to CSS Scroll Snap Spec Level 1

Add logic to compute snap offsets (those defined on snap container and snap
areas) according to the published spec [1].

SnapCoordinator maintains the mapping between snap containers and the snap
areas. This mapping is stored in |LayoutBoxRareData| and updated on style
change, attach, and detach events via hooks in |LayoutBox::styleWillChange| ,
|LayoutBox::insertedIntoTree| and |LayoutBox::willBeDestoryed|.

This implementation is the naive approach as described in [2]. It does not
attempt to track which snap areas have moved in relation to their container to
minimize  snap offsets re-computation. It simply re-computes all snap points
every time they are needed.

At the moment the computed snap offsets are not used for anything except to pass
the tests. In future,  |SnapCoordinator| will compute them as part of the
document lifecycle and send necessary updates to the Compositor Worker instance
which is going to be responsible for snapping during scrolls.

[1] https://www.w3.org/TR/2015/WD-css-snappoints-1-20150326/
[2]
https://docs.google.com/document/d/1BO0EgcMe3DTsO1uwM_psZeJ_Gz5OZR-oYrpJBKAJL...

Committed: https://crrev.com/3194544827727439d99dc72bec19d59a6004db0d
Cr-Commit-Position: refs/heads/master@{#391562}
==========

to

==========
Compute snap offsets according to CSS Scroll Snap Spec Level 1

Add logic to compute snap offsets (those defined on snap container and snap
areas) according to the published spec [1].

SnapCoordinator maintains the mapping between snap containers and the snap
areas. This mapping is stored in |LayoutBoxRareData| and updated on style
change, attach, and detach events via hooks in |LayoutBox::styleWillChange| ,
|LayoutBox::insertedIntoTree| and |LayoutBox::willBeDestoryed|.

This implementation is the naive approach as described in [2]. It does not
attempt to track which snap areas have moved in relation to their container to
minimize  snap offsets re-computation. It simply re-computes all snap points
every time they are needed.

At the moment the computed snap offsets are not used for anything except to pass
the tests. In future,  |SnapCoordinator| will compute them as part of the
document lifecycle and send necessary updates to the Compositor Worker instance
which is going to be responsible for snapping during scrolls.

[1] https://www.w3.org/TR/2015/WD-css-snappoints-1-20150326/
[2]
https://docs.google.com/document/d/1BO0EgcMe3DTsO1uwM_psZeJ_Gz5OZR-oYrpJBKAJL...

Committed: https://crrev.com/3194544827727439d99dc72bec19d59a6004db0d
Cr-Commit-Position: refs/heads/master@{#391562}
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
==========

[majidvp, 2016-06-29 14:23:59]

Patchset #32 (id:640001) has been deleted