Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1279543002:
Support needed to measure user and service traffic in Chrome. (Closed)
|
Created:
5 years, 4 months ago by amohammadkhan Modified:
5 years, 3 months ago Reviewers:
Avi (use Gerrit), sclittle, Lei Zhang, Mathieu, bengr, Alexei Svitkine (slow), brettw, Ilya Sherman, amohammadkhan1, mmenke, blundell CC:
chromium-reviews, cbentzel+watch_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@NewHistogram Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport needed to measure user and service traffic in Chrome.
Some of the traffic of Chrome is not coming from direct request of user and it
comes from services running on it such as Sync, Suggestion, or Complete. To
understand their contribution to total amount of Chrome traffic, it is necessary
to record services traffic use besides the user traffic use.
To be able to get more useful data, it is also necessary to record the condition
in which the request is made and the response is received. Two important aspects
are the connection type (Cellular or Wifi) and application state (Foreground or
Background). Also it is helpful to have the distribution of exchanged message
size for different directions (Upload or Download).
Besides the needed support for measuring, the suggestion service code is
instrumented to record its data use. This instrumentation is done as an example
and if this CL lands other target services will be instrumented too.
BUG=516434
Committed: https://crrev.com/092adb2451c74c4dc97e1808bfbe2352ad21da9d
Cr-Commit-Position: refs/heads/master@{#348490}
Patch Set 1 #
Total comments: 96
Patch Set 2 : Making DataUseMeasurement Class (With dependancy violation errors) #Patch Set 3 : Move User/Not-User traffic checks to ChromeNetworkDelegate #Patch Set 4 : Based on new architecture but with DEP errors #Patch Set 5 : Test added, with new dependency errors. #Patch Set 6 : Fix few formatting issues. #
Total comments: 43
Patch Set 7 : Making data_use_measurement component. #Patch Set 8 : Fixing a typo in BUILD.gn #Patch Set 9 : Removing few commented lines. #
Total comments: 41
Patch Set 10 : Addressing reviewer's comments but the ones related to making component #
Total comments: 10
Patch Set 11 : Addressed comments. #Patch Set 12 : Changing max value of histograms to 1M. #Patch Set 13 : Completing transition to a separate component. #
Total comments: 44
Patch Set 14 : Addressing reviewer's comments and adding test for component. #Patch Set 15 : Using Mockread in the test function of component. #Patch Set 16 : Adding sclittle to OWNERs and little fix in ChromeNetworkDelegate test. #
Total comments: 63
Patch Set 17 : Addressing reviewers' comments. #Patch Set 18 : Adding ifdef for android in tests and add dependencies to gn files. #Patch Set 19 : Continue fixing gn files. #Patch Set 20 : Fixing core/BUILD.gn #Patch Set 21 : Add if defined(ENABLE_EXTENSIONS) to tests. #
Total comments: 64
Patch Set 22 : Addressing reviewers' comments. #
Total comments: 22
Patch Set 23 : Addressing reviewers' comments. #Patch Set 24 : Rebased. #
Total comments: 4
Patch Set 25 : Addressing blundell's comments. #
Total comments: 48
Patch Set 26 : Addressing reviewers' comments. #
Total comments: 18
Patch Set 27 : Addressing reviewers' comments. #
Total comments: 21
Patch Set 28 : Addressing reviewers' comments. #Patch Set 29 : Improving the naming of source_sets in gn files. #
Total comments: 62
Patch Set 30 : Addressing reviewer's comments. #Patch Set 31 : Adding profileManager and fixing stack-use-after-return error. #
Total comments: 6
Patch Set 32 : Addressing reviewer's comments. #
Total comments: 8
Patch Set 33 : Addressing sclittle's comments. #
Total comments: 31
Patch Set 34 : Addressed mmenke's comments. #Patch Set 35 : Rebase to CL 1327763003 #Patch Set 36 : Minor updates in dep files. #
Total comments: 4
Patch Set 37 : Addressed comments and rebased. #Depends on Patchset: Dependent Patchsets: Messages
Total messages: 110 (19 generated)
amohammadkhan@google.com changed reviewers: + bengr@chromium.org
https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/chrome_n... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:491: // We have assumed that all the request for user contents and for services Don't personify code. Also, don't assume. How about the following: // Report the number of bytes received to UMA, separating user requests. i.e., those that are requested as part of a page load, from requests issued by Chrome services. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... File chrome/browser/net/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:19: // Copied from chromecast/base/metrics/cast_histograms.h Why are you copying code? Add a TODO to unify these macros with those used by cast_histograms.h https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:37: // NO_CACHE version of UMA_HISTOGRAM_CUSTOM_COUNT is used because the names of I can't parse this comment. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:46: // Enumerate the direction of data. Specifies the measured data was sent or Rewrite comment: // Specifies that data is received or sent, respectively. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:53: enum AppState { // The state of the application. Only available on Android. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:58: enum ConnectionType { The type of network connection that was used. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:60: WIFI Should this be tri-state: CELLULAR, WIFI, OTHER? https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:63: enum ChromeServiceType { The reason a request was sent. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:65: NOT_USER_TRAFFIC Maybe all this SERVICE_TRAFFIC. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:70: // somehow, it might be buggy until an event happens. Don't try to land buggy code. We should know for sure if we are in the background or foreground. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:71: // These are for just android, do I need to add if here? Strange comment. Is it needed? https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:77: bool listener_made = false; Don't use file scope variables. If you have state that needs to persist across function calls, your best bet is to create a class. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:86: std::string MakeHistogramName(ChromeServiceType service_type) { Delete this function and see my comment on line 126. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:98: AppState CurrentAppState() { AppState CurrentAppState() { #if defined(OS_ANDROID) if (app_state_ != base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES) return BACKGROUND; #endif return FOREGROUND; } https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:100: if (app_state == base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES) Is this really the right way to check? https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:126: if (dir == UPLOAD) { Remove curly braces. Also, I think you have the direction backwards. Just do: std::string full_histrogram_name = StringPrintf("DataUse.%s.%s.%s.%s", service_type == USER_TRAFFIC ? "UserData" : "ServiceData", dir == UPLOAD ? "Upload" : "Download", CurrentAppState() == BACKGROUND ? "Background" : "Foreground", CurrentConnectionType() == WIFI ? "WiFi" : "Cell"; https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:151: if (!listener_made) { What does listener_made mean? https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:160: if (!info) { Do: // All data from a renderer process is assumed to be user data. if (info && info->GetProcessType() == content::PROCESS_TYPE_RENDERER) renderer_flag = true; https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:167: int request_header_bytes = 0; Add a comment explaining that these won't be the number of bytes handed to the kernel because session layer framing and compression is not being accounted for. Then add a TODO to fix. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:173: if (request->GetFullRequestHeaders(&request_headers)) { Remove curly braces. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:176: total_upload_bytes = request_body_bytes + request_header_bytes; Remove extra space https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:178: if (renderer_flag) { Instead of having two cases, do: ReportDataUsage(renderer_flag ? USER_TRAFFIC : SERVICE_TRAFFIC, UPLOAD, total_upload_bytes); https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... File chrome/browser/net/data_use_measurement.h (right): https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.h:10: } // namespace net No need for the comment on a single line namespace. https://codereview.chromium.org/1279543002/diff/1/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1279543002/diff/1/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:1940: 'browser/net/safe_searco_util.h', typo. https://codereview.chromium.org/1279543002/diff/1/components/suggestions/sugg... File components/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/1279543002/diff/1/components/suggestions/sugg... components/suggestions/suggestions_service.cc:270: // Before deleting the request we record traffic use of this service // Before deleting the request, record the data user of the service. https://codereview.chromium.org/1279543002/diff/1/net/url_request/data_use_me... File net/url_request/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/1/net/url_request/data_use_me... net/url_request/data_use_measurement.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. This and the other data_use_measurement.cc should be merged into a single file and converted into a class. Ask someone on net/ where that class should live. https://codereview.chromium.org/1279543002/diff/1/net/url_request/data_use_me... net/url_request/data_use_measurement.cc:25: // Copied from chromecast/base/metrics/cast_histograms.h More copying? Add a TODO to unify with cast. Are these the same as in your other file? If so, try to put them somewhere common. https://codereview.chromium.org/1279543002/diff/1/net/url_request/data_use_me... net/url_request/data_use_measurement.cc:53: // Enumerates the direction of data. Specifies the measured data was sent or Remove the first sentence. https://codereview.chromium.org/1279543002/diff/1/net/url_request/data_use_me... net/url_request/data_use_measurement.cc:57: // Enumerates application state. For non android versions it is always Android. https://codereview.chromium.org/1279543002/diff/1/net/url_request/data_use_me... net/url_request/data_use_measurement.cc:58: // foreground Add a period. https://codereview.chromium.org/1279543002/diff/1/net/url_request/data_use_me... net/url_request/data_use_measurement.cc:65: // somehow, it might be buggy until an event happens. Be clearer about this. https://codereview.chromium.org/1279543002/diff/1/net/url_request/data_use_me... File net/url_request/data_use_measurement.h (right): https://codereview.chromium.org/1279543002/diff/1/net/url_request/data_use_me... net/url_request/data_use_measurement.h:17: void DataUseReport(const std::string& service_name, I don't think you should have files for this lone function. https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher.cc File net/url_request/url_fetcher.cc (right): https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher.cc:39: DataUseReport(service, this); Rename as ReportDataUseUMA, so something like that. https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher.h File net/url_request/url_fetcher.h (right): https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher.h:147: void ReportYourUsage(const std::string& service) const; How about this? void ReportUsageUMA(const std::string& service_name) const; https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher.h:323: // uncompressing Add a period. https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher.h:327: // this value is before compression, so it may not be a good representative of Why report it then? https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... File net/url_request/url_fetcher_core.cc (right): https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher_core.cc:105: total_response_bytes_(-1), Why are these initialized to -1 and not 0? https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher_core.cc:352: int64 URLFetcherCore::GetBytesSentSize() const { GetTotalBytesSent() https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher_core.cc:356: int64 URLFetcherCore::GetBytesReceivedSize() const { GetTotalReceivedBytes() https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher_core.cc:357: return total_downloaded_bytes_; total_received_bytes_. In general, please be consistent with names. https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher_core.cc:825: if (request_->has_upload()) { Remove curly braces. https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher_core.cc:829: if (request_->GetFullRequestHeaders(&request_headers)) { Remove curly braces. https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... File net/url_request/url_fetcher_core.h (right): https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher_core.h:339: // Total received bytes size Add a period. https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher_core.h:341: // total sent bytes size You can be more specific for these. Total bytes sent to the session layer, e.g., QUIC, which may, in turn, compress headers and frame the request. https://codereview.chromium.org/1279543002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1279543002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:5216: + <owner>amohammadkhan@chromium.org</owner> Add me as an owner to all of your histograms. You leave soon. :) https://codereview.chromium.org/1279543002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:5218: + The total amount of data use of Chrome for services. This traffic is not The total data use of Chrome's services. https://codereview.chromium.org/1279543002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:5219: + requested by Renderer process. Please verify with nasko@ that the trick of checking that the request came from the renderer process will continue to work once PlzNavigate is in use. https://codereview.chromium.org/1279543002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:72345: +<histogram_suffixes name="Datause.Dimensions" separator="."> DataUse
https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/chrome_n... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/chrome_n... chrome/browser/net/chrome_network_delegate.cc:491: // We have assumed that all the request for user contents and for services On 2015/08/07 18:00:00, bengr wrote: > Don't personify code. Also, don't assume. How about the following: > > // Report the number of bytes received to UMA, separating user requests. i.e., > those that are requested as part of a page load, from requests issued by Chrome > services. Done. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... File chrome/browser/net/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:19: // Copied from chromecast/base/metrics/cast_histograms.h On 2015/08/07 18:00:01, bengr wrote: > Why are you copying code? Add a TODO to unify these macros with those used by > cast_histograms.h I didn't like copying myself too, but I didn't want to include that file and I needed exact same functionality, so I copied the code. It was good if we had these macros as a part of histogram macros but UMA team may not be interested to add these macros to their header. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:37: // NO_CACHE version of UMA_HISTOGRAM_CUSTOM_COUNT is used because the names of On 2015/08/07 18:00:00, bengr wrote: > I can't parse this comment. Sorry. I tried to rephrase it. Please let me know if still it is not clear. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:46: // Enumerate the direction of data. Specifies the measured data was sent or On 2015/08/07 18:00:01, bengr wrote: > Rewrite comment: > > // Specifies that data is received or sent, respectively. Done. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:53: enum AppState { On 2015/08/07 18:00:00, bengr wrote: > // The state of the application. Only available on Android. Done. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:58: enum ConnectionType { On 2015/08/07 18:00:01, bengr wrote: > The type of network connection that was used. Done. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:60: WIFI On 2015/08/07 18:00:00, bengr wrote: > Should this be tri-state: CELLULAR, WIFI, OTHER? Basically I am checking isCellular, so I changed it to Cellular and Not_Cellular. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:63: enum ChromeServiceType { On 2015/08/07 18:00:01, bengr wrote: > The reason a request was sent. Done. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:65: NOT_USER_TRAFFIC On 2015/08/07 18:00:00, bengr wrote: > Maybe all this SERVICE_TRAFFIC. Done. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:70: // somehow, it might be buggy until an event happens. On 2015/08/07 18:00:00, bengr wrote: > Don't try to land buggy code. We should know for sure if we are in the > background or foreground. I think I was over thinking. The Chrome should always start in foreground and it should not be buggy. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:71: // These are for just android, do I need to add if here? On 2015/08/07 18:00:01, bengr wrote: > Strange comment. Is it needed? Done. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:77: bool listener_made = false; On 2015/08/07 18:00:00, bengr wrote: > Don't use file scope variables. If you have state that needs to persist across > function calls, your best bet is to create a class. Done. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:86: std::string MakeHistogramName(ChromeServiceType service_type) { On 2015/08/07 18:00:00, bengr wrote: > Delete this function and see my comment on line 126. Done. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:98: AppState CurrentAppState() { On 2015/08/07 18:00:01, bengr wrote: > AppState CurrentAppState() { > #if defined(OS_ANDROID) > if (app_state_ != base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES) > return BACKGROUND; > #endif > return FOREGROUND; > } I'm gonna use the similar approach to your proposed approach, but I'll separate the dimension parts because I want to use it in few different parts so it may be useful to define it as a function. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:100: if (app_state == base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES) On 2015/08/07 18:00:00, bengr wrote: > Is this really the right way to check? I think so. At least it is working well in my tests. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:126: if (dir == UPLOAD) { On 2015/08/07 18:00:01, bengr wrote: > Remove curly braces. Also, I think you have the direction backwards. Just do: > > std::string full_histrogram_name = StringPrintf("DataUse.%s.%s.%s.%s", > service_type == USER_TRAFFIC ? "UserData" : "ServiceData", > dir == UPLOAD ? "Upload" : "Download", > CurrentAppState() == BACKGROUND ? "Background" : "Foreground", > CurrentConnectionType() == WIFI ? "WiFi" : "Cell"; Done. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:151: if (!listener_made) { On 2015/08/07 18:00:01, bengr wrote: > What does listener_made mean? Removed in new version. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:160: if (!info) { On 2015/08/07 18:00:01, bengr wrote: > Do: > > // All data from a renderer process is assumed to be user data. > if (info && info->GetProcessType() == content::PROCESS_TYPE_RENDERER) > renderer_flag = true; Done. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:167: int request_header_bytes = 0; On 2015/08/07 18:00:01, bengr wrote: > Add a comment explaining that these won't be the number of bytes handed to the > kernel because session layer framing and compression is not being accounted for. > Then add a TODO to fix. Done. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:173: if (request->GetFullRequestHeaders(&request_headers)) { On 2015/08/07 18:00:01, bengr wrote: > Remove curly braces. Done. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:176: total_upload_bytes = request_body_bytes + request_header_bytes; On 2015/08/07 18:00:00, bengr wrote: > Remove extra space Done. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.cc:178: if (renderer_flag) { On 2015/08/07 18:00:00, bengr wrote: > Instead of having two cases, do: > ReportDataUsage(renderer_flag ? USER_TRAFFIC : SERVICE_TRAFFIC, UPLOAD, > total_upload_bytes); Done. https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... File chrome/browser/net/data_use_measurement.h (right): https://codereview.chromium.org/1279543002/diff/1/chrome/browser/net/data_use... chrome/browser/net/data_use_measurement.h:10: } // namespace net On 2015/08/07 18:00:01, bengr wrote: > No need for the comment on a single line namespace. Done. https://codereview.chromium.org/1279543002/diff/1/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1279543002/diff/1/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:1940: 'browser/net/safe_searco_util.h', On 2015/08/07 18:00:01, bengr wrote: > typo. Done. https://codereview.chromium.org/1279543002/diff/1/components/suggestions/sugg... File components/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/1279543002/diff/1/components/suggestions/sugg... components/suggestions/suggestions_service.cc:270: // Before deleting the request we record traffic use of this service On 2015/08/07 18:00:01, bengr wrote: > // Before deleting the request, record the data user of the service. Done. https://codereview.chromium.org/1279543002/diff/1/net/url_request/data_use_me... File net/url_request/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/1/net/url_request/data_use_me... net/url_request/data_use_measurement.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/08/07 18:00:01, bengr wrote: > This and the other data_use_measurement.cc should be merged into a single file > and converted into a class. Ask someone on net/ where that class should live. Done. https://codereview.chromium.org/1279543002/diff/1/net/url_request/data_use_me... net/url_request/data_use_measurement.cc:25: // Copied from chromecast/base/metrics/cast_histograms.h On 2015/08/07 18:00:01, bengr wrote: > More copying? Add a TODO to unify with cast. Are these the same as in your other > file? If so, try to put them somewhere common. Done. https://codereview.chromium.org/1279543002/diff/1/net/url_request/data_use_me... net/url_request/data_use_measurement.cc:53: // Enumerates the direction of data. Specifies the measured data was sent or On 2015/08/07 18:00:01, bengr wrote: > Remove the first sentence. Done. https://codereview.chromium.org/1279543002/diff/1/net/url_request/data_use_me... net/url_request/data_use_measurement.cc:57: // Enumerates application state. For non android versions it is always On 2015/08/07 18:00:01, bengr wrote: > Android. Done. https://codereview.chromium.org/1279543002/diff/1/net/url_request/data_use_me... net/url_request/data_use_measurement.cc:58: // foreground On 2015/08/07 18:00:01, bengr wrote: > Add a period. Done. https://codereview.chromium.org/1279543002/diff/1/net/url_request/data_use_me... net/url_request/data_use_measurement.cc:65: // somehow, it might be buggy until an event happens. On 2015/08/07 18:00:01, bengr wrote: > Be clearer about this. Done. https://codereview.chromium.org/1279543002/diff/1/net/url_request/data_use_me... File net/url_request/data_use_measurement.h (right): https://codereview.chromium.org/1279543002/diff/1/net/url_request/data_use_me... net/url_request/data_use_measurement.h:17: void DataUseReport(const std::string& service_name, On 2015/08/07 18:00:01, bengr wrote: > I don't think you should have files for this lone function. Done. https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher.cc File net/url_request/url_fetcher.cc (right): https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher.cc:39: DataUseReport(service, this); On 2015/08/07 18:00:01, bengr wrote: > Rename as ReportDataUseUMA, so something like that. Done. https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher.h File net/url_request/url_fetcher.h (right): https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher.h:147: void ReportYourUsage(const std::string& service) const; On 2015/08/07 18:00:01, bengr wrote: > How about this? > void ReportUsageUMA(const std::string& service_name) const; Done. https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher.h:323: // uncompressing On 2015/08/07 18:00:01, bengr wrote: > Add a period. Done. https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher.h:327: // this value is before compression, so it may not be a good representative of On 2015/08/07 18:00:01, bengr wrote: > Why report it then? Done. https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... File net/url_request/url_fetcher_core.cc (right): https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher_core.cc:105: total_response_bytes_(-1), On 2015/08/07 18:00:02, bengr wrote: > Why are these initialized to -1 and not 0? To be distinguishable if they are not touched at all by us, but I think it was not necessary so I am going to change it to zero. https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher_core.cc:352: int64 URLFetcherCore::GetBytesSentSize() const { On 2015/08/07 18:00:01, bengr wrote: > GetTotalBytesSent() Done. https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher_core.cc:356: int64 URLFetcherCore::GetBytesReceivedSize() const { On 2015/08/07 18:00:02, bengr wrote: > GetTotalReceivedBytes() Done. https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher_core.cc:357: return total_downloaded_bytes_; On 2015/08/07 18:00:02, bengr wrote: > total_received_bytes_. In general, please be consistent with names. At first I had use total_received_bytes. But when I rebased my code after a while, this name was used by someone else! so I changed the name to total_downloaded_bytes! I saw it's definition, it might be similar but it's value is increase in some other function than the function I had used. So I didn't want to risk and use some variable that I have not experiments on it and I am not sure about its value so I kept my own variable. https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher_core.cc:825: if (request_->has_upload()) { On 2015/08/07 18:00:02, bengr wrote: > Remove curly braces. Done. https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher_core.cc:829: if (request_->GetFullRequestHeaders(&request_headers)) { On 2015/08/07 18:00:02, bengr wrote: > Remove curly braces. Done. https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... File net/url_request/url_fetcher_core.h (right): https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher_core.h:339: // Total received bytes size On 2015/08/07 18:00:02, bengr wrote: > Add a period. Done. https://codereview.chromium.org/1279543002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher_core.h:341: // total sent bytes size On 2015/08/07 18:00:02, bengr wrote: > You can be more specific for these. Total bytes sent to the session layer, e.g., > QUIC, which may, in turn, compress headers and frame the request. Done. https://codereview.chromium.org/1279543002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1279543002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:5216: + <owner>amohammadkhan@chromium.org</owner> On 2015/08/07 18:00:02, bengr wrote: > Add me as an owner to all of your histograms. You leave soon. :) Done. https://codereview.chromium.org/1279543002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:5218: + The total amount of data use of Chrome for services. This traffic is not On 2015/08/07 18:00:02, bengr wrote: > The total data use of Chrome's services. Done. https://codereview.chromium.org/1279543002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:5219: + requested by Renderer process. On 2015/08/07 18:00:02, bengr wrote: > Please verify with nasko@ that the trick of checking that the request came from > the renderer process will continue to work once PlzNavigate is in use. I have asked him in an Email and I am waiting for his answer. https://codereview.chromium.org/1279543002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:72345: +<histogram_suffixes name="Datause.Dimensions" separator="."> On 2015/08/07 18:00:02, bengr wrote: > DataUse Done.
No test is added yet, because I am waiting to completely assure about architecture. The #include for content/public/browser/resource_request_info.h is commented intentionally, because it causes dependency errors. The code is uploaded to get feedback about other possible locations for this class and also the architecture of class itself. Please do not review the details, until I have reached a conclusion about high level problems like a good location for it or having it as a single file or two files.
On 2015/08/11 22:13:32, amohammadkhan wrote: > No test is added yet, because I am waiting to completely assure about > architecture. > The #include for content/public/browser/resource_request_info.h is commented > intentionally, because it causes dependency errors. The code is uploaded to get > feedback about other possible locations for this class and also the architecture > of class itself. > Please do not review the details, until I have reached a conclusion about high > level problems like a good location for it or having it as a single file or two > files. No test is added. Still trying to finalize the architecture. CL Uploaded to ask the following questions: 1. What is proper place to put DataUseMeasurement and DataUseUserData files? (Temporarily I have put them in net/url_request/ that I know is not a proper place for them) 2. Is there any problem with the implementation or architecture in high level? (known problem: All the instances of DataUseMeasurement are doing the same thing they are not really different, but I didn't use singleton because its possible problems and DataUseMeasurement in very light class so it has the minimum overhead.
On 2015/08/17 16:36:52, amohammadkhan wrote: > On 2015/08/11 22:13:32, amohammadkhan wrote: > > No test is added yet, because I am waiting to completely assure about > > architecture. > > The #include for content/public/browser/resource_request_info.h is commented > > intentionally, because it causes dependency errors. The code is uploaded to > get > > feedback about other possible locations for this class and also the > architecture > > of class itself. > > Please do not review the details, until I have reached a conclusion about high > > level problems like a good location for it or having it as a single file or > two > > files. > > No test is added. Still trying to finalize the architecture. > CL Uploaded to ask the following questions: > 1. What is proper place to put DataUseMeasurement and DataUseUserData files? > (Temporarily I have put them in net/url_request/ that I know is not a proper > place for them) > 2. Is there any problem with the implementation or architecture in high > level? (known problem: All the instances of DataUseMeasurement are doing the > same thing they are not really different, but I didn't use singleton because its > possible problems and DataUseMeasurement in very light class so it has the > minimum overhead. I'll review once there are tests.
amohammadkhan@google.com changed reviewers: + sclittle@chromium.org
On 2015/08/19 20:44:17, amohammadkhan wrote: Currently maybe the main issue is where to put data_use_user_data and data_use_measurement. It seems that chrome/browser/net is a good place for data_use_measurement but it is not a good place for data_use_user_data. Because when services include it, it causes dependency issues and I think I have to change its place. Any suggestions? For example can I put it in base/metrics or int net? (Currently I have commented the include line in suggestions_service.cc, because it causes dependency issues)
https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate.h:204: scoped_refptr<DataUseMeasurement> data_use_measurer_; nit: please either rename the class to DataUseMeasurer or change the name here to data_use_measurement_. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate.h:204: scoped_refptr<DataUseMeasurement> data_use_measurer_; Also, don't refcount this unless there's a really good reason to refcount it. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... File chrome/browser/net/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.cc:26: do { \ Why does this need to be a macro? Could this just be a function? https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.cc:34: #define STATIC_HISTOGRAM_POINTER_BLOCK_NO_CACHE( \ Same here, why a macro and not just a function? https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.cc:81: // fair comparison between foreground and background data use, -A histograms Just say "If the OS is not Android, all the requests are considered foreground." You could just mention in the histogram description or enum value description in histograms.xml that BACKGROUND only applies to android. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.cc:128: base::Bind(&DataUseMeasurement::OnApplicationStateChange, this)); Instead of posting this as a scoped_refptr, it might make more sense to post this as a WeakPtr instead. See weak_ptr.h, you could add a WeakPtrFactory attribute to this object. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.cc:132: void DataUseMeasurement::ReportDataUseUMA(const net::URLRequest* request) { nit: please add blank lines where appropriate in this method for readability. It's kinda dense right now. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.cc:157: int total_received_bytes = request->GetTotalReceivedBytes(); Always use int64_t for byte counts unless you have a really good reason not to. This would overflow if the user downloads a 5 GB file. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.cc:162: DataUseUserData* sData = static_cast<DataUseUserData*>( nit: use reinterpret_cast here instead, since it's just a pointer https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... File chrome/browser/net/data_use_measurement.h (right): https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.h:20: Put all the new classes and stuff in a namespace like chrome_browser_net https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.h:25: // platforms it is always FOREGROUND. These enums should be moved below into the DataUseMeasurement class. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.h:26: enum AppState { BACKGROUND, FOREGROUND }; Why is this enum needed? Could you just use a boolean? https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.h:29: enum ConnectionType { CELLULAR, NOT_CELLULAR }; Why is this enum needed? Could you just use a boolean? https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.h:36: : public base::RefCountedThreadSafe<DataUseMeasurement> { Why does this class need to be refcounted? https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.h:38: // This constructor initialize the finish this comment https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.h:48: // latter two are used if traffic is originated from a service. This comment is too wordy, could you cut this down and not describe the implementation? Even just the first sentence might be enough. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.h:69: base::android::ApplicationStatusListener* app_listener_; What owns this object? Please comment that here. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.h:73: void ReportDataUsageURLRequest(ChromeTrafficType service_type, Move these functions above the private attributes. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.h:82: // Making the current dimension of measured data traffic. I don't understand this comment. https://codereview.chromium.org/1279543002/diff/100001/components/suggestions... File components/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/1279543002/diff/100001/components/suggestions... components/suggestions/suggestions_service.cc:20: // #include "chrome/browser/net/data_use_user_data.h" Yeah, the data_use_measurement stuff should probably be moved into a new component if you want to depend on it from other components. Note that code in components isn't allowed to have chrome/ dependencies, like the commented out line here. Here's an example component for wifi prefetch: https://code.google.com/p/chromium/codesearch#chromium/src/components/precache/ You could put the data_use_measurement stuff inside a content/ subdirectory so that it can depend on the content/ stuff it needs, and put the data_use_user_data stuff inside a core/ subdirectory.
Patchset #7 (id:120001) has been deleted
Patchset #7 (id:140001) has been deleted
https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate.h:204: scoped_refptr<DataUseMeasurement> data_use_measurer_; On 2015/08/19 23:32:39, sclittle wrote: > nit: please either rename the class to DataUseMeasurer or change the name here > to data_use_measurement_. Done. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate.h:204: scoped_refptr<DataUseMeasurement> data_use_measurer_; On 2015/08/19 23:32:39, sclittle wrote: > Also, don't refcount this unless there's a really good reason to refcount it. Basically I used all the refcounts because of the callback in this class, because I was not sure is it okay to not use refcounts and handle the lifetime manually. But after your heads up and more reading about AppliactionStatusListener, it seems I was over cautious and it was not needed. So I changed it to normal class. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... File chrome/browser/net/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.cc:26: do { \ On 2015/08/19 23:32:39, sclittle wrote: > Why does this need to be a macro? Could this just be a function? Done. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.cc:34: #define STATIC_HISTOGRAM_POINTER_BLOCK_NO_CACHE( \ On 2015/08/19 23:32:39, sclittle wrote: > Same here, why a macro and not just a function? Done. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.cc:81: // fair comparison between foreground and background data use, -A histograms On 2015/08/19 23:32:39, sclittle wrote: > Just say "If the OS is not Android, all the requests are considered foreground." > You could just mention in the histogram description or enum value description in > histograms.xml that BACKGROUND only applies to android. Done. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.cc:128: base::Bind(&DataUseMeasurement::OnApplicationStateChange, this)); On 2015/08/19 23:32:39, sclittle wrote: > Instead of posting this as a scoped_refptr, it might make more sense to post > this as a WeakPtr instead. See weak_ptr.h, you could add a WeakPtrFactory > attribute to this object. I checked the examples of this class again, it seems it is even fine to use base::Unretained, so I used the same approach. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.cc:132: void DataUseMeasurement::ReportDataUseUMA(const net::URLRequest* request) { On 2015/08/19 23:32:39, sclittle wrote: > nit: please add blank lines where appropriate in this method for readability. > It's kinda dense right now. Done. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.cc:157: int total_received_bytes = request->GetTotalReceivedBytes(); On 2015/08/19 23:32:39, sclittle wrote: > Always use int64_t for byte counts unless you have a really good reason not to. > This would overflow if the user downloads a 5 GB file. I see your point. But the problem is that UMA is using int32 everywhere, (For the sample and for count) and I am worried about it too. So I need to find a way to solve that problem, and then I can change my variable to int64 too, but if I cannot, I have to find a trick to solve the problem, maybe changing the minimum or storing the log() of values instead of their original values. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.cc:162: DataUseUserData* sData = static_cast<DataUseUserData*>( On 2015/08/19 23:32:39, sclittle wrote: > nit: use reinterpret_cast here instead, since it's just a pointer Done. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... File chrome/browser/net/data_use_measurement.h (right): https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.h:20: On 2015/08/19 23:32:39, sclittle wrote: > Put all the new classes and stuff in a namespace like chrome_browser_net Done. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.h:25: // platforms it is always FOREGROUND. On 2015/08/19 23:32:40, sclittle wrote: > These enums should be moved below into the DataUseMeasurement class. Done. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.h:26: enum AppState { BACKGROUND, FOREGROUND }; On 2015/08/19 23:32:39, sclittle wrote: > Why is this enum needed? Could you just use a boolean? You are right a bool may be enough, but basically the states of app are more than these two ( I think they are four), so I thought maybe we may get interested in all of those later, so by having the enum, it will be an easier task to support those too. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.h:29: enum ConnectionType { CELLULAR, NOT_CELLULAR }; On 2015/08/19 23:32:40, sclittle wrote: > Why is this enum needed? Could you just use a boolean? The similar reason to AppState enum. Again the connection types are more than these two, so changing the code to support more detailed connection types will be easier by having this enum. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.h:36: : public base::RefCountedThreadSafe<DataUseMeasurement> { On 2015/08/19 23:32:40, sclittle wrote: > Why does this class need to be refcounted? I think I was over cautious because of callback function, I removed it. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.h:38: // This constructor initialize the On 2015/08/19 23:32:39, sclittle wrote: > finish this comment Done. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.h:48: // latter two are used if traffic is originated from a service. On 2015/08/19 23:32:40, sclittle wrote: > This comment is too wordy, could you cut this down and not describe the > implementation? Even just the first sentence might be enough. Done. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.h:69: base::android::ApplicationStatusListener* app_listener_; On 2015/08/19 23:32:40, sclittle wrote: > What owns this object? Please comment that here. Done. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.h:73: void ReportDataUsageURLRequest(ChromeTrafficType service_type, On 2015/08/19 23:32:40, sclittle wrote: > Move these functions above the private attributes. Done. https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.h:82: // Making the current dimension of measured data traffic. On 2015/08/19 23:32:39, sclittle wrote: > I don't understand this comment. Done. https://codereview.chromium.org/1279543002/diff/100001/components/suggestions... File components/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/1279543002/diff/100001/components/suggestions... components/suggestions/suggestions_service.cc:20: // #include "chrome/browser/net/data_use_user_data.h" On 2015/08/19 23:32:40, sclittle wrote: > Yeah, the data_use_measurement stuff should probably be moved into a new > component if you want to depend on it from other components. > > Note that code in components isn't allowed to have chrome/ dependencies, like > the commented out line here. > > Here's an example component for wifi prefetch: > https://code.google.com/p/chromium/codesearch#chromium/src/components/precache/ > > You could put the data_use_measurement stuff inside a content/ subdirectory so > that it can depend on the content/ stuff it needs, and put the > data_use_user_data stuff inside a core/ subdirectory. Done.
https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... File components/data_use_measurement.gypi (right): https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement.gypi:7: 'target_name': 'data_use_measurement', separate the core/ and content/ stuff into different targets https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... File components/data_use_measurement/DEPS (right): https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/DEPS:2: "+content/public/browser", Create separate DEPS files in the data_use_measurement/core/ and data_use_measurement/content/ directories, and only data_use_measurement/content/DEPS is allowed to list "+content/public/browser". https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... File components/data_use_measurement/content/data_use_user_data.h (right): https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/content/data_use_user_data.h:5: #ifndef COMPONENTS_DATA_USE_MEASUREMENT_CONTENT_DATA_USE_USER_DATA_H_ This file and it's .cc file should be in "data_use_measurement/core/", not content. https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/content/data_use_user_data.h:25: std::string service_name_; Instead of holding a string, could you create an enum in this file of all the services, and just store the enum value in the DataUseUserData? https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... File components/data_use_measurement/core/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.cc:27: void UMAHistogramRecorder(const std::string& name, int64_t sample) { Name this function more like a verb, e.g. RecordUMAHistogramCount or something https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.cc:31: 5000000, // Maximum sample size. Should cover most of the requests. Why just 5 MB? Lots of fetches are more than 5 MB. https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.cc:34: histogram_pointer->Add(sample); \ remove backslash https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.cc:40: void SparseHistogramWithValue(const std::string& name, int64_t sample, Put these helper functions that aren't part of the class into an anonymous namespace, also name this function like a verb https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.cc:89: DataUseUserData* sData = reinterpret_cast<DataUseUserData*>( Please be more descriptive than "sData", maybe "ServiceData" or something? https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.cc:107: if (app_state_ == base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES) Reorganize this function to be like #if defined(OS_ANDROID) if (app_state_ != base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES) return BACKGROUND; #endif return FOREGROUND; or something such that there's only one "return FOREGROUND;". https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.cc:140: SparseHistogramWithValue(combinedHistogramName, base::Hash(service_name), Instead of hashing the service name, which is dependent on the implementation of base::Hash and isn't guaranteed to be the same for all platforms/compliers, just have an enum in data_use_user_data.h or something of all the data types. https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... File components/data_use_measurement/core/data_use_measurement.h (right): https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.h:5: #ifndef COMPONENTS_DATA_USE_MEASUREMENT_H_ Change this to the full path, e.g. COMPONENTS_DATA_USE_MEASUREMENT_CORE_DATA_USE_MEASUREMENT_H_ https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.h:6: #define COMPONENTS_DATA_USE_MEASUREMENT_H_ Move this file and its .cc to the "data_use_measurement/content" directory, since it depends on content. https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.h:7: Add "#include <stdint.h>" for int64_t. https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.h:57: ConnectionType CurrentConnectionType(); nit: indentation, please run "git cl format" to format your code. https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.h:81: base::android::ApplicationStatusListener app_listener_; Make this a scoped_ptr, and call reset() on it to delete it in ~DataUseMeasurement. This is necessary to prevent the app_listener from being notified between destructing DataUseMeasurement and destructing the |app_listener_| member. https://codereview.chromium.org/1279543002/diff/200001/components/suggestions... File components/suggestions.gypi (right): https://codereview.chromium.org/1279543002/diff/200001/components/suggestions... components/suggestions.gypi:23: 'components.gyp:data_use_measurement', You should only have to depend on the "core/" target of data_use_measurement here. https://codereview.chromium.org/1279543002/diff/200001/components/suggestions... File components/suggestions/DEPS (right): https://codereview.chromium.org/1279543002/diff/200001/components/suggestions... components/suggestions/DEPS:2: "+components/data_use_measurement", change to "+components/data_use_measurement/core" https://codereview.chromium.org/1279543002/diff/200001/components/suggestions... File components/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/1279543002/diff/200001/components/suggestions... components/suggestions/suggestions_service.cc:260: "Suggestion")); Pass in an enum value instead here. https://codereview.chromium.org/1279543002/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1279543002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:53549: + <int value="700836285" label="Suggestion Service"/> Please don't use a hash as an enum value.
Thank you for your comments. I have not addressed component comments as I am waiting for mmenke@ to respond to my query. If my request get denied, I'll start working on completing component part. https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... File components/data_use_measurement/content/data_use_user_data.h (right): https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/content/data_use_user_data.h:25: std::string service_name_; On 2015/08/21 23:55:34, sclittle wrote: > Instead of holding a string, could you create an enum in this file of all the > services, and just store the enum value in the DataUseUserData? You are right. We had a problem about a good place for storing the enum in previous architecture and using the hash of service name was used then, but now it seems we do not have this problem anymore so we can use the enum. https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... File components/data_use_measurement/core/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.cc:27: void UMAHistogramRecorder(const std::string& name, int64_t sample) { On 2015/08/21 23:55:34, sclittle wrote: > Name this function more like a verb, e.g. RecordUMAHistogramCount or something Done. https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.cc:31: 5000000, // Maximum sample size. Should cover most of the requests. On 2015/08/21 23:55:34, sclittle wrote: > Why just 5 MB? Lots of fetches are more than 5 MB. I agree with you. What do you think is the good number here? My main concern was that I wasn't sure what would happen to histogram's precision if I make this number very large as there is limited number of buckets? https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.cc:34: histogram_pointer->Add(sample); \ On 2015/08/21 23:55:34, sclittle wrote: > remove backslash It is very odd that I don't see the backslash in my code(but I see it here too), maybe it is because of an error happened during one of my cl uploads. It gave me an error after cl upload and an erroneous cl was uploaded here. So I deleted it and I uploaded another version after making some changes. Maybe one of my commits is missing somehow! (Or maybe I have edited it later and I don't remember, if it exists in my next CL upload, I'll try to fix it) https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.cc:40: void SparseHistogramWithValue(const std::string& name, int64_t sample, On 2015/08/21 23:55:34, sclittle wrote: > Put these helper functions that aren't part of the class into an anonymous > namespace, also name this function like a verb Done. https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.cc:89: DataUseUserData* sData = reinterpret_cast<DataUseUserData*>( On 2015/08/21 23:55:34, sclittle wrote: > Please be more descriptive than "sData", maybe "ServiceData" or something? Done. https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.cc:107: if (app_state_ == base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES) On 2015/08/21 23:55:34, sclittle wrote: > Reorganize this function to be like > > #if defined(OS_ANDROID) > if (app_state_ != base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES) > return BACKGROUND; > #endif > > return FOREGROUND; > > or something such that there's only one "return FOREGROUND;". Done. https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.cc:140: SparseHistogramWithValue(combinedHistogramName, base::Hash(service_name), On 2015/08/21 23:55:34, sclittle wrote: > Instead of hashing the service name, which is dependent on the implementation of > base::Hash and isn't guaranteed to be the same for all platforms/compliers, just > have an enum in data_use_user_data.h or something of all the data types. Done. https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... File components/data_use_measurement/core/data_use_measurement.h (right): https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.h:5: #ifndef COMPONENTS_DATA_USE_MEASUREMENT_H_ On 2015/08/21 23:55:34, sclittle wrote: > Change this to the full path, e.g. > COMPONENTS_DATA_USE_MEASUREMENT_CORE_DATA_USE_MEASUREMENT_H_ Done. https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.h:7: On 2015/08/21 23:55:34, sclittle wrote: > Add "#include <stdint.h>" for int64_t. Done. https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.h:57: ConnectionType CurrentConnectionType(); On 2015/08/21 23:55:34, sclittle wrote: > nit: indentation, please run "git cl format" to format your code. Done. https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.h:81: base::android::ApplicationStatusListener app_listener_; On 2015/08/21 23:55:34, sclittle wrote: > Make this a scoped_ptr, and call reset() on it to delete it in > ~DataUseMeasurement. This is necessary to prevent the app_listener from being > notified between destructing DataUseMeasurement and destructing the > |app_listener_| member. Done. https://codereview.chromium.org/1279543002/diff/200001/components/suggestions... File components/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/1279543002/diff/200001/components/suggestions... components/suggestions/suggestions_service.cc:260: "Suggestion")); On 2015/08/21 23:55:34, sclittle wrote: > Pass in an enum value instead here. Done. https://codereview.chromium.org/1279543002/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1279543002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:53549: + <int value="700836285" label="Suggestion Service"/> On 2015/08/21 23:55:34, sclittle wrote: > Please don't use a hash as an enum value. Done.
https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... File components/data_use_measurement/core/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.cc:31: 5000000, // Maximum sample size. Should cover most of the requests. On 2015/08/24 23:18:48, amohammadkhan wrote: > On 2015/08/21 23:55:34, sclittle wrote: > > Why just 5 MB? Lots of fetches are more than 5 MB. > > I agree with you. What do you think is the good number here? My main concern was > that I wasn't sure what would happen to histogram's precision if I make this > number very large as there is limited number of buckets? After discussing offline, it seems like the service requests are typically on the order of kilobytes, so it should be fine to make this 1000000. https://codereview.chromium.org/1279543002/diff/220001/components/data_use_me... File components/data_use_measurement/content/data_use_user_data.cc (right): https://codereview.chromium.org/1279543002/diff/220001/components/data_use_me... components/data_use_measurement/content/data_use_user_data.cc:28: break; nit: Just put the NOTREACHED here under "default". https://codereview.chromium.org/1279543002/diff/220001/components/data_use_me... components/data_use_measurement/content/data_use_user_data.cc:30: NOTREACHED() << "Not known servie type in DataUseUserData"; nit: spelling "servie" https://codereview.chromium.org/1279543002/diff/220001/components/data_use_me... File components/data_use_measurement/core/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/220001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.cc:17: namespace { nit: add blank line after "namespace {" https://codereview.chromium.org/1279543002/diff/220001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.cc:41: } // Anonymous namespace. nit: this line should just be "} // namespace", don't need the "Anonymous" or the period afterwards. https://codereview.chromium.org/1279543002/diff/220001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.cc:141: std::string combinedHistogramName = "DataUse.Services" + DimensionMaker(dir); variable names should use underscores, not camel case.
https://codereview.chromium.org/1279543002/diff/220001/components/data_use_me... File components/data_use_measurement/content/data_use_user_data.cc (right): https://codereview.chromium.org/1279543002/diff/220001/components/data_use_me... components/data_use_measurement/content/data_use_user_data.cc:28: break; On 2015/08/24 23:59:34, sclittle wrote: > nit: Just put the NOTREACHED here under "default". Done. https://codereview.chromium.org/1279543002/diff/220001/components/data_use_me... components/data_use_measurement/content/data_use_user_data.cc:30: NOTREACHED() << "Not known servie type in DataUseUserData"; On 2015/08/24 23:59:34, sclittle wrote: > nit: spelling "servie" Done. https://codereview.chromium.org/1279543002/diff/220001/components/data_use_me... File components/data_use_measurement/core/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/220001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.cc:17: namespace { On 2015/08/24 23:59:34, sclittle wrote: > nit: add blank line after "namespace {" Done. https://codereview.chromium.org/1279543002/diff/220001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.cc:41: } // Anonymous namespace. On 2015/08/24 23:59:34, sclittle wrote: > nit: this line should just be "} // namespace", don't need the "Anonymous" or > the period afterwards. Done. https://codereview.chromium.org/1279543002/diff/220001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.cc:141: std::string combinedHistogramName = "DataUse.Services" + DimensionMaker(dir); On 2015/08/24 23:59:34, sclittle wrote: > variable names should use underscores, not camel case. Done.
bengr@chromium.org changed reviewers: + mef@chromium.org
Hey Misha, Ali wrote up a whole component just so he'd have a place to put a function to label requests and a class to report data use by the label. Would it be ok for him to put the former in net/ and the latter in chrome/browser/net/ instead? If the former goes in net/, where do you think is a good place? net/base/? Thanks.
bengr@chromium.org changed reviewers: + jkarlin@chromium.org - mef@chromium.org
Hey Josh, Ali wrote up a whole component just so he'd have a place to put a function to label requests and a class to report data use by the label. Would it be ok for him to put the former in net/ and the latter in chrome/browser/net/ instead? If the former goes in net/, where do you think is a good place? net/base/? Thanks.
bengr@chromium.org changed reviewers: + rch@chromium.org - jkarlin@chromium.org
Hey Ryan, Ali wrote up a whole component just so he'd have a place to put a function to label requests and a class to report data use by the label. Would it be ok for him to put the former in net/ and the latter in chrome/browser/net/ instead? If the former goes in net/, where do you think is a good place? net/base/? Thanks.
On 2015/08/25 19:50:40, bengr wrote: > Hey Ryan, > Ali wrote up a whole component just so he'd have a place to put a function to > label requests and a class to report data use by the label. Would it be ok for > him to put the former in net/ and the latter in chrome/browser/net/ instead? If > the former goes in net/, where do you think is a good place? net/base/? Thanks. I'm not sure. Which code (in this CL) are you thinking would live in net/?
On 2015/08/25 23:13:35, Ryan Hamilton wrote: > On 2015/08/25 19:50:40, bengr wrote: > > Hey Ryan, > > Ali wrote up a whole component just so he'd have a place to put a function to > > label requests and a class to report data use by the label. Would it be ok for > > him to put the former in net/ and the latter in chrome/browser/net/ instead? > If > > the former goes in net/, where do you think is a good place? net/base/? > Thanks. > > I'm not sure. Which code (in this CL) are you thinking would live in net/? data_use_user_data.cc and data_use_user_data.h
On 2015/08/25 23:17:27, amohammadkhan wrote: > On 2015/08/25 23:13:35, Ryan Hamilton wrote: > > On 2015/08/25 19:50:40, bengr wrote: > > > Hey Ryan, > > > Ali wrote up a whole component just so he'd have a place to put a function > to > > > label requests and a class to report data use by the label. Would it be ok > for > > > him to put the former in net/ and the latter in chrome/browser/net/ instead? > > If > > > the former goes in net/, where do you think is a good place? net/base/? > > Thanks. > > > > I'm not sure. Which code (in this CL) are you thinking would live in net/? > > data_use_user_data.cc and data_use_user_data.h I don't see anything net related in those files. I don't think it would make sense to put them in net. Is there a net element to that code that I'm missing?
On 2015/08/26 00:00:40, Ryan Hamilton wrote: > On 2015/08/25 23:17:27, amohammadkhan wrote: > > On 2015/08/25 23:13:35, Ryan Hamilton wrote: > > > On 2015/08/25 19:50:40, bengr wrote: > > > > Hey Ryan, > > > > Ali wrote up a whole component just so he'd have a place to put a function > > to > > > > label requests and a class to report data use by the label. Would it be ok > > for > > > > him to put the former in net/ and the latter in chrome/browser/net/ > instead? > > > If > > > > the former goes in net/, where do you think is a good place? net/base/? > > > Thanks. > > > > > > I'm not sure. Which code (in this CL) are you thinking would live in net/? > > > > data_use_user_data.cc and data_use_user_data.h > > I don't see anything net related in those files. I don't think it would make > sense to put them in net. Is there a net element to that code that I'm missing? You are right. It may not be directly related to net, but services are using net/ and they can use this class to attach their tag to the requests and later this information can be used to investigate their activity in different layers, for example know we are measuring their data use in ChromeNetworkDelegate to see their data use in different network/app states. I thought it may be a good idea to have this class in net/, because net is what they are using to send their requests, so the helper class for tagging the request can exist in the same place without causing concerns about dependency issues.
On 2015/08/26 17:00:16, amohammadkhan wrote: > On 2015/08/26 00:00:40, Ryan Hamilton wrote: > > On 2015/08/25 23:17:27, amohammadkhan wrote: > > > On 2015/08/25 23:13:35, Ryan Hamilton wrote: > > > > On 2015/08/25 19:50:40, bengr wrote: > > > > > Hey Ryan, > > > > > Ali wrote up a whole component just so he'd have a place to put a > function > > > to > > > > > label requests and a class to report data use by the label. Would it be > ok > > > for > > > > > him to put the former in net/ and the latter in chrome/browser/net/ > > instead? > > > > If > > > > > the former goes in net/, where do you think is a good place? net/base/? > > > > Thanks. > > > > > > > > I'm not sure. Which code (in this CL) are you thinking would live in net/? > > > > > > data_use_user_data.cc and data_use_user_data.h > > > > I don't see anything net related in those files. I don't think it would make > > sense to put them in net. Is there a net element to that code that I'm > missing? > > You are right. It may not be directly related to net, but services are using > net/ and they can use this class to attach their tag to the requests and later > this information can be used to investigate their activity in different layers, > for example know we are measuring their data use in ChromeNetworkDelegate to see > their data use in different network/app states. I thought it may be a good idea > to have this class in net/, because net is what they are using to send their > requests, so the helper class for tagging the request can exist in the same > place without causing concerns about dependency issues. I see. Alas, I don't think net/ would be a good fit for this code since it really isn't directly related to networking. ChromeNetworkDelegate, as you know, isn't in net/ proper and I think this CL is in a similar boat.
https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... File components/data_use_measurement.gypi (right): https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement.gypi:7: 'target_name': 'data_use_measurement', On 2015/08/21 23:55:33, sclittle wrote: > separate the core/ and content/ stuff into different targets Done. https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... File components/data_use_measurement/content/data_use_user_data.h (right): https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/content/data_use_user_data.h:5: #ifndef COMPONENTS_DATA_USE_MEASUREMENT_CONTENT_DATA_USE_USER_DATA_H_ On 2015/08/21 23:55:34, sclittle wrote: > This file and it's .cc file should be in "data_use_measurement/core/", not > content. Done. https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... File components/data_use_measurement/core/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.cc:31: 5000000, // Maximum sample size. Should cover most of the requests. On 2015/08/24 23:59:34, sclittle wrote: > On 2015/08/24 23:18:48, amohammadkhan wrote: > > On 2015/08/21 23:55:34, sclittle wrote: > > > Why just 5 MB? Lots of fetches are more than 5 MB. > > > > I agree with you. What do you think is the good number here? My main concern > was > > that I wasn't sure what would happen to histogram's precision if I make this > > number very large as there is limited number of buckets? > > After discussing offline, it seems like the service requests are typically on > the order of kilobytes, so it should be fine to make this 1000000. Done. https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... File components/data_use_measurement/core/data_use_measurement.h (right): https://codereview.chromium.org/1279543002/diff/200001/components/data_use_me... components/data_use_measurement/core/data_use_measurement.h:6: #define COMPONENTS_DATA_USE_MEASUREMENT_H_ On 2015/08/21 23:55:34, sclittle wrote: > Move this file and its .cc to the "data_use_measurement/content" directory, > since it depends on content. Done. https://codereview.chromium.org/1279543002/diff/200001/components/suggestions... File components/suggestions.gypi (right): https://codereview.chromium.org/1279543002/diff/200001/components/suggestions... components/suggestions.gypi:23: 'components.gyp:data_use_measurement', On 2015/08/21 23:55:34, sclittle wrote: > You should only have to depend on the "core/" target of data_use_measurement > here. Done. https://codereview.chromium.org/1279543002/diff/200001/components/suggestions... File components/suggestions/DEPS (right): https://codereview.chromium.org/1279543002/diff/200001/components/suggestions... components/suggestions/DEPS:2: "+components/data_use_measurement", On 2015/08/21 23:55:34, sclittle wrote: > change to "+components/data_use_measurement/core" Done.
https://codereview.chromium.org/1279543002/diff/280001/chrome/browser/net/DEPS File chrome/browser/net/DEPS (right): https://codereview.chromium.org/1279543002/diff/280001/chrome/browser/net/DEP... chrome/browser/net/DEPS:3: "+components/data_use_measurement", Move this above the comment, since that comment applies to the below dep, not this one https://codereview.chromium.org/1279543002/diff/280001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/280001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:20: #include "components/data_use_measurement/content/data_use_user_data.h" Isn't data_use_user_data under "core/"? https://codereview.chromium.org/1279543002/diff/280001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:139: measurement_component::DataUseUserData::kUserDataKey, That namespace looks wrong, "measurement_component" https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... File components/data_use_measurement.gypi (right): https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement.gypi:12: '../net/net.gyp:net', You also need a dependency on data_use_measurement_core here. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... File components/data_use_measurement/BUILD.gn (right): https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/BUILD.gn:7: "content/data_use_user_data.cc", Please fix this for the different core and content targets. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:24: 1000000, // Maximum sample size. Should cover most of the requests. nit: Maybe specify in this comment that this should cover most of the requests by services. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:55: app_listener_.reset(); nit: add a comment here describing why you're destroying the app_listener_ before the DataUseMeasurement instance is destroyed, e.g. so that a callback isn't sent between the DataUseMeasurement instance being destroyed and the app_listener_ being destroyed. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:59: bool user_traffic_flag = false; nit: rename to |is_user_traffic| https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:126: std::string DataUseMeasurement::DimensionMaker(TrafficDirection dir) { nit: rename this method to be like a verb, i.e. GetDimensionForHistogramName or something like that. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:160: } // data_use_measurement namespace nit: change comment to "// namespace data_use_measurement" https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:33: void ReportDataUseUMA(const net::URLRequest* request); Could you write some simple tests for this method in data_use_measurement_unittest.cc? They could be as simple as just using a histogram tester and calling ReportDataUseUMA in all the different situations. You could also have an android specific test for the BACKGROUND, FOREGROUND case, that just calls OnApplicationStateChange(state) then checks that ReportDataUseUMA is reported appropriately for each state of BACKGROUND and FOREGROUND. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:89: } add a blank line above, and add a comment on the same line as the '}', e.g. "} // namespace data_use_measurement" https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... File components/data_use_measurement/core/DEPS (right): https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/core/DEPS:2: "+net/base", You don't actually need this dep here, so delete this DEPS file. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... File components/data_use_measurement/core/data_use_user_data.cc (right): https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.cc:7: namespace data_use_measurement { nit: add blank line after https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.cc:17: // Helper function to create DataUseUserData. Move this comment to the .h file. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... File components/data_use_measurement/core/data_use_user_data.h (right): https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:10: namespace data_use_measurement { nit: add blank line after https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:30: base::SupportsUserData::Data* CreateDataUseUserData( nit: separate from class with a blank line https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:30: base::SupportsUserData::Data* CreateDataUseUserData( Also, specify in a comment for this function that the caller takes ownership of the returned object. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:33: const char* ReturnServiceName(DataUseUserData::ServiceType service); nit: name seems weird, maybe "GetServiceName" instead https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:34: } // data_use_measurement namespace nit: add blank line before https://codereview.chromium.org/1279543002/diff/280001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1279543002/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5577: +<histogram name="DataUse.Service" units="bytes"> Naming is confusing, you have both "DataUse.Service" and "DataUse.Services", and both have the same description. Please give them more specific names and different descriptions.
https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... File components/data_use_measurement.gypi (right): https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement.gypi:1: # Copyright 2015 The Chromium Authors. All rights reserved. Also, you'll want to add an OWNERS file in the components/data_use_measurement directory that lists your email and Ben's. You'll also want to add to components/OWNERS, see how the other components are handled in that file.
rch@chromium.org changed reviewers: - rch@chromium.org
Removing myself as a reviewer. Please add me back, if you think there's a need for a net/ review.
amohammadkhan@chromium.org changed reviewers: + amohammadkhan@chromium.org, blundell@chromium.org
blundell@chromium.org: Please review changes in components/*
amohammadkhan@chromium.org changed reviewers: + asvitkine@chromium.org, mmenke@chromium.org
asvitkine@chromium.org: Please review histograms.xml mmenke@chromium.org: Please review changes in chrome/browser/net/*
https://codereview.chromium.org/1279543002/diff/280001/chrome/browser/net/DEPS File chrome/browser/net/DEPS (right): https://codereview.chromium.org/1279543002/diff/280001/chrome/browser/net/DEP... chrome/browser/net/DEPS:3: "+components/data_use_measurement", On 2015/08/26 23:08:29, sclittle wrote: > Move this above the comment, since that comment applies to the below dep, not > this one Done. https://codereview.chromium.org/1279543002/diff/280001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/280001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:20: #include "components/data_use_measurement/content/data_use_user_data.h" On 2015/08/26 23:08:29, sclittle wrote: > Isn't data_use_user_data under "core/"? Done. https://codereview.chromium.org/1279543002/diff/280001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:139: measurement_component::DataUseUserData::kUserDataKey, On 2015/08/26 23:08:29, sclittle wrote: > That namespace looks wrong, "measurement_component" Done. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... File components/data_use_measurement.gypi (right): https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement.gypi:1: # Copyright 2015 The Chromium Authors. All rights reserved. On 2015/08/26 23:11:26, sclittle wrote: > Also, you'll want to add an OWNERS file in the components/data_use_measurement > directory that lists your email and Ben's. > > You'll also want to add to components/OWNERS, see how the other components are > handled in that file. Done. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement.gypi:12: '../net/net.gyp:net', On 2015/08/26 23:08:29, sclittle wrote: > You also need a dependency on data_use_measurement_core here. Done. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... File components/data_use_measurement/BUILD.gn (right): https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/BUILD.gn:7: "content/data_use_user_data.cc", On 2015/08/26 23:08:29, sclittle wrote: > Please fix this for the different core and content targets. Done. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:24: 1000000, // Maximum sample size. Should cover most of the requests. On 2015/08/26 23:08:30, sclittle wrote: > nit: Maybe specify in this comment that this should cover most of the requests > by services. Done. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:55: app_listener_.reset(); On 2015/08/26 23:08:29, sclittle wrote: > nit: add a comment here describing why you're destroying the app_listener_ > before the DataUseMeasurement instance is destroyed, e.g. so that a callback > isn't sent between the DataUseMeasurement instance being destroyed and the > app_listener_ being destroyed. Done. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:59: bool user_traffic_flag = false; On 2015/08/26 23:08:29, sclittle wrote: > nit: rename to |is_user_traffic| Done. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:126: std::string DataUseMeasurement::DimensionMaker(TrafficDirection dir) { On 2015/08/26 23:08:30, sclittle wrote: > nit: rename this method to be like a verb, i.e. GetDimensionForHistogramName or > something like that. Done. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:160: } // data_use_measurement namespace On 2015/08/26 23:08:29, sclittle wrote: > nit: change comment to "// namespace data_use_measurement" Done. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:33: void ReportDataUseUMA(const net::URLRequest* request); On 2015/08/26 23:08:30, sclittle wrote: > Could you write some simple tests for this method in > data_use_measurement_unittest.cc? They could be as simple as just using a > histogram tester and calling ReportDataUseUMA in all the different situations. > > You could also have an android specific test for the BACKGROUND, FOREGROUND > case, that just calls OnApplicationStateChange(state) then checks that > ReportDataUseUMA is reported appropriately for each state of BACKGROUND and > FOREGROUND. I tried to have a complete test as much as possible. But the class providing connection type didn't provide any facility to test in a way that can be used by this code. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:89: } On 2015/08/26 23:08:30, sclittle wrote: > add a blank line above, and add a comment on the same line as the '}', > > e.g. "} // namespace data_use_measurement" Done. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... File components/data_use_measurement/core/DEPS (right): https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/core/DEPS:2: "+net/base", On 2015/08/26 23:08:30, sclittle wrote: > You don't actually need this dep here, so delete this DEPS file. Done. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... File components/data_use_measurement/core/data_use_user_data.cc (right): https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.cc:7: namespace data_use_measurement { On 2015/08/26 23:08:30, sclittle wrote: > nit: add blank line after Done. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.cc:17: // Helper function to create DataUseUserData. On 2015/08/26 23:08:30, sclittle wrote: > Move this comment to the .h file. Done. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... File components/data_use_measurement/core/data_use_user_data.h (right): https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:10: namespace data_use_measurement { On 2015/08/26 23:08:30, sclittle wrote: > nit: add blank line after Done. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:30: base::SupportsUserData::Data* CreateDataUseUserData( On 2015/08/26 23:08:30, sclittle wrote: > nit: separate from class with a blank line Done. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:30: base::SupportsUserData::Data* CreateDataUseUserData( On 2015/08/26 23:08:30, sclittle wrote: > Also, specify in a comment for this function that the caller takes ownership of > the returned object. Done. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:33: const char* ReturnServiceName(DataUseUserData::ServiceType service); On 2015/08/26 23:08:30, sclittle wrote: > nit: name seems weird, maybe "GetServiceName" instead Done. https://codereview.chromium.org/1279543002/diff/280001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:34: } // data_use_measurement namespace On 2015/08/26 23:08:30, sclittle wrote: > nit: add blank line before Done. https://codereview.chromium.org/1279543002/diff/280001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1279543002/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5577: +<histogram name="DataUse.Service" units="bytes"> On 2015/08/26 23:08:30, sclittle wrote: > Naming is confusing, you have both "DataUse.Service" and "DataUse.Services", and > both have the same description. Please give them more specific names and > different descriptions. I see your point, but DataUse.Service is never used by itself and a suffix is added to it always, like DataUse.Service.Suggestions, while the DataUse.Services doesn't have this suffixes, so I think in the UMA they should be more distinguishable, and I agree with you about descriptions, I updated them, please let me know if you still see ambiguity here.
https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... File components/data_use_measurement/BUILD.gn (right): https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/BUILD.gn:5: source_set("data_use_measurement_content") { It's preferred in GN to just put a BUILD.gn file in each of the subdirectories with the target for that subdirectory. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/BUILD.gn:13: "//net/base", the //net deps here don't seem to exactly match the one in gyp. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/BUILD.gn:24: "//net/base", the deps here don't match the ones in GYP. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:65: const content::ResourceRequestInfo* info = IIUC, the only usage of //content in this code is to check whether a request is coming from the user. I would move the DataUseMeasurement class to the core code and have it take in a DataUseMeasurementDelegate, which would be an interface defined in the core code with a //content-based implementation in //components/data_use_measurement/content. Currently this interface would have only one method: RequestIsFromUser(net::URLRequest* request). In the future the interface could expand as needed. This organization will set up the code better for sharing with iOS both now and going forward IMO.
https://codereview.chromium.org/1279543002/diff/340001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/340001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:213: extensions::EventRouterForwarder* forwarder() { If this compiles when ENABLE_EXTENSIONS is not defined, it seems like it's ok to reference extensions::EventRouterForwarder in that mode, in which case you can probably remove some of the other ifdefs - e.g. around the scoped_refptr and the body of this function. https://codereview.chromium.org/1279543002/diff/340001/chrome/browser/profile... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1279543002/diff/340001/chrome/browser/profile... chrome/browser/profiles/profile_manager.cc:462: ProfileManager* pm = g_browser_process->profile_manager(); Nit: Just inline this into the if, if you're only using this once. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:24: 1000000, // Maximum sample size. Should cover most of the requests by What's the units? https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:25: class DataUseMeasurement { Class should have a comment. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:28: // bind its callback. This comment is not useful. First, it seems it's describing implementation detail. But second, it doesn't even describe it very well - what kind of listener object - what's the callback - why is it doing it, etc. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:41: #endif Add an empty line after this. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:89: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:80: net::NetworkChangeNotifier::GetConnectionType())) Nit: {}'s https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... File components/data_use_measurement/core/data_use_user_data.h (right): https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:26: static const void* kUserDataKey; Add a comment about what this is. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:30: }; DISALLOW_COPY_AND_ASSIGN() https://codereview.chromium.org/1279543002/diff/340001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1279543002/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5631: + Renderer process. Please rephrase the last sentence, as it's a bit confusing. Do you mean this only measures traffic requested by the browser process and not other processes (e.g. renderer)? (If so, phrase it in that way.) https://codereview.chromium.org/1279543002/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5635: +<histogram name="DataUse.Service" units="bytes"> I suggest naming this in a way that references it's meaning - e.g. DataUse.MessageSize. The fact that it's by service is implified by the specific suffixes. https://codereview.chromium.org/1279543002/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5639: + The request and reponse size of the messages exchanged by a service. The Over what period of time? When is it logged? https://codereview.chromium.org/1279543002/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5644: +<histogram name="DataUse.Services" enum="DataUseServices"> Same comment as for DataUse.Service. https://codereview.chromium.org/1279543002/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:74583: + <suffix name="Upload.Background.NotCellular"/> Nit: You can have multiple levels of histogram_suffixes - just make affected-histogram refer to the suffixed versions.
Perliminary comments. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:84: request_body_bytes = request->get_upload()->size(); This is -1 for chunked uploads, which a lot of internal requests are. I don't think there's currently any reliable way to measure upstream traffic. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:86: if (request->GetFullRequestHeaders(&request_headers)) This method doesn't really work - it's not fully hooked up, or reliable. You shouldn't be using this. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:87: request_header_bytes = request_headers.ToString().length(); Even if the method worked, this is only true for HTTP and SPDY. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:94: DOWNLOAD, total_received_bytes); Having "service traffic" an independent dimension from attached_service_data seems confusing, and doubling up on UMA seems wasteful, with sparse histograms. My suggestion: Put SERVICE_TRAFFIC with no known service type into one bucket, and bucket SERVICE_TRAFFIC with attached service data into the bucket corresponding to that service. Remove the redundant ReportDataUsageURLRequest / ReportDataUsage calls. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:122: DataUseMeasurement::ConnectionType DataUseMeasurement::CurrentConnectionType() { I don't think having this as a method, or mapping one enum to another, really gets you anything here. Suggest just inlining the logic in GetDimensionForHistogramName, and using a bool rather than an enum. Mapping the enum returned by net::NetworkChangeNotifier::GetConnectionType to a bool to an enum and then using that as a bool just seems a bit much. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:130: std::string DataUseMeasurement::GetDimensionForHistogramName( Dimension -> Suffix? This results a string,not dimensions. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:135: ".%s.%s.%s", dir == UPLOAD ? "Upload" : "Download", This isn't categorizing requests as uploads or downloads, right? It's for bytes in the two direction. upstream and downstream would make that clearer. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:147: "DataUse.Services" + GetDimensionForHistogramName(dir); Can't these two lines be combined, like in ReportDataUsageURLRequest? https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:9: #include <string> Blank line needed between C++ and C headers. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:25: class DataUseMeasurement { Need class level docs. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:40: base::android::ApplicationState application_state); If this is only public for tests, I suggest making it a private method, and then making a public method called OnApplicationStateChangeForTesting / OnApplicationStateChangeFors, which calls it. That makes it clearer it's not part of the public API.
https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:87: request_header_bytes = request_headers.ToString().length(); On 2015/08/31 17:41:28, mmenke wrote: > Even if the method worked, this is only true for HTTP and SPDY. "For HTTP, and not SPDY (Or QUIC)", rather
https://codereview.chromium.org/1279543002/diff/340001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/340001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:213: extensions::EventRouterForwarder* forwarder() { On 2015/08/31 17:26:31, Alexei Svitkine wrote: > If this compiles when ENABLE_EXTENSIONS is not defined, it seems like it's ok to > reference extensions::EventRouterForwarder in that mode, in which case you can > probably remove some of the other ifdefs - e.g. around the scoped_refptr and the > body of this function. Done. (I changed it, but after running tests on try bot, it gives me some errors, so I think we need ifdefs around those parts. (If ENABLE_EXTENSIONS is not defines, extensions::EventRouterForwarder is not a complete type, so for example we cannot call its constructor or destructor) https://codereview.chromium.org/1279543002/diff/340001/chrome/browser/profile... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1279543002/diff/340001/chrome/browser/profile... chrome/browser/profiles/profile_manager.cc:462: ProfileManager* pm = g_browser_process->profile_manager(); On 2015/08/31 17:26:31, Alexei Svitkine wrote: > Nit: Just inline this into the if, if you're only using this once. Done. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... File components/data_use_measurement/BUILD.gn (right): https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/BUILD.gn:5: source_set("data_use_measurement_content") { On 2015/08/31 09:15:40, blundell wrote: > It's preferred in GN to just put a BUILD.gn file in each of the subdirectories > with the target for that subdirectory. Done. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/BUILD.gn:13: "//net/base", On 2015/08/31 09:15:40, blundell wrote: > the //net deps here don't seem to exactly match the one in gyp. Done. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/BUILD.gn:24: "//net/base", On 2015/08/31 09:15:40, blundell wrote: > the deps here don't match the ones in GYP. Done. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:24: 1000000, // Maximum sample size. Should cover most of the requests by On 2015/08/31 17:26:31, Alexei Svitkine wrote: > What's the units? Done. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:65: const content::ResourceRequestInfo* info = On 2015/08/31 09:15:40, blundell wrote: > IIUC, the only usage of //content in this code is to check whether a request is > coming from the user. I would move the DataUseMeasurement class to the core code > and have it take in a DataUseMeasurementDelegate, which would be an interface > defined in the core code with a //content-based implementation in > //components/data_use_measurement/content. Currently this interface would have > only one method: RequestIsFromUser(net::URLRequest* request). In the future the > interface could expand as needed. > > This organization will set up the code better for sharing with iOS both now and > going forward IMO. Thanks for heads up. I see your point. But currently IOS is out of scope of our project and our main focus is on Android. So if you think it is not necessary, I thought it may be better to land this CL without this change and start getting data before I run out of time in my internship and improve the architecture later For example while I am getting data (of course if you don't think this change is crucial now) https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:84: request_body_bytes = request->get_upload()->size(); On 2015/08/31 17:41:29, mmenke wrote: > This is -1 for chunked uploads, which a lot of internal requests are. I don't > think there's currently any reliable way to measure upstream traffic. After reading your comment, I wanted to try to understand the cases which the request is chunked but I couldn't find any cases. (I checked by using, request->get_upload()->is_chunked() ) Is it a right function to check if the upload is chunked? If yes, do you know why I couldn't find any chunked upload? Are they more common in specific cases? I know my method, for getting byte sizes for upload and even download is not 100% accurate, but do you think have they the minimum accuracy to get a high level understanding to compare data use of different services? When I manually check the numbers, they seem reasonable. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:86: if (request->GetFullRequestHeaders(&request_headers)) On 2015/08/31 17:41:29, mmenke wrote: > This method doesn't really work - it's not fully hooked up, or reliable. You > shouldn't be using this. Do you have any suggestion for a better substitute? Or you think we do not have any good option to do it? https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:87: request_header_bytes = request_headers.ToString().length(); On 2015/08/31 17:41:28, mmenke wrote: > Even if the method worked, this is only true for HTTP and SPDY. I see your points. For now I am trying to get a rough estimation of data use of different services, but I've heard that total send and total received bytes are getting updated and accurate by someone else, so later I'll be able to update my code and benefit those updates. But till then, I am trying to have some estimation about them. So please let me know if you think if there is any good way without doing complex plumbing to increase the accuracy. (Do you think can we have a rough estimation by current values or the current values are not even accurate enough for rough estimation?) https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:87: request_header_bytes = request_headers.ToString().length(); On 2015/08/31 17:42:06, mmenke wrote: > On 2015/08/31 17:41:28, mmenke wrote: > > Even if the method worked, this is only true for HTTP and SPDY. > > "For HTTP, and not SPDY (Or QUIC)", rather I agree. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:94: DOWNLOAD, total_received_bytes); On 2015/08/31 17:41:29, mmenke wrote: > Having "service traffic" an independent dimension from attached_service_data > seems confusing, and doubling up on UMA seems wasteful, with sparse histograms. > > My suggestion: Put SERVICE_TRAFFIC with no known service type into one bucket, > and bucket SERVICE_TRAFFIC with attached service data into the bucket > corresponding to that service. Remove the redundant ReportDataUsageURLRequest / > ReportDataUsage calls. Good idea. I made a bucket for all untagged services ("NotTagged"), but it may still be good to keep the SERVICE_TRAFFIC, because it will have the traffic of all services for different dimension that we do not have in detail for services. (To reduce the number of histograms, just we have total exchanged bytes for each service in each dimension and we do not have the distribution of exchanged bytes) https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:122: DataUseMeasurement::ConnectionType DataUseMeasurement::CurrentConnectionType() { On 2015/08/31 17:41:29, mmenke wrote: > I don't think having this as a method, or mapping one enum to another, really > gets you anything here. Suggest just inlining the logic in > GetDimensionForHistogramName, and using a bool rather than an enum. Mapping the > enum returned by net::NetworkChangeNotifier::GetConnectionType to a bool to an > enum and then using that as a bool just seems a bit much. Done. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:130: std::string DataUseMeasurement::GetDimensionForHistogramName( On 2015/08/31 17:41:29, mmenke wrote: > Dimension -> Suffix? This results a string,not dimensions. Done. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:135: ".%s.%s.%s", dir == UPLOAD ? "Upload" : "Download", On 2015/08/31 17:41:29, mmenke wrote: > This isn't categorizing requests as uploads or downloads, right? It's for bytes > in the two direction. upstream and downstream would make that clearer. Done. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:147: "DataUse.Services" + GetDimensionForHistogramName(dir); On 2015/08/31 17:41:29, mmenke wrote: > Can't these two lines be combined, like in ReportDataUsageURLRequest? Our ideal was to have all dimensions for all services, but finally we compromised to these two histogram types: DataUse.Service.ServiceName shows distribution of exchanged data for all dimensions combined. DataUse.Services.Dimensions (Have ServiceNames as buckets) To have total dataUse of services in different dimensions. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:9: #include <string> On 2015/08/31 17:41:29, mmenke wrote: > Blank line needed between C++ and C headers. Done. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:25: class DataUseMeasurement { On 2015/08/31 17:26:31, Alexei Svitkine wrote: > Class should have a comment. Done. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:25: class DataUseMeasurement { On 2015/08/31 17:26:31, Alexei Svitkine wrote: > Class should have a comment. Done. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:28: // bind its callback. On 2015/08/31 17:26:31, Alexei Svitkine wrote: > This comment is not useful. > > First, it seems it's describing implementation detail. But second, it doesn't > even describe it very well - what kind of listener object - what's the callback > - why is it doing it, etc. Done. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:40: base::android::ApplicationState application_state); On 2015/08/31 17:41:29, mmenke wrote: > If this is only public for tests, I suggest making it a private method, and then > making a public method called OnApplicationStateChangeForTesting / > OnApplicationStateChangeFors, which calls it. That makes it clearer it's not > part of the public API. Done. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:41: #endif On 2015/08/31 17:26:31, Alexei Svitkine wrote: > Add an empty line after this. Done. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:89: }; On 2015/08/31 17:26:31, Alexei Svitkine wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:80: net::NetworkChangeNotifier::GetConnectionType())) On 2015/08/31 17:26:31, Alexei Svitkine wrote: > Nit: {}'s Done. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... File components/data_use_measurement/core/data_use_user_data.h (right): https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:26: static const void* kUserDataKey; On 2015/08/31 17:26:31, Alexei Svitkine wrote: > Add a comment about what this is. Done. https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:30: }; On 2015/08/31 17:26:31, Alexei Svitkine wrote: > DISALLOW_COPY_AND_ASSIGN() Done. https://codereview.chromium.org/1279543002/diff/340001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1279543002/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5631: + Renderer process. On 2015/08/31 17:26:31, Alexei Svitkine wrote: > Please rephrase the last sentence, as it's a bit confusing. > > Do you mean this only measures traffic requested by the browser process and not > other processes (e.g. renderer)? (If so, phrase it in that way.) Done. https://codereview.chromium.org/1279543002/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5635: +<histogram name="DataUse.Service" units="bytes"> On 2015/08/31 17:26:31, Alexei Svitkine wrote: > I suggest naming this in a way that references it's meaning - e.g. > DataUse.MessageSize. The fact that it's by service is implified by the specific > suffixes. Done. https://codereview.chromium.org/1279543002/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5639: + The request and reponse size of the messages exchanged by a service. The On 2015/08/31 17:26:31, Alexei Svitkine wrote: > Over what period of time? When is it logged? Done. https://codereview.chromium.org/1279543002/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5644: +<histogram name="DataUse.Services" enum="DataUseServices"> On 2015/08/31 17:26:31, Alexei Svitkine wrote: > Same comment as for DataUse.Service. For this one, If I remove "services", no ServiceName is attached to it, so it may be confusing if someone just look at its name. (ServiceNames are just buckets, so they are not reflected in its name.) Added comment about when it is logged. https://codereview.chromium.org/1279543002/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:74583: + <suffix name="Upload.Background.NotCellular"/> On 2015/08/31 17:26:31, Alexei Svitkine wrote: > Nit: You can have multiple levels of histogram_suffixes - just make > affected-histogram refer to the suffixed versions. I see, but I just have two different suffixes in each level and I think using multiple level suffixes may even make it harder to implement/understand.
> https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... > components/data_use_measurement/content/data_use_measurement.cc:65: const > content::ResourceRequestInfo* info = > On 2015/08/31 09:15:40, blundell wrote: > > IIUC, the only usage of //content in this code is to check whether a request > is > > coming from the user. I would move the DataUseMeasurement class to the core > code > > and have it take in a DataUseMeasurementDelegate, which would be an interface > > defined in the core code with a //content-based implementation in > > //components/data_use_measurement/content. Currently this interface would have > > only one method: RequestIsFromUser(net::URLRequest* request). In the future > the > > interface could expand as needed. > > > > This organization will set up the code better for sharing with iOS both now > and > > going forward IMO. > > Thanks for heads up. I see your point. But currently IOS is out of scope of our > project and our main focus is on Android. So if you think it is not necessary, I > thought it may be better to land this CL without this change and start getting > data before I run out of time in my internship and improve the architecture > later For example while I am getting data (of course if you don't think this > change is crucial now) > Will this code eventually be used on iOS? (or is it plausible at least to imagine that will be the case)? If not, it seems like the component shouldn't have to be a layered component at all.
On 2015/09/01 08:14:18, blundell wrote: > > > https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... > > components/data_use_measurement/content/data_use_measurement.cc:65: const > > content::ResourceRequestInfo* info = > > On 2015/08/31 09:15:40, blundell wrote: > > > IIUC, the only usage of //content in this code is to check whether a request > > is > > > coming from the user. I would move the DataUseMeasurement class to the core > > code > > > and have it take in a DataUseMeasurementDelegate, which would be an > interface > > > defined in the core code with a //content-based implementation in > > > //components/data_use_measurement/content. Currently this interface would > have > > > only one method: RequestIsFromUser(net::URLRequest* request). In the future > > the > > > interface could expand as needed. > > > > > > This organization will set up the code better for sharing with iOS both now > > and > > > going forward IMO. > > > > Thanks for heads up. I see your point. But currently IOS is out of scope of > our > > project and our main focus is on Android. So if you think it is not necessary, > I > > thought it may be better to land this CL without this change and start getting > > data before I run out of time in my internship and improve the architecture > > later For example while I am getting data (of course if you don't think this > > change is crucial now) > > > > Will this code eventually be used on iOS? (or is it plausible at least to > imagine that will be the case)? If not, it seems like the component shouldn't > have to be a layered component at all. It's a component mostly because the plan is for a bunch of services (some in other components) to register their requests with it (So it can't be in Chrome), for tracking purposes, and the feeling this doesn't make sense as a content/ or net/ API.
On 2015/09/01 15:41:19, mmenke wrote: > On 2015/09/01 08:14:18, blundell wrote: > > > > > > https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... > > > components/data_use_measurement/content/data_use_measurement.cc:65: const > > > content::ResourceRequestInfo* info = > > > On 2015/08/31 09:15:40, blundell wrote: > > > > IIUC, the only usage of //content in this code is to check whether a > request > > > is > > > > coming from the user. I would move the DataUseMeasurement class to the > core > > > code > > > > and have it take in a DataUseMeasurementDelegate, which would be an > > interface > > > > defined in the core code with a //content-based implementation in > > > > //components/data_use_measurement/content. Currently this interface would > > have > > > > only one method: RequestIsFromUser(net::URLRequest* request). In the > future > > > the > > > > interface could expand as needed. > > > > > > > > This organization will set up the code better for sharing with iOS both > now > > > and > > > > going forward IMO. > > > > > > Thanks for heads up. I see your point. But currently IOS is out of scope of > > our > > > project and our main focus is on Android. So if you think it is not > necessary, > > I > > > thought it may be better to land this CL without this change and start > getting > > > data before I run out of time in my internship and improve the architecture > > > later For example while I am getting data (of course if you don't think this > > > change is crucial now) > > > > > > > Will this code eventually be used on iOS? (or is it plausible at least to > > imagine that will be the case)? If not, it seems like the component shouldn't > > have to be a layered component at all. > > It's a component mostly because the plan is for a bunch of services (some in > other components) to register their requests with it (So it can't be in Chrome), > for tracking purposes, and the feeling this doesn't make sense as a content/ or > net/ API. I mean a *layered* component specifically. i.e., if it won't be targeting iOS even in the long-term, no need to separate out into core/ and content/ subdirs: the entire component could depend on //content and just not be built/used on iOS. Conversely, if it *would* be useful in the medium-term to have this code on iOS, I think that the division between core/ and content/ is not cut optimally, as I explained above (but I'd be OK with seeing that changed in a followup CL).
On 2015/09/01 15:43:55, blundell wrote: > On 2015/09/01 15:41:19, mmenke wrote: > > On 2015/09/01 08:14:18, blundell wrote: > > > > > > > > > > https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... > > > > components/data_use_measurement/content/data_use_measurement.cc:65: const > > > > content::ResourceRequestInfo* info = > > > > On 2015/08/31 09:15:40, blundell wrote: > > > > > IIUC, the only usage of //content in this code is to check whether a > > request > > > > is > > > > > coming from the user. I would move the DataUseMeasurement class to the > > core > > > > code > > > > > and have it take in a DataUseMeasurementDelegate, which would be an > > > interface > > > > > defined in the core code with a //content-based implementation in > > > > > //components/data_use_measurement/content. Currently this interface > would > > > have > > > > > only one method: RequestIsFromUser(net::URLRequest* request). In the > > future > > > > the > > > > > interface could expand as needed. > > > > > > > > > > This organization will set up the code better for sharing with iOS both > > now > > > > and > > > > > going forward IMO. > > > > > > > > Thanks for heads up. I see your point. But currently IOS is out of scope > of > > > our > > > > project and our main focus is on Android. So if you think it is not > > necessary, > > > I > > > > thought it may be better to land this CL without this change and start > > getting > > > > data before I run out of time in my internship and improve the > architecture > > > > later For example while I am getting data (of course if you don't think > this > > > > change is crucial now) > > > > > > > > > > Will this code eventually be used on iOS? (or is it plausible at least to > > > imagine that will be the case)? If not, it seems like the component > shouldn't > > > have to be a layered component at all. > > > > It's a component mostly because the plan is for a bunch of services (some in > > other components) to register their requests with it (So it can't be in > Chrome), > > for tracking purposes, and the feeling this doesn't make sense as a content/ > or > > net/ API. > > I mean a *layered* component specifically. i.e., if it won't be targeting iOS > even in the long-term, no need to separate out into core/ and content/ subdirs: > the entire component could depend on //content and just not be built/used on > iOS. Conversely, if it *would* be useful in the medium-term to have this code on > iOS, I think that the division between core/ and content/ is not cut optimally, > as I explained above (but I'd be OK with seeing that changed in a followup CL). For now our focus is just on Android, but I think it doesn't hurt to have layered architecture to be able to extend the measurements to IOS too, but as I told you I am in hurry for this CL to get data back sooner to be able to analyze them in the rest of my internship and I think I will have the followup CL during the time I am waiting for getting data.
Here are some comments. More to come... https://codereview.chromium.org/1279543002/diff/440001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/1279543002/diff/440001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate.h:206: // Measuring component to measure data use of services. // Component to measure data use. https://codereview.chromium.org/1279543002/diff/440001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate.h:207: data_use_measurement::DataUseMeasurement data_use_measurement_; Can we call this component 'data_use'? I can imagine expanding this component to include lots of useful classes for data use accounting. https://codereview.chromium.org/1279543002/diff/440001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/440001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:186: // This function queries a URLRequest. If |from_user| is true, so it attaches so it -> it the request assumed -> the request is assumed What does "the request ... attaches a DataUseUserData" mean? Do you mean "If |from_user| is false, the request is presumed to be from a service, and the service name is set in the request's user data." https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... File components/data_use_measurement/OWNERS (right): https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/OWNERS:3: amohammadkhan@chromium.org Can you be an owner and not a committer? https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... File components/data_use_measurement/content/BUILD.gn (right): https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights reserved. The year is 2015. :) https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:13: #include "build/build_config.h" Why do you need this? https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:26: // The class records the data use of different services and user in according // Records the data use of user traffic and various services in UMA histograms. The UMA is broken down by network technology used (Wi-Fi vs cellular). On Android, the UMA is further broken down by whether the app was in the background or foreground during the request. Is the foreground check at the start or end of the request? https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:29: class DataUseMeasurement { Is this an IO-thread object? https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:35: // Records the data use of the |request|. , thus |request| must be non-null. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:41: void OnApplicationStateChangeForTesting( // Called whenever the application transitions from foreground to background and vice versa. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:49: void OnApplicationStateChange( Why is there a separate method for testing? https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:58: enum AppState { BACKGROUND, FOREGROUND }; Does this really need to be an enum? https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:60: // The reason a request was sent. Whether the traffic originates from The comment is more confusing than the enum. // The request is from a user or from a service. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:62: enum ChromeTrafficType { USER_TRAFFIC, SERVICE_TRAFFIC }; Can you name this RequestInitiator { REQUEST_INITIATOR_USER, REQUEST_INITIATOR_SERVICE } https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:66: AppState CurrentAppState(); Can't this just return a bool? IsAppInBackground() https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:68: // The function produces a string representing the status of the Chrome when // Produces a string representing whether the data use was on the send ("Upload") or receive ("Download") path, whether the app was in the "Foreground" or "Background", and whether a "Cellular" or "WiFi" network was use. For example, "Upload.Foreground.Cellular" is one possible output. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:73: // Helper function used to record data use of services. Describe what these helper functions do and what their inputs are. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:89: // ApplicationStatusListener used to monitor that application is in foreground that -> whether the in foreground or in background -> in the foreground or in the background
https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... File components/data_use_measurement/OWNERS (right): https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/OWNERS:3: amohammadkhan@chromium.org On 2015/09/01 17:00:10, bengr wrote: > Can you be an owner and not a committer? No, or at least you shouldn't be, since you can't actually do reviews.
https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... File components/data_use_measurement/core/data_use_user_data.h (right): https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:14: class DataUseUserData : public base::SupportsUserData::Data { UrlRequestSourceInfo? https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:19: enum ServiceType { Maybe rename to "source" and add a label for "OTHER_PROCESS"? Then wouldn't need to care about ResourceRequestInfo anywhere, ChromeResourceDispatcherHostDelegate could just tag everything it sees. Could even separate out by process type, if it were useful. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:40: DataUseUserData::ServiceType service); Rather than exposing the details of how this works (kUserDataKey, etc), I'd suggest AttachSoureTypeToRequest(URLRequest*, SourceType) / AttachDataUserUserDataToFetcher(URLFetcher*, SourceType) and RecordUsageInformationForRequest(URLRequest*) (The last would be a method of the other class).
https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/340001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:84: request_body_bytes = request->get_upload()->size(); On 2015/09/01 04:52:48, amohammadkhan wrote: > On 2015/08/31 17:41:29, mmenke wrote: > > This is -1 for chunked uploads, which a lot of internal requests are. I don't > > think there's currently any reliable way to measure upstream traffic. > > After reading your comment, I wanted to try to understand the cases which the > request is chunked but I couldn't find any cases. (I checked by using, > request->get_upload()->is_chunked() ) Is it a right function to check if the > upload is chunked? If yes, do you know why I couldn't find any chunked upload? > Are they more common in specific cases? > > I know my method, for getting byte sizes for upload and even download is not > 100% accurate, but do you think have they the minimum accuracy to get a high > level understanding to compare data use of different services? When I manually > check the numbers, they seem reasonable. sclittle is working on adding methods to expose a reasonable approximation of bytes sense, which works like GetTotalReceivedBytes. Didn't know about the effort when I made that comment. The stuff he's working on is definitely the method you want to use.
https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:35: // Records the data use of the |request|. On 2015/09/01 17:00:11, bengr wrote: > , thus |request| must be non-null. Or could take it as const net::URLRequest& request, which implies it can't be NULL.
https://codereview.chromium.org/1279543002/diff/440001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/1279543002/diff/440001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate.h:206: // Measuring component to measure data use of services. On 2015/09/01 17:00:10, bengr wrote: > // Component to measure data use. Done. https://codereview.chromium.org/1279543002/diff/440001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate.h:207: data_use_measurement::DataUseMeasurement data_use_measurement_; On 2015/09/01 17:00:10, bengr wrote: > Can we call this component 'data_use'? I can imagine expanding this component to > include lots of useful classes for data use accounting. I'll update the name in a followup CL. Even currently I think the name is general enough to cover your other data use measurements. https://codereview.chromium.org/1279543002/diff/440001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/440001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:186: // This function queries a URLRequest. If |from_user| is true, so it attaches On 2015/09/01 17:00:10, bengr wrote: > so it -> it > the request assumed -> the request is assumed > > What does "the request ... attaches a DataUseUserData" mean? Do you mean "If > |from_user| is false, the request is presumed to be from a service, and the > service name is set in the request's user data." > Done. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... File components/data_use_measurement/OWNERS (right): https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/OWNERS:3: amohammadkhan@chromium.org On 2015/09/01 17:00:10, bengr wrote: > Can you be an owner and not a committer? I am not sure, do you think is it better to remove my name from OWNERS file? https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... File components/data_use_measurement/content/BUILD.gn (right): https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2015/09/01 17:00:10, bengr wrote: > The year is 2015. :) Done. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:13: #include "build/build_config.h" On 2015/09/01 17:00:11, bengr wrote: > Why do you need this? I was getting some errors, because OS_ANDROID was not defined here when it should have been defined and the problem was fixed by adding this. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:26: // The class records the data use of different services and user in according On 2015/09/01 17:00:11, bengr wrote: > // Records the data use of user traffic and various services in UMA histograms. > The UMA is broken down by network technology used (Wi-Fi vs cellular). On > Android, the UMA is further broken down by whether the app was in the background > or foreground during the request. > > > Is the foreground check at the start or end of the request? At the end of request. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:29: class DataUseMeasurement { On 2015/09/01 17:00:11, bengr wrote: > Is this an IO-thread object? I think it is (If ChromeNetworkDelegate is IO-thread too) https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:35: // Records the data use of the |request|. On 2015/09/01 17:00:11, bengr wrote: > , thus |request| must be non-null. Done. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:41: void OnApplicationStateChangeForTesting( On 2015/09/01 17:00:11, bengr wrote: > // Called whenever the application transitions from foreground to background and > vice versa. Done. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:49: void OnApplicationStateChange( On 2015/09/01 17:00:11, bengr wrote: > Why is there a separate method for testing? To have the testing method public while this method is not public and it should not be used by other classes. (Based on one of reviews) https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:58: enum AppState { BACKGROUND, FOREGROUND }; On 2015/09/01 17:00:11, bengr wrote: > Does this really need to be an enum? Basically apps have 4 states, but we are using 2 of them here. So having this as an enum make it possible to change it later based on later needs. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:60: // The reason a request was sent. Whether the traffic originates from On 2015/09/01 17:00:11, bengr wrote: > The comment is more confusing than the enum. > > // The request is from a user or from a service. Done. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:62: enum ChromeTrafficType { USER_TRAFFIC, SERVICE_TRAFFIC }; On 2015/09/01 17:00:11, bengr wrote: > Can you name this RequestInitiator { REQUEST_INITIATOR_USER, > REQUEST_INITIATOR_SERVICE } Done. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:66: AppState CurrentAppState(); On 2015/09/01 17:00:11, bengr wrote: > Can't this just return a bool? IsAppInBackground() similar reason to having the enum for it. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:68: // The function produces a string representing the status of the Chrome when On 2015/09/01 17:00:11, bengr wrote: > // Produces a string representing whether the data use was on the send > ("Upload") or receive ("Download") path, whether the app was in the "Foreground" > or "Background", and whether a "Cellular" or "WiFi" network was use. For > example, "Upload.Foreground.Cellular" is one possible output. Done. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:73: // Helper function used to record data use of services. On 2015/09/01 17:00:11, bengr wrote: > Describe what these helper functions do and what their inputs are. Done. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:89: // ApplicationStatusListener used to monitor that application is in foreground On 2015/09/01 17:00:11, bengr wrote: > that -> whether the > in foreground or in background -> in the foreground or in the background Done. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... File components/data_use_measurement/core/data_use_user_data.h (right): https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:14: class DataUseUserData : public base::SupportsUserData::Data { On 2015/09/01 17:52:37, mmenke wrote: > UrlRequestSourceInfo? I think if I don't make changes regarding your other comment, the current name still makes sense. So I will change the name if later we decide to apply you other comment about using ChromeResourceDispatcherHostDelegate and cover all the sources by the enum of this class. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:19: enum ServiceType { On 2015/09/01 17:52:37, mmenke wrote: > Maybe rename to "source" and add a label for "OTHER_PROCESS"? Then wouldn't > need to care about ResourceRequestInfo anywhere, > ChromeResourceDispatcherHostDelegate could just tag everything it sees. Could > even separate out by process type, if it were useful. It is very good that we can get rid of ResourceRequestInfo. But for now I prefer to keep current way of detecting user data by using ResourceRequestInfo unless one of reviewers thinks there is a big drawback in the current approach. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:40: DataUseUserData::ServiceType service); On 2015/09/01 17:52:37, mmenke wrote: > Rather than exposing the details of how this works (kUserDataKey, etc), > > I'd suggest AttachSoureTypeToRequest(URLRequest*, SourceType) / > AttachDataUserUserDataToFetcher(URLFetcher*, SourceType) and > RecordUsageInformationForRequest(URLRequest*) (The last would be a method of > the other class). Good idea. I made the second function and used it in suggestion services too and we already had the third function(But its name is a little different) and for now we aren't attaching sourceType to any request, so I think it can be added later.
amohammadkhan@google.com changed reviewers: + thestig@chromium.org
thestig@chromium.org: Please review changes in Chrome/Browser/BUILD.gn Thanks,
https://codereview.chromium.org/1279543002/diff/460001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/460001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:160: class ChromeNetworkDelegateDataUseMeasurementTest : public testing::Test { A lot of this class is similar to ChromeNetworkDelegateSafeSearchTest. Can you add a common base-class to reduce the code duplication? https://codereview.chromium.org/1279543002/diff/460001/chrome/browser/profile... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1279543002/diff/460001/chrome/browser/profile... chrome/browser/profiles/profile_manager.cc:463: if (!(g_browser_process->profile_manager())) Nit: No need for extra ()'s https://codereview.chromium.org/1279543002/diff/460001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/460001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:5: #include <components/data_use_measurement/content/data_use_measurement.h> Nit: <> to "" Add an empty line after. https://codereview.chromium.org/1279543002/diff/460001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:23: 1, // Minimum sample size in Bytes. Nit: No need to capitalize bytes. Same below. https://codereview.chromium.org/1279543002/diff/460001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:61: } Nit: Add an empty line after this. https://codereview.chromium.org/1279543002/diff/460001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:150: "DataUse.Services" + GetSuffixForHistogramName(dir); How about DataUse.MessageSize.AllServices.*? https://codereview.chromium.org/1279543002/diff/460001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/1279543002/diff/460001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:33: Nit: No empty line. https://codereview.chromium.org/1279543002/diff/460001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/460001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:87: base::MessageLoopForIO loop_; Nit: Put these in private: section and add DISALLOW_COPY_AND_ASSIGN(). https://codereview.chromium.org/1279543002/diff/460001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:111: #if defined(OS_ANDROID) You don't need this inner ifdef since you have an identical outer one. https://codereview.chromium.org/1279543002/diff/460001/components/data_use_me... File components/data_use_measurement/core/data_use_user_data.h (right): https://codereview.chromium.org/1279543002/diff/460001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:44: base::SupportsUserData::Data* CreateDataUseUserData( Why not have this as a static Create() on the class? Also, similar question for the other methods - seems if they're related to the class, they should be static? https://codereview.chromium.org/1279543002/diff/460001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1279543002/diff/460001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5715: +<histogram name="DataUse.User" units="bytes"> Similar comment I've had before. DataUse.User doesn't tell me what's being measured. How about DataUse.TrafficSize.User or similar. The other one can also use the same pattern - though I wonder if renaming "NotUser" to something like "System" or "Chrome" might be clearer.
https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:5: #include <components/data_use_measurement/content/data_use_measurement.h> #include "components/.../data_use_measurement.h" and add a blank line after it https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:58: // being destroyed and the |app_listener_| being destroyed. // Prevent notification between the destruction of the DataUseMeasurement instance and the destruction of |app_listener_|. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:72: if (info) { bool is_user_traffic = info != nullptr; https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:76: // These number won't be the number of bytes handed to the kernel because // Counts rely on URLRequest::GetTotalReceivedBytes(), which does not include the send path, network layer overhead, TLS overhead, and DNS. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:126: // If the OS is not Android, all the requests are considered Foreground. Make sure this is documented in histograms.xml. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:130: std::string DataUseMeasurement::GetSuffixForHistogramName( The method should be const. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:148: "DataUse.Services" + GetSuffixForHistogramName(dir); Why does this prefix not end with '.' and the other prefix does? Be consistent. And make sure your documentation matches. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:149: RecordUMAHistogramCount(service_histogram_name, message_size); I would get rid of service_histogram_name and replace with "DataUse.MessageSize." + GetServiceName(). https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:161: "DataUse.%s%s", service_type == USER_TRAFFIC ? "User" : "NotUser", NotUser -> Service? https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:68: -2, -2, true, false, true, true); What are these -2's and other parameters? I.e., why did you comment on one of the params but not the others?
On 2015/09/01 19:58:04, amohammadkhan wrote: > mailto:thestig@chromium.org: Please review changes in > > Chrome/Browser/BUILD.gn > > Thanks, Looks ok. Please ping me again after getting your other approvals.
https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:5: #include <components/data_use_measurement/content/data_use_measurement.h> On 2015/09/01 20:16:01, bengr wrote: > #include "components/.../data_use_measurement.h" and add a blank line after it Done. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:58: // being destroyed and the |app_listener_| being destroyed. On 2015/09/01 20:16:01, bengr wrote: > // Prevent notification between the destruction of the DataUseMeasurement > instance and the destruction of |app_listener_|. Done. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:72: if (info) { On 2015/09/01 20:16:01, bengr wrote: > bool is_user_traffic = info != nullptr; Done. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:76: // These number won't be the number of bytes handed to the kernel because On 2015/09/01 20:16:01, bengr wrote: > // Counts rely on URLRequest::GetTotalReceivedBytes(), which does not include > the send path, network layer overhead, TLS overhead, and DNS. Done. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:126: // If the OS is not Android, all the requests are considered Foreground. On 2015/09/01 20:16:01, bengr wrote: > Make sure this is documented in histograms.xml. Done. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:130: std::string DataUseMeasurement::GetSuffixForHistogramName( On 2015/09/01 20:16:01, bengr wrote: > The method should be const. Done. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:148: "DataUse.Services" + GetSuffixForHistogramName(dir); On 2015/09/01 20:16:00, bengr wrote: > Why does this prefix not end with '.' and the other prefix does? Be consistent. > And make sure your documentation matches. There is no inconsistency here. The '.' is needed in the first one because ServiceName is concatenated to it. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:149: RecordUMAHistogramCount(service_histogram_name, message_size); On 2015/09/01 20:16:01, bengr wrote: > I would get rid of service_histogram_name and replace with > "DataUse.MessageSize." + GetServiceName(). Done. https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:161: "DataUse.%s%s", service_type == USER_TRAFFIC ? "User" : "NotUser", On 2015/09/01 20:16:01, bengr wrote: > NotUser -> Service? Yes service may be better. But basically we had a lot of service in histogram names and I didn't want to add one more. Based on another reviewer's opinion I changed it "system". https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/440001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:68: -2, -2, true, false, true, true); On 2015/09/01 20:16:01, bengr wrote: > What are these -2's and other parameters? I.e., why did you comment on one of > the params but not the others? I was using negative process id to prevent an error in the tests, but later I solved the problem by making changes in profileManager. The numbers are some arbitrary numbers. https://codereview.chromium.org/1279543002/diff/460001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/460001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:160: class ChromeNetworkDelegateDataUseMeasurementTest : public testing::Test { On 2015/09/01 20:04:04, Alexei Svitkine wrote: > A lot of this class is similar to ChromeNetworkDelegateSafeSearchTest. Can you > add a common base-class to reduce the code duplication? You are right. But there are some difference here and there too. Also there is another class in that file which has some similarities with ChromeNetworkDelegateSafeSearchTest but it had been not merged with that one too so I didn't merge my class with it. https://codereview.chromium.org/1279543002/diff/460001/chrome/browser/profile... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1279543002/diff/460001/chrome/browser/profile... chrome/browser/profiles/profile_manager.cc:463: if (!(g_browser_process->profile_manager())) On 2015/09/01 20:04:04, Alexei Svitkine wrote: > Nit: No need for extra ()'s Done. https://codereview.chromium.org/1279543002/diff/460001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/460001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:5: #include <components/data_use_measurement/content/data_use_measurement.h> On 2015/09/01 20:04:04, Alexei Svitkine wrote: > Nit: <> to "" > > Add an empty line after. Done. I think git cl format changes it to this way, because I feel I have fixed it a couple of times. (Or maybe I am wrong) https://codereview.chromium.org/1279543002/diff/460001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:23: 1, // Minimum sample size in Bytes. On 2015/09/01 20:04:04, Alexei Svitkine wrote: > Nit: No need to capitalize bytes. Same below. Done. https://codereview.chromium.org/1279543002/diff/460001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:61: } On 2015/09/01 20:04:04, Alexei Svitkine wrote: > Nit: Add an empty line after this. Done. https://codereview.chromium.org/1279543002/diff/460001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:150: "DataUse.Services" + GetSuffixForHistogramName(dir); On 2015/09/01 20:04:04, Alexei Svitkine wrote: > How about DataUse.MessageSize.AllServices.*? Done. https://codereview.chromium.org/1279543002/diff/460001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/1279543002/diff/460001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:33: On 2015/09/01 20:04:04, Alexei Svitkine wrote: > Nit: No empty line. Done. https://codereview.chromium.org/1279543002/diff/460001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/460001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:87: base::MessageLoopForIO loop_; On 2015/09/01 20:04:04, Alexei Svitkine wrote: > Nit: Put these in private: section and add DISALLOW_COPY_AND_ASSIGN(). Done. https://codereview.chromium.org/1279543002/diff/460001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:111: #if defined(OS_ANDROID) On 2015/09/01 20:04:04, Alexei Svitkine wrote: > You don't need this inner ifdef since you have an identical outer one. Done. https://codereview.chromium.org/1279543002/diff/460001/components/data_use_me... File components/data_use_measurement/core/data_use_user_data.h (right): https://codereview.chromium.org/1279543002/diff/460001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:44: base::SupportsUserData::Data* CreateDataUseUserData( On 2015/09/01 20:04:04, Alexei Svitkine wrote: > Why not have this as a static Create() on the class? > > Also, similar question for the other methods - seems if they're related to the > class, they should be static? Done. https://codereview.chromium.org/1279543002/diff/460001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1279543002/diff/460001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5715: +<histogram name="DataUse.User" units="bytes"> On 2015/09/01 20:04:04, Alexei Svitkine wrote: > Similar comment I've had before. DataUse.User doesn't tell me what's being > measured. > > How about DataUse.TrafficSize.User or similar. > > The other one can also use the same pattern - though I wonder if renaming > "NotUser" to something like "System" or "Chrome" might be clearer. Done.
//components top-level LGTM with change made Please file a bug to track the refactoring that I outlined and reference that bug somewhere in the code. Thanks! https://codereview.chromium.org/1279543002/diff/500001/components/OWNERS File components/OWNERS (right): https://codereview.chromium.org/1279543002/diff/500001/components/OWNERS#newc... components/OWNERS:74: per-file data_use_measurement*=amohammadkhan@chromium.org I agree with others on the OWNERS question. https://codereview.chromium.org/1279543002/diff/500001/components/components_... File components/components_tests.gyp (right): https://codereview.chromium.org/1279543002/diff/500001/components/components_... components/components_tests.gyp:879: 'components.gyp:data_use_measurement_content', This should be in a !iOS section.
https://codereview.chromium.org/1279543002/diff/500001/components/OWNERS File components/OWNERS (right): https://codereview.chromium.org/1279543002/diff/500001/components/OWNERS#newc... components/OWNERS:74: per-file data_use_measurement*=amohammadkhan@chromium.org On 2015/09/02 09:29:32, blundell wrote: > I agree with others on the OWNERS question. Done. https://codereview.chromium.org/1279543002/diff/500001/components/components_... File components/components_tests.gyp (right): https://codereview.chromium.org/1279543002/diff/500001/components/components_... components/components_tests.gyp:879: 'components.gyp:data_use_measurement_content', On 2015/09/02 09:29:32, blundell wrote: > This should be in a !iOS section. Done.
amohammadkhan@google.com changed reviewers: + mathp@chromium.org
On 2015/09/02 17:37:07, amohammadkhan wrote: > https://codereview.chromium.org/1279543002/diff/500001/components/OWNERS > File components/OWNERS (right): > > https://codereview.chromium.org/1279543002/diff/500001/components/OWNERS#newc... > components/OWNERS:74: per-file mailto:data_use_measurement*=amohammadkhan@chromium.org > On 2015/09/02 09:29:32, blundell wrote: > > I agree with others on the OWNERS question. > > Done. > > https://codereview.chromium.org/1279543002/diff/500001/components/components_... > File components/components_tests.gyp (right): > > https://codereview.chromium.org/1279543002/diff/500001/components/components_... > components/components_tests.gyp:879: > 'components.gyp:data_use_measurement_content', > On 2015/09/02 09:29:32, blundell wrote: > > This should be in a !iOS section. > > Done. mathp@ please review changes in components/suggestions . Thanks.
lgtm https://codereview.chromium.org/1279543002/diff/520001/components/suggestions... File components/suggestions.gypi (right): https://codereview.chromium.org/1279543002/diff/520001/components/suggestions... components/suggestions.gypi:23: 'components.gyp:data_use_measurement_core', ordering looks off here
lgtm https://codereview.chromium.org/1279543002/diff/520001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/520001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:160: ? "TrafficSize.User" Nit: TrafficSize is common, so include it in the format string. https://codereview.chromium.org/1279543002/diff/520001/components/data_use_me... File components/data_use_measurement/core/data_use_user_data.cc (right): https://codereview.chromium.org/1279543002/diff/520001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.cc:34: default: Remove the default case and put the NOTREACHED() outside the switch below. This way, compilers will warn when you expand the enum without updating the switch. https://codereview.chromium.org/1279543002/diff/520001/components/data_use_me... File components/data_use_measurement/core/data_use_user_data.h (right): https://codereview.chromium.org/1279543002/diff/520001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:34: static base::SupportsUserData::Data* CreateDataUseUserData( Nit: No need to repeat the name of the type in the function. Can just be DataUseUserData::Create() https://codereview.chromium.org/1279543002/diff/520001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:42: static void AttachDataUserUserDataToFetcher(net::URLFetcher* fetcher, Ditto, can be AttachToFetcher() https://codereview.chromium.org/1279543002/diff/520001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:51: ServiceType service_type_; Nit: const
https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:118: "//components/data_use_measurement/core:data_use_measurement_core", This doesn't match the GYP equivalent in chrome/chrome_browser.gypi. https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:212: return NULL; nit: nullptr, ditto elsewhere. https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/profile... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/profile... chrome/browser/profiles/profile_manager.cc:463: if (!g_browser_process->profile_manager()) Curious, why is this even needed? What happens if you leave the method as it was? https://codereview.chromium.org/1279543002/diff/520001/chrome/chrome_tests_un... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/1279543002/diff/520001/chrome/chrome_tests_un... chrome/chrome_tests_unit.gypi:2164: '../components/components.gyp:data_use_measurement_core', Is there a GN equivalent to this target? If so, you should make a matching change.
Some comments on the tests. May not get back to the main file today. https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate.cc:494: data_use_measurement_.ReportDataUseUMA(request); You need to do this on redirects, too - following a redirect reset the bytes received/sent (Though if the request is cancelled instead of following the redirect, we'll end up here, and you'll double count the data, I believe. I don't see an easy way around that. Suggest a TODO) https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate.h:207: data_use_measurement::DataUseMeasurement data_use_measurement_; Worth noting this will miss PAC requests (Though that may change), requests made from the SystemPolicyRequestContext, gcm requests (Which don't use URLRequests), and requests made from any other private URLRequestContexts (https://code.google.com/p/chromium/codesearch#search/&q=URLRequestContextBuil... shows at least some of them). It should catch most TCP and QUIC activity, though. https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:160: class ChromeNetworkDelegateDataUseMeasurementTest : public testing::Test { This test fixture doesn't really seem to have anything DataUseMeasurement-specific. Can we just rename this ChromeNetworkDelegateTest, and make the existing ChromeNetworkDelegateTest reuse this text fixture? It would just become a 1-line test, I believe. EXPECT_FALSE(network_delegate_->FirstPartyOnlyCookieExperimentEnabled()); https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:163: : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP) { context_(true) { // Delay initialization context_.set_network_delegate(...); // Initialization depends on the network delegate, so should initialize the // context after setting the network delegate context_.Init(); https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:183: context_.set_network_delegate(network_delegate_); The NetworkDelegate is being destroyed before the context that uses it here, which is a little ugly. https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:190: void QueryURLDataUse(bool from_user) { This only really depends on the context. I'd suggest instead making this static, or maybe even moving it out into an anonymous namespace, and then calling it: void RunURLRequest(URLRequestContext* context, bool from_renderer, const GURL& url) { ... } https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:204: base::MessageLoop::current()->RunUntilIdle(); I think it's much more robust to do: // Use a new delegate, so we don't modify the state of the one that's part // of the test fixture net::TestDelegate test_delegate; test_delegate.set_quit_on_complete(true); scoped_ptr<net::URLRequest> request(context_.CreateRequest( GURL("http://example.com"), net::DEFAULT_PRIORITY, &test_delegate)); ... base::MessageLoop::Run(); https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:224: net::NetworkDelegate* network_delegate_; Can't this just be: net::NetworkDelegate network_delegate_; And then remove all the initialization code for it? https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:235: TEST_F(ChromeNetworkDelegateDataUseMeasurementTest, MainTest) { I'd suggest two different tests. https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:245: // One upload and one download message, so totalCount shoudl be 2. nit: Should https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:245: // One upload and one download message, so totalCount shoudl be 2. Check that the user counts are 0? https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:252: "DataUse.TrafficSize.User.Upstream.Foreground.NotCellular", 1); I suggest querying the exact bucket we expect, and using a URL with a redirect.
https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:118: "//components/data_use_measurement/core:data_use_measurement_core", On 2015/09/02 18:14:43, Lei Zhang wrote: > This doesn't match the GYP equivalent in chrome/chrome_browser.gypi. Done. https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate.cc:494: data_use_measurement_.ReportDataUseUMA(request); On 2015/09/02 21:48:08, mmenke wrote: > You need to do this on redirects, too - following a redirect reset the bytes > received/sent (Though if the request is cancelled instead of following the > redirect, we'll end up here, and you'll double count the data, I believe. I > don't see an easy way around that. Suggest a TODO) Done. https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate.h:207: data_use_measurement::DataUseMeasurement data_use_measurement_; On 2015/09/02 21:48:08, mmenke wrote: > Worth noting this will miss PAC requests (Though that may change), requests made > from the SystemPolicyRequestContext, gcm requests (Which don't use URLRequests), > and requests made from any other private URLRequestContexts > (https://code.google.com/p/chromium/codesearch#search/&q=URLRequestContextBuil... > shows at least some of them). > > It should catch most TCP and QUIC activity, though. Thanks for the heads up. I think currently most of our target services are covered, so it may be good enough at this stage but it is a good idea to cover those kinds of request too to have a more holistic view. https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:160: class ChromeNetworkDelegateDataUseMeasurementTest : public testing::Test { On 2015/09/02 21:48:09, mmenke wrote: > This test fixture doesn't really seem to have anything > DataUseMeasurement-specific. Can we just rename this ChromeNetworkDelegateTest, > and make the existing ChromeNetworkDelegateTest reuse this text fixture? It > would just become a 1-line test, I believe. > > EXPECT_FALSE(network_delegate_->FirstPartyOnlyCookieExperimentEnabled()); Done. https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:163: : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP) { On 2015/09/02 21:48:09, mmenke wrote: > context_(true) { // Delay initialization > > context_.set_network_delegate(...); > // Initialization depends on the network delegate, so should initialize the > // context after setting the network delegate > context_.Init(); Done. https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:183: context_.set_network_delegate(network_delegate_); On 2015/09/02 21:48:09, mmenke wrote: > The NetworkDelegate is being destroyed before the context that uses it here, > which is a little ugly. I agree. I used a similar approach to other classes in this test file, but it doesn't look something nice. But I think in this way we make a new NetworkDelegate for every test and we are sure tests do not affect each other. https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:190: void QueryURLDataUse(bool from_user) { On 2015/09/02 21:48:09, mmenke wrote: > This only really depends on the context. I'd suggest instead making this > static, or maybe even moving it out into an anonymous namespace, and then > calling it: > > void RunURLRequest(URLRequestContext* context, bool from_renderer, const GURL& > url) { > ... > } Done. https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:204: base::MessageLoop::current()->RunUntilIdle(); On 2015/09/02 21:48:09, mmenke wrote: > I think it's much more robust to do: > > // Use a new delegate, so we don't modify the state of the one that's part > // of the test fixture > net::TestDelegate test_delegate; > test_delegate.set_quit_on_complete(true); > > scoped_ptr<net::URLRequest> request(context_.CreateRequest( > GURL("http://example.com"), net::DEFAULT_PRIORITY, &test_delegate)); > > ... > base::MessageLoop::Run(); Done. https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:212: return NULL; On 2015/09/02 18:14:43, Lei Zhang wrote: > nit: nullptr, ditto elsewhere. Done. https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:224: net::NetworkDelegate* network_delegate_; On 2015/09/02 21:48:09, mmenke wrote: > Can't this just be: > > net::NetworkDelegate network_delegate_; > > And then remove all the initialization code for it? I think it can't, because we are making a new NetworkDelegate for each test. https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:235: TEST_F(ChromeNetworkDelegateDataUseMeasurementTest, MainTest) { On 2015/09/02 21:48:09, mmenke wrote: > I'd suggest two different tests. Done. https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:245: // One upload and one download message, so totalCount shoudl be 2. On 2015/09/02 21:48:09, mmenke wrote: > nit: Should Done. https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:245: // One upload and one download message, so totalCount shoudl be 2. On 2015/09/02 21:48:09, mmenke wrote: > Check that the user counts are 0? Done. https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:252: "DataUse.TrafficSize.User.Upstream.Foreground.NotCellular", 1); On 2015/09/02 21:48:09, mmenke wrote: > I suggest querying the exact bucket we expect, and using a URL with a redirect. The problem is that if I specify an exact size for a website it can be changed later and this test will fail, so I can just check the existence of a bucket. https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/profile... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/profile... chrome/browser/profiles/profile_manager.cc:463: if (!g_browser_process->profile_manager()) On 2015/09/02 18:14:43, Lei Zhang wrote: > Curious, why is this even needed? What happens if you leave the method as it > was? It was causing segmentation fault in tests. It seems in tests profileManager is not instantiated, so profiles_info_ in next line should not be reached. https://codereview.chromium.org/1279543002/diff/520001/chrome/chrome_tests_un... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/1279543002/diff/520001/chrome/chrome_tests_un... chrome/chrome_tests_unit.gypi:2164: '../components/components.gyp:data_use_measurement_core', On 2015/09/02 18:14:43, Lei Zhang wrote: > Is there a GN equivalent to this target? If so, you should make a matching > change. Done. https://codereview.chromium.org/1279543002/diff/520001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/520001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:160: ? "TrafficSize.User" On 2015/09/02 18:11:20, Alexei Svitkine wrote: > Nit: TrafficSize is common, so include it in the format string. Done. https://codereview.chromium.org/1279543002/diff/520001/components/data_use_me... File components/data_use_measurement/core/data_use_user_data.cc (right): https://codereview.chromium.org/1279543002/diff/520001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.cc:34: default: On 2015/09/02 18:11:20, Alexei Svitkine wrote: > Remove the default case and put the NOTREACHED() outside the switch below. > > This way, compilers will warn when you expand the enum without updating the > switch. Done. https://codereview.chromium.org/1279543002/diff/520001/components/data_use_me... File components/data_use_measurement/core/data_use_user_data.h (right): https://codereview.chromium.org/1279543002/diff/520001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:34: static base::SupportsUserData::Data* CreateDataUseUserData( On 2015/09/02 18:11:20, Alexei Svitkine wrote: > Nit: No need to repeat the name of the type in the function. Can just be > DataUseUserData::Create() Done. https://codereview.chromium.org/1279543002/diff/520001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:42: static void AttachDataUserUserDataToFetcher(net::URLFetcher* fetcher, On 2015/09/02 18:11:20, Alexei Svitkine wrote: > Ditto, can be AttachToFetcher() Done. https://codereview.chromium.org/1279543002/diff/520001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:51: ServiceType service_type_; On 2015/09/02 18:11:20, Alexei Svitkine wrote: > Nit: const Done. https://codereview.chromium.org/1279543002/diff/520001/components/suggestions... File components/suggestions.gypi (right): https://codereview.chromium.org/1279543002/diff/520001/components/suggestions... components/suggestions.gypi:23: 'components.gyp:data_use_measurement_core', On 2015/09/02 17:59:56, Mathieu Perreault wrote: > ordering looks off here Done.
https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/profile... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/profile... chrome/browser/profiles/profile_manager.cc:463: if (!g_browser_process->profile_manager()) On 2015/09/03 04:31:19, amohammadkhan wrote: > On 2015/09/02 18:14:43, Lei Zhang wrote: > > Curious, why is this even needed? What happens if you leave the method as it > > was? > > It was causing segmentation fault in tests. It seems in tests profileManager is > not instantiated, so profiles_info_ in next line should not be reached. I'm skeptical this is the right thing to do. ProfileManager::ShutdownSessionServices() is the only other place that checks to see if a ProfileManager exists, but that's because it dereferences the ProfileManager pointer. You are not doing that here, so it's not obvious that this is correct. Can you be specific about what tests crashed? https://codereview.chromium.org/1279543002/diff/540001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/1279543002/diff/540001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:1453: "//components/data_use_measurement/core:data_use_measurement_core", nit: alphabetical order.
https://codereview.chromium.org/1279543002/diff/540001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/1279543002/diff/540001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate.cc:499: data_use_measurement_.ReportDataUseUMA(request); Think first pass, you should measure usage for redirects. Can worry about the duplicate data later (If at all). https://codereview.chromium.org/1279543002/diff/540001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/1279543002/diff/540001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate.h:21: #if !defined(OS_IOS) Why aren't we hooking this up on iOS? Seems like a pretty self-contained change here, that should be trivial to get working on iOS (Less sure about tests). https://codereview.chromium.org/1279543002/diff/540001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/540001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:44: void QueryURLDataUse(const net::URLRequestContext& context, bool from_user) { Suggest QueryURLDataUse -> RequestURL. I've never seen requesting a URL called "querying" it in the chrome code base, and this doesn't really do anything that's specific to recording data use. https://codereview.chromium.org/1279543002/diff/540001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:49: GURL("http://example.com"), net::DEFAULT_PRIORITY, &test_delegate)); What are we actually requesting a URL from, in this case? We're not setting up a test server, though we're not using a real HostResolver, so I assume we're mapping the domain to localhost. We should know what we're actually requesting here, rather than talking to a server on localhost that may or may not exist. https://codereview.chromium.org/1279543002/diff/540001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:52: request.get(), content::RESOURCE_TYPE_MAIN_FRAME, NULL, -2, -2, -2, nullptr https://codereview.chromium.org/1279543002/diff/540001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:78: &enable_referrers_, NULL, NULL, NULL, profile_.GetTestingPrefService()); nullptr https://codereview.chromium.org/1279543002/diff/540001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:91: context_.Init(); Each test is run in a separate copy of the test fixture, so move delegate setup into the constructor. (If that weren't the case, this code would also be incorrect - calling Init() multiple times on the same context doesn't work) https://codereview.chromium.org/1279543002/diff/540001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:109: private: Shouldn't have more than one private section. Suggest just making everything private, and adding accessors as needed (e.g. net::TestURLRequestContext* context() { return context_; })
https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/profile... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/profile... chrome/browser/profiles/profile_manager.cc:463: if (!g_browser_process->profile_manager()) On 2015/09/03 04:47:11, Lei Zhang wrote: > On 2015/09/03 04:31:19, amohammadkhan wrote: > > On 2015/09/02 18:14:43, Lei Zhang wrote: > > > Curious, why is this even needed? What happens if you leave the method as it > > > was? > > > > It was causing segmentation fault in tests. It seems in tests profileManager > is > > not instantiated, so profiles_info_ in next line should not be reached. > > I'm skeptical this is the right thing to do. > ProfileManager::ShutdownSessionServices() is the only other place that checks to > see if a ProfileManager exists, but that's because it dereferences the > ProfileManager pointer. You are not doing that here, so it's not obvious that > this is correct. > > Can you be specific about what tests crashed? ChromeNetworkDelegateTest.DataUseMeasurementUserTest was crashing. This crash didn't happen in similar tests because they don't attach content::ResourceRequestInfo to request but this test does. The difference is that in this function (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net...) other tests return but the crashing test runs BrowserThread::PostTask. At first I had used a negative processID and I was checking the processID to see if its negative to return in that functions but later I changed it later to current implementation based on checking in ProfileManager because I wasn't sure that checking for negative processID is a good idea or not. https://codereview.chromium.org/1279543002/diff/540001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/1279543002/diff/540001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate.cc:499: data_use_measurement_.ReportDataUseUMA(request); On 2015/09/03 16:02:58, mmenke wrote: > Think first pass, you should measure usage for redirects. Can worry about the > duplicate data later (If at all). Sorry I think I didn't understand what you meant first time. Now I added the code for redirects and a TODO for double counted. https://codereview.chromium.org/1279543002/diff/540001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/1279543002/diff/540001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate.h:21: #if !defined(OS_IOS) On 2015/09/03 16:02:58, mmenke wrote: > Why aren't we hooking this up on iOS? Seems like a pretty self-contained change > here, that should be trivial to get working on iOS (Less sure about tests). Currently the data_use_measurement components in not optimally layered. A bug is made for making it layered in the way blundell@ proposed, but till then I thought it may be a good to having #ifs for !defined(OS_IOS) https://codereview.chromium.org/1279543002/diff/540001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/540001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:44: void QueryURLDataUse(const net::URLRequestContext& context, bool from_user) { On 2015/09/03 16:02:58, mmenke wrote: > Suggest QueryURLDataUse -> RequestURL. > > I've never seen requesting a URL called "querying" it in the chrome code base, > and this doesn't really do anything that's specific to recording data use. Done. https://codereview.chromium.org/1279543002/diff/540001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:49: GURL("http://example.com"), net::DEFAULT_PRIORITY, &test_delegate)); On 2015/09/03 16:02:59, mmenke wrote: > What are we actually requesting a URL from, in this case? We're not setting up > a test server, though we're not using a real HostResolver, so I assume we're > mapping the domain to localhost. > > We should know what we're actually requesting here, rather than talking to a > server on localhost that may or may not exist. Done. https://codereview.chromium.org/1279543002/diff/540001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:52: request.get(), content::RESOURCE_TYPE_MAIN_FRAME, NULL, -2, -2, -2, On 2015/09/03 16:02:58, mmenke wrote: > nullptr Done. https://codereview.chromium.org/1279543002/diff/540001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:78: &enable_referrers_, NULL, NULL, NULL, profile_.GetTestingPrefService()); On 2015/09/03 16:02:59, mmenke wrote: > nullptr Done. https://codereview.chromium.org/1279543002/diff/540001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:91: context_.Init(); On 2015/09/03 16:02:59, mmenke wrote: > Each test is run in a separate copy of the test fixture, so move delegate setup > into the constructor. > > (If that weren't the case, this code would also be incorrect - calling Init() > multiple times on the same context doesn't work) I tried it. It didn't work. One of the tests fails. (EnableFirstPartyOnlyCookiesIffFlagEnabled) I think its first line (base::CommandLine::ForCurrentProcess()->AppendSwitch(switches::kEnableExperimentalWebPlatformFeatures) should be executed before the initial setup. (I am not sure though) https://codereview.chromium.org/1279543002/diff/540001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:109: private: On 2015/09/03 16:02:59, mmenke wrote: > Shouldn't have more than one private section. Suggest just making everything > private, and adding accessors as needed (e.g. net::TestURLRequestContext* > context() { return context_; }) Done. https://codereview.chromium.org/1279543002/diff/540001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/1279543002/diff/540001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:1453: "//components/data_use_measurement/core:data_use_measurement_core", On 2015/09/03 04:47:11, Lei Zhang wrote: > nit: alphabetical order. Done.
https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/profile... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/profile... chrome/browser/profiles/profile_manager.cc:463: if (!g_browser_process->profile_manager()) On 2015/09/03 23:10:35, amohammadkhan wrote: > On 2015/09/03 04:47:11, Lei Zhang wrote: > > On 2015/09/03 04:31:19, amohammadkhan wrote: > > > On 2015/09/02 18:14:43, Lei Zhang wrote: > > > > Curious, why is this even needed? What happens if you leave the method as > it > > > > was? > > > > > > It was causing segmentation fault in tests. It seems in tests profileManager > > is > > > not instantiated, so profiles_info_ in next line should not be reached. > > > > I'm skeptical this is the right thing to do. > > ProfileManager::ShutdownSessionServices() is the only other place that checks > to > > see if a ProfileManager exists, but that's because it dereferences the > > ProfileManager pointer. You are not doing that here, so it's not obvious that > > this is correct. > > > > Can you be specific about what tests crashed? > > ChromeNetworkDelegateTest.DataUseMeasurementUserTest was crashing. This crash > didn't happen in similar tests because they don't attach > content::ResourceRequestInfo to request but this test does. The difference is > that in this function > (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net...) > other tests return but the crashing test runs BrowserThread::PostTask. > At first I had used a negative processID and I was checking the processID to see > if its negative to return in that functions but later I changed it later to > current implementation based on checking in ProfileManager because I wasn't sure > that checking for negative processID is a good idea or not. The right fix is to do the check in the caller, which is NotifyEPMRequestStatus() in chrome/browser/net/chrome_extensions_network_delegate.cc, or alternatively, to create a ProfileManager in your test. I don't know which is "more correct" so ask the networking code experts.
Want to do another pass over data_use_measurement.cc later today. https://codereview.chromium.org/1279543002/diff/560001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/560001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:42: // This function requests a URL. If |from_user| is true, it attaches a Maybe "This function requests a URL." -> "This function requests a URL, and makes it return a known response." https://codereview.chromium.org/1279543002/diff/560001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:47: // necessary socket data to have a redirection before the main request. "necessary socket data to have it follow redirect before getting the final response." https://codereview.chromium.org/1279543002/diff/560001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:48: void RequestURL(const net::URLRequestContext* context, remove const. https://codereview.chromium.org/1279543002/diff/560001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:55: net::MockRead(""), net::MockRead(net::SYNCHRONOUS, net::OK), nit: Here, and below, suggest one read per line. https://codereview.chromium.org/1279543002/diff/560001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:108: protected: nit: Protected doesn't really get us anything in tests. https://codereview.chromium.org/1279543002/diff/560001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:113: } On 2015/09/03 23:10:36, amohammadkhan wrote: > On 2015/09/03 16:02:59, mmenke wrote: > > Each test is run in a separate copy of the test fixture, so move delegate > setup > > into the constructor. > > > > (If that weren't the case, this code would also be incorrect - calling Init() > > multiple times on the same context doesn't work) > > I tried it. It didn't work. One of the tests fails. > (EnableFirstPartyOnlyCookiesIffFlagEnabled) I think its first line > (base::CommandLine::ForCurrentProcess()->AppendSwitch(switches::kEnableExperimentalWebPlatformFeatures) > should be executed before the initial setup. > (I am not sure though) You are correct. I think it's cleaner to just do: void Initialize() { // Create context and network delegate here // (Switch to using a scoped_ptr for both in the test fixture). } (Or call it CreateNetworkDelegateAndContext or something - happy with other ideas on the name) https://codereview.chromium.org/1279543002/diff/560001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:125: extensions::EventRouterForwarder* forwarder() { Suggest a blank line above forwarder, to make the code easier to read. https://codereview.chromium.org/1279543002/diff/560001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:136: net::NetworkDelegate* network_delegate_; This should go before the context_, as the context depends on it. https://codereview.chromium.org/1279543002/diff/560001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:142: BooleanPrefMember enable_referrers_; These last three should all go before the network delegate, as it depends on them. https://codereview.chromium.org/1279543002/diff/560001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:198: DataUseMeasurementServiceTestWithRedirecting) { nit: Redirecting->Redirect
https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/profile... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1279543002/diff/520001/chrome/browser/profile... chrome/browser/profiles/profile_manager.cc:463: if (!g_browser_process->profile_manager()) On 2015/09/04 01:18:06, Lei Zhang wrote: > On 2015/09/03 23:10:35, amohammadkhan wrote: > > On 2015/09/03 04:47:11, Lei Zhang wrote: > > > On 2015/09/03 04:31:19, amohammadkhan wrote: > > > > On 2015/09/02 18:14:43, Lei Zhang wrote: > > > > > Curious, why is this even needed? What happens if you leave the method > as > > it > > > > > was? > > > > > > > > It was causing segmentation fault in tests. It seems in tests > profileManager > > > is > > > > not instantiated, so profiles_info_ in next line should not be reached. > > > > > > I'm skeptical this is the right thing to do. > > > ProfileManager::ShutdownSessionServices() is the only other place that > checks > > to > > > see if a ProfileManager exists, but that's because it dereferences the > > > ProfileManager pointer. You are not doing that here, so it's not obvious > that > > > this is correct. > > > > > > Can you be specific about what tests crashed? > > > > ChromeNetworkDelegateTest.DataUseMeasurementUserTest was crashing. This crash > > didn't happen in similar tests because they don't attach > > content::ResourceRequestInfo to request but this test does. The difference is > > that in this function > > > (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net...) > > other tests return but the crashing test runs BrowserThread::PostTask. > > At first I had used a negative processID and I was checking the processID to > see > > if its negative to return in that functions but later I changed it later to > > current implementation based on checking in ProfileManager because I wasn't > sure > > that checking for negative processID is a good idea or not. > > The right fix is to do the check in the caller, which is > NotifyEPMRequestStatus() in > chrome/browser/net/chrome_extensions_network_delegate.cc, or alternatively, to > create a ProfileManager in your test. > > I don't know which is "more correct" so ask the networking code experts. Done. https://codereview.chromium.org/1279543002/diff/560001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/560001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:42: // This function requests a URL. If |from_user| is true, it attaches a On 2015/09/04 15:52:00, mmenke wrote: > Maybe "This function requests a URL." -> "This function requests a URL, and > makes it return a known response." Done. https://codereview.chromium.org/1279543002/diff/560001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:47: // necessary socket data to have a redirection before the main request. On 2015/09/04 15:52:01, mmenke wrote: > "necessary socket data to have it follow redirect before getting the final > response." Done. https://codereview.chromium.org/1279543002/diff/560001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:48: void RequestURL(const net::URLRequestContext* context, On 2015/09/04 15:52:00, mmenke wrote: > remove const. Done. https://codereview.chromium.org/1279543002/diff/560001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:55: net::MockRead(""), net::MockRead(net::SYNCHRONOUS, net::OK), On 2015/09/04 15:52:01, mmenke wrote: > nit: Here, and below, suggest one read per line. Done. Although "git cl format" makes it in that form and I had it in separate lines. https://codereview.chromium.org/1279543002/diff/560001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:108: protected: On 2015/09/04 15:52:00, mmenke wrote: > nit: Protected doesn't really get us anything in tests. Done. https://codereview.chromium.org/1279543002/diff/560001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:113: } On 2015/09/04 15:52:01, mmenke wrote: > On 2015/09/03 23:10:36, amohammadkhan wrote: > > On 2015/09/03 16:02:59, mmenke wrote: > > > Each test is run in a separate copy of the test fixture, so move delegate > > setup > > > into the constructor. > > > > > > (If that weren't the case, this code would also be incorrect - calling > Init() > > > multiple times on the same context doesn't work) > > > > I tried it. It didn't work. One of the tests fails. > > (EnableFirstPartyOnlyCookiesIffFlagEnabled) I think its first line > > > (base::CommandLine::ForCurrentProcess()->AppendSwitch(switches::kEnableExperimentalWebPlatformFeatures) > > should be executed before the initial setup. > > (I am not sure though) > > You are correct. I think it's cleaner to just do: > > void Initialize() { > // Create context and network delegate here > // (Switch to using a scoped_ptr for both in the test fixture). > } > > (Or call it CreateNetworkDelegateAndContext or something - happy with other > ideas on the name) Done. https://codereview.chromium.org/1279543002/diff/560001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:125: extensions::EventRouterForwarder* forwarder() { On 2015/09/04 15:52:01, mmenke wrote: > Suggest a blank line above forwarder, to make the code easier to read. Done. https://codereview.chromium.org/1279543002/diff/560001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:136: net::NetworkDelegate* network_delegate_; On 2015/09/04 15:52:01, mmenke wrote: > This should go before the context_, as the context depends on it. Done. https://codereview.chromium.org/1279543002/diff/560001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:142: BooleanPrefMember enable_referrers_; On 2015/09/04 15:52:00, mmenke wrote: > These last three should all go before the network delegate, as it depends on > them. Done. https://codereview.chromium.org/1279543002/diff/560001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:198: DataUseMeasurementServiceTestWithRedirecting) { On 2015/09/04 15:52:01, mmenke wrote: > nit: Redirecting->Redirect Done.
chrome/ sans c/b/net lgtm
https://codereview.chromium.org/1279543002/diff/560001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/560001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:55: net::MockRead(""), net::MockRead(net::SYNCHRONOUS, net::OK), On 2015/09/04 17:25:12, amohammadkhan wrote: > On 2015/09/04 15:52:01, mmenke wrote: > > nit: Here, and below, suggest one read per line. > > Done. Although "git cl format" makes it in that form and I had it in separate > lines. Ahh...I'm happy to defer to git cl format, then. https://codereview.chromium.org/1279543002/diff/600001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/600001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:56: net::MockRead(""), This isn't needed (And is redundant with the next net::OK, actually) https://codereview.chromium.org/1279543002/diff/600001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:63: } nit: Don't use braces on single-line if. https://codereview.chromium.org/1279543002/diff/600001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:78: GURL("http://example.com"), net::DEFAULT_PRIORITY, &test_delegate)); nit: include "url/gurl.h" https://codereview.chromium.org/1279543002/diff/600001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:111: scoped_ptr<net::NetworkDelegate> CreateNetworkDelegate() { Should get rid of this method and SetDelegate, and just inline them in Initialize() https://codereview.chromium.org/1279543002/diff/600001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:132: extensions::EventRouterForwarder* forwarder() { This code doesn't really depend on whether or not this is created. Can we just get rid of this (And forwarder_) and use nullptr instead? Then we wouldn't need the chrome_extensions_network_delegate.cc change, either, I believe. https://codereview.chromium.org/1279543002/diff/600001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:149: net::MockClientSocketFactory socket_factory_; This should be above TestURLRequestContext https://codereview.chromium.org/1279543002/diff/600001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:150: nit: Blank line not needed. https://codereview.chromium.org/1279543002/diff/600001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:164: RequestURL(context(), socket_factory(), 0, 0); 0, 0 -> false, false https://codereview.chromium.org/1279543002/diff/600001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:186: RequestURL(context(), socket_factory(), 1, 0); 1, 0 -> true, false https://codereview.chromium.org/1279543002/diff/600001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:208: RequestURL(context(), socket_factory(), 0, 1); false/true https://codereview.chromium.org/1279543002/diff/600001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:230: RequestURL(context(), socket_factory(), 1, 1); true/true https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:22: void RecordUMAHistogramCount(const std::string& name, int64_t sample) { #include <stdint.h> #include <string> https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:35: // |name| is not static. nit: the |name| -> |name| https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:36: void RecordSparseHistogramWithValue(const std::string& name, This name is confusing - it sounds like it's recording value in a histogram, which it isn't. IncreaseSparseHistogramByValue, maybe? https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:61: #endif This explicit reset is not needed - it's a scoped_ptr, so it will be deleted here anyways, and since it will always be run on this thread, and we're already running on this thread, it can't be called into during teardown, so destroying it slightly earlier while deleting |this| doesn't matter. https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:99: // foreground or in the background. It is owned by DataUseMeasurement nit: +period https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:32: protected: No need for the protected section. https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:41: net::MockRead("Test Content2")}; Relying on reusing the socket is really ugly. I suggest just splitting up the tests, with one request per test (Could also use the embedded test server, but that's a bigger change, which I suspect you'd rather not do). https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:65: scoped_ptr<net::URLRequest> request2(context_->CreateRequest( include base/memory/scoped_ptr.h https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:66: GURL("http://foo.com"), net::DEFAULT_PRIORITY, &test_delegate_)); include url/gurl.h and net/base/request_priority.h https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:78: 1); Check DataUse.MessageSize.AllServices, too? https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:83: net::NetworkChangeNotifier::GetConnectionType())) { This always returns CONNECTION_TYPE_UNKNOWN when we have no network change notifier, so can get rid of this method. Note that if we were reading live values, this test wouldn't be safe, as that can actually change during the coarse of a test. Could optionally add a DCHECK on connection type, with a comment, to make the dependency clearer. https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:90: DataUseMeasurement data_use_measurement_; This can be private. https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:96: net::TestDelegate test_delegate_; Suggest removing this as a member variable, and just adding it to the stack in TestForAUserAndAServicePacket. https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:97: std::string connection_type_; include <string> https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:98: net::MockClientSocketFactory socket_factory_; Order these based on dependencies: socket_factory_ test_network_delegate_ context_ https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... File components/data_use_measurement/core/data_use_user_data.h (right): https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:23: enum ServiceType { nit: ServiceName? https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:38: static const char* GetServiceName(ServiceType service); GetServiceNameAsString? https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:57: #endif // COMPONENTS_DATA_USE_MEASUREMENT_CORE_DATA_USE_USER_DATA_H_ nit: Blank line before #endif.
isherman@chromium.org changed reviewers: + isherman@chromium.org
https://codereview.chromium.org/1279543002/diff/600001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1279543002/diff/600001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5657: + added as a sufffix to this histogram name. nit: "sufffix" -> "suffix" https://codereview.chromium.org/1279543002/diff/600001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5667: + bytes are logged in this histogram. The buckets in this histogram are nit: "number ... are" -> "number ... is"
https://codereview.chromium.org/1279543002/diff/600001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/600001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:56: net::MockRead(""), On 2015/09/04 20:51:45, mmenke wrote: > This isn't needed (And is redundant with the next net::OK, actually) Done. https://codereview.chromium.org/1279543002/diff/600001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:63: } On 2015/09/04 20:51:45, mmenke wrote: > nit: Don't use braces on single-line if. Done. https://codereview.chromium.org/1279543002/diff/600001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:78: GURL("http://example.com"), net::DEFAULT_PRIORITY, &test_delegate)); On 2015/09/04 20:51:45, mmenke wrote: > nit: include "url/gurl.h" Done. https://codereview.chromium.org/1279543002/diff/600001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:111: scoped_ptr<net::NetworkDelegate> CreateNetworkDelegate() { On 2015/09/04 20:51:45, mmenke wrote: > Should get rid of this method and SetDelegate, and just inline them in > Initialize() Done. https://codereview.chromium.org/1279543002/diff/600001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:132: extensions::EventRouterForwarder* forwarder() { On 2015/09/04 20:51:45, mmenke wrote: > This code doesn't really depend on whether or not this is created. Can we just > get rid of this (And forwarder_) and use nullptr instead? Then we wouldn't need > the chrome_extensions_network_delegate.cc change, either, I believe. I tried to remove it, it give me the following error: chrome_extensions_network_delegate.cc(128)] Check failed: event_router. Also two tests that I have added to my class were using forwarder too. https://codereview.chromium.org/1279543002/diff/600001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:149: net::MockClientSocketFactory socket_factory_; On 2015/09/04 20:51:45, mmenke wrote: > This should be above TestURLRequestContext Done. https://codereview.chromium.org/1279543002/diff/600001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:150: On 2015/09/04 20:51:45, mmenke wrote: > nit: Blank line not needed. Done. https://codereview.chromium.org/1279543002/diff/600001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:164: RequestURL(context(), socket_factory(), 0, 0); On 2015/09/04 20:51:45, mmenke wrote: > 0, 0 -> false, false Done. https://codereview.chromium.org/1279543002/diff/600001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:186: RequestURL(context(), socket_factory(), 1, 0); On 2015/09/04 20:51:45, mmenke wrote: > 1, 0 -> true, false Done. https://codereview.chromium.org/1279543002/diff/600001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:208: RequestURL(context(), socket_factory(), 0, 1); On 2015/09/04 20:51:45, mmenke wrote: > false/true Done. https://codereview.chromium.org/1279543002/diff/600001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate_unittest.cc:230: RequestURL(context(), socket_factory(), 1, 1); On 2015/09/04 20:51:45, mmenke wrote: > true/true Done. https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:22: void RecordUMAHistogramCount(const std::string& name, int64_t sample) { On 2015/09/04 20:51:45, mmenke wrote: > #include <stdint.h> > > #include <string> They are included in data_use_measurement.h. do I need to include them again? https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:35: // |name| is not static. On 2015/09/04 20:51:45, mmenke wrote: > nit: the |name| -> |name| Done. https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:36: void RecordSparseHistogramWithValue(const std::string& name, On 2015/09/04 20:51:46, mmenke wrote: > This name is confusing - it sounds like it's recording value in a histogram, > which it isn't. IncreaseSparseHistogramByValue, maybe? Done. https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:61: #endif On 2015/09/04 20:51:46, mmenke wrote: > This explicit reset is not needed - it's a scoped_ptr, so it will be deleted > here anyways, and since it will always be run on this thread, and we're already > running on this thread, it can't be called into during teardown, so destroying > it slightly earlier while deleting |this| doesn't matter. Done. https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:99: // foreground or in the background. It is owned by DataUseMeasurement On 2015/09/04 20:51:46, mmenke wrote: > nit: +period Done. https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:32: protected: On 2015/09/04 20:51:46, mmenke wrote: > No need for the protected section. Done. https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:41: net::MockRead("Test Content2")}; On 2015/09/04 20:51:46, mmenke wrote: > Relying on reusing the socket is really ugly. I suggest just splitting up the > tests, with one request per test (Could also use the embedded test server, but > that's a bigger change, which I suspect you'd rather not do). Done. https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:65: scoped_ptr<net::URLRequest> request2(context_->CreateRequest( On 2015/09/04 20:51:46, mmenke wrote: > include base/memory/scoped_ptr.h Done. https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:66: GURL("http://foo.com"), net::DEFAULT_PRIORITY, &test_delegate_)); On 2015/09/04 20:51:46, mmenke wrote: > include url/gurl.h and net/base/request_priority.h Done. https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:78: 1); On 2015/09/04 20:51:46, mmenke wrote: > Check DataUse.MessageSize.AllServices, too? Done. https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:83: net::NetworkChangeNotifier::GetConnectionType())) { On 2015/09/04 20:51:46, mmenke wrote: > This always returns CONNECTION_TYPE_UNKNOWN when we have no network change > notifier, so can get rid of this method. Note that if we were reading live > values, this test wouldn't be safe, as that can actually change during the > coarse of a test. > > Could optionally add a DCHECK on connection type, with a comment, to make the > dependency clearer. Done. https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:90: DataUseMeasurement data_use_measurement_; On 2015/09/04 20:51:46, mmenke wrote: > This can be private. Done. https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:96: net::TestDelegate test_delegate_; On 2015/09/04 20:51:46, mmenke wrote: > Suggest removing this as a member variable, and just adding it to the stack in > TestForAUserAndAServicePacket. Done. https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:97: std::string connection_type_; On 2015/09/04 20:51:46, mmenke wrote: > include <string> Done. https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:98: net::MockClientSocketFactory socket_factory_; On 2015/09/04 20:51:46, mmenke wrote: > Order these based on dependencies: > > socket_factory_ > test_network_delegate_ > context_ Done. https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... File components/data_use_measurement/core/data_use_user_data.h (right): https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:23: enum ServiceType { On 2015/09/04 20:51:46, mmenke wrote: > nit: ServiceName? Done. https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:38: static const char* GetServiceName(ServiceType service); On 2015/09/04 20:51:46, mmenke wrote: > GetServiceNameAsString? Done. https://codereview.chromium.org/1279543002/diff/600001/components/data_use_me... components/data_use_measurement/core/data_use_user_data.h:57: #endif // COMPONENTS_DATA_USE_MEASUREMENT_CORE_DATA_USE_USER_DATA_H_ On 2015/09/04 20:51:46, mmenke wrote: > nit: Blank line before #endif. Done. https://codereview.chromium.org/1279543002/diff/600001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1279543002/diff/600001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5657: + added as a sufffix to this histogram name. On 2015/09/04 22:10:12, Ilya Sherman wrote: > nit: "sufffix" -> "suffix" Done. https://codereview.chromium.org/1279543002/diff/600001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5667: + bytes are logged in this histogram. The buckets in this histogram are On 2015/09/04 22:10:12, Ilya Sherman wrote: > nit: "number ... are" -> "number ... is" Done.
mmenke@ PTAL. Some changes made to fix the errors in trybots.
https://codereview.chromium.org/1279543002/diff/630001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/630001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:17: #include "testing/gmock/include/gmock/gmock.h" It doesn't look like you're actually using GMOCK here, so remove this include. https://codereview.chromium.org/1279543002/diff/630001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:53: loop_.Run(); Change this to loop_.RunUntilIdle(), same below in the other method. https://codereview.chromium.org/1279543002/diff/630001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:101: net::MockRead reads_[3] = {net::MockRead("HTTP/1.1 200 OK\r\n" This fails to compile on the windows trybots. Move the MockReads array up into each of the test methods above, and remove the "3" from "reads[3]" since the 3 isn't necessary and there's actually only 2 elements.
https://codereview.chromium.org/1279543002/diff/630001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/630001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:17: #include "testing/gmock/include/gmock/gmock.h" On 2015/09/08 18:47:59, sclittle wrote: > It doesn't look like you're actually using GMOCK here, so remove this include. Done. https://codereview.chromium.org/1279543002/diff/630001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:53: loop_.Run(); On 2015/09/08 18:48:00, sclittle wrote: > Change this to loop_.RunUntilIdle(), same below in the other method. Done. https://codereview.chromium.org/1279543002/diff/630001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:101: net::MockRead reads_[3] = {net::MockRead("HTTP/1.1 200 OK\r\n" On 2015/09/08 18:48:00, sclittle wrote: > This fails to compile on the windows trybots. Move the MockReads array up into > each of the test methods above, and remove the "3" from "reads[3]" since the 3 > isn't necessary and there's actually only 2 elements. You are right. After our offline discussion, I noticed that there is a Reset function for StaticSocketDataProvider, so I took advantages of that. So I think there is no unauthorized memory access/memory leak/restriction in calling functions anymore.
lgtm % nits in test. https://codereview.chromium.org/1279543002/diff/650001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/650001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:39: socket_provider_.reset(new net::StaticSocketDataProvider( nit: Use a different MockReads array and SocketDataProvider for each test, it would be much nicer than using the same one like you are here, since it would reduce the coupling between the tests. You could make the socket factory a scoped_ptr, and move all the initialization logic into a different method, e.g. InitializeContext, which resets all the scoped_ptrs with new TestURLRequestContexts and MockClientSocketFactories, and calls context_->set_client_socket_factory(socket_factory_.get()). https://codereview.chromium.org/1279543002/diff/650001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:104: scoped_ptr<net::StaticSocketDataProvider> socket_provider_; nit: remove this and the MockReads below it, and move them into the test methods themselves. These should not need to be scoped outside the individual test methods. See above comment. https://codereview.chromium.org/1279543002/diff/650001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:107: base::MessageLoopForIO loop_; nit: Just for convention, move loop_ to the top of the list here, right below "private:". https://codereview.chromium.org/1279543002/diff/650001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:109: net::TestNetworkDelegate test_network_delegate_; nit: This test_network_delegate looks like it isn't actually used, so remove it. Then the TestURLRequestContext will set it's own TestNetworkDelegate instead.
https://codereview.chromium.org/1279543002/diff/650001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement_unittest.cc (right): https://codereview.chromium.org/1279543002/diff/650001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:39: socket_provider_.reset(new net::StaticSocketDataProvider( On 2015/09/09 00:13:37, sclittle wrote: > nit: Use a different MockReads array and SocketDataProvider for each test, it > would be much nicer than using the same one like you are here, since it would > reduce the coupling between the tests. > > You could make the socket factory a scoped_ptr, and move all the initialization > logic into a different method, e.g. InitializeContext, which resets all the > scoped_ptrs with new TestURLRequestContexts and MockClientSocketFactories, and > calls context_->set_client_socket_factory(socket_factory_.get()). Done. https://codereview.chromium.org/1279543002/diff/650001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:104: scoped_ptr<net::StaticSocketDataProvider> socket_provider_; On 2015/09/09 00:13:37, sclittle wrote: > nit: remove this and the MockReads below it, and move them into the test methods > themselves. These should not need to be scoped outside the individual test > methods. See above comment. Done. https://codereview.chromium.org/1279543002/diff/650001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:107: base::MessageLoopForIO loop_; On 2015/09/09 00:13:37, sclittle wrote: > nit: Just for convention, move loop_ to the top of the list here, right below > "private:". Done. https://codereview.chromium.org/1279543002/diff/650001/components/data_use_me... components/data_use_measurement/content/data_use_measurement_unittest.cc:109: net::TestNetworkDelegate test_network_delegate_; On 2015/09/09 00:13:37, sclittle wrote: > nit: This test_network_delegate looks like it isn't actually used, so remove it. > Then the TestURLRequestContext will set it's own TestNetworkDelegate instead. Done.
https://codereview.chromium.org/1279543002/diff/670001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/1279543002/diff/670001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:117: "//components/data_use_measurement/content", I think this needs to be moved into the "deps +=" section below for non-iOS only.
https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:80: total_upload_bytes = request_body_bytes + request_header_bytes; Per earlier comments, all this upstream stuff is wrong. We really should get rid of it, until we can get the correct numbers. Mixing good and bad numbers is just a bad idea. Suggest just waiting until https://codereview.chromium.org/1327763003/ lands (Which I just signed off on), and then landing this with the correct new method from the start. https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:92: if (!is_user_traffic) { optional: I'd suggest just treating user traffic as its own service, rather than recording some values in two histograms. i.e. DataUseUserData::ServiceName service_name = DataUseUserData::NOT_TAGGED; if (content::ResourceRequestInfo::ForRequest(request)) { service_name = DataUseUserData::USER_DATA; DCHECK(!attached_service_data); } else if (attached_service_data) { service_name = attached_service_data->service_type(); } https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:95: ? attached_service_data->service_type() The naming here is inconsistent - the enumeration is ServiceName, but the local variable and accessor are both called service_type - should be consistent. https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:96: : data_use_measurement::DataUseUserData::NOT_TAGGED; data_use_measurement:: namespace prefix not needed (Should search through this file and remove it wherever it appears). https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:110: void DataUseMeasurement::OnApplicationStateChangeForTesting( Order in the cc file should match order in the header file. Goes for all methods. https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:142: std::string(DataUseUserData::GetServiceNameAsString(service)), To reduce string operations by one, suggest passing in "DataUse.MessageSize" to GetServiceNameAsString, and making it return the full string. https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:151: void DataUseMeasurement::ReportDataUsageGeneral(RequestInitiator service_type, Note that service_type and DataUseUserData::service_type() are of different types, which is very confusing. I've suggested renaming service_type(), but this should also be renamed to request_initiator (And the same for the prototype in the header) https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:38: void ReportDataUseUMA(const net::URLRequest* request); void ReportDataUseUMA(const net::URLRequest* request) const; https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:60: AppState CurrentAppState(); AppState CurrentAppState() const; https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:66: const std::string GetSuffixForHistogramName(TrafficDirection dir); The const on the return value doesn't get us anything. This should be: std::string GetSuffixForHistogramName(TrafficDirection dir) const; Note the const on the end. https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:82: int64_t message_size); int64_t message_size); -> int64_t message_size) const; https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:91: int64_t message_size); +const before semi-colon https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:103: DISALLOW_COPY_AND_ASSIGN(DataUseMeasurement); include base/macros.h for DIASLLOW_COPY_AND_ASSIGN
https://codereview.chromium.org/1279543002/diff/670001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/1279543002/diff/670001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:117: "//components/data_use_measurement/content", On 2015/09/09 23:19:14, sclittle wrote: > I think this needs to be moved into the "deps +=" section below for non-iOS > only. Done. https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:80: total_upload_bytes = request_body_bytes + request_header_bytes; On 2015/09/10 15:43:00, mmenke wrote: > Per earlier comments, all this upstream stuff is wrong. We really should get > rid of it, until we can get the correct numbers. Mixing good and bad numbers is > just a bad idea. > > Suggest just waiting until https://codereview.chromium.org/1327763003/ lands > (Which I just signed off on), and then landing this with the correct new method > from the start. Done. I made this CL dependent on that CL and used its function. https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:92: if (!is_user_traffic) { On 2015/09/10 15:43:00, mmenke wrote: > optional: I'd suggest just treating user traffic as its own service, rather > than recording some values in two histograms. > > i.e. > > DataUseUserData::ServiceName service_name = DataUseUserData::NOT_TAGGED; > if (content::ResourceRequestInfo::ForRequest(request)) { > service_name = DataUseUserData::USER_DATA; > DCHECK(!attached_service_data); > } else if (attached_service_data) { > service_name = attached_service_data->service_type(); > } I see your point. But we are interested to have more granular info about user traffic. (less data is recorded for each service in comparison to user data, for example we have the distribution of request sizes for each dimension for user traffic but we don't have it for services) https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:95: ? attached_service_data->service_type() On 2015/09/10 15:43:00, mmenke wrote: > The naming here is inconsistent - the enumeration is ServiceName, but the local > variable and accessor are both called service_type - should be consistent. Done. https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:96: : data_use_measurement::DataUseUserData::NOT_TAGGED; On 2015/09/10 15:43:00, mmenke wrote: > data_use_measurement:: namespace prefix not needed (Should search through this > file and remove it wherever it appears). Done. https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:110: void DataUseMeasurement::OnApplicationStateChangeForTesting( On 2015/09/10 15:43:00, mmenke wrote: > Order in the cc file should match order in the header file. Goes for all > methods. Done. https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:142: std::string(DataUseUserData::GetServiceNameAsString(service)), On 2015/09/10 15:43:00, mmenke wrote: > To reduce string operations by one, suggest passing in "DataUse.MessageSize" to > GetServiceNameAsString, and making it return the full string. I am not sure if I understood it correctly. Please let me know if it is not what you meant. https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:151: void DataUseMeasurement::ReportDataUsageGeneral(RequestInitiator service_type, On 2015/09/10 15:43:00, mmenke wrote: > Note that service_type and DataUseUserData::service_type() are of different > types, which is very confusing. I've suggested renaming service_type(), but > this should also be renamed to request_initiator (And the same for the prototype > in the header) Done. https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:38: void ReportDataUseUMA(const net::URLRequest* request); On 2015/09/10 15:43:00, mmenke wrote: > void ReportDataUseUMA(const net::URLRequest* request) const; Done. https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:60: AppState CurrentAppState(); On 2015/09/10 15:43:00, mmenke wrote: > AppState CurrentAppState() const; Done. https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:66: const std::string GetSuffixForHistogramName(TrafficDirection dir); On 2015/09/10 15:43:00, mmenke wrote: > The const on the return value doesn't get us anything. This should be: > > std::string GetSuffixForHistogramName(TrafficDirection dir) const; > > Note the const on the end. Done. https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:82: int64_t message_size); On 2015/09/10 15:43:00, mmenke wrote: > int64_t message_size); -> int64_t message_size) const; Done. https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:91: int64_t message_size); On 2015/09/10 15:43:00, mmenke wrote: > +const before semi-colon Done. https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.h:103: DISALLOW_COPY_AND_ASSIGN(DataUseMeasurement); On 2015/09/10 15:43:00, mmenke wrote: > include base/macros.h for DIASLLOW_COPY_AND_ASSIGN Done.
LGTM, modulo comments. https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:142: std::string(DataUseUserData::GetServiceNameAsString(service)), On 2015/09/10 20:09:00, amohammadkhan wrote: > On 2015/09/10 15:43:00, mmenke wrote: > > To reduce string operations by one, suggest passing in "DataUse.MessageSize" > to > > GetServiceNameAsString, and making it return the full string. > > I am not sure if I understood it correctly. Please let me know if it is not what > you meant. below have: IncreaseSparseHistogramByValue( GetHistogramName("DataUse.MessageSize.AllServices", GetSuffixForHistogramName(dir))); https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:147: service, message_size); Was thinking of having this here: IncreaseSparseHistogramByValue( GetHistogramName("DataUse.MessageSize.AllServices", GetSuffixForHistogramName(dir))); https://codereview.chromium.org/1279543002/diff/730001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/730001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:135: message_size); On 2015/09/10 20:09:00, amohammadkhan wrote: > On 2015/09/10 15:43:00, mmenke wrote: > > To reduce string operations by one, suggest passing in "DataUse.MessageSize" > to > > GetServiceNameAsString, and making it return the full string. > > I am not sure if I understood it correctly. Please let me know if it is not what > you meant. Sorry, put my comment at the wrong spot. See comment below. Also, at this spot, you should go back to what you had before, but remove the extra std::string. That is, this should be: RecordUMAHistogramCount( "DataUse.MessageSize." + DataUseUserData::GetServiceNameAsString(service), ... (No std::string around DataUseUserData::GetServiceNameAsString(service) - it already returns a string, not a char*) https://codereview.chromium.org/1279543002/diff/730001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:139: service, message_size); Was thinking of having this here: IncreaseSparseHistogramByValue( GetHistogramName("DataUse.MessageSize.AllServices", GetSuffixForHistogramName(dir)));
https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/670001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:142: std::string(DataUseUserData::GetServiceNameAsString(service)), On 2015/09/11 13:59:07, mmenke wrote: > On 2015/09/10 20:09:00, amohammadkhan wrote: > > On 2015/09/10 15:43:00, mmenke wrote: > > > To reduce string operations by one, suggest passing in "DataUse.MessageSize" > > to > > > GetServiceNameAsString, and making it return the full string. > > > > I am not sure if I understood it correctly. Please let me know if it is not > what > > you meant. > > below have: > > IncreaseSparseHistogramByValue( > GetHistogramName("DataUse.MessageSize.AllServices", > GetSuffixForHistogramName(dir))); Oops...meant to delete the above comment - ignore it, just look at the two made directly in your latest patch set.
https://codereview.chromium.org/1279543002/diff/730001/components/data_use_me... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/1279543002/diff/730001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:135: message_size); On 2015/09/11 13:59:07, mmenke wrote: > On 2015/09/10 20:09:00, amohammadkhan wrote: > > On 2015/09/10 15:43:00, mmenke wrote: > > > To reduce string operations by one, suggest passing in "DataUse.MessageSize" > > to > > > GetServiceNameAsString, and making it return the full string. > > > > I am not sure if I understood it correctly. Please let me know if it is not > what > > you meant. > > Sorry, put my comment at the wrong spot. See comment below. Also, at this > spot, you should go back to what you had before, but remove the extra > std::string. That is, this should be: > > RecordUMAHistogramCount( > "DataUse.MessageSize." + DataUseUserData::GetServiceNameAsString(service), > ... > > (No std::string around DataUseUserData::GetServiceNameAsString(service) - it > already returns a string, not a char*) Done. https://codereview.chromium.org/1279543002/diff/730001/components/data_use_me... components/data_use_measurement/content/data_use_measurement.cc:139: service, message_size); On 2015/09/11 13:59:07, mmenke wrote: > Was thinking of having this here: > > IncreaseSparseHistogramByValue( > GetHistogramName("DataUse.MessageSize.AllServices", > GetSuffixForHistogramName(dir))); Done.
The CQ bit was checked by amohammadkhan@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from blundell@chromium.org, mathp@chromium.org, asvitkine@chromium.org, thestig@chromium.org, sclittle@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1279543002/#ps750001 (title: "Addressed comments and rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1279543002/750001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1279543002/750001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
amohammadkhan@google.com changed reviewers: + brettw@chromium.org, jochen@chromium.org
brettw: +url in DEPS jochen: +content/public/browser in DEPS
amohammadkhan@google.com changed reviewers: + avi@chromium.org - jochen@chromium.org
avi: content/public/browser in DEPS -jochen
https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... File chrome/browser/net/data_use_measurement.h (right): https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.h:29: enum ConnectionType { CELLULAR, NOT_CELLULAR }; I like the enums even for binary states in many cases, to avoid function calls with (true, false, true, true) and you don't know what's going on. However, these are in the global namespace. I don't think you should be introducing values like FOREGROUND, etc. into the global namespace. Also, the enum names "AppState" and "ConnectionType" might be overly generic in a product as big as Chrome. If these can be moved inside of DataUseManagement, that would be best.
LGTM with enums moved.
https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... File chrome/browser/net/data_use_measurement.h (right): https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.h:29: enum ConnectionType { CELLULAR, NOT_CELLULAR }; On 2015/09/11 19:53:44, brettw wrote: > I like the enums even for binary states in many cases, to avoid function calls > with (true, false, true, true) and you don't know what's going on. > > However, these are in the global namespace. I don't think you should be > introducing values like FOREGROUND, etc. into the global namespace. Also, the > enum names "AppState" and "ConnectionType" might be overly generic in a product > as big as Chrome. > > If these can be moved inside of DataUseManagement, that would be best. You're looking at the wrong patch set.
https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... File chrome/browser/net/data_use_measurement.h (right): https://codereview.chromium.org/1279543002/diff/100001/chrome/browser/net/dat... chrome/browser/net/data_use_measurement.h:29: enum ConnectionType { CELLULAR, NOT_CELLULAR }; On 2015/09/11 19:53:44, brettw wrote: > I like the enums even for binary states in many cases, to avoid function calls > with (true, false, true, true) and you don't know what's going on. > > However, these are in the global namespace. I don't think you should be > introducing values like FOREGROUND, etc. into the global namespace. Also, the > enum names "AppState" and "ConnectionType" might be overly generic in a product > as big as Chrome. > > If these can be moved inside of DataUseManagement, that would be best. I totally agree with you and as mmenke@ said they are inside the namespace in last patch.
I'm assuming that components/data_use_measurement is one of those components designed to rely on content, so lgtm
The CQ bit was checked by amohammadkhan@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1279543002/750001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1279543002/750001
Message was sent while issue was closed.
Committed patchset #37 (id:750001)
Message was sent while issue was closed.
Patchset 37 (id:??) landed as https://crrev.com/092adb2451c74c4dc97e1808bfbe2352ad21da9d Cr-Commit-Position: refs/heads/master@{#348490}
Message was sent while issue was closed.
Patchset 37 (id:??) landed as https://crrev.com/092adb2451c74c4dc97e1808bfbe2352ad21da9d Cr-Commit-Position: refs/heads/master@{#348490} |
