Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2794963002:
Check callback is run in QuicStreamFactory::Job (Closed)
|
Created:
3 years, 8 months ago by xunjieli Modified:
3 years, 8 months ago Reviewers:
Zhongyi Shi CC:
chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCheck callback is run in QuicStreamFactory::Job
This CL adds a DCHECK in QuicStreamFactory::Job's destructor to make sure
|callback_| has been run.
This CL clears two related classes's callback instances so we only run
them once.
BUG=704949
Review-Url: https://codereview.chromium.org/2794963002
Cr-Commit-Position: refs/heads/master@{#461753}
Committed: https://chromium.googlesource.com/chromium/src/+/53b0eeafd14a0a5699ff8850df41ddef696caddc
Patch Set 1 #Patch Set 2 : fix tests #
Total comments: 8
Patch Set 3 : Address comment #
Messages
Total messages: 28 (22 generated)
Description was changed from ========== Check callback is run in QuicStreamFactory::Job This CL adds a DCHECK in QuicStreamFactory::Job's destructor to make sure |callback_| has been run. This CL clears |callback_| in two related classes so we only run |callback_| once. BUG=700617 ========== to ========== Check callback is run in QuicStreamFactory::Job This CL adds a DCHECK in QuicStreamFactory::Job's destructor to make sure |callback_| has been run. This CL clears |callback_| in two related classes so we only run |callback_| once. BUG=700617 ==========
Description was changed from ========== Check callback is run in QuicStreamFactory::Job This CL adds a DCHECK in QuicStreamFactory::Job's destructor to make sure |callback_| has been run. This CL clears |callback_| in two related classes so we only run |callback_| once. BUG=700617 ========== to ========== Check callback is run in QuicStreamFactory::Job This CL adds a DCHECK in QuicStreamFactory::Job's destructor to make sure |callback_| has been run. This CL clears two related classes's callback instances so we only run them once. BUG=700617 ==========
The CQ bit was checked by xunjieli@chromium.org 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 xunjieli@chromium.org to run a CQ dry run
xunjieli@chromium.org changed reviewers: + zhongyi@chromium.org
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.
Looks good, just some nits. Thanks for clean this up! https://codereview.chromium.org/2794963002/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2794963002/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:957: session_deps_.host_resolver->ResolveAllPending(); nit: could you add EXPECT_FALSE(job_controller_->alternative_job()) after host resolver being unblocked to verify that alt job falis and is cleaned up? https://codereview.chromium.org/2794963002/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:1019: EXPECT_EQ(1u, test_task_runner->GetPendingTaskCount()); ditto https://codereview.chromium.org/2794963002/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:1071: session_deps_.host_resolver->ResolveAllPending(); ditto https://codereview.chromium.org/2794963002/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:1207: test_task_runner->FastForwardUntilNoTasksRemain(); ditto
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Thanks, PTAL. https://codereview.chromium.org/2794963002/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2794963002/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:957: session_deps_.host_resolver->ResolveAllPending(); On 2017/04/03 22:55:39, Zhongyi Shi wrote: > nit: could you add EXPECT_FALSE(job_controller_->alternative_job()) after host > resolver being unblocked to verify that alt job falis and is cleaned up? Done. https://codereview.chromium.org/2794963002/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:1019: EXPECT_EQ(1u, test_task_runner->GetPendingTaskCount()); On 2017/04/03 22:55:39, Zhongyi Shi wrote: > ditto Done. https://codereview.chromium.org/2794963002/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:1071: session_deps_.host_resolver->ResolveAllPending(); On 2017/04/03 22:55:39, Zhongyi Shi wrote: > ditto Done. https://codereview.chromium.org/2794963002/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:1207: test_task_runner->FastForwardUntilNoTasksRemain(); On 2017/04/03 22:55:39, Zhongyi Shi wrote: > ditto Done.
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.
lgtm! Thanks!
The CQ bit was checked by xunjieli@chromium.org
The CQ bit was unchecked by xunjieli@chromium.org
Description was changed from ========== Check callback is run in QuicStreamFactory::Job This CL adds a DCHECK in QuicStreamFactory::Job's destructor to make sure |callback_| has been run. This CL clears two related classes's callback instances so we only run them once. BUG=700617 ========== to ========== Check callback is run in QuicStreamFactory::Job This CL adds a DCHECK in QuicStreamFactory::Job's destructor to make sure |callback_| has been run. This CL clears two related classes's callback instances so we only run them once. BUG=704949 ==========
The CQ bit was checked by xunjieli@chromium.org
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": 60001, "attempt_start_ts": 1491324046162410,
"parent_rev": "87fbafb0be4bf8ccc0cd5fa2578527c0aa88b704", "commit_rev":
"53b0eeafd14a0a5699ff8850df41ddef696caddc"}
Message was sent while issue was closed.
Description was changed from ========== Check callback is run in QuicStreamFactory::Job This CL adds a DCHECK in QuicStreamFactory::Job's destructor to make sure |callback_| has been run. This CL clears two related classes's callback instances so we only run them once. BUG=704949 ========== to ========== Check callback is run in QuicStreamFactory::Job This CL adds a DCHECK in QuicStreamFactory::Job's destructor to make sure |callback_| has been run. This CL clears two related classes's callback instances so we only run them once. BUG=704949 Review-Url: https://codereview.chromium.org/2794963002 Cr-Commit-Position: refs/heads/master@{#461753} Committed: https://chromium.googlesource.com/chromium/src/+/53b0eeafd14a0a5699ff8850df41... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/53b0eeafd14a0a5699ff8850df41... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
