Add NativeLibraryPreloader interface

Add NativeLibraryPreloader interface

Add NativeLibraryPreloader interface, so we could share more of
library in certain scenario. Refer bug for details

BUG=579237
TBR=mark

Committed: https://crrev.com/c0f648c844a83732562466e733f1dac8b5db4683
Cr-Commit-Position: refs/heads/master@{#384680}

[michaelbai, 2016-03-11 19:48:34]

Description was changed from

==========
CustomizedLibraryLoader

BUG=
==========

to

==========
Add NativeLibraryPreloader interface

Add NativeLibraryPreloader interface, so we could share more of
library in certain scenario. Refer bug for details

BUG=579237
==========

[michaelbai, 2016-03-11 19:48:34]

michaelbai@chromium.org changed reviewers:
+ torne@chromium.org

[michaelbai, 2016-03-31 15:58:40]

On 2016/03/11 19:48:51, michaelbai wrote:

ping

[Torne, 2016-03-31 16:33:55]

Sorry, got buried a bit.

Just a couple of minor comments, general idea is fine.

https://codereview.chromium.org/1607993003/diff/20001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/1607993003/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:52:
private static volatile NativeLibraryPreloader sLibraryPreloader;
Don't use volatile. It's not necessary here, and any time it seems like it is
necessary is probably a time when people are writing thread-unsafe code ;)

I assume you were just copying what's done for sInstance here; I *think* that's
to try to make isInitialized safe without holding the lock, and I can't think in
enough detail about  the Java memory model this late in the afternoon to know
whether that's really safe or not, but you don't have that problem with
sLibraryPreloader so it's not needed for you in any case :)

https://codereview.chromium.org/1607993003/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:95: *
before calling System.loadLibrary.
Maybe mention in the comment that this only applies when not using the chromium
linker, in case anyone ever tries to do that.

[michaelbai, 2016-03-31 19:17:27]

PTAL

https://codereview.chromium.org/1607993003/diff/20001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/1607993003/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:52:
private static volatile NativeLibraryPreloader sLibraryPreloader;
On 2016/03/31 16:33:55, Torne wrote:
> Don't use volatile. It's not necessary here, and any time it seems like it is
> necessary is probably a time when people are writing thread-unsafe code ;)
> 
> I assume you were just copying what's done for sInstance here; I *think*
that's
> to try to make isInitialized safe without holding the lock, and I can't think
in
> enough detail about  the Java memory model this late in the afternoon to know
> whether that's really safe or not, but you don't have that problem with
> sLibraryPreloader so it's not needed for you in any case :)

Yes, I were just copying the code, volatile isn't needed, thanks

https://codereview.chromium.org/1607993003/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:95: *
before calling System.loadLibrary.
On 2016/03/31 16:33:55, Torne wrote:
> Maybe mention in the comment that this only applies when not using the
chromium
> linker, in case anyone ever tries to do that.

Done.

[Torne, 2016-03-31 19:55:30]

lgtm

[michaelbai, 2016-04-01 19:09:29]

Description was changed from

==========
Add NativeLibraryPreloader interface

Add NativeLibraryPreloader interface, so we could share more of
library in certain scenario. Refer bug for details

BUG=579237
==========

to

==========
Add NativeLibraryPreloader interface

Add NativeLibraryPreloader interface, so we could share more of
library in certain scenario. Refer bug for details

BUG=579237

TBR=mark
==========

[michaelbai, 2016-04-01 19:09:44]

The CQ bit was checked by michaelbai@chromium.org

[michaelbai, 2016-04-01 19:09:44]

The patchset sent to the CQ was uploaded after l-g-t-m from torne@chromium.org
Link to the patchset: https://codereview.chromium.org/1607993003/#ps60001
(title: "sync and add Java in BUILD.gn")

[commit-bot: I haz the power, 2016-04-01 19:10:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1607993003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1607993003/60001

[commit-bot: I haz the power, 2016-04-01 20:37:33]

Description was changed from

==========
Add NativeLibraryPreloader interface

Add NativeLibraryPreloader interface, so we could share more of
library in certain scenario. Refer bug for details

BUG=579237

TBR=mark
==========

to

==========
Add NativeLibraryPreloader interface

Add NativeLibraryPreloader interface, so we could share more of
library in certain scenario. Refer bug for details

BUG=579237

TBR=mark
==========

[commit-bot: I haz the power, 2016-04-01 20:37:34]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-01 20:39:04]

Description was changed from

==========
Add NativeLibraryPreloader interface

Add NativeLibraryPreloader interface, so we could share more of
library in certain scenario. Refer bug for details

BUG=579237

TBR=mark
==========

to

==========
Add NativeLibraryPreloader interface

Add NativeLibraryPreloader interface, so we could share more of
library in certain scenario. Refer bug for details

BUG=579237

TBR=mark

Committed: https://crrev.com/c0f648c844a83732562466e733f1dac8b5db4683
Cr-Commit-Position: refs/heads/master@{#384680}
==========

[commit-bot: I haz the power, 2016-04-01 20:39:05]

Patchset 4 (id:??) landed as
https://crrev.com/c0f648c844a83732562466e733f1dac8b5db4683
Cr-Commit-Position: refs/heads/master@{#384680}