dbus: add ObjectPath type

dbus: add ObjectPath type

Rather than use std::string for object paths, add a dbus::ObjectPath type
that wraps one while allowing more type-safety. This solves all sorts of
issues with confusing object paths for strings, and allows us to do
Properties code using templates disambiguating them from strings.

BUG=chromium:109194
TEST=built and run tests

Change-Id: Icaf6f19daea4af23a9d2ec0ed76d2cbd379d680e


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

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

[satorux1, 2012-02-12 20:16:30]

Thank you for doing this. Some style comments:

http://codereview.chromium.org/9378039/diff/3001/dbus/object_path.h
File dbus/object_path.h (right):

http://codereview.chromium.org/9378039/diff/3001/dbus/object_path.h#newcode34
dbus/object_path.h:34: ObjectPath(const std::string& value) : value_(value) {}
explicit is missing.

http://codereview.chromium.org/9378039/diff/3001/dbus/object_path.h#newcode35
dbus/object_path.h:35: ObjectPath(const char* c_str) : value_(c_str) {}
I think we don't need the const char* version. The std::string version should
allow clients to pass string literals.

http://codereview.chromium.org/9378039/diff/3001/dbus/object_path.h#newcode37
dbus/object_path.h:37: // Retrieve value as a std::string and as a C-style
string.
Retrieve -> Retrieves

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

http://codereview.chromium.org/9378039/diff/3001/dbus/object_path.h#newcode39
dbus/object_path.h:39: const char* c_str() const { return value_.c_str(); }
I think we can drop c_str(). Clients can just go with value().c_str(). Less API
would be desirable.

http://codereview.chromium.org/9378039/diff/3001/dbus/object_path.h#newcode51
dbus/object_path.h:51: bool operator!=(const ObjectPath&, const ObjectPath&);
Can we go with Equals() instead?

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Operat...

http://codereview.chromium.org/9378039/diff/3001/dbus/object_path.h#newcode54
dbus/object_path.h:54: std::ostream& operator<<(std::ostream&, const
ObjectPath&);
Let's drop this. Per our C++ style guide, we rarely define this. Since we
already have value() that returns a string, we can just go with LOG(INFO) <<
object_path.value().

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Operat...

However, there may be rare cases where you need to overload an operator to
interoperate with templates or "standard" C++ classes (such as
operator<<(ostream&, const T&) for logging). These are acceptable if fully
justified, but you should try to avoid these whenever possible.

[keybuk, 2012-02-12 22:09:28]

http://codereview.chromium.org/9378039/diff/3001/dbus/object_path.h
File dbus/object_path.h (right):

http://codereview.chromium.org/9378039/diff/3001/dbus/object_path.h#newcode34
dbus/object_path.h:34: ObjectPath(const std::string& value) : value_(value) {}
see the comment - allowing implicit construction is explicitly intended here so
that you can do:

  bus->GetObjectProxy(kServiceManagerPath...)

which requires implicit construction of an object path from a constant.

The coding style allows this -- "Classes that are intended to be transparent
wrappers around other classes are also exceptions" -- and I wanted this to be as
transparent and painless to use as possible :)

http://codereview.chromium.org/9378039/diff/3001/dbus/object_path.h#newcode35
dbus/object_path.h:35: ObjectPath(const char* c_str) : value_(c_str) {}
yeah, I was convinced of that, but it doesn't work in the case above...

dbus::ObjectPath manager(kServiceManagerPath) will work because it has a const
char *, and expects a std::string, and can call a std::string into existence to
fulfill the constructor

but dbus::ObjectPath manager = kServiceManagerPath will not work, because the
compiler will not call intermediate type into existence

http://codereview.chromium.org/9378039/diff/3001/dbus/object_path.h#newcode39
dbus/object_path.h:39: const char* c_str() const { return value_.c_str(); }
sure thing, only useful inside dbus:: anyway (which is the only place that turns
them into C strings)

http://codereview.chromium.org/9378039/diff/3001/dbus/object_path.h#newcode51
dbus/object_path.h:51: bool operator!=(const ObjectPath&, const ObjectPath&);
note the "required for mocks to work" in the comment ;) since mocks don't
compile without these, I figured commenting that they were also useful for
observers didn't hurt

http://codereview.chromium.org/9378039/diff/3001/dbus/object_path.h#newcode54
dbus/object_path.h:54: std::ostream& operator<<(std::ostream&, const
ObjectPath&);
my hope is that we can actually get rid of value() entirely, code in general
shouldn't be peeking at the values of object paths and should treat them as
opaque types retrieved from dbus calls or instantiated from constants

there's a few places that won't work right now (mostly inside dbus::), but I'm
happy to file a bug to fix those so we can drop value() entirely, in which case
the logging operator is useful

[satorux1, 2012-02-13 19:07:36]

Believe me, I'm generally not a nit-picky reviewer, but for this case, I insist.
:)

http://codereview.chromium.org/9378039/diff/3001/dbus/object_path.h
File dbus/object_path.h (right):

http://codereview.chromium.org/9378039/diff/3001/dbus/object_path.h#newcode34
dbus/object_path.h:34: ObjectPath(const std::string& value) : value_(value) {}
On 2012/02/12 22:09:29, keybuk wrote:
> see the comment - allowing implicit construction is explicitly intended here
so
> that you can do:
> 
>   bus->GetObjectProxy(kServiceManagerPath...)
> 
> which requires implicit construction of an object path from a constant.
> 
> The coding style allows this -- "Classes that are intended to be transparent
> wrappers around other classes are also exceptions" -- and I wanted this to be
as
> transparent and painless to use as possible :)

On the contrary, I don't want to make it transparent for consistency with other
pieces of code in Chrome. Except for few places like implementation of
base::Bind(), Chrome code prefers making things clearer or very obvious over
making code succinct. In other words, less magic is preferred, which is also the
philosophy of the C++ style guide.

For instance, FilePath class in 'base' is conceptually similar to ObjectPath
here, but does not allow the implicit conversion from string to FilePath (i.e.
they use 'explicit' in the constructor.

That said, clients code will look like:

  bus->GetObjectProxy(ObjectPath(kServiceManagerPath), ...)

which is less succinct, but clearer about what's happening.

http://codereview.chromium.org/9378039/diff/3001/dbus/object_path.h#newcode51
dbus/object_path.h:51: bool operator!=(const ObjectPath&, const ObjectPath&);
On 2012/02/12 22:09:29, keybuk wrote:
> note the "required for mocks to work" in the comment ;) since mocks don't
> compile without these, I figured commenting that they were also useful for
> observers didn't hurt

BTW, FilePath has these operators inside the class. Can we follow the style?

I vaguely remember that the "effective C++" discussed on this topic, but I don't
have a copy right now.

http://codereview.chromium.org/9378039/diff/3001/dbus/object_path.h#newcode54
dbus/object_path.h:54: std::ostream& operator<<(std::ostream&, const
ObjectPath&);
On 2012/02/12 22:09:29, keybuk wrote:
> my hope is that we can actually get rid of value() entirely, code in general
> shouldn't be peeking at the values of object paths and should treat them as
> opaque types retrieved from dbus calls or instantiated from constants
> 
> there's a few places that won't work right now (mostly inside dbus::), but I'm
> happy to file a bug to fix those so we can drop value() entirely, in which
case
> the logging operator is useful

For the same reason mentioned above, making it transparent is not desirable.

[keybuk, 2012-02-13 19:18:25]

ok, one harder to use class coming right up ;-)

[keybuk, 2012-02-13 20:04:41]

http://codereview.chromium.org/9378039/diff/3001/dbus/object_path.h
File dbus/object_path.h (right):

http://codereview.chromium.org/9378039/diff/3001/dbus/object_path.h#newcode51
dbus/object_path.h:51: bool operator!=(const ObjectPath&, const ObjectPath&);
yup, I missed out the "const" from them when I tried it, which is why it didn't
work - will fix

http://codereview.chromium.org/9378039/diff/3001/dbus/object_path.h#newcode54
dbus/object_path.h:54: std::ostream& operator<<(std::ostream&, const
ObjectPath&);
meh meh meh :p

[keybuk, 2012-02-13 20:19:32]

ok, now all explicit and boring ;-)

[satorux1, 2012-02-13 20:27:39]

LGTM. Boring code is good! :)

BTW, since this patch touches many files, it's desirable to run try bots on
various platforms just in case (commit queue should do this, but in reality CQ
only builds on linux,win,mac IIRC). Do you have a provisional committer access?
Otherwise, I'd be happy to run try bots on behalf of you, and nominate you as a
provisional committer at the same time.

http://codereview.chromium.org/9378039/diff/10001/dbus/object_path.h
File dbus/object_path.h (right):

http://codereview.chromium.org/9378039/diff/10001/dbus/object_path.h#newcode27
dbus/object_path.h:27: explicit ObjectPath(const char* c_str) : value_(c_str) {}
Do we still need the c_str version? I thought the std::string version should be
sufficient to take string literals?

[satorux1, 2012-02-13 20:28:32]

+willchan for one-line change under 'net'.

[keybuk, 2012-02-13 20:41:12]

http://codereview.chromium.org/9378039/diff/10001/dbus/object_path.h
File dbus/object_path.h (right):

http://codereview.chromium.org/9378039/diff/10001/dbus/object_path.h#newcode27
dbus/object_path.h:27: explicit ObjectPath(const char* c_str) : value_(c_str) {}
nope, now we're only doing explicit initialization, I can drop this.

Done

[keybuk, 2012-02-13 20:41:52]

I don't have committer access, provision or otherwise - so if you can trybot
this for me, that would be great!

Thanks

[satorux1, 2012-02-13 21:12:28]

Started try bots. I'll be nominating you as a provisional committer.

trychange.py -R http://codereview.chromium.org/9378039/
--email=satorux@chromium.org,keybuk@chromium.org --name object_path_class --root
src -p 0
--bot=linux,win,mac,linux_chromeos,linux_chromeos_gtk,linux_clang,linux_chromeos_clang

[satorux1, 2012-02-13 21:14:39]

On 2012/02/13 21:12:28, satorux1 wrote:
> Started try bots. I'll be nominating you as a provisional committer.
> 
> trychange.py -R http://codereview.chromium.org/9378039/
> mailto:--email=satorux@chromium.org,keybuk@chromium.org --name
object_path_class --root
> src -p 0
>
--bot=linux,win,mac,linux_chromeos,linux_chromeos_gtk,linux_clang,linux_chromeos_clang

Patch failed. LKGR looks just old. Let me rerun the bots with a newer revision.

[satorux1, 2012-02-13 21:20:50]

On 2012/02/13 21:14:39, satorux1 wrote:
> On 2012/02/13 21:12:28, satorux1 wrote:
> > Started try bots. I'll be nominating you as a provisional committer.
> > 
> > trychange.py -R http://codereview.chromium.org/9378039/
> > mailto:--email=satorux@chromium.org,keybuk@chromium.org --name
> object_path_class --root
> > src -p 0
> >
>
--bot=linux,win,mac,linux_chromeos,linux_chromeos_gtk,linux_clang,linux_chromeos_clang
> 
> Patch failed. LKGR looks just old. Let me rerun the bots with a newer
revision.

Weird. Looking at
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos_gtk/bu...,
the patch is failed completely, rather than partially failed. I guess I
should've passed -p 1 instead of -p 0... trying again.

[keybuk, 2012-02-13 23:43:55]

From the trybot failures there were a few more places that needed changing that
for whatever reason weren't getting built my end(*)

(* probably because I can't "make -j16" due to GTK+ removal)

I've patched these and uploaded a new patchset, can you trybot again for me?

[satorux1, 2012-02-13 23:57:30]

Sure. Just restarted the try bots.

[commit-bot: I haz the power, 2012-02-14 04:50:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keybuk@chromium.org/9378039/12019

[keybuk, 2012-02-14 04:50:16]

marking for commit, the linux_chromeos_gtk looks more related to DiRT than the
patch

[commit-bot: I haz the power, 2012-02-14 04:50:26]

Presubmit check for 9378039-12019 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
net/base/network_change_notifier_linux.cc,content/browser/geolocation/wifi_data_provider_linux_unittest.cc,content/browser/geolocation/wifi_data_provider_linux.cc,net/base/network_change_notifier_linux_unittest.cc,content/browser/power_save_blocker_linux.cc

Presubmit checks took 4.9s to calculate.

[willchan no longer on Chromium, 2012-02-14 08:52:10]

net/ changes LGTM

On 2012/02/14 04:50:26, I haz the power (commit-bot) wrote:
> Presubmit check for 9378039-12019 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for:
>
net/base/network_change_notifier_linux.cc,content/browser/geolocation/wifi_data_provider_linux_unittest.cc,content/browser/geolocation/wifi_data_provider_linux.cc,net/base/network_change_notifier_linux_unittest.cc,content/browser/power_save_blocker_linux.cc
> 
> Presubmit checks took 4.9s to calculate.

[commit-bot: I haz the power, 2012-02-14 15:16:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keybuk@chromium.org/9378039/12019

[commit-bot: I haz the power, 2012-02-14 15:17:06]

Presubmit check for 9378039-12019 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/browser/geolocation/wifi_data_provider_linux_unittest.cc,content/browser/geolocation/wifi_data_provider_linux.cc,content/browser/power_save_blocker_linux.cc

Presubmit checks took 3.9s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[keybuk, 2012-02-14 15:29:31]

Hi, owners of:

chrome/browser/password_manager,
content/browser/,
content/browser/geolocation

I'd appreciate if it you could take the time to review the minor changes to your
code that this CL touches and give me an LGTM :)

[jam, 2012-02-14 15:42:42]

rubberstamp lgtm for content

[John Knottenbelt, 2012-02-14 15:56:14]

lgtm

[bulach, 2012-02-14 17:51:24]

rubberstamp lgtm for geolocation

[commit-bot: I haz the power, 2012-02-14 18:28:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keybuk@chromium.org/9378039/12019

[Mike Mammarella, 2012-02-14 19:37:33]

LGTM

[commit-bot: I haz the power, 2012-02-14 20:08:10]

Change committed as 121920