Update SetPaintToLayer to accept LayerType

Update SetPaintToLayer to accept LayerType

SetPaintToLayer is used to create layer for a specific view. It used to accept a
bool variable and always create a texture layer, which is very expensive because 
texture layer needs to be re-raster every time. It can now accept |layer_type| 
as input. Note that subsequent CLs will use this to create Views with 
ui::LAYER_SOLID_COLOR layers. 

BUG=680536
Review-Url: https://codereview.chromium.org/2639203007
Cr-Commit-Position: refs/heads/master@{#445940}
Committed: https://chromium.googlesource.com/chromium/src/+/0338037a696b84bb66efe37eee1cb16bdb113453

[yiyix, 2017-01-19 22:27:38]

Description was changed from

==========
Refactor

BUG=680536
==========

to

==========
Update SetPaintToLayer to accept LayerType

SetPaintToLayer is used to create layer for a specific view. It used to accept a
bool variable and always create a texture layer, which is very expensive because

texture layer needs to be re-raster every time. It can now accept |layer_type| 
and create such a layer.

BUG=680536
==========

[yiyix, 2017-01-19 22:28:32]

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

[commit-bot: I haz the power, 2017-01-19 22:29: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-01-19 22:33:38]

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

[commit-bot: I haz the power, 2017-01-19 22:33:38]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yiyix, 2017-01-20 00:52:54]

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

[yiyix, 2017-01-20 00:53:42]

Description was changed from

==========
Update SetPaintToLayer to accept LayerType

SetPaintToLayer is used to create layer for a specific view. It used to accept a
bool variable and always create a texture layer, which is very expensive because

texture layer needs to be re-raster every time. It can now accept |layer_type| 
and create such a layer.

BUG=680536
==========

to

==========
Update SetPaintToLayer to accept LayerType

SetPaintToLayer is used to create layer for a specific view. It used to accept a
bool variable and always create a texture layer, which is very expensive because

texture layer needs to be re-raster every time. It can now accept |layer_type| 
as input.

BUG=680536
==========

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

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-01-20 02:00:22]

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

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

Dry run: This issue passed the CQ dry run.

[yiyix, 2017-01-20 02:10:31]

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

[sadrul, 2017-01-20 19:50:11]

You should mention in the CL desc. that subsequent CLs will use this to create
Views with ui::LAYER_SOLID_COLOR layers. (and if you found any existing Views
that could use a different layer-type, please make sure to file separate bugs
for those too)

lgtm

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

https://codereview.chromium.org/2639203007/diff/20001/ui/views/view.cc#newcod...
ui/views/view.cc:534: bool is_paint_layer = !(layer_type ==
ui::LAYER_NOT_DRAWN);
layer_type != ui::LAYER_NOT_DRAW

[bruthig, 2017-01-20 20:14:17]

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

[bruthig, 2017-01-20 20:14:18]

https://codereview.chromium.org/2639203007/diff/20001/ui/views/animation/ink_...
File ui/views/animation/ink_drop.cc (right):

https://codereview.chromium.org/2639203007/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop.cc:14: SetPaintToLayer(ui::LAYER_TEXTURED);
nit: This may not need to be a textured layer.  A ui::LAYER_NOT_DRAWN should be
sufficient.  Assuming child Views will correctly paint to a textured layer owned
by an ancestor of this.

It would be preferable from an InkDrop standpoint for the API to allow Views to
have a LAYER_NOT_DRAWN Layer but I'm not sure if there are potential issues with
that (e.g. painting).

Sadrul, any insight?

[sky, 2017-01-20 20:32:07]

https://codereview.chromium.org/2639203007/diff/20001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/2639203007/diff/20001/ui/views/view.h#newcode342
ui/views/view.h:342: void SetPaintToLayer(ui::LayerType layer_type);
WDYT of a default of TEXTURED as that is typically what consumers want?

[sadrul, 2017-01-20 20:32:52]

https://codereview.chromium.org/2639203007/diff/20001/ui/views/animation/ink_...
File ui/views/animation/ink_drop.cc (right):

https://codereview.chromium.org/2639203007/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop.cc:14: SetPaintToLayer(ui::LAYER_TEXTURED);
On 2017/01/20 20:14:18, bruthig wrote:
> nit: This may not need to be a textured layer.  A ui::LAYER_NOT_DRAWN should
be
> sufficient.  Assuming child Views will correctly paint to a textured layer
owned
> by an ancestor of this.
> 
> It would be preferable from an InkDrop standpoint for the API to allow Views
to
> have a LAYER_NOT_DRAWN Layer but I'm not sure if there are potential issues
with
> that (e.g. painting).
> 
> Sadrul, any insight?

I think for cases like this, we can probably do a SOLID_COLOR with color ==
transparent? I think that's better than the alternative (SetPaintLayer(bool,
layer-type)).

But ...

> Assuming child Views will correctly paint to a textured layer owned
> by an ancestor of this.

What does this mean? Child views of InkDropContainerView (IDCV) should paint in
their own layers, or in the layer of IDCV. They should not pain in IDCV's
ancestor view's layer.

[yiyix, 2017-01-20 20:34:17]

Patchset #1 (id:1) has been deleted

[yiyix, 2017-01-20 20:58:28]

On 2017/01/20 20:32:07, sky wrote:
> https://codereview.chromium.org/2639203007/diff/20001/ui/views/view.h
> File ui/views/view.h (right):
> 
>
https://codereview.chromium.org/2639203007/diff/20001/ui/views/view.h#newcode342
> ui/views/view.h:342: void SetPaintToLayer(ui::LayerType layer_type);
> WDYT of a default of TEXTURED as that is typically what consumers want?

Originally, I wanted force the users to specify a |layer_type| so they need to
think what they want to do.
I agree that |LAYER_TEXTURED| is the most common usage. I can add it as the
default value.

[yiyix, 2017-01-20 21:03:08]

Description was changed from

==========
Update SetPaintToLayer to accept LayerType

SetPaintToLayer is used to create layer for a specific view. It used to accept a
bool variable and always create a texture layer, which is very expensive because

texture layer needs to be re-raster every time. It can now accept |layer_type| 
as input.

BUG=680536
==========

to

==========
Update SetPaintToLayer to accept LayerType

SetPaintToLayer is used to create layer for a specific view. It used to accept a
bool variable and always create a texture layer, which is very expensive because

texture layer needs to be re-raster every time. It can now accept |layer_type| 
as input. Note that subsequent CLs will use this to create Views with 
ui::LAYER_SOLID_COLOR layers. 

BUG=680536
==========

[yiyix, 2017-01-22 05:07:47]

Patchset #2 (id:40001) has been deleted

[yiyix, 2017-01-22 05:22:13]

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

[commit-bot: I haz the power, 2017-01-22 05:22:16]

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-01-22 06:11:36]

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

[commit-bot: I haz the power, 2017-01-22 06:11:37]

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

[yiyix, 2017-01-23 06:18:01]

Patchset #2 (id:60001) has been deleted

[yiyix, 2017-01-23 06:18:10]

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

[commit-bot: I haz the power, 2017-01-23 06:18:21]

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-01-23 07:15:37]

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

[commit-bot: I haz the power, 2017-01-23 07:15:38]

Dry run: This issue passed the CQ dry run.

[yiyix, 2017-01-23 15:20:46]

@sky, sadrul, bruthig, could you please take a look at this change again? Thank
you.

[bruthig, 2017-01-23 16:25:02]

https://codereview.chromium.org/2639203007/diff/80001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_host_view.cc (right):

https://codereview.chromium.org/2639203007/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_host_view.cc:152: SetPaintToLayer();
nit: It's unnecessary to call SetPaintToLayer() here.  I think you only need to
handle the else case here.

[yiyix, 2017-01-23 16:49:49]

Updated calls. Thanks, @bruthig.

@sky, sadrul, could you please take a look?

https://codereview.chromium.org/2639203007/diff/80001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_host_view.cc (right):

https://codereview.chromium.org/2639203007/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_host_view.cc:152: SetPaintToLayer();
On 2017/01/23 16:25:02, bruthig wrote:
> nit: It's unnecessary to call SetPaintToLayer() here.  I think you only need
to
> handle the else case here.

Thanks, I didn't really read this code. The create layer is handled in
AddInkDropLayer and destroy layer is handled in this function
RemoveInkDropLayer. I will update it.

[yiyix, 2017-01-23 16:50:07]

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

[commit-bot: I haz the power, 2017-01-23 16:50:17]

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

[sky, 2017-01-23 17:10:08]

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

https://codereview.chromium.org/2639203007/diff/100001/ui/views/view.cc#newco...
ui/views/view.cc:533: if (paint_to_layer_)
Shouldn't this also check the type? By that I mean if the type changes shouldn't
you recreate the layer?

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

https://codereview.chromium.org/2639203007/diff/100001/ui/views/view.h#newcod...
ui/views/view.h:343: void DetachLayer();
DetachLayer doesn't convey this destroys the layer. OTOH DestroyLayer or
DeleteLayer() better describe what this does. Please name this DestroyLayer()
and move the the implementation of View::DestroyLayer() to match new position.

[commit-bot: I haz the power, 2017-01-23 17:30:04]

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

[commit-bot: I haz the power, 2017-01-23 17:30:05]

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

[yiyix, 2017-01-23 20:27:26]

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

[commit-bot: I haz the power, 2017-01-23 20:27:52]

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

[yiyix, 2017-01-23 20:28:41]

@sky, could you please take a look at this version? Thank you.

[sky, 2017-01-23 21:21:06]

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

https://codereview.chromium.org/2639203007/diff/120001/ui/views/view.cc#newco...
ui/views/view.cc:537: if (layer()) {
Reduce the conditional to:

DestroyLayer(); (it handles the null checks already).
CreateLayer(layer_type);

https://codereview.chromium.org/2639203007/diff/120001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/2639203007/diff/120001/ui/views/view.h#newcod...
ui/views/view.h:343: // to the destroyed layer's parent.
Document this has no effect if the view does not currently have a layer.

[commit-bot: I haz the power, 2017-01-23 21:48:20]

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

[commit-bot: I haz the power, 2017-01-23 21:48:21]

Dry run: This issue passed the CQ dry run.

[yiyix, 2017-01-23 21:56:05]

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

[commit-bot: I haz the power, 2017-01-23 21:56:59]

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-01-23 22:55:39]

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

[commit-bot: I haz the power, 2017-01-23 22:55:40]

Dry run: This issue passed the CQ dry run.

[yiyix, 2017-01-24 03:14:54]

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

https://codereview.chromium.org/2639203007/diff/120001/ui/views/view.cc#newco...
ui/views/view.cc:537: if (layer()) {
On 2017/01/23 21:21:05, sky wrote:
> Reduce the conditional to:
> 
> DestroyLayer(); (it handles the null checks already).
> CreateLayer(layer_type);

You are right. Sorry, i should thought about it.

https://codereview.chromium.org/2639203007/diff/120001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/2639203007/diff/120001/ui/views/view.h#newcod...
ui/views/view.h:343: // to the destroyed layer's parent.
On 2017/01/23 21:21:05, sky wrote:
> Document this has no effect if the view does not currently have a layer.

Thanks, it is good to include in the description.

[yiyix, 2017-01-24 03:15:18]

On 2017/01/24 03:14:54, yiyix wrote:
> https://codereview.chromium.org/2639203007/diff/120001/ui/views/view.cc
> File ui/views/view.cc (right):
> 
>
https://codereview.chromium.org/2639203007/diff/120001/ui/views/view.cc#newco...
> ui/views/view.cc:537: if (layer()) {
> On 2017/01/23 21:21:05, sky wrote:
> > Reduce the conditional to:
> > 
> > DestroyLayer(); (it handles the null checks already).
> > CreateLayer(layer_type);
> 
> You are right. Sorry, i should thought about it.
> 
> https://codereview.chromium.org/2639203007/diff/120001/ui/views/view.h
> File ui/views/view.h (right):
> 
>
https://codereview.chromium.org/2639203007/diff/120001/ui/views/view.h#newcod...
> ui/views/view.h:343: // to the destroyed layer's parent.
> On 2017/01/23 21:21:05, sky wrote:
> > Document this has no effect if the view does not currently have a layer.
> 
> Thanks, it is good to include in the description.

@sky, could you take a look at this change?

[sky, 2017-01-24 17:47:52]

LGTM

https://codereview.chromium.org/2639203007/diff/140001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/2639203007/diff/140001/ui/views/view.h#newcod...
ui/views/view.h:344: // layer, this takes no effect.
takes -> has

[yiyix, 2017-01-24 22:26:01]

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

[commit-bot: I haz the power, 2017-01-24 22:26:30]

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-01-24 23:44:46]

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

[commit-bot: I haz the power, 2017-01-24 23:44:47]

Dry run: This issue passed the CQ dry run.

[yiyix, 2017-01-25 03:56:35]

The CQ bit was checked by yiyix@chromium.org

[yiyix, 2017-01-25 03:56:36]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2639203007/#ps160001
(title: "fix comments")

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

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-01-25 04:05:16]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1485316595715650,
"parent_rev": "d4dac1e3b1f3620c1db4aaf432ccfa0f9e144b3b", "commit_rev":
"0338037a696b84bb66efe37eee1cb16bdb113453"}

[commit-bot: I haz the power, 2017-01-25 04:06:30]

Description was changed from

==========
Update SetPaintToLayer to accept LayerType

SetPaintToLayer is used to create layer for a specific view. It used to accept a
bool variable and always create a texture layer, which is very expensive because

texture layer needs to be re-raster every time. It can now accept |layer_type| 
as input. Note that subsequent CLs will use this to create Views with 
ui::LAYER_SOLID_COLOR layers. 

BUG=680536
==========

to

==========
Update SetPaintToLayer to accept LayerType

SetPaintToLayer is used to create layer for a specific view. It used to accept a
bool variable and always create a texture layer, which is very expensive because

texture layer needs to be re-raster every time. It can now accept |layer_type| 
as input. Note that subsequent CLs will use this to create Views with 
ui::LAYER_SOLID_COLOR layers. 

BUG=680536

Review-Url: https://codereview.chromium.org/2639203007
Cr-Commit-Position: refs/heads/master@{#445940}
Committed:
https://chromium.googlesource.com/chromium/src/+/0338037a696b84bb66efe37eee1c...
==========

[commit-bot: I haz the power, 2017-01-25 04:06:31]

Committed patchset #6 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/0338037a696b84bb66efe37eee1c...

[varkha, 2017-01-31 19:47:45]

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

[varkha, 2017-01-31 19:47:47]

https://codereview.chromium.org/2639203007/diff/160001/ui/views/animation/ink...
File ui/views/animation/ink_drop_host_view.cc (right):

https://codereview.chromium.org/2639203007/diff/160001/ui/views/animation/ink...
ui/views/animation/ink_drop_host_view.cc:132: old_paint_to_layer_ = layer() !=
nullptr;
Drive-by after the fact. I think we should add a DCHECK(!layer() ||
layer()->type() == ui::LAYER_TEXTURED) here. Currently if the IDHV already has a
non-texture layer I suspect bad things could happen, e.g. we won't be restoring
the correct layer type in InkDropHostView::RemoveInkDropLayer() (and I don't
know if simply "upgrading" a layer to texture and then "downgrading" it back to
say SOLID_COLOR is a good idea either).