Submission for C++ Readability

chromeos/dbus/fake_bluetooth_profile_service_provider.h

 // Copyright (c) 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 CHROMEOS_DBUS_FAKE_BLUETOOTH_PROFILE_SERVICE_PROVIDER_H_
 #define CHROMEOS_DBUS_FAKE_BLUETOOTH_PROFILE_SERVICE_PROVIDER_H_
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/memory/scoped_ptr.h"
 #include "chromeos/chromeos_export.h"
 #include "chromeos/dbus/bluetooth_profile_service_provider.h"
 #include "dbus/file_descriptor.h"
 #include "dbus/object_path.h"
 
 namespace chromeos {
 
 // FakeBluetoothProfileServiceProvider simulates the behavior of a local
 // Bluetooth agent object and is used both in test cases in place of a
 // mock and on the Linux desktop.

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

Please also include some comments to document thread safety expectations for
this class.

(please do this for all files)

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

In general, classes in the device tree in Chromium do not worry about
thread-safety - they are only called from the UI thread.  I've added comments to
clarify this.

 class CHROMEOS_EXPORT FakeBluetoothProfileServiceProvider
     : public BluetoothProfileServiceProvider {
  public:
   FakeBluetoothProfileServiceProvider(const dbus::ObjectPath& object_path,
                                       Delegate *delegate);

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

This pointer has the space before '*', while the pointer at line  53 has it
before.  Please pick one format and be consistent.

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

Done.

   ~FakeBluetoothProfileServiceProvider() override;
 
   // Each of these calls the equivalent
   // BluetoothProfileServiceProvider::Delegate method on the object passed on
   // construction.
   virtual void Release();

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

"override" is preferred over "virtual" for inherited functions.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance

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

Done.

   virtual void NewConnection(
       const dbus::ObjectPath& device_path,
       scoped_ptr<dbus::FileDescriptor> fd,
       const Delegate::Options& options,
       const Delegate::ConfirmationCallback& callback);
   virtual void RequestDisconnection(
       const dbus::ObjectPath& device_path,
       const Delegate::ConfirmationCallback& callback);
   virtual void Cancel();
 
   const dbus::ObjectPath& object_path() { return object_path_; }

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

This function does not modify class members, and should be declared const.

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

Done.

 
  private:
   friend class FakeBluetoothProfileManagerClient;

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

Friends are usually declared last in the class, since knowing which other
mock/test code has access is somewhat an implementation detail.  Because most
people read code from top to bottom, it's preferable to move other declarations
near the top before all friend declarations.

(2 more files like this)

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

In Chromium code friends at the beginning is more common, see
nfc_device_client.h, shill_third_party_vpn_client.h, shill_profile_client.h,
etc. 

 
   // D-Bus object path we are faking.
   dbus::ObjectPath object_path_;
 
   // All incoming method calls are passed on to the Delegate and a callback
   // passed to generate the reply. |delegate_| is generally the object that
   // owns this one, and must outlive it.
   Delegate* delegate_;
 };

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

Copy constructors should be disabled.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Copyable_Mova...

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

Done.

 
 }  // namespace chromeos
 
 #endif  // CHROMEOS_DBUS_FAKE_BLUETOOTH_PROFILE_SERVICE_PROVIDER_H_
+