Replace misc. network stub flags with more flexible ones

Replace misc. network and power stub flags with more flexible ones

This replaces the following flags:
--default-stub-network-state-idle
--enable-stub-interactive (for Shill only)
--enable-stub-portalled-wifi
--enabled-stub-network-types

With:
--shill-stub={options}

Specific option values are commented in the code.

BUG=345033
R=pneubeck@chromium.org

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

[stevenjb, 2014-02-26 22:52:56]

I've been wanting to clean this up for a while; since we're pruning command line
switches, this seemed like a good time. Feedback welcome.

[Daniel Erat, 2014-02-27 03:32:47]

lgtm for all the non-networking parts

[pneubeck (no reviews), 2014-02-27 09:04:36]

high level comments from a first fly over.

https://codereview.chromium.org/181413006/diff/40001/chromeos/dbus/dbus_threa...
File chromeos/dbus/dbus_thread_manager.h (right):

https://codereview.chromium.org/181413006/diff/40001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.h:156: virtual int GetShillInteractiveDelay()
const;
not sure about GetPowerCycleDelay,
but the GetShill* should really not be part of the production code.
please move them to the FakeDBusThreadManager.

You could pass the FakeDBusThreadManager to the fake ShillClients' constructor
or alternatively pass only some configuration (which would cleanup the
dependencies).
See my later comments...

https://codereview.chromium.org/181413006/diff/40001/chromeos/dbus/fake_dbus_...
File chromeos/dbus/fake_dbus_thread_manager.cc (right):

https://codereview.chromium.org/181413006/diff/40001/chromeos/dbus/fake_dbus_...
chromeos/dbus/fake_dbus_thread_manager.cc:120:
scoped_ptr<ShillManagerClient>(new FakeShillManagerClient));
here you could pass the options or 'this'.

https://codereview.chromium.org/181413006/diff/40001/chromeos/dbus/fake_dbus_...
File chromeos/dbus/fake_dbus_thread_manager.h (right):

https://codereview.chromium.org/181413006/diff/40001/chromeos/dbus/fake_dbus_...
chromeos/dbus/fake_dbus_thread_manager.h:166: int shill_interactive_delay_;  //
Seconds to delay interactive actions
you could put these two shill_* members into a ShillTestConfiguration struct and
pass that directly to the fake Shill*Clients.
This config could be optional for the construction of the clients.

https://codereview.chromium.org/181413006/diff/40001/chromeos/dbus/shill_stub...
File chromeos/dbus/shill_stub_helper.cc (right):

https://codereview.chromium.org/181413006/diff/40001/chromeos/dbus/shill_stub...
chromeos/dbus/shill_stub_helper.cc:68: bool IsStubPortalledWifiEnabled(const
std::string& path) {
this super hacky function could move for example to
ShillServiceClient->GetTestInterface.

or the network_portal_detector_impl.cc:363 gets it's trigger more directly, e.g.
by reading the command line again.

or network_portal_detector_impl.cc:363 checks for default_service_path_ ==
kStubPortalledWifiPath, which is correct if that path is not a correct
Shill/DBus path (which should be commented at the definition).

https://codereview.chromium.org/181413006/diff/40001/chromeos/dbus/shill_stub...
chromeos/dbus/shill_stub_helper.cc:74: void SetupDefaultEnvironment() {
please pass FakeDBusThreadManager as an argument

[stevenjb, 2014-02-28 02:44:06]

https://codereview.chromium.org/181413006/diff/40001/chromeos/dbus/dbus_threa...
File chromeos/dbus/dbus_thread_manager.h (right):

https://codereview.chromium.org/181413006/diff/40001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.h:156: virtual int GetShillInteractiveDelay()
const;
On 2014/02/27 09:04:37, pneubeck wrote:
> not sure about GetPowerCycleDelay,
> but the GetShill* should really not be part of the production code.
> please move them to the FakeDBusThreadManager.
> 
> You could pass the FakeDBusThreadManager to the fake ShillClients' constructor
> or alternatively pass only some configuration (which would cleanup the
> dependencies).
> See my later comments...

I'm not super thrilled about passing FakeDBusThreadManager to every ShillClient
constructor. I think I would rather just move this to the appropriate clients
instead (ShillManagerClient for Shill).

https://codereview.chromium.org/181413006/diff/40001/chromeos/dbus/fake_dbus_...
File chromeos/dbus/fake_dbus_thread_manager.cc (right):

https://codereview.chromium.org/181413006/diff/40001/chromeos/dbus/fake_dbus_...
chromeos/dbus/fake_dbus_thread_manager.cc:120:
scoped_ptr<ShillManagerClient>(new FakeShillManagerClient));
On 2014/02/27 09:04:37, pneubeck wrote:
> here you could pass the options or 'this'.
Implementation moved.

https://codereview.chromium.org/181413006/diff/40001/chromeos/dbus/fake_dbus_...
File chromeos/dbus/fake_dbus_thread_manager.h (right):

https://codereview.chromium.org/181413006/diff/40001/chromeos/dbus/fake_dbus_...
chromeos/dbus/fake_dbus_thread_manager.h:166: int shill_interactive_delay_;  //
Seconds to delay interactive actions
On 2014/02/27 09:04:37, pneubeck wrote:
> you could put these two shill_* members into a ShillTestConfiguration struct
and
> pass that directly to the fake Shill*Clients.
> This config could be optional for the construction of the clients.

Moved implementation to clients.

https://codereview.chromium.org/181413006/diff/40001/chromeos/dbus/shill_stub...
File chromeos/dbus/shill_stub_helper.cc (right):

https://codereview.chromium.org/181413006/diff/40001/chromeos/dbus/shill_stub...
chromeos/dbus/shill_stub_helper.cc:68: bool IsStubPortalledWifiEnabled(const
std::string& path) {
On 2014/02/27 09:04:37, pneubeck wrote:
> this super hacky function could move for example to
> ShillServiceClient->GetTestInterface.
> 
> or the network_portal_detector_impl.cc:363 gets it's trigger more directly,
e.g.
> by reading the command line again.
> 
> or network_portal_detector_impl.cc:363 checks for default_service_path_ ==
> kStubPortalledWifiPath, which is correct if that path is not a correct
> Shill/DBus path (which should be commented at the definition).

Looking at the code that calls this, this shouldn't be needed at all; removed.

https://codereview.chromium.org/181413006/diff/40001/chromeos/dbus/shill_stub...
chromeos/dbus/shill_stub_helper.cc:74: void SetupDefaultEnvironment() {
On 2014/02/27 09:04:37, pneubeck wrote:
> please pass FakeDBusThreadManager as an argument
I ended up eliminating this class entirely. After eliminating some cruft, it
really just belongs in FakeShillManagerClient.

[pneubeck (no reviews), 2014-02-28 09:37:58]

Could you put the SharedProfilePath changes into a separate CL? That would it
clearer I think.

Overall I think the dependencies are much better now compared to the previous
CL.

It appears now to me, that the (Fake)ShillManagerClient mutated into a
management class of all Shill*Clients. You _might_ consider (maybe in a separate
CL) to make FakeShillManagerClient again a lightweight client as it was
originally intended and instead add a ShillHandler/ShillDbusManager that owns
all (Fake)Shill*Clients. Many test interface functions could move there. Thus,
many of the mutual dependencies between the FakeClients would vanish.

If you are not in a hurry, we can also take about this CL next week when I'm in
MTV.

https://codereview.chromium.org/181413006/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/user_image_loader.cc (right):

https://codereview.chromium.org/181413006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/user_image_loader.cc:71: if (!success) return;
please don't include this unrelated(?) functional change.

https://codereview.chromium.org/181413006/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/network_portal_detector_impl.cc (left):

https://codereview.chromium.org/181413006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl.cc:365: if
(shill_stub_helper::IsStubPortalledWifiEnabled(default_service_path_)) {
are you sure?
I thought this was for triggering the Captive portal detection by providing the
right command line flag. Does this still work or does ygorshenin@ not need it
anymore?

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

https://codereview.chromium.org/181413006/diff/60001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.cc:470: InitializeClients();
please make the SetupDefaultEnvironment explicit and not implicit on each
creation of the Shill*Clients:

fake_dbus_thread_manager->SetFakeClients();
scoped_ptr<FakeShillManagerClient> fake_shill_manager_client(new
FakeShillManagerClient);
fake_shill_manager_client->SetupDefaultEnvironment();
fake_dbus_thread_manager->SetShillManagerClient(scoped_ptr<ShillManagerClient>(fake_shill_manager_client));

if that's too much boilerplate for your taste, you could instead
pass a boolean setupDefaultEnvironment through some
FakeDBusThreadManager::SetupFake*Clients(...) but better don't change the
behavior of the existing SetupFakeClients().

https://codereview.chromium.org/181413006/diff/60001/chromeos/dbus/fake_shill...
File chromeos/dbus/fake_shill_manager_client.cc (right):

https://codereview.chromium.org/181413006/diff/60001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_manager_client.cc:101: SetupDefaultEnvironment();
IIRC, the idea at some time was that this DefaultEnvironment is optional (or
explicitly set) and it should be possible for unit tests to setup a the
Shill*Clients with an "empty" state.

https://codereview.chromium.org/181413006/diff/60001/chromeos/dbus/shill_prof...
File chromeos/dbus/shill_profile_client.h (right):

https://codereview.chromium.org/181413006/diff/60001/chromeos/dbus/shill_prof...
chromeos/dbus/shill_profile_client.h:80: static const char*
GetSharedProfilePath();
const char* -> std::string
to prevent any surprises?

https://codereview.chromium.org/181413006/diff/60001/chromeos/network/network...
File chromeos/network/network_device_handler_unittest.cc (right):

https://codereview.chromium.org/181413006/diff/60001/chromeos/network/network...
chromeos/network/network_device_handler_unittest.cc:36:
DBusThreadManager::InitializeForTesting(dbus_manager);
Do you change this order just to get rid of the devices that
FakeShillManagerClient::SetupDefaultEnvironment setup up?
Please call SetupDefaultEnvironment explicitly and don't change the order here.

[stevenjb, 2014-02-28 17:42:20]

I'll go ahead and separate the the shared path cleanup.

We're trying to emulate a single external process, Shill, with a handful of
individual stubs. They already talk to each other to accomplish this. It
makes sense to me for Manager to be the controller. When I first wrote them
there wasn't a specific intent to make any or all of them "lightweight",
and I'm not convinced that creating a separate "controller" class would
simplify things (and there's a limited amount of re-factoring I can afford
to do right now for what started as flag cleanup).

We can discuss more next week, but I'm going to make the changes I
mentioned as a pair of CLs in the hope that it will be satisfactory for now.




On Fri, Feb 28, 2014 at 1:37 AM, <pneubeck@chromium.org> wrote:

> Could you put the SharedProfilePath changes into a separate CL? That would
> it
> clearer I think.
>
> Overall I think the dependencies are much better now compared to the
> previous
> CL.
>
> It appears now to me, that the (Fake)ShillManagerClient mutated into a
> management class of all Shill*Clients. You _might_ consider (maybe in a
> separate
> CL) to make FakeShillManagerClient again a lightweight client as it was
> originally intended and instead add a ShillHandler/ShillDbusManager that
> owns
> all (Fake)Shill*Clients. Many test interface functions could move there.
> Thus,
> many of the mutual dependencies between the FakeClients would vanish.
>
> If you are not in a hurry, we can also take about this CL next week when
> I'm in
> MTV.
>
>
> https://codereview.chromium.org/181413006/diff/60001/
> chrome/browser/chromeos/login/user_image_loader.cc
> File chrome/browser/chromeos/login/user_image_loader.cc (right):
>
> https://codereview.chromium.org/181413006/diff/60001/
> chrome/browser/chromeos/login/user_image_loader.cc#newcode71
> chrome/browser/chromeos/login/user_image_loader.cc:71: if (!success)
> return;
> please don't include this unrelated(?) functional change.
>
> https://codereview.chromium.org/181413006/diff/60001/
> chrome/browser/chromeos/net/network_portal_detector_impl.cc
> File chrome/browser/chromeos/net/network_portal_detector_impl.cc (left):
>
> https://codereview.chromium.org/181413006/diff/60001/
> chrome/browser/chromeos/net/network_portal_detector_impl.cc#oldcode365
> chrome/browser/chromeos/net/network_portal_detector_impl.cc:365: if
> (shill_stub_helper::IsStubPortalledWifiEnabled(default_service_path_)) {
> are you sure?
> I thought this was for triggering the Captive portal detection by
> providing the right command line flag. Does this still work or does
> ygorshenin@ not need it anymore?
>
> https://codereview.chromium.org/181413006/diff/60001/
> chromeos/dbus/dbus_thread_manager.cc
> File chromeos/dbus/dbus_thread_manager.cc (right):
>
> https://codereview.chromium.org/181413006/diff/60001/
> chromeos/dbus/dbus_thread_manager.cc#newcode470
> chromeos/dbus/dbus_thread_manager.cc:470: InitializeClients();
> please make the SetupDefaultEnvironment explicit and not implicit on
> each creation of the Shill*Clients:
>
> fake_dbus_thread_manager->SetFakeClients();
> scoped_ptr<FakeShillManagerClient> fake_shill_manager_client(new
> FakeShillManagerClient);
> fake_shill_manager_client->SetupDefaultEnvironment();
> fake_dbus_thread_manager->SetShillManagerClient(scoped_
> ptr<ShillManagerClient>(fake_shill_manager_client));
>
> if that's too much boilerplate for your taste, you could instead
> pass a boolean setupDefaultEnvironment through some
> FakeDBusThreadManager::SetupFake*Clients(...) but better don't change
> the behavior of the existing SetupFakeClients().
>
> https://codereview.chromium.org/181413006/diff/60001/
> chromeos/dbus/fake_shill_manager_client.cc
> File chromeos/dbus/fake_shill_manager_client.cc (right):
>
> https://codereview.chromium.org/181413006/diff/60001/
> chromeos/dbus/fake_shill_manager_client.cc#newcode101
> chromeos/dbus/fake_shill_manager_client.cc:101:
> SetupDefaultEnvironment();
> IIRC, the idea at some time was that this DefaultEnvironment is optional
> (or explicitly set) and it should be possible for unit tests to setup a
> the Shill*Clients with an "empty" state.
>
> https://codereview.chromium.org/181413006/diff/60001/
> chromeos/dbus/shill_profile_client.h
> File chromeos/dbus/shill_profile_client.h (right):
>
> https://codereview.chromium.org/181413006/diff/60001/
> chromeos/dbus/shill_profile_client.h#newcode80
> chromeos/dbus/shill_profile_client.h:80: static const char*
> GetSharedProfilePath();
> const char* -> std::string
> to prevent any surprises?
>
> https://codereview.chromium.org/181413006/diff/60001/
> chromeos/network/network_device_handler_unittest.cc
> File chromeos/network/network_device_handler_unittest.cc (right):
>
> https://codereview.chromium.org/181413006/diff/60001/
> chromeos/network/network_device_handler_unittest.cc#newcode36
> chromeos/network/network_device_handler_unittest.cc:36:
> DBusThreadManager::InitializeForTesting(dbus_manager);
> Do you change this order just to get rid of the devices that
> FakeShillManagerClient::SetupDefaultEnvironment setup up?
> Please call SetupDefaultEnvironment explicitly and don't change the
> order here.
>
> https://codereview.chromium.org/181413006/
>

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

[stevenjb, 2014-02-28 17:43:05]

https://codereview.chromium.org/181413006/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/user_image_loader.cc (right):

https://codereview.chromium.org/181413006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/user_image_loader.cc:71: if (!success) return;
On 2014/02/28 09:37:59, pneubeck wrote:
> please don't include this unrelated(?) functional change.
Accidental.

https://codereview.chromium.org/181413006/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/network_portal_detector_impl.cc (left):

https://codereview.chromium.org/181413006/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_detector_impl.cc:365: if
(shill_stub_helper::IsStubPortalledWifiEnabled(default_service_path_)) {
On 2014/02/28 09:37:59, pneubeck wrote:
> are you sure?
> I thought this was for triggering the Captive portal detection by providing
the
> right command line flag. Does this still work or does ygorshenin@ not need it
> anymore?
If we need it again we should implement it better in the fake implementations
themselves. It didn't break any tests, and as you pointed out was a terrible
hack in production code. I will include ygorshenin@ on the CL.

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

https://codereview.chromium.org/181413006/diff/60001/chromeos/dbus/dbus_threa...
chromeos/dbus/dbus_thread_manager.cc:470: InitializeClients();
On 2014/02/28 09:37:59, pneubeck wrote:
> please make the SetupDefaultEnvironment explicit and not implicit on each
> creation of the Shill*Clients:
> 
> fake_dbus_thread_manager->SetFakeClients();
> scoped_ptr<FakeShillManagerClient> fake_shill_manager_client(new
> FakeShillManagerClient);
> fake_shill_manager_client->SetupDefaultEnvironment();
>
fake_dbus_thread_manager->SetShillManagerClient(scoped_ptr<ShillManagerClient>(fake_shill_manager_client));
> 
> if that's too much boilerplate for your taste, you could instead
> pass a boolean setupDefaultEnvironment through some
> FakeDBusThreadManager::SetupFake*Clients(...) but better don't change the
> behavior of the existing SetupFakeClients().

Constructing the Fake manger client twice doesn't make any sense to me, but I'll
figure out a better way to do this.

https://codereview.chromium.org/181413006/diff/60001/chromeos/dbus/fake_shill...
File chromeos/dbus/fake_shill_manager_client.cc (right):

https://codereview.chromium.org/181413006/diff/60001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_manager_client.cc:101: SetupDefaultEnvironment();
On 2014/02/28 09:37:59, pneubeck wrote:
> IIRC, the idea at some time was that this DefaultEnvironment is optional (or
> explicitly set) and it should be possible for unit tests to setup a the
> Shill*Clients with an "empty" state.

Anything that calls InitializeWithStub is going to get the default environment
regardless, and Fake instances created separately generally don't call Init(),
but I will move this as previously mentioned to make it a bit more clear.

https://codereview.chromium.org/181413006/diff/60001/chromeos/dbus/shill_prof...
File chromeos/dbus/shill_profile_client.h (right):

https://codereview.chromium.org/181413006/diff/60001/chromeos/dbus/shill_prof...
chromeos/dbus/shill_profile_client.h:80: static const char*
GetSharedProfilePath();
On 2014/02/28 09:37:59, pneubeck wrote:
> const char* -> std::string
> to prevent any surprises?
We do this a lot in the UI (where it makes compare faster), but we don't need
that here, so sure.

https://codereview.chromium.org/181413006/diff/60001/chromeos/network/network...
File chromeos/network/network_device_handler_unittest.cc (right):

https://codereview.chromium.org/181413006/diff/60001/chromeos/network/network...
chromeos/network/network_device_handler_unittest.cc:36:
DBusThreadManager::InitializeForTesting(dbus_manager);
On 2014/02/28 09:37:59, pneubeck wrote:
> Do you change this order just to get rid of the devices that
> FakeShillManagerClient::SetupDefaultEnvironment setup up?
> Please call SetupDefaultEnvironment explicitly and don't change the order
here.

I see. This confused me, the behavior changed form how I remembered it, I
thought this was incorrect. I forgot that InitializeForTesting doesn't actually
create any fake implementations. I'll change this back.

[stevenjb, 2014-02-28 20:26:16]

PTAL

(I moved the PowerManagerClient changes into a separate CL that I will put up
once/if we commit this)
-derat@

[pneubeck (no reviews), 2014-03-03 20:36:19]

Sorry for the long list of comments. But I think this could/should be
simplified.

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/dbus_comm...
File chromeos/dbus/dbus_command_line_helper.cc (right):

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/dbus_comm...
chromeos/dbus/dbus_command_line_helper.cc:21: arg0->clear();
just always clear at the beginning of this function

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/dbus_comm...
chromeos/dbus/dbus_command_line_helper.cc:39: const ParseOptionCallback&
callback) {
please use some existing parsing mechanism, e.g. json .
When the request was to reduce the number of command-line arguments, then I
don't think that it's a good answer to just nest the options into a single
argument.

On the other hand I can see purpose in hierarchically structured options.

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
File chromeos/dbus/fake_shill_manager_client.cc (right):

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:172: if (interactive_delay_) {
actually, why not PostDelayedTask(..., 0) and thereby ensuring that the same
control-flow is tested? (if so, better change it in a separate patch)

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:493: return shill::kStateOffline;  //
Not available
this comment isn't not informative. please make it a sentence.

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:517: // Ethernet. No service is not
added unless connected.
'No ... not added unless' ?
Remove 'not'?

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:831: void
FakeShillManagerClient::ParseCommandLineSwitch() {
we need a documentation of the format of this command line argument.

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:835: base::Unretained(this))))
nit: missing { }

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:857: std::string state_arg) {
I'd fail on state_arg.empty() instead of doing a default.
Just make the logic as trivial as possible.

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:860: if (state_arg == "0" ||
state_arg == "off" || state_arg == "inactive" ||
this is only a testing interface for developers/testers. I don't think that we
need synonyms.

same for the other options.

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:864: state = shill::kStateDisconnect;
 // Diabled but available
typo: Diabled -> Disabled

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:866: state = shill::kStateOffline; 
// Not available
this comment doesn't seem very explanatory. maybe make it a sentence (also in
the other cases).

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:870: state =
shill::kActivationStateActivated;  // Connected and activated
*teeth grinding*
maybe not such a good idea to assign constants from different "enums" to the
same variable (kState* vs. kActivationState* )

you could put a struct into the initial state map.

if you would use a dictionary argument per network type, then you could put
'active' in to a separate option. then the command line arg would e.g. look
like:
--shill-stub='{"cellular": {"active": True, "state": "idle"}}'

I know, that the syntax is very verbose compared to your solution. but we should
weigh up code simplicity against usability. considering that there are not many
people using this, I would go for code simplicity.

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:879:
shill_initial_state_map_[shill::kTypeEthernet] = state;
the if-branches could decide on the shill type string and the map assigments
could be merged into one statement after the if-branches.

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:884: } else if (type_arg ==
"wireless") {
considering that this is only an interface for testing,
couldn't you just forward the type_arg as a Shill network type? And to provide
some type checking, just verify that type_arg is an element of a list of all
network types?
For "wireless" you could keep the special handling.

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:898: std::string
FakeShillManagerClient::ParseInitialState(const std::string& type,
this is not really about parsing. maybe call it something like
GetInitialStateProperties

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:915: type != shill::kTypeCellular) {
better make this the default case and let it skip all previous if-branches if
the state is invalid.

[stevenjb, 2014-03-05 01:03:49]

ptal

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/dbus_comm...
File chromeos/dbus/dbus_command_line_helper.cc (right):

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/dbus_comm...
chromeos/dbus/dbus_command_line_helper.cc:21: arg0->clear();
On 2014/03/03 20:36:19, pneubeck wrote:
> just always clear at the beginning of this function

Done.

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/dbus_comm...
chromeos/dbus/dbus_command_line_helper.cc:39: const ParseOptionCallback&
callback) {
On 2014/03/03 20:36:19, pneubeck wrote:
> please use some existing parsing mechanism, e.g. json .
> When the request was to reduce the number of command-line arguments, then I
> don't think that it's a good answer to just nest the options into a single
> argument.
> 
> On the other hand I can see purpose in hierarchically structured options.
This is almost the same format as we use for vmodule with the helpful addition
of "shill-stub=wifi,cellular" enabling just wifi and cellular.

As a frequent consumer of these options, I find having a single, simple format
*much* easier to deal with.

While someday it would be nice to also support importing a configuration from a
json file, putting json on a command line doesn't seem very useful to me.

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
File chromeos/dbus/fake_shill_manager_client.cc (right):

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:172: if (interactive_delay_) {
On 2014/03/03 20:36:19, pneubeck wrote:
> actually, why not PostDelayedTask(..., 0) and thereby ensuring that the same
> control-flow is tested? (if so, better change it in a separate patch)
Good point, and I think that's pretty low risk; if the unit tests don't break
I'll go ahead and change that.

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:493: return shill::kStateOffline;  //
Not available
On 2014/03/03 20:36:19, pneubeck wrote:
> this comment isn't not informative. please make it a sentence.

I made this function private now that the portal detection logic doesn't depend
on it. Changed this to use a local constant kTechnologyUnavailable instead.

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:517: // Ethernet. No service is not
added unless connected.
On 2014/03/03 20:36:19, pneubeck wrote:
> 'No ... not added unless' ?
> Remove 'not'?

Removed comment, behavior is now consistent across technologies. Comment for
Ethernet is in SetInitialNetworkState.

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:831: void
FakeShillManagerClient::ParseCommandLineSwitch() {
On 2014/03/03 20:36:19, pneubeck wrote:
> we need a documentation of the format of this command line argument.

Documentation is in dbus_command_line_helper plus I added comments to
ParseOptions in the header.

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:835: base::Unretained(this))))
On 2014/03/03 20:36:19, pneubeck wrote:
> nit: missing { }

Done.

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:857: std::string state_arg) {
On 2014/03/03 20:36:19, pneubeck wrote:
> I'd fail on state_arg.empty() instead of doing a default.
> Just make the logic as trivial as possible.
I specifically want an empty state arg to default to enabled to support, e.g.,
'shill_stub=wifi,cellular'
I'll add a comment.

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:860: if (state_arg == "0" ||
state_arg == "off" || state_arg == "inactive" ||
On 2014/03/03 20:36:19, pneubeck wrote:
> this is only a testing interface for developers/testers. I don't think that we
> need synonyms.
> 
> same for the other options.
I feel the opposite. Since it's a testing interface it should be as forgiving as
possible.

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:864: state = shill::kStateDisconnect;
 // Diabled but available
On 2014/03/03 20:36:19, pneubeck wrote:
> typo: Diabled -> Disabled

Done.

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:866: state = shill::kStateOffline; 
// Not available
On 2014/03/03 20:36:19, pneubeck wrote:
> this comment doesn't seem very explanatory. maybe make it a sentence (also in
> the other cases).
Improved all comments.

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:870: state =
shill::kActivationStateActivated;  // Connected and activated
On 2014/03/03 20:36:19, pneubeck wrote:
> *teeth grinding*
> maybe not such a good idea to assign constants from different "enums" to the
> same variable (kState* vs. kActivationState* )
> 
> you could put a struct into the initial state map.
> 
> if you would use a dictionary argument per network type, then you could put
> 'active' in to a separate option. then the command line arg would e.g. look
> like:
> --shill-stub='{"cellular": {"active": True, "state": "idle"}}'
> 
> I know, that the syntax is very verbose compared to your solution. but we
should
> weigh up code simplicity against usability. considering that there are not
many
> people using this, I would go for code simplicity.

1. I used local values for special states "Unavailable" and "Activated"
2. I really want to keep the syntax as simple and versatile as possible. If
there is demand at some point for parsing JSON and someone implements it, great,
but I still think that being able to add something like
"--shill_stub=wifi,cellular=disabled" is really useful.

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:879:
shill_initial_state_map_[shill::kTypeEthernet] = state;
On 2014/03/03 20:36:19, pneubeck wrote:
> the if-branches could decide on the shill type string and the map assigments
> could be merged into one statement after the if-branches.

Sure. I thought I might have more synonyms, but I can just check for valid types
and handle the synonyms separately.

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:884: } else if (type_arg ==
"wireless") {
On 2014/03/03 20:36:19, pneubeck wrote:
> considering that this is only an interface for testing,
> couldn't you just forward the type_arg as a Shill network type? And to provide
> some type checking, just verify that type_arg is an element of a list of all
> network types?
> For "wireless" you could keep the special handling.
Yes. This was easier/faster, but sure.

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:898: std::string
FakeShillManagerClient::ParseInitialState(const std::string& type,
On 2014/03/03 20:36:19, pneubeck wrote:
> this is not really about parsing. maybe call it something like
> GetInitialStateProperties

Done.

https://codereview.chromium.org/181413006/diff/120001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:915: type != shill::kTypeCellular) {
On 2014/03/03 20:36:19, pneubeck wrote:
> better make this the default case and let it skip all previous if-branches if
> the state is invalid.

Done.

[pneubeck (no reviews), 2014-03-05 18:47:09]

I agree that json is too unwieldy to meet your intent.
Looking at the implementation of SplitStringIntoKeyValuePairs, you can use it
and just ignore the result value and you get exactly the behavior of your
implementation (to be exact, it additionally does whitespace trimming).

All other parts look good to me.

https://codereview.chromium.org/181413006/diff/160001/chromeos/dbus/dbus_comm...
File chromeos/dbus/dbus_command_line_helper.cc (right):

https://codereview.chromium.org/181413006/diff/160001/chromeos/dbus/dbus_comm...
chromeos/dbus/dbus_command_line_helper.cc:36: bool ParseOptions(const
std::string& switch_name,
If this implementation continues to exist (e.g. in base/), then this callback
style should be replaced by the usual std::vector<std::string>* argument.

https://codereview.chromium.org/181413006/diff/160001/chromeos/dbus/fake_shil...
File chromeos/dbus/fake_shill_manager_client.cc (right):

https://codereview.chromium.org/181413006/diff/160001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:862: // Technology diabled but
available, services are created but not connected.
nit: diabled -> disabled

https://codereview.chromium.org/181413006/diff/160001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:874: // Default: Technolgy is enabled
and a service is connected.
Technolgy -> Technology

https://codereview.chromium.org/181413006/diff/160001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:898: // Unconnected or disabled
etherned is the same as unavailable.
nit: etherned -> ethernet

[stevenjb, 2014-03-05 20:38:17]

So, the annoying difference with SplitStringIntoKeyValuePairs is that
--shill-stub=wifi,cellular won't work, it requires an '='. Rather than changing
SplitStringIntoKeyValuePairs and possibly breaking something else, or insisting
on a new parser, I'll just live with typing 'wifi=1' every time. (I know it
seems like a trivial thing, but when I'm working on UI I'll be using this a
lot...). Anyway, dbus_command_line_helper is gone, PTAL.

https://codereview.chromium.org/181413006/diff/160001/chromeos/dbus/fake_shil...
File chromeos/dbus/fake_shill_manager_client.cc (right):

https://codereview.chromium.org/181413006/diff/160001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:862: // Technology diabled but
available, services are created but not connected.
On 2014/03/05 18:47:09, pneubeck wrote:
> nit: diabled -> disabled

Done.

https://codereview.chromium.org/181413006/diff/160001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:874: // Default: Technolgy is enabled
and a service is connected.
On 2014/03/05 18:47:09, pneubeck wrote:
> Technolgy -> Technology

Done.

https://codereview.chromium.org/181413006/diff/160001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:898: // Unconnected or disabled
etherned is the same as unavailable.
On 2014/03/05 18:47:09, pneubeck wrote:
> nit: etherned -> ethernet

Done.

[pneubeck (no reviews), 2014-03-05 22:50:35]

lgtm

[stevenjb, 2014-03-05 22:58:03]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-03-05 22:59:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/181413006/180001

[commit-bot: I haz the power, 2014-03-05 22:59:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-05 22:59:46]

Failed to apply patch for chromeos/dbus/fake_shill_manager_client.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chromeos/dbus/fake_shill_manager_client.cc
  Hunk #2 succeeded at 86 with fuzz 1 (offset 10 lines).
  Hunk #3 FAILED at 134.
  Hunk #4 succeeded at 159 (offset 10 lines).
  Hunk #5 succeeded at 177 (offset 10 lines).
  Hunk #6 succeeded at 200 (offset 10 lines).
  Hunk #7 succeeded at 231 (offset 10 lines).
  Hunk #8 succeeded at 325 (offset 10 lines).
  Hunk #9 FAILED at 331.
  Hunk #10 FAILED at 408.
  Hunk #11 FAILED at 419.
  Hunk #12 FAILED at 436.
  Hunk #13 FAILED at 463.
  Hunk #14 FAILED at 517.
  Hunk #15 FAILED at 594.
  Hunk #16 FAILED at 647.
  9 out of 16 hunks FAILED -- saving rejects to file
chromeos/dbus/fake_shill_manager_client.cc.rej

Patch:       chromeos/dbus/fake_shill_manager_client.cc
Index: chromeos/dbus/fake_shill_manager_client.cc
diff --git a/chromeos/dbus/fake_shill_manager_client.cc
b/chromeos/dbus/fake_shill_manager_client.cc
index
d3ad56641aa07e74879092b1a6ad95559420655a..0300609d0bacc899b5aa84330884569e4caa1233
100644
--- a/chromeos/dbus/fake_shill_manager_client.cc
+++ b/chromeos/dbus/fake_shill_manager_client.cc
@@ -7,6 +7,9 @@
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/message_loop/message_loop.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
 #include "base/values.h"
 #include "chromeos/chromeos_switches.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
@@ -73,10 +76,23 @@ void AppendServicesForType(
   }
 }
 
+void UpdatePortaledWifiState(const std::string& service_path) {
+  DBusThreadManager::Get()->GetShillServiceClient()->GetTestInterface()
+      ->SetServiceProperty(service_path,
+                           shill::kStateProperty,
+                           base::StringValue(shill::kStatePortal));
+}
+
+const char* kTechnologyUnavailable = "unavailable";
+const char* kNetworkActivated = "activated";
+const char* kNetworkDisabled = "disabled";
+
 }  // namespace
 
 FakeShillManagerClient::FakeShillManagerClient()
-    : weak_ptr_factory_(this) {
+    : interactive_delay_(0),
+      weak_ptr_factory_(this) {
+  ParseCommandLineSwitch();
 }
 
 FakeShillManagerClient::~FakeShillManagerClient() {}
@@ -118,7 +134,7 @@ void FakeShillManagerClient::SetProperty(const std::string&
name,
                                          const base::Closure& callback,
                                          const ErrorCallback& error_callback) {
   stub_properties_.SetWithoutPathExpansion(name, value.DeepCopy());
-  CallNotifyObserversPropertyChanged(name, 0);
+  CallNotifyObserversPropertyChanged(name);
   base::MessageLoop::current()->PostTask(FROM_HERE, callback);
 }
 
@@ -133,21 +149,16 @@ void FakeShillManagerClient::RequestScan(const
std::string& type,
       DBusThreadManager::Get()->GetShillDeviceClient()->GetTestInterface();
   std::string device_path = device_client->GetDevicePathForType(device_type);
   if (!device_path.empty()) {
-    device_client->SetDeviceProperty(device_path,
-                                     shill::kScanningProperty,
-                                     base::FundamentalValue(true));
-  }
-  const int kScanDurationSeconds = 3;
-  int scan_duration_seconds = kScanDurationSeconds;
-  if (!CommandLine::ForCurrentProcess()->HasSwitch(
-      chromeos::switches::kEnableStubInteractive)) {
-    scan_duration_seconds = 0;
+    device_client->SetDeviceProperty(
+        device_path, shill::kScanningProperty, base::FundamentalValue(true));
   }
   base::MessageLoop::current()->PostDelayedTask(
       FROM_HERE,
       base::Bind(&FakeShillManagerClient::ScanCompleted,
-                 weak_ptr_factory_.GetWeakPtr(), device_path, callback),
-      base::TimeDelta::FromSeconds(scan_duration_seconds));
+                 weak_ptr_factory_.GetWeakPtr(),
+                 device_path,
+                 callback),
+      base::TimeDelta::FromSeconds(interactive_delay_));
 }
 
 void FakeShillManagerClient::EnableTechnology(
@@ -156,24 +167,21 @@ void FakeShillManagerClient::EnableTechnology(
     const ErrorCallback& error_callback) {
   base::ListValue* enabled_list = NULL;
   if (!stub_properties_.GetListWithoutPathExpansion(
-      shill::kEnabledTechnologiesProperty, &enabled_list)) {
+          shill::kAvailableTechnologiesProperty, &enabled_list)) {
     base::MessageLoop::current()->PostTask(FROM_HERE, callback);
     base::MessageLoop::current()->PostTask(
         FROM_HERE,
         base::Bind(error_callback, "StubError", "Property not found"));
     return;
   }
-  if (CommandLine::ForCurrentProcess()->HasSwitch(
-          chromeos::switches::kEnableStubInteractive)) {
-    const int kEnableTechnologyDelaySeconds = 3;
-    base::MessageLoop::current()->PostDelayedTask(
-        FROM_HERE,
-        base::Bind(&FakeShillManagerClient::SetTechnologyEnabled,
-                   weak_ptr_factory_.GetWeakPtr(), type, callback, true),
-        base::TimeDelta::FromSeconds(kEnableTechnologyDelaySeconds));
-  } else {
-    SetTechnologyEnabled(type, callback, true);
-  }
+  base::MessageLoop::current()->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&FakeShillManagerClient::SetTechnologyEnabled,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 type,
+                 callback,
+                 true),
+      base::TimeDelta::FromSeconds(interactive_delay_));
 }
 
 void FakeShillManagerClient::DisableTechnology(
@@ -182,20 +190,21 @@ void FakeShillManagerClient::DisableTechnology(
     const ErrorCallback& error_callback) {
   base::ListValue* enabled_list = NULL;
   if (!stub_properties_.GetListWithoutPathExpansion(
-      shill::kEnabledTechnologiesProperty, &enabled_list)) {
+          shill::kAvailableTechnologiesProperty, &enabled_list)) {
     base::MessageLoop::current()->PostTask(
         FROM_HERE,
         base::Bind(error_callback, "StubError", "Property not found"));
     return;
   }
-  if (CommandLine::ForCurrentProcess()->HasSwitch(
-          chromeos::switches::kEnableStubInteractive)) {
-    const int kDisableTechnologyDelaySeconds = 3;
+  if (interactive_delay_) {
     base::MessageLoop::current()->PostDelayedTask(
         FROM_HERE,
         base::Bind(&FakeShillManagerClient::SetTechnologyEnabled,
-                   weak_ptr_factory_.GetWeakPtr(), type, callback, false),
-        base::TimeDelta::FromSeconds(kDisableTechnologyDelaySeconds));
+                   weak_ptr_factory_.GetWeakPtr(),
+                   type,
+                   callback,
+                   false),
+        base::TimeDelta::FromSeconds(interactive_delay_));
   } else {
     SetTechnologyEnabled(type, callback, false);
   }
@@ -212,7 +221,7 @@ void FakeShillManagerClient::ConfigureService(
   std::string type;
   if (!properties.GetString(shill::kGuidProperty, &guid) ||
       !properties.GetString(shill::kTypeProperty, &type)) {
-    LOG(ERROR) << "ConfigureService requies GUID and Type to be defined";
+    LOG(ERROR) << "ConfigureService requires GUID and Type to be defined";
     // If the properties aren't filled out completely, then just return an
empty
     // object path.
     base::MessageLoop::current()->PostTask(
@@ -306,8 +315,8 @@ void FakeShillManagerClient::VerifyAndEncryptData(
     const std::string& data,
     const StringCallback& callback,
     const ErrorCallback& error_callback) {
-  base::MessageLoop::current()->PostTask(FROM_HERE,
-                                   base::Bind(callback, "encrypted_data"));
+  base::MessageLoop::current()->PostTask(
+      FROM_HERE, base::Bind(callback, "encrypted_data"));
 }
 
 void FakeShillManagerClient::ConnectToBestServices(
@@ -322,71 +331,87 @@ ShillManagerClient::TestInterface*
FakeShillManagerClient::GetTestInterface() {
 // ShillManagerClient::TestInterface overrides.
 
 void FakeShillManagerClient::AddDevice(const std::string& device_path) {
-  if (GetListProperty(shill::kDevicesProperty)->AppendIfNotPresent(
-      base::Value::CreateStringValue(device_path))) {
-    CallNotifyObserversPropertyChanged(shill::kDevicesProperty, 0);
+  if (GetListProperty(shill::kDevicesProperty)
+          ->AppendIfNotPresent(base::Value::CreateStringValue(device_path))) {
+    CallNotifyObserversPropertyChanged(shill::kDevicesProperty);
   }
 }
 
 void FakeShillManagerClient::RemoveDevice(const std::string& device_path) {
   base::StringValue device_path_value(device_path);
-  if (GetListProperty(shill::kDevicesProperty)->Remove(
-      device_path_value, NULL)) {
-    CallNotifyObserversPropertyChanged(shill::kDevicesProperty, 0);
+  if (GetListProperty(shill::kDevicesProperty)
+          ->Remove(device_path_value, NULL)) {
+    CallNotifyObserversPropertyChanged(shill::kDevicesProperty);
   }
 }
 
 void FakeShillManagerClient::ClearDevices() {
   GetListProperty(shill::kDevicesProperty)->Clear();
-  CallNotifyObserversPropertyChanged(shill::kDevicesProperty, 0);
+  CallNotifyObserversPropertyChanged(shill::kDevicesProperty);
 }
 
 void FakeShillManagerClient::AddTechnology(const std::string& type,
                                            bool enabled) {
-  if (GetListProperty(shill::kAvailableTechnologiesProperty)->
-      AppendIfNotPresent(base::Value::CreateStringValue(type))) {
-    CallNotifyObserversPropertyChanged(
-        shill::kAvailableTechnologiesProperty, 0);
+  if (GetListProperty(shill::kAvailableTechnologiesProperty)
+          ->AppendIfNotPresent(base::Value::CreateStringValue(type))) {
+    CallNotifyObserversPropertyChanged(shill::kAvailableTechnologiesProperty);
   }
   if (enabled &&
-      GetListProperty(shill::kEnabledTechnologiesProperty)->
-      AppendIfNotPresent(base::Value::CreateStringValue(type))) {
-    CallNotifyObserversPropertyChanged(
-        shill::kEnabledTechnologiesProperty, 0);
+      GetListProperty(shill::kEnabledTechnologiesProperty)
+         …
(message too large)

[stevenjb, 2014-03-05 23:16:40]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-03-05 23:17:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/181413006/200001

[commit-bot: I haz the power, 2014-03-06 00:09:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/181413006/200001

[commit-bot: I haz the power, 2014-03-06 00:49:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-06 00:49:29]

Try jobs failed on following builders: mac_chromium_rel

[stevenjb, 2014-03-06 20:50:57]

Committed patchset #12 manually as r255422 (presubmit successful).