Add display task to trigger display configuration

Add display task to trigger display configuration

This CL adds:
 1) A DisplayLayoutManager interface which will be implemented within
 DisplayConfigurator in a followup CL. This is in charge of providing
 enough information to the task to pick the correct display state.
 2) A task that will query to update the display list, pick the correct
 display state via DisplayLayoutManager then configure the displays
 via the ConfigureDisplaysTask.
 3) Basic unittests to validate the task. These are only basic since
 the DisplayConfigurator unittests are performing the same tests. Ideally
 the DisplayLayoutManager code in DisplayConfigurator could be extracted
 in its own class such that these unittests can re-use that logic.

BUG=429746
TBR=spang@chromium.org

Committed: https://crrev.com/6c7a24684066c2a9db4212546fe97a180cb240bb
Cr-Commit-Position: refs/heads/master@{#308033}

[dnicoara, 2014-12-11 01:02:39]

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

[dnicoara, 2014-12-11 01:02:40]

PTAL

derat@ for everything
spang@ for ui/ozone/*

[Daniel Erat, 2014-12-11 22:01:13]

Screenshot of my comments since Rietveld is broken.

​

On Wed, Dec 10, 2014 at 6:02 PM, <dnicoara@chromium.org> wrote:

> Reviewers: Daniel Erat, spang,
>
> Message:
> PTAL
>
> derat@ for everything
> spang@ for ui/ozone/*
>
> Description:
> Add display task to trigger display configuration
>
> This CL adds:
>  1) A DisplayLayoutManager interface which will be implemented within
>  DisplayConfigurator in a followup CL. This is in charge of providing
>  enough information to the task to pick the correct display state.
>  2) A task that will query to update the display list, pick the correct
>  display state via DisplayLayoutManager then configure the displays
>  via the ConfigureDisplaysTask.
>  3) Basic unittests to validate the task. These are only basic since
>  the DisplayConfigurator unittests are performing the same tests. Ideally
>  the DisplayLayoutManager code in DisplayConfigurator could be extracted
>  in its own class such that these unittests can re-use that logic.
>
> BUG=429746
>
> Please review this at https://codereview.chromium.org/788423002/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@async-
> refactor3
>
> Affected files (+890, -65 lines):
>   M ui/display/BUILD.gn
>   M ui/display/chromeos/display_configurator.h
>   M ui/display/chromeos/display_configurator.cc
>   A ui/display/chromeos/display_util.h
>   A ui/display/chromeos/display_util.cc
>   M ui/display/chromeos/test/test_native_display_delegate.h
>   M ui/display/chromeos/test/test_native_display_delegate.cc
>   A ui/display/chromeos/update_display_configuration_task.h
>   A ui/display/chromeos/update_display_configuration_task.cc
>   A ui/display/chromeos/update_display_configuration_task_unittest.cc
>   M ui/display/chromeos/x11/native_display_delegate_x11.h
>   M ui/display/chromeos/x11/native_display_delegate_x11.cc
>   M ui/display/display.gyp
>   M ui/display/types/native_display_delegate.h
>   M ui/ozone/common/native_display_delegate_ozone.h
>   M ui/ozone/common/native_display_delegate_ozone.cc
>   M ui/ozone/platform/dri/native_display_delegate_dri.h
>   M ui/ozone/platform/dri/native_display_delegate_dri.cc
>   M ui/ozone/platform/dri/native_display_delegate_proxy.h
>   M ui/ozone/platform/dri/native_display_delegate_proxy.cc
>
>
>

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

[dnicoara, 2014-12-11 22:27:55]

PTAL, I've went and updated it based on your suggestions.

https://codereview.chromium.org/788423002/diff/80001/ui/display/chromeos/upda.
..
File ui/display/chromeos/update_display_configuration_task.cc (right):

https://codereview.chromium.org/788423002/diff/80001/ui/display/chromeos/upda.
..
ui/display/chromeos/update_display_configuration_task.cc:33:
delegate_->GrabServer();
Moved it into the constructor for symmetry.

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

[Daniel Erat, 2014-12-11 23:30:45]

lgtm

(still just getting "Details: ApplicationError: 1 Internal error
[ApplicationError]" when i try to send mail via rietveld)

On Thu, Dec 11, 2014 at 3:27 PM, Daniel Nicoara <dnicoara@chromium.org>
wrote:

> PTAL, I've went and updated it based on your suggestions.
>
>
>
https://codereview.chromium.org/788423002/diff/80001/ui/display/chromeos/upda.
> ..
> File ui/display/chromeos/update_display_configuration_task.cc (right):
>
>
>
https://codereview.chromium.org/788423002/diff/80001/ui/display/chromeos/upda.
> ..
> ui/display/chromeos/update_display_configuration_task.cc:33:
> delegate_->GrabServer();
> Moved it into the constructor for symmetry.
>

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

[Daniel Erat, 2014-12-11 23:34:07]

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

https://codereview.chromium.org/788423002/diff/60001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.h:113: // Parses the displays and adds
|mirror_mode| and |selected_mode|.
the mirror_mode and selected_mode mentions seem confusing; there aren't any
arguments or members with those names. are those fields within DisplayState? if
so, i'd probably say something like "... and sets mirrored and selected modes
within the return DisplayStates." instead

https://codereview.chromium.org/788423002/diff/60001/ui/display/chromeos/disp...
File ui/display/chromeos/display_util.cc (right):

https://codereview.chromium.org/788423002/diff/60001/ui/display/chromeos/disp...
ui/display/chromeos/display_util.cc:13: // Returns a string describing |state|.
nit: these functions are already commented in the header; delete the comments in
this file

https://codereview.chromium.org/788423002/diff/60001/ui/display/chromeos/disp...
File ui/display/chromeos/display_util.h (right):

https://codereview.chromium.org/788423002/diff/60001/ui/display/chromeos/disp...
ui/display/chromeos/display_util.h:20: std::string
DisplayStateToString(MultipleDisplayState state);
MultipleDisplayStateToString?

https://codereview.chromium.org/788423002/diff/60001/ui/display/chromeos/upda...
File ui/display/chromeos/update_display_configuration_task.cc (right):

https://codereview.chromium.org/788423002/diff/60001/ui/display/chromeos/upda...
ui/display/chromeos/update_display_configuration_task.cc:36:
delegate_->UngrabServer();
you shouldn't do this unless Run() was called

https://codereview.chromium.org/788423002/diff/60001/ui/display/chromeos/upda...
File ui/display/chromeos/update_display_configuration_task.h (right):

https://codereview.chromium.org/788423002/diff/60001/ui/display/chromeos/upda...
ui/display/chromeos/update_display_configuration_task.h:80: int power_flags_;
nit: either document the flags here or point at the place where they are
documented

[Daniel Erat, 2014-12-11 23:35:06]

lgtm

hooray for rietveld yelling at me for not using the lgtm message after yelling
at me every time i tried to send my comments

[dnicoara, 2014-12-12 01:48:23]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2014-12-12 01:50:14]

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

[commit-bot: I haz the power, 2014-12-12 01:56:54]

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

[commit-bot: I haz the power, 2014-12-12 01:56:55]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dnicoara, 2014-12-12 02:26:20]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2014-12-12 02:27:47]

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

[commit-bot: I haz the power, 2014-12-12 02:38:43]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-12-12 02:39:24]

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