Introduce DisplayConfigurationController

Introduce DisplayConfigurationController

This class provides a consistent interface for UI and extension API
display configuration changes. It handles any animation transtions,
extracting animation code from DisplayManager and WindowTreeHostManager.

This also cleans up some of the code in DisplayOptionsHandler.

BUG=576375
Committed: https://crrev.com/537c5c3f3a4f96823e5923694eda37a34add3cbe
Cr-Commit-Position: refs/heads/master@{#370528}

[stevenjb, 2016-01-16 02:33:24]

Description was changed from

==========
Introduce DisplayConfigurationController

This class provides a consistent interface for UI and extension API
display configuration changes. It handles any animation transtions,
extracting animation code from DisplayManager and WindowTreeHostManager.

BUG=576375
==========

to

==========
Introduce DisplayConfigurationController

This class provides a consistent interface for UI and extension API
display configuration changes. It handles any animation transtions,
extracting animation code from DisplayManager and WindowTreeHostManager.

This also cleans up some of the code in DisplayOptionsHandler.


BUG=576375
==========

[stevenjb, 2016-01-16 02:33:53]

stevenjb@chromium.org changed reviewers:
+ oshima@chromium.org

[stevenjb, 2016-01-16 02:33:54]

LMKWYT

[oshima, 2016-01-19 18:49:16]

https://codereview.chromium.org/1594683002/diff/20001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/1594683002/diff/20001/ash/accelerators/accele...
ash/accelerators/accelerator_controller.cc:640: #if defined(OS_CHROMEOS)
I think you don't have to worry about other platforms as long as tests/bots
passes.

https://codereview.chromium.org/1594683002/diff/20001/ash/display/display_con...
File ash/display/display_configuration_controller.cc (right):

https://codereview.chromium.org/1594683002/diff/20001/ash/display/display_con...
ash/display/display_configuration_controller.cc:66:
display_configurator_->RemoveObserver(display_animator_.get());
can we move this (add/remove) code to animator?

https://codereview.chromium.org/1594683002/diff/20001/ash/display/display_con...
File ash/display/display_configuration_controller.h (right):

https://codereview.chromium.org/1594683002/diff/20001/ash/display/display_con...
ash/display/display_configuration_controller.h:44: // a fade in/out animation.
Currently |layout.primary_id| must be the primary
I'm planning to extend DisplayLayout to support more than 2 displays and the
primary_id will be primary.

https://codereview.chromium.org/1594683002/diff/20001/ash/display/display_man...
File ash/display/display_manager.cc (left):

https://codereview.chromium.org/1594683002/diff/20001/ash/display/display_man...
ash/display/display_manager.cc:937: #if defined(OS_CHROMEOS)
did you have a problem with this on other platforms?

[stevenjb, 2016-01-19 20:32:04]

https://codereview.chromium.org/1594683002/diff/20001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/1594683002/diff/20001/ash/accelerators/accele...
ash/accelerators/accelerator_controller.cc:640: #if defined(OS_CHROMEOS)
On 2016/01/19 18:49:16, oshima wrote:
> I think you don't have to worry about other platforms as long as tests/bots
> passes.

Currently display_configuration_controller() only exists on Chrome OS (because
display_configurator only exists on Chrome OS), so until / unless we clean all
of that up, this is needed.

https://codereview.chromium.org/1594683002/diff/20001/ash/display/display_con...
File ash/display/display_configuration_controller.cc (right):

https://codereview.chromium.org/1594683002/diff/20001/ash/display/display_con...
ash/display/display_configuration_controller.cc:66:
display_configurator_->RemoveObserver(display_animator_.get());
On 2016/01/19 18:49:16, oshima wrote:
> can we move this (add/remove) code to animator?

I'm not sure I understand. This class now owns the animator. I could change the
animator to require a Delegate to be passed in the constructor instead of
supporting observers if that is what you are suggesting?

https://codereview.chromium.org/1594683002/diff/20001/ash/display/display_con...
File ash/display/display_configuration_controller.h (right):

https://codereview.chromium.org/1594683002/diff/20001/ash/display/display_con...
ash/display/display_configuration_controller.h:44: // a fade in/out animation.
Currently |layout.primary_id| must be the primary
On 2016/01/19 18:49:16, oshima wrote:
> I'm planning to extend DisplayLayout to support more than 2 displays and the
> primary_id will be primary.
Simplified the comment for now, we can update it when we introduce 3+ display
support.

https://codereview.chromium.org/1594683002/diff/20001/ash/display/display_man...
File ash/display/display_manager.cc (left):

https://codereview.chromium.org/1594683002/diff/20001/ash/display/display_man...
ash/display/display_manager.cc:937: #if defined(OS_CHROMEOS)
On 2016/01/19 18:49:16, oshima wrote:
> did you have a problem with this on other platforms?

No, I just wanted to limit the #if defined(OS_CHROMES) to the actual chromeos
specific code. (I don't think this actually gets called on other platforms so we
could probably #ifdef out the entire method, but that would be extra work).

[stevenjb, 2016-01-19 20:32:11]

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

[commit-bot: I haz the power, 2016-01-19 20:33:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1594683002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1594683002/60001

[oshima, 2016-01-19 20:59:30]

https://codereview.chromium.org/1594683002/diff/20001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/1594683002/diff/20001/ash/accelerators/accele...
ash/accelerators/accelerator_controller.cc:640: #if defined(OS_CHROMEOS)
On 2016/01/19 20:32:04, stevenjb wrote:
> On 2016/01/19 18:49:16, oshima wrote:
> > I think you don't have to worry about other platforms as long as tests/bots
> > passes.
> 
> Currently display_configuration_controller() only exists on Chrome OS (because
> display_configurator only exists on Chrome OS), so until / unless we clean all
> of that up, this is needed.

ack. hopefully we can remove ifdef soon.

https://codereview.chromium.org/1594683002/diff/20001/ash/display/display_con...
File ash/display/display_configuration_controller.cc (right):

https://codereview.chromium.org/1594683002/diff/20001/ash/display/display_con...
ash/display/display_configuration_controller.cc:66:
display_configurator_->RemoveObserver(display_animator_.get());
On 2016/01/19 20:32:04, stevenjb wrote:
> On 2016/01/19 18:49:16, oshima wrote:
> > can we move this (add/remove) code to animator?
> 
> I'm not sure I understand. This class now owns the animator. I could change
the
> animator to require a Delegate to be passed in the constructor instead of
> supporting observers if that is what you are suggesting?

DisplayConfigurator is used only for animator, so you create an Animator (and
add/remove observer in ctor/dtor) and it instead. The animator already depends
on DisplayCOnfigurator, so we can minimize the dependency that way.

https://codereview.chromium.org/1594683002/diff/20001/ash/display/display_man...
File ash/display/display_manager.cc (left):

https://codereview.chromium.org/1594683002/diff/20001/ash/display/display_man...
ash/display/display_manager.cc:937: #if defined(OS_CHROMEOS)
On 2016/01/19 20:32:04, stevenjb wrote:
> On 2016/01/19 18:49:16, oshima wrote:
> > did you have a problem with this on other platforms?
> 
> No, I just wanted to limit the #if defined(OS_CHROMES) to the actual chromeos
> specific code. (I don't think this actually gets called on other platforms so
we
> could probably #ifdef out the entire method, but that would be extra work).

We're removing win_ash now, so you don't have to worry about it unless it has a
problem (crbug.com/558054). I'll leave it to you though.

[commit-bot: I haz the power, 2016-01-19 21:15:41]

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

[commit-bot: I haz the power, 2016-01-19 21:15:42]

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

[stevenjb, 2016-01-20 01:25:36]

PTAL

https://codereview.chromium.org/1594683002/diff/20001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/1594683002/diff/20001/ash/accelerators/accele...
ash/accelerators/accelerator_controller.cc:640: #if defined(OS_CHROMEOS)
On 2016/01/19 20:59:30, oshima wrote:
> On 2016/01/19 20:32:04, stevenjb wrote:
> > On 2016/01/19 18:49:16, oshima wrote:
> > > I think you don't have to worry about other platforms as long as
tests/bots
> > > passes.
> > 
> > Currently display_configuration_controller() only exists on Chrome OS
(because
> > display_configurator only exists on Chrome OS), so until / unless we clean
all
> > of that up, this is needed.
> 
> ack. hopefully we can remove ifdef soon.

Actually, I went ahead and fixed this.

https://codereview.chromium.org/1594683002/diff/20001/ash/display/display_con...
File ash/display/display_configuration_controller.cc (right):

https://codereview.chromium.org/1594683002/diff/20001/ash/display/display_con...
ash/display/display_configuration_controller.cc:66:
display_configurator_->RemoveObserver(display_animator_.get());
On 2016/01/19 20:59:30, oshima wrote:
> On 2016/01/19 20:32:04, stevenjb wrote:
> > On 2016/01/19 18:49:16, oshima wrote:
> > > can we move this (add/remove) code to animator?
> > 
> > I'm not sure I understand. This class now owns the animator. I could change
> the
> > animator to require a Delegate to be passed in the constructor instead of
> > supporting observers if that is what you are suggesting?
> 
> DisplayConfigurator is used only for animator, so you create an Animator (and
> add/remove observer in ctor/dtor) and it instead. The animator already depends
> on DisplayCOnfigurator, so we can minimize the dependency that way.

Ah, I think I understand what you mean. I did a bit of re-factoring and I think
the result is a bit simpler, with less #if OS_CHROMEOS in the code.

[oshima, 2016-01-20 02:02:51]

looks much better, thanks. The previous patch seems to have test failures. Can
you look into them?

https://codereview.chromium.org/1594683002/diff/80001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/1594683002/diff/80001/ash/accelerators/accele...
ash/accelerators/accelerator_controller.cc:694: #if defined(OS_CHROMEOS)
we can now remove this ifdef?

https://codereview.chromium.org/1594683002/diff/80001/ash/display/display_ani...
File ash/display/display_animator_chromeos.h (right):

https://codereview.chromium.org/1594683002/diff/80001/ash/display/display_ani...
ash/display/display_animator_chromeos.h:32: class ASH_EXPORT
DisplayAnimatorChromeOS
or maybe DisplayAnimatorImpl, and we can simply have a factory function that
returns the impl, especially because there will be no other implementation (or
we can have an empty impl for other platforms for the time being).

it's optional, so you may leave it as is.

https://codereview.chromium.org/1594683002/diff/80001/ash/display/display_con...
File ash/display/display_configuration_controller.h (right):

https://codereview.chromium.org/1594683002/diff/80001/ash/display/display_con...
ash/display/display_configuration_controller.h:59: void
OnDisplayConfigurationChanged() override;
We probably should stop all animations if there is a display configuration
change due to connect/disconnect. Can you add TODO?

[stevenjb, 2016-01-20 02:08:05]

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

[commit-bot: I haz the power, 2016-01-20 02:09:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1594683002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1594683002/100001

[stevenjb, 2016-01-20 02:13:13]

PS5 should fix the tests also, tested in Debug mode locally (destruction order
was incorrect).

https://codereview.chromium.org/1594683002/diff/80001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/1594683002/diff/80001/ash/accelerators/accele...
ash/accelerators/accelerator_controller.cc:694: #if defined(OS_CHROMEOS)
On 2016/01/20 02:02:51, oshima wrote:
> we can now remove this ifdef?

Yeah, I cleaned that up in PS 6.

https://codereview.chromium.org/1594683002/diff/80001/ash/display/display_ani...
File ash/display/display_animator_chromeos.h (right):

https://codereview.chromium.org/1594683002/diff/80001/ash/display/display_ani...
ash/display/display_animator_chromeos.h:32: class ASH_EXPORT
DisplayAnimatorChromeOS
On 2016/01/20 02:02:51, oshima wrote:
> or maybe DisplayAnimatorImpl, and we can simply have a factory function that
> returns the impl, especially because there will be no other implementation (or
> we can have an empty impl for other platforms for the time being).
> 
> it's optional, so you may leave it as is.

This seemed simpler to me simply because we won't have any other implementations
(and appending _chromeos.cc removes this automatically on other platforms). I
tested that this compiles and runs on linux at least...

https://codereview.chromium.org/1594683002/diff/80001/ash/display/display_con...
File ash/display/display_configuration_controller.h (right):

https://codereview.chromium.org/1594683002/diff/80001/ash/display/display_con...
ash/display/display_configuration_controller.h:59: void
OnDisplayConfigurationChanged() override;
On 2016/01/20 02:02:51, oshima wrote:
> We probably should stop all animations if there is a display configuration
> change due to connect/disconnect. Can you add TODO?

Done (in the C++ file).

[oshima, 2016-01-20 02:26:40]

lgtm

[stevenjb, 2016-01-20 18:00:47]

stevenjb@chromium.org changed reviewers:
+ rockot@chromium.org

[stevenjb, 2016-01-20 18:00:47]

+rockot@chromium.org for c/b/extensions OWNER
+isherman@chromium.org for actions.xml

[stevenjb, 2016-01-20 18:34:38]

stevenjb@chromium.org changed reviewers:
+ isherman@chromium.org

[stevenjb, 2016-01-20 18:34:47]

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

[commit-bot: I haz the power, 2016-01-20 18:36:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1594683002/70021
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1594683002/70021

[Ken Rockot(use gerrit already), 2016-01-20 18:40:54]

rs lgtm

[commit-bot: I haz the power, 2016-01-20 19:45:58]

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

[commit-bot: I haz the power, 2016-01-20 19:45:58]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2016-01-20 21:32:41]

stevenjb@chromium.org changed reviewers:
+ jwd@chromium.org, mpearson@chromium.org

[stevenjb, 2016-01-20 21:32:41]

+mpearson@, jwd@ for a quick OWNERS review of actions.xml, thanks!

[Ilya Sherman, 2016-01-20 23:13:27]

actions.xml lgtm

[stevenjb, 2016-01-20 23:24:55]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-01-20 23:24:55]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/1594683002/#ps70021
(title: "Rebase")

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

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

[commit-bot: I haz the power, 2016-01-20 23:38:24]

Description was changed from

==========
Introduce DisplayConfigurationController

This class provides a consistent interface for UI and extension API
display configuration changes. It handles any animation transtions,
extracting animation code from DisplayManager and WindowTreeHostManager.

This also cleans up some of the code in DisplayOptionsHandler.


BUG=576375
==========

to

==========
Introduce DisplayConfigurationController

This class provides a consistent interface for UI and extension API
display configuration changes. It handles any animation transtions,
extracting animation code from DisplayManager and WindowTreeHostManager.

This also cleans up some of the code in DisplayOptionsHandler.


BUG=576375
==========

[commit-bot: I haz the power, 2016-01-20 23:38:25]

Committed patchset #8 (id:70021)

[commit-bot: I haz the power, 2016-01-20 23:39:31]

Description was changed from

==========
Introduce DisplayConfigurationController

This class provides a consistent interface for UI and extension API
display configuration changes. It handles any animation transtions,
extracting animation code from DisplayManager and WindowTreeHostManager.

This also cleans up some of the code in DisplayOptionsHandler.


BUG=576375
==========

to

==========
Introduce DisplayConfigurationController

This class provides a consistent interface for UI and extension API
display configuration changes. It handles any animation transtions,
extracting animation code from DisplayManager and WindowTreeHostManager.

This also cleans up some of the code in DisplayOptionsHandler.

BUG=576375

Committed: https://crrev.com/537c5c3f3a4f96823e5923694eda37a34add3cbe
Cr-Commit-Position: refs/heads/master@{#370528}
==========

[commit-bot: I haz the power, 2016-01-20 23:39:31]

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