CC: Plumb LayerSettings parameter for cc::Layer construction.

CC: Plumb LayerSettings parameter for cc::Layer construction.

Next episode:
https://codereview.chromium.org/1101823002/

This is a prerequisite to implement compositor animation timelines.
https://codereview.chromium.org/947033002/

BUG=394777
R=ajuma@chromium.org
R=vollick@chromium.org
R=piman@chromium.org
R=danakj@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[danakj, 2015-05-12 17:33:02]

I was expecting a LayerSettings like you had the bool before, where we set it
once somewhere and cc::Layer just uses it internally, rather than passing to the
constructor.

Or, if it was passed to the constructor, then there'd be no need for a global in
cc/layers/layer.cc, since they can access it there.

This patch is a bit of a combination of both, which is a bit weird to me. I'm
having a bit of trouble seeing the point of both. If RenderThreadImpl sets the
global for the renderer, why does every creator of cc::Layer need to query it?
Or, were you picturing setting up the LayerSettings somewhere else?

[danakj, 2015-05-12 19:58:15]

ok talked about this with piman, and i misread, i see you have a global in
cc/blink/ not in cc::Layer so that sounds better than what I was thinking :)

https://codereview.chromium.org/1122393003/diff/60001/cc/blink/web_layer_impl.cc
File cc/blink/web_layer_impl.cc (right):

https://codereview.chromium.org/1122393003/diff/60001/cc/blink/web_layer_impl...
cc/blink/web_layer_impl.cc:88: cc::LayerSettings& WebLayerImpl::LayerSettings()
{
const?

[piman, 2015-05-12 20:06:54]

https://codereview.chromium.org/1122393003/diff/60001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1122393003/diff/60001/ui/compositor/composito...
ui/compositor/compositor.cc:93: Layer::SetUILayerSettings(ui_layer_settings);
I think we should do this initialization only once, rather than for every new
ui::Compositor. We can move that code to the ContextFactory initialization -
there's only 2: GpuProcessTransportFactory in
content/browser/compositor/gpu_process_transport_factory.cc (for production) and
InProcessContextFactory in 
ui/compositor/test/in_process_context_factory.cc (for tests). You can move this
code into a static ui::Layer::InitializeLayerSettings() and call it from those 2
places.

[piman, 2015-05-12 20:12:31]

piman@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[piman, 2015-05-12 20:12:33]

And LGTM assuming the above is fixed.

[piman, 2015-05-12 20:12:35]

And LGTM assuming the above is fixed.

[piman, 2015-05-12 20:12:59]

+dalecurtis for media

[DaleCurtis, 2015-05-13 02:53:13]

media/ lgtm

[loyso (OOO), 2015-05-18 04:42:52]

loyso@chromium.org changed reviewers:
+ dtrainor@chromium.org, yfriedman@chromium.org

[loyso (OOO), 2015-05-18 04:42:52]

yfriedman@chromium.org: Please review changes in 
dtrainor@chromium.org: Please review changes in

[loyso (OOO), 2015-05-18 05:30:26]

loyso@chromium.org changed reviewers:
+ boliu@chromium.org

[loyso (OOO), 2015-05-18 05:30:28]

boliu@chromium.org: Please review changes in

[loyso (OOO), 2015-05-18 07:13:35]

https://codereview.chromium.org/1122393003/diff/100001/android_webview/browse...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/1122393003/diff/100001/android_webview/browse...
android_webview/browser/hardware_renderer.cc:43:
root_layer_(cc::Layer::Create(content::Compositor::LayerSettings())),
I'm not sure about this. Do we need a special case/command line parameter?

[David Trainor- moved to gerrit, 2015-05-18 13:51:47]

On 2015/05/18 04:42:52, loyso wrote:
> mailto:yfriedman@chromium.org: Please review changes in 
> mailto:dtrainor@chromium.org: Please review changes in

chrome/browser/android lgtm.

Wasn't really sure what to review changes in, so I just looked at
chrome/browser/android.  Let me know if you need me to review other places.

[boliu, 2015-05-18 15:39:31]

Side note, the cc::*Layer::Create signature changes will need to be fixed in
downstream chrome on android code as well. Should talk to the current android
sheriff about fixing those, and coordinating land these CLs.

https://codereview.chromium.org/1122393003/diff/100001/android_webview/browse...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/1122393003/diff/100001/android_webview/browse...
android_webview/browser/hardware_renderer.cc:43:
root_layer_(cc::Layer::Create(content::Compositor::LayerSettings())),
On 2015/05/18 07:13:34, loyso wrote:
> I'm not sure about this. Do we need a special case/command line parameter?

This is a different cc instance, so should not re-use settings from
content::Compositor. Just create a LayerSettings here.

https://codereview.chromium.org/1122393003/diff/100001/content/public/browser...
File content/public/browser/android/compositor.h (right):

https://codereview.chromium.org/1122393003/diff/100001/content/public/browser...
content/public/browser/android/compositor.h:50: static void
SetLayerSettings(const cc::LayerSettings& settings);
I think nothing calls the setter? Remove it?

[enne (OOO), 2015-05-18 21:49:46]

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

[enne (OOO), 2015-05-18 21:49:49]

https://codereview.chromium.org/1122393003/diff/100001/cc/trees/layer_tree_se...
File cc/trees/layer_tree_settings.cc (right):

https://codereview.chromium.org/1122393003/diff/100001/cc/trees/layer_tree_se...
cc/trees/layer_tree_settings.cc:16: LayerSettings::LayerSettings() :
use_compositor_animation_timelines(false) {
Are you expecting more settings in the future? Does this bool have to be a ctor
parameter to Layer?

[loyso (OOO), 2015-05-19 01:03:02]

On 2015/05/18 15:39:31, boliu wrote:

> Side note, the cc::*Layer::Create signature changes will need to be fixed in
> downstream chrome on android code as well. Should talk to the current android
> sheriff about fixing those, and coordinating land these CLs.
Sure, I have those changes locally. Not sure if it's worth the effort.

[loyso (OOO), 2015-05-19 01:03:45]

https://codereview.chromium.org/1122393003/diff/100001/cc/trees/layer_tree_se...
File cc/trees/layer_tree_settings.cc (right):

https://codereview.chromium.org/1122393003/diff/100001/cc/trees/layer_tree_se...
cc/trees/layer_tree_settings.cc:16: LayerSettings::LayerSettings() :
use_compositor_animation_timelines(false) {
On 2015/05/18 21:49:49, enne wrote:
> Are you expecting more settings in the future? Does this bool have to be a
ctor
> parameter to Layer?

A bit of context/historical background: It's started here
https://codereview.chromium.org/1101823002/patch/120001/130002

Strictly IMHO:

1) No, I don't expect more settings in the future, especially in the context of
my project (cc animations). cc::Layer constructor will be eventually empty:
https://codereview.chromium.org/1130043003/diff/20001/cc/layers/layer.cc

Moreover, in a long term cc::Layers system is deprecated in favor of
DisplayLists (Slimming Paint).

2) No, that bool hasn't to be passed intro constructor if we choose
RegisterForAnimations approach. (Patch Set#7
https://codereview.chromium.org/1101823002/#ps120001)

https://codereview.chromium.org/1122393003/diff/100001/content/public/browser...
File content/public/browser/android/compositor.h (right):

https://codereview.chromium.org/1122393003/diff/100001/content/public/browser...
content/public/browser/android/compositor.h:50: static void
SetLayerSettings(const cc::LayerSettings& settings);
On 2015/05/18 15:39:31, boliu wrote:
> I think nothing calls the setter? Remove it?

Yes. That was a question. We have --enable-ui-compositor-timelines for ui::
compositor to switch that bool on. It looks like we need a separate command line
flag for android compositor. --enable-android-compositor-timelines?

[loyso (OOO), 2015-05-19 04:21:48]

https://codereview.chromium.org/1122393003/diff/60001/cc/blink/web_layer_impl.cc
File cc/blink/web_layer_impl.cc (right):

https://codereview.chromium.org/1122393003/diff/60001/cc/blink/web_layer_impl...
cc/blink/web_layer_impl.cc:88: cc::LayerSettings& WebLayerImpl::LayerSettings()
{
On 2015/05/12 19:58:14, danakj wrote:
> const?

Done.

https://codereview.chromium.org/1122393003/diff/60001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1122393003/diff/60001/ui/compositor/composito...
ui/compositor/compositor.cc:93: Layer::SetUILayerSettings(ui_layer_settings);
On 2015/05/12 20:06:54, piman (Very slow to review) wrote:
> I think we should do this initialization only once, rather than for every new
> ui::Compositor. We can move that code to the ContextFactory initialization -
> there's only 2: GpuProcessTransportFactory in
> content/browser/compositor/gpu_process_transport_factory.cc (for production)
and
> InProcessContextFactory in 
> ui/compositor/test/in_process_context_factory.cc (for tests). You can move
this
> code into a static ui::Layer::InitializeLayerSettings() and call it from those
2
> places.

Done.

[piman, 2015-05-19 17:46:54]

lgtm

[piman, 2015-05-19 17:48:25]

On Mon, May 18, 2015 at 6:03 PM, <loyso@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1122393003/diff/100001/cc/trees/layer_tree_se...
> File cc/trees/layer_tree_settings.cc (right):
>
>
>
https://codereview.chromium.org/1122393003/diff/100001/cc/trees/layer_tree_se...
> cc/trees/layer_tree_settings.cc:16: LayerSettings::LayerSettings() :
> use_compositor_animation_timelines(false) {
> On 2015/05/18 21:49:49, enne wrote:
>
>> Are you expecting more settings in the future? Does this bool have to
>>
> be a ctor
>
>> parameter to Layer?
>>
>
> A bit of context/historical background: It's started here
> https://codereview.chromium.org/1101823002/patch/120001/130002
>
> Strictly IMHO:
>
> 1) No, I don't expect more settings in the future, especially in the
> context of my project (cc animations). cc::Layer constructor will be
> eventually empty:
> https://codereview.chromium.org/1130043003/diff/20001/cc/layers/layer.cc
>
> Moreover, in a long term cc::Layers system is deprecated in favor of
> DisplayLists (Slimming Paint).
>
> 2) No, that bool hasn't to be passed intro constructor if we choose
> RegisterForAnimations approach. (Patch Set#7
> https://codereview.chromium.org/1101823002/#ps120001)
>
>
>
https://codereview.chromium.org/1122393003/diff/100001/content/public/browser...
> File content/public/browser/android/compositor.h (right):
>
>
>
https://codereview.chromium.org/1122393003/diff/100001/content/public/browser...
> content/public/browser/android/compositor.h:50: static void
> SetLayerSettings(const cc::LayerSettings& settings);
> On 2015/05/18 15:39:31, boliu wrote:
>
>> I think nothing calls the setter? Remove it?
>>
>
> Yes. That was a question. We have --enable-ui-compositor-timelines for
> ui:: compositor to switch that bool on. It looks like we need a separate
> command line flag for android compositor.
> --enable-android-compositor-timelines?
>

I think you can keep the same flag name for android.


>
> https://codereview.chromium.org/1122393003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[enne (OOO), 2015-05-19 18:21:01]

I still think a ctor parameter is a lot of plumbing for what's essentially a
const per-process global variable.  However, if this will go away in the future
when this feature ships, maybe it doesn't matter what has to be done in the
meantime and this is ok enough.

[loyso (OOO), 2015-05-20 02:44:29]

piman (Very slow to review) wrote:
> > Yes. That was a question. We have --enable-ui-compositor-timelines for
> > ui:: compositor to switch that bool on. It looks like we need a separate
> > command line flag for android compositor.
> > --enable-android-compositor-timelines?
> I think you can keep the same flag name for android.
The problem is that kUIEnableCompositorAnimationTimelines is defined in
ui/compositor target. I decided to establish separate OS_ANDROID-only
kEnableAndroidCompositorAnimationTimelines in
content/public/common/content_switches.h.
So we can use --enable-android-compositor-animation-timelines for Android Tab
Switcher.

[loyso (OOO), 2015-05-20 02:48:53]

https://codereview.chromium.org/1122393003/diff/100001/android_webview/browse...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/1122393003/diff/100001/android_webview/browse...
android_webview/browser/hardware_renderer.cc:43:
root_layer_(cc::Layer::Create(content::Compositor::LayerSettings())),
On 2015/05/18 15:39:31, boliu wrote:
> On 2015/05/18 07:13:34, loyso wrote:
> > I'm not sure about this. Do we need a special case/command line parameter?
> 
> This is a different cc instance, so should not re-use settings from
> content::Compositor. Just create a LayerSettings here.
I decided to go with HardwareRenderer-local g_layer_settings. PTAL!

https://codereview.chromium.org/1122393003/diff/100001/content/public/browser...
File content/public/browser/android/compositor.h (right):

https://codereview.chromium.org/1122393003/diff/100001/content/public/browser...
content/public/browser/android/compositor.h:50: static void
SetLayerSettings(const cc::LayerSettings& settings);
On 2015/05/18 15:39:31, boliu wrote:
> I think nothing calls the setter? Remove it?
Now we use switches::kEnableAndroidCompositorAnimationTimelines and we call the
setter from content/browser/android/content_startup_flags.cc
Is that the right place to do so?

[boliu, 2015-05-20 15:51:02]

android_webview lgtm, but had one suggestion

https://codereview.chromium.org/1122393003/diff/180001/android_webview/browse...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/1122393003/diff/180001/android_webview/browse...
android_webview/browser/hardware_renderer.cc:38: LAZY_INSTANCE_INITIALIZER;
Lazy instance feels over kill here. I don't think constructing a LayerSettings
is expensive?

And should use a function if the purpose is to unify LayerSettings creation,
since lazy instance doesn't have the opportunity to change any of the settings
from default.

[loyso (OOO), 2015-05-21 05:12:35]

https://codereview.chromium.org/1122393003/diff/180001/android_webview/browse...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/1122393003/diff/180001/android_webview/browse...
android_webview/browser/hardware_renderer.cc:38: LAZY_INSTANCE_INITIALIZER;
On 2015/05/20 15:51:02, boliu wrote:
> Lazy instance feels over kill here. I don't think constructing a LayerSettings
> is expensive?
> 
> And should use a function if the purpose is to unify LayerSettings creation,
> since lazy instance doesn't have the opportunity to change any of the settings
> from default.
Yes, thread-safety is over kill here. Our style guide says: "Static or global
variables of class type are forbidden". It allows only structs of POD (which is
better to be updated in C++11 trivial/non-trivial terms). I could replace lazy
instances with simple static functions everywhere.

Linters are ok with "cc::LayerSettings g_layer_settings" except that I need to
remove the empty destructor.

What do you think?

[boliu, 2015-05-21 05:23:33]

https://codereview.chromium.org/1122393003/diff/180001/android_webview/browse...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/1122393003/diff/180001/android_webview/browse...
android_webview/browser/hardware_renderer.cc:38: LAZY_INSTANCE_INITIALIZER;
On 2015/05/21 05:12:35, loyso wrote:
> On 2015/05/20 15:51:02, boliu wrote:
> > Lazy instance feels over kill here. I don't think constructing a
LayerSettings
> > is expensive?
> > 
> > And should use a function if the purpose is to unify LayerSettings creation,
> > since lazy instance doesn't have the opportunity to change any of the
settings
> > from default.
> Yes, thread-safety is over kill here. Our style guide says: "Static or global
> variables of class type are forbidden". It allows only structs of POD (which
is
> better to be updated in C++11 trivial/non-trivial terms). I could replace lazy
> instances with simple static functions everywhere.
> 
> Linters are ok with "cc::LayerSettings g_layer_settings" except that I need to
> remove the empty destructor.

That's definitely not allowed. Having the constructor means it creates a static
initializer. And not having a constructor means the members are not initialized.

> 
> What do you think?

Either just create LayerSettings on the stack where it's needed. Or create a
global function, in an anonymous namespace, that constructs and returns a *copy*
of LayerSettings.

No threading issues if you never have a global instance.

[David Trainor- moved to gerrit, 2015-05-21 17:17:53]

chrome/browser/android/compositor lgtm.  sorry for the delays!

[loyso (OOO), 2015-05-22 04:10:47]

The CQ bit was checked by loyso@chromium.org

[loyso (OOO), 2015-05-22 04:10:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, piman@chromium.org, boliu@chromium.org,
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/1122393003/#ps220001
(title: "Rebase")

[commit-bot: I haz the power, 2015-05-22 04:11:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1122393003/220001

[boliu, 2015-05-22 04:22:34]

The CQ bit was unchecked by boliu@chromium.org

[boliu, 2015-05-22 04:24:07]

unchecked cq due to more comments

https://codereview.chromium.org/1122393003/diff/220001/android_webview/browse...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/1122393003/diff/220001/android_webview/browse...
android_webview/browser/hardware_renderer.cc:98: return g_layer_settings.Get();
My recommendation was not use lazy instance. Just return layerSettings(); here

https://codereview.chromium.org/1122393003/diff/220001/android_webview/browse...
File android_webview/browser/hardware_renderer.h (right):

https://codereview.chromium.org/1122393003/diff/220001/android_webview/browse...
android_webview/browser/hardware_renderer.h:37: static const cc::LayerSettings&
LayerSettings();
This should not be a public method. Just put it in an anonymous namespace in the
cc file. Also return by value, not reference.

[loyso (OOO), 2015-05-22 04:34:31]

https://codereview.chromium.org/1122393003/diff/220001/android_webview/browse...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/1122393003/diff/220001/android_webview/browse...
android_webview/browser/hardware_renderer.cc:98: return g_layer_settings.Get();
On 2015/05/22 04:24:07, boliu wrote:
> My recommendation was not use lazy instance. Just return layerSettings(); here
Done. Really sorry. Misunderstood you in unification. PTAL?

[boliu, 2015-05-22 04:35:17]

On 2015/05/22 04:34:31, loyso wrote:
>
https://codereview.chromium.org/1122393003/diff/220001/android_webview/browse...
> File android_webview/browser/hardware_renderer.cc (right):
> 
>
https://codereview.chromium.org/1122393003/diff/220001/android_webview/browse...
> android_webview/browser/hardware_renderer.cc:98: return
g_layer_settings.Get();
> On 2015/05/22 04:24:07, boliu wrote:
> > My recommendation was not use lazy instance. Just return layerSettings();
here
> Done. Really sorry. Misunderstood you in unification. PTAL?

lgtm

[loyso (OOO), 2015-05-25 02:04:12]

The CQ bit was checked by loyso@chromium.org

[loyso (OOO), 2015-05-25 02:04:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, dalecurtis@chromium.org, piman@chromium.org,
boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/1122393003/#ps260001
(title: "Rebase")

[commit-bot: I haz the power, 2015-05-25 02:04:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1122393003/260001

[commit-bot: I haz the power, 2015-05-25 03:26:56]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-05-25 03:28:02]

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