Allow switching between extended and unified

Allow switching between extended and unified

Add option in display settings page to switch the mode

BUG=489809
TEST=manual + DisplayManagerTest.UnifiedDesktopBasic

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

[oshima, 2015-05-19 23:06:16]

Patchset #2 (id:20001) has been deleted

[oshima, 2015-05-19 23:06:29]

Patchset #2 (id:20001) has been deleted

[oshima, 2015-05-19 23:23:51]

Patchset #3 (id:60001) has been deleted

[oshima, 2015-05-19 23:28:06]

Patchset #2 (id:40001) has been deleted

[oshima, 2015-05-19 23:28:09]

Patchset #1 (id:1) has been deleted

[oshima, 2015-05-19 23:28:18]

Patchset #2 (id:80001) has been deleted

[oshima, 2015-05-19 23:28:21]

Patchset #1 (id:40001) has been deleted

[oshima, 2015-05-19 23:29:25]

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

[oshima, 2015-05-19 23:55:15]

https://codereview.chromium.org/1126933004/diff/120001/ash/display/display_ma...
File ash/display/display_manager.h (right):

https://codereview.chromium.org/1126933004/diff/120001/ash/display/display_ma...
ash/display/display_manager.h:303: void ReconfigureDisplays();
I tried to keep the CL minimum to make merge easier. I'll refactor and simplify
the code once the feature is settled on 44.

[Jun Mukai, 2015-05-20 02:47:49]

mostly style nits.
To be clear, right now the mode hasn't stored into the settings. That would be
done in a following CL?

https://codereview.chromium.org/1126933004/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/options/chromeos/display_options.html (right):

https://codereview.chromium.org/1126933004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/display_options.html:23: <div
class="selected-display-option-title">
Can you put a comment for why this is empty?
<!-- intentionally blank for the title column space. -->

https://codereview.chromium.org/1126933004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/display_options.html:26: <span
i18n-content="enableUnifiedDesktop"></span>
label for="display-options-toggle-unified-desktop" instead of span, so that
clicking on the text also toggles the checkbox.

https://codereview.chromium.org/1126933004/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/options/chromeos/display_options.js (right):

https://codereview.chromium.org/1126933004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/display_options.js:269:
this.show_unified_desktop_option_ == showUnifiedDesktop) {
name should be camelcased. showUnifiedDesktopOption_ = ...

https://codereview.chromium.org/1126933004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/display_options.js:957: * @param
{string} multi display mode: one of 'mirror', 'unified',
It's better to be an enum rather than a string.  See the usage of
SecondaryDisplayLayout.

https://codereview.chromium.org/1126933004/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

https://codereview.chromium.org/1126933004/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:274:
(display_manager->IsInUnifiedMode() ? "unified" : "extended"));
Just for safety,
if (mode == "unified") DCHECK(ash::switches::UnifiedDesktopEnabled());
?

(or setting to "mirror" or "extended" if
!ash::switches::UnifiedDesktopEnabled()) ?

https://codereview.chromium.org/1126933004/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:489: const
base::ListValue* args) {
Also DCHECK(ash::switches::UnifiedDesktopEnabled()) (or return if not)

[oshima, 2015-05-20 06:19:27]

Patchset #3 (id:140001) has been deleted

[oshima, 2015-05-20 06:40:01]

I'm working on saving status although it may not be merged to 44 (change may be
too big to merge).

https://codereview.chromium.org/1126933004/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/options/chromeos/display_options.html (right):

https://codereview.chromium.org/1126933004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/display_options.html:23: <div
class="selected-display-option-title">
On 2015/05/20 02:47:49, Jun Mukai wrote:
> Can you put a comment for why this is empty?
> <!-- intentionally blank for the title column space. -->

Done.

https://codereview.chromium.org/1126933004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/display_options.html:26: <span
i18n-content="enableUnifiedDesktop"></span>
On 2015/05/20 02:47:49, Jun Mukai wrote:
> label for="display-options-toggle-unified-desktop" instead of span, so that
> clicking on the text also toggles the checkbox.

Changed to use label, although I followed the other example as suggested by
presubmit. (label for="" seems to be banned)

https://codereview.chromium.org/1126933004/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/options/chromeos/display_options.js (right):

https://codereview.chromium.org/1126933004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/display_options.js:269:
this.show_unified_desktop_option_ == showUnifiedDesktop) {
On 2015/05/20 02:47:49, Jun Mukai wrote:
> name should be camelcased. showUnifiedDesktopOption_ = ...

Done.

https://codereview.chromium.org/1126933004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/display_options.js:957: * @param
{string} multi display mode: one of 'mirror', 'unified',
On 2015/05/20 02:47:49, Jun Mukai wrote:
> It's better to be an enum rather than a string.  See the usage of
> SecondaryDisplayLayout.

Done.

https://codereview.chromium.org/1126933004/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

https://codereview.chromium.org/1126933004/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:274:
(display_manager->IsInUnifiedMode() ? "unified" : "extended"));
On 2015/05/20 02:47:49, Jun Mukai wrote:
> Just for safety,
> if (mode == "unified") DCHECK(ash::switches::UnifiedDesktopEnabled());
> ?
> 
> (or setting to "mirror" or "extended" if
> !ash::switches::UnifiedDesktopEnabled()) ?

This is actually guarded by ash::switch, and DCHECK beflow, so I dont' think
that's helpful.

https://codereview.chromium.org/1126933004/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:489: const
base::ListValue* args) {
On 2015/05/20 02:47:49, Jun Mukai wrote:
> Also DCHECK(ash::switches::UnifiedDesktopEnabled()) (or return if not)

Done.

[oshima, 2015-05-20 06:40:12]

Patchset #3 (id:160001) has been deleted

[Jun Mukai, 2015-05-20 07:34:20]

lgtm

https://codereview.chromium.org/1126933004/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/options/chromeos/display_options.html (right):

https://codereview.chromium.org/1126933004/diff/120001/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/display_options.html:26: <span
i18n-content="enableUnifiedDesktop"></span>
On 2015/05/20 06:40:01, oshima wrote:
> On 2015/05/20 02:47:49, Jun Mukai wrote:
> > label for="display-options-toggle-unified-desktop" instead of span, so that
> > clicking on the text also toggles the checkbox.
> 
> Changed to use label, although I followed the other example as suggested by
> presubmit. (label for="" seems to be banned)

Oh, I didn't know that details.  Enclosing by <label> looks good.

[oshima, 2015-05-20 15:39:06]

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

[oshima, 2015-05-20 15:39:07]

xiyuan@chromium.org -> chrome/browser/resources/options/browser_options.js owner

[oshima, 2015-05-20 15:39:13]

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

[commit-bot: I haz the power, 2015-05-20 15:39:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1126933004/180001

[xiyuan, 2015-05-20 15:52:44]

lgtm

[commit-bot: I haz the power, 2015-05-20 16:49:11]

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

[commit-bot: I haz the power, 2015-05-20 16:49:12]

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

[oshima, 2015-05-20 17:13:16]

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

[oshima, 2015-05-20 17:13:17]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
mukai@chromium.org
Link to the patchset: https://codereview.chromium.org/1126933004/#ps200001
(title: " ")

[commit-bot: I haz the power, 2015-05-20 17:14:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1126933004/200001

[commit-bot: I haz the power, 2015-05-20 19:09:51]

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

[commit-bot: I haz the power, 2015-05-20 19:09:52]

Dry run: This issue passed the CQ dry run.

[oshima, 2015-05-20 20:12:13]

Patchset #5 (id:220001) has been deleted

[oshima, 2015-05-20 20:12:19]

Patchset #4 (id:200001) has been deleted

[oshima, 2015-05-20 20:15:41]

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

[oshima, 2015-05-20 20:15:41]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
mukai@chromium.org
Link to the patchset: https://codereview.chromium.org/1126933004/#ps240001
(title: "added div for class=checkbox, with controlled-setting-with-label +
inline-block")

[commit-bot: I haz the power, 2015-05-20 20:16:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1126933004/240001

[xiyuan, 2015-05-20 20:17:52]

https://codereview.chromium.org/1126933004/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/options/chromeos/display_options.css (right):

https://codereview.chromium.org/1126933004/diff/240001/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/display_options.css:76: .xxx {
Is this needed?

https://codereview.chromium.org/1126933004/diff/240001/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/display_options.css:108:
.controlled-setting-with-label,
This line has side effect because it applies to all elements with
controlled-setting-with-label class. Do we need it?

https://codereview.chromium.org/1126933004/diff/240001/chrome/browser/resourc...
File chrome/browser/resources/options/chromeos/display_options.html (right):

https://codereview.chromium.org/1126933004/diff/240001/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/display_options.html:28: <input
id="display-options-toggle-unified-desktop" type="checkbox">
nit: wrap type="checkbox" to the next line.

[xiyuan, 2015-05-20 20:27:21]

LGTM!

[oshima, 2015-05-20 20:39:16]

The CQ bit was checked by oshima@chromium.org

[oshima, 2015-05-20 20:39:16]

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

[commit-bot: I haz the power, 2015-05-20 20:40:18]

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

[commit-bot: I haz the power, 2015-05-20 22:05:04]

Committed patchset #5 (id:260001)

[commit-bot: I haz the power, 2015-05-20 22:06:00]

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