Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11345037:
Implemented BluetoothAdapterWin::IsPresent(). (Closed)
|
Created:
8 years, 1 month ago by youngki Modified:
8 years, 1 month ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplemented BluetoothAdapterWin::IsPresent().
BUG=135470
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167539
Patch Set 1 #
Total comments: 9
Patch Set 2 : Fixed namespace style #Patch Set 3 : Added unittest #Patch Set 4 : A bit more polishing #Patch Set 5 : Renaming constant #
Total comments: 6
Patch Set 6 : kPollIntervalMs now private and method name changes #
Total comments: 2
Patch Set 7 : Made adapter refcounted in unittest #
Total comments: 10
Patch Set 8 : Removed device:: and added parenthesis #
Total comments: 2
Patch Set 9 : Match the definition order to the declaration order #
Messages
Total messages: 19 (0 generated)
I fell back into polling solution; I looked into two different notification frameworks: src/net/base/network_change_notifier.h and WSANSPIoctl() provided by Windows SDK. network_change_notifier.h successfully notifies upon bluetooth adapter state changes, but after talking with @pauljensen (current NetworkChangeNotifier owner), we realized this is a bit of hack because NetworkChangeNotifier is intended to detect the internet connection and the new version of NetworkChangeNotifier might not signal on the bluetooth adapter state anymore. msdn.microsoft.com states WSANSPIoctl as a function to notify on network location changes, and we need a dedicated thread to wait for the event. I talked with @girard with this issue and he also agrees we need a separate thread to handle this. So I decided to go with polling the bluetooth adapter info every 500ms. This is simpler and guaranteed to work despite the fact that polling sucks. I will update the CL with unittest soon, but just wanted to get this CL out for initial review.
FYI: Scott, there is a question buried for you down in my comments. https://chromiumcodereview.appspot.com/11345037/diff/1/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_win.cc (right): https://chromiumcodereview.appspot.com/11345037/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_win.cc:19: const BLUETOOTH_FIND_RADIO_PARAMS bluetooth_adapter_param = no indent and a blank line after namespace and before the closing namespace brace https://chromiumcodereview.appspot.com/11345037/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_win.cc:43: bool BluetoothAdapterWin::IsPresent() const { This should probably try to update address_ immediately before returning. https://chromiumcodereview.appspot.com/11345037/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_win.cc:94: UpdateAdapterState(); This is going to start polling before it is necessary. There are two changes to fix this: 1) Make ExtensionBluetoothEventRouter lazily initialize the adapter. That can be done here, or in a prequel CL. Should be fairly straightforward. 2) Avoid calling TrackDefaultAdapter (on Windows) until there are actually event listeners waiting for events. This requires a bit of a change in semantics, but I think it makes sense in order to prevent unnecessary polling. Scott: thoughts?
Scott, let me know your thoughts on Bryan's comment. https://chromiumcodereview.appspot.com/11345037/diff/1/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_win.cc (right): https://chromiumcodereview.appspot.com/11345037/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_win.cc:19: const BLUETOOTH_FIND_RADIO_PARAMS bluetooth_adapter_param = On 2012/10/30 19:58:40, bryeung wrote: > no indent and a blank line after namespace and before the closing namespace > brace Done. https://chromiumcodereview.appspot.com/11345037/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_win.cc:43: bool BluetoothAdapterWin::IsPresent() const { On 2012/10/30 19:58:40, bryeung wrote: > This should probably try to update address_ immediately before returning. This is a const method.. so I don't think we can update address_ here. https://chromiumcodereview.appspot.com/11345037/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_win.cc:94: UpdateAdapterState(); On 2012/10/30 19:58:40, bryeung wrote: > This is going to start polling before it is necessary. > > There are two changes to fix this: > 1) Make ExtensionBluetoothEventRouter lazily initialize the adapter. That can > be done here, or in a prequel CL. Should be fairly straightforward. > > 2) Avoid calling TrackDefaultAdapter (on Windows) until there are actually event > listeners waiting for events. This requires a bit of a change in semantics, but > I think it makes sense in order to prevent unnecessary polling. > > Scott: thoughts? I will wait for Scott's thoughts here.
I am going to move the logic of polling the bluetooth info to a separate class, BluetoothManagerWin, and in the futuer put all the windows bluetooth API calls there. In that way I could create MockBluetoothManagerWin and use that for unittesting purpose. I will update the CL with the logic soon.
https://chromiumcodereview.appspot.com/11345037/diff/1/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_win.cc (right): https://chromiumcodereview.appspot.com/11345037/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_win.cc:94: UpdateAdapterState(); TrackDefaultAdapter should only be called when a new default adapter instance is created, and that should only happen when a class (like the settings page) calls for it. Likewise when nothing requires it (settings page is closed), the default adapter should be cleaned up. So polling will open happen during need anyway, no?
https://chromiumcodereview.appspot.com/11345037/diff/1/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_win.cc (right): https://chromiumcodereview.appspot.com/11345037/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_win.cc:94: UpdateAdapterState(); On 2012/10/31 18:19:47, keybuk wrote: > TrackDefaultAdapter should only be called when a new default adapter instance is > created, and that should only happen when a class (like the settings page) calls > for it. > > Likewise when nothing requires it (settings page is closed), the default adapter > should be cleaned up. > > So polling will open happen during need anyway, no? Maybe this is strictly a problem with ExtensionBluetoothEventRouter then (in that it is holding on to the adapter reference for too long). We'll fix it there and then hopefully this is okay.
https://chromiumcodereview.appspot.com/11345037/diff/1/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_win.cc (right): https://chromiumcodereview.appspot.com/11345037/diff/1/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_win.cc:94: UpdateAdapterState(); On 2012/10/31 18:46:01, bryeung wrote: > On 2012/10/31 18:19:47, keybuk wrote: > > TrackDefaultAdapter should only be called when a new default adapter instance > is > > created, and that should only happen when a class (like the settings page) > calls > > for it. > > > > Likewise when nothing requires it (settings page is closed), the default > adapter > > should be cleaned up. > > > > So polling will open happen during need anyway, no? > > Maybe this is strictly a problem with ExtensionBluetoothEventRouter then (in > that it is holding on to the adapter reference for too long). > > We'll fix it there and then hopefully this is okay. Okay then I will leave this as it is for now.
The other CL to modify event router is under review: https://chromiumcodereview.appspot.com/11368145 Meanwhile I added unittest into this CL. PTAL.
https://chromiumcodereview.appspot.com/11345037/diff/14001/device/bluetooth/b... File device/bluetooth/bluetooth_adapter_win.h (right): https://chromiumcodereview.appspot.com/11345037/diff/14001/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win.h:43: static const int kPollIntervalMs; why is this public? https://chromiumcodereview.appspot.com/11345037/diff/14001/device/bluetooth/b... File device/bluetooth/bluetooth_adapter_win_unittest.cc (right): https://chromiumcodereview.appspot.com/11345037/diff/14001/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win_unittest.cc:34: void set_adapter_address(const std::string& adapter_address) { SetAdapterAddressForTest is probably a better name https://chromiumcodereview.appspot.com/11345037/diff/14001/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win_unittest.cc:56: void UpdateAdapterAddress() { probably better to just call adapter_->SetAdapterAddressForTest(kAdapterAddress) in the individual test cases. This methods just obscures what's going on, especially since it has such a similar name to BluetoothAdapterWin::UpdateAdapterState.
https://chromiumcodereview.appspot.com/11345037/diff/14001/device/bluetooth/b... File device/bluetooth/bluetooth_adapter_win.h (right): https://chromiumcodereview.appspot.com/11345037/diff/14001/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win.h:43: static const int kPollIntervalMs; On 2012/11/09 17:59:09, bryeung wrote: > why is this public? moved it to private. https://chromiumcodereview.appspot.com/11345037/diff/14001/device/bluetooth/b... File device/bluetooth/bluetooth_adapter_win_unittest.cc (right): https://chromiumcodereview.appspot.com/11345037/diff/14001/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win_unittest.cc:34: void set_adapter_address(const std::string& adapter_address) { On 2012/11/09 17:59:09, bryeung wrote: > SetAdapterAddressForTest is probably a better name Done. https://chromiumcodereview.appspot.com/11345037/diff/14001/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win_unittest.cc:56: void UpdateAdapterAddress() { On 2012/11/09 17:59:09, bryeung wrote: > probably better to just call adapter_->SetAdapterAddressForTest(kAdapterAddress) > in the individual test cases. This methods just obscures what's going on, > especially since it has such a similar name to > BluetoothAdapterWin::UpdateAdapterState. I still need this function to use as a closure for MessageLoop. I renamed it as SetAdapterAddressForTest. Still not a great name, but hopefully it's more clear..
https://chromiumcodereview.appspot.com/11345037/diff/10006/device/bluetooth/b... File device/bluetooth/bluetooth_adapter_win.cc (right): https://chromiumcodereview.appspot.com/11345037/diff/10006/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win.cc:128: weak_ptr_factory_.GetWeakPtr()), Since this is a weak_ptr, couldn't the adapter have already been destroyed by the time this method is called, leading to a crash due to a NULL this?
https://chromiumcodereview.appspot.com/11345037/diff/10006/device/bluetooth/b... File device/bluetooth/bluetooth_adapter_win.cc (right): https://chromiumcodereview.appspot.com/11345037/diff/10006/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win.cc:128: weak_ptr_factory_.GetWeakPtr()), On 2012/11/09 21:55:48, bryeung wrote: > Since this is a weak_ptr, couldn't the adapter have already been destroyed by > the time this method is called, leading to a crash due to a NULL this? I ran a simple test by queuing this to message loop and destroying the adapter, and the program handled it gracefully by simply not running it. Also I asked to chromium-dev and I think in fact base::Bind will only run a task if the given weak_ptr is non-null: https://groups.google.com/a/chromium.org/d/topic/chromium-dev/l1PfCT_3fvI/dis...
lgtm, pending sorting out this RefCoutned/WeakPtr issue. https://chromiumcodereview.appspot.com/11345037/diff/19002/device/bluetooth/b... File device/bluetooth/bluetooth_adapter_win.h (right): https://chromiumcodereview.appspot.com/11345037/diff/19002/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win.h:16: class BluetoothAdapterWinTest; I don't think you need these if you're just making a friend. https://chromiumcodereview.appspot.com/11345037/diff/19002/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win.h:51: friend class device::BluetoothAdapterWinTest; s/device::// https://chromiumcodereview.appspot.com/11345037/diff/19002/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win.h:53: // Obtains the default adapter info (the first bluetooth radio info) and This comment isn't very clear to me: are you trying to say that the default adapter is the first bluetooth radio that is found on the system? https://chromiumcodereview.appspot.com/11345037/diff/19002/device/bluetooth/b... File device/bluetooth/bluetooth_adapter_win_unittest.cc (right): https://chromiumcodereview.appspot.com/11345037/diff/19002/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win_unittest.cc:53: adapter_ = new FakeBluetoothAdapterWin; parenthesis on the new please
https://chromiumcodereview.appspot.com/11345037/diff/19002/device/bluetooth/b... File device/bluetooth/bluetooth_adapter_win.h (right): https://chromiumcodereview.appspot.com/11345037/diff/19002/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win.h:16: class BluetoothAdapterWinTest; On 2012/11/12 20:35:13, bryeung wrote: > I don't think you need these if you're just making a friend. The compiler is throwing errors without this, so I will keep it here. https://chromiumcodereview.appspot.com/11345037/diff/19002/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win.h:51: friend class device::BluetoothAdapterWinTest; On 2012/11/12 20:35:13, bryeung wrote: > s/device::// Done. https://chromiumcodereview.appspot.com/11345037/diff/19002/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win.h:53: // Obtains the default adapter info (the first bluetooth radio info) and On 2012/11/12 20:35:13, bryeung wrote: > This comment isn't very clear to me: are you trying to say that the default > adapter is the first bluetooth radio that is found on the system? yes. I updated the comment. https://chromiumcodereview.appspot.com/11345037/diff/19002/device/bluetooth/b... File device/bluetooth/bluetooth_adapter_win_unittest.cc (right): https://chromiumcodereview.appspot.com/11345037/diff/19002/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win_unittest.cc:53: adapter_ = new FakeBluetoothAdapterWin; On 2012/11/12 20:35:13, bryeung wrote: > parenthesis on the new please Done. I thought new without parenthesis is preferred (at least bluetooth_adapter_factory has 'new chromeos::BluetoothAdapterChromeOs'.
lgtm https://chromiumcodereview.appspot.com/11345037/diff/19002/device/bluetooth/b... File device/bluetooth/bluetooth_adapter_win_unittest.cc (right): https://chromiumcodereview.appspot.com/11345037/diff/19002/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win_unittest.cc:53: adapter_ = new FakeBluetoothAdapterWin; On 2012/11/13 01:57:39, youngki wrote: > On 2012/11/12 20:35:13, bryeung wrote: > > parenthesis on the new please > > Done. I thought new without parenthesis is preferred (at least > bluetooth_adapter_factory has 'new chromeos::BluetoothAdapterChromeOs'. It only actually matters for POD types, but I think it's better to be in the habit of using parenthesis. https://chromiumcodereview.appspot.com/11345037/diff/6003/device/bluetooth/bl... File device/bluetooth/bluetooth_adapter_win.cc (right): https://chromiumcodereview.appspot.com/11345037/diff/6003/device/bluetooth/bl... device/bluetooth/bluetooth_adapter_win.cc:96: void BluetoothAdapterWin::TrackDefaultAdapter() { please make the order of definitions match the declaration order in the .h
Scott, could you take a look at this CL? I think we can land this CL regardless of the email discussion with Fred. https://chromiumcodereview.appspot.com/11345037/diff/19002/device/bluetooth/b... File device/bluetooth/bluetooth_adapter_win_unittest.cc (right): https://chromiumcodereview.appspot.com/11345037/diff/19002/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win_unittest.cc:53: adapter_ = new FakeBluetoothAdapterWin; On 2012/11/13 14:21:09, bryeung wrote: > On 2012/11/13 01:57:39, youngki wrote: > > On 2012/11/12 20:35:13, bryeung wrote: > > > parenthesis on the new please > > > > Done. I thought new without parenthesis is preferred (at least > > bluetooth_adapter_factory has 'new chromeos::BluetoothAdapterChromeOs'. > > It only actually matters for POD types, but I think it's better to be in the > habit of using parenthesis. Acknowledged. https://chromiumcodereview.appspot.com/11345037/diff/6003/device/bluetooth/bl... File device/bluetooth/bluetooth_adapter_win.cc (right): https://chromiumcodereview.appspot.com/11345037/diff/6003/device/bluetooth/bl... device/bluetooth/bluetooth_adapter_win.cc:96: void BluetoothAdapterWin::TrackDefaultAdapter() { On 2012/11/13 14:21:09, bryeung wrote: > please make the order of definitions match the declaration order in the .h Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/youngki@chromium.org/11345037/21001
Change committed as 167539 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
