chromeos: Support turning displays off in extended mode.

chromeos: Support turning displays off in extended mode.

This makes OutputConfigurator support requests from powerd
that either the primary or secondary monitor be turned off
in extended desktop mode.  Previously, the panel always
remained on.

BUG=218387
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=191558

[Daniel Erat, 2013-03-25 20:02:42]

I've been testing this in conjunction with a powerd change that requests
INTERNAL_OFF_EXTERNAL_ON when a device's lid is closed while an external display
is connected.  I'm seeing a few issues, though:

- On Pixel, the mouse cursor doesn't switch to low-DPI when only the external
display is on; it uses the high-DPI assets instead.

- It looks like the pointer barriers aren't getting updated, either.  With only
the external display on, the cursor is warped to the right edge of the screen
when I push it off to the left.

- The framebuffer looks like it gets resized correctly when the panel is turned
off, but windows that were on the panel don't get moved to the external display.

- The image displayed on the panel looks strange for a fraction of a second when
I reopen the lid.  It looks like the low-DPI image from the external display is
being briefly shown on the panel.  I don't know whether there's much we can do
about this.

[marcheu, 2013-03-25 20:57:44]

lgtm

[oshima, 2013-03-25 21:36:04]

On 2013/03/25 20:57:44, marcheu wrote:
> lgtm

Alex said he'll update the bug with his opinion.

[oshima, 2013-03-25 22:20:40]

https://codereview.chromium.org/13006006/diff/1/chromeos/display/output_confi...
File chromeos/display/output_configurator.cc (left):

https://codereview.chromium.org/13006006/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator.cc:1146: // windows) onto internal display
even when it's off.
I'm probably missing something but it sounds to me that this comment
contradicts the code below. That is, primary_power_on is always false when
power_state is DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON?

I know you're changing this code, but I'd like to understand how this CL changes
exiting behavior correctly.

https://codereview.chromium.org/13006006/diff/1/chromeos/display/output_confi...
File chromeos/display/output_configurator.cc (right):

https://codereview.chromium.org/13006006/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator.cc:1207: if (output_state ==
STATE_DUAL_EXTENDED) {
If we have two displays and not mirrored, it must be extended. Probably legacy
code that we no longer need?

[Daniel Erat, 2013-03-26 03:19:15]

Oshima, do you have any thoughts about the other issues that I mentioned in the
initial email message?

https://codereview.chromium.org/13006006/diff/1/chromeos/display/output_confi...
File chromeos/display/output_configurator.cc (left):

https://codereview.chromium.org/13006006/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator.cc:1146: // windows) onto internal display
even when it's off.
On 2013/03/25 22:20:40, oshima wrote:
> I'm probably missing something but it sounds to me that this comment
> contradicts the code below. That is, primary_power_on is always false when
> power_state is DISPLAY_POWER_INTERNAL_OFF_EXTERNAL_ON?
> 
> I know you're changing this code, but I'd like to understand how this CL
changes
> exiting behavior correctly.

Yes, I was confused by this too.  The old code seems wrong but I think it had
the desired effect -- on laptops, the internal panel is always the primary
display, right?  If so, then the primary display would be turned off for the
other two states, INTERNAL_ON_EXTERNAL_OFF and ALL_OFF, as expected.

secondary_power_on seems more problematic, since it looks like an external
secondary display would be turned off for INTERNAL_OFF_EXTERNAL_ON instead of
being kept on.

https://codereview.chromium.org/13006006/diff/1/chromeos/display/output_confi...
File chromeos/display/output_configurator.cc (right):

https://codereview.chromium.org/13006006/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator.cc:1207: if (output_state ==
STATE_DUAL_EXTENDED) {
On 2013/03/25 22:20:40, oshima wrote:
> If we have two displays and not mirrored, it must be extended. Probably legacy
> code that we no longer need?

Yeah, I think the only other state is one representing unknown values.  I'm
happy to kill the else clause here if it's unneeded.

[Daniel Erat, 2013-03-26 20:00:07]

I've changed this to also clean up EnterState().  I still see problems with
pointer barriers not getting updated correctly and existing windows not getting
moved to the external display after the internal panel is turned off.

[oshima, 2013-03-26 23:20:31]

lgtm, but I think I first need to fix chrome before landing this. I'll let you
know when ready.

https://codereview.chromium.org/13006006/diff/8001/chromeos/display/output_co...
File chromeos/display/output_configurator.cc (right):

https://codereview.chromium.org/13006006/diff/8001/chromeos/display/output_co...
chromeos/display/output_configurator.cc:1196: // Avoid ending up with 0-by-0 if
all outputs are off.
// Keep the full screen size if all outputs are off so that we can resume with
same desktop.

(or something like this) would be better.

[Daniel Erat, 2013-03-26 23:23:16]

Thanks.

https://codereview.chromium.org/13006006/diff/8001/chromeos/display/output_co...
File chromeos/display/output_configurator.cc (right):

https://codereview.chromium.org/13006006/diff/8001/chromeos/display/output_co...
chromeos/display/output_configurator.cc:1196: // Avoid ending up with 0-by-0 if
all outputs are off.
On 2013/03/26 23:20:31, oshima wrote:
> // Keep the full screen size if all outputs are off so that we can resume with
> same desktop.
> 
> (or something like this) would be better.

Done.

[oshima, 2013-03-26 23:58:50]

By the way, does this work after a user logs out?
I guess it probably starts as dual display?

[Daniel Erat, 2013-03-27 23:15:19]

I made some fixes to the suspend/resume code that I think should make us
preserve the old power state correctly.

https://codereview.chromium.org/13006006/diff/21001/chromeos/display/output_c...
File chromeos/display/output_configurator.cc (right):

https://codereview.chromium.org/13006006/diff/21001/chromeos/display/output_c...
chromeos/display/output_configurator.cc:771: true /*
only_if_single_internal_display */);
I think that we can only do this safely when there's just one internal display
connected.  Otherwise, we'll either turn external displays on as well (which the
user will be able to see) or resize the desktop, which we don't want to do here.

https://codereview.chromium.org/13006006/diff/21001/chromeos/display/output_c...
File chromeos/display/output_configurator.h (right):

https://codereview.chromium.org/13006006/diff/21001/chromeos/display/output_c...
chromeos/display/output_configurator.h:113: bool
only_if_single_internal_display);
I would've preferred to let callers deal with the "only if single internal"
check, but that seems tricky due us needing to reprobe every time this is called
-- I think that improving that is a bit more than we should try to deal with for
27.

[oshima, 2013-03-28 20:35:55]

lgtm.

https://codereview.chromium.org/13006006/diff/21001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/dbus/display_power_service_provider.cc (right):

https://codereview.chromium.org/13006006/diff/21001/chrome/browser/chromeos/d...
chrome/browser/chromeos/dbus/display_power_service_provider.cc:65: state, false,
false);
can you add comments for what these false means like /* force probe */ ?
enum would be better but it'll leave it to you.

[Daniel Erat, 2013-03-28 23:09:40]

https://codereview.chromium.org/13006006/diff/21001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/dbus/display_power_service_provider.cc (right):

https://codereview.chromium.org/13006006/diff/21001/chrome/browser/chromeos/d...
chrome/browser/chromeos/dbus/display_power_service_provider.cc:65: state, false,
false);
On 2013/03/28 20:35:55, oshima wrote:
> can you add comments for what these false means like /* force probe */ ?
> enum would be better but it'll leave it to you.

I've added constants for different flags.  (Haven't been able to test it on a
device yet due to http://crbug.com/224794.)

[commit-bot: I haz the power, 2013-03-30 17:09:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/derat@chromium.org/13006006/33001

[commit-bot: I haz the power, 2013-03-30 21:22:40]

Change committed as 191558