Implement the skeleton of an android content shell.

Implement the skeleton of an android content shell.

Many TODOs here that will allow a quicker distribution of work to get the
remaining pieces upstreamed.

BUG=118591
TEST=


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

[Yaron, 2012-04-19 00:35:34]

Just a bunch of nits but lgtm. Probably should get jam to look at this

http://codereview.chromium.org/10035034/diff/33/content/android/java/content.xml
File content/android/java/content.xml (right):

http://codereview.chromium.org/10035034/diff/33/content/android/java/content....
content/android/java/content.xml:15: <property name="build" location="build"/>
Please use the PRODUCT_DIR approach jrg is introducing:
http://codereview.chromium.org/10051021/patch/29001/29002

http://codereview.chromium.org/10035034/diff/33/content/android/java/org/chro...
File content/android/java/org/chromium/content/browser/ContentView.java (right):

http://codereview.chromium.org/10035034/diff/33/content/android/java/org/chro...
content/android/java/org/chromium/content/browser/ContentView.java:13: *
Automatically decide the number of renderer processes to use based on device
memory class.
Add TODO for these. Or actually, why bother including them?

http://codereview.chromium.org/10035034/diff/33/content/android/java/org/chro...
content/android/java/org/chromium/content/browser/ContentView.java:43: public
static void registerPopupOverlayResourceId(int id) {
Ya, also here... may as well remove.

http://codereview.chromium.org/10035034/diff/33/content/browser/android/libra...
File content/browser/android/library_loader_hooks.cc (right):

http://codereview.chromium.org/10035034/diff/33/content/browser/android/libra...
content/browser/android/library_loader_hooks.cc:90: 
TODO(yfriedman): Add net registration

http://codereview.chromium.org/10035034/diff/33/content/browser/android/libra...
content/browser/android/library_loader_hooks.cc:106: const char
kLibraryLoaderPath[] =
I'd just omit the libraryLaoder reference for now.

http://codereview.chromium.org/10035034/diff/33/content/content_shell.gypi
File content/content_shell.gypi (right):

http://codereview.chromium.org/10035034/diff/33/content/content_shell.gypi#ne...
content/content_shell.gypi:532: 'inputs':
['<(DEPTH)/base/android/java/dist/lib/chromium_base.jar'],
Note that these are all moving to PRODUCT_DIR in the patch I mentioned. Do we
need to copy them at that point?

http://codereview.chromium.org/10035034/diff/33/content/shell/android/java/or...
File
content/shell/android/java/org/chromium/content_shell/ContentShellApplication.java
(right):

http://codereview.chromium.org/10035034/diff/33/content/shell/android/java/or...
content/shell/android/java/org/chromium/content_shell/ContentShellApplication.java:15:
private static final String NATIVE_LIBRARY = "content_shell_chromeview";
Omit these

http://codereview.chromium.org/10035034/diff/33/content/shell/android/shell_v...
File content/shell/android/shell_view.h (right):

http://codereview.chromium.org/10035034/diff/33/content/shell/android/shell_v...
content/shell/android/shell_view.h:38: bool RegisterShellView(JNIEnv* env);
Nit: Make this a static Register member in ShellView. I was asked to do this as
part of upstreaming net and it seems like a change we should make throughout.

http://codereview.chromium.org/10035034/diff/33/content/shell/shell_android.cc
File content/shell/shell_android.cc (right):

http://codereview.chromium.org/10035034/diff/33/content/shell/shell_android.c...
content/shell/shell_android.cc:44: // TODO(tedchoc): Implement Close method for
android shell
#include "base/logging.h" and add NOTIMPLEMENTED();

http://codereview.chromium.org/10035034/diff/33/content/shell/shell_browser_m...
File content/shell/shell_browser_main.cc (right):

http://codereview.chromium.org/10035034/diff/33/content/shell/shell_browser_m...
content/shell/shell_browser_main.cc:22: // shell.
Add NOTIMPLEMENTED()

http://codereview.chromium.org/10035034/diff/33/content/shell/shell_browser_m...
File content/shell/shell_browser_main_parts_android.h (right):

http://codereview.chromium.org/10035034/diff/33/content/shell/shell_browser_m...
content/shell/shell_browser_main_parts_android.h:25: scoped_ptr<MessageLoop>
main_message_loop_;
Add DISALLOW_COPY_AND_ASSIGN(ShellBrowserMainPartsAndroid)

http://codereview.chromium.org/10035034/diff/33/content/shell/shell_main_dele...
File content/shell/shell_main_delegate_android.h (right):

http://codereview.chromium.org/10035034/diff/33/content/shell/shell_main_dele...
content/shell/shell_main_delegate_android.h:29:
scoped_ptr<content::BrowserMainRunner> browser_runner_;
Add DISALLOW_COPY_AND_ASSIGN..

[Ted C, 2012-04-19 01:17:07]

Most comments resolved...need to clean up the .xml files.

http://codereview.chromium.org/10035034/diff/33/content/android/java/org/chro...
File content/android/java/org/chromium/content/browser/ContentView.java (right):

http://codereview.chromium.org/10035034/diff/33/content/android/java/org/chro...
content/android/java/org/chromium/content/browser/ContentView.java:13: *
Automatically decide the number of renderer processes to use based on device
memory class.
On 2012/04/19 00:35:34, Yaron wrote:
> Add TODO for these. Or actually, why bother including them?

Removed all but the one used.

http://codereview.chromium.org/10035034/diff/33/content/android/java/org/chro...
content/android/java/org/chromium/content/browser/ContentView.java:43: public
static void registerPopupOverlayResourceId(int id) {
On 2012/04/19 00:35:34, Yaron wrote:
> Ya, also here... may as well remove.

These are used...added TODO's and javadocs.

http://codereview.chromium.org/10035034/diff/33/content/browser/android/libra...
File content/browser/android/library_loader_hooks.cc (right):

http://codereview.chromium.org/10035034/diff/33/content/browser/android/libra...
content/browser/android/library_loader_hooks.cc:90: 
On 2012/04/19 00:35:34, Yaron wrote:
> TODO(yfriedman): Add net registration

Done.

http://codereview.chromium.org/10035034/diff/33/content/browser/android/libra...
content/browser/android/library_loader_hooks.cc:106: const char
kLibraryLoaderPath[] =
On 2012/04/19 00:35:34, Yaron wrote:
> I'd just omit the libraryLaoder reference for now.

Removed the path but kept the rest to keep the unused reference complaints down.

http://codereview.chromium.org/10035034/diff/33/content/content_shell.gypi
File content/content_shell.gypi (right):

http://codereview.chromium.org/10035034/diff/33/content/content_shell.gypi#ne...
content/content_shell.gypi:532: 'inputs':
['<(DEPTH)/base/android/java/dist/lib/chromium_base.jar'],
On 2012/04/19 00:35:34, Yaron wrote:
> Note that these are all moving to PRODUCT_DIR in the patch I mentioned. Do we
> need to copy them at that point?

Will update once the other patch lands.  They do need to be copied though as the
.apk generator expects a single libs dir and I want to isolate them to allow
different apks to be built.

http://codereview.chromium.org/10035034/diff/33/content/shell/android/java/or...
File
content/shell/android/java/org/chromium/content_shell/ContentShellApplication.java
(right):

http://codereview.chromium.org/10035034/diff/33/content/shell/android/java/or...
content/shell/android/java/org/chromium/content_shell/ContentShellApplication.java:15:
private static final String NATIVE_LIBRARY = "content_shell_chromeview";
On 2012/04/19 00:35:34, Yaron wrote:
> Omit these

Done.

http://codereview.chromium.org/10035034/diff/33/content/shell/android/shell_v...
File content/shell/android/shell_view.h (right):

http://codereview.chromium.org/10035034/diff/33/content/shell/android/shell_v...
content/shell/android/shell_view.h:38: bool RegisterShellView(JNIEnv* env);
On 2012/04/19 00:35:34, Yaron wrote:
> Nit: Make this a static Register member in ShellView. I was asked to do this
as
> part of upstreaming net and it seems like a change we should make throughout.

Done.

http://codereview.chromium.org/10035034/diff/33/content/shell/shell_android.cc
File content/shell/shell_android.cc (right):

http://codereview.chromium.org/10035034/diff/33/content/shell/shell_android.c...
content/shell/shell_android.cc:44: // TODO(tedchoc): Implement Close method for
android shell
On 2012/04/19 00:35:34, Yaron wrote:
> #include "base/logging.h" and add NOTIMPLEMENTED();

Done.

http://codereview.chromium.org/10035034/diff/33/content/shell/shell_browser_m...
File content/shell/shell_browser_main.cc (right):

http://codereview.chromium.org/10035034/diff/33/content/shell/shell_browser_m...
content/shell/shell_browser_main.cc:22: // shell.
On 2012/04/19 00:35:34, Yaron wrote:
> Add NOTIMPLEMENTED()

Done.

http://codereview.chromium.org/10035034/diff/33/content/shell/shell_browser_m...
File content/shell/shell_browser_main_parts_android.h (right):

http://codereview.chromium.org/10035034/diff/33/content/shell/shell_browser_m...
content/shell/shell_browser_main_parts_android.h:25: scoped_ptr<MessageLoop>
main_message_loop_;
On 2012/04/19 00:35:34, Yaron wrote:
> Add DISALLOW_COPY_AND_ASSIGN(ShellBrowserMainPartsAndroid)

Done.

http://codereview.chromium.org/10035034/diff/33/content/shell/shell_main_dele...
File content/shell/shell_main_delegate_android.h (right):

http://codereview.chromium.org/10035034/diff/33/content/shell/shell_main_dele...
content/shell/shell_main_delegate_android.h:29:
scoped_ptr<content::BrowserMainRunner> browser_runner_;
On 2012/04/19 00:35:34, Yaron wrote:
> Add DISALLOW_COPY_AND_ASSIGN..

Done.

[John Grabowski, 2012-04-19 18:55:27]

LGTM with tweaks and re-sync with the other CL we've been talking about

Definitely get jam (or possibly erg) to review and LGTM as well; I'm not
familiar with a number of these classes.

http://codereview.chromium.org/10035034/diff/70/base/android/jni_android.h
File base/android/jni_android.h (right):

http://codereview.chromium.org/10035034/diff/70/base/android/jni_android.h#ne...
base/android/jni_android.h:15: #define JNI_EXPORT __attribute__
((visibility("default")))
I believe this is in http://chromiumcodereview.appspot.com/10051021/ just
re-sent to the CQ

http://codereview.chromium.org/10035034/diff/70/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/10035034/diff/70/build/common.gypi#newcode2425
build/common.gypi:2425: ['_type=="shared_library" or _type=="loadable_module"',
{
Can you briefly tell me why we're using loadable_module instead of
shared_library somewhere?  I don't think it's wrong but don't quite under the
distinction in the gyp sense.

http://codereview.chromium.org/10035034/diff/70/content/android/java/content.xml
File content/android/java/content.xml (right):

http://codereview.chromium.org/10035034/diff/70/content/android/java/content....
content/android/java/content.xml:47: <jar
jarfile="${dist}/lib/chromium_content.jar" basedir="${build}"/>
output dir needs updating so we don't dump it in the source tree
See changes in
http://chromiumcodereview.appspot.com/10051021/diff/40001/base/android/java/b...

http://codereview.chromium.org/10035034/diff/70/content/browser/android/jni_h...
File content/browser/android/jni_helper.cc (right):

http://codereview.chromium.org/10035034/diff/70/content/browser/android/jni_h...
content/browser/android/jni_helper.cc:31: DLOG(ERROR) << "The real object has
been deleted!";
CHECK()?

http://codereview.chromium.org/10035034/diff/70/content/browser/android/jni_h...
File content/browser/android/jni_helper.h (right):

http://codereview.chromium.org/10035034/diff/70/content/browser/android/jni_h...
content/browser/android/jni_helper.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
add content/browser/android/OWNERS file

http://codereview.chromium.org/10035034/diff/70/content/browser/android/libra...
File content/browser/android/library_loader_hooks.cc (right):

http://codereview.chromium.org/10035034/diff/70/content/browser/android/libra...
content/browser/android/library_loader_hooks.cc:28: // required by either
ChromeView or tests. They are registered in all
ChromeView --> ContentView

http://codereview.chromium.org/10035034/diff/70/content/browser/android/libra...
content/browser/android/library_loader_hooks.cc:32: static RegistrationMethod
kContentRegisteredMethods[] = {
(see below...)

http://codereview.chromium.org/10035034/diff/70/content/browser/android/libra...
content/browser/android/library_loader_hooks.cc:37: static RegistrationMethod
kBaseRegisteredMethods[] = {
Seems like this struct should live in base, and be referenced from this file.

http://codereview.chromium.org/10035034/diff/70/content/browser/android/libra...
File content/browser/android/library_loader_hooks.h (right):

http://codereview.chromium.org/10035034/diff/70/content/browser/android/libra...
content/browser/android/library_loader_hooks.h:15: // to be exposed to the
calling java code.  This handles only registering the
Explain why minimal?  Why not all?

http://codereview.chromium.org/10035034/diff/70/content/content_shell.gypi
File content/content_shell.gypi (right):

http://codereview.chromium.org/10035034/diff/70/content/content_shell.gypi#ne...
content/content_shell.gypi:147: # This dependency is for running DRT against the
content shell, and
Not sure I understand the comment.
We need this dep, yes?  But it doesn't work yet?

http://codereview.chromium.org/10035034/diff/70/content/content_shell.gypi#ne...
content/content_shell.gypi:488:
'shell/android/java/org/chromium/content_shell/ShellManager.java',
find . -name *.java
Note plans (by yaron?) to update the jni generator script to only require a
dirname; add TODO here for that work

http://codereview.chromium.org/10035034/diff/70/content/content_shell.gypi#ne...
content/content_shell.gypi:529: 'action_name': 'copy_base_jar',
make single action which copies all jars
update jar dest to be in $(PRODUCT_DIR) [apk CL stuff]

http://codereview.chromium.org/10035034/diff/70/content/shell/android/Android...
File content/shell/android/AndroidManifest.xml (right):

http://codereview.chromium.org/10035034/diff/70/content/shell/android/Android...
content/shell/android/AndroidManifest.xml:27: <uses-sdk
android:minSdkVersion="14" android:targetSdkVersion="14" />
I believe we're using 15?

[jam, 2012-04-19 21:56:26]

http://codereview.chromium.org/10035034/diff/22002/content/android/java/conte...
File content/android/java/content.xml (right):

http://codereview.chromium.org/10035034/diff/22002/content/android/java/conte...
content/android/java/content.xml:1: <!-- Copyright (c) 2012 The Chromium
Authors. All rights reserved.
why content/android? we don't have content/win or content/mac or content/linux

http://codereview.chromium.org/10035034/diff/22002/content/browser/android/jn...
File content/browser/android/jni_helper.h (right):

http://codereview.chromium.org/10035034/diff/22002/content/browser/android/jn...
content/browser/android/jni_helper.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
I'm not familiar with android or jni, but it's not clear to me that these files
belong in content/browser. who's the intended consumer, just content_shell? if
so, why not keep them there? if chrome as well, well it can't access code in
content/browser since that's not in the public directory. or perhaps this stuff
just belongs in base/android with the rest of the android files there?

http://codereview.chromium.org/10035034/diff/22002/content/shell/shell_browse...
File content/shell/shell_browser_main_parts_android.cc (right):

http://codereview.chromium.org/10035034/diff/22002/content/shell/shell_browse...
content/shell/shell_browser_main_parts_android.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
right now we got away with using one ShellBrowserMainParts for all platforms.
since you have very little code in here (two functions), can you just reuse that
with ifdefs?

http://codereview.chromium.org/10035034/diff/22002/content/shell/shell_main_d...
File content/shell/shell_main_delegate_android.cc (right):

http://codereview.chromium.org/10035034/diff/22002/content/shell/shell_main_d...
content/shell/shell_main_delegate_android.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
ditto

please see
http://dev.chromium.org/developers/design-documents/conventions-and-patterns-...

[Ted C, 2012-04-19 23:21:06]

Resolved jrg's comments...onto jam's

http://codereview.chromium.org/10035034/diff/70/base/android/jni_android.h
File base/android/jni_android.h (right):

http://codereview.chromium.org/10035034/diff/70/base/android/jni_android.h#ne...
base/android/jni_android.h:15: #define JNI_EXPORT __attribute__
((visibility("default")))
On 2012/04/19 18:55:27, John Grabowski wrote:
> I believe this is in http://chromiumcodereview.appspot.com/10051021/ just
> re-sent to the CQ

Done.

http://codereview.chromium.org/10035034/diff/70/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/10035034/diff/70/build/common.gypi#newcode2425
build/common.gypi:2425: ['_type=="shared_library" or _type=="loadable_module"',
{
On 2012/04/19 18:55:27, John Grabowski wrote:
> Can you briefly tell me why we're using loadable_module instead of
> shared_library somewhere?  I don't think it's wrong but don't quite under the
> distinction in the gyp sense.

This was taken from downstream to make skia happy during linking.  I'm not
currently using the loadable_module portion, so I will remove it.

I did need to remove the flag you just added though -nostdlib

As it was causing errors for:
multiple definition of '__dso_handle'
multiple definition of '__INIT_ARRAY__'
multiple definition of '__FINI_ARRAY__'

http://codereview.chromium.org/10035034/diff/70/content/android/java/content.xml
File content/android/java/content.xml (right):

http://codereview.chromium.org/10035034/diff/70/content/android/java/content....
content/android/java/content.xml:47: <jar
jarfile="${dist}/lib/chromium_content.jar" basedir="${build}"/>
On 2012/04/19 18:55:27, John Grabowski wrote:
> output dir needs updating so we don't dump it in the source tree
> See changes in
>
http://chromiumcodereview.appspot.com/10051021/diff/40001/base/android/java/b...

Done.

http://codereview.chromium.org/10035034/diff/70/content/browser/android/jni_h...
File content/browser/android/jni_helper.cc (right):

http://codereview.chromium.org/10035034/diff/70/content/browser/android/jni_h...
content/browser/android/jni_helper.cc:31: DLOG(ERROR) << "The real object has
been deleted!";
On 2012/04/19 18:55:27, John Grabowski wrote:
> CHECK()?

As this is a weak reference, the underlying object could be null, so this is a
valid use case and this log is only for debugging.

http://codereview.chromium.org/10035034/diff/70/content/browser/android/jni_h...
File content/browser/android/jni_helper.h (right):

http://codereview.chromium.org/10035034/diff/70/content/browser/android/jni_h...
content/browser/android/jni_helper.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/04/19 18:55:27, John Grabowski wrote:
> add content/browser/android/OWNERS file

Done.

http://codereview.chromium.org/10035034/diff/70/content/browser/android/libra...
File content/browser/android/library_loader_hooks.cc (right):

http://codereview.chromium.org/10035034/diff/70/content/browser/android/libra...
content/browser/android/library_loader_hooks.cc:28: // required by either
ChromeView or tests. They are registered in all
On 2012/04/19 18:55:27, John Grabowski wrote:
> ChromeView --> ContentView

Done.

http://codereview.chromium.org/10035034/diff/70/content/browser/android/libra...
content/browser/android/library_loader_hooks.cc:32: static RegistrationMethod
kContentRegisteredMethods[] = {
On 2012/04/19 18:55:27, John Grabowski wrote:
> (see below...)

Done.

http://codereview.chromium.org/10035034/diff/70/content/browser/android/libra...
content/browser/android/library_loader_hooks.cc:37: static RegistrationMethod
kBaseRegisteredMethods[] = {
On 2012/04/19 18:55:27, John Grabowski wrote:
> Seems like this struct should live in base, and be referenced from this file.

Done.

http://codereview.chromium.org/10035034/diff/70/content/browser/android/libra...
File content/browser/android/library_loader_hooks.h (right):

http://codereview.chromium.org/10035034/diff/70/content/browser/android/libra...
content/browser/android/library_loader_hooks.h:15: // to be exposed to the
calling java code.  This handles only registering the
On 2012/04/19 18:55:27, John Grabowski wrote:
> Explain why minimal?  Why not all?

Done.  Basically this loads all the generic content specific callbacks and the
application specific callbacks should happen after the library has finished
loading.

http://codereview.chromium.org/10035034/diff/70/content/content_shell.gypi
File content/content_shell.gypi (right):

http://codereview.chromium.org/10035034/diff/70/content/content_shell.gypi#ne...
content/content_shell.gypi:147: # This dependency is for running DRT against the
content shell, and
On 2012/04/19 18:55:27, John Grabowski wrote:
> Not sure I understand the comment.
> We need this dep, yes?  But it doesn't work yet?

We don't need this dep yet because I #ifdef'd out the DRT specific code from
content shell.

I'm not sure what the plan is for DRT in the future, so we need to evaluate
whether we will ever need this.

http://codereview.chromium.org/10035034/diff/70/content/content_shell.gypi#ne...
content/content_shell.gypi:488:
'shell/android/java/org/chromium/content_shell/ShellManager.java',
On 2012/04/19 18:55:27, John Grabowski wrote:
> find . -name *.java
> Note plans (by yaron?) to update the jni generator script to only require a
> dirname; add TODO here for that work
> 

Done.

http://codereview.chromium.org/10035034/diff/70/content/content_shell.gypi#ne...
content/content_shell.gypi:529: 'action_name': 'copy_base_jar',
On 2012/04/19 18:55:27, John Grabowski wrote:
> make single action which copies all jars
> update jar dest to be in $(PRODUCT_DIR) [apk CL stuff]

I'd rather be explicit about what jars are needed so when we start adding chrome
jar files at some point, they will not trickle into the content shell and allow
for sloppy dependencies.

Unless of course there is a copy action that allows for multiple inputs and a
single output folder.

http://codereview.chromium.org/10035034/diff/70/content/shell/android/Android...
File content/shell/android/AndroidManifest.xml (right):

http://codereview.chromium.org/10035034/diff/70/content/shell/android/Android...
content/shell/android/AndroidManifest.xml:27: <uses-sdk
android:minSdkVersion="14" android:targetSdkVersion="14" />
On 2012/04/19 18:55:27, John Grabowski wrote:
> I believe we're using 15?

14 is the first ICS, so I think this is fine.
http://developer.android.com/guide/appendix/api-levels.html

[Ted C, 2012-04-20 01:14:13]

Just the content package structure comment remaining...adding Grace to comment. 
I believe I've covered all other comments so PTAL at everything else.

http://codereview.chromium.org/10035034/diff/22002/content/android/java/conte...
File content/android/java/content.xml (right):

http://codereview.chromium.org/10035034/diff/22002/content/android/java/conte...
content/android/java/content.xml:1: <!-- Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/04/19 21:56:26, John Abd-El-Malek wrote:
> why content/android? we don't have content/win or content/mac or content/linux

I'll let Grace comment on this one as I'm just following the convention we
decided downstream, and she will be better able to explain why.

http://codereview.chromium.org/10035034/diff/22002/content/browser/android/jn...
File content/browser/android/jni_helper.h (right):

http://codereview.chromium.org/10035034/diff/22002/content/browser/android/jn...
content/browser/android/jni_helper.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/04/19 21:56:26, John Abd-El-Malek wrote:
> I'm not familiar with android or jni, but it's not clear to me that these
files
> belong in content/browser. who's the intended consumer, just content_shell? if
> so, why not keep them there? if chrome as well, well it can't access code in
> content/browser since that's not in the public directory. or perhaps this
stuff
> just belongs in base/android with the rest of the android files there?

Done...for now it better belongs in base.  If we we need to tweak this later, we
certainly can.

http://codereview.chromium.org/10035034/diff/22002/content/shell/shell_browse...
File content/shell/shell_browser_main_parts_android.cc (right):

http://codereview.chromium.org/10035034/diff/22002/content/shell/shell_browse...
content/shell/shell_browser_main_parts_android.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2012/04/19 21:56:26, John Abd-El-Malek wrote:
> right now we got away with using one ShellBrowserMainParts for all platforms.
> since you have very little code in here (two functions), can you just reuse
that
> with ifdefs?

I have removed this and merged into ShellBrowserMainParts.  It was initially
separate just to remain consistent with our MainParts for the Chrome portion,
but it is unnecessary here.

http://codereview.chromium.org/10035034/diff/22002/content/shell/shell_main_d...
File content/shell/shell_main_delegate_android.cc (right):

http://codereview.chromium.org/10035034/diff/22002/content/shell/shell_main_d...
content/shell/shell_main_delegate_android.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2012/04/19 21:56:26, John Abd-El-Malek wrote:
> ditto
> 
> please see
>
http://dev.chromium.org/developers/design-documents/conventions-and-patterns-...
> 

Removed/merged

[klobag.chromium, 2012-04-20 01:32:25]

http://codereview.chromium.org/10035034/diff/22002/content/android/java/conte...
File content/android/java/content.xml (right):

http://codereview.chromium.org/10035034/diff/22002/content/android/java/conte...
content/android/java/content.xml:1: <!-- Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/04/19 21:56:26, John Abd-El-Malek wrote:
> why content/android? we don't have content/win or content/mac or content/linux

When we start to upstream Java files, we came up with the conclusion that we
will put all the Java files under android in each top directory. The top
directory is defined where the *.gyp is. The Java files will keep the sub
directory structure. In this case, all Java files for content will be under
content/android.

[Ted C, 2012-04-20 02:38:03]

FWIW...the content_shell_content_view will successfully generate a .so with the
following Webkit change:
http://trac.webkit.org/changeset/114697

[jam, 2012-04-20 16:15:27]

http://codereview.chromium.org/10035034/diff/22002/content/android/java/conte...
File content/android/java/content.xml (right):

http://codereview.chromium.org/10035034/diff/22002/content/android/java/conte...
content/android/java/content.xml:1: <!-- Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/04/20 01:32:25, klobag.chromium wrote:
> On 2012/04/19 21:56:26, John Abd-El-Malek wrote:
> > why content/android? we don't have content/win or content/mac or
content/linux
> 
> When we start to upstream Java files, we came up with the conclusion that we
> will put all the Java files under android in each top directory. The top
> directory is defined where the *.gyp is. The Java files will keep the sub
> directory structure. In this case, all Java files for content will be under
> content/android.

as someone not familiar with android development, can you explain to me what
these java files are used for? are they used when building an application over
content? i.e. will every android embedder of content require these files, or
just content_shell and chrome on android which is in your own repo?

[klobag.chromium, 2012-04-20 17:13:57]

http://codereview.chromium.org/10035034/diff/22002/content/android/java/conte...
File content/android/java/content.xml (right):

http://codereview.chromium.org/10035034/diff/22002/content/android/java/conte...
content/android/java/content.xml:1: <!-- Copyright (c) 2012 The Chromium
Authors. All rights reserved.
These Java files will be shared by any Android embedder, which includes
content_shell, Chrome for Android and tests.

On 2012/04/20 16:15:28, John Abd-El-Malek wrote:
> On 2012/04/20 01:32:25, klobag.chromium wrote:
> > On 2012/04/19 21:56:26, John Abd-El-Malek wrote:
> > > why content/android? we don't have content/win or content/mac or
> content/linux
> > 
> > When we start to upstream Java files, we came up with the conclusion that we
> > will put all the Java files under android in each top directory. The top
> > directory is defined where the *.gyp is. The Java files will keep the sub
> > directory structure. In this case, all Java files for content will be under
> > content/android.
> 
> as someone not familiar with android development, can you explain to me what
> these java files are used for? are they used when building an application over
> content? i.e. will every android embedder of content require these files, or
> just content_shell and chrome on android which is in your own repo?

[Ted C, 2012-04-20 20:10:16]

Peter's patch made it's way into the chrome trunk, so the .so file now links and
I added the apk dependency.

To ensure I got the gyp dependencies down, I tested with the following to ensure
it works.
rm -fR out/ && make content_shell_apk

[jam, 2012-04-20 21:40:03]

http://codereview.chromium.org/10035034/diff/22002/content/android/java/conte...
File content/android/java/content.xml (right):

http://codereview.chromium.org/10035034/diff/22002/content/android/java/conte...
content/android/java/content.xml:1: <!-- Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/04/20 17:13:57, klobag.chromium wrote:
> These Java files will be shared by any Android embedder, which includes
> content_shell, Chrome for Android and tests.

I see. In that case, how about content/public/android? chrome right now can only
access content/public and content/test. Since content/public is what embedders
are supposed to use, it seems that these files belong there?

[Ted C, 2012-04-23 21:18:31]

Moved content/android -> content/public/android

PTAL

[Ted C, 2012-04-23 21:22:53]

Adding a couple reviewers for the gyp changes in the other directories I
touched:

fischman -> media
mark -> base

[Mark Mentovai, 2012-04-23 21:27:58]

http://codereview.chromium.org/10035034/diff/43001/base/base.gypi
File base/base.gypi (right):

http://codereview.chromium.org/10035034/diff/43001/base/base.gypi#newcode24
base/base.gypi:24: 'android/base_jni_registrar.cc',
That must be some kind of joke. Your changes to this file don’t need my review.

[Ted C, 2012-04-23 21:31:12]

http://codereview.chromium.org/10035034/diff/43001/base/base.gypi
File base/base.gypi (right):

http://codereview.chromium.org/10035034/diff/43001/base/base.gypi#newcode24
base/base.gypi:24: 'android/base_jni_registrar.cc',
On 2012/04/23 21:27:59, Mark Mentovai wrote:
> That must be some kind of joke. Your changes to this file don’t need my
review.

I was under the impression that I needed someone in OWNERS if I changed this
file.  I agree that it seemed somewhat odd.  When does one need an OWNERS stamp
of approval?  Are gyp files excluded?

[Mark Mentovai, 2012-04-23 21:34:21]

It’s stupid, but if that’s all you need, LGTM. I only “reviewed” base.gypi.

[jam, 2012-04-23 22:42:05]

content lgtm

[commit-bot: I haz the power, 2012-04-24 01:57:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedchoc@chromium.org/10035034/50001

[commit-bot: I haz the power, 2012-04-24 02:26:19]

Try job failure for 10035034-50001 (retry) on android for steps "Compile,
build".
It's a second try, previously, steps "Compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-04-24 18:33:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedchoc@chromium.org/10035034/54001

[commit-bot: I haz the power, 2012-04-24 19:00:48]

Try job failure for 10035034-54001 (retry) on android for steps "Compile,
build".
It's a second try, previously, steps "Compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[Ami GONE FROM CHROMIUM, 2012-04-24 19:44:55]

Only looked at media/

http://codereview.chromium.org/10035034/diff/54001/media/base/android/media_j...
File media/base/android/media_jni_registrar.cc (right):

http://codereview.chromium.org/10035034/diff/54001/media/base/android/media_j...
media/base/android/media_jni_registrar.cc:17:
MediaPlayerBridge::RegisterMediaPlayerListener },
Is there a design doc (or any description of the overall design/plan) for the
registration business?  

Is this list expected to grow?  If not, I'm not clear on the relative value of
media::RegisterJni vs. MPB::RegisterMediaPlayerListener.  If the former is the
only code that needs to know about the latter can one of them not be a public
method (or better yet, can one of them be deleted)?

[Ted C, 2012-04-24 22:28:10]

http://codereview.chromium.org/10035034/diff/54001/media/base/android/media_j...
File media/base/android/media_jni_registrar.cc (right):

http://codereview.chromium.org/10035034/diff/54001/media/base/android/media_j...
media/base/android/media_jni_registrar.cc:17:
MediaPlayerBridge::RegisterMediaPlayerListener },
On 2012/04/24 19:44:55, Ami Fischman wrote:
> Is there a design doc (or any description of the overall design/plan) for the
> registration business?  
> 
> Is this list expected to grow?  If not, I'm not clear on the relative value of
> media::RegisterJni vs. MPB::RegisterMediaPlayerListener.  If the former is the
> only code that needs to know about the latter can one of them not be a public
> method (or better yet, can one of them be deleted)?

The idea is to limit the knowledge the library loader in content needs to know
about the internals of the component packages that need to be setup during the
native library JNI initialization.  Previous the library loader had an explicit
list of each file that needed to register it's JNI, but as part of this CL
(comment from jrg) we decided to split of the registration so each module would
expose a single register method that would handle all the necessary classes
locally.  This will hopefully provide a bit more module isolation and prevent
having to change the content directory if you are simply adding a new media
file.

I'm not sure if the media list will grow, but by using the same registrar
concept provides a consistent interface for registering modules.  I will argue
that consistency is worth the a bit of overhead in the case of media.

As for making the other method private, I would definitely like to be able to do
that if there was some kind of concept of module privacy (something like java's
package protected concept), but I don't believe such a beast exists.  Sadly, the
RegisterNatives method in MediaPlayerBridge::RegisterMediaPlayerListener must
exist in the same file that includes the *_jni.h based on how we generate these
headers, so moving that call into this file is also not really possible either.

So, I think this is probably the best we can do for now.

[Ami GONE FROM CHROMIUM, 2012-04-25 01:32:32]

Thanks for addressing my concerns.

LGTM for media/

[commit-bot: I haz the power, 2012-04-25 01:33:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedchoc@chromium.org/10035034/61002

[commit-bot: I haz the power, 2012-04-25 03:18:49]

Change committed as 133857

[Steve Block, 2012-05-10 14:53:22]

http://codereview.chromium.org/10035034/diff/61002/base/android/jni_registrar.h
File base/android/jni_registrar.h (right):

http://codereview.chromium.org/10035034/diff/61002/base/android/jni_registrar...
base/android/jni_registrar.h:14: class RegistrationMethod;
I think this should be 'struct RegistrationMethod'.

[John Grabowski, 2012-05-10 14:54:43]

Thx for review

http://codereview.chromium.org/10035034/diff/61002/base/android/jni_registrar.h
File base/android/jni_registrar.h (right):

http://codereview.chromium.org/10035034/diff/61002/base/android/jni_registrar...
base/android/jni_registrar.h:14: class RegistrationMethod;
On 2012/05/10 14:53:23, Steve Block wrote:
> I think this should be 'struct RegistrationMethod'.

I'll hit this

[jam, 2012-05-30 01:33:58]

http://codereview.chromium.org/10035034/diff/54001/content/public/browser/and...
File content/public/browser/android_library_loader_hooks.h (right):

http://codereview.chromium.org/10035034/diff/54001/content/public/browser/and...
content/public/browser/android_library_loader_hooks.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
why did this file get moved to content/public/browser? this change got a lgtm
before that happened, so if you move stuff like this again, please double check
with owners of that directory. in this case, I don't see this file being used
from outside of content so it doesn't appear to belong there.