Issue 11437025: Linux: add library loader for GNOME keyring.

Issue 11437025: Linux: add library loader for GNOME keyring. (Closed)

Created:
8 years ago by Paweł Hajdan Jr.

Modified:
8 years ago

Reviewers:
Mike Mammarella, stuartmorgan

CC:
chromium-reviews, Ilya Sherman

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

Visibility:
Public.

More Reviews

Description

Linux: add library loader for GNOME keyring. BUG=162733 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171573

Patch Set 2 : fix for bots #

Created: 8 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+135 lines, -172 lines)			Patch
M	build/linux/system.gyp	View		2 chunks	+49 lines, -18 lines	0 comments	Download
M	chrome/browser/password_manager/native_backend_gnome_x.h	View		4 chunks	+11 lines, -45 lines	0 comments	Download
M	chrome/browser/password_manager/native_backend_gnome_x.cc	View		27 chunks	+43 lines, -90 lines	0 comments	Download
M	chrome/browser/password_manager/native_backend_gnome_x_unittest.cc	View	1	20 chunks	+32 lines, -19 lines	0 comments	Download

Messages

Total messages: 7 (0 generated)

Expand Messages | Collapse Messages

Ilya Sherman

Sorry, I don't understand this code well enough to review this change. Perhaps Mike or ...

8 years ago (2012-12-06 00:19:59 UTC) #2

Paweł Hajdan Jr.

https://codereview.chromium.org/11437025/diff/1/chrome/browser/password_manager/native_backend_gnome_x.h File chrome/browser/password_manager/native_backend_gnome_x.h (right): https://codereview.chromium.org/11437025/diff/1/chrome/browser/password_manager/native_backend_gnome_x.h#newcode16 chrome/browser/password_manager/native_backend_gnome_x.h:16: #include "library_loaders/libgnome-keyring.h" On 2012/12/06 00:19:59, Ilya Sherman wrote: > ...

8 years ago (2012-12-06 00:32:47 UTC) #3

Mike Mammarella

https://codereview.chromium.org/11437025/diff/1/chrome/browser/password_manager/native_backend_gnome_x_unittest.cc File chrome/browser/password_manager/native_backend_gnome_x_unittest.cc (right): https://codereview.chromium.org/11437025/diff/1/chrome/browser/password_manager/native_backend_gnome_x_unittest.cc#newcode393 chrome/browser/password_manager/native_backend_gnome_x_unittest.cc:393: ASSERT_TRUE(backend.Init()); I think on the bots gnome_keyring_is_available() might be ...

8 years ago (2012-12-06 04:29:56 UTC) #5

Paweł Hajdan Jr.

https://codereview.chromium.org/11437025/diff/1/chrome/browser/password_manager/native_backend_gnome_x_unittest.cc File chrome/browser/password_manager/native_backend_gnome_x_unittest.cc (right): https://codereview.chromium.org/11437025/diff/1/chrome/browser/password_manager/native_backend_gnome_x_unittest.cc#newcode393 chrome/browser/password_manager/native_backend_gnome_x_unittest.cc:393: ASSERT_TRUE(backend.Init()); On 2012/12/06 04:29:56, Mike Mammarella wrote: > I ...

8 years ago (2012-12-06 18:33:12 UTC) #6

Mike Mammarella

8 years ago (2012-12-06 20:37:30 UTC) #7

Message was sent while issue was closed.

LGTM

https://codereview.chromium.org/11437025/diff/1/chrome/browser/password_manag...
File chrome/browser/password_manager/native_backend_gnome_x_unittest.cc (right):

https://codereview.chromium.org/11437025/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/native_backend_gnome_x_unittest.cc:393:
ASSERT_TRUE(backend.Init());
On 2012/12/06 18:33:12, Paweł Hajdan Jr. wrote:
> On 2012/12/06 04:29:56, Mike Mammarella wrote:
> > I think on the bots gnome_keyring_is_available() might be returning false,
so
> > you can't call Init() before SetUpMockKeyring(). (In the old code, we set
the
> > global bool during SetUp() above, which prevents any loading from
occurring.)
> 
> Ah, you're right. Updated.

Heh, that's a pretty cheezy way to fix it, just allow Init() to return false.
Before it was actually returning true, though we weren't testing for it, but I
guess this is OK since if it fails "for real" then the rest of this test will
crash and burn anyway.

Expand Messages | Collapse Messages