Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
520093002:
Initial declaration of Cronet Async API. (Closed)
DescriptionInitial declaration of Cronet Async API.
BUG=409926
Committed: https://crrev.com/388ac9eb61450e7e4135e195974f36810c0c69e9
Cr-Commit-Position: refs/heads/master@{#293345}
Patch Set 1 #
Total comments: 26
Patch Set 2 : Address review comments, add to cronet.gyp #
Total comments: 22
Patch Set 3 : Address review comments. #
Total comments: 1
Patch Set 4 : Sync #
Total comments: 18
Messages
Total messages: 19 (5 generated)
mef@chromium.org changed reviewers: + mmenke@chromium.org, xunjieli@chromium.org
First draft.
https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... File components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java (right): https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java:3: // found in the LICENSE file. nit: Blank line after license header. https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java:8: // Note: All methods must be called on the executor passed in during creation. nit: Executor https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java:9: public abstract interface AsyncUrlRequest { Maybe some class level description? https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java:21: public void setRequestBodyByteBuffer(ByteBuffer buffer); Let's get rid of all the request body stuff - I think it's better using a separate class for this (Which is what the drafts I've been playing around with do). https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java:79: * @return @return {boolean} true if the request has been cancelled by the embedder. False in all other cases (Including errors). Hrm... I wonder if we really need this method. https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java:90: public void pause(); I want to take a little extra time to think about this. It seems like there are 3 ways to handle this: 1) Only pause when requests, like this. 2) Always pause, and require the embedder to explicitly continue the request. 3) Take a return value when passing along data, and rely on the embedded to resume only if a request is paused. This is how the network stack works internally. I think that in terms of complexity for the embedder, this approach is simplest. In approach 2, everyone has to worry about resuming the request in ever callback, which seems like a bit of a pain. In approach 3, rather than having every callback (On receive header, on redirect, on body data receive) all have their own return value to handle pausing, and maybe their own resume functions as well, we just have one pause function, which works at any point. One the downside, uploads can't work in this fashion, they instead have to work like 2) or 3). We also need more JNI calls when the embedder pauses than in 2) or 3), though 2) has more JNI calls in the never pause case. https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... File components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestFactory.java (right): https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestFactory.java:11: * Created by mef on 8/29/14. ? https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestFactory.java:17: * on the executor’s thread as well. nit: Executor (x3) https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... File components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java (right): https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java:22: * @param new_location Should we add documentation now? https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java:36: * Called for each chunk of data that’s received. The ByteBuffer remains nit: --chunk. Maybe "called whenever data is received"? https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java:40: * https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java:84: public boolean rewindUploadData(AsyncUrlRequest request); Again, let's get rid of these two upload functions.
Thanks, PTAL! https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... File components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java (right): https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java:3: // found in the LICENSE file. On 2014/09/02 19:48:03, mmenke wrote: > nit: Blank line after license header. Done. https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java:8: // Note: All methods must be called on the executor passed in during creation. On 2014/09/02 19:48:03, mmenke wrote: > nit: Executor Done. https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java:9: public abstract interface AsyncUrlRequest { On 2014/09/02 19:48:03, mmenke wrote: > Maybe some class level description? Done. https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java:21: public void setRequestBodyByteBuffer(ByteBuffer buffer); On 2014/09/02 19:48:03, mmenke wrote: > Let's get rid of all the request body stuff - I think it's better using a > separate class for this (Which is what the drafts I've been playing around with > do). Done. https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java:79: * @return On 2014/09/02 19:48:03, mmenke wrote: > @return {boolean} true if the request has been cancelled by the embedder. False > in all other cases (Including errors). > > Hrm... I wonder if we really need this method. Are you sure about error case? What would happen then? https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java:90: public void pause(); On 2014/09/02 19:48:03, mmenke wrote: > I want to take a little extra time to think about this. It seems like there are > 3 ways to handle this: > > 1) Only pause when requests, like this. > 2) Always pause, and require the embedder to explicitly continue the request. > 3) Take a return value when passing along data, and rely on the embedded to > resume only if a request is paused. This is how the network stack works > internally. > > I think that in terms of complexity for the embedder, this approach is simplest. > In approach 2, everyone has to worry about resuming the request in ever > callback, which seems like a bit of a pain. In approach 3, rather than having > every callback (On receive header, on redirect, on body data receive) all have > their own return value to handle pausing, and maybe their own resume functions > as well, we just have one pause function, which works at any point. > > One the downside, uploads can't work in this fashion, they instead have to work > like 2) or 3). We also need more JNI calls when the embedder pauses than in 2) > or 3), though 2) has more JNI calls in the never pause case. Acknowledged. https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... File components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestFactory.java (right): https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestFactory.java:11: * Created by mef on 8/29/14. On 2014/09/02 19:48:03, mmenke wrote: > ? Done. Autodoc. :) https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestFactory.java:17: * on the executor’s thread as well. On 2014/09/02 19:48:03, mmenke wrote: > nit: Executor (x3) Done. https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestFactory.java:23: public AsyncUrlRequest createAsyncRequest(URL url, Executor executor) { I think it also needs to take listener. https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... File components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java (right): https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java:22: * @param new_location On 2014/09/02 19:48:04, mmenke wrote: > Should we add documentation now? Done. https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java:36: * Called for each chunk of data that’s received. The ByteBuffer remains On 2014/09/02 19:48:03, mmenke wrote: > nit: --chunk. > > Maybe "called whenever data is received"? Done. https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java:40: On 2014/09/02 19:48:04, mmenke wrote: > * Done. https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java:84: public boolean rewindUploadData(AsyncUrlRequest request); On 2014/09/02 19:48:04, mmenke wrote: > Again, let's get rid of these two upload functions. Done.
https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... File components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java (right): https://codereview.chromium.org/520093002/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java:90: public void pause(); On 2014/09/03 20:58:58, mef wrote: > On 2014/09/02 19:48:03, mmenke wrote: > > I want to take a little extra time to think about this. It seems like there > are > > 3 ways to handle this: > > > > 1) Only pause when requests, like this. > > 2) Always pause, and require the embedder to explicitly continue the request. > > 3) Take a return value when passing along data, and rely on the embedded to > > resume only if a request is paused. This is how the network stack works > > internally. > > > > I think that in terms of complexity for the embedder, this approach is > simplest. > > In approach 2, everyone has to worry about resuming the request in ever > > callback, which seems like a bit of a pain. In approach 3, rather than having > > every callback (On receive header, on redirect, on body data receive) all have > > their own return value to handle pausing, and maybe their own resume functions > > as well, we just have one pause function, which works at any point. > > > > One the downside, uploads can't work in this fashion, they instead have to > work > > like 2) or 3). We also need more JNI calls when the embedder pauses than in > 2) > > or 3), though 2) has more JNI calls in the never pause case. > > Acknowledged. Wow, that's a mess. I really should be more careful about typos. https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java (right): https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java:12: // Remove empty comment and blank line. https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java:22: * "POST" or "PUT". I don't think there's any need for these restrictions. https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java:52: * @return {boolean} true if the request has been cancelled by the embedder. nit: Looks like I was wrong about using the {} https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java:52: * @return {boolean} true if the request has been cancelled by the embedder. On 2014/09/03 20:58:58, mef wrote: > On 2014/09/02 19:48:03, mmenke wrote: > > @return {boolean} true if the request has been cancelled by the embedder. > False > > in all other cases (Including errors). > > > > Hrm... I wonder if we really need this method. > > Are you sure about error case? What would happen then? If it returns true for errors, it should be renamed (failed? didFail?). We may just want one status() function, which returns one of: pending, paused, failed, cancelled, succeeded. https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestFactory.java (right): https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestFactory.java:19: * @param url URL for the frequest -f + period. https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java (right): https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java:11: * Note: All methods will be called on the thread of the Looper used during Looper -> Executor https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java:23: * @param new_location Location where request is redirected nit: +period https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java:56: * raw numeric error code, and a string version of the error. Remove comment about CronetError https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ChromiumAsyncUrlRequest.java (right): https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumAsyncUrlRequest.java:8: * Async request using the native http stack implementation. nit: HTTP? https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ResponseInfo.java (right): https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ResponseInfo.java:13: * this list is not intended to be complete, though comments are welcome. Should clean up these comments. Maybe something like: Contains basic information about a response. Sent to the embedder whenever headers are received. https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ResponseInfo.java:44: */ nit: Fix indent.
Thanks, PTAL! I'm open to idea of having one status() on the AsyncUrlRequest, but I'm wondering whether this will be useful to embedder. https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java (right): https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java:12: // On 2014/09/03 21:19:20, mmenke wrote: > Remove empty comment and blank line. Done. https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java:22: * "POST" or "PUT". On 2014/09/03 21:19:20, mmenke wrote: > I don't think there's any need for these restrictions. Done. https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java:52: * @return {boolean} true if the request has been cancelled by the embedder. On 2014/09/03 21:19:20, mmenke wrote: > nit: Looks like I was wrong about using the {} Done. https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java:52: * @return {boolean} true if the request has been cancelled by the embedder. On 2014/09/03 21:19:20, mmenke wrote: > On 2014/09/03 20:58:58, mef wrote: > > On 2014/09/02 19:48:03, mmenke wrote: > > > @return {boolean} true if the request has been cancelled by the embedder. > > False > > > in all other cases (Including errors). > > > > > > Hrm... I wonder if we really need this method. > > > > Are you sure about error case? What would happen then? > > If it returns true for errors, it should be renamed (failed? didFail?). We may > just want one status() function, which returns one of: pending, paused, failed, > cancelled, succeeded. Acknowledged. https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestFactory.java (right): https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestFactory.java:19: * @param url URL for the frequest On 2014/09/03 21:19:20, mmenke wrote: > -f + period. Done. https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java (right): https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java:11: * Note: All methods will be called on the thread of the Looper used during On 2014/09/03 21:19:20, mmenke wrote: > Looper -> Executor Done. https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java:23: * @param new_location Location where request is redirected On 2014/09/03 21:19:20, mmenke wrote: > nit: +period Done. https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java:56: * raw numeric error code, and a string version of the error. On 2014/09/03 21:19:20, mmenke wrote: > Remove comment about CronetError Done. https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ChromiumAsyncUrlRequest.java (right): https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumAsyncUrlRequest.java:8: * Async request using the native http stack implementation. On 2014/09/03 21:19:20, mmenke wrote: > nit: HTTP? Done. https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ResponseInfo.java (right): https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ResponseInfo.java:13: * this list is not intended to be complete, though comments are welcome. On 2014/09/03 21:19:20, mmenke wrote: > Should clean up these comments. Maybe something like: > > Contains basic information about a response. Sent to the embedder whenever > headers are received. Done. https://codereview.chromium.org/520093002/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ResponseInfo.java:44: */ On 2014/09/03 21:19:20, mmenke wrote: > nit: Fix indent. Done.
On 2014/09/03 21:55:39, mef wrote: > Thanks, PTAL! I'm open to idea of having one status() on the AsyncUrlRequest, > but I'm wondering whether this will be useful to embedder. That's a good question. My general feeling is if isCancelled and isPaused are useful, the other states may be as well. But I'm not convinced they're useful. Maybe remove both for now, and see if we need them? Anyhow, LGTM, either way. https://codereview.chromium.org/520093002/diff/40001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java (right): https://codereview.chromium.org/520093002/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java:61: * @param error information about error. nit: capitalize information?
The CQ bit was checked by mef@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/520093002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...) win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by mef@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/520093002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as 6135e8facbec020030e51024c004b77dc57b2140
Message was sent while issue was closed.
clm@google.com changed reviewers: + clm@google.com
Message was sent while issue was closed.
I'd also like to get rid of the Async prefix on all these, async should be the default. https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java (right): https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java:11: public abstract interface AsyncUrlRequest { Abstract interface is redundant. All interfaces are abstract. https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java:26: public void setHttpMethod(String method); I wonder if we ought to make this an enum of some kind. https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestFactory.java (right): https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestFactory.java:13: public abstract class AsyncUrlRequestFactory { This class is entirely abstract methods - let's make it an interface. https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java (right): https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java:11: * Note: All methods will be called on the thread of the Executor used during We should add more doc about what the threading behavior of this class is, and whether clients need to synchronize. https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java:14: public abstract interface AsyncUrlRequestListener { Abstract is redundant. https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java:25: public void onRedirect(AsyncUrlRequest request, ResponseInfo info, I suspect that most consumers won't care about listening for redirects. Maybe we should break this up into multiple sub-interfaces? https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ResponseInfo.java (right): https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ResponseInfo.java:15: public abstract interface ResponseInfo { Abstract is redundant https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ResponseInfo.java:51: boolean wasFetchedOverSPDY(); I'd rather have a method that returns an enum of some sort, otherwise we'll break compatibility when we add new protocols.
Message was sent while issue was closed.
https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java (right): https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java:26: public void setHttpMethod(String method); On 2014/09/05 18:10:56, Charles wrote: > I wonder if we ought to make this an enum of some kind. I feel we shouldn't be limiting methods here, and should just let the embedder set what they feel like. https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java (right): https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java:26: URL new_location); This argument uses C++ naming style, rather than Java. https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ResponseInfo.java (right): https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ResponseInfo.java:51: boolean wasFetchedOverSPDY(); On 2014/09/05 18:10:57, Charles wrote: > I'd rather have a method that returns an enum of some sort, otherwise we'll > break compatibility when we add new protocols. We've been discussing this over at https://codereview.chromium.org/544223003/
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/388ac9eb61450e7e4135e195974f36810c0c69e9 Cr-Commit-Position: refs/heads/master@{#293345}
Message was sent while issue was closed.
Thanks! I've created https://codereview.chromium.org/566833003/ to address comments added after landing. https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java (right): https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java:11: public abstract interface AsyncUrlRequest { On 2014/09/05 18:10:56, Charles wrote: > Abstract interface is redundant. All interfaces are abstract. Done. https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequest.java:26: public void setHttpMethod(String method); On 2014/09/05 19:14:39, mmenke wrote: > On 2014/09/05 18:10:56, Charles wrote: > > I wonder if we ought to make this an enum of some kind. > > I feel we shouldn't be limiting methods here, and should just let the embedder > set what they feel like. Done. https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestFactory.java (right): https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestFactory.java:13: public abstract class AsyncUrlRequestFactory { On 2014/09/05 18:10:57, Charles wrote: > This class is entirely abstract methods - let's make it an interface. Done. https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestFactory.java:25: AsyncUrlRequestListener listener, Executor executor); Should listener be passed to createRequest or to UrlRequest.start? https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java (right): https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java:14: public abstract interface AsyncUrlRequestListener { On 2014/09/05 18:10:57, Charles wrote: > Abstract is redundant. Done. https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java:25: public void onRedirect(AsyncUrlRequest request, ResponseInfo info, On 2014/09/05 18:10:57, Charles wrote: > I suspect that most consumers won't care about listening for redirects. Maybe we > should break this up into multiple sub-interfaces? Acknowledged. https://codereview.chromium.org/520093002/diff/60001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/AsyncUrlRequestListener.java:26: URL new_location); On 2014/09/05 19:14:39, mmenke wrote: > This argument uses C++ naming style, rather than Java. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
