arc: bluetooth: Add/Remove BT observer only when ARC is ready

components/arc/bluetooth/arc_bluetooth_bridge.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/arc/bluetooth/arc_bluetooth_bridge.h"
 
 #include <bluetooth/bluetooth.h>
 #include <fcntl.h>
 #include <stddef.h>
 #include <sys/socket.h>
@@ skipping 161 matching lines @@
 bool IsGattOffsetValid(int offset) {
   return 0 <= offset && offset < kMaxGattAttributeLength;
 }
 
 }  // namespace
 
 namespace arc {
 
 ArcBluetoothBridge::ArcBluetoothBridge(ArcBridgeService* bridge_service)
     : ArcService(bridge_service), binding_(this), weak_factory_(this) {
   if (BluetoothAdapterFactory::IsBluetoothAdapterAvailable()) {

[rkc, 2016/08/08 21:58:29]

This is a useless check. This only checks if Bluetooth is available on this
'platform'. We won't be running ARC++ ever on a platform that doesn't support
Bluetooth (currently only IOS?).

[puthik_chromium, 2016/08/08 22:33:32]

It's out of scope of this CL so I will leave it here.

-    VLOG(1) << "registering bluetooth adapter";
+    VLOG(1) << "Registering bluetooth adapter.";
     BluetoothAdapterFactory::GetAdapter(base::Bind(
         &ArcBluetoothBridge::OnAdapterInitialized, weak_factory_.GetWeakPtr()));
   } else {
-    VLOG(1) << "no bluetooth adapter available";
+    VLOG(1) << "No bluetooth adapter available.";
   }
-
   arc_bridge_service()->bluetooth()->AddObserver(this);
 }
 
 ArcBluetoothBridge::~ArcBluetoothBridge() {
   DCHECK(CalledOnValidThread());
 
   arc_bridge_service()->bluetooth()->RemoveObserver(this);
 
   if (bluetooth_adapter_)
     bluetooth_adapter_->RemoveObserver(this);
 }
 
 void ArcBluetoothBridge::OnAdapterInitialized(
     scoped_refptr<BluetoothAdapter> adapter) {
   DCHECK(CalledOnValidThread());
 
   // We can downcast here because we are always running on Chrome OS, and
   // so our adapter uses BlueZ.
   bluetooth_adapter_ =
       static_cast<bluez::BluetoothAdapterBlueZ*>(adapter.get());
-  bluetooth_adapter_->AddObserver(this);
+
+  // Arc instance is ready before Bluetooth adapter.
+  // We should register ourselves to bluetooth observer here.
+  if (!bluetooth_observer_flag_ &&

[Luis Héctor Chávez, 2016/08/08 21:54:16]

s/bluetooth_observer_flag_/bluetooth_adapter_->HasObserver(this)/

across all the file.

[puthik_chromium, 2016/08/08 22:33:32]

There is no HasObserver(this) for bluetooth_adapter_.

[rkc, 2016/08/08 22:36:32]

Yes, it is trivial to add it though.

[puthik_chromium, 2016/08/08 22:46:51]

Done.

+      arc_bridge_service()->bluetooth()->instance()) {
+    bluetooth_observer_flag_ = true;
+    bluetooth_adapter_->AddObserver(this);
+  }
 }
 
 void ArcBluetoothBridge::OnInstanceReady() {
   mojom::BluetoothInstance* bluetooth_instance =
       arc_bridge_service()->bluetooth()->instance();
   if (!bluetooth_instance) {
     LOG(ERROR) << "OnBluetoothInstanceReady called, "
                << "but no bluetooth instance found";
     return;
   }
+
   bluetooth_instance->Init(binding_.CreateInterfacePtrAndBind());
+
+  // Bluetooth adapter is ready before Arc instance.
+  // We should register ourselves to bluetooth observer here.

[rkc, 2016/08/08 21:58:29]

This comment isn't clear. Instead, it should read something like,

The Bluetooth adapter was ready before the ARC instance, hence we didn't
register ourselves as an observer with it then. Since our instance is ready, we
should register it now.

[puthik_chromium, 2016/08/08 22:33:32]

Done.

+  if (!bluetooth_observer_flag_ && bluetooth_adapter_) {
+    bluetooth_observer_flag_ = true;
+    bluetooth_adapter_->AddObserver(this);
+  }
+}
+
+void ArcBluetoothBridge::OnInstanceClosed() {
+  if (bluetooth_observer_flag_ && bluetooth_adapter_)
+    bluetooth_adapter_->RemoveObserver(this);
+  bluetooth_observer_flag_ = false;
 }
 
 void ArcBluetoothBridge::AdapterPoweredChanged(BluetoothAdapter* adapter,
                                                bool powered) {
   if (!HasBluetoothInstance())
     return;
 
   // TODO(smbarber): Invoke EnableAdapter or DisableAdapter via ARC bridge
   // service.
 }
@@ skipping 1408 matching lines @@
   LOG(WARNING) << "Bluetooth instance is too old (version " << version
                << ") need version " << version_need;
   return false;
 }
 
 bool ArcBluetoothBridge::CalledOnValidThread() {
   return thread_checker_.CalledOnValidThread();
 }
 
 }  // namespace arc