Issue 7835021: Linux: use our spiffy new DBus client library for KWallet, instead of dbus-glib.

Issue 7835021: Linux: use our spiffy new DBus client library for KWallet, instead of dbus-glib. (Closed)

Created:
9 years, 3 months ago by Mike Mammarella

Modified:
9 years, 3 months ago

Reviewers:
jam, satorux1

CC:
chromium-reviews, Evan Martin, tony, Avi (use Gerrit)

Base URL:
svn://svn.chromium.org/chrome/trunk/src/

Visibility:
Public.

Description

Linux: use our spiffy new DBus client library for KWallet, instead of dbus-glib. This will also allow us to add unit tests for our KWallet integration. BUG=90036 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=101251

Patch Set 1 : '' #

Patch Set 2 : '' #

Total comments: 16

Patch Set 3 : '' #

Patch Set 4 : '' #

Patch Set 5 : '' #

Total comments: 29

Patch Set 6 : '' #

Patch Set 7 : '' #

Created: 9 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+465 lines, -254 lines)			Patch
A	chrome/browser/password_manager/DEPS	View	1 2 3 4	1 chunk	+3 lines, -0 lines	0 comments	Download
M	chrome/browser/password_manager/native_backend_kwallet_x.h	View	1 2 3 4 5 6	7 chunks	+23 lines, -17 lines	0 comments	Download
M	chrome/browser/password_manager/native_backend_kwallet_x.cc	View	1 2 3 4 5 6	11 chunks	+429 lines, -233 lines	0 comments	Download
M	chrome/chrome_browser.gypi	View	1 2 3 4 5 6	1 chunk	+2 lines, -0 lines	0 comments	Download
M	content/browser/browser_main.cc	View	1 2 3 4 5 6	2 chunks	+8 lines, -4 lines	0 comments	Download

Messages

Total messages: 11 (0 generated)

Expand Messages | Collapse Messages

satorux1

Thank you for reworking the kwallet code using the new D-Bus library! http://codereview.chromium.org/7835021/diff/8001/chrome/browser/browser_main.cc File chrome/browser/browser_main.cc ...

9 years, 3 months ago (2011-09-06 20:42:40 UTC) #2

Mike Mammarella

http://codereview.chromium.org/7835021/diff/8001/chrome/browser/browser_main.cc File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/7835021/diff/8001/chrome/browser/browser_main.cc#newcode610 chrome/browser/browser_main.cc:610: // We use glib-dbus for geolocation and it's possible ...

9 years, 3 months ago (2011-09-07 01:01:27 UTC) #3

http://codereview.chromium.org/7835021/diff/8001/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/7835021/diff/8001/chrome/browser/browser_main....
chrome/browser/browser_main.cc:610: // We use glib-dbus for geolocation and it's
possible other libraries
On 2011/09/06 20:42:40, satorux1 wrote:
> Could you update the comment? geolocation has switched to our D-Bus library.

Done.

http://codereview.chromium.org/7835021/diff/8001/chrome/browser/password_mana...
File chrome/browser/password_manager/native_backend_kwallet_x.cc (right):

http://codereview.chromium.org/7835021/diff/8001/chrome/browser/password_mana...
chrome/browser/password_manager/native_backend_kwallet_x.cc:50:
session_bus_->ShutdownAndBlock();
On 2011/09/06 20:42:40, satorux1 wrote:
> This would crash with DCHECK() if D-Bus thread is set.
> ShutdownOnDBusThreadAndBlock() should be used instead if D-Bus thread is in
use.

Ah, one of the perils of code you have never actually run, I suppose. :)
I'm having some problem loading the preferences in my builds and so I can't
actually test this change yet. I'll figure out what's going on and test it
though.

http://codereview.chromium.org/7835021/diff/8001/chrome/browser/password_mana...
chrome/browser/password_manager/native_backend_kwallet_x.cc:59:
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::DB);
On 2011/09/06 20:42:40, satorux1 wrote:
> Why do we use the DB thread? Could you add a comment?
> 
> Looking at the code, all D-Bus calls are issued by CallMethodAndBlock()
> synchronously. Then, you don't need the D-Bus thread. The D-Bus thread is only
> used for asynchronous operations.

Hmm. Now that I think about it more, I think I can actually just do everything
on the DB thread, since I've already moved most of the init work to the DB
thread anyway.

The constructor and Init() will be called on the UI thread, but everything else
(possibly excluding the destructor, hmm) will be called on the DB thread. This
is an external constraint for this code.

http://codereview.chromium.org/7835021/diff/8001/chrome/browser/password_mana...
chrome/browser/password_manager/native_backend_kwallet_x.cc:127: std::string
dbusName;
On 2011/09/06 20:42:40, satorux1 wrote:
> dbus_name

Done. Huh. I wonder where this came from. I'm pretty sure I wouldn't have
written it like that unless I copied it from somewhere, but I can't find where.

http://codereview.chromium.org/7835021/diff/8001/chrome/browser/password_mana...
chrome/browser/password_manager/native_backend_kwallet_x.cc:132: LOG(ERROR) <<
"Error reading response from klauncher to start kwalletd";
On 2011/09/06 20:42:40, satorux1 wrote:
> You could add << response->ToString(). It'll make debugging easier.

Done.

http://codereview.chromium.org/7835021/diff/8001/chrome/browser/password_mana...
chrome/browser/password_manager/native_backend_kwallet_x.cc:142: }
On 2011/09/06 20:42:40, satorux1 wrote:
> Maybe you could merge the two conditionals, by using ToString():
> 
> if (!error.empty() || ret) {
>   LOG(ERROR) <<  "Error launching kwalletd: " << response->ToString();

Done.

http://codereview.chromium.org/7835021/diff/8001/chrome/browser/password_mana...
chrome/browser/password_manager/native_backend_kwallet_x.cc:161: LOG(ERROR) <<
"Error reading response from kwalletd (isEnabled)";
On 2011/09/06 20:42:40, satorux1 wrote:
> Likewise, I'd recommend including response->ToString(). I found it useful for
> debugging. Please consider adding it in other places where it makes sense.

Done.

http://codereview.chromium.org/7835021/diff/8001/chrome/browser/password_mana...
chrome/browser/password_manager/native_backend_kwallet_x.cc:268: // We could
probably also use readEntryList here.
On 2011/09/06 20:42:40, satorux1 wrote:
> What's readEntryList? Is this comment still valid?

It's another possible method call in the KWallet interface, instead of using
entryList and then readEntry on each. But the documentation is pretty sparse on
how to use these in the first place so I'm probably never going to actually make
that change.

Mike Mammarella

OK, fixed an issue with which thread the destructor runs on, and tested that this ...

9 years, 3 months ago (2011-09-10 00:43:24 UTC) #4

satorux1

Mostly looks good to me. Only minor comments: http://codereview.chromium.org/7835021/diff/21002/chrome/browser/password_manager/native_backend_kwallet_x.cc File chrome/browser/password_manager/native_backend_kwallet_x.cc (right): http://codereview.chromium.org/7835021/diff/21002/chrome/browser/password_manager/native_backend_kwallet_x.cc#newcode57 chrome/browser/password_manager/native_backend_kwallet_x.cc:57: NewRunnableMethod( ...

9 years, 3 months ago (2011-09-12 19:28:57 UTC) #6

Mike Mammarella

http://codereview.chromium.org/7835021/diff/21002/chrome/browser/password_manager/native_backend_kwallet_x.cc File chrome/browser/password_manager/native_backend_kwallet_x.cc (right): http://codereview.chromium.org/7835021/diff/21002/chrome/browser/password_manager/native_backend_kwallet_x.cc#newcode57 chrome/browser/password_manager/native_backend_kwallet_x.cc:57: NewRunnableMethod( On 2011/09/12 19:28:57, satorux1 wrote: > base::Bind is ...

9 years, 3 months ago (2011-09-12 21:26:13 UTC) #7

http://codereview.chromium.org/7835021/diff/21002/chrome/browser/password_man...
File chrome/browser/password_manager/native_backend_kwallet_x.cc (right):

http://codereview.chromium.org/7835021/diff/21002/chrome/browser/password_man...
chrome/browser/password_manager/native_backend_kwallet_x.cc:57:
NewRunnableMethod(
On 2011/09/12 19:28:57, satorux1 wrote:
> base::Bind is preferable.

I am inclined to stick with this for now since I know how to use it. I can look
into base::Bind in a follow-up CL: it's not obvious to me how to use it for a
member method, for instance.

http://codereview.chromium.org/7835021/diff/21002/chrome/browser/password_man...
chrome/browser/password_manager/native_backend_kwallet_x.cc:71:
NewRunnableMethod(
On 2011/09/12 19:28:57, satorux1 wrote:
> ditto.

same

http://codereview.chromium.org/7835021/diff/21002/chrome/browser/password_man...
chrome/browser/password_manager/native_backend_kwallet_x.cc:92: if
(optional_bus.get()) {
On 2011/09/12 19:28:57, satorux1 wrote:
> This is a bit tricky. You might want to add a comment like
> 
> // optional_bus is supplied when this function is called for testing.

Done.

http://codereview.chromium.org/7835021/diff/21002/chrome/browser/password_man...
chrome/browser/password_manager/native_backend_kwallet_x.cc:105: *success =
InitWallet() || (StartKWalletd() && InitWallet());
On 2011/09/12 19:28:57, satorux1 wrote:
> I wasn't sure if || had higher precedence than =. Adding parens may make it
> clearer.
> 
> 
> *success = (InitWallet() || (StartKWalletd() && InitWallet()));

Some of the operators (the bitwise ones especially) don't have well-known
precedence, but I was under the impression that most people know that = and
friends are at the bottom of the list (above only the little-used , operator),
but I've added the extra () anyway.

http://codereview.chromium.org/7835021/diff/21002/chrome/browser/password_man...
chrome/browser/password_manager/native_backend_kwallet_x.cc:110: bool
NativeBackendKWallet::StartKWalletd() {
On 2011/09/12 19:28:57, satorux1 wrote:
> You might want to add 
> 
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));

Done.

http://codereview.chromium.org/7835021/diff/21002/chrome/browser/password_man...
chrome/browser/password_manager/native_backend_kwallet_x.cc:154: bool
NativeBackendKWallet::InitWallet() {
On 2011/09/12 19:28:57, satorux1 wrote:
> You might want to add 
> 
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));

Done.

http://codereview.chromium.org/7835021/diff/21002/chrome/browser/password_man...
chrome/browser/password_manager/native_backend_kwallet_x.cc:174: return false;
On 2011/09/12 19:28:57, satorux1 wrote:
> VLOG(1) may be helpful for debugging?

Done.

http://codereview.chromium.org/7835021/diff/21002/chrome/browser/password_man...
chrome/browser/password_manager/native_backend_kwallet_x.cc:272: const
base::Time& delete_end) {
On 2011/09/12 19:28:57, satorux1 wrote:
> You might want to add 
> 
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));

WalletHandle() has such a check, and this method necessarily calls
WalletHandle(). So I don't think we need one here; in the extreme, we could add
such a check to every method everywhere that's only supposed to be called on one
thread, but I think that's probably overkill.

http://codereview.chromium.org/7835021/diff/21002/chrome/browser/password_man...
chrome/browser/password_manager/native_backend_kwallet_x.cc:393: int
wallet_handle) {
On 2011/09/12 19:28:57, satorux1 wrote:
> You might want to add 
> 
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));

Although this method doesn't actually call WalletHandle(), it takes a wallet
handle that came from such a call. I think it's overkill here.

http://codereview.chromium.org/7835021/diff/21002/chrome/browser/password_man...
chrome/browser/password_manager/native_backend_kwallet_x.cc:450: // just saving
a new password within this realm to overwrite it).
On 2011/09/12 19:28:57, satorux1 wrote:
> Worth adding a LOG(WARNING) or something?

CheckSerializedValue() has one.

http://codereview.chromium.org/7835021/diff/21002/chrome/browser/password_man...
chrome/browser/password_manager/native_backend_kwallet_x.cc:505: int
wallet_handle) {
On 2011/09/12 19:28:57, satorux1 wrote:
> You might want to add 
> 
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));

Same reasoning about WalletHandle().

http://codereview.chromium.org/7835021/diff/21002/chrome/browser/password_man...
chrome/browser/password_manager/native_backend_kwallet_x.cc:528: while
(array.HasMoreData()) {
On 2011/09/12 19:28:57, satorux1 wrote:
> This is the second time I see this. I guess it's worth adding
PopArrayOfString()
> in dbus::Message, which is generic enough to be there. If you think it's a
good
> idea, you can add do it in a separate patch.

That would be useful. I'll consider adding one in a follow-up CL.

http://codereview.chromium.org/7835021/diff/21002/chrome/browser/password_man...
chrome/browser/password_manager/native_backend_kwallet_x.cc:575: int
wallet_handle) {
On 2011/09/12 19:28:57, satorux1 wrote:
> You might want to add 
> 
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));

Same.

satorux1

LGTM. Comments below are optional. You can do it in the patch if you want, ...

9 years, 3 months ago (2011-09-12 22:28:21 UTC) #8

LGTM. Comments below are optional. You can do it in the patch if you want, but
it's up to you.

http://codereview.chromium.org/7835021/diff/21002/chrome/browser/password_man...
File chrome/browser/password_manager/native_backend_kwallet_x.cc (right):

http://codereview.chromium.org/7835021/diff/21002/chrome/browser/password_man...
chrome/browser/password_manager/native_backend_kwallet_x.cc:57:
NewRunnableMethod(
On 2011/09/12 21:26:13, Mike Mammarella wrote:
> On 2011/09/12 19:28:57, satorux1 wrote:
> > base::Bind is preferable.
> 
> I am inclined to stick with this for now since I know how to use it. I can
look
> into base::Bind in a follow-up CL: it's not obvious to me how to use it for a
> member method, for instance.

That's fine, but it should be as easy as:

BrowserThread::PostTask(BrowserThread::DB, FROM_HERE,
                        base::Bind(&dbus::Bus::ShutdownAndBlock),
                                   session_bus_));

session_bus_ is a ref counted object so this should compile.

If the object is not a ref counted object, you need to surround it with
base::Unretained() like base::Unretained(this).

http://codereview.chromium.org/7835021/diff/21002/chrome/browser/password_man...
chrome/browser/password_manager/native_backend_kwallet_x.cc:272: const
base::Time& delete_end) {
On 2011/09/12 21:26:13, Mike Mammarella wrote:
> On 2011/09/12 19:28:57, satorux1 wrote:
> > You might want to add 
> > 
> > DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
> 
> WalletHandle() has such a check, and this method necessarily calls
> WalletHandle(). So I don't think we need one here; in the extreme, we could
add
> such a check to every method everywhere that's only supposed to be called on
one
> thread, but I think that's probably overkill.

It's a matter of taste so it's up to you, but I think adding one line DCHECK
won't be overkill.

I'd add that line where we called CallMethodAndBlock() which should only be
called on the DB thread, to make it clear that we are on the right thread to do
this.

Mike Mammarella

+avi for trivial content/browser/browser_main.cc changes

9 years, 3 months ago (2011-09-12 22:41:53 UTC) #9

Mike Mammarella

On 2011/09/12 22:41:53, Mike Mammarella wrote: > +avi for trivial content/browser/browser_main.cc changes Try jam for ...

9 years, 3 months ago (2011-09-14 06:59:16 UTC) #10

content lgtm

Expand Messages | Collapse Messages