MD Settings: Display: Add unified desktop control and modify api

MD Settings: Display: Add unified desktop control and modify api

This CL:
* Updates the chrome.system.display API to support unified
  desktop for Settings UI.
* Adds a unified desktop setting to the MD Settings UI when
  the flag is enabled.

BUG=703423
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2802603005
Cr-Commit-Position: refs/heads/master@{#463865}
Committed: https://chromium.googlesource.com/chromium/src/+/717b7c3c86906d088c16fb30f4588fb0a0ea15e3

[stevenjb, 2017-04-05 19:52:36]

Description was changed from

==========
MD Settings: Display: Add unified desktop control and fix api

This CL:
* FIxes the chrome.system.display API to correctly support
  unified desktop.
* Adds a unified desktop setting to the MD Settings UI when
  the flag is enabled.

BUG=703423
==========

to

==========
MD Settings: Display: Add unified desktop control and fix api

This CL:
* FIxes the chrome.system.display API to correctly support
  unified desktop.
* Adds a unified desktop setting to the MD Settings UI when
  the flag is enabled.

BUG=703423
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[stevenjb, 2017-04-05 19:56:16]

stevenjb@chromium.org changed reviewers:
+ malaykeshav@chromium.org, rdevlin.cronin@chromium.org, xiyuan@chromium.org

[stevenjb, 2017-04-05 19:56:17]

rdevlin.cronin@ for extensions owner

[stevenjb, 2017-04-05 19:56:31]

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

[commit-bot: I haz the power, 2017-04-05 19:57:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 20:11:25]

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

[commit-bot: I haz the power, 2017-04-05 20:11:26]

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

[xiyuan, 2017-04-05 21:41:46]

lgtm

Just nits

https://codereview.chromium.org/2802603005/diff/1/chrome/browser/extensions/d...
File chrome/browser/extensions/display_info_provider_chromeos.cc (right):

https://codereview.chromium.org/2802603005/diff/1/chrome/browser/extensions/d...
chrome/browser/extensions/display_info_provider_chromeos.cc:652:
unit_info.is_unified = true;
Can you document why we are doing this? I was puzzled initially then figured
that out after I saw line 638-639 that this code only runs for unified mode.

https://codereview.chromium.org/2802603005/diff/1/third_party/closure_compile...
File third_party/closure_compiler/externs/system_display.js (right):

https://codereview.chromium.org/2802603005/diff/1/third_party/closure_compile...
third_party/closure_compiler/externs/system_display.js:248: *     caller that
the touch      calibration has ended. The argument of the
nit: The space between "touch" and "calibration" here and similar one in the
next line looks strange.

https://codereview.chromium.org/2802603005/diff/1/third_party/closure_compile...
third_party/closure_compiler/externs/system_display.js:272: *     touch
calibration was performed.     |bounds.left| and |bounds.top|
nit: remove extra spaces ?

https://codereview.chromium.org/2802603005/diff/1/third_party/closure_compile...
File third_party/closure_compiler/interfaces/system_display_interface.js
(right):

https://codereview.chromium.org/2802603005/diff/1/third_party/closure_compile...
third_party/closure_compiler/interfaces/system_display_interface.js:60: *    
$(ref:runtime.lastError) should be     queried.
nit: fix the extra spaces ?

https://codereview.chromium.org/2802603005/diff/1/third_party/closure_compile...
third_party/closure_compiler/interfaces/system_display_interface.js:121: *    
callback informs if the      calibration was a success or not.
nit: fix extra spaces ?

[malaykeshav, 2017-04-05 23:02:57]

https://codereview.chromium.org/2802603005/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/device_page/display.html (right):

https://codereview.chromium.org/2802603005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/device_page/display.html:151:
on-change="onOrientationChange_"">
remove extra "

https://codereview.chromium.org/2802603005/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/device_page/display.js (right):

https://codereview.chromium.org/2802603005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/device_page/display.js:282: unifiedDesktopMode
? 'displayMirrorOn' : 'displayMirrorOff');
Shouldn't they have a string of their own?
i18n probably needs a context for unified desktop that is different from display
mirror.

https://codereview.chromium.org/2802603005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/device_page/display.js:291: showMirror_:
function(unifiedDesktopMode, displays) {
'unifiedDesktopMode' unused?

[Devlin, 2017-04-06 00:03:26]

https://codereview.chromium.org/2802603005/diff/1/extensions/common/api/syste...
File extensions/common/api/system_display.idl (right):

https://codereview.chromium.org/2802603005/diff/1/extensions/common/api/syste...
extensions/common/api/system_display.idl:132: // True if this display is in
unified desktop mode.
Is it common knowledge what this is?

[stevenjb, 2017-04-06 01:56:09]

PTAL

https://codereview.chromium.org/2802603005/diff/1/chrome/browser/extensions/d...
File chrome/browser/extensions/display_info_provider_chromeos.cc (right):

https://codereview.chromium.org/2802603005/diff/1/chrome/browser/extensions/d...
chrome/browser/extensions/display_info_provider_chromeos.cc:652:
unit_info.is_unified = true;
On 2017/04/05 21:41:45, xiyuan wrote:
> Can you document why we are doing this? I was puzzled initially then figured
> that out after I saw line 638-639 that this code only runs for unified mode.

That was the intent of the comment at line 641 :) I'm not sure how much a
comment here will help.

https://codereview.chromium.org/2802603005/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/device_page/display.html (right):

https://codereview.chromium.org/2802603005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/device_page/display.html:151:
on-change="onOrientationChange_"">
On 2017/04/05 23:02:57, malaykeshav wrote:
> remove extra "

opps, thanks. done.

https://codereview.chromium.org/2802603005/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/device_page/display.js (right):

https://codereview.chromium.org/2802603005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/device_page/display.js:282: unifiedDesktopMode
? 'displayMirrorOn' : 'displayMirrorOff');
On 2017/04/05 23:02:57, malaykeshav wrote:
> Shouldn't they have a string of their own?
> i18n probably needs a context for unified desktop that is different from
display
> mirror.

Actually, we should use the more generic phase that we have since added for
toggles for both. If/when that becomes an issue we can create specific terms.

https://codereview.chromium.org/2802603005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/device_page/display.js:291: showMirror_:
function(unifiedDesktopMode, displays) {
On 2017/04/05 23:02:57, malaykeshav wrote:
> 'unifiedDesktopMode' unused?

Oops, it should be, thanks for catching. Done.

https://codereview.chromium.org/2802603005/diff/1/extensions/common/api/syste...
File extensions/common/api/system_display.idl (right):

https://codereview.chromium.org/2802603005/diff/1/extensions/common/api/syste...
extensions/common/api/system_display.idl:132: // True if this display is in
unified desktop mode.
On 2017/04/06 00:03:26, Devlin wrote:
> Is it common knowledge what this is?

None of this API is "common knowledge" :P Expanded the comment to reference the
enableUnifiedDesktop call FWIW.

https://codereview.chromium.org/2802603005/diff/1/third_party/closure_compile...
File third_party/closure_compiler/externs/system_display.js (right):

https://codereview.chromium.org/2802603005/diff/1/third_party/closure_compile...
third_party/closure_compiler/externs/system_display.js:248: *     caller that
the touch      calibration has ended. The argument of the
On 2017/04/05 21:41:45, xiyuan wrote:
> nit: The space between "touch" and "calibration" here and similar one in the
> next line looks strange.

This is generated.

https://codereview.chromium.org/2802603005/diff/1/third_party/closure_compile...
third_party/closure_compiler/externs/system_display.js:272: *     touch
calibration was performed.     |bounds.left| and |bounds.top|
On 2017/04/05 21:41:45, xiyuan wrote:
> nit: remove extra spaces ?

ditto.

https://codereview.chromium.org/2802603005/diff/1/third_party/closure_compile...
File third_party/closure_compiler/interfaces/system_display_interface.js
(right):

https://codereview.chromium.org/2802603005/diff/1/third_party/closure_compile...
third_party/closure_compiler/interfaces/system_display_interface.js:60: *    
$(ref:runtime.lastError) should be     queried.
On 2017/04/05 21:41:45, xiyuan wrote:
> nit: fix the extra spaces ?

ditto

https://codereview.chromium.org/2802603005/diff/1/third_party/closure_compile...
third_party/closure_compiler/interfaces/system_display_interface.js:121: *    
callback informs if the      calibration was a success or not.
On 2017/04/05 21:41:45, xiyuan wrote:
> nit: fix extra spaces ?

ditto

[Devlin, 2017-04-06 14:51:43]

idl lgtm if there are no issues with linkifying.

https://codereview.chromium.org/2802603005/diff/20001/extensions/common/api/s...
File extensions/common/api/system_display.idl (right):

https://codereview.chromium.org/2802603005/diff/20001/extensions/common/api/s...
extensions/common/api/system_display.idl:133: // for
<code>enableUnifiedDesktop</code>).
I think that $(ref:enableUnifiedDesktop) should linkify this.  Can you try that
with the preview.py script?

[stevenjb, 2017-04-06 16:21:12]

https://codereview.chromium.org/2802603005/diff/20001/extensions/common/api/s...
File extensions/common/api/system_display.idl (right):

https://codereview.chromium.org/2802603005/diff/20001/extensions/common/api/s...
extensions/common/api/system_display.idl:133: // for
<code>enableUnifiedDesktop</code>).
On 2017/04/06 14:51:43, Devlin wrote:
> I think that $(ref:enableUnifiedDesktop) should linkify this.  Can you try
that
> with the preview.py script?

Done.

[stevenjb, 2017-04-06 16:21:30]

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

[commit-bot: I haz the power, 2017-04-06 16:33:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 17:48:01]

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

[commit-bot: I haz the power, 2017-04-06 17:48:03]

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

[malaykeshav, 2017-04-06 18:37:36]

lgtm

[stevenjb, 2017-04-06 20:30:33]

stevenjb@chromium.org changed reviewers:
+ robert.bradford@intel.com

[stevenjb, 2017-04-06 20:30:33]

Please RE-REVIEW the API changes; it is a fairly significantly different
approach.

[stevenjb, 2017-04-06 20:32:49]

Description was changed from

==========
MD Settings: Display: Add unified desktop control and fix api

This CL:
* FIxes the chrome.system.display API to correctly support
  unified desktop.
* Adds a unified desktop setting to the MD Settings UI when
  the flag is enabled.

BUG=703423
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Display: Add unified desktop control and modify api

This CL:
* FIxes the chrome.system.display API to correctly support
  unified desktop.
* Adds a unified desktop setting to the MD Settings UI when
  the flag is enabled.

BUG=703423
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[stevenjb, 2017-04-06 20:33:17]

Description was changed from

==========
MD Settings: Display: Add unified desktop control and modify api

This CL:
* FIxes the chrome.system.display API to correctly support
  unified desktop.
* Adds a unified desktop setting to the MD Settings UI when
  the flag is enabled.

BUG=703423
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Display: Add unified desktop control and modify api

This CL:
* Updates the chrome.system.display API to support unified
  desktop for Settings UI.
* Adds a unified desktop setting to the MD Settings UI when
  the flag is enabled.

BUG=703423
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[stevenjb, 2017-04-11 15:45:37]

xuyuan@, malaykeshav@, ping

[xiyuan, 2017-04-11 15:57:56]

still lgtm

Sorry, thought I stamped it already.

[stevenjb, 2017-04-11 17:08:30]

You did :) But I made a non trivial change to the API so I requested a
re-review.


On Tue, Apr 11, 2017 at 9:57 AM, <xiyuan@chromium.org> wrote:

> still lgtm
>
> Sorry, thought I stamped it already.
>
> https://codereview.chromium.org/2802603005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[malaykeshav, 2017-04-11 17:29:13]

lgtm

https://codereview.chromium.org/2802603005/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/display_info_provider_chromeos.cc (right):

https://codereview.chromium.org/2802603005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/display_info_provider_chromeos.cc:250: *error =
"Unified desktop mode can not be set with mirroringSourceId.";
What does this error mean? 
It cannot be set when displays are mirrored?

[stevenjb, 2017-04-11 17:34:38]

https://codereview.chromium.org/2802603005/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/display_info_provider_chromeos.cc (right):

https://codereview.chromium.org/2802603005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/display_info_provider_chromeos.cc:250: *error =
"Unified desktop mode can not be set with mirroringSourceId.";
On 2017/04/11 17:29:12, malaykeshav wrote:
> What does this error mean? 
> It cannot be set when displays are mirrored?

We disallow setting both 'is_unified' and 'mirroring_source_id' at the same
time. (|info| is a dictionary of scoped_ptr, so testing info.mirroring_source_id
tests the *presence* of the property, not the value).

In theory we could allow this value to be empty if is_unified is true, or
anything if *info.is_unified is false (and vice versa), but it's simpler just to
require that only one value or the other be set at a time, rather than duplicate
the model logic here.

[stevenjb, 2017-04-11 17:34:44]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2017-04-11 17:34:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2802603005/#ps80001
(title: "git cl format")

[commit-bot: I haz the power, 2017-04-11 17:35:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 17:39:50]

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

[commit-bot: I haz the power, 2017-04-11 17:39:52]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[stevenjb, 2017-04-11 17:51:39]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2017-04-11 17:51:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
malaykeshav@chromium.org, xiyuan@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2802603005/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2017-04-11 17:52:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2017-04-11 18:54:08]

Sorry I didn't get this out sooner :/

https://codereview.chromium.org/2802603005/diff/100001/extensions/common/api/...
File extensions/common/api/system_display.idl (right):

https://codereview.chromium.org/2802603005/diff/100001/extensions/common/api/...
extensions/common/api/system_display.idl:173: // other properties may not apply.
Comment what the default is.

https://codereview.chromium.org/2802603005/diff/100001/extensions/common/api/...
extensions/common/api/system_display.idl:219: // $(ref:enableUnifiedDesktop)).
Comment also what the default is.

https://codereview.chromium.org/2802603005/diff/100001/extensions/common/api/...
extensions/common/api/system_display.idl:220: boolean? singleUnified;
singleUnified seems redundant to me - maybe singleDisplay or unifiedDisplay?

https://codereview.chromium.org/2802603005/diff/100001/extensions/common/api/...
extensions/common/api/system_display.idl:231: // |flags|: See
$(ref:GetInfoFlags).
similar to the others, |flags| will already have a type that indicates what it
is.  Instead, say something like "The options used to determine which
information to return".

https://codereview.chromium.org/2802603005/diff/100001/extensions/common/api/...
extensions/common/api/system_display.idl:232: // |callback|: This will be
invoked with an array of $(ref:DisplayUnitInfo).
same as 238 (sorry, wrote these backwards)

https://codereview.chromium.org/2802603005/diff/100001/extensions/common/api/...
extensions/common/api/system_display.idl:238: // |callback|: This will be
invoked with an array of $(ref:DisplayLayout).
The docs already say this by virtue of what the callback looks like.  Just say
something like "the callback to invoke with the results."

[Devlin, 2017-04-11 18:54:17]

The CQ bit was unchecked by rdevlin.cronin@chromium.org

[stevenjb, 2017-04-11 19:34:47]

https://codereview.chromium.org/2802603005/diff/100001/extensions/common/api/...
File extensions/common/api/system_display.idl (right):

https://codereview.chromium.org/2802603005/diff/100001/extensions/common/api/...
extensions/common/api/system_display.idl:173: // other properties may not apply.
On 2017/04/11 18:54:08, Devlin wrote:
> Comment what the default is.

Done.

https://codereview.chromium.org/2802603005/diff/100001/extensions/common/api/...
extensions/common/api/system_display.idl:219: // $(ref:enableUnifiedDesktop)).
On 2017/04/11 18:54:08, Devlin wrote:
> Comment also what the default is.

Done.

https://codereview.chromium.org/2802603005/diff/100001/extensions/common/api/...
extensions/common/api/system_display.idl:220: boolean? singleUnified;
On 2017/04/11 18:54:08, Devlin wrote:
> singleUnified seems redundant to me - maybe singleDisplay or unifiedDisplay?

single = one result, unified = when in unified desktop mode.
'singleDisplay' is incorrect because multiple displays will be returned if not
in unified desktop mode.
'unifiedDisplay' is also misleading because we may not be in unified desktop
mode.

I don't think there is a good clear property name here. This is a restricted API
that only devs familiar with the concepts should be using.

https://codereview.chromium.org/2802603005/diff/100001/extensions/common/api/...
extensions/common/api/system_display.idl:231: // |flags|: See
$(ref:GetInfoFlags).
On 2017/04/11 18:54:08, Devlin wrote:
> similar to the others, |flags| will already have a type that indicates what it
> is.  Instead, say something like "The options used to determine which
> information to return".

Done.

https://codereview.chromium.org/2802603005/diff/100001/extensions/common/api/...
extensions/common/api/system_display.idl:232: // |callback|: This will be
invoked with an array of $(ref:DisplayUnitInfo).
On 2017/04/11 18:54:08, Devlin wrote:
> same as 238 (sorry, wrote these backwards)

Done.

https://codereview.chromium.org/2802603005/diff/100001/extensions/common/api/...
extensions/common/api/system_display.idl:238: // |callback|: This will be
invoked with an array of $(ref:DisplayLayout).
On 2017/04/11 18:54:08, Devlin wrote:
> The docs already say this by virtue of what the callback looks like.  Just say
> something like "the callback to invoke with the results."

Done.

[stevenjb, 2017-04-11 19:34:55]

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

[commit-bot: I haz the power, 2017-04-11 19:35:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 20:00:35]

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

[commit-bot: I haz the power, 2017-04-11 20:00:37]

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

[Devlin, 2017-04-11 22:02:31]

lgtm

[stevenjb, 2017-04-11 22:51:52]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2017-04-11 22:51:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
malaykeshav@chromium.org, xiyuan@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2802603005/#ps160001
(title: "Fix closure and tests")

[commit-bot: I haz the power, 2017-04-11 22:52:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 00:59:38]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1491951112960940,
"parent_rev": "9b0d77b15fee21af7508c5dbce762a82599b2f3e", "commit_rev":
"717b7c3c86906d088c16fb30f4588fb0a0ea15e3"}

[commit-bot: I haz the power, 2017-04-12 01:00:24]

Description was changed from

==========
MD Settings: Display: Add unified desktop control and modify api

This CL:
* Updates the chrome.system.display API to support unified
  desktop for Settings UI.
* Adds a unified desktop setting to the MD Settings UI when
  the flag is enabled.

BUG=703423
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Display: Add unified desktop control and modify api

This CL:
* Updates the chrome.system.display API to support unified
  desktop for Settings UI.
* Adds a unified desktop setting to the MD Settings UI when
  the flag is enabled.

BUG=703423
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2802603005
Cr-Commit-Position: refs/heads/master@{#463865}
Committed:
https://chromium.googlesource.com/chromium/src/+/717b7c3c86906d088c16fb30f458...
==========

[commit-bot: I haz the power, 2017-04-12 01:00:27]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/717b7c3c86906d088c16fb30f458...