Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11411130:
Implemented BluetoothTaskManagerWin class. (Closed)
|
Created:
8 years, 1 month ago by youngki Modified:
7 years, 11 months ago CC:
chromium-reviews, keybuk, grt (UTC plus 2), jam Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionImplemented BluetoothTaskManagerWin class.
BluetoothAdapterWin owns BluetoothTaskManagerWin and registers itself as an observer to BluetoothTaskManagerWin. The BluetoothTaskManagerWin polls the Bluetooth Windows API to get the up-to-date adapter status, and notifies BluetoothAdapterWin upon changes.
BUG=135470
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177024
Patch Set 1 #Patch Set 2 : Added comment #Patch Set 3 : Fixed merge error #Patch Set 4 : Updated bluetooth_adapter_win_unittest.cc. #Patch Set 5 : Removed unnecessary lines. #
Total comments: 10
Patch Set 6 : Use Observer pattern #
Total comments: 12
Patch Set 7 : Have all Windows API calls run on BluetoothPollingThread. #
Total comments: 2
Patch Set 8 : Renamed properties to adapter state #
Total comments: 11
Patch Set 9 : Made AdapterStateChange() private. #Patch Set 10 : Turned thread_checker_ and cancellation_flag_ from pointer to value. #Patch Set 11 : Changed ThreadChecker to base::ThreadChecker #Patch Set 12 : Turned thread_checker_ to value in polling thread. #
Total comments: 22
Patch Set 13 : Put GetAdapterState into anonymous namespace #
Total comments: 5
Patch Set 14 : Added BluetoothTaskManagerWin class #
Total comments: 28
Patch Set 15 : Added BluetoothPollingThreadWin::StartPolling() #Patch Set 16 : Replace naked MessageLoop pointer with SequencedTaskRunner refcounted pointer #
Total comments: 1
Patch Set 17 : Implemented Unittests #
Total comments: 2
Patch Set 18 : Replaced BluetoothPollingThreadWin with SequencedWorkerPool. #Patch Set 19 : Reverted DEPS #
Total comments: 16
Patch Set 20 : Fixed some style issues. #
Total comments: 16
Patch Set 21 : Use ScopedHandle #Patch Set 22 : Removed content dependency. #Patch Set 23 : updated device.gyp #Patch Set 24 : added back DEPS #Patch Set 25 : added back content dependency due to Device Test Suite. #
Total comments: 2
Patch Set 26 : changed to const char[] #Messages
Total messages: 47 (0 generated)
https://chromiumcodereview.appspot.com/11411130/diff/2010/device/bluetooth/bl... File device/bluetooth/bluetooth_adapter_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/2010/device/bluetooth/bl... device/bluetooth/bluetooth_adapter_win.cc:103: if (ERROR_SUCCESS == BluetoothGetRadioInfo(adapter_handle_, I think we should move all of the code to interface with the Windows API on to the other thread. That thread should post updates to this thread via some kind of observer interface. That is the same pattern used for BluetoothAdapterChromeOS and BluetoothAdapterClient (though you'll need different observer methods). You should make BluetoothAdapterWin implement the observer interface. Then you should be able to pass this as a raw pointer to the BluetoothManagerWin, since this will always outlive that thread. https://chromiumcodereview.appspot.com/11411130/diff/2010/device/bluetooth/bl... device/bluetooth/bluetooth_adapter_win.cc:113: powered_ = BluetoothIsConnectable(adapter_handle_) ? true : false; what happened to the check for BluetoothIsDiscoverable? And why use the ternary with true/false as values? https://chromiumcodereview.appspot.com/11411130/diff/2010/device/bluetooth/bl... File device/bluetooth/bluetooth_manager_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/2010/device/bluetooth/bl... device/bluetooth/bluetooth_manager_win.cc:16: const char* kBluetoothManagerThreadName = "BluetoothManagerThread"; no indent https://chromiumcodereview.appspot.com/11411130/diff/2010/device/bluetooth/bl... device/bluetooth/bluetooth_manager_win.cc:18: { sizeof(BLUETOOTH_FIND_RADIO_PARAMS) }; blank line https://chromiumcodereview.appspot.com/11411130/diff/2010/device/bluetooth/bl... File device/bluetooth/bluetooth_manager_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/2010/device/bluetooth/bl... device/bluetooth/bluetooth_manager_win.h:15: class BluetoothManagerWin : public base::Thread { maybe something more descriptive like "BluetoothPollingThread"?
https://chromiumcodereview.appspot.com/11411130/diff/2010/device/bluetooth/bl... File device/bluetooth/bluetooth_adapter_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/2010/device/bluetooth/bl... device/bluetooth/bluetooth_adapter_win.cc:103: if (ERROR_SUCCESS == BluetoothGetRadioInfo(adapter_handle_, On 2012/11/22 18:13:25, bryeung wrote: > I think we should move all of the code to interface with the Windows API on to > the other thread. That thread should post updates to this thread via some kind > of observer interface. That is the same pattern used for > BluetoothAdapterChromeOS and BluetoothAdapterClient (though you'll need > different observer methods). > > You should make BluetoothAdapterWin implement the observer interface. Then you > should be able to pass this as a raw pointer to the BluetoothManagerWin, since > this will always outlive that thread. Done. Now all the Windows specific API calls are moved to BluetoothPollingThreadWin. I think this will make it easier to write a unittest for BluetoothAdapterWin. https://chromiumcodereview.appspot.com/11411130/diff/2010/device/bluetooth/bl... device/bluetooth/bluetooth_adapter_win.cc:113: powered_ = BluetoothIsConnectable(adapter_handle_) ? true : false; On 2012/11/22 18:13:25, bryeung wrote: > what happened to the check for BluetoothIsDiscoverable? And why use the ternary > with true/false as values? BluetoothIsConnectable() does not exactly return bool; it returns BOOL, which is a typedef DWORD that only contains 1 or 0. Windows compiler has trouble casting BOOL into bool in assignment statement. I changed this by adding double negation in front of it. https://chromiumcodereview.appspot.com/11411130/diff/2010/device/bluetooth/bl... File device/bluetooth/bluetooth_manager_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/2010/device/bluetooth/bl... device/bluetooth/bluetooth_manager_win.cc:16: const char* kBluetoothManagerThreadName = "BluetoothManagerThread"; On 2012/11/22 18:13:25, bryeung wrote: > no indent Done. https://chromiumcodereview.appspot.com/11411130/diff/2010/device/bluetooth/bl... device/bluetooth/bluetooth_manager_win.cc:18: { sizeof(BLUETOOTH_FIND_RADIO_PARAMS) }; On 2012/11/22 18:13:25, bryeung wrote: > blank line Done. https://chromiumcodereview.appspot.com/11411130/diff/2010/device/bluetooth/bl... File device/bluetooth/bluetooth_manager_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/2010/device/bluetooth/bl... device/bluetooth/bluetooth_manager_win.h:15: class BluetoothManagerWin : public base::Thread { On 2012/11/22 18:13:25, bryeung wrote: > maybe something more descriptive like "BluetoothPollingThread"? Done.
I haven't updated the unittest yet; let me know if the change is good enough to have the unittest updated.
https://chromiumcodereview.appspot.com/11411130/diff/9002/device/bluetooth/bl... File device/bluetooth/bluetooth_adapter_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/9002/device/bluetooth/bl... device/bluetooth/bluetooth_adapter_win.cc:85: const BluetoothPollingThreadWin::Properties& properties) { CHECK the thread here https://chromiumcodereview.appspot.com/11411130/diff/9002/device/bluetooth/bl... File device/bluetooth/bluetooth_polling_thread_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/9002/device/bluetooth/bl... device/bluetooth/bluetooth_polling_thread_win.cc:58: void BluetoothPollingThreadWin::FindAdapterHandleAndReply( This method name no longer makes sense. PollAdapater would be better (especially with my comment below about where to re-poll). https://chromiumcodereview.appspot.com/11411130/diff/9002/device/bluetooth/bl... device/bluetooth/bluetooth_polling_thread_win.cc:59: base::WeakPtr<BluetoothPollingThreadWin> weak_ptr) const { CHECK for thread would be nice here https://chromiumcodereview.appspot.com/11411130/diff/9002/device/bluetooth/bl... device/bluetooth/bluetooth_polling_thread_win.cc:76: adapter_handle_ = adapter_handle; I'd prefer if the only thing that happened on the UI thread was calling the observers. That probably means having a function that takes a scoped_ptr<const Properties> that does the observer calls and posting a call to that method on the UI thread. https://chromiumcodereview.appspot.com/11411130/diff/9002/device/bluetooth/bl... device/bluetooth/bluetooth_polling_thread_win.cc:82: message_loop()->PostDelayedTask( This would probably be better up in FindAdapterHandleAndReply (and then rename it to something like PollAdapter). https://chromiumcodereview.appspot.com/11411130/diff/9002/device/bluetooth/bl... File device/bluetooth/bluetooth_polling_thread_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/9002/device/bluetooth/bl... device/bluetooth/bluetooth_polling_thread_win.h:26: virtual void AdapterPropertyChanged(const Properties& properties) {} Please document that this will always be called on the UI thread.
https://chromiumcodereview.appspot.com/11411130/diff/9002/device/bluetooth/bl... File device/bluetooth/bluetooth_adapter_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/9002/device/bluetooth/bl... device/bluetooth/bluetooth_adapter_win.cc:85: const BluetoothPollingThreadWin::Properties& properties) { On 2012/11/23 13:32:22, bryeung wrote: > CHECK the thread here Done. https://chromiumcodereview.appspot.com/11411130/diff/9002/device/bluetooth/bl... File device/bluetooth/bluetooth_polling_thread_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/9002/device/bluetooth/bl... device/bluetooth/bluetooth_polling_thread_win.cc:58: void BluetoothPollingThreadWin::FindAdapterHandleAndReply( On 2012/11/23 13:32:22, bryeung wrote: > This method name no longer makes sense. PollAdapater would be better > (especially with my comment below about where to re-poll). Done. https://chromiumcodereview.appspot.com/11411130/diff/9002/device/bluetooth/bl... device/bluetooth/bluetooth_polling_thread_win.cc:59: base::WeakPtr<BluetoothPollingThreadWin> weak_ptr) const { On 2012/11/23 13:32:22, bryeung wrote: > CHECK for thread would be nice here Done. https://chromiumcodereview.appspot.com/11411130/diff/9002/device/bluetooth/bl... device/bluetooth/bluetooth_polling_thread_win.cc:76: adapter_handle_ = adapter_handle; On 2012/11/23 13:32:22, bryeung wrote: > I'd prefer if the only thing that happened on the UI thread was calling the > observers. That probably means having a function that takes a scoped_ptr<const > Properties> that does the observer calls and posting a call to that method on > the UI thread. Done. https://chromiumcodereview.appspot.com/11411130/diff/9002/device/bluetooth/bl... device/bluetooth/bluetooth_polling_thread_win.cc:82: message_loop()->PostDelayedTask( On 2012/11/23 13:32:22, bryeung wrote: > This would probably be better up in FindAdapterHandleAndReply (and then rename > it to something like PollAdapter). Done. https://chromiumcodereview.appspot.com/11411130/diff/9002/device/bluetooth/bl... File device/bluetooth/bluetooth_polling_thread_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/9002/device/bluetooth/bl... device/bluetooth/bluetooth_polling_thread_win.h:26: virtual void AdapterPropertyChanged(const Properties& properties) {} On 2012/11/23 13:32:22, bryeung wrote: > Please document that this will always be called on the UI thread. Done.
https://chromiumcodereview.appspot.com/11411130/diff/3014/device/bluetooth/bl... File device/bluetooth/bluetooth_polling_thread_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/3014/device/bluetooth/bl... device/bluetooth/bluetooth_polling_thread_win.h:27: }; Confused about this - it appears to "emulate" a D-Bus properties structure, while not actually implementing those methods. What's the intent here? Could this be renamed to prevent confusion?
https://chromiumcodereview.appspot.com/11411130/diff/3014/device/bluetooth/bl... File device/bluetooth/bluetooth_polling_thread_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/3014/device/bluetooth/bl... device/bluetooth/bluetooth_polling_thread_win.h:27: }; On 2012/11/26 18:36:13, keybuk wrote: > Confused about this - it appears to "emulate" a D-Bus properties structure, > while not actually implementing those methods. > > What's the intent here? Could this be renamed to prevent confusion? Renamed this to AdapterState.
Didn't have time to get through the whole CL...but thought I'd send along these comments at least. https://chromiumcodereview.appspot.com/11411130/diff/1014/device/bluetooth/bl... File device/bluetooth/bluetooth_adapter_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/1014/device/bluetooth/bl... device/bluetooth/bluetooth_adapter_win.h:52: // BluetoothPollingThreadWin override you probably mean ::Observer here Also, should this really be public? https://chromiumcodereview.appspot.com/11411130/diff/1014/device/bluetooth/bl... device/bluetooth/bluetooth_adapter_win.h:75: scoped_ptr<base::ThreadChecker> thread_checker_; why is this a pointer? https://chromiumcodereview.appspot.com/11411130/diff/1014/device/bluetooth/bl... File device/bluetooth/bluetooth_polling_thread_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/1014/device/bluetooth/bl... device/bluetooth/bluetooth_polling_thread_win.cc:95: thread_checker_.reset(new base::ThreadChecker()); I think the right pattern here would be for the class member to be a value (not a pointer) and to call DetachFromThread on it in the constructor. https://chromiumcodereview.appspot.com/11411130/diff/1014/device/bluetooth/bl... File device/bluetooth/bluetooth_polling_thread_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/1014/device/bluetooth/bl... device/bluetooth/bluetooth_polling_thread_win.h:59: scoped_ptr<base::CancellationFlag> cancellation_flag_; why a pointer?
https://chromiumcodereview.appspot.com/11411130/diff/1014/device/bluetooth/bl... File device/bluetooth/bluetooth_adapter_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/1014/device/bluetooth/bl... device/bluetooth/bluetooth_adapter_win.h:52: // BluetoothPollingThreadWin override On 2012/11/27 20:37:04, bryeung wrote: > you probably mean ::Observer here > > Also, should this really be public? Done. https://chromiumcodereview.appspot.com/11411130/diff/1014/device/bluetooth/bl... device/bluetooth/bluetooth_adapter_win.h:75: scoped_ptr<base::ThreadChecker> thread_checker_; On 2012/11/27 20:37:04, bryeung wrote: > why is this a pointer? Because I want to forward-declare it instead of including header: http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Forward.... https://chromiumcodereview.appspot.com/11411130/diff/1014/device/bluetooth/bl... File device/bluetooth/bluetooth_polling_thread_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/1014/device/bluetooth/bl... device/bluetooth/bluetooth_polling_thread_win.cc:95: thread_checker_.reset(new base::ThreadChecker()); On 2012/11/27 20:37:04, bryeung wrote: > I think the right pattern here would be for the class member to be a value (not > a pointer) and to call DetachFromThread on it in the constructor. I made this pointer because I thought forward-declaring is preferred in chrome. If that's not the case I can then turn this to be a value and call DetachFromThread. Let me know. https://chromiumcodereview.appspot.com/11411130/diff/1014/device/bluetooth/bl... File device/bluetooth/bluetooth_polling_thread_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/1014/device/bluetooth/bl... device/bluetooth/bluetooth_polling_thread_win.h:59: scoped_ptr<base::CancellationFlag> cancellation_flag_; On 2012/11/27 20:37:04, bryeung wrote: > why a pointer? I want to forward-declare it instead of adding header: http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Forward....
https://chromiumcodereview.appspot.com/11411130/diff/1014/device/bluetooth/bl... File device/bluetooth/bluetooth_adapter_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/1014/device/bluetooth/bl... device/bluetooth/bluetooth_adapter_win.h:75: scoped_ptr<base::ThreadChecker> thread_checker_; On 2012/11/27 21:43:34, youngki wrote: > On 2012/11/27 20:37:04, bryeung wrote: > > why is this a pointer? > > Because I want to forward-declare it instead of including header: > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Forward.... I think you are misinterpreting that section of the style guide. The intention is use forward-declaration when possible, not write the code specifically to make forward declarations possible. You should generally prefer value members to pointers, unless the pointer is necessary for the implementation.
https://chromiumcodereview.appspot.com/11411130/diff/1014/device/bluetooth/bl... File device/bluetooth/bluetooth_adapter_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/1014/device/bluetooth/bl... device/bluetooth/bluetooth_adapter_win.h:75: scoped_ptr<base::ThreadChecker> thread_checker_; On 2012/11/28 13:41:11, bryeung wrote: > On 2012/11/27 21:43:34, youngki wrote: > > On 2012/11/27 20:37:04, bryeung wrote: > > > why is this a pointer? > > > > Because I want to forward-declare it instead of including header: > > > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Forward.... > > I think you are misinterpreting that section of the style guide. The intention > is use forward-declaration when possible, not write the code specifically to > make forward declarations possible. You should generally prefer value members > to pointers, unless the pointer is necessary for the implementation. Done. https://chromiumcodereview.appspot.com/11411130/diff/1014/device/bluetooth/bl... File device/bluetooth/bluetooth_polling_thread_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/1014/device/bluetooth/bl... device/bluetooth/bluetooth_polling_thread_win.cc:95: thread_checker_.reset(new base::ThreadChecker()); On 2012/11/27 20:37:04, bryeung wrote: > I think the right pattern here would be for the class member to be a value (not > a pointer) and to call DetachFromThread on it in the constructor. I think we should use pointer for this because if we use value and call DetachFromThread from the constructor, here we don't know whether the thread_checker_ has been reattached or not.
https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... File device/bluetooth/bluetooth_adapter_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win.h:18: class ThreadChecker; no longer necessary now that you have the header included https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win.h:74: scoped_ptr<BluetoothPollingThreadWin> thread_; it occurs to me that you could get an equivalently good guarantee by just making this a value member as well :-) https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... File device/bluetooth/bluetooth_polling_thread_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.cc:36: cancellation_flag_.Set(); Are you sure this is the right pattern? To me, this looks like the cancellation flag will be set and then immediately destructed when this method completes. It's not clear to me how that is helpful. https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.cc:41: DCHECK(observer); CHECK is probably better here (and below) https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.cc:87: base::WeakPtr<BluetoothPollingThreadWin> weak_ptr) { why is this taking a weak_ptr? When do you intend for the argument weak_ptr to be different than |this|? Why? https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.cc:119: const AdapterState* state) { CHECK the thread here https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... File device/bluetooth/bluetooth_polling_thread_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.h:16: class ThreadChecker; not needed with the headers above https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.h:49: static void GetAdapterState(const HANDLE adapter_handle, this should go inside an anonymous namespace in the .cc file if there is no reason for it be part of the this class https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.h:52: // Polls Windows Bluetooth API on BluetoothPollingThread. More descriptive comments would be nice, e.g.: "Called periodically to get the latest state of the Bluetooth Adapter from the Windows APIs. Posts another call to PollAdapter unless |cancellation_flag_| is set." https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.h:55: // Callback to be run on the UI thread. "Notify all Observers of updated AdapterState. Should only be called on the UI thread." https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.h:66: // List of observers interested in event notifications from us. s/from us//
https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... File device/bluetooth/bluetooth_adapter_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win.h:18: class ThreadChecker; On 2012/11/28 16:26:08, bryeung wrote: > no longer necessary now that you have the header included Done. https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win.h:74: scoped_ptr<BluetoothPollingThreadWin> thread_; On 2012/11/28 16:26:08, bryeung wrote: > it occurs to me that you could get an equivalently good guarantee by just making > this a value member as well :-) My only concern is testing; I was going to replace this with MockBluetoothPollingThreadWin for unittest, and if I make this a value I don't know how I could effectively test this class. https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... File device/bluetooth/bluetooth_polling_thread_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.cc:36: cancellation_flag_.Set(); On 2012/11/28 16:26:08, bryeung wrote: > Are you sure this is the right pattern? To me, this looks like the cancellation > flag will be set and then immediately destructed when this method completes. > It's not clear to me how that is helpful. Thread::Stop() on the next line waits until all the pending tasks in BluetoothPollingThread finish. So setting cancellation_flag_ here will make those tasks skip the most of the logics and quit. Also this class won't get destroyed until it makes sure the thread completely terminates. I documented this as comments here. https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.cc:41: DCHECK(observer); On 2012/11/28 16:26:08, bryeung wrote: > CHECK is probably better here (and below) Done. https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.cc:87: base::WeakPtr<BluetoothPollingThreadWin> weak_ptr) { On 2012/11/28 16:26:08, bryeung wrote: > why is this taking a weak_ptr? When do you intend for the argument weak_ptr to > be different than |this|? Why? All the instances that we intend to post tasks to UI thread have to be weak pointers because we cannot guarantee that the instance will live until the UI thread terminates. https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.cc:119: const AdapterState* state) { On 2012/11/28 16:26:08, bryeung wrote: > CHECK the thread here Done. I also changed all DCHECK in this file to CHECK. https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... File device/bluetooth/bluetooth_polling_thread_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.h:16: class ThreadChecker; On 2012/11/28 16:26:08, bryeung wrote: > not needed with the headers above Done. https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.h:49: static void GetAdapterState(const HANDLE adapter_handle, On 2012/11/28 16:26:08, bryeung wrote: > this should go inside an anonymous namespace in the .cc file if there is no > reason for it be part of the this class Done. https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.h:52: // Polls Windows Bluetooth API on BluetoothPollingThread. On 2012/11/28 16:26:08, bryeung wrote: > More descriptive comments would be nice, e.g.: > "Called periodically to get the latest state of the Bluetooth Adapter from the > Windows APIs. Posts another call to PollAdapter unless |cancellation_flag_| is > set." Done. https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.h:55: // Callback to be run on the UI thread. On 2012/11/28 16:26:08, bryeung wrote: > "Notify all Observers of updated AdapterState. Should only be called on the UI > thread." Done. https://chromiumcodereview.appspot.com/11411130/diff/10006/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.h:66: // List of observers interested in event notifications from us. On 2012/11/28 16:26:08, bryeung wrote: > s/from us// Done.
Bryan, I looked at several other examples how the separate threads are managed, and I noticed that they have "manager" that owns the thread, and manager code is run on UI thread and the other thread code runs on its own thread. And the manager is RefCountedThreadSafe inherited class, so the thread holds the pointer to it and passes its reference-counted pointer when posting a task. So I created BluetoothTaskManagerWin class that owns BluetoothPollingThreadWin. Now all the code in BluetoothTaskManagerWin class runs in UI thread, whereas all the code except constructor, destructor, and Cancel() in BluetoothPollingThreadWin runs in its own thread. Also there is no weak pointer involved in those two classes anymore. Please take a look.
ping
Please ignore these! (These are a bunch of old drafts that probably no longer apply...but I don't see a way to discard them all in the UI without just sending them. Please ignore.) https://chromiumcodereview.appspot.com/11411130/diff/2014/device/bluetooth/bl... File device/bluetooth/bluetooth_polling_thread_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/2014/device/bluetooth/bl... device/bluetooth/bluetooth_polling_thread_win.cc:79: void BluetoothPollingThreadWin::Init() { CHECK that you're on the UI thread here https://chromiumcodereview.appspot.com/11411130/diff/2014/device/bluetooth/bl... device/bluetooth/bluetooth_polling_thread_win.cc:80: if (!message_loop()) { // WeakPtrs are bound to the thread that creates the first WeakPtr for the object. Try to check that the WeakPtr below will be bound to the this (the UI) thread. This should be the ONLY place where GetWeakPtr() is called on this weak_ptr_factory_. CHECK(!weak_ptr_factory_.HasWeakPtrs()); base::WeakPtr<BluetoothPollingThreadWin> weak_ptr(weak_ptr_factory_.GetWeakPtr()); https://chromiumcodereview.appspot.com/11411130/diff/2014/device/bluetooth/bl... device/bluetooth/bluetooth_polling_thread_win.cc:91: base::WeakPtr<BluetoothPollingThreadWin> weak_ptr) { rename this to weak_ptr_for_ui_thread https://chromiumcodereview.appspot.com/11411130/diff/2014/device/bluetooth/bl... device/bluetooth/bluetooth_polling_thread_win.cc:124: CHECK(thread_checker_.CalledOnValidThread()); use a separate, ui_thread_checker_ here https://chromiumcodereview.appspot.com/11411130/diff/2014/device/bluetooth/bl... File device/bluetooth/bluetooth_polling_thread_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/2014/device/bluetooth/bl... device/bluetooth/bluetooth_polling_thread_win.h:43: // |cancellation_flag_| is set. Include a comment here about why the weak_ptr is necessary. Something like: "This methods requires |weak_ptr_for_ui_thread| because OnAdapterStateChanged must be called on a weak reference to this object on the UI thread. We cannot create the weak reference on this thread because weak_ptrs cannot be dereferenced from a thread different than their owning thread."
https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... File device/bluetooth/bluetooth_adapter_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win.cc:26: // destructor because it is reference counted. who else would be holding a reference to it? https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... File device/bluetooth/bluetooth_polling_thread_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.cc:59: DCHECK_EQ(task_manager_->message_loop(), MessageLoop::current()); don't use task_manager_ here...you've passed it in from the task manager constructor and you should not make use of the |this| pointer from within an initializer list. https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.cc:62: // Stop() waits until all the pending tasks in BluetoothPollingThread finish. this comment doesn't make sense here: maybe you mean "Cancel() ..."? The next two sentences don't seem to add much: probably not necessary. https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.cc:68: Cancel(); is there a way to CHECK that the message loop has no outstanding tasks on it? https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.cc:81: DCHECK_EQ(message_loop(), MessageLoop::current()); does this have to go below the cancellation_flag_ check? If not, I'd prefer it above. https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.cc:94: task_manager_->message_loop()->PostTask( if adapter_handle was NULL, we probably shouldn't send the update https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... File device/bluetooth/bluetooth_polling_thread_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.h:17: // should run on its own thread. would be nice to specify here (and on the methods below) that the constructor, destruct and Cancel MUST be run from the task manager's message loop. https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... File device/bluetooth/bluetooth_task_manager_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:17: DCHECK(ui_message_loop_->type() == MessageLoop::TYPE_UI); why have you switched to checking the message loop instead of the Thread? https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:33: void BluetoothTaskManagerWin::StartThread() { CHECKing for thread here would be good https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:35: thread_.Start(); can thread_.Start() be overridden so that it posts this first task after calling Thread::Start() ? https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:43: void BluetoothTaskManagerWin::StopThread() { CHECKing for thread here would be good https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... File device/bluetooth/bluetooth_task_manager_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.h:21: // All the code in this class runs in the UI thread. This deserves a more thorough comment about why we have two classes for this, and the ownership/thread conventions that were necessary. https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.h:50: friend class BluetoothPollingThreadWin; why is this necessary?
https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... File device/bluetooth/bluetooth_adapter_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win.h:23: private BluetoothTaskManagerWin::Observer { you'll probably want this public so you can call BluetoothTaskManagerWin::Observer methods from test cases
Thanks for the reviews. Please take another look. https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... File device/bluetooth/bluetooth_adapter_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win.cc:26: // destructor because it is reference counted. On 2012/12/10 19:51:28, bryeung wrote: > who else would be holding a reference to it? BluetoothPollingThreadWin::PollAdapter() queues base::Bind(BluetoothTaskManagerWin::OnAdapterStateChanged, make_scoped_refptr(task_manager_)) to UI thread. So there is a possibility that BluetoothAdapterWin is destroyed but that message_loop still holds the reference to BluetoothTaskManagerWin. https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... File device/bluetooth/bluetooth_adapter_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win.h:23: private BluetoothTaskManagerWin::Observer { On 2012/12/12 18:01:53, keybuk wrote: > you'll probably want this public so you can call > BluetoothTaskManagerWin::Observer methods from test cases Done. https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... File device/bluetooth/bluetooth_polling_thread_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.cc:59: DCHECK_EQ(task_manager_->message_loop(), MessageLoop::current()); On 2012/12/10 19:51:28, bryeung wrote: > don't use task_manager_ here...you've passed it in from the task manager > constructor and you should not make use of the |this| pointer from within an > initializer list. Done. https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.cc:62: // Stop() waits until all the pending tasks in BluetoothPollingThread finish. On 2012/12/10 19:51:28, bryeung wrote: > this comment doesn't make sense here: maybe you mean "Cancel() ..."? > > The next two sentences don't seem to add much: probably not necessary. I moved the comment to Cancel() and modified it. https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.cc:68: Cancel(); On 2012/12/10 19:51:28, bryeung wrote: > is there a way to CHECK that the message loop has no outstanding tasks on it? I don't think that's necessary.. That must be guaranteed by Thread::Stop(), which is actually being used in a lot of places in Chrome. I think it's Thread::Stop() responsibility to make sure there is no outstanding task. https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.cc:81: DCHECK_EQ(message_loop(), MessageLoop::current()); On 2012/12/10 19:51:28, bryeung wrote: > does this have to go below the cancellation_flag_ check? If not, I'd prefer it > above. Done. https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.cc:94: task_manager_->message_loop()->PostTask( On 2012/12/10 19:51:28, bryeung wrote: > if adapter_handle was NULL, we probably shouldn't send the update That's not true. If adapter_handle is NULL we still have to send an update to invalidate the previous adapter state since adapter is no longer available. https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... File device/bluetooth/bluetooth_polling_thread_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.h:17: // should run on its own thread. On 2012/12/10 19:51:28, bryeung wrote: > would be nice to specify here (and on the methods below) that the constructor, > destruct and Cancel MUST be run from the task manager's message loop. Done. https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... File device/bluetooth/bluetooth_task_manager_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:17: DCHECK(ui_message_loop_->type() == MessageLoop::TYPE_UI); On 2012/12/10 19:51:28, bryeung wrote: > why have you switched to checking the message loop instead of the Thread? I was following the existing patterns from other files that have the similar thread relationship: https://cs.corp.google.com/#chrome/src/chrome/browser/printing/print_job.cc&s... I think this is better for unit-testing purpose although I haven't added MessageLoop::TYPE_DEFAULT since I haven't updated the test yet. https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:33: void BluetoothTaskManagerWin::StartThread() { On 2012/12/10 19:51:28, bryeung wrote: > CHECKing for thread here would be good Done. https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:35: thread_.Start(); On 2012/12/10 19:51:28, bryeung wrote: > can thread_.Start() be overridden so that it posts this first task after calling > Thread::Start() ? Thread::Start() is not virtual, but I added BluetoothPollingThreadWin::StartPolling(). https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:43: void BluetoothTaskManagerWin::StopThread() { On 2012/12/10 19:51:28, bryeung wrote: > CHECKing for thread here would be good Done. https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... File device/bluetooth/bluetooth_task_manager_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.h:21: // All the code in this class runs in the UI thread. On 2012/12/10 19:51:28, bryeung wrote: > This deserves a more thorough comment about why we have two classes for this, > and the ownership/thread conventions that were necessary. I put some more comments to explain the message passing between this class and the thread, and why this is reference-counted. https://chromiumcodereview.appspot.com/11411130/diff/16001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.h:50: friend class BluetoothPollingThreadWin; On 2012/12/10 19:51:28, bryeung wrote: > why is this necessary? This is necessary so that BluetoothPollingThreadWin can call OnAdapterStateChanged() below.
I replaced raw MessageLoop* pointer with safer SequencedTaskRunner refcounted pointer. Please take a look.
ping?
lgtm
lgtm https://chromiumcodereview.appspot.com/11411130/diff/30001/device/bluetooth/b... File device/bluetooth/bluetooth_polling_thread_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/30001/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.cc:30: // A frame-based exception handler filter functionj for a handler for exceptions typo: "functionj"?
The following unittests are implemented: bluetooth_adapter_win_unittest.cc bluetooth_task_manager_win_unittest.cc bluetooth_polling_thread_win_unittest.cc Please take a look at the unittests as well. Also I will ask Brett if he could review this CL as well.
Sorry if I don't have a lot of background on this before now. Just to double-check, you don't start doing anything until an extension requests so, right? I wasn't sure how the high-level dependencies should work for this so I talked to John. We're not sure why the device directory is a consumer of content. I would have expected this to be at a lower level of abstraction. It looks like the only thing you need this for is BrowserThread for a few things, and some unit test stuff which is probably only needed to set up the browser thread stuff. If this is correct, we should try to remove the content dependency since this layer isn't really consuming "web page rendering" APIs. If you have some reasonable init or creation function that's called on the main thread, you can stash a pointer to the message loop proxy for that thread and use it for your assertion comparisons later. You would also pass in a pointer to the appropriate thread to use for blocking I/O operations that dbus should use. It would be much better to use the blocking thread pool for this, but I don't know what our dbus library's requirements are so this may not be feasible. As for your dedicated bluetooth thread, it looks like all you need it to do is some slow blocking thing every 1/2 second? Are there other requirements of this thread? If that's indeed all you need, I recommend using the sequenced worker pool for this. It has a set of threads and will do your work on the "next available one" which should be fine for your purpose as far as I understand and will nicely load balance on the existing threads without making new ones. It will also save a bunch of memory since each thread has a lot of stack, etc. reserved for it. You can get the TaskRunner interface for this in your constructor and keep a ref to it. Then you can just re-post to this periodically, and it may even go through a different thread each time (depending on load). https://chromiumcodereview.appspot.com/11411130/diff/36001/device/bluetooth/b... File device/bluetooth/bluetooth_polling_thread_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/36001/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.cc:43: bool HasBluetoothStack() { It looks like this is never called?
Thanks for the detailed review Brett..! I am implementing a new patch based on your suggestion, but here is my response for now. On 2013/01/04 22:05:34, brettw wrote: > Sorry if I don't have a lot of background on this before now. Just to > double-check, you don't start doing anything until an extension requests so, > right? Yes that's right. > > I wasn't sure how the high-level dependencies should work for this so I talked > to John. We're not sure why the device directory is a consumer of content. I > would have expected this to be at a lower level of abstraction. It looks like > the only thing you need this for is BrowserThread for a few things, and some > unit test stuff which is probably only needed to set up the browser thread > stuff. > > If this is correct, we should try to remove the content dependency since this > layer isn't really consuming "web page rendering" APIs. If you have some > reasonable init or creation function that's called on the main thread, you can > stash a pointer to the message loop proxy for that thread and use it for your > assertion comparisons later. Yes I added content/ dependency just for BrowserThread and for unittesting stuff. I haven't looked at it in detail I could do this without BrowserThread but I will take a look and try to apply your suggestion. > > You would also pass in a pointer to the appropriate thread to use for blocking > I/O operations that dbus should use. It would be much better to use the > blocking thread pool for this, but I don't know what our dbus library's > requirements are so this may not be feasible. Are you suggesting that I should use blocking thread pool instead of a dedicated thread: BluetoothPollingThread in this CL? Is SequencedWorkerPool a blocking thread pool? > > > As for your dedicated bluetooth thread, it looks like all you need it to do is > some slow blocking thing every 1/2 second? Are there other requirements of this > thread? If that's indeed all you need, I recommend using the sequenced worker > pool for this. It has a set of threads and will do your work on the "next > available one" which should be fine for your purpose as far as I understand and > will nicely load balance on the existing threads without making new ones. It > will also save a bunch of memory since each thread has a lot of stack, etc. > reserved for it. Yes that's the only requirement. I will take a look into SequencedWorkerPool. > > You can get the TaskRunner interface for this in your constructor and keep a > ref to it. Then you can just re-post to this periodically, and it may even go > through a different thread each time (depending on load). > > https://chromiumcodereview.appspot.com/11411130/diff/36001/device/bluetooth/b... > File device/bluetooth/bluetooth_polling_thread_win.cc (right): > > https://chromiumcodereview.appspot.com/11411130/diff/36001/device/bluetooth/b... > device/bluetooth/bluetooth_polling_thread_win.cc:43: bool HasBluetoothStack() { > It looks like this is never called? I will add this function call as well.
The blocking pool is the name (in browser_thread.h) for an instance of a SequencedWorkerPool in base that we use for random blocking I/O. Brett On Mon, Jan 7, 2013 at 8:20 AM, <youngki@chromium.org> wrote: > Thanks for the detailed review Brett..! I am implementing a new patch based > on > your suggestion, but here is my response for now. > > > On 2013/01/04 22:05:34, brettw wrote: >> >> Sorry if I don't have a lot of background on this before now. Just to >> double-check, you don't start doing anything until an extension requests >> so, >> right? > > > Yes that's right. > > > >> I wasn't sure how the high-level dependencies should work for this so I >> talked >> to John. We're not sure why the device directory is a consumer of >> content. I >> would have expected this to be at a lower level of abstraction. It looks >> like >> the only thing you need this for is BrowserThread for a few things, and >> some >> unit test stuff which is probably only needed to set up the browser thread >> stuff. > > >> If this is correct, we should try to remove the content dependency since >> this >> layer isn't really consuming "web page rendering" APIs. If you have some >> reasonable init or creation function that's called on the main thread, you >> can >> stash a pointer to the message loop proxy for that thread and use it for >> your >> assertion comparisons later. > > > Yes I added content/ dependency just for BrowserThread and for unittesting > stuff. I haven't looked at it in detail I could do this without > BrowserThread > but I will take a look and try to apply your suggestion. > > > >> You would also pass in a pointer to the appropriate thread to use for >> blocking >> I/O operations that dbus should use. It would be much better to use the >> blocking thread pool for this, but I don't know what our dbus library's >> requirements are so this may not be feasible. > > > Are you suggesting that I should use blocking thread pool instead of a > dedicated > thread: BluetoothPollingThread in this CL? Is SequencedWorkerPool a blocking > thread pool? > > > > >> As for your dedicated bluetooth thread, it looks like all you need it to >> do is >> some slow blocking thing every 1/2 second? Are there other requirements of > > this >> >> thread? If that's indeed all you need, I recommend using the sequenced >> worker >> pool for this. It has a set of threads and will do your work on the "next >> available one" which should be fine for your purpose as far as I >> understand > > and >> >> will nicely load balance on the existing threads without making new ones. >> It >> will also save a bunch of memory since each thread has a lot of stack, >> etc. >> reserved for it. > > > Yes that's the only requirement. I will take a look into > SequencedWorkerPool. > > > >> You can get the TaskRunner interface for this in your constructor and keep >> a >> ref to it. Then you can just re-post to this periodically, and it may even >> go >> through a different thread each time (depending on load). > > > > https://chromiumcodereview.appspot.com/11411130/diff/36001/device/bluetooth/b... >> >> File device/bluetooth/bluetooth_polling_thread_win.cc (right): > > > > https://chromiumcodereview.appspot.com/11411130/diff/36001/device/bluetooth/b... >> >> device/bluetooth/bluetooth_polling_thread_win.cc:43: bool >> HasBluetoothStack() > > { >> >> It looks like this is never called? > > > I will add this function call as well. > > https://chromiumcodereview.appspot.com/11411130/
Hey Brett, could you take another look? I implemented BluetoothTaskManager using SequencedWorkerPool and removed BluetoothPollingThreadWin entirely. I removed content dependency from device, although content/public/test is still being used in unittests. But I think it's okay to use it in unittests. https://chromiumcodereview.appspot.com/11411130/diff/36001/device/bluetooth/b... File device/bluetooth/bluetooth_polling_thread_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/36001/device/bluetooth/b... device/bluetooth/bluetooth_polling_thread_win.cc:43: bool HasBluetoothStack() { On 2013/01/04 22:05:34, brettw wrote: > It looks like this is never called? Added the function call. I also added TODO that this check should really be handled when BluetoothAdapter is initialized; I couldn't do it in this CL because adding BluetoothAdapterWin::Init() depends on few other pending CLs including https://chromiumcodereview.appspot.com/11743024/ .
On 2013/01/07 23:56:34, youngki wrote: > Hey Brett, could you take another look? I implemented BluetoothTaskManager using > SequencedWorkerPool and removed BluetoothPollingThreadWin entirely. > > I removed content dependency from device, although content/public/test is still > being used in unittests. But I think it's okay to use it in unittests. > > https://chromiumcodereview.appspot.com/11411130/diff/36001/device/bluetooth/b... > File device/bluetooth/bluetooth_polling_thread_win.cc (right): > > https://chromiumcodereview.appspot.com/11411130/diff/36001/device/bluetooth/b... > device/bluetooth/bluetooth_polling_thread_win.cc:43: bool HasBluetoothStack() { > On 2013/01/04 22:05:34, brettw wrote: > > It looks like this is never called? > > Added the function call. I also added TODO that this check should really be > handled when BluetoothAdapter is initialized; I couldn't do it in this CL > because adding BluetoothAdapterWin::Init() depends on few other pending CLs > including https://chromiumcodereview.appspot.com/11743024/ . Friendly ping. Brett, could you take a look?
Thread stuff looks good. I have a few general comments. We should try to get the team remove the rest of the content dependency in a followup. https://chromiumcodereview.appspot.com/11411130/diff/46001/device/bluetooth/b... File device/bluetooth/bluetooth_adapter_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/46001/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win.h:58: // Starts BluetoothPollingThreadWin thread to obtain the default adapter info. I think this comment is out of date. https://chromiumcodereview.appspot.com/11411130/diff/46001/device/bluetooth/b... File device/bluetooth/bluetooth_task_manager_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/46001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:13: #include <string> Blank line after this as well as before (separating C includes, C++ includes, and our includes). https://chromiumcodereview.appspot.com/11411130/diff/46001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:24: #pragma comment(lib, "delayimp.lib") Carlos seemed to be of the opinion that this is not necessary. Does it still work without this line? https://chromiumcodereview.appspot.com/11411130/diff/46001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:55: } __except(FilterVisualCPPExceptions(::GetExceptionCode())) { It looks like you only need 3 bluetooth functions. Is that right? If so, it seems more clear to me to just use LoadLibrary/GetProcAddress rather than dealing with this exception stuff which most people don't have any experience with. https://chromiumcodereview.appspot.com/11411130/diff/46001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:89: } Should have a " // namespace" comment https://chromiumcodereview.appspot.com/11411130/diff/46001/device/bluetooth/b... File device/bluetooth/bluetooth_task_manager_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/46001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.h:8: #include <string> Blank line after this.
Greg, could you answer Brett's question? He's asking if we could remove exception and just use LoadLibrary on HasBluetoothStack() function. https://chromiumcodereview.appspot.com/11411130/diff/46001/device/bluetooth/b... File device/bluetooth/bluetooth_adapter_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/46001/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win.h:58: // Starts BluetoothPollingThreadWin thread to obtain the default adapter info. On 2013/01/09 20:06:04, brettw wrote: > I think this comment is out of date. I just removed the comment. https://chromiumcodereview.appspot.com/11411130/diff/46001/device/bluetooth/b... File device/bluetooth/bluetooth_task_manager_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/46001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:13: #include <string> On 2013/01/09 20:06:04, brettw wrote: > Blank line after this as well as before (separating C includes, C++ includes, > and our includes). Done. https://chromiumcodereview.appspot.com/11411130/diff/46001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:24: #pragma comment(lib, "delayimp.lib") On 2013/01/09 20:06:04, brettw wrote: > Carlos seemed to be of the opinion that this is not necessary. Does it still > work without this line? Done. https://chromiumcodereview.appspot.com/11411130/diff/46001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:55: } __except(FilterVisualCPPExceptions(::GetExceptionCode())) { On 2013/01/09 20:06:04, brettw wrote: > It looks like you only need 3 bluetooth functions. Is that right? If so, it > seems more clear to me to just use LoadLibrary/GetProcAddress rather than > dealing with this exception stuff which most people don't have any experience > with. By 3 bluetooth functions, do you mean BluetoothGetRadioInfo, BluetoothFindFirstRadio, and BluetoothFindRadioClose? Then the answer is no. In the later CLs we will add more bluetooth functions to implement the rest of Bluetooth adapter functionalities. Honestly I am not familiar with this function; it is added by @grt at https://chromiumcodereview.appspot.com/11299332. Greg, do you think it is okay to use LoadLibrary/GetProcAddress for this? https://chromiumcodereview.appspot.com/11411130/diff/46001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:89: } On 2013/01/09 20:06:04, brettw wrote: > Should have a " // namespace" comment Done. https://chromiumcodereview.appspot.com/11411130/diff/46001/device/bluetooth/b... File device/bluetooth/bluetooth_task_manager_win.h (right): https://chromiumcodereview.appspot.com/11411130/diff/46001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.h:8: #include <string> On 2013/01/09 20:06:04, brettw wrote: > Blank line after this. Done.
https://chromiumcodereview.appspot.com/11411130/diff/46001/device/bluetooth/b... File device/bluetooth/bluetooth_task_manager_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/46001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:24: #pragma comment(lib, "delayimp.lib") On 2013/01/09 22:20:14, youngki wrote: > On 2013/01/09 20:06:04, brettw wrote: > > Carlos seemed to be of the opinion that this is not necessary. Does it still > > work without this line? > > Done. Is this not needed for __HrLoadAllImportsForDll? https://chromiumcodereview.appspot.com/11411130/diff/46001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:55: } __except(FilterVisualCPPExceptions(::GetExceptionCode())) { On 2013/01/09 22:20:14, youngki wrote: > On 2013/01/09 20:06:04, brettw wrote: > > It looks like you only need 3 bluetooth functions. Is that right? If so, it > > seems more clear to me to just use LoadLibrary/GetProcAddress rather than > > dealing with this exception stuff which most people don't have any experience > > with. > > By 3 bluetooth functions, do you mean BluetoothGetRadioInfo, > BluetoothFindFirstRadio, and BluetoothFindRadioClose? Then the answer is no. In > the later CLs we will add more bluetooth functions to implement the rest of > Bluetooth adapter functionalities. > > Honestly I am not familiar with this function; it is added by @grt at > https://chromiumcodereview.appspot.com/11299332. > > Greg, do you think it is okay to use LoadLibrary/GetProcAddress for this? If you'll eventually be using the full BT API, I think you'll end up with a lot of boilerplate (typedefs, casts, a function table somewhere, etc) if you try to switch from delayloading the dll to using LoadLibrary/GetProcAddress for all functions. Another approach, if __HrLoadAllImportsForDll is considered too ugly/obscure/etc, would be to keep using delayload as now, but replace _HrLoadAllImportsForDll with a single LoadLibrary/GetProcAddress pair just to test the waters, so to speak. If you succeed in looking up even one BT function that way (you don't need to ever call it), then all of the other calls (which go through the delayload machinery) should succeed. A part of me likes the fact that the current approach resolves all imports in one shot at a well-known place in the code so that you don't take a resolution hit on the first call of each function. https://chromiumcodereview.appspot.com/11411130/diff/54001/device/bluetooth/b... File device/bluetooth/bluetooth_task_manager_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/54001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:23: #include "base/win/scoped_handle.h" https://chromiumcodereview.appspot.com/11411130/diff/54001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:29: const char* kBluetoothThreadName = "BluetoothPollingThreadWin"; const char kBluetoothThreadName[] = "..."; https://chromiumcodereview.appspot.com/11411130/diff/54001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:30: const BLUETOOTH_FIND_RADIO_PARAMS adapter_param = since this is a constant, how about kEmptyAdapterParam or somesuch? actually, what's the benefit of this constant rather than making an instance on the stack in PollAdapter? https://chromiumcodereview.appspot.com/11411130/diff/54001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:65: void GetAdapterState(const HANDLE adapter_handle, nit: no const https://chromiumcodereview.appspot.com/11411130/diff/54001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:97: : ui_task_runner_(MessageLoop::current()->message_loop_proxy()), i was under the impression that using ThreadTaskRunnerHandle::Get() to get the current thread's SingleThreadTaskRunner is the new hotness (and that MLP was deprecated). https://chromiumcodereview.appspot.com/11411130/diff/54001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:110: CHECK(observer); why not DCHECK here and on line 115? https://chromiumcodereview.appspot.com/11411130/diff/54001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:150: HBLUETOOTH_RADIO_FIND adapter_handle = NULL; this has the wrong type and is being leaked. you should use a base::win::ScopedHandle like so: base::win::ScopedHandle adapter_handler; https://chromiumcodereview.appspot.com/11411130/diff/54001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:152: &adapter_handle); and change &adapter_handle to adapter_handle.Receive()
If you're going to be adding a bunch more bluetooth function calls, I think the current delayload + exception approach makes the most sense. I was thinking the ~3 you had now would be all when I suggested changing.
https://chromiumcodereview.appspot.com/11411130/diff/46001/device/bluetooth/b... File device/bluetooth/bluetooth_task_manager_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/46001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:24: #pragma comment(lib, "delayimp.lib") On 2013/01/10 04:01:47, grt wrote: > On 2013/01/09 22:20:14, youngki wrote: > > On 2013/01/09 20:06:04, brettw wrote: > > > Carlos seemed to be of the opinion that this is not necessary. Does it still > > > work without this line? > > > > Done. > > Is this not needed for __HrLoadAllImportsForDll? At least this compiles without delayimp.lib and the bluetooth functions are detecting bluetooth adapters without it... https://chromiumcodereview.appspot.com/11411130/diff/46001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:55: } __except(FilterVisualCPPExceptions(::GetExceptionCode())) { On 2013/01/10 04:01:47, grt wrote: > On 2013/01/09 22:20:14, youngki wrote: > > On 2013/01/09 20:06:04, brettw wrote: > > > It looks like you only need 3 bluetooth functions. Is that right? If so, it > > > seems more clear to me to just use LoadLibrary/GetProcAddress rather than > > > dealing with this exception stuff which most people don't have any > experience > > > with. > > > > By 3 bluetooth functions, do you mean BluetoothGetRadioInfo, > > BluetoothFindFirstRadio, and BluetoothFindRadioClose? Then the answer is no. > In > > the later CLs we will add more bluetooth functions to implement the rest of > > Bluetooth adapter functionalities. > > > > Honestly I am not familiar with this function; it is added by @grt at > > https://chromiumcodereview.appspot.com/11299332. > > > > Greg, do you think it is okay to use LoadLibrary/GetProcAddress for this? > > If you'll eventually be using the full BT API, I think you'll end up with a lot > of boilerplate (typedefs, casts, a function table somewhere, etc) if you try to > switch from delayloading the dll to using LoadLibrary/GetProcAddress for all > functions. > > Another approach, if __HrLoadAllImportsForDll is considered too > ugly/obscure/etc, would be to keep using delayload as now, but replace > _HrLoadAllImportsForDll with a single LoadLibrary/GetProcAddress pair just to > test the waters, so to speak. If you succeed in looking up even one BT function > that way (you don't need to ever call it), then all of the other calls (which go > through the delayload machinery) should succeed. > > A part of me likes the fact that the current approach resolves all imports in > one shot at a well-known place in the code so that you don't take a resolution > hit on the first call of each function. I will leave this as is.
I didn't know Greg had further comments on the code; I just addressed all of them. Please take a look. https://chromiumcodereview.appspot.com/11411130/diff/54001/device/bluetooth/b... File device/bluetooth/bluetooth_task_manager_win.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/54001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:23: On 2013/01/10 04:01:47, grt wrote: > #include "base/win/scoped_handle.h" Done. https://chromiumcodereview.appspot.com/11411130/diff/54001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:29: const char* kBluetoothThreadName = "BluetoothPollingThreadWin"; On 2013/01/10 04:01:47, grt wrote: > const char kBluetoothThreadName[] = "..."; Done. https://chromiumcodereview.appspot.com/11411130/diff/54001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:30: const BLUETOOTH_FIND_RADIO_PARAMS adapter_param = On 2013/01/10 04:01:47, grt wrote: > since this is a constant, how about kEmptyAdapterParam or somesuch? actually, > what's the benefit of this constant rather than making an instance on the stack > in PollAdapter? Moved it into PollAdapter. https://chromiumcodereview.appspot.com/11411130/diff/54001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:65: void GetAdapterState(const HANDLE adapter_handle, On 2013/01/10 04:01:47, grt wrote: > nit: no const Done. https://chromiumcodereview.appspot.com/11411130/diff/54001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:97: : ui_task_runner_(MessageLoop::current()->message_loop_proxy()), On 2013/01/10 04:01:47, grt wrote: > i was under the impression that using ThreadTaskRunnerHandle::Get() to get the > current thread's SingleThreadTaskRunner is the new hotness (and that MLP was > deprecated). Done. If that's the new hotness, I want to be in that group too. :) https://chromiumcodereview.appspot.com/11411130/diff/54001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:110: CHECK(observer); On 2013/01/10 04:01:47, grt wrote: > why not DCHECK here and on line 115? Done. https://chromiumcodereview.appspot.com/11411130/diff/54001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:150: HBLUETOOTH_RADIO_FIND adapter_handle = NULL; On 2013/01/10 04:01:47, grt wrote: > this has the wrong type and is being leaked. you should use a > base::win::ScopedHandle like so: > base::win::ScopedHandle adapter_handler; Done. I never knew about ScopedHandle. Thanks for letting me know..! https://chromiumcodereview.appspot.com/11411130/diff/54001/device/bluetooth/b... device/bluetooth/bluetooth_task_manager_win.cc:152: &adapter_handle); On 2013/01/10 04:01:47, grt wrote: > and change &adapter_handle to adapter_handle.Receive() Done.
lgtm
Thanks Greg..! Brett, let me know if you are okay with this change. I'd like to land this CL hopefully over the weekend.
On 2013/01/09 16:33:12, youngki wrote: > On 2013/01/07 23:56:34, youngki wrote: > > Hey Brett, could you take another look? I implemented BluetoothTaskManager > using > > SequencedWorkerPool and removed BluetoothPollingThreadWin entirely. > > > > I removed content dependency from device, although content/public/test is > still > > being used in unittests. But I think it's okay to use it in unittests. why? i think it would be great to remove any dependency on content at all. content\test exists for modules that depend on content, which src\device shouldn't
Done. I removed content dependency entirely from testing as well by making ui and bluetooth task runners passable from the task manager owners. Please take a look.
Actually I had to put back DEPS and also content dependency to device.gyp because this is used to create device test suite: http://src.chromium.org/viewvc/chrome/trunk/src/device/test/device_test_suite.h , which is not related to this CL. At least I removed all content dependency related to this CL. BluetoothAdapterWin and BluetoothTaskManagerWin do not use content browser thread anymore. I will see if I can remove the content dependency entirely from device in the other CL. Please take a look.
lgtm, with one nit https://chromiumcodereview.appspot.com/11411130/diff/65004/device/bluetooth/b... File device/bluetooth/bluetooth_adapter_win_unittest.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/65004/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win_unittest.cc:17: const char* kAdapterName = "Bluetooth Adapter Name"; Should these not be "const char kAdapterAddress[]"?
On 2013/01/15 15:57:05, youngki wrote: > Actually I had to put back DEPS and also content dependency to device.gyp > because this is used to create device test suite: > http://src.chromium.org/viewvc/chrome/trunk/src/device/test/device_test_suite.h > , which is not related to this CL. > > At least I removed all content dependency related to this CL. > BluetoothAdapterWin and BluetoothTaskManagerWin do not use content browser > thread anymore. I will see if I can remove the content dependency entirely from > device in the other CL. Please take a look. ok, please loop me into that, thanks
LGTM for the general thread approach. I didn't look at the details.
Thanks for the review everyone..! I am putting this into CQ. https://chromiumcodereview.appspot.com/11411130/diff/65004/device/bluetooth/b... File device/bluetooth/bluetooth_adapter_win_unittest.cc (right): https://chromiumcodereview.appspot.com/11411130/diff/65004/device/bluetooth/b... device/bluetooth/bluetooth_adapter_win_unittest.cc:17: const char* kAdapterName = "Bluetooth Adapter Name"; On 2013/01/15 19:34:07, keybuk wrote: > Should these not be "const char kAdapterAddress[]"? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/youngki@chromium.org/11411130/89001
Message was sent while issue was closed.
Change committed as 177024 |
