Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2903383003:
Get network resource usage from ChromeResourceDispatcherHostDelegate (Closed)
|
Created:
3 years, 7 months ago by Pete Williamson Modified:
3 years, 6 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, dewittj+watch_chromium.org, tburkard+watch_chromium.org, fgorski+watch_chromium.org, romax+watch_chromium.org, Randy Smith (Not in Mondays), petewil+watch_chromium.org, gavinp+prer_chromium.org, chili+watch_chromium.org, loading-reviews_chromium.org, mmenke, dimich+watch_chromium.org, bmcquade1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionGet network resource usage from ChromeResourceDispatcherHostDelegate
BUG=699313
Review-Url: https://codereview.chromium.org/2903383003
Cr-Commit-Position: refs/heads/master@{#481606}
Committed: https://chromium.googlesource.com/chromium/src/+/d277ad3a4263f0a2446af6f3afa1301e4f73618b
Patch Set 1 #Patch Set 2 : Reworked design to minimize dependencies. #Patch Set 3 : Fix build warning #Patch Set 4 : Fix another build warning #Patch Set 5 : Merge with headers #Patch Set 6 : Get rid of file accidentally introduced in the merge #Patch Set 7 : Move io_data definition back outside the offline pages enabling ifdef. #Patch Set 8 : Fix build for offline pages disabled. #
Total comments: 2
Patch Set 9 : CR feedback per Chili #
Total comments: 8
Patch Set 10 : CR feedback per CSHarrison #Patch Set 11 : CR feedback per CSHarrison 2 #
Total comments: 6
Patch Set 12 : CR feedback per FGorski #
Messages
Total messages: 47 (36 generated)
The CQ bit was checked by petewil@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: This issue passed the CQ dry run.
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
The CQ bit was unchecked by petewil@chromium.org
The CQ bit was checked by petewil@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_tsan_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 petewil@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: 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_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by petewil@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by petewil@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: 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 petewil@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_tsan_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 petewil@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: This issue passed the CQ dry run.
petewil@chromium.org changed reviewers: + chili@chromium.org, csharrison@chromium.org
The second half of the resource signal percentage work is now ready for review. Design doc is here:https://docs.google.com/document/d/1dxGOMsUkmxfoNUfQKqZRi4qAhWVGh9DZDiFI1arkiJ0
looks good! small nit https://codereview.chromium.org/2903383003/diff/140001/chrome/browser/loader/... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2903383003/diff/140001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:434: if (was_cached) I think you want... if !was_cached
CR feedback per Chili https://codereview.chromium.org/2903383003/diff/140001/chrome/browser/loader/... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2903383003/diff/140001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:434: if (was_cached) On 2017/06/21 17:18:18, chili wrote: > I think you want... if !was_cached Done.
c/b/l generally looks good https://codereview.chromium.org/2903383003/diff/160001/chrome/browser/loader/... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2903383003/diff/160001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:373: const content::GlobalRequestID& request_id, nit: Remove request_id https://codereview.chromium.org/2903383003/diff/160001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:375: bool is_download, nit: remove is_download https://codereview.chromium.org/2903383003/diff/160001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:376: base::TimeTicks request_creation_time) { nit: remove request_creation_time https://codereview.chromium.org/2903383003/diff/160001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:377: content::WebContents* web_contents = web_contents_getter.Run(); DCHECK_CURRENTLY_ON(BrowserTHread::UI)
https://codereview.chromium.org/2903383003/diff/160001/chrome/browser/loader/... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2903383003/diff/160001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:373: const content::GlobalRequestID& request_id, On 2017/06/21 18:06:01, Charlie Harrison wrote: > nit: Remove request_id Done. https://codereview.chromium.org/2903383003/diff/160001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:375: bool is_download, On 2017/06/21 18:06:01, Charlie Harrison wrote: > nit: remove is_download Done. https://codereview.chromium.org/2903383003/diff/160001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:376: base::TimeTicks request_creation_time) { On 2017/06/21 18:06:01, Charlie Harrison wrote: > nit: remove request_creation_time Done. https://codereview.chromium.org/2903383003/diff/160001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:377: content::WebContents* web_contents = web_contents_getter.Run(); On 2017/06/21 18:06:01, Charlie Harrison wrote: > DCHECK_CURRENTLY_ON(BrowserTHread::UI) Done.
lgtm
LGTM
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
drive by nits. lgtm otherwise. https://codereview.chromium.org/2903383003/diff/200001/chrome/browser/loader/... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2903383003/diff/200001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:362: case (content::RESOURCE_TYPE_STYLESHEET): nit: () not necessary. https://codereview.chromium.org/2903383003/diff/200001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:364: case (content::RESOURCE_TYPE_IMAGE): nit: same https://codereview.chromium.org/2903383003/diff/200001/chrome/browser/offline... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2903383003/diff/200001/chrome/browser/offline... chrome/browser/offline_pages/background_loader_offliner.cc:362: signal_data_.SetDouble("StartedImages", started_count_); why setting both if only one changed?
https://codereview.chromium.org/2903383003/diff/200001/chrome/browser/loader/... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2903383003/diff/200001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:362: case (content::RESOURCE_TYPE_STYLESHEET): On 2017/06/22 16:52:38, fgorski wrote: > nit: () not necessary. Done. https://codereview.chromium.org/2903383003/diff/200001/chrome/browser/loader/... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:364: case (content::RESOURCE_TYPE_IMAGE): On 2017/06/22 16:52:38, fgorski wrote: > nit: same Done. https://codereview.chromium.org/2903383003/diff/200001/chrome/browser/offline... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2903383003/diff/200001/chrome/browser/offline... chrome/browser/offline_pages/background_loader_offliner.cc:362: signal_data_.SetDouble("StartedImages", started_count_); On 2017/06/22 16:52:38, fgorski wrote: > why setting both if only one changed? Done.
The CQ bit was checked by petewil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from csharrison@chromium.org, chili@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2903383003/#ps220001 (title: "CR feedback per FGorski")
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": 220001, "attempt_start_ts": 1498151687948760,
"parent_rev": "af44ef8e33d3d20a6ebbaceb728d7df6340245f4", "commit_rev":
"d277ad3a4263f0a2446af6f3afa1301e4f73618b"}
Message was sent while issue was closed.
Description was changed from ========== Get network resource usage from ChromeResourceDispatcherHostDelegate BUG=699313 ========== to ========== Get network resource usage from ChromeResourceDispatcherHostDelegate BUG=699313 Review-Url: https://codereview.chromium.org/2903383003 Cr-Commit-Position: refs/heads/master@{#481606} Committed: https://chromium.googlesource.com/chromium/src/+/d277ad3a4263f0a2446af6f3afa1... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/d277ad3a4263f0a2446af6f3afa1... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
