Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
423133003:
[Suggestions Service] Add support for expiring the SuggestionsStore (Closed)
|
Created:
6 years, 4 months ago by gayane -on leave until 09-2017 Modified:
6 years, 4 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionSuggestionsStore should take the suggestion's expiry timestamp into consideration.
-----------------------------------------------------------
1. filter expired suggestions before loading from cache
2. assign default expiry timestamps (now + 72 hours), to those ones which don't have expiry timestamps, before storing in cache
BUG=387926
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288375
Patch Set 1 : Expiry timestamps for suggestions #
Total comments: 92
Patch Set 2 : Coding style fixes and refactoring #
Total comments: 46
Patch Set 3 : more coding style changes and refactoring #
Total comments: 20
Patch Set 4 : more fixes #
Total comments: 34
Patch Set 5 : Some fixes. Test class for SuggestionStoreTest #
Total comments: 4
Patch Set 6 : minor changes and test improvement #Patch Set 7 : Test improvement: scoped_ptr for SuggestionsStore object in unittests #
Total comments: 14
Patch Set 8 : Last changes. #Patch Set 9 : unittest fixes #Messages
Total messages: 31 (0 generated)
Looking good! Don't mind the high number of comments, it's mostly all about formatting and it's totally expected. You can now start thinking about looking at the style guide. My comments will point to specific links, but feel free to poke around the guide. When reading the comments, keep in mind they generally apply to the line above the comment. Once you've addressed the comments, upload a new patch and send you're replies to the comments. https://codereview.chromium.org/423133003/diff/20001/chrome/browser/search/su... File chrome/browser/search/suggestions/suggestions_source.cc (right): https://codereview.chromium.org/423133003/diff/20001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:39: std::string FormatTimeDelta(int64 timedelta){ Include the unit in the name, eg time_delta_usec if it's microseconds. https://codereview.chromium.org/423133003/diff/20001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:40: No need for this empty line. https://codereview.chromium.org/423133003/diff/20001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:41: timedelta /= 1000; I'd argue the code will be more readable / less error prone if you never change the meaning of time_delta, eg: milliseconds = (time_delta_usec / Time.kMicrosecondsPerMillisecond) % kMillisecondsPerSecond; seconds = (time_delta_us / Time.kMicrosecondsPerSecond) % 60; https://codereview.chromium.org/423133003/diff/20001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:42: int milliseconds = timedelta % 1000; int64? https://codereview.chromium.org/423133003/diff/20001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:44: int seconds = timedelta%60; Put space around binary operators (timedelta % 60). Here and throughout. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Horizontal_Whi... https://codereview.chromium.org/423133003/diff/20001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:50: hours, minutes, seconds, milliseconds); Fixup the indentation as per: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... Likely the best is to send everythin after the open parenthesis to the next line and indent by 4 more. https://codereview.chromium.org/423133003/diff/20001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:65: -base::Time::UnixEpoch()).ToInternalValue(); Indentation. I would go with: int64 now = (Time::NowFromSystemTime() - Time::UnixEpoch()) <4spaces>.ToInternalValue(); Though other options exist. https://codereview.chromium.org/423133003/diff/20001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:69: Remove empty line: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Vertic... https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... File components/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_service.cc:329: SuggestionsProfile* suggestions){ Indent is wrong. You also want a space between ){ https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_service.cc:331: // now + 72 hours I don't think the comment adds much. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_service.cc:332: int64 now = (base::Time::NowFromSystemTime() now_usec? https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_service.cc:335: int64 timestamp = now + default_expiry * 60 * 60 * 1000 * 1000; expiry_timestamp_usec? https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_service.cc:337: for (int i = 0; i < suggestions->suggestions_size(); i++) { Preincrement i. See the other comment for details. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_service.cc:338: auto* suggestion = suggestions->mutable_suggestions(i); No auto in chromium afaik. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... File components/suggestions/suggestions_service.h (right): https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_service.h:95: // adds deafult expiry timestamp if neccessary Capitalize. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... File components/suggestions/suggestions_service_unittest.cc (right): https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_service_unittest.cc:98: SuggestionsProfile CreateSuggestionsProfile_MultipleSuggestions() { CreateSuggestionsProfileWithSomeExpiryTimestamps? https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_service_unittest.cc:100: -base::Time::UnixEpoch()).ToInternalValue(); indent. See other comments for the details. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_service_unittest.cc:530: EXPECT_LT(suggestions.suggestions(0).expiry_ts(), now); You could simplify testing by having AddDefaultExpiryTimestamps take in the expiry timestamps as a parameter. Then you'd check you get the exact values you expect. Of course this wouldn't cover the computation of the expiry timestamp, but that could be done seperatly. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... File components/suggestions/suggestions_store.cc (right): https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store.cc:7: #include <ctime> Used? https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store.cc:16: No need for this line. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store.cc:46: // save number of suggestions before filtering expired suggestions Capitalize Save https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store.cc:55: // if all suggestions were expired return false. These 3 code blocks don't need to be seperated by empty lines and you could have a single comment for all of them, eg: // Filter expired suggestions and update the stored suggestions if at least one was filtered. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store.cc:64: // private I think I've only seen this kind of comment for static functions, so I'd remove it. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store.cc:69: -base::Time::UnixEpoch()).ToInternalValue(); Indentation. See suggestions_source for details. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store.cc:71: for (int i = 0; i < suggestions->suggestions_size(); i++) { Even though not mandatory in this case, I believe preincrementing i is more common in this case: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Vertic... https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store.cc:72: auto* suggestion = suggestions->mutable_suggestions(i); AFAIK auto is not allowed yet since chromium hasn't moved to 11 yet. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store.cc:74: // copy if valid Comment does not add much so I'd remove it. You're also not copying. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... File components/suggestions/suggestions_store.h (right): https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store.h:52: // filters expired suggestions. Capitalize "filters". https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... File components/suggestions/suggestions_store_unittest.cc (right): https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:22: const int time_gap = 1000; Put the unit in the name. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:33: SuggestionsProfile CreateTestSuggestions_LoadExpiryTests(int expired, expired_count? valid_count? https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:33: SuggestionsProfile CreateTestSuggestions_LoadExpiryTests(int expired, I don't think CreateTestSuggestions_LoadExpiryTests follows the style. CreateTestSuggestionsProfileWithExpiry? You might use it for something other than the expiry tests. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:36: -base::Time::UnixEpoch()).ToInternalValue(); Fix indentation. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:37: No need for the space. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:38: // constant seed for rand() function. Move the comment to the srand line, 2 spaces after the ';' https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:44: Too much vertical whitespace. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:48: suggestion->set_title(kTestTitle); These 3 lines are duplicated 3 times. Have a AddSuggestion function? https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:49: int r = rand()%rand_limit; r isn't very descriptive. Something like future_offset_usec? https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:50: suggestion->set_expiry_ts(now + r + time_gap); I'd move the time gap into the computation of r. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:53: No need for this line. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:82: // tests LoadSuggestions function to filter expired suggestions Capitalize. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:91: // store and load. Expired suggestion should not be loaded. Capitalize. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:97: suggestions_store.ClearSuggestions(); Does this do anything? Isn't clean up done from TestingPrefServiceSyncable's destructor? https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:101: TEST(SuggestionsStoreTest, LoadOneValid) { Hm, this test is identical to the previous up to 3 characters. Perhaps a fixture would help? https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:119: TEST(SuggestionsStoreTest, LoadMultipleExpired) { What is the value of this test compared to the single expired? Tests are good, but long files have a cost in mental complexity for the reader. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:155: TEST(SuggestionsStoreTest, LoadValidAndExpiredStore) { I don't understand what the test does from the name (the store suffix). https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:155: TEST(SuggestionsStoreTest, LoadValidAndExpiredStore) { Perhaps keep only this test and the AllExpired one?
Thanks Pierre-Antoine for the feedback. I have fix most of them. Some of them I am unsure. You can find my replies for each of the comment. https://codereview.chromium.org/423133003/diff/20001/chrome/browser/search/su... File chrome/browser/search/suggestions/suggestions_source.cc (right): https://codereview.chromium.org/423133003/diff/20001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:39: std::string FormatTimeDelta(int64 timedelta){ On 2014/07/31 15:31:49, manzagop wrote: > Include the unit in the name, eg time_delta_usec if it's microseconds. Done. https://codereview.chromium.org/423133003/diff/20001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:40: On 2014/07/31 15:31:49, manzagop wrote: > No need for this empty line. Done. https://codereview.chromium.org/423133003/diff/20001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:41: timedelta /= 1000; On 2014/07/31 15:31:49, manzagop wrote: > I'd argue the code will be more readable / less error prone if you never change > the meaning of time_delta, eg: > > milliseconds = (time_delta_usec / Time.kMicrosecondsPerMillisecond) % > kMillisecondsPerSecond; > seconds = (time_delta_us / Time.kMicrosecondsPerSecond) % 60; Done. https://codereview.chromium.org/423133003/diff/20001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:42: int milliseconds = timedelta % 1000; On 2014/07/31 15:31:49, manzagop wrote: > int64? Isn't it always going to be less then 1000? https://codereview.chromium.org/423133003/diff/20001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:44: int seconds = timedelta%60; On 2014/07/31 15:31:49, manzagop wrote: > Put space around binary operators (timedelta % 60). Here and throughout. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Horizontal_Whi... Done. https://codereview.chromium.org/423133003/diff/20001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:50: hours, minutes, seconds, milliseconds); On 2014/07/31 15:31:49, manzagop wrote: > Fixup the indentation as per: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... > > Likely the best is to send everythin after the open parenthesis to the next line > and indent by 4 more. Done. https://codereview.chromium.org/423133003/diff/20001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:65: -base::Time::UnixEpoch()).ToInternalValue(); On 2014/07/31 15:31:49, manzagop wrote: > Indentation. I would go with: > int64 now = (Time::NowFromSystemTime() - Time::UnixEpoch()) > <4spaces>.ToInternalValue(); > > Though other options exist. I am not sure I did this right. Should it be 4 spaces from the beginning of line or after '=' int64 now = (base::Time::NowFromSystemTime() - base::Time::UnixEpoch()) .ToInternalValue(); OR int64 now = (base::Time::NowFromSystemTime() - base::Time::UnixEpoch()) .ToInternalValue(); https://codereview.chromium.org/423133003/diff/20001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:69: On 2014/07/31 15:31:49, manzagop wrote: > Remove empty line: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Vertic... Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... File components/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_service.cc:329: SuggestionsProfile* suggestions){ On 2014/07/31 15:31:50, manzagop wrote: > Indent is wrong. You also want a space between ){ Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_service.cc:331: // now + 72 hours On 2014/07/31 15:31:50, manzagop wrote: > I don't think the comment adds much. I have removed the comments and renamed default_expiry to default_expiry_hours https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_service.cc:332: int64 now = (base::Time::NowFromSystemTime() On 2014/07/31 15:31:50, manzagop wrote: > now_usec? Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_service.cc:335: int64 timestamp = now + default_expiry * 60 * 60 * 1000 * 1000; On 2014/07/31 15:31:50, manzagop wrote: > expiry_timestamp_usec? Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_service.cc:337: for (int i = 0; i < suggestions->suggestions_size(); i++) { On 2014/07/31 15:31:50, manzagop wrote: > Preincrement i. See the other comment for details. Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_service.cc:338: auto* suggestion = suggestions->mutable_suggestions(i); On 2014/07/31 15:31:50, manzagop wrote: > No auto in chromium afaik. Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... File components/suggestions/suggestions_service.h (right): https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_service.h:95: // adds deafult expiry timestamp if neccessary On 2014/07/31 15:31:50, manzagop wrote: > Capitalize. Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... File components/suggestions/suggestions_service_unittest.cc (right): https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_service_unittest.cc:98: SuggestionsProfile CreateSuggestionsProfile_MultipleSuggestions() { On 2014/07/31 15:31:50, manzagop wrote: > CreateSuggestionsProfileWithSomeExpiryTimestamps? Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_service_unittest.cc:100: -base::Time::UnixEpoch()).ToInternalValue(); On 2014/07/31 15:31:50, manzagop wrote: > indent. See other comments for the details. Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_service_unittest.cc:530: EXPECT_LT(suggestions.suggestions(0).expiry_ts(), now); On 2014/07/31 15:31:50, manzagop wrote: > You could simplify testing by having AddDefaultExpiryTimestamps take in the > expiry timestamps as a parameter. Then you'd check you get the exact values you > expect. Of course this wouldn't cover the computation of the expiry timestamp, > but that could be done seperatly. ---- 1. added a const private field in suggestions_service.h const int64 default_expiry_usec = 72 * base::Time.kMicrosecondsPerHour; 2. changed AddDefaultExpiryTimestamps(suggestions) to SetDefaultExpiryTimestamps(suggestions, timestamp) 3. changed unittest to check that first suggestions is not equal to passed timestamp while the second one is equal 4. changed OnURLFetchComplete to pass now_usec + default_expiry_usec to SetDefaultExpiryTimestamps method https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... File components/suggestions/suggestions_store.cc (right): https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store.cc:7: #include <ctime> On 2014/07/31 15:31:51, manzagop wrote: > Used? Removed https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store.cc:16: On 2014/07/31 15:31:51, manzagop wrote: > No need for this line. Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store.cc:46: // save number of suggestions before filtering expired suggestions On 2014/07/31 15:31:51, manzagop wrote: > Capitalize Save Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store.cc:55: // if all suggestions were expired return false. On 2014/07/31 15:31:51, manzagop wrote: > These 3 code blocks don't need to be seperated by empty lines and you could have > a single comment for all of them, eg: > > // Filter expired suggestions and update the stored suggestions if at least one > was filtered. Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store.cc:64: // private On 2014/07/31 15:31:51, manzagop wrote: > I think I've only seen this kind of comment for static functions, so I'd remove > it. Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store.cc:69: -base::Time::UnixEpoch()).ToInternalValue(); On 2014/07/31 15:31:50, manzagop wrote: > Indentation. See suggestions_source for details. Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store.cc:71: for (int i = 0; i < suggestions->suggestions_size(); i++) { On 2014/07/31 15:31:50, manzagop wrote: > Even though not mandatory in this case, I believe preincrementing i is more > common in this case: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Vertic... Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store.cc:72: auto* suggestion = suggestions->mutable_suggestions(i); On 2014/07/31 15:31:50, manzagop wrote: > AFAIK auto is not allowed yet since chromium hasn't moved to 11 yet. Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store.cc:74: // copy if valid On 2014/07/31 15:31:51, manzagop wrote: > Comment does not add much so I'd remove it. You're also not copying. Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... File components/suggestions/suggestions_store.h (right): https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store.h:52: // filters expired suggestions. On 2014/07/31 15:31:51, manzagop wrote: > Capitalize "filters". Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... File components/suggestions/suggestions_store_unittest.cc (right): https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:22: const int time_gap = 1000; On 2014/07/31 15:31:52, manzagop wrote: > Put the unit in the name. Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:33: SuggestionsProfile CreateTestSuggestions_LoadExpiryTests(int expired, On 2014/07/31 15:31:52, manzagop wrote: > I don't think CreateTestSuggestions_LoadExpiryTests follows the style. > CreateTestSuggestionsProfileWithExpiry? > > You might use it for something other than the expiry tests. Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:33: SuggestionsProfile CreateTestSuggestions_LoadExpiryTests(int expired, On 2014/07/31 15:31:53, manzagop wrote: > expired_count? valid_count? Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:36: -base::Time::UnixEpoch()).ToInternalValue(); On 2014/07/31 15:31:51, manzagop wrote: > Fix indentation. Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:37: On 2014/07/31 15:31:52, manzagop wrote: > No need for the space. Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:38: // constant seed for rand() function. On 2014/07/31 15:31:51, manzagop wrote: > Move the comment to the srand line, 2 spaces after the ';' Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:44: On 2014/07/31 15:31:51, manzagop wrote: > Too much vertical whitespace. Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:48: suggestion->set_title(kTestTitle); On 2014/07/31 15:31:52, manzagop wrote: > These 3 lines are duplicated 3 times. Have a AddSuggestion function? Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:49: int r = rand()%rand_limit; On 2014/07/31 15:31:51, manzagop wrote: > r isn't very descriptive. Something like future_offset_usec? Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:50: suggestion->set_expiry_ts(now + r + time_gap); On 2014/07/31 15:31:53, manzagop wrote: > I'd move the time gap into the computation of r. Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:53: On 2014/07/31 15:31:52, manzagop wrote: > No need for this line. Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:82: // tests LoadSuggestions function to filter expired suggestions On 2014/07/31 15:31:52, manzagop wrote: > Capitalize. Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:91: // store and load. Expired suggestion should not be loaded. On 2014/07/31 15:31:52, manzagop wrote: > Capitalize. Done. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:97: suggestions_store.ClearSuggestions(); On 2014/07/31 15:31:52, manzagop wrote: > Does this do anything? Isn't clean up done from TestingPrefServiceSyncable's > destructor? Removed https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:101: TEST(SuggestionsStoreTest, LoadOneValid) { On 2014/07/31 15:31:52, manzagop wrote: > Hm, this test is identical to the previous up to 3 characters. Perhaps a fixture > would help? What is fixture? https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:119: TEST(SuggestionsStoreTest, LoadMultipleExpired) { On 2014/07/31 15:31:51, manzagop wrote: > What is the value of this test compared to the single expired? Tests are good, > but long files have a cost in mental complexity for the reader. well I just thought of testing different cases. Unless you messed up the loop in filtration both should work or both should fail. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:155: TEST(SuggestionsStoreTest, LoadValidAndExpiredStore) { On 2014/07/31 15:31:52, manzagop wrote: > I don't understand what the test does from the name (the store suffix). yeah I changed the name, but I am not happy about this test actually. It is intended to check that after filtering expired suggestions it stores the updated list. Currently I check that by loading it again which is wrong for more than one reason. First Load might not store and filter each time. Second, if more suggestions get expired before second load is called because of time difference then the test will fail. https://codereview.chromium.org/423133003/diff/20001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:155: TEST(SuggestionsStoreTest, LoadValidAndExpiredStore) { On 2014/07/31 15:31:52, manzagop wrote: > Perhaps keep only this test and the AllExpired one? Done.
Here are my comments! https://codereview.chromium.org/423133003/diff/40001/chrome/browser/search/su... File chrome/browser/search/suggestions/suggestions_source.cc (right): https://codereview.chromium.org/423133003/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:39: std::string FormatTimeDelta(int64 time_delta_usec){ I searched and found this class, which may contain a method that would be equivalent to this: https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/l10n/time_... https://codereview.chromium.org/423133003/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:69: int64 diff = (suggestion.expiry_ts()-now); no () around the expression, add one space each side of the '-' https://codereview.chromium.org/423133003/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:85: remove extra line https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... File components/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_service.cc:330: remove extra new line https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_service.cc:334: remove extra line https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_service.cc:338: suggestion->set_expiry_ts(default_timestamp_usec); indent 2 less https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... File components/suggestions/suggestions_service.h (right): https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_service.h:95: // Sets default timestamp for suggestions which do not have expiry timestamp Period at the end of comment. https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_service.h:99: static const int64 default_expiry_usec = 72 * naming style dictates kConstantName https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_service.h:99: static const int64 default_expiry_usec = 72 * this constant should be declared above (similar to the field trial constants), and defined in the .cc https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... File components/suggestions/suggestions_service_unittest.cc (right): https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_service_unittest.cc:97: // Create one suggestion with expiry timestamp and one without *Creates Also period at the end of the comment. https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_service_unittest.cc:99: int64 now = (base::Time::NowFromSystemTime() - base::Time::UnixEpoch()) Have you tried ToJavaTime? I think it could be combined with NowFromSystemTime https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&l... https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_service_unittest.cc:523: CreateSuggestionsProfileWithExpiryTimestamps(); Indentation is off. Indent 4 from the start of the previous line (6 total). https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_service_unittest.cc:527: SuggestionsService::default_expiry_usec; do you need the "SuggestionsService::"? https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... File components/suggestions/suggestions_store.cc (right): https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_store.cc:45: // at least one was filtered. Return false if all suggestions are filtered. move as many words to previous line as you can. https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_store.cc:51: if (suggestions->suggestions_size() == 0){ if (!suggestions->suggesitons_size()) https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_store.cc:61: SuggestionsProfile* filteredSuggestions = new SuggestionsProfile(); to construct easily: SuggestionsProfile filtered_suggestions; then below you do filtered_suggestions.add_suggestions() and ->Swap(&filtered_suggestions) https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_store.cc:68: ChromeSuggestion* tmp = filteredSuggestions->add_suggestions(); Can probably do filteredSuggestions->add_suggestions()->Swap(suggestion) all in one line https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... File components/suggestions/suggestions_store_unittest.cc (right): https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:22: const int time_gap_usec = 1000; check naming style for constants https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:25: const char *url, int64 expiry_ts = 0) { indent one more space https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:25: const char *url, int64 expiry_ts = 0) { Hmm, I don't think I've ever seen default values in Google C++. Perhaps it's mentioned in the style guide? https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:29: if (expiry_ts != 0) { if (expiry_ts) { ... } But then again you may not need the check if you're removing the default value of the argument https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:42: int valid_count) { indent 1 more space https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:46: const int64 offset_limit_usec = 30 * base::Time::kMicrosecondsPerDay; kConstantName
fixed most if the comments except ToJavaTime(). See comments bellow. https://codereview.chromium.org/423133003/diff/40001/chrome/browser/search/su... File chrome/browser/search/suggestions/suggestions_source.cc (right): https://codereview.chromium.org/423133003/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:39: std::string FormatTimeDelta(int64 time_delta_usec){ On 2014/08/04 14:39:10, Mathieu Perreault wrote: > I searched and found this class, which may contain a method that would be > equivalent to this: > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/l10n/time_... With this class I can only show the two values(e.g. days/hours, or hours/minutes, etc.) from the remaining time, while previously I was showing all hours/minutes/seconds/milliseconds. I think two-value output is good enough and might be even better. https://codereview.chromium.org/423133003/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:69: int64 diff = (suggestion.expiry_ts()-now); On 2014/08/04 14:39:10, Mathieu Perreault wrote: > no () around the expression, add one space each side of the '-' Done. https://codereview.chromium.org/423133003/diff/40001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:85: On 2014/08/04 14:39:10, Mathieu Perreault wrote: > remove extra line Done. https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... File components/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_service.cc:330: On 2014/08/04 14:39:10, Mathieu Perreault wrote: > remove extra new line Done. https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_service.cc:334: On 2014/08/04 14:39:10, Mathieu Perreault wrote: > remove extra line Done. https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_service.cc:338: suggestion->set_expiry_ts(default_timestamp_usec); On 2014/08/04 14:39:10, Mathieu Perreault wrote: > indent 2 less Done. https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... File components/suggestions/suggestions_service.h (right): https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_service.h:95: // Sets default timestamp for suggestions which do not have expiry timestamp On 2014/08/04 14:39:10, Mathieu Perreault wrote: > Period at the end of comment. Done. https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_service.h:99: static const int64 default_expiry_usec = 72 * On 2014/08/04 14:39:10, Mathieu Perreault wrote: > this constant should be declared above (similar to the field trial constants), > and defined in the .cc Done. https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_service.h:99: static const int64 default_expiry_usec = 72 * On 2014/08/04 14:39:10, Mathieu Perreault wrote: > naming style dictates kConstantName Done. https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... File components/suggestions/suggestions_service_unittest.cc (right): https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_service_unittest.cc:97: // Create one suggestion with expiry timestamp and one without On 2014/08/04 14:39:10, Mathieu Perreault wrote: > *Creates > Also period at the end of the comment. Done. https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_service_unittest.cc:99: int64 now = (base::Time::NowFromSystemTime() - base::Time::UnixEpoch()) On 2014/08/04 14:39:10, Mathieu Perreault wrote: > Have you tried ToJavaTime? I think it could be combined with NowFromSystemTime > > https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&l... then I would need to do base::Time::NowFromSystemTime().ToJavaTime() * base::Time::kMicrosecondsPerMillisecond which is long as well https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_service_unittest.cc:523: CreateSuggestionsProfileWithExpiryTimestamps(); On 2014/08/04 14:39:10, Mathieu Perreault wrote: > Indentation is off. Indent 4 from the start of the previous line (6 total). Done. https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_service_unittest.cc:527: SuggestionsService::default_expiry_usec; On 2014/08/04 14:39:10, Mathieu Perreault wrote: > do you need the "SuggestionsService::"? Done. https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... File components/suggestions/suggestions_store.cc (right): https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_store.cc:45: // at least one was filtered. Return false if all suggestions are filtered. On 2014/08/04 14:39:11, Mathieu Perreault wrote: > move as many words to previous line as you can. Done. https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_store.cc:51: if (suggestions->suggestions_size() == 0){ On 2014/08/04 14:39:11, Mathieu Perreault wrote: > if (!suggestions->suggesitons_size()) Done. https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_store.cc:61: SuggestionsProfile* filteredSuggestions = new SuggestionsProfile(); On 2014/08/04 14:39:11, Mathieu Perreault wrote: > to construct easily: > > SuggestionsProfile filtered_suggestions; > > then below you do filtered_suggestions.add_suggestions() > > and ->Swap(&filtered_suggestions) Done. https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_store.cc:68: ChromeSuggestion* tmp = filteredSuggestions->add_suggestions(); On 2014/08/04 14:39:10, Mathieu Perreault wrote: > Can probably do filteredSuggestions->add_suggestions()->Swap(suggestion) all in > one line Done. https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... File components/suggestions/suggestions_store_unittest.cc (right): https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:22: const int time_gap_usec = 1000; On 2014/08/04 14:39:11, Mathieu Perreault wrote: > check naming style for constants Done. https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:25: const char *url, int64 expiry_ts = 0) { On 2014/08/04 14:39:11, Mathieu Perreault wrote: > indent one more space Done. https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:25: const char *url, int64 expiry_ts = 0) { On 2014/08/04 14:39:11, Mathieu Perreault wrote: > Hmm, I don't think I've ever seen default values in Google C++. Perhaps it's > mentioned in the style guide? Done. https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:29: if (expiry_ts != 0) { On 2014/08/04 14:39:11, Mathieu Perreault wrote: > if (expiry_ts) { > ... > } > > But then again you may not need the check if you're removing the default value > of the argument Done. https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:42: int valid_count) { On 2014/08/04 14:39:11, Mathieu Perreault wrote: > indent 1 more space Done. https://codereview.chromium.org/423133003/diff/40001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:46: const int64 offset_limit_usec = 30 * base::Time::kMicrosecondsPerDay; On 2014/08/04 14:39:11, Mathieu Perreault wrote: > kConstantName removed const
Mostly nits, have a look! https://codereview.chromium.org/423133003/diff/60001/chrome/browser/search/su... File chrome/browser/search/suggestions/suggestions_source.cc (right): https://codereview.chromium.org/423133003/diff/60001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:58: int64 diff = (suggestion.expiry_ts() - now); inline this expression in FromMicroseconds, below https://codereview.chromium.org/423133003/diff/60001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:59: base::TimeDelta td =base::TimeDelta::FromMicroseconds(diff); space after = https://codereview.chromium.org/423133003/diff/60001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:61: ui::TimeFormat::Format::FORMAT_DURATION, indent all parameters line by 4 from the previous line (8 total) https://codereview.chromium.org/423133003/diff/60001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:63: cutoff, td); directly pass -1, here https://codereview.chromium.org/423133003/diff/60001/components/suggestions/s... File components/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/423133003/diff/60001/components/suggestions/s... components/suggestions/suggestions_service.cc:104: const int64 kDefaultExpiryUsec = 72 * base::Time::kMicrosecondsPerHour; new line after this. https://codereview.chromium.org/423133003/diff/60001/components/suggestions/s... components/suggestions/suggestions_service.cc:321: SetDefaultExpiryTimestamps(&suggestions, timestamp_usec); inline now_usec + kDefaultExpiryUsec here https://codereview.chromium.org/423133003/diff/60001/components/suggestions/s... components/suggestions/suggestions_service.cc:336: if (!suggestion->has_expiry_ts()){ add a comment above this line: // Do not set expiry if the server has already provided a more specific expiry time for this suggestions. https://codereview.chromium.org/423133003/diff/60001/components/suggestions/s... File components/suggestions/suggestions_store.cc (right): https://codereview.chromium.org/423133003/diff/60001/components/suggestions/s... components/suggestions/suggestions_store.cc:68: filtered_suggestions.add_suggestions()->Swap(suggestion); indent 2 less
On 2014/08/04 18:29:22, Mathieu Perreault wrote: > Mostly nits, have a look! > > https://codereview.chromium.org/423133003/diff/60001/chrome/browser/search/su... > File chrome/browser/search/suggestions/suggestions_source.cc (right): > > https://codereview.chromium.org/423133003/diff/60001/chrome/browser/search/su... > chrome/browser/search/suggestions/suggestions_source.cc:58: int64 diff = > (suggestion.expiry_ts() - now); > inline this expression in FromMicroseconds, below > > https://codereview.chromium.org/423133003/diff/60001/chrome/browser/search/su... > chrome/browser/search/suggestions/suggestions_source.cc:59: base::TimeDelta td > =base::TimeDelta::FromMicroseconds(diff); > space after = > > https://codereview.chromium.org/423133003/diff/60001/chrome/browser/search/su... > chrome/browser/search/suggestions/suggestions_source.cc:61: > ui::TimeFormat::Format::FORMAT_DURATION, > indent all parameters line by 4 from the previous line (8 total) > > https://codereview.chromium.org/423133003/diff/60001/chrome/browser/search/su... > chrome/browser/search/suggestions/suggestions_source.cc:63: cutoff, td); > directly pass -1, here > > https://codereview.chromium.org/423133003/diff/60001/components/suggestions/s... > File components/suggestions/suggestions_service.cc (right): > > https://codereview.chromium.org/423133003/diff/60001/components/suggestions/s... > components/suggestions/suggestions_service.cc:104: const int64 > kDefaultExpiryUsec = 72 * base::Time::kMicrosecondsPerHour; > new line after this. > > https://codereview.chromium.org/423133003/diff/60001/components/suggestions/s... > components/suggestions/suggestions_service.cc:321: > SetDefaultExpiryTimestamps(&suggestions, timestamp_usec); > inline now_usec + kDefaultExpiryUsec here > > https://codereview.chromium.org/423133003/diff/60001/components/suggestions/s... > components/suggestions/suggestions_service.cc:336: if > (!suggestion->has_expiry_ts()){ > add a comment above this line: > > // Do not set expiry if the server has already provided a more specific expiry > time for this suggestions. > > https://codereview.chromium.org/423133003/diff/60001/components/suggestions/s... > File components/suggestions/suggestions_store.cc (right): > > https://codereview.chromium.org/423133003/diff/60001/components/suggestions/s... > components/suggestions/suggestions_store.cc:68: > filtered_suggestions.add_suggestions()->Swap(suggestion); > indent 2 less Please change your description to [Suggestions Service] Add support for expiring the SuggestionsStore
Do you want to have that sentence somewhere in the description or you want to substitute whole description text with that sentence? On Mon, Aug 4, 2014 at 2:46 PM, <mathp@chromium.org> wrote: > On 2014/08/04 18:29:22, Mathieu Perreault wrote: > >> Mostly nits, have a look! >> > > > https://codereview.chromium.org/423133003/diff/60001/ > chrome/browser/search/suggestions/suggestions_source.cc > >> File chrome/browser/search/suggestions/suggestions_source.cc (right): >> > > > https://codereview.chromium.org/423133003/diff/60001/ > chrome/browser/search/suggestions/suggestions_source.cc#newcode58 > >> chrome/browser/search/suggestions/suggestions_source.cc:58: int64 diff = >> (suggestion.expiry_ts() - now); >> inline this expression in FromMicroseconds, below >> > > > https://codereview.chromium.org/423133003/diff/60001/ > chrome/browser/search/suggestions/suggestions_source.cc#newcode59 > >> chrome/browser/search/suggestions/suggestions_source.cc:59: >> base::TimeDelta td >> =base::TimeDelta::FromMicroseconds(diff); >> space after = >> > > > https://codereview.chromium.org/423133003/diff/60001/ > chrome/browser/search/suggestions/suggestions_source.cc#newcode61 > >> chrome/browser/search/suggestions/suggestions_source.cc:61: >> ui::TimeFormat::Format::FORMAT_DURATION, >> indent all parameters line by 4 from the previous line (8 total) >> > > > https://codereview.chromium.org/423133003/diff/60001/ > chrome/browser/search/suggestions/suggestions_source.cc#newcode63 > >> chrome/browser/search/suggestions/suggestions_source.cc:63: cutoff, td); >> directly pass -1, here >> > > > https://codereview.chromium.org/423133003/diff/60001/ > components/suggestions/suggestions_service.cc > >> File components/suggestions/suggestions_service.cc (right): >> > > > https://codereview.chromium.org/423133003/diff/60001/ > components/suggestions/suggestions_service.cc#newcode104 > >> components/suggestions/suggestions_service.cc:104: const int64 >> kDefaultExpiryUsec = 72 * base::Time::kMicrosecondsPerHour; >> new line after this. >> > > > https://codereview.chromium.org/423133003/diff/60001/ > components/suggestions/suggestions_service.cc#newcode321 > >> components/suggestions/suggestions_service.cc:321: >> SetDefaultExpiryTimestamps(&suggestions, timestamp_usec); >> inline now_usec + kDefaultExpiryUsec here >> > > > https://codereview.chromium.org/423133003/diff/60001/ > components/suggestions/suggestions_service.cc#newcode336 > >> components/suggestions/suggestions_service.cc:336: if >> (!suggestion->has_expiry_ts()){ >> add a comment above this line: >> > > // Do not set expiry if the server has already provided a more specific >> expiry >> time for this suggestions. >> > > > https://codereview.chromium.org/423133003/diff/60001/ > components/suggestions/suggestions_store.cc > >> File components/suggestions/suggestions_store.cc (right): >> > > > https://codereview.chromium.org/423133003/diff/60001/ > components/suggestions/suggestions_store.cc#newcode68 > >> components/suggestions/suggestions_store.cc:68: >> filtered_suggestions.add_suggestions()->Swap(suggestion); >> indent 2 less >> > > Please change your description to > > [Suggestions Service] Add support for expiring the SuggestionsStore > > https://codereview.chromium.org/423133003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Almost there! https://codereview.chromium.org/423133003/diff/60001/chrome/browser/search/su... File chrome/browser/search/suggestions/suggestions_source.cc (right): https://codereview.chromium.org/423133003/diff/60001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:53: int cutoff = -1; // forces two-value output for TimeFormat Capitalize 'forces'. https://codereview.chromium.org/423133003/diff/60001/components/suggestions/s... File components/suggestions/suggestions_service.h (right): https://codereview.chromium.org/423133003/diff/60001/components/suggestions/s... components/suggestions/suggestions_service.h:99: int64 timestamp_usec); nit: I'd align this with SuggestionsProfile.
The title should be something like the one I wrote. The rest is OK. Note that ChromeSuggestionsStore is not a thing that exists. 2014-08-04 14:56 GMT-04:00 Gayane Petrosyan <gayane@google.com>: > Do you want to have that sentence somewhere in the description or you want > to substitute whole description text with that sentence? > > > On Mon, Aug 4, 2014 at 2:46 PM, <mathp@chromium.org> wrote: > >> On 2014/08/04 18:29:22, Mathieu Perreault wrote: >> >>> Mostly nits, have a look! >>> >> >> >> https://codereview.chromium.org/423133003/diff/60001/ >> chrome/browser/search/suggestions/suggestions_source.cc >> >>> File chrome/browser/search/suggestions/suggestions_source.cc (right): >>> >> >> >> https://codereview.chromium.org/423133003/diff/60001/ >> chrome/browser/search/suggestions/suggestions_source.cc#newcode58 >> >>> chrome/browser/search/suggestions/suggestions_source.cc:58: int64 diff = >>> (suggestion.expiry_ts() - now); >>> inline this expression in FromMicroseconds, below >>> >> >> >> https://codereview.chromium.org/423133003/diff/60001/ >> chrome/browser/search/suggestions/suggestions_source.cc#newcode59 >> >>> chrome/browser/search/suggestions/suggestions_source.cc:59: >>> base::TimeDelta td >>> =base::TimeDelta::FromMicroseconds(diff); >>> space after = >>> >> >> >> https://codereview.chromium.org/423133003/diff/60001/ >> chrome/browser/search/suggestions/suggestions_source.cc#newcode61 >> >>> chrome/browser/search/suggestions/suggestions_source.cc:61: >>> ui::TimeFormat::Format::FORMAT_DURATION, >>> indent all parameters line by 4 from the previous line (8 total) >>> >> >> >> https://codereview.chromium.org/423133003/diff/60001/ >> chrome/browser/search/suggestions/suggestions_source.cc#newcode63 >> >>> chrome/browser/search/suggestions/suggestions_source.cc:63: cutoff, td); >>> directly pass -1, here >>> >> >> >> https://codereview.chromium.org/423133003/diff/60001/ >> components/suggestions/suggestions_service.cc >> >>> File components/suggestions/suggestions_service.cc (right): >>> >> >> >> https://codereview.chromium.org/423133003/diff/60001/ >> components/suggestions/suggestions_service.cc#newcode104 >> >>> components/suggestions/suggestions_service.cc:104: const int64 >>> kDefaultExpiryUsec = 72 * base::Time::kMicrosecondsPerHour; >>> new line after this. >>> >> >> >> https://codereview.chromium.org/423133003/diff/60001/ >> components/suggestions/suggestions_service.cc#newcode321 >> >>> components/suggestions/suggestions_service.cc:321: >>> SetDefaultExpiryTimestamps(&suggestions, timestamp_usec); >>> inline now_usec + kDefaultExpiryUsec here >>> >> >> >> https://codereview.chromium.org/423133003/diff/60001/ >> components/suggestions/suggestions_service.cc#newcode336 >> >>> components/suggestions/suggestions_service.cc:336: if >>> (!suggestion->has_expiry_ts()){ >>> add a comment above this line: >>> >> >> // Do not set expiry if the server has already provided a more specific >>> expiry >>> time for this suggestions. >>> >> >> >> https://codereview.chromium.org/423133003/diff/60001/ >> components/suggestions/suggestions_store.cc >> >>> File components/suggestions/suggestions_store.cc (right): >>> >> >> >> https://codereview.chromium.org/423133003/diff/60001/ >> components/suggestions/suggestions_store.cc#newcode68 >> >>> components/suggestions/suggestions_store.cc:68: >>> filtered_suggestions.add_suggestions()->Swap(suggestion); >>> indent 2 less >>> >> >> Please change your description to >> >> [Suggestions Service] Add support for expiring the SuggestionsStore >> >> https://codereview.chromium.org/423133003/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
here is it https://codereview.chromium.org/423133003/diff/60001/chrome/browser/search/su... File chrome/browser/search/suggestions/suggestions_source.cc (right): https://codereview.chromium.org/423133003/diff/60001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:53: int cutoff = -1; // forces two-value output for TimeFormat On 2014/08/04 19:02:59, manzagop wrote: > Capitalize 'forces'. skipped because the variable is removed. If you want I can add the comment directly before the function call https://codereview.chromium.org/423133003/diff/60001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:58: int64 diff = (suggestion.expiry_ts() - now); On 2014/08/04 18:29:21, Mathieu Perreault wrote: > inline this expression in FromMicroseconds, below Done. https://codereview.chromium.org/423133003/diff/60001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:59: base::TimeDelta td =base::TimeDelta::FromMicroseconds(diff); On 2014/08/04 18:29:21, Mathieu Perreault wrote: > space after = Done. https://codereview.chromium.org/423133003/diff/60001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:61: ui::TimeFormat::Format::FORMAT_DURATION, On 2014/08/04 18:29:21, Mathieu Perreault wrote: > indent all parameters line by 4 from the previous line (8 total) Done. https://codereview.chromium.org/423133003/diff/60001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_source.cc:63: cutoff, td); On 2014/08/04 18:29:21, Mathieu Perreault wrote: > directly pass -1, here Done. https://codereview.chromium.org/423133003/diff/60001/components/suggestions/s... File components/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/423133003/diff/60001/components/suggestions/s... components/suggestions/suggestions_service.cc:104: const int64 kDefaultExpiryUsec = 72 * base::Time::kMicrosecondsPerHour; On 2014/08/04 18:29:21, Mathieu Perreault wrote: > new line after this. Done. https://codereview.chromium.org/423133003/diff/60001/components/suggestions/s... components/suggestions/suggestions_service.cc:321: SetDefaultExpiryTimestamps(&suggestions, timestamp_usec); On 2014/08/04 18:29:21, Mathieu Perreault wrote: > inline now_usec + kDefaultExpiryUsec here Done. https://codereview.chromium.org/423133003/diff/60001/components/suggestions/s... components/suggestions/suggestions_service.cc:336: if (!suggestion->has_expiry_ts()){ On 2014/08/04 18:29:21, Mathieu Perreault wrote: > add a comment above this line: > > // Do not set expiry if the server has already provided a more specific expiry > time for this suggestions. Done. https://codereview.chromium.org/423133003/diff/60001/components/suggestions/s... File components/suggestions/suggestions_service.h (right): https://codereview.chromium.org/423133003/diff/60001/components/suggestions/s... components/suggestions/suggestions_service.h:99: int64 timestamp_usec); On 2014/08/04 19:02:59, manzagop wrote: > nit: I'd align this with SuggestionsProfile. Done. https://codereview.chromium.org/423133003/diff/60001/components/suggestions/s... File components/suggestions/suggestions_store.cc (right): https://codereview.chromium.org/423133003/diff/60001/components/suggestions/s... components/suggestions/suggestions_store.cc:68: filtered_suggestions.add_suggestions()->Swap(suggestion); On 2014/08/04 18:29:22, Mathieu Perreault wrote: > indent 2 less Done.
A few more! https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... File components/suggestions/suggestions_service_unittest.cc (right): https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_service_unittest.cc:518: extra line https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_service_unittest.cc:529: EXPECT_NE(suggestions.suggestions(0).expiry_ts(), default_timestamp_usec); Perhaps you could ensure it's a specific value by defining and setting it to a kTestExpiry? https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... File components/suggestions/suggestions_store.cc (right): https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_store.cc:48: if (suggestions->suggestions_size() != unfiltered_size){ nit: space before the curly bracket https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_store.cc:51: if (!suggestions->suggestions_size()){ nit: same here. https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_store.cc:51: if (!suggestions->suggestions_size()){ Perhaps in this case we should call ClearSuggestions() and there's no need to call StoreSuggestions. https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... File components/suggestions/suggestions_store_unittest.cc (right): https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:22: const int kTimeGapUsec = 1000; IIUC this is the timespan within which tests must complete in order for them valid suggestions to be ensured validity. As discussed in the first round, a time dependency isn't ideal usually. You can usually pass in the cutoff timestamp as argument to avoid it. That said it's ok here. However, 1ms sounds short. Perhaps increase to 100ms? https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:85: // store and load. expired suggestions should not be loaded. Capitals, here and below.
Suggestions to improve tests https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... File components/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_service.cc:104: const int64 kDefaultExpiryUsec = 72 * base::Time::kMicrosecondsPerHour; Add a comment above saying that the default expiry for suggestions is 72 hours. https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_service.cc:337: // expiry time for this suggestions. *this suggestion https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_service.cc:338: if (!suggestion->has_expiry_ts()){ space before { https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... File components/suggestions/suggestions_service.h (right): https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_service.h:98: void SetDefaultExpiryTimestamps(SuggestionsProfile* suggestions, nit: rename to SetDefaultExpiryTimestamp https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... File components/suggestions/suggestions_store_unittest.cc (right): https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:8: remove extra newline https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:78: TestingPrefServiceSyncable prefs; since this is now duplicated many times, could you create the SuggestionsStoreTest class and store things such as |prefs| there? The test class can also own a scoped_ptr<SuggestionsStore> so you won't need to create it in every test. Example: https://code.google.com/p/chromium/codesearch#chromium/src/components/suggest... (Note TEST_F instead of TEST: it means that tests inherit the test class). https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:85: // store and load. expired suggestions should not be loaded. nit: Capital letter to start sentences. https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:117: EXPECT_TRUE(suggestions_store.LoadSuggestions(&filtered_suggestions)); you can remove the lines that are replicated in the test above (related to filtered_suggestions)
https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... File components/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_service.cc:104: const int64 kDefaultExpiryUsec = 72 * base::Time::kMicrosecondsPerHour; On 2014/08/04 19:54:55, Mathieu Perreault wrote: > Add a comment above saying that the default expiry for suggestions is 72 hours. Done. https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_service.cc:104: const int64 kDefaultExpiryUsec = 72 * base::Time::kMicrosecondsPerHour; On 2014/08/04 19:54:55, Mathieu Perreault wrote: > Add a comment above saying that the default expiry for suggestions is 72 hours. Done. https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_service.cc:337: // expiry time for this suggestions. On 2014/08/04 19:54:54, Mathieu Perreault wrote: > *this suggestion Done. https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... File components/suggestions/suggestions_service.h (right): https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_service.h:98: void SetDefaultExpiryTimestamps(SuggestionsProfile* suggestions, On 2014/08/04 19:54:55, Mathieu Perreault wrote: > nit: rename to SetDefaultExpiryTimestamp Done. https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... File components/suggestions/suggestions_service_unittest.cc (right): https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_service_unittest.cc:518: On 2014/08/04 19:50:26, manzagop wrote: > extra line Done. https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_service_unittest.cc:529: EXPECT_NE(suggestions.suggestions(0).expiry_ts(), default_timestamp_usec); On 2014/08/04 19:50:25, manzagop wrote: > Perhaps you could ensure it's a specific value by defining and setting it to a > kTestExpiry? Done. https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... File components/suggestions/suggestions_store.cc (right): https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_store.cc:48: if (suggestions->suggestions_size() != unfiltered_size){ On 2014/08/04 19:50:26, manzagop wrote: > nit: space before the curly bracket Done. https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_store.cc:51: if (!suggestions->suggestions_size()){ On 2014/08/04 19:50:26, manzagop wrote: > Perhaps in this case we should call ClearSuggestions() and there's no need to > call StoreSuggestions. Done. https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_store.cc:51: if (!suggestions->suggestions_size()){ On 2014/08/04 19:50:26, manzagop wrote: > Perhaps in this case we should call ClearSuggestions() and there's no need to > call StoreSuggestions. Done. https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... File components/suggestions/suggestions_store_unittest.cc (right): https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:8: On 2014/08/04 19:54:55, Mathieu Perreault wrote: > remove extra newline Done. https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:22: const int kTimeGapUsec = 1000; On 2014/08/04 19:50:26, manzagop wrote: > IIUC this is the timespan within which tests must complete in order for them > valid suggestions to be ensured validity. As discussed in the first round, a > time dependency isn't ideal usually. You can usually pass in the cutoff > timestamp as argument to avoid it. That said it's ok here. > > However, 1ms sounds short. Perhaps increase to 100ms? Done. https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:78: TestingPrefServiceSyncable prefs; On 2014/08/04 19:54:55, Mathieu Perreault wrote: > since this is now duplicated many times, could you create the > SuggestionsStoreTest class and store things such as |prefs| there? The test > class can also own a scoped_ptr<SuggestionsStore> so you won't need to create it > in every test. > > Example: > https://code.google.com/p/chromium/codesearch#chromium/src/components/suggest... > > (Note TEST_F instead of TEST: it means that tests inherit the test class). test class created. but didn't manage to put scoped_ptr<SuggestionStore> in the test class. Maybe you can help me with that. https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:85: // store and load. expired suggestions should not be loaded. On 2014/08/04 19:50:26, manzagop wrote: > Capitals, here and below. Done. https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:85: // store and load. expired suggestions should not be loaded. On 2014/08/04 19:54:55, Mathieu Perreault wrote: > nit: Capital letter to start sentences. Done. https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:117: EXPECT_TRUE(suggestions_store.LoadSuggestions(&filtered_suggestions)); On 2014/08/04 19:54:55, Mathieu Perreault wrote: > you can remove the lines that are replicated in the test above (related to > filtered_suggestions) Done.
A few more comments. https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... File components/suggestions/suggestions_store_unittest.cc (right): https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:8: On 2014/08/05 14:39:15, gayane wrote: > On 2014/08/04 19:54:55, Mathieu Perreault wrote: > > remove extra newline > > Done. Nope! ;) https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:22: const int kTimeGapUsec = 1000; On 2014/08/05 14:39:16, gayane wrote: > On 2014/08/04 19:50:26, manzagop wrote: > > IIUC this is the timespan within which tests must complete in order for them > > valid suggestions to be ensured validity. As discussed in the first round, a > > time dependency isn't ideal usually. You can usually pass in the cutoff > > timestamp as argument to avoid it. That said it's ok here. > > > > However, 1ms sounds short. Perhaps increase to 100ms? > > Done. Probably warrants a comment. https://codereview.chromium.org/423133003/diff/100001/components/suggestions/... File components/suggestions/suggestions_service_unittest.cc (right): https://codereview.chromium.org/423133003/diff/100001/components/suggestions/... components/suggestions/suggestions_service_unittest.cc:526: EXPECT_NE(suggestions.suggestions(0).expiry_ts(), kTestExpiry); An equality test is a stronger statement. I'd keep the equality test you had for this one. Sorry if my previous comment was unclear: typically a comment will apply to the line right above it. https://codereview.chromium.org/423133003/diff/100001/components/suggestions/... File components/suggestions/suggestions_store.cc (right): https://codereview.chromium.org/423133003/diff/100001/components/suggestions/... components/suggestions/suggestions_store.cc:50: ClearSuggestions(); Any reason why you removed "suggestions->Clear();"?
https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... File components/suggestions/suggestions_store_unittest.cc (right): https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:8: On 2014/08/05 15:30:26, manzagop wrote: > On 2014/08/05 14:39:15, gayane wrote: > > On 2014/08/04 19:54:55, Mathieu Perreault wrote: > > > remove extra newline > > > > Done. > > Nope! ;) Done this time. https://codereview.chromium.org/423133003/diff/100001/components/suggestions/... File components/suggestions/suggestions_service_unittest.cc (right): https://codereview.chromium.org/423133003/diff/100001/components/suggestions/... components/suggestions/suggestions_service_unittest.cc:526: EXPECT_NE(suggestions.suggestions(0).expiry_ts(), kTestExpiry); On 2014/08/05 15:30:26, manzagop wrote: > An equality test is a stronger statement. I'd keep the equality test you had for > this one. > > Sorry if my previous comment was unclear: typically a comment will apply to the > line right above it. Done. https://codereview.chromium.org/423133003/diff/100001/components/suggestions/... File components/suggestions/suggestions_store.cc (right): https://codereview.chromium.org/423133003/diff/100001/components/suggestions/... components/suggestions/suggestions_store.cc:50: ClearSuggestions(); On 2014/08/05 15:30:26, manzagop wrote: > Any reason why you removed "suggestions->Clear();"? added back before CLearSuggestions() call.
https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... File components/suggestions/suggestions_store_unittest.cc (right): https://codereview.chromium.org/423133003/diff/80001/components/suggestions/s... components/suggestions/suggestions_store_unittest.cc:78: TestingPrefServiceSyncable prefs; On 2014/08/04 19:54:55, Mathieu Perreault wrote: > since this is now duplicated many times, could you create the > SuggestionsStoreTest class and store things such as |prefs| there? The test > class can also own a scoped_ptr<SuggestionsStore> so you won't need to create it > in every test. > > Example: > https://code.google.com/p/chromium/codesearch#chromium/src/components/suggest... > > (Note TEST_F instead of TEST: it means that tests inherit the test class). Done.
lgtm, wait for Pierre-Antoine's approval too, please :) https://codereview.chromium.org/423133003/diff/140001/components/suggestions/... File components/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/423133003/diff/140001/components/suggestions/... components/suggestions/suggestions_service.cc:104: // The default expiry timeout it 72 hours. *is https://codereview.chromium.org/423133003/diff/140001/components/suggestions/... File components/suggestions/suggestions_service.h (right): https://codereview.chromium.org/423133003/diff/140001/components/suggestions/... components/suggestions/suggestions_service.h:98: int64 timestamp_usec); indent 1 less https://codereview.chromium.org/423133003/diff/140001/components/suggestions/... File components/suggestions/suggestions_store.cc (right): https://codereview.chromium.org/423133003/diff/140001/components/suggestions/... components/suggestions/suggestions_store.cc:49: if (!suggestions->suggestions_size()) { indent 2 less for the whole block https://codereview.chromium.org/423133003/diff/140001/components/suggestions/... components/suggestions/suggestions_store.cc:54: else { bring else to previous line
LGTM with nits Well done! https://codereview.chromium.org/423133003/diff/140001/chrome/browser/search/s... File chrome/browser/search/suggestions/suggestions_source.cc (right): https://codereview.chromium.org/423133003/diff/140001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_source.cc:55: base::TimeDelta td = base::TimeDelta::FromMicroseconds( td isn't self-explanatory. Can you find better? eg expiry_delay_usec? http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera... https://codereview.chromium.org/423133003/diff/140001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_source.cc:57: base::string16 formatted = ui::TimeFormat::Detailed( Perhaps td_formatted so it's clear what this? https://codereview.chromium.org/423133003/diff/140001/components/suggestions/... File components/suggestions/suggestions_service_unittest.cc (right): https://codereview.chromium.org/423133003/diff/140001/components/suggestions/... components/suggestions/suggestions_service_unittest.cc:50: (base::Time::NowFromSystemTime() - base::Time::UnixEpoch()) Does calling those functions bring added value compare to just using 123456 (i mean a reasonable constant number)? Otherwise, just use a number to get the test as simple as possible (probably will run faster too - there are thousands of tests).
last changes https://codereview.chromium.org/423133003/diff/140001/chrome/browser/search/s... File chrome/browser/search/suggestions/suggestions_source.cc (right): https://codereview.chromium.org/423133003/diff/140001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_source.cc:55: base::TimeDelta td = base::TimeDelta::FromMicroseconds( On 2014/08/06 19:11:18, manzagop wrote: > td isn't self-explanatory. Can you find better? eg expiry_delay_usec? > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera... renamed as remaining_time https://codereview.chromium.org/423133003/diff/140001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_source.cc:57: base::string16 formatted = ui::TimeFormat::Detailed( On 2014/08/06 19:11:18, manzagop wrote: > Perhaps td_formatted so it's clear what this? renamed as remaining_time_formatted https://codereview.chromium.org/423133003/diff/140001/components/suggestions/... File components/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/423133003/diff/140001/components/suggestions/... components/suggestions/suggestions_service.cc:104: // The default expiry timeout it 72 hours. On 2014/08/06 15:23:53, Mathieu Perreault wrote: > *is Done. https://codereview.chromium.org/423133003/diff/140001/components/suggestions/... File components/suggestions/suggestions_service.h (right): https://codereview.chromium.org/423133003/diff/140001/components/suggestions/... components/suggestions/suggestions_service.h:98: int64 timestamp_usec); On 2014/08/06 15:23:53, Mathieu Perreault wrote: > indent 1 less Done. https://codereview.chromium.org/423133003/diff/140001/components/suggestions/... File components/suggestions/suggestions_service_unittest.cc (right): https://codereview.chromium.org/423133003/diff/140001/components/suggestions/... components/suggestions/suggestions_service_unittest.cc:50: (base::Time::NowFromSystemTime() - base::Time::UnixEpoch()) On 2014/08/06 19:11:18, manzagop wrote: > Does calling those functions bring added value compare to just using 123456 (i > mean a reasonable constant number)? Otherwise, just use a number to get the test > as simple as possible (probably will run faster too - there are thousands of > tests). Done. https://codereview.chromium.org/423133003/diff/140001/components/suggestions/... File components/suggestions/suggestions_store.cc (right): https://codereview.chromium.org/423133003/diff/140001/components/suggestions/... components/suggestions/suggestions_store.cc:49: if (!suggestions->suggestions_size()) { On 2014/08/06 15:23:54, Mathieu Perreault wrote: > indent 2 less for the whole block Done. https://codereview.chromium.org/423133003/diff/140001/components/suggestions/... components/suggestions/suggestions_store.cc:54: else { On 2014/08/06 15:23:54, Mathieu Perreault wrote: > bring else to previous line Done.
The CQ bit was checked by gayane@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gayane@chromium.org/423133003/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
The CQ bit was checked by manzagop@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gayane@chromium.org/423133003/160001
The CQ bit was unchecked by manzagop@chromium.org
Looks like there are actual errors, perhaps related to the old tests and the addition of default timestamps. http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_... Try running these tests using eg --gtest_filter=Suggestions*:BlacklistStore* On Thu, Aug 7, 2014 at 9:05 AM, <commit-bot@chromium.org> wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/gayane@ > chromium.org/423133003/160001 > > > https://codereview.chromium.org/423133003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by gayane@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gayane@chromium.org/423133003/180001
Message was sent while issue was closed.
Change committed as 288375 |
