Moved parent_->SchedulePaint() to Views::SetFillsBoundsOpaquely() and Views::SchedulePaint().

This CL fixes a more general issue in Views that had a quick fix
that landed here: https://codereview.chromium.org/1460733002/

The general problem is that the layer owned by a parent View can
become dirty/stale when a child view is painting to it's own layer.
This CL fixes the problem by scheduling paints on the parent view
when it may be dirty.

BUG=551492, 546683
TEST=ViewLayerTest.NoCrashWhenParentlessViewSchedulesPaintOnParent
     ViewLayerTest.ScheduledRectsInParentAfterSchedulingPaint
     ViewLayerTest.ParentPaintWhenSwitchingPaintToLayerFromFalseToTrue
     ViewLayerTest.NoParentPaintWhenSwitchingPaintToLayerFromTrueToTrue

Committed: https://crrev.com/c8761106916aac604f30fb2223cdbfb2bd3ffda3
Cr-Commit-Position: refs/heads/master@{#367536}

[bruthig, 2015-11-18 16:48:42]

Description was changed from

==========
Moved parent_->SchedulePaint() to Views::SetFillsBoundsOpaquely() and
Views::SchedulePaint().
==========

to

==========
Moved parent_->SchedulePaint() to Views::SetFillsBoundsOpaquely() and
Views::SchedulePaint().

Proposed changes to CL: https://codereview.chromium.org/1460733002/
==========

[bruthig, 2015-11-18 16:48:42]

bruthig@chromium.org changed reviewers:
+ sadrul@chromium.org

[jonross, 2015-11-18 16:55:44]

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

[jonross, 2015-11-18 16:55:45]

https://codereview.chromium.org/1461673002/diff/40001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1461673002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:730: if (parent_ && (!layer() ||
!layer()->fills_bounds_opaquely())) {
This will trigger for all schedule paints on a non-opaque layer.

We really need it for the initial transition where the parent is dirty.
Subsequently parent and child should track when they get dirty.

[bruthig, 2015-11-18 17:02:48]

https://codereview.chromium.org/1461673002/diff/40001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1461673002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:730: if (parent_ && (!layer() ||
!layer()->fills_bounds_opaquely())) {
On 2015/11/18 16:55:45, jonross wrote:
> This will trigger for all schedule paints on a non-opaque layer.
> 
> We really need it for the initial transition where the parent is dirty.
> Subsequently parent and child should track when they get dirty.

Agreed, I just want to make sure the following scenario is addressed then.

If the non-opaque child layer changes what is drawn, and more specifically
changes some previously opaque pixles to now be transparent, then what mechanism
causes the newly transparent pixels to be 'filled' in with the parents pixels?
In other words, the parent never became dirty here, but it does indeed need to
be painted again.

[bruthig, 2015-11-18 17:46:43]

jonross@, I've updated the CL to only schedule paints on the parent when "paint
to layer" and "set fills bounds opaquely" transition.  Can you PTAL?

https://codereview.chromium.org/1461673002/diff/40001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1461673002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:730: if (parent_ && (!layer() ||
!layer()->fills_bounds_opaquely())) {
On 2015/11/18 17:02:48, bruthig wrote:
> On 2015/11/18 16:55:45, jonross wrote:
> > This will trigger for all schedule paints on a non-opaque layer.
> > 
> > We really need it for the initial transition where the parent is dirty.
> > Subsequently parent and child should track when they get dirty.
> 
> Agreed, I just want to make sure the following scenario is addressed then.
> 
> If the non-opaque child layer changes what is drawn, and more specifically
> changes some previously opaque pixles to now be transparent, then what
mechanism
> causes the newly transparent pixels to be 'filled' in with the parents pixels?
> In other words, the parent never became dirty here, but it does indeed need to
> be painted again.

Discussed offline.

[jonross, 2015-11-18 18:37:22]

On 2015/11/18 17:46:43, bruthig wrote:
> jonross@, I've updated the CL to only schedule paints on the parent when
"paint
> to layer" and "set fills bounds opaquely" transition.  Can you PTAL?
> 
> https://codereview.chromium.org/1461673002/diff/40001/ui/views/view.cc
> File ui/views/view.cc (right):
> 
>
https://codereview.chromium.org/1461673002/diff/40001/ui/views/view.cc#newcod...
> ui/views/view.cc:730: if (parent_ && (!layer() ||
> !layer()->fills_bounds_opaquely())) {
> On 2015/11/18 17:02:48, bruthig wrote:
> > On 2015/11/18 16:55:45, jonross wrote:
> > > This will trigger for all schedule paints on a non-opaque layer.
> > > 
> > > We really need it for the initial transition where the parent is dirty.
> > > Subsequently parent and child should track when they get dirty.
> > 
> > Agreed, I just want to make sure the following scenario is addressed then.
> > 
> > If the non-opaque child layer changes what is drawn, and more specifically
> > changes some previously opaque pixles to now be transparent, then what
> mechanism
> > causes the newly transparent pixels to be 'filled' in with the parents
pixels?
> > In other words, the parent never became dirty here, but it does indeed need
to
> > be painted again.
> 
> Discussed offline.

LGTM

[sadrul, 2015-11-19 22:26:15]

sadrul@chromium.org changed reviewers:
+ sky@chromium.org

[sadrul, 2015-11-19 22:26:16]

+sky@

Looks good in general. Can this have a test?

[sky, 2015-11-19 22:31:49]

https://codereview.chromium.org/1461673002/diff/60001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1461673002/diff/60001/ui/views/view.cc#newcod...
ui/views/view.cc:464: SchedulePaintOnParent();
Why does this case necessitate a paint on the parent? A comment would help here.

https://codereview.chromium.org/1461673002/diff/60001/ui/views/view.cc#newcod...
ui/views/view.cc:1389: bool schedule_paint_on_parent =
Please add an early return to make this code easier to reason about, eg:

if (!layer() || layer()->fills_bounds_opaquely() == fills_...)
  return;

[sky, 2015-11-19 22:32:06]

And agree with Sadrul that test coverage would be nice.

[bruthig, 2015-12-22 21:50:56]

Description was changed from

==========
Moved parent_->SchedulePaint() to Views::SetFillsBoundsOpaquely() and
Views::SchedulePaint().

Proposed changes to CL: https://codereview.chromium.org/1460733002/
==========

to

==========
This CL fixes a more general issue in Views that had a quick fix
that landed here: https://codereview.chromium.org/1460733002/

The general problem is that the layer owned by a parent View can
become dirty/stale when a child view is painting to it's own layer.
This CL fixes the problem by scheduling paints on the parent view
when it may be dirty.

BUG=551492, 546683
TEST=ViewLayerTest.NoCrashWhenParentlessViewSchedulesPaintOnParent,
     ViewLayerTest.ScheduledRectsInParentAfterSchedulingPaint,
     ViewLayerTest.ParentPaintWhenSwitchingPaintToLayerFromFalseToTrue,
     ViewLayerTest.NoParentPaintWhenSwitchingPaintToLayerFromTrueToTrue,
     ViewLayerTest.ParentPaintWhenSwitchingFillBoundsOpaquelyFromTrueToFalse,
     ViewLayerTest.ParentPaintWhenSwitchingFillBoundsOpaquelyFromFalseToTrue
==========

[bruthig, 2015-12-22 22:32:40]

I have added some tests.

sky@, sadrul@ can you PTAL?

https://codereview.chromium.org/1461673002/diff/60001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1461673002/diff/60001/ui/views/view.cc#newcod...
ui/views/view.cc:464: SchedulePaintOnParent();
On 2015/11/19 22:31:48, sky OOO until 28th wrote:
> Why does this case necessitate a paint on the parent? A comment would help
here.

Done.

https://codereview.chromium.org/1461673002/diff/60001/ui/views/view.cc#newcod...
ui/views/view.cc:1389: bool schedule_paint_on_parent =
On 2015/11/19 22:31:48, sky OOO until 28th wrote:
> Please add an early return to make this code easier to reason about, eg:
> 
> if (!layer() || layer()->fills_bounds_opaquely() == fills_...)
>   return;

Done.

[sky, 2015-12-27 23:55:04]

https://codereview.chromium.org/1461673002/diff/100001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1461673002/diff/100001/ui/views/view.cc#newco...
ui/views/view.cc:472: SchedulePaintOnParent();
I think you should move this to CreateLayer, just as DestoryLayer does the
paint.

https://codereview.chromium.org/1461673002/diff/100001/ui/views/view.cc#newco...
ui/views/view.cc:1430: SchedulePaintOnParent();
I'm not following why this needed. If it is needed, why isn't it always needed?

[bruthig, 2016-01-04 19:00:15]

sky@, PTAL

https://codereview.chromium.org/1461673002/diff/100001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1461673002/diff/100001/ui/views/view.cc#newco...
ui/views/view.cc:472: SchedulePaintOnParent();
On 2015/12/27 23:55:04, sky wrote:
> I think you should move this to CreateLayer, just as DestoryLayer does the
> paint.

Done.

https://codereview.chromium.org/1461673002/diff/100001/ui/views/view.cc#newco...
ui/views/view.cc:1430: SchedulePaintOnParent();
On 2015/12/27 23:55:04, sky wrote:
> I'm not following why this needed. If it is needed, why isn't it always
needed?

Removed, I talked with ajuma@ about layers and this should not be required.

[bruthig, 2016-01-04 21:48:29]

Description was changed from

==========
This CL fixes a more general issue in Views that had a quick fix
that landed here: https://codereview.chromium.org/1460733002/

The general problem is that the layer owned by a parent View can
become dirty/stale when a child view is painting to it's own layer.
This CL fixes the problem by scheduling paints on the parent view
when it may be dirty.

BUG=551492, 546683
TEST=ViewLayerTest.NoCrashWhenParentlessViewSchedulesPaintOnParent,
     ViewLayerTest.ScheduledRectsInParentAfterSchedulingPaint,
     ViewLayerTest.ParentPaintWhenSwitchingPaintToLayerFromFalseToTrue,
     ViewLayerTest.NoParentPaintWhenSwitchingPaintToLayerFromTrueToTrue,
     ViewLayerTest.ParentPaintWhenSwitchingFillBoundsOpaquelyFromTrueToFalse,
     ViewLayerTest.ParentPaintWhenSwitchingFillBoundsOpaquelyFromFalseToTrue
==========

to

==========
This CL fixes a more general issue in Views that had a quick fix
that landed here: https://codereview.chromium.org/1460733002/

The general problem is that the layer owned by a parent View can
become dirty/stale when a child view is painting to it's own layer.
This CL fixes the problem by scheduling paints on the parent view
when it may be dirty.

BUG=551492, 546683
TEST=ViewLayerTest.NoCrashWhenParentlessViewSchedulesPaintOnParent
     ViewLayerTest.ScheduledRectsInParentAfterSchedulingPaint
     ViewLayerTest.ParentPaintWhenSwitchingPaintToLayerFromFalseToTrue
     ViewLayerTest.NoParentPaintWhenSwitchingPaintToLayerFromTrueToTrue
==========

[bruthig, 2016-01-04 21:48:45]

sky@, PTAL

[sky, 2016-01-04 22:45:57]

LGTM - thanks for the comment!

[bruthig, 2016-01-05 15:08:36]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2016-01-05 15:08:36]

The patchset sent to the CQ was uploaded after l-g-t-m from jonross@chromium.org
Link to the patchset: https://codereview.chromium.org/1461673002/#ps140001
(title: "Changed doc comment in View::CreateLayer().")

[commit-bot: I haz the power, 2016-01-05 15:08:49]

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

[commit-bot: I haz the power, 2016-01-05 15:47:19]

Description was changed from

==========
This CL fixes a more general issue in Views that had a quick fix
that landed here: https://codereview.chromium.org/1460733002/

The general problem is that the layer owned by a parent View can
become dirty/stale when a child view is painting to it's own layer.
This CL fixes the problem by scheduling paints on the parent view
when it may be dirty.

BUG=551492, 546683
TEST=ViewLayerTest.NoCrashWhenParentlessViewSchedulesPaintOnParent
     ViewLayerTest.ScheduledRectsInParentAfterSchedulingPaint
     ViewLayerTest.ParentPaintWhenSwitchingPaintToLayerFromFalseToTrue
     ViewLayerTest.NoParentPaintWhenSwitchingPaintToLayerFromTrueToTrue
==========

to

==========
This CL fixes a more general issue in Views that had a quick fix
that landed here: https://codereview.chromium.org/1460733002/

The general problem is that the layer owned by a parent View can
become dirty/stale when a child view is painting to it's own layer.
This CL fixes the problem by scheduling paints on the parent view
when it may be dirty.

BUG=551492, 546683
TEST=ViewLayerTest.NoCrashWhenParentlessViewSchedulesPaintOnParent
     ViewLayerTest.ScheduledRectsInParentAfterSchedulingPaint
     ViewLayerTest.ParentPaintWhenSwitchingPaintToLayerFromFalseToTrue
     ViewLayerTest.NoParentPaintWhenSwitchingPaintToLayerFromTrueToTrue
==========

[commit-bot: I haz the power, 2016-01-05 15:47:21]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-01-05 15:48:20]

Description was changed from

==========
This CL fixes a more general issue in Views that had a quick fix
that landed here: https://codereview.chromium.org/1460733002/

The general problem is that the layer owned by a parent View can
become dirty/stale when a child view is painting to it's own layer.
This CL fixes the problem by scheduling paints on the parent view
when it may be dirty.

BUG=551492, 546683
TEST=ViewLayerTest.NoCrashWhenParentlessViewSchedulesPaintOnParent
     ViewLayerTest.ScheduledRectsInParentAfterSchedulingPaint
     ViewLayerTest.ParentPaintWhenSwitchingPaintToLayerFromFalseToTrue
     ViewLayerTest.NoParentPaintWhenSwitchingPaintToLayerFromTrueToTrue
==========

to

==========
This CL fixes a more general issue in Views that had a quick fix
that landed here: https://codereview.chromium.org/1460733002/

The general problem is that the layer owned by a parent View can
become dirty/stale when a child view is painting to it's own layer.
This CL fixes the problem by scheduling paints on the parent view
when it may be dirty.

BUG=551492, 546683
TEST=ViewLayerTest.NoCrashWhenParentlessViewSchedulesPaintOnParent
     ViewLayerTest.ScheduledRectsInParentAfterSchedulingPaint
     ViewLayerTest.ParentPaintWhenSwitchingPaintToLayerFromFalseToTrue
     ViewLayerTest.NoParentPaintWhenSwitchingPaintToLayerFromTrueToTrue

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

[commit-bot: I haz the power, 2016-01-05 15:48:21]

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

[Ken Rockot(use gerrit already), 2016-01-05 17:41:42]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1560723002/ by rockot@chromium.org.

The reason for reverting is: These tests leak

https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2....

[bruthig, 2016-01-06 16:37:33]

Re-landed here: https://codereview.chromium.org/1566693002/