Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2265873002:
Adjust callers and networking delegates in net/ to modified APIs (Closed)
|
Created:
4 years, 4 months ago by maksims (do not use this acc) Modified:
4 years, 3 months ago Reviewers:
mmenke CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@URLRequestRead Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdjust callers and networking delegates in net/ to modified APIs
Use modified Read and delegate methods from the following CL-
https://codereview.chromium.org/2262653003/
BUG=423484
Committed: https://crrev.com/65db2c48753ba5a516e9a4147f93a9c218d5f76e
Cr-Commit-Position: refs/heads/master@{#416511}
Patch Set 1 #Patch Set 2 #Patch Set 3 : rebased #
Total comments: 18
Patch Set 4 : Matt's comments #
Total comments: 17
Patch Set 5 : more comments #Patch Set 6 : rebased on top of URLRequest::Read CL #Depends on Patchset: Messages
Total messages: 117 (103 generated)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:80001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Patchset #1 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Patchset #1 (id:120001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:140001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Patchset #1 (id:160001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:180001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Patchset #1 (id:200001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #5 (id:300001) has been deleted
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Adjust callers and networking delegates in net/ to modified APIs BUG= ========== to ========== Adjust callers and networking delegates in net/ to modified APIs Use modified Read and delegate methods from the following CL- https://codereview.chromium.org/2262653003/ BUG=423484 ==========
maksim.sisov@intel.com changed reviewers: + mmenke@chromium.org
Patchset #5 (id:320001) has been deleted
Patchset #4 (id:280001) has been deleted
Patchset #3 (id:260001) has been deleted
would you please take a look into this as well?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Haven't made it all the way through. Was initially very skeptical of Canceled() returning a value, but it's begun to grow on me. https://codereview.chromium.org/2265873002/diff/340001/net/cert_net/cert_net_... File net/cert_net/cert_net_fetcher_impl.cc (right): https://codereview.chromium.org/2265873002/diff/340001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:314: int status = request->CancelWithError(error); Can we call these results, instead? net:Errors are generally thought of more as the result of an operation, rather than an ongoing status. (There's a comment below about replacing these two lines with a function call, but I mean in general) https://codereview.chromium.org/2265873002/diff/340001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:392: void CertNetFetcherImpl::Job::OnTimeout() { Suggestion: Make this FailRequest(net::Error error). Then have this call CancelWithError / OnJobCompleted. Then have everything that does CancelWithError and then calls OnUrlRequestCompleted just call this instead. The one caller of OnTimeout can use base::Bind(Job::OnTimeout&, this, ERR_TIMED_OUT) instead. That makes all the cancellation code go through the same cancel/complete calls. https://codereview.chromium.org/2265873002/diff/340001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:405: result_net_error_ = static_cast<Error>(net_error); This whole thing is just: result_net_error_ = static_cast<Error>(net_error); https://codereview.chromium.org/2265873002/diff/340001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:410: void CertNetFetcherImpl::Job::OnJobCompleted() { Suggestion: Make this OnJobCompleted(net::Error), and set result_net_error_ here (And only here). That ensures all paths set it, and gets rid of redundant code. OR better, I think you can just get rid of result_net_error_ entirely, if you do that? It's only used in this method, I believe. https://codereview.chromium.org/2265873002/diff/340001/net/cert_net/nss_ocsp.cc File net/cert_net/nss_ocsp.cc (right): https://codereview.chromium.org/2265873002/diff/340001/net/cert_net/nss_ocsp.... net/cert_net/nss_ocsp.cc:326: } while (bytes_read > 0); This final while condition is no longer needed. This loop is now the same as: while (bytes_read <= 0) { /* append / read calls go here, same as before */ } https://codereview.chromium.org/2265873002/diff/340001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2265873002/diff/340001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:848: if (net_error != OK || net_error == ERR_IO_PENDING) The ERR_IO_PENDING part of this comparison is redundant. This whole call seems a bit problematic (Even with the old code), but fixing it seems beyond the scope of this CL. https://codereview.chromium.org/2265873002/diff/340001/net/proxy/proxy_script... File net/proxy/proxy_script_fetcher_impl.cc (right): https://codereview.chromium.org/2265873002/diff/340001/net/proxy/proxy_script... net/proxy/proxy_script_fetcher_impl.cc:266: } Think this is clearer as: if (num_bytes == ERR_IO_PENDING) return; if (num_bytes < 0) { ... return; } if (!ConsumeBytesRead(...)) return; A tad more code, but keeping each case separate makes it easier to follow, rather than grouping the ERR_IO_PENDING and error cases in the first if. https://codereview.chromium.org/2265873002/diff/340001/net/test/spawned_test_... File net/test/spawned_test_server/spawner_communicator.cc (right): https://codereview.chromium.org/2265873002/diff/340001/net/test/spawned_test_... net/test/spawned_test_server/spawner_communicator.cc:233: OnSpawnerCommandCompleted(cur_request_.get(), result); This code is actually buggy - note the ERR_TIMED_OUT above, and that OnSpawnerCommandCompleted clears that error. Just remove the SetResultCode line, and use CancelWithError(ERR_TIMED_OUT), and this should be fine. OnSpawnerCommandCompleted(cur_request_.get(), ERR_TIMED_OUT); https://codereview.chromium.org/2265873002/diff/340001/net/test/spawned_test_... net/test/spawned_test_server/spawner_communicator.cc:289: } As suggested elsewhere, suggest 3 non-nested if statements here
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #4 (id:360001) has been deleted
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Patchset #4 (id:380001) has been deleted
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2265873002/diff/340001/net/cert_net/cert_net_... File net/cert_net/cert_net_fetcher_impl.cc (right): https://codereview.chromium.org/2265873002/diff/340001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:314: int status = request->CancelWithError(error); On 2016/08/30 22:13:22, mmenke wrote: > Can we call these results, instead? net:Errors are generally thought of more as > the result of an operation, rather than an ongoing status. (There's a comment > below about replacing these two lines with a function call, but I mean in > general) Done. https://codereview.chromium.org/2265873002/diff/340001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:392: void CertNetFetcherImpl::Job::OnTimeout() { On 2016/08/30 22:13:22, mmenke wrote: > Suggestion: Make this FailRequest(net::Error error). Then have this call > CancelWithError / OnJobCompleted. > > Then have everything that does CancelWithError and then calls > OnUrlRequestCompleted just call this instead. The one caller of OnTimeout can > use base::Bind(Job::OnTimeout&, this, ERR_TIMED_OUT) instead. That makes all > the cancellation code go through the same cancel/complete calls. Done. https://codereview.chromium.org/2265873002/diff/340001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:405: result_net_error_ = static_cast<Error>(net_error); On 2016/08/30 22:13:22, mmenke wrote: > This whole thing is just: > > result_net_error_ = static_cast<Error>(net_error); Done. https://codereview.chromium.org/2265873002/diff/340001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:410: void CertNetFetcherImpl::Job::OnJobCompleted() { On 2016/08/30 22:13:22, mmenke wrote: > Suggestion: Make this OnJobCompleted(net::Error), and set result_net_error_ > here (And only here). That ensures all paths set it, and gets rid of redundant > code. OR better, I think you can just get rid of result_net_error_ entirely, if > you do that? It's only used in this method, I believe. Yes, you're right. Done! https://codereview.chromium.org/2265873002/diff/340001/net/cert_net/nss_ocsp.cc File net/cert_net/nss_ocsp.cc (right): https://codereview.chromium.org/2265873002/diff/340001/net/cert_net/nss_ocsp.... net/cert_net/nss_ocsp.cc:326: } while (bytes_read > 0); On 2016/08/30 22:13:22, mmenke wrote: > This final while condition is no longer needed. This loop is now the same as: > > while (bytes_read <= 0) { /* append / read calls go here, same as before */ } Done! https://codereview.chromium.org/2265873002/diff/340001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2265873002/diff/340001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:848: if (net_error != OK || net_error == ERR_IO_PENDING) On 2016/08/30 22:13:22, mmenke wrote: > The ERR_IO_PENDING part of this comparison is redundant. > > This whole call seems a bit problematic (Even with the old code), but fixing it > seems beyond the scope of this CL. Do you mean all the paths here? Do you think they should be separate functions? https://codereview.chromium.org/2265873002/diff/340001/net/proxy/proxy_script... File net/proxy/proxy_script_fetcher_impl.cc (right): https://codereview.chromium.org/2265873002/diff/340001/net/proxy/proxy_script... net/proxy/proxy_script_fetcher_impl.cc:266: } On 2016/08/30 22:13:22, mmenke wrote: > Think this is clearer as: > > if (num_bytes == ERR_IO_PENDING) > return; > > if (num_bytes < 0) { > ... > return; > } > > if (!ConsumeBytesRead(...)) > return; > > > A tad more code, but keeping each case separate makes it easier to follow, > rather than grouping the ERR_IO_PENDING and error cases in the first if. Done. https://codereview.chromium.org/2265873002/diff/340001/net/test/spawned_test_... File net/test/spawned_test_server/spawner_communicator.cc (right): https://codereview.chromium.org/2265873002/diff/340001/net/test/spawned_test_... net/test/spawned_test_server/spawner_communicator.cc:233: OnSpawnerCommandCompleted(cur_request_.get(), result); On 2016/08/30 22:13:22, mmenke wrote: > This code is actually buggy - note the ERR_TIMED_OUT above, and that > OnSpawnerCommandCompleted clears that error. > > Just remove the SetResultCode line, and use CancelWithError(ERR_TIMED_OUT), and > this should be fine. > OnSpawnerCommandCompleted(cur_request_.get(), ERR_TIMED_OUT); Done. https://codereview.chromium.org/2265873002/diff/340001/net/test/spawned_test_... net/test/spawned_test_server/spawner_communicator.cc:289: } On 2016/08/30 22:13:22, mmenke wrote: > As suggested elsewhere, suggest 3 non-nested if statements here Done.
Looks good! https://codereview.chromium.org/2265873002/diff/400001/net/cert_net/cert_net_... File net/cert_net/cert_net_fetcher_impl.cc (right): https://codereview.chromium.org/2265873002/diff/400001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:306: base::Bind(&Job::OnTimeout, base::Unretained(this), ERR_TIMED_OUT)); Can get rid of OnTimeout entirely and just use FailRequest here instead. https://codereview.chromium.org/2265873002/diff/400001/net/url_request/sdch_d... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/2265873002/diff/400001/net/url_request/sdch_d... net/url_request/sdch_dictionary_fetcher.cc:37: int rv = bytes_read; nit: Let's just rename bytes_read to "rv" instead. https://codereview.chromium.org/2265873002/diff/400001/net/url_request/url_fe... File net/url_request/url_fetcher_core.cc (right): https://codereview.chromium.org/2265873002/diff/400001/net/url_request/url_fe... net/url_request/url_fetcher_core.cc:471: } while (bytes_read > 0); This final while is now redundant with the if at the start of this loop. Just remove them both and use: while (bytes_read > 0) {...} https://codereview.chromium.org/2265873002/diff/400001/net/url_request/url_re... File net/url_request/url_request_file_dir_job_unittest.cc (right): https://codereview.chromium.org/2265873002/diff/400001/net/url_request/url_re... net/url_request/url_request_file_dir_job_unittest.cc:133: EXPECT_THAT(bytes_read, ERR_FILE_NOT_FOUND); This should be "EXPECT_THAT(bytes_read, IsError(ERR_FILE_NOTFOUND));" If that doesn't work (I think it should), then it should be "EXPECT_EQ(ERR_FILE_NOT_FOUND, bytes_read);" https://codereview.chromium.org/2265873002/diff/400001/net/url_request/url_re... File net/url_request/url_request_ftp_job_unittest.cc (right): https://codereview.chromium.org/2265873002/diff/400001/net/url_request/url_re... net/url_request/url_request_ftp_job_unittest.cc:312: EXPECT_EQ(OK, request_delegate.request_status()); These should probably be "EXPECT_THAT(request_delegate.request_status(), IsOk())" (And include net/test/gtest_util.h). Same goes for all instances of EXPECT_EQ(OK, blah); in this CL. They print out more useful messages on failure. https://codereview.chromium.org/2265873002/diff/400001/net/url_request/url_re... File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/2265873002/diff/400001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:165: EXPECT_EQ(OK, delegate.request_status()); "EXPECT_THAT(delegate.request_status(), IsOk())" (And include net/test/gtest_util.h). https://codereview.chromium.org/2265873002/diff/400001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:225: EXPECT_EQ(ERR_FAILED, delegate.request_status()); Much like the OK checks, this should be: "EXPECT_THAT(delegate.request_status(), IsError(ERR_FAILED))" (And include net/test/gtest_util.h). https://codereview.chromium.org/2265873002/diff/400001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:817: base::RunLoop().RunUntilIdle(); Can we still check that delegate.request_status() is ERR_IO_PENDING? That's what it's initialized to, right? https://codereview.chromium.org/2265873002/diff/400001/net/url_request/url_re... File net/url_request/url_request_simple_job_unittest.cc (right): https://codereview.chromium.org/2265873002/diff/400001/net/url_request/url_re... net/url_request/url_request_simple_job_unittest.cc:246: EXPECT_EQ(1, cancel_delegate.response_started_count()); Why can't we check if the request was cancelled? cancel_delegate subclasses TestDelegate, so it should have its status set, right?
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
Patchset #5 (id:420001) has been deleted
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2265873002/diff/400001/net/cert_net/cert_net_... File net/cert_net/cert_net_fetcher_impl.cc (right): https://codereview.chromium.org/2265873002/diff/400001/net/cert_net/cert_net_... net/cert_net/cert_net_fetcher_impl.cc:306: base::Bind(&Job::OnTimeout, base::Unretained(this), ERR_TIMED_OUT)); On 2016/09/01 16:57:25, mmenke wrote: > Can get rid of OnTimeout entirely and just use FailRequest here instead. Done. https://codereview.chromium.org/2265873002/diff/400001/net/url_request/sdch_d... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/2265873002/diff/400001/net/url_request/sdch_d... net/url_request/sdch_dictionary_fetcher.cc:37: int rv = bytes_read; On 2016/09/01 16:57:25, mmenke wrote: > nit: Let's just rename bytes_read to "rv" instead. Done. https://codereview.chromium.org/2265873002/diff/400001/net/url_request/url_fe... File net/url_request/url_fetcher_core.cc (right): https://codereview.chromium.org/2265873002/diff/400001/net/url_request/url_fe... net/url_request/url_fetcher_core.cc:471: } while (bytes_read > 0); On 2016/09/01 16:57:25, mmenke wrote: > This final while is now redundant with the if at the start of this loop. Just > remove them both and use: > > while (bytes_read > 0) {...} Done. https://codereview.chromium.org/2265873002/diff/400001/net/url_request/url_re... File net/url_request/url_request_file_dir_job_unittest.cc (right): https://codereview.chromium.org/2265873002/diff/400001/net/url_request/url_re... net/url_request/url_request_file_dir_job_unittest.cc:133: EXPECT_THAT(bytes_read, ERR_FILE_NOT_FOUND); On 2016/09/01 16:57:25, mmenke wrote: > This should be "EXPECT_THAT(bytes_read, IsError(ERR_FILE_NOTFOUND));" > > If that doesn't work (I think it should), then it should be > "EXPECT_EQ(ERR_FILE_NOT_FOUND, bytes_read);" Done. https://codereview.chromium.org/2265873002/diff/400001/net/url_request/url_re... File net/url_request/url_request_ftp_job_unittest.cc (right): https://codereview.chromium.org/2265873002/diff/400001/net/url_request/url_re... net/url_request/url_request_ftp_job_unittest.cc:312: EXPECT_EQ(OK, request_delegate.request_status()); On 2016/09/01 16:57:25, mmenke wrote: > These should probably be "EXPECT_THAT(request_delegate.request_status(), > IsOk())" (And include net/test/gtest_util.h). Same goes for all instances of > EXPECT_EQ(OK, blah); in this CL. They print out more useful messages on > failure. Got it! https://codereview.chromium.org/2265873002/diff/400001/net/url_request/url_re... File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/2265873002/diff/400001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:165: EXPECT_EQ(OK, delegate.request_status()); On 2016/09/01 16:57:26, mmenke wrote: > "EXPECT_THAT(delegate.request_status(), IsOk())" (And include > net/test/gtest_util.h). Done. https://codereview.chromium.org/2265873002/diff/400001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:225: EXPECT_EQ(ERR_FAILED, delegate.request_status()); On 2016/09/01 16:57:25, mmenke wrote: > Much like the OK checks, this should be: > > "EXPECT_THAT(delegate.request_status(), IsError(ERR_FAILED))" > > (And include net/test/gtest_util.h). Done. https://codereview.chromium.org/2265873002/diff/400001/net/url_request/url_re... File net/url_request/url_request_simple_job_unittest.cc (right): https://codereview.chromium.org/2265873002/diff/400001/net/url_request/url_re... net/url_request/url_request_simple_job_unittest.cc:246: EXPECT_EQ(1, cancel_delegate.response_started_count()); On 2016/09/01 16:57:26, mmenke wrote: > Why can't we check if the request was cancelled? cancel_delegate subclasses > TestDelegate, so it should have its status set, right? Well, cancel_delegate overrides the OnReadCompleted method and has a EXPECT_EQ(ERR_ABORTED, bytes_read) there. But to make in more obvious, I've removed this method from this subclass and check the status normally, because TestDelegate::OnReadCompleted will now set the status.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM!
The CQ bit was checked by maksim.sisov@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Adjust callers and networking delegates in net/ to modified APIs Use modified Read and delegate methods from the following CL- https://codereview.chromium.org/2262653003/ BUG=423484 ========== to ========== Adjust callers and networking delegates in net/ to modified APIs Use modified Read and delegate methods from the following CL- https://codereview.chromium.org/2262653003/ BUG=423484 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:460001)
Message was sent while issue was closed.
Description was changed from ========== Adjust callers and networking delegates in net/ to modified APIs Use modified Read and delegate methods from the following CL- https://codereview.chromium.org/2262653003/ BUG=423484 ========== to ========== Adjust callers and networking delegates in net/ to modified APIs Use modified Read and delegate methods from the following CL- https://codereview.chromium.org/2262653003/ BUG=423484 Committed: https://crrev.com/65db2c48753ba5a516e9a4147f93a9c218d5f76e Cr-Commit-Position: refs/heads/master@{#416511} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/65db2c48753ba5a516e9a4147f93a9c218d5f76e Cr-Commit-Position: refs/heads/master@{#416511}
Message was sent while issue was closed.
On 2016/09/05 07:19:57, commit-bot: I haz the power wrote: > Patchset 6 (id:??) landed as > https://crrev.com/65db2c48753ba5a516e9a4147f93a9c218d5f76e > Cr-Commit-Position: refs/heads/master@{#416511} It seemed that this patch made below bot flaky https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20Mar...
Message was sent while issue was closed.
On 2016/09/06 18:19:32, michaelbai wrote: > On 2016/09/05 07:19:57, commit-bot: I haz the power wrote: > > Patchset 6 (id:??) landed as > > https://crrev.com/65db2c48753ba5a516e9a4147f93a9c218d5f76e > > Cr-Commit-Position: refs/heads/master@{#416511} > > It seemed that this patch made below bot flaky > > https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20Mar... That's unfortunate. The time range is certainly right. maksims: Do any of the other CLs you've landed depend on this one, so we can we just revert it without breaking stuff? Also, do you have an Android device to debug on? I'm happy to help you set one up, if you have trouble. It also requires Linux to compile Chrome for Android.
Message was sent while issue was closed.
On 2016/09/06 18:27:30, mmenke wrote: > On 2016/09/06 18:19:32, michaelbai wrote: > > On 2016/09/05 07:19:57, commit-bot: I haz the power wrote: > > > Patchset 6 (id:??) landed as > > > https://crrev.com/65db2c48753ba5a516e9a4147f93a9c218d5f76e > > > Cr-Commit-Position: refs/heads/master@{#416511} > > > > It seemed that this patch made below bot flaky > > > > > https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20Mar... > > That's unfortunate. The time range is certainly right. > > maksims: Do any of the other CLs you've landed depend on this one, so we can we > just revert it without breaking stuff? > > Also, do you have an Android device to debug on? I'm happy to help you set one > up, if you have trouble. It also requires Linux to compile Chrome for Android. I'm going to go ahead and create a revert, and CQ it.
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:460001) has been created in https://codereview.chromium.org/2313013002/ by mmenke@chromium.org. The reason for reverting is: This appears to have made Android bots flaky..
Message was sent while issue was closed.
On 2016/09/06 18:27:30, mmenke wrote: > On 2016/09/06 18:19:32, michaelbai wrote: > > On 2016/09/05 07:19:57, commit-bot: I haz the power wrote: > > > Patchset 6 (id:??) landed as > > > https://crrev.com/65db2c48753ba5a516e9a4147f93a9c218d5f76e > > > Cr-Commit-Position: refs/heads/master@{#416511} > > > > It seemed that this patch made below bot flaky > > > > > https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20Mar... > > That's unfortunate. The time range is certainly right. > > maksims: Do any of the other CLs you've landed depend on this one, so we can we > just revert it without breaking stuff? > > Also, do you have an Android device to debug on? I'm happy to help you set one > up, if you have trouble. It also requires Linux to compile Chrome for Android. If I remember right, nothing depends on this. Yes, I have an Android device. |
