Map signal_strength from percentage to dBm per spec

Map signal_strength from percentage to dBm per spec

BUG=463334
Committed: https://crrev.com/b0675f841d16debd7960f0b89d03c9fba8e1e1ef
Cr-Commit-Position: refs/heads/master@{#325646}

[Andrew T Wilson (Slow), 2015-04-15 15:03:16]

atwilson@chromium.org changed reviewers:
+ tnagel@chromium.org

[Andrew T Wilson (Slow), 2015-04-15 15:03:16]

PTAL

[Thiemo Nagel, 2015-04-16 15:16:33]

https://codereview.chromium.org/1062853005/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_status_collector.cc (right):

https://codereview.chromium.org/1062853005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:640: // Shill attempts
to convert attempts to convert from its internal dBm to a
Please remove duplicate "attempts to convert".

https://codereview.chromium.org/1062853005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:641: // percentage
range (from 1-100) by subtracting 120 from the raw dBm value,
s/1-100/0-100/
s/substracting 120 from/adding 120 to/

https://codereview.chromium.org/1062853005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:642: // and then
clamping the result to the range 1-100.
s/1-100/0-100/

I'd suggest to include some kind of cross-reference to the shill function, eg.
its name "shill::WiFiService::SignalToStrength()".

https://codereview.chromium.org/1062853005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:727: true,  //
configured_only
At that occasion: Please insert a blank in front of "// configured_only".

https://codereview.chromium.org/1062853005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:747: // If shill has
provided a signal strength, convert it to dBm and store it
As far as I can see, this specific conversion should be limited to wifi
networks.  Also, it seems that zero signal strength implies "no measurement"
only for wifi (and wimax).

https://codereview.chromium.org/1062853005/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
(right):

https://codereview.chromium.org/1062853005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:1007: {
"idle", "/device/cellular1", shill::kTypeCellular, 0, 0, shill::kStateIdle,
I'm unsure what the expected behaviour for cellular should be.  My hunch is that
signal strength should be passed through unchanged and thus I'd suggest to
replace "0, 0" by "10, 10" to test for that.

[Andrew T Wilson (Slow), 2015-04-17 14:41:02]

ptal

https://codereview.chromium.org/1062853005/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_status_collector.cc (right):

https://codereview.chromium.org/1062853005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:640: // Shill attempts
to convert attempts to convert from its internal dBm to a
On 2015/04/16 15:16:33, Thiemo Nagel wrote:
> Please remove duplicate "attempts to convert".

Done.

https://codereview.chromium.org/1062853005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:641: // percentage
range (from 1-100) by subtracting 120 from the raw dBm value,
On 2015/04/16 15:16:33, Thiemo Nagel wrote:
> s/1-100/0-100/
> s/substracting 120 from/adding 120 to/

Done.

https://codereview.chromium.org/1062853005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:642: // and then
clamping the result to the range 1-100.
On 2015/04/16 15:16:33, Thiemo Nagel wrote:
> s/1-100/0-100/
> 
> I'd suggest to include some kind of cross-reference to the shill function, eg.
> its name "shill::WiFiService::SignalToStrength()".

Done.

https://codereview.chromium.org/1062853005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:727: true,  //
configured_only
On 2015/04/16 15:16:33, Thiemo Nagel wrote:
> At that occasion: Please insert a blank in front of "// configured_only".

Done.

https://codereview.chromium.org/1062853005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:747: // If shill has
provided a signal strength, convert it to dBm and store it
On 2015/04/16 15:16:33, Thiemo Nagel wrote:
> As far as I can see, this specific conversion should be limited to wifi
> networks.  Also, it seems that zero signal strength implies "no measurement"
> only for wifi (and wimax).

Only report this now for wifi networks.

https://codereview.chromium.org/1062853005/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
(right):

https://codereview.chromium.org/1062853005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:1007: {
"idle", "/device/cellular1", shill::kTypeCellular, 0, 0, shill::kStateIdle,
On 2015/04/16 15:16:33, Thiemo Nagel wrote:
> I'm unsure what the expected behaviour for cellular should be.  My hunch is
that
> signal strength should be passed through unchanged and thus I'd suggest to
> replace "0, 0" by "10, 10" to test for that.

I've changed this to just not report for cellular networks, and added a test for
that case.

[Thiemo Nagel, 2015-04-17 15:04:49]

lgtm with nit

https://codereview.chromium.org/1062853005/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_status_collector.cc (right):

https://codereview.chromium.org/1062853005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:727: true,  //
configured_only
> Done.

You fixed it in the other file, but not here.  :)

[Andrew T Wilson (Slow), 2015-04-17 15:37:27]

The CQ bit was checked by atwilson@chromium.org

[Andrew T Wilson (Slow), 2015-04-17 15:37:28]

The patchset sent to the CQ was uploaded after l-g-t-m from tnagel@chromium.org
Link to the patchset: https://codereview.chromium.org/1062853005/#ps100001
(title: "Really fixed comment")

[commit-bot: I haz the power, 2015-04-17 15:37:41]

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

[Andrew T Wilson (Slow), 2015-04-17 15:37:52]

https://codereview.chromium.org/1062853005/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_status_collector.cc (right):

https://codereview.chromium.org/1062853005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:727: true,  //
configured_only
On 2015/04/17 15:04:49, Thiemo Nagel wrote:
> > Done.
> 
> You fixed it in the other file, but not here.  :)

Gah, fixed here too.

[commit-bot: I haz the power, 2015-04-17 16:10:46]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-04-17 16:12:24]

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