[Cronet] Make CronetHttpURLStreamHandler.openConnection public

Make CronetHttpURLStreamHandler.openConnection public

This CL makes CronetHttpURLStreamHandler.openConnection() public, so it is
easier to construct CronetHttpURLConnection.

BUG=398997
Committed: https://crrev.com/3b2a6c62340bde336fbd6f423a40b1cb99e35e3e
Cr-Commit-Position: refs/heads/master@{#313942}

[xunjieli, 2015-01-26 21:43:15]

xunjieli@chromium.org changed reviewers:
+ mef@chromium.org, mmenk@chromium.org, pauljensen@chromium.org

[xunjieli, 2015-01-26 21:43:15]

Per email discussion, we should make CronetHttpURLConnection's constructor
public as well as openConnection methods in the stream handler. PTAL Thanks!

[xunjieli, 2015-01-26 21:43:52]

xunjieli@chromium.org changed reviewers:
+ mmenke@chromium.org
- mmenk@chromium.org

[pauljensen, 2015-01-27 15:46:34]

1. Should we add a test for this? so we know this method of construction remains
supported?
2. Now that I think about it, maybe we should only make one method public
instead of both, for simplicity of use and to decrease the API we have to
support long term.  My vote is for openConnection() as I think it's probably the
API more familiar to Java developers.
3. The CL description only describes half the CL; I think we should adjust it to
match the CL.

[pauljensen, 2015-01-27 15:53:03]

Thinking about it more, it'd be nice if UrlRequestContext had the two public
openConnection() APIs instead, so developers don't have to follow the extra step
of constructing a URLStreamHandler at all.  URLStreamHandler isn't an API
familiar to Java developers I assume, it's really just for overloading behavior
which is a rarity rather than the norm.

[mef, 2015-01-27 16:23:33]

On 2015/01/27 15:53:03, pauljensen wrote:
> Thinking about it more, it'd be nice if UrlRequestContext had the two public
> openConnection() APIs instead, so developers don't have to follow the extra
step
> of constructing a URLStreamHandler at all.  URLStreamHandler isn't an API
> familiar to Java developers I assume, it's really just for overloading
behavior
> which is a rarity rather than the norm.

I like that UrlRequestContext doesn't know anything about HttpURLConnection, so
I'd vote against adding another openConnection() api to it.
How do people normally create HttpURLConnection?

[pauljensen, 2015-01-27 16:24:26]

On 2015/01/27 16:23:33, mef wrote:
> On 2015/01/27 15:53:03, pauljensen wrote:
> > Thinking about it more, it'd be nice if UrlRequestContext had the two public
> > openConnection() APIs instead, so developers don't have to follow the extra
> step
> > of constructing a URLStreamHandler at all.  URLStreamHandler isn't an API
> > familiar to Java developers I assume, it's really just for overloading
> behavior
> > which is a rarity rather than the norm.
> 
> I like that UrlRequestContext doesn't know anything about HttpURLConnection,
so
> I'd vote against adding another openConnection() api to it.
> How do people normally create HttpURLConnection?

URL.openConnection()

[mef, 2015-01-27 16:31:12]

On 2015/01/27 16:24:26, pauljensen wrote:
> On 2015/01/27 16:23:33, mef wrote:
> > On 2015/01/27 15:53:03, pauljensen wrote:
> > > Thinking about it more, it'd be nice if UrlRequestContext had the two
public
> > > openConnection() APIs instead, so developers don't have to follow the
extra
> > step
> > > of constructing a URLStreamHandler at all.  URLStreamHandler isn't an API
> > > familiar to Java developers I assume, it's really just for overloading
> > behavior
> > > which is a rarity rather than the norm.
> > 
> > I like that UrlRequestContext doesn't know anything about HttpURLConnection,
> so
> > I'd vote against adding another openConnection() api to it.
> > How do people normally create HttpURLConnection?
> 
> URL.openConnection()

Ok, but that one is already covered by CronetHttpURLStreamHandler, isn't it?
Would it make sense to add a static method like
CronetHttpUrlConnection.installAsDefaultStreamHandler() or somesuch?

[pauljensen, 2015-01-27 16:44:44]

On 2015/01/27 16:31:12, mef wrote:
> On 2015/01/27 16:24:26, pauljensen wrote:
> > On 2015/01/27 16:23:33, mef wrote:
> > > On 2015/01/27 15:53:03, pauljensen wrote:
> > > > Thinking about it more, it'd be nice if UrlRequestContext had the two
> public
> > > > openConnection() APIs instead, so developers don't have to follow the
> extra
> > > step
> > > > of constructing a URLStreamHandler at all.  URLStreamHandler isn't an
API
> > > > familiar to Java developers I assume, it's really just for overloading
> > > behavior
> > > > which is a rarity rather than the norm.
> > > 
> > > I like that UrlRequestContext doesn't know anything about
HttpURLConnection,
> > so
> > > I'd vote against adding another openConnection() api to it.
> > > How do people normally create HttpURLConnection?
> > 
> > URL.openConnection()
> 
> Ok, but that one is already covered by CronetHttpURLStreamHandler, isn't it?
> Would it make sense to add a static method like
> CronetHttpUrlConnection.installAsDefaultStreamHandler() or somesuch?

To quote my email that prompted this CL:
I think we need to expose CronetHttpURLConnection's constructor or 
CronetHttpURLStreamHandler.openConnection otherwise it's very hard to use; it
can only be used for all requests in the JVM via URL.setURLStreamHandlerFactory.

Without this it's like we're selling a shirt that requires you replace your
whole wardrobe with only instances of this shirt.

[xunjieli, 2015-01-27 16:53:25]

On 2015/01/27 16:44:44, pauljensen wrote:
> On 2015/01/27 16:31:12, mef wrote:
> > On 2015/01/27 16:24:26, pauljensen wrote:
> > > On 2015/01/27 16:23:33, mef wrote:
> > > > On 2015/01/27 15:53:03, pauljensen wrote:
> > > > > Thinking about it more, it'd be nice if UrlRequestContext had the two
> > public
> > > > > openConnection() APIs instead, so developers don't have to follow the
> > extra
> > > > step
> > > > > of constructing a URLStreamHandler at all.  URLStreamHandler isn't an
> API
> > > > > familiar to Java developers I assume, it's really just for overloading
> > > > behavior
> > > > > which is a rarity rather than the norm.
> > > > 
> > > > I like that UrlRequestContext doesn't know anything about
> HttpURLConnection,
> > > so
> > > > I'd vote against adding another openConnection() api to it.
> > > > How do people normally create HttpURLConnection?
> > > 
> > > URL.openConnection()
> > 
> > Ok, but that one is already covered by CronetHttpURLStreamHandler, isn't it?
> > Would it make sense to add a static method like
> > CronetHttpUrlConnection.installAsDefaultStreamHandler() or somesuch?
> 
> To quote my email that prompted this CL:
> I think we need to expose CronetHttpURLConnection's constructor or 
> CronetHttpURLStreamHandler.openConnection otherwise it's very hard to use; it
> can only be used for all requests in the JVM via
URL.setURLStreamHandlerFactory.
> 
> Without this it's like we're selling a shirt that requires you replace your
> whole wardrobe with only instances of this shirt.

Points taken. I agree with Misha that we probably dont want to put any info abt
HttpURLConnection in the UrlRequestContext. Otherwise, I have added tests. PTAL.

[pauljensen, 2015-01-27 18:59:41]

https://codereview.chromium.org/876123002/diff/20001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLStreamHandlerTest.java
(right):

https://codereview.chromium.org/876123002/diff/20001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLStreamHandlerTest.java:12:
import org.chromium.cronet_test_apk.UploadTestServer;
Why does this have "Upload" in the name?  it looks like it's being used for
download testing.

https://codereview.chromium.org/876123002/diff/20001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLStreamHandlerTest.java:44:
CronetHttpURLConnection connection =
Can we make this HttpURLConnection instead of CronetHttpURLConnection to ensure
we're testing the general interface and not something else.

https://codereview.chromium.org/876123002/diff/20001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLStreamHandlerTest.java:58:
streamHandler.openConnection(null, null);
It's a little unclear what this is testing, either that a null URL should give
an UnsupportedOperationException because it represents an unsupported protocol
(i.e. not HTTP or HTTPS) or that our openConnection that takes a proxy argument
always returns UnsupportedOperationException.  If it's the latter, can we pass
in a supported URL?

[xunjieli, 2015-01-27 21:17:13]

Thanks for the review!

https://codereview.chromium.org/876123002/diff/20001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLStreamHandlerTest.java
(right):

https://codereview.chromium.org/876123002/diff/20001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLStreamHandlerTest.java:12:
import org.chromium.cronet_test_apk.UploadTestServer;
On 2015/01/27 18:59:41, pauljensen wrote:
> Why does this have "Upload" in the name?  it looks like it's being used for
> download testing.

It was originally intended for testing upload. Now we use it as a general test
server.  Filed crbug.com/452643 just now to rename it accordingly.

https://codereview.chromium.org/876123002/diff/20001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLStreamHandlerTest.java:44:
CronetHttpURLConnection connection =
On 2015/01/27 18:59:41, pauljensen wrote:
> Can we make this HttpURLConnection instead of CronetHttpURLConnection to
ensure
> we're testing the general interface and not something else.

Done.

https://codereview.chromium.org/876123002/diff/20001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLStreamHandlerTest.java:58:
streamHandler.openConnection(null, null);
On 2015/01/27 18:59:41, pauljensen wrote:
> It's a little unclear what this is testing, either that a null URL should give
> an UnsupportedOperationException because it represents an unsupported protocol
> (i.e. not HTTP or HTTPS) or that our openConnection that takes a proxy
argument
> always returns UnsupportedOperationException.  If it's the latter, can we pass
> in a supported URL?

Done. To eliminate the ambiguity, i've also added a proxy object.

[pauljensen, 2015-01-28 13:46:42]

lgtm other than the one testing comment

https://codereview.chromium.org/876123002/diff/40001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLStreamHandlerTest.java
(right):

https://codereview.chromium.org/876123002/diff/40001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLStreamHandlerTest.java:50:
connection.disconnect();
I think we should extend this to test all paths through
CronetHttpURLStreamHandler.openConnection(URL), so that our unit test provides
full testing coverage, by adding a test for an https URL (you could just call
openConnection() to see that it doesn't throw an exception, which doesn't
actually perform any network activity AFAIK) and a bogus protocol (e.g.
"xyz://foo.com").

[xunjieli, 2015-01-28 16:16:53]

https://codereview.chromium.org/876123002/diff/40001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLStreamHandlerTest.java
(right):

https://codereview.chromium.org/876123002/diff/40001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLStreamHandlerTest.java:50:
connection.disconnect();
On 2015/01/28 13:46:42, pauljensen wrote:
> I think we should extend this to test all paths through
> CronetHttpURLStreamHandler.openConnection(URL), so that our unit test provides
> full testing coverage, by adding a test for an https URL (you could just call
> openConnection() to see that it doesn't throw an exception, which doesn't
> actually perform any network activity AFAIK) and a bogus protocol (e.g.
> "xyz://foo.com").

Partially done. Added the HTTPS url. Unknown schemes/protocols are caught in URL
constructor. Using "xyz://foo.com", URL constructor fails with a
"java.net.MalformedURLException: Unknown protocol: xyz". This is the case tested
in CronetHttpURLConnection#testBadScheme
(https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/...).

[pauljensen, 2015-01-28 16:24:59]

https://codereview.chromium.org/876123002/diff/40001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLStreamHandlerTest.java
(right):

https://codereview.chromium.org/876123002/diff/40001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLStreamHandlerTest.java:50:
connection.disconnect();
On 2015/01/28 16:16:53, xunjieli wrote:
> On 2015/01/28 13:46:42, pauljensen wrote:
> > I think we should extend this to test all paths through
> > CronetHttpURLStreamHandler.openConnection(URL), so that our unit test
provides
> > full testing coverage, by adding a test for an https URL (you could just
call
> > openConnection() to see that it doesn't throw an exception, which doesn't
> > actually perform any network activity AFAIK) and a bogus protocol (e.g.
> > "xyz://foo.com").
> 
> Partially done. Added the HTTPS url. Unknown schemes/protocols are caught in
URL
> constructor. Using "xyz://foo.com", URL constructor fails with a
> "java.net.MalformedURLException: Unknown protocol: xyz". This is the case
tested
> in CronetHttpURLConnection#testBadScheme
>
(https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/...).

How about using ftp://? That should get through URL constructor to our code.

[xunjieli, 2015-01-28 16:38:57]

On 2015/01/28 16:24:59, pauljensen wrote:
>
https://codereview.chromium.org/876123002/diff/40001/components/cronet/androi...
> File
>
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLStreamHandlerTest.java
> (right):
> 
>
https://codereview.chromium.org/876123002/diff/40001/components/cronet/androi...
>
components/cronet/android/test/javatests/src/org/chromium/cronet_test_apk/urlconnection/CronetHttpURLStreamHandlerTest.java:50:
> connection.disconnect();
> On 2015/01/28 16:16:53, xunjieli wrote:
> > On 2015/01/28 13:46:42, pauljensen wrote:
> > > I think we should extend this to test all paths through
> > > CronetHttpURLStreamHandler.openConnection(URL), so that our unit test
> provides
> > > full testing coverage, by adding a test for an https URL (you could just
> call
> > > openConnection() to see that it doesn't throw an exception, which doesn't
> > > actually perform any network activity AFAIK) and a bogus protocol (e.g.
> > > "xyz://foo.com").
> > 
> > Partially done. Added the HTTPS url. Unknown schemes/protocols are caught in
> URL
> > constructor. Using "xyz://foo.com", URL constructor fails with a
> > "java.net.MalformedURLException: Unknown protocol: xyz". This is the case
> tested
> > in CronetHttpURLConnection#testBadScheme
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/...).
> 
> How about using ftp://? That should get through URL constructor to our code.

Good idea! Done. thanks!

[pauljensen, 2015-01-28 16:54:38]

lgtm

[mef, 2015-01-30 15:48:46]

lgtm

[mmenke, 2015-01-30 17:19:59]

On 2015/01/30 15:48:46, mef wrote:
> lgtm

LGTM, deferring to Paul and Misha.

[xunjieli, 2015-01-30 17:23:02]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2015-01-30 17:24:05]

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

[commit-bot: I haz the power, 2015-01-30 18:07:52]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-30 18:08:49]

Patchset 5 (id:??) landed as
https://crrev.com/3b2a6c62340bde336fbd6f423a40b1cb99e35e3e
Cr-Commit-Position: refs/heads/master@{#313942}