Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2798953002:
[PageLoadMetrics] Keep track of Ad Sizes on Pages (Closed)
|
Created:
3 years, 8 months ago by jkarlin Modified:
3 years, 7 months ago CC:
chromium-reviews, csharrison+watch_chromium.org, darin-cc_chromium.org, jam, loading-reviews+metrics_chromium.org, loading-reviews_chromium.org, mmenke, Randy Smith (Not in Mondays), speed-metrics-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[PageLoadMetrics] Keep track of Ad Sizes on Pages
In order to help us understand the resource footprint that ads have on
webpages, we need to measure things like their network and cache utilizations.
This CL adds a PageLoadObserver that keeps track of frames with ads and
reports statistics on the number of ad frames found on the page, the size of
the ad frames, and the percentage that came from cache vs network.
As part of the work, the following additional changes were necessary:
1) Frame Tree ID and URL are added to extra request info
2) Add GetRenderFrameHostGetterForRequest to ResourceRequestInfo so that
ChromeResourceDispatcherHostDelegate can get at the frame tree node id of
the resource.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
BUG=708570
Review-Url: https://codereview.chromium.org/2798953002
Cr-Commit-Position: refs/heads/master@{#468738}
Committed: https://chromium.googlesource.com/chromium/src/+/6616a885b4f18edd4f6aa178146c14538ca1d429
Patch Set 1 #Patch Set 2 : Comment nit #Patch Set 3 : Add histograms.xml #Patch Set 4 : Improved heuristic #Patch Set 5 : Fix existing tests #Patch Set 6 : Add tests #
Total comments: 4
Patch Set 7 : Rebase #Patch Set 8 : Updates and address Ojan's comments #
Total comments: 39
Patch Set 9 : SubFrame nav must be committed #
Total comments: 1
Patch Set 10 : Address comments from PS8 #
Total comments: 24
Patch Set 11 : Address comments from PS10 plus many improvements #
Total comments: 14
Patch Set 12 : Fix test framework #Patch Set 13 : Remove dcheck #
Total comments: 12
Patch Set 14 : Address comments from PS11-13 #
Total comments: 55
Patch Set 15 : Address comments from PS13 14 #Patch Set 16 : Painful Rebase #Patch Set 17 : Clean up test #
Total comments: 28
Patch Set 18 : Address comments from PS17 #
Total comments: 6
Patch Set 19 : Rebase #Patch Set 20 : Address comments from PS18 #
Total comments: 18
Patch Set 21 : Rebase #Patch Set 22 : Address comments from PS20 #
Total comments: 18
Patch Set 23 : Address comments from PS22 #
Total comments: 2
Patch Set 24 : Rebase #Depends on Patchset: Messages
Total messages: 172 (130 generated)
The CQ bit was checked by jkarlin@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 jkarlin@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_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 jkarlin@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by jkarlin@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_...)
Initial drive-by review... https://codereview.chromium.org/2798953002/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2798953002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:21: return base::StartsWith(name, "google_ads_iframe", People will stumble across this in public. Anywhere we special case google will get attention. Probably worth having a comment explaining why we're doing this (e.g. just for initial measurement, detecting other ad networks is hard because XYZ, etc.). https://codereview.chromium.org/2798953002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:107: void AdsPageLoadMetricsObserver::OnComplete( Do you also need to hook FlushMetricsOnAppEnterBackground?
The CQ bit was checked by jkarlin@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...)
The CQ bit was checked by jkarlin@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...
https://codereview.chromium.org/2798953002/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2798953002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:21: return base::StartsWith(name, "google_ads_iframe", On 2017/04/07 19:40:06, ojan wrote: > People will stumble across this in public. Anywhere we special case google will > get attention. Probably worth having a comment explaining why we're doing this > (e.g. just for initial measurement, detecting other ad networks is hard because > XYZ, etc.). Done. https://codereview.chromium.org/2798953002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:107: void AdsPageLoadMetricsObserver::OnComplete( On 2017/04/07 19:40:06, ojan wrote: > Do you also need to hook FlushMetricsOnAppEnterBackground? Good idea. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, build hasn't started yet, builder probably lacks capacity)
jkarlin@chromium.org changed reviewers: + csharrison@chromium.org
csharrison@ PTAL at everything, thanks!
Description was changed from ========== [PageLoadMetrics] Keep track of Ad Sizes on Pages This CL adds a PageLoadObserver that keeps track of frames with ads and them and reports their byte usage. BUG=708570 ========== to ========== [PageLoadMetrics] Keep track of Ad Sizes on Pages This CL adds a PageLoadObserver that keeps track of frames with ads and them and reports their byte usage. As part of the work, the following additional changes were necessary: 1) Frame Tree ID and URL are added to extra request info 2) Add GetRenderFrameHostGetterForRequest to ResourceRequestInfo so that ChromeResourceDispatcherHostDelegate can get at the frame tree node id of the resource. BUG=708570 ==========
Only looked at PLM non test parts. Just some initial comments. https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:277: committed_load_->CommitSubFrame(navigation_handle); DidCommitSubFrame? CommitSubFrame sounds like a directive. https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:14: int frame_tree_node_id = navigation_handle->GetFrameTreeNodeId(); #include navigation_handle https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:18: const std::string& name = current_frame_host->GetFrameName(); #include <string> https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:19: const std::string& url_spec = navigation_handle->GetURL().spec(); #include gurl https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:26: return base::StartsWith(name, "google_ads_iframe", #include base/strings/string_util.h https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:26: return base::StartsWith(name, "google_ads_iframe", Can you document that frame names can be very big, so additional code should be careful not to do full string scans. https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:30: base::StartsWith(url_spec, Replace these last two with: url.host_piece() == "tcp.googlesyndication.com" && url.path_piece() == "/safeframe/" Also include stringpiece. If you are really concerned about non-http/s you can use url.SchemeIsHttpOrHttps https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:65: if (ancestor_id != kInvalidFrameTreeNodeId) { optional: slightly prefer: if (ancestor_id != kInvalidFrameTreeNodeId) { ad_ancestors_[frame_tree_nod_id] = ancestor_id } else if (FrameIsAd(navigation_handle) { ad_frames_[frame_tree_node_id] = frame_tree_node_id; ad_ancestors_[frame_tree_nod_id] = frame_tree_node_id; } else { ad_ancestors_[frame_tree_nod_id] = KInvalidFrameTreeNodeId; } ProcessDelayedResources(frame_tree_node_id); return CONTINUE_OBSERVING; https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:97: if (!base::ContainsKey(ad_ancestors_, Lots of map lookups per resource request :( I wonder if we can consolidate and share a map for some of this data? I am nervous this will perform badly on sites with many frames (~50-100) https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:118: if (!extra_request_info.was_cached) nit: needs {} https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:138: for (const auto& frame_id_and_size : ad_frames_) { Is logging on the order of 100 histograms in a loop costly on the CPU? I remember seeing some histogram logging in CPU profiles at some point but not sure how expensive these particular macros are. https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h (right): https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:9: #include <unordered_set> not used https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:21: // page_load_metrics::PageLoadMetricsObserver implementation: just "// page_load_metrics::PageLoadMetricsObserver:" for consistency. https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:46: void ProcessDelayedResources(FrameTreeNodeId frame_tree_node_id); Please document this method. https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:48: std::unordered_map<FrameTreeNodeId, AdFrameData> ad_frames_; Haven't looked deeply but do you really need three maps? Can you consolidate? https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:49: std::unordered_map<FrameTreeNodeId, FrameTreeNodeId> ad_ancestors_; Can you document what the keys and values are? https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:50: std::unordered_map<FrameTreeNodeId, page_load_metrics::ExtraRequestInfo> Can you document what a "delayed" resource is? https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_observer.cc (right): https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_observer.cc:63: : url(url), I really dislike doing an extra URL copy here... Are GURLs still not move()-able? https://codereview.chromium.org/2798953002/diff/140001/content/public/browser... File content/public/browser/resource_request_info.h (right): https://codereview.chromium.org/2798953002/diff/140001/content/public/browser... content/public/browser/resource_request_info.h:83: using RenderFrameHostGetter = base::Callback<RenderFrameHost*(void)>; Did you ask //content owners that this approach is OK?
The CQ bit was checked by jkarlin@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
bmcquade@chromium.org changed reviewers: + bmcquade@chromium.org
seems good to me - just one suggestion around policy for tracking sub frame commits. thanks! https://codereview.chromium.org/2798953002/diff/160001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/2798953002/diff/160001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:277: committed_load_->CommitSubFrame(navigation_handle); we should probably ignore same frame navs and perhaps navs that failed to commit here? if (navigation_handle->HasCommitted() && !nav_handle->IsSameDocument()) we might also consider checking ShouldTrackNavigation (and removing the IsMainFrame dcheck from that method) so we only track commits with the same properties as main frame navs (e.g. only http/https urls, only non-error pages, only html or xhtml, etc.
The CQ bit was checked by jkarlin@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...
Address comments from PS8. PTAL. Now digging into why plznavigate tests are failing. https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:277: committed_load_->CommitSubFrame(navigation_handle); On 2017/04/10 18:39:41, Charlie Harrison wrote: > DidCommitSubFrame? CommitSubFrame sounds like a directive. Done. https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:14: int frame_tree_node_id = navigation_handle->GetFrameTreeNodeId(); On 2017/04/10 18:39:41, Charlie Harrison wrote: > #include navigation_handle Done. https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:18: const std::string& name = current_frame_host->GetFrameName(); On 2017/04/10 18:39:41, Charlie Harrison wrote: > #include <string> Done. https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:19: const std::string& url_spec = navigation_handle->GetURL().spec(); On 2017/04/10 18:39:41, Charlie Harrison wrote: > #include gurl Done. https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:26: return base::StartsWith(name, "google_ads_iframe", On 2017/04/10 18:39:41, Charlie Harrison wrote: > #include base/strings/string_util.h Done. https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:26: return base::StartsWith(name, "google_ads_iframe", On 2017/04/10 18:39:41, Charlie Harrison wrote: > Can you document that frame names can be very big, so additional code should be > careful not to do full string scans. Done. https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:30: base::StartsWith(url_spec, On 2017/04/10 18:39:41, Charlie Harrison wrote: > Replace these last two with: > url.host_piece() == "tcp.googlesyndication.com" && url.path_piece() == > "/safeframe/" > > Also include stringpiece. If you are really concerned about non-http/s you can > use url.SchemeIsHttpOrHttps Done. https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:65: if (ancestor_id != kInvalidFrameTreeNodeId) { On 2017/04/10 18:39:41, Charlie Harrison wrote: > optional: slightly prefer: > > if (ancestor_id != kInvalidFrameTreeNodeId) { > ad_ancestors_[frame_tree_nod_id] = ancestor_id > } else if (FrameIsAd(navigation_handle) { > ad_frames_[frame_tree_node_id] = frame_tree_node_id; > ad_ancestors_[frame_tree_nod_id] = frame_tree_node_id; > } else { > ad_ancestors_[frame_tree_nod_id] = KInvalidFrameTreeNodeId; > } > ProcessDelayedResources(frame_tree_node_id); > return CONTINUE_OBSERVING; Done. https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:97: if (!base::ContainsKey(ad_ancestors_, On 2017/04/10 18:39:41, Charlie Harrison wrote: > Lots of map lookups per resource request :( > > I wonder if we can consolidate and share a map for some of this data? I am > nervous this will perform badly on sites with many frames (~50-100) Done. https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:118: if (!extra_request_info.was_cached) On 2017/04/10 18:39:41, Charlie Harrison wrote: > nit: needs {} Done. https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:138: for (const auto& frame_id_and_size : ad_frames_) { On 2017/04/10 18:39:41, Charlie Harrison wrote: > Is logging on the order of 100 histograms in a loop costly on the CPU? I > remember seeing some histogram logging in CPU profiles at some point but not > sure how expensive these particular macros are. https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... "In general, the histogram code is highly optimized. Do not be concerned about the processing cost of emitting to a histogram (unless you're using sparse histograms)." https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h (right): https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:9: #include <unordered_set> On 2017/04/10 18:39:42, Charlie Harrison wrote: > not used Done. https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:21: // page_load_metrics::PageLoadMetricsObserver implementation: On 2017/04/10 18:39:41, Charlie Harrison wrote: > just "// page_load_metrics::PageLoadMetricsObserver:" for consistency. Done. https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:46: void ProcessDelayedResources(FrameTreeNodeId frame_tree_node_id); On 2017/04/10 18:39:41, Charlie Harrison wrote: > Please document this method. Done. https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:48: std::unordered_map<FrameTreeNodeId, AdFrameData> ad_frames_; On 2017/04/10 18:39:42, Charlie Harrison wrote: > Haven't looked deeply but do you really need three maps? Can you consolidate? A bit. ad_ancestors_ is now merged into ad_frames_, plus a new list to hold the data. https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:49: std::unordered_map<FrameTreeNodeId, FrameTreeNodeId> ad_ancestors_; On 2017/04/10 18:39:41, Charlie Harrison wrote: > Can you document what the keys and values are? Done. https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:50: std::unordered_map<FrameTreeNodeId, page_load_metrics::ExtraRequestInfo> On 2017/04/10 18:39:42, Charlie Harrison wrote: > Can you document what a "delayed" resource is? Done. https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_observer.cc (right): https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_observer.cc:63: : url(url), On 2017/04/10 18:39:42, Charlie Harrison wrote: > I really dislike doing an extra URL copy here... Are GURLs still not > move()-able? Done. https://codereview.chromium.org/2798953002/diff/140001/content/public/browser... File content/public/browser/resource_request_info.h (right): https://codereview.chromium.org/2798953002/diff/140001/content/public/browser... content/public/browser/resource_request_info.h:83: using RenderFrameHostGetter = base::Callback<RenderFrameHost*(void)>; On 2017/04/10 18:39:42, Charlie Harrison wrote: > Did you ask //content owners that this approach is OK? Not yet. Was planning on doing that after you took a look at the rest of it.
Given the subtleties of the code wrt navigation, I think it might warrant browser tests as well. https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:55: return STOP_OBSERVING; Can you do this feature detection at construction time? https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h (right): https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:53: std::list<AdFrameData> ad_frames_data_storage_; It is append-only. Why not use std::vector? https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:55: // Maps a frame (by id) to the AdFraneData responsible for the frame. The s/AdFraneData/AdFrameData https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:15: using content::RenderFrameHostTester; include test_renderer_host.h https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:17: namespace { nit: namespace { enum class ResourceCached { NOT_CACHED, CACHED }; } // namespace https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:39: return subframe; I wonder if you could use the NavigationSimulator here and above, which will ensure the code is correct for OOPIF / transfer navigations if you navigate cross-site. This is newish territory for the PLM test code but I think it might be needed now that we care about subframes. https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:46: GURL(), frame->GetFrameTreeNodeId(), include gurl https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:54: void RegisterObservers(page_load_metrics::PageLoadTracker* tracker) override { include page_load_tracker.h https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:55: tracker->AddObserver(base::WrapUnique(new AdsPageLoadMetricsObserver())); nit: prefer base::MakeUnique<AdsPageLoadMetricsObserver>() https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:59: DISALLOW_COPY_AND_ASSIGN(AdsPageLoadMetricsObserverTest); include base/macros. https://codereview.chromium.org/2798953002/diff/180001/url/gurl.h File url/gurl.h (right): https://codereview.chromium.org/2798953002/diff/180001/url/gurl.h#newcode55 url/gurl.h:55: // to reallocating the srtcing. It does not re-parse. nit: typo
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_...)
https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:261: std::move(url), frame_tree_node_id, was_cached, raw_body_bytes, we chatted about this & I think you concluded that we'd remove the std::move(), but just to document our reasoning, we concluded: calling std::move() on a const object doesn't end up avoiding a copy, as the |const GURL&&| resulting from the std::move() can't be passed to the GURL move constructor, which takes a non-const |GURL&&| (move constructors take non-const rvalues since the rvalue is being modified). Instead, IIUC, the |const GURL&&| is implicitly converted to a const GURL& and the GURL copy constructor gets invoked.
The CQ bit was checked by jkarlin@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 checked by jkarlin@chromium.org to run a CQ dry run
Patchset #11 (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...
Thanks! PTAL I'm not sure about the need for browser tests. I think the unit tests have good coverage. Would it be okay to add browser tests in a followup? https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_observer.cc (right): https://codereview.chromium.org/2798953002/diff/140001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_observer.cc:63: : url(url), On 2017/04/11 16:46:28, jkarlin wrote: > On 2017/04/10 18:39:42, Charlie Harrison wrote: > > I really dislike doing an extra URL copy here... Are GURLs still not > > move()-able? > > Done. Undone. It added too much complexity. For one thing the caller was taking a const GURL& and std::moving it (a mistake on my part), which results in a copy and no compiler warning. Next, the top of the call stack, NotifyUIThreadOfRequestComplete, takes the url as a const GURL& from Bind. std::moving that GURL results in a copy anyway. So we don't actually reduce the number of copies and then everyone has to be more careful about using the object after it has been std::moved. https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:261: std::move(url), frame_tree_node_id, was_cached, raw_body_bytes, On 2017/04/12 15:01:17, Bryan McQuade wrote: > we chatted about this & I think you concluded that we'd remove the std::move(), > but just to document our reasoning, we concluded: > calling std::move() on a const object doesn't end up avoiding a copy, as the > |const GURL&&| resulting from the std::move() can't be passed to the GURL move > constructor, which takes a non-const |GURL&&| (move constructors take non-const > rvalues since the rvalue is being modified). Instead, IIUC, the |const GURL&&| > is implicitly converted to a const GURL& and the GURL copy constructor gets > invoked. Done. https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:55: return STOP_OBSERVING; On 2017/04/11 17:30:06, Charlie Harrison wrote: > Can you do this feature detection at construction time? Done. https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h (right): https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:53: std::list<AdFrameData> ad_frames_data_storage_; On 2017/04/11 17:30:06, Charlie Harrison wrote: > It is append-only. Why not use std::vector? Because I'm taking pointers to its elements. When vectors grow they copy the elements to a new vector of larger size. https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:55: // Maps a frame (by id) to the AdFraneData responsible for the frame. The On 2017/04/11 17:30:06, Charlie Harrison wrote: > s/AdFraneData/AdFrameData Done. https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:15: using content::RenderFrameHostTester; On 2017/04/11 17:30:07, Charlie Harrison wrote: > include test_renderer_host.h Done. https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:17: namespace { On 2017/04/11 17:30:06, Charlie Harrison wrote: > nit: > namespace { > > enum class ResourceCached { NOT_CACHED, CACHED }; > > } // namespace Done. https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:39: return subframe; On 2017/04/11 17:30:06, Charlie Harrison wrote: > I wonder if you could use the NavigationSimulator here and above, which will > ensure the code is correct for OOPIF / transfer navigations if you navigate > cross-site. > > This is newish territory for the PLM test code but I think it might be needed > now that we care about subframes. Done. https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:46: GURL(), frame->GetFrameTreeNodeId(), On 2017/04/11 17:30:06, Charlie Harrison wrote: > include gurl Already there. https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:54: void RegisterObservers(page_load_metrics::PageLoadTracker* tracker) override { On 2017/04/11 17:30:06, Charlie Harrison wrote: > include page_load_tracker.h Done. https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:55: tracker->AddObserver(base::WrapUnique(new AdsPageLoadMetricsObserver())); On 2017/04/11 17:30:06, Charlie Harrison wrote: > nit: prefer base::MakeUnique<AdsPageLoadMetricsObserver>() Done. https://codereview.chromium.org/2798953002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:59: DISALLOW_COPY_AND_ASSIGN(AdsPageLoadMetricsObserverTest); On 2017/04/11 17:30:06, Charlie Harrison wrote: > include base/macros. Already there. https://codereview.chromium.org/2798953002/diff/180001/url/gurl.h File url/gurl.h (right): https://codereview.chromium.org/2798953002/diff/180001/url/gurl.h#newcode55 url/gurl.h:55: // to reallocating the srtcing. It does not re-parse. On 2017/04/11 17:30:07, Charlie Harrison wrote: > nit: typo Done.
jkarlin@chromium.org changed reviewers: + clamy@chromium.org
clamy@chromium.org: Please review changes in content/public/ Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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 jkarlin@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...
jkarlin@chromium.org changed reviewers: + jam@chromium.org
jam@chromium.org: Please review changes in chrome/browser/loader and content/public/ isherman@: PTAL at histograms.xml removing clamy as she is OOO
The CQ bit was checked by jkarlin@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_...)
Looks great. It looks like you are prefacing these histogram names with "Google", how do you plan on extending this code / naming scheme if we want to track more ad networks? https://codereview.chromium.org/2798953002/diff/220001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2798953002/diff/220001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:47: : enabled_(base::FeatureList::IsEnabled(kAdsFeature)) {} Sorry I don't think I was clear. Why do we even need to add this observer if the feature isn't enabled? Let's just make a static MaybeCreate() method which returns nullptr or something if the feature isn't enabled. https://codereview.chromium.org/2798953002/diff/220001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:73: bool top_level_frame = !parent_frame_host->GetParent(); optional: top_level_frame (and the associated histogram) implies main frame to me. What do you think about top_level_subframe? https://codereview.chromium.org/2798953002/diff/220001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:130: auto it_and_success = delayed_resources_.insert(std::make_pair( delayed_resources_ is a little generic. What do you think about "ongoing_navigation_resources_" or something to that affect? https://codereview.chromium.org/2798953002/diff/220001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:183: UMA_HISTOGRAM_COUNTS("PageLoad.Clients.Ads.Google.PageHasNoAds", 1); Use the Boolean specific histogram macros, they're cheaper. https://codereview.chromium.org/2798953002/diff/220001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2798953002/diff/220001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:35: return main_frame; Careful, if you navigated across process the RFH can be swapped from under you. Use NavigationSimulator::GetFinalRenderFrameHost() instead. Can you make sure tests pass and add the site isolation trybots to this CL? CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation https://codereview.chromium.org/2798953002/diff/220001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:44: return subframe; ditto. https://codereview.chromium.org/2798953002/diff/220001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:160: // coutned). Also verify that the various ad signals (urls and names) are s/coutned/counted https://codereview.chromium.org/2798953002/diff/260001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/2798953002/diff/260001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:278: committed_load_->DidCommitSubFrame(navigation_handle); Do we really want to intentially send this for error pages? https://codereview.chromium.org/2798953002/diff/260001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h (right): https://codereview.chromium.org/2798953002/diff/260001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:64: size_t page_bytes_ = 0; nit: 0u for size_ts
thanks! core page load metrics infra changes look good. i haven't looked at the new observer yet but will do so shortly. https://codereview.chromium.org/2798953002/diff/260001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/2798953002/diff/260001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:278: committed_load_->DidCommitSubFrame(navigation_handle); On 2017/04/13 at 19:09:57, Charlie Harrison wrote: > Do we really want to intentially send this for error pages? Good question. I do think we want the downstream client to be able to know that the page they thought was committed in a frame is no longer committed. Telling about every commit, including error pages, seems like one way to accomplish that, at the cost that clients also have to think about error pages. Maybe the right thing for now is to keep it simple and inform about all commits, but add comments. If more observers start using this API we may get a better sense for what their needs/goals are and can refine & refactor to address those needs well. How does that sound? https://codereview.chromium.org/2798953002/diff/260001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right): https://codereview.chromium.org/2798953002/diff/260001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_observer.h:281: virtual ObservePolicy OnCommitSubFrame( i dont see a strong reason that observers might want to stop observing for a certain subframe commit, so i'm inclined to have this return void for now. how does that sound?
https://codereview.chromium.org/2798953002/diff/260001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/2798953002/diff/260001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:278: committed_load_->DidCommitSubFrame(navigation_handle); On 2017/04/13 19:21:27, Bryan McQuade wrote: > On 2017/04/13 at 19:09:57, Charlie Harrison wrote: > > Do we really want to intentially send this for error pages? > > Good question. I do think we want the downstream client to be able to know that > the page they thought was committed in a frame is no longer committed. Telling > about every commit, including error pages, seems like one way to accomplish > that, at the cost that clients also have to think about error pages. > > Maybe the right thing for now is to keep it simple and inform about all commits, > but add comments. If more observers start using this API we may get a better > sense for what their needs/goals are and can refine & refactor to address those > needs well. How does that sound? I dunno. I feel like making observers do all the legwork is an easy way for bugs to leak in. OTOH specifying a policy leads to churn later on. I'm OK with either choice.
Thanks! As explained in the comment below, I don't agree with the approach of the RenderFrameHostGetter for navigations. It is hiding a lot of subtlety from the user of the callback, and is likely to cause issues IMO. https://codereview.chromium.org/2798953002/diff/260001/content/public/browser... File content/public/browser/resource_request_info.h (right): https://codereview.chromium.org/2798953002/diff/260001/content/public/browser... content/public/browser/resource_request_info.h:98: // example, requests made by a ServiceWorker. Looking at the implementation, this comment is simply not true for navigation requests with PlzNavigate enabled. The request is simply not associated with the current RenderFrameHost. It may be a browser-initiated cross-process navigation, in which case the current RFH will _never_ hear about it. Thus, I am opposed to this RenderFrameHostGetter returning anything but nullptr for navigation requests when PlzNavigate is enabled. Navigation requests _are not_ associated with a RFH, and we should not pretend otherwise. We can discuss other ways to address the issues you're facing, but this direction seems wrong.
https://codereview.chromium.org/2798953002/diff/260001/content/public/browser... File content/public/browser/resource_request_info.h (right): https://codereview.chromium.org/2798953002/diff/260001/content/public/browser... content/public/browser/resource_request_info.h:98: // example, requests made by a ServiceWorker. On 2017/04/14 13:56:09, clamy wrote: > Looking at the implementation, this comment is simply not true for navigation > requests with PlzNavigate enabled. The request is simply not associated with the > current RenderFrameHost. It may be a browser-initiated cross-process navigation, > in which case the current RFH will _never_ hear about it. > > Thus, I am opposed to this RenderFrameHostGetter returning anything but nullptr > for navigation requests when PlzNavigate is enabled. Navigation requests _are > not_ associated with a RFH, and we should not pretend otherwise. > > We can discuss other ways to address the issues you're facing, but this > direction seems wrong. I think returning nullptr here for navigations is reasonable, provided that we can properly get the frame tree node ids unconditionally. That is the only thing we are pulling out of the RFH currently. Correct me if I'm wrong jkarlin
Description was changed from ========== [PageLoadMetrics] Keep track of Ad Sizes on Pages This CL adds a PageLoadObserver that keeps track of frames with ads and them and reports their byte usage. As part of the work, the following additional changes were necessary: 1) Frame Tree ID and URL are added to extra request info 2) Add GetRenderFrameHostGetterForRequest to ResourceRequestInfo so that ChromeResourceDispatcherHostDelegate can get at the frame tree node id of the resource. BUG=708570 ========== to ========== [PageLoadMetrics] Keep track of Ad Sizes on Pages This CL adds a PageLoadObserver that keeps track of frames with ads and them and reports their byte usage. As part of the work, the following additional changes were necessary: 1) Frame Tree ID and URL are added to extra request info 2) Add GetRenderFrameHostGetterForRequest to ResourceRequestInfo so that ChromeResourceDispatcherHostDelegate can get at the frame tree node id of the resource. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation BUG=708570 ==========
The CQ bit was checked by jkarlin@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...
Patchset #14 (id:280001) has been deleted
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 jkarlin@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...
Patchset #14 (id:300001) has been deleted
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...)
The CQ bit was checked by jkarlin@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...
Thanks all! PTAL In terms of naming in the future, the current plan is to put all the ad data together at the top level, and then also have a separate Google prefix and potentially other ad network prefixes as well. https://codereview.chromium.org/2798953002/diff/220001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2798953002/diff/220001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:47: : enabled_(base::FeatureList::IsEnabled(kAdsFeature)) {} On 2017/04/13 19:09:57, Charlie Harrison wrote: > Sorry I don't think I was clear. Why do we even need to add this observer if the > feature isn't enabled? Let's just make a static MaybeCreate() method which > returns nullptr or something if the feature isn't enabled. Done. https://codereview.chromium.org/2798953002/diff/220001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:73: bool top_level_frame = !parent_frame_host->GetParent(); On 2017/04/13 19:09:57, Charlie Harrison wrote: > optional: top_level_frame (and the associated histogram) implies main frame to > me. What do you think about top_level_subframe? Agreed, done. https://codereview.chromium.org/2798953002/diff/220001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:130: auto it_and_success = delayed_resources_.insert(std::make_pair( On 2017/04/13 19:09:57, Charlie Harrison wrote: > delayed_resources_ is a little generic. What do you think about > "ongoing_navigation_resources_" or something to that affect? Done. https://codereview.chromium.org/2798953002/diff/220001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:183: UMA_HISTOGRAM_COUNTS("PageLoad.Clients.Ads.Google.PageHasNoAds", 1); On 2017/04/13 19:09:57, Charlie Harrison wrote: > Use the Boolean specific histogram macros, they're cheaper. Done. https://codereview.chromium.org/2798953002/diff/220001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2798953002/diff/220001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:35: return main_frame; On 2017/04/13 19:09:57, Charlie Harrison wrote: > Careful, if you navigated across process the RFH can be swapped from under you. > Use NavigationSimulator::GetFinalRenderFrameHost() instead. > > Can you make sure tests pass and add the site isolation trybots to this CL? > CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Done. Thanks! https://codereview.chromium.org/2798953002/diff/220001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:44: return subframe; On 2017/04/13 19:09:57, Charlie Harrison wrote: > ditto. Done. https://codereview.chromium.org/2798953002/diff/220001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:160: // coutned). Also verify that the various ad signals (urls and names) are On 2017/04/13 19:09:57, Charlie Harrison wrote: > s/coutned/counted Done. https://codereview.chromium.org/2798953002/diff/260001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h (right): https://codereview.chromium.org/2798953002/diff/260001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:64: size_t page_bytes_ = 0; On 2017/04/13 19:09:57, Charlie Harrison wrote: > nit: 0u for size_ts Done. https://codereview.chromium.org/2798953002/diff/260001/content/public/browser... File content/public/browser/resource_request_info.h (right): https://codereview.chromium.org/2798953002/diff/260001/content/public/browser... content/public/browser/resource_request_info.h:98: // example, requests made by a ServiceWorker. On 2017/04/14 14:02:25, Charlie Harrison wrote: > On 2017/04/14 13:56:09, clamy wrote: > > Looking at the implementation, this comment is simply not true for navigation > > requests with PlzNavigate enabled. The request is simply not associated with > the > > current RenderFrameHost. It may be a browser-initiated cross-process > navigation, > > in which case the current RFH will _never_ hear about it. > > > > Thus, I am opposed to this RenderFrameHostGetter returning anything but > nullptr > > for navigation requests when PlzNavigate is enabled. Navigation requests _are > > not_ associated with a RFH, and we should not pretend otherwise. > > > > We can discuss other ways to address the issues you're facing, but this > > direction seems wrong. > > I think returning nullptr here for navigations is reasonable, provided that we > can properly get the frame tree node ids unconditionally. That is the only thing > we are pulling out of the RFH currently. > > Correct me if I'm wrong jkarlin I spoke with clamy@ and csharrison@. We agreed on a frame tree node id getter instead of a render frame host getter. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
this is a really nice change - great to see how all these components can be used together to achieve this level of page understanding. i'm part way through the observer & wanted to share a few comments so far. will be circling back shortly to finish the review. https://codereview.chromium.org/2798953002/diff/260001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right): https://codereview.chromium.org/2798953002/diff/260001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_observer.h:281: virtual ObservePolicy OnCommitSubFrame( On 2017/04/13 at 19:21:27, Bryan McQuade wrote: > i dont see a strong reason that observers might want to stop observing for a certain subframe commit, so i'm inclined to have this return void for now. how does that sound? looks like this comment might've gotten lost (or i might've just missed your reply - sorry if that's the case). just wanted to re-ping to make sure you see it. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:25: navigation_handle->GetWebContents()->FindFrameByFrameTreeNodeId( is it possible for FindFrameByFrameTreeNodeId to return nullptr? should we guard against that case? https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:87: // This frame was previously not an ad, process it as usual. If it had do we expect repeat navs for ads? if not, it might be worth counting how common it is for either a previously committed frame that wasn't considered an ad to load an ad, or a previously committed frame that was an ad to commit. you could add an enum counter histogram to understand the frequency here. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:101: ad_frames_data_storage_.push_back(AdFrameData()); i'm not deeply familiar with it, but this looks like a case where you could use emplace_back() without any argument instead of push_back. take a look and see what you think. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h (right): https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:58: // responsible frame is the top-most frame labeled as an ad in the frame's it sounds like it's possible for multiple FrameTreeNodeIds to point at the same AdFrameData* - is that right? If so, might be worth stating this explicitly in the comment.
https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:25: navigation_handle->GetWebContents()->FindFrameByFrameTreeNodeId( On 2017/04/14 19:19:23, Bryan McQuade wrote: > is it possible for FindFrameByFrameTreeNodeId to return nullptr? should we guard > against that case? I think we should be able to DCHECK this especially if we're only calling this from DidFinishNavigation. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:61: DCHECK(ad_frames_data_.empty()); nit: include base/logging for DCHECK https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:87: // This frame was previously not an ad, process it as usual. If it had On 2017/04/14 19:19:23, Bryan McQuade wrote: > do we expect repeat navs for ads? if not, it might be worth counting how common > it is for either a previously committed frame that wasn't considered an ad to > load an ad, or a previously committed frame that was an ad to commit. you could > add an enum counter histogram to understand the frequency here. +1 https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h (right): https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:26: // page_load_metrics::PageLoadMetricsObserver:s nit: remove the s. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:48: // Checks to see if a resource is waiting for |frame_tree_node_id| to commit for a navigation with the given |frame_tree_node_id| to commit https://codereview.chromium.org/2798953002/diff/320001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2798953002/diff/320001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:45926: +<histogram name="PageLoad.Clients.Ads.Google.AdFrameCount" units="AdFrames"> If we're planning in the future to have non suffixed "Google" histograms, should we try to structure these as histogram suffixes?
lgtm https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:26: frame_tree_node_id); these two lines can just be navigation_handle->GetRenderFrameHost which is guaranteed to be null https://codereview.chromium.org/2798953002/diff/320001/content/public/browser... File content/public/browser/resource_request_info.h (right): https://codereview.chromium.org/2798953002/diff/320001/content/public/browser... content/public/browser/resource_request_info.h:119: // TODO(jkarlin): Set the id for all requests from frames. nit: remove this TODO. you can file a bug to discuss this if you want. IMO it would be confusing to both store the frame tree node ID and RFH IDs for a non-plznavigate request (i.e. what if they're not consistent?). while ideally we could just use the frame tree node ID, there's a lot of code that depends on the RFH IDs for maps and for checking the process part only, so it would be a big task to convert everything to FTN IDs.
I looked at everything but RecordHistograms. See what you think. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:25: navigation_handle->GetWebContents()->FindFrameByFrameTreeNodeId( On 2017/04/14 at 20:16:26, Charlie Harrison wrote: > On 2017/04/14 19:19:23, Bryan McQuade wrote: > > is it possible for FindFrameByFrameTreeNodeId to return nullptr? should we guard > > against that case? > > I think we should be able to DCHECK this especially if we're only calling this from DidFinishNavigation. actually, can we just call navigation_handle->GetRenderFrameHost() instead? ah, ok, i see jam's comment - seems like we can do this https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:141: if (!it_and_success.second) should we dcheck that no entry already exists for the given frame? could the following happen? 1. OnLoadedResource fires for a frame which has't yet committed, so we stuff the resource into ongoing_navigation_resources_ 2. the navigation in the frame fails to commit 3. a new navigation commits in the same frame 4. the OnLoadedResource for the nav that failed to commit gets associated with the new nav If so, is this undesirable behavior? Perhaps we should change OnCommitSubFrame to DidFinishSubframeNavigation, and invoke it for both commits and non-commits, and in step (2) above, clear the entry created in step (1)? https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:251: OnLoadedResource(frame_id_and_request->second); can we instead create a method ProcessLoadedResource that does the processing and invoke that here, and in OnLoadedResource in cases where the frame has already committed? It's potentially surprising for a newcomer to the code that this class invokes a method that's normally only invoked by PageLoadTracker. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h (right): https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:51: void ProcessOngoingNavigationResources(FrameTreeNodeId frame_tree_node_id); nit: when i read this name, which has Resources plural in the name, I assumed it was going to process multiple resources. It only processes one. I see now the name is derived from the member it operates on, but I think it's clearer to name it based on what it does, which is processes the single ongoing resource for the given frame tree node id. Perhaps ProcessOngoingNavigationResourceForFrame or just ProcessOngoingNavigationResource (singular)? https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:55: std::list<AdFrameData> ad_frames_data_storage_; My sense is that ad_frames_data_storage_ and ad_frames_data_ could be wrapped up in a helper class that hides some common logic. What do you think of something like: class AdFrameDataManager { public: const std::list<AdFrameData>& ad_frame_data() const; bool HasEntry(FrameTreeNodeId id) const; // Registers the given frame if not already registered, by associating it with // AdFrameData for an ancestor ad frame if any, then returns the associated // AdFrameData, or nullptr. AdFrameData* RegisterFrameAndGetData(content::RenderFrameHost* host); // Creates a new AdFrameData for the given frame. This method should only be // called if RegisterFrameAndGetData returns nullptr for the given frame. AdFrameData* CreateAdFrameData(FrameTreeNodeId id); // (if return false, the frame for the given ExtraRequestInfo is not yet known) bool ProcessLoadedResource(const ExtraRequestInfo& info); private: // Stores the size data of each ad frame ... std::list<AdFrameData> ad_frames_data_storage_; // Maps a frame (by id) to the AdFrameData responsible for ... std::unordered_map<FrameTreeNodeId, AdFrameData*> ad_frames_data_; }; This API can probably be cleaned up a bit, but I think this will simplify code that currently interacts with these fields directly - those call sites can instead focus on their policy and not worry about managing / updating these structs as well. WDYT? OnSubFrameCommit might look something like: if (manager.HasEntry(id)) { // An existing subframe is navigating again. // ... rest of repeat frame commit code here ... } AdFrameData* data = manager.RegisterFrameAndGetData(frame_host); if (!data && FrameIsAd(...)) data = manager.CreateAdFrameData(id); https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:63: GURL(), frame->GetFrameTreeNodeId(), just to make sure i understand, a FrameTreeNodeId is unique for the lifetime of the webcontents, right? So for example if main frame A commits, then child frame B commits, then B's child C commits, then B re-commits a new document, the C frame is destroyed. Once C frame is destroyed, its FrameTreeNodeId can't be re-used for the lifetime of the WebContents, right?
here are some suggestions for histogram names to make them slightly more consistent w/ existing histograms. please see these as optional. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:179: PAGE_BYTES_HISTOGRAM("PageLoad.Clients.Ads.Google.Bytes.AdFrameBytes", wdyt about triyng to use a naming scheme more consistent with our existing bytes histograms? we use '.Network', '.Cache', and '.Total'. could we use the same suffixes in this class as well? e.g. PageLoad.Clients.Ads.Google.Bytes.PerAdFrame.Total and PageLoad.Clients.Ads.Google.Bytes.PerAdFrame.Network? https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:186: "PageLoad.Clients.Ads.Google.Bytes.PercentAdFrameBytesFromNetwork", these percentage histograms have historically been confusing since pages w/ small and large number of bytes are mixed together. so if this says 90% for example, does that mean there's a big opportunity to reduce network utilization, or does it just mean that most frames only load a small number of bytes, mostly from network? my experience is that these histos are not very actionable. would it be possible to infer something as useful by just taking the ratio of sums for network bytes and total bytes in aggregate when looking at the uma dash? i'm fine keeping this if you want it - just want to raise the potential concern that these can be misleading early on. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:191: UMA_HISTOGRAM_BOOLEAN("PageLoad.Clients.Ads.Google.PageHasNoAds", could we instead have a single counter for number of ad frames per page, and the %/count where count is zero can be used to compute this histogram? https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:195: if (ad_frames_data_storage_.empty()) can we move the earlier histo that reports ad counts, as well as this return statement, to the top of this function? https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:200: UMA_HISTOGRAM_COUNTS_1000("PageLoad.Clients.Ads.Google.TopLevelAdFrameCount", since you have 'Ads.Google.AdFrameCount' below, could we suffix this, and make it 'Ads.Google.AdFrameCount.TopLevel'? and maybe also SubFrameCount.TopLevel above as well? https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:208: PAGE_BYTES_HISTOGRAM("PageLoad.Clients.Ads.Google.Bytes.AllAdFramesBytes", here too if we use the 'Bytes.PerAdFrame.Total' above, we can use 'Bytes.AllAdFrames.Total' here which is more consistent with naming elsewhere. (could potentially shorten to Bytes.Ads.Total and Bytes.NonAds.Total, but that may be slightly misleading in that it's only reporting ad frame bytes whereas some ads are loaded in the top level page, so AllAdFrames is probably more accurate naming) https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:212: "PageLoad.Clients.Ads.Google.Bytes.PageBytesSansAllAdFrames", Bytes.NonAdFrames.Total? https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:215: PAGE_BYTES_HISTOGRAM("PageLoad.Clients.Ads.Google.Bytes.PageBytes", Bytes.Total? https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:217: PAGE_BYTES_HISTOGRAM("PageLoad.Clients.Ads.Google.Bytes.PageBytesFromNetwork", Bytes.Network? https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:229: "PageLoad.Clients.Ads.Google.Bytes.AllAdFramesBytesFromNetwork", Bytes.AllAdFrames.Total?
Thanks! content/ lgtm.
The CQ bit was checked by jkarlin@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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
Patchset #15 (id:340001) has been deleted
The CQ bit was checked by jkarlin@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) 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 jkarlin@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 checked by jkarlin@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_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 jkarlin@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...
Patchset #17 (id:400001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
The CQ bit was checked by jkarlin@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...
Patchset #17 (id:420001) has been deleted
Thanks all, PTAL! https://codereview.chromium.org/2798953002/diff/260001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/2798953002/diff/260001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:278: committed_load_->DidCommitSubFrame(navigation_handle); On 2017/04/13 19:26:36, Charlie Harrison wrote: > On 2017/04/13 19:21:27, Bryan McQuade wrote: > > On 2017/04/13 at 19:09:57, Charlie Harrison wrote: > > > Do we really want to intentially send this for error pages? > > > > Good question. I do think we want the downstream client to be able to know > that > > the page they thought was committed in a frame is no longer committed. Telling > > about every commit, including error pages, seems like one way to accomplish > > that, at the cost that clients also have to think about error pages. > > > > Maybe the right thing for now is to keep it simple and inform about all > commits, > > but add comments. If more observers start using this API we may get a better > > sense for what their needs/goals are and can refine & refactor to address > those > > needs well. How does that sound? > > I dunno. I feel like making observers do all the legwork is an easy way for bugs > to leak in. OTOH specifying a policy leads to churn later on. I'm OK with either > choice. I've changed it to not send for error pages as I'm guessing (of course that's going to be wrong) that most observers (including ads) won't want them and we can change it later if they do. https://codereview.chromium.org/2798953002/diff/260001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right): https://codereview.chromium.org/2798953002/diff/260001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_observer.h:281: virtual ObservePolicy OnCommitSubFrame( On 2017/04/14 19:19:23, Bryan McQuade wrote: > On 2017/04/13 at 19:21:27, Bryan McQuade wrote: > > i dont see a strong reason that observers might want to stop observing for a > certain subframe commit, so i'm inclined to have this return void for now. how > does that sound? > > looks like this comment might've gotten lost (or i might've just missed your > reply - sorry if that's the case). just wanted to re-ping to make sure you see > it. Sorry, I did miss it. Thanks for pointing it out again. I originally wrote it to return void but INVOKE_AND_PRUNE_OBSERVERS requires an ObservePolicy, so I figured it didn't hurt to leave it. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:25: navigation_handle->GetWebContents()->FindFrameByFrameTreeNodeId( On 2017/04/17 22:00:04, Bryan McQuade wrote: > On 2017/04/14 at 20:16:26, Charlie Harrison wrote: > > On 2017/04/14 19:19:23, Bryan McQuade wrote: > > > is it possible for FindFrameByFrameTreeNodeId to return nullptr? should we > guard > > > against that case? > > > > I think we should be able to DCHECK this especially if we're only calling this > from DidFinishNavigation. > > actually, can we just call > navigation_handle->GetRenderFrameHost() instead? > > ah, ok, i see jam's comment - seems like we can do this Implemented jam's suggestion and dcheck the pointer. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:26: frame_tree_node_id); On 2017/04/17 15:41:55, jam wrote: > these two lines can just be navigation_handle->GetRenderFrameHost which is > guaranteed to be null Done. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:61: DCHECK(ad_frames_data_.empty()); On 2017/04/14 20:16:26, Charlie Harrison wrote: > nit: include base/logging for DCHECK Done. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:87: // This frame was previously not an ad, process it as usual. If it had On 2017/04/14 19:19:23, Bryan McQuade wrote: > do we expect repeat navs for ads? if not, it might be worth counting how common > it is for either a previously committed frame that wasn't considered an ad to > load an ad, or a previously committed frame that was an ad to commit. you could > add an enum counter histogram to understand the frequency here. Certainly doable. What would we do with this information? https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:101: ad_frames_data_storage_.push_back(AdFrameData()); On 2017/04/14 19:19:23, Bryan McQuade wrote: > i'm not deeply familiar with it, but this looks like a case where you could use > emplace_back() without any argument instead of push_back. take a look and see > what you think. Done. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:141: if (!it_and_success.second) On 2017/04/17 22:00:04, Bryan McQuade wrote: > should we dcheck that no entry already exists for the given frame? > > could the following happen? > > 1. OnLoadedResource fires for a frame which has't yet committed, so we stuff the > resource into ongoing_navigation_resources_ > > 2. the navigation in the frame fails to commit > > 3. a new navigation commits in the same frame > > 4. the OnLoadedResource for the nav that failed to commit gets associated with > the new nav Yes, that can happen hence the lack of dcheck. > If so, is this undesirable behavior? Perhaps we should change OnCommitSubFrame > to DidFinishSubframeNavigation, and invoke it for both commits and non-commits, > and in step (2) above, clear the entry created in step (1)? Good idea, done. Also added a new test. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:179: PAGE_BYTES_HISTOGRAM("PageLoad.Clients.Ads.Google.Bytes.AdFrameBytes", On 2017/04/18 17:11:32, Bryan McQuade wrote: > wdyt about triyng to use a naming scheme more consistent with our existing bytes > histograms? we use '.Network', '.Cache', and '.Total'. could we use the same > suffixes in this class as well? e.g. > PageLoad.Clients.Ads.Google.Bytes.PerAdFrame.Total and > PageLoad.Clients.Ads.Google.Bytes.PerAdFrame.Network? Good idea, done. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:186: "PageLoad.Clients.Ads.Google.Bytes.PercentAdFrameBytesFromNetwork", On 2017/04/18 17:11:33, Bryan McQuade wrote: > these percentage histograms have historically been confusing since pages w/ > small and large number of bytes are mixed together. so if this says 90% for > example, does that mean there's a big opportunity to reduce network utilization, > or does it just mean that most frames only load a small number of bytes, mostly > from network? my experience is that these histos are not very actionable. I think it's useful to be able to make statements like, "the median page loads less than 30% of its ad data from the network, and for 95% of page loads ads make up less than 40% of bytes" etc. So I'd like to keep them. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:191: UMA_HISTOGRAM_BOOLEAN("PageLoad.Clients.Ads.Google.PageHasNoAds", On 2017/04/18 17:11:33, Bryan McQuade wrote: > could we instead have a single counter for number of ad frames per page, and the > %/count where count is zero can be used to compute this histogram? Done. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:195: if (ad_frames_data_storage_.empty()) On 2017/04/18 17:11:32, Bryan McQuade wrote: > can we move the earlier histo that reports ad counts, as well as this return > statement, to the top of this function? Done. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:200: UMA_HISTOGRAM_COUNTS_1000("PageLoad.Clients.Ads.Google.TopLevelAdFrameCount", On 2017/04/18 17:11:33, Bryan McQuade wrote: > since you have 'Ads.Google.AdFrameCount' below, could we suffix this, and make > it 'Ads.Google.AdFrameCount.TopLevel'? and maybe also SubFrameCount.TopLevel > above as well? Done, though I didn't use proper suffixes in the histograms.xml as I'm not sure it's worth the added complexity for reviewers. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:208: PAGE_BYTES_HISTOGRAM("PageLoad.Clients.Ads.Google.Bytes.AllAdFramesBytes", On 2017/04/18 17:11:32, Bryan McQuade wrote: > here too if we use the 'Bytes.PerAdFrame.Total' above, we can use > 'Bytes.AllAdFrames.Total' here which is more consistent with naming elsewhere. > (could potentially shorten to Bytes.Ads.Total and Bytes.NonAds.Total, but that > may be slightly misleading in that it's only reporting ad frame bytes whereas > some ads are loaded in the top level page, so AllAdFrames is probably more > accurate naming) Done. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:212: "PageLoad.Clients.Ads.Google.Bytes.PageBytesSansAllAdFrames", On 2017/04/18 17:11:33, Bryan McQuade wrote: > Bytes.NonAdFrames.Total? Done. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:215: PAGE_BYTES_HISTOGRAM("PageLoad.Clients.Ads.Google.Bytes.PageBytes", On 2017/04/18 17:11:32, Bryan McQuade wrote: > Bytes.Total? Done. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:217: PAGE_BYTES_HISTOGRAM("PageLoad.Clients.Ads.Google.Bytes.PageBytesFromNetwork", On 2017/04/18 17:11:32, Bryan McQuade wrote: > Bytes.Network? Done. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:229: "PageLoad.Clients.Ads.Google.Bytes.AllAdFramesBytesFromNetwork", On 2017/04/18 17:11:32, Bryan McQuade wrote: > Bytes.AllAdFrames.Total? I think you mean Bytes.AllAdFrames.Network. Done. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:251: OnLoadedResource(frame_id_and_request->second); On 2017/04/17 22:00:04, Bryan McQuade wrote: > can we instead create a method ProcessLoadedResource that does the processing > and invoke that here, and in OnLoadedResource in cases where the frame has > already committed? It's potentially surprising for a newcomer to the code that > this class invokes a method that's normally only invoked by PageLoadTracker. Done. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h (right): https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:26: // page_load_metrics::PageLoadMetricsObserver:s On 2017/04/14 20:16:26, Charlie Harrison wrote: > nit: remove the s. Done. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:48: // Checks to see if a resource is waiting for |frame_tree_node_id| to commit On 2017/04/14 20:16:26, Charlie Harrison wrote: > for a navigation with the given |frame_tree_node_id| to commit Done. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:51: void ProcessOngoingNavigationResources(FrameTreeNodeId frame_tree_node_id); On 2017/04/17 22:00:04, Bryan McQuade wrote: > nit: when i read this name, which has Resources plural in the name, I assumed it > was going to process multiple resources. It only processes one. I see now the > name is derived from the member it operates on, but I think it's clearer to name > it based on what it does, which is processes the single ongoing resource for the > given frame tree node id. Perhaps ProcessOngoingNavigationResourceForFrame or > just ProcessOngoingNavigationResource (singular)? Done. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:55: std::list<AdFrameData> ad_frames_data_storage_; On 2017/04/17 22:00:04, Bryan McQuade wrote: > My sense is that ad_frames_data_storage_ and ad_frames_data_ could be wrapped up > in a helper class that hides some common logic. What do you think of something > like: > > class AdFrameDataManager { > public: > const std::list<AdFrameData>& ad_frame_data() const; > > bool HasEntry(FrameTreeNodeId id) const; > > // Registers the given frame if not already registered, by associating it with > // AdFrameData for an ancestor ad frame if any, then returns the associated > // AdFrameData, or nullptr. > AdFrameData* RegisterFrameAndGetData(content::RenderFrameHost* host); > > // Creates a new AdFrameData for the given frame. This method should only be > // called if RegisterFrameAndGetData returns nullptr for the given frame. > AdFrameData* CreateAdFrameData(FrameTreeNodeId id); > > // (if return false, the frame for the given ExtraRequestInfo is not yet > known) > bool ProcessLoadedResource(const ExtraRequestInfo& info); > > private: > // Stores the size data of each ad frame ... > std::list<AdFrameData> ad_frames_data_storage_; > > // Maps a frame (by id) to the AdFrameData responsible for ... > std::unordered_map<FrameTreeNodeId, AdFrameData*> ad_frames_data_; > }; > > This API can probably be cleaned up a bit, but I think this will simplify code > that currently interacts with these fields directly - those call sites can > instead focus on their policy and not worry about managing / updating these > structs as well. WDYT? > > OnSubFrameCommit might look something like: > > if (manager.HasEntry(id)) { > // An existing subframe is navigating again. > // ... rest of repeat frame commit code here ... > } > > AdFrameData* data = manager.RegisterFrameAndGetData(frame_host); > > if (!data && FrameIsAd(...)) > data = manager.CreateAdFrameData(id); I think pretty much all of the logic would wind up in this new class. We'd also need iterators over all of the ad frames (or to return the ad_frames_data_storage_) so we could report metrics. Can we postpone a refactoring to a followup? https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:58: // responsible frame is the top-most frame labeled as an ad in the frame's On 2017/04/14 19:19:23, Bryan McQuade wrote: > it sounds like it's possible for multiple FrameTreeNodeIds to point at the same > AdFrameData* - is that right? If so, might be worth stating this explicitly in > the comment. Done. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:63: GURL(), frame->GetFrameTreeNodeId(), On 2017/04/17 22:00:04, Bryan McQuade wrote: > just to make sure i understand, a FrameTreeNodeId is unique for the lifetime of > the webcontents, right? So for example if main frame A commits, then child frame > B commits, then B's child C commits, then B re-commits a new document, the C > frame is destroyed. Once C frame is destroyed, its FrameTreeNodeId can't be > re-used for the lifetime of the WebContents, right? Correct, see: https://cs.chromium.org/chromium/src/content/browser/frame_host/frame_tree_no... and https://cs.chromium.org/chromium/src/content/browser/frame_host/frame_tree_no... https://codereview.chromium.org/2798953002/diff/320001/content/public/browser... File content/public/browser/resource_request_info.h (right): https://codereview.chromium.org/2798953002/diff/320001/content/public/browser... content/public/browser/resource_request_info.h:119: // TODO(jkarlin): Set the id for all requests from frames. On 2017/04/17 15:41:55, jam wrote: > nit: remove this TODO. you can file a bug to discuss this if you want. > > IMO it would be confusing to both store the frame tree node ID and RFH IDs for a > non-plznavigate request (i.e. what if they're not consistent?). while ideally we > could just use the frame tree node ID, there's a lot of code that depends on the > RFH IDs for maps and for checking the process part only, so it would be a big > task to convert everything to FTN IDs. Done. https://codereview.chromium.org/2798953002/diff/320001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2798953002/diff/320001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:45926: +<histogram name="PageLoad.Clients.Ads.Google.AdFrameCount" units="AdFrames"> On 2017/04/14 20:16:26, Charlie Harrison wrote: > If we're planning in the future to have non suffixed "Google" histograms, should > we try to structure these as histogram suffixes? I'd like to avoid adding the suffixes to the histograms.xml code until it's needed. When we decide to make these suffixes the idea is to change the names in histograms.xml to remove the 'Clients.Ads.Google' bits and then make a histogram_suffixes with ordering="prefix" and suffix name = "Clients.Ads.Google", "Clients.Ads", and any other suffixes we need. The histograms would then be 'Pageload.AdFrameCount, etc..' in histograms.xml. That uses up some top-level Pageload namespace, which isn't ideal, but I'm not aware of a better option. I wish we had more control over where the suffix goes in the histogram because then I could make the suffix name = "Google" and tell it to go after the "Pageload.Clients.Ads" part.
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_...)
Looks really good, just a few more comments. https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:111: ad_frames_data_storage_.emplace_back(); It looks like this call can initialize the storage with uninitialized values. Can you make sure the struct initializes the data to 0 in the header? https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:151: // here. Blobs for PLZNavigate shouldn't be counted as the http resource nit: s/PLZ/Plz https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:165: auto it_and_success = ongoing_navigation_resources_.emplace( I thought map emplace wasn't allowed in Chromium yet? https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:166: std::piecewise_construct, #include <utility> https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:168: std::forward_as_tuple( This seems a bit weird. Are we constructing a new ExtraRequestInfo here? Maybe we should just copy the original. https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:32: RenderFrameHost* NavigateMainFrame(const GURL& url) { Can you document that these return the final RFH? https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:33: RenderFrameHost* main_frame = web_contents()->GetMainFrame(); #include web_contents https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:40: RenderFrameHost* RenavigateFrame(const GURL& url, Just rename this NavigateFrame since it is pretty generic. Then you can implement NavigateMainFrame as just "return NavigateFrame(url, web_contents()->GetMainFrame());" https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:49: const std::string& frame_name, Optional: Unless you really need the frame name, inlining a placeholder in this method might make the tests a bit more readable. https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:62: page_load_metrics::ExtraRequestInfo request( #include page_load_metrics_observer https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:101: LoadResource(main_frame, ResourceCached::NOT_CACHED, 1); Optional: I know it's annoying to ask (hence why this is only optional), but using "1" for the size in kb makes these tests hard to read, as it conflates the size with the count. Would you mind changing it to something like "10" as a default for size? It would make reading the expected histogram lines much easier. https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_observer.h (left): https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_observer.h:211: const bool was_cached; Why do all these members need to be non-const now? https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right): https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_observer.h:278: // observer to query the navigation_handle to determine which happened. The Be consistent when referencing navigation_handle in this paragraph. I think I slightly prefer "navigation handle" over "navigation_handle"
https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h (right): https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:41: struct AdFrameData { cam we specify sane default values for the members of this struct?
https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:87: // This frame was previously not an ad, process it as usual. If it had On 2017/04/24 at 17:27:58, jkarlin wrote: > On 2017/04/14 19:19:23, Bryan McQuade wrote: > > do we expect repeat navs for ads? if not, it might be worth counting how common > > it is for either a previously committed frame that wasn't considered an ad to > > load an ad, or a previously committed frame that was an ad to commit. you could > > add an enum counter histogram to understand the frequency here. > > Certainly doable. What would we do with this information? I expect most people looking at the rest of the histograms this class produces aren't thinking about these cases when they try to reason about what the data means, so I think it'd be good to confirm that these cases are rare and thus it's reasonable for us to effectively ignore them. For instance if a frame starts as an ad and then becomes a non-ad but we continue to attribute its bytes as ads, is that correct? Incorrect? If it's a rare event it doesn't matter. If we don't know if it's rare then we are facing an unknown in our data that could make it hard to reason about that data when thinking about questions like this. If it's hard to add these counters, no need, but if it's easy, I think it'd be helpful to confirm that these cases are rare. https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h (right): https://codereview.chromium.org/2798953002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:55: std::list<AdFrameData> ad_frames_data_storage_; On 2017/04/24 at 17:27:59, jkarlin wrote: > On 2017/04/17 22:00:04, Bryan McQuade wrote: > > My sense is that ad_frames_data_storage_ and ad_frames_data_ could be wrapped up > > in a helper class that hides some common logic. What do you think of something > > like: > > > > class AdFrameDataManager { > > public: > > const std::list<AdFrameData>& ad_frame_data() const; > > > > bool HasEntry(FrameTreeNodeId id) const; > > > > // Registers the given frame if not already registered, by associating it with > > // AdFrameData for an ancestor ad frame if any, then returns the associated > > // AdFrameData, or nullptr. > > AdFrameData* RegisterFrameAndGetData(content::RenderFrameHost* host); > > > > // Creates a new AdFrameData for the given frame. This method should only be > > // called if RegisterFrameAndGetData returns nullptr for the given frame. > > AdFrameData* CreateAdFrameData(FrameTreeNodeId id); > > > > // (if return false, the frame for the given ExtraRequestInfo is not yet > > known) > > bool ProcessLoadedResource(const ExtraRequestInfo& info); > > > > private: > > // Stores the size data of each ad frame ... > > std::list<AdFrameData> ad_frames_data_storage_; > > > > // Maps a frame (by id) to the AdFrameData responsible for ... > > std::unordered_map<FrameTreeNodeId, AdFrameData*> ad_frames_data_; > > }; > > > > This API can probably be cleaned up a bit, but I think this will simplify code > > that currently interacts with these fields directly - those call sites can > > instead focus on their policy and not worry about managing / updating these > > structs as well. WDYT? > > > > OnSubFrameCommit might look something like: > > > > if (manager.HasEntry(id)) { > > // An existing subframe is navigating again. > > // ... rest of repeat frame commit code here ... > > } > > > > AdFrameData* data = manager.RegisterFrameAndGetData(frame_host); > > > > if (!data && FrameIsAd(...)) > > data = manager.CreateAdFrameData(id); > > I think pretty much all of the logic would wind up in this new class. We'd also need iterators over all of the ad frames (or to return the ad_frames_data_storage_) so we could report metrics. Can we postpone a refactoring to a followup? Sure - I'm fine with that.
The CQ bit was checked by jkarlin@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...
Thanks all. I appreciate the review effort you've put in. PTAL. https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:111: ad_frames_data_storage_.emplace_back(); On 2017/04/24 19:18:26, Charlie Harrison wrote: > It looks like this call can initialize the storage with uninitialized values. > Can you make sure the struct initializes the data to 0 in the header? Eek!. I thought it was initialized. Thanks! https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:151: // here. Blobs for PLZNavigate shouldn't be counted as the http resource On 2017/04/24 19:18:26, Charlie Harrison wrote: > nit: s/PLZ/Plz Done. https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:165: auto it_and_success = ongoing_navigation_resources_.emplace( On 2017/04/24 19:18:25, Charlie Harrison wrote: > I thought map emplace wasn't allowed in Chromium yet? I actually had assumed it was allowed (as it's allowed for vectors) but until yesterday it wasn't allowed for maps. But now it is :) https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:166: std::piecewise_construct, On 2017/04/24 19:18:26, Charlie Harrison wrote: > #include <utility> Done. https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:168: std::forward_as_tuple( On 2017/04/24 19:18:26, Charlie Harrison wrote: > This seems a bit weird. Are we constructing a new ExtraRequestInfo here? Maybe > we should just copy the original. We're creating a new one yes, but that's always required for a map. At least with emplace we only construct once instead of twice. Also, since ExtraRequestInfo is now non-copyable, we have little choice. https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h (right): https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:41: struct AdFrameData { On 2017/04/24 19:20:47, Bryan McQuade wrote: > cam we specify sane default values for the members of this struct? Thanks for the catch! https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:32: RenderFrameHost* NavigateMainFrame(const GURL& url) { On 2017/04/24 19:18:26, Charlie Harrison wrote: > Can you document that these return the final RFH? Done. https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:33: RenderFrameHost* main_frame = web_contents()->GetMainFrame(); On 2017/04/24 19:18:26, Charlie Harrison wrote: > #include web_contents Done. https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:40: RenderFrameHost* RenavigateFrame(const GURL& url, On 2017/04/24 19:18:26, Charlie Harrison wrote: > Just rename this NavigateFrame since it is pretty generic. Then you can > implement NavigateMainFrame as just "return NavigateFrame(url, > web_contents()->GetMainFrame());" Good idea, done. https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:49: const std::string& frame_name, On 2017/04/24 19:18:26, Charlie Harrison wrote: > Optional: Unless you really need the frame name, inlining a placeholder in this > method might make the tests a bit more readable. It's needed, but I've cleaned up the strings to make them more readable. https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:62: page_load_metrics::ExtraRequestInfo request( On 2017/04/24 19:18:26, Charlie Harrison wrote: > #include page_load_metrics_observer Done. https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc:101: LoadResource(main_frame, ResourceCached::NOT_CACHED, 1); On 2017/04/24 19:18:26, Charlie Harrison wrote: > Optional: I know it's annoying to ask (hence why this is only optional), but > using "1" for the size in kb makes these tests hard to read, as it conflates the > size with the count. Would you mind changing it to something like "10" as a > default for size? It would make reading the expected histogram lines much > easier. Done. https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_observer.h (left): https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_observer.h:211: const bool was_cached; On 2017/04/24 19:18:26, Charlie Harrison wrote: > Why do all these members need to be non-const now? Remnant from when earlier when I could copy the class. Removed. https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right): https://codereview.chromium.org/2798953002/diff/440001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_observer.h:278: // observer to query the navigation_handle to determine which happened. The On 2017/04/24 19:18:26, Charlie Harrison wrote: > Be consistent when referencing navigation_handle in this paragraph. I think I > slightly prefer "navigation handle" over "navigation_handle" Replaced with |navigation_handle|
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_...)
LGTM, thanks! https://codereview.chromium.org/2798953002/diff/460001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/2798953002/diff/460001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:295: if (!navigation_handle->IsInMainFrame() && committed_load_) { i think we should still return early if it's not a main frame nav, even if we don't have a committed load https://codereview.chromium.org/2798953002/diff/460001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h (right): https://codereview.chromium.org/2798953002/diff/460001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:45: FrameTreeNodeId frame_tree_node_id; can we make this member const, given that it is set in the constructor? https://codereview.chromium.org/2798953002/diff/460001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right): https://codereview.chromium.org/2798953002/diff/460001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_observer.h:229: std::unique_ptr<data_reduction_proxy::DataReductionProxyData> why the change to make this non-const?
https://codereview.chromium.org/2798953002/diff/460001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right): https://codereview.chromium.org/2798953002/diff/460001/chrome/browser/page_lo... chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:295: if (!navigation_handle->IsInMainFrame() && committed_load_) { On 2017/04/25 at 16:36:44, Bryan McQuade wrote: > i think we should still return early if it's not a main frame nav, even if we don't have a committed load sorry, my brain has apparently not fired up yet. please ignore this comment. :)
The CQ bit was checked by jkarlin@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...
https://codereview.chromium.org/2798953002/diff/460001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h (right): https://codereview.chromium.org/2798953002/diff/460001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:45: FrameTreeNodeId frame_tree_node_id; On 2017/04/25 16:36:44, Bryan McQuade wrote: > can we make this member const, given that it is set in the constructor? Done. https://codereview.chromium.org/2798953002/diff/460001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right): https://codereview.chromium.org/2798953002/diff/460001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_observer.h:229: std::unique_ptr<data_reduction_proxy::DataReductionProxyData> On 2017/04/25 16:36:44, Bryan McQuade wrote: > why the change to make this non-const? oversight, thanks!
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...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm/2 thanks
The CQ bit was checked by jkarlin@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...
jkarlin@chromium.org changed reviewers: + isherman@chromium.org
isherman@ PTAL at histograms.xml, thanks!
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
First of all, has there been a conversation with the privacy team about recording these metrics? They seem probably ok, since they're measuring something fairly generic (as opposed to user-identifying). However, I'd still like to make sure the privacy team is explicitly aware of and comfortable with these metrics, as they have a much more nuanced understanding of potential privacy ramifications than I do. Secondly, sheesh, that is a lot of new histograms. Do you really need to start with all of these? Each histogram has some memory cost associated with it, so recording fewer is better, if possible. It's also more manageable for people viewing the data -- it's hard to keep track of a bunch of histograms all at once. https://codereview.chromium.org/2798953002/diff/500001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2798953002/diff/500001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46703: +<histogram name="PageLoad.Clients.Ads.Google.AdFrameCount" units="AdFrames"> nit: Units are allowed to have spaces in them. Probably "Ad frames" is the most appropriate spelling, since this will be displayed as either a table column header or an axis label. https://codereview.chromium.org/2798953002/diff/500001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46712: + (which may also be ads, but are counted as part of the ancestor ad frame). This parenthetical statement is not entirely clear to me. Are you saying that if ad frames are nested in one another, only the topmost ad frame is counted for this metric? https://codereview.chromium.org/2798953002/diff/500001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46720: + The number of top-level frames on the page identified as Google Ad Frames. What is a top-level frame? I would have expected it to be the main document, but I guess you might mean frames that are children of the main document? Might be worth clarifying, if this is not a standard terminology that I just happen to be unfamiliar with. https://codereview.chromium.org/2798953002/diff/500001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46723: + measured as over-the-wire (e.g., compressed) response body KBs and does not nit: The comment about bytes seems irrelevant to this histogram. https://codereview.chromium.org/2798953002/diff/500001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46731: + units="Boolean"> nit: (Just within this file), please define a custom enum with more semantically rich labels. https://codereview.chromium.org/2798953002/diff/500001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46743: + units="KB"> Could you use a histogram_suffixes element (or a sequence of them) to reduce the amount of repeated text among these histograms? You'll likely find the base="true" attribute useful. https://codereview.chromium.org/2798953002/diff/500001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46906: + units="Boolean"> Ditto about the enum. https://codereview.chromium.org/2798953002/diff/500001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46910: + whether it renavigated to an ad frame or a non-ad frame. Is this metric only recorded if the page originally had an ad on it? The description for "PageLoad.Clients.Ads.Google.AdFrameCount" suggests the answer is yes; but the lack of text to this effect makes me ask, since the other histograms are all careful to reiterate this. https://codereview.chromium.org/2798953002/diff/500001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46936: + Only recorded if the page has at least one identified ad frame. Bytes are nit: Whitespace
The CQ bit was checked by jkarlin@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_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 jkarlin@chromium.org to run a CQ dry run
Patchset #22 (id:540001) 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...
Patchset #22 (id:560001) has been deleted
The CQ bit was checked by jkarlin@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...
Thanks Ilya! > First of all, has there been a conversation with the privacy team about > recording these metrics? They seem probably ok, since they're measuring > something fairly generic (as opposed to user-identifying). However, I'd still > like to make sure the privacy team is explicitly aware of and comfortable with > these metrics, as they have a much more nuanced understanding of potential > privacy ramifications than I do. Minor discussions, not a formal review. Sent out for formal review in https://crbug.com/715547 and cc'd you. > Secondly, sheesh, that is a lot of new histograms. Do you really need to start > with all of these? Each histogram has some memory cost associated with it, so > recording fewer is better, if possible. It's also more manageable for people > viewing the data -- it's hard to keep track of a bunch of histograms all at > once. bmcquade@ and I sat down and whittled it as far as we could (it was originally bigger!). We'll be sure to proactively prune the ones that we don't wind up needing long term. But for now we have a lot of questions that need answers. https://codereview.chromium.org/2798953002/diff/500001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2798953002/diff/500001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46703: +<histogram name="PageLoad.Clients.Ads.Google.AdFrameCount" units="AdFrames"> On 2017/04/26 00:09:44, Ilya Sherman wrote: > nit: Units are allowed to have spaces in them. Probably "Ad frames" is the most > appropriate spelling, since this will be displayed as either a table column > header or an axis label. Done. https://codereview.chromium.org/2798953002/diff/500001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46712: + (which may also be ads, but are counted as part of the ancestor ad frame). On 2017/04/26 00:09:45, Ilya Sherman wrote: > This parenthetical statement is not entirely clear to me. Are you saying that > if ad frames are nested in one another, only the topmost ad frame is counted for > this metric? Fixed. https://codereview.chromium.org/2798953002/diff/500001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46720: + The number of top-level frames on the page identified as Google Ad Frames. On 2017/04/26 00:09:44, Ilya Sherman wrote: > What is a top-level frame? I would have expected it to be the main document, > but I guess you might mean frames that are children of the main document? Might > be worth clarifying, if this is not a standard terminology that I just happen to > be unfamiliar with. Clarified and renamed to .MainFrame https://codereview.chromium.org/2798953002/diff/500001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46723: + measured as over-the-wire (e.g., compressed) response body KBs and does not On 2017/04/26 00:09:45, Ilya Sherman wrote: > nit: The comment about bytes seems irrelevant to this histogram. Done. https://codereview.chromium.org/2798953002/diff/500001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46731: + units="Boolean"> On 2017/04/26 00:09:44, Ilya Sherman wrote: > nit: (Just within this file), please define a custom enum with more semantically > rich labels. Done. https://codereview.chromium.org/2798953002/diff/500001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46743: + units="KB"> On 2017/04/26 00:09:44, Ilya Sherman wrote: > Could you use a histogram_suffixes element (or a sequence of them) to reduce the > amount of repeated text among these histograms? You'll likely find the > base="true" attribute useful. I think suffixes will make these much harder to review and maintain. I have a strong preference to keep these as is. Eventually there is a good chance that we'll want to have Clients.Ads.* and Clients.Ads.Google.* at which point I think breaking into suffixes will make sense. https://codereview.chromium.org/2798953002/diff/500001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46906: + units="Boolean"> On 2017/04/26 00:09:44, Ilya Sherman wrote: > Ditto about the enum. Done. https://codereview.chromium.org/2798953002/diff/500001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46910: + whether it renavigated to an ad frame or a non-ad frame. On 2017/04/26 00:09:44, Ilya Sherman wrote: > Is this metric only recorded if the page originally had an ad on it? The > description for "PageLoad.Clients.Ads.Google.AdFrameCount" suggests the answer > is yes; but the lack of text to this effect makes me ask, since the other > histograms are all careful to reiterate this. Good question. This metric is a special case. Updated the text. https://codereview.chromium.org/2798953002/diff/500001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46936: + Only recorded if the page has at least one identified ad frame. Bytes are On 2017/04/26 00:09:44, Ilya Sherman wrote: > nit: Whitespace Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
If you really want to record all of these metrics, I'd suggest rejiggering the naming structure to more clearly align parallel metrics. Here's my best attempt at doing so: PageLoad.Clients.Ads.Google.FrameCounts.AnyParentFrame.AdFrames PageLoad.Clients.Ads.Google.FrameCounts.MainFrameParent.AdFrames PageLoad.Clients.Ads.Google.FrameCounts.MainFrameParent.TotalFrames PageLoad.Clients.Ads.Google.FrameCounts.MainFrameParent.PercentAdFrames PageLoad.Clients.Ads.Google.Navigations.AdFrameRenavigatedToAd PageLoad.Clients.Ads.Google.Navigations.NonAdFrameRenavigatedToAd PageLoad.Clients.Ads.Google.Bytes.AdFrames.PerFrame.Network PageLoad.Clients.Ads.Google.Bytes.AdFrames.PerFrame.PercentNetwork PageLoad.Clients.Ads.Google.Bytes.AdFrames.PerFrame.Total PageLoad.Clients.Ads.Google.Bytes.AdFrames.Summed.Network PageLoad.Clients.Ads.Google.Bytes.AdFrames.Summed.PercentNetwork PageLoad.Clients.Ads.Google.Bytes.AdFrames.Summed.Total PageLoad.Clients.Ads.Google.Bytes.FullPage.Network PageLoad.Clients.Ads.Google.Bytes.FullPage.Total PageLoad.Clients.Ads.Google.Bytes.FullPage.PercentAds.Network PageLoad.Clients.Ads.Google.Bytes.FullPage.PercentAds.Total PageLoad.Clients.Ads.Google.Bytes.NonAdFrames.Summed.Network PageLoad.Clients.Ads.Google.Bytes.NonAdFrames.Summed.Total -- FWIW, I was debating whether PageLoad.Clients.Ads.Google.Bytes.FullPage.PercentAds.Network PageLoad.Clients.Ads.Google.Bytes.FullPage.PercentAds.Total or PageLoad.Clients.Ads.Google.Bytes.FullPage.Network.PercentAds PageLoad.Clients.Ads.Google.Bytes.FullPage.Total.PercentAds is more appropriate. I could be convinced either way. Feel free to further refine my naming scheme; hopefully I've conveyed at least the idea of how I was thinking about better aligning these. https://codereview.chromium.org/2798953002/diff/580001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2798953002/diff/580001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46709: + not recorded. nit: unless otherwise specified https://codereview.chromium.org/2798953002/diff/580001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46728: + units="DidNavigateToAd"> nit: s/units/enum https://codereview.chromium.org/2798953002/diff/580001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46750: + measured as over-the-wire (e.g., compressed) response body KBs and does not nit: s/does/do https://codereview.chromium.org/2798953002/diff/580001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46888: + identified as Google Ad Frames. I don't understand the last sentence in this paragraph. How does it relate to this histogram? https://codereview.chromium.org/2798953002/diff/580001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46901: + ancestor ad frame). This histogram does not measure ad frames, right? https://codereview.chromium.org/2798953002/diff/580001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46906: + units="DidNavigateToAd"> nit: s/units/enum https://codereview.chromium.org/2798953002/diff/580001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46913: + like the rest of the Clients.Ads metrics (unless otherwise specified). I find this sentence somewhat confusing. I'd rephrase it as something like the following (of course, feel free to refine further!): This metric is recorded as the event happens. Note that this is unlike most other Clients.Ads metrics, which are recorded when the page load is complete. https://codereview.chromium.org/2798953002/diff/580001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46937: + The number of frames frames (with parent frame of main frame) on the page. nit: s/frames frames/frames https://codereview.chromium.org/2798953002/diff/580001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46941: + include header bytes. An ad frame consists of the identified ad frame and This histogram does not measure bytes, nor does it measure ad frames (right?)
The CQ bit was checked by jkarlin@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...
Patchset #23 (id:600001) has been deleted
The CQ bit was checked by jkarlin@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...
Thanks for the great review isherman@. I've made the renaming changes as well as those below. PTAL. https://codereview.chromium.org/2798953002/diff/580001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2798953002/diff/580001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46709: + not recorded. On 2017/04/27 00:25:28, Ilya Sherman wrote: > nit: unless otherwise specified Done. https://codereview.chromium.org/2798953002/diff/580001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46728: + units="DidNavigateToAd"> On 2017/04/27 00:25:28, Ilya Sherman wrote: > nit: s/units/enum Done. https://codereview.chromium.org/2798953002/diff/580001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46750: + measured as over-the-wire (e.g., compressed) response body KBs and does not On 2017/04/27 00:25:28, Ilya Sherman wrote: > nit: s/does/do Done. Here and elsewhere. https://codereview.chromium.org/2798953002/diff/580001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46888: + identified as Google Ad Frames. On 2017/04/27 00:25:28, Ilya Sherman wrote: > I don't understand the last sentence in this paragraph. How does it relate to > this histogram? Wasn't supposed to be there. Removed. https://codereview.chromium.org/2798953002/diff/580001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46901: + ancestor ad frame). On 2017/04/27 00:25:29, Ilya Sherman wrote: > This histogram does not measure ad frames, right? Done. https://codereview.chromium.org/2798953002/diff/580001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46906: + units="DidNavigateToAd"> On 2017/04/27 00:25:28, Ilya Sherman wrote: > nit: s/units/enum Done. https://codereview.chromium.org/2798953002/diff/580001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46913: + like the rest of the Clients.Ads metrics (unless otherwise specified). On 2017/04/27 00:25:28, Ilya Sherman wrote: > I find this sentence somewhat confusing. I'd rephrase it as something like the > following (of course, feel free to refine further!): > > This metric is recorded as the event happens. Note that this is unlike most > other Clients.Ads metrics, which are recorded when the page load is > complete. Done. https://codereview.chromium.org/2798953002/diff/580001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46937: + The number of frames frames (with parent frame of main frame) on the page. On 2017/04/27 00:25:29, Ilya Sherman wrote: > nit: s/frames frames/frames Done. https://codereview.chromium.org/2798953002/diff/580001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46941: + include header bytes. An ad frame consists of the identified ad frame and On 2017/04/27 00:25:28, Ilya Sherman wrote: > This histogram does not measure bytes, nor does it measure ad frames (right?) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! Metrics LGTM. That said, please wait for final confirmation from the privacy team prior to landing this change. https://codereview.chromium.org/2798953002/diff/620001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2798953002/diff/620001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46789: + ancestor ad frame). Optional: Note that this paragraph is repeated verbatim across eight histograms; and the first two sentences across two additional ones as well. I think that histogram_suffixes could help reduce this repetition while still giving the metrics a clear structure. That said, if you prefer to duplicate the text, that's fine too.
The CQ bit was checked by jkarlin@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...
Thanks Ilya! And thanks to all of the reviewers. https://codereview.chromium.org/2798953002/diff/620001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2798953002/diff/620001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46789: + ancestor ad frame). On 2017/04/28 22:57:19, Ilya Sherman wrote: > Optional: Note that this paragraph is repeated verbatim across eight histograms; > and the first two sentences across two additional ones as well. I think that > histogram_suffixes could help reduce this repetition while still giving the > metrics a clear structure. That said, if you prefer to duplicate the text, > that's fine too. Acknowledged.
Description was changed from ========== [PageLoadMetrics] Keep track of Ad Sizes on Pages This CL adds a PageLoadObserver that keeps track of frames with ads and them and reports their byte usage. As part of the work, the following additional changes were necessary: 1) Frame Tree ID and URL are added to extra request info 2) Add GetRenderFrameHostGetterForRequest to ResourceRequestInfo so that ChromeResourceDispatcherHostDelegate can get at the frame tree node id of the resource. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation BUG=708570 ========== to ========== [PageLoadMetrics] Keep track of Ad Sizes on Pages In order to help us understand the resource footprint that ads have on webpages, we need to measure things like their network and cache utilizations. This CL adds a PageLoadObserver that keeps track of frames with ads and reports statistics on the number of ad frames found on the page, the size of the ad frames, and the percentage that came from cache vs network. As part of the work, the following additional changes were necessary: 1) Frame Tree ID and URL are added to extra request info 2) Add GetRenderFrameHostGetterForRequest to ResourceRequestInfo so that ChromeResourceDispatcherHostDelegate can get at the frame tree node id of the resource. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation BUG=708570 ==========
Description was changed from ========== [PageLoadMetrics] Keep track of Ad Sizes on Pages In order to help us understand the resource footprint that ads have on webpages, we need to measure things like their network and cache utilizations. This CL adds a PageLoadObserver that keeps track of frames with ads and reports statistics on the number of ad frames found on the page, the size of the ad frames, and the percentage that came from cache vs network. As part of the work, the following additional changes were necessary: 1) Frame Tree ID and URL are added to extra request info 2) Add GetRenderFrameHostGetterForRequest to ResourceRequestInfo so that ChromeResourceDispatcherHostDelegate can get at the frame tree node id of the resource. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation BUG=708570 ========== to ========== [PageLoadMetrics] Keep track of Ad Sizes on Pages In order to help us understand the resource footprint that ads have on webpages, we need to measure things like their network and cache utilizations. This CL adds a PageLoadObserver that keeps track of frames with ads and reports statistics on the number of ad frames found on the page, the size of the ad frames, and the percentage that came from cache vs network. As part of the work, the following additional changes were necessary: 1) Frame Tree ID and URL are added to extra request info 2) Add GetRenderFrameHostGetterForRequest to ResourceRequestInfo so that ChromeResourceDispatcherHostDelegate can get at the frame tree node id of the resource. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation BUG=708570 ==========
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 jkarlin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org, jam@chromium.org, bmcquade@chromium.org, csharrison@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2798953002/#ps640001 (title: "Rebase")
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": 640001, "attempt_start_ts": 1493752870003390,
"parent_rev": "25a65028b3fd10ffe5850daf25187e4d5d930a66", "commit_rev":
"6616a885b4f18edd4f6aa178146c14538ca1d429"}
Message was sent while issue was closed.
Description was changed from ========== [PageLoadMetrics] Keep track of Ad Sizes on Pages In order to help us understand the resource footprint that ads have on webpages, we need to measure things like their network and cache utilizations. This CL adds a PageLoadObserver that keeps track of frames with ads and reports statistics on the number of ad frames found on the page, the size of the ad frames, and the percentage that came from cache vs network. As part of the work, the following additional changes were necessary: 1) Frame Tree ID and URL are added to extra request info 2) Add GetRenderFrameHostGetterForRequest to ResourceRequestInfo so that ChromeResourceDispatcherHostDelegate can get at the frame tree node id of the resource. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation BUG=708570 ========== to ========== [PageLoadMetrics] Keep track of Ad Sizes on Pages In order to help us understand the resource footprint that ads have on webpages, we need to measure things like their network and cache utilizations. This CL adds a PageLoadObserver that keeps track of frames with ads and reports statistics on the number of ad frames found on the page, the size of the ad frames, and the percentage that came from cache vs network. As part of the work, the following additional changes were necessary: 1) Frame Tree ID and URL are added to extra request info 2) Add GetRenderFrameHostGetterForRequest to ResourceRequestInfo so that ChromeResourceDispatcherHostDelegate can get at the frame tree node id of the resource. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation BUG=708570 Review-Url: https://codereview.chromium.org/2798953002 Cr-Commit-Position: refs/heads/master@{#468738} Committed: https://chromium.googlesource.com/chromium/src/+/6616a885b4f18edd4f6aa178146c... ==========
Message was sent while issue was closed.
Committed patchset #24 (id:640001) as https://chromium.googlesource.com/chromium/src/+/6616a885b4f18edd4f6aa178146c...
Message was sent while issue was closed.
Reverted in https://codereview.chromium.org/2857963002 |
