[Cronet] Clear mock cert verifier reference in CronetEngine.Builder

[Cronet] Clear mock cert verifier reference in CronetEngine.Builder

Ownership of the mock cert verifier is passed to CronetEngine after
CronetEngine.Builder.build() is called. We should clear the reference.

This CL additionally fixes test setup so that
CronetEngine.Builder.build() is only called once for the
life time of a CronetTestFramework.

R=mef@chromium.org

BUG=557425
Committed: https://crrev.com/f06e9c86dc630aefee9027cf764b9e898c580372
Cr-Commit-Position: refs/heads/master@{#361510}

[xunjieli, 2015-11-23 22:00:25]

Description was changed from

==========
[Cronet] Clear mock cert verifier reference in CronetEngine.Builder

Ownership of the mock cert verifier is passed to CronetEngine after
CronetEngine.Builder.build() is called. We should clear the reference.

R=mef@chromium.org

BUG=557425
==========

to

==========
[Cronet] Clear mock cert verifier reference in CronetEngine.Builder

Ownership of the mock cert verifier is passed to CronetEngine after
CronetEngine.Builder.build() is called. We should clear the reference.

R=mef@chromium.org

BUG=557425
==========

[xunjieli, 2015-11-23 22:00:25]

xunjieli@chromium.org changed reviewers:
- mef@chromium.org

[xunjieli, 2015-11-23 23:08:26]

Patchset #1 (id:1) has been deleted

[xunjieli, 2015-11-23 23:09:16]

Description was changed from

==========
[Cronet] Clear mock cert verifier reference in CronetEngine.Builder

Ownership of the mock cert verifier is passed to CronetEngine after
CronetEngine.Builder.build() is called. We should clear the reference.

R=mef@chromium.org

BUG=557425
==========

to

==========
[Cronet] Clear mock cert verifier reference in CronetEngine.Builder

Ownership of the mock cert verifier is passed to CronetEngine after
CronetEngine.Builder.build() is called. We should clear the reference.

This CL additionally fixes test so that 
CronetEngine.Builder.build() is only called once for the
life time of a CronetTestFramework.

R=mef@chromium.org

BUG=557425
==========

[xunjieli, 2015-11-23 23:09:26]

Description was changed from

==========
[Cronet] Clear mock cert verifier reference in CronetEngine.Builder

Ownership of the mock cert verifier is passed to CronetEngine after
CronetEngine.Builder.build() is called. We should clear the reference.

This CL additionally fixes test so that 
CronetEngine.Builder.build() is only called once for the
life time of a CronetTestFramework.

R=mef@chromium.org

BUG=557425
==========

to

==========
[Cronet] Clear mock cert verifier reference in CronetEngine.Builder

Ownership of the mock cert verifier is passed to CronetEngine after
CronetEngine.Builder.build() is called. We should clear the reference.

This CL additionally fixes test setup so that 
CronetEngine.Builder.build() is only called once for the
life time of a CronetTestFramework.

R=mef@chromium.org

BUG=557425
==========

[xunjieli, 2015-11-23 23:14:05]

Patchset #1 (id:20001) has been deleted

[xunjieli, 2015-11-23 23:15:25]

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

[xunjieli, 2015-11-23 23:15:25]

PTAL

[mef, 2015-11-24 20:38:33]

https://codereview.chromium.org/1472693003/diff/40001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlTest.java
(right):

https://codereview.chromium.org/1472693003/diff/40001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlTest.java:125:
CronetTestFramework testFramework =
Can this construct be extracted into a method like
startCronetTestFrameworkForLegacyApiWithUrl(URL)?

https://codereview.chromium.org/1472693003/diff/40001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/UploadTest.java
(right):

https://codereview.chromium.org/1472693003/diff/40001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/UploadTest.java:33:
String[] commandLineArgs = new String[] {
not used?

https://codereview.chromium.org/1472693003/diff/40001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/UploadTest.java:35:
mTestFramework = new CronetTestFramework(null, mTestFramework, getContext(),
null);
wrong copy-paste?

https://codereview.chromium.org/1472693003/diff/40001/components/cronet/andro...
File
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java
(right):

https://codereview.chromium.org/1472693003/diff/40001/components/cronet/andro...
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java:105:
String appUrl, String[] commandLine, Context context, CronetEngine.Builder
builder) {
I think the whole idea of commandLine is now outdated and kludgy. 

It was used to pass configuration from instrumentation tests apk to the test
app, but now I think it should be replaced by a builder with explicit setters.

https://codereview.chromium.org/1472693003/diff/40001/components/cronet/andro...
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java:137:
case LibraryInitType.HTTP_URL_CONNECTION:
can this be combined with default?

[xunjieli, 2015-11-24 21:28:06]

Filed a bug to remove commandLineArgs.
Thanks for the review. PTAL

build/android/test_runner.py instrumentation 
--test-apk=CronetTestInstrumentation 
W    1.444s Main  Creating 1 test runner.
W    1.445s 3a6b  Creating shard 0 for device 0a023a6b.
W    2.613s 3a6b  Unable to enable java asserts for 0a023a6b, non rooted device
W    5.753s Main  Running tests with 1 test runner.
C  735.519s Main 
********************************************************************************
C  735.519s Main  Summary
C  735.519s Main 
********************************************************************************
C  735.519s Main  [==========] 223 tests ran.
C  735.519s Main  [  PASSED  ] 223 tests.
C  735.519s Main 
********************************************************************************

https://codereview.chromium.org/1472693003/diff/40001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlTest.java
(right):

https://codereview.chromium.org/1472693003/diff/40001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlTest.java:125:
CronetTestFramework testFramework =
On 2015/11/24 20:38:33, mef wrote:
> Can this construct be extracted into a method like
> startCronetTestFrameworkForLegacyApiWithUrl(URL)?

Done.

https://codereview.chromium.org/1472693003/diff/40001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/UploadTest.java
(right):

https://codereview.chromium.org/1472693003/diff/40001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/UploadTest.java:33:
String[] commandLineArgs = new String[] {
On 2015/11/24 20:38:33, mef wrote:
> not used?

Done.

https://codereview.chromium.org/1472693003/diff/40001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/UploadTest.java:35:
mTestFramework = new CronetTestFramework(null, mTestFramework, getContext(),
null);
On 2015/11/24 20:38:33, mef wrote:
> wrong copy-paste?

Done. Thanks! I wonder how the tests managed to pass..

https://codereview.chromium.org/1472693003/diff/40001/components/cronet/andro...
File
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java
(right):

https://codereview.chromium.org/1472693003/diff/40001/components/cronet/andro...
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java:105:
String appUrl, String[] commandLine, Context context, CronetEngine.Builder
builder) {
On 2015/11/24 20:38:33, mef wrote:
> I think the whole idea of commandLine is now outdated and kludgy. 
> 
> It was used to pass configuration from instrumentation tests apk to the test
> app, but now I think it should be replaced by a builder with explicit setters.

Yea. Paul considered doing that too, but it was not very straightforward. Let's
do that in a separate CL.

https://codereview.chromium.org/1472693003/diff/40001/components/cronet/andro...
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java:137:
case LibraryInitType.HTTP_URL_CONNECTION:
On 2015/11/24 20:38:33, mef wrote:
> can this be combined with default?

I am not sure how to combine it with the default. The mStreamHandlerFactory
happens after the mCronetEngine line. Java cannot switch on two cases. I can
change this block to if-else, but that might reduce readability.

[xunjieli, 2015-11-24 21:28:17]

Patchset #2 (id:60001) has been deleted

[mef, 2015-11-24 21:43:42]

https://codereview.chromium.org/1472693003/diff/40001/components/cronet/andro...
File
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java
(right):

https://codereview.chromium.org/1472693003/diff/40001/components/cronet/andro...
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java:105:
String appUrl, String[] commandLine, Context context, CronetEngine.Builder
builder) {
On 2015/11/24 21:28:06, xunjieli wrote:
> On 2015/11/24 20:38:33, mef wrote:
> > I think the whole idea of commandLine is now outdated and kludgy. 
> > 
> > It was used to pass configuration from instrumentation tests apk to the test
> > app, but now I think it should be replaced by a builder with explicit
setters.
> 
> Yea. Paul considered doing that too, but it was not very straightforward.
Let's
> do that in a separate CL.

Acknowledged.

https://codereview.chromium.org/1472693003/diff/40001/components/cronet/andro...
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java:137:
case LibraryInitType.HTTP_URL_CONNECTION:
On 2015/11/24 21:28:06, xunjieli wrote:
> On 2015/11/24 20:38:33, mef wrote:
> > can this be combined with default?
> 
> I am not sure how to combine it with the default. The mStreamHandlerFactory
> happens after the mCronetEngine line. Java cannot switch on two cases. I can
> change this block to if-else, but that might reduce readability.

Would it hurt anything if we always createURLStreamHandlerFactory? 
We don't have to set it as default handler, right?
Before we had distinct cases because CronetHttpURLStreamHandler had its own
UrlRequestContext, but now it just takes CronetEngine.

[xunjieli, 2015-11-24 21:54:13]

https://codereview.chromium.org/1472693003/diff/40001/components/cronet/andro...
File
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java
(right):

https://codereview.chromium.org/1472693003/diff/40001/components/cronet/andro...
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java:137:
case LibraryInitType.HTTP_URL_CONNECTION:
On 2015/11/24 21:43:42, mef wrote:
> On 2015/11/24 21:28:06, xunjieli wrote:
> > On 2015/11/24 20:38:33, mef wrote:
> > > can this be combined with default?
> > 
> > I am not sure how to combine it with the default. The mStreamHandlerFactory
> > happens after the mCronetEngine line. Java cannot switch on two cases. I can
> > change this block to if-else, but that might reduce readability.
> 
> Would it hurt anything if we always createURLStreamHandlerFactory? 
> We don't have to set it as default handler, right?
> Before we had distinct cases because CronetHttpURLStreamHandler had its own
> UrlRequestContext, but now it just takes CronetEngine.

Yes, you are right. But I'd like to think that the three LibraryInitTypes are
disjoint, and each has their own paths. This makes things easier to reason
about. When we only init Cronet, we do not care about mStreamHandlerFactory, so
it makes sense that it is null. It surely does not hurt if we always
createURLStreamHandlerFactory now, but I am a little apprehensive given how
often we ran into cases like this where initializing one API influences another
because of a new feature added. (In the future we might do something crazy in
createURLStreamHandlerFactory that might inadvertently influence the async API)

[mef, 2015-11-24 22:40:56]

On 2015/11/24 21:54:13, xunjieli wrote:
>
https://codereview.chromium.org/1472693003/diff/40001/components/cronet/andro...
> File
> components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java
> (right):
> 
>
https://codereview.chromium.org/1472693003/diff/40001/components/cronet/andro...
>
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java:137:
> case LibraryInitType.HTTP_URL_CONNECTION:
> On 2015/11/24 21:43:42, mef wrote:
> > On 2015/11/24 21:28:06, xunjieli wrote:
> > > On 2015/11/24 20:38:33, mef wrote:
> > > > can this be combined with default?
> > > 
> > > I am not sure how to combine it with the default. The
mStreamHandlerFactory
> > > happens after the mCronetEngine line. Java cannot switch on two cases. I
can
> > > change this block to if-else, but that might reduce readability.
> > 
> > Would it hurt anything if we always createURLStreamHandlerFactory? 
> > We don't have to set it as default handler, right?
> > Before we had distinct cases because CronetHttpURLStreamHandler had its own
> > UrlRequestContext, but now it just takes CronetEngine.
> 
> Yes, you are right. But I'd like to think that the three LibraryInitTypes are
> disjoint, and each has their own paths. This makes things easier to reason
> about. When we only init Cronet, we do not care about mStreamHandlerFactory,
so
> it makes sense that it is null. It surely does not hurt if we always
> createURLStreamHandlerFactory now, but I am a little apprehensive given how
> often we ran into cases like this where initializing one API influences
another
> because of a new feature added. (In the future we might do something crazy in
> createURLStreamHandlerFactory that might inadvertently influence the async
API)

sgtm, lgtm

[xunjieli, 2015-11-24 22:41:45]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2015-11-24 22:44:14]

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

[commit-bot: I haz the power, 2015-11-25 01:11:19]

Committed patchset #2 (id:80001)

[commit-bot: I haz the power, 2015-11-25 01:13:16]

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