Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2561883002:
bluetooth: web: Chooser RSSI indicator now based on percentile buckets (Closed)
Descriptionbluetooth: web: Chooser RSSI indicator now based on percentile buckets
This change updates the mapping from RSSI values into signal strength
icon levels. It is made after reviewing a month of recorded UMA
data for RSSI values encountered by users.
A new design of how we map values to displayed levels is documented
in the change.
See these charts for measured data:
https://goo.gl/photos/pCoAkF7mPyza9B1k7
BUG=629689
Committed: https://crrev.com/085fd9524d8369206beaddeed429c0a2f7e0738b
Cr-Commit-Position: refs/heads/master@{#438009}
Patch Set 1 #Patch Set 2 : Changed to 5 equal percentage based buckets. #
Total comments: 6
Patch Set 3 : unittest and comments #
Messages
Total messages: 33 (21 generated)
The CQ bit was checked by scheib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by scheib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
scheib@chromium.org changed reviewers: + juncai@chromium.org
Description was changed from ========== bluetooth: Chooser RSSI indicator range adjusted. This change updates the mapping from RSSI values into signal strength icon levels. It is made after reviewing a month of recorded UMA data for RSSI values encountered by users. See these charts for measured data: https://goo.gl/photos/pCoAkF7mPyza9B1k7 There was a previous math error in how output levels were computed, that is corrected here by refactoring the code. BUG=629689 ========== to ========== bluetooth: Chooser RSSI indicator range adjusted. This change updates the mapping from RSSI values into signal strength icon levels. It is made after reviewing a month of recorded UMA data for RSSI values encountered by users. See these charts for measured data: https://goo.gl/photos/pCoAkF7mPyza9B1k7 BUG=629689 ==========
//content/browser/bluetooth/bluetooth_device_chooser_controller_unittest.cc needs to be updated too.
https://codereview.chromium.org/2561883002/diff/20001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (left): https://codereview.chromium.org/2561883002/diff/20001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_device_chooser_controller.cc:441: UMARSSISignalStrengthLevel::LESS_THAN_OR_EQUAL_TO_MIN_RSSI); "LESS_THAN_OR_EQUAL_TO_MIN_RSSI" is not used, maybe can remove it from: //content/browser/bluetooth/bluetooth_metrics.h https://codereview.chromium.org/2561883002/diff/20001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_device_chooser_controller.cc:447: UMARSSISignalStrengthLevel::GREATER_THAN_OR_EQUAL_TO_MAX_RSSI); "GREATER_THAN_OR_EQUAL_TO_MAX_RSSI" is not used, maybe can remove it from: //content/browser/bluetooth/bluetooth_metrics.h https://codereview.chromium.org/2561883002/diff/20001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right): https://codereview.chromium.org/2561883002/diff/20001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_device_chooser_controller.cc:37: // names is a secondary goal. It is important that a user be able to move closer s/be/is
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by scheib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, https://codereview.chromium.org/2561883002/diff/20001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (left): https://codereview.chromium.org/2561883002/diff/20001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_device_chooser_controller.cc:441: UMARSSISignalStrengthLevel::LESS_THAN_OR_EQUAL_TO_MIN_RSSI); On 2016/12/09 23:31:40, juncai wrote: > "LESS_THAN_OR_EQUAL_TO_MIN_RSSI" is not used, maybe can remove it from: > //content/browser/bluetooth/bluetooth_metrics.h We can't remove values for histograms. We could remove the name from C++, but I don't think there's much need or value to do so. We would have to keep the other enum values constant including COUNT, which just seems to be awkward. I think we should just leave the enums unused. https://codereview.chromium.org/2561883002/diff/20001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_device_chooser_controller.cc:447: UMARSSISignalStrengthLevel::GREATER_THAN_OR_EQUAL_TO_MAX_RSSI); On 2016/12/09 23:31:40, juncai wrote: > "GREATER_THAN_OR_EQUAL_TO_MAX_RSSI" is not used, maybe can remove it from: > //content/browser/bluetooth/bluetooth_metrics.h ditto https://codereview.chromium.org/2561883002/diff/20001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right): https://codereview.chromium.org/2561883002/diff/20001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_device_chooser_controller.cc:37: // names is a secondary goal. It is important that a user be able to move closer On 2016/12/09 23:31:40, juncai wrote: > s/be/is Done.
Description was changed from ========== bluetooth: Chooser RSSI indicator range adjusted. This change updates the mapping from RSSI values into signal strength icon levels. It is made after reviewing a month of recorded UMA data for RSSI values encountered by users. See these charts for measured data: https://goo.gl/photos/pCoAkF7mPyza9B1k7 BUG=629689 ========== to ========== bluetooth: Chooser RSSI indicator range now uses percentile buckets This change updates the mapping from RSSI values into signal strength icon levels. It is made after reviewing a month of recorded UMA data for RSSI values encountered by users. A new design of how we map values to displayed levels is documented in the change. See these charts for measured data: https://goo.gl/photos/pCoAkF7mPyza9B1k7 BUG=629689 ==========
LGTM.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Description was changed from ========== bluetooth: Chooser RSSI indicator range now uses percentile buckets This change updates the mapping from RSSI values into signal strength icon levels. It is made after reviewing a month of recorded UMA data for RSSI values encountered by users. A new design of how we map values to displayed levels is documented in the change. See these charts for measured data: https://goo.gl/photos/pCoAkF7mPyza9B1k7 BUG=629689 ========== to ========== bluetooth: web: Chooser RSSI indicator now based on percentile buckets This change updates the mapping from RSSI values into signal strength icon levels. It is made after reviewing a month of recorded UMA data for RSSI values encountered by users. A new design of how we map values to displayed levels is documented in the change. See these charts for measured data: https://goo.gl/photos/pCoAkF7mPyza9B1k7 BUG=629689 ==========
scheib@chromium.org changed reviewers: + ortuno@chromium.org
Thanks juncai. I'll have ortuno take a look too to agree or not on the approach.
On 2016/12/10 02:28:07, scheib wrote: > Thanks juncai. I'll have ortuno take a look too to agree or not on the approach. Oh, and I'll note that we also discussed this with reillyg.
I wonder if we are marking devices that are in an acceptable range as having 1 or 2 bars. For example: - Android considers anything above -75 to be great https://android.googlesource.com/platform/packages/apps/Messaging/+/461a34b46... - iOS considers anything above -91 be 5 bars - Microsoft considers anything above -75 to be medium to high quality http://stackoverflow.com/questions/15797920/how-to-convert-wifi-signal-streng... http://www.speedguide.net/faq/how-does-rssi-dbm-relate-to-signal-quality-perc... Though those are a mix of wifi and cellular signals. The only bluetooth-related article I could find considered anything above -60 to be "Good" https://www.cnet.com/au/how-to/how-to-check-bluetooth-connection-strength-in-... Given our objective I think it's OK to try to be more granular in the range where we see most devices but we should keep in mind this difference between quality and range in our Icon discussions. So lgtm.
On 2016/12/12 22:16:23, ortuno wrote: > I wonder if we are marking devices that are in an acceptable range as having 1 > or 2 bars. For example: > > - Android considers anything above -75 to be great > https://android.googlesource.com/platform/packages/apps/Messaging/+/461a34b46... > - iOS considers anything above -91 be 5 bars > - Microsoft considers anything above -75 to be medium to high quality > http://stackoverflow.com/questions/15797920/how-to-convert-wifi-signal-streng... > http://www.speedguide.net/faq/how-does-rssi-dbm-relate-to-signal-quality-perc... > > Though those are a mix of wifi and cellular signals. The only bluetooth-related > article I could find considered anything above -60 to be "Good" > https://www.cnet.com/au/how-to/how-to-check-bluetooth-connection-strength-in-... > > Given our objective I think it's OK to try to be more granular in the range > where we see most devices but we should keep in mind this difference between > quality and range in our Icon discussions. So lgtm. I think it's OK for a device that will work to be displayed with a low signal strength. The top priority is to help users understand a relative comparison between two of the same objects. Even with signal strength displayed at lower level this will be possible. Your raised concern is that for a single entry, there may be a problem displaying the signal strength at a lower level. E.g. users may be concerned that a device may not work because the signal is low. The process proposed in this patch is that we will use data of measured RSSI to inform how we make the display. If, indeed, a device is in the bottom 20% of signal strengths we encounter, it would be displayed very low. We should be gathering a large amount of data over time, and I suspect it will include many devices at low RSSI values. I think this is a fair representation then of a new device encountered with that RSSI. I also do not think it will stop a user from selecting that device and attempting to use it.
The CQ bit was checked by scheib@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481585495852740,
"parent_rev": "31fbc13c89a3719f9ca2d89bc71f4b3651bf5c79", "commit_rev":
"b440e8b10ca241f6eec98447418950d4e7840f94"}
Message was sent while issue was closed.
Description was changed from ========== bluetooth: web: Chooser RSSI indicator now based on percentile buckets This change updates the mapping from RSSI values into signal strength icon levels. It is made after reviewing a month of recorded UMA data for RSSI values encountered by users. A new design of how we map values to displayed levels is documented in the change. See these charts for measured data: https://goo.gl/photos/pCoAkF7mPyza9B1k7 BUG=629689 ========== to ========== bluetooth: web: Chooser RSSI indicator now based on percentile buckets This change updates the mapping from RSSI values into signal strength icon levels. It is made after reviewing a month of recorded UMA data for RSSI values encountered by users. A new design of how we map values to displayed levels is documented in the change. See these charts for measured data: https://goo.gl/photos/pCoAkF7mPyza9B1k7 BUG=629689 Review-Url: https://codereview.chromium.org/2561883002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== bluetooth: web: Chooser RSSI indicator now based on percentile buckets This change updates the mapping from RSSI values into signal strength icon levels. It is made after reviewing a month of recorded UMA data for RSSI values encountered by users. A new design of how we map values to displayed levels is documented in the change. See these charts for measured data: https://goo.gl/photos/pCoAkF7mPyza9B1k7 BUG=629689 Review-Url: https://codereview.chromium.org/2561883002 ========== to ========== bluetooth: web: Chooser RSSI indicator now based on percentile buckets This change updates the mapping from RSSI values into signal strength icon levels. It is made after reviewing a month of recorded UMA data for RSSI values encountered by users. A new design of how we map values to displayed levels is documented in the change. See these charts for measured data: https://goo.gl/photos/pCoAkF7mPyza9B1k7 BUG=629689 Committed: https://crrev.com/085fd9524d8369206beaddeed429c0a2f7e0738b Cr-Commit-Position: refs/heads/master@{#438009} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/085fd9524d8369206beaddeed429c0a2f7e0738b Cr-Commit-Position: refs/heads/master@{#438009} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
