cryptohome: Merge FakeCryptohomeClient and CryptohomeClinentStubImpl

cryptohome: Merge FakeCryptohomeClient and CryptohomeClinentStubImpl

The new FakeCryptohomeClient is based on CryptohomeClinentStubImpl.
The only addition is set_unmout_result().

BUG=298729
TEST=the existing tests should pass
R=hashimoto@chromium.org, stevenjb@chromium.org

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

[satorux1, 2013-09-26 08:20:15]

stevenjb:
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc

pneubeck: chrome/browser/chromeos/policy/...

hashimoto: chromeos/dbus/...


As mentioned in the patch description, the new FakeCryptohomeClient is based on
CryptohomeClinentStubImpl. The only addition is set_unmount_result(). The diff
tool was not smart enough to detect this.

[hashimoto, 2013-09-26 08:41:29]

https://codereview.chromium.org/24637004/diff/1/chrome/browser/extensions/api...
File
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc
(right):

https://codereview.chromium.org/24637004/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:81:
// Do the same as FakeCryptohomeClient::GetSanitizedUsername
Did you mean CryptohomeClient::GetSanitizedUsername?
btw, why don't you directly use the static function
CryptohomeClient::GetSanitizedUsername here?

https://codereview.chromium.org/24637004/diff/1/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/24637004/diff/1/chromeos/chromeos.gyp#newcode413
chromeos/chromeos.gyp:413: 'dbus/fake_cryptohome_client.h',
Do we need these?

https://codereview.chromium.org/24637004/diff/1/chromeos/dbus/fake_cryptohome...
File chromeos/dbus/fake_cryptohome_client.cc (right):

https://codereview.chromium.org/24637004/diff/1/chromeos/dbus/fake_cryptohome...
chromeos/dbus/fake_cryptohome_client.cc:198:
std::string(crypto::kTestTPMTokenName),
Linker is complaining about this.

[917/4779] SOLINK lib/libsync.so
FAILED: clang++ -Wl,-z,now -Wl,-z,relro -pthread -Wl,-z,noexecstack -fPIC
-B../../third_party/gold -L. -Wl,-uIsHeapProfilerRunning,-uProfilerStart
-Wl,-u_Z21InitialMallocHook_NewPKvj,-u_Z22InitialMallocHook_MMapPKvS0_jiiix,-u_Z22InitialMallocHook_SbrkPKvi
-Wl,-u_Z21InitialMallocHook_NewPKvm,-u_Z22InitialMallocHook_MMapPKvS0_miiil,-u_Z22InitialMallocHook_SbrkPKvl
-Wl,-u_ZN15HeapLeakChecker12IgnoreObjectEPKv,-u_ZN15HeapLeakChecker14UnIgnoreObjectEPKv
-Wl,-rpath=\$ORIGIN/lib/ -Wl,-rpath-link=lib/ -o device_unittests
-Wl,--start-group
obj/device/bluetooth/device_unittests.bluetooth_chromeos_unittest.o
obj/device/bluetooth/device_unittests.bluetooth_profile_chromeos_unittest.o
obj/device/bluetooth/device_unittests.bluetooth_utils_unittest.o
obj/device/usb/device_unittests.usb_ids_unittest.o
obj/base/librun_all_unittests.a obj/base/libtest_support_base.a
obj/testing/libgmock.a obj/testing/libgtest.a
obj/device/bluetooth/libdevice_bluetooth.a
obj/device/bluetooth/libdevice_bluetooth_mocks.a obj/device/usb/libdevice_usb.a
obj/chromeos/libchromeos_test_support.a
obj/chromeos/libchromeos_test_support_without_gmock.a
obj/base/allocator/liballocator.a obj/base/libbase_static.a
obj/third_party/libxml/libxml2.a obj/third_party/zlib/libchrome_zlib.a
obj/base/third_party/dynamic_annotations/libdynamic_annotations.a
obj/chromeos/libpower_manager_proto.a lib/libui.so lib/libdbus.so lib/libbase.so
lib/libprotobuf_lite.so lib/libnet.so lib/libchromeos.so lib/libbase_i18n.so
lib/libicuuc.so -Wl,--end-group -lX11 -lXcursor -lXrender -lrt -ldl -ldbus-1
-lpthread -lXext -lXi -lXrandr
obj/chromeos/libchromeos_test_support_without_gmock.a(obj/chromeos/dbus/chromeos_test_support_without_gmock.fake_cryptohome_client.o):../../chromeos/dbus/fake_cryptohome_client.cc:function
chromeos::FakeCryptohomeClient::Pkcs11GetTpmTokenInfo(base::Callback<void
(chromeos::DBusMethodCallStatus, std::string const&, std::string const&)>
const&):error: undefined reference to 'crypto::kTestTPMTokenName'
clang:error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.

[satorux1, 2013-09-26 09:58:36]

https://codereview.chromium.org/24637004/diff/1/chrome/browser/extensions/api...
File
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc
(right):

https://codereview.chromium.org/24637004/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:81:
// Do the same as FakeCryptohomeClient::GetSanitizedUsername
On 2013/09/26 08:41:29, hashimoto wrote:
> Did you mean CryptohomeClient::GetSanitizedUsername?
> btw, why don't you directly use the static function
> CryptohomeClient::GetSanitizedUsername here?

Not sure why CryptohomeClient::GetStubSanitizedUsername() is not used here. I
guess this is done this way on purpose. I'd keep this as-is.

https://codereview.chromium.org/24637004/diff/1/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/24637004/diff/1/chromeos/chromeos.gyp#newcode413
chromeos/chromeos.gyp:413: 'dbus/fake_cryptohome_client.h',
On 2013/09/26 08:41:29, hashimoto wrote:
> Do we need these?

I think so. These are used with MockDBusThreadManagerWithoutGMock

https://codereview.chromium.org/24637004/diff/1/chromeos/dbus/fake_cryptohome...
File chromeos/dbus/fake_cryptohome_client.cc (right):

https://codereview.chromium.org/24637004/diff/1/chromeos/dbus/fake_cryptohome...
chromeos/dbus/fake_cryptohome_client.cc:198:
std::string(crypto::kTestTPMTokenName),
On 2013/09/26 08:41:29, hashimoto wrote:
> Linker is complaining about this.
> 
> [917/4779] SOLINK lib/libsync.so
> FAILED: clang++ -Wl,-z,now -Wl,-z,relro -pthread -Wl,-z,noexecstack -fPIC
> -B../../third_party/gold -L. -Wl,-uIsHeapProfilerRunning,-uProfilerStart
>
-Wl,-u_Z21InitialMallocHook_NewPKvj,-u_Z22InitialMallocHook_MMapPKvS0_jiiix,-u_Z22InitialMallocHook_SbrkPKvi
>
-Wl,-u_Z21InitialMallocHook_NewPKvm,-u_Z22InitialMallocHook_MMapPKvS0_miiil,-u_Z22InitialMallocHook_SbrkPKvl
>
-Wl,-u_ZN15HeapLeakChecker12IgnoreObjectEPKv,-u_ZN15HeapLeakChecker14UnIgnoreObjectEPKv
> -Wl,-rpath=\$ORIGIN/lib/ -Wl,-rpath-link=lib/ -o device_unittests
> -Wl,--start-group
> obj/device/bluetooth/device_unittests.bluetooth_chromeos_unittest.o
> obj/device/bluetooth/device_unittests.bluetooth_profile_chromeos_unittest.o
> obj/device/bluetooth/device_unittests.bluetooth_utils_unittest.o
> obj/device/usb/device_unittests.usb_ids_unittest.o
> obj/base/librun_all_unittests.a obj/base/libtest_support_base.a
> obj/testing/libgmock.a obj/testing/libgtest.a
> obj/device/bluetooth/libdevice_bluetooth.a
> obj/device/bluetooth/libdevice_bluetooth_mocks.a
obj/device/usb/libdevice_usb.a
> obj/chromeos/libchromeos_test_support.a
> obj/chromeos/libchromeos_test_support_without_gmock.a
> obj/base/allocator/liballocator.a obj/base/libbase_static.a
> obj/third_party/libxml/libxml2.a obj/third_party/zlib/libchrome_zlib.a
> obj/base/third_party/dynamic_annotations/libdynamic_annotations.a
> obj/chromeos/libpower_manager_proto.a lib/libui.so lib/libdbus.so
lib/libbase.so
> lib/libprotobuf_lite.so lib/libnet.so lib/libchromeos.so lib/libbase_i18n.so
> lib/libicuuc.so -Wl,--end-group -lX11 -lXcursor -lXrender -lrt -ldl -ldbus-1
> -lpthread -lXext -lXi -lXrandr
>
obj/chromeos/libchromeos_test_support_without_gmock.a(obj/chromeos/dbus/chromeos_test_support_without_gmock.fake_cryptohome_client.o):../../chromeos/dbus/fake_cryptohome_client.cc:function
> chromeos::FakeCryptohomeClient::Pkcs11GetTpmTokenInfo(base::Callback<void
> (chromeos::DBusMethodCallStatus, std::string const&, std::string const&)>
> const&):error: undefined reference to 'crypto::kTestTPMTokenName'
> clang:error: linker command failed with exit code 1 (use -v to see invocation)
> ninja: build stopped: subcommand failed.

Fixed in Patch Set 2.

[satorux1, 2013-09-26 09:59:58]

Failures in PowerPolicyInSessionBrowserTest.AllowScreenWakeLocks and
PowerPolicyInSessionBrowserTest.SetUserPolicy seem to be valid. I'll take a look
tomorrow.

[pneubeck (no reviews), 2013-09-26 12:47:18]

https://codereview.chromium.org/24637004/diff/4001/chrome/browser/extensions/...
File
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc
(right):

https://codereview.chromium.org/24637004/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:82:
std::string sanitized_user = login_user + kUserIdStubHashSuffix;
I think, I did this. I'm not sure whether the static function
GetStubSanitizedUsername existed already at that time.
Please modify as Ryo suggested (using the GetStubSanitizedUsername function).

[stevenjb, 2013-09-26 17:40:36]

Nice. lgtm.

[satorux1, 2013-09-27 02:31:01]

https://codereview.chromium.org/24637004/diff/4001/chrome/browser/extensions/...
File
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc
(right):

https://codereview.chromium.org/24637004/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:82:
std::string sanitized_user = login_user + kUserIdStubHashSuffix;
On 2013/09/26 12:47:19, pneubeck wrote:
> I think, I did this. I'm not sure whether the static function
> GetStubSanitizedUsername existed already at that time.
> Please modify as Ryo suggested (using the GetStubSanitizedUsername function).

Done.

https://codereview.chromium.org/24637004/diff/25001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/power_policy_browsertest.cc (right):

https://codereview.chromium.org/24637004/diff/25001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/power_policy_browsertest.cc:177:
chromeos::UserManager::kStubUser);
This change was needed as the new FakeCryptohomeClient now returns a sanitized
username, which is a more correct behavior.

[hashimoto, 2013-09-27 04:07:10]

https://codereview.chromium.org/24637004/diff/1/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/24637004/diff/1/chromeos/chromeos.gyp#newcode413
chromeos/chromeos.gyp:413: 'dbus/fake_cryptohome_client.h',
On 2013/09/26 09:58:36, satorux1 wrote:
> On 2013/09/26 08:41:29, hashimoto wrote:
> > Do we need these?
> 
> I think so. These are used with MockDBusThreadManagerWithoutGMock

You added fake_cryptohome_client.cc/h to the target 'chromeos' and this
'chromeos_test_support_without_gmock' target has a dependency to 'chromeos'.
I thought we could remove fake_cryptohome_client.cc/h from this target.
Doesn't this work?

[satorux1, 2013-09-27 04:12:37]

https://codereview.chromium.org/24637004/diff/1/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/24637004/diff/1/chromeos/chromeos.gyp#newcode413
chromeos/chromeos.gyp:413: 'dbus/fake_cryptohome_client.h',
On 2013/09/27 04:07:11, hashimoto wrote:
> On 2013/09/26 09:58:36, satorux1 wrote:
> > On 2013/09/26 08:41:29, hashimoto wrote:
> > > Do we need these?
> > 
> > I think so. These are used with MockDBusThreadManagerWithoutGMock
> 
> You added fake_cryptohome_client.cc/h to the target 'chromeos' and this
> 'chromeos_test_support_without_gmock' target has a dependency to 'chromeos'.
> I thought we could remove fake_cryptohome_client.cc/h from this target.
> Doesn't this work?

Good point. Removed. Removed some other stuff too.

[hashimoto, 2013-09-27 04:18:22]

lgtm

[satorux1, 2013-09-27 05:11:18]

Committed patchset #6 manually as r225664 (presubmit successful).