Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2020923002:
bluetooth: Update ARC Bridge to use BluetoothDevice::GetName (Closed)
|
Created:
4 years, 6 months ago by scheib Modified:
4 years, 4 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, yusukes+watch_chromium.org, mcchou, Miao, dmitrygr Base URL:
https://chromium.googlesource.com/chromium/src.git@bt-filter-by-empty-name- Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbluetooth: Update ARC Bridge to use BluetoothDevice::GetName
Previously GetNameForDisplay was used, which exposes Chrome specific
fallbacks for non-existant or empty names.
GetName returns a value or empty.
http://crrev.com/2017393002 GetName -> GetNameForDisplay
http://crrev.com/2009753002 New GetName
http://crrev.com/2014473002 Web Bluetooth updated
http://crrev.com/2020923002 Arc Bridge updated <<< This change.
BUG=615720
Committed: https://crrev.com/5cf017029c4d8f7724940b07bbbcf39b54ddec7b
Cr-Commit-Position: refs/heads/master@{#409357}
Patch Set 1 : #
Total comments: 1
Patch Set 2 : Null mojo string when GetName is null. #
Depends on Patchset: Messages
Total messages: 58 (43 generated)
Description was changed from ========== bluetooth: Update ARC Bridge to use BluetoothDevice::GetName Previously GetNameForDisplay was used, which exposes Chrome specific fallbacks for non-existant or empty names. GetName returns a value or empty. BUG=615720 ========== to ========== bluetooth: Update ARC Bridge to use BluetoothDevice::GetName Previously GetNameForDisplay was used, which exposes Chrome specific fallbacks for non-existant or empty names. GetName returns a value or empty. http://crrev.com/2017393002 GetName -> GetNameForDisplay http://crrev.com/2009753002 New GetName http://crrev.com/2014473002 Web Bluetooth updated http://crrev.com/2020923002 Arc Bridge updated <<< This change. BUG=615720 ==========
scheib@chromium.org changed reviewers: + mcchou@google.com
mcchou, I'm looking to expose a raw device name (that may be non-existent). Previous version of this code used a name that would always be non-empty - with Chrome code making up a name based on MAC address if needed. Do you have any ideas for what to do in this bridge code when a name doesn't exist? It seems unlikely that making up a name with the MAC address is the right behavior.
mcchou@chromium.org changed reviewers: + mcchou@chromium.org
+ejcaruso One solution is to check the emptiness of the return of GetName() here and return "Unnamed"/"Unkown" string if it is empty. Since Android doesn't check the emptiness of returned name, and it would be confusing to the users if leaving the name empty.
On 2016/06/06 07:07:09, Miao wrote: > +ejcaruso > > One solution is to check the emptiness of the return of GetName() here and > return "Unnamed"/"Unkown" string if it is empty. Since Android doesn't check the > emptiness of returned name, and it would be confusing to the users if leaving > the name empty. Some devices really are unnamed. I guess I'm surprised Android doesn't handle generic naming devices with no name somewhere in higher level code.
Is this change currently just adding comments...?
On 2016/06/06 17:49:51, Eric Caruso wrote: > Is this change currently just adding comments...? This is a placeholder for work that needs to be done -- I don't yet know the right thing to pass up to Android when a device name doesn't exist at all (not even empty, it's null).
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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 to run a CQ dry run
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
Here is a proposal of what to do when names do not exist. I don't know what the Android code will expect, or the other side of the bridge expects.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by scheib@chromium.org to run a CQ dry run
Patchset #7 (id:120001) has been deleted
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: This issue passed the CQ dry run.
Patchset #3 (id:40001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:60001) has been deleted
scheib@chromium.org changed reviewers: + puthik@chromium.org - mcchou@chromium.org, mcchou@google.com
+dmitrygr When scanning for Bluetooth device in Nexus 5x settings app, it will show the Mac Address for the unnamed device. It might be better to leave it as is. https://codereview.chromium.org/2020923002/diff/140001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2020923002/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:1521: local_name->set_local_name(base::UTF16ToUTF8(device->GetNameForDisplay())); We need to change here too if we want to use GetName()
On 2016/08/02 20:12:55, puthik_chromium wrote: > +dmitrygr > > When scanning for Bluetooth device in Nexus 5x settings app, it will show the > Mac Address for the unnamed device. > It might be better to leave it as is. > > https://codereview.chromium.org/2020923002/diff/140001/components/arc/bluetoo... > File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): > > https://codereview.chromium.org/2020923002/diff/140001/components/arc/bluetoo... > components/arc/bluetooth/arc_bluetooth_bridge.cc:1521: > local_name->set_local_name(base::UTF16ToUTF8(device->GetNameForDisplay())); > We need to change here too if we want to use GetName() Thanks, As currently implemented, ChromeOS will give different results vs Nexus devices. Before this patch series, GetName was deceptively named and produced generated strings for devices without names. Now I've split it into GetName which may return nullopt, and GetNameForDisplay which offers the string. I expect Android to, somewhere, have code to handle devices that do not advertise a name and consistently name them. If so, we should find the right way to return the lack of a name to that layer. Perhaps Android relies on this being done at a lower level than the ARC Bridge. If so, then we can either decide that the previous ChromeOS method of naming unnamed devices is fine (use GetNameForDisplay) or implement an Android compliant/consistent implementation here. Thanks for helping find whomever can look into what ARC/Android needs.
On 2016/08/02 20:34:39, scheib wrote: > On 2016/08/02 20:12:55, puthik_chromium wrote: > > +dmitrygr > > > > When scanning for Bluetooth device in Nexus 5x settings app, it will show the > > Mac Address for the unnamed device. > > It might be better to leave it as is. > > > > > https://codereview.chromium.org/2020923002/diff/140001/components/arc/bluetoo... > > File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): > > > > > https://codereview.chromium.org/2020923002/diff/140001/components/arc/bluetoo... > > components/arc/bluetooth/arc_bluetooth_bridge.cc:1521: > > local_name->set_local_name(base::UTF16ToUTF8(device->GetNameForDisplay())); > > We need to change here too if we want to use GetName() > > Thanks, > > As currently implemented, ChromeOS will give different results vs Nexus devices. > > Before this patch series, GetName was deceptively named and produced generated > strings for devices without names. Now I've split it into GetName which may > return nullopt, and GetNameForDisplay which offers the string. > > I expect Android to, somewhere, have code to handle devices that do not > advertise a name and consistently name them. If so, we should find the right way > to return the lack of a name to that layer. > > Perhaps Android relies on this being done at a lower level than the ARC Bridge. > If so, then we can either decide that the previous ChromeOS method of naming > unnamed devices is fine (use GetNameForDisplay) or implement an Android > compliant/consistent implementation here. > > Thanks for helping find whomever can look into what ARC/Android needs. I take back the last comment. The blank name will make ChromeOS give same result with Nexus devices. In nrF Connect app[1]. It will show N/A when the name of the device is missing. I think Android relied on app developer to implement this correctly. I think what we should do is just giving Android an empty String when device.GetName() is empty. [1] https://play.google.com/store/apps/details?id=no.nordicsemi.android.mcp
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...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:100001) has been deleted
lgtm Per offline discussion, we will use null mojo string when device does not have name. This still works with ToT Android arc implementation.
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
rs-lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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...
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Update ARC Bridge to use BluetoothDevice::GetName Previously GetNameForDisplay was used, which exposes Chrome specific fallbacks for non-existant or empty names. GetName returns a value or empty. http://crrev.com/2017393002 GetName -> GetNameForDisplay http://crrev.com/2009753002 New GetName http://crrev.com/2014473002 Web Bluetooth updated http://crrev.com/2020923002 Arc Bridge updated <<< This change. BUG=615720 ========== to ========== bluetooth: Update ARC Bridge to use BluetoothDevice::GetName Previously GetNameForDisplay was used, which exposes Chrome specific fallbacks for non-existant or empty names. GetName returns a value or empty. http://crrev.com/2017393002 GetName -> GetNameForDisplay http://crrev.com/2009753002 New GetName http://crrev.com/2014473002 Web Bluetooth updated http://crrev.com/2020923002 Arc Bridge updated <<< This change. BUG=615720 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Update ARC Bridge to use BluetoothDevice::GetName Previously GetNameForDisplay was used, which exposes Chrome specific fallbacks for non-existant or empty names. GetName returns a value or empty. http://crrev.com/2017393002 GetName -> GetNameForDisplay http://crrev.com/2009753002 New GetName http://crrev.com/2014473002 Web Bluetooth updated http://crrev.com/2020923002 Arc Bridge updated <<< This change. BUG=615720 ========== to ========== bluetooth: Update ARC Bridge to use BluetoothDevice::GetName Previously GetNameForDisplay was used, which exposes Chrome specific fallbacks for non-existant or empty names. GetName returns a value or empty. http://crrev.com/2017393002 GetName -> GetNameForDisplay http://crrev.com/2009753002 New GetName http://crrev.com/2014473002 Web Bluetooth updated http://crrev.com/2020923002 Arc Bridge updated <<< This change. BUG=615720 Committed: https://crrev.com/5cf017029c4d8f7724940b07bbbcf39b54ddec7b Cr-Commit-Position: refs/heads/master@{#409357} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5cf017029c4d8f7724940b07bbbcf39b54ddec7b Cr-Commit-Position: refs/heads/master@{#409357} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
