Allow dbus clients to silence logging when a service is unavailable.

Allow dbus clients to silence logging when a service is unavailable.

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

[adamk, 2012-02-09 21:36:05]

Satoru: note that I've taken a different approach here than discussed previously
in email. If you don't like this minimal way around the logging issue, I think
the right thing to do is to fully expose the error information to callers and
have them log as appropriate for their varying circumstances.

[satorux1, 2012-02-10 08:12:52]

Let's make it a bit more generic, and then I think it's an acceptable solution.
See below.

Please also split this patch to two patches: 1) changes in 'dbus', 2) changes in
the network code. Smaller patches are preferable. splitting will make 'dbus' and
'net' owners happier, I think.

http://codereview.chromium.org/9373039/diff/1/dbus/bus.h
File dbus/bus.h (right):

http://codereview.chromium.org/9373039/diff/1/dbus/bus.h#newcode203
dbus/bus.h:203: virtual ObjectProxy* GetObjectProxyIgnoreUnknownService(
Let's make this more generic, like GetObjectProxyWithOptions(
  const std::string& service_name, const std::string& object_path, int options);

See my other comments about 'options'.

http://codereview.chromium.org/9373039/diff/1/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

http://codereview.chromium.org/9373039/diff/1/dbus/object_proxy.cc#newcode85
dbus/object_proxy.cc:85: const bool error_is_set = error.is_set();
The caching is unnecessary. You'll only save two bytes "()".

http://codereview.chromium.org/9373039/diff/1/dbus/object_proxy.cc#newcode88
dbus/object_proxy.cc:88: strcmp(error_name, kErrorServiceUnknown)) {
strcmp() is hard to read. Please make it something like

 std::string(error_name) != kErrorServiceUnknown

http://codereview.chromium.org/9373039/diff/1/dbus/object_proxy.h
File dbus/object_proxy.h (right):

http://codereview.chromium.org/9373039/diff/1/dbus/object_proxy.h#newcode39
dbus/object_proxy.h:39: IGNORE_SERVICE_UNKNOWN_ERRORS
Let's make this more generic like

// OR-ed options.
enum ObjectProxyOptions {
  NONE = 0,
  IGNORE_SERVICE_UNKNOWN_ERRORS = 1 << 0,
  // The options are OR-ed so the next option value should be 1 << 1
}

http://codereview.chromium.org/9373039/diff/1/dbus/object_proxy.h#newcode47
dbus/object_proxy.h:47: ServiceUnknownBehavior service_unknown_behavior);
And make it 'int'

http://codereview.chromium.org/9373039/diff/1/net/base/network_change_notifie...
File net/base/network_change_notifier_linux.cc (right):

http://codereview.chromium.org/9373039/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier_linux.cc:139:
system_bus_->GetObjectProxyIgnoreUnknownService(
Please add some comment why you do this. Having a reference to the bug URL would
be nice. crbug.com/...

[satorux1, 2012-02-10 08:15:13]

http://codereview.chromium.org/9373039/diff/1/dbus/object_proxy.h
File dbus/object_proxy.h (right):

http://codereview.chromium.org/9373039/diff/1/dbus/object_proxy.h#newcode39
dbus/object_proxy.h:39: IGNORE_SERVICE_UNKNOWN_ERRORS
On 2012/02/10 08:12:52, satorux1 wrote:
> Let's make this more generic like
> 
> // OR-ed options.
> enum ObjectProxyOptions {
>   NONE = 0,
>   IGNORE_SERVICE_UNKNOWN_ERRORS = 1 << 0,
>   // The options are OR-ed so the next option value should be 1 << 1
> }

Since it's inside ObjectProxy class, just 'Options' should suffice.

[adamk, 2012-02-10 17:57:16]

Thanks for the review, I've made it a generic options argument as requested, and
split out the NetworkChangeNotifier change.

http://codereview.chromium.org/9373039/diff/1/dbus/bus.h
File dbus/bus.h (right):

http://codereview.chromium.org/9373039/diff/1/dbus/bus.h#newcode203
dbus/bus.h:203: virtual ObjectProxy* GetObjectProxyIgnoreUnknownService(
On 2012/02/10 08:12:52, satorux1 wrote:
> Let's make this more generic, like GetObjectProxyWithOptions(
>   const std::string& service_name, const std::string& object_path, int
options);
> 
> See my other comments about 'options'.

Done.

http://codereview.chromium.org/9373039/diff/1/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

http://codereview.chromium.org/9373039/diff/1/dbus/object_proxy.cc#newcode85
dbus/object_proxy.cc:85: const bool error_is_set = error.is_set();
On 2012/02/10 08:12:52, satorux1 wrote:
> The caching is unnecessary. You'll only save two bytes "()".

It wasn't obvious to me that error.is_set() was cheap. Changed to not cache.

http://codereview.chromium.org/9373039/diff/1/dbus/object_proxy.cc#newcode88
dbus/object_proxy.cc:88: strcmp(error_name, kErrorServiceUnknown)) {
On 2012/02/10 08:12:52, satorux1 wrote:
> strcmp() is hard to read. Please make it something like
> 
>  std::string(error_name) != kErrorServiceUnknown

Changed error_name to be a base::StringPiece.

http://codereview.chromium.org/9373039/diff/1/dbus/object_proxy.h
File dbus/object_proxy.h (right):

http://codereview.chromium.org/9373039/diff/1/dbus/object_proxy.h#newcode39
dbus/object_proxy.h:39: IGNORE_SERVICE_UNKNOWN_ERRORS
On 2012/02/10 08:12:52, satorux1 wrote:
> Let's make this more generic like
> 
> // OR-ed options.
> enum ObjectProxyOptions {
>   NONE = 0,
>   IGNORE_SERVICE_UNKNOWN_ERRORS = 1 << 0,
>   // The options are OR-ed so the next option value should be 1 << 1
> }

Done.

http://codereview.chromium.org/9373039/diff/1/dbus/object_proxy.h#newcode39
dbus/object_proxy.h:39: IGNORE_SERVICE_UNKNOWN_ERRORS
On 2012/02/10 08:12:52, satorux1 wrote:
> Let's make this more generic like
> 
> // OR-ed options.
> enum ObjectProxyOptions {
>   NONE = 0,
>   IGNORE_SERVICE_UNKNOWN_ERRORS = 1 << 0,
>   // The options are OR-ed so the next option value should be 1 << 1
> }

Done.

http://codereview.chromium.org/9373039/diff/1/dbus/object_proxy.h#newcode47
dbus/object_proxy.h:47: ServiceUnknownBehavior service_unknown_behavior);
On 2012/02/10 08:12:52, satorux1 wrote:
> And make it 'int'

Done.

http://codereview.chromium.org/9373039/diff/1/net/base/network_change_notifie...
File net/base/network_change_notifier_linux.cc (right):

http://codereview.chromium.org/9373039/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier_linux.cc:139:
system_bus_->GetObjectProxyIgnoreUnknownService(
On 2012/02/10 08:12:52, satorux1 wrote:
> Please add some comment why you do this. Having a reference to the bug URL
would
> be nice. crbug.com/...

Split out into another patch. Sorry, the lack of comment here comes from
spending all my time in WebKit land.

[satorux1, 2012-02-10 18:09:01]

Thanks. I like the new version. I have one more request:

http://codereview.chromium.org/9373039/diff/8009/dbus/bus.cc
File dbus/bus.cc (right):

http://codereview.chromium.org/9373039/diff/8009/dbus/bus.cc#newcode224
dbus/bus.cc:224: key += base::IntToString(options);
This looks like a hack. Can you change the key to std::pair<std::string, int> ?

http://codereview.chromium.org/9373039/diff/8009/dbus/bus.h
File dbus/bus.h (right):

http://codereview.chromium.org/9373039/diff/8009/dbus/bus.h#newcode479
dbus/bus.h:479: // int to the end of the key.
This doesn't seem to be the right place. Move this to where ObjectProxyTable is
defined above. Besides, encoding the options at the end of the string isn't the
best thing we can do. see my comment elsewhere.

http://codereview.chromium.org/9373039/diff/8009/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

http://codereview.chromium.org/9373039/diff/8009/dbus/object_proxy.cc#newcode84
dbus/object_proxy.cc:84: base::StringPiece error_name(error.is_set() ?
error.name() : "");
Cool. StringPiece is cheap. :)

[adamk, 2012-02-10 19:28:21]

Besides your request, I've also factored out the error-logging logic into its
own method to avoid duplicating the if statement, and switched the bool member
to have the same name and boolean status as the option (my impetus to do that
was that the last patch actually got the logic wrong when I switched to
StringPiece!).

http://codereview.chromium.org/9373039/diff/8009/dbus/bus.cc
File dbus/bus.cc (right):

http://codereview.chromium.org/9373039/diff/8009/dbus/bus.cc#newcode224
dbus/bus.cc:224: key += base::IntToString(options);
On 2012/02/10 18:09:01, satorux1 wrote:
> This looks like a hack. Can you change the key to std::pair<std::string, int>
?

Done.

http://codereview.chromium.org/9373039/diff/8009/dbus/bus.h
File dbus/bus.h (right):

http://codereview.chromium.org/9373039/diff/8009/dbus/bus.h#newcode479
dbus/bus.h:479: // int to the end of the key.
On 2012/02/10 18:09:01, satorux1 wrote:
> This doesn't seem to be the right place. Move this to where ObjectProxyTable
is
> defined above. Besides, encoding the options at the end of the string isn't
the
> best thing we can do. see my comment elsewhere.

Woops, this was meant to go above.  I've updated the type and comment as
requested.

[satorux1, 2012-02-10 20:00:11]

LGTM

http://codereview.chromium.org/9373039/diff/11002/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

http://codereview.chromium.org/9373039/diff/11002/dbus/object_proxy.cc#newcod...
dbus/object_proxy.cc:428: return;
Nice. it's now easier to understand and cleaner!

[commit-bot: I haz the power, 2012-02-10 20:05:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adamk@chromium.org/9373039/11002