Submission for C++ Readability

device/bluetooth/bluetooth_adapter_chromeos.cc

 // Copyright 2013 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 "device/bluetooth/bluetooth_adapter_chromeos.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/location.h"
@@ skipping 43 matching lines @@
     return;
 
   LOG(WARNING) << "Failed to unregister pairing agent: "
                << error_name << ": " << error_message;
 }
 
 }  // namespace
 
 namespace device {
 
 // static

[omoikane, 2015/05/22 23:55:27]

These "// static" comments are often a maintenance burden and do not add much
value, and should be deleted.

[Marie Janssen, 2015/06/18 21:08:35]

This "// static" is convention in Chromium code.

 base::WeakPtr<BluetoothAdapter> BluetoothAdapter::CreateAdapter(
     const InitCallback& init_callback) {
   return chromeos::BluetoothAdapterChromeOS::CreateAdapter();
 }
 
 }

[omoikane, 2015/05/22 23:55:27]

End of namespace here should have "// namespace device" comments.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespace_For...

Comments are not required for short namespaces, but it seems inconsistent that
it's present for the relatively short namespace ending at line 60 and not here.

[Marie Janssen, 2015/06/18 21:08:35]

Done.

 
 namespace chromeos {
 
 // static
 base::WeakPtr<BluetoothAdapter> BluetoothAdapterChromeOS::CreateAdapter() {
   BluetoothAdapterChromeOS* adapter = new BluetoothAdapterChromeOS();
   return adapter->weak_ptr_factory_.GetWeakPtr();
 }
 
 void BluetoothAdapterChromeOS::Shutdown() {
   if (dbus_is_shutdown_)
     return;
   DCHECK(DBusThreadManager::IsInitialized())
       << "Call BluetoothAdapterFactory::Shutdown() before "
          "DBusThreadManager::Shutdown().";
 
   if (IsPresent())
     RemoveAdapter();  // Also deletes devices_.
   DCHECK(devices_.empty());
-  // profiles_ should be empty because all BluetoothSockets have been signaled
+  // profiles_ is empty because all BluetoothSockets have been notified
   // that this adapter is disappearing.
   DCHECK(profiles_.empty());
 
   for (auto& it : profile_queues_)
     delete it.second;
   profile_queues_.clear();
 
   DBusThreadManager::Get()->GetBluetoothAdapterClient()->RemoveObserver(this);
   DBusThreadManager::Get()->GetBluetoothDeviceClient()->RemoveObserver(this);
   DBusThreadManager::Get()->GetBluetoothInputClient()->RemoveObserver(this);
@@ skipping 216 matching lines @@
     scoped_ptr<device::BluetoothAdvertisement::Data> advertisement_data,
     const CreateAdvertisementCallback& callback,
     const CreateAdvertisementErrorCallback& error_callback) {
   scoped_refptr<BluetoothAdvertisementChromeOS> advertisement(
       new BluetoothAdvertisementChromeOS(advertisement_data.Pass(), this));
   advertisement->Register(base::Bind(callback, advertisement), error_callback);
 }
 
 void BluetoothAdapterChromeOS::RemovePairingDelegateInternal(
     BluetoothDevice::PairingDelegate* pairing_delegate) {
-  // Before removing a pairing delegate make sure that there aren't any devices
-  // currently using it; if there are, clear the pairing context which will
-  // make any responses no-ops.
+  // Check if any device is using the pairing delegate.
+  // If so, clear the pairing context which will make any responses no-ops.
   for (DevicesMap::iterator iter = devices_.begin();
        iter != devices_.end(); ++iter) {
     BluetoothDeviceChromeOS* device_chromeos =
         static_cast<BluetoothDeviceChromeOS*>(iter->second);
 
     BluetoothPairingChromeOS* pairing = device_chromeos->GetPairing();
     if (pairing && pairing->GetPairingDelegate() == pairing_delegate)
       device_chromeos->EndPairing();
   }
 }
@@ skipping 16 matching lines @@
     const std::string& property_name) {
   if (object_path != object_path_)
     return;
   DCHECK(IsPresent());
 
   BluetoothAdapterClient::Properties* properties =
       DBusThreadManager::Get()->GetBluetoothAdapterClient()->
           GetProperties(object_path_);
 
   if (property_name == properties->powered.name())
     PoweredChanged(properties->powered.value());

[omoikane, 2015/05/22 23:55:27]

These conditional blocks should be enclosed in braces.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals

(please fix everywhere)

[Marie Janssen, 2015/06/18 21:08:35]

Done.

   else if (property_name == properties->discoverable.name())
     DiscoverableChanged(properties->discoverable.value());
   else if (property_name == properties->discovering.name())
     DiscoveringChanged(properties->discovering.value());
 }
 
 void BluetoothAdapterChromeOS::DeviceAdded(
   const dbus::ObjectPath& object_path) {
   DCHECK(DBusThreadManager::Get());
   BluetoothDeviceClient::Properties* properties =
@@ skipping 45 matching lines @@
           GetProperties(object_path);
 
   if (property_name == properties->bluetooth_class.name() ||
       property_name == properties->address.name() ||
       property_name == properties->alias.name() ||
       property_name == properties->paired.name() ||
       property_name == properties->trusted.name() ||
       property_name == properties->connected.name() ||
       property_name == properties->uuids.name() ||
       property_name == properties->rssi.name())
     NotifyDeviceChanged(device_chromeos);

[omoikane, 2015/05/22 23:55:27]

Body of this conditional should be enclosed in braces because the conditional
expression already spanned multiple lines.

(please fix everywhere)

[Marie Janssen, 2015/06/18 21:08:35]

Done.

 
   // When a device becomes paired, mark it as trusted so that the user does
   // not need to approve every incoming connection
   if (property_name == properties->paired.name() &&
       properties->paired.value() && !properties->trusted.value())
     device_chromeos->SetTrusted();
 
   // UMA connection counting
   if (property_name == properties->connected.name()) {
     // PlayStation joystick tries to reconnect after disconnection from USB.
@@ skipping 363 matching lines @@
   FOR_EACH_OBSERVER(BluetoothAdapter::Observer, observers_,
                     AdapterDiscoverableChanged(this, discoverable));
 }
 
 void BluetoothAdapterChromeOS::DiscoveringChanged(
     bool discovering) {
   // If the adapter stopped discovery due to a reason other than a request by
   // us, reset the count to 0.
   VLOG(1) << "Discovering changed: " << discovering;
   if (!discovering && !discovery_request_pending_
       && num_discovery_sessions_ > 0) {

[omoikane, 2015/05/22 23:55:27]

"&&" usually appears at the end of the previous line instead of the beginning of
the line.  Even among files in this changelist, this is the only place that has
&& at the beginning of the line.

[Marie Janssen, 2015/06/18 21:08:35]

Done.

     VLOG(1) << "Marking sessions as inactive.";
     num_discovery_sessions_ = 0;
     MarkDiscoverySessionsAsInactive();
   }
   FOR_EACH_OBSERVER(BluetoothAdapter::Observer, observers_,
                     AdapterDiscoveringChanged(this, discovering));
 }
 
 void BluetoothAdapterChromeOS::PresentChanged(bool present) {
   FOR_EACH_OBSERVER(BluetoothAdapter::Observer, observers_,
@@ skipping 297 matching lines @@
         BluetoothDiscoveryFilter::Transport::TRANSPORT_DUAL));
     df->CopyFrom(*discovery_filter);
     SetDiscoveryFilter(
         df.Pass(),
         base::Bind(&BluetoothAdapterChromeOS::OnPreSetDiscoveryFilter,
                    weak_ptr_factory_.GetWeakPtr(), callback, error_callback),
         base::Bind(&BluetoothAdapterChromeOS::OnPreSetDiscoveryFilterError,
                    weak_ptr_factory_.GetWeakPtr(), callback, error_callback));
     return;
   } else
     current_filter_.reset();

[omoikane, 2015/05/22 23:55:27]

This conditional block should be enclosed in braces.

Note that cpplint will catch stuff like this.  cpplint is less accurate for
chromium code, but it still emits useful warnings, and should be used as a first
order check.

[Marie Janssen, 2015/06/18 21:08:35]

Done.

 
   // This is the first request to start device discovery.
   discovery_request_pending_ = true;
   DBusThreadManager::Get()->GetBluetoothAdapterClient()->StartDiscovery(
       object_path_,
       base::Bind(&BluetoothAdapterChromeOS::OnStartDiscovery,
                  weak_ptr_factory_.GetWeakPtr(), callback, error_callback),
       base::Bind(&BluetoothAdapterChromeOS::OnStartDiscoveryError,
                  weak_ptr_factory_.GetWeakPtr(), callback, error_callback));
 }
@@ skipping 268 matching lines @@
 
     // If the queued request resulted in a pending call, then let it
     // asynchonously process the remaining queued requests once the pending
     // call returns.
     if (discovery_request_pending_)
       return;
   }
 }
 
 }  // namespace chromeos