Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2227973002:
Add request throttler to thumbnail fetching for articles on mobile NTP (Closed)
DescriptionAdd request throttler to thumbnail fetching for articles on mobile NTP
As a safety mechanism, this CL introduces throttling mechanism for
thumbnail fetching for articles for you on mobile NTP. Per default, the
quota is unlimited but can be easily decreased by finch.
Since all thumbnail requests are user initiated (by scrolling below
the fold), the CL also extends the throttler to support separate quota
for interactive requests (previously called "forced").
Lastly, it fixes a bug that the callback passed in in FetchSuggestionImage() got called with a wrong id (with the ID of the snippet "[url]" and not with the ID of the suggestion "[category_id]|[url]").
BUG=627073
Committed: https://crrev.com/ba735f09ac398664ea81fda2d948bef853469a4b
Cr-Commit-Position: refs/heads/master@{#412555}
Patch Set 1 #
Total comments: 23
Patch Set 2 : Rebase #Patch Set 3 : Marc's comments #
Total comments: 14
Patch Set 4 : Marc's comments #2 #
Total comments: 2
Patch Set 5 : Marc's comments #3 #
Total comments: 6
Patch Set 6 : Gayane's comments #Patch Set 7 : Fixing the compile error #
Total comments: 4
Patch Set 8 : Rebase #Patch Set 9 : Robert's and Tim's comments #
Total comments: 10
Patch Set 10 : Tim's comments #2 #
Total comments: 8
Patch Set 11 : Tim's comments #3 #Patch Set 12 : One forgotten comment #
Total comments: 10
Patch Set 13 : Tims's comments #4 #Patch Set 14 : Build issue #Messages
Total messages: 56 (31 generated)
jkrcal@chromium.org changed reviewers: + treib@chromium.org, tschumann@chromium.org
Marc, PTAL! (Tim, FYI, as the CL is based on our discussions.)
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...
https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:645: base::Bind(callback, it != snippets_.end() I don't understand the snippet_id-building here - why do we make a unique ID only in one case? https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/pre... File components/ntp_snippets/pref_names.h (right): https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/pre... components/ntp_snippets/pref_names.h:18: extern const char kSnippetFetcherInteractiveQuotaCount[]; I still think we should remove "Quota" from these names; I get confused every time I read this. How about kSnippetFetcherRequestCount, kSnippetFetcherInteractiveRequestCount etc? https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/req... File components/ntp_snippets/request_throttler.cc (right): https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/req... components/ntp_snippets/request_throttler.cc:28: FORCED, Note that you can rename the enum entry, just not reorder or remove. The important thing is that the actual int value stays constant. https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/req... components/ntp_snippets/request_throttler.cc:113: if (count < INT_MAX) Do we ever expect to actually hit INT_MAX?! That would correspond to about 46 requests per millisecond. https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/req... components/ntp_snippets/request_throttler.cc:118: if (interactive_request) { if (interactive_request && available) ? Or maybe we should just add a new histogram bin? Currently this is a bit inconsistent... https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/req... components/ntp_snippets/request_throttler.cc:137: // previous day. Hm. Should we also report the number of interactive requests? https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/req... components/ntp_snippets/request_throttler.cc:157: : pref_service_->GetInteger(type_info_.count_pref); You could move the ? into the GetInteger() https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/req... File components/ntp_snippets/request_throttler.h (right): https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/req... components/ntp_snippets/request_throttler.h:8: #include <limits.h> Move to the .cc https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/req... components/ntp_snippets/request_throttler.h:83: // The quotas come from variation parameters or default quotas as a fallback. I'd formulate this the other way around: The quotas are hardcoded, but can be overridden by variation params.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
jkrcal@chromium.org changed reviewers: + rkaplow@chromium.org
Thanks, Marc. PTAL, again. Robert, PTAL at histograms.xml. https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:645: base::Bind(callback, it != snippets_.end() On 2016/08/09 14:04:53, Marc Treib wrote: > I don't understand the snippet_id-building here - why do we make a unique ID > only in one case? Huh, I just kept the code equivalent :) This was a bug, actually several bugs. We need to pass snippet ID in the Image Fetcher because this is the key that we use to store the raw data in the DB etc. Eventually, the callback should be called with the suggestion ID - I added a OnSnippetImageDecodedFromNetwork wrapper function (and changed the order of parameters for all On* functions to have them consistent). https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/pre... File components/ntp_snippets/pref_names.h (right): https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/pre... components/ntp_snippets/pref_names.h:18: extern const char kSnippetFetcherInteractiveQuotaCount[]; On 2016/08/09 14:04:53, Marc Treib wrote: > I still think we should remove "Quota" from these names; I get confused every > time I read this. How about kSnippetFetcherRequestCount, > kSnippetFetcherInteractiveRequestCount etc? Done. https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/req... File components/ntp_snippets/request_throttler.cc (right): https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/req... components/ntp_snippets/request_throttler.cc:28: FORCED, On 2016/08/09 14:04:53, Marc Treib wrote: > Note that you can rename the enum entry, just not reorder or remove. The > important thing is that the actual int value stays constant. Done. https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/req... components/ntp_snippets/request_throttler.cc:113: if (count < INT_MAX) On 2016/08/09 14:04:53, Marc Treib wrote: > Do we ever expect to actually hit INT_MAX?! That would correspond to about 46 > requests per millisecond. Okay, good point. https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/req... components/ntp_snippets/request_throttler.cc:118: if (interactive_request) { On 2016/08/09 14:04:53, Marc Treib wrote: > if (interactive_request && available) ? > Or maybe we should just add a new histogram bin? Currently this is a bit > inconsistent... Done. I tried to avoid touching the histograms but it is better to do it :) https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/req... components/ntp_snippets/request_throttler.cc:137: // previous day. On 2016/08/09 14:04:53, Marc Treib wrote: > Hm. Should we also report the number of interactive requests? Done. https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/req... components/ntp_snippets/request_throttler.cc:157: : pref_service_->GetInteger(type_info_.count_pref); On 2016/08/09 14:04:53, Marc Treib wrote: > You could move the ? into the GetInteger() Done. https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/req... File components/ntp_snippets/request_throttler.h (right): https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/req... components/ntp_snippets/request_throttler.h:8: #include <limits.h> On 2016/08/09 14:04:53, Marc Treib wrote: > Move to the .cc Done. https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/req... components/ntp_snippets/request_throttler.h:83: // The quotas come from variation parameters or default quotas as a fallback. On 2016/08/09 14:04:53, Marc Treib wrote: > I'd formulate this the other way around: The quotas are hardcoded, but can be > overridden by variation params. Done.
https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:645: base::Bind(callback, it != snippets_.end() On 2016/08/10 10:27:26, jkrcal wrote: > On 2016/08/09 14:04:53, Marc Treib wrote: > > I don't understand the snippet_id-building here - why do we make a unique ID > > only in one case? > > Huh, I just kept the code equivalent :) This was a bug, actually several bugs. > > We need to pass snippet ID in the Image Fetcher because this is the key that we > use to store the raw data in the DB etc. Eventually, the callback should be > called with the suggestion ID - I added a OnSnippetImageDecodedFromNetwork > wrapper function (and changed the order of parameters for all On* functions to > have them consistent). Interesting! How did the fetched images ever arrive at the UI before, if we called it back with the wrong ID?! Might be worth mentioning this fix in the issue description :) https://codereview.chromium.org/2227973002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2227973002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:664: void NTPSnippetsService::OnSnippetImageDecodedFromNetwork( nit: move this below FetchSnippetImageFromNetwork, so the order of methods roughly matches the actual order of events? https://codereview.chromium.org/2227973002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:684: base::ThreadTaskRunnerHandle::Get()->PostTask( I think we can directly call the method here, since async things have already happened before we got here. If you do keep the PostTask: I don't think Unretained is safe here. https://codereview.chromium.org/2227973002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:699: base::Unretained(this), callback)); misaligned https://codereview.chromium.org/2227973002/diff/40001/components/ntp_snippets... File components/ntp_snippets/pref_names.h (right): https://codereview.chromium.org/2227973002/diff/40001/components/ntp_snippets... components/ntp_snippets/pref_names.h:23: // The pref name for today's count of NTPSnippetsFetcher requests, so far. Update comment https://codereview.chromium.org/2227973002/diff/40001/components/ntp_snippets... components/ntp_snippets/pref_names.h:25: // The pref name for today's count of NTPSnippetsFetcher requests, so far. Here too https://codereview.chromium.org/2227973002/diff/40001/components/ntp_snippets... components/ntp_snippets/pref_names.h:27: // The pref name for the current day for the counter of NTPSnippetsFetcher And here
Description was changed from ========== Add request throttler to thumbnail fetching for articles on mobile NTP As a safety mechanism, this CL introduces throttling mechanism for thumbnail fetching for articles for you on mobile NTP. Per default, the quota is unlimited but can be easily decreased by finch. Since all thumbnail requests are user initiated (by scrolling below the fold), the CL also extends the throttler to support separate quota for interactive requests (previously called "forced"). BUG=627073 ========== to ========== Add request throttler to thumbnail fetching for articles on mobile NTP As a safety mechanism, this CL introduces throttling mechanism for thumbnail fetching for articles for you on mobile NTP. Per default, the quota is unlimited but can be easily decreased by finch. Since all thumbnail requests are user initiated (by scrolling below the fold), the CL also extends the throttler to support separate quota for interactive requests (previously called "forced"). Lastly, it fixes a bug that the callback passed in in FetchSuggestionImage() got called with a wrong id (with the ID of the snippet "[url]" and not with the ID of the suggestion "[category_id]|[url]"). BUG=627073 ==========
Thanks! PTAL, again. https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:645: base::Bind(callback, it != snippets_.end() On 2016/08/10 11:38:14, Marc Treib wrote: > On 2016/08/10 10:27:26, jkrcal wrote: > > On 2016/08/09 14:04:53, Marc Treib wrote: > > > I don't understand the snippet_id-building here - why do we make a unique ID > > > only in one case? > > > > Huh, I just kept the code equivalent :) This was a bug, actually several bugs. > > > > > We need to pass snippet ID in the Image Fetcher because this is the key that > we > > use to store the raw data in the DB etc. Eventually, the callback should be > > called with the suggestion ID - I added a OnSnippetImageDecodedFromNetwork > > wrapper function (and changed the order of parameters for all On* functions to > > have them consistent). > > Interesting! > How did the fetched images ever arrive at the UI before, if we called it back > with the wrong ID?! > > Might be worth mentioning this fix in the issue description :) I've updated the issue. Interestingly, the UI does not care about the id, this is thrown away in the bridge (did not check how it then works). https://codereview.chromium.org/2227973002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2227973002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:664: void NTPSnippetsService::OnSnippetImageDecodedFromNetwork( On 2016/08/10 11:38:15, Marc Treib wrote: > nit: move this below FetchSnippetImageFromNetwork, so the order of methods > roughly matches the actual order of events? Done. https://codereview.chromium.org/2227973002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:684: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/08/10 11:38:15, Marc Treib wrote: > I think we can directly call the method here, since async things have already > happened before we got here. > If you do keep the PostTask: I don't think Unretained is safe here. Done. Why is it unsafe here when it is safe anywhere else in the code? Is it safe 10 lines below, in particular? https://codereview.chromium.org/2227973002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:699: base::Unretained(this), callback)); On 2016/08/10 11:38:15, Marc Treib wrote: > misaligned Done. https://codereview.chromium.org/2227973002/diff/40001/components/ntp_snippets... File components/ntp_snippets/pref_names.h (right): https://codereview.chromium.org/2227973002/diff/40001/components/ntp_snippets... components/ntp_snippets/pref_names.h:23: // The pref name for today's count of NTPSnippetsFetcher requests, so far. On 2016/08/10 11:38:15, Marc Treib wrote: > Update comment Done. https://codereview.chromium.org/2227973002/diff/40001/components/ntp_snippets... components/ntp_snippets/pref_names.h:25: // The pref name for today's count of NTPSnippetsFetcher requests, so far. On 2016/08/10 11:38:15, Marc Treib wrote: > Here too Done. https://codereview.chromium.org/2227973002/diff/40001/components/ntp_snippets... components/ntp_snippets/pref_names.h:27: // The pref name for the current day for the counter of NTPSnippetsFetcher On 2016/08/10 11:38:15, Marc Treib wrote: > And here Done.
lgtm https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:645: base::Bind(callback, it != snippets_.end() On 2016/08/10 11:57:03, jkrcal wrote: > On 2016/08/10 11:38:14, Marc Treib wrote: > > On 2016/08/10 10:27:26, jkrcal wrote: > > > On 2016/08/09 14:04:53, Marc Treib wrote: > > > > I don't understand the snippet_id-building here - why do we make a unique > ID > > > > only in one case? > > > > > > Huh, I just kept the code equivalent :) This was a bug, actually several > bugs. > > > > > > > > We need to pass snippet ID in the Image Fetcher because this is the key that > > we > > > use to store the raw data in the DB etc. Eventually, the callback should be > > > called with the suggestion ID - I added a OnSnippetImageDecodedFromNetwork > > > wrapper function (and changed the order of parameters for all On* functions > to > > > have them consistent). > > > > Interesting! > > How did the fetched images ever arrive at the UI before, if we called it back > > with the wrong ID?! > > > > Might be worth mentioning this fix in the issue description :) > > I've updated the issue. Interestingly, the UI does not care about the id, this > is thrown away in the bridge (did not check how it then works). Ah, it binds a callback directly to the UI element that wants the image, so it doesn't need the ID to assign it. https://codereview.chromium.org/2227973002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2227973002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:684: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/08/10 11:57:03, jkrcal wrote: > On 2016/08/10 11:38:15, Marc Treib wrote: > > I think we can directly call the method here, since async things have already > > happened before we got here. > > If you do keep the PostTask: I don't think Unretained is safe here. > > Done. > > Why is it unsafe here when it is safe anywhere else in the code? Is it safe 10 > lines below, in particular? It should be safe below because it's the image_fetcher_ that would call the callback - if we get destroyed, we'll also destroy image_fetcher_, so it won't call the callback anymore. Here, we're just posting an "unowned" task, which could get executed even if we get destroyed in the meantime. (Which is unlikely, but still) https://codereview.chromium.org/2227973002/diff/60001/components/ntp_snippets... File components/ntp_snippets/pref_names.h (right): https://codereview.chromium.org/2227973002/diff/60001/components/ntp_snippets... components/ntp_snippets/pref_names.h:25: // The pref name for today's count of requests for article thumbnails, so far. nit: *interactive* requests
Thanks, Marc! Robert, PTAL at histograms.xml (and happy birthday, by the way!) https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:645: base::Bind(callback, it != snippets_.end() On 2016/08/10 12:04:59, Marc Treib wrote: > On 2016/08/10 11:57:03, jkrcal wrote: > > On 2016/08/10 11:38:14, Marc Treib wrote: > > > On 2016/08/10 10:27:26, jkrcal wrote: > > > > On 2016/08/09 14:04:53, Marc Treib wrote: > > > > > I don't understand the snippet_id-building here - why do we make a > unique > > ID > > > > > only in one case? > > > > > > > > Huh, I just kept the code equivalent :) This was a bug, actually several > > bugs. > > > > > > > > > > > We need to pass snippet ID in the Image Fetcher because this is the key > that > > > we > > > > use to store the raw data in the DB etc. Eventually, the callback should > be > > > > called with the suggestion ID - I added a OnSnippetImageDecodedFromNetwork > > > > wrapper function (and changed the order of parameters for all On* > functions > > to > > > > have them consistent). > > > > > > Interesting! > > > How did the fetched images ever arrive at the UI before, if we called it > back > > > with the wrong ID?! > > > > > > Might be worth mentioning this fix in the issue description :) > > > > I've updated the issue. Interestingly, the UI does not care about the id, this > > is thrown away in the bridge (did not check how it then works). > > Ah, it binds a callback directly to the UI element that wants the image, so it > doesn't need the ID to assign it. Acknowledged. https://codereview.chromium.org/2227973002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2227973002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:684: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/08/10 12:04:59, Marc Treib wrote: > On 2016/08/10 11:57:03, jkrcal wrote: > > On 2016/08/10 11:38:15, Marc Treib wrote: > > > I think we can directly call the method here, since async things have > already > > > happened before we got here. > > > If you do keep the PostTask: I don't think Unretained is safe here. > > > > Done. > > > > Why is it unsafe here when it is safe anywhere else in the code? Is it safe 10 > > lines below, in particular? > > It should be safe below because it's the image_fetcher_ that would call the > callback - if we get destroyed, we'll also destroy image_fetcher_, so it won't > call the callback anymore. > Here, we're just posting an "unowned" task, which could get executed even if we > get destroyed in the meantime. (Which is unlikely, but still) Thanks, makes sense! https://codereview.chromium.org/2227973002/diff/60001/components/ntp_snippets... File components/ntp_snippets/pref_names.h (right): https://codereview.chromium.org/2227973002/diff/60001/components/ntp_snippets... components/ntp_snippets/pref_names.h:25: // The pref name for today's count of requests for article thumbnails, so far. On 2016/08/10 12:05:00, Marc Treib wrote: > nit: *interactive* requests Done.
rkaplow@chromium.org changed reviewers: + gayane@chromium.org
https://codereview.chromium.org/2227973002/diff/80001/components/ntp_snippets... File components/ntp_snippets/request_throttler.cc (right): https://codereview.chromium.org/2227973002/diff/80001/components/ntp_snippets... components/ntp_snippets/request_throttler.cc:50: // When adding a new type here, extend also the "RequestCounterTypes" Cannot find "RequestCounterTypes" in histograms.xml. Seems to be renamed to "RequestThrottlerTypes". Please update the comment. https://codereview.chromium.org/2227973002/diff/80001/components/ntp_snippets... components/ntp_snippets/request_throttler.cc:57: {"SuggestionThumbnailFetcher", prefs::kSnippetThumbnailsRequestCount, should be added to <histogram_suffixes> ? https://codereview.chromium.org/2227973002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2227973002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:86194: + <int value="0" label="Interactive request - granted"/> nit: maybe "Interactive request - quota granted" to correspond to RequestStatus enum
Thanks, Gayane! PTAL, again. https://codereview.chromium.org/2227973002/diff/80001/components/ntp_snippets... File components/ntp_snippets/request_throttler.cc (right): https://codereview.chromium.org/2227973002/diff/80001/components/ntp_snippets... components/ntp_snippets/request_throttler.cc:50: // When adding a new type here, extend also the "RequestCounterTypes" On 2016/08/10 18:47:54, gayane wrote: > Cannot find "RequestCounterTypes" in histograms.xml. Seems to be renamed to > "RequestThrottlerTypes". Please update the comment. Done. https://codereview.chromium.org/2227973002/diff/80001/components/ntp_snippets... components/ntp_snippets/request_throttler.cc:57: {"SuggestionThumbnailFetcher", prefs::kSnippetThumbnailsRequestCount, On 2016/08/10 18:47:54, gayane wrote: > should be added to <histogram_suffixes> ? Oh, thanks! Done. https://codereview.chromium.org/2227973002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2227973002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:86194: + <int value="0" label="Interactive request - granted"/> On 2016/08/10 18:47:54, gayane wrote: > nit: maybe "Interactive request - quota granted" to correspond to RequestStatus > enum 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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
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...
https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2227973002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:645: base::Bind(callback, it != snippets_.end() On 2016/08/10 16:22:27, jkrcal wrote: > On 2016/08/10 12:04:59, Marc Treib wrote: > > On 2016/08/10 11:57:03, jkrcal wrote: > > > On 2016/08/10 11:38:14, Marc Treib wrote: > > > > On 2016/08/10 10:27:26, jkrcal wrote: > > > > > On 2016/08/09 14:04:53, Marc Treib wrote: > > > > > > I don't understand the snippet_id-building here - why do we make a > > unique > > > ID > > > > > > only in one case? > > > > > > > > > > Huh, I just kept the code equivalent :) This was a bug, actually several > > > bugs. > > > > > > > > > > > > > > We need to pass snippet ID in the Image Fetcher because this is the key > > that > > > > we > > > > > use to store the raw data in the DB etc. Eventually, the callback should > > be > > > > > called with the suggestion ID - I added a > OnSnippetImageDecodedFromNetwork > > > > > wrapper function (and changed the order of parameters for all On* > > functions > > > to > > > > > have them consistent). > > > > > > > > Interesting! > > > > How did the fetched images ever arrive at the UI before, if we called it > > back > > > > with the wrong ID?! > > > > > > > > Might be worth mentioning this fix in the issue description :) > > > > > > I've updated the issue. Interestingly, the UI does not care about the id, > this > > > is thrown away in the bridge (did not check how it then works). > > > > Ah, it binds a callback directly to the UI element that wants the image, so it > > doesn't need the ID to assign it. > > Acknowledged. Given that was quite a bug, is there a chance to put a test in place which fails before this fix and passes afterwards?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM for histograms. Still need approval from rkaplow.
https://codereview.chromium.org/2227973002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2227973002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:86290: <enum name="NtpRequestThrottlerStatus" type="int"> hm, if you're changing the meaning of the histogram enum, you should replace the histogram with a new name (ex. suffix with 2). Otherwise the data will get mixed up https://codereview.chromium.org/2227973002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2227973002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:35372: +<histogram name="NewTabPage.RequestThrottler.PerDayInteractive" where is this being recorded? I don't see "PerDayInteractive"
Thanks, Robert! PTAL, again. Tim, I added the unittest. https://codereview.chromium.org/2227973002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2227973002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:86290: <enum name="NtpRequestThrottlerStatus" type="int"> On 2016/08/11 17:13:15, rkaplow wrote: > hm, if you're changing the meaning of the histogram enum, you should replace the > histogram with a new name (ex. suffix with 2). Otherwise the data will get mixed > up It is not very obvious but the meaning of the buckets stays the same: - terminology changed from "forced" to "interactive", - the meaning of the first bucket always was "granted", and - the other two buckets always were meant for "background requests". https://codereview.chromium.org/2227973002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2227973002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:35372: +<histogram name="NewTabPage.RequestThrottler.PerDayInteractive" On 2016/08/11 17:13:15, rkaplow wrote: > where is this being recorded? I don't see "PerDayInteractive" Oh, I was not very focused, yesterday :| Thanks for pointing out! Done.
https://codereview.chromium.org/2227973002/diff/160001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2227973002/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:275: void SetImageFetcherDelegate(ImageFetcherDelegate* delegate) override {} let's not mix up Mocks with fake implementations. If you go with the mock, you can make all methods MOCK_METHODs and then use a ::testing::NiceMock<MockImageFetcher>(). The nice mock (unlike the ::testing::StrictMock<>) allows unexpected calls and just defaults to doing nothing. https://codereview.chromium.org/2227973002/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:375: MOCK_METHOD2(OnImageFetched, void(const std::string&, const gfx::Image&)); while it might sound tempting, this is not how you should use MOCK_METHODs. MOCK_METHODs are for mock classes only. If you need a mock function, there are MockFunction for you ;-). (see https://codesearch.chromium.org/chromium/src/testing/gmock/test/gmock-actions... for an example). https://codereview.chromium.org/2227973002/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:396: static void ReturnEmptyImage( no need to define this inside the text fixture class. just make it a plain function in an unnamed namespace). nit on naming: this is not strictly 'return;'-ing. Rename to ServeEmptyImage() or the like? https://codereview.chromium.org/2227973002/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:951: EXPECT_TRUE(image.IsEmpty()); hmm... do we have some test image? i guess the default constructed image would already count as empty, no?
Thanks, Tim! PTAL, again. https://codereview.chromium.org/2227973002/diff/160001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2227973002/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:275: void SetImageFetcherDelegate(ImageFetcherDelegate* delegate) override {} On 2016/08/12 09:55:36, tschumann wrote: > let's not mix up Mocks with fake implementations. > > If you go with the mock, you can make all methods MOCK_METHODs and then use a > ::testing::NiceMock<MockImageFetcher>(). The nice mock (unlike the > ::testing::StrictMock<>) allows unexpected calls and just defaults to doing > nothing. Done. https://codereview.chromium.org/2227973002/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:375: MOCK_METHOD2(OnImageFetched, void(const std::string&, const gfx::Image&)); On 2016/08/12 09:55:36, tschumann wrote: > while it might sound tempting, this is not how you should use MOCK_METHODs. > MOCK_METHODs are for mock classes only. If you need a mock function, there are > MockFunction for you ;-). > (see > https://codesearch.chromium.org/chromium/src/testing/gmock/test/gmock-actions... > for an example). Whoo, that's fancy! Thanks! https://codereview.chromium.org/2227973002/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:396: static void ReturnEmptyImage( On 2016/08/12 09:55:36, tschumann wrote: > no need to define this inside the text fixture class. just make it a plain > function in an unnamed namespace). > > nit on naming: this is not strictly 'return;'-ing. Rename to ServeEmptyImage() > or the like? Done. https://codereview.chromium.org/2227973002/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:951: EXPECT_TRUE(image.IsEmpty()); On 2016/08/12 09:55:36, tschumann wrote: > hmm... do we have some test image? i guess the default constructed image would > already count as empty, no? Well, this is a bit pointless, I agree. This is to document what should happen. I was not sure how to test the non-trivial case in the other test above. Even if I had a test image, gfx::Image does not support "==" operator, so the only thing I can anyway do is to check that the thing I serve and the thing I become have the same address. Does it make sense?
https://codereview.chromium.org/2227973002/diff/160001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2227973002/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:275: void SetImageFetcherDelegate(ImageFetcherDelegate* delegate) override {} On 2016/08/12 12:06:10, jkrcal wrote: > On 2016/08/12 09:55:36, tschumann wrote: > > let's not mix up Mocks with fake implementations. > > > > If you go with the mock, you can make all methods MOCK_METHODs and then use a > > ::testing::NiceMock<MockImageFetcher>(). The nice mock (unlike the > > ::testing::StrictMock<>) allows unexpected calls and just defaults to doing > > nothing. > > Done. One alternative to make things more explicit: Use a StrictMock, but add ON_CALL(...).WillBeDefault statements for uninteresting calls. (It's fine to leave this as-is, just something to consider in general)
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...
almost =) https://codereview.chromium.org/2227973002/diff/160001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2227973002/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:951: EXPECT_TRUE(image.IsEmpty()); On 2016/08/12 12:06:09, jkrcal wrote: > On 2016/08/12 09:55:36, tschumann wrote: > > hmm... do we have some test image? i guess the default constructed image would > > already count as empty, no? > > Well, this is a bit pointless, I agree. This is to document what should happen. > I was not sure how to test the non-trivial case in the other test above. > > Even if I had a test image, gfx::Image does not support "==" operator, so the > only thing I can anyway do is to check that the thing I serve and the thing I > become have the same address. Does it make sense? src/ui/gfx/image/image_unittest_util.h has some helpers that allow you to create simple images (e.g. just with some dimension). In this test you could initialize it with something like that. In the previous test, you could change ServeEmptyImage() to serve a provided image and then compare the dimensions. https://codereview.chromium.org/2227973002/diff/180001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2227973002/diff/180001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:353: auto image_fetcher = nit: no need to allocate the mock on the heap. Simply put this in your member fields: testing::NiceMock<MockImageFetcher> image_fetcher_; https://codereview.chromium.org/2227973002/diff/180001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:927: .Times(1) Times(1) is implicit, you can remove that line. Same in the other calls below. https://codereview.chromium.org/2227973002/diff/180001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:930: on_image_fetched_mock_function; nit: feel free to pick shorter names (the test is short, no need to include the type in the name), e.g. 'image_fetched'.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, PTAL again. https://codereview.chromium.org/2227973002/diff/180001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2227973002/diff/180001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:353: auto image_fetcher = On 2016/08/12 12:37:09, tschumann wrote: > nit: no need to allocate the mock on the heap. > Simply put this in your member fields: > testing::NiceMock<MockImageFetcher> image_fetcher_; I am a bit puzzled by this comment. I need to wrap it in a unique_ptr and pass it to the constructor of the NTPSnippetsService. Can I even wrap a member field into a unique_ptr and pass it by std::move? If I can, is it not against the spirit of unique_ptrs? https://codereview.chromium.org/2227973002/diff/180001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:927: .Times(1) On 2016/08/12 12:37:09, tschumann wrote: > Times(1) is implicit, you can remove that line. > > Same in the other calls below. Done. https://codereview.chromium.org/2227973002/diff/180001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:930: on_image_fetched_mock_function; On 2016/08/12 12:37:09, tschumann wrote: > nit: feel free to pick shorter names (the test is short, no need to include the > type in the name), e.g. 'image_fetched'. Done.
https://codereview.chromium.org/2227973002/diff/180001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2227973002/diff/180001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:353: auto image_fetcher = On 2016/08/16 12:31:43, jkrcal wrote: > On 2016/08/12 12:37:09, tschumann wrote: > > nit: no need to allocate the mock on the heap. > > Simply put this in your member fields: > > testing::NiceMock<MockImageFetcher> image_fetcher_; > > I am a bit puzzled by this comment. > > I need to wrap it in a unique_ptr and pass it to the constructor of the > NTPSnippetsService. Can I even wrap a member field into a unique_ptr and pass it > by std::move? If I can, is it not against the spirit of unique_ptrs? I think Tim just didn't notice that you have to pass a unique_ptr :) Passing a unique_ptr to something on the stack certainly isn't legal. (It would get deleted twice) Keeping a raw pointer around while passing ownership to the service also isn't particularly clean, but that's a fairly common practice in tests and IMO acceptable. Don't do that in real code though :)
lgtm most of my comments can be addressed in a follow-up CL (if you're too busy, vitaliii@ might have some cycles). Given the time zone differences I really don't want to block this and refactorings should be done in separate CLs anyways =). https://codereview.chromium.org/2227973002/diff/180001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2227973002/diff/180001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:353: auto image_fetcher = On 2016/08/16 13:29:18, Marc Treib wrote: > On 2016/08/16 12:31:43, jkrcal wrote: > > On 2016/08/12 12:37:09, tschumann wrote: > > > nit: no need to allocate the mock on the heap. > > > Simply put this in your member fields: > > > testing::NiceMock<MockImageFetcher> image_fetcher_; > > > > I am a bit puzzled by this comment. > > > > I need to wrap it in a unique_ptr and pass it to the constructor of the > > NTPSnippetsService. Can I even wrap a member field into a unique_ptr and pass > it > > by std::move? If I can, is it not against the spirit of unique_ptrs? > > I think Tim just didn't notice that you have to pass a unique_ptr :) > Passing a unique_ptr to something on the stack certainly isn't legal. (It would > get deleted twice) > > Keeping a raw pointer around while passing ownership to the service also isn't > particularly clean, but that's a fairly common practice in tests and IMO > acceptable. Don't do that in real code though :) yes to both. Actually I realized this later and *thought* I removed that comment... apparently not ;-) Sorry for the confusion. https://codereview.chromium.org/2227973002/diff/220001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2227973002/diff/220001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:679: // happened in the meantime. i guess we want to record such events somewhere, hm? https://codereview.chromium.org/2227973002/diff/220001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:686: // The image fetcher calls OnImageDataFetched() with the raw data (this object probably outside of this CL but I find comments like these a bit confusing. We probably should rename OnImageDataFetched() to CacheImageData(). This would document that this is actually independent from the individual fetch-flow. For the code here, it really doesn't matter that image caching is taking place in-between; hence I'd be in favor of just dropping the comment. That said, this class grows really, really large. The fact that NTPSnippetService also implements ImageFetcherDelegate adds unnecssary complexity (and after all the Service is conceptually not an ImagerFetcherDeletage ;-)). Instead, the cleaner solution would be to define a CachedImageFetcher class that handles the caching aspects and looks like an image fetcher to the NTPSnippetService. As said, not in this CL, but it's definitely a smell. https://codereview.chromium.org/2227973002/diff/220001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2227973002/diff/220001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:946: gfx::Image image = gfx::test::CreateImage(1, 1); please add a comment explaining that we set up the picture like that to verify we get an empty image set in error cases. https://codereview.chromium.org/2227973002/diff/220001/components/ntp_snippet... File components/ntp_snippets/request_throttler.cc (right): https://codereview.chromium.org/2227973002/diff/220001/components/ntp_snippet... components/ntp_snippets/request_throttler.cc:154: const char* RequestThrottler::GetRequestTypeAsString() const { actually this method name is misleading -- you don't return a string. GetRequestTypeName()? https://codereview.chromium.org/2227973002/diff/220001/components/ntp_snippet... components/ntp_snippets/request_throttler.cc:158: int RequestThrottler::GetQuota(bool interactive_request) const { It seems like we should turn RequestTypeInfo into a proper class, move those methods onto the class and hide the members. Can happen in a follow-up CL but a TODO() would be nice.
lgtm
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: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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 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, gayane@chromium.org, rkaplow@chromium.org, tschumann@chromium.org Link to the patchset: https://codereview.chromium.org/2227973002/#ps260001 (title: "Build issue")
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 ========== Add request throttler to thumbnail fetching for articles on mobile NTP As a safety mechanism, this CL introduces throttling mechanism for thumbnail fetching for articles for you on mobile NTP. Per default, the quota is unlimited but can be easily decreased by finch. Since all thumbnail requests are user initiated (by scrolling below the fold), the CL also extends the throttler to support separate quota for interactive requests (previously called "forced"). Lastly, it fixes a bug that the callback passed in in FetchSuggestionImage() got called with a wrong id (with the ID of the snippet "[url]" and not with the ID of the suggestion "[category_id]|[url]"). BUG=627073 ========== to ========== Add request throttler to thumbnail fetching for articles on mobile NTP As a safety mechanism, this CL introduces throttling mechanism for thumbnail fetching for articles for you on mobile NTP. Per default, the quota is unlimited but can be easily decreased by finch. Since all thumbnail requests are user initiated (by scrolling below the fold), the CL also extends the throttler to support separate quota for interactive requests (previously called "forced"). Lastly, it fixes a bug that the callback passed in in FetchSuggestionImage() got called with a wrong id (with the ID of the snippet "[url]" and not with the ID of the suggestion "[category_id]|[url]"). BUG=627073 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Add request throttler to thumbnail fetching for articles on mobile NTP As a safety mechanism, this CL introduces throttling mechanism for thumbnail fetching for articles for you on mobile NTP. Per default, the quota is unlimited but can be easily decreased by finch. Since all thumbnail requests are user initiated (by scrolling below the fold), the CL also extends the throttler to support separate quota for interactive requests (previously called "forced"). Lastly, it fixes a bug that the callback passed in in FetchSuggestionImage() got called with a wrong id (with the ID of the snippet "[url]" and not with the ID of the suggestion "[category_id]|[url]"). BUG=627073 ========== to ========== Add request throttler to thumbnail fetching for articles on mobile NTP As a safety mechanism, this CL introduces throttling mechanism for thumbnail fetching for articles for you on mobile NTP. Per default, the quota is unlimited but can be easily decreased by finch. Since all thumbnail requests are user initiated (by scrolling below the fold), the CL also extends the throttler to support separate quota for interactive requests (previously called "forced"). Lastly, it fixes a bug that the callback passed in in FetchSuggestionImage() got called with a wrong id (with the ID of the snippet "[url]" and not with the ID of the suggestion "[category_id]|[url]"). BUG=627073 Committed: https://crrev.com/ba735f09ac398664ea81fda2d948bef853469a4b Cr-Commit-Position: refs/heads/master@{#412555} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/ba735f09ac398664ea81fda2d948bef853469a4b Cr-Commit-Position: refs/heads/master@{#412555}
Message was sent while issue was closed.
Tim, just for completeness: I've forgotten to send out drafted comments in a long-closed CL. https://codereview.chromium.org/2227973002/diff/220001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2227973002/diff/220001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:679: // happened in the meantime. On 2016/08/16 14:56:55, tschumann wrote: > i guess we want to record such events somewhere, hm? This event is recorded in the request throttler. Do we need more? https://codereview.chromium.org/2227973002/diff/220001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:686: // The image fetcher calls OnImageDataFetched() with the raw data (this object On 2016/08/16 14:56:55, tschumann wrote: > probably outside of this CL but I find comments like these a bit confusing. > We probably should rename OnImageDataFetched() to CacheImageData(). > This would document that this is actually independent from the individual > fetch-flow. > For the code here, it really doesn't matter that image caching is taking place > in-between; hence I'd be in favor of just dropping the comment. > > That said, this class grows really, really large. The fact that > NTPSnippetService also implements ImageFetcherDelegate adds unnecssary > complexity (and after all the Service is conceptually not an > ImagerFetcherDeletage ;-)). > Instead, the cleaner solution would be to define a CachedImageFetcher class > that handles the caching aspects and looks like an image fetcher to the > NTPSnippetService. > > As said, not in this CL, but it's definitely a smell. Ok, let's leave it up to a follow-up CL. https://codereview.chromium.org/2227973002/diff/220001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2227973002/diff/220001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:946: gfx::Image image = gfx::test::CreateImage(1, 1); On 2016/08/16 14:56:55, tschumann wrote: > please add a comment explaining that we set up the picture like that to verify > we get an empty image set in error cases. Done. https://codereview.chromium.org/2227973002/diff/220001/components/ntp_snippet... File components/ntp_snippets/request_throttler.cc (right): https://codereview.chromium.org/2227973002/diff/220001/components/ntp_snippet... components/ntp_snippets/request_throttler.cc:154: const char* RequestThrottler::GetRequestTypeAsString() const { On 2016/08/16 14:56:55, tschumann wrote: > actually this method name is misleading -- you don't return a string. > GetRequestTypeName()? Done. https://codereview.chromium.org/2227973002/diff/220001/components/ntp_snippet... components/ntp_snippets/request_throttler.cc:158: int RequestThrottler::GetQuota(bool interactive_request) const { On 2016/08/16 14:56:55, tschumann wrote: > It seems like we should turn RequestTypeInfo into a proper class, move those > methods onto the class and hide the members. Can happen in a follow-up CL but a > TODO() would be nice. Addded a TODO. |
