API for connection type (Ethernet/WIFI/WWAN ...) in NetworkChangeNotifier.

Support for navigator.connection API

This replaces the online state (a boolean) by a more complex state (UNKNOWN/NONE
/2G/3G/4G/WIFI/ETHERNET) inspired by the Network Information API
(http://www.w3.org/TR/netinfo-api/).

Breakdown of the modified files:
- network_change_notifier.h/.cc: actual API change
- network_change_notifier_*: platform specific implementations. This CL only provide a basic implementation where CONNECTION_UNKNOWN is returned when online and CONNECTION_NONE is returned when offline.
- other files: call sites for the static function and observer implementations. Most of the time this only changes the test IsOffline() by (GetConnectionType() == CONNECTION_NONE).

BUG=112937
TEST=NONE

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

[joth, 2012-01-10 10:48:35]

a couple drive by comments.
Thanks for doing this, it will be useful for platforms that can provide this
detail.

http://codereview.chromium.org/9147026/diff/1/chrome/browser/sessions/session...
File chrome/browser/sessions/session_restore.cc (right):

http://codereview.chromium.org/9147026/diff/1/chrome/browser/sessions/session...
chrome/browser/sessions/session_restore.cc:323: if (state !=
net::NetworkChangeNotifier::NONE) {
nit: wrong indent

http://codereview.chromium.org/9147026/diff/1/net/base/network_change_notifie...
File net/base/network_change_notifier.cc (right):

http://codereview.chromium.org/9147026/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier.cc:77: ETHERNET;
UNKNOWN, I would have thought?

Also, this probably needs a comment or TODO about fetching more detailed info,
if possible.

http://codereview.chromium.org/9147026/diff/1/net/base/network_change_notifier.h
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/9147026/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier.h:29: NONE,
Suggest putting NONE first, with a value of 0, as this reinforces the "if (state
!= NONE) ..." pattern for detecting online state.

http://codereview.chromium.org/9147026/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier.h:57: // See NetworkChangeNotifier::IsOffline()
for important caveats about
IsOffline -> GetConnectionState

http://codereview.chromium.org/9147026/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier.h:107: static ConnectionState
GetConnectionState();
I'm in two minds about rolling state and connection mode together into one
setting. I guess the contract would be only show a mode beyond 'None' when that
interface is fully connected and operational. Most platforms go through some
intermediate states though (e.g. connected via wifi, not yet got an IP address)
but I think the way the docs are worded (i.e. NONE is the only authoritative
state, the rest are just hints) probably covers this.


Also need to clarify what value would be returned if more than one interface is
available. (presumably report the "faster" - that is enum highest - of them).
Or, could make state into a bitmask of available connection types.

http://codereview.chromium.org/9147026/diff/1/net/base/network_change_notifie...
File net/base/network_change_notifier_linux.cc (right):

http://codereview.chromium.org/9147026/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier_linux.cc:238: NetworkChangeNotifier::ETHERNET;
again, seems like UNKNOWN would be a better fit?

[droger_google, 2012-01-10 11:48:26]

Thank you joth,
I applied some comments and responded to others.

http://codereview.chromium.org/9147026/diff/1/net/base/network_change_notifie...
File net/base/network_change_notifier.cc (right):

http://codereview.chromium.org/9147026/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier.cc:77: ETHERNET;
On 2012/01/10 10:48:35, joth wrote:
> UNKNOWN, I would have thought?
> 
> Also, this probably needs a comment or TODO about fetching more detailed info,
> if possible.

I used ETHERNET as a default value, but we could totally use UNKNOWN instead.

http://codereview.chromium.org/9147026/diff/1/net/base/network_change_notifier.h
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/9147026/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier.h:107: static ConnectionState
GetConnectionState();
On 2012/01/10 10:48:35, joth wrote:
> I'm in two minds about rolling state and connection mode together into one
> setting. I guess the contract would be only show a mode beyond 'None' when
that
> interface is fully connected and operational. Most platforms go through some
> intermediate states though (e.g. connected via wifi, not yet got an IP
address)
> but I think the way the docs are worded (i.e. NONE is the only authoritative
> state, the rest are just hints) probably covers this.
> 
> 
> Also need to clarify what value would be returned if more than one interface
is
> available. (presumably report the "faster" - that is enum highest - of them).
> Or, could make state into a bitmask of available connection types.

About the bitmasks:
When there are several connections available, the network stack will chose one
of them (usually the faster), and I think we should report only this one. I
don't think the users of these notifications need to know all the connection
types that are not used, and even less to be notified of the changes in unused
connections types.

http://codereview.chromium.org/9147026/diff/1/net/base/network_change_notifie...
File net/base/network_change_notifier_mac.cc (right):

http://codereview.chromium.org/9147026/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier_mac.cc:46: while (connection_state_ == UNKNOWN)
{
Here, the Mac implementation uses UNKNOWN with a UNINITIALIZED semantic. This is
probably bad, I'll change that.

[mmenke, 2012-01-10 21:59:10]

Quick driveby comments:

Not a Chrome OS guy, but thought I'd point you at
chrome/browser/chromeos/net/network_change_notifier_chromeos.h.

Also, Chrome OS has similar states in
chrome/browser/chromeos/cros/network_library.h, which would presumably be
useful.

Don't wait for me to signoff on the change, but you'll probably want a CROS
network reviewer.

[wtc, 2012-01-12 23:27:57]

http://codereview.chromium.org/9147026/diff/8001/net/base/network_change_noti...
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/9147026/diff/8001/net/base/network_change_noti...
net/base/network_change_notifier.h:27: enum ConnectionState {

This enum type should be named ConnectionType?  I guess
you named it ConnectionState because it is the input
parameter of the OnConnectionStateChanged callback?

Please find out in our Style Guide whether enumerators should
follow the macro or constant naming convention.  Either way,
I suggest you precede the enumerator names with either
CONNECTION or Connection so that you can write 2G, 3G, for
example,
  CONNECTION_2G
  CONNECTION_3G

which are more readable.

[droger_google, 2012-05-02 11:46:07]

Reviving this very old CL.
I have fixed all the call sites now, and tested the patch on the try bots.

http://codereview.chromium.org/9147026/diff/8001/net/base/network_change_noti...
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/9147026/diff/8001/net/base/network_change_noti...
net/base/network_change_notifier.h:27: enum ConnectionState {
On 2012/01/12 23:27:57, wtc wrote:
> 
> This enum type should be named ConnectionType?  I guess
> you named it ConnectionState because it is the input
> parameter of the OnConnectionStateChanged callback?
> 
> Please find out in our Style Guide whether enumerators should
> follow the macro or constant naming convention.  Either way,
> I suggest you precede the enumerator names with either
> CONNECTION or Connection so that you can write 2G, 3G, for
> example,
>   CONNECTION_2G
>   CONNECTION_3G
> 
> which are more readable.

There is now another enum in that file which uses the macro convention.
Done.

[droger_google, 2012-05-10 15:44:00]

Added OWNERS reviewers.

[marja, 2012-05-10 15:48:23]

chrome/browser/sessions/session_restore.cc LGTM

[whywhat, 2012-05-10 15:55:53]

On 2012/05/10 15:48:23, marja wrote:
> chrome/browser/sessions/session_restore.cc LGTM

Replaced myself with Nikita as a better candidate for making the review.

[Nikita (slow), 2012-05-10 16:06:05]

+Steven

[stevenjb, 2012-05-10 17:07:30]

chroem/browser/chromeos/ changes lgtm.

[wtc, 2012-05-11 01:35:05]

Review comments on patch set 9:

This seems good.  Most of my comments below are about nits.
But some of my comments are about more substantive problems.

I don't understand why you want to add the GetConnectionType
function.  None of the code uses it, and none of the
platform-specific implementations returns the actual network
connection type.  They all return CONNECTION_UNKNOWN (or even
CONNECTION_ETHERNET) now.

It may be a good idea to keep the IsOffline() function
as a convenience function to reduce the number of changes.

http://codereview.chromium.org/9147026/diff/36003/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/9147026/diff/36003/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider_chromeos.cc:382:
net::NetworkChangeNotifier::CONNECTION_NONE);

Nit: this line doesn't eed to be indented.

http://codereview.chromium.org/9147026/diff/36003/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/9147026/diff/36003/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:1236:
net::NetworkChangeNotifier::CONNECTION_NONE) {

Nit: this line doesn't need to be indented.

http://codereview.chromium.org/9147026/diff/36003/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:1242: // end up cancelling all
request when initial network state is processed.

initial network state => initial network connection type

http://codereview.chromium.org/9147026/diff/36003/chrome/browser/net/network_...
File chrome/browser/net/network_stats.cc (right):

http://codereview.chromium.org/9147026/diff/36003/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:807:
DCHECK(net::NetworkChangeNotifier::GetConnectionType() !=

Nit: use DCHECK_NE

http://codereview.chromium.org/9147026/diff/36003/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/offline_resource_throttle.cc (right):

http://codereview.chromium.org/9147026/diff/36003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/offline_resource_throttle.cc:44:
net::NetworkChangeNotifier::CONNECTION_NONE) {

Nit: this line doesn't need to be indented. Similarly for line
133.

http://codereview.chromium.org/9147026/diff/36003/chrome/browser/sessions/ses...
File chrome/browser/sessions/session_restore.cc (right):

http://codereview.chromium.org/9147026/diff/36003/chrome/browser/sessions/ses...
chrome/browser/sessions/session_restore.cc:227:
net::NetworkChangeNotifier::CONNECTION_NONE) {

Nit: this line doesn't need to be indented.

http://codereview.chromium.org/9147026/diff/36003/net/android/network_change_...
File net/android/network_change_notifier_android.cc (right):

http://codereview.chromium.org/9147026/diff/36003/net/android/network_change_...
net/android/network_change_notifier_android.cc:40: // TODO: Return something
more detailed than CONNECTION_UNKNOWN.

Nit: add 'droger' to TODO.

http://codereview.chromium.org/9147026/diff/36003/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/9147026/diff/36003/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:745: NetworkChangeNotifier::CONNECTION_NONE) {

Nit: this line doesn't need to be indented.

http://codereview.chromium.org/9147026/diff/36003/net/base/network_change_not...
File net/base/network_change_notifier.cc (right):

http://codereview.chromium.org/9147026/diff/36003/net/base/network_change_not...
net/base/network_change_notifier.cc:32: return CONNECTION_ETHERNET;

Nit: CONNECTION_UNKNOWN seems better than CONNECTION_ETHERNET

http://codereview.chromium.org/9147026/diff/36003/net/base/network_change_not...
net/base/network_change_notifier.cc:82: return (g_network_change_notifier) ?

Nit: no need to add parentheses around g_network_change_notifier.

http://codereview.chromium.org/9147026/diff/36003/net/base/network_change_not...
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/9147026/diff/36003/net/base/network_change_not...
net/base/network_change_notifier.h:43: CONNECTION_ETHERNET

Nit: I suggest listing these enumerators in the same order
as in the Netwotk Information API:
  CONNECTION_UNKNOWN, // A connection exists, but its type is unknown.
  CONNECTION_ETHERNET
  CONNECTION_WIFI,
  CONNECTION_2G,
  CONNECTION_3G,
  CONNECTION_4G,
  CONNECTION_NONE, // No connection.

We can also list CONNECTION_NONE first so that it gets the
value 0.  But it seems that we use CONNECTION_UNKNOWN as the
default initial values.

http://codereview.chromium.org/9147026/diff/36003/net/base/network_change_not...
net/base/network_change_notifier.h:67: // about the unreliability of this
signal.

Does this comment still apply?  Is the "connection type changed"
signal also unreliable?

Perhaps we should change this comment to:
  // Will be called when the connection type of the system has changed.
  // See NetworkChangeNotifier::GetConnectionType() for important caveats
  // about the unreliability of using this signal to infer the ability to
  // reach remote sites.

Note the change from "may have changed" to "has changed".

http://codereview.chromium.org/9147026/diff/36003/net/base/network_change_not...
net/base/network_change_notifier.h:111: // A return value of |NONE| is a pretty
strong indicator that the user

|NONE| => |CONNECTION_NONE|

http://codereview.chromium.org/9147026/diff/36003/net/base/network_change_not...
net/base/network_change_notifier.h:112: // won't be able to connect to remote
sites. However, another return value is

Nit: "another return value is inconclusive" needs more rewriting because
"inconclusive" now means the returned
connection type (rather than the online/offline state) is
inconclusive.

I think we should say "another return value doesn't imply
the user will be able to connect to remote sites."  It's
a little verbose but more clear.

http://codereview.chromium.org/9147026/diff/36003/net/base/network_change_not...
net/base/network_change_notifier.h:114: // connection attempt to a particular
remote site will be successfully.

successfully => successful

http://codereview.chromium.org/9147026/diff/36003/net/base/network_change_not...
File net/base/network_change_notifier_linux.cc (right):

http://codereview.chromium.org/9147026/diff/36003/net/base/network_change_not...
net/base/network_change_notifier_linux.cc:265: // TODO: Return something more
detailed than CONNECTION_UNKNOWN.

Nit: please add 'droger' after TODO.  This is recommended
by cpplint.py.

http://codereview.chromium.org/9147026/diff/36003/net/base/network_change_not...
net/base/network_change_notifier_linux.cc:314: // Used to detect connection
state changes.

state => type

http://codereview.chromium.org/9147026/diff/36003/net/base/network_change_not...
File net/base/network_change_notifier_linux_unittest.cc (right):

http://codereview.chromium.org/9147026/diff/36003/net/base/network_change_not...
net/base/network_change_notifier_linux_unittest.cc:147:
NetworkChangeNotifier::CONNECTION_NONE);

Nit: these should ideally be replaced by EXPECT_EQ and
EXPECT_NE.  For example, this one can be replaced by

  EXPECT_EQ(NetworkChangeNotifier::CONNECTION_NONE,
            NetworkChangeNotifier::GetConnectionType());

http://codereview.chromium.org/9147026/diff/36003/net/base/network_change_not...
File net/base/network_change_notifier_mac.cc (right):

http://codereview.chromium.org/9147026/diff/36003/net/base/network_change_not...
net/base/network_change_notifier_mac.cc:11: static
NetworkChangeNotifier::ConnectionType

Nit: NetworkChangeNotifier::ConnectionType is very long.
It may make sense to un-nest it and name it
NetworkConnectionType.

http://codereview.chromium.org/9147026/diff/36003/net/base/network_change_not...
net/base/network_change_notifier_mac.cc:12:
CalculateReachability(SCNetworkConnectionFlags flags) {

IMPORTANT: It seems wrong for a function named
CalculateReachability to return a connection type.

Are you sure we need to modify this function?

It seems that the only function in this file that we need
to modify is the NetworkChangeNotifierMac::GetCurrentConnectionType()
function.  We will change
  return online_state_ == OFFLINE;
to
  return online_state_ == OFFLINE ?
      NetworkChangeNotifier::CONNECTION_NONE :
      NetworkChangeNotifier::CONNECTION_UNKNOWN;

http://codereview.chromium.org/9147026/diff/36003/net/base/network_change_not...
File net/base/network_change_notifier_mac.h (right):

http://codereview.chromium.org/9147026/diff/36003/net/base/network_change_not...
net/base/network_change_notifier_mac.h:71: bool initialized_;

initialized_ => connection_type_initialized_

http://codereview.chromium.org/9147026/diff/36003/net/base/network_change_not...
File net/base/network_change_notifier_win.cc (right):

http://codereview.chromium.org/9147026/diff/36003/net/base/network_change_not...
net/base/network_change_notifier_win.cc:87: // executing 'ipconfig /release'.

This whole comment block needs to be updated because it was
written for IsCurrentlyOffline().

http://codereview.chromium.org/9147026/diff/36003/net/base/network_change_not...
net/base/network_change_notifier_win.cc:113: return
NetworkChangeNotifier::CONNECTION_NONE;

We should return NetworkChangeNotifier::CONNECTION_UNKNOWN
here to match the original code (IsCurrentlyOffline()=false).

http://codereview.chromium.org/9147026/diff/36003/net/base/network_change_not...
net/base/network_change_notifier_win.cc:158: return found_connection ?
NetworkChangeNotifier::CONNECTION_ETHERNET :

Add a TODO comment here, because you're returning a hardcoded
CONNECTION_ETHERNET.

It seems that CONNECTION_UNKNOWN is better than
CONNECTION_ETHERNET.

http://codereview.chromium.org/9147026/diff/36003/net/base/network_change_not...
File net/base/network_change_notifier_win_unittest.cc (right):

http://codereview.chromium.org/9147026/diff/36003/net/base/network_change_not...
net/base/network_change_notifier_win_unittest.cc:36: return
NetworkChangeNotifier::CONNECTION_ETHERNET;;

Nit: CONNECTION_UNKNOWN seems better than CONNECTION_ETHERNET.

http://codereview.chromium.org/9147026/diff/36003/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/9147026/diff/36003/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_libevent.cc:112:
NetworkChangeNotifier::CONNECTION_NONE) {

Nit: this line doesn't need to be indented.

http://codereview.chromium.org/9147026/diff/36003/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_win.cc (right):

http://codereview.chromium.org/9147026/diff/36003/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_win.cc:146: NetworkChangeNotifier::CONNECTION_NONE)
{

Nit: this line doesn't need to be indented.

http://codereview.chromium.org/9147026/diff/36003/net/url_request/url_request...
File net/url_request/url_request_throttler_unittest.cc (right):

http://codereview.chromium.org/9147026/diff/36003/net/url_request/url_request...
net/url_request/url_request_throttler_unittest.cc:515:
net::NetworkChangeNotifier::CONNECTION_ETHERNET);

Nit: CONNECTION_UNKNOWN seems better than CONNECTION_ETHERNET.

http://codereview.chromium.org/9147026/diff/36003/remoting/host/signaling_con...
File remoting/host/signaling_connector.cc (right):

http://codereview.chromium.org/9147026/diff/36003/remoting/host/signaling_con...
remoting/host/signaling_connector.cc:126:
net::NetworkChangeNotifier::CONNECTION_NONE)

Nit: this line doesn't need to be indented.

[droger_google, 2012-05-11 16:10:21]

Thank you for the very detailed comments.

I put back the IsOffline() method to avoid some of the changes as you suggested.
I applied all the review comments.

However, I did not remove the GetConnectionType method, as I will need it very
soon. Furthermore, it is now used by the IsOffline method.

[jar (doing other things), 2012-05-11 18:44:51]

On 2012/05/11 01:35:05, wtc wrote:
> Review comments on patch set 9:
> 
> This seems good.  Most of my comments below are about nits.
> But some of my comments are about more substantive problems.
> 
> I don't understand why you want to add the GetConnectionType
> function.  None of the code uses it, and none of the
> platform-specific implementations returns the actual network
> connection type.  They all return CONNECTION_UNKNOWN (or even
> CONNECTION_ETHERNET) now.
> 
> It may be a good idea to keep the IsOffline() function
> as a convenience function to reduce the number of changes.
> 

Drive by commentary:

If you are looking at surfacing interesting data about the connection (and you
can access it sometimes?), it would be very interesting to surface the expected
local wire bandwidth.  For instance, understanding the expected bandwidth as
being a gigabit wire, vs a 100MB wire, vs a WIFI that it running 802.11n vs
802.11g vs 802.11b, would (in the future) be a valuable item to surface. 
Offline would of course be an expected bandwidth of 0 ;-).  I think that
applications and protocols could benefit greatly from making such data
available.  I can go into more details in private communications, but I'd very
much like to see such data surfaced.

[wtc, 2012-05-11 18:46:22]

Patch set 10 LGTM.  Thanks.

Please go over the CL's description to make sure it is still
accurate for the current patch set.  For example, the mention
of IsOffline() probably should be removed because we are
keeping IsOffline() as a convenience method now.

http://codereview.chromium.org/9147026/diff/31005/net/base/network_change_not...
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/9147026/diff/31005/net/base/network_change_not...
net/base/network_change_notifier.h:112: // Teturns the connection type.

Typo: Teturns => Returns

http://codereview.chromium.org/9147026/diff/31005/net/base/network_change_not...
File net/base/network_change_notifier_linux.cc (right):

http://codereview.chromium.org/9147026/diff/31005/net/base/network_change_not...
net/base/network_change_notifier_linux.cc:326:
::NotifyObserversOfConnectionTypeChange),

Nit: put :: on the previous line.  Our convention is to
put the operator at the end of the previous line when we
need to break a line.

http://codereview.chromium.org/9147026/diff/31005/net/base/network_change_not...
File net/base/network_change_notifier_mac.h (right):

http://codereview.chromium.org/9147026/diff/31005/net/base/network_change_not...
net/base/network_change_notifier_mac.h:62: void SetInitialState();

Nit: "InitialState", "initial_state", and "initial state"
in this file and the corresponding .cc file should probably
be changed to "InitialConnectionType", "initial_connection_type",
and "initial connection type".

http://codereview.chromium.org/9147026/diff/31005/net/base/network_change_not...
File net/base/network_change_notifier_win.h (right):

http://codereview.chromium.org/9147026/diff/31005/net/base/network_change_not...
net/base/network_change_notifier_win.h:64: // Forwards online state
notifications to parent class.

online state => network connection type

[commit-bot: I haz the power, 2012-05-23 12:14:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/droger@google.com/9147026/50003

[commit-bot: I haz the power, 2012-05-23 12:15:28]

Presubmit check for 9147026-50003 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/browser
    remoting

Presubmit checks took 10.7s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[droger_google, 2012-05-23 12:21:55]

I am still missing approval for:
- remoting (added sergeyu)
- content/browser (added sky)

[sky, 2012-05-23 14:08:31]

LGTM

[Sergey Ulanov, 2012-05-23 17:51:00]

remoting - lgtm

http://codereview.chromium.org/9147026/diff/50003/net/base/network_change_not...
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/9147026/diff/50003/net/base/network_change_not...
net/base/network_change_notifier.h:41: // Using the terminology of the Netwotk
Information API:
typo: Netwotk

[commit-bot: I haz the power, 2012-05-24 09:27:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/droger@google.com/9147026/50003

[commit-bot: I haz the power, 2012-05-24 09:33:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/droger@google.com/9147026/67003

[commit-bot: I haz the power, 2012-05-24 11:30:08]

Change committed as 138780