MD Settings: Dual-role charger selection

MD Settings: Dual-role charger selection

UI for Chromebooks that support dual-role devices to select which device to
charge from.

If a dedicated charger is attached, it will be used anyway, so we don't show the
dropdown menu.

If the Chrome device does not support dual-role devices, this row is not shown.

BUG=633484
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2629173004
Cr-Commit-Position: refs/heads/master@{#443801}
Committed: https://chromium.googlesource.com/chromium/src/+/c9c5ea2a110bd0721860bf2899f1dfecc150fc06

[michaelpg, 2017-01-13 00:01:47]

Description was changed from

==========
MD Settings: Dual-role charger selection

UI for Chromebooks that support dual-role devices to select which device to
charge from.

If a dedicated charger is attached, it will be used anyway, so we don't show the
dropdown menu.

If the Chrome device does not support dual-role devices, this row is not shown.

BUG=633484
R=stevenjb@chromium.org
==========

to

==========
MD Settings: Dual-role charger selection

UI for Chromebooks that support dual-role devices to select which device to
charge from.

If a dedicated charger is attached, it will be used anyway, so we don't show the
dropdown menu.

If the Chrome device does not support dual-role devices, this row is not shown.

BUG=633484
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[michaelpg, 2017-01-13 00:11:30]

PTAL.

[michaelpg, 2017-01-13 17:15:43]

Description was changed from

==========
MD Settings: Dual-role charger selection

UI for Chromebooks that support dual-role devices to select which device to
charge from.

If a dedicated charger is attached, it will be used anyway, so we don't show the
dropdown menu.

If the Chrome device does not support dual-role devices, this row is not shown.

BUG=633484
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Dual-role charger selection

UI for Chromebooks that support dual-role devices to select which device to
charge from.

If a dedicated charger is attached, it will be used anyway, so we don't show the
dropdown menu.

If the Chrome device does not support dual-role devices, this row is not shown.

BUG=633484
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[michaelpg, 2017-01-13 17:15:43]

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

[michaelpg, 2017-01-13 17:15:48]

+derat

[stevenjb, 2017-01-13 17:22:43]

https://codereview.chromium.org/2629173004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/device_page.html (right):

https://codereview.chromium.org/2629173004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/device_page.html:73:
selected$="[[isEqual_('', selectedPowerSourceId_)]]">
nit: Can we easily use a descriptive value for battery instead of ""?

https://codereview.chromium.org/2629173004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/device_page.js (right):

https://codereview.chromium.org/2629173004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/device_page.js:150: * True if lhs
=== rhs.
@return {boolean} (comment is pretty unnecessary...)

https://codereview.chromium.org/2629173004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/device_page.js:162: var icon =
'settings:battery-';
nit: prefix since by itself this is not a valid icon name?

https://codereview.chromium.org/2629173004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/device_page.js:207: *     and no
dedicated chargers are attached.
nit: 'no non dual-role USB (i.e. dedicated) power sources'? It wasn't clear to
me at first that the comment matches the logic.

https://codereview.chromium.org/2629173004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/device_page.js:220: * @return
{string} Description of the power source.
nit: consistent @private / @return order.

https://codereview.chromium.org/2629173004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/device_page.js:272:
this.$$('#powerSource').value);
nit: this.$.powerSource.value ?

https://codereview.chromium.org/2629173004/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/device_power_handler.h (right):

https://codereview.chromium.org/2629173004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/device_power_handler.h:34: void
OnPowerStatusChanged() override;
Why private?

[Daniel Erat, 2017-01-13 17:44:54]

https://codereview.chromium.org/2629173004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/device_page.html (right):

https://codereview.chromium.org/2629173004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/device_page.html:73:
selected$="[[isEqual_('', selectedPowerSourceId_)]]">
On 2017/01/13 17:22:42, stevenjb wrote:
> nit: Can we easily use a descriptive value for battery instead of ""?

the IDs are opaque strings that come from powerd. the field from powerd is
actually specified to be external, so an empty string is used to indicate no
external power source:

  // ID of the PowerSource that is currently providing power to the system.
  optional string external_power_source_id = 17;

so i'm not sure that it makes sense to map it to something else here.

[stevenjb, 2017-01-13 18:03:41]

https://codereview.chromium.org/2629173004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/device_page.html (right):

https://codereview.chromium.org/2629173004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/device_page.html:73:
selected$="[[isEqual_('', selectedPowerSourceId_)]]">
On 2017/01/13 17:44:54, Daniel Erat wrote:
> On 2017/01/13 17:22:42, stevenjb wrote:
> > nit: Can we easily use a descriptive value for battery instead of ""?
> 
> the IDs are opaque strings that come from powerd. the field from powerd is
> actually specified to be external, so an empty string is used to indicate no
> external power source:
> 
>   // ID of the PowerSource that is currently providing power to the system.
>   optional string external_power_source_id = 17;
> 
> so i'm not sure that it makes sense to map it to something else here.

OK, that makes sense.

[michaelpg, 2017-01-13 18:52:03]

PTAL

https://codereview.chromium.org/2629173004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/device_page.js (right):

https://codereview.chromium.org/2629173004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/device_page.js:150: * True if lhs
=== rhs.
On 2017/01/13 17:22:42, stevenjb wrote:
> @return {boolean} (comment is pretty unnecessary...)

Done.

https://codereview.chromium.org/2629173004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/device_page.js:162: var icon =
'settings:battery-';
On 2017/01/13 17:22:42, stevenjb wrote:
> nit: prefix since by itself this is not a valid icon name?

Done.

https://codereview.chromium.org/2629173004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/device_page.js:207: *     and no
dedicated chargers are attached.
On 2017/01/13 17:22:42, stevenjb wrote:
> nit: 'no non dual-role USB (i.e. dedicated) power sources'? It wasn't clear to
> me at first that the comment matches the logic.
> 

How's this? "True if at least one power source is attached and all of them are
dual-role (no dedicated chargers)."

https://codereview.chromium.org/2629173004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/device_page.js:220: * @return
{string} Description of the power source.
On 2017/01/13 17:22:42, stevenjb wrote:
> nit: consistent @private / @return order.

Done.

https://codereview.chromium.org/2629173004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/device_page.js:272:
this.$$('#powerSource').value);
On 2017/01/13 17:22:42, stevenjb wrote:
> nit: this.$.powerSource.value ?

powerSource lives in a dom-if so it's not part of the $ map.

https://codereview.chromium.org/2629173004/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/device_power_handler.h (right):

https://codereview.chromium.org/2629173004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/device_power_handler.h:34: void
OnPowerStatusChanged() override;
On 2017/01/13 17:22:42, stevenjb wrote:
> Why private?

why not?

[stevenjb, 2017-01-13 18:58:56]

https://codereview.chromium.org/2629173004/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/device_power_handler.h (right):

https://codereview.chromium.org/2629173004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/device_power_handler.h:34: void
OnPowerStatusChanged() override;
On 2017/01/13 18:52:03, michaelpg (NYC) wrote:
> On 2017/01/13 17:22:42, stevenjb wrote:
> > Why private?
> 
> why not?

Why make this private and not the SettingsPageUIHandler implementation?
Typically we make methods that override a public interface public.

[michaelpg, 2017-01-13 22:21:40]

https://codereview.chromium.org/2629173004/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/device_power_handler.h (right):

https://codereview.chromium.org/2629173004/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/device_power_handler.h:34: void
OnPowerStatusChanged() override;
On 2017/01/13 18:58:55, stevenjb wrote:
> On 2017/01/13 18:52:03, michaelpg (NYC) wrote:
> > On 2017/01/13 17:22:42, stevenjb wrote:
> > > Why private?
> > 
> > why not?
> 
> Why make this private and not the SettingsPageUIHandler implementation?
> Typically we make methods that override a public interface public.
> 

OK, keeping it public makes more sense. I have a vague memory of being told to
do it this way a long time ago. But IDK why that would have been; the method
would be available anyway via a base-class pointer.

[stevenjb, 2017-01-14 06:16:49]

lgtm

[michaelpg, 2017-01-14 18:31:41]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2017-01-14 18:31:41]

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

[commit-bot: I haz the power, 2017-01-14 18:31:51]

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-01-14 19:41:29]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484418701397090,
"parent_rev": "5f9b3d7676beeb194091dae650ada4ee1a59f13e", "commit_rev":
"c9c5ea2a110bd0721860bf2899f1dfecc150fc06"}

[commit-bot: I haz the power, 2017-01-14 19:41:57]

Description was changed from

==========
MD Settings: Dual-role charger selection

UI for Chromebooks that support dual-role devices to select which device to
charge from.

If a dedicated charger is attached, it will be used anyway, so we don't show the
dropdown menu.

If the Chrome device does not support dual-role devices, this row is not shown.

BUG=633484
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Dual-role charger selection

UI for Chromebooks that support dual-role devices to select which device to
charge from.

If a dedicated charger is attached, it will be used anyway, so we don't show the
dropdown menu.

If the Chrome device does not support dual-role devices, this row is not shown.

BUG=633484
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2629173004
Cr-Commit-Position: refs/heads/master@{#443801}
Committed:
https://chromium.googlesource.com/chromium/src/+/c9c5ea2a110bd0721860bf2899f1...
==========

[commit-bot: I haz the power, 2017-01-14 19:41:58]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/c9c5ea2a110bd0721860bf2899f1...