Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2277213003:
[TTS] Add aggregation of CTR metrics to the CS component. (Closed)
|
Created:
4 years, 3 months ago by Donn Denman Modified:
4 years, 3 months ago CC:
chromium-reviews, twellington+watch_chromium.org, donnd+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[TTS] Add aggregation of CTR metrics to the CS component.
Adds a new class that knows how to aggregate impressions and clicks
over one-week and 28-day intervals.
This will be used by Contextual Search to record impressions and CTR by
user to use as signals for Tap triggering.
BUG=609668
Committed: https://crrev.com/ce321e2acadf1b001651d2e41cec0d5ed96e50ed
Cr-Commit-Position: refs/heads/master@{#417698}
Patch Set 1 #
Total comments: 21
Patch Set 2 : Using the cache now, renamed NativeIntStorage DeviceIntStorage. #Patch Set 3 : Just changed some test names. #Patch Set 4 : Just added comments to device_int_storage.h. #
Total comments: 12
Patch Set 5 : Major refactoring to separate storage implementation details from the aggregator, as suggested by m… #Patch Set 6 : Removed the cache from WeeklyActivityStorage. #
Total comments: 4
Patch Set 7 : Renamed method names in WeeklyActivityStorage ending in "Activity", and elaborated why initing shou… #Patch Set 8 : Updated components/OWNERS to forward Contextual Search to its owners. #Patch Set 9 : Just a rebase and removal of gyp files no longer needed. #Patch Set 10 : Just added a dependency for tests to components/contextual_search/BUILD.gn. #Patch Set 11 : Just a rebase. #Patch Set 12 : Just a rebase. #
Total comments: 2
Patch Set 13 : Moved the build rule to the non-iOS section for the new unittest. #Patch Set 14 : Just added an include file to hopefully fix the windows build. #Patch Set 15 : Updated the unittest to cast expected results to float since windows seems to default to double. #Messages
Total messages: 57 (23 generated)
donnd@chromium.org changed reviewers: + marq@chromium.org, twellington@chromium.org
Theresa and Mark, This is the first addition to the CS component designed to be cross platform, so PTAL with an eye to how both iOS and Android can take advantage of shared logic. E.g. is it in the right place? Should we be leveraging some Chromium storage instead of having it injected (with a future CL defining how to store on Android -- I'll cc you both on that now).
On 2016/08/26 16:26:25, Donn Denman wrote: > Theresa and Mark, > > This is the first addition to the CS component designed to be cross platform, so > PTAL with an eye to how both iOS and Android can take advantage of shared logic. > E.g. is it in the right place? Should we be leveraging some Chromium storage > instead of having it injected (with a future CL defining how to store on Android > -- I'll cc you both on that now). Let me know if you think the "NativeIntStorage" name could be improved. Maybe DeviceIntegerStorage?
https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... File components/contextual_search/browser/ctr_aggregator.cc (right): https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... components/contextual_search/browser/ctr_aggregator.cc:26: const int kReasonableMinWeek = 2000; Why 2000? https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... components/contextual_search/browser/ctr_aggregator.cc:41: storage_.WriteInt(kFirstWeekWrittenKey, week_number_); Does this line need to get included in the 1-param constructor as well? https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... components/contextual_search/browser/ctr_aggregator.cc:80: return (week_number_ - first_week_written >= 4); use > kNumWeeksToRecord instead of 4? https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... components/contextual_search/browser/ctr_aggregator.cc:98: for (int week = 1; week <= 4; week++) { week < kNumWeeksToRecord? https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... components/contextual_search/browser/ctr_aggregator.cc:107: int CTRAggregator::GetStorageIndex(int which_week) { Is this called anywhere other than GetWeekKey? If not, maybe we should collapse those two methods? https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... components/contextual_search/browser/ctr_aggregator.cc:188: void CTRAggregator::WriteInt(std::string storage_key, int value) { What's calling this? https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... File components/contextual_search/browser/ctr_aggregator.h (right): https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... components/contextual_search/browser/ctr_aggregator.h:94: void Increment(std::string key); nit: I would put the GetPreviousWeekClicks() and getPrevious28DayClicks() next to GetWeekImpressionsKey() and move this to it's own block. https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... File components/contextual_search/browser/ctr_aggregator_unittest.cc (right): https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... components/contextual_search/browser/ctr_aggregator_unittest.cc:109: TEST_F(CTRAggregatorTest, SkipThreeWeekTest) { Week -> Weeks? https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... File components/contextual_search/browser/native_int_storage.h (right): https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... components/contextual_search/browser/native_int_storage.h:14: class NativeIntStorage { I think DeviceIntStorage does make more sense or PersistentIntStorage or something like that. Also, this class could use a description.
Theresa, PTAL. Mark, do you want to review the overall structure of this code to be sure it can be used in iOS? Or is it enough to know that it's in components/, no dependencies on Java or Android, etc. https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... File components/contextual_search/browser/ctr_aggregator.cc (right): https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... components/contextual_search/browser/ctr_aggregator.cc:26: const int kReasonableMinWeek = 2000; On 2016/08/26 23:32:08, Theresa Wellington wrote: > Why 2000? Updated the comment: // Used for validation in debug build. Week numbers are > 2300 as of year 2016. https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... components/contextual_search/browser/ctr_aggregator.cc:41: storage_.WriteInt(kFirstWeekWrittenKey, week_number_); On 2016/08/26 23:32:08, Theresa Wellington wrote: > Does this line need to get included in the 1-param constructor as well? It's problematic to put it into the constructor since that can cause a write, which calls back into the Java code. So I decided to just make sure each public accessor checks. I've updated the comment on that method to describe this. It's also no longer needed in this test constructor, so I removed it. https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... components/contextual_search/browser/ctr_aggregator.cc:80: return (week_number_ - first_week_written >= 4); On 2016/08/26 23:32:08, Theresa Wellington wrote: > use > kNumWeeksToRecord instead of 4? Done: made a const kNumWeeksNeededFor28DayData = 4 (since using 5 won't work). :-) https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... components/contextual_search/browser/ctr_aggregator.cc:98: for (int week = 1; week <= 4; week++) { On 2016/08/26 23:32:08, Theresa Wellington wrote: > week < kNumWeeksToRecord? Done: seems better to use the new kNumWeeksNeededFor28DayData symbol. https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... components/contextual_search/browser/ctr_aggregator.cc:107: int CTRAggregator::GetStorageIndex(int which_week) { On 2016/08/26 23:32:08, Theresa Wellington wrote: > Is this called anywhere other than GetWeekKey? If not, maybe we should collapse > those two methods? Done. https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... components/contextual_search/browser/ctr_aggregator.cc:188: void CTRAggregator::WriteInt(std::string storage_key, int value) { On 2016/08/26 23:32:08, Theresa Wellington wrote: > What's calling this? Oops -- thanks for catching this! I added caching at the end, and forgot to call it. Doh! Tested this time, and found a flaw with overwriting values, and fixed. https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... File components/contextual_search/browser/ctr_aggregator.h (right): https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... components/contextual_search/browser/ctr_aggregator.h:94: void Increment(std::string key); On 2016/08/26 23:32:08, Theresa Wellington wrote: > nit: I would put the GetPreviousWeekClicks() and getPrevious28DayClicks() next > to GetWeekImpressionsKey() and move this to it's own block. Done. https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... File components/contextual_search/browser/ctr_aggregator_unittest.cc (right): https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... components/contextual_search/browser/ctr_aggregator_unittest.cc:109: TEST_F(CTRAggregatorTest, SkipThreeWeekTest) { On 2016/08/26 23:32:08, Theresa Wellington wrote: > Week -> Weeks? Done. https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... File components/contextual_search/browser/native_int_storage.h (right): https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... components/contextual_search/browser/native_int_storage.h:14: class NativeIntStorage { On 2016/08/26 23:32:08, Theresa Wellington wrote: > I think DeviceIntStorage does make more sense or PersistentIntStorage or > something like that. > > Also, this class could use a description. Done.
lgtm https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... File components/contextual_search/browser/ctr_aggregator.cc (right): https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... components/contextual_search/browser/ctr_aggregator.cc:26: const int kReasonableMinWeek = 2000; On 2016/08/29 22:58:06, Donn Denman wrote: > On 2016/08/26 23:32:08, Theresa Wellington wrote: > > Why 2000? > > Updated the comment: // Used for validation in debug build. Week numbers are > > 2300 as of year 2016. Acknowledged. https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... components/contextual_search/browser/ctr_aggregator.cc:80: return (week_number_ - first_week_written >= 4); On 2016/08/29 22:58:06, Donn Denman wrote: > On 2016/08/26 23:32:08, Theresa Wellington wrote: > > use > kNumWeeksToRecord instead of 4? > > Done: made a const kNumWeeksNeededFor28DayData = 4 (since using 5 won't work). > :-) Done.
https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... File components/contextual_search/browser/ctr_aggregator.cc (right): https://codereview.chromium.org/2277213003/diff/1/components/contextual_searc... components/contextual_search/browser/ctr_aggregator.cc:80: return (week_number_ - first_week_written >= 4); On 2016/08/29 23:57:34, Theresa Wellington wrote: > On 2016/08/29 22:58:06, Donn Denman wrote: > > On 2016/08/26 23:32:08, Theresa Wellington wrote: > > > use > kNumWeeksToRecord instead of 4? > > > > Done: made a const kNumWeeksNeededFor28DayData = 4 (since using 5 won't work). > > :-) > > Done. Err.. Acknowledged.
On 2016/08/29 22:58:06, Donn Denman wrote: > Theresa, PTAL. > > Mark, do you want to review the overall structure of this code to be sure it can > be used in iOS? Or is it enough to know that it's in components/, no > dependencies on Java or Android, etc. I'll take a pass at it today.
At a high level, this seems fine for iOS, although without yet seeing the details of how a concrete DeviceIntStorage() instance is created, I can't be 100% sure. From a general design perspective, I have some comments. Storage in prefs seems the correct approach, and building an abstraction layer to separate the aggregation logic from the storage mechanism is a very good idea. I would suggest moving more of the storage implementation details into the DeviceIntStorage() implementation. The API that the aggregator uses should only be read_clicks(week_number), write_clicks(week_number, value) and clear_clicks(weak_number) (with _impressions() versions of these as well). Mapping these to pref names is the job of the storage layer (if it's backed by prefs). Likewise caching, if it is needed at all, belongs to the storage layer. If prefs are used, I'd want to be sure that a cache improves on the existing performance of the prefs system. This means you can write tests for the aggregator against a storage object that's very simple, and write tests for the storage implementation without also having to test the aggregator. https://codereview.chromium.org/2277213003/diff/60001/components/contextual_s... File components/contextual_search/browser/ctr_aggregator.h (right): https://codereview.chromium.org/2277213003/diff/60001/components/contextual_s... components/contextual_search/browser/ctr_aggregator.h:5: // Provides aggregation of feature usage by tracking impressions and clicks Define "impression" and "click" in this context ("click" is confusing when the devices this is used on have touch interfaces) https://codereview.chromium.org/2277213003/diff/60001/components/contextual_s... components/contextual_search/browser/ctr_aggregator.h:7: // Used by Contextual Search to record impressions of the Bar and CTR of Define "CTR". https://codereview.chromium.org/2277213003/diff/60001/components/contextual_s... components/contextual_search/browser/ctr_aggregator.h:22: // aggregated data. Get data from the previous week or previous 4-week period How is this data used? Does it leave the device? How can the user delete it? https://codereview.chromium.org/2277213003/diff/60001/components/contextual_s... components/contextual_search/browser/ctr_aggregator.h:34: // Record an impression. Records a click if |did_click| is true. s/Record/Records/ https://codereview.chromium.org/2277213003/diff/60001/components/contextual_s... components/contextual_search/browser/ctr_aggregator.h:77: // Ensure that the week number of the first week ever used is recorded. Ensures. https://codereview.chromium.org/2277213003/diff/60001/components/contextual_s... File components/contextual_search/browser/device_int_storage.h (right): https://codereview.chromium.org/2277213003/diff/60001/components/contextual_s... components/contextual_search/browser/device_int_storage.h:26: If this class is intended to be an abstraction layer that could be backed by (for example) Prefs values, then there should also be a Clear() method that removes the storage bucket.
Mark and Theresa, I separated the storage from the aggregator, and I do think the design is cleaner now, but at the expense of some code expansion. Most of the logic was moved to the storage class with the aggregator now just a pretty simple wrapper. I don't think prefs are the right storage mechanism for persisting this data because I understand prefs to be user-controlled, and this data is machine generated. There's a very simple storage mechanism on Android that's used in the dependent CL to do persist the data on the device. I'm guessing it won't be too hard to find something similar for iOS, and we can use prefs if that seems reasonable. Mark, PTAL, and thanks for the review and great design feedback! https://codereview.chromium.org/2277213003/diff/60001/components/contextual_s... File components/contextual_search/browser/ctr_aggregator.h (right): https://codereview.chromium.org/2277213003/diff/60001/components/contextual_s... components/contextual_search/browser/ctr_aggregator.h:5: // Provides aggregation of feature usage by tracking impressions and clicks On 2016/08/30 08:30:36, marq wrote: > Define "impression" and "click" in this context ("click" is confusing when the > devices this is used on have touch interfaces) Done. https://codereview.chromium.org/2277213003/diff/60001/components/contextual_s... components/contextual_search/browser/ctr_aggregator.h:7: // Used by Contextual Search to record impressions of the Bar and CTR of On 2016/08/30 08:30:36, marq wrote: > Define "CTR". Done. https://codereview.chromium.org/2277213003/diff/60001/components/contextual_s... components/contextual_search/browser/ctr_aggregator.h:22: // aggregated data. Get data from the previous week or previous 4-week period On 2016/08/30 08:30:36, marq wrote: > How is this data used? Does it leave the device? How can the user delete it? Done. https://codereview.chromium.org/2277213003/diff/60001/components/contextual_s... components/contextual_search/browser/ctr_aggregator.h:34: // Record an impression. Records a click if |did_click| is true. On 2016/08/30 08:30:36, marq wrote: > s/Record/Records/ Done. https://codereview.chromium.org/2277213003/diff/60001/components/contextual_s... components/contextual_search/browser/ctr_aggregator.h:77: // Ensure that the week number of the first week ever used is recorded. On 2016/08/30 08:30:36, marq wrote: > Ensures. Done. https://codereview.chromium.org/2277213003/diff/60001/components/contextual_s... File components/contextual_search/browser/device_int_storage.h (right): https://codereview.chromium.org/2277213003/diff/60001/components/contextual_s... components/contextual_search/browser/device_int_storage.h:26: On 2016/08/30 08:30:36, marq wrote: > If this class is intended to be an abstraction layer that could be backed by > (for example) Prefs values, then there should also be a Clear() method that > removes the storage bucket. Done.
Mark, this is ready for another look from you, however brief. Thanks! On Aug 30, 2016 9:35 PM, <donnd@chromium.org> wrote: Mark and Theresa, I separated the storage from the aggregator, and I do think the design is cleaner now, but at the expense of some code expansion. Most of the logic was moved to the storage class with the aggregator now just a pretty simple wrapper. I don't think prefs are the right storage mechanism for persisting this data because I understand prefs to be user-controlled, and this data is machine generated. There's a very simple storage mechanism on Android that's used in the dependent CL to do persist the data on the device. I'm guessing it won't be too hard to find something similar for iOS, and we can use prefs if that seems reasonable. Mark, PTAL, and thanks for the review and great design feedback! https://codereview.chromium.org/2277213003/diff/60001/ components/contextual_search/browser/ctr_aggregator.h File components/contextual_search/browser/ctr_aggregator.h (right): https://codereview.chromium.org/2277213003/diff/60001/ components/contextual_search/browser/ctr_aggregator.h#newcode5 components/contextual_search/browser/ctr_aggregator.h:5: // Provides aggregation of feature usage by tracking impressions and clicks On 2016/08/30 08:30:36, marq wrote: > Define "impression" and "click" in this context ("click" is confusing when the > devices this is used on have touch interfaces) Done. https://codereview.chromium.org/2277213003/diff/60001/ components/contextual_search/browser/ctr_aggregator.h#newcode7 components/contextual_search/browser/ctr_aggregator.h:7: // Used by Contextual Search to record impressions of the Bar and CTR of On 2016/08/30 08:30:36, marq wrote: > Define "CTR". Done. https://codereview.chromium.org/2277213003/diff/60001/ components/contextual_search/browser/ctr_aggregator.h#newcode22 components/contextual_search/browser/ctr_aggregator.h:22: // aggregated data. Get data from the previous week or previous 4-week period On 2016/08/30 08:30:36, marq wrote: > How is this data used? Does it leave the device? How can the user delete it? Done. https://codereview.chromium.org/2277213003/diff/60001/ components/contextual_search/browser/ctr_aggregator.h#newcode34 components/contextual_search/browser/ctr_aggregator.h:34: // Record an impression. Records a click if |did_click| is true. On 2016/08/30 08:30:36, marq wrote: > s/Record/Records/ Done. https://codereview.chromium.org/2277213003/diff/60001/ components/contextual_search/browser/ctr_aggregator.h#newcode77 components/contextual_search/browser/ctr_aggregator.h:77: // Ensure that the week number of the first week ever used is recorded. On 2016/08/30 08:30:36, marq wrote: > Ensures. Done. https://codereview.chromium.org/2277213003/diff/60001/ components/contextual_search/browser/device_int_storage.h File components/contextual_search/browser/device_int_storage.h (right): https://codereview.chromium.org/2277213003/diff/60001/ components/contextual_search/browser/device_int_storage.h#newcode26 components/contextual_search/browser/device_int_storage.h:26: On 2016/08/30 08:30:36, marq wrote: > If this class is intended to be an abstraction layer that could be backed by > (for example) Prefs values, then there should also be a Clear() method that > removes the storage bucket. Done. https://codereview.chromium.org/2277213003/ On Aug 30, 2016 9:35 PM, <donnd@chromium.org> wrote: > Mark and Theresa, > > I separated the storage from the aggregator, and I do think the design is > cleaner now, but at the expense of some code expansion. Most of the logic > was > moved to the storage class with the aggregator now just a pretty simple > wrapper. > > I don't think prefs are the right storage mechanism for persisting this > data > because I understand prefs to be user-controlled, and this data is machine > generated. There's a very simple storage mechanism on Android that's used > in > the dependent CL to do persist the data on the device. I'm guessing it > won't be > too hard to find something similar for iOS, and we can use prefs if that > seems > reasonable. > > Mark, PTAL, and thanks for the review and great design feedback! > > > https://codereview.chromium.org/2277213003/diff/60001/ > components/contextual_search/browser/ctr_aggregator.h > File components/contextual_search/browser/ctr_aggregator.h (right): > > https://codereview.chromium.org/2277213003/diff/60001/ > components/contextual_search/browser/ctr_aggregator.h#newcode5 > components/contextual_search/browser/ctr_aggregator.h:5: // Provides > aggregation of feature usage by tracking impressions and clicks > On 2016/08/30 08:30:36, marq wrote: > > Define "impression" and "click" in this context ("click" is confusing > when the > > devices this is used on have touch interfaces) > > Done. > > https://codereview.chromium.org/2277213003/diff/60001/ > components/contextual_search/browser/ctr_aggregator.h#newcode7 > components/contextual_search/browser/ctr_aggregator.h:7: // Used by > Contextual Search to record impressions of the Bar and CTR of > On 2016/08/30 08:30:36, marq wrote: > > Define "CTR". > > Done. > > https://codereview.chromium.org/2277213003/diff/60001/ > components/contextual_search/browser/ctr_aggregator.h#newcode22 > components/contextual_search/browser/ctr_aggregator.h:22: // aggregated > data. Get data from the previous week or previous 4-week period > On 2016/08/30 08:30:36, marq wrote: > > How is this data used? Does it leave the device? How can the user > delete it? > > Done. > > https://codereview.chromium.org/2277213003/diff/60001/ > components/contextual_search/browser/ctr_aggregator.h#newcode34 > components/contextual_search/browser/ctr_aggregator.h:34: // Record an > impression. Records a click if |did_click| is true. > On 2016/08/30 08:30:36, marq wrote: > > s/Record/Records/ > > Done. > > https://codereview.chromium.org/2277213003/diff/60001/ > components/contextual_search/browser/ctr_aggregator.h#newcode77 > components/contextual_search/browser/ctr_aggregator.h:77: // Ensure that > the week number of the first week ever used is recorded. > On 2016/08/30 08:30:36, marq wrote: > > Ensures. > > Done. > > https://codereview.chromium.org/2277213003/diff/60001/ > components/contextual_search/browser/device_int_storage.h > File components/contextual_search/browser/device_int_storage.h (right): > > https://codereview.chromium.org/2277213003/diff/60001/ > components/contextual_search/browser/device_int_storage.h#newcode26 > components/contextual_search/browser/device_int_storage.h:26: > On 2016/08/30 08:30:36, marq wrote: > > If this class is intended to be an abstraction layer that could be > backed by > > (for example) Prefs values, then there should also be a Clear() method > that > > removes the storage bucket. > > Done. > > https://codereview.chromium.org/2277213003/ > -- 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.
Gave it a quick pass, overall LGTM and this seems like a much cleaner design. https://codereview.chromium.org/2277213003/diff/100001/components/contextual_... File components/contextual_search/browser/weekly_activity_storage.h (right): https://codereview.chromium.org/2277213003/diff/100001/components/contextual_... components/contextual_search/browser/weekly_activity_storage.h:40: void InitActivity(int week_number); Any reason why calling this isn't handled automatically internally to this class?
still lgtm % 1 comment on method naming https://codereview.chromium.org/2277213003/diff/100001/components/contextual_... File components/contextual_search/browser/weekly_activity_storage.h (right): https://codereview.chromium.org/2277213003/diff/100001/components/contextual_... components/contextual_search/browser/weekly_activity_storage.h:43: bool HasActivity(int week_number); I think this method the "Activity" methods need different names. Activity automatically makes me think of an Android Activity. Maybe "HasData(int week_number)", ClearData() and InitWeek() or something like that?
Thanks for the multiple reviews! https://chromiumcodereview.appspot.com/2277213003/diff/100001/components/cont... File components/contextual_search/browser/weekly_activity_storage.h (right): https://chromiumcodereview.appspot.com/2277213003/diff/100001/components/cont... components/contextual_search/browser/weekly_activity_storage.h:40: void InitActivity(int week_number); On 2016/09/02 13:09:45, marq wrote: > Any reason why calling this isn't handled automatically internally to this > class? Yes. Renamed the method and added this comment to clarify: // Advances the accessible storage range to end at the given |week_number|. // Since only a limited number of storage weeks are supported, advancing to // a different week makes data from weeks than the range size inaccessible. // This must be called for each week before reading or writing any data // for that week. // HasData will return true for all the weeks that still have accessible data. void AdvanceToWeek(int week_number); https://chromiumcodereview.appspot.com/2277213003/diff/100001/components/cont... components/contextual_search/browser/weekly_activity_storage.h:43: bool HasActivity(int week_number); On 2016/09/02 16:25:29, Theresa Wellington wrote: > I think this method the "Activity" methods need different names. Activity > automatically makes me think of an Android Activity. Maybe "HasData(int > week_number)", ClearData() and InitWeek() or something like that? Done.
donnd@chromium.org changed reviewers: + blundell@chromium.org
Colin, PTAL at components/BUILD.gn, components/contextual_search.gypi, and components/OWNERS. I decided to update components/OWNERS, even though you said it may not be needed when reviewing CL https://codereview.chromium.org/2288353003/, because I need to bother you about BUILD.gn anyway, and it seems like it may help to have this done as well. Thanks in advance!
On 2016/09/02 23:08:23, Donn Denman wrote: > Colin, PTAL at components/BUILD.gn, components/contextual_search.gypi, and > components/OWNERS. > > I decided to update components/OWNERS, even though you said it may not be needed > when reviewing CL https://codereview.chromium.org/2288353003/, because I need to > bother you about BUILD.gn anyway, and it seems like it may help to have this > done as well. > > Thanks in advance! LGTM What I was trying to get at is that per https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NZkPr-CXvQ0/dn1rb..., you no longer have to keep the GYP build updated on trunk. So for example you don't need to make the changes to the gyp(i) files that you've made in this CL. Similarly, the gyp build isn't even guaranteed to work anymore regardless of whether you make those changes, since other people won't necessarily be keeping it updated when they make changes. That said, it's up to you whether you want to leave in the changes in this CL or not.
Colin, Thanks for the explanation and the review!
The CQ bit was checked by donnd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org, marq@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2277213003/#ps140001 (title: "Updated components/OWNERS to forward Contextual Search to its owners.")
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
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) 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_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by donnd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org, marq@chromium.org, blundell@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2277213003/#ps160001 (title: "Just a rebase and removal of gyp files no longer needed.")
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
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_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 donnd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org, marq@chromium.org, blundell@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2277213003/#ps180001 (title: "Just added a dependency for tests to components/contextual_search/BUILD.gn.")
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
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by donnd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org, marq@chromium.org, blundell@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2277213003/#ps220001 (title: "Just a rebase.")
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
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...)
https://chromiumcodereview.appspot.com/2277213003/diff/220001/components/BUIL... File components/BUILD.gn (right): https://chromiumcodereview.appspot.com/2277213003/diff/220001/components/BUIL... components/BUILD.gn:76: "//components/contextual_search:unit_tests", I think that moving this to the !iOS section should solve the problem you're seeing. (There's also a larger question of whether you'll eventually want this feature to be shared with iOS, which would require reworking).
Colin, Thanks for noticing my problem and suggesting a fix! https://chromiumcodereview.appspot.com/2277213003/diff/220001/components/BUIL... File components/BUILD.gn (right): https://chromiumcodereview.appspot.com/2277213003/diff/220001/components/BUIL... components/BUILD.gn:76: "//components/contextual_search:unit_tests", On 2016/09/09 07:41:44, blundell wrote: > I think that moving this to the !iOS section should solve the problem you're > seeing. (There's also a larger question of whether you'll eventually want this > feature to be shared with iOS, which would require reworking). I had though it would work for iOS but I guess we're bringing in some Blink dependency.
The CQ bit was checked by donnd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org, marq@chromium.org, blundell@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2277213003/#ps240001 (title: "Moved the build rule to the non-iOS section for the new unittest.")
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by donnd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org, marq@chromium.org, blundell@chromium.org Link to the patchset: https://codereview.chromium.org/2277213003/#ps260001 (title: "Just added an include file to hopefully fix the windows build.")
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by donnd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org, marq@chromium.org, blundell@chromium.org Link to the patchset: https://codereview.chromium.org/2277213003/#ps280001 (title: "Updated the unittest to cast expected results to float since windows seems to default to double.")
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.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== [TTS] Add aggregation of CTR metrics to the CS component. Adds a new class that knows how to aggregate impressions and clicks over one-week and 28-day intervals. This will be used by Contextual Search to record impressions and CTR by user to use as signals for Tap triggering. BUG=609668 ========== to ========== [TTS] Add aggregation of CTR metrics to the CS component. Adds a new class that knows how to aggregate impressions and clicks over one-week and 28-day intervals. This will be used by Contextual Search to record impressions and CTR by user to use as signals for Tap triggering. BUG=609668 Committed: https://crrev.com/ce321e2acadf1b001651d2e41cec0d5ed96e50ed Cr-Commit-Position: refs/heads/master@{#417698} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/ce321e2acadf1b001651d2e41cec0d5ed96e50ed Cr-Commit-Position: refs/heads/master@{#417698}
Message was sent while issue was closed.
On 2016/09/09 17:43:05, Donn Denman wrote: > Colin, Thanks for noticing my problem and suggesting a fix! > > https://chromiumcodereview.appspot.com/2277213003/diff/220001/components/BUIL... > File components/BUILD.gn (right): > > https://chromiumcodereview.appspot.com/2277213003/diff/220001/components/BUIL... > components/BUILD.gn:76: "//components/contextual_search:unit_tests", > On 2016/09/09 07:41:44, blundell wrote: > > I think that moving this to the !iOS section should solve the problem you're > > seeing. (There's also a larger question of whether you'll eventually want this > > feature to be shared with iOS, which would require reworking). > > I had though it would work for iOS but I guess we're bringing in some Blink > dependency. If intended for sharing with iOS, the component will likely need to be made a layered component: https://www.chromium.org/developers/design-documents/layered-components-design |
