Update DisplayConfigurator to use the asynchronous tasks

Update DisplayConfigurator to use the asynchronous tasks

Now DisplayConfigurator can take advantage of asynchronous configuration APIs.
Note: For X11, NativeDisplayDelegate is still implemented via synchronous calls
so all configurations will be synchronous on X11.

Notable changes:
 1) EnterState(), UpdateCachedDisplays() and FindMirrorMode() have been moved
into the DisplayLayoutManager implementation.
 2) NativeDisplayDelegate's initialization has been moved before grabbing the
server. Shouldn't cause any issues since the server doesn't need to be grabbed
when initializing NDD.

BUG=429746
TEST=display_unittests and manually on Link with X11

Committed: https://crrev.com/7547705543d6b711a166e513a43a4e43f93fd254
Cr-Commit-Position: refs/heads/master@{#308175}

[dnicoara, 2014-12-11 20:51:51]

dnicoara@chromium.org changed reviewers:
+ derat@chromium.org

[dnicoara, 2014-12-11 20:51:52]

Depends on https://codereview.chromium.org/788423002/

PTAL. This CL adds the last bits needed to asynchronously configure displays.

[Daniel Erat, 2014-12-11 23:15:54]

to make this a bit easier to review, could you provide some quick notes about
which parts are new code vs. moved code?

[dnicoara, 2014-12-12 01:45:36]

On 2014/12/11 23:15:54, Daniel Erat wrote:
> to make this a bit easier to review, could you provide some quick notes about
> which parts are new code vs. moved code?

Sure, everything in DisplayLayoutManagerImpl is just moved code with minimal
changes to conform with the interfaces.

The important parts are in the DisplayConfigurator functions
(ForceInitialConfigure, SetDisplayPower, SetDisplayMode, ConfigureDisplays,
RunPendingConfiguration, OnConfigured).

[Daniel Erat, 2014-12-12 16:25:29]

thanks. some comments but i'm generally happier with this approach. :-)

https://codereview.chromium.org/801493002/diff/20001/ui/display/chromeos/disp...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/801493002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.cc:85: // mode wasn't found or created.
nit: fix wrapping

https://codereview.chromium.org/801493002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.cc:89: // it may lead to a crash if
|internal_info| is not capable of panel fitting).
s/internal_info/internal_display/ ?

https://codereview.chromium.org/801493002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.cc:136: for (size_t i = 0; i <
snapshots.size(); ++i) {
nit: for (auto snapshot : snapshots) ?

https://codereview.chromium.org/801493002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.cc:167: LOG_IF(WARNING,
internal_displays == 2)
nit: >= 2 ?

https://codereview.chromium.org/801493002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.cc:177: if (one_is_internal) {
nit: to remove duplication, maybe rewrite as:

  can_mirror = FindMirrorMode(
      &cached_displays[one_is_internal ? 0 : 1],
      &cached_displays[one_is_internal ? 1 : 0],
      configurator_->is_panel_fitting_enabled_,
      preserve_aspect);

https://codereview.chromium.org/801493002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.cc:879: 
nit: check that the requested_ fields don't have the invalid values from below
(checking requested_display_state_ != MULTIPLE_DISPLAY_STATE_INVALID is probably
sufficient) and consider logging an error and bailing out

https://codereview.chromium.org/801493002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.cc:886: // Reset the flags before
running the task otherwise it may end up scheduling
nit: "... before running the task; otherwise ..."

https://codereview.chromium.org/801493002/diff/20001/ui/display/chromeos/disp...
File ui/display/chromeos/display_configurator.h (right):

https://codereview.chromium.org/801493002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.h:320: void RunPendingConfiguration();
please add comments describing these new methods

https://codereview.chromium.org/801493002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.h:360: int requested_power_flags_;
add a comment describing where the flag values are defined

https://codereview.chromium.org/801493002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.h:369: gfx::Size framebuffer_size_;
is this the current framebuffer size?

[dnicoara, 2014-12-12 17:45:09]

Thanks, I've updated the CL.

https://codereview.chromium.org/801493002/diff/20001/ui/display/chromeos/disp...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/801493002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.cc:85: // mode wasn't found or created.
On 2014/12/12 16:25:28, Daniel Erat wrote:
> nit: fix wrapping

Done.

https://codereview.chromium.org/801493002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.cc:89: // it may lead to a crash if
|internal_info| is not capable of panel fitting).
On 2014/12/12 16:25:28, Daniel Erat wrote:
> s/internal_info/internal_display/ ?

Done.

https://codereview.chromium.org/801493002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.cc:136: for (size_t i = 0; i <
snapshots.size(); ++i) {
On 2014/12/12 16:25:29, Daniel Erat wrote:
> nit: for (auto snapshot : snapshots) ?

Done.

https://codereview.chromium.org/801493002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.cc:167: LOG_IF(WARNING,
internal_displays == 2)
On 2014/12/12 16:25:28, Daniel Erat wrote:
> nit: >= 2 ?

Done.

https://codereview.chromium.org/801493002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.cc:177: if (one_is_internal) {
On 2014/12/12 16:25:29, Daniel Erat wrote:
> nit: to remove duplication, maybe rewrite as:
> 
>   can_mirror = FindMirrorMode(
>       &cached_displays[one_is_internal ? 0 : 1],
>       &cached_displays[one_is_internal ? 1 : 0],
>       configurator_->is_panel_fitting_enabled_,
>       preserve_aspect);

Done.

https://codereview.chromium.org/801493002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.cc:879: 
On 2014/12/12 16:25:29, Daniel Erat wrote:
> nit: check that the requested_ fields don't have the invalid values from below
> (checking requested_display_state_ != MULTIPLE_DISPLAY_STATE_INVALID is
probably
> sufficient) and consider logging an error and bailing out

I've slightly changed ShouldRunConfigurationTask() and added an additional check
here.

https://codereview.chromium.org/801493002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.cc:886: // Reset the flags before
running the task otherwise it may end up scheduling
On 2014/12/12 16:25:29, Daniel Erat wrote:
> nit: "... before running the task; otherwise ..."

Done.

https://codereview.chromium.org/801493002/diff/20001/ui/display/chromeos/disp...
File ui/display/chromeos/display_configurator.h (right):

https://codereview.chromium.org/801493002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.h:320: void RunPendingConfiguration();
On 2014/12/12 16:25:29, Daniel Erat wrote:
> please add comments describing these new methods

Done.

https://codereview.chromium.org/801493002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.h:360: int requested_power_flags_;
On 2014/12/12 16:25:29, Daniel Erat wrote:
> add a comment describing where the flag values are defined

Done.

https://codereview.chromium.org/801493002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.h:369: gfx::Size framebuffer_size_;
On 2014/12/12 16:25:29, Daniel Erat wrote:
> is this the current framebuffer size?

Yes, I've added a comment.

[Daniel Erat, 2014-12-12 20:36:30]

lgtm

https://codereview.chromium.org/801493002/diff/40001/ui/display/chromeos/disp...
File ui/display/chromeos/display_configurator.h (right):

https://codereview.chromium.org/801493002/diff/40001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.h:320: // If |configuration_task_|
isn't initialized, initialize it and start the
nit: s/initialize/initializes/, s/start/starts/

[dnicoara, 2014-12-12 20:56:24]

https://codereview.chromium.org/801493002/diff/40001/ui/display/chromeos/disp...
File ui/display/chromeos/display_configurator.h (right):

https://codereview.chromium.org/801493002/diff/40001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.h:320: // If |configuration_task_|
isn't initialized, initialize it and start the
On 2014/12/12 20:36:30, Daniel Erat wrote:
> nit: s/initialize/initializes/, s/start/starts/

Done.

[dnicoara, 2014-12-12 21:24:04]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2014-12-12 21:25:23]

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

[commit-bot: I haz the power, 2014-12-12 22:00:49]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-12-12 22:02:23]

Patchset 4 (id:??) landed as
https://crrev.com/7547705543d6b711a166e513a43a4e43f93fd254
Cr-Commit-Position: refs/heads/master@{#308175}

[samuong, 2014-12-12 23:45:34]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/797383002/ by samuong@chromium.org.

The reason for reverting is: seems to be causing problems in display_unittests
on the following bot:
https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2....