[Cronet] Hook up library loader, system proxy and network change notifier to async api.

[Cronet] Hook up library loader, system proxy and network change notifier to Async API.

BUG=409926
TEST=CronetUrlRequestContextTest.*Init*
TEST=ContextInitTest.*

Committed: https://crrev.com/63643dbe9fd4db0dafaf5055dca59bef904e8431
Cr-Commit-Position: refs/heads/master@{#315474}

[mef, 2015-01-02 22:59:03]

mef@chromium.org changed reviewers:
+ mmenke@chromium.org, xunjieli@chromium.org

[mef, 2015-01-02 23:00:41]

Happy New Year, please take a look! 

I've refactored Helen's code for Proxy and NCN so it would work for Async API
without HttpUrlRequestFactory initialization.

[xunjieli, 2015-01-05 14:48:03]

thanks for doing this!

https://codereview.chromium.org/726013002/diff/60001/components/cronet/androi...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/726013002/diff/60001/components/cronet/androi...
components/cronet/android/cronet_url_request_context_adapter.cc:136:
proxy_config_service_.reset(net::ProxyService::CreateSystemProxyConfigService(
I wonder whether it makes a difference for each context to have its own
ProxyConfigService. Should we make it global across contexts. Any thought?

https://codereview.chromium.org/726013002/diff/60001/components/cronet/androi...
components/cronet/android/cronet_url_request_context_adapter.cc:137:
GetNetworkTaskRunner(), NULL));
I think we moved the task of InitializeOnNetworkThread to the end of
InitRequestContextOnMainThread in url_request_context_adapter.cc, as we need to
wait for proxy_config_service_ to be initialized before we init everything else
on network thread. Maybe we need to do sth similar here?

https://codereview.chromium.org/726013002/diff/60001/components/cronet/androi...
File
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java
(right):

https://codereview.chromium.org/726013002/diff/60001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:43:
nativeCronetInitOnMainThread(context);
I wonder whether we should do "nativeCronetInitOnMainThread"  in front of
"NetworkChangeNotifier.init(context);" and
"NetworkChangeNotifier.registerToReceiveNotificationsAlways();"

https://codereview.chromium.org/726013002/diff/60001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:54:
sInitialized.block(5000);
Why do we need to wait for initialization to complete? I thought we made the
initialization asynchronous, and all posted tasks will guaranteed to happen
after initialization anyway (through RunTaskAfterContextInitOnNetworkThread).

https://codereview.chromium.org/726013002/diff/60001/components/cronet/androi...
File
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/726013002/diff/60001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:36:
CronetLibraryLoader.loadAndInitialize(context, config);
Maybe rename "loadAndInitialize" to sth like "initIfNeeded"? since
CronetLibraryLoader only loads and initializes when this function is invoked the
first time.

[mef, 2015-01-05 16:52:49]

mef@chromium.org changed reviewers:
+ pauljensen@chromium.org

[mef, 2015-01-05 16:59:21]

Thanks, PTAL.

https://codereview.chromium.org/726013002/diff/60001/components/cronet/androi...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/726013002/diff/60001/components/cronet/androi...
components/cronet/android/cronet_url_request_context_adapter.cc:136:
proxy_config_service_.reset(net::ProxyService::CreateSystemProxyConfigService(
On 2015/01/05 14:48:03, xunjieli wrote:
> I wonder whether it makes a difference for each context to have its own
> ProxyConfigService. Should we make it global across contexts. Any thought?

CreateSystemProxyConfigService takes io_task_runner, so making global
ProxyConfigService would require global io (aka network) thread. This seems like
a reasonable idea as we don't have any embedder callbacks performed on it, so it
shouldn't block. Matt, WDYT?

https://codereview.chromium.org/726013002/diff/60001/components/cronet/androi...
components/cronet/android/cronet_url_request_context_adapter.cc:137:
GetNetworkTaskRunner(), NULL));
On 2015/01/05 14:48:03, xunjieli wrote:
> I think we moved the task of InitializeOnNetworkThread to the end of
> InitRequestContextOnMainThread in url_request_context_adapter.cc, as we need
to
> wait for proxy_config_service_ to be initialized before we init everything
else
> on network thread. Maybe we need to do sth similar here?

Definitely, thanks for spotting that. Done.

https://codereview.chromium.org/726013002/diff/60001/components/cronet/androi...
File
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java
(right):

https://codereview.chromium.org/726013002/diff/60001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:43:
nativeCronetInitOnMainThread(context);
On 2015/01/05 14:48:03, xunjieli wrote:
> I wonder whether we should do "nativeCronetInitOnMainThread"  in front of
> "NetworkChangeNotifier.init(context);" and
> "NetworkChangeNotifier.registerToReceiveNotificationsAlways();"
Good question. I've copied it from ChromiumUrlRequestContext, which seems to
initialize NCN first, and 
adapted second, but I'm not sure about correct sequence. Adding Paul to comment.

https://codereview.chromium.org/726013002/diff/60001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:54:
sInitialized.block(5000);
On 2015/01/05 14:48:03, xunjieli wrote:
> Why do we need to wait for initialization to complete? I thought we made the
> initialization asynchronous, and all posted tasks will guaranteed to happen
> after initialization anyway (through RunTaskAfterContextInitOnNetworkThread).

The goal is to prevent race in case of concurrent initialization. 
I've reworked it to remove the wait. Done?

https://codereview.chromium.org/726013002/diff/60001/components/cronet/androi...
File
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/726013002/diff/60001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:36:
CronetLibraryLoader.loadAndInitialize(context, config);
On 2015/01/05 14:48:03, xunjieli wrote:
> Maybe rename "loadAndInitialize" to sth like "initIfNeeded"? since
> CronetLibraryLoader only loads and initializes when this function is invoked
the
> first time.

Done. ensureInitialized seems to be commonly used in Chrome.

[mmenke, 2015-01-05 18:20:39]

Quick response.  I'll do a full pass later today or tomorrow.

https://codereview.chromium.org/726013002/diff/60001/components/cronet/androi...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/726013002/diff/60001/components/cronet/androi...
components/cronet/android/cronet_url_request_context_adapter.cc:136:
proxy_config_service_.reset(net::ProxyService::CreateSystemProxyConfigService(
On 2015/01/05 16:59:21, mef wrote:
> On 2015/01/05 14:48:03, xunjieli wrote:
> > I wonder whether it makes a difference for each context to have its own
> > ProxyConfigService. Should we make it global across contexts. Any thought?
> 
> CreateSystemProxyConfigService takes io_task_runner, so making global
> ProxyConfigService would require global io (aka network) thread. This seems
like
> a reasonable idea as we don't have any embedder callbacks performed on it, so
it
> shouldn't block. Matt, WDYT?

I'm not really a big fan of globals, unless they're strictly necessary, or
result in major gains.  Not convinced we need them here.

They also result in bugs (See the bug I filed against Cronet cache startup not
being thread safe...because of a global...)

[xunjieli, 2015-01-05 18:22:45]

https://codereview.chromium.org/726013002/diff/80001/components/cronet/androi...
File components/cronet/android/cronet_url_request_context.cc (right):

https://codereview.chromium.org/726013002/diff/80001/components/cronet/androi...
components/cronet/android/cronet_url_request_context.cc:49:
base::android::ScopedJavaLocalRef<jobject> scoped_context(env, japp_context);
base::android::InitApplicationContext is already done in
"CronetInitOnMainThread".

https://codereview.chromium.org/726013002/diff/80001/components/cronet/androi...
components/cronet/android/cronet_url_request_context.cc:114: if
(!context_config->LoadFromJSON(config_string))
Hmm.. we probably should avoid doing things on the main UI thread. Converting
config string and loading from JSON should probably still remain on the network
thread. Since it is already done in CreateRequestContextAdapter, could you get
it from a member variable instead?

[mef, 2015-01-05 18:22:57]

On 2015/01/05 18:20:39, mmenke wrote:
> Quick response.  I'll do a full pass later today or tomorrow.
> 
>
https://codereview.chromium.org/726013002/diff/60001/components/cronet/androi...
> File components/cronet/android/cronet_url_request_context_adapter.cc (right):
> 
>
https://codereview.chromium.org/726013002/diff/60001/components/cronet/androi...
> components/cronet/android/cronet_url_request_context_adapter.cc:136:
> proxy_config_service_.reset(net::ProxyService::CreateSystemProxyConfigService(
> On 2015/01/05 16:59:21, mef wrote:
> > On 2015/01/05 14:48:03, xunjieli wrote:
> > > I wonder whether it makes a difference for each context to have its own
> > > ProxyConfigService. Should we make it global across contexts. Any thought?
> > 
> > CreateSystemProxyConfigService takes io_task_runner, so making global
> > ProxyConfigService would require global io (aka network) thread. This seems
> like
> > a reasonable idea as we don't have any embedder callbacks performed on it,
so
> it
> > shouldn't block. Matt, WDYT?
> 
> I'm not really a big fan of globals, unless they're strictly necessary, or
> result in major gains.  Not convinced we need them here.
> 
> They also result in bugs (See the bug I filed against Cronet cache startup not
> being thread safe...because of a global...)

Thanks, I'll leave it as is then.

[mef, 2015-01-05 19:32:41]

https://codereview.chromium.org/726013002/diff/80001/components/cronet/androi...
File components/cronet/android/cronet_url_request_context.cc (right):

https://codereview.chromium.org/726013002/diff/80001/components/cronet/androi...
components/cronet/android/cronet_url_request_context.cc:49:
base::android::ScopedJavaLocalRef<jobject> scoped_context(env, japp_context);
On 2015/01/05 18:22:45, xunjieli wrote:
> base::android::InitApplicationContext is already done in
> "CronetInitOnMainThread".

Done.

https://codereview.chromium.org/726013002/diff/80001/components/cronet/androi...
components/cronet/android/cronet_url_request_context.cc:114: if
(!context_config->LoadFromJSON(config_string))
On 2015/01/05 18:22:45, xunjieli wrote:
> Hmm.. we probably should avoid doing things on the main UI thread. Converting
> config string and loading from JSON should probably still remain on the
network
> thread. Since it is already done in CreateRequestContextAdapter, could you get
> it from a member variable instead?

The config used to be a member of CronetURLRequestContextAdapter, but  IIRC it
was removed by Matt's suggestion. 

I'm fine with bringing it back in (it would also be useful for default load
flags) if there are no objections. Matt, WDYT?

[pauljensen, 2015-01-06 17:38:40]

https://codereview.chromium.org/726013002/diff/60001/components/cronet/androi...
File
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java
(right):

https://codereview.chromium.org/726013002/diff/60001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:43:
nativeCronetInitOnMainThread(context);
On 2015/01/05 16:59:21, mef wrote:
> On 2015/01/05 14:48:03, xunjieli wrote:
> > I wonder whether we should do "nativeCronetInitOnMainThread"  in front of
> > "NetworkChangeNotifier.init(context);" and
> > "NetworkChangeNotifier.registerToReceiveNotificationsAlways();"
> Good question. I've copied it from ChromiumUrlRequestContext, which seems to
> initialize NCN first, and 
> adapted second, but I'm not sure about correct sequence. Adding Paul to
comment.

I don't think the order matters.  The only advantage I can see from swapping
them is that the NetworkChangeNotifier.init() call is redundant with the
nativeCronetInitOnMainThread() call, so if they were swapped, the init() call
could be removed.
I'd weakly recommend doing whatever Chrome for Android does but I'm not sure
what that is as I can't make much sense of their initialization code as it's
quite complicated.

[mmenke, 2015-01-08 19:06:17]

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
File components/cronet/android/cronet_loader.cc (right):

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
components/cronet/android/cronet_loader.cc:87: base::i18n::InitializeICU();
Don't think we need to do this twice, and we're already doing this in OnLoad.

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
components/cronet/android/cronet_loader.cc:93:
base::MessageLoopForUI::current()->Start();
What code depends on the main Java thread having a MessageLoop?  Think we always
jump to it Java-side.

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context.cc (right):

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
components/cronet/android/cronet_url_request_context.cc:46: return 0;
We parse the config here...And then we just throw it away.

I suppose we probably want to fail here if things go badly, so the embedder gets
a reasonable error, so we can't just get rid of parsing here.  Maybe just have
the context_adapter temporarily take ownership of the context_config, and delete
it once we've set everything up?

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:112:
network_thread_ = new base::Thread("network");
Should be in an initializer list.

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.h (right):

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.h:62: 
nit:  Remove blank line.

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.h:73: bool
is_context_initialized_ = false;
Don't inline initialization like this is a C++0x11 feature that has been
approved yet.  should put this in the initializer list.

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.h:73: bool
is_context_initialized_ = false;
Member variables should go after methods (Hrm...Did I really sign off on it,
as-is?  THat's not good).

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java
(right):

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:42:
if (Looper.getMainLooper() == Looper.myLooper()) {
If we're on a random executor thread, what does Looper.myLooper do?

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:63:
sLoadAndInitComplete = true;
IS there a reason we have to set load and init complete to true here, and not in
ensureInitialized?  Seems weird that we can end up calling System.loadLibrary
arbitrarily many times at once, and run initOnMainThread each time, though it
will only do stuff once.

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:51:
mUrlRequestContextAdapter, configString);
This isn't threadsafe.  shutdown can be called while we're calling into
nativeInitRequestContextOnMainThread.

[mef, 2015-01-08 21:17:03]

Thanks, PTAL!

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
File components/cronet/android/cronet_loader.cc (right):

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
components/cronet/android/cronet_loader.cc:87: base::i18n::InitializeICU();
On 2015/01/08 19:06:16, mmenke wrote:
> Don't think we need to do this twice, and we're already doing this in OnLoad.

Done.

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
components/cronet/android/cronet_loader.cc:93:
base::MessageLoopForUI::current()->Start();
On 2015/01/08 19:06:16, mmenke wrote:
> What code depends on the main Java thread having a MessageLoop?  Think we
always
> jump to it Java-side.

Helen will correct me if I'm wrong, but my understanding is that it is needed
for auto proxy service, which gets/handles system notifications on main thread.

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context.cc (right):

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
components/cronet/android/cronet_url_request_context.cc:46: return 0;
On 2015/01/08 19:06:16, mmenke wrote:
> We parse the config here...And then we just throw it away.
> 
> I suppose we probably want to fail here if things go badly, so the embedder
gets
> a reasonable error, so we can't just get rid of parsing here.  Maybe just have
> the context_adapter temporarily take ownership of the context_config, and
delete
> it once we've set everything up?

Done.

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:112:
network_thread_ = new base::Thread("network");
On 2015/01/08 19:06:16, mmenke wrote:
> Should be in an initializer list.

Done.

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.h (right):

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.h:62: 
On 2015/01/08 19:06:16, mmenke wrote:
> nit:  Remove blank line.

Done.

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.h:73: bool
is_context_initialized_ = false;
On 2015/01/08 19:06:16, mmenke wrote:
> Don't inline initialization like this is a C++0x11 feature that has been
> approved yet.  should put this in the initializer list.

Done.

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.h:73: bool
is_context_initialized_ = false;
On 2015/01/08 19:06:16, mmenke wrote:
> Member variables should go after methods (Hrm...Did I really sign off on it,
> as-is?  THat's not good).

Done.

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java
(right):

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:42:
if (Looper.getMainLooper() == Looper.myLooper()) {
On 2015/01/08 19:06:16, mmenke wrote:
> If we're on a random executor thread, what does Looper.myLooper do?

Returns null if the calling thread is not associated with a Looper.

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:63:
sLoadAndInitComplete = true;
On 2015/01/08 19:06:16, mmenke wrote:
> IS there a reason we have to set load and init complete to true here, and not
in
> ensureInitialized?  Seems weird that we can end up calling System.loadLibrary
> arbitrarily many times at once, and run initOnMainThread each time, though it
> will only do stuff once.

In order to set sLoadAndInitComplete in ensureInitialized we have to wait for
initialization to complete on main thread, which seems even uglier.

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:51:
mUrlRequestContextAdapter, configString);
On 2015/01/08 19:06:16, mmenke wrote:
> This isn't threadsafe.  shutdown can be called while we're calling into
> nativeInitRequestContextOnMainThread.

Good point. This seems to be a problem with shutdown rather than init though.
Done.

[xunjieli, 2015-01-08 21:34:24]

Going to take a closer look tomorrow.

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
File components/cronet/android/cronet_loader.cc (right):

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
components/cronet/android/cronet_loader.cc:93:
base::MessageLoopForUI::current()->Start();
On 2015/01/08 21:17:02, mef wrote:
> On 2015/01/08 19:06:16, mmenke wrote:
> > What code depends on the main Java thread having a MessageLoop?  Think we
> always
> > jump to it Java-side.
> 
> Helen will correct me if I'm wrong, but my understanding is that it is needed
> for auto proxy service, which gets/handles system notifications on main
thread.

Yes, we need to set up a message loop for the main UI thread. It is needed for
both ProxyConfigServiceAndroid and NetworkChangeNotifierAndroid behind the
scene. 

Taken from network_change_notifier_android: "// - The calls to the Java side
(stateful) object must be performed from a
//   single thread. This object happens to be a singleton which is used on the
//   application side on the main thread. Therefore all the method calls from
//   the native NetworkChangeNotifierAndroid class to its Java counterpart are
//   performed on the main thread."

[xunjieli, 2015-01-12 15:12:35]

Our testing app runs initialization code for both the old API and the new async
API. This just seems a bit error-prone to me. Not sure if we can be certain that
either set will work correctly if used alone. I wonder if we can make two
CronetTestActivity, something like CronetOldAPIActivity and
CronetAsyncAPIActivity, and launch one accordingly in test. WDYT?

https://codereview.chromium.org/726013002/diff/120001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/726013002/diff/120001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:43:
final String configString = config.toString();
nit: could you make config.toString() inline?

[mmenke, 2015-01-12 15:28:26]

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java
(right):

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:63:
sLoadAndInitComplete = true;
On 2015/01/08 21:17:02, mef wrote:
> On 2015/01/08 19:06:16, mmenke wrote:
> > IS there a reason we have to set load and init complete to true here, and
not
> in
> > ensureInitialized?  Seems weird that we can end up calling
System.loadLibrary
> > arbitrarily many times at once, and run initOnMainThread each time, though
it
> > will only do stuff once.
> 
> In order to set sLoadAndInitComplete in ensureInitialized we have to wait for
> initialization to complete on main thread, which seems even uglier.

I don't follow.  We're not blocking anywhere until sLoadAndInitComplete is set
to true, we're just using it to avoid double-initialize.  For correctness, we're
relying on the main loop executing things in order instead, and context
initialization also jumping over to the main thread.

[mef, 2015-01-12 17:19:56]

Thanks, PTAL. 

I think having 2 separate activities for old and new API could work, although
current code that skips initialization  also provides test for api-specific
initialization.

We should also create a new sample app for new api, and that will be another
test for initialization...

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java
(right):

https://codereview.chromium.org/726013002/diff/100001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:63:
sLoadAndInitComplete = true;
On 2015/01/12 15:28:26, mmenke wrote:
> On 2015/01/08 21:17:02, mef wrote:
> > On 2015/01/08 19:06:16, mmenke wrote:
> > > IS there a reason we have to set load and init complete to true here, and
> not
> > in
> > > ensureInitialized?  Seems weird that we can end up calling
> System.loadLibrary
> > > arbitrarily many times at once, and run initOnMainThread each time, though
> it
> > > will only do stuff once.
> > 
> > In order to set sLoadAndInitComplete in ensureInitialized we have to wait
for
> > initialization to complete on main thread, which seems even uglier.
> 
> I don't follow.  We're not blocking anywhere until sLoadAndInitComplete is set
> to true, we're just using it to avoid double-initialize.  For correctness,
we're
> relying on the main loop executing things in order instead, and context
> initialization also jumping over to the main thread.

Done.

https://codereview.chromium.org/726013002/diff/120001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/726013002/diff/120001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:43:
final String configString = config.toString();
On 2015/01/12 15:12:34, xunjieli wrote:
> nit: could you make config.toString() inline?

Done.

[mmenke, 2015-01-14 16:18:22]

https://codereview.chromium.org/726013002/diff/180001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/726013002/diff/180001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:145:
}
I don't think this is enough to ensure thread safety - consider:

1)  shutdown is called right after construction.
2)  shutdown calls nativeDestroyRequestContextAdapter(mUrlRequestContextAdapter)
on a child thread.
3)  On main looper,
nativeInitRequestContextOnMainThread(mUrlRequestContextAdapter) is called.

Then we're in trouble.

Think you need to grab the lock in your task's run method as well, and check if
mUrlRequestContextAdapter is 0 there.  I'd also like a test where shutdown
happens first (Can't really run them at the same time, but can block the main
thread until after shutdown is called)

[mef, 2015-01-22 20:38:45]

Sorry for loooong delay with addressing the comment, PTAL.

https://codereview.chromium.org/726013002/diff/180001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/726013002/diff/180001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:145:
}
On 2015/01/14 16:18:22, mmenke wrote:
> I don't think this is enough to ensure thread safety - consider:
> 
> 1)  shutdown is called right after construction.
> 2)  shutdown calls
nativeDestroyRequestContextAdapter(mUrlRequestContextAdapter)
> on a child thread.
> 3)  On main looper,
> nativeInitRequestContextOnMainThread(mUrlRequestContextAdapter) is called.
> 
> Then we're in trouble.
> 
> Think you need to grab the lock in your task's run method as well, and check
if
> mUrlRequestContextAdapter is 0 there.  I'd also like a test where shutdown
> happens first (Can't really run them at the same time, but can block the main
> thread until after shutdown is called)

Done.

[mmenke, 2015-01-23 20:41:42]

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
File components/cronet/android/cronet_loader.cc (right):

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/cronet_loader.cc:31: {"ChromiumUrlRequest",
cronet::ChromiumUrlRequestRegisterJni},
We're in the cronet namespace.  All of these cronet::'s aren't needed, including
the new one.

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/cronet_loader.cc:87: base::i18n::InitializeICU();
I don't think we need to initialize this both here and in onload.

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/cronet_loader.cc:90: if
(!base::MessageLoop::current()) {
Is there any case where this will have been initialized?

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:115:
default_load_flags_(0) {
LOAD_NORMAL

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:116:
context_config_.reset(context_config.release());
Should be in initializer list, and should use Pass instead of release.

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:130:
GetNetworkTaskRunner(), NULL));
nullptr is preferred now, I believe.

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.h (right):

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.h:31: typedef
base::Callback<void(void)> RunAfterContextInitTask;
Should just use base::Closure (Defined in callback.h)

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.h:52: const
RunAfterContextInitTask& callback);
With this change, GetNetworkTaskRunner() seems no longer safe to expose.  Let's
get rid of it, and make a method IsOnNetworkThread in its place.

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java
(right):

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:17:
public class CronetLibraryLoader {
Do we need to expose this externally?

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:22:
private static boolean sLoadComplete = false;
sLoadComplete doesn't seem like a great name.   sInitTaskPosted, maybe?

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:37:
private final ConditionVariable mInitCompleted = new ConditionVariable(false);
A couple lines in here that could be reformatted to 80 characters.

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:49:
}
Suggest a blank line before a comment that applies to a block of code.

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:53:
// API to handle the case where we are already on main thread.
Should be clearer here about what cases are bad.  Also, suggest either a blank
comment before the TODO, or indenting other lines of the TODO for legibility.

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:74:
"Context is shutdown.");
Think this can fit on a single line.

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:93:
synchronized (mLock) {
Hrm...Still really not a fan of this.  I'll see if I can come up with something
better next week.

[mef, 2015-01-28 21:32:28]

Thanks, PTAL.

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
File components/cronet/android/cronet_loader.cc (right):

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/cronet_loader.cc:31: {"ChromiumUrlRequest",
cronet::ChromiumUrlRequestRegisterJni},
On 2015/01/23 20:41:41, mmenke wrote:
> We're in the cronet namespace.  All of these cronet::'s aren't needed,
including
> the new one.

Done.

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/cronet_loader.cc:87: base::i18n::InitializeICU();
On 2015/01/23 20:41:41, mmenke wrote:
> I don't think we need to initialize this both here and in onload.

Done.

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/cronet_loader.cc:90: if
(!base::MessageLoop::current()) {
On 2015/01/23 20:41:41, mmenke wrote:
> Is there any case where this will have been initialized?

I can't think of any reason to call this method twice. Done.

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:115:
default_load_flags_(0) {
On 2015/01/23 20:41:41, mmenke wrote:
> LOAD_NORMAL

Done.

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:116:
context_config_.reset(context_config.release());
On 2015/01/23 20:41:41, mmenke wrote:
> Should be in initializer list, and should use Pass instead of release.

Done.

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:130:
GetNetworkTaskRunner(), NULL));
On 2015/01/23 20:41:41, mmenke wrote:
> nullptr is preferred now, I believe.

Done.

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.h (right):

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.h:31: typedef
base::Callback<void(void)> RunAfterContextInitTask;
On 2015/01/23 20:41:41, mmenke wrote:
> Should just use base::Closure (Defined in callback.h)

Done.

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.h:52: const
RunAfterContextInitTask& callback);
On 2015/01/23 20:41:41, mmenke wrote:
> With this change, GetNetworkTaskRunner() seems no longer safe to expose. 
Let's
> get rid of it, and make a method IsOnNetworkThread in its place.

Done.

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java
(right):

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:17:
public class CronetLibraryLoader {
On 2015/01/23 20:41:41, mmenke wrote:
> Do we need to expose this externally?

Good point. Done.

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:22:
private static boolean sLoadComplete = false;
On 2015/01/23 20:41:41, mmenke wrote:
> sLoadComplete doesn't seem like a great name.   sInitTaskPosted, maybe?

Done.

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:37:
private final ConditionVariable mInitCompleted = new ConditionVariable(false);
On 2015/01/23 20:41:41, mmenke wrote:
> A couple lines in here that could be reformatted to 80 characters.

Acknowledged.

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:49:
}
On 2015/01/23 20:41:41, mmenke wrote:
> Suggest a blank line before a comment that applies to a block of code.

Done.

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:53:
// API to handle the case where we are already on main thread.
On 2015/01/23 20:41:41, mmenke wrote:
> Should be clearer here about what cases are bad.  Also, suggest either a blank
> comment before the TODO, or indenting other lines of the TODO for legibility.

Hrm, are they actually bad?

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:74:
"Context is shutdown.");
On 2015/01/23 20:41:41, mmenke wrote:
> Think this can fit on a single line.

Done.

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:93:
synchronized (mLock) {
On 2015/01/23 20:41:41, mmenke wrote:
> Hrm...Still really not a fan of this.  I'll see if I can come up with
something
> better next week.

SG, let's chat tomorrow.

[mef, 2015-01-28 22:30:57]

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
File components/cronet/android/cronet_loader.cc (right):

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/cronet_loader.cc:87: base::i18n::InitializeICU();
On 2015/01/23 20:41:41, mmenke wrote:
> I don't think we need to initialize this both here and in onload.

It turns out that initializing it here instead of OnLoad fixes the crash on
startup observed in regular Android builds due to InitializeICU expecting
initialized application context, so I've removed it from CronetOnLoad instead.

[xunjieli, 2015-01-29 16:38:59]

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.h (right):

https://codereview.chromium.org/726013002/diff/220001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.h:31: typedef
base::Callback<void(void)> RunAfterContextInitTask;
On 2015/01/28 21:32:28, mef wrote:
> On 2015/01/23 20:41:41, mmenke wrote:
> > Should just use base::Closure (Defined in callback.h)
> 
> Done.

I might be wrong, but I think we should still keep the name
"RunAfterContextInitTask" since the queuing logic in the cc file is still there.
I think what Matt meant might be to change it to "typedef base::Closure
RunAfterContextInitTask". If this makes sense, could you change the typedef in
url_request_context_adapter.h as well?

[mmenke, 2015-02-02 19:30:27]

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
File components/cronet/android/cronet_loader.cc (right):

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/cronet_loader.cc:88: DCHECK(!g_main_message_loop);
include base/logging.h

While we're here, should probably also be including <jni.h>

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context.cc (right):

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/cronet_url_request_context.cc:36: // Sets global
user-agent to be used for all subsequent requests.
This comment seems a bit outdated

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:142:
DCHECK(GetNetworkTaskRunner()->BelongsToCurrentThread());
DCHECK(!is_context_initialized_);?

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:239: if
(is_context_initialized_) {
Maybe DCHECK(tasks_waiting_for_context_.empty());?

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.h (right):

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.h:30: struct
URLRequestContextConfig;
On 2015/01/29 16:38:59, xunjieli wrote:
> On 2015/01/28 21:32:28, mef wrote:
> > On 2015/01/23 20:41:41, mmenke wrote:
> > > Should just use base::Closure (Defined in callback.h)
> > 
> > Done.
> 
> I might be wrong, but I think we should still keep the name
> "RunAfterContextInitTask" since the queuing logic in the cc file is still
there.
> I think what Matt meant might be to change it to "typedef base::Closure
> RunAfterContextInitTask". If this makes sense, could you change the typedef in
> url_request_context_adapter.h as well?

Actually, I prefer keeping it a base::Closure, as that makes it clearer the type
of the callback.  I'm fine with renaming the argument to
run_after_context_init_task, or task_to_run_after_context_init.

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.h:95:
std::queue<base::Closure> tasks_waiting_for_context_;
Need to include <queue>

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java
(right):

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
2015?

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:54:
// API to handle the case where we are already on main thread.
Random comment:  Because this is a non-blocking API, I think this is something
we should do sooner rather than later.

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:111:
mInitCompleted.block();
This doesn't work if shutdown is called on two threads at once.  Both could end
up blocking.  Then when both are released...Either both end up trying to delete
the adapter, one ends up trying to delete a null adapter.  Simplest solution is
just to add a check in the synchronized block below to early return if the
adapter is null.

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:118:
nativeDestroyRequestContextAdapter(urlRequestContextAdapter);
Can we just move this into the synchronized block, and get rid of the temporary?

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:172:
block.open();
Hrm...what thread are the tests running on?

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:180:
assertEquals("Context is shutdown.", e.getMessage());
shutdown -> shut down.  (shutdown is a noun, shut down is a verb phrase which
can be used as an adjective.  Like say "Fred is happiness" rather than "Fred is
happy"....  I believe)

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:192:
ResponseStep.ON_RESPONSE_STARTED);
Any reason we have a request in this one?

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:202:
mActivity.mUrlRequestContext.shutdown();
Think you just need 2 - if the second one doesn't throw an exception, you don't
want to give the third call another chance to do so, hiding the first error.

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:474:
public void testInitContextAndStartRequest() {
Does either this or the next one test anything that other tests don't, like
testConfigUserAgent?

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:514:
RequestThread thread2 = new RequestThread(activity, URL_404);
Should we have both block on a condition variable, to make it more likely they
really go off at once?  Unclear how much overhead starting a thread has.

[mef, 2015-02-03 01:28:57]

Thanks, PTAL.

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
File components/cronet/android/cronet_loader.cc (right):

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/cronet_loader.cc:88: DCHECK(!g_main_message_loop);
On 2015/02/02 19:30:26, mmenke wrote:
> include base/logging.h
> 
> While we're here, should probably also be including <jni.h>

Done.

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context.cc (right):

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/cronet_url_request_context.cc:36: // Sets global
user-agent to be used for all subsequent requests.
On 2015/02/02 19:30:26, mmenke wrote:
> This comment seems a bit outdated

Done.

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:142:
DCHECK(GetNetworkTaskRunner()->BelongsToCurrentThread());
On 2015/02/02 19:30:26, mmenke wrote:
> DCHECK(!is_context_initialized_);?

Done.

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:239: if
(is_context_initialized_) {
On 2015/02/02 19:30:26, mmenke wrote:
> Maybe DCHECK(tasks_waiting_for_context_.empty());?

Done.

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.h (right):

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.h:30: struct
URLRequestContextConfig;
On 2015/02/02 19:30:26, mmenke wrote:
> On 2015/01/29 16:38:59, xunjieli wrote:
> > On 2015/01/28 21:32:28, mef wrote:
> > > On 2015/01/23 20:41:41, mmenke wrote:
> > > > Should just use base::Closure (Defined in callback.h)
> > > 
> > > Done.
> > 
> > I might be wrong, but I think we should still keep the name
> > "RunAfterContextInitTask" since the queuing logic in the cc file is still
> there.
> > I think what Matt meant might be to change it to "typedef base::Closure
> > RunAfterContextInitTask". If this makes sense, could you change the typedef
in
> > url_request_context_adapter.h as well?
> 
> Actually, I prefer keeping it a base::Closure, as that makes it clearer the
type
> of the callback.  I'm fine with renaming the argument to
> run_after_context_init_task, or task_to_run_after_context_init.

Done.

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.h:95:
std::queue<base::Closure> tasks_waiting_for_context_;
On 2015/02/02 19:30:26, mmenke wrote:
> Need to include <queue>

Done.

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java
(right):

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
On 2015/02/02 19:30:26, mmenke wrote:
> 2015?

Done.

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:54:
// API to handle the case where we are already on main thread.
On 2015/02/02 19:30:26, mmenke wrote:
> Random comment:  Because this is a non-blocking API, I think this is something
> we should do sooner rather than later.

Done.

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:111:
mInitCompleted.block();
On 2015/02/02 19:30:26, mmenke wrote:
> This doesn't work if shutdown is called on two threads at once.  Both could
end
> up blocking.  Then when both are released...Either both end up trying to
delete
> the adapter, one ends up trying to delete a null adapter.  Simplest solution
is
> just to add a check in the synchronized block below to early return if the
> adapter is null.

Done.

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:118:
nativeDestroyRequestContextAdapter(urlRequestContextAdapter);
On 2015/02/02 19:30:26, mmenke wrote:
> Can we just move this into the synchronized block, and get rid of the
temporary?

Unfortunately nativeDestroyRequestContextAdapter under lock deadlocks in
testShutdownDuringInit but I cannot find a plausible explanation why. I'll try
to debug it.

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:172:
block.open();
On 2015/02/02 19:30:26, mmenke wrote:
> Hrm...what thread are the tests running on?

Umm, some other, worker, thread?

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:180:
assertEquals("Context is shutdown.", e.getMessage());
On 2015/02/02 19:30:26, mmenke wrote:
> shutdown -> shut down.  (shutdown is a noun, shut down is a verb phrase which
> can be used as an adjective.  Like say "Fred is happiness" rather than "Fred
is
> happy"....  I believe)

Yeah, this one was tricky for me... If shutdown is not a verb, then should we
change the method name to shutDown (looks unusual) or destroy?

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:192:
ResponseStep.ON_RESPONSE_STARTED);
On 2015/02/02 19:30:26, mmenke wrote:
> Any reason we have a request in this one?

Done.

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:202:
mActivity.mUrlRequestContext.shutdown();
On 2015/02/02 19:30:26, mmenke wrote:
> Think you just need 2 - if the second one doesn't throw an exception, you
don't
> want to give the third call another chance to do so, hiding the first error.

Done.

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:474:
public void testInitContextAndStartRequest() {
On 2015/02/02 19:30:26, mmenke wrote:
> Does either this or the next one test anything that other tests don't, like
> testConfigUserAgent?

These skip factory initialization on app startup to test library initialized by
creating request context being immediately followed by requests.

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:514:
RequestThread thread2 = new RequestThread(activity, URL_404);
On 2015/02/02 19:30:26, mmenke wrote:
> Should we have both block on a condition variable, to make it more likely they
> really go off at once?  Unclear how much overhead starting a thread has.

Done.

[mef, 2015-02-03 14:29:03]

PTAL, thanks.

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:118:
nativeDestroyRequestContextAdapter(urlRequestContextAdapter);
On 2015/02/03 01:28:57, mef wrote:
> On 2015/02/02 19:30:26, mmenke wrote:
> > Can we just move this into the synchronized block, and get rid of the
> temporary?
> 
> Unfortunately nativeDestroyRequestContextAdapter under lock deadlocks in
> testShutdownDuringInit but I cannot find a plausible explanation why. I'll try
> to debug it.

Done. The deadlock was due to synchronized (mLock) in initNetworkThread().
I've moved mInitCompleted.open() after initialization completes on network
thread,
so there is no deadlock.

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:180:
assertEquals("Context is shutdown.", e.getMessage());
On 2015/02/03 01:28:57, mef wrote:
> On 2015/02/02 19:30:26, mmenke wrote:
> > shutdown -> shut down.  (shutdown is a noun, shut down is a verb phrase
which
> > can be used as an adjective.  Like say "Fred is happiness" rather than "Fred
> is
> > happy"....  I believe)
> 
> Yeah, this one was tricky for me... If shutdown is not a verb, then should we
> change the method name to shutDown (looks unusual) or destroy?

Done.

[xunjieli, 2015-02-03 18:37:59]

https://codereview.chromium.org/726013002/diff/340001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/726013002/diff/340001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:507:
ConditionVariable runBlocker = new ConditionVariable(false);
Suggest add a comment on why a condition variable is used here.

https://codereview.chromium.org/726013002/diff/340001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/HistogramManagerTest.java
(left):

https://codereview.chromium.org/726013002/diff/340001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/HistogramManagerTest.java:28:
assertTrue(delta1.length != 0);
This change doesn't seem relevant to this CL.

https://codereview.chromium.org/726013002/diff/360001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java
(right):

https://codereview.chromium.org/726013002/diff/360001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:61:
nativeCronetInitOnMainThread(context);
Maybe move nativeCronetInitOnMainThread(context) in front of
NetworkChangeNotifier.registerToReceiveNotificationsAlways() ?
Then we can get rid of NetworkChangeNotifier.init(context).

https://codereview.chromium.org/726013002/diff/360001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/726013002/diff/360001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:37:
private final ConditionVariable mInitCompleted = new ConditionVariable(false);
nit: group final fields together?

https://codereview.chromium.org/726013002/diff/360001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:39:
private AtomicInteger mActiveRequestCount = new AtomicInteger(0);
can this be final?

https://codereview.chromium.org/726013002/diff/360001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:53:
public void run() {
nit: add @Override?

[mmenke, 2015-02-03 19:36:18]

Quick comment.  Was expecting to have time to do another full pass today, but
looks like I may not.

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:180:
assertEquals("Context is shutdown.", e.getMessage());
On 2015/02/03 01:28:57, mef wrote:
> On 2015/02/02 19:30:26, mmenke wrote:
> > shutdown -> shut down.  (shutdown is a noun, shut down is a verb phrase
which
> > can be used as an adjective.  Like say "Fred is happiness" rather than "Fred
> is
> > happy"....  I believe)
> 
> Yeah, this one was tricky for me... If shutdown is not a verb, then should we
> change the method name to shutDown (looks unusual) or destroy?

shutDown does look weird, I agree there (And the test name actually works fine).
 I'm fine with either shutdown or destroy for the method.

[mmenke, 2015-02-03 19:37:57]

On 2015/02/03 19:36:18, mmenke wrote:
> Quick comment.  Was expecting to have time to do another full pass today, but
> looks like I may not.
> 
>
https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
> File
>
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java
> (right):
> 
>
https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
>
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:180:
> assertEquals("Context is shutdown.", e.getMessage());
> On 2015/02/03 01:28:57, mef wrote:
> > On 2015/02/02 19:30:26, mmenke wrote:
> > > shutdown -> shut down.  (shutdown is a noun, shut down is a verb phrase
> which
> > > can be used as an adjective.  Like say "Fred is happiness" rather than
"Fred
> > is
> > > happy"....  I believe)
> > 
> > Yeah, this one was tricky for me... If shutdown is not a verb, then should
we
> > change the method name to shutDown (looks unusual) or destroy?
> 
> shutDown does look weird, I agree there (And the test name actually works
fine).
>  I'm fine with either shutdown or destroy for the method.

Oops...I thought I had another comment queued...

The create on main thread stuff doesn't work, I believe:  You need to do a
similar change to the library loader

[mmenke, 2015-02-03 19:52:06]

On 2015/02/03 19:37:57, mmenke wrote:
> On 2015/02/03 19:36:18, mmenke wrote:
> > Quick comment.  Was expecting to have time to do another full pass today,
but
> > looks like I may not.
> > 
> >
>
https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
> > File
> >
>
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/726013002/diff/300001/components/cronet/andro...
> >
>
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:180:
> > assertEquals("Context is shutdown.", e.getMessage());
> > On 2015/02/03 01:28:57, mef wrote:
> > > On 2015/02/02 19:30:26, mmenke wrote:
> > > > shutdown -> shut down.  (shutdown is a noun, shut down is a verb phrase
> > which
> > > > can be used as an adjective.  Like say "Fred is happiness" rather than
> "Fred
> > > is
> > > > happy"....  I believe)
> > > 
> > > Yeah, this one was tricky for me... If shutdown is not a verb, then should
> we
> > > change the method name to shutDown (looks unusual) or destroy?
> > 
> > shutDown does look weird, I agree there (And the test name actually works
> fine).
> >  I'm fine with either shutdown or destroy for the method.
> 
> Oops...I thought I had another comment queued...
> 
> The create on main thread stuff doesn't work, I believe:  You need to do a
> similar change to the library loader

For the record:  Misha pointed out to me he had already added the behavior I was
asking for, and I asked for a test that validates the whole thing works,
possibly using Helen's HttpURLConnection wrapper (Which would deadlock when used
for the first time on the main thread, without the fix)

[mef, 2015-02-03 20:08:55]

Thanks! I'll add couple more tests per discussion with Matt.

https://codereview.chromium.org/726013002/diff/340001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/726013002/diff/340001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:507:
ConditionVariable runBlocker = new ConditionVariable(false);
On 2015/02/03 18:37:59, xunjieli wrote:
> Suggest add a comment on why a condition variable is used here.

Done.

https://codereview.chromium.org/726013002/diff/340001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/HistogramManagerTest.java
(left):

https://codereview.chromium.org/726013002/diff/340001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/HistogramManagerTest.java:28:
assertTrue(delta1.length != 0);
On 2015/02/03 18:37:59, xunjieli wrote:
> This change doesn't seem relevant to this CL.

Oops, Done.

https://codereview.chromium.org/726013002/diff/360001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java
(right):

https://codereview.chromium.org/726013002/diff/360001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:61:
nativeCronetInitOnMainThread(context);
On 2015/02/03 18:37:59, xunjieli wrote:
> Maybe move nativeCronetInitOnMainThread(context) in front of
> NetworkChangeNotifier.registerToReceiveNotificationsAlways() ?
> Then we can get rid of NetworkChangeNotifier.init(context).

I'll be glad to do that as I was just copying whatever was done before.
Should it be 

nativeCronetInitOnMainThread(context);
NetworkChangeNotifier.registerToReceiveNotificationsAlways();

without 

NetworkChangeNotifier.init(context);

https://codereview.chromium.org/726013002/diff/360001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/726013002/diff/360001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:37:
private final ConditionVariable mInitCompleted = new ConditionVariable(false);
On 2015/02/03 18:37:59, xunjieli wrote:
> nit: group final fields together?

Done.

https://codereview.chromium.org/726013002/diff/360001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:39:
private AtomicInteger mActiveRequestCount = new AtomicInteger(0);
On 2015/02/03 18:37:59, xunjieli wrote:
> can this be final?

Done.

https://codereview.chromium.org/726013002/diff/360001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:53:
public void run() {
On 2015/02/03 18:37:59, xunjieli wrote:
> nit: add @Override?

Done.

[xunjieli, 2015-02-03 20:11:48]

https://codereview.chromium.org/726013002/diff/360001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java
(right):

https://codereview.chromium.org/726013002/diff/360001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:61:
nativeCronetInitOnMainThread(context);
On 2015/02/03 20:08:55, mef wrote:
> On 2015/02/03 18:37:59, xunjieli wrote:
> > Maybe move nativeCronetInitOnMainThread(context) in front of
> > NetworkChangeNotifier.registerToReceiveNotificationsAlways() ?
> > Then we can get rid of NetworkChangeNotifier.init(context).
> 
> I'll be glad to do that as I was just copying whatever was done before.
> Should it be 
> 
> nativeCronetInitOnMainThread(context);
> NetworkChangeNotifier.registerToReceiveNotificationsAlways();
> 
> without 
> 
> NetworkChangeNotifier.init(context);
> 

Yes, that should do it. Thanks!

[mef, 2015-02-03 22:09:26]

PTAL, I've added a test that creates and shuts down context on the main thread.

https://codereview.chromium.org/726013002/diff/360001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java
(right):

https://codereview.chromium.org/726013002/diff/360001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:61:
nativeCronetInitOnMainThread(context);
On 2015/02/03 20:11:48, xunjieli wrote:
> On 2015/02/03 20:08:55, mef wrote:
> > On 2015/02/03 18:37:59, xunjieli wrote:
> > > Maybe move nativeCronetInitOnMainThread(context) in front of
> > > NetworkChangeNotifier.registerToReceiveNotificationsAlways() ?
> > > Then we can get rid of NetworkChangeNotifier.init(context).
> > 
> > I'll be glad to do that as I was just copying whatever was done before.
> > Should it be 
> > 
> > nativeCronetInitOnMainThread(context);
> > NetworkChangeNotifier.registerToReceiveNotificationsAlways();
> > 
> > without 
> > 
> > NetworkChangeNotifier.init(context);
> > 
> 
> Yes, that should do it. Thanks!

Done.

[xunjieli, 2015-02-03 22:52:43]

lgtm. didn't see anything obvious, so I will sign off.

[pauljensen, 2015-02-04 14:33:20]

https://codereview.chromium.org/726013002/diff/390001/components/cronet/andro...
File components/cronet/android/cronet_loader.cc (right):

https://codereview.chromium.org/726013002/diff/390001/components/cronet/andro...
components/cronet/android/cronet_loader.cc:49: net::NetworkChangeNotifier*
g_network_change_notifier = nullptr;
g_network_change_notifier is redundant,
NetworkChangeNotifier::NetworkChangeNotifier() already does this DCHECK:
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/network_c...

https://codereview.chromium.org/726013002/diff/390001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java
(right):

https://codereview.chromium.org/726013002/diff/390001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:64:
private static native long nativeCronetInitOnMainThread(Context context);
why does this function have a return value if it's not used?

https://codereview.chromium.org/726013002/diff/390001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/726013002/diff/390001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:38:
private long mUrlRequestContextAdapter = 0;
nit: part of me feels like this variable would be better typed as a boolean so
we don't have so many comparisons to a magical "0".  Perhaps "boolean
mHaveUrlRequestContextAdapter" or "boolean
mUrlRequestContextAdapterInitialized".

[mmenke, 2015-02-04 16:18:25]

https://codereview.chromium.org/726013002/diff/390001/components/cronet/andro...
File components/cronet/android/cronet_loader.cc (right):

https://codereview.chromium.org/726013002/diff/390001/components/cronet/andro...
components/cronet/android/cronet_loader.cc:49: net::NetworkChangeNotifier*
g_network_change_notifier = nullptr;
On 2015/02/04 14:33:20, pauljensen wrote:
> g_network_change_notifier is redundant,
> NetworkChangeNotifier::NetworkChangeNotifier() already does this DCHECK:
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/network_c...

I prefer to keep g_network_change_notifier here, myself, to make it clearer
we're leaking the NCN.

Don't have strong feelings about it, though.

https://codereview.chromium.org/726013002/diff/390001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/726013002/diff/390001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:38:
private long mUrlRequestContextAdapter = 0;
On 2015/02/04 14:33:20, pauljensen wrote:
> nit: part of me feels like this variable would be better typed as a boolean so
> we don't have so many comparisons to a magical "0".  Perhaps "boolean
> mHaveUrlRequestContextAdapter" or "boolean
> mUrlRequestContextAdapterInitialized".

We do need the long to actually talk to the adapter, though, so if we went with
a bool, we'd need both the bool and this.

I'm not a big fan of redundant variables (The more things you have that have to
be kept consistent, the more chance you have of getting into an inconsistent
state).  Maybe a method?  private bool HaveRequestContextAdapter() { return
mUrlRequestContextAdapter != 0; }?

[pauljensen, 2015-02-04 16:22:11]

https://codereview.chromium.org/726013002/diff/390001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/726013002/diff/390001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:38:
private long mUrlRequestContextAdapter = 0;
On 2015/02/04 16:18:25, mmenke wrote:
> On 2015/02/04 14:33:20, pauljensen wrote:
> > nit: part of me feels like this variable would be better typed as a boolean
so
> > we don't have so many comparisons to a magical "0".  Perhaps "boolean
> > mHaveUrlRequestContextAdapter" or "boolean
> > mUrlRequestContextAdapterInitialized".
> 
> We do need the long to actually talk to the adapter, though, so if we went
with
> a bool, we'd need both the bool and this.
> 
> I'm not a big fan of redundant variables (The more things you have that have
to
> be kept consistent, the more chance you have of getting into an inconsistent
> state).  Maybe a method?  private bool HaveRequestContextAdapter() { return
> mUrlRequestContextAdapter != 0; }?

Oops, I didn't see the use of it to talk to the adaper, you can ignore my
comment, though I do like the use of a method to hide all the inlined constant
0's.

[mef, 2015-02-04 17:34:15]

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

[mef, 2015-02-04 17:35:01]

Thanks, PTAL.

https://codereview.chromium.org/726013002/diff/390001/components/cronet/andro...
File components/cronet/android/cronet_loader.cc (right):

https://codereview.chromium.org/726013002/diff/390001/components/cronet/andro...
components/cronet/android/cronet_loader.cc:49: net::NetworkChangeNotifier*
g_network_change_notifier = nullptr;
On 2015/02/04 16:18:25, mmenke wrote:
> On 2015/02/04 14:33:20, pauljensen wrote:
> > g_network_change_notifier is redundant,
> > NetworkChangeNotifier::NetworkChangeNotifier() already does this DCHECK:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/network_c...
> 
> I prefer to keep g_network_change_notifier here, myself, to make it clearer
> we're leaking the NCN.
> 
> Don't have strong feelings about it, though.

The comment to NCN::Create
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/network_c...
says that 'caller owns the returned pointer', so I guess formally we should hold
on to it.

https://codereview.chromium.org/726013002/diff/390001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java
(right):

https://codereview.chromium.org/726013002/diff/390001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:64:
private static native long nativeCronetInitOnMainThread(Context context);
On 2015/02/04 14:33:20, pauljensen wrote:
> why does this function have a return value if it's not used?

Done.

https://codereview.chromium.org/726013002/diff/390001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/726013002/diff/390001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:38:
private long mUrlRequestContextAdapter = 0;
On 2015/02/04 16:22:11, pauljensen wrote:
> On 2015/02/04 16:18:25, mmenke wrote:
> > On 2015/02/04 14:33:20, pauljensen wrote:
> > > nit: part of me feels like this variable would be better typed as a
boolean
> so
> > > we don't have so many comparisons to a magical "0".  Perhaps "boolean
> > > mHaveUrlRequestContextAdapter" or "boolean
> > > mUrlRequestContextAdapterInitialized".
> > 
> > We do need the long to actually talk to the adapter, though, so if we went
> with
> > a bool, we'd need both the bool and this.
> > 
> > I'm not a big fan of redundant variables (The more things you have that have
> to
> > be kept consistent, the more chance you have of getting into an inconsistent
> > state).  Maybe a method?  private bool HaveRequestContextAdapter() { return
> > mUrlRequestContextAdapter != 0; }?
> 
> Oops, I didn't see the use of it to talk to the adaper, you can ignore my
> comment, though I do like the use of a method to hide all the inlined constant
> 0's.

Done.

[mmenke, 2015-02-04 21:23:08]

LGTM

https://codereview.chromium.org/726013002/diff/410001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/726013002/diff/410001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:173:
// Create new request context, but its initialization on the UI thread
We're switching between calling it the "UI thread" and the "main thread" every
other comment.  Should be consistent.  Suggest calling it the main thread, to
match "getMainLooper".

https://codereview.chromium.org/726013002/diff/410001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:201:
// Shutdown will wait for init to complete on UI thread.
This comment is confusing - makes it sound like we're not running on the UI
thread.

https://codereview.chromium.org/726013002/diff/410001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:213:
// Ensure that test is not running on the main thread.
I don't follow this comment.

[mef, 2015-02-04 21:47:34]

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

[mef, 2015-02-04 21:51:28]

Thanks!

https://codereview.chromium.org/726013002/diff/410001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/726013002/diff/410001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:173:
// Create new request context, but its initialization on the UI thread
On 2015/02/04 21:23:08, mmenke wrote:
> We're switching between calling it the "UI thread" and the "main thread" every
> other comment.  Should be consistent.  Suggest calling it the main thread, to
> match "getMainLooper".

Done.

https://codereview.chromium.org/726013002/diff/410001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:201:
// Shutdown will wait for init to complete on UI thread.
On 2015/02/04 21:23:08, mmenke wrote:
> This comment is confusing - makes it sound like we're not running on the UI
> thread.

Done.

https://codereview.chromium.org/726013002/diff/410001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:213:
// Ensure that test is not running on the main thread.
On 2015/02/04 21:23:08, mmenke wrote:
> I don't follow this comment.

Done.

[mmenke, 2015-02-04 21:54:59]

https://codereview.chromium.org/726013002/diff/450001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/726013002/diff/450001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:213:
// Ensure that test is not running on the main thread.
On 2015/02/04 21:51:28, mef wrote:
> On 2015/02/04 21:23:08, mmenke wrote:
> > I don't follow this comment.
> 
> Done.

Doesn't look like you changed (Or removed) it.  :)

https://codereview.chromium.org/726013002/diff/450001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:215:
// Wait for shutdown to complete on main thread
nit:  +.

[mef, 2015-02-05 19:28:56]

Good catch, Matt, sorry about that. 

Also I came this morning to 9 failing tests, which turned out to be result of
initialization order of NCN, RequestContext and Histogram Manager.

https://codereview.chromium.org/726013002/diff/360001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java
(right):

https://codereview.chromium.org/726013002/diff/360001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:61:
nativeCronetInitOnMainThread(context);
On 2015/02/03 22:09:26, mef wrote:
> On 2015/02/03 20:11:48, xunjieli wrote:
> > On 2015/02/03 20:08:55, mef wrote:
> > > On 2015/02/03 18:37:59, xunjieli wrote:
> > > > Maybe move nativeCronetInitOnMainThread(context) in front of
> > > > NetworkChangeNotifier.registerToReceiveNotificationsAlways() ?
> > > > Then we can get rid of NetworkChangeNotifier.init(context).
> > > 
> > > I'll be glad to do that as I was just copying whatever was done before.
> > > Should it be 
> > > 
> > > nativeCronetInitOnMainThread(context);
> > > NetworkChangeNotifier.registerToReceiveNotificationsAlways();
> > > 
> > > without 
> > > 
> > > NetworkChangeNotifier.init(context);
> > > 
> > 
> > Yes, that should do it. Thanks!
> 
> Done.

This change is causing first request to fail with error
net::ERR_NETWORK_CHANGED, 
so I've reverted sequence back to NCN::init ->
NCN::registerToReceiveNotifications -> CronetInit.

https://codereview.chromium.org/726013002/diff/450001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/726013002/diff/450001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:213:
// Ensure that test is not running on the main thread.
On 2015/02/04 21:54:58, mmenke wrote:
> On 2015/02/04 21:51:28, mef wrote:
> > On 2015/02/04 21:23:08, mmenke wrote:
> > > I don't follow this comment.
> > 
> > Done.
> 
> Doesn't look like you changed (Or removed) it.  :)

Oops, forgot to save the file before commit. Done.

https://codereview.chromium.org/726013002/diff/450001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/CronetUrlRequestContextTest.java:215:
// Wait for shutdown to complete on main thread
On 2015/02/04 21:54:58, mmenke wrote:
> nit:  +.

Done.

https://codereview.chromium.org/726013002/diff/470001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/HistogramManagerTest.java
(right):

https://codereview.chromium.org/726013002/diff/470001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/HistogramManagerTest.java:28:
assertEquals(0, delta1.length);
HistogramManager is now created after UrlRequestContext, and therefore doesn't
capture the data that was captured before, so the first delta is emtpy.

[xunjieli, 2015-02-05 19:47:49]

https://codereview.chromium.org/726013002/diff/360001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java
(right):

https://codereview.chromium.org/726013002/diff/360001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:61:
nativeCronetInitOnMainThread(context);
On 2015/02/05 19:28:56, mef wrote:
> On 2015/02/03 22:09:26, mef wrote:
> > On 2015/02/03 20:11:48, xunjieli wrote:
> > > On 2015/02/03 20:08:55, mef wrote:
> > > > On 2015/02/03 18:37:59, xunjieli wrote:
> > > > > Maybe move nativeCronetInitOnMainThread(context) in front of
> > > > > NetworkChangeNotifier.registerToReceiveNotificationsAlways() ?
> > > > > Then we can get rid of NetworkChangeNotifier.init(context).
> > > > 
> > > > I'll be glad to do that as I was just copying whatever was done before.
> > > > Should it be 
> > > > 
> > > > nativeCronetInitOnMainThread(context);
> > > > NetworkChangeNotifier.registerToReceiveNotificationsAlways();
> > > > 
> > > > without 
> > > > 
> > > > NetworkChangeNotifier.init(context);
> > > > 
> > > 
> > > Yes, that should do it. Thanks!
> > 
> > Done.
> 
> This change is causing first request to fail with error
> net::ERR_NETWORK_CHANGED, 
> so I've reverted sequence back to NCN::init ->
> NCN::registerToReceiveNotifications -> CronetInit.

I don't understand why this happens. We post initOnMainThread task to the main
thread, we then post initialize request context to the main thread. A url
request will be delayed after request context is fully initialized. I don't see
why changing the order here will make the first request to fail with
net::ERR_NETWORK_CHANGED. Paul, any guess?

https://codereview.chromium.org/726013002/diff/470001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java
(right):

https://codereview.chromium.org/726013002/diff/470001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:17:
class CronetLibraryLoader {
One last request. How about changing CronetLibraryLoader to CronetLoader to
match the cc file?

[mef, 2015-02-05 20:31:06]

https://codereview.chromium.org/726013002/diff/360001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java
(right):

https://codereview.chromium.org/726013002/diff/360001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:61:
nativeCronetInitOnMainThread(context);
On 2015/02/05 19:47:49, xunjieli wrote:
> On 2015/02/05 19:28:56, mef wrote:
> > On 2015/02/03 22:09:26, mef wrote:
> > > On 2015/02/03 20:11:48, xunjieli wrote:
> > > > On 2015/02/03 20:08:55, mef wrote:
> > > > > On 2015/02/03 18:37:59, xunjieli wrote:
> > > > > > Maybe move nativeCronetInitOnMainThread(context) in front of
> > > > > > NetworkChangeNotifier.registerToReceiveNotificationsAlways() ?
> > > > > > Then we can get rid of NetworkChangeNotifier.init(context).
> > > > > 
> > > > > I'll be glad to do that as I was just copying whatever was done
before.
> > > > > Should it be 
> > > > > 
> > > > > nativeCronetInitOnMainThread(context);
> > > > > NetworkChangeNotifier.registerToReceiveNotificationsAlways();
> > > > > 
> > > > > without 
> > > > > 
> > > > > NetworkChangeNotifier.init(context);
> > > > > 
> > > > 
> > > > Yes, that should do it. Thanks!
> > > 
> > > Done.
> > 
> > This change is causing first request to fail with error
> > net::ERR_NETWORK_CHANGED, 
> > so I've reverted sequence back to NCN::init ->
> > NCN::registerToReceiveNotifications -> CronetInit.
> 
> I don't understand why this happens. We post initOnMainThread task to the main
> thread, we then post initialize request context to the main thread. A url
> request will be delayed after request context is fully initialized. I don't
see
> why changing the order here will make the first request to fail with
> net::ERR_NETWORK_CHANGED. Paul, any guess?

Why was it originally done in this order here:
https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/...

https://codereview.chromium.org/726013002/diff/470001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java
(right):

https://codereview.chromium.org/726013002/diff/470001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:17:
class CronetLibraryLoader {
On 2015/02/05 19:47:49, xunjieli wrote:
> One last request. How about changing CronetLibraryLoader to CronetLoader to
> match the cc file?

Done. I've renamed the cc file as I think CronetLoader is a bit too generic.

[xunjieli, 2015-02-05 20:51:22]

On 2015/02/05 20:31:06, mef wrote:
>
https://codereview.chromium.org/726013002/diff/360001/components/cronet/andro...
> File
> components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java
> (right):
> 
>
https://codereview.chromium.org/726013002/diff/360001/components/cronet/andro...
>
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:61:
> nativeCronetInitOnMainThread(context);
> On 2015/02/05 19:47:49, xunjieli wrote:
> > On 2015/02/05 19:28:56, mef wrote:
> > > On 2015/02/03 22:09:26, mef wrote:
> > > > On 2015/02/03 20:11:48, xunjieli wrote:
> > > > > On 2015/02/03 20:08:55, mef wrote:
> > > > > > On 2015/02/03 18:37:59, xunjieli wrote:
> > > > > > > Maybe move nativeCronetInitOnMainThread(context) in front of
> > > > > > > NetworkChangeNotifier.registerToReceiveNotificationsAlways() ?
> > > > > > > Then we can get rid of NetworkChangeNotifier.init(context).
> > > > > > 
> > > > > > I'll be glad to do that as I was just copying whatever was done
> before.
> > > > > > Should it be 
> > > > > > 
> > > > > > nativeCronetInitOnMainThread(context);
> > > > > > NetworkChangeNotifier.registerToReceiveNotificationsAlways();
> > > > > > 
> > > > > > without 
> > > > > > 
> > > > > > NetworkChangeNotifier.init(context);
> > > > > > 
> > > > > 
> > > > > Yes, that should do it. Thanks!
> > > > 
> > > > Done.
> > > 
> > > This change is causing first request to fail with error
> > > net::ERR_NETWORK_CHANGED, 
> > > so I've reverted sequence back to NCN::init ->
> > > NCN::registerToReceiveNotifications -> CronetInit.
> > 
> > I don't understand why this happens. We post initOnMainThread task to the
main
> > thread, we then post initialize request context to the main thread. A url
> > request will be delayed after request context is fully initialized. I don't
> see
> > why changing the order here will make the first request to fail with
> > net::ERR_NETWORK_CHANGED. Paul, any guess?
> 
> Why was it originally done in this order here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/...
> 
>
https://codereview.chromium.org/726013002/diff/470001/components/cronet/andro...
> File
> components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java
> (right):
> 

I have no idea. Probably because it worked :P and I didn't dig further. I am
fine with land this CL as it is. We can improve later.

[mef, 2015-02-05 20:54:29]

On 2015/02/05 20:51:22, xunjieli wrote:
> On 2015/02/05 20:31:06, mef wrote:
> >
>
https://codereview.chromium.org/726013002/diff/360001/components/cronet/andro...
> > File
> > components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/726013002/diff/360001/components/cronet/andro...
> >
>
components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java:61:
> > nativeCronetInitOnMainThread(context);
> > On 2015/02/05 19:47:49, xunjieli wrote:
> > > On 2015/02/05 19:28:56, mef wrote:
> > > > On 2015/02/03 22:09:26, mef wrote:
> > > > > On 2015/02/03 20:11:48, xunjieli wrote:
> > > > > > On 2015/02/03 20:08:55, mef wrote:
> > > > > > > On 2015/02/03 18:37:59, xunjieli wrote:
> > > > > > > > Maybe move nativeCronetInitOnMainThread(context) in front of
> > > > > > > > NetworkChangeNotifier.registerToReceiveNotificationsAlways() ?
> > > > > > > > Then we can get rid of NetworkChangeNotifier.init(context).
> > > > > > > 
> > > > > > > I'll be glad to do that as I was just copying whatever was done
> > before.
> > > > > > > Should it be 
> > > > > > > 
> > > > > > > nativeCronetInitOnMainThread(context);
> > > > > > > NetworkChangeNotifier.registerToReceiveNotificationsAlways();
> > > > > > > 
> > > > > > > without 
> > > > > > > 
> > > > > > > NetworkChangeNotifier.init(context);
> > > > > > > 
> > > > > > 
> > > > > > Yes, that should do it. Thanks!
> > > > > 
> > > > > Done.
> > > > 
> > > > This change is causing first request to fail with error
> > > > net::ERR_NETWORK_CHANGED, 
> > > > so I've reverted sequence back to NCN::init ->
> > > > NCN::registerToReceiveNotifications -> CronetInit.
> > > 
> > > I don't understand why this happens. We post initOnMainThread task to the
> main
> > > thread, we then post initialize request context to the main thread. A url
> > > request will be delayed after request context is fully initialized. I
don't
> > see
> > > why changing the order here will make the first request to fail with
> > > net::ERR_NETWORK_CHANGED. Paul, any guess?
> > 
> > Why was it originally done in this order here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/...
> > 
> >
>
https://codereview.chromium.org/726013002/diff/470001/components/cronet/andro...
> > File
> > components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java
> > (right):
> > 
> 
> I have no idea. Probably because it worked :P and I didn't dig further. I am
> fine with land this CL as it is. We can improve later.

IIRC there was a concern to have NCN completely initialized and functional prior
to first request, or was it about proxy?

I don't really know NCN, so my theories are purely speculative at best.

[pauljensen, 2015-02-06 16:46:35]

I'll look at the NCN issue Monday, when I'm done with net bug triage.

[pauljensen, 2015-02-09 17:04:33]

In terms of NCN.java, it looks like registerToReceiveNotificationsAlways() calls
setAutoDetectConnectivityStateInternal() which calls
updateCurrentConnectionType() which sends out notifications.  The Java
updateCurrentConnectionType() call doesn't actually send any notifications if
there are no native NCNAndroid's registered however.  In Cronet it's
nativeCronetInitOnMainThread() that creates and registers the NCNAndroid.
In terms of this CL, I think its current state of calling
registerToReceiveNotificationsAlways() and then calling
nativeCronetInitOnMainThread() which registers the NCNAndroid should be fine and
should avoid the inadvertent and undesired initial network changed observer
notification (causing the ERR_NETWORK_CHANGED).

[mef, 2015-02-09 17:15:10]

On 2015/02/09 17:04:33, pauljensen wrote:
> In terms of NCN.java, it looks like registerToReceiveNotificationsAlways()
calls
> setAutoDetectConnectivityStateInternal() which calls
> updateCurrentConnectionType() which sends out notifications.  The Java
> updateCurrentConnectionType() call doesn't actually send any notifications if
> there are no native NCNAndroid's registered however.  In Cronet it's
> nativeCronetInitOnMainThread() that creates and registers the NCNAndroid.
> In terms of this CL, I think its current state of calling
> registerToReceiveNotificationsAlways() and then calling
> nativeCronetInitOnMainThread() which registers the NCNAndroid should be fine
and
> should avoid the inadvertent and undesired initial network changed observer
> notification (causing the ERR_NETWORK_CHANGED).

Great, thanks a lot for looking into this!

[xunjieli, 2015-02-09 17:15:22]

On 2015/02/09 17:04:33, pauljensen wrote:
> In terms of NCN.java, it looks like registerToReceiveNotificationsAlways()
calls
> setAutoDetectConnectivityStateInternal() which calls
> updateCurrentConnectionType() which sends out notifications.  The Java
> updateCurrentConnectionType() call doesn't actually send any notifications if
> there are no native NCNAndroid's registered however.  In Cronet it's
> nativeCronetInitOnMainThread() that creates and registers the NCNAndroid.
> In terms of this CL, I think its current state of calling
> registerToReceiveNotificationsAlways() and then calling
> nativeCronetInitOnMainThread() which registers the NCNAndroid should be fine
and
> should avoid the inadvertent and undesired initial network changed observer
> notification (causing the ERR_NETWORK_CHANGED).

Wow, thanks for the update! That's a very interesting behavior. Misha, maybe add
a comment above "NetworkChangeNotifier.init(context);" alone the line that
registerToReceiveNotificationsAlways() is called before the native
NetworkChangeNotifierAndroid is created, so as to avoid receiving the undesired
initial network change observer notification, which will cause an
ERR_NETWORK_CHANGED ? So we won't change the order in future CLs. Thanks

[mef, 2015-02-10 02:13:21]

The CQ bit was checked by mef@chromium.org

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

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

[commit-bot: I haz the power, 2015-02-10 02:18:53]

Committed patchset #29 (id:550001)

[commit-bot: I haz the power, 2015-02-10 02:20:07]

Patchset 29 (id:??) landed as
https://crrev.com/63643dbe9fd4db0dafaf5055dca59bef904e8431
Cr-Commit-Position: refs/heads/master@{#315474}