Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12022004:
D-Bus: ObjectProxy remove function for Bus object. (Closed)
|
Created:
7 years, 11 months ago by deymo Modified:
7 years, 10 months ago CC:
chromium-reviews Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionD-Bus: ObjectProxy remove function for Bus object.
This implements a remove function for an ObjectProxy owned by a bus
object. Although the Bus object removes all the objects at shutdown,
this functions permits to reduce the memory usage for objects no
longer needed and reduce the number of dbus matching rules used in
the bus connection.
BUG=chromium:170182
TEST=BusTest.RemoveObjectProxy (run out/Debug/dbus_unittests)
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179400
Patch Set 1 #
Total comments: 1
Patch Set 2 : Calls made from the right thread. #
Total comments: 4
Patch Set 3 : Haruki's suggestions. #
Total comments: 12
Patch Set 4 : satorux's comments implemented. #
Total comments: 6
Patch Set 5 : satorux comments in unittest #
Messages
Total messages: 21 (0 generated)
First step for fixing this bug: Implement the RemoveObjectProxy in the Bus class. satorux, can you review it?
Thank you for working on this! https://codereview.chromium.org/12022004/diff/1/dbus/bus.cc File dbus/bus.cc (right): https://codereview.chromium.org/12022004/diff/1/dbus/bus.cc#newcode255 dbus/bus.cc:255: iter->second.get()->Detach(); You cannot all Detach() here, as Detach() should be called on the D-Bus thread. You can do something like PostTaskToDBusThread( FROM_HERE, base::Bind(&ObjectProxy::Detach, its->second.get())); This will call Detach on the D-Bus thread, but the caller cannot tell when it's done. I think RemoveObjectProxy* should take a callback like: void Bus::RemoveObjectProxy(..., RemoveObjectProxyCallback callback); Then, the caller can tell when the remove is done. We are doing something similar in RequestOwnership(). Looking at the code, OnOwnership() function looks unnecessary: BEFORE: PostTaskToOriginThread(FROM_HERE, base::Bind(&Bus::OnOwnership, this, on_ownership_callback, service_name, success)); AFTER: PostTaskToOriginThread(FROM_HERE, base::Bind(on_ownership_callback, service_name, success)); This should work.
Patch set 2 upload. Implemented the same mechanism than in RequestOwnership to call Detach in the DBus thread. Hope now is right =) Test case modified to handle this two threads scenario.
Thank you for the help! https://codereview.chromium.org/12022004/diff/4001/dbus/bus.cc File dbus/bus.cc (right): https://codereview.chromium.org/12022004/diff/4001/dbus/bus.cc#newcode265 dbus/bus.cc:265: } else { maybe returns in this if clause make it simpler? e.g. if (...) { ... return true; } return false; https://codereview.chromium.org/12022004/diff/4001/dbus/bus_unittest.cc File dbus/bus_unittest.cc (right): https://codereview.chromium.org/12022004/diff/4001/dbus/bus_unittest.cc#newco... dbus/bus_unittest.cc:31: extra line?
Thanks for the comments Haruki, I fixed those in the patch set 3. https://codereview.chromium.org/12022004/diff/4001/dbus/bus.cc File dbus/bus.cc (right): https://codereview.chromium.org/12022004/diff/4001/dbus/bus.cc#newcode265 dbus/bus.cc:265: } else { On 2013/01/23 09:34:05, Haruki Sato wrote: > maybe returns in this if clause make it simpler? > e.g. > if (...) { > ... > return true; > } > return false; Done. https://codereview.chromium.org/12022004/diff/4001/dbus/bus_unittest.cc File dbus/bus_unittest.cc (right): https://codereview.chromium.org/12022004/diff/4001/dbus/bus_unittest.cc#newco... dbus/bus_unittest.cc:31: On 2013/01/23 09:34:05, Haruki Sato wrote: > extra line? Done.
lgtm
Hi, I still need an OWNER review for this CL. satorux/keybuk/stevenjb ? Thanks!
I'll take a look shortly.
https://codereview.chromium.org/12022004/diff/9001/dbus/bus.cc File dbus/bus.cc (right): https://codereview.chromium.org/12022004/diff/9001/dbus/bus.cc#newcode284 dbus/bus.cc:284: options)); I think you don't need OnRemoveObjectProxy. You should be able to do something like: base::Bind(callback, service_name, object_path, options); https://codereview.chromium.org/12022004/diff/9001/dbus/bus.cc#newcode475 dbus/bus.cc:475: success)); While you are at it, could you remove OnOwnership? https://codereview.chromium.org/12022004/diff/9001/dbus/bus.h File dbus/bus.h (right): https://codereview.chromium.org/12022004/diff/9001/dbus/bus.h#newcode203 dbus/bus.h:203: typedef base::Callback<void (const std::string&, bool)> OnOwnershipCallback; For the same reason mentioned below, we don't need to pass the service name. Could you add a TODO like: // TODO(satorux): Remove the service name parameter as the caller of RequestOwnership() knows the service name. https://codereview.chromium.org/12022004/diff/9001/dbus/bus.h#newcode210 dbus/bus.h:210: typedef base::Callback<void (const std::string&, const ObjectPath&, int)> I think these parameters are unnecessary, as the caller of RemoveObjectProxy() knows them. Let's just use base::Closure. https://codereview.chromium.org/12022004/diff/9001/dbus/bus.h#newcode262 dbus/bus.h:262: // never called. Please also mention that |callback| must not be null. https://codereview.chromium.org/12022004/diff/9001/dbus/bus_unittest.cc File dbus/bus_unittest.cc (right): https://codereview.chromium.org/12022004/diff/9001/dbus/bus_unittest.cc#newco... dbus/bus_unittest.cc:114: base::Bind(&OnRemoveObjectProxy))); If you switch to base::Closure, you can use base::DoNothing.
Patch set 4 up! Implemented all the satorux's comments. Local tests passed. https://codereview.chromium.org/12022004/diff/9001/dbus/bus.cc File dbus/bus.cc (right): https://codereview.chromium.org/12022004/diff/9001/dbus/bus.cc#newcode284 dbus/bus.cc:284: options)); On 2013/01/25 01:46:06, satorux1 wrote: > I think you don't need OnRemoveObjectProxy. > > You should be able to do something like: > > base::Bind(callback, service_name, object_path, options); Done. https://codereview.chromium.org/12022004/diff/9001/dbus/bus.cc#newcode475 dbus/bus.cc:475: success)); On 2013/01/25 01:46:06, satorux1 wrote: > While you are at it, could you remove OnOwnership? Done. https://codereview.chromium.org/12022004/diff/9001/dbus/bus.h File dbus/bus.h (right): https://codereview.chromium.org/12022004/diff/9001/dbus/bus.h#newcode203 dbus/bus.h:203: typedef base::Callback<void (const std::string&, bool)> OnOwnershipCallback; On 2013/01/25 01:46:06, satorux1 wrote: > For the same reason mentioned below, we don't need to pass the service name. > Could you add a TODO like: > > // TODO(satorux): Remove the service name parameter as the caller of > RequestOwnership() knows the service name. Done. https://codereview.chromium.org/12022004/diff/9001/dbus/bus.h#newcode210 dbus/bus.h:210: typedef base::Callback<void (const std::string&, const ObjectPath&, int)> On 2013/01/25 01:46:06, satorux1 wrote: > I think these parameters are unnecessary, as the caller of RemoveObjectProxy() > knows them. > > Let's just use base::Closure. Done. https://codereview.chromium.org/12022004/diff/9001/dbus/bus.h#newcode262 dbus/bus.h:262: // never called. On 2013/01/25 01:46:06, satorux1 wrote: > Please also mention that |callback| must not be null. Done. https://codereview.chromium.org/12022004/diff/9001/dbus/bus_unittest.cc File dbus/bus_unittest.cc (right): https://codereview.chromium.org/12022004/diff/9001/dbus/bus_unittest.cc#newco... dbus/bus_unittest.cc:114: base::Bind(&OnRemoveObjectProxy))); On 2013/01/25 01:46:06, satorux1 wrote: > If you switch to base::Closure, you can use base::DoNothing. Done.
LGTM with some suggestions for making comments clearer. Thank you for adding this! https://codereview.chromium.org/12022004/diff/15001/dbus/bus_unittest.cc File dbus/bus_unittest.cc (right): https://codereview.chromium.org/12022004/diff/15001/dbus/bus_unittest.cc#newc... dbus/bus_unittest.cc:119: // object_proxy1 other than destroy it. Maybe add: We keep this object for a comparison at a later time. https://codereview.chromium.org/12022004/diff/15001/dbus/bus_unittest.cc#newc... dbus/bus_unittest.cc:126: // exists thanks to the increased reference. The comment was a bit confusing. Maybe: // This should return a different object because the first object was removed from the bus. https://codereview.chromium.org/12022004/diff/15001/dbus/bus_unittest.cc#newc... dbus/bus_unittest.cc:130: ASSERT_TRUE(object_proxy2); Then add something like // Compare the new object with the first object. The first object still exists thanks to the increased reference.
Patch 5 uploaded with the comments fixed. I'll mark it as commit ready. https://codereview.chromium.org/12022004/diff/15001/dbus/bus_unittest.cc File dbus/bus_unittest.cc (right): https://codereview.chromium.org/12022004/diff/15001/dbus/bus_unittest.cc#newc... dbus/bus_unittest.cc:119: // object_proxy1 other than destroy it. On 2013/01/29 03:58:39, satorux1 wrote: > Maybe add: We keep this object for a comparison at a later time. Done. https://codereview.chromium.org/12022004/diff/15001/dbus/bus_unittest.cc#newc... dbus/bus_unittest.cc:126: // exists thanks to the increased reference. On 2013/01/29 03:58:39, satorux1 wrote: > The comment was a bit confusing. Maybe: > > // This should return a different object because the first object was removed > from the bus. Done but added the fact that the object_proxy1 is still in memory. This is an important fact because without this it will be a flaky test (see http://crrev.com/12039033/ ). https://codereview.chromium.org/12022004/diff/15001/dbus/bus_unittest.cc#newc... dbus/bus_unittest.cc:130: ASSERT_TRUE(object_proxy2); On 2013/01/29 03:58:39, satorux1 wrote: > Then add something like > > // Compare the new object with the first object. The first object still exists > thanks to the increased reference. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/deymo@chromium.org/12022004/18002
Sorry for I got bad news for ya. Compile failed with a clobber build on mac. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number... 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.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/deymo@chromium.org/12022004/18002
Sorry for I got bad news for ya. Compile failed with a clobber build on win. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... 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.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/deymo@chromium.org/12022004/18002
Sorry for I got bad news for ya. Compile failed with a clobber build on win. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... 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.
Ok, now the problem compiling on win trybot should be fixed.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/deymo@chromium.org/12022004/18002
Message was sent while issue was closed.
Change committed as 179400 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
