Submission for C++ Readability

device/bluetooth/bluetooth_adapter_chromeos.h

 // 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.
 
 #ifndef DEVICE_BLUETOOTH_BLUETOOTH_ADAPTER_CHROMEOS_H_
 #define DEVICE_BLUETOOTH_BLUETOOTH_ADAPTER_CHROMEOS_H_
 
 #include <map>
 #include <queue>
 #include <string>
@@ skipping 39 matching lines @@
 // causes IsPresent to return false just before the dbus system is shutdown but
 // while references to the BluetoothAdapterChromeOS object still exists.
 //
 // When adding methods to this class verify shutdown behavior in
 // BluetoothChromeOSTest, Shutdown.
 class DEVICE_BLUETOOTH_EXPORT BluetoothAdapterChromeOS
     : public device::BluetoothAdapter,
       public chromeos::BluetoothAdapterClient::Observer,
       public chromeos::BluetoothDeviceClient::Observer,
       public chromeos::BluetoothInputClient::Observer,
       public chromeos::BluetoothAgentServiceProvider::Delegate {

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

Multiple inheritance is generally not allowed.  When they are allowed, the base
classes are usually pure interfaces and are named with Interface suffix.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Multiple_Inhe...

This might not be something that you can fix in this readability review so I
won't hold up this review for this change, but perhaps you can raise the issue
with your team to see if they can rename the base classes accordingly.

(2 more files like this)

  public:
   typedef base::Callback<void(const std::string& error_message)>
       ErrorCompletionCallback;
   typedef base::Callback<void(BluetoothAdapterProfileChromeOS* profile)>
       ProfileRegisteredCallback;
 
   static base::WeakPtr<BluetoothAdapter> CreateAdapter();
 
   // BluetoothAdapter:
   void Shutdown() override;
@@ skipping 31 matching lines @@
   void RegisterAdvertisement(
       scoped_ptr<device::BluetoothAdvertisement::Data> advertisement_data,
       const CreateAdvertisementCallback& callback,
       const CreateAdvertisementErrorCallback& error_callback) override;
 
   // Locates the device object by object path (the devices map and
   // BluetoothDevice methods are by address).
   BluetoothDeviceChromeOS* GetDeviceWithPath(
       const dbus::ObjectPath& object_path);
 
-  // Announce to observers a change in device state that is not reflected by
+  // Announces to observers a change in device state that is not reflected by
   // its D-Bus properties.

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

All function parameters should be documented with comments.  In particular, for
pointer parameters like "device" here, be sure to say who owns the pointer and
whether they can be NULL.

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

Done.

   void NotifyDeviceChanged(BluetoothDeviceChromeOS* device);
 
   // The following methods are used to send various GATT observer events to
   // observers.
   void NotifyGattServiceAdded(BluetoothRemoteGattServiceChromeOS* service);
   void NotifyGattServiceRemoved(BluetoothRemoteGattServiceChromeOS* service);
   void NotifyGattServiceChanged(BluetoothRemoteGattServiceChromeOS* service);
   void NotifyGattDiscoveryComplete(BluetoothRemoteGattServiceChromeOS* service);
   void NotifyGattCharacteristicAdded(
       BluetoothRemoteGattCharacteristicChromeOS* characteristic);
@@ skipping 16 matching lines @@
   // Request a profile on the adapter for a custom service with a
   // specific UUID for the device at |device_path| to be sent to |delegate|.
   // If |device_path| is the empty string, incoming connections will be
   // assigned to |delegate|.  When the profile is
   // successfully registered, |success_callback| will be called with a pointer
   // to the profile which is managed by BluetoothAdapterChromeOS.  On failure,
   // |error_callback| will be called.
   void UseProfile(const device::BluetoothUUID& uuid,
                   const dbus::ObjectPath& device_path,
                   const BluetoothProfileManagerClient::Options& options,
                   BluetoothProfileServiceProvider::Delegate* delegate,

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

"delegate" looks like an output parameter because it's not const, and output
parameter are generally declared last in the function parameter list.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Para...

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

|delegate| is not an output parameter - it's an input, and cannot be const
because incoming connection ownership is passed from the
BluetoothAdapterProfileChromeOS (which acts as a
BluetoothProfileServiceProvider::Delegate) to the specific Delegate passed here.
 This is explained in the function description.

                   const ProfileRegisteredCallback& success_callback,
                   const ErrorCompletionCallback& error_callback);
 
   // Release use of a profile by a device.
   void ReleaseProfile(const dbus::ObjectPath& device_path,
                       BluetoothAdapterProfileChromeOS* profile);
 
  protected:
   // BluetoothAdapter:
   void RemovePairingDelegateInternal(
@@ skipping 216 matching lines @@
   // Note: This should remain the last member so it'll be destroyed and
   // invalidate its weak pointers before any other members are destroyed.
   base::WeakPtrFactory<BluetoothAdapterChromeOS> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(BluetoothAdapterChromeOS);
 };
 
 }  // namespace chromeos
 
 #endif  // DEVICE_BLUETOOTH_BLUETOOTH_ADAPTER_CHROMEOS_H_