Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2699433003:
Add net-log entries when HttpStream jobs are forced to wait, and when they're delayed and resumed. (Closed)
|
Created:
3 years, 10 months ago by Ryan Hamilton Modified:
3 years, 10 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, bnc+watch_chromium.org, eroman, mmenke, net-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd net-log entries when HttpStream jobs are forced to wait, and when they're delayed and resumed.
BUG=691688
Review-Url: https://codereview.chromium.org/2699433003
Cr-Commit-Position: refs/heads/master@{#450608}
Committed: https://chromium.googlesource.com/chromium/src/+/d27cf4ed111ab7f3bdab607fe48e4446d6cf15ca
Patch Set 1 #
Total comments: 9
Patch Set 2 : fix comments from eroman #
Total comments: 2
Messages
Total messages: 23 (11 generated)
rch@chromium.org changed reviewers: + zhongyi@chromium.org
The CQ bit was checked by rch@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...
eroman@chromium.org changed reviewers: + eroman@chromium.org
drive-by comments on NetLog integration specifically. https://codereview.chromium.org/2699433003/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/2699433003/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job.cc:138: std::unique_ptr<base::Value> NetLogHttpStreamJobWaitingCallback( Have you considered using NetLog::BoolCallback() ? (can save writing some boiler plate for simple cases like this). (https://cs.chromium.org/chromium/src/net/log/net_log.h?q=net_log.h&dr=CSs&l=172) https://codereview.chromium.org/2699433003/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2699433003/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job_controller.cc:36: dict->SetInteger("delay_ms", static_cast<int>(delay.InMilliseconds())); Optional: replace this funciton with NetLog::Int64Callback() or NetLog::IntCallback() ? https://codereview.chromium.org/2699433003/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job_controller.cc:555: base::Bind(&HttpStreamFactoryImpl::JobController::ResumeMainJob, FTR I didn't review this in detail (i.e. wether there are sharp edges between this async resumption). Will leave that to main reviewer... https://codereview.chromium.org/2699433003/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/2699433003/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job_controller.h:234: void ResumeMainJobLater(const base::TimeDelta& delay); Add comments? https://codereview.chromium.org/2699433003/diff/1/net/log/net_log_event_type_... File net/log/net_log_event_type_list.h (right): https://codereview.chromium.org/2699433003/diff/1/net/log/net_log_event_type_... net/log/net_log_event_type_list.h:1059: // another job. Please mention the parameter "should_wait"
Thanks, Eric. https://codereview.chromium.org/2699433003/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/2699433003/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job.cc:138: std::unique_ptr<base::Value> NetLogHttpStreamJobWaitingCallback( On 2017/02/14 21:18:36, eroman wrote: > Have you considered using NetLog::BoolCallback() ? (can save writing some boiler > plate for simple cases like this). > (https://cs.chromium.org/chromium/src/net/log/net_log.h?q=net_log.h&dr=CSs&l=172) Oh nice! I didn't know that existed. Thanks! https://codereview.chromium.org/2699433003/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2699433003/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job_controller.cc:555: base::Bind(&HttpStreamFactoryImpl::JobController::ResumeMainJob, On 2017/02/14 21:18:36, eroman wrote: > FTR I didn't review this in detail (i.e. wether there are sharp edges between > this async resumption). Will leave that to main reviewer... *nod* I don't believe I've actually changed any behavior, just pushed code around. But zhongyi will confirm. https://codereview.chromium.org/2699433003/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/2699433003/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job_controller.h:234: void ResumeMainJobLater(const base::TimeDelta& delay); On 2017/02/14 21:18:36, eroman wrote: > Add comments? Done. https://codereview.chromium.org/2699433003/diff/1/net/log/net_log_event_type_... File net/log/net_log_event_type_list.h (right): https://codereview.chromium.org/2699433003/diff/1/net/log/net_log_event_type_... net/log/net_log_event_type_list.h:1059: // another job. On 2017/02/14 21:18:36, eroman wrote: > Please mention the parameter "should_wait" Done. (Dangit! Initially this didn't have any params but then I added one and didn't update the comment.) Thanks!
lgtm https://codereview.chromium.org/2699433003/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2699433003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:543: NetLog::Int64Callback("delay", delay.InMilliseconds())); I guess the downside to my recommendation here is we do repeat "delay" twice. But I am OK with that in this case if you are :)
lgtm! Thanks for doing this!
https://codereview.chromium.org/2699433003/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2699433003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:543: NetLog::Int64Callback("delay", delay.InMilliseconds())); On 2017/02/14 21:46:36, eroman wrote: > I guess the downside to my recommendation here is we do repeat "delay" twice. > But I am OK with that in this case if you are :) Yeah, I'm totally fine with that 'cause it's two different event types in the two different places so while it's repeating the same string literal, they are not semantically required to be identical. So I'm happy :)
The CQ bit was checked by rch@chromium.org
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
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by rch@chromium.org
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
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by rch@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": 20001, "attempt_start_ts": 1487133176510820,
"parent_rev": "2e2b33af5466a930f5201400ba2e487c64c620ca", "commit_rev":
"d27cf4ed111ab7f3bdab607fe48e4446d6cf15ca"}
Message was sent while issue was closed.
Description was changed from ========== Add net-log entries when HttpStream jobs are forced to wait, and when they're delayed and resumed. BUG=691688 ========== to ========== Add net-log entries when HttpStream jobs are forced to wait, and when they're delayed and resumed. BUG=691688 Review-Url: https://codereview.chromium.org/2699433003 Cr-Commit-Position: refs/heads/master@{#450608} Committed: https://chromium.googlesource.com/chromium/src/+/d27cf4ed111ab7f3bdab607fe48e... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/d27cf4ed111ab7f3bdab607fe48e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
