Make DBUS an optional dependency so Chromium can be built without it....

Make DBUS an optional dependency so Chromium can be built without it.

BUG=63981
TEST=Build with -Duse_dbus=0 without dbus installed and with -Duse_dbus=1. The former build should work and the later should fail.

[Paweł Hajdan Jr., 2011-03-08 21:13:13]

LGTM with a comment, thank you.

Have you signed the CLA/CCLA? Please see
http://www.chromium.org/developers/contributing-code#TOC-Instructions-for-Rev...

Also, please add yourself to the AUTHORS file.

Just a note (writing here for simplicity): please re-open the Gentoo bug after
this lands, to make necessary changes in the ebuild.

http://codereview.chromium.org/6648002/diff/1/content/browser/geolocation/wif...
File content/browser/geolocation/wifi_data_provider_linux.cc (right):

http://codereview.chromium.org/6648002/diff/1/content/browser/geolocation/wif...
content/browser/geolocation/wifi_data_provider_linux.cc:400: #endif
nit: Add // defined(USE_DBUS)

There should be two spaces between #endif and the beginning of the comment.

[Evan Martin, 2011-03-08 21:33:31]

I don't like how this patch makes features silently fail.  I think it will
increase traffic for those features on our bug tracker.  Perhaps we should
extend the patch to make them fail more loudly?  (like when you click a
geolocation button, it should say "you disabled geolocation at compile time")

[Evan Martin, 2011-03-08 21:34:02]

I think the worse one is that if you disable dbus and use a KDE desktop, we will
silently stop encrypting your passwords.

[mounir.lamouri, 2011-03-08 22:22:14]

On 2011/03/08 21:34:02, Evan Martin wrote:
> I think the worse one is that if you disable dbus and use a KDE desktop, we
will
> silently stop encrypting your passwords.

A more complex setup that doesn't involve silently disabling features would be
to add a flag for geolocation and another one for kwallet (even KDE?). That way,
if dbus is manually disabled, those options would have to be disabled too. Does
that sound better to you?

Though, for geolocation, if I have dbus installed but no dbus daemon running,
geolocation wouldn't work too, right?

[Evan Martin, 2011-03-08 22:32:49]

On 2011/03/08 22:22:14, mounir.lamouri wrote:
> A more complex setup that doesn't involve silently disabling features would be
> to add a flag for geolocation and another one for kwallet (even KDE?). That
way,
> if dbus is manually disabled, those options would have to be disabled too.
Does
> that sound better to you?
> 
> Though, for geolocation, if I have dbus installed but no dbus daemon running,
> geolocation wouldn't work too, right?

This is a good point.  (It relies on NetworkManager.)
Maybe I shouldn't worry about these configurations too much, and we're ok with
stuff being broken.  I retract my objection.

[mounir.lamouri, 2011-03-09 15:29:16]

Updated patch with my name added to AUTHORS and the other change requested by
Paweł. I've also submitted a CLA.

[joth, 2011-03-16 07:10:27]

On 8 March 2011 22:32, <evan@chromium.org> wrote:

> On 2011/03/08 22:22:14, mounir.lamouri wrote:
>
>> Though, for geolocation, if I have dbus installed but no dbus daemon
>> running,
>
> geolocation wouldn't work too, right?
>>
>
> This is a good point.  (It relies on NetworkManager.)
> Maybe I shouldn't worry about these configurations too much, and we're ok
> with
> stuff being broken.  I retract my objection.
>
>
> Without DBUS / Network manager, geolocation will (silently) fallback to
using IP address for a coarse location. The geolocation API doesn't provide
any real means to bubble up additional diagnostic info to the JS application
as to why it is receiving a poor location fix, so to date I'd been relying
on providing some additional debug logging to give the curious user
something to go on.
Would there be a simple place to hook in a "NOTIMPLEMENTED: no wifi location
for non-DBUS platform" message?


>
http://codereview.chromium.org/6648002/diff/1/content/browser/geolocation/wif...

rather than alter this .cc, it would be better to edit
http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/geolocation/e...
to have WifiDataProvider::DefaultFactoryFunction
return the "Empty" data provider implementation on "linux" platforms without
dbus. (The empty provider would then be the place to do any appropriate
NOTIMPLEMENTED diagnostic)


This has a knock on implication that the
wifi_data_provider_linux.cc<http://codereview.chromium.org/6648002/diff/1/content/browser/geolocation/wifi_data_provider_linux.cc#newcode400>
be
renamed to
wifi_data_provider_dbus.cc<http://codereview.chromium.org/6648002/diff/1/content/browser/geolocation/wifi_data_provider_linux.cc#newcode400>
(I
presume?) and the .gypi file instructed to only include this file if the
use_dbus flag is set.

Bit more work, but I think this will be cleaner than have multiple layers of
dummying out of the functionality.

Cheers!