Bluetooth extension api fixes for WebUI

Bluetooth extension api fixes for WebUI

This fixes a couple of issues in order to work with WebUI (which does
not have an associated extension and may not attach have an observer
which would normally instintiate an adapter).

BUG=543294
Committed: https://crrev.com/43d826bb5fa1d218b6f56da410137b387df970ee
Cr-Commit-Position: refs/heads/master@{#355499}

[stevenjb, 2015-10-15 22:13:25]

stevenjb@chromium.org changed reviewers:
+ armansito@chromium.org, keybuk@chromium.org

[stevenjb, 2015-10-15 22:13:25]

https://codereview.chromium.org/1412493002/diff/1/extensions/browser/api/blue...
File extensions/browser/api/bluetooth/bluetooth_event_router.cc (left):

https://codereview.chromium.org/1412493002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:194: self_callback));
This could potentially generate an infinite loop if the adapter was unable to be
created. Now it will just fail in that case.

https://codereview.chromium.org/1412493002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:269:
MaybeReleaseAdapter();
This was destroying the adapter before DispatchAdapterStateEvent() (which sends
the adapter state) was getting called. If there is an active observer this
wouldn't be a problem, but with Settings there may not be.

[armansito, 2015-10-21 23:19:19]

https://codereview.chromium.org/1412493002/diff/40001/extensions/browser/api/...
File extensions/browser/api/bluetooth/bluetooth_event_router.cc (left):

https://codereview.chromium.org/1412493002/diff/40001/extensions/browser/api/...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:81: if (adapter !=
adapter_.get()) {
Since you removed this check, then maybe add a DHECK(adapter == adapter_.get())
here?

https://codereview.chromium.org/1412493002/diff/40001/extensions/browser/api/...
File extensions/browser/api/bluetooth/bluetooth_event_router.cc (right):

https://codereview.chromium.org/1412493002/diff/40001/extensions/browser/api/...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:98: if
(!adapter_.get() || adapter != adapter_.get()) {
You removed the "adapter != adapter_.get()" above but kept it here. Let's be
consistent.

https://codereview.chromium.org/1412493002/diff/40001/extensions/browser/api/...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:100: LOG(ERROR) <<
"Unable to get addapter";
s/addapter/adapter/

https://codereview.chromium.org/1412493002/diff/40001/extensions/browser/api/...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:102: LOG(ERROR) <<
"Adapter mismatch";
Also this logic is extremely convoluted for a mostly uninformative log message.
Just simplify it all entirely like this:

if (!adapter_.get()) {
  LOG(ERROR) << "|adapter_| was not initialized".
  error_callback.Run();
  return;
}

if (adapter != adapter_.get()) {
  LOG(ERROR) << "Adapters don't match";
  error_callback.Run();
  return;
}

Again for the second part, I'm not sure if it's better to just have
DCHECK(adapter == adapter_.get());

[stevenjb, 2015-10-21 23:33:27]

Patchset #4 (id:60001) has been deleted

[stevenjb, 2015-10-21 23:36:40]

Thanks for the reviews! PTAL

https://codereview.chromium.org/1412493002/diff/40001/extensions/browser/api/...
File extensions/browser/api/bluetooth/bluetooth_event_router.cc (left):

https://codereview.chromium.org/1412493002/diff/40001/extensions/browser/api/...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:81: if (adapter !=
adapter_.get()) {
On 2015/10/21 23:19:19, armansito wrote:
> Since you removed this check, then maybe add a DHECK(adapter ==
adapter_.get())
> here?

Actually, the test is effectively moved to StartDiscoverySessionImpl below.

https://codereview.chromium.org/1412493002/diff/40001/extensions/browser/api/...
File extensions/browser/api/bluetooth/bluetooth_event_router.cc (right):

https://codereview.chromium.org/1412493002/diff/40001/extensions/browser/api/...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:98: if
(!adapter_.get() || adapter != adapter_.get()) {
On 2015/10/21 23:19:19, armansito wrote:
> You removed the "adapter != adapter_.get()" above but kept it here. Let's be
> consistent.

This is called only from StartDiscoverySession() (thus
StartDiscoverySessionImpl()). I added a clarifying comment.

https://codereview.chromium.org/1412493002/diff/40001/extensions/browser/api/...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:100: LOG(ERROR) <<
"Unable to get addapter";
On 2015/10/21 23:19:19, armansito wrote:
> s/addapter/adapter/

Done.

https://codereview.chromium.org/1412493002/diff/40001/extensions/browser/api/...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:102: LOG(ERROR) <<
"Adapter mismatch";
On 2015/10/21 23:19:19, armansito wrote:
> Also this logic is extremely convoluted for a mostly uninformative log
message.
> Just simplify it all entirely like this:
> 
> if (!adapter_.get()) {
>   LOG(ERROR) << "|adapter_| was not initialized".
>   error_callback.Run();
>   return;
> }
> 
> if (adapter != adapter_.get()) {
>   LOG(ERROR) << "Adapters don't match";
>   error_callback.Run();
>   return;
> }
> 
> Again for the second part, I'm not sure if it's better to just have
> DCHECK(adapter == adapter_.get());

Logging clarified.

[armansito, 2015-10-21 23:39:40]

lgtm, thanks!

[stevenjb, 2015-10-22 05:34:38]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2015-10-22 05:34:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412493002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412493002/80001

[commit-bot: I haz the power, 2015-10-22 06:31:16]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-10-22 06:31:55]

Patchset 4 (id:??) landed as
https://crrev.com/43d826bb5fa1d218b6f56da410137b387df970ee
Cr-Commit-Position: refs/heads/master@{#355499}