Remove top controls clipping adjustment for document.rootScroller

Remove top controls clipping adjustment for document.rootScroller

This is basically a revert of
https://crrev.com/7ab964b30f73e3507ac3a424f56b1a9101b9bca3

Turns out that mucking with clips at the paint stage of the lifecycle is
problematic for document.rootScroller so I'm taking a different approach and
removing all the modifications to painting code.

BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2750463005
Cr-Commit-Position: refs/heads/master@{#459602}
Committed: https://chromium.googlesource.com/chromium/src/+/fa2b8f683b4ed28047f8bcb6f159b28478f68191

[bokan, 2017-03-14 14:17:54]

Description was changed from

==========
Remove top controls clipping adjustment for document.rootScroller

This is basically a revert of
https://crrev.com/7ab964b30f73e3507ac3a424f56b1a9101b9bca3

Turns out that mucking with clips at the paint stage of the lifecycle is
problematic for document.rootScroller so I'm taking a different approach and
removing all the modifications to painting code.

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

to

==========
Remove top controls clipping adjustment for document.rootScroller

This is basically a revert of
https://crrev.com/7ab964b30f73e3507ac3a424f56b1a9101b9bca3

Turns out that mucking with clips at the paint stage of the lifecycle is
problematic for document.rootScroller so I'm taking a different approach and
removing all the modifications to painting code.

BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[bokan, 2017-03-14 14:28:39]

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

[commit-bot: I haz the power, 2017-03-14 14:29: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-03-14 14:36:41]

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

[commit-bot: I haz the power, 2017-03-14 14:36:45]

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

[bokan, 2017-03-14 15:06:00]

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

[commit-bot: I haz the power, 2017-03-14 15:06:22]

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-03-14 16:05:31]

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

[commit-bot: I haz the power, 2017-03-14 16:05:32]

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

[bokan, 2017-03-14 17:48:34]

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

[commit-bot: I haz the power, 2017-03-14 17:49:13]

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-03-14 19:25:03]

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

[commit-bot: I haz the power, 2017-03-14 19:25:03]

Dry run: This issue passed the CQ dry run.

[bokan, 2017-03-14 19:28:04]

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

[bokan, 2017-03-14 19:28:05]

...and a second patch removing compositing hacks for document.rootScroller.

[bokan, 2017-03-16 12:17:36]

ping

[chrishtr, 2017-03-16 18:46:57]

https://codereview.chromium.org/2750463005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2750463005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1637:
layoutView.layer()->setNeedsCompositingInputsUpdate();
This is new since last time you landed - a new dependency on RLS?

[bokan, 2017-03-16 18:59:49]

https://codereview.chromium.org/2750463005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2750463005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1637:
layoutView.layer()->setNeedsCompositingInputsUpdate();
On 2017/03/16 18:46:57, chrishtr wrote:
> This is new since last time you landed - a new dependency on RLS?

So I'm hitting a DCHECK in RLS tests without this:

'[FATAL:FindPropertiesNeedingUpdate.h(160)] Check failed:
*m_originalProperties->overflowClip() == *objectProperties->overflowClip().
Property was updated without the layout object ("LayoutBlockFlow (positioned)
DIV id='stream-view' class='view'") needing a paint property update.

Either this check was added in that time or RLS started doing something new. It
seems that we're changing the clip rect on the layer without telling paint about
it (I believe the root layer's overflow clip uses the rect supplied from the
FrameView but we're missing some kind of invalidation). I suspect this may not
be the right fix?

[chrishtr, 2017-03-16 19:04:18]

chrishtr@chromium.org changed reviewers:
+ wangxianzhu@chromium.org

[chrishtr, 2017-03-16 19:04:19]

+Xianzhu to help with that latest question.

[Xianzhu, 2017-03-16 19:16:54]

https://codereview.chromium.org/2750463005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2750463005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1637:
layoutView.layer()->setNeedsCompositingInputsUpdate();
On 2017/03/16 18:59:48, bokan wrote:
> On 2017/03/16 18:46:57, chrishtr wrote:
> > This is new since last time you landed - a new dependency on RLS?
> 
> So I'm hitting a DCHECK in RLS tests without this:
> 
> '[FATAL:FindPropertiesNeedingUpdate.h(160)] Check failed:
> *m_originalProperties->overflowClip() == *objectProperties->overflowClip().
> Property was updated without the layout object ("LayoutBlockFlow (positioned)
> DIV id='stream-view' class='view'") needing a paint property update.
> 
> Either this check was added in that time or RLS started doing something new.
It
> seems that we're changing the clip rect on the layer without telling paint
about
> it (I believe the root layer's overflow clip uses the rect supplied from the
> FrameView but we're missing some kind of invalidation). I suspect this may not
> be the right fix?

I'm trying to understand the error, and have some questions:

- I know viewport size affects LayoutView's overflow clip with RLS, but how does
it affect overflow clip of a normal LayoutBlockFlow? Which test is it?

- This question is about "viewport size affects LayutView's overflow clip", : Is
LayoutView::overflowClipRect() equivalent to LayoutBox::overflowClipRect() with
RLS? If not, why is the difference?

[bokan, 2017-03-16 20:11:09]

https://codereview.chromium.org/2750463005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2750463005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1637:
layoutView.layer()->setNeedsCompositingInputsUpdate();
On 2017/03/16 19:16:54, Xianzhu wrote:
> On 2017/03/16 18:59:48, bokan wrote:
> > On 2017/03/16 18:46:57, chrishtr wrote:
> > > This is new since last time you landed - a new dependency on RLS?
> > 
> > So I'm hitting a DCHECK in RLS tests without this:
> > 
> > '[FATAL:FindPropertiesNeedingUpdate.h(160)] Check failed:
> > *m_originalProperties->overflowClip() == *objectProperties->overflowClip().
> > Property was updated without the layout object ("LayoutBlockFlow
(positioned)
> > DIV id='stream-view' class='view'") needing a paint property update.
> > 
> > Either this check was added in that time or RLS started doing something new.
> It
> > seems that we're changing the clip rect on the layer without telling paint
> about
> > it (I believe the root layer's overflow clip uses the rect supplied from the
> > FrameView but we're missing some kind of invalidation). I suspect this may
not
> > be the right fix?
> 
> I'm trying to understand the error, and have some questions:
> 
> - I know viewport size affects LayoutView's overflow clip with RLS, but how
does
> it affect overflow clip of a normal LayoutBlockFlow? Which test is it?

It shouldn't affect a regular LayoutBlockFlow at all, but perhaps I'm
misunderstanding - URL bar affects only the PaintLayerCompositor/LayoutView. In
LayoutView::overflowClipRect we use the viewRect which is really the FrameView's
visibleContentRect. That changes as the URL bar hides/shows. I think for non-RLS
we don't clip LayoutView at all which is why this doesn't trigger for non-RLS -
we never seem to read LayoutView::overflowClipRect.

These tests fail without this line:
   
All/ParameterizedWebFrameTest.SetForceZeroLayoutHeightWorksWithWrapContentMode/1
    All/VisualViewportTest.InvalidateLayoutViewWhenDocumentSmallerThanView/1 
    All/VisualViewportTest.ResizeNonCompositedAndFixedBackground/1 
    All/VisualViewportTest.ResizeNonFixedBackgroundNoLayoutOrInvalidation/1 

> - This question is about "viewport size affects LayutView's overflow clip", :
Is
> LayoutView::overflowClipRect() equivalent to LayoutBox::overflowClipRect()
with
> RLS? If not, why is the difference?

I believe not. The reason being that LayoutBox::overflowClipRect is basically
equivalent to the border box. The two will be the same while the URL bar is
showing. When the URL bar is hidden, we don't resize the ICB so the LayoutView's
border box remains unchanged while it's overflowClipRect will match the
FrameView visibleContentRect which is now expanded by the hidden URL bar.

[chrishtr, 2017-03-21 16:44:19]

Hi Xianzhu, do you have any more comments?

[Xianzhu, 2017-03-21 16:59:56]

https://codereview.chromium.org/2750463005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2750463005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1637:
layoutView.layer()->setNeedsCompositingInputsUpdate();
On 2017/03/16 20:11:09, bokan (OOO until Mar 22) wrote:
> On 2017/03/16 19:16:54, Xianzhu wrote:
> > On 2017/03/16 18:59:48, bokan wrote:
> > > On 2017/03/16 18:46:57, chrishtr wrote:
> > > > This is new since last time you landed - a new dependency on RLS?
> > > 
> > > So I'm hitting a DCHECK in RLS tests without this:
> > > 
> > > '[FATAL:FindPropertiesNeedingUpdate.h(160)] Check failed:
> > > *m_originalProperties->overflowClip() ==
*objectProperties->overflowClip().
> > > Property was updated without the layout object ("LayoutBlockFlow
> (positioned)
> > > DIV id='stream-view' class='view'") needing a paint property update.
> > > 
> > > Either this check was added in that time or RLS started doing something
new.
> > It
> > > seems that we're changing the clip rect on the layer without telling paint
> > about
> > > it (I believe the root layer's overflow clip uses the rect supplied from
the
> > > FrameView but we're missing some kind of invalidation). I suspect this may
> not
> > > be the right fix?
> > 
> > I'm trying to understand the error, and have some questions:
> > 
> > - I know viewport size affects LayoutView's overflow clip with RLS, but how
> does
> > it affect overflow clip of a normal LayoutBlockFlow? Which test is it?
> 
> It shouldn't affect a regular LayoutBlockFlow at all, but perhaps I'm
> misunderstanding - URL bar affects only the PaintLayerCompositor/LayoutView.
In
> LayoutView::overflowClipRect we use the viewRect which is really the
FrameView's
> visibleContentRect. That changes as the URL bar hides/shows. I think for
non-RLS
> we don't clip LayoutView at all which is why this doesn't trigger for non-RLS
-
> we never seem to read LayoutView::overflowClipRect.
> 
> These tests fail without this line:
>    
>
All/ParameterizedWebFrameTest.SetForceZeroLayoutHeightWorksWithWrapContentMode/1
>     All/VisualViewportTest.InvalidateLayoutViewWhenDocumentSmallerThanView/1 
>     All/VisualViewportTest.ResizeNonCompositedAndFixedBackground/1 
>     All/VisualViewportTest.ResizeNonFixedBackgroundNoLayoutOrInvalidation/1 

Did these tests all fail with error messages similar to the one you pasted? If
yes, it seems that viewport resize does affect a normal LayoutBlockFlow's
overflowClip. Can you make clear how this happened, and how
setNeedsCompositingInputsUpdate() fixes it?

[Xianzhu, 2017-03-21 17:26:41]

https://codereview.chromium.org/2750463005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2750463005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1637:
layoutView.layer()->setNeedsCompositingInputsUpdate();
On 2017/03/21 16:59:56, Xianzhu wrote:
> Did these tests all fail with error messages similar to the one you pasted? If
> yes, it seems that viewport resize does affect a normal LayoutBlockFlow's
> overflowClip. Can you make clear how this happened, and how
> setNeedsCompositingInputsUpdate() fixes it?

I just tried the patch and found that all the failures are about LayoutView's
paint property, not a "normal LayoutBlockFlow" like in the pasted error message.
I think my questions are resolved.

I think we should put setNeedsPaintPropertyUpdate() here, in addition to
setNeedsCompositingInputsUpdate(). The latter may imply the former for SPv1 but
may not for SPv2.

[bokan, 2017-03-23 15:09:58]

https://codereview.chromium.org/2750463005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2750463005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1637:
layoutView.layer()->setNeedsCompositingInputsUpdate();
On 2017/03/21 17:26:41, Xianzhu wrote:
> On 2017/03/21 16:59:56, Xianzhu wrote:
> > Did these tests all fail with error messages similar to the one you pasted?
If
> > yes, it seems that viewport resize does affect a normal LayoutBlockFlow's
> > overflowClip. Can you make clear how this happened, and how
> > setNeedsCompositingInputsUpdate() fixes it?
> 
> I just tried the patch and found that all the failures are about LayoutView's
> paint property, not a "normal LayoutBlockFlow" like in the pasted error
message.
> I think my questions are resolved.
> 
> I think we should put setNeedsPaintPropertyUpdate() here, in addition to
> setNeedsCompositingInputsUpdate(). The latter may imply the former for SPv1
but
> may not for SPv2.

Yeah, sorry, I think I may have pasted that DCHECK from a separate failure that
I was having in a followup patch that I thought was the same.

I've added setNeedsPaintPropertyUpdate. Should it be guarded by
slimmingPaintInvalidationEnabled()?

[Xianzhu, 2017-03-23 15:39:16]

lgtm

https://codereview.chromium.org/2750463005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2750463005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1637:
layoutView.layer()->setNeedsCompositingInputsUpdate();
On 2017/03/23 15:09:57, bokan wrote:
> On 2017/03/21 17:26:41, Xianzhu wrote:
> > On 2017/03/21 16:59:56, Xianzhu wrote:
> > > Did these tests all fail with error messages similar to the one you
pasted?
> If
> > > yes, it seems that viewport resize does affect a normal LayoutBlockFlow's
> > > overflowClip. Can you make clear how this happened, and how
> > > setNeedsCompositingInputsUpdate() fixes it?
> > 
> > I just tried the patch and found that all the failures are about
LayoutView's
> > paint property, not a "normal LayoutBlockFlow" like in the pasted error
> message.
> > I think my questions are resolved.
> > 
> > I think we should put setNeedsPaintPropertyUpdate() here, in addition to
> > setNeedsCompositingInputsUpdate(). The latter may imply the former for SPv1
> but
> > may not for SPv2.
> 
> Yeah, sorry, I think I may have pasted that DCHECK from a separate failure
that
> I was having in a followup patch that I thought was the same.
> 
> I've added setNeedsPaintPropertyUpdate. Should it be guarded by
> slimmingPaintInvalidationEnabled()?

It's better to be guarded to be consistent with other places.

[bokan, 2017-03-23 16:01:03]

https://codereview.chromium.org/2750463005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2750463005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1637:
layoutView.layer()->setNeedsCompositingInputsUpdate();
On 2017/03/23 15:39:16, Xianzhu wrote:
> On 2017/03/23 15:09:57, bokan wrote:
> > On 2017/03/21 17:26:41, Xianzhu wrote:
> > > On 2017/03/21 16:59:56, Xianzhu wrote:
> > > > Did these tests all fail with error messages similar to the one you
> pasted?
> > If
> > > > yes, it seems that viewport resize does affect a normal
LayoutBlockFlow's
> > > > overflowClip. Can you make clear how this happened, and how
> > > > setNeedsCompositingInputsUpdate() fixes it?
> > > 
> > > I just tried the patch and found that all the failures are about
> LayoutView's
> > > paint property, not a "normal LayoutBlockFlow" like in the pasted error
> > message.
> > > I think my questions are resolved.
> > > 
> > > I think we should put setNeedsPaintPropertyUpdate() here, in addition to
> > > setNeedsCompositingInputsUpdate(). The latter may imply the former for
SPv1
> > but
> > > may not for SPv2.
> > 
> > Yeah, sorry, I think I may have pasted that DCHECK from a separate failure
> that
> > I was having in a followup patch that I thought was the same.
> > 
> > I've added setNeedsPaintPropertyUpdate. Should it be guarded by
> > slimmingPaintInvalidationEnabled()?
> 
> It's better to be guarded to be consistent with other places.

Done.

[bokan, 2017-03-24 17:38:01]

Chris, is this ok now?

[chrishtr, 2017-03-24 21:22:00]

lgtm

[bokan, 2017-03-24 22:16:53]

The CQ bit was checked by bokan@chromium.org

[bokan, 2017-03-24 22:16:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
wangxianzhu@chromium.org, chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/2750463005/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-24 22:17:27]

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-03-24 23:51:08]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490393813652660,
"parent_rev": "e1f56b7e3d7cc916df8a22831638ec1c4041ca7d", "commit_rev":
"fa2b8f683b4ed28047f8bcb6f159b28478f68191"}

[commit-bot: I haz the power, 2017-03-24 23:51:54]

Description was changed from

==========
Remove top controls clipping adjustment for document.rootScroller

This is basically a revert of
https://crrev.com/7ab964b30f73e3507ac3a424f56b1a9101b9bca3

Turns out that mucking with clips at the paint stage of the lifecycle is
problematic for document.rootScroller so I'm taking a different approach and
removing all the modifications to painting code.

BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Remove top controls clipping adjustment for document.rootScroller

This is basically a revert of
https://crrev.com/7ab964b30f73e3507ac3a424f56b1a9101b9bca3

Turns out that mucking with clips at the paint stage of the lifecycle is
problematic for document.rootScroller so I'm taking a different approach and
removing all the modifications to painting code.

BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2750463005
Cr-Commit-Position: refs/heads/master@{#459602}
Committed:
https://chromium.googlesource.com/chromium/src/+/fa2b8f683b4ed28047f8bcb6f159...
==========

[commit-bot: I haz the power, 2017-03-24 23:51:55]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/fa2b8f683b4ed28047f8bcb6f159...