Know the process shared library loaded in

Know the process shared library loaded in.

- Added LibraryLoader.get() to instance LibraryLoader signleton.
- Added LibraryProcessType to indicate the process it is loaded in.
- Changed BrowserStartupController to accept libraryProcessType parameter.
- Added ChromeBrowserStartupController for helping to create BrowserStartupController for Chrome.

TBR=mnaganov, ben, mark
BUG=454870
Committed: https://crrev.com/c8b2039345b270d23e02f1d4819f2049a5b9f58b
Cr-Commit-Position: refs/heads/master@{#316170}

[michaelbai, 2015-02-03 19:10:12]

michaelbai@chromium.org changed reviewers:
+ nyquist@chromium.org, tedchoc@chromium.org

[michaelbai, 2015-02-05 20:16:36]

On 2015/02/03 19:11:35, michaelbai wrote:

ping

[Ted C, 2015-02-06 01:37:44]

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

https://codereview.chromium.org/897683003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:35:
public class LibraryLoader extends LibraryProcessType {
having this extend the generated constants file is weird to me, I would just
have callers deal with the two file locations.

https://codereview.chromium.org/897683003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:458:
public static int getLibraryProcessType() {
javadoc for this as well.

https://codereview.chromium.org/897683003/diff/20001/base/android/library_loa...
File base/android/library_loader/library_process_type_list.h (right):

https://codereview.chromium.org/897683003/diff/20001/base/android/library_loa...
base/android/library_loader/library_process_type_list.h:8: // The LibraryLoad
has been initialized.
has [not]

https://codereview.chromium.org/897683003/diff/20001/base/android/library_loa...
base/android/library_loader/library_process_type_list.h:15:
LIBRARY_PROCESS_TYPE(CHILD, 2)
Can this be shared with WebView or is it specific to Chrome?

https://codereview.chromium.org/897683003/diff/20001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/897683003/diff/20001/base/base.gyp#newcode1387
base/base.gyp:1387: 'includes': [ '../build/android/java_cpp_template.gypi' ],
instead of using the template, can you use java_cpp_enum?  I think that is the
preferred way to do this from now on.

https://codereview.chromium.org/897683003/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeBrowserStartupController.java
(right):

https://codereview.chromium.org/897683003/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBrowserStartupController.java:17:
return BrowserStartupController.get(context, LibraryLoader.BROWSER);
I think this indirection is overkill.  I would just have all callers pass
BROWSER.

https://codereview.chromium.org/897683003/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
(right):

https://codereview.chromium.org/897683003/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:169:
.loadNow(getApplicationContext(), false);
should be indented 8, same below.

https://codereview.chromium.org/897683003/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
(right):

https://codereview.chromium.org/897683003/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:114:
* @param libraryProcessType the type of process the shared library is loaded. it
could be
It [must] be ..BROWSER or ..WEBVIEW.

[Peter Beverloo, 2015-02-06 11:29:49]

peter@chromium.org changed reviewers:
+ peter@chromium.org

[Peter Beverloo, 2015-02-06 11:29:50]

https://codereview.chromium.org/897683003/diff/20001/base/android/library_loa...
File base/android/library_loader/library_process_type_list.h (right):

https://codereview.chromium.org/897683003/diff/20001/base/android/library_loa...
base/android/library_loader/library_process_type_list.h:15:
LIBRARY_PROCESS_TYPE(CHILD, 2)
On 2015/02/06 01:37:43, Ted C wrote:
> Can this be shared with WebView or is it specific to Chrome?

As a drive-by, I'm not too fussed about base/ (or content/ for that matter)
knowing about WebView processes, as that's a content/ embedder and therefore a
level higher.

This may be a naive question, but what in base/ or content/ has to behave
differently if it's a WebView process that cannot be controlled using command
line flags?

[michaelbai, 2015-02-06 17:29:33]

See, my reply about the process type, I will address other comments.

https://codereview.chromium.org/897683003/diff/20001/base/android/library_loa...
File base/android/library_loader/library_process_type_list.h (right):

https://codereview.chromium.org/897683003/diff/20001/base/android/library_loa...
base/android/library_loader/library_process_type_list.h:15:
LIBRARY_PROCESS_TYPE(CHILD, 2)
On 2015/02/06 01:37:43, Ted C wrote:
> Can this be shared with WebView or is it specific to Chrome?

It is not shared with WebView as it has one process, we still need it to have
different JNI registration for chrome browser process and child process.

https://codereview.chromium.org/897683003/diff/20001/base/android/library_loa...
base/android/library_loader/library_process_type_list.h:15:
LIBRARY_PROCESS_TYPE(CHILD, 2)
On 2015/02/06 11:29:49, Peter Beverloo wrote:
> On 2015/02/06 01:37:43, Ted C wrote:
> > Can this be shared with WebView or is it specific to Chrome?
> 
> As a drive-by, I'm not too fussed about base/ (or content/ for that matter)
> knowing about WebView processes, as that's a content/ embedder and therefore a
> level higher.
> 
> This may be a naive question, but what in base/ or content/ has to behave
> differently if it's a WebView process that cannot be controlled using command
> line flags?


We need to know the process type because the JNI registration is different for
each process, the JNI registration is very first thing need to be done when
shared library is loaded, command line even is not available before JNI method
is registered, we didn't have anything special according these values in base,
the process type is set in embedder and used in embedder, it just likes command
line, is passed through base.

[Torne, 2015-02-06 17:31:56]

On 2015/02/06 17:29:33, michaelbai wrote:
> See, my reply about the process type, I will address other comments.
> 
>
https://codereview.chromium.org/897683003/diff/20001/base/android/library_loa...
> File base/android/library_loader/library_process_type_list.h (right):
> 
>
https://codereview.chromium.org/897683003/diff/20001/base/android/library_loa...
> base/android/library_loader/library_process_type_list.h:15:
> LIBRARY_PROCESS_TYPE(CHILD, 2)
> On 2015/02/06 01:37:43, Ted C wrote:
> > Can this be shared with WebView or is it specific to Chrome?
> 
> It is not shared with WebView as it has one process, we still need it to have
> different JNI registration for chrome browser process and child process.
> 
>
https://codereview.chromium.org/897683003/diff/20001/base/android/library_loa...
> base/android/library_loader/library_process_type_list.h:15:
> LIBRARY_PROCESS_TYPE(CHILD, 2)
> On 2015/02/06 11:29:49, Peter Beverloo wrote:
> > On 2015/02/06 01:37:43, Ted C wrote:
> > > Can this be shared with WebView or is it specific to Chrome?
> > 
> > As a drive-by, I'm not too fussed about base/ (or content/ for that matter)
> > knowing about WebView processes, as that's a content/ embedder and therefore
a
> > level higher.
> > 
> > This may be a naive question, but what in base/ or content/ has to behave
> > differently if it's a WebView process that cannot be controlled using
command
> > line flags?
> 
> 
> We need to know the process type because the JNI registration is different for
> each process, the JNI registration is very first thing need to be done when
> shared library is loaded, command line even is not available before JNI method
> is registered, we didn't have anything special according these values in base,
> the process type is set in embedder and used in embedder, it just likes
command
> line, is passed through base.

Michael, why do we need to use a runtime mechanism for this? I thought you
concluded that we still need to have a separate JNI_OnLoad for each binary; in
that case can't we just build it to call the right JNI registrations determined
at compile time?

[michaelbai, 2015-02-06 17:42:45]

On 2015/02/06 17:31:56, Torne wrote:
> On 2015/02/06 17:29:33, michaelbai wrote:
> > See, my reply about the process type, I will address other comments.
> > 
> >
>
https://codereview.chromium.org/897683003/diff/20001/base/android/library_loa...
> > File base/android/library_loader/library_process_type_list.h (right):
> > 
> >
>
https://codereview.chromium.org/897683003/diff/20001/base/android/library_loa...
> > base/android/library_loader/library_process_type_list.h:15:
> > LIBRARY_PROCESS_TYPE(CHILD, 2)
> > On 2015/02/06 01:37:43, Ted C wrote:
> > > Can this be shared with WebView or is it specific to Chrome?
> > 
> > It is not shared with WebView as it has one process, we still need it to
have
> > different JNI registration for chrome browser process and child process.
> > 
> >
>
https://codereview.chromium.org/897683003/diff/20001/base/android/library_loa...
> > base/android/library_loader/library_process_type_list.h:15:
> > LIBRARY_PROCESS_TYPE(CHILD, 2)
> > On 2015/02/06 11:29:49, Peter Beverloo wrote:
> > > On 2015/02/06 01:37:43, Ted C wrote:
> > > > Can this be shared with WebView or is it specific to Chrome?
> > > 
> > > As a drive-by, I'm not too fussed about base/ (or content/ for that
matter)
> > > knowing about WebView processes, as that's a content/ embedder and
therefore
> a
> > > level higher.
> > > 
> > > This may be a naive question, but what in base/ or content/ has to behave
> > > differently if it's a WebView process that cannot be controlled using
> command
> > > line flags?
> > 
> > 
> > We need to know the process type because the JNI registration is different
for
> > each process, the JNI registration is very first thing need to be done when
> > shared library is loaded, command line even is not available before JNI
method
> > is registered, we didn't have anything special according these values in
base,
> > the process type is set in embedder and used in embedder, it just likes
> command
> > line, is passed through base.
> 
> Michael, why do we need to use a runtime mechanism for this? I thought you
> concluded that we still need to have a separate JNI_OnLoad for each binary; in
> that case can't we just build it to call the right JNI registrations
determined
> at compile time?

Since the JNI registration and initialization is separated, as to clank, we need
process type to have different registration and initialization for browser and
child process. as to MonoChrome, the JNI registration is not needed, but we
still need different initialization for WebView, Browser process and child
process.

[Torne, 2015-02-06 18:44:22]

On 2015/02/06 17:42:45, michaelbai wrote:
> On 2015/02/06 17:31:56, Torne wrote:
> > On 2015/02/06 17:29:33, michaelbai wrote:
> > > See, my reply about the process type, I will address other comments.
> > > 
> > >
> >
>
https://codereview.chromium.org/897683003/diff/20001/base/android/library_loa...
> > > File base/android/library_loader/library_process_type_list.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/897683003/diff/20001/base/android/library_loa...
> > > base/android/library_loader/library_process_type_list.h:15:
> > > LIBRARY_PROCESS_TYPE(CHILD, 2)
> > > On 2015/02/06 01:37:43, Ted C wrote:
> > > > Can this be shared with WebView or is it specific to Chrome?
> > > 
> > > It is not shared with WebView as it has one process, we still need it to
> have
> > > different JNI registration for chrome browser process and child process.
> > > 
> > >
> >
>
https://codereview.chromium.org/897683003/diff/20001/base/android/library_loa...
> > > base/android/library_loader/library_process_type_list.h:15:
> > > LIBRARY_PROCESS_TYPE(CHILD, 2)
> > > On 2015/02/06 11:29:49, Peter Beverloo wrote:
> > > > On 2015/02/06 01:37:43, Ted C wrote:
> > > > > Can this be shared with WebView or is it specific to Chrome?
> > > > 
> > > > As a drive-by, I'm not too fussed about base/ (or content/ for that
> matter)
> > > > knowing about WebView processes, as that's a content/ embedder and
> therefore
> > a
> > > > level higher.
> > > > 
> > > > This may be a naive question, but what in base/ or content/ has to
behave
> > > > differently if it's a WebView process that cannot be controlled using
> > command
> > > > line flags?
> > > 
> > > 
> > > We need to know the process type because the JNI registration is different
> for
> > > each process, the JNI registration is very first thing need to be done
when
> > > shared library is loaded, command line even is not available before JNI
> method
> > > is registered, we didn't have anything special according these values in
> base,
> > > the process type is set in embedder and used in embedder, it just likes
> > command
> > > line, is passed through base.
> > 
> > Michael, why do we need to use a runtime mechanism for this? I thought you
> > concluded that we still need to have a separate JNI_OnLoad for each binary;
in
> > that case can't we just build it to call the right JNI registrations
> determined
> > at compile time?
> 
> Since the JNI registration and initialization is separated, as to clank, we
need
> process type to have different registration and initialization for browser and
> child process. as to MonoChrome, the JNI registration is not needed, but we
> still need different initialization for WebView, Browser process and child
> process.

Surely Clank already has this problem and some solution for it, though. If it
needs to do different initialisation in browser and renderer processes, isn't
there already a mechanism for that? I guess I'm not understanding some larger
picture here :)

[michaelbai, 2015-02-06 19:08:20]

On 2015/02/06 18:44:22, Torne wrote:
> On 2015/02/06 17:42:45, michaelbai wrote:
> > On 2015/02/06 17:31:56, Torne wrote:
> > > On 2015/02/06 17:29:33, michaelbai wrote:
> > > > See, my reply about the process type, I will address other comments.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/897683003/diff/20001/base/android/library_loa...
> > > > File base/android/library_loader/library_process_type_list.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/897683003/diff/20001/base/android/library_loa...
> > > > base/android/library_loader/library_process_type_list.h:15:
> > > > LIBRARY_PROCESS_TYPE(CHILD, 2)
> > > > On 2015/02/06 01:37:43, Ted C wrote:
> > > > > Can this be shared with WebView or is it specific to Chrome?
> > > > 
> > > > It is not shared with WebView as it has one process, we still need it to
> > have
> > > > different JNI registration for chrome browser process and child process.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/897683003/diff/20001/base/android/library_loa...
> > > > base/android/library_loader/library_process_type_list.h:15:
> > > > LIBRARY_PROCESS_TYPE(CHILD, 2)
> > > > On 2015/02/06 11:29:49, Peter Beverloo wrote:
> > > > > On 2015/02/06 01:37:43, Ted C wrote:
> > > > > > Can this be shared with WebView or is it specific to Chrome?
> > > > > 
> > > > > As a drive-by, I'm not too fussed about base/ (or content/ for that
> > matter)
> > > > > knowing about WebView processes, as that's a content/ embedder and
> > therefore
> > > a
> > > > > level higher.
> > > > > 
> > > > > This may be a naive question, but what in base/ or content/ has to
> behave
> > > > > differently if it's a WebView process that cannot be controlled using
> > > command
> > > > > line flags?
> > > > 
> > > > 
> > > > We need to know the process type because the JNI registration is
different
> > for
> > > > each process, the JNI registration is very first thing need to be done
> when
> > > > shared library is loaded, command line even is not available before JNI
> > method
> > > > is registered, we didn't have anything special according these values in
> > base,
> > > > the process type is set in embedder and used in embedder, it just likes
> > > command
> > > > line, is passed through base.
> > > 
> > > Michael, why do we need to use a runtime mechanism for this? I thought you
> > > concluded that we still need to have a separate JNI_OnLoad for each
binary;
> in
> > > that case can't we just build it to call the right JNI registrations
> > determined
> > > at compile time?
> > 
> > Since the JNI registration and initialization is separated, as to clank, we
> need
> > process type to have different registration and initialization for browser
and
> > child process. as to MonoChrome, the JNI registration is not needed, but we
> > still need different initialization for WebView, Browser process and child
> > process.
> 
> Surely Clank already has this problem and some solution for it, though. If it
> needs to do different initialisation in browser and renderer processes, isn't
> there already a mechanism for that? I guess I'm not understanding some larger
> picture here :)

Clank relies on the command line for different registration, as we will move the
registration to the one place, current way will not work

[michaelbai, 2015-02-10 05:17:31]

PTAL

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

https://codereview.chromium.org/897683003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:35:
public class LibraryLoader extends LibraryProcessType {
On 2015/02/06 01:37:43, Ted C wrote:
> having this extend the generated constants file is weird to me, I would just
> have callers deal with the two file locations.

Done.

https://codereview.chromium.org/897683003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:458:
public static int getLibraryProcessType() {
On 2015/02/06 01:37:43, Ted C wrote:
> javadoc for this as well.

Done.

https://codereview.chromium.org/897683003/diff/20001/base/android/library_loa...
File base/android/library_loader/library_process_type_list.h (right):

https://codereview.chromium.org/897683003/diff/20001/base/android/library_loa...
base/android/library_loader/library_process_type_list.h:8: // The LibraryLoad
has been initialized.
On 2015/02/06 01:37:43, Ted C wrote:
> has [not]

Done.

https://codereview.chromium.org/897683003/diff/20001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/897683003/diff/20001/base/base.gyp#newcode1387
base/base.gyp:1387: 'includes': [ '../build/android/java_cpp_template.gypi' ],
On 2015/02/06 01:37:43, Ted C wrote:
> instead of using the template, can you use java_cpp_enum?  I think that is the
> preferred way to do this from now on.

Done.

https://codereview.chromium.org/897683003/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeBrowserStartupController.java
(right):

https://codereview.chromium.org/897683003/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBrowserStartupController.java:17:
return BrowserStartupController.get(context, LibraryLoader.BROWSER);
On 2015/02/06 01:37:43, Ted C wrote:
> I think this indirection is overkill.  I would just have all callers pass
> BROWSER.

Done.

https://codereview.chromium.org/897683003/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
(right):

https://codereview.chromium.org/897683003/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:169:
.loadNow(getApplicationContext(), false);
On 2015/02/06 01:37:43, Ted C wrote:
> should be indented 8, same below.

Done.

https://codereview.chromium.org/897683003/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
(right):

https://codereview.chromium.org/897683003/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:114:
* @param libraryProcessType the type of process the shared library is loaded. it
could be
On 2015/02/06 01:37:43, Ted C wrote:
> It [must] be ..BROWSER or ..WEBVIEW.

Done.

[Ted C, 2015-02-10 19:32:43]

this seems ok to me now, but I'd like to hear that torne's questions are
properly addressed.

https://codereview.chromium.org/897683003/diff/40001/content/shell/android/li...
File
content/shell/android/linker_test_apk/src/org/chromium/chromium_linker_test_apk/ChromiumLinkerTestActivity.java
(right):

https://codereview.chromium.org/897683003/diff/40001/content/shell/android/li...
content/shell/android/linker_test_apk/src/org/chromium/chromium_linker_test_apk/ChromiumLinkerTestActivity.java:112:
new BrowserStartupController.StartupCallback() {
this block only needs to be indented 8, so it looks indented 4 too many

[Torne, 2015-02-10 19:58:05]

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

[Torne, 2015-02-10 19:58:06]

LGTM. Sorry for disappearing on this thread, it got lost in my inbox. After
reading some of the related changes to the JNI startup code this approach makes
sense.

[Ted C, 2015-02-10 20:00:56]

lgtm

[michaelbai, 2015-02-10 20:18:17]

michaelbai@chromium.org changed reviewers:
+ agl@chromium.org, ben@chromium.org, mnaganov@chromium.org, zea@chromium.org

[michaelbai, 2015-02-10 20:19:44]

PTAL 

nyquist@ for base/android
ben@ for mojo

[michaelbai, 2015-02-10 20:21:09]

PTAL 

nyquist@ for base/android
ben@ for mojo
agl@ for net
zea@ for component/gcm_driver
mnaganov@ for component/devtools_bridge

[agl, 2015-02-11 01:23:11]

RS LGTM for net/

[Nicolas Zea, 2015-02-11 18:52:36]

gcm LGTM

[nyquist, 2015-02-12 00:42:47]

https://codereview.chromium.org/897683003/diff/40001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/897683003/diff/40001/base/BUILD.gn#newcode1537
base/BUILD.gn:1537: # GYP: //base/base.gyp:base_library_process_type_gen
I think this one should go away now.

https://codereview.chromium.org/897683003/diff/40001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/897683003/diff/40001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:47:
private boolean mLoaded = false;
Member fields have false as their default value. Remove = false for all these
members?

https://codereview.chromium.org/897683003/diff/40001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/897683003/diff/40001/base/base.gyp#newcode1378
base/base.gyp:1378: 'target_name': 'base_library_process_type',
base_java_library_process_type. Same for GN.

[michaelbai, 2015-02-12 04:37:57]

New patchsets have been uploaded after l-g-t-m from
torne@chromium.org,tedchoc@chromium.org,agl@chromium.org,zea@chromium.org

[michaelbai, 2015-02-12 04:43:51]

Patchset #4 (id:60001) has been deleted

[michaelbai, 2015-02-12 06:43:58]

Patchset #4 (id:80001) has been deleted

[michaelbai, 2015-02-12 06:48:28]

PTAL, I also removed the default BrowserStartupController.get(Context), it will
not make the downstream merge smoothly, as LibraryLoader is also used in
downstream.

https://codereview.chromium.org/897683003/diff/40001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/897683003/diff/40001/base/BUILD.gn#newcode1537
base/BUILD.gn:1537: # GYP: //base/base.gyp:base_library_process_type_gen
On 2015/02/12 00:42:46, nyquist wrote:
> I think this one should go away now.

Done.

https://codereview.chromium.org/897683003/diff/40001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/897683003/diff/40001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:47:
private boolean mLoaded = false;
On 2015/02/12 00:42:46, nyquist wrote:
> Member fields have false as their default value. Remove = false for all these
> members?

Done.

https://codereview.chromium.org/897683003/diff/40001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/897683003/diff/40001/base/base.gyp#newcode1378
base/base.gyp:1378: 'target_name': 'base_library_process_type',
On 2015/02/12 00:42:47, nyquist wrote:
> base_java_library_process_type. Same for GN.

Done.

https://codereview.chromium.org/897683003/diff/40001/content/shell/android/li...
File
content/shell/android/linker_test_apk/src/org/chromium/chromium_linker_test_apk/ChromiumLinkerTestActivity.java
(right):

https://codereview.chromium.org/897683003/diff/40001/content/shell/android/li...
content/shell/android/linker_test_apk/src/org/chromium/chromium_linker_test_apk/ChromiumLinkerTestActivity.java:112:
new BrowserStartupController.StartupCallback() {
On 2015/02/10 19:32:43, Ted C wrote:
> this block only needs to be indented 8, so it looks indented 4 too many

Done.

[nyquist, 2015-02-12 17:49:53]

https://codereview.chromium.org/897683003/diff/100001/base/BUILD.gn
File base/BUILD.gn (left):

https://codereview.chromium.org/897683003/diff/100001/base/BUILD.gn#oldcode1521
base/BUILD.gn:1521: java_cpp_template("base_native_libraries_gen") {
Are you sure this should be removed? It seems to still be used on line 1470.

[michaelbai, 2015-02-12 18:24:41]

PTAL

https://codereview.chromium.org/897683003/diff/100001/base/BUILD.gn
File base/BUILD.gn (left):

https://codereview.chromium.org/897683003/diff/100001/base/BUILD.gn#oldcode1521
base/BUILD.gn:1521: java_cpp_template("base_native_libraries_gen") {
On 2015/02/12 17:49:53, nyquist wrote:
> Are you sure this should be removed? It seems to still be used on line 1470.

Done.

[nyquist, 2015-02-12 19:34:11]

base lgtm with just one small comment

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

https://codereview.chromium.org/897683003/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:44:
private static LibraryLoader sInstance;
Every other member of this class seems to have a comment. Think you could add
one?

[michaelbai, 2015-02-12 19:59:31]

New patchsets have been uploaded after l-g-t-m from nyquist@chromium.org

[michaelbai, 2015-02-12 20:05:00]

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

https://codereview.chromium.org/897683003/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:44:
private static LibraryLoader sInstance;
On 2015/02/12 19:34:11, nyquist wrote:
> Every other member of this class seems to have a comment. Think you could add
> one?

Done.

[michaelbai, 2015-02-12 20:08:32]

The CQ bit was checked by michaelbai@chromium.org

[commit-bot: I haz the power, 2015-02-12 20:09:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/897683003/140001

[commit-bot: I haz the power, 2015-02-12 20:36:32]

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

[commit-bot: I haz the power, 2015-02-12 20:36:35]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[michaelbai, 2015-02-13 03:26:07]

The CQ bit was checked by michaelbai@chromium.org

[commit-bot: I haz the power, 2015-02-13 03:27:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/897683003/160001

[commit-bot: I haz the power, 2015-02-13 03:40:41]

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

[commit-bot: I haz the power, 2015-02-13 03:40:43]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[michaelbai, 2015-02-13 04:02:39]

The CQ bit was checked by michaelbai@chromium.org

[commit-bot: I haz the power, 2015-02-13 04:03:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/897683003/180001

[commit-bot: I haz the power, 2015-02-13 04:17:44]

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

[commit-bot: I haz the power, 2015-02-13 04:17:46]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[michaelbai, 2015-02-13 04:40:53]

The CQ bit was checked by michaelbai@chromium.org

[commit-bot: I haz the power, 2015-02-13 04:41:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/897683003/200001

[commit-bot: I haz the power, 2015-02-13 05:47:11]

Committed patchset #9 (id:200001)

[commit-bot: I haz the power, 2015-02-13 05:48:10]

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