Adding rootlayerscrolls-specific test expectations.

Adding rootlayerscrolls-specific test expectations.

Adding rootlayerscrolls-specific test expectations for these test cases

virtual/rootlayerscrolls/css3/filters/blur-filter-page-scroll-parents.html
virtual/rootlayerscrolls/css3/filters/blur-filter-page-scroll-self.html
virtual/rootlayerscrolls/css3/filters/blur-filter-page-scroll.html

BUG=651793
Committed: https://crrev.com/28c207c7b4d78a3f9d926d697b599878324069e5
Cr-Commit-Position: refs/heads/master@{#427145}

[MuVen, 2016-10-12 07:56:34]

Description was changed from

==========
done


done

BUG=
==========

to

==========
Offset LayerBounds by root layer's scroll offset.

BUG=651793
==========

[MuVen, 2016-10-12 07:56:45]

The CQ bit was checked by sataya.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 07:57:01]

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

[commit-bot: I haz the power, 2016-10-12 08:07:28]

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

[commit-bot: I haz the power, 2016-10-12 08:07:29]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[MuVen, 2016-10-12 09:11:39]

The CQ bit was checked by sataya.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 09:12:05]

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

[commit-bot: I haz the power, 2016-10-12 09:24:25]

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

[commit-bot: I haz the power, 2016-10-12 09:24:25]

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

[MuVen, 2016-10-12 09:26:11]

The CQ bit was checked by sataya.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 09:26:28]

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

[commit-bot: I haz the power, 2016-10-12 11:18:53]

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

[commit-bot: I haz the power, 2016-10-12 11:18:53]

Dry run: This issue passed the CQ dry run.

[MuVen, 2016-10-12 13:35:50]

PTAL.

[MuVen, 2016-10-13 16:05:42]

Description was changed from

==========
Offset LayerBounds by root layer's scroll offset.

BUG=651793
==========

to

==========
Add root layer scroll offset to paintlayer LayerBounds.

Add root layer scroll offset to paintlayer LayerBounds in case of
Root layer scrolls

BUG=651793
==========

[MuVen, 2016-10-13 16:05:42]

sataya.m@samsung.com changed reviewers:
+ skobes@chromium.org

[MuVen, 2016-10-13 16:06:01]

Description was changed from

==========
Add root layer scroll offset to paintlayer LayerBounds.

Add root layer scroll offset to paintlayer LayerBounds in case of
Root layer scrolls

BUG=651793
==========

to

==========
Add root layer scroll offset to paintlayer LayerBounds.

Add root layer scroll offset to paintlayer LayerBounds in case of
Root layer scrolls.

BUG=651793
==========

[MuVen, 2016-10-13 16:06:51]

Skobes, PTAL

[szager1, 2016-10-13 18:02:09]

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

[szager1, 2016-10-13 18:02:10]

This probably needs to be fixed in PaintLayerClipper rather than here.

[skobes, 2016-10-13 18:06:28]

On 2016/10/13 18:02:10, szager1 wrote:
> This probably needs to be fixed in PaintLayerClipper rather than here.

I disagree, see http://crbug.com/651793#c2.

https://codereview.chromium.org/2412013003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTreeAsText.cpp (right):

https://codereview.chromium.org/2412013003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTreeAsText.cpp:690:
scrollableArea->scrollPosition() + scrollableArea->scrollOrigin();
Adding the scroll origin doesn't make sense, the scroll position should already
include it.

[szager1, 2016-10-13 18:44:31]

On 2016/10/13 18:06:28, skobes wrote:
> On 2016/10/13 18:02:10, szager1 wrote:
> > This probably needs to be fixed in PaintLayerClipper rather than here.
> 
> I disagree, see http://crbug.com/651793#c2.

Have you tried with slimming paint v2 enabled?  I'm not sure GeometryMapper will
behave this way with RLS enabled.

[skobes, 2016-10-13 18:50:18]

On 2016/10/13 18:44:31, szager1 wrote:
> Have you tried with slimming paint v2 enabled?  I'm not sure GeometryMapper
will
> behave this way with RLS enabled.

I haven't.  Is GeometryMapper a replacement for PaintLayerClipper?

My reasoning on PaintLayerClipper is that it shouldn't treat the root layer as
"special".  If it adjusts for scroll offsets of other layers it should adjust
for scroll offset of the root layer too.

[szager1, 2016-10-13 18:55:16]

On 2016/10/13 18:50:18, skobes wrote:
> On 2016/10/13 18:44:31, szager1 wrote:
> > Have you tried with slimming paint v2 enabled?  I'm not sure GeometryMapper
> will
> > behave this way with RLS enabled.
> 
> I haven't.  Is GeometryMapper a replacement for PaintLayerClipper?

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai...

m_geometryMapper is only non-null when spv2 is enabled.  So this turns on an
entirely new code path for calculating the clipping.


> My reasoning on PaintLayerClipper is that it shouldn't treat the root layer as
> "special".  If it adjusts for scroll offsets of other layers it should adjust
> for scroll offset of the root layer too.

From what I can tell, there are various places in the blink code that call into
geometry mapping code (mapLocalToAncestor, LayoutGeometryMap, etc.), and they
expect to get back document coordinates.  I have no objection to allowing the
geometry code to start returning frame coordinates instead, but we have to fix
the call sites as well.

[skobes, 2016-10-13 19:06:56]

On 2016/10/13 18:55:16, szager1 wrote:
> From what I can tell, there are various places in the blink code that call
into
> geometry mapping code (mapLocalToAncestor, LayoutGeometryMap, etc.), and they
> expect to get back document coordinates.  I have no objection to allowing the
> geometry code to start returning frame coordinates instead, but we have to fix
> the call sites as well.

I think changing PaintLayerClipper to return document coordinates probably
requires fixing various things in hit testing and such that expect the current
behavior.  But I could be convinced if that turns out to be much easier than
making spv2 conform.

[szager1, 2016-10-13 19:32:46]

On 2016/10/13 19:06:56, skobes wrote:
> On 2016/10/13 18:55:16, szager1 wrote:
> > From what I can tell, there are various places in the blink code that call
> into
> > geometry mapping code (mapLocalToAncestor, LayoutGeometryMap, etc.), and
they
> > expect to get back document coordinates.  I have no objection to allowing
the
> > geometry code to start returning frame coordinates instead, but we have to
fix
> > the call sites as well.
> 
> I think changing PaintLayerClipper to return document coordinates probably
> requires fixing various things in hit testing and such that expect the current
> behavior.  But I could be convinced if that turns out to be much easier than
> making spv2 conform.

I'd like to get chrishtr@ into the discussion, this has come up a couple of
times recently and I'm in the process of figuring out what to do with
LayoutGeometryMap.

I'm OK with landing this patch if it lets us eliminate a lot of spurious layout
test failures so we can focus on genuine breakage.

[skobes, 2016-10-13 19:44:37]

@muven: how many tests does this fix?

On 2016/10/13 19:32:46, szager1 wrote:
> On 2016/10/13 19:06:56, skobes wrote:
> > On 2016/10/13 18:55:16, szager1 wrote:
> > > From what I can tell, there are various places in the blink code that call
> > into
> > > geometry mapping code (mapLocalToAncestor, LayoutGeometryMap, etc.), and
> they
> > > expect to get back document coordinates.  I have no objection to allowing
> the
> > > geometry code to start returning frame coordinates instead, but we have to
> fix
> > > the call sites as well.
> > 
> > I think changing PaintLayerClipper to return document coordinates probably
> > requires fixing various things in hit testing and such that expect the
current
> > behavior.  But I could be convinced if that turns out to be much easier than
> > making spv2 conform.
> 
> I'd like to get chrishtr@ into the discussion, this has come up a couple of
> times recently and I'm in the process of figuring out what to do with
> LayoutGeometryMap.
> 
> I'm OK with landing this patch if it lets us eliminate a lot of spurious
layout
> test failures so we can focus on genuine breakage.

[MuVen, 2016-10-14 00:46:34]

On 2016/10/13 19:44:37, skobes wrote:
> @muven: how many tests does this fix?
> 
> On 2016/10/13 19:32:46, szager1 wrote:
> > On 2016/10/13 19:06:56, skobes wrote:
> > > On 2016/10/13 18:55:16, szager1 wrote:
> > > > From what I can tell, there are various places in the blink code that
call
> > > into
> > > > geometry mapping code (mapLocalToAncestor, LayoutGeometryMap, etc.), and
> > they
> > > > expect to get back document coordinates.  I have no objection to
allowing
> > the
> > > > geometry code to start returning frame coordinates instead, but we have
to
> > fix
> > > > the call sites as well.
> > > 
> > > I think changing PaintLayerClipper to return document coordinates probably
> > > requires fixing various things in hit testing and such that expect the
> current
> > > behavior.  But I could be convinced if that turns out to be much easier
than
> > > making spv2 conform.
> > 
> > I'd like to get chrishtr@ into the discussion, this has come up a couple of
> > times recently and I'm in the process of figuring out what to do with
> > LayoutGeometryMap.
> > 
> > I'm OK with landing this patch if it lets us eliminate a lot of spurious
> layout
> > test failures so we can focus on genuine breakage.

This fixes 3 test cases

[MuVen, 2016-10-14 00:50:25]

On 2016/10/14 00:46:34, MuVen wrote:
> On 2016/10/13 19:44:37, skobes wrote:
> > @muven: how many tests does this fix?
> > 
> > On 2016/10/13 19:32:46, szager1 wrote:
> > > On 2016/10/13 19:06:56, skobes wrote:
> > > > On 2016/10/13 18:55:16, szager1 wrote:
> > > > > From what I can tell, there are various places in the blink code that
> call
> > > > into
> > > > > geometry mapping code (mapLocalToAncestor, LayoutGeometryMap, etc.),
and
> > > they
> > > > > expect to get back document coordinates.  I have no objection to
> allowing
> > > the
> > > > > geometry code to start returning frame coordinates instead, but we
have
> to
> > > fix
> > > > > the call sites as well.
> > > > 
> > > > I think changing PaintLayerClipper to return document coordinates
probably
> > > > requires fixing various things in hit testing and such that expect the
> > current
> > > > behavior.  But I could be convinced if that turns out to be much easier
> than
> > > > making spv2 conform.
> > > 
> > > I'd like to get chrishtr@ into the discussion, this has come up a couple
of
> > > times recently and I'm in the process of figuring out what to do with
> > > LayoutGeometryMap.
> > > 
> > > I'm OK with landing this patch if it lets us eliminate a lot of spurious
> > layout
> > > test failures so we can focus on genuine breakage.
> 

This fixes 3 test cases in the css3.0/filters/folder, may be few more in the
css3.0 folder.

[MuVen, 2016-10-14 08:32:18]

The CQ bit was checked by sataya.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 08:32:32]

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

[MuVen, 2016-10-14 08:40:01]

Patchset #5 (id:80001) has been deleted

[MuVen, 2016-10-14 08:41:38]

The CQ bit was checked by sataya.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 08:41:55]

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

[MuVen, 2016-10-14 08:42:56]

Patchset #6 (id:120001) has been deleted

[MuVen, 2016-10-14 08:44:55]

The CQ bit was checked by sataya.m@samsung.com to run a CQ dry run

[MuVen, 2016-10-14 08:45:00]

Patchset #4 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-10-14 08:45:10]

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

[commit-bot: I haz the power, 2016-10-14 09:33:57]

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

[commit-bot: I haz the power, 2016-10-14 09:33:58]

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

[MuVen, 2016-10-14 11:18:20]

4 test cases are fixed, with this patch.

virtual/rootlayerscrolls/css2.1/20110323/abspos-containing-block-initial-001.htm
virtual/rootlayerscrolls/css3/filters/blur-filter-page-scroll-parents.html
virtual/rootlayerscrolls/css3/filters/blur-filter-page-scroll-self.html
virtual/rootlayerscrolls/css3/filters/blur-filter-page-scroll.html

[skobes, 2016-10-14 16:59:16]

On 2016/10/14 11:18:20, MuVen wrote:
> 4 test cases are fixed, with this patch.

Oh, I thought it would be a lot more than that.  If it's only 4 tests maybe we
can just add flag-specific baselines?  If you create a file like

 
third_party/WebKit/LayoutTests/flag-specific/root-layer-scrolls/css3/filters/blur-filter-page-scroll-expected.txt

then the test runner should use that baseline when you pass
--additional-driver-flag=--root-layer-scrolls.

You will have to create the LayoutTests/flag-specific directory since you are
the first person to use this feature. :)

[MuVen, 2016-10-21 11:01:07]

Description was changed from

==========
Add root layer scroll offset to paintlayer LayerBounds.

Add root layer scroll offset to paintlayer LayerBounds in case of
Root layer scrolls.

BUG=651793
==========

to

==========
Adding rootlayerscrolls-specific test expectations.

Adding rootlayerscrolls-specific test expectations for these test cases

virtual/rootlayerscrolls/css3/filters/blur-filter-page-scroll-parents.html
virtual/rootlayerscrolls/css3/filters/blur-filter-page-scroll-self.html
virtual/rootlayerscrolls/css3/filters/blur-filter-page-scroll.html

BUG=651793
==========

[MuVen, 2016-10-21 11:01:28]

The CQ bit was checked by sataya.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 11:01:46]

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

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

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

[commit-bot: I haz the power, 2016-10-21 12:12:02]

Dry run: This issue passed the CQ dry run.

[MuVen, 2016-10-21 15:21:06]

PTAL

[MuVen, 2016-10-24 15:29:50]

Friendly Ping !!!

[skobes, 2016-10-24 18:32:50]

lgtm

[MuVen, 2016-10-24 18:44:42]

The CQ bit was checked by sataya.m@samsung.com

[commit-bot: I haz the power, 2016-10-24 18:45:32]

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

[commit-bot: I haz the power, 2016-10-24 21:00:09]

Description was changed from

==========
Adding rootlayerscrolls-specific test expectations.

Adding rootlayerscrolls-specific test expectations for these test cases

virtual/rootlayerscrolls/css3/filters/blur-filter-page-scroll-parents.html
virtual/rootlayerscrolls/css3/filters/blur-filter-page-scroll-self.html
virtual/rootlayerscrolls/css3/filters/blur-filter-page-scroll.html

BUG=651793
==========

to

==========
Adding rootlayerscrolls-specific test expectations.

Adding rootlayerscrolls-specific test expectations for these test cases

virtual/rootlayerscrolls/css3/filters/blur-filter-page-scroll-parents.html
virtual/rootlayerscrolls/css3/filters/blur-filter-page-scroll-self.html
virtual/rootlayerscrolls/css3/filters/blur-filter-page-scroll.html

BUG=651793
==========

[commit-bot: I haz the power, 2016-10-24 21:00:10]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2016-10-24 21:04:15]

Description was changed from

==========
Adding rootlayerscrolls-specific test expectations.

Adding rootlayerscrolls-specific test expectations for these test cases

virtual/rootlayerscrolls/css3/filters/blur-filter-page-scroll-parents.html
virtual/rootlayerscrolls/css3/filters/blur-filter-page-scroll-self.html
virtual/rootlayerscrolls/css3/filters/blur-filter-page-scroll.html

BUG=651793
==========

to

==========
Adding rootlayerscrolls-specific test expectations.

Adding rootlayerscrolls-specific test expectations for these test cases

virtual/rootlayerscrolls/css3/filters/blur-filter-page-scroll-parents.html
virtual/rootlayerscrolls/css3/filters/blur-filter-page-scroll-self.html
virtual/rootlayerscrolls/css3/filters/blur-filter-page-scroll.html

BUG=651793

Committed: https://crrev.com/28c207c7b4d78a3f9d926d697b599878324069e5
Cr-Commit-Position: refs/heads/master@{#427145}
==========

[commit-bot: I haz the power, 2016-10-24 21:04:16]

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