Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11027024:
Android: Integrates native and java SystemMonitor. (Closed)
|
Created:
8 years, 2 months ago by bulach Modified:
8 years, 2 months ago CC:
chromium-reviews, erikwright+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAndroid: Integrates native and java SystemMonitor.
BUG=154293
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162124
Patch Set 1 #
Total comments: 4
Patch Set 2 : Comments #
Total comments: 10
Patch Set 3 : Vandebo's comments #
Total comments: 4
Patch Set 4 : Comments #
Total comments: 4
Patch Set 5 : Last nits #
Total comments: 2
Patch Set 6 : Initialize java side for tests. #
Total comments: 2
Messages
Total messages: 25 (0 generated)
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; }
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?
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?
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?
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.
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?
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.
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.
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
Also, please fill out BUG= and TEST= appropriately.
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.
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
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 ;-)
On 2012/10/05 15:35:39, vandebo wrote: > LGTM +jam for OWNERS I'm not an owner for base
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.
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??
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...
satish: I needed to initialize the java side in the test harness, would you mind a quick look please?
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.
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..
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/11027024/13007
Retried try job too often for step(s) interactive_ui_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/11027024/13007
Change committed as 162124 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
