Added switch that let us 'un-stub' certain dbus clients.

Added switch that let us 'un-stub' certain dbus clients.

Usage:

chrome --dbus-stub --dbus-unstub-clients=client_1[,client_2[,...]]

where <client_n> is in from the set of (bluetooth, cras, cros_disks,
cryptohome, debug_daemon, easy_unlock, lorgnette_manager, shill, gsm_sms, imageburner,
introspectable, modem_messaging, nfc, permission_broker, power_manager, 
session_manager, sms, system_clock, update_engine)

BUG=401192
TEST=manual + DBusClientBundle.UnstubFlagParser

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

[satorux1, 2014-08-07 20:00:06]

+stevenjb@ and +hashimoto@ as I'll be ooo on Friday.

https://codereview.chromium.org/444263002/diff/20001/chromeos/dbus/dbus_threa...
File chromeos/dbus/dbus_thread_manager.cc (right):

https://codereview.chromium.org/444263002/diff/20001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.cc:68: BluetoothLowEnergy = 1 << 2,
Per http://www.chromium.org/developers/coding-style#Naming please rename enums
to look like UNKNOWN and BLUETOOTH_LOW_ENERGY

https://codereview.chromium.org/444263002/diff/20001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.cc:336: explicit DBusThreadManagerImpl(int
client_mask) : client_mask_(client_mask) {
please document |client_mask|

https://codereview.chromium.org/444263002/diff/20001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.cc:554: class MixedDBusThreadManager : public
DBusThreadManager {
Class comment is missing.

https://codereview.chromium.org/444263002/diff/20001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.cc:569: static
DBusClientBundle::DBusClientType GetDBusClientType(
function comment is missing.

https://codereview.chromium.org/444263002/diff/20001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.cc:572: void ParseUnstubList(const
std::string& unstub_list) {
function comment is missing.

https://codereview.chromium.org/444263002/diff/20001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.cc:582: unstub_mask_ |= client;
instead of modifying unstub_mask_ directly, return 'int' from this function?
Then, this function becomes easily unit testable. Maybe make this and enums
public and add a unit test?

https://codereview.chromium.org/444263002/diff/20001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.cc:849: bool force_bluetooth_client =
CommandLine::ForCurrentProcess()->HasSwitch(
force_bluetooth_client  -> unstub_clients ?

https://codereview.chromium.org/444263002/diff/20001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.cc:853: if (!use_dbus_stub) {
slightly easier to read with:

if (unstub_clients) {
  InitializeWithPartialStub()
} else if (use_dbus_stub) {
  InitializeWithStub();
} else {
  InitializeClients();
}

[stevenjb, 2014-08-07 20:20:32]

https://codereview.chromium.org/444263002/diff/20001/chromeos/dbus/dbus_threa...
File chromeos/dbus/dbus_thread_manager.cc (right):

https://codereview.chromium.org/444263002/diff/20001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.cc:821: 
This class is large enough that we should really put it in a separate file.

https://codereview.chromium.org/444263002/diff/20001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.cc:824:
MixedDBusThreadManager::GetDBusClientType(const std::string& client_type) {
This looks like it could just be a local helper function?

[zel, 2014-08-07 21:40:45]

https://codereview.chromium.org/444263002/diff/20001/chromeos/dbus/dbus_threa...
File chromeos/dbus/dbus_thread_manager.cc (right):

https://codereview.chromium.org/444263002/diff/20001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.cc:68: BluetoothLowEnergy = 1 << 2,
On 2014/08/07 20:00:05, satorux1 wrote:
> Per http://www.chromium.org/developers/coding-style#Naming please rename enums
> to look like UNKNOWN and BLUETOOTH_LOW_ENERGY

Done.

https://codereview.chromium.org/444263002/diff/20001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.cc:554: class MixedDBusThreadManager : public
DBusThreadManager {
On 2014/08/07 20:00:05, satorux1 wrote:
> Class comment is missing.

Done.

https://codereview.chromium.org/444263002/diff/20001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.cc:569: static
DBusClientBundle::DBusClientType GetDBusClientType(
On 2014/08/07 20:00:05, satorux1 wrote:
> function comment is missing.

Done.

https://codereview.chromium.org/444263002/diff/20001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.cc:572: void ParseUnstubList(const
std::string& unstub_list) {
On 2014/08/07 20:00:05, satorux1 wrote:
> function comment is missing.

Done.

https://codereview.chromium.org/444263002/diff/20001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.cc:821: 
On 2014/08/07 20:20:31, stevenjb wrote:
> This class is large enough that we should really put it in a separate file.

so does DBusClientBundle... done.

https://codereview.chromium.org/444263002/diff/20001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.cc:849: bool force_bluetooth_client =
CommandLine::ForCurrentProcess()->HasSwitch(
On 2014/08/07 20:00:05, satorux1 wrote:
> force_bluetooth_client  -> unstub_clients ?

Done.

https://codereview.chromium.org/444263002/diff/20001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.cc:853: if (!use_dbus_stub) {
On 2014/08/07 20:00:05, satorux1 wrote:
> slightly easier to read with:
> 
> if (unstub_clients) {
>   InitializeWithPartialStub()
> } else if (use_dbus_stub) {
>   InitializeWithStub();
> } else {
>   InitializeClients();
> }

Done.

[stevenjb, 2014-08-07 21:58:48]

lgtm w/ one nit

https://codereview.chromium.org/444263002/diff/60001/chromeos/dbus/mixed_dbus...
File chromeos/dbus/mixed_dbus_thread_manager.cc (right):

https://codereview.chromium.org/444263002/diff/60001/chromeos/dbus/mixed_dbus...
chromeos/dbus/mixed_dbus_thread_manager.cc:23:
real_thread_manager_->AddObserver(observer);
It doesn't look like DBusThreadManagerObserver is being used (anymore?).
Otherwise this could be incorrect since OnDBusThreadManagerDestroying passes the
DBusThreadManager instance to its observers.

At minimum we should add a TODO: Unused, remove here and file an issue to do so.

[zel, 2014-08-07 22:36:57]

https://codereview.chromium.org/444263002/diff/60001/chromeos/dbus/mixed_dbus...
File chromeos/dbus/mixed_dbus_thread_manager.cc (right):

https://codereview.chromium.org/444263002/diff/60001/chromeos/dbus/mixed_dbus...
chromeos/dbus/mixed_dbus_thread_manager.cc:23:
real_thread_manager_->AddObserver(observer);
On 2014/08/07 21:58:48, stevenjb wrote:
> It doesn't look like DBusThreadManagerObserver is being used (anymore?).
> Otherwise this could be incorrect since OnDBusThreadManagerDestroying passes
the
> DBusThreadManager instance to its observers.
> 
> At minimum we should add a TODO: Unused, remove here and file an issue to do
so.

Yeah - that's exactly why I can't instantiate all dbus clients in
DBusClientBundle::DBusClientBundle(). Only those that are instantiated will
receive OnDBusThreadManagerDestroying, so this is working correctly now.

[zel, 2014-08-07 23:16:18]

https://codereview.chromium.org/444263002/diff/20001/chromeos/dbus/dbus_threa...
File chromeos/dbus/dbus_thread_manager.cc (right):

https://codereview.chromium.org/444263002/diff/20001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.cc:582: unstub_mask_ |= client;
On 2014/08/07 20:00:05, satorux1 wrote:
> instead of modifying unstub_mask_ directly, return 'int' from this function?
> Then, this function becomes easily unit testable. Maybe make this and enums
> public and add a unit test?

Done.

[zel, 2014-08-08 01:06:28]

The CQ bit was checked by zelidrag@chromium.org

[commit-bot: I haz the power, 2014-08-08 01:13:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zelidrag@chromium.org/444263002/120001

[chromium-reviews, 2014-08-08 02:06:11]

I only got part way through trying this patch today using
bluez_5.21-2_amd64.deb
<http://ftp.us.debian.org/debian/pool/main/b/bluez/bluez_5.21-2_amd64.deb>
and I haven't seen it fully working yet. Chrome crashed after some call to
the bluetooth API, but I'm not sure which one. Tomorrow I will dig a bit
deeper and see what is going on.
On Thu Aug 07 2014 at 6:13:27 PM <commit-bot@chromium.org> wrote:

> CQ is trying da patch. Follow status at
>
> https://chromium-status.appspot.com/cq/zelidrag@
> chromium.org/444263002/120001
>
>
> https://codereview.chromium.org/444263002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hashimoto, 2014-08-08 02:06:16]

https://codereview.chromium.org/444263002/diff/120001/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/444263002/diff/120001/chromeos/chromeos.gyp#n...
chromeos/chromeos.gyp:118: 'dbus/dbus_client_bundle.h',
nit: sort

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_client_bundle.cc (right):

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.cc:122:
cryptohome_client_.reset(CryptohomeClient::Create());
nit: indent.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.cc:125:
debug_daemon_client_.reset(DebugDaemonClient::Create());
ditto.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.cc:128:
easy_unlock_client_.reset(EasyUnlockClient::Create());
ditto.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.cc:162: nfc_tag_client_.get()));
nit: indent.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.cc:199: }
nit: How about emitting an ERROR when client==UNKNOWN?

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.cc:206: return bluetooth_adapter_client_.get();
How about inlining these getters in the header?

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_client_bundle.h (right):

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.h:79: All =                  0xFFFFFFFF,
nit: This should be "ALL".

Also, 0xFFFFFFFF should be ~static_cast<[typedef-ed name of unsigned int]>(0) so
that this value becomes 64-bit when the type is changed to accommodate more than
32 clients.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.h:82: explicit DBusClientBundle(unsigned int
client_mask);
This "unsigned int" should be typedef-ed so that we can easily change the type
to uint64 when the number of clients exceeds 32.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.h:158: 
nit: Unneeded blank line.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_thre...
File chromeos/dbus/dbus_thread_manager.cc (right):

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_thre...
chromeos/dbus/dbus_thread_manager.cc:64: explicit DBusThreadManagerImpl(int
client_mask) : client_mask_(client_mask) {
The bundle class is using unsigned int.
Please use the same type.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_thre...
chromeos/dbus/dbus_thread_manager.cc:283: 
nit: unneeded blank line.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_thre...
chromeos/dbus/dbus_thread_manager.cc:351: DBusThreadManager* real_thread_manager
=
How about performing these initialization in MixedDBusThreadManager's ctor?

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_thre...
File chromeos/dbus/dbus_thread_manager.h (right):

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_thre...
chromeos/dbus/dbus_thread_manager.h:105: static void InitializeRegular();
Do InitilizeRegular() and InitializeWithPartialStub() need to be public?
If not, please make them private.

[zel, 2014-08-08 02:25:45]

The CQ bit was unchecked by zelidrag@chromium.org

[hashimoto, 2014-08-08 07:08:45]

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_client_bundle.cc (right):

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.cc:181:
update_engine_client_.reset(UpdateEngineClient::Create(type));
It seems POWER_POLICY is not handled.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_client_bundle.h (right):

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.h:56: enum DBusClientType {
Do we need all of these clients?
If not, maybe we should start from the minimum set of clients we really need.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/mixed_dbu...
File chromeos/dbus/mixed_dbus_thread_manager.cc (right):

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/mixed_dbu...
chromeos/dbus/mixed_dbus_thread_manager.cc:22: void
MixedDBusThreadManager::AddObserver(DBusThreadManagerObserver* observer) {
nit: Please fix indents here and below.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/mixed_dbu...
chromeos/dbus/mixed_dbus_thread_manager.cc:54:
DBusClientBundle::BLUETOOTH)->GetBluetoothGattCharacteristicClient();
Shouldn't this be BLUETOOTH_LOW_ENERGY?

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/mixed_dbu...
chromeos/dbus/mixed_dbus_thread_manager.cc:146:
DBusClientBundle::IMAGE_BURNER)->GetIntrospectableClient();
Shouldn't this be INTROSPECTABLE?

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/mixed_dbu...
File chromeos/dbus/mixed_dbus_thread_manager.h (right):

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/mixed_dbu...
chromeos/dbus/mixed_dbus_thread_manager.h:18: class CHROMEOS_EXPORT
MixedDBusThreadManager : public DBusThreadManager {
Instead of introducing MixedDBusThreadManager, can't we just use
FakeDBusThreadManager?
You can use SetAAAClient() methods to set real client implementations to
FakeDBusThreadManager.

[zel, 2014-08-08 19:00:15]

https://codereview.chromium.org/444263002/diff/120001/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/444263002/diff/120001/chromeos/chromeos.gyp#n...
chromeos/chromeos.gyp:118: 'dbus/dbus_client_bundle.h',
On 2014/08/08 02:06:15, hashimoto wrote:
> nit: sort

Done.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_client_bundle.cc (right):

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.cc:122:
cryptohome_client_.reset(CryptohomeClient::Create());
On 2014/08/08 02:06:15, hashimoto wrote:
> nit: indent.

Done.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.cc:125:
debug_daemon_client_.reset(DebugDaemonClient::Create());
On 2014/08/08 02:06:15, hashimoto wrote:
> ditto.

Done.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.cc:128:
easy_unlock_client_.reset(EasyUnlockClient::Create());
On 2014/08/08 02:06:15, hashimoto wrote:
> ditto.

Done.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.cc:162: nfc_tag_client_.get()));
On 2014/08/08 02:06:15, hashimoto wrote:
> nit: indent.

Done.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.cc:181:
update_engine_client_.reset(UpdateEngineClient::Create(type));
On 2014/08/08 07:08:44, hashimoto wrote:
> It seems POWER_POLICY is not handled.

POWER_POLICY client is for some reasons owned in DBusThreadManagerImpl and
handled in  DBusThreadManagerImpl::CreateDefaultClients(). I've just kept it
there even though I am sure if it has legit reasons to stay there at all.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.cc:199: }
On 2014/08/08 02:06:15, hashimoto wrote:
> nit: How about emitting an ERROR when client==UNKNOWN?

Done.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.cc:206: return bluetooth_adapter_client_.get();
On 2014/08/08 02:06:15, hashimoto wrote:
> How about inlining these getters in the header?

Done.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_client_bundle.h (right):

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.h:56: enum DBusClientType {
On 2014/08/08 07:08:44, hashimoto wrote:
> Do we need all of these clients?
> If not, maybe we should start from the minimum set of clients we really need.

Yes, we have to define them all since in DBusClientBundle I need to be able to
prevent creation of those clients that are covered with "fakes" - otherwise I
run into crash on thread manager destruction.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.h:79: All =                  0xFFFFFFFF,
On 2014/08/08 02:06:15, hashimoto wrote:
> nit: This should be "ALL".
> 
> Also, 0xFFFFFFFF should be ~static_cast<[typedef-ed name of unsigned int]>(0)
so
> that this value becomes 64-bit when the type is changed to accommodate more
than
> 32 clients.

Done.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.h:82: explicit DBusClientBundle(unsigned int
client_mask);
On 2014/08/08 02:06:15, hashimoto wrote:
> This "unsigned int" should be typedef-ed so that we can easily change the type
> to uint64 when the number of clients exceeds 32.

Done.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.h:158: 
On 2014/08/08 02:06:15, hashimoto wrote:
> nit: Unneeded blank line.

Done.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_thre...
File chromeos/dbus/dbus_thread_manager.cc (right):

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_thre...
chromeos/dbus/dbus_thread_manager.cc:64: explicit DBusThreadManagerImpl(int
client_mask) : client_mask_(client_mask) {
On 2014/08/08 02:06:15, hashimoto wrote:
> The bundle class is using unsigned int.
> Please use the same type.

Good catch. Done.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_thre...
chromeos/dbus/dbus_thread_manager.cc:283: 
On 2014/08/08 02:06:15, hashimoto wrote:
> nit: unneeded blank line.

Done.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_thre...
chromeos/dbus/dbus_thread_manager.cc:351: DBusThreadManager* real_thread_manager
=
On 2014/08/08 02:06:15, hashimoto wrote:
> How about performing these initialization in MixedDBusThreadManager's ctor?

It was there initially, I had to move it out once I've split these files apart -
otherwise I will need to create an additional header file for
DBusThreadManagerImpl as well.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_thre...
File chromeos/dbus/dbus_thread_manager.h (right):

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_thre...
chromeos/dbus/dbus_thread_manager.h:105: static void InitializeRegular();
On 2014/08/08 02:06:16, hashimoto wrote:
> Do InitilizeRegular() and InitializeWithPartialStub() need to be public?
> If not, please make them private.

Done.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/mixed_dbu...
File chromeos/dbus/mixed_dbus_thread_manager.cc (right):

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/mixed_dbu...
chromeos/dbus/mixed_dbus_thread_manager.cc:54:
DBusClientBundle::BLUETOOTH)->GetBluetoothGattCharacteristicClient();
On 2014/08/08 07:08:44, hashimoto wrote:
> Shouldn't this be BLUETOOTH_LOW_ENERGY?

Yes. Good catch!

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/mixed_dbu...
chromeos/dbus/mixed_dbus_thread_manager.cc:146:
DBusClientBundle::IMAGE_BURNER)->GetIntrospectableClient();
On 2014/08/08 07:08:44, hashimoto wrote:
> Shouldn't this be INTROSPECTABLE?

Done.

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/mixed_dbu...
File chromeos/dbus/mixed_dbus_thread_manager.h (right):

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/mixed_dbu...
chromeos/dbus/mixed_dbus_thread_manager.h:18: class CHROMEOS_EXPORT
MixedDBusThreadManager : public DBusThreadManager {
On 2014/08/08 07:08:44, hashimoto wrote:
> Instead of introducing MixedDBusThreadManager, can't we just use
> FakeDBusThreadManager?
> You can use SetAAAClient() methods to set real client implementations to
> FakeDBusThreadManager.

I am not sure about that. DBusThreadManagerImpl seems be doing way more stuff
than FakeDBusThreadManager. For example, it sets up and controls the lifetime of
the dbus thread.

[satorux1, 2014-08-11 01:52:10]

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_client_bundle.cc (right):

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.cc:181:
update_engine_client_.reset(UpdateEngineClient::Create(type));
On 2014/08/08 19:00:14, zel wrote:
> On 2014/08/08 07:08:44, hashimoto wrote:
> > It seems POWER_POLICY is not handled.
> 
> POWER_POLICY client is for some reasons owned in DBusThreadManagerImpl and
> handled in  DBusThreadManagerImpl::CreateDefaultClients(). I've just kept it
> there even though I am sure if it has legit reasons to stay there at all.

hashimoto: good catch: Let's remove POWER_POLICY. PowerPolicyController is not a
D-Bus client and shouldn't belong to DBusThreadManager:
https://code.google.com/p/chromium/issues/detail?id=345586

In CreateDefaultClients(), I think you can check POWER_MANAGER instead.

https://codereview.chromium.org/444263002/diff/200001/chromeos/chromeos_switc...
File chromeos/chromeos_switches.cc (right):

https://codereview.chromium.org/444263002/diff/200001/chromeos/chromeos_switc...
chromeos/chromeos_switches.cc:24: // Comma-spearated list of dbus clients that
should be unstubbed.
maybe add a reference like:

// See chromeos/dbus/dbus_client_bundle.cc for the names of the dbus clients.

https://codereview.chromium.org/444263002/diff/200001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_client_bundle.cc (right):

https://codereview.chromium.org/444263002/diff/200001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.cc:57: { "bluetoothlowenergy", 
DBusClientBundle::BLUETOOTH_LOW_ENERGY },
bluetooth_low_energy to be consistent with the enum?

https://codereview.chromium.org/444263002/diff/200001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_client_bundle.h (right):

https://codereview.chromium.org/444263002/diff/200001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.h:81: ALL_CLIENTS =          0xFFFFFFFF,
maybe: 1 << 21 - 1

https://codereview.chromium.org/444263002/diff/200001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_client_bundle_unittest.cc (right):

https://codereview.chromium.org/444263002/diff/200001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle_unittest.cc:22: TEST_F(DBusClientBundleTest,
UnstubFlagParser) {
You can use TEST() and remove DBusClientBundleTest class

[hashimoto, 2014-08-11 11:01:05]

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/mixed_dbu...
File chromeos/dbus/mixed_dbus_thread_manager.h (right):

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/mixed_dbu...
chromeos/dbus/mixed_dbus_thread_manager.h:18: class CHROMEOS_EXPORT
MixedDBusThreadManager : public DBusThreadManager {
On 2014/08/08 19:00:15, zel wrote:
> On 2014/08/08 07:08:44, hashimoto wrote:
> > Instead of introducing MixedDBusThreadManager, can't we just use
> > FakeDBusThreadManager?
> > You can use SetAAAClient() methods to set real client implementations to
> > FakeDBusThreadManager.
> 
> I am not sure about that. DBusThreadManagerImpl seems be doing way more stuff
> than FakeDBusThreadManager. For example, it sets up and controls the lifetime
of
> the dbus thread. 

My primary concern here is that we have to maintain fragile <DBusClientType =>
each client> mapping in multiple places.
When developers want to change the mapping (e.g. adding a new value to
DBusClientType, changing how clients are grouped),
they have to carefully change DBusClientBundle's ctor and getters in
MixedDBusThreadManager (and Create() methods of bluetooth client classes),
otherwise the code gets completely broken.

Though it seems FakeDBusThreadManager cannot be used here as you said.
How about initializing clients with fakes in DBusClientBundle()'s ctor if
IsUsingStub() returns true?

[zel, 2014-08-12 01:41:01]

On 2014/08/11 11:01:05, hashimoto wrote:
>
https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/mixed_dbu...
> File chromeos/dbus/mixed_dbus_thread_manager.h (right):
> 
>
https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/mixed_dbu...
> chromeos/dbus/mixed_dbus_thread_manager.h:18: class CHROMEOS_EXPORT
> MixedDBusThreadManager : public DBusThreadManager {
> On 2014/08/08 19:00:15, zel wrote:
> > On 2014/08/08 07:08:44, hashimoto wrote:
> > > Instead of introducing MixedDBusThreadManager, can't we just use
> > > FakeDBusThreadManager?
> > > You can use SetAAAClient() methods to set real client implementations to
> > > FakeDBusThreadManager.
> > 
> > I am not sure about that. DBusThreadManagerImpl seems be doing way more
stuff
> > than FakeDBusThreadManager. For example, it sets up and controls the
lifetime
> of
> > the dbus thread. 
> 
> My primary concern here is that we have to maintain fragile <DBusClientType =>
> each client> mapping in multiple places.
> When developers want to change the mapping (e.g. adding a new value to
> DBusClientType, changing how clients are grouped),
> they have to carefully change DBusClientBundle's ctor and getters in
> MixedDBusThreadManager (and Create() methods of bluetooth client classes),
> otherwise the code gets completely broken.
> 
> Though it seems FakeDBusThreadManager cannot be used here as you said.
> How about initializing clients with fakes in DBusClientBundle()'s ctor if
> IsUsingStub() returns true?

I tried this path (centralize fake management in DBusClientType), it's a pretty
big yak to shave. If I don't manage to complete it tomorrow, I will split it in
another CL and land this one first.

[zel, 2014-08-12 17:32:32]

I will address remaining hashimoto's comments (from 
https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/mixed_dbu...)
in another CL.

It's simple to centralize code that manages fake vs. real dbus clients, but when
I do that tons of tests of sorts fail. I will need to spend mode time on that
part,

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_client_bundle.cc (right):

https://codereview.chromium.org/444263002/diff/120001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.cc:181:
update_engine_client_.reset(UpdateEngineClient::Create(type));
On 2014/08/11 01:52:09, satorux1 wrote:
> On 2014/08/08 19:00:14, zel wrote:
> > On 2014/08/08 07:08:44, hashimoto wrote:
> > > It seems POWER_POLICY is not handled.
> > 
> > POWER_POLICY client is for some reasons owned in DBusThreadManagerImpl and
> > handled in  DBusThreadManagerImpl::CreateDefaultClients(). I've just kept it
> > there even though I am sure if it has legit reasons to stay there at all.
> 
> hashimoto: good catch: Let's remove POWER_POLICY. PowerPolicyController is not
a
> D-Bus client and shouldn't belong to DBusThreadManager:
> https://code.google.com/p/chromium/issues/detail?id=345586
> 
> In CreateDefaultClients(), I think you can check POWER_MANAGER instead.

Done.

https://codereview.chromium.org/444263002/diff/200001/chromeos/chromeos_switc...
File chromeos/chromeos_switches.cc (right):

https://codereview.chromium.org/444263002/diff/200001/chromeos/chromeos_switc...
chromeos/chromeos_switches.cc:24: // Comma-spearated list of dbus clients that
should be unstubbed.
On 2014/08/11 01:52:09, satorux1 wrote:
> maybe add a reference like:
> 
> // See chromeos/dbus/dbus_client_bundle.cc for the names of the dbus clients.

Done.

https://codereview.chromium.org/444263002/diff/200001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_client_bundle.cc (right):

https://codereview.chromium.org/444263002/diff/200001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.cc:57: { "bluetoothlowenergy", 
DBusClientBundle::BLUETOOTH_LOW_ENERGY },
On 2014/08/11 01:52:09, satorux1 wrote:
> bluetooth_low_energy to be consistent with the enum?

Done.

https://codereview.chromium.org/444263002/diff/200001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_client_bundle.h (right):

https://codereview.chromium.org/444263002/diff/200001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.h:81: ALL_CLIENTS =          0xFFFFFFFF,
On 2014/08/11 01:52:09, satorux1 wrote:
> maybe: 1 << 21 - 1

maybe not. why would create an extra place that one can screw up when adding a
new enum here?

https://codereview.chromium.org/444263002/diff/200001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_client_bundle_unittest.cc (right):

https://codereview.chromium.org/444263002/diff/200001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle_unittest.cc:22: TEST_F(DBusClientBundleTest,
UnstubFlagParser) {
On 2014/08/11 01:52:10, satorux1 wrote:
> You can use TEST() and remove DBusClientBundleTest class

Done.

[satorux1, 2014-08-13 01:09:20]

LGTM with nits and a question

https://codereview.chromium.org/444263002/diff/220001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_client_bundle_unittest.cc (right):

https://codereview.chromium.org/444263002/diff/220001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle_unittest.cc:15: DBusClientBundle::UNKNOWN);
per gtest.h, the expected value should come first. the right order gives you a
better error message. please fix other places too.

https://codereview.chromium.org/444263002/diff/220001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle_unittest.cc:24:
"Cras,CrosDisks,debugdaemon,Shill"),
I'm confused. Why can "CrosDisks" be parsed, where as the string in the mapping
table is "cros_disks"?

GetDBusClientType() does case-insensitive matching but doesn't seem to ignore
'_'.

https://codereview.chromium.org/444263002/diff/220001/chromeos/dbus/dbus_thre...
File chromeos/dbus/dbus_thread_manager.cc (right):

https://codereview.chromium.org/444263002/diff/220001/chromeos/dbus/dbus_thre...
chromeos/dbus/dbus_thread_manager.cc:268: if
(!DBusThreadManager::IsUsingStub(DBusClientBundle::POWER_MANAGER))
while you are at it, could you add:

// TODO(crbug.com/345586): Move PowerPolicyController out of
DBusThreadManagerImpl.

[zel, 2014-08-13 01:36:03]

https://codereview.chromium.org/444263002/diff/220001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_client_bundle_unittest.cc (right):

https://codereview.chromium.org/444263002/diff/220001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle_unittest.cc:15: DBusClientBundle::UNKNOWN);
On 2014/08/13 01:09:20, satorux1 wrote:
> per gtest.h, the expected value should come first. the right order gives you a
> better error message. please fix other places too.

Done.

https://codereview.chromium.org/444263002/diff/220001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle_unittest.cc:24:
"Cras,CrosDisks,debugdaemon,Shill"),
On 2014/08/13 01:09:20, satorux1 wrote:
> I'm confused. Why can "CrosDisks" be parsed, where as the string in the
mapping
> table is "cros_disks"?
> 
> GetDBusClientType() does case-insensitive matching but doesn't seem to ignore
> '_'.

Yeah, this test should not pass :). Fixed.

https://codereview.chromium.org/444263002/diff/220001/chromeos/dbus/dbus_thre...
File chromeos/dbus/dbus_thread_manager.cc (right):

https://codereview.chromium.org/444263002/diff/220001/chromeos/dbus/dbus_thre...
chromeos/dbus/dbus_thread_manager.cc:268: if
(!DBusThreadManager::IsUsingStub(DBusClientBundle::POWER_MANAGER))
On 2014/08/13 01:09:20, satorux1 wrote:
> while you are at it, could you add:
> 
> // TODO(crbug.com/345586): Move PowerPolicyController out of
> DBusThreadManagerImpl.

Done.

[zel, 2014-08-13 01:37:19]

The CQ bit was checked by zelidrag@chromium.org

[commit-bot: I haz the power, 2014-08-13 01:43:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zelidrag@chromium.org/444263002/240001

[zel, 2014-08-13 01:44:32]

The CQ bit was unchecked by zelidrag@chromium.org

[hashimoto, 2014-08-13 06:14:49]

lgtm

https://codereview.chromium.org/444263002/diff/200001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_client_bundle.h (right):

https://codereview.chromium.org/444263002/diff/200001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.h:81: ALL_CLIENTS =          0xFFFFFFFF,
On 2014/08/12 17:32:32, zel wrote:
> On 2014/08/11 01:52:09, satorux1 wrote:
> > maybe: 1 << 21 - 1
> 
> maybe not. why would create an extra place that one can screw up when adding a
> new enum here?

Then how about "~static_cast<DBusClientTypeMask>(0)"?

https://codereview.chromium.org/444263002/diff/240001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_client_bundle.cc (right):

https://codereview.chromium.org/444263002/diff/240001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.cc:55: } client_type_map[] = {
This should be const.

https://codereview.chromium.org/444263002/diff/240001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.cc:90: }  // namespace
nit: Here should be a blank line.

https://codereview.chromium.org/444263002/diff/240001/chromeos/dbus/dbus_thre...
File chromeos/dbus/dbus_thread_manager.cc (right):

https://codereview.chromium.org/444263002/diff/240001/chromeos/dbus/dbus_thre...
chromeos/dbus/dbus_thread_manager.cc:286: 
nit: Unneeded blank line.

https://codereview.chromium.org/444263002/diff/240001/chromeos/dbus/mixed_dbu...
File chromeos/dbus/mixed_dbus_thread_manager.h (right):

https://codereview.chromium.org/444263002/diff/240001/chromeos/dbus/mixed_dbu...
chromeos/dbus/mixed_dbus_thread_manager.h:25: private:
nit: Methods below are overriding public method. No need to have this private.

[zel, 2014-08-13 17:57:44]

https://codereview.chromium.org/444263002/diff/200001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_client_bundle.h (right):

https://codereview.chromium.org/444263002/diff/200001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.h:81: ALL_CLIENTS =          0xFFFFFFFF,
On 2014/08/13 06:14:48, hashimoto wrote:
> On 2014/08/12 17:32:32, zel wrote:
> > On 2014/08/11 01:52:09, satorux1 wrote:
> > > maybe: 1 << 21 - 1
> > 
> > maybe not. why would create an extra place that one can screw up when adding
a
> > new enum here?
> 
> Then how about "~static_cast<DBusClientTypeMask>(0)"?

Done.

https://codereview.chromium.org/444263002/diff/240001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_client_bundle.cc (right):

https://codereview.chromium.org/444263002/diff/240001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.cc:55: } client_type_map[] = {
On 2014/08/13 06:14:48, hashimoto wrote:
> This should be const.

Done.

https://codereview.chromium.org/444263002/diff/240001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.cc:90: }  // namespace
On 2014/08/13 06:14:48, hashimoto wrote:
> nit: Here should be a blank line.

Done.

https://codereview.chromium.org/444263002/diff/240001/chromeos/dbus/dbus_thre...
File chromeos/dbus/dbus_thread_manager.cc (right):

https://codereview.chromium.org/444263002/diff/240001/chromeos/dbus/dbus_thre...
chromeos/dbus/dbus_thread_manager.cc:286: 
On 2014/08/13 06:14:48, hashimoto wrote:
> nit: Unneeded blank line.

Done.

https://codereview.chromium.org/444263002/diff/240001/chromeos/dbus/mixed_dbu...
File chromeos/dbus/mixed_dbus_thread_manager.h (right):

https://codereview.chromium.org/444263002/diff/240001/chromeos/dbus/mixed_dbu...
chromeos/dbus/mixed_dbus_thread_manager.h:25: private:
On 2014/08/13 06:14:49, hashimoto wrote:
> nit: Methods below are overriding public method. No need to have this private.

Done.

[zel, 2014-08-13 17:57:48]

The CQ bit was checked by zelidrag@chromium.org

[commit-bot: I haz the power, 2014-08-13 17:58:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zelidrag@chromium.org/444263002/260001

[zel, 2014-08-13 18:15:49]

The CQ bit was unchecked by zelidrag@chromium.org

[zel, 2014-08-13 18:18:58]

The CQ bit was checked by zelidrag@chromium.org

[commit-bot: I haz the power, 2014-08-13 18:20:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zelidrag@chromium.org/444263002/300001

[zel, 2014-08-13 18:25:17]

The CQ bit was unchecked by zelidrag@chromium.org

[zel, 2014-08-13 18:25:33]

The CQ bit was checked by zelidrag@chromium.org

[commit-bot: I haz the power, 2014-08-13 18:27:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zelidrag@chromium.org/444263002/320001

[armansito, 2014-08-13 20:59:28]

https://codereview.chromium.org/444263002/diff/340001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_chromeos_unittest.cc (right):

https://codereview.chromium.org/444263002/diff/340001/device/bluetooth/blueto...
device/bluetooth/bluetooth_chromeos_unittest.cc:238:
DBusThreadManager::InitializeForTesting(fake_dbus_thread_manager);
Can you add a note here explaining why this has to happen before the following?

[armansito, 2014-08-13 21:06:44]

device/bluetooth/bluetooth_chromeos_unittest.cc lgtm

[armansito, 2014-08-13 21:09:13]

https://codereview.chromium.org/444263002/diff/380001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_client_bundle.cc (right):

https://codereview.chromium.org/444263002/diff/380001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.cc:57: { "bluetooth_low_energy", 
DBusClientBundle::BLUETOOTH_LOW_ENERGY },
What's the reason for having two separate flags for bluetooth? If you unstub one
and not the other, would things even work?

[armansito, 2014-08-13 21:39:07]

https://codereview.chromium.org/444263002/diff/380001/chromeos/dbus/mixed_dbu...
File chromeos/dbus/mixed_dbus_thread_manager.cc (right):

https://codereview.chromium.org/444263002/diff/380001/chromeos/dbus/mixed_dbu...
chromeos/dbus/mixed_dbus_thread_manager.cc:43:
DBusClientBundle::BLUETOOTH)->GetBluetoothGattCharacteristicClient();
Shouldn't this also be BLUETOOTH_LOW_ENERGY?

[zel, 2014-08-13 22:22:41]

https://codereview.chromium.org/444263002/diff/380001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_client_bundle.cc (right):

https://codereview.chromium.org/444263002/diff/380001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.cc:57: { "bluetooth_low_energy", 
DBusClientBundle::BLUETOOTH_LOW_ENERGY },
On 2014/08/13 21:09:13, armansito wrote:
> What's the reason for having two separate flags for bluetooth? If you unstub
one
> and not the other, would things even work?

I was suspecting that some systems might support BT but not BLE. We could make
BLE flag imply BT automatically - makes sense?

https://codereview.chromium.org/444263002/diff/380001/chromeos/dbus/mixed_dbu...
File chromeos/dbus/mixed_dbus_thread_manager.cc (right):

https://codereview.chromium.org/444263002/diff/380001/chromeos/dbus/mixed_dbu...
chromeos/dbus/mixed_dbus_thread_manager.cc:43:
DBusClientBundle::BLUETOOTH)->GetBluetoothGattCharacteristicClient();
On 2014/08/13 21:39:06, armansito wrote:
> Shouldn't this also be BLUETOOTH_LOW_ENERGY?

Done.

[armansito, 2014-08-13 22:28:46]

https://codereview.chromium.org/444263002/diff/380001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_client_bundle.cc (right):

https://codereview.chromium.org/444263002/diff/380001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.cc:57: { "bluetooth_low_energy", 
DBusClientBundle::BLUETOOTH_LOW_ENERGY },
On 2014/08/13 22:22:40, zel wrote:
> On 2014/08/13 21:09:13, armansito wrote:
> > What's the reason for having two separate flags for bluetooth? If you unstub
> one
> > and not the other, would things even work?
> 
> I was suspecting that some systems might support BT but not BLE. We could make
> BLE flag imply BT automatically - makes sense?

Per our offline discussion, let's merge these two into a single BLUETOOTH flag.

[zel, 2014-08-13 22:35:54]

https://codereview.chromium.org/444263002/diff/380001/chromeos/dbus/dbus_clie...
File chromeos/dbus/dbus_client_bundle.cc (right):

https://codereview.chromium.org/444263002/diff/380001/chromeos/dbus/dbus_clie...
chromeos/dbus/dbus_client_bundle.cc:57: { "bluetooth_low_energy", 
DBusClientBundle::BLUETOOTH_LOW_ENERGY },
On 2014/08/13 22:28:46, armansito wrote:
> On 2014/08/13 22:22:40, zel wrote:
> > On 2014/08/13 21:09:13, armansito wrote:
> > > What's the reason for having two separate flags for bluetooth? If you
unstub
> > one
> > > and not the other, would things even work?
> > 
> > I was suspecting that some systems might support BT but not BLE. We could
make
> > BLE flag imply BT automatically - makes sense?
> 
> Per our offline discussion, let's merge these two into a single BLUETOOTH
flag.

Done.

[zel, 2014-08-13 22:36:04]

The CQ bit was checked by zelidrag@chromium.org

[commit-bot: I haz the power, 2014-08-13 22:37:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zelidrag@chromium.org/444263002/420001

[armansito, 2014-08-13 22:38:39]

lgtm

[zel, 2014-08-13 22:55:03]

The CQ bit was checked by zelidrag@chromium.org

[commit-bot: I haz the power, 2014-08-13 22:57:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zelidrag@chromium.org/444263002/440001

[commit-bot: I haz the power, 2014-08-14 08:00:40]

Committed patchset #23 (440001) as 289485