Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1921613003:
Count successes of link rel=prefetch (Closed)
|
Created:
4 years, 8 months ago by jkarlin Modified:
4 years, 8 months ago CC:
chromium-reviews, loading-reviews_chromium.org, darin-cc_chromium.org, cbentzel+watch_chromium.org, jam, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCount successes of link rel=prefetch
Add metrics to count the number of bytes for requests fulfilled by the
cache that were filled by earlier prefetches. Also measures how long
those particular requests take (to compare against time taken for
prefetch network requests).
BUG=606366
Committed: https://crrev.com/cc6943dbb0984ae198fa36706126d42a87a65d7b
Cr-Commit-Position: refs/heads/master@{#389534}
Patch Set 1 #Patch Set 2 : Nit #
Total comments: 5
Patch Set 3 : Remove braces #
Messages
Total messages: 25 (10 generated)
jkarlin@chromium.org changed reviewers: + gavinp@chromium.org, isherman@chromium.org
gavinp@ PTAL at everything isherman@ PTAL at histograms.xml Thanks!
histograms lgtm
Looks good! https://codereview.chromium.org/1921613003/diff/20001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/1921613003/diff/20001/content/browser/loader/... content/browser/loader/resource_loader.cc:722: TimeDelta total_time = base::TimeTicks::Now() - request_->creation_time(); Since this is defined on both sides of the if(), should it be moved up above it? https://codereview.chromium.org/1921613003/diff/20001/net/url_request/url_req... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/1921613003/diff/20001/net/url_request/url_req... net/url_request/url_request_http_job.cc:1625: } Nit: I don't believe you need these braces.
Thanks for looking. https://codereview.chromium.org/1921613003/diff/20001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/1921613003/diff/20001/content/browser/loader/... content/browser/loader/resource_loader.cc:722: TimeDelta total_time = base::TimeTicks::Now() - request_->creation_time(); On 2016/04/25 15:39:05, gavinp wrote: > Since this is defined on both sides of the if(), should it be moved up above it? Either case is likely to be rare (if prefetch or unused_since_prefetch) so it seemed better to wait until necessary. I could go either way, let me know which you prefer. https://codereview.chromium.org/1921613003/diff/20001/net/url_request/url_req... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/1921613003/diff/20001/net/url_request/url_req... net/url_request/url_request_http_job.cc:1625: } On 2016/04/25 15:39:05, gavinp wrote: > Nit: I don't believe you need these braces. Done.
lgtm. https://codereview.chromium.org/1921613003/diff/20001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/1921613003/diff/20001/content/browser/loader/... content/browser/loader/resource_loader.cc:722: TimeDelta total_time = base::TimeTicks::Now() - request_->creation_time(); On 2016/04/25 15:46:14, jkarlin wrote: > On 2016/04/25 15:39:05, gavinp wrote: > > Since this is defined on both sides of the if(), should it be moved up above > it? > > Either case is likely to be rare (if prefetch or unused_since_prefetch) so it > seemed better to wait until necessary. I could go either way, let me know which > you prefer. Oh, you're right. I hadn't paid enough attention to see that it was an "else if". Yes, duplicate it in each section.
The CQ bit was checked by jkarlin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1921613003/#ps40001 (title: "Remove braces")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1921613003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1921613003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
jkarlin@chromium.org changed reviewers: + mmenke@chromium.org
mmenke@chromium.org: Please review changes in content/browser/loader for OWNERS Thanks!
On 2016/04/25 16:44:14, jkarlin wrote: > mailto:mmenke@chromium.org: Please review changes in content/browser/loader for OWNERS > > Thanks! LGTM, rubberstamp
The CQ bit was checked by jkarlin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1921613003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1921613003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to commit the patch.
The CQ bit was unchecked by commit-bot@chromium.org
Everything is green, can't figure out why the commit failed. Retrying.
The CQ bit was checked by jkarlin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1921613003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1921613003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Count successes of link rel=prefetch Add metrics to count the number of bytes for requests fulfilled by the cache that were filled by earlier prefetches. Also measures how long those particular requests take (to compare against time taken for prefetch network requests). BUG=606366 ========== to ========== Count successes of link rel=prefetch Add metrics to count the number of bytes for requests fulfilled by the cache that were filled by earlier prefetches. Also measures how long those particular requests take (to compare against time taken for prefetch network requests). BUG=606366 Committed: https://crrev.com/cc6943dbb0984ae198fa36706126d42a87a65d7b Cr-Commit-Position: refs/heads/master@{#389534} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/cc6943dbb0984ae198fa36706126d42a87a65d7b Cr-Commit-Position: refs/heads/master@{#389534} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
