Android: Integrates native and java SystemMonitor.

Android: Integrates native and java SystemMonitor.

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

[Satish, 2012-10-04 11:09:51]

http://codereview.chromium.org/11027024/diff/1/base/system_monitor/system_mon...
File base/system_monitor/system_monitor_android.cc (right):

http://codereview.chromium.org/11027024/diff/1/base/system_monitor/system_mon...
base/system_monitor/system_monitor_android.cc:22: return
base::android::g_is_battery_charging;
Since IsBatteryPower() is a querying API, I think it is simplest to implement
the java side of it without having to register a power status listener. i.e. the
java SystemMonitor class could have an IsBatteryPower() method that does

    public static boolean isBatteryPower(Context context) {
        Intent intent = context.registerReceiver(null, new
IntentFilter(Intent.ACTION_BATTERY_CHANGED));
        int plugged = intent.getIntExtra(BatteryManager.EXTRA_PLUGGED, -1);
        return plugged == BatteryManager.BATTERY_PLUGGED_AC || plugged ==
BatteryManager.BATTERY_PLUGGED_USB;
    }

[bulach, 2012-10-04 11:16:32]

thanks satish! clarification inline:

http://codereview.chromium.org/11027024/diff/1/base/system_monitor/system_mon...
File base/system_monitor/system_monitor_android.cc (right):

http://codereview.chromium.org/11027024/diff/1/base/system_monitor/system_mon...
base/system_monitor/system_monitor_android.cc:22: return
base::android::g_is_battery_charging;
On 2012/10/04 11:09:51, Satish wrote:
> Since IsBatteryPower() is a querying API, I think it is simplest to implement
> the java side of it without having to register a power status listener. i.e.
the
> java SystemMonitor class could have an IsBatteryPower() method that does
> 
>     public static boolean isBatteryPower(Context context) {
>         Intent intent = context.registerReceiver(null, new
> IntentFilter(Intent.ACTION_BATTERY_CHANGED));
>         int plugged = intent.getIntExtra(BatteryManager.EXTRA_PLUGGED, -1);
>         return plugged == BatteryManager.BATTERY_PLUGGED_AC || plugged ==
> BatteryManager.BATTERY_PLUGGED_USB;
>     }


hmm, I'm not sure if that would save us anything..
IsBatteryPower() is not really a querying API, it's a private method withing
SystemMonitor that is called after SystemMonitor::Get()->ProcessPowerMessage
(from line 16)..

so the java side would still have to register the listener to call the function
above, and then this one would have to call up to JNI again...

does it make sense?

[Satish, 2012-10-04 11:31:07]

http://codereview.chromium.org/11027024/diff/1/base/system_monitor/system_mon...
File base/system_monitor/system_monitor_android.cc (right):

http://codereview.chromium.org/11027024/diff/1/base/system_monitor/system_mon...
base/system_monitor/system_monitor_android.cc:22: return
base::android::g_is_battery_charging;
On 2012/10/04 11:16:32, bulach wrote:
> On 2012/10/04 11:09:51, Satish wrote:
> > Since IsBatteryPower() is a querying API, I think it is simplest to
implement
> > the java side of it without having to register a power status listener. i.e.
> the
> > java SystemMonitor class could have an IsBatteryPower() method that does
> > 
> >     public static boolean isBatteryPower(Context context) {
> >         Intent intent = context.registerReceiver(null, new
> > IntentFilter(Intent.ACTION_BATTERY_CHANGED));
> >         int plugged = intent.getIntExtra(BatteryManager.EXTRA_PLUGGED, -1);
> >         return plugged == BatteryManager.BATTERY_PLUGGED_AC || plugged ==
> > BatteryManager.BATTERY_PLUGGED_USB;
> >     }
> 
> 
> hmm, I'm not sure if that would save us anything..
> IsBatteryPower() is not really a querying API, it's a private method withing
> SystemMonitor that is called after SystemMonitor::Get()->ProcessPowerMessage
> (from line 16)..
> 
> so the java side would still have to register the listener to call the
function
> above, and then this one would have to call up to JNI again...
> 
> does it make sense?

Ah yes I missed the part about informing power status change events. Yes we do
need registration.

The global variable here doesn't look right though.. just to store state that
gets used during the ProcessPowerMessage call. Can it be added as a parameter to
ProcessPowerMessage ? I see the windows implementation has a separate function
as well and it queries windows directly, whereas we are trying to save a JNI
call by storing state here which doesn't look as clean. We can either add it as
a parameter to ProcessPowerMessage or make a JNI call and store the battery
state in the java SystemMonitor object.

What do you think?

[bulach, 2012-10-04 13:58:50]

thanks satish! comments addressed, ptal..

http://codereview.chromium.org/11027024/diff/1/base/system_monitor/system_mon...
File base/system_monitor/system_monitor_android.cc (right):

http://codereview.chromium.org/11027024/diff/1/base/system_monitor/system_mon...
base/system_monitor/system_monitor_android.cc:22: return
base::android::g_is_battery_charging;
On 2012/10/04 11:31:07, Satish wrote:
> On 2012/10/04 11:16:32, bulach wrote:
> > On 2012/10/04 11:09:51, Satish wrote:
> > > Since IsBatteryPower() is a querying API, I think it is simplest to
> implement
> > > the java side of it without having to register a power status listener.
i.e.
> > the
> > > java SystemMonitor class could have an IsBatteryPower() method that does
> > > 
> > >     public static boolean isBatteryPower(Context context) {
> > >         Intent intent = context.registerReceiver(null, new
> > > IntentFilter(Intent.ACTION_BATTERY_CHANGED));
> > >         int plugged = intent.getIntExtra(BatteryManager.EXTRA_PLUGGED,
-1);
> > >         return plugged == BatteryManager.BATTERY_PLUGGED_AC || plugged ==
> > > BatteryManager.BATTERY_PLUGGED_USB;
> > >     }
> > 
> > 
> > hmm, I'm not sure if that would save us anything..
> > IsBatteryPower() is not really a querying API, it's a private method withing
> > SystemMonitor that is called after SystemMonitor::Get()->ProcessPowerMessage
> > (from line 16)..
> > 
> > so the java side would still have to register the listener to call the
> function
> > above, and then this one would have to call up to JNI again...
> > 
> > does it make sense?
> 
> Ah yes I missed the part about informing power status change events. Yes we do
> need registration.
> 
> The global variable here doesn't look right though.. just to store state that
> gets used during the ProcessPowerMessage call. Can it be added as a parameter
to
> ProcessPowerMessage ? I see the windows implementation has a separate function
> as well and it queries windows directly, whereas we are trying to save a JNI
> call by storing state here which doesn't look as clean. We can either add it
as
> a parameter to ProcessPowerMessage or make a JNI call and store the battery
> state in the java SystemMonitor object.
> 
> What do you think?

Yes, good point about the global variable.
rather than going through another JNI, since we can get the value right here,
it's now storing in SystemMonitor itself.. this way, there's a cleaner interface
from the java to native side, i.e., it passes the bool and then calls
ProcessPowerMessage internally...
wdyt?

[Satish, 2012-10-04 14:31:45]

lgtm

http://codereview.chromium.org/11027024/diff/2002/base/system_monitor/system_...
File base/system_monitor/system_monitor.h (right):

http://codereview.chromium.org/11027024/diff/2002/base/system_monitor/system_...
base/system_monitor/system_monitor.h:238: bool is_battery_charging_;
Good idea. This could also be used on windows so before sending the power
message this variable can be set by the code in system_monitor_win.cc and the
method IsBatteryPower() can be removed altogether. That way there is lesser
android specific code here.

But I'm fine with leaving this as is if OWNERS are happy with this approach.

[bulach, 2012-10-04 15:12:05]

thanks satish! 
adding vandebo, as he owns the refactoring for SystemMonitor:
http://code.google.com/p/chromium/issues/detail?id=149059


vandebo: the approach we need for android is somewhat similar to discussion in
the bug... there's a system component that listens to framework notifications,
and then it delegates down to SystemMonitor.. 
maybe we could use this approach more generic?
that is, base/system_monitor.h would be basically cross-platform data storage /
notification dispatcher, then we'd have platform-specific implementations to
tell it about new incoming data? would that work?

[vandebo (ex-Chrome), 2012-10-04 17:55:56]

On 2012/10/04 15:12:05, bulach wrote:
> thanks satish! 
> adding vandebo, as he owns the refactoring for SystemMonitor:
> http://code.google.com/p/chromium/issues/detail?id=149059
> 
> 
> vandebo: the approach we need for android is somewhat similar to discussion in
> the bug... there's a system component that listens to framework notifications,
> and then it delegates down to SystemMonitor.. 
> maybe we could use this approach more generic?
> that is, base/system_monitor.h would be basically cross-platform data storage
/
> notification dispatcher, then we'd have platform-specific implementations to
> tell it about new incoming data? would that work?

Not sure I really understand your question, so ask again if the below doesn't
answer it.

It looks like the android implementation can live completely in base so you're
not really hitting any of the issues that have triggered the need for
refactoring.

After the refactoring, we'll do away with the ProcessFoo methods.  I assume you
can't extend a C++ class with a Java class using JNI.  So probably the best
thing to do is to just friend the JNI C++ function in SystemMonitor so it can do
what it needs to do.

http://codereview.chromium.org/11027024/diff/2002/base/system_monitor/system_...
File base/system_monitor/system_monitor.h (right):

http://codereview.chromium.org/11027024/diff/2002/base/system_monitor/system_...
base/system_monitor/system_monitor.h:42: #include <jni.h>
Can you declare instead of include?

http://codereview.chromium.org/11027024/diff/2002/base/system_monitor/system_...
base/system_monitor/system_monitor.h:103: static bool
RegisterSystemMonitor(JNIEnv* env);
I'm not familiar with how JNI is being used in Chrome, so please forgive me if
this is a silly question, but why does this need to be on this class?

http://codereview.chromium.org/11027024/diff/2002/base/system_monitor/system_...
base/system_monitor/system_monitor.h:238: bool is_battery_charging_;
On 2012/10/04 14:31:45, Satish wrote:
> Good idea. This could also be used on windows so before sending the power
> message this variable can be set by the code in system_monitor_win.cc and the
> method IsBatteryPower() can be removed altogether. That way there is lesser
> android specific code here.
> 
> But I'm fine with leaving this as is if OWNERS are happy with this approach.

But adding SetBatteryIsCharging exposes more of the internal  state of
SystemMonitor as public.  I much prefer adding a parameter to the
ProcesssPowerMessage method.  It can be a generic argument that is interpreted
differently depending on the powerevent it.

http://codereview.chromium.org/11027024/diff/2002/base/system_monitor/system_...
File base/system_monitor/system_monitor_android.cc (right):

http://codereview.chromium.org/11027024/diff/2002/base/system_monitor/system_...
base/system_monitor/system_monitor_android.cc:11: void
OnBatteryChargingChanged(JNIEnv* env,
This is nativeOnBatteryChargingChanged?  A comment to that effect wouldn't hurt.
 Took me a bit to figure that out.

[bulach, 2012-10-04 18:33:33]

thanks for the clarifications vandebo!

I changed the implementation to be inline with other platforms, as in: our java
side notifies something changed, then SystemMonitor::IsBatteryPower() will do
the "system call"..

it's true that for android it can all live in base.. the instantation though is
done at the app level (which dependency wise is fine), and we also require a
"manifest" entry so that the app can listen for such notifications..

my question was more in the sense: after ProcessFoo() method goes away, is this
class still going to call up to the platform or would it hold the various system
data itself?
I mean, afaict, the platforms could notify this class passing their flags
already, right?

anyways... this patch makes android consistent with other platforms for the time
being, I'll be happy to help with the refactor later on.

another look please? thanks!

http://codereview.chromium.org/11027024/diff/2002/base/system_monitor/system_...
File base/system_monitor/system_monitor.h (right):

http://codereview.chromium.org/11027024/diff/2002/base/system_monitor/system_...
base/system_monitor/system_monitor.h:42: #include <jni.h>
On 2012/10/04 17:55:56, vandebo wrote:
> Can you declare instead of include?

unfortunately not.. :( there are some nasty typedefs and indirections for the
JNIEnv..

http://codereview.chromium.org/11027024/diff/2002/base/system_monitor/system_...
base/system_monitor/system_monitor.h:103: static bool
RegisterSystemMonitor(JNIEnv* env);
On 2012/10/04 17:55:56, vandebo wrote:
> I'm not familiar with how JNI is being used in Chrome, so please forgive me if
> this is a silly question, but why does this need to be on this class?

there's never a silly question, just poorly documented code :)
so let me clarify:
we need a way to bind / register the java side methods with the native ones. we
do all this registration via:
http://codereview.chromium.org/11027024/diff/2002/base/android/base_jni_regis...

so this method doesn't _need_ to be here, but since there are other
platform-specific initializations above, it seemed a good idea.
I could of course create a header for system_monitor_android.h and expose this
method there, but that wouldn't be entirely in line with other platforms..

happy to go either way anyways.

http://codereview.chromium.org/11027024/diff/2002/base/system_monitor/system_...
base/system_monitor/system_monitor.h:238: bool is_battery_charging_;
On 2012/10/04 17:55:56, vandebo wrote:
> On 2012/10/04 14:31:45, Satish wrote:
> > Good idea. This could also be used on windows so before sending the power
> > message this variable can be set by the code in system_monitor_win.cc and
the
> > method IsBatteryPower() can be removed altogether. That way there is lesser
> > android specific code here.
> > 
> > But I'm fine with leaving this as is if OWNERS are happy with this approach.
> 
> But adding SetBatteryIsCharging exposes more of the internal  state of
> SystemMonitor as public.  I much prefer adding a parameter to the
> ProcesssPowerMessage method.  It can be a generic argument that is interpreted
> differently depending on the powerevent it.

this was all to avoid one extra JNI hop.. since this method shouldn't be in the
critical path, and other platforms already call into the system to get the
state, I changed this to pull from java rather than the other way around..

http://codereview.chromium.org/11027024/diff/2002/base/system_monitor/system_...
File base/system_monitor/system_monitor_android.cc (right):

http://codereview.chromium.org/11027024/diff/2002/base/system_monitor/system_...
base/system_monitor/system_monitor_android.cc:11: void
OnBatteryChargingChanged(JNIEnv* env,
On 2012/10/04 17:55:56, vandebo wrote:
> This is nativeOnBatteryChargingChanged?  A comment to that effect wouldn't
hurt.
>  Took me a bit to figure that out.

done, sorry about all the magic going in here.
I can explain in lengthy details if you want, but a super high level overview:
we parse the java side and auto-generate bindings / stubs (above,
jni/SystemMonitor_jni.h).. 

these methods will then be bound to the native side via the registration
function mentioned above.

[vandebo (ex-Chrome), 2012-10-04 19:19:22]

On 2012/10/04 18:33:33, bulach wrote:
> thanks for the clarifications vandebo!
> 
> I changed the implementation to be inline with other platforms, as in: our
java
> side notifies something changed, then SystemMonitor::IsBatteryPower() will do
> the "system call"..
> 
> it's true that for android it can all live in base.. the instantation though
is
> done at the app level (which dependency wise is fine), and we also require a
> "manifest" entry so that the app can listen for such notifications..
> 
> my question was more in the sense: after ProcessFoo() method goes away, is
this
> class still going to call up to the platform or would it hold the various
system
> data itself?
> I mean, afaict, the platforms could notify this class passing their flags
> already, right?

Removable storage is going to move to chrome, device change will move to
content, but power will stay in base so that net can use it.  This is a problem
on windows since in order to implement the power notification we need to get a
window broadcast message, i.e. a dependency on a UI component.  That's why the
ProcessPower method is public right now.  Having ProcessPower being private will
work fine for android (with a friend) so it won't be an issue.

http://codereview.chromium.org/11027024/diff/2002/base/system_monitor/system_...
File base/system_monitor/system_monitor.h (right):

http://codereview.chromium.org/11027024/diff/2002/base/system_monitor/system_...
base/system_monitor/system_monitor.h:103: static bool
RegisterSystemMonitor(JNIEnv* env);
On 2012/10/04 18:33:33, bulach wrote:
> On 2012/10/04 17:55:56, vandebo wrote:
> > I'm not familiar with how JNI is being used in Chrome, so please forgive me
if
> > this is a silly question, but why does this need to be on this class?
> 
> there's never a silly question, just poorly documented code :)
> so let me clarify:
> we need a way to bind / register the java side methods with the native ones.
we
> do all this registration via:
>
http://codereview.chromium.org/11027024/diff/2002/base/android/base_jni_regis...
> 
> so this method doesn't _need_ to be here, but since there are other
> platform-specific initializations above, it seemed a good idea.
> I could of course create a header for system_monitor_android.h and expose this
> method there, but that wouldn't be entirely in line with other platforms..
> 
> happy to go either way anyways.

If it doesn't need to be a member of the class, then doing a separate header
file sounds good.

http://codereview.chromium.org/11027024/diff/13001/base/android/java/src/org/...
File base/android/java/src/org/chromium/base/SystemMonitor.java (right):

http://codereview.chromium.org/11027024/diff/13001/base/android/java/src/org/...
base/android/java/src/org/chromium/base/SystemMonitor.java:45: status ==
BatteryManager.BATTERY_STATUS_FULL;
If the battery is full, then we're not charging, true, but that doesn't mean
that we're on battery power either.  If there's a good reason to do this, then a
comment might be appropriate.

http://codereview.chromium.org/11027024/diff/13001/base/system_monitor/system...
File base/system_monitor/system_monitor_android.cc (right):

http://codereview.chromium.org/11027024/diff/13001/base/system_monitor/system...
base/system_monitor/system_monitor_android.cc:25: bool
SystemMonitor::RegisterSystemMonitor(JNIEnv* env) {
nit: // static

[vandebo (ex-Chrome), 2012-10-04 19:20:04]

Also, please fill out BUG= and TEST= appropriately.

[bulach, 2012-10-05 13:12:30]

thanks! all comments addressed, another look please?

http://codereview.chromium.org/11027024/diff/13001/base/android/java/src/org/...
File base/android/java/src/org/chromium/base/SystemMonitor.java (right):

http://codereview.chromium.org/11027024/diff/13001/base/android/java/src/org/...
base/android/java/src/org/chromium/base/SystemMonitor.java:45: status ==
BatteryManager.BATTERY_STATUS_FULL;
On 2012/10/04 19:19:22, vandebo wrote:
> If the battery is full, then we're not charging, true, but that doesn't mean
> that we're on battery power either.  If there's a good reason to do this, then
a
> comment might be appropriate.

oh, good point! using the "BATTERY_PLUGGED_" status instead.

http://codereview.chromium.org/11027024/diff/13001/base/system_monitor/system...
File base/system_monitor/system_monitor_android.cc (right):

http://codereview.chromium.org/11027024/diff/13001/base/system_monitor/system...
base/system_monitor/system_monitor_android.cc:25: bool
SystemMonitor::RegisterSystemMonitor(JNIEnv* env) {
On 2012/10/04 19:19:22, vandebo wrote:
> nit: // static

Done.

[Satish, 2012-10-05 14:13:38]

lgtm

http://codereview.chromium.org/11027024/diff/2004/base/system_monitor/system_...
File base/system_monitor/system_monitor_android.h (right):

http://codereview.chromium.org/11027024/diff/2004/base/system_monitor/system_...
base/system_monitor/system_monitor_android.h:8: 
remove empty newline

[vandebo (ex-Chrome), 2012-10-05 15:35:39]

LGTM  +jam for OWNERS

You can now omit TEST=NONE lines.  However, if there is a way for QA to easily
test this CL it wouldn't hurt to add instructions there since it's not testable
with a unit test AFAIK.

http://codereview.chromium.org/11027024/diff/2004/base/system_monitor/system_...
File base/system_monitor/system_monitor_android.cc (right):

http://codereview.chromium.org/11027024/diff/2004/base/system_monitor/system_...
base/system_monitor/system_monitor_android.cc:25: // static
nit: remove static - no longer a class member ;-)

[jam, 2012-10-05 16:14:25]

On 2012/10/05 15:35:39, vandebo wrote:
> LGTM  +jam for OWNERS

I'm not an owner for base

[bulach, 2012-10-05 16:22:49]

thanks satish, vandebo! comments addressed.

jar: this needs OWNERS approval for base/, would you mind taking a look please?

http://codereview.chromium.org/11027024/diff/2004/base/system_monitor/system_...
File base/system_monitor/system_monitor_android.cc (right):

http://codereview.chromium.org/11027024/diff/2004/base/system_monitor/system_...
base/system_monitor/system_monitor_android.cc:25: // static
On 2012/10/05 15:35:40, vandebo wrote:
> nit: remove static - no longer a class member ;-)

indeed! :) removed..

http://codereview.chromium.org/11027024/diff/2004/base/system_monitor/system_...
File base/system_monitor/system_monitor_android.h (right):

http://codereview.chromium.org/11027024/diff/2004/base/system_monitor/system_...
base/system_monitor/system_monitor_android.h:8: 
On 2012/10/05 14:13:38, Satish wrote:
> remove empty newline

Done.

[jar (doing other things), 2012-10-05 17:39:51]

Predominantly rubber stamp LGTM for base

See nit/comment below.

http://codereview.chromium.org/11027024/diff/4007/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/SystemMonitor.java (right):

http://codereview.chromium.org/11027024/diff/4007/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/SystemMonitor.java:27: IntentFilter
ifilter = new IntentFilter(Intent.ACTION_BATTERY_CHANGED);
Although the style guide allows for either 80 or 100 characters, can't we stay
under 80 (and be more like the rest of the chrome codebase)?.... or is there a
newer style??

[bulach, 2012-10-05 18:23:52]

thanks jar! clarification inline:

http://codereview.chromium.org/11027024/diff/4007/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/SystemMonitor.java (right):

http://codereview.chromium.org/11027024/diff/4007/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/SystemMonitor.java:27: IntentFilter
ifilter = new IntentFilter(Intent.ACTION_BATTERY_CHANGED);
On 2012/10/05 17:39:51, jar wrote:
> Although the style guide allows for either 80 or 100 characters, can't we stay
> under 80 (and be more like the rest of the chrome codebase)?.... or is there a
> newer style??

depends on what you call "newer" :)
for java, we follow android's style guide (most dramatic changes are indent by 4
and 100 characters)..

see the announcement I made a while ago when we started upstreaming the android
port:
https://groups.google.com/a/chromium.org/forum/?fromgroups=#!topic/chromium-d...

[bulach, 2012-10-08 13:35:42]

satish: I needed to initialize the java side in the test harness, would you mind
a quick look please?

[Satish, 2012-10-08 13:58:10]

lgtm

http://codereview.chromium.org/11027024/diff/13007/testing/android/java/src/o...
File
testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java
(right):

http://codereview.chromium.org/11027024/diff/13007/testing/android/java/src/o...
testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java:44:
// Needed by system_monitor_unittest.cc
Since this is required only by 1 test, it is overhead for all the other tests..
but because it is a c++ test I guess there is no alternative other than putting
it in common code here.

[bulach, 2012-10-08 14:01:04]

thanks satish! clarification inline, will put through the CQ once the tries are
happy.

http://codereview.chromium.org/11027024/diff/13007/testing/android/java/src/o...
File
testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java
(right):

http://codereview.chromium.org/11027024/diff/13007/testing/android/java/src/o...
testing/android/java/src/org/chromium/native_test/ChromeNativeTestActivity.java:44:
// Needed by system_monitor_unittest.cc
On 2012/10/08 13:58:10, Satish wrote:
> Since this is required only by 1 test, it is overhead for all the other
tests..
> but because it is a c++ test I guess there is no alternative other than
putting
> it in common code here.

thanks! that's correct, there are no alternatives, as the c++ test requires this
to be set up by the application / harness... note though that the overhead is
insignificant, as this is run per test suite, not per test..

[commit-bot: I haz the power, 2012-10-08 14:38:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/11027024/13007

[commit-bot: I haz the power, 2012-10-08 16:03:16]

Retried try job too often for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2012-10-16 10:02:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/11027024/13007

[commit-bot: I haz the power, 2012-10-16 11:58:10]

Change committed as 162124