Android resource PAKs and native lib file path changes.

Android requires some extra functionality to set the path to it's resource pak files and native libraries.
    
This patch adds a new value to the PathService to implement the resource pak override and update the places that load pak files to take advantage of it.
    
Also adds a function for embedders to set the path to native libraries.


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

[benm (inactive), 2012-07-23 14:34:39]

ptal!

[joth, 2012-07-23 21:46:39]

https://chromiumcodereview.appspot.com/10802065/diff/2001/content/public/andr...
File content/public/android/java/src/org/chromium/content/app/LibraryLoader.java
(right):

https://chromiumcodereview.appspot.com/10802065/diff/2001/content/public/andr...
content/public/android/java/src/org/chromium/content/app/LibraryLoader.java:92:
* Override the logic for inferring the path to native libraries.
This is slightly confusing as it doesn't impact the path used for the actual
System.loadLibrary call (line 197) -- that's fully controlled be sLibrary.

Would it be sufficient to unconditionally pass sLibrary over to the native side
after the loadLibrary call?

Rather than make a new native method, how about just pass it as an additional
param of nativeLibraryLoadedOnMainThread ?
(technically we could pass it as the arv[0] of the command line array already
passed there, but it's probably not worth the effort at this point of splicing
it in and ensuring that doesn't stomp any other illicit usage of that array
slot)

[benm (inactive), 2012-07-24 17:23:18]

http://codereview.chromium.org/10802065/diff/2001/content/public/android/java...
File content/public/android/java/src/org/chromium/content/app/LibraryLoader.java
(right):

http://codereview.chromium.org/10802065/diff/2001/content/public/android/java...
content/public/android/java/src/org/chromium/content/app/LibraryLoader.java:92:
* Override the logic for inferring the path to native libraries.
On 2012/07/23 21:46:39, joth wrote:
> This is slightly confusing as it doesn't impact the path used for the actual
> System.loadLibrary call (line 197) -- that's fully controlled be sLibrary.
>
> Would it be sufficient to unconditionally pass sLibrary over to the native
side
> after the loadLibrary call?

That's a fair point. However sLibrary isn't really helpful at this point, as it
is only a name for the system library loader - it isn't a filename or path as
such (it must not have a lib prefix or .so suffix). But I see where you are
coming from with the confusion - this doesn't actually affect where the library
is loaded from in Java code, only where chromium reports it was loaded from, and
where it will try to load libraries from in native code. Hmm.

> Rather than make a new native method, how about just pass it as an additional
> param of nativeLibraryLoadedOnMainThread ?

Certainly. Although I think the public setter API is still a bit confusing, I
wonder if a different name would be better.

> (technically we could pass it as the arv[0] of the command line array already
> passed there, but it's probably not worth the effort at this point of splicing
> it in and ensuring that doesn't stomp any other illicit usage of that array
> slot)
>

[benm (inactive), 2012-08-01 12:50:43]

Please take a look!

[joth, 2012-08-01 14:57:27]

lgtm but need someone more familiar with the paths & resources upstream to check
too.

http://codereview.chromium.org/10802065/diff/11001/ui/base/ui_base_paths.h
File ui/base/ui_base_paths.h (right):

http://codereview.chromium.org/10802065/diff/11001/ui/base/ui_base_paths.h#ne...
ui/base/ui_base_paths.h:21: // Valid only in development environment;
TODO(darin): move these
nit: the 'these' comment looks like it now applies to your new define. Maybe
change to "move this" ?

[benm (inactive), 2012-08-01 15:03:30]

Thanks joth!

+sky, +willchan would you mind taking a look?

Thanks!

[sky, 2012-08-01 15:34:10]

LGTM

[benm (inactive), 2012-08-02 16:09:19]

+brettw as willchan is OOO.

Brett would you mind taking a look? Thanks!

http://codereview.chromium.org/10802065/diff/11001/ui/base/ui_base_paths.h
File ui/base/ui_base_paths.h (right):

http://codereview.chromium.org/10802065/diff/11001/ui/base/ui_base_paths.h#ne...
ui/base/ui_base_paths.h:21: // Valid only in development environment;
TODO(darin): move these
On 2012/08/01 14:57:27, joth wrote:
> nit: the 'these' comment looks like it now applies to your new define. Maybe
> change to "move this" ?

Done.

[brettw, 2012-08-02 19:38:53]

lgtm

http://codereview.chromium.org/10802065/diff/23001/base/android/path_service.cc
File base/android/path_service.cc (right):

http://codereview.chromium.org/10802065/diff/23001/base/android/path_service....
base/android/path_service.cc:12: #include "jni/PathService_jni.h"
I'd probably not put the extra blank line above this.

http://codereview.chromium.org/10802065/diff/23001/chrome/common/chrome_paths.cc
File chrome/common/chrome_paths.cc (right):

http://codereview.chromium.org/10802065/diff/23001/chrome/common/chrome_paths...
chrome/common/chrome_paths.cc:17: #if defined(OS_ANDROID)
I'd not bother with this ifdef and put the include in the group above. Generally
we only put the includes in an ifdef that won't compile, which keeps things a
bit easier to follow for files with a lot of ifdefs.

[nilesh, 2012-08-03 00:48:52]

Some drive by comments.

http://codereview.chromium.org/10802065/diff/23001/base/android/java/src/org/...
File base/android/java/src/org/chromium/base/PathService.java (right):

http://codereview.chromium.org/10802065/diff/23001/base/android/java/src/org/...
base/android/java/src/org/chromium/base/PathService.java:14: public static final
int DIR_MODULE = 3;
I think this is fragile. Its safer to have different methods to override
different paths.
nativeOverrideDirModulePath etc.

[benm (inactive), 2012-08-03 11:11:11]

Thanks for looking folks!

http://codereview.chromium.org/10802065/diff/23001/base/android/java/src/org/...
File base/android/java/src/org/chromium/base/PathService.java (right):

http://codereview.chromium.org/10802065/diff/23001/base/android/java/src/org/...
base/android/java/src/org/chromium/base/PathService.java:14: public static final
int DIR_MODULE = 3;
That's a fair point, and I did consider that approach while writing this patch.
Ultimately I opted for the const approach though for two reasons, the first
being the symmetry with the native class (this is purely intended to be a way to
call methods on the native object from Java). Secondly I think this approach
provides more flexibility to embedders as to which paths can be overridden.
Indeed, in a future patch for the WebView work I intend to add code that will
override the resource paks path. Due to layering between base/ and ui/ it would
not be possible to add a nativeOverrideResourcePaksPath here.

http://codereview.chromium.org/10802065/diff/23001/base/android/path_service.cc
File base/android/path_service.cc (right):

http://codereview.chromium.org/10802065/diff/23001/base/android/path_service....
base/android/path_service.cc:12: #include "jni/PathService_jni.h"
On 2012/08/02 19:38:53, brettw wrote:
> I'd probably not put the extra blank line above this.

Done.

http://codereview.chromium.org/10802065/diff/23001/chrome/common/chrome_paths.cc
File chrome/common/chrome_paths.cc (right):

http://codereview.chromium.org/10802065/diff/23001/chrome/common/chrome_paths...
chrome/common/chrome_paths.cc:17: #if defined(OS_ANDROID)
On 2012/08/02 19:38:53, brettw wrote:
> I'd not bother with this ifdef and put the include in the group above.
Generally
> we only put the includes in an ifdef that won't compile, which keeps things a
> bit easier to follow for files with a lot of ifdefs.

Done.

[commit-bot: I haz the power, 2012-08-03 11:11:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benm@chromium.org/10802065/14004

[commit-bot: I haz the power, 2012-08-03 12:43:31]

Change committed as 149842