Changed DefaultAdapter to RunCallbackOnAdapterReady function.

Changed DefaultAdapter to RunCallbackOnAdapterReady function.

Instead of returning BluetoothAdapter instance directly, RunCallbackOnAdapterReady will run the callback with the instance of BluetoothAdapter. The idea is that we want to make sure that the BluetoothAdapter is used when it is fully initialized.

Right now the function is still synchronous; the callback will be run immediately after BluetoothAdapter is created within BluetoothAdapterFactory::RunCallbackOnAdapterReady(), so nothing should be broken in BluetoothOptionsHandler and AshSystemTrayDelegate. But later I will eventually change BluetoothAdapterFactory::RunCallbackOnAdapterReady() asynchronous, so we should discuss how we change BluetoothOptionsHandler and AshTryDelegate. Jenny, could you make changes on BluetoothOptionsHandler and AshSystemTrayDelegate to accomodate asynchronous RunCallbackOnAdapterReady() calls? We can also discuss how to make those changes together.

Also is BluetoothAdapterFactory::Create() ever used? Maybe this function will be needed in the future, but currently I don't see this function being called; can we just remove this if it is not used currently and add it back when we actually use it?

I will update unittests soon, but I wanted to send this out so that I can get initial preliminary review first.


BUG=135470
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176523

[youngki, 2013-01-08 21:26:17]

Hi Scott, I just changed DefaultAdapter to receive callback and also changed the
function name. Could you review this change? I will update the unittests soon,
but wanted to get a preliminary review first to make sure I am doing everything
okay.

[bryeung, 2013-01-08 22:24:26]

drive by...

https://chromiumcodereview.appspot.com/11819007/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://chromiumcodereview.appspot.com/11819007/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:275:
base::Unretained(this)));
Remind me why using an Unretained here is safe...?

https://chromiumcodereview.appspot.com/11819007/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:278: bool
InitializeOnAdapterReady(
why does this need to return a value?

https://chromiumcodereview.appspot.com/11819007/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc (right):

https://chromiumcodereview.appspot.com/11819007/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc:46:
device::BluetoothAdapterFactory::RunCallbackOnAdapterReady(callback);
Since this is just a pass-through method now...why does it need to remain part
of the public API?

https://chromiumcodereview.appspot.com/11819007/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/bluetooth/bluetooth_event_router.h (right):

https://chromiumcodereview.appspot.com/11819007/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router.h:27: //
|callback| will not be run if the bluetooth is not supported in the current
This seems like a frustrating API.  Can we have the |callback| be run with NULL
if Bluetooth is not supported on the platform or there is an error getting the
adapter?

https://chromiumcodereview.appspot.com/11819007/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/bluetooth/bluetooth_extension_function.cc
(right):

https://chromiumcodereview.appspot.com/11819007/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth/bluetooth_extension_function.cc:42:
RunCallbackOnAdapterReady(base::Bind(&BluetoothExtensionFunction::DoWork,
why do we try this when is_bluetooth_supported == false?

https://chromiumcodereview.appspot.com/11819007/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_factory.cc (right):

https://chromiumcodereview.appspot.com/11819007/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_factory.cc:35: return is_bluetooth_supported;
any reason not to write this as:

#if defined(OS_CHROMEOS)
  return true;
#endif
  return false;

https://chromiumcodereview.appspot.com/11819007/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_factory.h (right):

https://chromiumcodereview.appspot.com/11819007/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_factory.h:25: // Returns true if the
bluetooth is supported in the current platform.
s/if the bluetooth/if Bluetooth/

[youngki, 2013-01-09 15:12:03]

https://chromiumcodereview.appspot.com/11819007/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://chromiumcodereview.appspot.com/11819007/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:275:
base::Unretained(this)));
On 2013/01/08 22:24:27, bryeung wrote:
> Remind me why using an Unretained here is safe...?

First of all my understanding is that AshSystemTrayDelegate is destroyed only
when the browser shuts down. That means that base::Unretained(this) is safe
unless it is a shutdown time. But during shutdown BluetoothAdapter will also be
destroyed and this callback won't be run then.

https://chromiumcodereview.appspot.com/11819007/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:278: bool
InitializeOnAdapterReady(
On 2013/01/08 22:24:27, bryeung wrote:
> why does this need to return a value?

This is @asargent's suggestion to make BluetoothExtensionFunction::DoWork() to
return bool, not because the return data is meaningful, but so that we don't
have to manually put in the code to duplicate EXTENSION_FUNCTION_VALIDATE macro
in that function. EXTENSION_FUNCTION_VALIDATE returns false, and that requires
the signature of the function to return bool.

https://chromiumcodereview.appspot.com/11819007/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc (right):

https://chromiumcodereview.appspot.com/11819007/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc:46:
device::BluetoothAdapterFactory::RunCallbackOnAdapterReady(callback);
On 2013/01/08 22:24:27, bryeung wrote:
> Since this is just a pass-through method now...why does it need to remain part
> of the public API?

We could.. But then we cannot mock the bluetooth adapter in the unittest..
Without this I couldn't find a suitable way to inject the mock bluetooth adapter
to the unittest.

https://chromiumcodereview.appspot.com/11819007/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/bluetooth/bluetooth_event_router.h (right):

https://chromiumcodereview.appspot.com/11819007/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router.h:27: //
|callback| will not be run if the bluetooth is not supported in the current
On 2013/01/08 22:24:27, bryeung wrote:
> This seems like a frustrating API.  Can we have the |callback| be run with
NULL
> if Bluetooth is not supported on the platform or there is an error getting the
> adapter?

Done.

https://chromiumcodereview.appspot.com/11819007/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/bluetooth/bluetooth_extension_function.cc
(right):

https://chromiumcodereview.appspot.com/11819007/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth/bluetooth_extension_function.cc:42:
RunCallbackOnAdapterReady(base::Bind(&BluetoothExtensionFunction::DoWork,
On 2013/01/08 22:24:27, bryeung wrote:
> why do we try this when is_bluetooth_supported == false?

!! You are right. Done.

https://chromiumcodereview.appspot.com/11819007/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_factory.cc (right):

https://chromiumcodereview.appspot.com/11819007/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_factory.cc:35: return is_bluetooth_supported;
On 2013/01/08 22:24:27, bryeung wrote:
> any reason not to write this as:
> 
> #if defined(OS_CHROMEOS)
>   return true;
> #endif
>   return false;

Done.

https://chromiumcodereview.appspot.com/11819007/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_factory.h (right):

https://chromiumcodereview.appspot.com/11819007/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_factory.h:25: // Returns true if the
bluetooth is supported in the current platform.
On 2013/01/08 22:24:27, bryeung wrote:
> s/if the bluetooth/if Bluetooth/

Done.

[bryeung, 2013-01-09 15:28:01]

lgtm

https://chromiumcodereview.appspot.com/11819007/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://chromiumcodereview.appspot.com/11819007/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:275:
base::Unretained(this)));
On 2013/01/09 15:12:03, youngki wrote:
> On 2013/01/08 22:24:27, bryeung wrote:
> > Remind me why using an Unretained here is safe...?
> 
> First of all my understanding is that AshSystemTrayDelegate is destroyed only
> when the browser shuts down. That means that base::Unretained(this) is safe
> unless it is a shutdown time. But during shutdown BluetoothAdapter will also
be
> destroyed and this callback won't be run then.

And you're sure that:
- this is also true of the other places you've used this pattern?
- this doesn't introduce a race condition where shutdown has started and then
the callback is called

If so, then I agree it's okay.

https://chromiumcodereview.appspot.com/11819007/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:278: bool
InitializeOnAdapterReady(
On 2013/01/09 15:12:03, youngki wrote:
> On 2013/01/08 22:24:27, bryeung wrote:
> > why does this need to return a value?
> 
> This is @asargent's suggestion to make BluetoothExtensionFunction::DoWork() to
> return bool, not because the return data is meaningful, but so that we don't
> have to manually put in the code to duplicate EXTENSION_FUNCTION_VALIDATE
macro
> in that function. EXTENSION_FUNCTION_VALIDATE returns false, and that requires
> the signature of the function to return bool.

Makes sense.  Okay.

https://chromiumcodereview.appspot.com/11819007/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc (right):

https://chromiumcodereview.appspot.com/11819007/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc:46:
device::BluetoothAdapterFactory::RunCallbackOnAdapterReady(callback);
On 2013/01/09 15:12:03, youngki wrote:
> On 2013/01/08 22:24:27, bryeung wrote:
> > Since this is just a pass-through method now...why does it need to remain
part
> > of the public API?
> 
> We could.. But then we cannot mock the bluetooth adapter in the unittest..
> Without this I couldn't find a suitable way to inject the mock bluetooth
adapter
> to the unittest.

Okay.

https://chromiumcodereview.appspot.com/11819007/diff/8001/chrome/browser/chro...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://chromiumcodereview.appspot.com/11819007/diff/8001/chrome/browser/chro...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:279:
scoped_refptr<device::BluetoothAdapter> adapter) {
should CHECK adapter here

https://chromiumcodereview.appspot.com/11819007/diff/8001/chrome/browser/ui/w...
File chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc
(right):

https://chromiumcodereview.appspot.com/11819007/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:204:
DCHECK(adapter_.get());
This should either be a CHECK, or we should let it crash on the next line.

[keybuk, 2013-01-09 17:54:04]

lgtm

[youngki, 2013-01-09 19:32:24]

Unittests are coming soon...

https://chromiumcodereview.appspot.com/11819007/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://chromiumcodereview.appspot.com/11819007/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:275:
base::Unretained(this)));
On 2013/01/09 15:28:01, bryeung wrote:
> On 2013/01/09 15:12:03, youngki wrote:
> > On 2013/01/08 22:24:27, bryeung wrote:
> > > Remind me why using an Unretained here is safe...?
> > 
> > First of all my understanding is that AshSystemTrayDelegate is destroyed
only
> > when the browser shuts down. That means that base::Unretained(this) is safe
> > unless it is a shutdown time. But during shutdown BluetoothAdapter will also
> be
> > destroyed and this callback won't be run then.
> 
> And you're sure that:
> - this is also true of the other places you've used this pattern?
> - this doesn't introduce a race condition where shutdown has started and then
> the callback is called
> 
> If so, then I agree it's okay.

I gave a second thought, and there might be race conditions if BluetoothAdapter
is destroyed later than the other modules during the shutdown. Also
BluetoothExtensionFunction is not live for the entire browser lifetime anyways.

So I changed the code to use weak pointer instead of base::Unretained(this).

https://chromiumcodereview.appspot.com/11819007/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:278: bool
InitializeOnAdapterReady(
On 2013/01/09 15:28:01, bryeung wrote:
> On 2013/01/09 15:12:03, youngki wrote:
> > On 2013/01/08 22:24:27, bryeung wrote:
> > > why does this need to return a value?
> > 
> > This is @asargent's suggestion to make BluetoothExtensionFunction::DoWork()
to
> > return bool, not because the return data is meaningful, but so that we don't
> > have to manually put in the code to duplicate EXTENSION_FUNCTION_VALIDATE
> macro
> > in that function. EXTENSION_FUNCTION_VALIDATE returns false, and that
requires
> > the signature of the function to return bool.
> 
> Makes sense.  Okay.

I actually changed it to void because weak pointer does not allow to bind the
method that returns. Instead, I added a void RunOnAdapterCallback() which simply
calls DoWork() in BluetoothExtensionFunction and used that as a callback
instead.

https://chromiumcodereview.appspot.com/11819007/diff/8001/chrome/browser/chro...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://chromiumcodereview.appspot.com/11819007/diff/8001/chrome/browser/chro...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:279:
scoped_refptr<device::BluetoothAdapter> adapter) {
On 2013/01/09 15:28:02, bryeung wrote:
> should CHECK adapter here

Done.

https://chromiumcodereview.appspot.com/11819007/diff/8001/chrome/browser/ui/w...
File chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc
(right):

https://chromiumcodereview.appspot.com/11819007/diff/8001/chrome/browser/ui/w...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:204:
DCHECK(adapter_.get());
On 2013/01/09 15:28:02, bryeung wrote:
> This should either be a CHECK, or we should let it crash on the next line.

Done.

[youngki, 2013-01-09 22:03:18]

Actually all the tests are passing and there is no change in terms of
functionalities, so I didn't need to update the unittests.

Mike, could you review the extension changes for OWNERS approval?
chrome/browser/extensions/*
chrome/common/extensions/*

Also Nikita, could you review the chromeos changes for OWNERS approval?
chrome/browser/chromeos/*
chrome/browser/ui/webui/options/chromeos/*


Also Scott, it'd be good if you could LGTM once more if you are okay with weak
pointer change I made.

Thanks

[Nikita (slow), 2013-01-10 10:29:54]

https://codereview.chromium.org/11819007/diff/19001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/11819007/diff/19001/chrome/browser/chromeos/s...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:281:
CHECK(bluetooth_adapter_);
You're passing adapter as a weak pointer so NULL could be passed?

https://codereview.chromium.org/11819007/diff/19001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc
(right):

https://codereview.chromium.org/11819007/diff/19001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:204:
CHECK(adapter_);
Same question here.

[youngki, 2013-01-10 18:58:54]

https://codereview.chromium.org/11819007/diff/19001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/11819007/diff/19001/chrome/browser/chromeos/s...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:281:
CHECK(bluetooth_adapter_);
On 2013/01/10 10:29:55, Nikita Kostylev wrote:
> You're passing adapter as a weak pointer so NULL could be passed?

Yes, a weak pointer passed from BluetoothAdapterFactory. And NULL would be
passed if the BluetoothAdapterFactory::RunCallbackOnAdapterReady() is called on
other platforms (i.e. OSX).

https://codereview.chromium.org/11819007/diff/19001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc
(right):

https://codereview.chromium.org/11819007/diff/19001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:204:
CHECK(adapter_);
On 2013/01/10 10:29:55, Nikita Kostylev wrote:
> Same question here.

Here is the same as well.

[Nikita (slow), 2013-01-11 11:15:09]

https://codereview.chromium.org/11819007/diff/19001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/11819007/diff/19001/chrome/browser/chromeos/s...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:281:
CHECK(bluetooth_adapter_);
On 2013/01/10 18:58:54, youngki wrote:
> On 2013/01/10 10:29:55, Nikita Kostylev wrote:
> > You're passing adapter as a weak pointer so NULL could be passed?
> 
> Yes, a weak pointer passed from BluetoothAdapterFactory. And NULL would be
> passed if the BluetoothAdapterFactory::RunCallbackOnAdapterReady() is called
on
> other platforms (i.e. OSX).

So do you actually need a CHECK here or would it be enough to return if
bluetooth_adapter_ hasn't been specified?

[youngki, 2013-01-11 13:18:46]

https://codereview.chromium.org/11819007/diff/19001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/11819007/diff/19001/chrome/browser/chromeos/s...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:281:
CHECK(bluetooth_adapter_);
On 2013/01/11 11:15:09, Nikita Kostylev wrote:
> On 2013/01/10 18:58:54, youngki wrote:
> > On 2013/01/10 10:29:55, Nikita Kostylev wrote:
> > > You're passing adapter as a weak pointer so NULL could be passed?
> > 
> > Yes, a weak pointer passed from BluetoothAdapterFactory. And NULL would be
> > passed if the BluetoothAdapterFactory::RunCallbackOnAdapterReady() is called
> on
> > other platforms (i.e. OSX).
> 
> So do you actually need a CHECK here or would it be enough to return if
> bluetooth_adapter_ hasn't been specified?

I think we need a check here since bluetooth_adapter_ being NULL in this case
should not happen. And many parts in this file depend on the fact that
bluetooth_adapter_ is properly initialized.

[miket_OOO, 2013-01-11 14:52:30]

> Mike, could you review the extension changes for OWNERS approval?
> chrome/browser/extensions/*
> chrome/common/extensions/*

OWNERS LGTM

[Nikita (slow), 2013-01-11 15:34:20]

https://codereview.chromium.org/11819007/diff/19001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/11819007/diff/19001/chrome/browser/chromeos/s...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:281:
CHECK(bluetooth_adapter_);
On 2013/01/11 13:18:47, youngki wrote:
> On 2013/01/11 11:15:09, Nikita Kostylev wrote:
> > On 2013/01/10 18:58:54, youngki wrote:
> > > On 2013/01/10 10:29:55, Nikita Kostylev wrote:
> > > > You're passing adapter as a weak pointer so NULL could be passed?
> > > 
> > > Yes, a weak pointer passed from BluetoothAdapterFactory. And NULL would be
> > > passed if the BluetoothAdapterFactory::RunCallbackOnAdapterReady() is
called
> > on
> > > other platforms (i.e. OSX).
> > 
> > So do you actually need a CHECK here or would it be enough to return if
> > bluetooth_adapter_ hasn't been specified?
> 
> I think we need a check here since bluetooth_adapter_ being NULL in this case
> should not happen. And many parts in this file depend on the fact that
> bluetooth_adapter_ is properly initialized.

I don't want to block this CL but it seems a bit strange that on the one hand
you allow the possibility of passing NULL to this method (because weak pointer
is used) and at the same time you have a strict requirement here (CHECK) that
makes such situations invalid.

[youngki, 2013-01-11 16:47:56]

https://codereview.chromium.org/11819007/diff/19001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/11819007/diff/19001/chrome/browser/chromeos/s...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:281:
CHECK(bluetooth_adapter_);
On 2013/01/11 15:34:21, Nikita Kostylev wrote:
> On 2013/01/11 13:18:47, youngki wrote:
> > On 2013/01/11 11:15:09, Nikita Kostylev wrote:
> > > On 2013/01/10 18:58:54, youngki wrote:
> > > > On 2013/01/10 10:29:55, Nikita Kostylev wrote:
> > > > > You're passing adapter as a weak pointer so NULL could be passed?
> > > > 
> > > > Yes, a weak pointer passed from BluetoothAdapterFactory. And NULL would
be
> > > > passed if the BluetoothAdapterFactory::RunCallbackOnAdapterReady() is
> called
> > > on
> > > > other platforms (i.e. OSX).
> > > 
> > > So do you actually need a CHECK here or would it be enough to return if
> > > bluetooth_adapter_ hasn't been specified?
> > 
> > I think we need a check here since bluetooth_adapter_ being NULL in this
case
> > should not happen. And many parts in this file depend on the fact that
> > bluetooth_adapter_ is properly initialized.
> 
> I don't want to block this CL but it seems a bit strange that on the one hand
> you allow the possibility of passing NULL to this method (because weak pointer
> is used) and at the same time you have a strict requirement here (CHECK) that
> makes such situations invalid.

That's a valid concern; I appreciate you point it out.

I still think we should put CHECK here;
device::BluetoothAdapterFactory::RunCallbackOnAdapterReady() is a
platform-independent call. This passes a bluetooth adapter pointer if the
bluetooth functionalities are supported in the currentplatform, otherwise it
passes NULL. Since ChromeOs is a supported platform, BluetoothAdapterFactory
should always pass a bluetooth adapter pointer to this method. So, although
there is a possibility of passing NULL because of generality of the
BluetoothAdapterFactory, this shouldn't pass NULL in this specific case because
this code only runs on ChromeOs, and we'd like to make that explicit by having
CHECK here.

[Nikita (slow), 2013-01-11 17:53:35]

On 2013/01/11 16:47:56, youngki wrote:
>
https://codereview.chromium.org/11819007/diff/19001/chrome/browser/chromeos/s...
> File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):
> 
>
https://codereview.chromium.org/11819007/diff/19001/chrome/browser/chromeos/s...
> chrome/browser/chromeos/system/ash_system_tray_delegate.cc:281:
> CHECK(bluetooth_adapter_);
> On 2013/01/11 15:34:21, Nikita Kostylev wrote:
> > On 2013/01/11 13:18:47, youngki wrote:
> > > On 2013/01/11 11:15:09, Nikita Kostylev wrote:
> > > > On 2013/01/10 18:58:54, youngki wrote:
> > > > > On 2013/01/10 10:29:55, Nikita Kostylev wrote:
> > > > > > You're passing adapter as a weak pointer so NULL could be passed?
> > > > > 
> > > > > Yes, a weak pointer passed from BluetoothAdapterFactory. And NULL
would
> be
> > > > > passed if the BluetoothAdapterFactory::RunCallbackOnAdapterReady() is
> > called
> > > > on
> > > > > other platforms (i.e. OSX).
> > > > 
> > > > So do you actually need a CHECK here or would it be enough to return if
> > > > bluetooth_adapter_ hasn't been specified?
> > > 
> > > I think we need a check here since bluetooth_adapter_ being NULL in this
> case
> > > should not happen. And many parts in this file depend on the fact that
> > > bluetooth_adapter_ is properly initialized.
> > 
> > I don't want to block this CL but it seems a bit strange that on the one
hand
> > you allow the possibility of passing NULL to this method (because weak
pointer
> > is used) and at the same time you have a strict requirement here (CHECK)
that
> > makes such situations invalid.
> 
> That's a valid concern; I appreciate you point it out.
> 
> I still think we should put CHECK here;
> device::BluetoothAdapterFactory::RunCallbackOnAdapterReady() is a
> platform-independent call. This passes a bluetooth adapter pointer if the
> bluetooth functionalities are supported in the currentplatform, otherwise it
> passes NULL. Since ChromeOs is a supported platform, BluetoothAdapterFactory
> should always pass a bluetooth adapter pointer to this method. So, although
> there is a possibility of passing NULL because of generality of the
> BluetoothAdapterFactory, this shouldn't pass NULL in this specific case
because
> this code only runs on ChromeOs, and we'd like to make that explicit by having
> CHECK here.

The only possible case that I've though of was some kind of shutdown /  shutdown
in tests sequence.

[Nikita (slow), 2013-01-11 17:53:40]

lgtm

[youngki, 2013-01-11 20:23:26]

Thanks for the reviews..!

All the try jobs are passing, so I am putting this into CQ now.

[commit-bot: I haz the power, 2013-01-11 20:27:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/youngki@chromium.org/11819007/28001

[commit-bot: I haz the power, 2013-01-12 09:14:10]

Change committed as 176523