Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2158843002:
Introduce a request throttler for limiting requests in mobile NTP. (Closed)
DescriptionIntroduce a request throttler for limiting requests in mobile NTP.
This CL adds a generic throttler of requests that
- checks new requests against daily quota (configurable by Finch),
- reports to UMA status of each request w.r.t. the quota,
- reports to UMA the total count of request per day.
As an example application, it adds throttler for NTPSnippetsFetcher. This throttler will be used in another CL.
BUG=627073
Committed: https://crrev.com/505ea3a812f9a245c4e056e86c5dc59347d9440f
Cr-Commit-Position: refs/heads/master@{#407124}
Patch Set 1 #Patch Set 2 : Fixing #Patch Set 3 : Fixing #2 #Patch Set 4 : Disallow copy and assign #Patch Set 5 : A minor fix #Patch Set 6 : Fix a build error #Patch Set 7 : A minor fix #2 #Patch Set 8 : A minor fix #3 #
Total comments: 27
Patch Set 9 : Marc's comments #
Total comments: 39
Patch Set 10 : Marc's comments #2 #Patch Set 11 : A minor fix #
Total comments: 30
Patch Set 12 : Marc's comments #3 #Patch Set 13 : A minor fix #2 #
Total comments: 10
Patch Set 14 : Marc's comments #4 #
Total comments: 10
Patch Set 15 : Rebase #Patch Set 16 : Marc's comments #5 + a bit of polish #Patch Set 17 : Fix to compile unittests #
Total comments: 12
Patch Set 18 : Alexei's and Tim's comments #Patch Set 19 : A minor change #
Total comments: 10
Patch Set 20 : Tim's comments #Patch Set 21 : Tim's comments (renaming) #
Total comments: 8
Dependent Patchsets: Messages
Total messages: 69 (35 generated)
jkrcal@chromium.org changed reviewers: + treib@chromium.org
PTAL, Marc.
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/18 12:58:28, jkrcal wrote: > PTAL, Marc. As a reference, first application of the RequestCounter is here: https://codereview.chromium.org/2162533002/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... File components/ntp_snippets/pref_names.cc (right): https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/pref_names.cc:13: const char kSnippetQuotaCountFormat[] = "ntp_snippets.quota.%s.count"; Hmmm, this is very uncommon AFAIK. Do you have any precedent for generated pref names? If not, then I'd prefer a scheme where the client (i.e. NTPSnippetsFetcher) registers the prefs itself, and passes the names to the RequestCounter. https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... File components/ntp_snippets/pref_names.h (right): https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/pref_names.h:14: extern const char kSnippetQuotaCountFormat[]; Could you add comments on what exactly these mean? What is "day" and "count"? https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... File components/ntp_snippets/request_counter.cc (right): https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:39: if (!base::StringToInt(quota, "a_)) { Maybe warn if the param is non-empty, but fails to parse? https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:67: GetTotalHistogramName(), This doesn't work - the UMA_HISTOGRAM macros require that the histogram name is a constant. Example of what to do instead: https://cs.chromium.org/chromium/src/components/ntp_tiles/most_visited_sites.... https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:85: counter_ = pref_service_->GetInteger(GetCountPref(id_)); I assume the problem is that the perf service is null in tests? If so, I'd prefer to keep the initialization in the ctor, behind an "if (prefs_)". (Alternatively, IIRC it's not very hard to pass in a fake pref service.) https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:88: base::TimeDelta::FromMilliseconds(base::Time::Now().ToJavaTime()) Why not just Now().InDays()? https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... File components/ntp_snippets/request_counter.h (right): https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/request_counter.h:25: // "NewTabPage.RequestsPerUser.|Id|" histogram RequestsPerUser and RequestTotal would only be different if there are multiple profiles? Also, these don't match the actual UMA names in the .cc file. https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/request_counter.h:26: // (note that your "example_id" is autocapitalized to "ExampleId") Why? https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/request_counter.h:30: // - define the two histograms in histogram.xml (see previous applications). histograms.xml has a concept of "histogram suffixes", would that work here? (I think the "suffix" can also be in the middle of the name) https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/request_counter.h:30: // - define the two histograms in histogram.xml (see previous applications). Hmmmm, I don't really like this. So, there's a central list of "allowed" IDs (histograms.xml), but the actual names in code are scattered all over the place, and even re-formatted automatically. That will make it super hard to search for "where is this histogram coming from". Alternative suggestion: Have a central list (enum) of counters here, similar to what components/data_use_measurement does: https://cs.chromium.org/chromium/src/components/data_use_measurement/core/dat... WDYT? https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/request_counter.h:55: bool IsQuotaAvailable(); The name sounds like this is a pure getter that doesn't change anything. Maybe "RequestQuota", to make clearer what actually happens? (Though "Request" is also not that clear...) https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/request_counter.h:68: void ResetCounterIfDayChanged(); Methods come before data members. https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... File components/ntp_snippets/request_counter_unittest.cc (right): https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/request_counter_unittest.cc:20: : counter_(&test_prefs_, kTestId, 2) { nit: I think this should be indented by four more spaces?
Description was changed from ========== Introduce a request counter for throttling requests in mobile NTP. This CL adds a generic counter of requests that - checks new requests against daily quota (configurable by Finch), - reports to UMA status of each request w.r.t. the quota, - reports to UMA the total count of request per user per day. BUG=627073 ========== to ========== Introduce a request counter for throttling requests in mobile NTP. This CL adds a generic counter of requests that - checks new requests against daily quota (configurable by Finch), - reports to UMA status of each request w.r.t. the quota, - reports to UMA the total count of request per user per day. As an example it adds application counter for NTPSnippetsFetcher. This counter will be used in another CL. BUG=627073 ==========
Thanks, PTAL, again! (I am sorry for not having a separate rebase pathset :|) https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... File components/ntp_snippets/pref_names.cc (right): https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/pref_names.cc:13: const char kSnippetQuotaCountFormat[] = "ntp_snippets.quota.%s.count"; On 2016/07/19 09:06:17, Marc Treib wrote: > Hmmm, this is very uncommon AFAIK. Do you have any precedent for generated pref > names? > If not, then I'd prefer a scheme where the client (i.e. NTPSnippetsFetcher) > registers the prefs itself, and passes the names to the RequestCounter. Now, I register all of them in RequestCounter. https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... File components/ntp_snippets/pref_names.h (right): https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/pref_names.h:14: extern const char kSnippetQuotaCountFormat[]; On 2016/07/19 09:06:17, Marc Treib wrote: > Could you add comments on what exactly these mean? What is "day" and "count"? Done. https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... File components/ntp_snippets/request_counter.cc (right): https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:39: if (!base::StringToInt(quota, "a_)) { On 2016/07/19 09:06:17, Marc Treib wrote: > Maybe warn if the param is non-empty, but fails to parse? Done. https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:67: GetTotalHistogramName(), On 2016/07/19 09:06:17, Marc Treib wrote: > This doesn't work - the UMA_HISTOGRAM macros require that the histogram name is > a constant. Example of what to do instead: > https://cs.chromium.org/chromium/src/components/ntp_tiles/most_visited_sites.... Done. https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:85: counter_ = pref_service_->GetInteger(GetCountPref(id_)); On 2016/07/19 09:06:17, Marc Treib wrote: > I assume the problem is that the perf service is null in tests? If so, I'd > prefer to keep the initialization in the ctor, behind an "if (prefs_)". > (Alternatively, IIRC it's not very hard to pass in a fake pref service.) Done. https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:88: base::TimeDelta::FromMilliseconds(base::Time::Now().ToJavaTime()) On 2016/07/19 09:06:17, Marc Treib wrote: > Why not just Now().InDays()? Found something that looks much better to me. https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... File components/ntp_snippets/request_counter.h (right): https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/request_counter.h:25: // "NewTabPage.RequestsPerUser.|Id|" histogram On 2016/07/19 09:06:17, Marc Treib wrote: > RequestsPerUser and RequestTotal would only be different if there are multiple > profiles? > Also, these don't match the actual UMA names in the .cc file. Clarified. https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/request_counter.h:26: // (note that your "example_id" is autocapitalized to "ExampleId") On 2016/07/19 09:06:17, Marc Treib wrote: > Why? Removed. https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/request_counter.h:30: // - define the two histograms in histogram.xml (see previous applications). On 2016/07/19 09:06:17, Marc Treib wrote: > Hmmmm, I don't really like this. So, there's a central list of "allowed" IDs > (histograms.xml), but the actual names in code are scattered all over the place, > and even re-formatted automatically. That will make it super hard to search for > "where is this histogram coming from". > Alternative suggestion: Have a central list (enum) of counters here, similar to > what components/data_use_measurement does: > https://cs.chromium.org/chromium/src/components/data_use_measurement/core/dat... > > WDYT? Done. https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/request_counter.h:55: bool IsQuotaAvailable(); On 2016/07/19 09:06:17, Marc Treib wrote: > The name sounds like this is a pure getter that doesn't change anything. Maybe > "RequestQuota", to make clearer what actually happens? (Though "Request" is also > not that clear...) Changed to GetQuota() which avoids overloading the "Request" word. https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/request_counter.h:68: void ResetCounterIfDayChanged(); On 2016/07/19 09:06:17, Marc Treib wrote: > Methods come before data members. Done. https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... File components/ntp_snippets/request_counter_unittest.cc (right): https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/request_counter_unittest.cc:20: : counter_(&test_prefs_, kTestId, 2) { On 2016/07/19 09:06:17, Marc Treib wrote: > nit: I think this should be indented by four more spaces? Done.
https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... File components/ntp_snippets/request_counter.cc (right): https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:39: if (!base::StringToInt(quota, "a_)) { On 2016/07/20 09:40:40, jkrcal wrote: > On 2016/07/19 09:06:17, Marc Treib wrote: > > Maybe warn if the param is non-empty, but fails to parse? > > Done. This should be LOG, not DCHECK - getting invalid input is not a bug in Chrome :) (There's LOG_IF for this purpose!) https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... File components/ntp_snippets/request_counter.cc (right): https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:31: RequestCounter::RequestType type, nit: "RequestCounter::" not needed https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:36: std::string quota = variations::GetVariationParamValue( This should really be GetVariationParamValueByFeature, but I guess that's not possible yet because the feature is currently defined somewhere in c/b/android/... oh well. https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:73: DLOG(WARNING) << "Counter " << now_day; This is too spammy - DVLOG? https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:80: // Day has changed - report the no of requests from the previous day. s/no/number https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:89: counter_ = 0; Another argument for just having the pref as the single source-of-truth: You're just setting the variable here, but leaving the pref at its old value. https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:96: switch (type_) { Hm. Could we just have a global array mapping from RequestType to string, pref name, and whatever else is needed? Then there'd be a single place where all these things need to be added. https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:99: default: I would leave out the default case, then you should get a compiler error if a new enum value is created but not added to the switch. (Also in the other switches) https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:126: void RequestCounter::RegisterProfilePrefs(PrefRegistrySimple* registry) { nit: move up, to match the order in the header https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... File components/ntp_snippets/request_counter.h (right): https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.h:48: enum class RequestStatus { I think if you add " : int" here (before the "{"), then you might not need all the static_casts? https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.h:52: REQUEST_STATUS_MAX This is a COUNT, not a MAX. https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.h:56: RequestCounter::RequestType type, nit: "RequestCounter::" not needed https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.h:64: bool GetQuota(); Now it sounds like "tell me the quota"... https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.h:77: RequestCounter::RequestType type_; const, to make it clear? https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.h:79: int counter_; Do you need to store this? Might as well just make a getter that reads the pref. Also I'd like a more unambiguous name - request_count_today_ or something like that? https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.h:83: base::HistogramBase* histogram_total_; If you do this for one histogram, then you should do it for other one too IMO, for consistency. https://codereview.chromium.org/2158843002/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2158843002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:35021: + is tracked only after midnight of the given day passes - right before the "tracked" isn't quite right - the values are always tracked, it's *emitted* after midnight https://codereview.chromium.org/2158843002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:35028: +<histogram name="NewTabPage.RequestCounter.Total" Can we replace "Total" by something more meaningful - "RequestStatus" maybe?
Thanks, again! Very useful. PTAL. https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... File components/ntp_snippets/request_counter.cc (right): https://codereview.chromium.org/2158843002/diff/140001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:39: if (!base::StringToInt(quota, "a_)) { On 2016/07/20 10:19:05, Marc Treib wrote: > On 2016/07/20 09:40:40, jkrcal wrote: > > On 2016/07/19 09:06:17, Marc Treib wrote: > > > Maybe warn if the param is non-empty, but fails to parse? > > > > Done. > > This should be LOG, not DCHECK - getting invalid input is not a bug in Chrome :) > (There's LOG_IF for this purpose!) Done. https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... File components/ntp_snippets/request_counter.cc (right): https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:31: RequestCounter::RequestType type, On 2016/07/20 10:19:05, Marc Treib wrote: > nit: "RequestCounter::" not needed Done. https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:36: std::string quota = variations::GetVariationParamValue( On 2016/07/20 10:19:05, Marc Treib wrote: > This should really be GetVariationParamValueByFeature, but I guess that's not > possible yet because the feature is currently defined somewhere in > c/b/android/... oh well. This is exactly the reason. I do not think it is worth passing the feature to the service / fetcher / ... https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:73: DLOG(WARNING) << "Counter " << now_day; On 2016/07/20 10:19:05, Marc Treib wrote: > This is too spammy - DVLOG? Oh, I am sorry, a debugging line leaked into the CL :) https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:80: // Day has changed - report the no of requests from the previous day. On 2016/07/20 10:19:05, Marc Treib wrote: > s/no/number Done. https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:89: counter_ = 0; On 2016/07/20 10:19:05, Marc Treib wrote: > Another argument for just having the pref as the single source-of-truth: You're > just setting the variable here, but leaving the pref at its old value. Not a bug, the value is set right after in GetQuota. Place for potential future bug, though... Okay, I removed |counter_|. https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:96: switch (type_) { On 2016/07/20 10:19:05, Marc Treib wrote: > Hm. Could we just have a global array mapping from RequestType to string, pref > name, and whatever else is needed? Then there'd be a single place where all > these things need to be added. Done. https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:99: default: On 2016/07/20 10:19:05, Marc Treib wrote: > I would leave out the default case, then you should get a compiler error if a > new enum value is created but not added to the switch. > (Also in the other switches) You are right. The code is not there, anymore. https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:126: void RequestCounter::RegisterProfilePrefs(PrefRegistrySimple* registry) { On 2016/07/20 10:19:05, Marc Treib wrote: > nit: move up, to match the order in the header Done. https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... File components/ntp_snippets/request_counter.h (right): https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.h:48: enum class RequestStatus { On 2016/07/20 10:19:05, Marc Treib wrote: > I think if you add " : int" here (before the "{"), then you might not need all > the static_casts? No, it does not compile without the static cast. https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.h:52: REQUEST_STATUS_MAX On 2016/07/20 10:19:06, Marc Treib wrote: > This is a COUNT, not a MAX. Done. https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.h:56: RequestCounter::RequestType type, On 2016/07/20 10:19:05, Marc Treib wrote: > nit: "RequestCounter::" not needed Done. https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.h:64: bool GetQuota(); On 2016/07/20 10:19:06, Marc Treib wrote: > Now it sounds like "tell me the quota"... Huh, you are right. I've forgotten about |quota_|. What about Demand? https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.h:77: RequestCounter::RequestType type_; On 2016/07/20 10:19:06, Marc Treib wrote: > const, to make it clear? Done. https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.h:79: int counter_; On 2016/07/20 10:19:06, Marc Treib wrote: > Do you need to store this? Might as well just make a getter that reads the pref. > Also I'd like a more unambiguous name - request_count_today_ or something like > that? Done. https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.h:83: base::HistogramBase* histogram_total_; On 2016/07/20 10:19:05, Marc Treib wrote: > If you do this for one histogram, then you should do it for other one too IMO, > for consistency. This histogram is used in each request and on two different places in code. The other is used at most once per day and only on one place in code. Do you insist on consistency, here? https://codereview.chromium.org/2158843002/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2158843002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:35021: + is tracked only after midnight of the given day passes - right before the On 2016/07/20 10:19:06, Marc Treib wrote: > "tracked" isn't quite right - the values are always tracked, it's *emitted* > after midnight Done. https://codereview.chromium.org/2158843002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:35028: +<histogram name="NewTabPage.RequestCounter.Total" On 2016/07/20 10:19:06, Marc Treib wrote: > Can we replace "Total" by something more meaningful - "RequestStatus" maybe? Done.
https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... File components/ntp_snippets/request_counter.cc (right): https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:36: std::string quota = variations::GetVariationParamValue( On 2016/07/20 12:09:27, jkrcal wrote: > On 2016/07/20 10:19:05, Marc Treib wrote: > > This should really be GetVariationParamValueByFeature, but I guess that's not > > possible yet because the feature is currently defined somewhere in > > c/b/android/... oh well. > > This is exactly the reason. I do not think it is worth passing the feature to > the service / fetcher / ... No, we should just move the feature definition into the component. (That's for another CL though) https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... File components/ntp_snippets/request_counter.h (right): https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.h:48: enum class RequestStatus { On 2016/07/20 12:09:27, jkrcal wrote: > On 2016/07/20 10:19:05, Marc Treib wrote: > > I think if you add " : int" here (before the "{"), then you might not need all > > the static_casts? > > No, it does not compile without the static cast. Alright :( Then no need to add the ": int" I guess https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.h:64: bool GetQuota(); On 2016/07/20 12:09:27, jkrcal wrote: > On 2016/07/20 10:19:06, Marc Treib wrote: > > Now it sounds like "tell me the quota"... > > Huh, you are right. I've forgotten about |quota_|. > What about Demand? Yeah, this is much better, thanks! Maybe add "ForRequest", for some sort of consistency with ReportForcedRequest? https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.h:83: base::HistogramBase* histogram_total_; On 2016/07/20 12:09:28, jkrcal wrote: > On 2016/07/20 10:19:05, Marc Treib wrote: > > If you do this for one histogram, then you should do it for other one too IMO, > > for consistency. > > This histogram is used in each request and on two different places in code. > The other is used at most once per day and only on one place in code. > Do you insist on consistency, here? Ah, so it's for performance reasons? Okay, I guess... I'd still prefer consistency, because I like consistency and it doesn't really hurt either IMO, but I won't insist. https://codereview.chromium.org/2158843002/diff/200001/chrome/browser/prefs/b... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/2158843002/diff/200001/chrome/browser/prefs/b... chrome/browser/prefs/browser_prefs.cc:465: ntp_snippets::RequestCounter::RegisterProfilePrefs(registry); Hm, you could call this from NTPSnippetsService::RegisterProfilePrefs to avoid adding another line here, but I guess that won't make sense anymore when it's used in multiple places... WDYT? https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... File components/ntp_snippets/request_counter.cc (right): https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:61: // Construct the histogram that is used for every request. The other histogram s/Construct/Lookup https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:67: 1, status_count, status_count + 1, Is this correct? I think you're adding one more bin to the histogram than you need. https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... File components/ntp_snippets/request_counter.h (right): https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... components/ntp_snippets/request_counter.h:35: // RegisterProfilePrefs()), Can't RegisterProfilePrefs just loop over the array and register them all? https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... components/ntp_snippets/request_counter.h:41: // used for uma. nit: UMA, all caps https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... components/ntp_snippets/request_counter.h:74: std::string GetDaysPref() const; GetDayPref I'd just have made "GetDay", "SetDay" etc, so the rest of the code doesn't need to know about the prefs at all. https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... File components/ntp_snippets/request_counter_unittest.cc (right): https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... components/ntp_snippets/request_counter_unittest.cc:23: &test_prefs_, RequestCounter::RequestType::ARTICLE_CONTENT_FETCHER, 2)); I'd make the "2" a constant, so it's clearer what it means. https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... components/ntp_snippets/request_counter_unittest.cc:50: // Now fake the counter to believe the count comes from yesterday. nit: You're faking the "day" pref, not the counter. Generally, it would be much nicer to have a controllable FakeTime class, then we wouldn't have to muck around with the prefs, but it seems we don't have that in Chrome?! https://codereview.chromium.org/2158843002/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2158843002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:35019: + It records how many requests of the given type the user performed each day nitty nit: Remove the leading "It" https://codereview.chromium.org/2158843002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:35019: + It records how many requests of the given type the user performed each day nit: "the user" kinda sounds like these are all user-initiated requests https://codereview.chromium.org/2158843002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:35023: + not use chrome in the meantime). Forced requests are not counted in this nitty nit 2: capitalize Chrome https://codereview.chromium.org/2158843002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:35024: + histogram. ...but requests that weren't actually made, because the quota was exhausted, *are* counted, which is a bit surprising and at least worth a mention here.
Thanks, again! I must admit, this whole CL does not make me feel very competent ;) PTAL. https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... File components/ntp_snippets/request_counter.cc (right): https://codereview.chromium.org/2158843002/diff/160001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:36: std::string quota = variations::GetVariationParamValue( On 2016/07/20 12:58:27, Marc Treib wrote: > On 2016/07/20 12:09:27, jkrcal wrote: > > On 2016/07/20 10:19:05, Marc Treib wrote: > > > This should really be GetVariationParamValueByFeature, but I guess that's > not > > > possible yet because the feature is currently defined somewhere in > > > c/b/android/... oh well. > > > > This is exactly the reason. I do not think it is worth passing the feature to > > the service / fetcher / ... > > No, we should just move the feature definition into the component. (That's for > another CL though) Okay, this would make sense. https://codereview.chromium.org/2158843002/diff/200001/chrome/browser/prefs/b... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/2158843002/diff/200001/chrome/browser/prefs/b... chrome/browser/prefs/browser_prefs.cc:465: ntp_snippets::RequestCounter::RegisterProfilePrefs(registry); On 2016/07/20 12:58:27, Marc Treib wrote: > Hm, you could call this from NTPSnippetsService::RegisterProfilePrefs to avoid > adding another line here, but I guess that won't make sense anymore when it's > used in multiple places... WDYT? I've though about it. I do not think that the RequestCounter has conceptually much to do with the NTPSnippetsService. Thus, I do not like this idea of saving one line here. https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... File components/ntp_snippets/request_counter.cc (right): https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:61: // Construct the histogram that is used for every request. The other histogram On 2016/07/20 12:58:27, Marc Treib wrote: > s/Construct/Lookup Done. https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:67: 1, status_count, status_count + 1, On 2016/07/20 12:58:27, Marc Treib wrote: > Is this correct? I think you're adding one more bin to the histogram than you > need. I emulate the behaviour of UMA_HISTOGRAM_ENUMERATION (https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?sq=packa...). I did not dare to question correctness of this macro as there are multiple weird things I do not understand (such as that the minimum must be set to 1 even though 0 can be reported). Maybe they expect to have one bin for values beyond the range for catching errors? https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... File components/ntp_snippets/request_counter.h (right): https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... components/ntp_snippets/request_counter.h:35: // RegisterProfilePrefs()), On 2016/07/20 12:58:27, Marc Treib wrote: > Can't RegisterProfilePrefs just loop over the array and register them all? Done. https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... components/ntp_snippets/request_counter.h:41: // used for uma. On 2016/07/20 12:58:27, Marc Treib wrote: > nit: UMA, all caps Done. https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... components/ntp_snippets/request_counter.h:74: std::string GetDaysPref() const; On 2016/07/20 12:58:27, Marc Treib wrote: > GetDayPref > > I'd just have made "GetDay", "SetDay" etc, so the rest of the code doesn't need > to know about the prefs at all. Done. https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... File components/ntp_snippets/request_counter_unittest.cc (right): https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... components/ntp_snippets/request_counter_unittest.cc:23: &test_prefs_, RequestCounter::RequestType::ARTICLE_CONTENT_FETCHER, 2)); On 2016/07/20 12:58:27, Marc Treib wrote: > I'd make the "2" a constant, so it's clearer what it means. Done. https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... components/ntp_snippets/request_counter_unittest.cc:50: // Now fake the counter to believe the count comes from yesterday. On 2016/07/20 12:58:27, Marc Treib wrote: > nit: You're faking the "day" pref, not the counter. > > Generally, it would be much nicer to have a controllable FakeTime class, then we > wouldn't have to muck around with the prefs, but it seems we don't have that in > Chrome?! I wanted to have that but did not find anything reasonable. This was then a way simpler solution. https://codereview.chromium.org/2158843002/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2158843002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:35019: + It records how many requests of the given type the user performed each day On 2016/07/20 12:58:27, Marc Treib wrote: > nitty nit: Remove the leading "It" Done. https://codereview.chromium.org/2158843002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:35019: + It records how many requests of the given type the user performed each day On 2016/07/20 12:58:27, Marc Treib wrote: > nit: "the user" kinda sounds like these are all user-initiated requests Done. https://codereview.chromium.org/2158843002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:35023: + not use chrome in the meantime). Forced requests are not counted in this On 2016/07/20 12:58:27, Marc Treib wrote: > nitty nit 2: capitalize Chrome Done. https://codereview.chromium.org/2158843002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:35024: + histogram. On 2016/07/20 12:58:27, Marc Treib wrote: > ...but requests that weren't actually made, because the quota was exhausted, > *are* counted, which is a bit surprising and at least worth a mention here. Maybe surprising but I think it gives the best insight into the behaviour of the throttled fetcher. Comment changed.
https://codereview.chromium.org/2158843002/diff/200001/chrome/browser/prefs/b... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/2158843002/diff/200001/chrome/browser/prefs/b... chrome/browser/prefs/browser_prefs.cc:465: ntp_snippets::RequestCounter::RegisterProfilePrefs(registry); On 2016/07/20 14:20:17, jkrcal wrote: > On 2016/07/20 12:58:27, Marc Treib wrote: > > Hm, you could call this from NTPSnippetsService::RegisterProfilePrefs to avoid > > adding another line here, but I guess that won't make sense anymore when it's > > used in multiple places... WDYT? > > I've though about it. I do not think that the RequestCounter has conceptually > much to do with the NTPSnippetsService. Thus, I do not like this idea of saving > one line here. Acknowledged. https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... File components/ntp_snippets/request_counter.cc (right): https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:67: 1, status_count, status_count + 1, On 2016/07/20 14:20:17, jkrcal wrote: > On 2016/07/20 12:58:27, Marc Treib wrote: > > Is this correct? I think you're adding one more bin to the histogram than you > > need. > > I emulate the behaviour of UMA_HISTOGRAM_ENUMERATION > (https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?sq=packa...). > I did not dare to question correctness of this macro as there are multiple weird > things I do not understand (such as that the minimum must be set to 1 even > though 0 can be reported). Maybe they expect to have one bin for values beyond > the range for catching errors? Huh, indeed! I thought it was supposed to be "max" and "max + 1", but looks like I was wrong. https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... File components/ntp_snippets/request_counter.h (right): https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... components/ntp_snippets/request_counter.h:35: // RegisterProfilePrefs()), On 2016/07/20 14:20:17, jkrcal wrote: > On 2016/07/20 12:58:27, Marc Treib wrote: > > Can't RegisterProfilePrefs just loop over the array and register them all? > > Done. Great! Please update the comment too :) https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... File components/ntp_snippets/request_counter_unittest.cc (right): https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... components/ntp_snippets/request_counter_unittest.cc:50: // Now fake the counter to believe the count comes from yesterday. On 2016/07/20 14:20:17, jkrcal wrote: > On 2016/07/20 12:58:27, Marc Treib wrote: > > nit: You're faking the "day" pref, not the counter. > > > > Generally, it would be much nicer to have a controllable FakeTime class, then > we > > wouldn't have to muck around with the prefs, but it seems we don't have that > in > > Chrome?! > > I wanted to have that but did not find anything reasonable. > This was then a way simpler solution. Hah, found it: https://cs.chromium.org/chromium/src/base/test/simple_test_clock.h (I did vaguely remember seeing something like that) The way to use it would be to have a Clock instance in the RequestCounter class and use that instead of base::Time. Then tests can set it to a SimpleTestClock instance and control that. I'll leave it up to you whether you want to do that :) https://codereview.chromium.org/2158843002/diff/240001/components/ntp_snippet... File components/ntp_snippets/request_counter.cc (right): https://codereview.chromium.org/2158843002/diff/240001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:77: for (unsigned int i = 0; i < arraysize(kRequestTypeInfo); i++) { I think you can use a range-based for loop: for (const RequestTypeInfo& info : kRequestTypeInfo) https://codereview.chromium.org/2158843002/diff/240001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:142: kRequestTypeInfo[static_cast<int>(type_)].day_pref); optional: You could store a reference to the proper RequestTypeInfo as a member. https://codereview.chromium.org/2158843002/diff/240001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2158843002/diff/240001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:35034: + imposed. The type of request is specified by the _suffix of the histogram. Grammar nit: "Records the status w.r.t. the quota for all requests of the given type." https://codereview.chromium.org/2158843002/diff/240001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:50727: + "animated"if the animation would have run otherwise. Unintentional?
Marc, thanks and PTAL, again! https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... File components/ntp_snippets/request_counter.h (right): https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... components/ntp_snippets/request_counter.h:35: // RegisterProfilePrefs()), On 2016/07/20 15:13:57, Marc Treib wrote: > On 2016/07/20 14:20:17, jkrcal wrote: > > On 2016/07/20 12:58:27, Marc Treib wrote: > > > Can't RegisterProfilePrefs just loop over the array and register them all? > > > > Done. > > Great! Please update the comment too :) Done. https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... File components/ntp_snippets/request_counter_unittest.cc (right): https://codereview.chromium.org/2158843002/diff/200001/components/ntp_snippet... components/ntp_snippets/request_counter_unittest.cc:50: // Now fake the counter to believe the count comes from yesterday. On 2016/07/20 15:13:57, Marc Treib wrote: > On 2016/07/20 14:20:17, jkrcal wrote: > > On 2016/07/20 12:58:27, Marc Treib wrote: > > > nit: You're faking the "day" pref, not the counter. > > > > > > Generally, it would be much nicer to have a controllable FakeTime class, > then > > we > > > wouldn't have to muck around with the prefs, but it seems we don't have that > > in > > > Chrome?! > > > > I wanted to have that but did not find anything reasonable. > > This was then a way simpler solution. > > Hah, found it: > https://cs.chromium.org/chromium/src/base/test/simple_test_clock.h > (I did vaguely remember seeing something like that) > The way to use it would be to have a Clock instance in the RequestCounter class > and use that instead of base::Time. Then tests can set it to a SimpleTestClock > instance and control that. > I'll leave it up to you whether you want to do that :) Thanks for the pointer! I would like to move on. Thus, I leave it for a further CL :) https://codereview.chromium.org/2158843002/diff/240001/components/ntp_snippet... File components/ntp_snippets/request_counter.cc (right): https://codereview.chromium.org/2158843002/diff/240001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:77: for (unsigned int i = 0; i < arraysize(kRequestTypeInfo); i++) { On 2016/07/20 15:13:58, Marc Treib wrote: > I think you can use a range-based for loop: > for (const RequestTypeInfo& info : kRequestTypeInfo) Indeed! Is it a general C++(11) feature or is there some chrome magic involved (as for arraysize())? https://codereview.chromium.org/2158843002/diff/240001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:142: kRequestTypeInfo[static_cast<int>(type_)].day_pref); On 2016/07/20 15:13:57, Marc Treib wrote: > optional: You could store a reference to the proper RequestTypeInfo as a member. Done. https://codereview.chromium.org/2158843002/diff/240001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2158843002/diff/240001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:35034: + imposed. The type of request is specified by the _suffix of the histogram. On 2016/07/20 15:13:58, Marc Treib wrote: > Grammar nit: "Records the status w.r.t. the quota for all requests of the given > type." Done. https://codereview.chromium.org/2158843002/diff/240001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:50727: + "animated"if the animation would have run otherwise. On 2016/07/20 15:13:58, Marc Treib wrote: > Unintentional? Indeed!
jkrcal@chromium.org changed reviewers: + asvitkine@chromium.org, battre@chromium.org
Dominic, PTAL at browser_prefs.cc. Alexei, PTAL at histograms.xml + a bonus question: Why does histogram created by UMA_HISTOGRAM_ENUMERATION have one more bucket than the enum has members?
LGTM with some last nits :) https://codereview.chromium.org/2158843002/diff/240001/components/ntp_snippet... File components/ntp_snippets/request_counter.cc (right): https://codereview.chromium.org/2158843002/diff/240001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:77: for (unsigned int i = 0; i < arraysize(kRequestTypeInfo); i++) { On 2016/07/20 15:46:23, jkrcal wrote: > On 2016/07/20 15:13:58, Marc Treib wrote: > > I think you can use a range-based for loop: > > for (const RequestTypeInfo& info : kRequestTypeInfo) > > Indeed! > Is it a general C++(11) feature or is there some chrome magic involved (as for > arraysize())? It's a C++11 thing, in particular non-member begin/end I think (which might be implemented in a similar way to the arraysize() magic, but the behavior is defined in the standard). https://codereview.chromium.org/2158843002/diff/260001/components/ntp_snippet... File components/ntp_snippets/request_counter.cc (right): https://codereview.chromium.org/2158843002/diff/260001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:29: using ntp_snippets::RequestCounter; You can also put the anonymous namespace into the ntp_snippets namespace to avoid having to say "ntp_snippets::" all the time. (Either way is fine though) https://codereview.chromium.org/2158843002/diff/260001/components/ntp_snippet... File components/ntp_snippets/request_counter.h (right): https://codereview.chromium.org/2158843002/diff/260001/components/ntp_snippet... components/ntp_snippets/request_counter.h:34: // b) constants for days/count prefs; s/days/day https://codereview.chromium.org/2158843002/diff/260001/components/ntp_snippet... components/ntp_snippets/request_counter.h:40: // used for UMA. "Not used for UMA" is the default assumption, no need to mention explicitly https://codereview.chromium.org/2158843002/diff/260001/components/ntp_snippet... components/ntp_snippets/request_counter.h:56: struct RequestTypeInfo { You can just forward-declare this and keep the definition in the .cc file. In any case, it should be private. https://codereview.chromium.org/2158843002/diff/260001/components/ntp_snippet... components/ntp_snippets/request_counter.h:77: // Also emmits the PerDay histogram if the day changed. s/emmits/emits
Huge thanks, Marc! https://codereview.chromium.org/2158843002/diff/240001/components/ntp_snippet... File components/ntp_snippets/request_counter.cc (right): https://codereview.chromium.org/2158843002/diff/240001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:77: for (unsigned int i = 0; i < arraysize(kRequestTypeInfo); i++) { On 2016/07/20 16:15:14, Marc Treib wrote: > On 2016/07/20 15:46:23, jkrcal wrote: > > On 2016/07/20 15:13:58, Marc Treib wrote: > > > I think you can use a range-based for loop: > > > for (const RequestTypeInfo& info : kRequestTypeInfo) > > > > Indeed! > > Is it a general C++(11) feature or is there some chrome magic involved (as for > > arraysize())? > > It's a C++11 thing, in particular non-member begin/end I think (which might be > implemented in a similar way to the arraysize() magic, but the behavior is > defined in the standard). Thanks! https://codereview.chromium.org/2158843002/diff/260001/components/ntp_snippet... File components/ntp_snippets/request_counter.cc (right): https://codereview.chromium.org/2158843002/diff/260001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:29: using ntp_snippets::RequestCounter; On 2016/07/20 16:15:14, Marc Treib wrote: > You can also put the anonymous namespace into the ntp_snippets namespace to > avoid having to say "ntp_snippets::" all the time. (Either way is fine though) Done. https://codereview.chromium.org/2158843002/diff/260001/components/ntp_snippet... File components/ntp_snippets/request_counter.h (right): https://codereview.chromium.org/2158843002/diff/260001/components/ntp_snippet... components/ntp_snippets/request_counter.h:34: // b) constants for days/count prefs; On 2016/07/20 16:15:14, Marc Treib wrote: > s/days/day Done. https://codereview.chromium.org/2158843002/diff/260001/components/ntp_snippet... components/ntp_snippets/request_counter.h:40: // used for UMA. On 2016/07/20 16:15:14, Marc Treib wrote: > "Not used for UMA" is the default assumption, no need to mention explicitly Done. https://codereview.chromium.org/2158843002/diff/260001/components/ntp_snippet... components/ntp_snippets/request_counter.h:56: struct RequestTypeInfo { On 2016/07/20 16:15:14, Marc Treib wrote: > You can just forward-declare this and keep the definition in the .cc file. In > any case, it should be private. Done. https://codereview.chromium.org/2158843002/diff/260001/components/ntp_snippet... components/ntp_snippets/request_counter.h:77: // Also emmits the PerDay histogram if the day changed. On 2016/07/20 16:15:14, Marc Treib wrote: > s/emmits/emits Done.
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tschumann@chromium.org changed reviewers: + tschumann@chromium.org
https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... File components/ntp_snippets/pref_names.h (right): https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... components/ntp_snippets/pref_names.h:17: // The count of NTPSnippetsFetcher requests for today, so far. sorry for being nit-picky: But this constant is a name not a count :-) Would you mind updating the comment like: // The pref-name for the NTPSnippetsFetcher requests for today counter. [i'm unsure if i'm using proper terminology though] https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... File components/ntp_snippets/request_counter.h (right): https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... components/ntp_snippets/request_counter.h:33: // Thus, when you add a new RequestType of RequestCounter, you must also: it would be nice to clearly separate implementation notes for further extensions from the usage documentation. Maybe something like: // Implementation notes: When extending this class for a new RequestType, ... https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... components/ntp_snippets/request_counter.h:68: void ReportForcedRequest() const; as discussed offline, it would be nice to have a more uniform API, e.g. also have a demand functionality for forced requests and hide the reporting logic from the public interface.
https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... File components/ntp_snippets/pref_names.h (right): https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... components/ntp_snippets/pref_names.h:17: // The count of NTPSnippetsFetcher requests for today, so far. On 2016/07/21 11:37:09, tschumann wrote: > sorry for being nit-picky: But this constant is a name not a count :-) Would you > mind updating the comment like: > // The pref-name for the NTPSnippetsFetcher requests for today counter. > > [i'm unsure if i'm using proper terminology though] I think it's fairly common to have this type of comment on pref names, but generally you're right, and I don't object to being precise :) (counter-nit: Leave out the "-" in "pref name") https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... File components/ntp_snippets/request_counter.h (right): https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... components/ntp_snippets/request_counter.h:68: void ReportForcedRequest() const; On 2016/07/21 11:37:09, tschumann wrote: > as discussed offline, it would be nice to have a more uniform API, e.g. also > have a demand functionality for forced requests and hide the reporting logic > from the public interface. +1, I like this idea.
lgtm % comments > Why does histogram created by UMA_HISTOGRAM_ENUMERATION have one more bucket > than the enum has members? That's because each histogram has an overflow bucket whose range is max value to int max. This way, if the code has a bug and logs a value beyond the max value, it gets put in that bucket. https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... File components/ntp_snippets/request_counter.cc (right): https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:34: const char kUMAHistogramPerDayFormat[] = "NewTabPage.RequestCounter.PerDay_%s"; Nit: If you're only using these once and in the same file, I'd actually suggest inlining the format string so it's easier when reading the code to understand where the param goes and that it's the right number/types of params per the format string. https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:63: histogram_request_status_ = base::LinearHistogram::FactoryGet( For each of these, do you mind adding a comment about what histogram macro it corresponds to?
Thanks for the comments! (Alexei, wouldn't the overflow bucket be worth a comment in the code? Currently the HISTOGRAM_ENUMERATION_WITH_FLAG macro looks fishy, given the names of the arguments of LinearHistogram::FactoryGet.) Dominic, friendly ping! https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... File components/ntp_snippets/pref_names.h (right): https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... components/ntp_snippets/pref_names.h:17: // The count of NTPSnippetsFetcher requests for today, so far. On 2016/07/21 11:43:50, Marc Treib wrote: > On 2016/07/21 11:37:09, tschumann wrote: > > sorry for being nit-picky: But this constant is a name not a count :-) Would > you > > mind updating the comment like: > > // The pref-name for the NTPSnippetsFetcher requests for today counter. > > > > [i'm unsure if i'm using proper terminology though] > > I think it's fairly common to have this type of comment on pref names, but > generally you're right, and I don't object to being precise :) > (counter-nit: Leave out the "-" in "pref name") Done. https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... File components/ntp_snippets/request_counter.cc (right): https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:34: const char kUMAHistogramPerDayFormat[] = "NewTabPage.RequestCounter.PerDay_%s"; On 2016/07/21 14:13:29, Alexei Svitkine (slow) wrote: > Nit: If you're only using these once and in the same file, I'd actually suggest > inlining the format string so it's easier when reading the code to understand > where the param goes and that it's the right number/types of params per the > format string. Done. https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... components/ntp_snippets/request_counter.cc:63: histogram_request_status_ = base::LinearHistogram::FactoryGet( On 2016/07/21 14:13:29, Alexei Svitkine (slow) wrote: > For each of these, do you mind adding a comment about what histogram macro it > corresponds to? Done. https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... File components/ntp_snippets/request_counter.h (right): https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... components/ntp_snippets/request_counter.h:33: // Thus, when you add a new RequestType of RequestCounter, you must also: On 2016/07/21 11:37:09, tschumann wrote: > it would be nice to clearly separate implementation notes for further extensions > from the usage documentation. > Maybe something like: > // Implementation notes: When extending this class for a new RequestType, ... Done. https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... components/ntp_snippets/request_counter.h:68: void ReportForcedRequest() const; On 2016/07/21 11:43:50, Marc Treib wrote: > On 2016/07/21 11:37:09, tschumann wrote: > > as discussed offline, it would be nice to have a more uniform API, e.g. also > > have a demand functionality for forced requests and hide the reporting logic > > from the public interface. > > +1, I like this idea. Done. I documented the current behaviour. I guess that in the future, we may want to be more restrictive w.r.t. forced requests.
+1 to documenting the macro better. Happy to review your CL if you want to give it a shot! :) On Thu, Jul 21, 2016 at 2:17 PM, <jkrcal@chromium.org> wrote: > Thanks for the comments! > (Alexei, wouldn't the overflow bucket be worth a comment in the code? > Currently > the HISTOGRAM_ENUMERATION_WITH_FLAG macro looks fishy, given the names of > the > arguments of LinearHistogram::FactoryGet.) > > Dominic, friendly ping! > > > > https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... > File components/ntp_snippets/pref_names.h (right): > > > https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... > components/ntp_snippets/pref_names.h:17: // The count of > NTPSnippetsFetcher requests for today, so far. > On 2016/07/21 11:43:50, Marc Treib wrote: > > On 2016/07/21 11:37:09, tschumann wrote: > > > sorry for being nit-picky: But this constant is a name not a count > :-) Would > > you > > > mind updating the comment like: > > > // The pref-name for the NTPSnippetsFetcher requests for today > counter. > > > > > > [i'm unsure if i'm using proper terminology though] > > > > I think it's fairly common to have this type of comment on pref names, > but > > generally you're right, and I don't object to being precise :) > > (counter-nit: Leave out the "-" in "pref name") > > Done. > > > https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... > File components/ntp_snippets/request_counter.cc (right): > > > https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... > components/ntp_snippets/request_counter.cc:34: const char > kUMAHistogramPerDayFormat[] = "NewTabPage.RequestCounter.PerDay_%s"; > On 2016/07/21 14:13:29, Alexei Svitkine (slow) wrote: > > Nit: If you're only using these once and in the same file, I'd > actually suggest > > inlining the format string so it's easier when reading the code to > understand > > where the param goes and that it's the right number/types of params > per the > > format string. > > Done. > > > https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... > components/ntp_snippets/request_counter.cc:63: histogram_request_status_ > = base::LinearHistogram::FactoryGet( > On 2016/07/21 14:13:29, Alexei Svitkine (slow) wrote: > > For each of these, do you mind adding a comment about what histogram > macro it > > corresponds to? > > Done. > > > https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... > File components/ntp_snippets/request_counter.h (right): > > > https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... > components/ntp_snippets/request_counter.h:33: // Thus, when you add a > new RequestType of RequestCounter, you must also: > On 2016/07/21 11:37:09, tschumann wrote: > > it would be nice to clearly separate implementation notes for further > extensions > > from the usage documentation. > > Maybe something like: > > // Implementation notes: When extending this class for a new > RequestType, ... > > Done. > > > https://codereview.chromium.org/2158843002/diff/320001/components/ntp_snippet... > components/ntp_snippets/request_counter.h:68: void ReportForcedRequest() > const; > On 2016/07/21 11:43:50, Marc Treib wrote: > > On 2016/07/21 11:37:09, tschumann wrote: > > > as discussed offline, it would be nice to have a more uniform API, > e.g. also > > > have a demand functionality for forced requests and hide the > reporting logic > > > from the public interface. > > > > +1, I like this idea. > > Done. I documented the current behaviour. I guess that in the future, we > may want to be more restrictive w.r.t. forced requests. > > https://codereview.chromium.org/2158843002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2158843002/diff/360001/components/ntp_snippet... File components/ntp_snippets/request_counter.h (right): https://codereview.chromium.org/2158843002/diff/360001/components/ntp_snippet... components/ntp_snippets/request_counter.h:44: CONTENT_SUGGESTION_FETCHER are there plans for further types? if not, we might be better off hard-coding this (YAGNI) https://codereview.chromium.org/2158843002/diff/360001/components/ntp_snippet... components/ntp_snippets/request_counter.h:50: enum class RequestStatus { why is this exposed as part of the public interface? make it private or -- even better -- move into the .cc file? https://codereview.chromium.org/2158843002/diff/360001/components/ntp_snippet... components/ntp_snippets/request_counter.h:65: // information to UMA. Forced requests do not count against the quota and i consider the counting aspect an implementation detail. IMO, such a comment would be better in the .cc file and not on the header. For users of this class those internals shouldn't matter. What you should say is that ForcedRequests are guaranteed to get granted. If you want to you can add that they should be directly initiated by the user.
Thanks, Tim! https://codereview.chromium.org/2158843002/diff/360001/components/ntp_snippet... File components/ntp_snippets/request_counter.h (right): https://codereview.chromium.org/2158843002/diff/360001/components/ntp_snippet... components/ntp_snippets/request_counter.h:44: CONTENT_SUGGESTION_FETCHER On 2016/07/21 18:27:45, tschumann wrote: > are there plans for further types? if not, we might be better off hard-coding > this (YAGNI) Sure there are: - Fetching thumbnails, - Fetching favicons for articles, - Fetching favicons for NTP tiles, and maybe more. https://codereview.chromium.org/2158843002/diff/360001/components/ntp_snippet... components/ntp_snippets/request_counter.h:50: enum class RequestStatus { On 2016/07/21 18:27:45, tschumann wrote: > why is this exposed as part of the public interface? > make it private or -- even better -- move into the .cc file? Oh, thanks for a pair of fresh eyes. I could not spot such a mistake after so many rounds of review :) https://codereview.chromium.org/2158843002/diff/360001/components/ntp_snippet... components/ntp_snippets/request_counter.h:65: // information to UMA. Forced requests do not count against the quota and On 2016/07/21 18:27:45, tschumann wrote: > i consider the counting aspect an implementation detail. IMO, such a comment > would be better in the .cc file and not on the header. For users of this class > those internals shouldn't matter. What you should say is that ForcedRequests are > guaranteed to get granted. > If you want to you can add that they should be directly initiated by the user. I do not think this is an implementation detail. If I want to use the class, I need to understand how to pick my quota so that it never hits the user under normal conditions. Do not want to oppose you too strongly, though. I am just trying to understand your arguments for future coding.
lgtm
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
The CQ bit was unchecked by jkrcal@chromium.org
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2158843002/#ps380001 (title: "Tim's comments")
The CQ bit was unchecked by jkrcal@chromium.org
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm one more thing: I'm not sure RequestCounter is the best name for this class as it's doing much more than counting. RequestThrottler or RequestRateLimiter would be better names IMO. On the other hand, I realize I'm joining this party quite late and don't want to force things on you now. https://codereview.chromium.org/2158843002/diff/360001/components/ntp_snippet... File components/ntp_snippets/request_counter.h (right): https://codereview.chromium.org/2158843002/diff/360001/components/ntp_snippet... components/ntp_snippets/request_counter.h:44: CONTENT_SUGGESTION_FETCHER On 2016/07/21 19:17:38, jkrcal wrote: > On 2016/07/21 18:27:45, tschumann wrote: > > are there plans for further types? if not, we might be better off hard-coding > > this (YAGNI) > > Sure there are: > - Fetching thumbnails, > - Fetching favicons for articles, > - Fetching favicons for NTP tiles, > and maybe more. ok. Let's have a quick in-person chat about those and how paranoid we plan to be. Do we plan any pre-fetching of those? (if not, I'm not sure if we had any non-forced loading of such resources). https://codereview.chromium.org/2158843002/diff/360001/components/ntp_snippet... components/ntp_snippets/request_counter.h:65: // information to UMA. Forced requests do not count against the quota and On 2016/07/21 19:17:38, jkrcal wrote: > On 2016/07/21 18:27:45, tschumann wrote: > > i consider the counting aspect an implementation detail. IMO, such a comment > > would be better in the .cc file and not on the header. For users of this class > > those internals shouldn't matter. What you should say is that ForcedRequests > are > > guaranteed to get granted. > > If you want to you can add that they should be directly initiated by the user. > > I do not think this is an implementation detail. If I want to use the class, I > need to understand how to pick my quota so that it never hits the user under > normal conditions. > > Do not want to oppose you too strongly, though. I am just trying to understand > your arguments for future coding. My point would be that places which use this class only have a local view of things -- that's also not where the quota is being defined. Those places only care about having their requests properly throttled, and if somebody is interested in the details of throttling, they can read the implementation of the the throttler. IMO, this class has one task -- to throttle requests. How that's done is this classes implementation. All other parts of the system just need to make sure they apply some throttling. This might get a bit esotheric, but my main point is the following: There's a steady risk of comments getting out of sync, especially if they are not right at the implementation. If somebody really wants to understand the details of throttling, he needs to read that code. That said, I'm not feeling too strongly, as it's just a little detail. But we need to make sure the comment stays up-to-date.
Description was changed from ========== Introduce a request counter for throttling requests in mobile NTP. This CL adds a generic counter of requests that - checks new requests against daily quota (configurable by Finch), - reports to UMA status of each request w.r.t. the quota, - reports to UMA the total count of request per user per day. As an example it adds application counter for NTPSnippetsFetcher. This counter will be used in another CL. BUG=627073 ========== to ========== Introduce a request throttler for limiting requests in mobile NTP. This CL adds a generic throttler of requests that - checks new requests against daily quota (configurable by Finch), - reports to UMA status of each request w.r.t. the quota, - reports to UMA the total count of request per day. As an example application, it adds throttler for NTPSnippetsFetcher. This throttler will be used in another CL. BUG=627073 ==========
Thanks, Tim. I renamed the whole thing. After all, it is the same amount of work now as it would have been a few days ago. https://codereview.chromium.org/2158843002/diff/360001/components/ntp_snippet... File components/ntp_snippets/request_counter.h (right): https://codereview.chromium.org/2158843002/diff/360001/components/ntp_snippet... components/ntp_snippets/request_counter.h:44: CONTENT_SUGGESTION_FETCHER On 2016/07/22 07:49:08, tschumann wrote: > On 2016/07/21 19:17:38, jkrcal wrote: > > On 2016/07/21 18:27:45, tschumann wrote: > > > are there plans for further types? if not, we might be better off > hard-coding > > > this (YAGNI) > > > > Sure there are: > > - Fetching thumbnails, > > - Fetching favicons for articles, > > - Fetching favicons for NTP tiles, > > and maybe more. > > ok. Let's have a quick in-person chat about those and how paranoid we plan to > be. Do we plan any pre-fetching of those? (if not, I'm not sure if we had any > non-forced loading of such resources). Acknowledged. https://codereview.chromium.org/2158843002/diff/360001/components/ntp_snippet... components/ntp_snippets/request_counter.h:65: // information to UMA. Forced requests do not count against the quota and On 2016/07/22 07:49:08, tschumann wrote: > On 2016/07/21 19:17:38, jkrcal wrote: > > On 2016/07/21 18:27:45, tschumann wrote: > > > i consider the counting aspect an implementation detail. IMO, such a comment > > > would be better in the .cc file and not on the header. For users of this > class > > > those internals shouldn't matter. What you should say is that ForcedRequests > > are > > > guaranteed to get granted. > > > If you want to you can add that they should be directly initiated by the > user. > > > > I do not think this is an implementation detail. If I want to use the class, I > > need to understand how to pick my quota so that it never hits the user under > > normal conditions. > > > > Do not want to oppose you too strongly, though. I am just trying to understand > > your arguments for future coding. > > My point would be that places which use this class only have a local view of > things -- that's also not where the quota is being defined. Those places only > care about having their requests properly throttled, and if somebody is > interested in the details of throttling, they can read the implementation of the > the throttler. > IMO, this class has one task -- to throttle requests. How that's done is this > classes implementation. All other parts of the system just need to make sure > they apply some throttling. > > This might get a bit esotheric, but my main point is the following: There's a > steady risk of comments getting out of sync, especially if they are not right at > the implementation. If somebody really wants to understand the details of > throttling, he needs to read that code. > > That said, I'm not feeling too strongly, as it's just a little detail. But we > need to make sure the comment stays up-to-date. Thanks for explaining. Now I tend to agree (not too strongly :). Done.
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
lgtm lgtm Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, asvitkine@chromium.org, battre@chromium.org Link to the patchset: https://codereview.chromium.org/2158843002/#ps400001 (title: "Tim's comments (renaming)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Introduce a request throttler for limiting requests in mobile NTP. This CL adds a generic throttler of requests that - checks new requests against daily quota (configurable by Finch), - reports to UMA status of each request w.r.t. the quota, - reports to UMA the total count of request per day. As an example application, it adds throttler for NTPSnippetsFetcher. This throttler will be used in another CL. BUG=627073 ========== to ========== Introduce a request throttler for limiting requests in mobile NTP. This CL adds a generic throttler of requests that - checks new requests against daily quota (configurable by Finch), - reports to UMA status of each request w.r.t. the quota, - reports to UMA the total count of request per day. As an example application, it adds throttler for NTPSnippetsFetcher. This throttler will be used in another CL. BUG=627073 ==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== Introduce a request throttler for limiting requests in mobile NTP. This CL adds a generic throttler of requests that - checks new requests against daily quota (configurable by Finch), - reports to UMA status of each request w.r.t. the quota, - reports to UMA the total count of request per day. As an example application, it adds throttler for NTPSnippetsFetcher. This throttler will be used in another CL. BUG=627073 ========== to ========== Introduce a request throttler for limiting requests in mobile NTP. This CL adds a generic throttler of requests that - checks new requests against daily quota (configurable by Finch), - reports to UMA status of each request w.r.t. the quota, - reports to UMA the total count of request per day. As an example application, it adds throttler for NTPSnippetsFetcher. This throttler will be used in another CL. BUG=627073 Committed: https://crrev.com/505ea3a812f9a245c4e056e86c5dc59347d9440f Cr-Commit-Position: refs/heads/master@{#407124} ==========
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/505ea3a812f9a245c4e056e86c5dc59347d9440f Cr-Commit-Position: refs/heads/master@{#407124}
Message was sent while issue was closed.
bauerb@chromium.org changed reviewers: + bauerb@chromium.org
Message was sent while issue was closed.
Some post-commit drive-by nits: https://codereview.chromium.org/2158843002/diff/400001/components/ntp_snippet... File components/ntp_snippets/request_throttler.h (right): https://codereview.chromium.org/2158843002/diff/400001/components/ntp_snippet... components/ntp_snippets/request_throttler.h:21: struct RequestTypeInfo; This type could be made a private member of the class. https://codereview.chromium.org/2158843002/diff/400001/components/ntp_snippet... components/ntp_snippets/request_throttler.h:49: RequestType type, Alignment
Message was sent while issue was closed.
Thanks, Bernhard! https://codereview.chromium.org/2158843002/diff/400001/components/ntp_snippet... File components/ntp_snippets/request_throttler.h (right): https://codereview.chromium.org/2158843002/diff/400001/components/ntp_snippet... components/ntp_snippets/request_throttler.h:21: struct RequestTypeInfo; On 2016/07/22 13:58:24, Bernhard Bauer wrote: > This type could be made a private member of the class. I would keep it there as I did already move it back and forth. Out of curiosity: what is dividing line in your opinion between the situations where one of the following is more appropriate? - defining such a struct as a private member - defining it in the .cc file (potentially in an anonymous namespace) https://codereview.chromium.org/2158843002/diff/400001/components/ntp_snippet... components/ntp_snippets/request_throttler.h:49: RequestType type, On 2016/07/22 13:58:24, Bernhard Bauer wrote: > Alignment Done (in the follow-up CL).
Message was sent while issue was closed.
https://codereview.chromium.org/2158843002/diff/400001/components/ntp_snippet... File components/ntp_snippets/request_throttler.h (right): https://codereview.chromium.org/2158843002/diff/400001/components/ntp_snippet... components/ntp_snippets/request_throttler.h:21: struct RequestTypeInfo; On 2016/07/25 10:02:36, jkrcal wrote: > On 2016/07/22 13:58:24, Bernhard Bauer wrote: > > This type could be made a private member of the class. > > I would keep it there as I did already move it back and forth. > > Out of curiosity: what is dividing line in your opinion between the situations > where one of the following is more appropriate? > - defining such a struct as a private member > - defining it in the .cc file (potentially in an anonymous namespace) Generally, keep the scope as small as possible. That means, prefer the anonymous namespace, unless you need it in the header. If you do, make it a private member, but keep the definition in the .cc file if possible (that's the case here). So, you can just move this one line into the class.
Message was sent while issue was closed.
https://codereview.chromium.org/2158843002/diff/400001/components/ntp_snippet... File components/ntp_snippets/request_throttler.h (right): https://codereview.chromium.org/2158843002/diff/400001/components/ntp_snippet... components/ntp_snippets/request_throttler.h:21: struct RequestTypeInfo; On 2016/07/25 10:06:16, Marc Treib wrote: > On 2016/07/25 10:02:36, jkrcal wrote: > > On 2016/07/22 13:58:24, Bernhard Bauer wrote: > > > This type could be made a private member of the class. > > > > I would keep it there as I did already move it back and forth. > > > > Out of curiosity: what is dividing line in your opinion between the situations > > where one of the following is more appropriate? > > - defining such a struct as a private member > > - defining it in the .cc file (potentially in an anonymous namespace) > > Generally, keep the scope as small as possible. That means, prefer the anonymous > namespace, unless you need it in the header. If you do, make it a private > member, but keep the definition in the .cc file if possible (that's the case > here). So, you can just move this one line into the class. Thanks! If it is a private member, how should I define the kRequestTypeInfo constant list in the .cc file. Do I need to make kRequestTypeInfo a member of RequestThrottler? Is it desirable?
Message was sent while issue was closed.
https://codereview.chromium.org/2158843002/diff/400001/components/ntp_snippet... File components/ntp_snippets/request_throttler.h (right): https://codereview.chromium.org/2158843002/diff/400001/components/ntp_snippet... components/ntp_snippets/request_throttler.h:21: struct RequestTypeInfo; On 2016/07/25 10:14:45, jkrcal wrote: > On 2016/07/25 10:06:16, Marc Treib wrote: > > On 2016/07/25 10:02:36, jkrcal wrote: > > > On 2016/07/22 13:58:24, Bernhard Bauer wrote: > > > > This type could be made a private member of the class. > > > > > > I would keep it there as I did already move it back and forth. > > > > > > Out of curiosity: what is dividing line in your opinion between the > situations > > > where one of the following is more appropriate? > > > - defining such a struct as a private member > > > - defining it in the .cc file (potentially in an anonymous namespace) > > > > Generally, keep the scope as small as possible. That means, prefer the > anonymous > > namespace, unless you need it in the header. If you do, make it a private > > member, but keep the definition in the .cc file if possible (that's the case > > here). So, you can just move this one line into the class. > > Thanks! > If it is a private member, how should I define the kRequestTypeInfo constant > list in the .cc file. Do I need to make kRequestTypeInfo a member of > RequestThrottler? Is it desirable? Ah, right - that would need to become a static member then. That's kinda awkward. I'm not sure if the actual definition could stay in the .cc file then?
Message was sent while issue was closed.
https://codereview.chromium.org/2158843002/diff/400001/components/ntp_snippet... File components/ntp_snippets/request_throttler.h (right): https://codereview.chromium.org/2158843002/diff/400001/components/ntp_snippet... components/ntp_snippets/request_throttler.h:21: struct RequestTypeInfo; On 2016/07/25 10:22:15, Marc Treib wrote: > On 2016/07/25 10:14:45, jkrcal wrote: > > On 2016/07/25 10:06:16, Marc Treib wrote: > > > On 2016/07/25 10:02:36, jkrcal wrote: > > > > On 2016/07/22 13:58:24, Bernhard Bauer wrote: > > > > > This type could be made a private member of the class. > > > > > > > > I would keep it there as I did already move it back and forth. > > > > > > > > Out of curiosity: what is dividing line in your opinion between the > > situations > > > > where one of the following is more appropriate? > > > > - defining such a struct as a private member > > > > - defining it in the .cc file (potentially in an anonymous namespace) > > > > > > Generally, keep the scope as small as possible. That means, prefer the > > anonymous > > > namespace, unless you need it in the header. If you do, make it a private > > > member, but keep the definition in the .cc file if possible (that's the case > > > here). So, you can just move this one line into the class. > > > > Thanks! > > If it is a private member, how should I define the kRequestTypeInfo constant > > list in the .cc file. Do I need to make kRequestTypeInfo a member of > > RequestThrottler? Is it desirable? > > Ah, right - that would need to become a static member then. That's kinda > awkward. I'm not sure if the actual definition could stay in the .cc file then? Yeah, you would still declare it in the header as a private static member, then define it in the .cc file. (And I think that's still better than what this does right now, which is exporting the type declaration to files that include this header, without making the definition available). |
