Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2010253003:
Sending a data saver Pageload metrics pingback (Closed)
DescriptionSending a data saver Pageload metrics pingback
This sends a request to the data saver proxy server containing page load
metrics for any page loads through the data saver proxy server.
BUG=604853
Committed: https://crrev.com/5c5a6bc2a6d02710a32fc762caa782d0941fbb98
Cr-Commit-Position: refs/heads/master@{#400240}
Patch Set 1 #
Total comments: 53
Patch Set 2 : Addressing comments from tbansal #
Total comments: 46
Patch Set 3 : More fixes for tbansal comments #Patch Set 4 : Turning live pingbacks off, waiting for client config probability #
Total comments: 31
Patch Set 5 : Some fixes, verified android UMA state is consistent with the check. #
Total comments: 24
Patch Set 6 : addressing comments from tbansal #
Total comments: 8
Patch Set 7 : Added comments #Patch Set 8 : rebase #Patch Set 9 : Adding a command line switch to force pingbacks. #Patch Set 10 : Remove UMA check #
Total comments: 8
Patch Set 11 : Sending partial messages #Patch Set 12 : rebase #Patch Set 13 : Preventing pingbacks from the holdback experiment #
Total comments: 2
Patch Set 14 : Adding tamper detection experiment check #Patch Set 15 : rebase #Depends on Patchset: Messages
Total messages: 45 (14 generated)
ryansturm@chromium.org changed reviewers: + tbansal@chromium.org
tbansal: PTAL
Please split the CL as discussed offline. https://codereview.chromium.org/2010253003/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2010253003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:93: if (pingback_client_) { Where is |pingback_client_| set? Would it always be nullptr? https://codereview.chromium.org/2010253003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:97: if (!browser_context_) Move this up before you create the DataReductionProxyPageLoadTiming struct. https://codereview.chromium.org/2010253003/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h (right): https://codereview.chromium.org/2010253003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:56: std::unique_ptr<DataReductionProxyData> data_; #include <memory> https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.cc (right): https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_data.cc:23: copy->used_data_reduction_proxy_ = used_data_reduction_proxy_; nit: Use the same order as the order in which they are declared in .h file. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h (right): https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:72: std::string original_request_url_; Why not GURL? https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc (right): https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc:42: EXPECT_EQ(data->session_key(), ""); minor nits: Generally, the expected string is the first argument. Also, preferable to use std::string() instead of "". So, EXPECT_EQ(std::string(), data->session_key()); https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc:99: EXPECT_EQ(test_url, copy->original_request_url()); Reverse the order of these two statements to match the order above. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc (right): https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:63: DCHECK(source == current_fetcher_.get()); File a bug to record histograms. Then, add a TODO here referencing that bug. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:80: current_fetcher_ = CreateFetcherForData(serialized_request); DCHECK(data_to_send_.empty()); https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:86: DataReductionProxyPingbackClient::CreateFetcherForData( const method? https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:88: std::unique_ptr<net::URLFetcher> fetcher( File a bug to record histograms. Then, add a TODO here referencing that bug. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h (right): https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:27: class DataReductionProxyPingbackClient : public net::URLFetcherDelegate { Add a ThreadChecker to the class. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:31: DataReductionProxyPingbackClient( s/DataReductionProxyPingbackClient/explicit DataReductionProxyPingbackClient/ explicit is needed when the constructor takes exactly one argument. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:35: // URLFetcherDelegate s/URLFetcherDelegate/URLFetcherDelegate implementation:/ https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:36: void OnURLFetchComplete(const net::URLFetcher* source) override; I think OnURLFetchComplete can be private. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:44: net::URLFetcher* current_fetcher() const { return current_fetcher_.get(); } Why is this public? IIUC, outside classes have no need to know about the fetcher in use. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:54: // The url for the data saver proxy. s/The url for the data saver proxy./The url for the data saver proxy's ping back service./ https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:55: GURL secure_proxy_url_; const GURL https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:55: GURL secure_proxy_url_; s/secure_proxy_url_/pageload_metrics_url_/ or whatever name you decide upon. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client_unittest.cc (right): https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client_unittest.cc:42: std::unique_ptr<DataReductionProxyPingbackClient> pingback_client_; #include <memory> https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client_unittest.cc:49: std::string fake_session_key = "fake-session"; #include <string> https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.h (right): https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.h:14: base::Time navigation_start; It is preferable to have struct members as const to prevent accidental modification. Also, add constructor which takes all the values as arguments, and initializes them. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:52: const char kPageloadMetricsURL[] = I would try to be consistent in naming with the class name DataReductionProxyPingbackClient. I would suggest using PingbackURL or changing the classname to maychmatch what you have here. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h (right): https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:117: GURL GetPageloadMetricsURL(); Change the name to be consistent with names used elsewhere. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/common/data_reduction_proxy_util.h (right): https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/common/data_reduction_proxy_util.h:25: GURL AddApiKeyToUrl(const GURL& url); Can you do this in a separate CL? That CL would be easy to review, and then we can focus on pingback stuff in this CL.
On 2016/05/27 20:54:58, tbansal1 wrote: > Please split the CL as discussed offline. Split CL for the move to common: https://codereview.chromium.org/2025443002/ I'll update this CL after that lands, since most of the concerns here not fundamental issues with the CL.
https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc (right): https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:67: current_fetcher_ = CreateFetcherForData(data_to_send_.front()); This should happen only if user has opted into UMA. See the doc for more details.
https://codereview.chromium.org/2010253003/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2010253003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:93: if (pingback_client_) { On 2016/05/27 20:54:57, tbansal1 wrote: > Where is |pingback_client_| set? Would it always be nullptr? I moved the setter to private and added the unittest as a friend class. https://codereview.chromium.org/2010253003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:97: if (!browser_context_) On 2016/05/27 20:54:56, tbansal1 wrote: > Move this up before you create the DataReductionProxyPageLoadTiming struct. Done. https://codereview.chromium.org/2010253003/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h (right): https://codereview.chromium.org/2010253003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:56: std::unique_ptr<DataReductionProxyData> data_; On 2016/05/27 20:54:57, tbansal1 wrote: > #include <memory> Done. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.cc (right): https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_data.cc:23: copy->used_data_reduction_proxy_ = used_data_reduction_proxy_; On 2016/05/27 20:54:57, tbansal1 wrote: > nit: Use the same order as the order in which they are declared in .h file. Done. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h (right): https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:72: std::string original_request_url_; On 2016/05/27 20:54:57, tbansal1 wrote: > Why not GURL? Seemed like more than was needed; I suspect a string is good enough for now, and when the full page size detector is built, this likely can be removed here. In fact, once the page size detector is built, I suspect that plumbing path through content won't be strictly needed. I don't mind changing it to a GURL though, so that works for me. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc (right): https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc:42: EXPECT_EQ(data->session_key(), ""); On 2016/05/27 20:54:57, tbansal1 wrote: > minor nits: Generally, the expected string is the first argument. Also, > preferable to use std::string() instead of "". So, > EXPECT_EQ(std::string(), data->session_key()); Done. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc:99: EXPECT_EQ(test_url, copy->original_request_url()); On 2016/05/27 20:54:57, tbansal1 wrote: > Reverse the order of these two statements to match the order above. Done. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc (right): https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:63: DCHECK(source == current_fetcher_.get()); On 2016/05/27 20:54:57, tbansal1 wrote: > File a bug to record histograms. Then, add a TODO here referencing that bug. Done. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:67: current_fetcher_ = CreateFetcherForData(data_to_send_.front()); On 2016/05/31 16:11:46, tbansal1 wrote: > This should happen only if user has opted into UMA. See the doc for more > details. Added this check in the observer. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:80: current_fetcher_ = CreateFetcherForData(serialized_request); On 2016/05/27 20:54:57, tbansal1 wrote: > DCHECK(data_to_send_.empty()); Done. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:86: DataReductionProxyPingbackClient::CreateFetcherForData( On 2016/05/27 20:54:57, tbansal1 wrote: > const method? It passes this into Create, which needs to be non-const. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:88: std::unique_ptr<net::URLFetcher> fetcher( On 2016/05/27 20:54:57, tbansal1 wrote: > File a bug to record histograms. Then, add a TODO here referencing that bug. Done. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h (right): https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:27: class DataReductionProxyPingbackClient : public net::URLFetcherDelegate { On 2016/05/27 20:54:57, tbansal1 wrote: > Add a ThreadChecker to the class. Done. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:31: DataReductionProxyPingbackClient( On 2016/05/27 20:54:57, tbansal1 wrote: > s/DataReductionProxyPingbackClient/explicit DataReductionProxyPingbackClient/ > > explicit is needed when the constructor takes exactly one argument. Done. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:35: // URLFetcherDelegate On 2016/05/27 20:54:58, tbansal1 wrote: > s/URLFetcherDelegate/URLFetcherDelegate implementation:/ Done. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:36: void OnURLFetchComplete(const net::URLFetcher* source) override; On 2016/05/27 20:54:57, tbansal1 wrote: > I think OnURLFetchComplete can be private. OnUrlFetchComplete is called by UrlFetcherCore, so it should be public. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:44: net::URLFetcher* current_fetcher() const { return current_fetcher_.get(); } On 2016/05/27 20:54:57, tbansal1 wrote: > Why is this public? IIUC, outside classes have no need to know about the fetcher > in use. Done. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:54: // The url for the data saver proxy. On 2016/05/27 20:54:57, tbansal1 wrote: > s/The url for the data saver proxy./The url for the data saver proxy's ping back > service./ Done. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:55: GURL secure_proxy_url_; On 2016/05/27 20:54:57, tbansal1 wrote: > const GURL Done. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:55: GURL secure_proxy_url_; On 2016/05/27 20:54:57, tbansal1 wrote: > const GURL Done. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client_unittest.cc (right): https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client_unittest.cc:42: std::unique_ptr<DataReductionProxyPingbackClient> pingback_client_; On 2016/05/27 20:54:58, tbansal1 wrote: > #include <memory> Done. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client_unittest.cc:49: std::string fake_session_key = "fake-session"; On 2016/05/27 20:54:58, tbansal1 wrote: > #include <string> Done. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.h (right): https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.h:14: base::Time navigation_start; On 2016/05/27 20:54:58, tbansal1 wrote: > It is preferable to have struct members as const to prevent accidental > modification. Also, add constructor which takes all the values as arguments, and > initializes them. Done. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:52: const char kPageloadMetricsURL[] = On 2016/05/27 20:54:58, tbansal1 wrote: > I would try to be consistent in naming with the class name > DataReductionProxyPingbackClient. I would suggest using PingbackURL or changing > the classname to maychmatch what you have here. Done. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h (right): https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:117: GURL GetPageloadMetricsURL(); On 2016/05/27 20:54:58, tbansal1 wrote: > Change the name to be consistent with names used elsewhere. Done. https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/common/data_reduction_proxy_util.h (right): https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/common/data_reduction_proxy_util.h:25: GURL AddApiKeyToUrl(const GURL& url); On 2016/05/27 20:54:58, tbansal1 wrote: > Can you do this in a separate CL? That CL would be easy to review, and then we > can focus on pingback stuff in this CL. Done.
https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h (right): https://codereview.chromium.org/2010253003/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:36: void OnURLFetchComplete(const net::URLFetcher* source) override; On 2016/06/01 22:19:10, RyanSturm wrote: > On 2016/05/27 20:54:57, tbansal1 wrote: > > I think OnURLFetchComplete can be private. > > OnUrlFetchComplete is called by UrlFetcherCore, so it should be public. I thought overriding private functions can still be called as long as the base class has declared that function as public. e.g., see https://code.google.com/p/chromium/codesearch#chromium/src/blimp/client/sessi... https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:81: if (!WasStartedInForegroundEventInForeground(timing.load_event_start, info)) { braces not needed. https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:89: if (pingback_client_) { If you add GetPingbackClient() this code would be easier to read. Right now, it is unclear why pingback_client_ is not being set equal to settings->data_reduction_proxy_service()->pingback_client(). https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:96: if (!settings->IsMetricsAndCrashReportingEnabled()) you can move this up so that function returns early if this check fails. https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:107: info)) Add braces since if conditional spans multiple lines. https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h (right): https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:55: void set_pingback_client(DataReductionProxyPingbackClient* pingback_client) { If a function is strictly used for testing, it is preferable if its name has suffix "ForTesting". This helps in readability, and there is also a presubmit check which ensures that ForTesting() functions are not called from non-testing code. https://code.google.com/p/chromium/codesearch#chromium/src/PRESUBMIT.py&l=349 https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:66: DataReductionProxyPingbackClient* pingback_client_; Instead of having a private member that is only set in tests, I would rather add a virtual function: DataReductionProxyPingbackClient* GetPingbackClient(). In the regular code, it will return the pingback client from settings->data_reduction_proxy_service()->pingback_client(). In tests, this function would be overriden, and would return something else. https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:57: data_reduction_proxy::DataReductionProxyPageLoadTiming* timing() { const function. https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:66: bool send_pingback_called_; All your test classes should also contain DISALLOW_COPY_AND_ASSIGN. https://groups.google.com/a/chromium.org/d/msg/chromium-dev/eT-8FIcgRq8/tRqrC... https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:73: class DataReductionProxyMetricsObserverWrapper Add class comments. https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:73: class DataReductionProxyMetricsObserverWrapper s/DataReductionProxyMetricsObserverWrapper/PageLoadMetricsObserverTest/? Essentially, the class name should contain test somewhere so when you friend it, it is clear from the .h file that the friend class is a test class. e.g., https://code.google.com/p/chromium/codesearch#search/&q=f:net%20f:h$%20friend... https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:197: TEST_F(DataReductionProxyMetricsObserverTest, CorrectDataToPingbackClient) { nit: It would be nice if each of the test had some comment indicating what exactly it is testing. https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:270: TEST_F(DataReductionProxyMetricsObserverTest, Missing Linebreak. https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h (right): https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:45: void set_original_request_url(GURL original_request_url) { pass by reference? https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:69: // The session key used for this request. s/request/request or navigation/ https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc (right): https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc:89: for (size_t i = 0; i < 2; ++i) { Why is this running only on the first two datasets? https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc:90: std::string session_key = "test-key"; static const char kSessionKey[] (Not sure if this will work). https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc:91: GURL test_url("test-url"); static const GURL kTestURL https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc (right): https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:45: request->set_first_request_url(request_data.original_request_url().spec()); Do you need to check if the URL is valid before calling spec()? https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:60: DataReductionProxyPingbackClient::~DataReductionProxyPingbackClient() {} Add thread checker in the destructor too. https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h (right): https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:46: // |url_request_context|. The max retires is set to 5. s/|url_request_context|/|url_request_context_| https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:52: // The url for the data saver proxy's ping back service. s/url/URL/ https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.h (right): https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.h:13: DataReductionProxyPageLoadTiming(base::Time navigation_start, pass all these as const refs?
https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc (right): https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:90: DataReductionProxyPingbackClient::CreateFetcherForData( You can change this to void MaybeCreateFetcherForDataAndStart(const std::string& request_data) Two advantages: You can move the setting of create_fetcher_ and current_fetcher_->Start() both inside this function which will remove code duplication. Also, you can control (at one single place) if the fetcher needs to be created or not based on random number. Background: The design doc from buettner says that the probability of pingback would be controlled using field trial parameters.
https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:81: if (!WasStartedInForegroundEventInForeground(timing.load_event_start, info)) { On 2016/06/01 23:36:17, tbansal1 wrote: > braces not needed. Done. https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:89: if (pingback_client_) { On 2016/06/01 23:36:17, tbansal1 wrote: > If you add GetPingbackClient() this code would be easier to read. Right now, it > is unclear why pingback_client_ is not being set equal to > settings->data_reduction_proxy_service()->pingback_client(). Done. https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:96: if (!settings->IsMetricsAndCrashReportingEnabled()) On 2016/06/01 23:36:17, tbansal1 wrote: > you can move this up so that function returns early if this check fails. Done. https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:107: info)) On 2016/06/01 23:36:17, tbansal1 wrote: > Add braces since if conditional spans multiple lines. Done. https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h (right): https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:55: void set_pingback_client(DataReductionProxyPingbackClient* pingback_client) { On 2016/06/01 23:36:17, tbansal1 wrote: > If a function is strictly used for testing, it is preferable if its name has > suffix "ForTesting". This helps in readability, and there is also a presubmit > check which ensures that ForTesting() functions are not called from non-testing > code. > https://code.google.com/p/chromium/codesearch#chromium/src/PRESUBMIT.py&l=349 I switched the test to override two helper methods, so this is cleaner. https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:66: DataReductionProxyPingbackClient* pingback_client_; On 2016/06/01 23:36:17, tbansal1 wrote: > Instead of having a private member that is only set in tests, I would rather add > a virtual function: > DataReductionProxyPingbackClient* GetPingbackClient(). > In the regular code, it will return the pingback client from > settings->data_reduction_proxy_service()->pingback_client(). > In tests, this function would be overriden, and would return something else. Done. https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:57: data_reduction_proxy::DataReductionProxyPageLoadTiming* timing() { On 2016/06/01 23:36:17, tbansal1 wrote: > const function. Done. https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:66: bool send_pingback_called_; On 2016/06/01 23:36:17, tbansal1 wrote: > All your test classes should also contain DISALLOW_COPY_AND_ASSIGN. > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/eT-8FIcgRq8/tRqrC... Done. https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:73: class DataReductionProxyMetricsObserverWrapper On 2016/06/01 23:36:17, tbansal1 wrote: > s/DataReductionProxyMetricsObserverWrapper/PageLoadMetricsObserverTest/? > Essentially, the class name should contain test somewhere so when you friend it, > it is clear from the .h file that the friend class is a test class. e.g., > https://code.google.com/p/chromium/codesearch#search/&q=f:net%20f:h$%20friend... Done. https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:73: class DataReductionProxyMetricsObserverWrapper On 2016/06/01 23:36:17, tbansal1 wrote: > Add class comments. Done. https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:197: TEST_F(DataReductionProxyMetricsObserverTest, CorrectDataToPingbackClient) { On 2016/06/01 23:36:17, tbansal1 wrote: > nit: It would be nice if each of the test had some comment indicating what > exactly it is testing. Done. https://codereview.chromium.org/2010253003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:270: TEST_F(DataReductionProxyMetricsObserverTest, On 2016/06/01 23:36:17, tbansal1 wrote: > Missing Linebreak. Done. https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h (right): https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:45: void set_original_request_url(GURL original_request_url) { On 2016/06/01 23:36:17, tbansal1 wrote: > pass by reference? Done. https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:69: // The session key used for this request. On 2016/06/01 23:36:17, tbansal1 wrote: > s/request/request or navigation/ Done. https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc (right): https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc:89: for (size_t i = 0; i < 2; ++i) { On 2016/06/01 23:36:17, tbansal1 wrote: > Why is this running only on the first two datasets? Done. https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc:90: std::string session_key = "test-key"; On 2016/06/01 23:36:18, tbansal1 wrote: > static const char kSessionKey[] > (Not sure if this will work). Done. https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc:91: GURL test_url("test-url"); On 2016/06/01 23:36:18, tbansal1 wrote: > static const GURL kTestURL Done. https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc (right): https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:45: request->set_first_request_url(request_data.original_request_url().spec()); On 2016/06/01 23:36:18, tbansal1 wrote: > Do you need to check if the URL is valid before calling spec()? Done. https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:60: DataReductionProxyPingbackClient::~DataReductionProxyPingbackClient() {} On 2016/06/01 23:36:18, tbansal1 wrote: > Add thread checker in the destructor too. Done. https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:90: DataReductionProxyPingbackClient::CreateFetcherForData( On 2016/06/02 16:50:57, tbansal1 wrote: > You can change this to void MaybeCreateFetcherForDataAndStart(const std::string& > request_data) > Two advantages: > You can move the setting of create_fetcher_ and current_fetcher_->Start() both > inside this function which will remove code duplication. > Also, you can control (at one single place) if the fetcher needs to be created > or not based on random number. Background: The design doc from buettner says > that the probability of pingback would be controlled using field trial > parameters. Done. https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h (right): https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:46: // |url_request_context|. The max retires is set to 5. On 2016/06/01 23:36:18, tbansal1 wrote: > s/|url_request_context|/|url_request_context_| Done. https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:52: // The url for the data saver proxy's ping back service. On 2016/06/01 23:36:18, tbansal1 wrote: > s/url/URL/ Done. https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.h (right): https://codereview.chromium.org/2010253003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.h:13: DataReductionProxyPageLoadTiming(base::Time navigation_start, On 2016/06/01 23:36:18, tbansal1 wrote: > pass all these as const refs? Done.
https://codereview.chromium.org/2010253003/diff/60001/chrome/browser/net/spdy... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h (right): https://codereview.chromium.org/2010253003/diff/60001/chrome/browser/net/spdy... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:77: static bool IsMetricsAndCrashReportingEnabled(); Add a comment which addresses: (i) What happens if the user changes the setting. Would this return false immediately or would it require Chrome restart (ii) how does it play with the connection type? https://codereview.chromium.org/2010253003/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2010253003/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:119: return DataReductionProxyChromeSettingsFactory::GetForBrowserContext( Is it possible to access ChromeMetricsServiceAccessor::IsMetricsAndCrashReportingEnabled(); directly here instead of going through DRP settings? https://codereview.chromium.org/2010253003/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:120: browser_context_) Can you check with somebody on metrics team to make sure that this is the right check to make. https://codereview.chromium.org/2010253003/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:121: ->IsMetricsAndCrashReportingEnabled(); Can you manually check to make sure that this behaves as intended on Chrome on Android and also on desktop Chrome with multiple profiles? For Android also check that if the user turns off UMA upload, then that disables pingbacks immediately without requiring Chrome to restart. Third, make sure that the upload only on Wi-Fi setting plays well with this setting. You may have to disable Wi-Fi on your device to test this. https://codereview.chromium.org/2010253003/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h (right): https://codereview.chromium.org/2010253003/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:53: virtual DataReductionProxyPingbackClient* GetPingbackClient() const; Say "Virtualized for testing" in the comments for this function, and the one below it. https://codereview.chromium.org/2010253003/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2010253003/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:145: DISALLOW_COPY_AND_ASSIGN(DataReductionProxyMetricsObserverTest); Move DISALLOW_COPY_AND_ASSIGN(DataReductionProxyMetricsObserverTest); to private: https://codereview.chromium.org/2010253003/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:344: SendPingbackNotCalledWhenNotProxied) { Is it possible to combine this test with the previous one? Seems like the two tests are very similar. If it is not possible, an alternative way may be to add a function RunTestWithParams() to DataReductionProxyMetricsObserverTest. This function can take params like is drp enabled, is reporting enabled, is FCP zero, and what is the expected results. Then, the tests can be simplified to simply calling RunTestWithParams(). See an example here: https://cs.chromium.org/chromium/src/components/data_reduction_proxy/core/bro... https://codereview.chromium.org/2010253003/diff/60001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/2010253003/diff/60001/components/components_t... components/components_tests.gyp:92: 'browsing_data_ui_unittest_sources': [ It is better not to rebase and make modifications to your code in the same patchset. You can make modifcations to your code in one patch, upload that. Then rebase, then upload the rebased patch (or do it in the other order). This makes it easier to review especially if the CL is large. https://codereview.chromium.org/2010253003/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2010253003/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:7: #include <string> This is already included in .h file. So, no need to include here. (I might have asked you to include this, in that case, my bad). https://codereview.chromium.org/2010253003/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc (right): https://codereview.chromium.org/2010253003/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:484: EXPECT_EQ(data->original_request_url(), GURL("http://www.google.com/")); nit: Expected value should be the first argument. Observed value should be second. So, may be swap these two. Same for the statement below. https://codereview.chromium.org/2010253003/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc (right): https://codereview.chromium.org/2010253003/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:45: if (request_data.original_request_url().is_valid()) { braces not needed. https://codereview.chromium.org/2010253003/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:72: while (!current_fetcher_) { simplify: while(!current_fetcher_ && !data_to_send_.empty()) https://codereview.chromium.org/2010253003/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:84: std::string serialized_request = SerializeData(request_data, timing); Is it possible that |request_data| is empty or not set? In that case, we should not send pingback. Similarly, if the URL request is invalid, may be we should not send pingback? https://codereview.chromium.org/2010253003/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:94: DataReductionProxyPingbackClient::MaybeCreateFetcherForDataAndStart( This can probably be a const function. https://codereview.chromium.org/2010253003/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h (right): https://codereview.chromium.org/2010253003/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:45: // Whether a pingback should be sent. Virtualized for testing.
https://codereview.chromium.org/2010253003/diff/60001/chrome/browser/net/spdy... File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h (right): https://codereview.chromium.org/2010253003/diff/60001/chrome/browser/net/spdy... chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h:77: static bool IsMetricsAndCrashReportingEnabled(); On 2016/06/02 22:56:37, tbansal1 wrote: > Add a comment which addresses: (i) What happens if the user changes the setting. > Would this return false immediately or would it require Chrome restart (ii) how > does it play with the connection type? Done. https://codereview.chromium.org/2010253003/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2010253003/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:119: return DataReductionProxyChromeSettingsFactory::GetForBrowserContext( On 2016/06/02 22:56:37, tbansal1 wrote: > Is it possible to access > ChromeMetricsServiceAccessor::IsMetricsAndCrashReportingEnabled(); directly here > instead of going through DRP settings? I considered it, but I didn't want to add this as a friend class to ChromeMetricsServiceAccessor... For some reason they want private methods with friend classes... https://codereview.chromium.org/2010253003/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:120: browser_context_) On 2016/06/02 22:56:37, tbansal1 wrote: > Can you check with somebody on metrics team to make sure that this is the right > check to make. Done. https://codereview.chromium.org/2010253003/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:121: ->IsMetricsAndCrashReportingEnabled(); On 2016/06/02 22:56:37, tbansal1 wrote: > Can you manually check to make sure that this behaves as intended on Chrome on > Android and also on desktop Chrome with multiple profiles? This works as expected on android. Building an official branded desktop that doesn't crash has been difficult, but I'll trust the guidance from the metrics team and my reading of the desktop code to say that it is based on a pref that is changed here: https://cs.chromium.org/chromium/src/chrome/browser/resources/options/browser... > For Android also check that if the user turns off UMA upload, then that disables > pingbacks immediately without requiring Chrome to restart. Works as expected. > Third, make sure that the upload only on Wi-Fi setting plays well with this > setting. You may have to disable Wi-Fi on your device to test this. Works as expected. https://codereview.chromium.org/2010253003/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h (right): https://codereview.chromium.org/2010253003/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:53: virtual DataReductionProxyPingbackClient* GetPingbackClient() const; On 2016/06/02 22:56:37, tbansal1 wrote: > Say "Virtualized for testing" in the comments for this function, and the one > below it. Done. https://codereview.chromium.org/2010253003/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2010253003/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:145: DISALLOW_COPY_AND_ASSIGN(DataReductionProxyMetricsObserverTest); On 2016/06/02 22:56:38, tbansal1 wrote: > Move DISALLOW_COPY_AND_ASSIGN(DataReductionProxyMetricsObserverTest); to > private: Done. https://codereview.chromium.org/2010253003/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:344: SendPingbackNotCalledWhenNotProxied) { On 2016/06/02 22:56:38, tbansal1 wrote: > Is it possible to combine this test with the previous one? Seems like the two > tests are very similar. If it is not possible, an alternative way may be to add > a function RunTestWithParams() to DataReductionProxyMetricsObserverTest. This > function can take params like is drp enabled, is reporting enabled, is FCP zero, > and what is the expected results. Then, the tests can be simplified to simply > calling RunTestWithParams(). > > See an example here: > https://cs.chromium.org/chromium/src/components/data_reduction_proxy/core/bro... I collapsed the common code to a RunTest and a ResetTest functionality, and grouped the pingback tests into one test. Due to the nature of how timing is changed, I didn't want to pass that information as a parameter, so it just made sense to call ResetTest then change state, then RunTest. https://codereview.chromium.org/2010253003/diff/60001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/2010253003/diff/60001/components/components_t... components/components_tests.gyp:92: 'browsing_data_ui_unittest_sources': [ On 2016/06/02 22:56:38, tbansal1 wrote: > It is better not to rebase and make modifications to your code in the same > patchset. You can make modifcations to your code in one patch, upload that. Then > rebase, then upload the rebased patch (or do it in the other order). This makes > it easier to review especially if the CL is large. Sorry, I meant to upload after I rebased, but must have cancelled the upload somehow. https://codereview.chromium.org/2010253003/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2010253003/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:7: #include <string> On 2016/06/02 22:56:38, tbansal1 wrote: > This is already included in .h file. So, no need to include here. (I might have > asked you to include this, in that case, my bad). Done. https://codereview.chromium.org/2010253003/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc (right): https://codereview.chromium.org/2010253003/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:484: EXPECT_EQ(data->original_request_url(), GURL("http://www.google.com/")); On 2016/06/02 22:56:38, tbansal1 wrote: > nit: Expected value should be the first argument. Observed value should be > second. So, may be swap these two. Same for the statement below. Done. https://codereview.chromium.org/2010253003/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc (right): https://codereview.chromium.org/2010253003/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:45: if (request_data.original_request_url().is_valid()) { On 2016/06/02 22:56:38, tbansal1 wrote: > braces not needed. Done. https://codereview.chromium.org/2010253003/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:72: while (!current_fetcher_) { On 2016/06/02 22:56:38, tbansal1 wrote: > simplify: > while(!current_fetcher_ && !data_to_send_.empty()) Done. https://codereview.chromium.org/2010253003/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:84: std::string serialized_request = SerializeData(request_data, timing); On 2016/06/02 22:56:38, tbansal1 wrote: > Is it possible that |request_data| is empty or not set? In that case, we should > not send pingback. Similarly, if the URL request is invalid, may be we should > not send pingback? It should not be possible, but there's a lot of steps involved here. This URLRequest succeeded to get to this point, and if it is proxied through data saver, then there shouldn't be a way that this is null (barring memory allocation issues). I believe the if (!data_ || !data_->used_data_reduction_proxy()) check in OnComplete of the observer is sufficient to verify the |request_data| for the pingback. If for some reason we see a large number of 0 session keys, or empty url strings, that would inform us more than not sending the pingback. https://codereview.chromium.org/2010253003/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:94: DataReductionProxyPingbackClient::MaybeCreateFetcherForDataAndStart( On 2016/06/02 22:56:38, tbansal1 wrote: > This can probably be a const function. The |this| parameter in URLFetcher::Create is non-const. https://codereview.chromium.org/2010253003/diff/60001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h (right): https://codereview.chromium.org/2010253003/diff/60001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:45: // Whether a pingback should be sent. On 2016/06/02 22:56:38, tbansal1 wrote: > Virtualized for testing. Done.
This time I looked at the tests too. Most of the comments are nits. https://codereview.chromium.org/2010253003/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2010253003/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:119: return DataReductionProxyChromeSettingsFactory::GetForBrowserContext( On 2016/06/03 21:20:50, RyanSturm wrote: > On 2016/06/02 22:56:37, tbansal1 wrote: > > Is it possible to access > > ChromeMetricsServiceAccessor::IsMetricsAndCrashReportingEnabled(); directly > here > > instead of going through DRP settings? > > I considered it, but I didn't want to add this as a friend class to > ChromeMetricsServiceAccessor... For some reason they want private methods with > friend classes... Acknowledged. https://codereview.chromium.org/2010253003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2010253003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:67: if (!WasStartedInForegroundEventInForeground(timing.response_start, info)) Can you add comments to describe why there is a return if this conditional is false. Similarly for other if-conditionals below. https://codereview.chromium.org/2010253003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h (right): https://codereview.chromium.org/2010253003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:52: // Gets the default |DataReductionProxyPingbackClient|. Virtualized in nit: No need to put pipes around CamelCase names. https://codereview.chromium.org/2010253003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2010253003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:135: reporting_enabled_(true) {} This is slightly confusing that data_reduction_proxy_used_ is initialized to false, but reporting_enabled_ to true. One way to fix it might be to init all of them to false, and then add 3 booleans as arguments to ResetTest. Then, you can also chnage the 3 variables from protected to private :) https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h (right): https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:41: void set_session_key(std::string session_key) { session_key_ = session_key; } pass by const ref? https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc (right): https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:466: { Not related to this CL but this parenthesis seems redundant. https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc (right): https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:23: std::string SerializeData(const DataReductionProxyData& request_data, Add function comment. https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:36: request->set_allocated_time_to_first_byte( Do you need to check if these timings are also non-zero? https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:47: request->set_session_key(request_data.session_key()); Minor nit: Can you serialize them in the order they are specified in the proto. That will make it clearer as to what fields are currently populated. https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h (right): https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:27: // server. This object is only created on the UI thread. Re: This object is only created on the UI thread. Is this really necessary? May be you want to say that "This class is not thread safe". The latter is more flexible than the former. https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client_unittest.cc (right): https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client_unittest.cc:38: It seems that you can add a function here: void CreateAndSendAPingback() { DataReductionProxyData request_data; std::string fake_session_key = "fake-session"; GURL fake_url("http://www.google.com"); request_data.set_session_key(fake_session_key); request_data.set_original_request_url(fake_url); DataReductionProxyPageLoadTiming timing( base::Time::FromJsTime(1500), base::TimeDelta::FromMilliseconds(1600), base::TimeDelta::FromMilliseconds(1700), base::TimeDelta::FromMilliseconds(1800), base::TimeDelta::FromMilliseconds(1900)); SendPingback(request_data, timing); } Then, you can remove this common code from all three tests below. https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client_unittest.cc:73: std::string fake_session_key = "fake-session"; These are static const variables. So, static const char kSessionKey[] Similarly for the fake url below. Also, since these are used in multiple tests, you can move to an anonymous namespace at the top. https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.h (right): https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.h:12: struct DataReductionProxyPageLoadTiming { Add a struct comment.
https://codereview.chromium.org/2010253003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2010253003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:67: if (!WasStartedInForegroundEventInForeground(timing.response_start, info)) On 2016/06/03 23:28:18, tbansal1 wrote: > Can you add comments to describe why there is a return if this conditional is > false. Similarly for other if-conditionals below. I'm going to add a general comment at the top. https://codereview.chromium.org/2010253003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h (right): https://codereview.chromium.org/2010253003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:52: // Gets the default |DataReductionProxyPingbackClient|. Virtualized in On 2016/06/03 23:28:18, tbansal1 wrote: > nit: No need to put pipes around CamelCase names. Done. https://codereview.chromium.org/2010253003/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2010253003/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:135: reporting_enabled_(true) {} On 2016/06/03 23:28:18, tbansal1 wrote: > This is slightly confusing that data_reduction_proxy_used_ is initialized to > false, but reporting_enabled_ to true. One way to fix it might be to init all of > them to false, and then add 3 booleans as arguments to ResetTest. > > Then, you can also chnage the 3 variables from protected to private :) Done. https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h (right): https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:41: void set_session_key(std::string session_key) { session_key_ = session_key; } On 2016/06/03 23:28:18, tbansal1 wrote: > pass by const ref? Done. https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc (right): https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:466: { On 2016/06/03 23:28:19, tbansal1 wrote: > Not related to this CL but this parenthesis seems redundant. Done. https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc (right): https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:23: std::string SerializeData(const DataReductionProxyData& request_data, On 2016/06/03 23:28:19, tbansal1 wrote: > Add function comment. Done. https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:36: request->set_allocated_time_to_first_byte( On 2016/06/03 23:28:19, tbansal1 wrote: > Do you need to check if these timings are also non-zero? No. In the observer, there is already a check. Image paints are special in that there can be a page with no images. https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:47: request->set_session_key(request_data.session_key()); On 2016/06/03 23:28:19, tbansal1 wrote: > Minor nit: Can you serialize them in the order they are specified in the proto. > That will make it clearer as to what fields are currently populated. Done. https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h (right): https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:27: // server. This object is only created on the UI thread. On 2016/06/03 23:28:19, tbansal1 wrote: > Re: This object is only created on the UI thread. > Is this really necessary? > > May be you want to say that > "This class is not thread safe". > The latter is more flexible than the former. Done. https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client_unittest.cc (right): https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client_unittest.cc:38: On 2016/06/03 23:28:19, tbansal1 wrote: > It seems that you can add a function here: > void CreateAndSendAPingback() { > DataReductionProxyData request_data; > std::string fake_session_key = "fake-session"; > GURL fake_url("http://www.google.com"); > request_data.set_session_key(fake_session_key); > request_data.set_original_request_url(fake_url); > > DataReductionProxyPageLoadTiming timing( > base::Time::FromJsTime(1500), base::TimeDelta::FromMilliseconds(1600), > base::TimeDelta::FromMilliseconds(1700), > base::TimeDelta::FromMilliseconds(1800), > base::TimeDelta::FromMilliseconds(1900)); > SendPingback(request_data, timing); > } > > Then, you can remove this common code from all three tests below. Done. https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client_unittest.cc:73: std::string fake_session_key = "fake-session"; On 2016/06/03 23:28:19, tbansal1 wrote: > These are static const variables. So, > static const char kSessionKey[] > Similarly for the fake url below. > > Also, since these are used in multiple tests, you can move to an anonymous > namespace at the top. Done. https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... File components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.h (right): https://codereview.chromium.org/2010253003/diff/80001/components/data_reducti... components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.h:12: struct DataReductionProxyPageLoadTiming { On 2016/06/03 23:28:19, tbansal1 wrote: > Add a struct comment. Done.
lgtm
The CQ bit was checked by ryansturm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2010253003/100001
The CQ bit was unchecked by ryansturm@chromium.org
ryansturm@chromium.org changed reviewers: + asvitkine@chromium.org, csharrison@chromium.org
csharrison: ptal @ chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer* asvitkine: ptal @ IsMetricsAndCrashReportingEnabled usage in data_reduction_proxy_chrome_settings.cc / data_reduction_proxy_metrics_observer.cc
Looks okay, though I am concerned about the raw browser_context_ pointer during WebContents shutdown. https://codereview.chromium.org/2010253003/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2010253003/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:71: if (!WasStartedInForegroundEventInForeground(timing.response_start, info)) Why is this conditional needed? Seems like the FCP variant captures this too. https://codereview.chromium.org/2010253003/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h (right): https://codereview.chromium.org/2010253003/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:52: // Gets the default DataReductionProxyPingbackClient. Virtualized in testing. Replace "virtualized" with "overriden" as it's more clear. https://codereview.chromium.org/2010253003/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:62: content::BrowserContext* browser_context_; This seems a little dangerous, given that afaik we can run code in the PLMO destructor when the web contents is invalid. Can you either: 1. Add a non-virtual web_contents() method on PLMO with a default implementation that reaches into the MetricsWebContentsObserver. 2. Make sure this can never cause use after free. Explain the reasoning in comments when you initialize the browser_context_.
https://codereview.chromium.org/2010253003/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2010253003/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:71: if (!WasStartedInForegroundEventInForeground(timing.response_start, info)) On 2016/06/06 14:00:12, csharrison wrote: > Why is this conditional needed? Seems like the FCP variant captures this too. This also checks if timing.response_start is 0, which means the timing wasn't set. https://codereview.chromium.org/2010253003/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:88: if (!IsMetricsReportingEnabled()) Remove this (and related added code) pending further privacy review. https://codereview.chromium.org/2010253003/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h (right): https://codereview.chromium.org/2010253003/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:52: // Gets the default DataReductionProxyPingbackClient. Virtualized in testing. On 2016/06/06 14:00:12, csharrison wrote: > Replace "virtualized" with "overriden" as it's more clear. Done. https://codereview.chromium.org/2010253003/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:62: content::BrowserContext* browser_context_; On 2016/06/06 14:00:12, csharrison wrote: > This seems a little dangerous, given that afaik we can run code in the PLMO > destructor when the web contents is invalid. Can you either: > 1. Add a non-virtual web_contents() method on PLMO with a default implementation > that reaches into the MetricsWebContentsObserver. > > 2. Make sure this can never cause use after free. Explain the reasoning in > comments when you initialize the browser_context_. I added a comment explaining this, and created a cl (https://codereview.chromium.org/2046713002/) to make it obvious that the lifetime of BrowserContext explicitly outlives WebContents destructor.
Thanks, this l-g-t-m except for the fact that the OnComplete call for PLMO sometimes is not triggered for a long time on some devices (it is triggered on destruction or on the next navigation commit). Is this okay for the pingback client? Otherwise you may want to move the logic to the paint / load event callback. https://codereview.chromium.org/2010253003/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h (right): https://codereview.chromium.org/2010253003/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:62: content::BrowserContext* browser_context_; On 2016/06/06 19:59:00, RyanSturm wrote: > On 2016/06/06 14:00:12, csharrison wrote: > > This seems a little dangerous, given that afaik we can run code in the PLMO > > destructor when the web contents is invalid. Can you either: > > 1. Add a non-virtual web_contents() method on PLMO with a default > implementation > > that reaches into the MetricsWebContentsObserver. > > > > 2. Make sure this can never cause use after free. Explain the reasoning in > > comments when you initialize the browser_context_. > > I added a comment explaining this, and created a cl > (https://codereview.chromium.org/2046713002/) to make it obvious that the > lifetime of BrowserContext explicitly outlives WebContents destructor. Thank you!
On 2016/06/07 13:47:20, csharrison wrote: > Thanks, this l-g-t-m except for the fact that the OnComplete call for PLMO > sometimes is not triggered for a long time on some devices (it is triggered on > destruction or on the next navigation commit). > > Is this okay for the pingback client? Otherwise you may want to move the logic > to the paint / load event callback. > > https://codereview.chromium.org/2010253003/diff/100001/chrome/browser/page_lo... > File > chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h > (right): > > https://codereview.chromium.org/2010253003/diff/100001/chrome/browser/page_lo... > chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:62: > content::BrowserContext* browser_context_; > On 2016/06/06 19:59:00, RyanSturm wrote: > > On 2016/06/06 14:00:12, csharrison wrote: > > > This seems a little dangerous, given that afaik we can run code in the PLMO > > > destructor when the web contents is invalid. Can you either: > > > 1. Add a non-virtual web_contents() method on PLMO with a default > > implementation > > > that reaches into the MetricsWebContentsObserver. > > > > > > 2. Make sure this can never cause use after free. Explain the reasoning in > > > comments when you initialize the browser_context_. > > > > I added a comment explaining this, and created a cl > > (https://codereview.chromium.org/2046713002/) to make it obvious that the > > lifetime of BrowserContext explicitly outlives WebContents destructor. > > Thank you! This URL request is really low priority, and it's expected that it won't happen at the same time as the load. The session key, the timestamp, and the first request's URL will be used to try to correlate this to corresponding server side information after the fact.
On 2016/06/07 14:59:49, RyanSturm wrote: > On 2016/06/07 13:47:20, csharrison wrote: > > Thanks, this l-g-t-m except for the fact that the OnComplete call for PLMO > > sometimes is not triggered for a long time on some devices (it is triggered on > > destruction or on the next navigation commit). > > > > Is this okay for the pingback client? Otherwise you may want to move the logic > > to the paint / load event callback. > > > > > https://codereview.chromium.org/2010253003/diff/100001/chrome/browser/page_lo... > > File > > > chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h > > (right): > > > > > https://codereview.chromium.org/2010253003/diff/100001/chrome/browser/page_lo... > > > chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:62: > > content::BrowserContext* browser_context_; > > On 2016/06/06 19:59:00, RyanSturm wrote: > > > On 2016/06/06 14:00:12, csharrison wrote: > > > > This seems a little dangerous, given that afaik we can run code in the > PLMO > > > > destructor when the web contents is invalid. Can you either: > > > > 1. Add a non-virtual web_contents() method on PLMO with a default > > > implementation > > > > that reaches into the MetricsWebContentsObserver. > > > > > > > > 2. Make sure this can never cause use after free. Explain the reasoning in > > > > comments when you initialize the browser_context_. > > > > > > I added a comment explaining this, and created a cl > > > (https://codereview.chromium.org/2046713002/) to make it obvious that the > > > lifetime of BrowserContext explicitly outlives WebContents destructor. > > > > Thank you! > > This URL request is really low priority, and it's expected that it won't happen > at the same time as the load. The session key, the timestamp, and the first > request's URL will be used to try to correlate this to corresponding server side > information after the fact. Ok, as long as that's expected, lgtm.
bmcquade@chromium.org changed reviewers: + bmcquade@chromium.org
https://codereview.chromium.org/2010253003/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2010253003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:69: void DataReductionProxyMetricsObserver::OnComplete( keep in mind that on android, about 7% of page loads fail to invoke OnComplete when they had a first contentful paint, due to android fast shutdown. The missing stats end up skewing statistics. We should probably instead find a good way to notify you the first time the page is backgrounded, and you can run your logic then, since android gives a clear signal that the app is being backgrounded that you can rely on. https://codereview.chromium.org/2010253003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:78: if (!WasStartedInForegroundEventInForeground(timing.response_start, info)) i'm planning to remove responseStart soon as part of transitioning PageLoadTiming to report only metrics that come from the render process. This check should be redundant since first_contentful_paint must come strictly after response_start so if that's in the foreground, response_start has to be as well. can we remove this? https://codereview.chromium.org/2010253003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:91: if (!WasStartedInForegroundEventInForeground(timing.load_event_start, info)) there are many pages that reach first_contentful_paint but not the load event. I'd advise against only logging metrics if we reached the load event, as that will significantly skew your metrics by only including paint samples for pages that reached load event. In general we're strongly encouraging teams to move away from tracking the load event. Can you give more info on why you think you need this check? https://codereview.chromium.org/2010253003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:93: if (!browser_context_) might as well do this at the top of the function before we do any of the other work.
Still waiting on privacy to review the UMA opt-in no longer being necessary. https://codereview.chromium.org/2010253003/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2010253003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:69: void DataReductionProxyMetricsObserver::OnComplete( On 2016/06/07 19:49:46, Bryan McQuade wrote: > keep in mind that on android, about 7% of page loads fail to invoke OnComplete > when they had a first contentful paint, due to android fast shutdown. The > missing stats end up skewing statistics. We should probably instead find a good > way to notify you the first time the page is backgrounded, and you can run your > logic then, since android gives a clear signal that the app is being > backgrounded that you can rely on. I'll open a bug to add a new OnFirstBackground event, unless you're suggesting this is trivial to implement. https://codereview.chromium.org/2010253003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:78: if (!WasStartedInForegroundEventInForeground(timing.response_start, info)) On 2016/06/07 19:49:46, Bryan McQuade wrote: > i'm planning to remove responseStart soon as part of transitioning > PageLoadTiming to report only metrics that come from the render process. This > check should be redundant since first_contentful_paint must come strictly after > response_start so if that's in the foreground, response_start has to be as well. > can we remove this? response_start is also passed to the PingbackClient in SendPingback. We can move to an alternative when the switch happens. https://codereview.chromium.org/2010253003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:91: if (!WasStartedInForegroundEventInForeground(timing.load_event_start, info)) On 2016/06/07 19:49:46, Bryan McQuade wrote: > there are many pages that reach first_contentful_paint but not the load event. > I'd advise against only logging metrics if we reached the load event, as that > will significantly skew your metrics by only including paint samples for pages > that reached load event. In general we're strongly encouraging teams to move > away from tracking the load event. Can you give more info on why you think you > need this check? I should be able to ignore this if it's zero often. I will make sure that everyone is aware that this can be zero/empty. I'll add a is_zero check to these deltas similar to images, and we can send a best effort message to the proxy server. It will be good to have page load, but I agree that it is not critical. https://codereview.chromium.org/2010253003/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:93: if (!browser_context_) On 2016/06/07 19:49:46, Bryan McQuade wrote: > might as well do this at the top of the function before we do any of the other > work. Done.
lgtm
https://codereview.chromium.org/2010253003/diff/240001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2010253003/diff/240001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:78: if (data_reduction_proxy::params::IsIncludedInHoldbackFieldTrial()) Also, skip the pig backs if the tamper detection field trial is enabled. https://cs.chromium.org/chromium/src/components/data_reduction_proxy/core/com...
https://codereview.chromium.org/2010253003/diff/240001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2010253003/diff/240001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:78: if (data_reduction_proxy::params::IsIncludedInHoldbackFieldTrial()) On 2016/06/14 19:55:19, tbansal1 wrote: > Also, skip the pig backs if the tamper detection field trial is enabled. > https://cs.chromium.org/chromium/src/components/data_reduction_proxy/core/com... Done.
The CQ bit was checked by ryansturm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org, csharrison@chromium.org, bmcquade@chromium.org Link to the patchset: https://codereview.chromium.org/2010253003/#ps260001 (title: "Adding tamper detection experiment check")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2010253003/260001
The CQ bit was unchecked by ryansturm@chromium.org
The CQ bit was checked by ryansturm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2010253003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Patchset #15 (id:280001) has been deleted
The CQ bit was checked by ryansturm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmcquade@chromium.org, csharrison@chromium.org, tbansal@chromium.org Link to the patchset: https://codereview.chromium.org/2010253003/#ps300001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2010253003/300001
Message was sent while issue was closed.
Committed patchset #15 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Sending a data saver Pageload metrics pingback This sends a request to the data saver proxy server containing page load metrics for any page loads through the data saver proxy server. BUG=604853 ========== to ========== Sending a data saver Pageload metrics pingback This sends a request to the data saver proxy server containing page load metrics for any page loads through the data saver proxy server. BUG=604853 Committed: https://crrev.com/5c5a6bc2a6d02710a32fc762caa782d0941fbb98 Cr-Commit-Position: refs/heads/master@{#400240} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/5c5a6bc2a6d02710a32fc762caa782d0941fbb98 Cr-Commit-Position: refs/heads/master@{#400240} |
