Code cleaning: Uses scoped_ptr<> to express ownership rather than writing ownership in comments.

Code cleaning: Uses scoped_ptr<> to express ownership rather than writing ownership in comments.

Replaces Response* with scoped_ptr<Response> in dbus code and its related code.

BUG=163231
TEST=no regression / no behavior changes


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181266

[alias of yukishiino, 2013-01-30 06:15:43]

Hi Satoru-san,

Could you review this CL?
Also could you run try-bots because I don't have rights to run try-bots.

Cheers,
Yuki Shiino

[satorux1, 2013-01-30 06:36:57]

Thank you for working on this!

https://codereview.chromium.org/12092061/diff/1/dbus/exported_object.h
File dbus/exported_object.h (right):

https://codereview.chromium.org/12092061/diff/1/dbus/exported_object.h#newcode46
dbus/exported_object.h:46: // not delete |response|.
Please remove the line 44-46. The comment is no longer necessary, as it's now
clear that the callback takes the ownership of |response|.

https://codereview.chromium.org/12092061/diff/1/dbus/message.h
File dbus/message.h (right):

https://codereview.chromium.org/12092061/diff/1/dbus/message.h#newcode204
dbus/message.h:204: // type DBUS_MESSAGE_TYPE_METHOD_RETURN.
Please keep: Takes the ownership of |raw_message|

https://codereview.chromium.org/12092061/diff/1/dbus/message.h#newcode208
dbus/message.h:208: // Used for implementing exported methods.
ditto.

https://codereview.chromium.org/12092061/diff/1/dbus/message.h#newcode229
dbus/message.h:229: static scoped_ptr<ErrorResponse> FromRawMessage(DBusMessage*
raw_message);
ditto

https://codereview.chromium.org/12092061/diff/1/dbus/mock_object_proxy.h
File dbus/mock_object_proxy.h (right):

https://codereview.chromium.org/12092061/diff/1/dbus/mock_object_proxy.h#newc...
dbus/mock_object_proxy.h:24: MOCK_METHOD2(MockCallMethodAndBlock,
Response*(MethodCall* method_call,
I guess this indirection is needed because CallMethodAndBlock() returns a
scoped_ptr<>. Please add some comment about it.

[alias of yukishiino, 2013-01-30 13:13:44]

The CL was missing unittests, and I've added them.
Could you take another look and kick try-bots again?

Cheers,
Yuki Shiino

https://codereview.chromium.org/12092061/diff/1/dbus/exported_object.h
File dbus/exported_object.h (right):

https://codereview.chromium.org/12092061/diff/1/dbus/exported_object.h#newcode46
dbus/exported_object.h:46: // not delete |response|.
On 2013/01/30 06:36:58, satorux1 wrote:
> Please remove the line 44-46. The comment is no longer necessary, as it's now
> clear that the callback takes the ownership of |response|.

Done.

https://codereview.chromium.org/12092061/diff/1/dbus/message.h
File dbus/message.h (right):

https://codereview.chromium.org/12092061/diff/1/dbus/message.h#newcode204
dbus/message.h:204: // type DBUS_MESSAGE_TYPE_METHOD_RETURN.
On 2013/01/30 06:36:58, satorux1 wrote:
> Please keep: Takes the ownership of |raw_message|

Done.

https://codereview.chromium.org/12092061/diff/1/dbus/message.h#newcode208
dbus/message.h:208: // Used for implementing exported methods.
On 2013/01/30 06:36:58, satorux1 wrote:
> ditto.

My understanding is that FromMethodCall does NOT take the ownership of
|method_call|.  So I wrote it in the comment.

https://codereview.chromium.org/12092061/diff/1/dbus/message.h#newcode229
dbus/message.h:229: static scoped_ptr<ErrorResponse> FromRawMessage(DBusMessage*
raw_message);
On 2013/01/30 06:36:58, satorux1 wrote:
> ditto

Done.

https://codereview.chromium.org/12092061/diff/1/dbus/mock_object_proxy.h
File dbus/mock_object_proxy.h (right):

https://codereview.chromium.org/12092061/diff/1/dbus/mock_object_proxy.h#newc...
dbus/mock_object_proxy.h:24: MOCK_METHOD2(MockCallMethodAndBlock,
Response*(MethodCall* method_call,
On 2013/01/30 06:36:58, satorux1 wrote:
> I guess this indirection is needed because CallMethodAndBlock() returns a
> scoped_ptr<>. Please add some comment about it.

Done.

[satorux1, 2013-01-31 01:32:54]

LGTM

https://codereview.chromium.org/12092061/diff/1/dbus/message.h
File dbus/message.h (right):

https://codereview.chromium.org/12092061/diff/1/dbus/message.h#newcode208
dbus/message.h:208: // Used for implementing exported methods.
On 2013/01/30 13:13:44, Yuki wrote:
> On 2013/01/30 06:36:58, satorux1 wrote:
> > ditto.
> 
> My understanding is that FromMethodCall does NOT take the ownership of
> |method_call|.  So I wrote it in the comment.

You are right. Thanks.

[commit-bot: I haz the power, 2013-01-31 13:21:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yuki@chromium.org/12092061/2037

[commit-bot: I haz the power, 2013-01-31 13:21:40]

Presubmit check for 12092061-2037 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/browser/geolocation

Presubmit checks took 2.0s to calculate.

[alias of yukishiino, 2013-01-31 13:23:59]

Hi Steven,

Could you review this CL as an owner of content/browser/geolocation?

[stevenjb, 2013-01-31 18:08:48]

lgtm

[commit-bot: I haz the power, 2013-02-01 02:34:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yuki@chromium.org/12092061/2037

[commit-bot: I haz the power, 2013-02-01 02:34:16]

Failed to apply patch for chromeos/dbus/ibus/ibus_engine_service.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chromeos/dbus/ibus/ibus_engine_service.cc
  Hunk #5 FAILED at 284.
  Hunk #6 succeeded at 300 (offset -1 lines).
  Hunk #7 succeeded at 316 (offset -1 lines).
  Hunk #8 succeeded at 333 (offset -1 lines).
  Hunk #9 succeeded at 341 (offset -1 lines).
  Hunk #10 succeeded at 372 (offset -1 lines).
  Hunk #11 succeeded at 414 (offset -1 lines).
  Hunk #12 succeeded at 444 (offset -1 lines).
  1 out of 12 hunks FAILED -- saving rejects to file
chromeos/dbus/ibus/ibus_engine_service.cc.rej

Patch:       chromeos/dbus/ibus/ibus_engine_service.cc
Index: chromeos/dbus/ibus/ibus_engine_service.cc
diff --git a/chromeos/dbus/ibus/ibus_engine_service.cc
b/chromeos/dbus/ibus/ibus_engine_service.cc
index
58051263cf2b3f46649c6f5386c5af46e3ed3070..546c2646ee820a02261b6ac98b2804e7fb917ee3
100644
--- a/chromeos/dbus/ibus/ibus_engine_service.cc
+++ b/chromeos/dbus/ibus/ibus_engine_service.cc
@@ -232,8 +232,7 @@ class IBusEngineServiceImpl : public IBusEngineService {
     if (engine_handler_ == NULL)
       return;
     engine_handler_->FocusIn();
-    dbus::Response* response = dbus::Response::FromMethodCall(method_call);
-    response_sender.Run(response);
+    response_sender.Run(dbus::Response::FromMethodCall(method_call));
   }
 
   // Handles FocusOut method call from ibus-daemon.
@@ -242,8 +241,7 @@ class IBusEngineServiceImpl : public IBusEngineService {
     if (engine_handler_ == NULL)
       return;
     engine_handler_->FocusOut();
-    dbus::Response* response = dbus::Response::FromMethodCall(method_call);
-    response_sender.Run(response);
+    response_sender.Run(dbus::Response::FromMethodCall(method_call));
   }
 
   // Handles Enable method call from ibus-daemon.
@@ -252,8 +250,7 @@ class IBusEngineServiceImpl : public IBusEngineService {
     if (engine_handler_ == NULL)
       return;
     engine_handler_->Enable();
-    dbus::Response* response = dbus::Response::FromMethodCall(method_call);
-    response_sender.Run(response);
+    response_sender.Run(dbus::Response::FromMethodCall(method_call));
   }
 
   // Handles Disable method call from ibus-daemon.
@@ -262,8 +259,7 @@ class IBusEngineServiceImpl : public IBusEngineService {
     if (engine_handler_ == NULL)
       return;
     engine_handler_->Disable();
-    dbus::Response* response = dbus::Response::FromMethodCall(method_call);
-    response_sender.Run(response);
+    response_sender.Run(dbus::Response::FromMethodCall(method_call));
   }
 
   // Handles PropertyActivate method call from ibus-daemon.
@@ -288,8 +284,7 @@ class IBusEngineServiceImpl : public IBusEngineService {
         property_name,
         static_cast<IBusEngineHandlerInterface::IBusPropertyState>(
             property_state));
-    dbus::Response* response = dbus::Response::FromMethodCall(method_call);
-    response_sender.Run(response);
+    response_sender.Run(dbus::Response::FromMethodCall(method_call));
   }
 
   // Handles PropertyShow method call from ibus-daemon.
@@ -305,8 +300,7 @@ class IBusEngineServiceImpl : public IBusEngineService {
       return;
     }
     engine_handler_->PropertyShow(property_name);
-    dbus::Response* response = dbus::Response::FromMethodCall(method_call);
-    response_sender.Run(response);
+    response_sender.Run(dbus::Response::FromMethodCall(method_call));
   }
 
   // Handles PropertyHide method call from ibus-daemon.
@@ -322,8 +316,7 @@ class IBusEngineServiceImpl : public IBusEngineService {
       return;
     }
     engine_handler_->PropertyHide(property_name);
-    dbus::Response* response = dbus::Response::FromMethodCall(method_call);
-    response_sender.Run(response);
+    response_sender.Run(dbus::Response::FromMethodCall(method_call));
   }
 
   // Handles SetCapability method call from ibus-daemon.
@@ -340,8 +333,7 @@ class IBusEngineServiceImpl : public IBusEngineService {
     }
     engine_handler_->SetCapability(
         static_cast<IBusEngineHandlerInterface::IBusCapability>(capability));
-    dbus::Response* response = dbus::Response::FromMethodCall(method_call);
-    response_sender.Run(response);
+    response_sender.Run(dbus::Response::FromMethodCall(method_call));
   }
 
   void Reset(dbus::MethodCall* method_call,
@@ -349,8 +341,7 @@ class IBusEngineServiceImpl : public IBusEngineService {
     if (engine_handler_ == NULL)
       return;
     engine_handler_->Reset();
-    dbus::Response* response = dbus::Response::FromMethodCall(method_call);
-    response_sender.Run(response);
+    response_sender.Run(dbus::Response::FromMethodCall(method_call));
   }
 
   // Handles ProcessKeyEvent method call from ibus-daemon.
@@ -381,19 +372,18 @@ class IBusEngineServiceImpl : public IBusEngineService {
         keysym, keycode, state,
         base::Bind(&IBusEngineServiceImpl::KeyEventDone,
                    weak_ptr_factory_.GetWeakPtr(),
-                   base::Unretained(
-                       dbus::Response::FromMethodCall(method_call)),
+                   base::Passed(dbus::Response::FromMethodCall(method_call)),
                    response_sender));
   }
 
-  void KeyEventDone(dbus::Response* response,
+  void KeyEventDone(scoped_ptr<dbus::Response> response,
                     const dbus::ExportedObject::ResponseSender&
response_sender,
                     bool consume) {
     if (engine_handler_ == NULL)
       return;
-    dbus::MessageWriter writer(response);
+    dbus::MessageWriter writer(response.get());
     writer.AppendBool(consume);
-    response_sender.Run(response);
+    response_sender.Run(response.Pass());
   }
 
   // Handles CandidateClicked method call from ibus-daemon.
@@ -424,8 +414,7 @@ class IBusEngineServiceImpl : public IBusEngineService {
         index,
         static_cast<ibus::IBusMouseButton>(button),
         state);
-    dbus::Response* response = dbus::Response::FromMethodCall(method_call);
-    response_sender.Run(response);
+    response_sender.Run(dbus::Response::FromMethodCall(method_call));
   }
 
   // Handles SetSurroundingText method call from ibus-daemon.
@@ -455,8 +444,7 @@ class IBusEngineServiceImpl : public IBusEngineService {
     }
 
     engine_handler_->SetSurroundingText(text, cursor_pos, anchor_pos);
-    dbus::Response* response = dbus::Response::FromMethodCall(method_call);
-    response_sender.Run(response);
+    response_sender.Run(dbus::Response::FromMethodCall(method_call));
   }
 
   // Called when the method call is exported.

[commit-bot: I haz the power, 2013-02-01 05:35:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yuki@chromium.org/12092061/15005

[commit-bot: I haz the power, 2013-02-01 05:35:11]

Presubmit check for 12092061-15005 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/browser/geolocation

Presubmit checks took 3.1s to calculate.

[alias of yukishiino, 2013-02-01 05:47:25]

Hi John,

Could you review this CL as an owner of content/browser/geolocation/?

This CL simply replaces dbus::Response* with scoped_ptr<dbus::Response> so the
ownership of an object will be clear.

Cheers,
Yuki Shiino

[John Knottenbelt, 2013-02-01 11:48:32]

LGTM. Nice refactoring!

https://codereview.chromium.org/12092061/diff/15005/content/browser/geolocati...
File content/browser/geolocation/wifi_data_provider_linux_unittest.cc (right):

https://codereview.chromium.org/12092061/diff/15005/content/browser/geolocati...
content/browser/geolocation/wifi_data_provider_linux_unittest.cc:117:
dbus::Response* CreateNetowrkManagerProxyResponse(
Why do you not return a scoped_ptr<dbus::Response> here? Maybe some trouble with
gtest?

optional: if it is easy to do, it would be great to fix the typo in
CreateNetowrkManagerProxyResponse function name.

[alias of yukishiino, 2013-02-01 13:49:36]

Thanks for the review.

I've fixed the typo, and will submit the CL soon.

Cheers,
Yuki Shiino

https://codereview.chromium.org/12092061/diff/15005/content/browser/geolocati...
File content/browser/geolocation/wifi_data_provider_linux_unittest.cc (right):

https://codereview.chromium.org/12092061/diff/15005/content/browser/geolocati...
content/browser/geolocation/wifi_data_provider_linux_unittest.cc:117:
dbus::Response* CreateNetowrkManagerProxyResponse(
On 2013/02/01 11:48:32, John Knottenbelt wrote:
> Why do you not return a scoped_ptr<dbus::Response> here? Maybe some trouble
with
> gtest?

Yes.  scoped_ptr<> is not copyable, and GMock doesn't work for uncopyable
parameters and/or return type.
(I think it's because GMock internally copies expected values to hold them for
later use.)

You might be interested in this discussion.
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/01sDxsJ1OYw...

> optional: if it is easy to do, it would be great to fix the typo in
> CreateNetowrkManagerProxyResponse function name.

Done.

[commit-bot: I haz the power, 2013-02-01 13:50:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yuki@chromium.org/12092061/14004

[commit-bot: I haz the power, 2013-02-01 18:24:21]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[alias of yukishiino, 2013-02-02 18:14:19]

Hi Mike,

Could you review this CL as an owner of chrome/browser/password_manager/?

This CL replaces dbus::Response* with scoped_ptr<dbus::Response> as refactoring.

Cheers,
Yuki Shiino

[Mike Mammarella, 2013-02-05 02:27:48]

password_manager/native_backend_kwallet_x_unittest.cc LGTM

[commit-bot: I haz the power, 2013-02-06 06:19:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yuki@chromium.org/12092061/38001

[commit-bot: I haz the power, 2013-02-06 07:14:40]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-02-07 07:12:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yuki@chromium.org/12092061/38001

[commit-bot: I haz the power, 2013-02-07 09:46:25]

Change committed as 181266