Remove chromeos::CryptohomeLibrary::IsMounted and convert

Remove chromeos::CryptohomeLibrary::IsMounted

CryptohomeClient::IsMounted is converted to asynchronous and used instead.
Mock's default action for IsMounted is set in the ctor of MockCryptohomeClient.

BUG=126674
TEST=git try -b linux_chromeos


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

[hashimoto, 2012-07-23 10:58:50]

Steven, 
Could you review this change?

David,
Could you review profile_manager.cc?

[stevenjb, 2012-07-23 17:42:50]

http://codereview.chromium.org/10817007/diff/2001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/10817007/diff/2001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:169: CHECK(call_status ==
chromeos::DBUS_METHOD_CALL_SUCCESS && is_mounted);
This shouldn't be a CHECK - it *could* fail. We should LOG(ERROR) instead,
possibly LOG(FATAL) if other behavior/assumptions would break badly (might need
to check with folks working on cryptohome in that case). We should also
differentiate between the DBus call failing and is_mounted being false (at least
for the error message).

[hashimoto, 2012-07-24 01:02:58]

http://codereview.chromium.org/10817007/diff/2001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/10817007/diff/2001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:169: CHECK(call_status ==
chromeos::DBUS_METHOD_CALL_SUCCESS && is_mounted);
On 2012/07/23 17:42:50, stevenjb (chromium) wrote:
> This shouldn't be a CHECK - it *could* fail. We should LOG(ERROR) instead,
> possibly LOG(FATAL) if other behavior/assumptions would break badly (might
need
> to check with folks working on cryptohome in that case). We should also
> differentiate between the DBus call failing and is_mounted being false (at
least
> for the error message).
Done.
"CHECK(!GetProfileByPath(default_profile_dir))" below the IsMounted() call
should be also replaced with LOG(ERROR)?

[stevenjb, 2012-07-24 16:46:36]

lgtm

http://codereview.chromium.org/10817007/diff/2001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/10817007/diff/2001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:169: CHECK(call_status ==
chromeos::DBUS_METHOD_CALL_SUCCESS && is_mounted);
On 2012/07/24 01:02:58, hashimoto wrote:
> On 2012/07/23 17:42:50, stevenjb (chromium) wrote:
> > This shouldn't be a CHECK - it *could* fail. We should LOG(ERROR) instead,
> > possibly LOG(FATAL) if other behavior/assumptions would break badly (might
> need
> > to check with folks working on cryptohome in that case). We should also
> > differentiate between the DBus call failing and is_mounted being false (at
> least
> > for the error message).
> Done.
> "CHECK(!GetProfileByPath(default_profile_dir))" below the IsMounted() call
> should be also replaced with LOG(ERROR)?

I don't think that getting the profile relies on external services? Even if it
does, I expect that it would be unrecoverable, so CHECK seems fine there. DBus
calls on the other hand could fail for a host of reasons and may be recoverable
(e.g. with a re-try), so we should only CHECK on their success if we would crash
later anyway.

[hashimoto, 2012-07-25 11:50:49]

David, could you review this change as a browser/profiles owner?
Evan, could you review this change as a chrome/browser/ui/webui owner?

[hashimoto, 2012-07-26 01:43:21]

Sending for the correct address
Evan, could you take a look?

[Evan Stade, 2012-07-26 04:04:08]

webui lgtm

[commit-bot: I haz the power, 2012-07-26 05:06:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/10817007/9001

[commit-bot: I haz the power, 2012-07-26 05:06:20]

Presubmit check for 10817007-9001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/chromeos/login
    chrome/browser/profiles

Presubmit checks took 1.5s to calculate.

[hashimoto, 2012-07-26 05:20:00]

Nikita, could you review chrome/browser/login changes as an owner?

[DaveMoore, 2012-07-27 14:19:39]

LGTM

Is it possible now to add the check that it's mounted as the proper user?

[hashimoto, 2012-07-27 14:38:29]

On 2012/07/27 14:19:39, DaveMoore wrote:
> LGTM
> 
> Is it possible now to add the check that it's mounted as the proper user?

Looks like now Cryptohome service supports IsMountedForUser() method and it may
be what we want. (though I'm not sure about what it exactly does.)
It may not be hard to call IsMountedForUser() instead of IsMounted() when the
username is given.
(http://git.chromium.org/gitweb/?p=chromiumos/platform/cryptohome.git;a=blob;f...)

[Nikita (slow), 2012-07-27 22:06:46]

lgtm with one comment

http://codereview.chromium.org/10817007/diff/9001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc
(left):

http://codereview.chromium.org/10817007/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:131:
EXPECT_CALL(*mock_cryptohome_library_, IsMounted())
How do we check this expectation now?

[hashimoto, 2012-07-28 02:56:37]

http://codereview.chromium.org/10817007/diff/9001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc
(left):

http://codereview.chromium.org/10817007/diff/9001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:131:
EXPECT_CALL(*mock_cryptohome_library_, IsMounted())
On 2012/07/27 22:06:46, Nikita Kostylev wrote:
> How do we check this expectation now?
This expectation is set in the ctor of MockCryptohomeClient now.
Anyway, IsMounted() is only used in ProfileManager for check purpose, this has
nothing to do with the actual login functionality.

[commit-bot: I haz the power, 2012-07-28 02:56:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/10817007/9001

[commit-bot: I haz the power, 2012-07-28 04:14:59]

Change committed as 148883