Android: cleans up hand-written JNI for video_capture_device_android.cc

Android: cleans up hand-written JNI for video_capture_device_android.cc

It's dangerous to rely on hand-written, run-time binding for JNI.
By wrapping the usage in a thin class, we get build-time type-safety.
It also clarifies the boundaries, and make the code on each side clearer.

BUG=


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

[bulach, 2013-03-07 12:29:01]

ptal.. main motivation here is to rely the auto-generated infrastructure for
more type-safety and clarify :)
I hope you all agree that both the java and the c++ side are clearer, and the
gross details are hidden in autogen files.

[Andrew Hayden (chromium.org), 2013-03-07 15:48:49]

https://codereview.chromium.org/12604003/diff/2001/media/base/android/java/sr...
File media/base/android/java/src/org/chromium/media/VideoCapture.java (right):

https://codereview.chromium.org/12604003/diff/2001/media/base/android/java/sr...
media/base/android/java/src/org/chromium/media/VideoCapture.java:334: return 
"camera " + mId + " facing " +
There's a missing comma here that was in the original source, in case folks are
parsing the output.  Original was:
"camera %d, facing %s" (note comma after the camera id)

[Yaron, 2013-03-07 16:53:52]

lgtm

https://codereview.chromium.org/12604003/diff/2001/media/base/android/java/sr...
File media/base/android/java/src/org/chromium/media/VideoCapture.java (right):

https://codereview.chromium.org/12604003/diff/2001/media/base/android/java/sr...
media/base/android/java/src/org/chromium/media/VideoCapture.java:308: private
final int mId;
Nit: Indent all of these to 4 not 2

[bulach, 2013-03-07 17:03:54]

thanks! all comments addressed.

[wjia(left Chromium), 2013-03-07 17:26:41]

lgtm with one question and one suggestion.

Thanks for doing this! It would be great if jar_file_jni_generator.gypi can
support system inner class too. In that case, no extra class (e.g.,
ChromiumCameraInfo) is needed.

https://codereview.chromium.org/12604003/diff/5002/media/video/capture/androi...
File media/video/capture/android/video_capture_device_android.cc (right):

https://codereview.chromium.org/12604003/diff/5002/media/video/capture/androi...
media/video/capture/android/video_capture_device_android.cc:45: int num_cameras
= Java_ChromiumCameraInfo_getNumberOfCameras(env);
Just curious, any reason to move this function getNumberOfCameras into
ChromiumCameraInfo? We can do build-time binding for Camera class. Does this
mean we'd try to avoid binding for multiple java classes?

[bulach, 2013-03-08 09:58:16]

thanks both!

wjia: the problem is that the inner system class doesn't expose methods, only
fields, which we intentionally don't support: directly inspecting fields across
the boundaries breaks the encapsulation and makes it harder to follow... a
simple wrapper with clear getters (and setters, if needed), make the API between
the two environments simpler to read.

also, for the num cameras: generating the bindings for system class is somewhat
costly. as it doesn't have any "@CalledNyNative" annotation, we have to generate
bindings from every single function, and then rely on the linker to remove the
unused ones.. as above, having our own thinner wrapper helps in here, as we'd
only generate bindings for the specific, annotated functions...

the ideal pattern we try to follow is to have just one #include binding per .cc
file (which roughly maps to a java and c++ classes being paired and have well
defined counter-parts in the 'other' side, but sometimes aggregation / inner
classes in either side skew that a bit..)


I'll start writing even more docs about this, so please, feel free to keep
sending suggestion / questions!! :)

[commit-bot: I haz the power, 2013-03-08 09:58:47]

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

[commit-bot: I haz the power, 2013-03-08 09:58:49]

Failed to apply patch for media/media.gyp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file media/media.gyp
  Hunk #1 FAILED at 1418.
  1 out of 1 hunk FAILED -- saving rejects to file media/media.gyp.rej

Patch:       media/media.gyp
Index: media/media.gyp
diff --git a/media/media.gyp b/media/media.gyp
index
0b369ea85195a687b69700fb87db5eea23e8831f..adc7de08ce83555a43ac75fdbb8fbff27344ab73
100644
--- a/media/media.gyp
+++ b/media/media.gyp
@@ -1418,21 +1418,8 @@
           'includes': [ '../build/jni_generator.gypi' ],
         },
         {
-          'target_name': 'camera_jni_headers',
-          'type': 'none',
-          'variables': {
-            'jni_gen_dir': 'media',
-            'input_java_class': 'android/hardware/Camera.class',
-            'input_jar_file': '<(android_sdk)/android.jar',
-          },
-          'includes': [ '../build/jar_file_jni_generator.gypi' ],
-        },
-        {
           'target_name': 'video_capture_android_jni_headers',
           'type': 'none',
-          'dependencies': [
-            'camera_jni_headers',
-          ],
           'sources': [
             'base/android/java/src/org/chromium/media/VideoCapture.java',
           ],

[commit-bot: I haz the power, 2013-03-08 14:25:59]

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

[commit-bot: I haz the power, 2013-03-08 14:26:03]

Presubmit check for 12604003-17001 failed and returned exit status 1.

INFO:root:Found 3 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    media/media.gyp

Presubmit checks took 1.2s to calculate.

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

[bulach, 2013-03-08 15:05:00]

+dalecurtis for media/ OWNER

[DaleCurtis, 2013-03-08 20:32:47]

mostly rubberstamp lgtm % nits.

https://codereview.chromium.org/12604003/diff/17001/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/VideoCapture.java (right):

https://codereview.chromium.org/12604003/diff/17001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/VideoCapture.java:138: Log.e(TAG,
"allocate: can not find a resolution whose width is multiple of 32");
Not yours, but 80 chars please.

https://codereview.chromium.org/12604003/diff/17001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/VideoCapture.java:335:
(mCameraInfo.facing == Camera.CameraInfo.CAMERA_FACING_FRONT ?
80 chars.

[bulach, 2013-03-11 09:55:46]

thanks Dale!
nits addressed but please note that .java files follow this style-guide:
http://dev.chromium.org/developers/coding-style/java
which points to:
http://source.android.com/source/code-style.html
where line length is 100cols.. :)

amongst many reasons, its 4 space (rather than 2) identation makes it tough to
fit in 80cols.

https://codereview.chromium.org/12604003/diff/17001/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/VideoCapture.java (right):

https://codereview.chromium.org/12604003/diff/17001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/VideoCapture.java:138: Log.e(TAG,
"allocate: can not find a resolution whose width is multiple of 32");
On 2013/03/08 20:32:47, DaleCurtis wrote:
> Not yours, but 80 chars please.

Done.

https://codereview.chromium.org/12604003/diff/17001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/VideoCapture.java:335:
(mCameraInfo.facing == Camera.CameraInfo.CAMERA_FACING_FRONT ?
On 2013/03/08 20:32:47, DaleCurtis wrote:
> 80 chars.

Done.

[commit-bot: I haz the power, 2013-03-11 09:56:14]

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

[commit-bot: I haz the power, 2013-03-11 12:07:45]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-03-11 12:10:20]

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

[commit-bot: I haz the power, 2013-03-11 14:40:55]

Change committed as 187295