Animations: Implement runtime flag for enabling compositor animation timelines.

Animations: Implement runtime flag for enabling compositor animation timelines.

Blink part: https://codereview.chromium.org/946323002

BUG=394777
R=ajuma@chromium.org
R=vollick@chromium.org

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

[loyso (OOO), 2015-03-09 00:47:19]

loyso@chromium.org changed reviewers:
+ aelias@chromium.org, ben@chromium.org, brettw@chromium.org

[loyso (OOO), 2015-03-09 00:49:15]

On 2015/03/09 00:47:19, loyso wrote:

This is a prerequisite to implement this:
https://codereview.chromium.org/947033002/

[Ian Vollick, 2015-03-09 03:42:19]

On 2015/03/09 00:49:15, loyso wrote:
> On 2015/03/09 00:47:19, loyso wrote:
> 
> This is a prerequisite to implement this:
> https://codereview.chromium.org/947033002/

lgtm.

[loyso (OOO), 2015-03-10 03:55:40]

content/ review, anyone please?

[loyso (OOO), 2015-03-10 04:03:06]

loyso@chromium.org changed reviewers:
+ piman@google.com

[danakj, 2015-03-10 17:07:09]

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

[danakj, 2015-03-10 17:07:09]

https://codereview.chromium.org/986173002/diff/20001/cc/base/switches.h
File cc/base/switches.h (right):

https://codereview.chromium.org/986173002/diff/20001/cc/base/switches.h#newco...
cc/base/switches.h:38: CC_EXPORT extern const char
kEnableCompositorAnimationTimelines[];
You have this in the set of switches for both renderer and ui, but you only use
the flag for the renderer compositor?

[loyso (OOO), 2015-03-10 23:18:48]

https://codereview.chromium.org/986173002/diff/20001/cc/base/switches.h
File cc/base/switches.h (right):

https://codereview.chromium.org/986173002/diff/20001/cc/base/switches.h#newco...
cc/base/switches.h:38: CC_EXPORT extern const char
kEnableCompositorAnimationTimelines[];
On 2015/03/10 17:07:09, danakj wrote:
> You have this in the set of switches for both renderer and ui, but you only
use
> the flag for the renderer compositor?

I'm going to use it in ui::Compositor as well.

[chromium-reviews, 2015-03-10 23:28:27]

Yes, exactly the same flag in exactly the same way:

ui::Compositor::Compositor() {
...
  settings.use_compositor_animation_timelines = command_line->HasSwitch(
      cc::switches::kEnableCompositorAnimationTimelines);
}

On Wed, Mar 11, 2015 at 10:21 AM, Dana Jansens <danakj@chromium.org> wrote:

> On Tue, Mar 10, 2015 at 4:18 PM, <loyso@chromium.org> wrote:
>
>>
>> https://codereview.chromium.org/986173002/diff/20001/cc/base/switches.h
>> File cc/base/switches.h (right):
>>
>> https://codereview.chromium.org/986173002/diff/20001/cc/
>> base/switches.h#newcode38
>> cc/base/switches.h:38: CC_EXPORT extern const char
>> kEnableCompositorAnimationTimelines[];
>> On 2015/03/10 17:07:09, danakj wrote:
>>
>>> You have this in the set of switches for both renderer and ui, but you
>>>
>> only use
>>
>>> the flag for the renderer compositor?
>>>
>>
>> I'm going to use it in ui::Compositor as well.
>>
>
> You're planning to use the same flag, rather than a --ui-foo-bar like we
> have in ui/compositor/switches.cc? I feel like putting it here is a lie, at
> least for now, for people reading this file anyhow.
>

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

[danakj, 2015-03-10 23:29:07]

On Tue, Mar 10, 2015 at 4:18 PM, <loyso@chromium.org> wrote:

>
> https://codereview.chromium.org/986173002/diff/20001/cc/base/switches.h
> File cc/base/switches.h (right):
>
> https://codereview.chromium.org/986173002/diff/20001/cc/
> base/switches.h#newcode38
> cc/base/switches.h:38: CC_EXPORT extern const char
> kEnableCompositorAnimationTimelines[];
> On 2015/03/10 17:07:09, danakj wrote:
>
>> You have this in the set of switches for both renderer and ui, but you
>>
> only use
>
>> the flag for the renderer compositor?
>>
>
> I'm going to use it in ui::Compositor as well.
>

You're planning to use the same flag, rather than a --ui-foo-bar like we
have in ui/compositor/switches.cc? I feel like putting it here is a lie, at
least for now, for people reading this file anyhow.

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

[danakj, 2015-03-10 23:46:07]

On Tue, Mar 10, 2015 at 4:28 PM, Alexey Baskakov <loyso@google.com> wrote:

> Yes, exactly the same flag in exactly the same way:
>
> ui::Compositor::Compositor() {
> ...
>   settings.use_compositor_animation_timelines = command_line->HasSwitch(
>       cc::switches::kEnableCompositorAnimationTimelines);
> }
>

Why not do both in this patch then? It feels like you're trying to stage it
as one compositor at a time. But if you're doing that then 2 different
flags makes more sense.


>
> On Wed, Mar 11, 2015 at 10:21 AM, Dana Jansens <danakj@chromium.org>
> wrote:
>
>> On Tue, Mar 10, 2015 at 4:18 PM, <loyso@chromium.org> wrote:
>>
>>>
>>> https://codereview.chromium.org/986173002/diff/20001/cc/base/switches.h
>>> File cc/base/switches.h (right):
>>>
>>> https://codereview.chromium.org/986173002/diff/20001/cc/
>>> base/switches.h#newcode38
>>> cc/base/switches.h:38: CC_EXPORT extern const char
>>> kEnableCompositorAnimationTimelines[];
>>> On 2015/03/10 17:07:09, danakj wrote:
>>>
>>>> You have this in the set of switches for both renderer and ui, but you
>>>>
>>> only use
>>>
>>>> the flag for the renderer compositor?
>>>>
>>>
>>> I'm going to use it in ui::Compositor as well.
>>>
>>
>> You're planning to use the same flag, rather than a --ui-foo-bar like we
>> have in ui/compositor/switches.cc? I feel like putting it here is a lie, at
>> least for now, for people reading this file anyhow.
>>
>
>

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

[loyso (OOO), 2015-03-11 01:14:36]

loyso@chromium.org changed reviewers:
- ben@chromium.org

[danakj, 2015-03-12 20:29:58]

LGTM w/ a couple comments

https://codereview.chromium.org/986173002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/chrome_restart_request.cc (right):

https://codereview.chromium.org/986173002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/chrome_restart_request.cc:173:
::switches::kEnableCompositorAnimationTimelines,
please add these in alphabetical order

https://codereview.chromium.org/986173002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/986173002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:1243:
switches::kEnablePreferCompositingToLCDText,
mind moving this to be alphabetical while you're here?

https://codereview.chromium.org/986173002/diff/40001/ui/compositor/compositor...
File ui/compositor/compositor_switches.cc (right):

https://codereview.chromium.org/986173002/diff/40001/ui/compositor/compositor...
ui/compositor/compositor_switches.cc:20: const char
kUIEnableCompositorAnimationTimelines[] =
Do you wanna set the LayerTreeSettings from this in ui/compositor/compositor.cc
as well?

[piman, 2015-03-12 22:04:40]

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

[piman, 2015-03-12 22:04:40]

LGTM for content after Dana's comments are addressed.

[loyso (OOO), 2015-03-12 23:18:50]

https://codereview.chromium.org/986173002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/chrome_restart_request.cc (right):

https://codereview.chromium.org/986173002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/chrome_restart_request.cc:173:
::switches::kEnableCompositorAnimationTimelines,
On 2015/03/12 20:29:57, danakj wrote:
> please add these in alphabetical order

Done.

https://codereview.chromium.org/986173002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/986173002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:1243:
switches::kEnablePreferCompositingToLCDText,
On 2015/03/12 20:29:57, danakj wrote:
> mind moving this to be alphabetical while you're here?

Acknowledged.

https://codereview.chromium.org/986173002/diff/40001/ui/compositor/compositor...
File ui/compositor/compositor_switches.cc (right):

https://codereview.chromium.org/986173002/diff/40001/ui/compositor/compositor...
ui/compositor/compositor_switches.cc:20: const char
kUIEnableCompositorAnimationTimelines[] =
On 2015/03/12 20:29:57, danakj wrote:
> Do you wanna set the LayerTreeSettings from this in
ui/compositor/compositor.cc
> as well?

Yes. I'll use it.

[danakj, 2015-03-13 17:45:54]

LGTM

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

https://codereview.chromium.org/986173002/diff/60001/ui/compositor/compositor...
ui/compositor/compositor.cc:88: 
don't add unrelated whitespace change

https://codereview.chromium.org/986173002/diff/60001/ui/compositor/compositor...
ui/compositor/compositor.cc:136: settings.use_image_texture_target =
context_factory_->GetImageTextureTarget();
please try to keep the diff minimal if it makes sense. here these were grouped
together because they are all impl-side painting flags.  can you leave them as
they were and just put this new one on its own?

[loyso (OOO), 2015-03-15 23:29:22]

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

https://codereview.chromium.org/986173002/diff/60001/ui/compositor/compositor...
ui/compositor/compositor.cc:88: 
On 2015/03/13 17:45:53, danakj wrote:
> don't add unrelated whitespace change

Done.

https://codereview.chromium.org/986173002/diff/60001/ui/compositor/compositor...
ui/compositor/compositor.cc:136: settings.use_image_texture_target =
context_factory_->GetImageTextureTarget();
On 2015/03/13 17:45:53, danakj wrote:
> please try to keep the diff minimal if it makes sense. here these were grouped
> together because they are all impl-side painting flags.  can you leave them as
> they were and just put this new one on its own?

Done.

[loyso (OOO), 2015-04-01 23:39:38]

The CQ bit was checked by loyso@chromium.org

[loyso (OOO), 2015-04-01 23:39:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
vollick@chromium.org, piman@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/986173002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2015-04-01 23:40:23]

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

[commit-bot: I haz the power, 2015-04-02 01:22:34]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-04-03 20:20:55]

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