Merge UrlRequest.java into ChromiumUrlRequest.java

Merge UrlRequest.java into ChromiumUrlRequest.java

BUG=390267
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289894

[mef, 2014-08-08 22:17:39]

Hi Matt,

I would appreciate if you could glance at this CL. 
It is WIP, but I would appreciate any comments to confirm whether I'm moving in
the right direction.

One of the open questions is whether ChromiumUrlRequestContext should somehow
merge with ChromiumUrlRequestFactory (or be exposed from it), to make netlog and
statistics publicly available.

thanks,
-m

[mmenke, 2014-08-12 14:36:29]

On 2014/08/08 22:17:39, mef wrote:
> Hi Matt,
> 
> I would appreciate if you could glance at this CL. 
> It is WIP, but I would appreciate any comments to confirm whether I'm moving
in
> the right direction.
> 
> One of the open questions is whether ChromiumUrlRequestContext should somehow
> merge with ChromiumUrlRequestFactory (or be exposed from it), to make netlog
and
> statistics publicly available.
> 
> thanks,
> -m

Oops, though I'd commented on this one already, but I'd only started writing
comments.  I have to prepare for a top user issues thing late this afternoon, so
may not get back to this until Monday.  I'll try to do your other ones today.

[mmenke, 2014-08-13 15:33:51]

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
(right):

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:99:
mPriority);
Hrm...Not sure about the android style guide, but there are a lot of values
we're implicitly relying on the constructors of (mOffset to 0,
mBufferFullResponse to false, etc).

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:113:
* server returns the wrong number, which happens even with Google servers.
This doesn't seem quite right.  Maybe "The compressed content length as reported
by the server.  May be -1 if the server did not provide a length.  Some servers
may also report the wrong number.  Since this is the compressed content length,
and only uncompressed content is returned by the consumer, the consumer should
not rely on this value."

I'm not really sure what the intended use of this function is.  Maybe to
determine if something is likely to be a large/slow download?

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:113:
* server returns the wrong number, which happens even with Google servers.
Should we add Javadoc style comments here?  @returns and all that stuff?  Fine
with putting it off for later, or an answer of "Maybe?"

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:195:
public void addHeader(String header, String value) {
Should this be synchronized?  Heck, should all setters be synchronized?  I
eagerly await the day we move to a more reasonable threading model.

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:206:
* @param contentType MIME type of the post content or null if this is not a
nit:  post -> POST

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:223:
* @param contentType MIME type of the post content or null if this is not a
nit POST?

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:280:
}
indent +2

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:287:
entry.getValue());
Is is really correct indentation?  It looks wrong.  :(

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:288:
}
indent +2

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:375:
mContentLength -= mOffset;
This just seems bizarre...But fixing it is beyond the scope of this CL.

[mef, 2014-08-13 18:52:41]

mmenke: Thanks, PTAL.

others: FYI.

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
(right):

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:99:
mPriority);
On 2014/08/13 15:33:50, mmenke wrote:
> Hrm...Not sure about the android style guide, but there are a lot of values
> we're implicitly relying on the constructors of (mOffset to 0,
> mBufferFullResponse to false, etc).

Acknowledged.

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:113:
* server returns the wrong number, which happens even with Google servers.
On 2014/08/13 15:33:50, mmenke wrote:
> This doesn't seem quite right.  Maybe "The compressed content length as
reported
> by the server.  May be -1 if the server did not provide a length.  Some
servers
> may also report the wrong number.  Since this is the compressed content
length,
> and only uncompressed content is returned by the consumer, the consumer should
> not rely on this value."
> 
> I'm not really sure what the intended use of this function is.  Maybe to
> determine if something is likely to be a large/slow download?

Done.

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:113:
* server returns the wrong number, which happens even with Google servers.
On 2014/08/13 15:33:51, mmenke wrote:
> Should we add Javadoc style comments here?  @returns and all that stuff?  Fine
> with putting it off for later, or an answer of "Maybe?"

I think so? I think that ideally (or possibly absolutely) ALL public methods
should have Javadoc comments.

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:195:
public void addHeader(String header, String value) {
On 2014/08/13 15:33:50, mmenke wrote:
> Should this be synchronized?  Heck, should all setters be synchronized?  I
> eagerly await the day we move to a more reasonable threading model.

Acknowledged. I'd prefer to change it in separate CL though (just to keep this
one as simple as possible).

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:206:
* @param contentType MIME type of the post content or null if this is not a
On 2014/08/13 15:33:49, mmenke wrote:
> nit:  post -> POST

Done.

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:223:
* @param contentType MIME type of the post content or null if this is not a
On 2014/08/13 15:33:49, mmenke wrote:
> nit POST?

Done.

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:280:
}
On 2014/08/13 15:33:51, mmenke wrote:
> indent +2

Done.

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:287:
entry.getValue());
On 2014/08/13 15:33:50, mmenke wrote:
> Is is really correct indentation?  It looks wrong.  :(
Yeah, I'm not sure, I think continuation line is indented to the start of
parameters. I've changed it to 8 spaces.

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:288:
}
On 2014/08/13 15:33:50, mmenke wrote:
> indent +2

Done.

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:375:
mContentLength -= mOffset;
On 2014/08/13 15:33:50, mmenke wrote:
> This just seems bizarre...But fixing it is beyond the scope of this CL.

Ack. Added TODO.

[mmenke, 2014-08-13 19:01:15]

LGTM

[mef, 2014-08-13 19:10:03]

The CQ bit was checked by mef@chromium.org

[commit-bot: I haz the power, 2014-08-13 19:13:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mef@chromium.org/458633002/80001

[commit-bot: I haz the power, 2014-08-14 05:49:27]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[commit-bot: I haz the power, 2014-08-14 11:51:49]

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

[commit-bot: I haz the power, 2014-08-14 11:51:50]

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

[Charles, 2014-08-14 18:23:51]

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
(right):

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:373:
if (getHttpStatusCode() == 200) {
This breaks range requests. Lower down in the file we're rewriting all 206 calls
to be 200 calls.

https://codereview.chromium.org/458633002/diff/80001/components/cronet/androi...
File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
(right):

https://codereview.chromium.org/458633002/diff/80001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:95:
mLock = new Object();
Can we put this where the field is declared?

[mef, 2014-08-14 20:58:02]

clm, thanks, PTAL.

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
(right):

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:373:
if (getHttpStatusCode() == 200) {
On 2014/08/14 18:23:50, clm wrote:
> This breaks range requests. Lower down in the file we're rewriting all 206
calls
> to be 200 calls.

Done.

https://codereview.chromium.org/458633002/diff/80001/components/cronet/androi...
File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
(right):

https://codereview.chromium.org/458633002/diff/80001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:95:
mLock = new Object();
On 2014/08/14 18:23:50, clm wrote:
> Can we put this where the field is declared?

Done.

[Charles, 2014-08-14 21:33:28]

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java
(right):

https://codereview.chromium.org/458633002/diff/40001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:373:
if (getHttpStatusCode() == 200) {
This is still broken - the server will return 20
This isn't fixed, we're still rewriting the status codes. Range requests won't
work.

On 2014/08/14 20:58:02, mef wrote:
> On 2014/08/14 18:23:50, clm wrote:
> > This breaks range requests. Lower down in the file we're rewriting all 206
> calls
> > to be 200 calls.
> 
> Done.

[mef, 2014-08-14 21:46:22]

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

https://codereview.chromium.org/458633002/diff/100001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequest.java:428:
if (nativeGetHttpStatusCode(mUrlRequestAdapter) == 200) {
Using nativeGetHttpStatusCode avoids rewriting in getHttpStatusCode.

[mef, 2014-08-15 13:18:16]

The CQ bit was checked by mef@chromium.org

[commit-bot: I haz the power, 2014-08-15 13:19:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mef@chromium.org/458633002/100001

[commit-bot: I haz the power, 2014-08-15 16:11:54]

Committed patchset #6 (100001) as 289894