Upstreaming RegisterProcessUtils()

Upstreaming RegisterProcessUtils()

Adding RegisterProcessUtils() and the chrome_jni_registrar

BUG=137674
TEST=Compile unit_tests_apk target


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

[aurimas (slooooooooow), 2012-07-24 17:04:54]

Hey Yaron,

Could you take a look at my CL?

Thanks,
Aurimas

[Yaron, 2012-07-24 17:15:47]

lgtm but please wait for satish

https://chromiumcodereview.appspot.com/10816038/diff/1/chrome/browser/android...
File chrome/browser/android/process_utils.cc (right):

https://chromiumcodereview.appspot.com/10816038/diff/1/chrome/browser/android...
chrome/browser/android/process_utils.cc:25: void CloseIdleConnectionsForProfile(
Not familiar with this. Adding Satish

https://chromiumcodereview.appspot.com/10816038/diff/1/chrome/browser/android...
chrome/browser/android/process_utils.cc:89: // TODO(pliard):
http://crbug.com/137674
Remove TODO

[aurimas (slooooooooow), 2012-07-24 18:12:14]

I removed the TODO and added intent_helper to the jni_registrar.

Aurimas

https://chromiumcodereview.appspot.com/10816038/diff/1/chrome/browser/android...
File chrome/browser/android/process_utils.cc (right):

https://chromiumcodereview.appspot.com/10816038/diff/1/chrome/browser/android...
chrome/browser/android/process_utils.cc:89: // TODO(pliard):
http://crbug.com/137674
On 2012/07/24 17:15:47, Yaron wrote:
> Remove TODO

Done.

[Satish, 2012-07-25 09:07:20]

https://chromiumcodereview.appspot.com/10816038/diff/5001/chrome/browser/andr...
File chrome/browser/android/process_utils.cc (right):

https://chromiumcodereview.appspot.com/10816038/diff/5001/chrome/browser/andr...
chrome/browser/android/process_utils.cc:25: void CloseIdleConnectionsForProfile(
where is this method being used? downstream there is the jni method 'static void
CloseIdleConnections()' in this file which you haven't included in this CL. If
this CL is only about fixing the TODO looks like you can remove this method. If
however you want to unfork the whole of process_utils.cc then I'd suggest
pulling in all methods in this file and associated java code in
ProcessUtils.java

https://chromiumcodereview.appspot.com/10816038/diff/5001/chrome/browser/andr...
chrome/browser/android/process_utils.cc:37: // TODO(satish): Remove this early
return after verifying the above doesn't
looks like this if condition and the one below can be removed as we haven't seen
the DCHECKs getting hit so far. And if they happen in the field we'll get a
crash report so would be quicker to find/fix

[aurimas (slooooooooow), 2012-07-25 15:28:29]

Hello Satish,

I've got a few questions for you. See below.

Aurimas

https://chromiumcodereview.appspot.com/10816038/diff/5001/chrome/browser/andr...
File chrome/browser/android/process_utils.cc (right):

https://chromiumcodereview.appspot.com/10816038/diff/5001/chrome/browser/andr...
chrome/browser/android/process_utils.cc:25: void CloseIdleConnectionsForProfile(
On 2012/07/25 09:07:21, Satish wrote:
> where is this method being used? downstream there is the jni method 'static
void
> CloseIdleConnections()' in this file which you haven't included in this CL. If
> this CL is only about fixing the TODO looks like you can remove this method.
If
> however you want to unfork the whole of process_utils.cc then I'd suggest
> pulling in all methods in this file and associated java code in
> ProcessUtils.java

I wanted to include CloseIdleConnections(), but when I do and try to compile I
get "cc1plus: warnings being treated as errors
chrome/browser/android/process_utils.cc:89: error: 'void
CloseIdleConnections(JNIEnv*, _jclass*)' defined but not used". Is there a way
to mark this method so it would ignore the fact that it is not used?

https://chromiumcodereview.appspot.com/10816038/diff/5001/chrome/browser/andr...
chrome/browser/android/process_utils.cc:37: // TODO(satish): Remove this early
return after verifying the above doesn't
On 2012/07/25 09:07:21, Satish wrote:
> looks like this if condition and the one below can be removed as we haven't
seen
> the DCHECKs getting hit so far. And if they happen in the field we'll get a
> crash report so would be quicker to find/fix

Do I keep the DCHECKs and just remove if statements?

[aurimas (slooooooooow), 2012-07-25 16:44:57]

I added the CloseIdleConnections() function wrapped in a #if
!defined(ANDROID_UPSTREAM_BRINGUP). It is needed as java side of jni is not
fully set up here.

[Satish, 2012-07-26 13:55:09]

On 2012/07/25 16:44:57, aurimas1 wrote:
> I added the CloseIdleConnections() function wrapped in a #if
> !defined(ANDROID_UPSTREAM_BRINGUP). It is needed as java side of jni is not
> fully set up here.

I'd prefer we wait until the chrome/ layer jni is set up and we do this CL with
the java code instead of landing it now. Otherwise the code in the ifdef doesn't
even get compiled upstream. If the jni setup is not looked at by someone else
perhaps you can pick that up?

[Yaron, 2012-07-30 17:42:56]

On 2012/07/26 13:55:09, Satish wrote:
> On 2012/07/25 16:44:57, aurimas1 wrote:
> > I added the CloseIdleConnections() function wrapped in a #if
> > !defined(ANDROID_UPSTREAM_BRINGUP). It is needed as java side of jni is not
> > fully set up here.
> 
> I'd prefer we wait until the chrome/ layer jni is set up and we do this CL
with
> the java code instead of landing it now. Otherwise the code in the ifdef
doesn't
> even get compiled upstream. If the jni setup is not looked at by someone else
> perhaps you can pick that up?

To be precise: this is setting up the chrome/ layer jni. :) I think you're
referring to Java code in the chrome/ layer. We're almost ready and I have an
approved CL for that ( http://codereview.chromium.org/10830012/) but we're
blocked on getting the checked in version of SDK/SDK tools repos in. So ya,
Aurimas, you  should add the Java code and tests.

[aurimas (slooooooooow), 2012-08-01 20:44:39]

Hello guys,

I've added java part of the Process Utils.

Aurimas

[Satish, 2012-08-02 09:36:03]

lgtm

[commit-bot: I haz the power, 2012-08-07 15:41:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/10816038/11003

[commit-bot: I haz the power, 2012-08-07 15:41:46]

Presubmit check for 10816038-11003 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.4s to calculate.

[aurimas (slooooooooow), 2012-08-07 15:44:44]

Hello Nico,

I have received LGTM's from two reviewers already. Could I get a rubber-stamp
from you as you are the owner of chrome directory.

Thanks,
Aurimas

[Nico, 2012-08-07 16:13:08]

chrome_browser.gypi change lgtm.

[commit-bot: I haz the power, 2012-08-07 16:15:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aurimas@chromium.org/10816038/11003

[commit-bot: I haz the power, 2012-08-07 18:35:01]

Change committed as 150379

[Philippe, 2012-12-03 13:17:45]

https://chromiumcodereview.appspot.com/10816038/diff/11003/chrome/browser/and...
File chrome/browser/android/process_utils.cc (right):

https://chromiumcodereview.appspot.com/10816038/diff/11003/chrome/browser/and...
chrome/browser/android/process_utils.cc:39: DCHECK(session);
Are we missing 'session->CloseIdleConnections()' here?

I came across this function while investigating a Spdy crash (bug 163616 FYI).
It seems that this function does nothing more than getting the session.
I think the call to CloseIdleConnections() got unintentionally lost during the
review process (it was there in the first patch set).

[digit1, 2012-12-03 13:42:02]

https://chromiumcodereview.appspot.com/10816038/diff/11003/chrome/browser/and...
File chrome/browser/android/process_utils.cc (right):

https://chromiumcodereview.appspot.com/10816038/diff/11003/chrome/browser/and...
chrome/browser/android/process_utils.cc:39: DCHECK(session);
Yes, this looks suspicious. What happens if you add the missing call? I.e. do
all our unit tests still pass? It might be worthwhile to try to add a regression
test if this doesn't turn out too long.

[Philippe, 2012-12-03 13:48:37]

https://chromiumcodereview.appspot.com/10816038/diff/11003/chrome/browser/and...
File chrome/browser/android/process_utils.cc (right):

https://chromiumcodereview.appspot.com/10816038/diff/11003/chrome/browser/and...
chrome/browser/android/process_utils.cc:39: DCHECK(session);
On 2012/12/03 13:42:02, digit1 wrote:
> Yes, this looks suspicious. What happens if you add the missing call? I.e. do
> all our unit tests still pass? It might be worthwhile to try to add a
regression
> test if this doesn't turn out too long.

Yes, I'm building/testing this right now. FYI, we are supposed to close idle
connections after 5min of inactivity to avoid consuming too much battery when
the app is in the background. This is triggered by the onPause() override on the
application side.
This has basically been a no-op upstream. I will send a patch fixing this soon.