Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1784333002:
Add Device Policy Handler for Bluetooth, and allow disabling the Bluetooth adapter on Chrome OS (Closed)
|
Created:
4 years, 9 months ago by Ivan Šandrk Modified:
4 years, 8 months ago CC:
chromium-reviews, davemoore+watch_chromium.org, ortuno+watch_chromium.org, oshima+watch_chromium.org, scheib+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd Device Policy Handler for Bluetooth, and allow disabling the Bluetooth adapter on Chrome OS
This is cl #2 of 3 to tackle this issue:
#1 boolean policy
#2 policy handler + changes in Bluetooth code
#3 UI part.
(+ cl #4 with the tests)
BUG=463578
Committed: https://crrev.com/bf114686c1e861b93a891f4f3ba32bbf087f23f6
Cr-Commit-Position: refs/heads/master@{#387923}
Patch Set 1 #Patch Set 2 : wired up the code, code cleanup #Patch Set 3 : last upload wasnt good #Patch Set 4 : removed IsDisabled for now, added conditional compilation (cros only) #
Total comments: 2
Patch Set 5 : Made SetDisabled pure virtual #
Total comments: 30
Patch Set 6 : code fix #
Total comments: 14
Patch Set 7 : refactoring, enforce policy run on startup, set policy default value to true, cros only [also rebase] #Patch Set 8 : scoped_ptr to std::unique_ptr, cros-only for mock, minor refactor #Patch Set 9 : SetDisabled cros only in bluetooth_adapter_bluez #Patch Set 10 : Save a reference to BluetoothAdapter #Patch Set 11 : Fix buildbots failure on linux_chromeos #Patch Set 12 : Fix buildbots failure on linux_chromeos, try #2 #Patch Set 13 : Use Shutdown instead of SetDisabled, reverted changes in device/bluetooth/* #
Total comments: 22
Patch Set 14 : Bartfab's nits #
Total comments: 2
Messages
Total messages: 45 (15 generated)
Description was changed from ========== Added a policy handler and changed Bluetooth code to enable deactivation of Bluetooth for managed devices This is cl #2 of 3 to tackle this issue: BUG=463578 ========== to ========== Added a policy handler and changed Bluetooth code to enable deactivation of Bluetooth for managed devices This is cl #2 of 3 to tackle this issue: #1 boolean policy #2 policy handler + changes in Bluetooth code #3 UI part. BUG=463578 ==========
isandrk@chromium.org changed reviewers: + atwilson@chromium.org, cschuet@chromium.org
cschuet@chromium.org: Please review changes in atwilson@chromium.org: Please review changes in
scheib@chromium.org changed reviewers: + scheib@chromium.org
Perhaps this should be a cross platform policy, similar to https://codereview.chromium.org/1706503002/ (which only addresses Web Bluetooth). Why is this currently ChromeOS specific?
On 2016/03/31 16:56:54, scheib wrote: > Perhaps this should be a cross platform policy, similar to > https://codereview.chromium.org/1706503002/ (which only addresses Web > Bluetooth). Why is this currently ChromeOS specific? This must be a device policy and device policies are only supported on CrOS.
scheib, friendly ping
Hello, I haven't seen activity on this patch, or other reviews? Typically an OWNERS review is requested after a general patch is reviewed and or with specific review requests (e.g. review particular directories). Get a policy review. The name of this patch should be descriptive: Add Device Policy Handler for Bluetooth, and enable adapter shutdown on Chrome OS. The concepts should be described somewhat in change list - but more so my earlier question of if this should be a cross platform policy should be answered in code for future readability. Where is testing? It seems the shutdown testing may cover most of this -- but SetDisabled can be revoked later? https://codereview.chromium.org/1784333002/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter.h (right): https://codereview.chromium.org/1784333002/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter.h:265: virtual void SetDisabled(bool disabled); Make pure virtual so that we know this is implemented on any platforms that #if defined it.
Description was changed from ========== Added a policy handler and changed Bluetooth code to enable deactivation of Bluetooth for managed devices This is cl #2 of 3 to tackle this issue: #1 boolean policy #2 policy handler + changes in Bluetooth code #3 UI part. BUG=463578 ========== to ========== Add Device Policy Handler for Bluetooth, and allow disabling the Bluetooth adapter on Chrome OS This is cl #2 of 3 to tackle this issue: #1 boolean policy #2 policy handler + changes in Bluetooth code #3 UI part. BUG=463578 ==========
Description was changed from ========== Add Device Policy Handler for Bluetooth, and allow disabling the Bluetooth adapter on Chrome OS This is cl #2 of 3 to tackle this issue: #1 boolean policy #2 policy handler + changes in Bluetooth code #3 UI part. BUG=463578 ========== to ========== Add Device Policy Handler for Bluetooth, and allow disabling the Bluetooth adapter on Chrome OS This is cl #2 of 3 to tackle this issue: #1 boolean policy #2 policy handler + changes in Bluetooth code #3 UI part. (+ cl #4 with the tests) BUG=463578 ==========
On 2016/04/11 23:27:11, scheib wrote: > Hello, I haven't seen activity on this patch, or other reviews? Typically an > OWNERS review is requested after a general patch is reviewed and or with > specific review requests (e.g. review particular directories). Get a policy > review. Yeah sorry about that, cschuet went through the code together with me so I guess it's mostly fine, I'll ping atwilson for his opinion. I need your review for device/bluetooth/*. > The name of this patch should be descriptive: > Add Device Policy Handler for Bluetooth, and enable adapter shutdown on Chrome > OS. Changed it. > The concepts should be described somewhat in change list - but more so my > earlier question of if this should be a cross platform policy should be > answered in code for future readability. At which point in the code exactly? > Where is testing? It seems the shutdown testing may cover most of this -- but > SetDisabled can be revoked later? Not implemented yet, I plan on writing all the tests in cl #4 (after all the actual code is working). Yes, the idea is that SetDisabled can be revoked. https://codereview.chromium.org/1784333002/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter.h (right): https://codereview.chromium.org/1784333002/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter.h:265: virtual void SetDisabled(bool disabled); On 2016/04/11 23:27:11, scheib wrote: > Make pure virtual so that we know this is implemented on any platforms that #if > defined it. Done.
Now that I've made SetDisabled pure virtual, I'm getting a lot of errors like
the following one:
../../device/bluetooth/test/mock_bluetooth_discovery_session.cc:19:19: error:
allocating an object of abstract class type
'testing::NiceMock<MockBluetoothAdapter>'
new testing::NiceMock<MockBluetoothAdapter>()),
^
../../device/bluetooth/bluetooth_adapter.h:265:16: note: unimplemented pure
virtual method 'SetDisabled' in 'NiceMock'
virtual void SetDisabled(bool disabled) = 0;
I'm assuming I need to add my method to mock_bluetooth_adapter, and to do
something similar to what is done with Shutdown:
mock_bluetooth_adapter.h:
#if defined(OS_CHROMEOS) || defined(OS_LINUX)
void Shutdown() override;
#endif
mock_bluetooth_adapter.cc:
#if defined(OS_CHROMEOS) || defined(OS_LINUX)
void MockBluetoothAdapter::Shutdown() {
}
#endif
Will fix this tomorrow.
On 2016/04/12 17:31:11, Ivan Šandrk wrote: > On 2016/04/11 23:27:11, scheib wrote: > > Hello, I haven't seen activity on this patch, or other reviews? Typically an > > OWNERS review is requested after a general patch is reviewed and or with > > specific review requests (e.g. review particular directories). Get a policy > > review. > Yeah sorry about that, cschuet went through the code together with me so I guess > it's mostly fine, I'll ping atwilson for his opinion. I need your review for > device/bluetooth/*. Chromium review style is to have reviewers comment on codereview.chromium.org - even if they did an in person review. cschuet should comment here on record. > > > The name of this patch should be descriptive: > > Add Device Policy Handler for Bluetooth, and enable adapter shutdown on Chrome > > OS. > Changed it. > > > The concepts should be described somewhat in change list - but more so my > > earlier question of if this should be a cross platform policy should be > > answered in code for future readability. > At which point in the code exactly? I was confused when reading the platform specific code in the cross platform interface, I'll follow up with a line comment pointing to that. > > > Where is testing? It seems the shutdown testing may cover most of this -- but > > SetDisabled can be revoked later? > Not implemented yet, I plan on writing all the tests in cl #4 (after all the > actual code is working). Yes, the idea is that SetDisabled can be revoked. It's good for patches to be split up to help review - but the tests need to be written before landing this patch. I'm not clear yet on how you'll test this. > > https://codereview.chromium.org/1784333002/diff/60001/device/bluetooth/blueto... > File device/bluetooth/bluetooth_adapter.h (right): > > https://codereview.chromium.org/1784333002/diff/60001/device/bluetooth/blueto... > device/bluetooth/bluetooth_adapter.h:265: virtual void SetDisabled(bool > disabled); > On 2016/04/11 23:27:11, scheib wrote: > > Make pure virtual so that we know this is implemented on any platforms that > #if > > defined it. > > Done.
https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter.h (right): https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter.h:263: #if defined(OS_CHROMEOS) Explain here why this is only ChromeOS.
https://codereview.chromium.org/1784333002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/bluetooth_policy_handler.cc (right): https://codereview.chromium.org/1784333002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/bluetooth_policy_handler.cc:17: bluetooth_policy_subscription_ = cros_settings_->AddSettingsObserver( Are we always guaranteed to get an OnBluetoothPolicyChanged() invocation on startup? https://codereview.chromium.org/1784333002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/bluetooth_policy_handler.cc:34: bool allow_bluetooth = false; Should allow_bluetooth really default to false (we don't allow bluetooth if there's no policy set?) That seems incorrect. https://codereview.chromium.org/1784333002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/bluetooth_policy_handler.cc:37: // !allow_bluetooth because we have a switch in logic I'd suggest moving the !allow_bluetooth logic into SetBluetoothPolicy(). So change SetBluetoothPolicy(true) to mean "allow bluetooth". https://codereview.chromium.org/1784333002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/bluetooth_policy_handler.cc:45: bool bt_disabled, scoped_refptr<device::BluetoothAdapter> adapter) { This doesn't reference any member data in BluetoothPolicyHandler, so I recommend making it just a callback function, not a member function on BluetoothPolicyHandler. https://codereview.chromium.org/1784333002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/bluetooth_policy_handler.h (right): https://codereview.chromium.org/1784333002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/bluetooth_policy_handler.h:16: // this policy are communicated to SetBluetoothPolicy method by calling This doesn't actually explain what the class does. The first sentence is great ("observes the device setting |DeviceAllowBluetooth|". But the next sentence should explain what it does after that in terms of functionality - telling the reader that various private member functions are called isn't very useful. Instead, maybe something like "This class observes the device setting |DeviceAllowBluetooth|, and calls BluetoothAdapter::SetDisabled() appropriately based on the value of that setting." - that way the reader knows exactly what this object does: it maps policy values into API calls into BluetoothAdapter. Whether various internal member functions are called should not really be part of the public class documentation. https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter.cc (left): https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter.cc:41: void BluetoothAdapter::Shutdown() { Restore this :) https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter.cc (right): https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter.cc:52: NOTIMPLEMENTED(); Remove this since it's now pure virtual. https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter.h (right): https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter.h:263: #if defined(OS_CHROMEOS) On 2016/04/12 19:20:39, scheib wrote: > Explain here why this is only ChromeOS. Or just make it on all platforms - the API and implementation seems reasonable to have as part of the bluetooth adapter everywhere. https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_bluez.cc (right): https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_bluez.cc:263: SetPowered(false, base::Bind(&base::DoNothing), I am slightly nervous that any operations that are in progress when SetDisabled() is called may continue - maybe this is fine, but would want to make sure that the bluetooth folks are OK with how we're disabling the interface here and that things like active discovery sessions will appropriately shut down. https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_bluez.cc:264: base::Bind(&base::DoNothing)); This line is indented wrong. https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_bluez.cc:345: DCHECK(!dbus_is_shutdown_); Should this (and code below) also be checking if the device is present? Seems like anyone can call CreateXXXXService() regardless of whether there's any bluetooth adapter? Maybe add some DCHECKs() here and below? https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_bluez.h (right): https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_bluez.h:351: // (just disables the interface, doesn't actually shut it down). One question - why did we decide to change the value of IsPresent() rather than asking callers to call IsDisabled() to check whether it's disabled?
https://codereview.chromium.org/1784333002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/bluetooth_policy_handler.cc (right): https://codereview.chromium.org/1784333002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/bluetooth_policy_handler.cc:34: bool allow_bluetooth = false; On 2016/04/13 13:55:10, Andrew T Wilson (Slow) wrote: > Should allow_bluetooth really default to false (we don't allow bluetooth if > there's no policy set?) That seems incorrect. Done. https://codereview.chromium.org/1784333002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/bluetooth_policy_handler.cc:37: // !allow_bluetooth because we have a switch in logic On 2016/04/13 13:55:10, Andrew T Wilson (Slow) wrote: > I'd suggest moving the !allow_bluetooth logic into SetBluetoothPolicy(). So > change SetBluetoothPolicy(true) to mean "allow bluetooth". Done. https://codereview.chromium.org/1784333002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/bluetooth_policy_handler.h (right): https://codereview.chromium.org/1784333002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/bluetooth_policy_handler.h:16: // this policy are communicated to SetBluetoothPolicy method by calling On 2016/04/13 13:55:10, Andrew T Wilson (Slow) wrote: > This doesn't actually explain what the class does. The first sentence is great > ("observes the device setting |DeviceAllowBluetooth|". But the next sentence > should explain what it does after that in terms of functionality - telling the > reader that various private member functions are called isn't very useful. > > Instead, maybe something like "This class observes the device setting > |DeviceAllowBluetooth|, and calls BluetoothAdapter::SetDisabled() appropriately > based on the value of that setting." - that way the reader knows exactly what > this object does: it maps policy values into API calls into BluetoothAdapter. > Whether various internal member functions are called should not really be part > of the public class documentation. Done. https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter.cc (left): https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter.cc:41: void BluetoothAdapter::Shutdown() { On 2016/04/13 13:55:10, Andrew T Wilson (Slow) wrote: > Restore this :) Done. https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter.cc (right): https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter.cc:52: NOTIMPLEMENTED(); On 2016/04/13 13:55:11, Andrew T Wilson (Slow) wrote: > Remove this since it's now pure virtual. Done. https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter.h (right): https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter.h:263: #if defined(OS_CHROMEOS) On 2016/04/13 13:55:11, Andrew T Wilson (Slow) wrote: > On 2016/04/12 19:20:39, scheib wrote: > > Explain here why this is only ChromeOS. > > Or just make it on all platforms - the API and implementation seems reasonable > to have as part of the bluetooth adapter everywhere. Done. https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter.h:263: #if defined(OS_CHROMEOS) On 2016/04/12 19:20:39, scheib wrote: > Explain here why this is only ChromeOS. Made it available on all platforms. https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_bluez.cc (right): https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_bluez.cc:264: base::Bind(&base::DoNothing)); On 2016/04/13 13:55:11, Andrew T Wilson (Slow) wrote: > This line is indented wrong. Done. https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_bluez.h (right): https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_bluez.h:351: // (just disables the interface, doesn't actually shut it down). On 2016/04/13 13:55:11, Andrew T Wilson (Slow) wrote: > One question - why did we decide to change the value of IsPresent() rather than > asking callers to call IsDisabled() to check whether it's disabled? It seemed like a simpler solution since no code changes elsewhere were needed, and it also imposes less on users of the Bluetooth adapter (maybe someone forgets to call IsDisabled together with IsPresent). But it's not the cleanest solution, that's true.
https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter.h (right): https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter.h:263: #if defined(OS_CHROMEOS) On 2016/04/13 17:19:04, Ivan Šandrk wrote: > On 2016/04/12 19:20:39, scheib wrote: > > Explain here why this is only ChromeOS. > > Made it available on all platforms. But not implemented there, thus the try job failures. Also, the scope of this would increase considerably to actually make the other platforms able to be disabled.
https://codereview.chromium.org/1784333002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/bluetooth_policy_handler.cc (right): https://codereview.chromium.org/1784333002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/bluetooth_policy_handler.cc:37: // device::BluetoothAdapterFactory::GetAdapter( Don't leave commented out code.
https://codereview.chromium.org/1784333002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/bluetooth_policy_handler.cc (right): https://codereview.chromium.org/1784333002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/bluetooth_policy_handler.cc:25: void BluetoothPolicyHandler::OnBluetoothPolicyChanged() { You didn't answer atwilson's question: "Are we always guaranteed to get an OnBluetoothPolicyChanged() invocation on startup?" https://codereview.chromium.org/1784333002/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_bluez.cc (right): https://codereview.chromium.org/1784333002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_bluez.cc:156: is_disabled_(false), You never set this to true on non-chromeos platforms. This results in disabling bluetooth for all linux users D: https://codereview.chromium.org/1784333002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_bluez.cc:263: SetPowered(false, base::Bind(&base::DoNothing), Does powering off the adapter remove already discovered devices? It's not clear that users of the adapter will no longer be able to use device they've already discovered. https://codereview.chromium.org/1784333002/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_bluez.h (right): https://codereview.chromium.org/1784333002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_bluez.h:352: bool is_disabled_; I don't think this is the right place to store this. BluetoothAdapter is only alive as long as someone holds a reference to it. If everyone drops their references to it and it gets destroyed no one will be able to use the adapter because the policy will not be set to true again.
isandrk@chromium.org changed reviewers: + bartfab@chromium.org
bartfab@chromium.org: Please review changes in chrome/browser/chromeos/settings/device_settings_provider.cc https://codereview.chromium.org/1784333002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/bluetooth_policy_handler.cc (right): https://codereview.chromium.org/1784333002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/bluetooth_policy_handler.cc:17: bluetooth_policy_subscription_ = cros_settings_->AddSettingsObserver( On 2016/04/13 13:55:10, Andrew T Wilson (Slow) wrote: > Are we always guaranteed to get an OnBluetoothPolicyChanged() invocation on > startup? No guarantee. That's why now OnBluetoothPolicyChanged is called once in the constructor just to make sure it fires on startup. https://codereview.chromium.org/1784333002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/policy/bluetooth_policy_handler.cc:45: bool bt_disabled, scoped_refptr<device::BluetoothAdapter> adapter) { On 2016/04/13 13:55:10, Andrew T Wilson (Slow) wrote: > This doesn't reference any member data in BluetoothPolicyHandler, so I recommend > making it just a callback function, not a member function on > BluetoothPolicyHandler. Done. https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter.h (right): https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter.h:263: #if defined(OS_CHROMEOS) On 2016/04/13 19:45:41, scheib wrote: > On 2016/04/13 17:19:04, Ivan Šandrk wrote: > > On 2016/04/12 19:20:39, scheib wrote: > > > Explain here why this is only ChromeOS. > > > > Made it available on all platforms. > > But not implemented there, thus the try job failures. Also, the scope of this > would increase considerably to actually make the other platforms able to be > disabled. That's a very good point. My apologies for pushing code that wasn't working, I just wanted to see what the trybots had to say, shouldn't have sent it out for reviews. Reverted the change (cros only again), and added a comment explaining why it's cros only. https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_bluez.cc (right): https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_bluez.cc:263: SetPowered(false, base::Bind(&base::DoNothing), On 2016/04/13 13:55:11, Andrew T Wilson (Slow) wrote: > I am slightly nervous that any operations that are in progress when > SetDisabled() is called may continue - maybe this is fine, but would want to > make sure that the bluetooth folks are OK with how we're disabling the interface > here and that things like active discovery sessions will appropriately shut > down. https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... Methods tolerate a shutdown scenario where BluetoothAdapterBlueZ::Shutdown causes IsPresent to return false just before the dbus system is shutdown but while references to the BluetoothAdapterBlueZ object still exists. So I'm assuming it's fine because I also use IsPresent. Would like to hear what the Bluetooth folks have to say :) https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_bluez.cc:345: DCHECK(!dbus_is_shutdown_); On 2016/04/13 13:55:11, Andrew T Wilson (Slow) wrote: > Should this (and code below) also be checking if the device is present? Seems > like anyone can call CreateXXXXService() regardless of whether there's any > bluetooth adapter? Maybe add some DCHECKs() here and below? That's a very good point. Bluetooth guys, what do you think of this? https://codereview.chromium.org/1784333002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/bluetooth_policy_handler.cc (right): https://codereview.chromium.org/1784333002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/bluetooth_policy_handler.cc:25: void BluetoothPolicyHandler::OnBluetoothPolicyChanged() { On 2016/04/13 21:12:21, ortuno wrote: > You didn't answer atwilson's question: "Are we always guaranteed to get an > OnBluetoothPolicyChanged() invocation on startup?" Answered it now. C/P: No guarantee. That's why now OnBluetoothPolicyChanged is called once in the constructor just to make sure it fires on startup. https://codereview.chromium.org/1784333002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/policy/bluetooth_policy_handler.cc:37: // device::BluetoothAdapterFactory::GetAdapter( On 2016/04/13 19:47:44, scheib wrote: > Don't leave commented out code. Once again sorry for the half-baked code, wanted to see what the trybots had to say, shouldn't have sent it out for reviews. https://codereview.chromium.org/1784333002/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_bluez.cc (right): https://codereview.chromium.org/1784333002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_bluez.cc:156: is_disabled_(false), On 2016/04/13 21:12:21, ortuno wrote: > You never set this to true on non-chromeos platforms. This results in disabling > bluetooth for all linux users D: Removed it for non-chromeos platforms. https://codereview.chromium.org/1784333002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_bluez.cc:263: SetPowered(false, base::Bind(&base::DoNothing), On 2016/04/13 21:12:21, ortuno wrote: > Does powering off the adapter remove already discovered devices? It's not clear > that users of the adapter will no longer be able to use device they've already > discovered. https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... Methods tolerate a shutdown scenario where BluetoothAdapterBlueZ::Shutdown causes IsPresent to return false just before the dbus system is shutdown but while references to the BluetoothAdapterBlueZ object still exists. So I'm assuming it's fine because I also use IsPresent. Would like to hear what you think about this :) https://codereview.chromium.org/1784333002/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_bluez.h (right): https://codereview.chromium.org/1784333002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_bluez.h:352: bool is_disabled_; On 2016/04/13 21:12:21, ortuno wrote: > I don't think this is the right place to store this. BluetoothAdapter is only > alive as long as someone holds a reference to it. If everyone drops their > references to it and it gets destroyed no one will be able to use the adapter > because the policy will not be set to true again. In my code, I call SetDisabled through device::BluetoothAdapterFactory::GetAdapter, which fetches the BluetoothAdapter (and creates one if it doesn't exist) so this shouldn't be an issue. But you have raised a valid point, something else goes wrong. If the BluetoothAdapter gets disabled through SetDisabled, then afterwards all the references go away and BluetoothAdapter is destroyed, then someone uses BluetoothAdapter again and a new instance gets created, the new instance won't be disabled. This may not be a problem tho, as many pieces of code hold references to the BluetoothAdapter and some have sufficiently long lifetimes (I have to check this). Maybe the best solution is that I keep a reference to it myself, that way I'm sure? What is your opinion on this?
Patchset #9 (id:160001) has been deleted
Patchset #8 (id:140001) has been deleted
Patchset #7 (id:120001) has been deleted
Patchset #7 (id:180001) has been deleted
Will still need a testing patch. https://codereview.chromium.org/1784333002/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_bluez.cc (right): https://codereview.chromium.org/1784333002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_bluez.cc:263: SetPowered(false, base::Bind(&base::DoNothing), On 2016/04/14 18:08:48, Ivan Šandrk wrote: > On 2016/04/13 21:12:21, ortuno wrote: > > Does powering off the adapter remove already discovered devices? It's not > clear > > that users of the adapter will no longer be able to use device they've already > > discovered. > > https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... > > Methods tolerate a shutdown scenario where BluetoothAdapterBlueZ::Shutdown > causes IsPresent to return false just before the dbus system is shutdown but > while references to the BluetoothAdapterBlueZ object still exists. > > So I'm assuming it's fine because I also use IsPresent. Would like to hear what > you think about this :) But you're not calling ::Shutdown which removes those devices. https://codereview.chromium.org/1784333002/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_bluez.h (right): https://codereview.chromium.org/1784333002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_bluez.h:352: bool is_disabled_; On 2016/04/14 18:08:48, Ivan Šandrk wrote: > On 2016/04/13 21:12:21, ortuno wrote: > > I don't think this is the right place to store this. BluetoothAdapter is only > > alive as long as someone holds a reference to it. If everyone drops their > > references to it and it gets destroyed no one will be able to use the adapter > > because the policy will not be set to true again. > > In my code, I call SetDisabled through > device::BluetoothAdapterFactory::GetAdapter, which fetches the BluetoothAdapter > (and creates one if it doesn't exist) so this shouldn't be an issue. > > But you have raised a valid point, something else goes wrong. If the > BluetoothAdapter gets disabled through SetDisabled, then afterwards all the > references go away and BluetoothAdapter is destroyed, then someone uses > BluetoothAdapter again and a new instance gets created, the new instance won't > be disabled. > > This may not be a problem tho, as many pieces of code hold references to the > BluetoothAdapter and some have sufficiently long lifetimes (I have to check > this). Maybe the best solution is that I keep a reference to it myself, that way > I'm sure? What is your opinion on this? You'll need to hold a reference to keep it disabled.
I'm also wondering what is your opinion on the question atwilson raised here https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... https://codereview.chromium.org/1784333002/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_bluez.cc (right): https://codereview.chromium.org/1784333002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_bluez.cc:263: SetPowered(false, base::Bind(&base::DoNothing), On 2016/04/14 21:41:10, scheib wrote: > On 2016/04/14 18:08:48, Ivan Šandrk wrote: > > On 2016/04/13 21:12:21, ortuno wrote: > > > Does powering off the adapter remove already discovered devices? It's not > > clear > > > that users of the adapter will no longer be able to use device they've > already > > > discovered. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... > > > > Methods tolerate a shutdown scenario where BluetoothAdapterBlueZ::Shutdown > > causes IsPresent to return false just before the dbus system is shutdown but > > while references to the BluetoothAdapterBlueZ object still exists. > > > > So I'm assuming it's fine because I also use IsPresent. Would like to hear > what > > you think about this :) > > But you're not calling ::Shutdown which removes those devices. Should I then copy some of the functionality that Shutdown has? Or that wouldn't work? https://codereview.chromium.org/1784333002/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_bluez.h (right): https://codereview.chromium.org/1784333002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_bluez.h:352: bool is_disabled_; On 2016/04/14 21:41:10, scheib wrote: > On 2016/04/14 18:08:48, Ivan Šandrk wrote: > > On 2016/04/13 21:12:21, ortuno wrote: > > > I don't think this is the right place to store this. BluetoothAdapter is > only > > > alive as long as someone holds a reference to it. If everyone drops their > > > references to it and it gets destroyed no one will be able to use the > adapter > > > because the policy will not be set to true again. > > > > In my code, I call SetDisabled through > > device::BluetoothAdapterFactory::GetAdapter, which fetches the > BluetoothAdapter > > (and creates one if it doesn't exist) so this shouldn't be an issue. > > > > But you have raised a valid point, something else goes wrong. If the > > BluetoothAdapter gets disabled through SetDisabled, then afterwards all the > > references go away and BluetoothAdapter is destroyed, then someone uses > > BluetoothAdapter again and a new instance gets created, the new instance won't > > be disabled. > > > > This may not be a problem tho, as many pieces of code hold references to the > > BluetoothAdapter and some have sufficiently long lifetimes (I have to check > > this). Maybe the best solution is that I keep a reference to it myself, that > way > > I'm sure? What is your opinion on this? > > You'll need to hold a reference to keep it disabled. Done.
https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_bluez.cc (right): https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_bluez.cc:263: SetPowered(false, base::Bind(&base::DoNothing), On 2016/04/14 18:08:47, Ivan Šandrk wrote: > On 2016/04/13 13:55:11, Andrew T Wilson (Slow) wrote: > > I am slightly nervous that any operations that are in progress when > > SetDisabled() is called may continue - maybe this is fine, but would want to > > make sure that the bluetooth folks are OK with how we're disabling the > interface > > here and that things like active discovery sessions will appropriately shut > > down. > > https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... > > Methods tolerate a shutdown scenario where BluetoothAdapterBlueZ::Shutdown > causes IsPresent to return false just before the dbus system is shutdown but > while references to the BluetoothAdapterBlueZ object still exists. > > So I'm assuming it's fine because I also use IsPresent. Would like to hear what > the Bluetooth folks have to say :) At shutdown we believe things won't crash -- handlers should all simply early exit. But, Shutdown was implemented with the assumption we'd never need to bring things back to a working state. Shutdown also clears data structures, where currently SetDisabled does not. I'm tempted to request that the policy simply call 'Shutdown' and require a restart to re-enable. This leaves the implementation much simpler, and would seem to achieve the product goal. In almost all cases the device policy will be set and devices will discover it at start up, and the device policy won't change. The policy isn't expected to change frequently. If it does change, requiring a restart doesn't seem so onerous.
https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_bluez.cc (right): https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_bluez.cc:263: SetPowered(false, base::Bind(&base::DoNothing), On 2016/04/15 17:46:53, scheib wrote: > On 2016/04/14 18:08:47, Ivan Šandrk wrote: > > On 2016/04/13 13:55:11, Andrew T Wilson (Slow) wrote: > > > I am slightly nervous that any operations that are in progress when > > > SetDisabled() is called may continue - maybe this is fine, but would want to > > > make sure that the bluetooth folks are OK with how we're disabling the > > interface > > > here and that things like active discovery sessions will appropriately shut > > > down. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... > > > > Methods tolerate a shutdown scenario where BluetoothAdapterBlueZ::Shutdown > > causes IsPresent to return false just before the dbus system is shutdown but > > while references to the BluetoothAdapterBlueZ object still exists. > > > > So I'm assuming it's fine because I also use IsPresent. Would like to hear > what > > the Bluetooth folks have to say :) > > At shutdown we believe things won't crash -- handlers should all simply early > exit. But, Shutdown was implemented with the assumption we'd never need to bring > things back to a working state. Shutdown also clears data structures, where > currently SetDisabled does not. > > I'm tempted to request that the policy simply call 'Shutdown' and require a > restart to re-enable. This leaves the implementation much simpler, and would > seem to achieve the product goal. In almost all cases the device policy will be > set and devices will discover it at start up, and the device policy won't > change. The policy isn't expected to change frequently. If it does change, > requiring a restart doesn't seem so onerous. Okay thanks for your input. I'll ask my supervisors on Monday and see what they think.
LGTM with nits, assuming you provide tests in a follow-up CL. https://codereview.chromium.org/1784333002/diff/320001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/bluetooth_policy_handler.cc (right): https://codereview.chromium.org/1784333002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/policy/bluetooth_policy_handler.cc:9: #include "chrome/browser/chromeos/settings/cros_settings.h" Nit: Already included by header file. https://codereview.chromium.org/1784333002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/policy/bluetooth_policy_handler.cc:18: // Helper function used in device::BluetoothAdapterFactory::GetAdapter Nit: Comments are nice but I think this method is simple enough to be selfexplanatory. https://codereview.chromium.org/1784333002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/policy/bluetooth_policy_handler.cc:21: scoped_refptr<device::BluetoothAdapter> adapter) { Nit: #include "device/bluetooth/bluetooth_adapter.h" https://codereview.chromium.org/1784333002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/policy/bluetooth_policy_handler.cc:22: // !allow_bluetooth because we have a switch in logic Nit: This is quite obvious. No need for a comment. https://codereview.chromium.org/1784333002/diff/320001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/bluetooth_policy_handler.h (right): https://codereview.chromium.org/1784333002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/policy/bluetooth_policy_handler.h:11: #include "base/memory/ref_counted.h" Nit: Not used. https://codereview.chromium.org/1784333002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/policy/bluetooth_policy_handler.h:14: #include "device/bluetooth/bluetooth_adapter.h" Nit: Not used. https://codereview.chromium.org/1784333002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/policy/bluetooth_policy_handler.h:17: // This class observes the device setting |DeviceAllowBluetooth|, and calls Nit: Add blank line above. https://codereview.chromium.org/1784333002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/policy/bluetooth_policy_handler.h:34: // scoped_refptr<device::BluetoothAdapter> adapter_; Nit: Remove this dead code. https://codereview.chromium.org/1784333002/diff/320001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h (right): https://codereview.chromium.org/1784333002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h:16: #include "chrome/browser/chromeos/policy/bluetooth_policy_handler.h" Nit: Forward-declare |chromeos::BluetoothPolicyHandler| instead and move the include to the implementation file. https://codereview.chromium.org/1784333002/diff/320001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1784333002/diff/320001/components/policy/reso... components/policy/resources/policy_templates.json:8354: 'supported_on': ['chrome_os:51-'], Nit: s/51/52/ https://codereview.chromium.org/1784333002/diff/320001/components/policy/reso... components/policy/resources/policy_templates.json:8357: 'dynamic_refresh': False, Nit: I think we should explicitly document that after enabling Bluetooth, the device must be rebooted.
On 2016/04/15 17:52:28, Ivan Šandrk wrote: > https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... > File device/bluetooth/bluetooth_adapter_bluez.cc (right): > > https://codereview.chromium.org/1784333002/diff/80001/device/bluetooth/blueto... > device/bluetooth/bluetooth_adapter_bluez.cc:263: SetPowered(false, > base::Bind(&base::DoNothing), > On 2016/04/15 17:46:53, scheib wrote: > > On 2016/04/14 18:08:47, Ivan Šandrk wrote: > > > On 2016/04/13 13:55:11, Andrew T Wilson (Slow) wrote: > > > > I am slightly nervous that any operations that are in progress when > > > > SetDisabled() is called may continue - maybe this is fine, but would want > to > > > > make sure that the bluetooth folks are OK with how we're disabling the > > > interface > > > > here and that things like active discovery sessions will appropriately > shut > > > > down. > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... > > > > > > Methods tolerate a shutdown scenario where BluetoothAdapterBlueZ::Shutdown > > > causes IsPresent to return false just before the dbus system is shutdown but > > > while references to the BluetoothAdapterBlueZ object still exists. > > > > > > So I'm assuming it's fine because I also use IsPresent. Would like to hear > > what > > > the Bluetooth folks have to say :) > > > > At shutdown we believe things won't crash -- handlers should all simply early > > exit. But, Shutdown was implemented with the assumption we'd never need to > bring > > things back to a working state. Shutdown also clears data structures, where > > currently SetDisabled does not. > > > > I'm tempted to request that the policy simply call 'Shutdown' and require a > > restart to re-enable. This leaves the implementation much simpler, and would > > seem to achieve the product goal. In almost all cases the device policy will > be > > set and devices will discover it at start up, and the device policy won't > > change. The policy isn't expected to change frequently. If it does change, > > requiring a restart doesn't seem so onerous. > > Okay thanks for your input. I'll ask my supervisors on Monday and see what they > think. By the way, we are not your supervisors. We are your peers.
scheib: Thanks for your help, we decided to just use Shutdown instead. https://codereview.chromium.org/1784333002/diff/320001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/bluetooth_policy_handler.cc (right): https://codereview.chromium.org/1784333002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/policy/bluetooth_policy_handler.cc:9: #include "chrome/browser/chromeos/settings/cros_settings.h" On 2016/04/18 13:43:44, bartfab (slow) wrote: > Nit: Already included by header file. Done. https://codereview.chromium.org/1784333002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/policy/bluetooth_policy_handler.cc:18: // Helper function used in device::BluetoothAdapterFactory::GetAdapter On 2016/04/18 13:43:44, bartfab (slow) wrote: > Nit: Comments are nice but I think this method is simple enough to be > selfexplanatory. Done. https://codereview.chromium.org/1784333002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/policy/bluetooth_policy_handler.cc:21: scoped_refptr<device::BluetoothAdapter> adapter) { On 2016/04/18 13:43:44, bartfab (slow) wrote: > Nit: #include "device/bluetooth/bluetooth_adapter.h" Done. https://codereview.chromium.org/1784333002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/policy/bluetooth_policy_handler.cc:22: // !allow_bluetooth because we have a switch in logic On 2016/04/18 13:43:44, bartfab (slow) wrote: > Nit: This is quite obvious. No need for a comment. Done. https://codereview.chromium.org/1784333002/diff/320001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/bluetooth_policy_handler.h (right): https://codereview.chromium.org/1784333002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/policy/bluetooth_policy_handler.h:11: #include "base/memory/ref_counted.h" On 2016/04/18 13:43:44, bartfab (slow) wrote: > Nit: Not used. Done. https://codereview.chromium.org/1784333002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/policy/bluetooth_policy_handler.h:14: #include "device/bluetooth/bluetooth_adapter.h" On 2016/04/18 13:43:44, bartfab (slow) wrote: > Nit: Not used. Done. https://codereview.chromium.org/1784333002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/policy/bluetooth_policy_handler.h:17: // This class observes the device setting |DeviceAllowBluetooth|, and calls On 2016/04/18 13:43:44, bartfab (slow) wrote: > Nit: Add blank line above. Done. https://codereview.chromium.org/1784333002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/policy/bluetooth_policy_handler.h:34: // scoped_refptr<device::BluetoothAdapter> adapter_; On 2016/04/18 13:43:44, bartfab (slow) wrote: > Nit: Remove this dead code. Done. https://codereview.chromium.org/1784333002/diff/320001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h (right): https://codereview.chromium.org/1784333002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h:16: #include "chrome/browser/chromeos/policy/bluetooth_policy_handler.h" On 2016/04/18 13:43:44, bartfab (slow) wrote: > Nit: Forward-declare |chromeos::BluetoothPolicyHandler| instead and move the > include to the implementation file. Done. https://codereview.chromium.org/1784333002/diff/320001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1784333002/diff/320001/components/policy/reso... components/policy/resources/policy_templates.json:8354: 'supported_on': ['chrome_os:51-'], On 2016/04/18 13:43:44, bartfab (slow) wrote: > Nit: s/51/52/ Done. https://codereview.chromium.org/1784333002/diff/320001/components/policy/reso... components/policy/resources/policy_templates.json:8357: 'dynamic_refresh': False, On 2016/04/18 13:43:44, bartfab (slow) wrote: > Nit: I think we should explicitly document that after enabling Bluetooth, the > device must be rebooted. Done.
lgtm
The CQ bit was checked by isandrk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1784333002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1784333002/340001
Message was sent while issue was closed.
Description was changed from ========== Add Device Policy Handler for Bluetooth, and allow disabling the Bluetooth adapter on Chrome OS This is cl #2 of 3 to tackle this issue: #1 boolean policy #2 policy handler + changes in Bluetooth code #3 UI part. (+ cl #4 with the tests) BUG=463578 ========== to ========== Add Device Policy Handler for Bluetooth, and allow disabling the Bluetooth adapter on Chrome OS This is cl #2 of 3 to tackle this issue: #1 boolean policy #2 policy handler + changes in Bluetooth code #3 UI part. (+ cl #4 with the tests) BUG=463578 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Add Device Policy Handler for Bluetooth, and allow disabling the Bluetooth adapter on Chrome OS This is cl #2 of 3 to tackle this issue: #1 boolean policy #2 policy handler + changes in Bluetooth code #3 UI part. (+ cl #4 with the tests) BUG=463578 ========== to ========== Add Device Policy Handler for Bluetooth, and allow disabling the Bluetooth adapter on Chrome OS This is cl #2 of 3 to tackle this issue: #1 boolean policy #2 policy handler + changes in Bluetooth code #3 UI part. (+ cl #4 with the tests) BUG=463578 Committed: https://crrev.com/bf114686c1e861b93a891f4f3ba32bbf087f23f6 Cr-Commit-Position: refs/heads/master@{#387923} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/bf114686c1e861b93a891f4f3ba32bbf087f23f6 Cr-Commit-Position: refs/heads/master@{#387923}
Message was sent while issue was closed.
not lgtm. https://codereview.chromium.org/1784333002/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/bluetooth_policy_handler.cc (right): https://codereview.chromium.org/1784333002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/policy/bluetooth_policy_handler.cc:21: adapter->Shutdown(); If all references to the adapter are released then a new adapter can be constructed and it will be available. You must hold a reference to the adapter to keep it shut down.
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:340001) has been created in https://codereview.chromium.org/1902473002/ by isandrk@chromium.org. The reason for reverting is: not lgtm.
Message was sent while issue was closed.
Patchset #15 (id:360001) has been deleted
Message was sent while issue was closed.
Patchset #15 (id:380001) has been deleted
Message was sent while issue was closed.
Fix in CL/1900163002. https://codereview.chromium.org/1784333002/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/bluetooth_policy_handler.cc (right): https://codereview.chromium.org/1784333002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/policy/bluetooth_policy_handler.cc:21: adapter->Shutdown(); On 2016/04/18 17:10:57, scheib wrote: > If all references to the adapter are released then a new adapter can be > constructed and it will be available. You must hold a reference to the adapter > to keep it shut down. Done.
Message was sent while issue was closed.
On 2016/04/19 15:53:58, Ivan Šandrk wrote: > Fix in CL/1900163002. > > https://codereview.chromium.org/1784333002/diff/340001/chrome/browser/chromeo... > File chrome/browser/chromeos/policy/bluetooth_policy_handler.cc (right): > > https://codereview.chromium.org/1784333002/diff/340001/chrome/browser/chromeo... > chrome/browser/chromeos/policy/bluetooth_policy_handler.cc:21: > adapter->Shutdown(); > On 2016/04/18 17:10:57, scheib wrote: > > If all references to the adapter are released then a new adapter can be > > constructed and it will be available. You must hold a reference to the adapter > > to keep it shut down. > > Done. Thanks, http://crrev.com/1900163002 looks good. |
