Make rootScroller the layout viewport.

Make rootScroller the layout viewport.

In order to properly support pinch zoom and other viewport scrolling actions, we
need to promote the rootScroller element's ScrollableArea to be the layout
viewport. This is primarily accomplished by replacing the layout viewport in
RootFrameViewport.

I've moved the ViewportScrollCallback creation to happen at Document attachment
time. This is necessary since the FrameView may not have fully initialized its
ScrollableArea until this happens. Notably, this is needed for
root-layer-scrolls since the ScrollableArea for a FrameView will be the
LayoutView's PaintLayerScrollableArea which won't be initialized until
attachment.

I've also updated the set-root-scroller test to break up the one big test and
replace it with many small ones.

BUG=505516
Committed: https://crrev.com/de92895a8fd3c8f3719b8875651cffc7ebebd3e7
Cr-Commit-Position: refs/heads/master@{#404250}

[bokan, 2016-07-06 15:55:12]

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

[commit-bot: I haz the power, 2016-07-06 15:55:43]

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-07-06 17:15:22]

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

[commit-bot: I haz the power, 2016-07-06 17:15:23]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[bokan, 2016-07-06 17:50:16]

Description was changed from

==========
Make rootScroller the layout viewport.

In order to properly support pinch zoom and other viewport scrolling actions, we
need to promote the rootScroller element's ScrollableArea to be the layout
viewport. This is primarily accomplished by replacing the layout viewport in
RootFrameViewport.

I've moved the ViewportScrollCallback creation to happen at Document attachment
time. This is necessary since the FrameView may not have fully initialized its
ScrollableArea until this happens. Notably, this is needed for
root-layer-scrolls since the ScrollableArea for a FrameView will be the
LayoutView's PaintLayerScrollableArea which won't be initialized until
attachment.

I've also updated the set-root-scroller test to break up the one big test and
replace it with many small ones.

BUG=505516
==========

to

==========
Make rootScroller the layout viewport.

In order to properly support pinch zoom and other viewport scrolling actions, we
need to promote the rootScroller element's ScrollableArea to be the layout
viewport. This is primarily accomplished by replacing the layout viewport in
RootFrameViewport.

I've moved the ViewportScrollCallback creation to happen at Document attachment
time. This is necessary since the FrameView may not have fully initialized its
ScrollableArea until this happens. Notably, this is needed for
root-layer-scrolls since the ScrollableArea for a FrameView will be the
LayoutView's PaintLayerScrollableArea which won't be initialized until
attachment.

I've also updated the set-root-scroller test to break up the one big test and
replace it with many small ones.

BUG=505516
==========

[bokan, 2016-07-06 17:50:16]

bokan@chromium.org changed reviewers:
+ tdresser@chromium.org

[tdresser, 2016-07-07 16:37:33]

LGTM.

https://codereview.chromium.org/2128553002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/scrolling/set-root-scroller.html
(right):

https://codereview.chromium.org/2128553002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scrolling/set-root-scroller.html:138: //
documentElement, but the rootScroller property should remain the same.
Woah, that's kinda weird. Why did we decide to keep the rootScroller property
the same?

https://codereview.chromium.org/2128553002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scrolling/set-root-scroller.html:143: //
Reset #parent's scrolling, make container fully scrolled.
I'd prefer to refer to the variable name, instead of the id.

[bokan, 2016-07-07 16:45:47]

https://codereview.chromium.org/2128553002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/scrolling/set-root-scroller.html
(right):

https://codereview.chromium.org/2128553002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scrolling/set-root-scroller.html:138: //
documentElement, but the rootScroller property should remain the same.
On 2016/07/07 16:37:33, tdresser wrote:
> Woah, that's kinda weird. Why did we decide to keep the rootScroller property
> the same?

This was actually some feedback I got from Elliott:

"...it means if you display: none the root scroller, force a layout, and then
display: block it again the scroller is reset. I don't think this feature should
work that way."

I think it makes sense. We don't want transient changes to reset the
rootScroller. Specifically since you'll want to do things like animate in the
root scroller, this makes that kind of case much simpler. Instead, we always try
to use element set as the rootScroller but if it's invalid we (silently)
fallback to the default. We'll see if we need some kind of feedback to the
developer since I'm a little worried about the silent part, but I'm also weary
of warning spam.

https://codereview.chromium.org/2128553002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scrolling/set-root-scroller.html:143: //
Reset #parent's scrolling, make container fully scrolled.
On 2016/07/07 16:37:33, tdresser wrote:
> I'd prefer to refer to the variable name, instead of the id.

Done.

[tdresser, 2016-07-07 16:51:59]

https://codereview.chromium.org/2128553002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/scrolling/set-root-scroller.html
(right):

https://codereview.chromium.org/2128553002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scrolling/set-root-scroller.html:138: //
documentElement, but the rootScroller property should remain the same.
On 2016/07/07 16:45:47, bokan wrote:
> On 2016/07/07 16:37:33, tdresser wrote:
> > Woah, that's kinda weird. Why did we decide to keep the rootScroller
property
> > the same?
> 
> This was actually some feedback I got from Elliott:
> 
> "...it means if you display: none the root scroller, force a layout, and then
> display: block it again the scroller is reset. I don't think this feature
should
> work that way."
> 
> I think it makes sense. We don't want transient changes to reset the
> rootScroller. Specifically since you'll want to do things like animate in the
> root scroller, this makes that kind of case much simpler. Instead, we always
try
> to use element set as the rootScroller but if it's invalid we (silently)
> fallback to the default. We'll see if we need some kind of feedback to the
> developer since I'm a little worried about the silent part, but I'm also weary
> of warning spam. 

Maybe you should provide developer feedback if they try to programmatically
scroll when there is an invalid root scroller?

I think it would be easy to accidentally have your scroll not be applied
correctly.

[bokan, 2016-07-07 16:54:35]

On 2016/07/07 16:51:59, tdresser wrote:
>
https://codereview.chromium.org/2128553002/diff/40001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/fast/scrolling/set-root-scroller.html
> (right):
> 
>
https://codereview.chromium.org/2128553002/diff/40001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/fast/scrolling/set-root-scroller.html:138: //
> documentElement, but the rootScroller property should remain the same.
> On 2016/07/07 16:45:47, bokan wrote:
> > On 2016/07/07 16:37:33, tdresser wrote:
> > > Woah, that's kinda weird. Why did we decide to keep the rootScroller
> property
> > > the same?
> > 
> > This was actually some feedback I got from Elliott:
> > 
> > "...it means if you display: none the root scroller, force a layout, and
then
> > display: block it again the scroller is reset. I don't think this feature
> should
> > work that way."
> > 
> > I think it makes sense. We don't want transient changes to reset the
> > rootScroller. Specifically since you'll want to do things like animate in
the
> > root scroller, this makes that kind of case much simpler. Instead, we always
> try
> > to use element set as the rootScroller but if it's invalid we (silently)
> > fallback to the default. We'll see if we need some kind of feedback to the
> > developer since I'm a little worried about the silent part, but I'm also
weary
> > of warning spam. 
> 
> Maybe you should provide developer feedback if they try to programmatically
> scroll when there is an invalid root scroller?
> 
> I think it would be easy to accidentally have your scroll not be applied
> correctly.

Root scroller doesn't really affect programmatic scrolls (since they don't chain
or affect topcontrols/overscroller, etc). In any case, I think once we have a
near-complete implementation we can see how bad this is in practice and what
kind of mitigation might work best.

[tdresser, 2016-07-07 16:56:20]

On 2016/07/07 16:54:35, bokan wrote:
> On 2016/07/07 16:51:59, tdresser wrote:
> >
>
https://codereview.chromium.org/2128553002/diff/40001/third_party/WebKit/Layo...
> > File third_party/WebKit/LayoutTests/fast/scrolling/set-root-scroller.html
> > (right):
> > 
> >
>
https://codereview.chromium.org/2128553002/diff/40001/third_party/WebKit/Layo...
> > third_party/WebKit/LayoutTests/fast/scrolling/set-root-scroller.html:138: //
> > documentElement, but the rootScroller property should remain the same.
> > On 2016/07/07 16:45:47, bokan wrote:
> > > On 2016/07/07 16:37:33, tdresser wrote:
> > > > Woah, that's kinda weird. Why did we decide to keep the rootScroller
> > property
> > > > the same?
> > > 
> > > This was actually some feedback I got from Elliott:
> > > 
> > > "...it means if you display: none the root scroller, force a layout, and
> then
> > > display: block it again the scroller is reset. I don't think this feature
> > should
> > > work that way."
> > > 
> > > I think it makes sense. We don't want transient changes to reset the
> > > rootScroller. Specifically since you'll want to do things like animate in
> the
> > > root scroller, this makes that kind of case much simpler. Instead, we
always
> > try
> > > to use element set as the rootScroller but if it's invalid we (silently)
> > > fallback to the default. We'll see if we need some kind of feedback to the
> > > developer since I'm a little worried about the silent part, but I'm also
> weary
> > > of warning spam. 
> > 
> > Maybe you should provide developer feedback if they try to programmatically
> > scroll when there is an invalid root scroller?
> > 
> > I think it would be easy to accidentally have your scroll not be applied
> > correctly.
> 
> Root scroller doesn't really affect programmatic scrolls (since they don't
chain
> or affect topcontrols/overscroller, etc). In any case, I think once we have a
> near-complete implementation we can see how bad this is in practice and what
> kind of mitigation might work best.

I could try to scroll via document.rootScroller, couldn't I? Which might not do
what I expect it to?

You're right though, this is fine for now.

[bokan, 2016-07-07 16:58:29]

On 2016/07/07 16:56:20, tdresser wrote:
> On 2016/07/07 16:54:35, bokan wrote:
> > On 2016/07/07 16:51:59, tdresser wrote:
> > >
> >
>
https://codereview.chromium.org/2128553002/diff/40001/third_party/WebKit/Layo...
> > > File third_party/WebKit/LayoutTests/fast/scrolling/set-root-scroller.html
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2128553002/diff/40001/third_party/WebKit/Layo...
> > > third_party/WebKit/LayoutTests/fast/scrolling/set-root-scroller.html:138:
//
> > > documentElement, but the rootScroller property should remain the same.
> > > On 2016/07/07 16:45:47, bokan wrote:
> > > > On 2016/07/07 16:37:33, tdresser wrote:
> > > > > Woah, that's kinda weird. Why did we decide to keep the rootScroller
> > > property
> > > > > the same?
> > > > 
> > > > This was actually some feedback I got from Elliott:
> > > > 
> > > > "...it means if you display: none the root scroller, force a layout, and
> > then
> > > > display: block it again the scroller is reset. I don't think this
feature
> > > should
> > > > work that way."
> > > > 
> > > > I think it makes sense. We don't want transient changes to reset the
> > > > rootScroller. Specifically since you'll want to do things like animate
in
> > the
> > > > root scroller, this makes that kind of case much simpler. Instead, we
> always
> > > try
> > > > to use element set as the rootScroller but if it's invalid we (silently)
> > > > fallback to the default. We'll see if we need some kind of feedback to
the
> > > > developer since I'm a little worried about the silent part, but I'm also
> > weary
> > > > of warning spam. 
> > > 
> > > Maybe you should provide developer feedback if they try to
programmatically
> > > scroll when there is an invalid root scroller?
> > > 
> > > I think it would be easy to accidentally have your scroll not be applied
> > > correctly.
> > 
> > Root scroller doesn't really affect programmatic scrolls (since they don't
> chain
> > or affect topcontrols/overscroller, etc). In any case, I think once we have
a
> > near-complete implementation we can see how bad this is in practice and what
> > kind of mitigation might work best.
> 
> I could try to scroll via document.rootScroller, couldn't I? Which might not
do
> what I expect it to?

Good point, I guess it could affect programmatic scrolls in that way. But in
that case, having it scroll the "invalid" root scroller is probably what you
want anyway, assuming it'll become valid at some point. Anyway, probably
requires some experience using it to really tell :)

[bokan, 2016-07-07 19:00:01]

The CQ bit was checked by bokan@chromium.org

[bokan, 2016-07-07 19:00:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/2128553002/#ps60001
(title: "Rebase + comment fix in test")

[commit-bot: I haz the power, 2016-07-07 19:01:08]

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-07-07 19:01:10]

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-07-07 22:15:39]

Description was changed from

==========
Make rootScroller the layout viewport.

In order to properly support pinch zoom and other viewport scrolling actions, we
need to promote the rootScroller element's ScrollableArea to be the layout
viewport. This is primarily accomplished by replacing the layout viewport in
RootFrameViewport.

I've moved the ViewportScrollCallback creation to happen at Document attachment
time. This is necessary since the FrameView may not have fully initialized its
ScrollableArea until this happens. Notably, this is needed for
root-layer-scrolls since the ScrollableArea for a FrameView will be the
LayoutView's PaintLayerScrollableArea which won't be initialized until
attachment.

I've also updated the set-root-scroller test to break up the one big test and
replace it with many small ones.

BUG=505516
==========

to

==========
Make rootScroller the layout viewport.

In order to properly support pinch zoom and other viewport scrolling actions, we
need to promote the rootScroller element's ScrollableArea to be the layout
viewport. This is primarily accomplished by replacing the layout viewport in
RootFrameViewport.

I've moved the ViewportScrollCallback creation to happen at Document attachment
time. This is necessary since the FrameView may not have fully initialized its
ScrollableArea until this happens. Notably, this is needed for
root-layer-scrolls since the ScrollableArea for a FrameView will be the
LayoutView's PaintLayerScrollableArea which won't be initialized until
attachment.

I've also updated the set-root-scroller test to break up the one big test and
replace it with many small ones.

BUG=505516
==========

[commit-bot: I haz the power, 2016-07-07 22:15:40]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-07 22:15:44]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-07 22:17:22]

Description was changed from

==========
Make rootScroller the layout viewport.

In order to properly support pinch zoom and other viewport scrolling actions, we
need to promote the rootScroller element's ScrollableArea to be the layout
viewport. This is primarily accomplished by replacing the layout viewport in
RootFrameViewport.

I've moved the ViewportScrollCallback creation to happen at Document attachment
time. This is necessary since the FrameView may not have fully initialized its
ScrollableArea until this happens. Notably, this is needed for
root-layer-scrolls since the ScrollableArea for a FrameView will be the
LayoutView's PaintLayerScrollableArea which won't be initialized until
attachment.

I've also updated the set-root-scroller test to break up the one big test and
replace it with many small ones.

BUG=505516
==========

to

==========
Make rootScroller the layout viewport.

In order to properly support pinch zoom and other viewport scrolling actions, we
need to promote the rootScroller element's ScrollableArea to be the layout
viewport. This is primarily accomplished by replacing the layout viewport in
RootFrameViewport.

I've moved the ViewportScrollCallback creation to happen at Document attachment
time. This is necessary since the FrameView may not have fully initialized its
ScrollableArea until this happens. Notably, this is needed for
root-layer-scrolls since the ScrollableArea for a FrameView will be the
LayoutView's PaintLayerScrollableArea which won't be initialized until
attachment.

I've also updated the set-root-scroller test to break up the one big test and
replace it with many small ones.

BUG=505516

Committed: https://crrev.com/de92895a8fd3c8f3719b8875651cffc7ebebd3e7
Cr-Commit-Position: refs/heads/master@{#404250}
==========

[commit-bot: I haz the power, 2016-07-07 22:17:23]

Patchset 4 (id:??) landed as
https://crrev.com/de92895a8fd3c8f3719b8875651cffc7ebebd3e7
Cr-Commit-Position: refs/heads/master@{#404250}