Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(8)

Issue 2833523002: Adding opt out and previews type information to DRP pingback (Closed)

Created:
7 months ago by RyanSturm
Modified:
7 months ago
CC:
chromium-reviews, subresource-filter-reviews_chromium.org, speed-metrics-reviews_chromium.org, Randy Smith (Not in Mondays), loading-reviews+metrics_chromium.org, csharrison+watch_chromium.org, loading-reviews_chromium.org, tbansal+watch-data-reduction-proxy_chromium.org, mmenke
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Adding opt out and previews type information to DRP pingback This CL adds the previews type that was shown for a page load and also reports when a page load was an opt out (clicked show original). Due to backgrounding behavior, when app background occurs or there is not a previews shown a third opt out state of unknown is reported. LoFi page state is determined if any subresources were LoFi images. Lite page uses the main resource to determine preview state. BUG=713236 Review-Url: https://codereview.chromium.org/2833523002 Cr-Commit-Position: refs/heads/master@{#466376} Committed: https://chromium.googlesource.com/chromium/src/+/6ffce94e929ce46e50e364b726e07e2b4773c82b

Patch Set 1 #

Patch Set 2 : moved a line #

Patch Set 3 : null check #

Patch Set 4 : typo #

Patch Set 5 : unit test fix #

Patch Set 6 : moved definition up #

Total comments: 43

Patch Set 7 : bengr+csharrison comments #

Total comments: 9

Patch Set 8 : rebase #

Patch Set 9 : fewer string copies and csharrison nits #

Total comments: 8

Patch Set 10 : bengr comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+818 lines, -182 lines) Patch
M chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc View 1 2 3 4 5 6 7 4 chunks +14 lines, -8 lines 0 comments Download
M chrome/browser/page_load_metrics/metrics_web_contents_observer.h View 1 2 3 4 5 6 2 chunks +10 lines, -7 lines 0 comments Download
M chrome/browser/page_load_metrics/metrics_web_contents_observer.cc View 1 2 3 4 5 6 2 chunks +4 lines, -3 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc View 1 2 3 4 5 6 7 8 9 3 chunks +13 lines, -13 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h View 1 2 3 4 5 6 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc View 1 2 3 4 5 6 7 8 9 5 chunks +13 lines, -5 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc View 1 2 3 4 5 6 7 8 9 8 chunks +56 lines, -20 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +9 lines, -9 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc View 1 2 3 4 5 6 7 2 chunks +9 lines, -4 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer_unittest.cc View 1 2 3 4 5 6 7 8 9 4 chunks +12 lines, -12 lines 0 comments Download
M chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +9 lines, -9 lines 0 comments Download
M chrome/browser/page_load_metrics/page_load_metrics_observer.h View 1 2 3 4 5 6 7 8 3 chunks +9 lines, -7 lines 0 comments Download
M chrome/browser/page_load_metrics/page_load_metrics_observer.cc View 1 2 3 4 5 6 7 8 2 chunks +10 lines, -8 lines 0 comments Download
M chrome/browser/previews/previews_infobar_delegate.cc View 1 2 3 4 5 6 7 2 chunks +17 lines, -5 lines 0 comments Download
M chrome/browser/previews/previews_infobar_delegate_unittest.cc View 1 2 3 4 5 6 7 7 chunks +102 lines, -0 lines 0 comments Download
M chrome/browser/previews/previews_infobar_tab_helper.h View 1 2 3 4 5 6 2 chunks +18 lines, -0 lines 0 comments Download
M chrome/browser/previews/previews_infobar_tab_helper.cc View 1 2 3 4 5 6 5 chunks +45 lines, -5 lines 0 comments Download
M chrome/browser/previews/previews_infobar_tab_helper_unittest.cc View 1 2 3 4 5 6 2 chunks +4 lines, -0 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h View 1 2 3 4 5 6 7 8 4 chunks +21 lines, -3 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_data.cc View 2 chunks +4 lines, -0 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc View 1 2 3 4 5 6 7 4 chunks +25 lines, -5 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h View 1 2 3 4 5 6 7 1 chunk +9 lines, -0 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc View 1 2 3 4 5 6 7 8 2 chunks +30 lines, -8 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc View 1 2 3 4 5 6 7 8 3 chunks +9 lines, -5 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h View 1 2 3 4 5 6 5 chunks +23 lines, -0 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc View 1 2 3 4 5 6 7 8 9 6 chunks +60 lines, -2 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client_unittest.cc View 1 2 3 4 5 6 7 8 9 17 chunks +246 lines, -39 lines 0 comments Download
M components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.h View 1 2 3 4 5 6 7 8 9 2 chunks +4 lines, -1 line 0 comments Download
M components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.cc View 1 2 3 4 5 6 7 8 9 2 chunks +4 lines, -2 lines 0 comments Download
M components/data_reduction_proxy/proto/pageload_metrics.proto View 2 chunks +26 lines, -0 lines 0 comments Download

Messages

Total messages: 69 (53 generated)
RyanSturm
csharrison, bengr: PTAL
7 months ago (2017-04-19 19:43:14 UTC) #18
bengr
I skimmed quickly. Seems reasonable. I'll give it a careful read soon. https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h ...
7 months ago (2017-04-20 01:02:33 UTC) #29
Charlie Harrison
https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc#newcode152 chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:152: tab_identifier_key_ = navigation_handle->GetWebContents(); This makes me really nervous. Is ...
7 months ago (2017-04-20 14:09:00 UTC) #30
RyanSturm
https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc#newcode152 chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:152: tab_identifier_key_ = navigation_handle->GetWebContents(); On 2017/04/20 14:09:00, Charlie Harrison wrote: ...
7 months ago (2017-04-20 15:37:58 UTC) #31
Charlie Harrison
https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc#newcode152 chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:152: tab_identifier_key_ = navigation_handle->GetWebContents(); On 2017/04/20 15:37:57, Ryan Sturm wrote: ...
7 months ago (2017-04-20 16:35:06 UTC) #32
bengr
https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc#newcode355 chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:355: bool was_lofi_reponse, What is a lofi response? Does this ...
7 months ago (2017-04-20 17:36:19 UTC) #33
RyanSturm
Addressed all comments (a lot were obsolete due to changing keys) jpfeiff: PTAL at pageload_metrics.proto ...
7 months ago (2017-04-20 20:25:46 UTC) #35
RyanSturm
https://codereview.chromium.org/2833523002/diff/120001/chrome/browser/page_load_metrics/page_load_metrics_observer.h File chrome/browser/page_load_metrics/page_load_metrics_observer.h (left): https://codereview.chromium.org/2833523002/diff/120001/chrome/browser/page_load_metrics/page_load_metrics_observer.h#oldcode205 chrome/browser/page_load_metrics/page_load_metrics_observer.h:205: ExtraRequestInfo(const ExtraRequestInfo& other); csharrison@, I removed this, but I ...
7 months ago (2017-04-20 20:30:32 UTC) #39
Charlie Harrison
https://codereview.chromium.org/2833523002/diff/120001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2833523002/diff/120001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc#newcode850 chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:850: if (data) Can you comment when this would be ...
7 months ago (2017-04-20 20:52:50 UTC) #42
jpfeiff
On 2017/04/20 20:25:46, Ryan Sturm wrote: > Addressed all comments (a lot were obsolete due ...
7 months ago (2017-04-20 21:14:43 UTC) #45
RyanSturm
https://codereview.chromium.org/2833523002/diff/120001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2833523002/diff/120001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc#newcode850 chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:850: if (data) On 2017/04/20 20:52:50, Charlie Harrison wrote: > ...
7 months ago (2017-04-20 21:33:01 UTC) #52
Charlie Harrison
page_load_metrics and c/b/l LGTM, but I didn't look at anything else https://codereview.chromium.org/2833523002/diff/120001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): ...
7 months ago (2017-04-20 22:00:52 UTC) #55
bengr
lgtm https://codereview.chromium.org/2833523002/diff/200001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2833523002/diff/200001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc#newcode831 chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:831: data_reduction_proxy_data = data->DeepCopy(); It's too bad you have ...
7 months ago (2017-04-20 23:19:36 UTC) #58
RyanSturm
https://codereview.chromium.org/2833523002/diff/200001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2833523002/diff/200001/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc#newcode831 chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:831: data_reduction_proxy_data = data->DeepCopy(); On 2017/04/20 23:19:35, bengr wrote: > ...
7 months ago (2017-04-21 16:25:04 UTC) #60
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2833523002/220001
7 months ago (2017-04-21 17:18:25 UTC) #66
commit-bot: I haz the power
7 months ago (2017-04-21 17:25:53 UTC) #69
Message was sent while issue was closed.
Committed patchset #10 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/6ffce94e929ce46e50e364b726e0...

Powered by Google App Engine
This is Rietveld efc10ee0f