Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2844803002:
[Cronet] Make metrics reporting happen after terminal callbacks. (Closed)
Description[Cronet] Make metrics reporting happen after terminal callbacks.
This CL re-arranges metrics report to happen after terminal callbacks
(UrlRequest.onSucceeded, onCanceled, onFailed). This CL adds additional
tests to check that RequestFinishedInfo callback is strictly after UrlRequest's terminal callbacks.
This CL also fixes a bug where if we get an exception from embedder
(onCallbackException), we did not set RequestFinishedReason
to FAILED. testOrderFailedRequestJava is a regression test.
BUG=710877
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
Review-Url: https://codereview.chromium.org/2844803002
Cr-Commit-Position: refs/heads/master@{#468826}
Committed: https://chromium.googlesource.com/chromium/src/+/5d3b9f7cbcf8d2b9b88364ba361935e4de535b42
Patch Set 1 : add tests #Patch Set 2 : self #Patch Set 3 : self #Patch Set 4 : remove an invalid comment #
Total comments: 3
Patch Set 5 : second attempt #Patch Set 6 : self #Patch Set 7 : self #Patch Set 8 : self #
Total comments: 28
Patch Set 9 : address comments #Patch Set 10 : self #
Total comments: 4
Patch Set 11 : address comments #
Messages
Total messages: 36 (25 generated)
Description was changed from ========== Make CronetUrlRequestAdapter destruction happen before terminal callbacks. BUG=710877 ========== to ========== Make CronetUrlRequestAdapter destruction happen before terminal callbacks. BUG=710877 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
Description was changed from ========== Make CronetUrlRequestAdapter destruction happen before terminal callbacks. BUG=710877 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== Make CronetUrlRequestAdapter destruction happen before terminal callbacks. This CL re-arranges native adapter destruction to always happen before terminal callbacks (UrlRequest.onSucceeded, onCanceled, onFailed). A benefit of this arrangement is that now we can guarantee that metrics reporting will always happen after the UrlRequest's terminal callbacks. This CL adds additional tests to make sure that the native object is cleaned up correctly. BUG=710877 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
Description was changed from ========== Make CronetUrlRequestAdapter destruction happen before terminal callbacks. This CL re-arranges native adapter destruction to always happen before terminal callbacks (UrlRequest.onSucceeded, onCanceled, onFailed). A benefit of this arrangement is that now we can guarantee that metrics reporting will always happen after the UrlRequest's terminal callbacks. This CL adds additional tests to make sure that the native object is cleaned up correctly. BUG=710877 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== Make CronetUrlRequestAdapter destruction happen synchronously after terminal callbacks. This CL re-arranges native adapter destruction to always happen after terminal callbacks (UrlRequest.onSucceeded, onCanceled, onFailed). A benefit of this arrangement is that now we can guarantee that metrics reporting will always happen after the UrlRequest's terminal callbacks. This CL adds additional tests to make sure that the native object is cleaned up correctly. BUG=710877 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Description was changed from ========== Make CronetUrlRequestAdapter destruction happen synchronously after terminal callbacks. This CL re-arranges native adapter destruction to always happen after terminal callbacks (UrlRequest.onSucceeded, onCanceled, onFailed). A benefit of this arrangement is that now we can guarantee that metrics reporting will always happen after the UrlRequest's terminal callbacks. This CL adds additional tests to make sure that the native object is cleaned up correctly. BUG=710877 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== Make CronetUrlRequestAdapter destruction happen synchronously after terminal callbacks. This CL re-arranges native adapter destruction to always happen after terminal callbacks (UrlRequest.onSucceeded, onCanceled, onFailed). A benefit of this arrangement is that now we can guarantee that metrics reporting will always happen after the UrlRequest's terminal callbacks. This CL adds additional tests to check that RequestFinishedInfo callback is strictly after UrlRequest's terminal callbacks. This CL also fixes a bug where if we get an exception from embedder (onCallbackException), we did not set RequestFinishedReason to FAILED. testOrderFailedRequestJava is a regression test. BUG=710877 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
xunjieli@chromium.org changed reviewers: + mef@chromium.org
Misha, PTAL. Thanks. Miriam (cc-ed), please feel free to jump in if you have cycles.
Thanks, I'll look at java later, but I have a question for C++ part. https://codereview.chromium.org/2844803002/diff/180001/components/cronet/andr... File components/cronet/android/cronet_url_request_adapter.cc (right): https://codereview.chromium.org/2844803002/diff/180001/components/cronet/andr... components/cronet/android/cronet_url_request_adapter.cc:209: FROM_HERE, base::Bind(&CronetURLRequestAdapter::CancelOnNetworkThread, Hrm, from comment on WeakPtrFactory: // Note that GetWeakPtr must be called on one and only one TaskQueue or thread // and the WeakPtr must only be dereferenced and invalidated on that same // TaskQueue/thread. A WeakPtr instance can be copied and posted to other // sequences though as long as it is not dereferenced (WeakPtr<T>::get()). CronetURLRequestAdapter::Cancel() could be called on any thread though. What am I missing? https://codereview.chromium.org/2844803002/diff/180001/components/cronet/andr... components/cronet/android/cronet_url_request_adapter.cc:250: ReportError(request, net_error); nice!
Description was changed from ========== Make CronetUrlRequestAdapter destruction happen synchronously after terminal callbacks. This CL re-arranges native adapter destruction to always happen after terminal callbacks (UrlRequest.onSucceeded, onCanceled, onFailed). A benefit of this arrangement is that now we can guarantee that metrics reporting will always happen after the UrlRequest's terminal callbacks. This CL adds additional tests to check that RequestFinishedInfo callback is strictly after UrlRequest's terminal callbacks. This CL also fixes a bug where if we get an exception from embedder (onCallbackException), we did not set RequestFinishedReason to FAILED. testOrderFailedRequestJava is a regression test. BUG=710877 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== Make CronetUrlRequestAdapter destruction happen after terminal callbacks. This CL re-arranges native adapter destruction to always happen after terminal callbacks (UrlRequest.onSucceeded, onCanceled, onFailed). A benefit of this arrangement is that now we can guarantee that metrics reporting will always happen after the UrlRequest's terminal callbacks. This CL adds additional tests to check that RequestFinishedInfo callback is strictly after UrlRequest's terminal callbacks. This CL also fixes a bug where if we get an exception from embedder (onCallbackException), we did not set RequestFinishedReason to FAILED. testOrderFailedRequestJava is a regression test. BUG=710877 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
Hopefully the second attempt is slightly more sane? PTAL. Thanks! https://codereview.chromium.org/2844803002/diff/180001/components/cronet/andr... File components/cronet/android/cronet_url_request_adapter.cc (right): https://codereview.chromium.org/2844803002/diff/180001/components/cronet/andr... components/cronet/android/cronet_url_request_adapter.cc:209: FROM_HERE, base::Bind(&CronetURLRequestAdapter::CancelOnNetworkThread, On 2017/04/27 21:33:13, mef wrote: > Hrm, from comment on WeakPtrFactory: > > // Note that GetWeakPtr must be called on one and only one TaskQueue or thread > // and the WeakPtr must only be dereferenced and invalidated on that same > // TaskQueue/thread. A WeakPtr instance can be copied and posted to other > // sequences though as long as it is not dereferenced (WeakPtr<T>::get()). > > CronetURLRequestAdapter::Cancel() could be called on any thread though. > What am I missing? Ah, thanks, You are right! I missed this. I tried another approach which keeps the native code as it is. PTAL.
Patchset #5 (id:200001) has been deleted
Patchset #5 (id:220001) has been deleted
Patchset #5 (id:240001) has been deleted
Patchset #5 (id:260001) has been deleted
Patchset #5 (id:280001) has been deleted
Description was changed from ========== Make CronetUrlRequestAdapter destruction happen after terminal callbacks. This CL re-arranges native adapter destruction to always happen after terminal callbacks (UrlRequest.onSucceeded, onCanceled, onFailed). A benefit of this arrangement is that now we can guarantee that metrics reporting will always happen after the UrlRequest's terminal callbacks. This CL adds additional tests to check that RequestFinishedInfo callback is strictly after UrlRequest's terminal callbacks. This CL also fixes a bug where if we get an exception from embedder (onCallbackException), we did not set RequestFinishedReason to FAILED. testOrderFailedRequestJava is a regression test. BUG=710877 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== [Cronet] Make metrics reporting happen after terminal callbacks. This CL re-arranges metrics report to happen after terminal callbacks (UrlRequest.onSucceeded, onCanceled, onFailed). This CL adds additional tests to check that RequestFinishedInfo callback is strictly after UrlRequest's terminal callbacks. This CL also fixes a bug where if we get an exception from embedder (onCallbackException), we did not set RequestFinishedReason to FAILED. testOrderFailedRequestJava is a regression test. BUG=710877 ==========
Description was changed from ========== [Cronet] Make metrics reporting happen after terminal callbacks. This CL re-arranges metrics report to happen after terminal callbacks (UrlRequest.onSucceeded, onCanceled, onFailed). This CL adds additional tests to check that RequestFinishedInfo callback is strictly after UrlRequest's terminal callbacks. This CL also fixes a bug where if we get an exception from embedder (onCallbackException), we did not set RequestFinishedReason to FAILED. testOrderFailedRequestJava is a regression test. BUG=710877 ========== to ========== [Cronet] Make metrics reporting happen after terminal callbacks. This CL re-arranges metrics report to happen after terminal callbacks (UrlRequest.onSucceeded, onCanceled, onFailed). This CL adds additional tests to check that RequestFinishedInfo callback is strictly after UrlRequest's terminal callbacks. This CL also fixes a bug where if we get an exception from embedder (onCallbackException), we did not set RequestFinishedReason to FAILED. testOrderFailedRequestJava is a regression test. BUG=710877 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
Looks much better! Few comments, mostly to simplify things. I'm yet to look at tests. https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... File components/cronet/android/cronet_url_request_adapter.h (right): https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/cronet_url_request_adapter.h:159: // Whether metrics has been reported to Java. nit: have https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java (right): https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:90: // They are read on executor's thread after the last update. Should they be @GuardedBy("mUrlRequestAdapterLock")? https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:242: synchronized (mUrlRequestAdapterLock) { This whole method is synchronized() at line 191, so I don't think this is needed here. https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:322: private void setIsDoneLocked( I really think it should still be called destroyRequestAdapterLocked as that's what it does. https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:470: setIsDoneLocked(exception, RequestFinishedInfo.FAILED); Can we set mException = exception; here and just pass RequestFinishedInfo.Reason to destroyRequestAdapterLocked()? This way other callers wouldn't need to pass null. https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:472: // Wait until onDestroy() is called to propagate the error to caller to I would rephrase this like 'The onFailed callback will be invoked from onNativeAdapterDestroyed() to ensure metrics collection. https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:646: CronetException exception; Why do we need local |exception| here? https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:669: if (mMetrics != null) { Does it make sense to extract this into maybeReportFinished or somesuch from here and in onSucceeded and onDestroyed? https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:718: } Maybe add comment that metrics will be reported after final callback? https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:726: private void onDestroyed() { Maybe (not strongly) call it onNativeAdapterDestroyed? https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:731: if (mException == null) { Need a comment as to why we check mException and possibly post onFailed from here. https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java (right): https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java:35: // listener runs callbacks on a separated thread, so tests use this is racy by design. See nit: could you rephrase 'so tests use this is racy by design'.
mgersh@chromium.org changed reviewers: + mgersh@chromium.org
Thank you so much for picking this up! https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java (right): https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:90: // They are read on executor's thread after the last update. this could clarify which executor https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java (right): https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java:33: // A subclass of TestRequestFinishedListener to additionally assert that UrlRequest.Callback's What do you think about modifying TestRequestFinishedListener to do this instead of having a subclass and separate tests that use it? Some of these new tests are essentially copies of existing ones with an extra assert, and in those cases I don't think we need two separate tests.
Description was changed from ========== [Cronet] Make metrics reporting happen after terminal callbacks. This CL re-arranges metrics report to happen after terminal callbacks (UrlRequest.onSucceeded, onCanceled, onFailed). This CL adds additional tests to check that RequestFinishedInfo callback is strictly after UrlRequest's terminal callbacks. This CL also fixes a bug where if we get an exception from embedder (onCallbackException), we did not set RequestFinishedReason to FAILED. testOrderFailedRequestJava is a regression test. BUG=710877 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== [Cronet] Make metrics reporting happen after terminal callbacks. This CL re-arranges metrics report to happen after terminal callbacks (UrlRequest.onSucceeded, onCanceled, onFailed). This CL adds additional tests to check that RequestFinishedInfo callback is strictly after UrlRequest's terminal callbacks. This CL also fixes a bug where if we get an exception from embedder (onCallbackException), we did not set RequestFinishedReason to FAILED. testOrderFailedRequestJava is a regression test. BUG=710877 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
Thank you both! PTAL. https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... File components/cronet/android/cronet_url_request_adapter.h (right): https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/cronet_url_request_adapter.h:159: // Whether metrics has been reported to Java. On 2017/04/28 21:16:54, mef wrote: > nit: have Done. https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java (right): https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:90: // They are read on executor's thread after the last update. On 2017/05/01 15:29:22, mgersh wrote: > this could clarify which executor Done. https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:90: // They are read on executor's thread after the last update. On 2017/04/28 21:16:54, mef wrote: > Should they be @GuardedBy("mUrlRequestAdapterLock")? These three are not guarded by the lock because they are read on the executor's thread without holding the lock. https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:242: synchronized (mUrlRequestAdapterLock) { On 2017/04/28 21:16:54, mef wrote: > This whole method is synchronized() at line 191, so I don't think this is needed > here. Done. https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:322: private void setIsDoneLocked( On 2017/04/28 21:16:55, mef wrote: > I really think it should still be called destroyRequestAdapterLocked as that's > what it does. Done. Partly yes, but now it also updates |mException| and |mFinishedReason|. I think it will be a nice counterpart to isDone(). I guess we could discuss that in a separate CL. I have changed it back. https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:470: setIsDoneLocked(exception, RequestFinishedInfo.FAILED); On 2017/04/28 21:16:54, mef wrote: > Can we set mException = exception; here and just pass RequestFinishedInfo.Reason > to destroyRequestAdapterLocked()? > This way other callers wouldn't need to pass null. Done. https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:472: // Wait until onDestroy() is called to propagate the error to caller to On 2017/04/28 21:16:54, mef wrote: > I would rephrase this like 'The onFailed callback will be invoked from > onNativeAdapterDestroyed() to ensure metrics collection. Done. https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:646: CronetException exception; On 2017/04/28 21:16:54, mef wrote: > Why do we need local |exception| here? Done. Because we can acquire mUrlRequestAdapterLock once for all in failWithException() ? I got rid of it. https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:669: if (mMetrics != null) { On 2017/04/28 21:16:54, mef wrote: > Does it make sense to extract this into maybeReportFinished or somesuch from > here and in onSucceeded and onDestroyed? Done. https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:718: } On 2017/04/28 21:16:54, mef wrote: > Maybe add comment that metrics will be reported after final callback? Done. https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:726: private void onDestroyed() { On 2017/04/28 21:16:55, mef wrote: > Maybe (not strongly) call it onNativeAdapterDestroyed? Done. https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:731: if (mException == null) { On 2017/04/28 21:16:55, mef wrote: > Need a comment as to why we check mException and possibly post onFailed from > here. Done. https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java (right): https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java:33: // A subclass of TestRequestFinishedListener to additionally assert that UrlRequest.Callback's On 2017/05/01 15:29:22, mgersh wrote: > What do you think about modifying TestRequestFinishedListener to do this instead > of having a subclass and separate tests that use it? This subclass associates with exactly on UrlRequest.callback, but a RequestFinishedListener can be used to report metrics for many UrlRequests. I think it's better to keep them separate so we can use TestRequestFinishedListener for multiple requests scenario. > Some of these new tests are > essentially copies of existing ones with an extra assert, and in those cases I > don't think we need two separate tests. I started by modifying the existing tests, but found myself adding tests for scenarios that weren't covered (e.g. error from Java). I think it's slightly more self-contained to do it this way, so we do not modify some test cases to use AssertCallbackDoneRequestFinishedListener and but not others? https://codereview.chromium.org/2844803002/diff/60002/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java:35: // listener runs callbacks on a separated thread, so tests use this is racy by design. See On 2017/04/28 21:16:55, mef wrote: > nit: could you rephrase 'so tests use this is racy by design'. Done.
lgtm with minor comments https://codereview.chromium.org/2844803002/diff/390001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java (right): https://codereview.chromium.org/2844803002/diff/390001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:90: // URLRequest.Callback executor's and RequestFinishedListener's thread after the last update. nits: UrlRequest, RequestFinishedInfo.Listener. These are two different executors, so "thread" should be "threads", or maybe rephrase to something like "both UrlRequest.Callback and RequestFinishedInfo.Listener's executors". https://codereview.chromium.org/2844803002/diff/390001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:471: // collection nit: missing period at the end
lgtm
Thanks for the review! https://codereview.chromium.org/2844803002/diff/390001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java (right): https://codereview.chromium.org/2844803002/diff/390001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:90: // URLRequest.Callback executor's and RequestFinishedListener's thread after the last update. On 2017/05/02 18:21:12, mgersh wrote: > nits: UrlRequest, RequestFinishedInfo.Listener. > > These are two different executors, so "thread" should be "threads", or maybe > rephrase to something like "both UrlRequest.Callback and > RequestFinishedInfo.Listener's executors". Done. https://codereview.chromium.org/2844803002/diff/390001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java:471: // collection On 2017/05/02 18:21:13, mgersh wrote: > nit: missing period at the end Done.
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mgersh@chromium.org, mef@chromium.org Link to the patchset: https://codereview.chromium.org/2844803002/#ps410001 (title: "address comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 410001, "attempt_start_ts": 1493767828599630,
"parent_rev": "421b9c4b9736e52c903123994d977b4ed406cf72", "commit_rev":
"5d3b9f7cbcf8d2b9b88364ba361935e4de535b42"}
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Make metrics reporting happen after terminal callbacks. This CL re-arranges metrics report to happen after terminal callbacks (UrlRequest.onSucceeded, onCanceled, onFailed). This CL adds additional tests to check that RequestFinishedInfo callback is strictly after UrlRequest's terminal callbacks. This CL also fixes a bug where if we get an exception from embedder (onCallbackException), we did not set RequestFinishedReason to FAILED. testOrderFailedRequestJava is a regression test. BUG=710877 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== [Cronet] Make metrics reporting happen after terminal callbacks. This CL re-arranges metrics report to happen after terminal callbacks (UrlRequest.onSucceeded, onCanceled, onFailed). This CL adds additional tests to check that RequestFinishedInfo callback is strictly after UrlRequest's terminal callbacks. This CL also fixes a bug where if we get an exception from embedder (onCallbackException), we did not set RequestFinishedReason to FAILED. testOrderFailedRequestJava is a regression test. BUG=710877 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2844803002 Cr-Commit-Position: refs/heads/master@{#468826} Committed: https://chromium.googlesource.com/chromium/src/+/5d3b9f7cbcf8d2b9b88364ba3619... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:410001) as https://chromium.googlesource.com/chromium/src/+/5d3b9f7cbcf8d2b9b88364ba3619... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
