[root layer scrolls] TopDocumentRootScrollerController needs to use layoutViewportScrollableArea

[root layer scrolls] TopDocumentRootScrollerController needs to use layoutViewportScrollableArea

R=skobes@chromium.org
BUG=695257

[cbiesinger, 2017-02-25 01:02:51]

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

[commit-bot: I haz the power, 2017-02-25 01:03:04]

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, 2017-02-25 01:04:45]

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

[commit-bot: I haz the power, 2017-02-25 01:04:46]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[cbiesinger, 2017-02-25 01:07:16]

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

[commit-bot: I haz the power, 2017-02-25 01:07:29]

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

[skobes, 2017-02-25 01:45:03]

lgtm

[cbiesinger1, 2017-02-25 02:15:32]

The CQ bit was unchecked by cbiesinger@google.com

[cbiesinger1, 2017-02-25 02:15:36]

The CQ bit was checked by cbiesinger@google.com

[commit-bot: I haz the power, 2017-02-25 02:15:53]

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, 2017-02-25 02:37:52]

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

[commit-bot: I haz the power, 2017-02-25 02:37:53]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[bokan, 2017-02-27 14:09:37]

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

[bokan, 2017-02-27 14:09:38]

Good catch, this looks like the correct fix to me. Could we get a test for this?

[cbiesinger, 2017-02-27 23:32:39]

On 2017/02/27 14:09:38, bokan wrote:
> Good catch, this looks like the correct fix to me. Could we get a test for
this?

So I still need to investigate the failing VisualViewportTest, but in the
meantime can you advise what the usual approach is for writing tests specific to
root layer scrolling?

[bokan, 2017-02-27 23:36:48]

On 2017/02/27 23:32:39, cbiesinger wrote:
> On 2017/02/27 14:09:38, bokan wrote:
> > Good catch, this looks like the correct fix to me. Could we get a test for
> this?
> 
> So I still need to investigate the failing VisualViewportTest, but in the
> meantime can you advise what the usual approach is for writing tests specific
to
> root layer scrolling?

There's a suite of tests in RootScrollerTest.cpp where this would probably
belong. I think it should be converted to run with and without RLS enabled. If
it's simple, you could add a parameterized test type like we have for
VisualViewportTest. If that causes tests to fail, feel free to just add a
one-off test that turns on RLS and I'll convert the suite to parameterize on
RLS.

[skobes, 2017-02-27 23:41:11]

On 2017/02/27 23:32:39, cbiesinger wrote:
> On 2017/02/27 14:09:38, bokan wrote:
> > Good catch, this looks like the correct fix to me. Could we get a test for
> this?
> 
> So I still need to investigate the failing VisualViewportTest, but in the
> meantime can you advise what the usual approach is for writing tests specific
to
> root layer scrolling?

We have some unit tests that are parameterized to run with and without RLS.  You
could also add a test case to web/tests/ScrollbarsTest.cpp which is currently an
RLS-only SimTest.

[skobes, 2017-03-10 19:04:58]

What's the status of this patch?

[cbiesinger, 2017-03-10 21:58:24]

On 2017/03/10 19:04:58, skobes wrote:
> What's the status of this patch?

The status is that I haven't figured out yet why the test is failing :( still
working on that though...

[skobes, 2017-03-14 22:44:51]

I looked into the test failures here and they are caused by incorrect
PaintLayer::size() when page scale is > 1.  I filed http://crbug.com/701575 to
track setting the correct PaintLayer size.

I have a workaround in http://crrev.com/2751833003, but I think it's too hacky
to land.

A complete fix for http://crbug.com/701575 may take some time due to the
lifecycle refactoring (setting size during LayoutView::layout instead of
WebViewImpl::postLayoutResize), but I suspect a partial fix that just touches
PaintLayer::updateLayerPosition might be enough to unblock this patch.

[cbiesinger, 2017-05-05 20:59:45]

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

[commit-bot: I haz the power, 2017-05-05 21:00:48]

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, 2017-05-05 22:10:51]

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

[commit-bot: I haz the power, 2017-05-05 22:10:52]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[cbiesinger, 2017-05-19 22:46:21]

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

[commit-bot: I haz the power, 2017-05-19 22:46:55]

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, 2017-05-20 00:05:00]

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

[commit-bot: I haz the power, 2017-05-20 00:05:01]

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

[skobes, 2017-05-23 03:10:03]

On 2017/03/14 22:44:51, skobes wrote:
> I looked into the test failures here and they are caused by incorrect
> PaintLayer::size() when page scale is > 1.  I filed http://crbug.com/701575 to
> track setting the correct PaintLayer size.
> 
> I have a workaround in http://crrev.com/2751833003, but I think it's too hacky
> to land.
> 
> A complete fix for http://crbug.com/701575 may take some time due to the
> lifecycle refactoring (setting size during LayoutView::layout instead of
> WebViewImpl::postLayoutResize), but I suspect a partial fix that just touches
> PaintLayer::updateLayerPosition might be enough to unblock this patch.

cbiesinger: I think you can revive this patch now that szager has fixed
http://crbug.com/701575.

[cbiesinger, 2017-05-24 16:02:22]

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

[commit-bot: I haz the power, 2017-05-24 16:02:54]

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, 2017-05-24 17:30:28]

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

[commit-bot: I haz the power, 2017-05-24 17:30:29]

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_...)

[cbiesinger, 2017-05-24 18:48:06]

On 2017/05/23 03:10:03, skobes wrote:
> On 2017/03/14 22:44:51, skobes wrote:
> > I looked into the test failures here and they are caused by incorrect
> > PaintLayer::size() when page scale is > 1.  I filed http://crbug.com/701575
to
> > track setting the correct PaintLayer size.
> > 
> > I have a workaround in http://crrev.com/2751833003, but I think it's too
hacky
> > to land.
> > 
> > A complete fix for http://crbug.com/701575 may take some time due to the
> > lifecycle refactoring (setting size during LayoutView::layout instead of
> > WebViewImpl::postLayoutResize), but I suspect a partial fix that just
touches
> > PaintLayer::updateLayerPosition might be enough to unblock this patch.
> 
> cbiesinger: I think you can revive this patch now that szager has fixed
> http://crbug.com/701575.

Hm... looks like I still have two failing tests. I don't remember what was
failing before, so I don't know if this is the same test that's failing. I need
to debug this...

[cbiesinger, 2017-06-09 20:51:35]

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

[commit-bot: I haz the power, 2017-06-09 20:52:02]

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, 2017-06-09 22:04:59]

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

[commit-bot: I haz the power, 2017-06-09 22:05:00]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[cbiesinger, 2017-06-14 16:40:08]

Fixed by skobes in https://chromium-review.googlesource.com/531934 (thanks!), so
I'll close this one.

[cbiesinger, 2017-06-14 16:40:19]

Description was changed from

==========
[root layer scrolls] TopDocumentRootScrollerController needs to use
layoutViewportScrollableArea

R=skobes@chromium.org
BUG=695257
==========

to

==========
[root layer scrolls] TopDocumentRootScrollerController needs to use
layoutViewportScrollableArea

R=skobes@chromium.org
BUG=695257
==========