READABILITY for Keybuk

BUG=none
TEST=dbus_unittests & emerge chromeos-chrome

change-Id: I111b9e60a2c6c35edd9e0ea9f6976928c6c6474b


Review URL: http://codereview.chromium.org/9407019

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@128286 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 36 insertions, 34 deletions

diff --git a/dbus/property.cc b/dbus/property.cc
index 0abe001..092135e 100644
--- a/dbus/property.cc
+++ b/dbus/property.cc
@@ -125,11 +125,8 @@ bool PropertySet::UpdatePropertiesFromReader(MessageReader* reader) {
 
   while (array_reader.HasMoreData()) {
     MessageReader dict_entry_reader(NULL);
-    if (!array_reader.PopDictEntry(&dict_entry_reader))
-      continue;
-
-    if (!UpdatePropertyFromReader(&dict_entry_reader))
-      continue;
+    if (array_reader.PopDictEntry(&dict_entry_reader))
+      UpdatePropertyFromReader(&dict_entry_reader);
   }
 
   return true;
diff --git a/dbus/property.h b/dbus/property.h
index 2d2cc82..b214839 100644
--- a/dbus/property.h
+++ b/dbus/property.h
@@ -136,8 +136,10 @@ class PropertyBase {
 
   // Initializes the |property_set| and property |name| so that method
   // calls may be made from this class. This method is called by
-  // PropertySet::RegisterProperty(), there should be no need to call it
-  // directly.
+  // PropertySet::RegisterProperty() passing |this| for |property_set| so
+  // there should be no need to call it directly. If you do beware that
+  // no ownership or reference to |property_set| is taken so that object
+  // must outlive this one.
   void Init(PropertySet* property_set, const std::string& name);
 
   // Retrieves the name of this property, this may be useful in observers
@@ -150,7 +152,7 @@ class PropertyBase {
   //       // Handle version property changing
   //     }
   //   }
-  const std::string& name() { return name_; }
+  const std::string& name() const { return name_; }
 
   // Method used by PropertySet to retrieve the value from a MessageReader,
   // no knowledge of the contained type is required, this method returns
@@ -162,7 +164,8 @@ class PropertyBase {
   PropertySet* property_set() { return property_set_; }
 
  private:
-  // Pointer to the associated property set.
+  // Pointer to the PropertySet instance that this instance is a member of,
+  // no ownership is taken and |property_set_| must outlive this class.
   PropertySet* property_set_;
 
   // Name of the property.
@@ -188,8 +191,8 @@ class PropertySet {
   // argument specifies the name of the property changed.
   typedef base::Callback<void(const std::string& name)> PropertyChangedCallback;
 
-  // Construct a property set, |object_proxy| specifies the proxy for the
-  // remote object that these properties are for, care should be taken to
+  // Constructs a property set, where |object_proxy| specifies the proxy for
+  // the/ remote object that these properties are for, care should be taken to
   // ensure that this object does not outlive the lifetime of the proxy;
   // |interface| specifies the D-Bus interface of these properties, and
   // |property_changed_callback| specifies the callback for when properties
@@ -207,9 +210,10 @@ class PropertySet {
   // call the PropertyBase::Init method.
   void RegisterProperty(const std::string& name, PropertyBase* property);
 
-  // Call after construction to connect property change notification
-  // signals. Sub-classes may override to use different D-Bus signals.
-  void ConnectSignals();
+  // Connects property change notification signals to the object, generally
+  // called immediately after the object is created and before calls to other
+  // methods. Sub-classes may override to use different D-Bus signals.
+  virtual void ConnectSignals();
 
   // Methods connected by ConnectSignals() and called by dbus:: when
   // a property is changed. Sub-classes may override if the property
@@ -223,17 +227,17 @@ class PropertySet {
   // initial values. Sub-classes may override to use a different D-Bus
   // method, or if the remote object does not support retrieving all
   // properties, either ignore or obtain each property value individually.
-  void GetAll();
+  virtual void GetAll();
   virtual void OnGetAll(Response* response);
 
   // Update properties by reading an array of dictionary entries, each
   // containing a string with the name and a variant with the value, from
-  // |message_reader|. Returns false if message in incorrect format.
+  // |message_reader|. Returns false if message is in incorrect format.
   bool UpdatePropertiesFromReader(MessageReader* reader);
 
   // Updates a single property by reading a string with the name and a
   // variant with the value from |message_reader|. Returns false if message
-  // in incorrect format, or property type doesn't match.
+  // is in incorrect format, or property type doesn't match.
   bool UpdatePropertyFromReader(MessageReader* reader);
 
   // Calls the property changed callback passed to the constructor, used
@@ -243,11 +247,11 @@ class PropertySet {
 
   // Retrieves the object proxy this property set was initialized with,
   // provided for sub-classes overriding methods that make D-Bus calls
-  // and for Property<>.
+  // and for Property<>. Not permitted with const references to this class.
   ObjectProxy* object_proxy() { return object_proxy_; }
 
   // Retrieves the interface of this property set.
-  const std::string& interface() { return interface_; }
+  const std::string& interface() const { return interface_; }
 
  protected:
   // Get a weak pointer to this property set, provided so that sub-classes
@@ -258,7 +262,8 @@ class PropertySet {
   }
 
  private:
-  // Pointer to object proxy for making method calls.
+  // Pointer to object proxy for making method calls, no ownership is taken
+  // so this must outlive this class.
   ObjectProxy* object_proxy_;
 
   // Interface of property, e.g. "org.chromium.ExampleService", this is
@@ -324,9 +329,9 @@ class Property : public PropertyBase {
 
   // Requests an updated value from the remote object incurring a
   // round-trip. |callback| will be called when the new value is available.
-  // This may not be implemented by some interfaces, and may be overriden by
-  // sub-classes if interfaces use different method calls.
-  void Get(GetCallback callback) {
+  // This may not be implemented by some interfaces, and may be overriden
+  // by sub-classes if interfaces use different method calls.
+  virtual void Get(GetCallback callback) {
     MethodCall method_call(kPropertiesInterface, kPropertiesGet);
     MessageWriter writer(&method_call);
     writer.AppendString(property_set()->interface());
@@ -360,9 +365,9 @@ class Property : public PropertyBase {
   // Requests that the remote object change the property value to |value|,
   // |callback| will be called to indicate the success or failure of the
   // request, however the new value may not be available depending on the
-  // remote object. This method may be overriden by sub-classes if
+  // remote object. This method may be overridden by sub-classes if
   // interfaces use different method calls.
-  void Set(const T& value, SetCallback callback) {
+  virtual void Set(const T& value, SetCallback callback) {
     MethodCall method_call(kPropertiesInterface, kPropertiesSet);
     MessageWriter writer(&method_call);
     writer.AppendString(property_set()->interface());
diff --git a/dbus/property_unittest.cc b/dbus/property_unittest.cc
index 71ce533..bd37a53 100644
--- a/dbus/property_unittest.cc
+++ b/dbus/property_unittest.cc
@@ -112,7 +112,7 @@ class PropertyTest : public testing::Test {
     message_loop_.Quit();
   }
 
-  // Wait for the given number of updates.
+  // Waits for the given number of updates.
   void WaitForUpdates(size_t num_updates) {
     while (updated_properties_.size() < num_updates)
       message_loop_.Run();
@@ -123,12 +123,12 @@ class PropertyTest : public testing::Test {
   // Name, Version, Methods, Objects
   static const int kExpectedSignalUpdates = 4;
 
-  // Wait for initial values to be set.
+  // Waits for initial values to be set.
   void WaitForGetAll() {
     WaitForUpdates(kExpectedSignalUpdates);
   }
 
-  // Wait for the callback. |id| is the string bound to the callback when
+  // Waits for the callback. |id| is the string bound to the callback when
   // the method call is made that identifies it and distinguishes from any
   // other; you can set this to whatever you wish.
   void WaitForCallback(const std::string& id) {
@@ -152,8 +152,8 @@ class PropertyTest : public testing::Test {
 TEST_F(PropertyTest, InitialValues) {
   WaitForGetAll();
 
-  EXPECT_EQ(properties_->name.value(), "TestService");
-  EXPECT_EQ(properties_->version.value(), 10);
+  EXPECT_EQ("TestService", properties_->name.value());
+  EXPECT_EQ(10, properties_->version.value());
 
   std::vector<std::string> methods = properties_->methods.value();
   ASSERT_EQ(4U, methods.size());
@@ -177,7 +177,7 @@ TEST_F(PropertyTest, UpdatedValues) {
   WaitForCallback("Name");
   WaitForUpdates(1);
 
-  EXPECT_EQ(properties_->name.value(), "TestService");
+  EXPECT_EQ("TestService", properties_->name.value());
 
   // Update the value of the "Version" property, this value should be changed.
   properties_->version.Get(base::Bind(&PropertyTest::PropertyCallback,
@@ -186,7 +186,7 @@ TEST_F(PropertyTest, UpdatedValues) {
   WaitForCallback("Version");
   WaitForUpdates(1);
 
-  EXPECT_EQ(properties_->version.value(), 20);
+  EXPECT_EQ(20, properties_->version.value());
 
   // Update the value of the "Methods" property, this value should not change
   // and should not grow to contain duplicate entries.
@@ -228,7 +228,7 @@ TEST_F(PropertyTest, Get) {
   // Make sure we got a property update too.
   WaitForUpdates(1);
 
-  EXPECT_EQ(properties_->version.value(), 20);
+  EXPECT_EQ(20, properties_->version.value());
 }
 
 TEST_F(PropertyTest, Set) {
@@ -244,5 +244,5 @@ TEST_F(PropertyTest, Set) {
   // TestService sends a property update.
   WaitForUpdates(1);
 
-  EXPECT_EQ(properties_->name.value(), "NewService");
+  EXPECT_EQ("NewService", properties_->name.value());
 }