Implement Bus and ObjectProxy classes for our D-Bus library.

Implement Bus and ObjectProxy classes for our D-Bus library.

ObjectProxy is used to access remote objects.
ExportedObject is used to export objects to other D-Bus 

BUG=90036
TEST=run unit tests. The code is not yet used in Chrome.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=97204

[satorux1, 2011-07-23 01:14:10]

This patch is still work-in-progress, but I'd appreciate it if you could take a
look and give feedback.

Thanks.

[stevenjb, 2011-07-29 21:54:17]

Some comments on dbus.h/dbus.cpp. Have not looked at the other files yet.

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.cc
File dbus/bus.cc (right):

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.cc#newcode43
dbus/bus.cc:43: if (flags & DBUS_WATCH_READABLE)
else if

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.cc#newcode45
dbus/bus.cc:45: if (flags & DBUS_WATCH_WRITABLE)
else if

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.cc#newcode46
dbus/bus.cc:46: mode = MessageLoopForIO::WATCH_WRITE;
else NOTREACHED()

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.cc#newcode122
dbus/bus.cc:122: DBusError error = {};
nit: don't need to initialize error (and better not to; compiler will warn you
if it doesn't get set anywhere)

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.cc#newcode184
dbus/bus.cc:184: Watch* watch = new Watch(raw_watch);
This confused me for a while re: when Watch gets deleted.
It might be more clear do do something like:
  Watch* watch = new Watch();
  // Set raw_watch data -> watch; watch will be deleted when raw_watch is
removed.
  watch->SetDBusWatch(raw_watch);

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.cc#newcode192
dbus/bus.cc:192: Watch* watch =
reinterpret_cast<Watch*>(dbus_watch_get_data(raw_watch));
dbus_watch_get_data should return a void*, so static_cast<> can be used instead.

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.cc#newcode201
dbus/bus.cc:201: watch->StopWatching();
Is it safe to call this if StartWatching() was never called?

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.cc#newcode212
dbus/bus.cc:212: dbus_bool_t Bus::OnAddWatchStub(DBusWatch* raw_watch, void*
data) {
These could be local functions instead of class statics.

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.h
File dbus/bus.h (right):

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.h#newcode53
dbus/bus.h:53: Options();
For a struct should probably just inline the constructor.

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.h#newcode64
dbus/bus.h:64: Bus(const Options& options);
explicit

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.h#newcode81
dbus/bus.h:81: friend class ObjectProxy;
We should avoid necessitating this friendship if possible.

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.h#newcode92
dbus/bus.h:92: bool Init();
Should probably rename this since Init() is typically called immediately after
construction. Maybe InitializeConnection() or just Connect()?

[satorux1, 2011-08-01 19:56:41]

Steven,

Thank you for the early feedback. Addressed the comments locally. I'm now adding
server-side (service-side) code, as well as the end-to-end test. I'll let you
know once I upload the new patch.

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.cc
File dbus/bus.cc (right):

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.cc#newcode43
dbus/bus.cc:43: if (flags & DBUS_WATCH_READABLE)
On 2011/07/29 21:54:17, Steven Bennetts wrote:
> else if

Done.

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.cc#newcode45
dbus/bus.cc:45: if (flags & DBUS_WATCH_WRITABLE)
On 2011/07/29 21:54:17, Steven Bennetts wrote:
> else if

Done.

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.cc#newcode46
dbus/bus.cc:46: mode = MessageLoopForIO::WATCH_WRITE;
On 2011/07/29 21:54:17, Steven Bennetts wrote:
> else NOTREACHED()

Done.

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.cc#newcode122
dbus/bus.cc:122: DBusError error = {};
On 2011/07/29 21:54:17, Steven Bennetts wrote:
> nit: don't need to initialize error (and better not to; compiler will warn you
> if it doesn't get set anywhere)

Done.

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.cc#newcode184
dbus/bus.cc:184: Watch* watch = new Watch(raw_watch);
On 2011/07/29 21:54:17, Steven Bennetts wrote:
> This confused me for a while re: when Watch gets deleted.
> It might be more clear do do something like:
>   Watch* watch = new Watch();
>   // Set raw_watch data -> watch; watch will be deleted when raw_watch is
> removed.
>   watch->SetDBusWatch(raw_watch);

Good point. I think adding a comment is sufficient, so added a comment about
when it'll be deleted.

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.cc#newcode192
dbus/bus.cc:192: Watch* watch =
reinterpret_cast<Watch*>(dbus_watch_get_data(raw_watch));
On 2011/07/29 21:54:17, Steven Bennetts wrote:
> dbus_watch_get_data should return a void*, so static_cast<> can be used
instead.

You are right. Done.

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.cc#newcode201
dbus/bus.cc:201: watch->StopWatching();
On 2011/07/29 21:54:17, Steven Bennetts wrote:
> Is it safe to call this if StartWatching() was never called?

Good point. Yes it's safe. Added a comment about it.

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.cc#newcode212
dbus/bus.cc:212: dbus_bool_t Bus::OnAddWatchStub(DBusWatch* raw_watch, void*
data) {
On 2011/07/29 21:54:17, Steven Bennetts wrote:
> These could be local functions instead of class statics.

Here, we need to call self->OnAddWatch(), which is private, hence these stub
functions are class statics.

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.h
File dbus/bus.h (right):

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.h#newcode53
dbus/bus.h:53: Options();
On 2011/07/29 21:54:17, Steven Bennetts wrote:
> For a struct should probably just inline the constructor.

That will break the Clang bot. I learned it hard way. :)

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.h#newcode64
dbus/bus.h:64: Bus(const Options& options);
On 2011/07/29 21:54:17, Steven Bennetts wrote:
> explicit

Done.

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.h#newcode81
dbus/bus.h:81: friend class ObjectProxy;
On 2011/07/29 21:54:17, Steven Bennetts wrote:
> We should avoid necessitating this friendship if possible.

Removed them for now.

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.h#newcode92
dbus/bus.h:92: bool Init();
On 2011/07/29 21:54:17, Steven Bennetts wrote:
> Should probably rename this since Init() is typically called immediately after
> construction. Maybe InitializeConnection() or just Connect()?

Good point. Renamed to Connect().

[satorux1, 2011-08-01 20:01:49]

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.h
File dbus/bus.h (right):

http://codereview.chromium.org/7491029/diff/6001/dbus/bus.h#newcode53
dbus/bus.h:53: Options();
On 2011/08/01 19:56:41, satorux1 wrote:
> On 2011/07/29 21:54:17, Steven Bennetts wrote:
> > For a struct should probably just inline the constructor.
> 
> That will break the Clang bot. I learned it hard way. :)

I take it back. This struct is probably ok, as members are all primitive types,
but it'll break if we add something like std::string in the future, so I'd keep
the constructor in .cc file.

[satorux1, 2011-08-03 00:42:26]

Just uploaded a new patch. The patch is still work-in-progress, but both client
and service features are working.

I'll be writing end_to_end_unittest.cc that exercises client and service
features (i.e. calling methods, handling method calls).

dbus_test_client.cc, and dbus_test_service.cc are used for manual testing. I'll
probably get rid of them once I finish end_to_end_unittest.cc.

On 2011/08/01 20:01:49, satorux1 wrote:
> http://codereview.chromium.org/7491029/diff/6001/dbus/bus.h
> File dbus/bus.h (right):
> 
> http://codereview.chromium.org/7491029/diff/6001/dbus/bus.h#newcode53
> dbus/bus.h:53: Options();
> On 2011/08/01 19:56:41, satorux1 wrote:
> > On 2011/07/29 21:54:17, Steven Bennetts wrote:
> > > For a struct should probably just inline the constructor.
> > 
> > That will break the Clang bot. I learned it hard way. :)
> 
> I take it back. This struct is probably ok, as members are all primitive
types,
> but it'll break if we add something like std::string in the future, so I'd
keep
> the constructor in .cc file.

[satorux1, 2011-08-06 01:32:26]

Steven,

The change is now in a good shape (and large). Now end-to-end tests are written
for both async and synchronous method calls. Timeout for asynchronous method
calls are also implemented.

I plan to add some more features like signal sending/receiving, but the change
is already very big, so I'll do it in a separate patch.

Hopefully, this will be the last large change. The following patches will be
much smaller.




On 2011/08/03 00:42:26, satorux1 wrote:
> Just uploaded a new patch. The patch is still work-in-progress, but both
client
> and service features are working.
> 
> I'll be writing end_to_end_unittest.cc that exercises client and service
> features (i.e. calling methods, handling method calls).
> 
> dbus_test_client.cc, and dbus_test_service.cc are used for manual testing.
I'll
> probably get rid of them once I finish end_to_end_unittest.cc.
> 
> On 2011/08/01 20:01:49, satorux1 wrote:
> > http://codereview.chromium.org/7491029/diff/6001/dbus/bus.h
> > File dbus/bus.h (right):
> > 
> > http://codereview.chromium.org/7491029/diff/6001/dbus/bus.h#newcode53
> > dbus/bus.h:53: Options();
> > On 2011/08/01 19:56:41, satorux1 wrote:
> > > On 2011/07/29 21:54:17, Steven Bennetts wrote:
> > > > For a struct should probably just inline the constructor.
> > > 
> > > That will break the Clang bot. I learned it hard way. :)
> > 
> > I take it back. This struct is probably ok, as members are all primitive
> types,
> > but it'll break if we add something like std::string in the future, so I'd
> keep
> > the constructor in .cc file.

[stevenjb, 2011-08-10 19:40:07]

Reviewed everything except test code so far.

You pushed a patch while I was reviewing, so my comments are across patchsets 18
and 19.

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.cc
File dbus/bus.cc (right):

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.cc#newcode137
dbus/bus.cc:137: void Destroy() {
"Destroy" and "is_destroyed_" is a bit confusing, since we're not actually
delsting/destroying the object.  "Complete" and "is_completed_" might be better.

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.cc#newcode218
dbus/bus.cc:218: object_proxies_.push_back(object_proxy);
Rather than calling AddRef and Release manually, shouldn't object_proxies_ just
be a list of scoped_refptr<ObjectProxy>?

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.cc#newcode230
dbus/bus.cc:230: exported_objects_.push_back(exported_object);
scoped_refptr<> here also?

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.cc#newcode273
dbus/bus.cc:273: }
ReleaseOwnership may fail to release a service, and will log an error but not
crash, causing this to spin infinitely.
A better way to do this would be:

for (std::set<std::string>::iterator iter = owner_service_names_.begin();
    iter != owner_service_names_.end(); ) {
  const std::string& service_name = *iter++;  // safely increments iter
  ReleaseOwnership(service_name);
}

if (!owner_service_names_.empty())
  // log an error and/or force release

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.cc#newcode486
dbus/bus.cc:486: // watch will be deleted when raw_watch is removed in
OnRemoveWatch().
Is OnRemoveWatch() guaranteed to be called, even if StartWatching() never is?

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.cc#newcode520
dbus/bus.cc:520: timeout->AddRef();  // Balanced on OnRemoveTimeout().
It might be more clear to put AddRef() in Timeout() and Release() in
Destroy/Complete()

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.h
File dbus/bus.h (right):

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.h#newcode146
dbus/bus.h:146: ~Options();
nit: a struct shouldn't need a destructor, and I find it is better to inline the
constructor so that it is self-documenting (i.e. the default comments next to
the members below become unnecessary).

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.h#newcode190
dbus/bus.h:190: // Shutdowns the bus and blocks until it's done. More
specifically, this
nit: Shuts down

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.h#newcode191
dbus/bus.h:191: // function does the followings:
nit: following

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.h#newcode195
dbus/bus.h:195: // - Close the connection to dbus-daemon.
nit: Closes

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.h#newcode200
dbus/bus.h:200: // Shutdowns the bus in the D-Bus thread. |callback| will be
called in
nit: Shuts down

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.h#newcode342
dbus/bus.h:342: void* data);
Can these be declared in a local namespace inside the .cc file?

http://codereview.chromium.org/7491029/diff/33001/dbus/bus.cc
File dbus/bus.cc (right):

http://codereview.chromium.org/7491029/diff/33001/dbus/bus.cc#newcode422
dbus/bus.cc:422: object_path.c_str());
Since this will only fail on out of memory, and the only thing we can do there
is crash, it makes more sense to do the CHECK() here and return void.

http://codereview.chromium.org/7491029/diff/33001/dbus/exported_object.cc
File dbus/exported_object.cc (right):

http://codereview.chromium.org/7491029/diff/33001/dbus/exported_object.cc#new...
dbus/exported_object.cc:65: absolute_method_name, method_call_callback)).second)
{
This will actually do the insertion. What you really want here is to do a find
first and early exit if an entry already exists, then do a simple map []
insertion (which is easier to read anyway).

http://codereview.chromium.org/7491029/diff/33001/dbus/exported_object.cc#new...
dbus/exported_object.cc:164: const std::string member =
method_call->GetMember();
Can these be references, or are they constructed?

http://codereview.chromium.org/7491029/diff/33001/dbus/exported_object.cc#new...
dbus/exported_object.cc:200: // If the D-Bus thread is not use, just call the
method directly. We
s/use/used

http://codereview.chromium.org/7491029/diff/33001/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

http://codereview.chromium.org/7491029/diff/33001/dbus/object_proxy.cc#newcod...
dbus/object_proxy.cc:114: timeout_ms);
nit: alignment

http://codereview.chromium.org/7491029/diff/33001/dbus/object_proxy.h
File dbus/object_proxy.h (right):

http://codereview.chromium.org/7491029/diff/33001/dbus/object_proxy.h#newcode83
dbus/object_proxy.h:83: ResponseCallback in_response_callback);
struct constructors can generally be inlined.

[satorux1, 2011-08-10 21:14:27]

Thank you for the valuable comments. I was impressed that you caught a potential
infinite loop in the original code!

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.cc
File dbus/bus.cc (right):

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.cc#newcode137
dbus/bus.cc:137: void Destroy() {
On 2011/08/10 19:40:07, Steven Bennetts wrote:
> "Destroy" and "is_destroyed_" is a bit confusing, since we're not actually
> delsting/destroying the object.  "Complete" and "is_completed_" might be
better.

You are right. Changed accordingly.

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.cc#newcode218
dbus/bus.cc:218: object_proxies_.push_back(object_proxy);
On 2011/08/10 19:40:07, Steven Bennetts wrote:
> Rather than calling AddRef and Release manually, shouldn't object_proxies_
just
> be a list of scoped_refptr<ObjectProxy>?

Good point. I thought vector<scoped_refptr> wouldn't compile just like
vector<scoped_ptr> does not. However, it turned out that vector<scoped_refptr>
was widely used in Chrome. Changed accordingly.

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.cc#newcode230
dbus/bus.cc:230: exported_objects_.push_back(exported_object);
On 2011/08/10 19:40:07, Steven Bennetts wrote:
> scoped_refptr<> here also?

Done.

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.cc#newcode273
dbus/bus.cc:273: }
On 2011/08/10 19:40:07, Steven Bennetts wrote:
> ReleaseOwnership may fail to release a service, and will log an error but not
> crash, causing this to spin infinitely.
> A better way to do this would be:
> 
> for (std::set<std::string>::iterator iter = owner_service_names_.begin();
>     iter != owner_service_names_.end(); ) {
>   const std::string& service_name = *iter++;  // safely increments iter
>   ReleaseOwnership(service_name);
> }
> 
> if (!owner_service_names_.empty())
>   // log an error and/or force release

Very good catch! Changed accordingly.

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.cc#newcode486
dbus/bus.cc:486: // watch will be deleted when raw_watch is removed in
OnRemoveWatch().
On 2011/08/10 19:40:07, Steven Bennetts wrote:
> Is OnRemoveWatch() guaranteed to be called, even if StartWatching() never is?

I think it's guaranteed, but I wasn't able to find the documentation saying it
so. So far, valgrind hasn't detected any leaks from the Watch object.

Anyway, I think your point is valid. I've added a counter to make sure that # of
AddWatch and # of RemoveWatch are balanced. I also did the same for Timeout.

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.cc#newcode520
dbus/bus.cc:520: timeout->AddRef();  // Balanced on OnRemoveTimeout().
On 2011/08/10 19:40:07, Steven Bennetts wrote:
> It might be more clear to put AddRef() in Timeout() and Release() in
> Destroy/Complete()

Agreed. Done.

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.h
File dbus/bus.h (right):

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.h#newcode146
dbus/bus.h:146: ~Options();
On 2011/08/10 19:40:07, Steven Bennetts wrote:
> nit: a struct shouldn't need a destructor, and I find it is better to inline
the
> constructor so that it is self-documenting (i.e. the default comments next to
> the members below become unnecessary).

I agree with you that it'd be nicer to inline the constructor. The reason I put
the definition in .cc file was that I was bitten by the clang bot more than once
where they raised an error for "complex" constructor and destructor:

http://www.chromium.org/developers/coding-style/chromium-style-checker-errors

Here, we are safe, but we'll be bitten by the clang bot, if we add more members,
so I'd keep the definition in .cc file.

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.h#newcode190
dbus/bus.h:190: // Shutdowns the bus and blocks until it's done. More
specifically, this
On 2011/08/10 19:40:07, Steven Bennetts wrote:
> nit: Shuts down

Done.

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.h#newcode191
dbus/bus.h:191: // function does the followings:
On 2011/08/10 19:40:07, Steven Bennetts wrote:
> nit: following

Done.

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.h#newcode195
dbus/bus.h:195: // - Close the connection to dbus-daemon.
On 2011/08/10 19:40:07, Steven Bennetts wrote:
> nit: Closes

Done.

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.h#newcode200
dbus/bus.h:200: // Shutdowns the bus in the D-Bus thread. |callback| will be
called in
On 2011/08/10 19:40:07, Steven Bennetts wrote:
> nit: Shuts down

Done.

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.h#newcode342
dbus/bus.h:342: void* data);
On 2011/08/10 19:40:07, Steven Bennetts wrote:
> Can these be declared in a local namespace inside the .cc file?

These need to be static member functions, as these call private non-static
member functions like:

dbus_bool_t Bus::OnAddWatchThunk(DBusWatch* raw_watch, void* data) {
  Bus* self = static_cast<Bus*>(data);
  return self->OnAddWatch(raw_watch);  // OnAddWatch() is private.

[stevenjb, 2011-08-10 21:31:17]

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.h
File dbus/bus.h (right):

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.h#newcode146
dbus/bus.h:146: ~Options();
On 2011/08/10 21:14:27, satorux1 wrote:
> On 2011/08/10 19:40:07, Steven Bennetts wrote:
> > nit: a struct shouldn't need a destructor, and I find it is better to inline
> the
> > constructor so that it is self-documenting (i.e. the default comments next
to
> > the members below become unnecessary).
> 
> I agree with you that it'd be nicer to inline the constructor. The reason I
put
> the definition in .cc file was that I was bitten by the clang bot more than
once
> where they raised an error for "complex" constructor and destructor:
> 
> http://www.chromium.org/developers/coding-style/chromium-style-checker-errors
> 
> Here, we are safe, but we'll be bitten by the clang bot, if we add more
members,
> so I'd keep the definition in .cc file.

Meh. I didn't realize clang would trigger on number of members. The whole point
of a struct is that it shouldn't have any logic, which is why I prefer inline
constructors, but whatever. Given clang's behavior, your reasoning is sound.

http://codereview.chromium.org/7491029/diff/31001/dbus/bus.h#newcode342
dbus/bus.h:342: void* data);
On 2011/08/10 21:14:27, satorux1 wrote:
> On 2011/08/10 19:40:07, Steven Bennetts wrote:
> > Can these be declared in a local namespace inside the .cc file?
> 
> These need to be static member functions, as these call private non-static
> member functions like:
> 
> dbus_bool_t Bus::OnAddWatchThunk(DBusWatch* raw_watch, void* data) {
>   Bus* self = static_cast<Bus*>(data);
>   return self->OnAddWatch(raw_watch);  // OnAddWatch() is private.
> 

That makes sense.

[satorux1, 2011-08-10 21:40:55]

http://codereview.chromium.org/7491029/diff/33001/dbus/bus.cc
File dbus/bus.cc (right):

http://codereview.chromium.org/7491029/diff/33001/dbus/bus.cc#newcode422
dbus/bus.cc:422: object_path.c_str());
On 2011/08/10 19:40:07, Steven Bennetts wrote:
> Since this will only fail on out of memory, and the only thing we can do there
> is crash, it makes more sense to do the CHECK() here and return void.

You are right. Done.

http://codereview.chromium.org/7491029/diff/33001/dbus/exported_object.cc
File dbus/exported_object.cc (right):

http://codereview.chromium.org/7491029/diff/33001/dbus/exported_object.cc#new...
dbus/exported_object.cc:65: absolute_method_name, method_call_callback)).second)
{
On 2011/08/10 19:40:07, Steven Bennetts wrote:
> This will actually do the insertion. What you really want here is to do a find
> first and early exit if an entry already exists, then do a simple map []
> insertion (which is easier to read anyway).

insert() won't insert if the key is already present but I agree with you that
relying on insert()'s return value is not easy to read. Changed the code
accordingly.

http://codereview.chromium.org/7491029/diff/33001/dbus/exported_object.cc#new...
dbus/exported_object.cc:164: const std::string member =
method_call->GetMember();
On 2011/08/10 19:40:07, Steven Bennetts wrote:
> Can these be references, or are they constructed?

These function return a string, not a const reference.

http://codereview.chromium.org/7491029/diff/33001/dbus/exported_object.cc#new...
dbus/exported_object.cc:200: // If the D-Bus thread is not use, just call the
method directly. We
On 2011/08/10 19:40:07, Steven Bennetts wrote:
> s/use/used

Done.

http://codereview.chromium.org/7491029/diff/33001/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

http://codereview.chromium.org/7491029/diff/33001/dbus/object_proxy.cc#newcod...
dbus/object_proxy.cc:114: timeout_ms);
On 2011/08/10 19:40:07, Steven Bennetts wrote:
> nit: alignment

Done.

http://codereview.chromium.org/7491029/diff/33001/dbus/object_proxy.h
File dbus/object_proxy.h (right):

http://codereview.chromium.org/7491029/diff/33001/dbus/object_proxy.h#newcode83
dbus/object_proxy.h:83: ResponseCallback in_response_callback);
On 2011/08/10 19:40:07, Steven Bennetts wrote:
> struct constructors can generally be inlined.

As mentioned before, this may make the clang bot upset as
ResponseCalback is a complex object. Instead of worrying about the clang bot's
anger, I'd keep this in .cc file, with the corresponding destructor.

http://www.chromium.org/developers/coding-style/chromium-style-checker-errors

[stevenjb, 2011-08-10 21:57:54]

I'll try to give this one more look over and glance through the test code later
today or tomorrow.

http://codereview.chromium.org/7491029/diff/33001/dbus/exported_object.cc
File dbus/exported_object.cc (right):

http://codereview.chromium.org/7491029/diff/33001/dbus/exported_object.cc#new...
dbus/exported_object.cc:65: absolute_method_name, method_call_callback)).second)
{
On 2011/08/10 21:40:55, satorux1 wrote:
> On 2011/08/10 19:40:07, Steven Bennetts wrote:
> > This will actually do the insertion. What you really want here is to do a
find
> > first and early exit if an entry already exists, then do a simple map []
> > insertion (which is easier to read anyway).
> 
> insert() won't insert if the key is already present but I agree with you that
> relying on insert()'s return value is not easy to read. Changed the code
> accordingly.

Ah, yes, you're right, but yeah, I generally prefer to avoid map insert since it
is so unusual. Cheers.

http://codereview.chromium.org/7491029/diff/33001/dbus/exported_object.cc#new...
dbus/exported_object.cc:164: const std::string member =
method_call->GetMember();
On 2011/08/10 21:40:55, satorux1 wrote:
> On 2011/08/10 19:40:07, Steven Bennetts wrote:
> > Can these be references, or are they constructed?
> 
> These function return a string, not a const reference.
ok, cool.

http://codereview.chromium.org/7491029/diff/33001/dbus/object_proxy.h
File dbus/object_proxy.h (right):

http://codereview.chromium.org/7491029/diff/33001/dbus/object_proxy.h#newcode83
dbus/object_proxy.h:83: ResponseCallback in_response_callback);
On 2011/08/10 21:40:55, satorux1 wrote:
> On 2011/08/10 19:40:07, Steven Bennetts wrote:
> > struct constructors can generally be inlined.
> 
> As mentioned before, this may make the clang bot upset as
> ResponseCalback is a complex object. Instead of worrying about the clang bot's
> anger, I'd keep this in .cc file, with the corresponding destructor.
> 
> http://www.chromium.org/developers/coding-style/chromium-style-checker-errors
Yep, cool.

[satorux1, 2011-08-10 23:04:19]

On 2011/08/10 21:57:54, Steven Bennetts wrote:
Thank you for all the feedback.

I've just uploaded the patch set 21, that changes the async method call API so
callbacks of async method calls do not need to delete the Response object. It's
much less error-prone to delete it in the library.

> I'll try to give this one more look over and glance through the test code
later
> today or tomorrow.
> 
> http://codereview.chromium.org/7491029/diff/33001/dbus/exported_object.cc
> File dbus/exported_object.cc (right):
> 
>
http://codereview.chromium.org/7491029/diff/33001/dbus/exported_object.cc#new...
> dbus/exported_object.cc:65: absolute_method_name,
method_call_callback)).second)
> {
> On 2011/08/10 21:40:55, satorux1 wrote:
> > On 2011/08/10 19:40:07, Steven Bennetts wrote:
> > > This will actually do the insertion. What you really want here is to do a
> find
> > > first and early exit if an entry already exists, then do a simple map []
> > > insertion (which is easier to read anyway).
> > 
> > insert() won't insert if the key is already present but I agree with you
that
> > relying on insert()'s return value is not easy to read. Changed the code
> > accordingly.
> 
> Ah, yes, you're right, but yeah, I generally prefer to avoid map insert since
it
> is so unusual. Cheers.
> 
>
http://codereview.chromium.org/7491029/diff/33001/dbus/exported_object.cc#new...
> dbus/exported_object.cc:164: const std::string member =
> method_call->GetMember();
> On 2011/08/10 21:40:55, satorux1 wrote:
> > On 2011/08/10 19:40:07, Steven Bennetts wrote:
> > > Can these be references, or are they constructed?
> > 
> > These function return a string, not a const reference.
> ok, cool.
> 
> http://codereview.chromium.org/7491029/diff/33001/dbus/object_proxy.h
> File dbus/object_proxy.h (right):
> 
>
http://codereview.chromium.org/7491029/diff/33001/dbus/object_proxy.h#newcode83
> dbus/object_proxy.h:83: ResponseCallback in_response_callback);
> On 2011/08/10 21:40:55, satorux1 wrote:
> > On 2011/08/10 19:40:07, Steven Bennetts wrote:
> > > struct constructors can generally be inlined.
> > 
> > As mentioned before, this may make the clang bot upset as
> > ResponseCalback is a complex object. Instead of worrying about the clang
bot's
> > anger, I'd keep this in .cc file, with the corresponding destructor.
> > 
> >
http://www.chromium.org/developers/coding-style/chromium-style-checker-errors
> Yep, cool.

[satorux1, 2011-08-10 23:23:09]

The patch set 22 has fixed incorrect comments about deleting objects in
callbacks...

On 2011/08/10 23:04:19, satorux1 wrote:
> On 2011/08/10 21:57:54, Steven Bennetts wrote:
> Thank you for all the feedback.
> 
> I've just uploaded the patch set 21, that changes the async method call API so
> callbacks of async method calls do not need to delete the Response object.
It's
> much less error-prone to delete it in the library.
> 
> > I'll try to give this one more look over and glance through the test code
> later
> > today or tomorrow.
> > 
> > http://codereview.chromium.org/7491029/diff/33001/dbus/exported_object.cc
> > File dbus/exported_object.cc (right):
> > 
> >
>
http://codereview.chromium.org/7491029/diff/33001/dbus/exported_object.cc#new...
> > dbus/exported_object.cc:65: absolute_method_name,
> method_call_callback)).second)
> > {
> > On 2011/08/10 21:40:55, satorux1 wrote:
> > > On 2011/08/10 19:40:07, Steven Bennetts wrote:
> > > > This will actually do the insertion. What you really want here is to do
a
> > find
> > > > first and early exit if an entry already exists, then do a simple map []
> > > > insertion (which is easier to read anyway).
> > > 
> > > insert() won't insert if the key is already present but I agree with you
> that
> > > relying on insert()'s return value is not easy to read. Changed the code
> > > accordingly.
> > 
> > Ah, yes, you're right, but yeah, I generally prefer to avoid map insert
since
> it
> > is so unusual. Cheers.
> > 
> >
>
http://codereview.chromium.org/7491029/diff/33001/dbus/exported_object.cc#new...
> > dbus/exported_object.cc:164: const std::string member =
> > method_call->GetMember();
> > On 2011/08/10 21:40:55, satorux1 wrote:
> > > On 2011/08/10 19:40:07, Steven Bennetts wrote:
> > > > Can these be references, or are they constructed?
> > > 
> > > These function return a string, not a const reference.
> > ok, cool.
> > 
> > http://codereview.chromium.org/7491029/diff/33001/dbus/object_proxy.h
> > File dbus/object_proxy.h (right):
> > 
> >
>
http://codereview.chromium.org/7491029/diff/33001/dbus/object_proxy.h#newcode83
> > dbus/object_proxy.h:83: ResponseCallback in_response_callback);
> > On 2011/08/10 21:40:55, satorux1 wrote:
> > > On 2011/08/10 19:40:07, Steven Bennetts wrote:
> > > > struct constructors can generally be inlined.
> > > 
> > > As mentioned before, this may make the clang bot upset as
> > > ResponseCalback is a complex object. Instead of worrying about the clang
> bot's
> > > anger, I'd keep this in .cc file, with the corresponding destructor.
> > > 
> > >
> http://www.chromium.org/developers/coding-style/chromium-style-checker-errors
> > Yep, cool.

[stevenjb, 2011-08-15 21:42:04]

Did a fresh pass through everything. Just a few more comments.

http://codereview.chromium.org/7491029/diff/39004/dbus/bus.cc
File dbus/bus.cc (right):

http://codereview.chromium.org/7491029/diff/39004/dbus/bus.cc#newcode261
dbus/bus.cc:261: // Delete exported objects. Need to unregister them beforehand.
Currently we're not actually deleting the object here, just unregistering them.
Do we want to clear() exported_objects_ here to release them, instead of waiting
for the destructor?

http://codereview.chromium.org/7491029/diff/39004/dbus/bus.cc#newcode326
dbus/bus.cc:326: // Check if we already own the service name.
Minor optimization: set
std::set<std::string>::iterator found = owned_service_names_.find(service_name);
Then use |found| in erase() below to avoid a second lookup.

http://codereview.chromium.org/7491029/diff/39004/dbus/end_to_end_async_unitt...
File dbus/end_to_end_async_unittest.cc (right):

http://codereview.chromium.org/7491029/diff/39004/dbus/end_to_end_async_unitt...
dbus/end_to_end_async_unittest.cc:44: dbus::Bus::Options bus_options;
nit: Might be convenient to have a constructor for Options, especially since
they members are typed and thus self-documenting.

http://codereview.chromium.org/7491029/diff/39004/dbus/end_to_end_async_unitt...
dbus/end_to_end_async_unittest.cc:81: while (response_strings_.size() <
num_responses) {
nit: add timeout here? Default test timeout is pretty long.

http://codereview.chromium.org/7491029/diff/39004/dbus/error.h
File dbus/error.h (right):

http://codereview.chromium.org/7491029/diff/39004/dbus/error.h#newcode6
dbus/error.h:6: #define DBUS_ERROR_H_
This should be scoped_dbus_error.h. That will also help for finding the file.

http://codereview.chromium.org/7491029/diff/39004/dbus/exported_object.cc
File dbus/exported_object.cc (right):

http://codereview.chromium.org/7491029/diff/39004/dbus/exported_object.cc#new...
dbus/exported_object.cc:65: LOG(ERROR) << absolute_method_name << " is already
exported";
Should we do this check earlier, i.e. before Register(), possibly other calls?
Lookup is relatively quick.

http://codereview.chromium.org/7491029/diff/39004/dbus/exported_object.cc#new...
dbus/exported_object.cc:195: on_method_is_called_.Wait();
Does this have a built-in timeout? If not, should we add one? It could be
difficult to debug if the DBus thread gets stuck causing other valid requests to
timeout or fail.  Apologies if I already asked this :)

http://codereview.chromium.org/7491029/diff/39004/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

http://codereview.chromium.org/7491029/diff/39004/dbus/object_proxy.cc#newcod...
dbus/object_proxy.cc:100: if (!bus_->Connect() || !bus_->SetUpAsyncOperations())
{
Comment describing this case? Is this an error? Is the NULL response the failure
indicator?

[satorux1, 2011-08-16 22:25:37]

http://codereview.chromium.org/7491029/diff/39004/dbus/bus.cc
File dbus/bus.cc (right):

http://codereview.chromium.org/7491029/diff/39004/dbus/bus.cc#newcode261
dbus/bus.cc:261: // Delete exported objects. Need to unregister them beforehand.
On 2011/08/15 21:42:04, Steven Bennetts wrote:
> Currently we're not actually deleting the object here, just unregistering
them.
> Do we want to clear() exported_objects_ here to release them, instead of
waiting
> for the destructor?
> 

Good catch. The comment was obsolete. Updated.

http://codereview.chromium.org/7491029/diff/39004/dbus/bus.cc#newcode326
dbus/bus.cc:326: // Check if we already own the service name.
On 2011/08/15 21:42:04, Steven Bennetts wrote:
> Minor optimization: set
> std::set<std::string>::iterator found =
owned_service_names_.find(service_name);
> Then use |found| in erase() below to avoid a second lookup.

Done.

http://codereview.chromium.org/7491029/diff/39004/dbus/end_to_end_async_unitt...
File dbus/end_to_end_async_unittest.cc (right):

http://codereview.chromium.org/7491029/diff/39004/dbus/end_to_end_async_unitt...
dbus/end_to_end_async_unittest.cc:44: dbus::Bus::Options bus_options;
On 2011/08/15 21:42:04, Steven Bennetts wrote:
> nit: Might be convenient to have a constructor for Options, especially since
> they members are typed and thus self-documenting.

I thought about that but I think having a constructor that takes parameters is
not desirable for options, as you need to change every call site when you add a
new parameter.

http://codereview.chromium.org/7491029/diff/39004/dbus/end_to_end_async_unitt...
dbus/end_to_end_async_unittest.cc:81: while (response_strings_.size() <
num_responses) {
On 2011/08/15 21:42:04, Steven Bennetts wrote:
> nit: add timeout here? Default test timeout is pretty long.

In theory, we don't need timeout here, as the test should not take long time.
Let's revisit this once it becomes a problem.

http://codereview.chromium.org/7491029/diff/39004/dbus/error.h
File dbus/error.h (right):

http://codereview.chromium.org/7491029/diff/39004/dbus/error.h#newcode6
dbus/error.h:6: #define DBUS_ERROR_H_
On 2011/08/15 21:42:04, Steven Bennetts wrote:
> This should be scoped_dbus_error.h. That will also help for finding the file.

Good idea. Done.

http://codereview.chromium.org/7491029/diff/39004/dbus/exported_object.cc
File dbus/exported_object.cc (right):

http://codereview.chromium.org/7491029/diff/39004/dbus/exported_object.cc#new...
dbus/exported_object.cc:65: LOG(ERROR) << absolute_method_name << " is already
exported";
On 2011/08/15 21:42:04, Steven Bennetts wrote:
> Should we do this check earlier, i.e. before Register(), possibly other calls?
> Lookup is relatively quick.

I think it's slightly easier to read to keep this near
method_table_[absolute_method_name] = ... below.

If an method is already exported, all initialization like Connect() and
Register() has been done, so calls above almost do nothing.

http://codereview.chromium.org/7491029/diff/39004/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

http://codereview.chromium.org/7491029/diff/39004/dbus/object_proxy.cc#newcod...
dbus/object_proxy.cc:100: if (!bus_->Connect() || !bus_->SetUpAsyncOperations())
{
On 2011/08/15 21:42:04, Steven Bennetts wrote:
> Comment describing this case? Is this an error? Is the NULL response the
failure
> indicator?

Yes. Added a comment about it.

[stevenjb, 2011-08-16 22:53:29]

One last comment about the while loop in ExportedObject::HandleMessage.
Otherwise looks good.

http://codereview.chromium.org/7491029/diff/39004/dbus/end_to_end_async_unitt...
File dbus/end_to_end_async_unittest.cc (right):

http://codereview.chromium.org/7491029/diff/39004/dbus/end_to_end_async_unitt...
dbus/end_to_end_async_unittest.cc:44: dbus::Bus::Options bus_options;
On 2011/08/16 22:25:37, satorux1 wrote:
> On 2011/08/15 21:42:04, Steven Bennetts wrote:
> > nit: Might be convenient to have a constructor for Options, especially since
> > they members are typed and thus self-documenting.
> 
> I thought about that but I think having a constructor that takes parameters is
> not desirable for options, as you need to change every call site when you add
a
> new parameter.
> 
Fair enough.

http://codereview.chromium.org/7491029/diff/39004/dbus/end_to_end_async_unitt...
dbus/end_to_end_async_unittest.cc:81: while (response_strings_.size() <
num_responses) {
On 2011/08/16 22:25:37, satorux1 wrote:
> On 2011/08/15 21:42:04, Steven Bennetts wrote:
> > nit: add timeout here? Default test timeout is pretty long.
> 
> In theory, we don't need timeout here, as the test should not take long time.
> Let's revisit this once it becomes a problem.

OK, but if we ever see these tests timing out due to this, we should really
change this. Tests that time out take much longer to complete, and the failures
are much more difficult to identify.

http://codereview.chromium.org/7491029/diff/39004/dbus/exported_object.cc
File dbus/exported_object.cc (right):

http://codereview.chromium.org/7491029/diff/39004/dbus/exported_object.cc#new...
dbus/exported_object.cc:65: LOG(ERROR) << absolute_method_name << " is already
exported";
On 2011/08/16 22:25:37, satorux1 wrote:
> On 2011/08/15 21:42:04, Steven Bennetts wrote:
> > Should we do this check earlier, i.e. before Register(), possibly other
calls?
> > Lookup is relatively quick.
> 
> I think it's slightly easier to read to keep this near
> method_table_[absolute_method_name] = ... below.
> 
> If an method is already exported, all initialization like Connect() and
> Register() has been done, so calls above almost do nothing.

OK, it looks like Register() just returns true if already registered, so that's
fine.

http://codereview.chromium.org/7491029/diff/39004/dbus/exported_object.cc#new...
dbus/exported_object.cc:195: on_method_is_called_.Wait();
On 2011/08/15 21:42:04, Steven Bennetts wrote:
> Does this have a built-in timeout? If not, should we add one? It could be
> difficult to debug if the DBus thread gets stuck causing other valid requests
to
> timeout or fail.  Apologies if I already asked this :)

Looking at this more closely, if the callback (iter->second) gets stuck waiting
or in an infinite loop, then on_method_call_.Signal() will never get called, and
this will be stuck in an infinite loop on a background thread. These types of
errors can be very difficult to debug, so I think we need a timeout here.
Fortunately ConditionVariable has a TimedWait() function that we can use:

const TimeDelta delta_time(TimeDelta::FromSeconds(10));
Time start_time = Time::Now();
while (!method_is_called_) {
  on_method_is_called_.TimedWait(delta_time);
  CHECK(Time::Now() - start_time < delta_time);
}

[satorux1, 2011-08-16 23:19:40]

Steven, that was an excellent point. I somehow missed the original comment.

http://codereview.chromium.org/7491029/diff/39004/dbus/exported_object.cc
File dbus/exported_object.cc (right):

http://codereview.chromium.org/7491029/diff/39004/dbus/exported_object.cc#new...
dbus/exported_object.cc:195: on_method_is_called_.Wait();
On 2011/08/16 22:53:29, Steven Bennetts wrote:
> On 2011/08/15 21:42:04, Steven Bennetts wrote:
> > Does this have a built-in timeout? If not, should we add one? It could be
> > difficult to debug if the DBus thread gets stuck causing other valid
requests
> to
> > timeout or fail.  Apologies if I already asked this :)
> 
> Looking at this more closely, if the callback (iter->second) gets stuck
waiting
> or in an infinite loop, then on_method_call_.Signal() will never get called,
and
> this will be stuck in an infinite loop on a background thread. These types of
> errors can be very difficult to debug, so I think we need a timeout here.
> Fortunately ConditionVariable has a TimedWait() function that we can use:
> 
> const TimeDelta delta_time(TimeDelta::FromSeconds(10));
> Time start_time = Time::Now();
> while (!method_is_called_) {
>   on_method_is_called_.TimedWait(delta_time);
>   CHECK(Time::Now() - start_time < delta_time);
> }

My bad, I missed the original comment. You are absolutely right that infinite
blocking should be avoided. Changed accordingly.

[stevenjb, 2011-08-16 23:22:07]

LGTM!

[commit-bot: I haz the power, 2011-08-17 07:12:14]

Try job failure for 7491029-48005 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2011-08-17 15:50:36]

Try job failure for 7491029-52001 (retry) on linux for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[commit-bot: I haz the power, 2011-08-17 18:32:09]

Try job failure for 7491029-50005 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2011-08-17 20:58:16]

Change committed as 97204