Move the android library loader from content to base

Move the android library loader from content to base

The android library loader has no logical connection with content. Mojo
shell, which doesn't depend on content, wants to use it. This CL
moves it from content to base.

BUG=225101
NOTRY=true

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

[aberent, 2014-01-16 21:47:49]

yfriedman@chromium.org: Please review the whole CL.

darin@chromium.org: I need an OWNER review for some of the gyp(i) files for
which you seem to be the only known owner, and for content/public/app. I would
welcome your review of the rest of this CL. 

Note that many (but by no means all) the changes are just moving files or
changing import/include statements.

[Yaron, 2014-01-17 02:18:24]

Looks like there's still a bunch of comments and small bits to fix up. Please
take another look through modified files.

https://codereview.chromium.org/141223002/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/141223002/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:25: *
See also content/app/android/library_loader_hooks.cc, which contains
Tons of content references

https://codereview.chromium.org/141223002/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/library_loader/Linker.java (right):

https://codereview.chromium.org/141223002/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/library_loader/Linker.java:107: *   
process. This is because various content-based projects have vastly
comment

https://codereview.chromium.org/141223002/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/library_loader/LinkerParams.java
(right):

https://codereview.chromium.org/141223002/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/library_loader/LinkerParams.java:11: *
service one when using the content linker. For more information, read the
update

https://codereview.chromium.org/141223002/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/library_loader/LinkerParams.java:26:
"org.chromium.content.common.linker_params.base_load_address";
update these

https://codereview.chromium.org/141223002/diff/1/base/android/java/src/org/ch...
File
base/android/java/src/org/chromium/base/library_loader/ProcessInitException.java
(right):

https://codereview.chromium.org/141223002/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/library_loader/ProcessInitException.java:14:
* @param errorCode The error code could be one from
content/public/common/result_codes.h
Update

https://codereview.chromium.org/141223002/diff/1/base/android/java/templates/...
File base/android/java/templates/NativeLibraries.template (right):

https://codereview.chromium.org/141223002/diff/1/base/android/java/templates/...
base/android/java/templates/NativeLibraries.template:16: *   o
.../gen/templates/org/chromium/content/app/NativeLibraries.java
Lots of paths to update here.

https://codereview.chromium.org/141223002/diff/1/base/android/java/templates/...
File base/android/java/templates/native_libraries_array.h (right):

https://codereview.chromium.org/141223002/diff/1/base/android/java/templates/...
base/android/java/templates/native_libraries_array.h:5: // This is a placeholder
for compiling content_java.  The content java library
base_java (throughout this file)

https://codereview.chromium.org/141223002/diff/1/base/android/java/templates/...
File base/android/java/templates/native_libraries_version.h (right):

https://codereview.chromium.org/141223002/diff/1/base/android/java/templates/...
base/android/java/templates/native_libraries_version.h:5: // This is a
placeholder for compiling content_java.  The content java library
base_java (throughout this file)

https://codereview.chromium.org/141223002/diff/1/base/android/library_loader/...
File base/android/library_loader/library_loader_hooks.cc (right):

https://codereview.chromium.org/141223002/diff/1/base/android/library_loader/...
base/android/library_loader/library_loader_hooks.cc:23:
g_jni_registration_callback = func;
Don't think this needs to refer to "jni"

https://codereview.chromium.org/141223002/diff/1/content/public/app/android_l...
File content/public/app/android_library_loader_hooks.h (right):

https://codereview.chromium.org/141223002/diff/1/content/public/app/android_l...
content/public/app/android_library_loader_hooks.h:19: CONTENT_EXPORT bool
LibraryLoaded(JNIEnv* env, jclass clazz,
nit: clazz on separate line

https://codereview.chromium.org/141223002/diff/1/content/public/app/content_j...
File content/public/app/content_jni_register.h (right):

https://codereview.chromium.org/141223002/diff/1/content/public/app/content_j...
content/public/app/content_jni_register.h:18: CONTENT_EXPORT bool
EnsureJniRegistered(JNIEnv* env);
Why do you need this? Who's calling it?

https://codereview.chromium.org/141223002/diff/1/content/shell/android/browse...
File content/shell/android/browsertests_apk/content_browser_tests_android.cc
(right):

https://codereview.chromium.org/141223002/diff/1/content/shell/android/browse...
content/shell/android/browsertests_apk/content_browser_tests_android.cc:103:
startup::SetLibraryLoadedHook(&content::LibraryLoaded);
isn't this base::android ?

https://codereview.chromium.org/141223002/diff/1/content/shell/android/browse...
content/shell/android/browsertests_apk/content_browser_tests_android.cc:108: if
(!content::RegisterLibraryLoaderEntryHook(env))
I don't think this compiles

https://codereview.chromium.org/141223002/diff/1/content/shell/android/linker...
File content/shell/android/linker_test_apk/content_linker_test_android.cc
(right):

https://codereview.chromium.org/141223002/diff/1/content/shell/android/linker...
content/shell/android/linker_test_apk/content_linker_test_android.cc:21:
startup::SetLibraryLoadedHook(&content::LibraryLoaded);
base::android?

[aberent, 2014-01-30 17:46:05]

https://codereview.chromium.org/141223002/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/141223002/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:25: *
See also content/app/android/library_loader_hooks.cc, which contains
On 2014/01/17 02:18:25, Yaron wrote:
> Tons of content references

Done.

https://codereview.chromium.org/141223002/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/library_loader/Linker.java (right):

https://codereview.chromium.org/141223002/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/library_loader/Linker.java:107: *   
process. This is because various content-based projects have vastly
On 2014/01/17 02:18:25, Yaron wrote:
> comment

Done.

https://codereview.chromium.org/141223002/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/library_loader/LinkerParams.java
(right):

https://codereview.chromium.org/141223002/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/library_loader/LinkerParams.java:11: *
service one when using the content linker. For more information, read the
On 2014/01/17 02:18:25, Yaron wrote:
> update

Took another look at this, and moved this file back to content. This is a helper
for using the 'content linker' (now renamed the 'chromium linker') with content,
so belongs in content.

https://codereview.chromium.org/141223002/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/library_loader/LinkerParams.java:26:
"org.chromium.content.common.linker_params.base_load_address";
On 2014/01/17 02:18:25, Yaron wrote:
> update these
Not done, since this is back in content.

https://codereview.chromium.org/141223002/diff/1/base/android/java/src/org/ch...
File
base/android/java/src/org/chromium/base/library_loader/ProcessInitException.java
(right):

https://codereview.chromium.org/141223002/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/library_loader/ProcessInitException.java:14:
* @param errorCode The error code could be one from
content/public/common/result_codes.h
On 2014/01/17 02:18:25, Yaron wrote:
> Update

Done.

https://codereview.chromium.org/141223002/diff/1/base/android/java/templates/...
File base/android/java/templates/NativeLibraries.template (right):

https://codereview.chromium.org/141223002/diff/1/base/android/java/templates/...
base/android/java/templates/NativeLibraries.template:16: *   o
.../gen/templates/org/chromium/content/app/NativeLibraries.java
On 2014/01/17 02:18:25, Yaron wrote:
> Lots of paths to update here.

Done.

https://codereview.chromium.org/141223002/diff/1/base/android/java/templates/...
File base/android/java/templates/native_libraries_array.h (right):

https://codereview.chromium.org/141223002/diff/1/base/android/java/templates/...
base/android/java/templates/native_libraries_array.h:5: // This is a placeholder
for compiling content_java.  The content java library
On 2014/01/17 02:18:25, Yaron wrote:
> base_java (throughout this file)

Done.

https://codereview.chromium.org/141223002/diff/1/base/android/java/templates/...
File base/android/java/templates/native_libraries_version.h (right):

https://codereview.chromium.org/141223002/diff/1/base/android/java/templates/...
base/android/java/templates/native_libraries_version.h:5: // This is a
placeholder for compiling content_java.  The content java library
On 2014/01/17 02:18:25, Yaron wrote:
> base_java (throughout this file)

Done.

https://codereview.chromium.org/141223002/diff/1/base/android/library_loader/...
File base/android/library_loader/library_loader_hooks.cc (right):

https://codereview.chromium.org/141223002/diff/1/base/android/library_loader/...
base/android/library_loader/library_loader_hooks.cc:23:
g_jni_registration_callback = func;
On 2014/01/17 02:18:25, Yaron wrote:
> Don't think this needs to refer to "jni"

Done.

https://codereview.chromium.org/141223002/diff/1/content/public/app/android_l...
File content/public/app/android_library_loader_hooks.h (right):

https://codereview.chromium.org/141223002/diff/1/content/public/app/android_l...
content/public/app/android_library_loader_hooks.h:19: CONTENT_EXPORT bool
LibraryLoaded(JNIEnv* env, jclass clazz,
On 2014/01/17 02:18:25, Yaron wrote:
> nit: clazz on separate line

Done.

https://codereview.chromium.org/141223002/diff/1/content/public/app/content_j...
File content/public/app/content_jni_register.h (right):

https://codereview.chromium.org/141223002/diff/1/content/public/app/content_j...
content/public/app/content_jni_register.h:18: CONTENT_EXPORT bool
EnsureJniRegistered(JNIEnv* env);
It is called by webview_entry_point.cc, because WebView needs to register the
JNI early. I have, however, just realized that this is also declared in
android_library_loader_hooks.h, so this isn't needed.

https://codereview.chromium.org/141223002/diff/1/content/shell/android/browse...
File content/shell/android/browsertests_apk/content_browser_tests_android.cc
(right):

https://codereview.chromium.org/141223002/diff/1/content/shell/android/browse...
content/shell/android/browsertests_apk/content_browser_tests_android.cc:103:
startup::SetLibraryLoadedHook(&content::LibraryLoaded);
On 2014/01/17 02:18:25, Yaron wrote:
> isn't this base::android ?

Done.

https://codereview.chromium.org/141223002/diff/1/content/shell/android/browse...
content/shell/android/browsertests_apk/content_browser_tests_android.cc:108: if
(!content::RegisterLibraryLoaderEntryHook(env))
On 2014/01/17 02:18:25, Yaron wrote:
> I don't think this compiles
True. I hadn't realized that the 'all' target didn't build this. Fixed.

P.S. I see you have now fixed the all target to build this.

https://codereview.chromium.org/141223002/diff/1/content/shell/android/linker...
File content/shell/android/linker_test_apk/content_linker_test_android.cc
(right):

https://codereview.chromium.org/141223002/diff/1/content/shell/android/linker...
content/shell/android/linker_test_apk/content_linker_test_android.cc:21:
startup::SetLibraryLoadedHook(&content::LibraryLoaded);
On 2014/01/17 02:18:25, Yaron wrote:
> base::android?

Done.

[aberent, 2014-01-30 17:49:25]

Digit - FYI because I have moved and renamed the Content Linker as part of this.

[digit1, 2014-01-30 18:15:21]

lgtm - excellent work!

[aberent, 2014-02-03 11:31:17]

On 2014/01/30 18:15:21, digit1 wrote:
> lgtm - excellent work!

FYI - The Android downstream CL
(https://chrome-internal-review.googlesource.com/153179, Google internal only)
is now also ready for review. I have also created a preliminary CL for removing
the shim classes and TODOs (https://codereview.chromium.org/152663002/) but that
may have to change in response to comments on the other two reviews, so should
probably not be formally reviewed yet.

[Yaron, 2014-02-04 05:50:00]

lgtm

https://codereview.chromium.org/141223002/diff/330001/base/android/library_lo...
File base/android/library_loader/library_loader_hooks.h (right):

https://codereview.chromium.org/141223002/diff/330001/base/android/library_lo...
base/android/library_loader/library_loader_hooks.h:28: typedef bool
LibraryLoadedHook(JNIEnv* env, jclass clazz,
nit: one per line

https://codereview.chromium.org/141223002/diff/330001/base/android/linker/DEPS
File base/android/linker/DEPS (right):

https://codereview.chromium.org/141223002/diff/330001/base/android/linker/DEP...
base/android/linker/DEPS:2: # This code cannot depend on anything from base/ or
content/
no longer needs to mention "content"

https://codereview.chromium.org/141223002/diff/330001/content/shell/android/l...
File content/shell/android/linker_test_apk/chromium_linker_test_android.cc
(right):

https://codereview.chromium.org/141223002/diff/330001/content/shell/android/l...
content/shell/android/linker_test_apk/chromium_linker_test_android.cc:21:
base::android::SetLibraryLoadedHook(&content::LibraryLoaded);
It's not really clear what the difference is between SetLibraryLoadedHook and
RegisterLibraryLoaderEntryHook. Perhaps they can be merged in a follow-up since
one is essentially saying to register the JNI function for when the library is
loaded and the other is the callback once loading is done. Ok to do in a
follow-up

[aberent, 2014-02-05 13:40:13]

Adding benm@ as WebView reviewer:- Please check and approve the (trivial)
changes to files in android_webview directory.

darin@ - are you happy to do owner review on the remaining files (base.gyp,
base.gypi, content.gyp, content_tests.gypi, and android_library_loader_hooks.h)
or would you prefer me to find other owners?

https://codereview.chromium.org/141223002/diff/330001/base/android/library_lo...
File base/android/library_loader/library_loader_hooks.h (right):

https://codereview.chromium.org/141223002/diff/330001/base/android/library_lo...
base/android/library_loader/library_loader_hooks.h:28: typedef bool
LibraryLoadedHook(JNIEnv* env, jclass clazz,
On 2014/02/04 05:50:01, Yaron wrote:
> nit: one per line

Done.

https://codereview.chromium.org/141223002/diff/330001/base/android/linker/DEPS
File base/android/linker/DEPS (right):

https://codereview.chromium.org/141223002/diff/330001/base/android/linker/DEP...
base/android/linker/DEPS:2: # This code cannot depend on anything from base/ or
content/
On 2014/02/04 05:50:01, Yaron wrote:
> no longer needs to mention "content"

Done.

https://codereview.chromium.org/141223002/diff/330001/content/shell/android/l...
File content/shell/android/linker_test_apk/chromium_linker_test_android.cc
(right):

https://codereview.chromium.org/141223002/diff/330001/content/shell/android/l...
content/shell/android/linker_test_apk/chromium_linker_test_android.cc:21:
base::android::SetLibraryLoadedHook(&content::LibraryLoaded);
I think their functions are actually different, although they almost always both
have to be called during JNI_OnLoad. RegisterLibraryLoaderEntryHook registers,
when it is called, the JNI functions needed by the library loader itself.
SetLibraryLoadedHook sets up a function to be called when the Java side chooses
to call nativeLibraryLoaded (which, in theory, might be some time after the
library is loaded).  Having said this, if I can find a good name for what the
combined function does I will combine them in a future CL.

I did consider changing the name of RegisterLibraryLoaderEntryHook 
to something like RegisterLibraryLoaderJni, but felt I was already changing
enough in this CL.

On 2014/02/04 05:50:01, Yaron wrote:
> It's not really clear what the difference is between SetLibraryLoadedHook and
> RegisterLibraryLoaderEntryHook. Perhaps they can be merged in a follow-up
since
> one is essentially saying to register the JNI function for when the library is
> loaded and the other is the callback once loading is done. Ok to do in a
> follow-up

[benm (inactive), 2014-02-05 16:28:52]

aw lgtm

[darin (slow to review), 2014-02-07 20:48:26]

LGTM

https://codereview.chromium.org/141223002/diff/410001/base/android/library_lo...
File base/android/library_loader/library_loader_hooks.cc (right):

https://codereview.chromium.org/141223002/diff/410001/base/android/library_lo...
base/android/library_loader/library_loader_hooks.cc:30: } else {
nit: no need for else after return.

https://codereview.chromium.org/141223002/diff/410001/base/android/library_lo...
File base/android/library_loader/library_loader_hooks.h (right):

https://codereview.chromium.org/141223002/diff/410001/base/android/library_lo...
base/android/library_loader/library_loader_hooks.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
nit: 2012 -> 2014.  no "(c)" anymore.

[aberent, 2014-02-10 11:01:33]

The CQ bit was checked by aberent@chromium.org

[commit-bot: I haz the power, 2014-02-10 11:02:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aberent@chromium.org/141223002/540001

[commit-bot: I haz the power, 2014-02-10 11:27:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-10 11:27:24]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[aberent, 2014-02-10 14:02:53]

The CQ bit was checked by aberent@chromium.org

[commit-bot: I haz the power, 2014-02-10 14:03:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aberent@chromium.org/141223002/540001

[commit-bot: I haz the power, 2014-02-10 14:29:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-10 14:29:22]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[aberent, 2014-02-10 14:50:44]

The CQ bit was checked by aberent@chromium.org

[commit-bot: I haz the power, 2014-02-10 14:50:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aberent@chromium.org/141223002/540001

[commit-bot: I haz the power, 2014-02-10 15:25:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-10 15:25:09]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[aberent, 2014-02-10 16:15:53]

On 2014/02/10 15:25:09, I haz the power (commit-bot) wrote:
> Retried try job too often on chromium_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

chromium_presubmit is failing with a warning because I added to
findbugs_known_bugs.txt (temporarily, but unavoidably if I am not to break clank
builds by landing this). All other relevant bots have run successfully, so will
resubmit with NOTRY=true.

[aberent, 2014-02-10 16:16:40]

The CQ bit was checked by aberent@chromium.org

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aberent@chromium.org/141223002/540001

[commit-bot: I haz the power, 2014-02-10 16:19:26]

Change committed as 250113

[Noel Gordon, 2014-02-12 00:33:54]

https://codereview.chromium.org/141223002/diff/540001/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/141223002/diff/540001/base/base.gypi#newcode67
base/base.gypi:67: 'android/library_loader/results_codes_list.h',
http://src.chromium.org/viewvc/chrome/trunk/src/base/android/library_loader/?...
dose not seem to have 

 results_codes.h, results_codes_list.h

but they're required by base.gypi, no?

[Yaron, 2014-02-12 01:16:11]

https://codereview.chromium.org/141223002/diff/540001/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/141223002/diff/540001/base/base.gypi#newcode67
base/base.gypi:67: 'android/library_loader/results_codes_list.h',
On 2014/02/12 00:33:55, Noel Gordon (Google) wrote:
>
http://src.chromium.org/viewvc/chrome/trunk/src/base/android/library_loader/?...
> dose not seem to have 
> 
>  results_codes.h, results_codes_list.h
> 
> but they're required by base.gypi, no?
> 
> 
> 

Strange. Seems like they're actually unused and could be removed from gyp.

[Noel Gordon, 2014-02-12 01:43:19]

Let's try to remove them from base.gypi then. 
https://codereview.chromium.org/149773006