Improve NetworkStateHandler API

Improve NetworkStateHandler API

The UI needs more flexible accessors than "ActiveNetwork," which is also a somewhat unclear term. This replaces it with DefaultNetwork and provides better accessors for the UI.

BUG=none
For webui:
TBR=jhawkins@chromium.org

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

[stevenjb, 2012-12-19 04:10:11]

OK, after a bit of back and forth with myself, here is what I decided to do with
regard to "active" networks and NetworkStateHandler:

1. I renamed "ActiveNetwork()" to "DefaultNetwork()" and updated the comment.
This now matches the Shill terminology. It is the first connected network, which
will generally be the VPN if one is provided.

2. I added some pseudo-types to NetworkState that allow the UI to get the
"Connnected" or "Connecting" network that is:
* Any type of network (i.e. the default)
* A non-virtual network (will be useful in the status area code)
* Any wireless network (also useful in the status area code)
* A specific type of network (e.g. VPN, also needed by the UI code)

Let me know what you think / if you have any concerns.

[pneubeck (no reviews), 2012-12-19 15:22:38]

If find the updates and notifications to DefaultNetworkChanged rather scattered
in the code and hard to follow.

Perhaps the following would be an acceptable alternative implementation:

In every update function called by ShillPropertyHandler that potentially changes
the default network or its state,
- store in a local variable at the beginning of the function the default
network's path and its state
- and the end of the functions call CheckDefaultNetwork (which should then
renamed to UpdateDefaultNetwork(old_network_path, old_network_connection_state))

Then you could remove the interspersed updates/notifications regarding the
default network. Also would that update look and work the same for every
function called by ShillPropertyHandler.

https://codereview.chromium.org/11614035/diff/8019/chromeos/network/network_s...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/11614035/diff/8019/chromeos/network/network_s...
chromeos/network/network_state.h:42: // * kMatchTypeDefault to match any network
Why not call it kMatchTypeAny?

https://codereview.chromium.org/11614035/diff/8019/chromeos/network/network_s...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/11614035/diff/8019/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:320: if (key ==
flimflam::kStateProperty && network->state() == prev_state)
Why is this required? The return value of ParseNetworkServiceProperty should
capture this case already.

https://codereview.chromium.org/11614035/diff/8019/chromeos/network/network_s...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/11614035/diff/8019/chromeos/network/network_s...
chromeos/network/network_state_handler.h:93: // Returns the first connected
network of type |type|, otherwise NULL.
Do we assume that the first connected network has some special meaning/purpose,
because Shill sorts networks/services in a certain way?

If so, we should at least document that here. A lot of code will depend on that
function.

However, as explained to me by Darin Petkov, what Shill keeps track of is the
network through which all (or by default) traffic is routed. And that is
something completely different from only being connected.

E.g. what do we want to do if Ethernet, Wifi and Cellular are connected?
How do we decide what to show in the network menu icon in the tray menu?

Please, please ensure that this decision is never done on Chrome's side.

Currently it is done in NetworkLibrary::active_network() with a random heuristic
where wifi overwrites cellular. But that is simply wrong and easily out of sync
with the decisions Shill is taking.

https://codereview.chromium.org/11614035/diff/8019/chromeos/network/network_s...
chromeos/network/network_state_handler.h:185: // Helper function called to parse
|network| properties.
Please document this function more clearly.

Something like the follwing I'd guess:
Updates the property |key| of the stored NetworkState associated with |network|
to |value|. Returns whether |value| was different from the stored value."

https://codereview.chromium.org/11614035/diff/8019/chromeos/network/network_s...
File chromeos/network/network_state_handler_observer.h (right):

https://codereview.chromium.org/11614035/diff/8019/chromeos/network/network_s...
chromeos/network/network_state_handler_observer.h:37: // The default network
changed (includes VPNs) or its state changed.
Aahh! I think we have terminology issue here. Please distinguish between state
(like NetworkState) and something like connection state (like
NetworkState::state_).

https://codereview.chromium.org/11614035/diff/8019/chromeos/network/network_s...
chromeos/network/network_state_handler_observer.h:44: // One or more non-state
network service properties changed. Note: this will
missing reference to |network|?

This is actually not restricted to non-state, but triggers if any member of
NetworkState (including |state_|) changes. Please fix the comment or
implementation.

"Use this to ..." should perhaps be before the "Note: ".

"...when the _state_ property changes..." seems to contradict "One or more
_non-state_ properties changed".

[stevenjb, 2012-12-19 17:59:28]

On 2012/12/19 15:22:38, pneubeck wrote:
> If find the updates and notifications to DefaultNetworkChanged rather
scattered
> in the code and hard to follow.
>
There are only two: 
1) When the network list changes, which may affect the priority order (I'll add
a comment)
2) When a network service's connection state has changed (hopefully this is more
clear now)

> Perhaps the following would be an acceptable alternative implementation:
> 
> In every update function called by ShillPropertyHandler that potentially
changes
> the default network or its state,
> - store in a local variable at the beginning of the function the default
> network's path and its state
> - and the end of the functions call CheckDefaultNetwork (which should then
> renamed to UpdateDefaultNetwork(old_network_path,
old_network_connection_state))
> 
> Then you could remove the interspersed updates/notifications regarding the
> default network. Also would that update look and work the same for every
> function called by ShillPropertyHandler.
> 
I'm not sure that I follow; there are just the two places. Again, hopefully it
is more clear now.

[stevenjb, 2012-12-19 18:01:07]

https://codereview.chromium.org/11614035/diff/8019/chromeos/network/network_s...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/11614035/diff/8019/chromeos/network/network_s...
chromeos/network/network_state.h:42: // * kMatchTypeDefault to match any network
On 2012/12/19 15:22:38, pneubeck wrote:
> Why not call it kMatchTypeAny?
I wanted to match the intention here; "Any" implies that it is arbitrary. I
could have used "First", but that is really an implementation detail.

Would it be more clear if these were defined in NetworkStateHandler which is
what uses them (and the logic put there also)?

https://codereview.chromium.org/11614035/diff/8019/chromeos/network/network_s...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/11614035/diff/8019/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:320: if (key ==
flimflam::kStateProperty && network->state() == prev_state)
On 2012/12/19 15:22:38, pneubeck wrote:
> Why is this required? The return value of ParseNetworkServiceProperty should
> capture this case already.

This logic is now in ParseNetworkServiceProperty, but "parse" returns true if it
was parsed, regardless of whether or not it actually changed.

https://codereview.chromium.org/11614035/diff/8019/chromeos/network/network_s...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/11614035/diff/8019/chromeos/network/network_s...
chromeos/network/network_state_handler.h:93: // Returns the first connected
network of type |type|, otherwise NULL.
On 2012/12/19 15:22:38, pneubeck wrote:
> Do we assume that the first connected network has some special
meaning/purpose,
> because Shill sorts networks/services in a certain way?
Yes.
> 
> If so, we should at least document that here. A lot of code will depend on
that
> function.
Replaced "first" with primary. Shill passes us the networks in the same priority
order that it uses, so "first" is really an implementation detail.

> 
> However, as explained to me by Darin Petkov, what Shill keeps track of is the
> network through which all (or by default) traffic is routed. And that is
> something completely different from only being connected.
> 
> E.g. what do we want to do if Ethernet, Wifi and Cellular are connected?
> How do we decide what to show in the network menu icon in the tray menu?
> 
> Please, please ensure that this decision is never done on Chrome's side.
> 
This decision is partly up to Shill and partly up to the UI. e.g. we don't want
to show any icon if the Default is ethernet, but do want to show an icon if we
are conneting to any Wireless. It is impossible to completely separate the two.
That is why I created these additional types so that the UI logic can be more
flexible and independent of the Shill implementation. 

> Currently it is done in NetworkLibrary::active_network() with a random
heuristic
> where wifi overwrites cellular. But that is simply wrong and easily out of
sync
> with the decisions Shill is taking.

The heuristic is not random. The purpose of the highest_priority() logic in
active_network() is designed to take that into account. However, that logic is
(clearly) complex and difficult to follow, which is why we are trying to
simplify it here.

On several occasions we have stumbled upon places where different parts of the
code want different results for active_network() which is why I am changing the
terminology and adding additional flexibility for the UI.

https://codereview.chromium.org/11614035/diff/8019/chromeos/network/network_s...
chromeos/network/network_state_handler.h:185: // Helper function called to parse
|network| properties.
On 2012/12/19 15:22:38, pneubeck wrote:
> Please document this function more clearly.
> 
> Something like the follwing I'd guess:
> Updates the property |key| of the stored NetworkState associated with
|network|
> to |value|. Returns whether |value| was different from the stored value."

I separated out the observer notification code. I also noticed a bug as I did so
- we want to be sure to only call the NetworkStateChanged() observer when an
actual change occurred; we don't care for the generic NetworkPropertyChanged().
Added a test to verify that.

https://codereview.chromium.org/11614035/diff/8019/chromeos/network/network_s...
File chromeos/network/network_state_handler_observer.h (right):

https://codereview.chromium.org/11614035/diff/8019/chromeos/network/network_s...
chromeos/network/network_state_handler_observer.h:37: // The default network
changed (includes VPNs) or its state changed.
On 2012/12/19 15:22:38, pneubeck wrote:
> Aahh! I think we have terminology issue here. Please distinguish between state
> (like NetworkState) and something like connection state (like
> NetworkState::state_).

That's a good observation. Will replace "state" with "connection state" (which
is mostly accurate) throughout.

https://codereview.chromium.org/11614035/diff/8019/chromeos/network/network_s...
chromeos/network/network_state_handler_observer.h:44: // One or more non-state
network service properties changed. Note: this will
On 2012/12/19 15:22:38, pneubeck wrote:
> missing reference to |network|?
> 
> This is actually not restricted to non-state, but triggers if any member of
> NetworkState (including |state_|) changes. Please fix the comment or
> implementation.
> 
> "Use this to ..." should perhaps be before the "Note: ".
> 
> "...when the _state_ property changes..." seems to contradict "One or more
> _non-state_ properties changed".

Fixed and clarified.

[pneubeck (no reviews), 2012-12-19 19:40:06]

Overall, my only major concern is the usage of the word "changed" throughout the
implementation.
Up to now, I thought that it really means that a state property changed, but
apparently it only says that possibly something changed. That's also why I
didn't mention it during the initial CL.

https://codereview.chromium.org/11614035/diff/8019/chromeos/network/network_s...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/11614035/diff/8019/chromeos/network/network_s...
chromeos/network/network_state.h:42: // * kMatchTypeDefault to match any network
On 2012/12/19 18:01:07, stevenjb (chromium) wrote:
> On 2012/12/19 15:22:38, pneubeck wrote:
> > Why not call it kMatchTypeAny?
> I wanted to match the intention here; "Any" implies that it is arbitrary. I
> could have used "First", but that is really an implementation detail.
> 
> Would it be more clear if these were defined in NetworkStateHandler which is
> what uses them (and the logic put there also)?

But First makes only sense in the context of the functions of
NetworkStateHandler, where it's already written:
 "first/primary connected network of type |type|"
In that sentence, "Any" is completely understandable.

I don't see a problem with Any.
If you move it, you get a dependency from this class back to the handler. Not
sure, if that's worth it.

https://codereview.chromium.org/11614035/diff/8019/chromeos/network/network_s...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/11614035/diff/8019/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:320: if (key ==
flimflam::kStateProperty && network->state() == prev_state)
On 2012/12/19 18:01:07, stevenjb (chromium) wrote:
> On 2012/12/19 15:22:38, pneubeck wrote:
> > Why is this required? The return value of ParseNetworkServiceProperty should
> > capture this case already.
> 
> This logic is now in ParseNetworkServiceProperty, but "parse" returns true if
it
> was parsed, regardless of whether or not it actually changed.

Yeah, I was confused by the line
  if (!network->PropertyChanged(key, value))
which really reads like:
  if not the property |key| changed

https://codereview.chromium.org/11614035/diff/8019/chromeos/network/network_s...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/11614035/diff/8019/chromeos/network/network_s...
chromeos/network/network_state_handler.h:93: // Returns the first connected
network of type |type|, otherwise NULL.
As there are several users (uses in the code) of "active network" which also
have different expectations on what "active" means, that indicates that we
should either provide

A) a set of more basic functions which have to be combined by the users.
In that case, the functions need a very clear semantic.
For example, we could provide the list of all connected/connecting networks and
the interested caller has to sort/pick the right network on his own.

or
B) we could provide specifically tailored functions which solve each of the use
cases and their semantics cannot be explained easily. But then, we should
mention for which usage each function is meant for.
A really sketchy example:
 a) ... returns primary network which is the network that the ChromeOS user is
primarily interested in.
 b) ... returns the network with the current default route of the network
traffic. Used for proxy and network notifications of the browser.

https://codereview.chromium.org/11614035/diff/3020/chromeos/network/network_s...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/11614035/diff/3020/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:290: if (ParseNetworkServiceProperty(
Why are you interested here in if something was successfully parsed and not if
something actually changed?

That something changed implies that we parsed that value successfully. Thus it
would only reduce the amount of notifications.

https://codereview.chromium.org/11614035/diff/3020/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:425:
OnNetworkConnectionStateChanged(network);
looks better. Now it's clearer which conditions are checked before a
notification about the state change is send out.

https://codereview.chromium.org/11614035/diff/3020/chromeos/network/network_s...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/11614035/diff/3020/chromeos/network/network_s...
chromeos/network/network_state_handler.h:86: // observer this class and
implement NetworkPropertyChanged().
nit: observer -> observe

https://codereview.chromium.org/11614035/diff/3020/chromeos/network/network_s...
chromeos/network/network_state_handler.h:186: // OnNetworkConnectionStateChanged
if the connection_state property changes.
nit:
OnNetworkConnectionStateChanged -> OnNetworkConnectionStateChanged()

for consistency.

https://codereview.chromium.org/11614035/diff/3020/chromeos/network/network_s...
File chromeos/network/network_state_handler_observer.h (right):

https://codereview.chromium.org/11614035/diff/3020/chromeos/network/network_s...
chromeos/network/network_state_handler_observer.h:44: // One or more network
service properties changed. Note: this will get
I would still add a reference to |network|
... properties of |network| ...

https://codereview.chromium.org/11614035/diff/3020/chromeos/network/network_s...
File chromeos/network/network_state_handler_unittest.cc (right):

https://codereview.chromium.org/11614035/diff/3020/chromeos/network/network_s...
chromeos/network/network_state_handler_unittest.cc:204:
DBusThreadManager::Get()->GetShillServiceClient()->SetProperty(
Is that really a requirement about the NetworkStateHandler?
Who relies on such repeated notifications? It's also not explicitly stated in
the class/method descriptions. So nobody should rely on that.

[stevenjb, 2012-12-19 21:36:50]

"Changed" and "Updated" (may have changed) should now be accurate.

https://codereview.chromium.org/11614035/diff/3020/chromeos/network/network_s...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/11614035/diff/3020/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:290: if (ParseNetworkServiceProperty(
On 2012/12/19 19:40:06, pneubeck wrote:
> Why are you interested here in if something was successfully parsed and not if
> something actually changed?
> 
> That something changed implies that we parsed that value successfully. Thus it
> would only reduce the amount of notifications.

That is misleading. I renamed network_property_changed to
network_property_updated and renamed the observer function to
NetworkPropertiesUpdated.

I also renamed NetworkStateChanged to NetworkConnectionStateChanged, missed that
in the last change. This method (and the other methods) should only get called
when something actually changes.

https://codereview.chromium.org/11614035/diff/3020/chromeos/network/network_s...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/11614035/diff/3020/chromeos/network/network_s...
chromeos/network/network_state_handler.h:86: // observer this class and
implement NetworkPropertyChanged().
On 2012/12/19 19:40:06, pneubeck wrote:
> nit: observer -> observe

Done.

https://codereview.chromium.org/11614035/diff/3020/chromeos/network/network_s...
chromeos/network/network_state_handler.h:186: // OnNetworkConnectionStateChanged
if the connection_state property changes.
On 2012/12/19 19:40:06, pneubeck wrote:
> nit:
> OnNetworkConnectionStateChanged -> OnNetworkConnectionStateChanged()
> 
> for consistency.

Done.

https://codereview.chromium.org/11614035/diff/3020/chromeos/network/network_s...
File chromeos/network/network_state_handler_observer.h (right):

https://codereview.chromium.org/11614035/diff/3020/chromeos/network/network_s...
chromeos/network/network_state_handler_observer.h:44: // One or more network
service properties changed. Note: this will get
On 2012/12/19 19:40:06, pneubeck wrote:
> I would still add a reference to |network|
> ... properties of |network| ...

Done.

https://codereview.chromium.org/11614035/diff/3020/chromeos/network/network_s...
File chromeos/network/network_state_handler_unittest.cc (right):

https://codereview.chromium.org/11614035/diff/3020/chromeos/network/network_s...
chromeos/network/network_state_handler_unittest.cc:204:
DBusThreadManager::Get()->GetShillServiceClient()->SetProperty(
On 2012/12/19 19:40:06, pneubeck wrote:
> Is that really a requirement about the NetworkStateHandler?
> Who relies on such repeated notifications? It's also not explicitly stated in
> the class/method descriptions. So nobody should rely on that.

Good point. I added that to confirm that the behavior is what I expected, but
you're right, it's not something we especially desire or should rely on. Removed
the extra test.

[pneubeck (no reviews), 2012-12-19 22:04:35]

Looks good to me.

Considering our last discussion about error handling in chromeos/network/onc/*,
which now uses NetworkEventLog, I just wondered why the chromeos/network/*.cc
code uses LOG(*)? :-)


Btw. if you'd like to reduce the number of property notifications only a small
change to the Get*Value(..) methods in managed_state.cc would be required. It's
certainly not critical to have that and could also be added later if the need
arises.

E.g. for string it could like:

bool ManagedState::GetStringValue(const std::string& key,
                                  const base::Value& value,
                                  std::string* out_value) {
   std::string old_value = *out_value;  
   bool res = value.GetAsString(out_value);
   if (!res) {
      LOG...
      return false;
   }
   return old_value != *out_value;
}

[stevenjb (google-dont-use), 2012-12-19 22:19:29]

That's a fair point about value updates, I'd forgotten that we already had
wrappers. I'll think about doing that (in a separate CL).

NetworkEventLog() should be used for anything that might help us debug an
end user networking problem. We should try to avoid spamming it. For logic
errors that we never expect to happen but that we can easily recover from,
we should still use LOG(ERROR) or LOG(WARNING). For errors that should
logically never happen and are not easily recoverable we should use DCHECK
(if it would cause an immediate crash anyway) or CHECK (if it would cause a
more subtle error later). VLOG is still useful for stuff that is helpful
for debugging but would otherwise be spammy.


On Wed, Dec 19, 2012 at 2:04 PM, <pneubeck@chromium.org> wrote:

> Looks good to me.
>
> Considering our last discussion about error handling in
> chromeos/network/onc/*,
> which now uses NetworkEventLog, I just wondered why the
> chromeos/network/*.cc
> code uses LOG(*)? :-)
>
>
> Btw. if you'd like to reduce the number of property notifications only a
> small
> change to the Get*Value(..) methods in managed_state.cc would be required.
> It's
> certainly not critical to have that and could also be added later if the
> need
> arises.
>
> E.g. for string it could like:
>
> bool ManagedState::GetStringValue(**const std::string& key,
>                                   const base::Value& value,
>                                   std::string* out_value) {
>    std::string old_value = *out_value;
>    bool res = value.GetAsString(out_value);
>    if (!res) {
>       LOG...
>       return false;
>    }
>    return old_value != *out_value;
> }
>
>
>
https://codereview.chromium.**org/11614035/<https://codereview.chromium.org/1...
>

[gauravsh, 2012-12-19 23:48:00]

Can you update the commit message with more detail (since there's no linked
bug).

https://codereview.chromium.org/11614035/diff/7007/chromeos/network/network_s...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/11614035/diff/7007/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:438: if (!signaled_observers &&
network->path() == default_network_path_) {
CheckDefaultNetworkChanged() could perhaps use a better name. The return value
is reflective of whether the default network changed or not. (To my mind, the
observer signalling is just a convenient side effect if the default network did
change).

Also, do you really need |signaled_observers|? It's only used once.

https://codereview.chromium.org/11614035/diff/7007/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:440:
FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
You don't log DefaultNetworkChanged to the event log here. Should you?

[gauravsh, 2012-12-19 23:50:15]

and lgtm modulo the nits in my previous message

[stevenjb, 2012-12-20 02:17:50]

Addressed feedback and moved kMatchType* to NetworkStateHandler because I think
they make more sense there; they are only used by
NetworkStateHandler::Connect*State().

https://codereview.chromium.org/11614035/diff/7007/chromeos/network/network_s...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/11614035/diff/7007/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:438: if (!signaled_observers &&
network->path() == default_network_path_) {
On 2012/12/19 23:48:00, gauravsh wrote:
> CheckDefaultNetworkChanged() could perhaps use a better name. The return value
> is reflective of whether the default network changed or not. (To my mind, the
> observer signalling is just a convenient side effect if the default network
did
> change).
> 
> Also, do you really need |signaled_observers|? It's only used once. 

Split out logging and notification from CheckDefaultNetworkChanged which I think
makes this all clearer.

https://codereview.chromium.org/11614035/diff/7007/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:440:
FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
On 2012/12/19 23:48:00, gauravsh wrote:
> You don't log DefaultNetworkChanged to the event log here. Should you?

Yes, thanks. Fixed with splitting CheckDefaultNetworkChanged().

[Greg Spencer (Chromium), 2012-12-20 17:37:47]

LGTM

https://codereview.chromium.org/11614035/diff/7010/chromeos/network/network_s...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/11614035/diff/7010/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:480: network_event_log::AddEntry(
Do you want to use the NET_LOG macros here?  You don't have to, but seems like
the messages would be more consistent that way...

[stevenjb, 2012-12-20 17:46:26]

https://codereview.chromium.org/11614035/diff/7010/chromeos/network/network_s...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/11614035/diff/7010/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:480: network_event_log::AddEntry(
On 2012/12/20 17:37:47, Greg Spencer (Chromium) wrote:
> Do you want to use the NET_LOG macros here?  You don't have to, but seems like
> the messages would be more consistent that way...
As discussed in IM, we should make a pass through this and maybe introduce
NET_LOG_EVENT that doesn't have file/line info for non-warnings or error (in a
separate CL)

[commit-bot: I haz the power, 2012-12-20 17:46:39]

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

[commit-bot: I haz the power, 2012-12-20 17:46:45]

Presubmit check for 11614035-7010 failed and returned exit status 1.


Running presubmit commit checks ...

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

Presubmit checks took 1.4s to calculate.

[commit-bot: I haz the power, 2012-12-20 17:50:49]

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