Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1330773002:
[Android] Persist ordering of NTP suggestions. (Closed)
Description[Android] Persist ordering of NTP suggestions.
Persist order of NTP suggestions by storing them in preferences.
Previously the order was guided by popular sites unless the personal
suggestions are completely different.
This is wrong in two important cases namely:
-Users who get into the popular suggestions experiment for the first time.
-When the popular suggestions are updated.
BUG=528915
Committed: https://crrev.com/cbbd754d90f22a594d5c10c2f6c2e447cfabb5b5
Cr-Commit-Position: refs/heads/master@{#347938}
Patch Set 1 #
Total comments: 47
Patch Set 2 : rebase #
Total comments: 8
Patch Set 3 : Move things around in a struct #
Total comments: 23
Patch Set 4 : Comments #
Total comments: 2
Patch Set 5 : return instead of taking an argument #
Total comments: 29
Patch Set 6 : #
Total comments: 11
Patch Set 7 : gurl #Patch Set 8 : nit #
Total comments: 2
Patch Set 9 : nit #
Dependent Patchsets: Messages
Total messages: 36 (5 generated)
knn@chromium.org changed reviewers: + bauerb@chromium.org, treib@chromium.org
PTAL. Thanks!
First bunch of comments. All this is getting *way* too complicated... but nothing to be done about it I guess. Since we're storing the suggestions in prefs now, we don't really need to keep the json file around. But that can be done in another CL. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:135: void GetPreviousNTPSites(std::vector<std::string>* old_sites_url, nit: Inputs first, then outputs. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:148: old_sites_url->emplace_back(url_string); push_back - no C++11 library yet. It'll do the same thing here anyway. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:151: if (num_tiles == 0) { Move this above the loop. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:454: void MostVisitedSites::AddPopularSites( Any reason why this doesn't just update |titles| and |urls| in-place anymore? Would save the caller from having to create two extra vectors. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:463: std::vector<std::string> personal_hosts; Make this an std::set? Would make checking membership later more convenient. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:468: // Collect non-blacklisted popular suggestions removing duplicates from nit: You're not removing anything from personal suggestions. "Collect non-blacklisted popular suggestions, skipping those already present in the personal suggestions" ? https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:481: if (popular_titles.size() >= num_popular_suggestions) Put this at the end, right after you've added one? https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:495: popular_hosts.emplace_back(host); push_back https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:542: DCHECK_LE(num_old_tiles, num_tiles); I don't think this is necessarily always true? https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:559: if (old_sites_host[position] == personal_hosts[i]) I think here you also need to check that the old suggestions was a popular one? There can be multiple personal suggestions with the same host. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:565: (*result_urls)[position] = std::move((*personal_urls)[i]); We're not allowed to use C++11 library features yet, so no std::move. On the upside, that also means the inputs can be const& instead of * :) https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:585: (*result_urls)[position] = std::move((*popular_urls)[i]); ^^ https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:603: // Ignore setting result_is_filled as it is isn't used anymore. s/Ignore/Skip/g https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.h:86: // Adds the suggestions from |popular_sites_| into |titles| and |urls|. This Comment isn't accurate anymore now. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.h:97: // Return values. nit: Order of arguments should be inputs, then outputs. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... File chrome/browser/android/most_visited_sites_unittest.cc (left): https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites_unittest.cc:168: TEST_F(MostVisitedSitesTest, PopularSitesComplex) { Why did you remove this test? I don't think your current tests cover the overflow case in particular (i.e. more total suggestions than we have tiles). https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... File chrome/browser/android/most_visited_sites_unittest.cc (right): https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites_unittest.cc:52: // - Removing blacklisted suggestions. Also, blacklisting suggestions in the first place. I don't think we particularly care about the blacklisting case though :) https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites_unittest.cc:55: // - Correct Host extraction from the URL. This is just a feature of GURL and (hopefully) tested elsewhere? I guess you actually mean the "duplicate" elimination? https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites_unittest.cc:56: // - Ensuring popular suggestions don't contain personal ones. What does this mean? https://codereview.chromium.org/1330773002/diff/1/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/1330773002/diff/1/chrome/common/pref_names.cc... chrome/common/pref_names.cc:1498: const char kNTPSuggestionsSource[] = "ntp.suggestions_source"; This is actually a list of bools, so what does "source" mean? Maybe call it "is_personal" or something?
https://codereview.chromium.org/1330773002/diff/20001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1330773002/diff/20001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:565: (*result_urls)[position] = std::move((*personal_urls)[i]); std::move is a C++11 library feature, and while we use libc++ on Android now, we don't on other platforms, and I'm not quite sure whether the style guide allows it. https://codereview.chromium.org/1330773002/diff/20001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1330773002/diff/20001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:103: size_t num_personal_suggestions, This is the number of elements in |personal_*| below, right? Why do we need to pass it in again? https://codereview.chromium.org/1330773002/diff/20001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:105: // Passed as ptr for move semantics only. I don't quite understand what you mean by move semantics here. To allow modification (which would include moving elements out of the container)? https://codereview.chromium.org/1330773002/diff/20001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:106: std::vector<std::string>* personal_urls, Could we put all of these into a ScopedVector of some struct? Something like: struct Suggestion { base::string16 title; string url; }; And then extend as necessary and pass a ScopedVector for each of personal suggestions, popular suggestions, and output... or something.
https://codereview.chromium.org/1330773002/diff/20001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1330773002/diff/20001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:106: std::vector<std::string>* personal_urls, On 2015/09/07 11:22:58, Bernhard Bauer wrote: > Could we put all of these into a ScopedVector of some struct? Something like: > > struct Suggestion { > base::string16 title; > string url; > }; > > And then extend as necessary and pass a ScopedVector for each of personal > suggestions, popular suggestions, and output... or something. +1 for the struct, but why ScopedVector? A regular vector should do just fine?!
On 2015/09/07 11:32:07, Marc Treib wrote: > https://codereview.chromium.org/1330773002/diff/20001/chrome/browser/android/... > File chrome/browser/android/most_visited_sites.h (right): > > https://codereview.chromium.org/1330773002/diff/20001/chrome/browser/android/... > chrome/browser/android/most_visited_sites.h:106: std::vector<std::string>* > personal_urls, > On 2015/09/07 11:22:58, Bernhard Bauer wrote: > > Could we put all of these into a ScopedVector of some struct? Something like: > > > > struct Suggestion { > > base::string16 title; > > string url; > > }; > > > > And then extend as necessary and pass a ScopedVector for each of personal > > suggestions, popular suggestions, and output... or something. > > +1 for the struct, but why ScopedVector? A regular vector should do just fine?! Might make it easier to move entries in.
Moving things around in a struct now. I thought I could get away with std::move since only Mac does not have C++11 libraries yet. PTAL. I have addressed all the old comments. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:135: void GetPreviousNTPSites(std::vector<std::string>* old_sites_url, On 2015/09/07 11:12:59, Marc Treib wrote: > nit: Inputs first, then outputs. Done. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:148: old_sites_url->emplace_back(url_string); On 2015/09/07 11:12:59, Marc Treib wrote: > push_back - no C++11 library yet. It'll do the same thing here anyway. Done. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:151: if (num_tiles == 0) { On 2015/09/07 11:12:59, Marc Treib wrote: > Move this above the loop. Done. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:454: void MostVisitedSites::AddPopularSites( On 2015/09/07 11:12:59, Marc Treib wrote: > Any reason why this doesn't just update |titles| and |urls| in-place anymore? > Would save the caller from having to create two extra vectors. Did you mean inplace? I did not want to complicate the logic too much. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:463: std::vector<std::string> personal_hosts; On 2015/09/07 11:12:59, Marc Treib wrote: > Make this an std::set? Would make checking membership later more convenient. Done. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:468: // Collect non-blacklisted popular suggestions removing duplicates from On 2015/09/07 11:12:59, Marc Treib wrote: > nit: You're not removing anything from personal suggestions. > "Collect non-blacklisted popular suggestions, skipping those already present in > the personal suggestions" ? Done. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:481: if (popular_titles.size() >= num_popular_suggestions) On 2015/09/07 11:12:59, Marc Treib wrote: > Put this at the end, right after you've added one? Done. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:495: popular_hosts.emplace_back(host); On 2015/09/07 11:12:59, Marc Treib wrote: > push_back Done. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:542: DCHECK_LE(num_old_tiles, num_tiles); On 2015/09/07 11:12:59, Marc Treib wrote: > I don't think this is necessarily always true? It is because GetPreviousNTPSites ensures that. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:559: if (old_sites_host[position] == personal_hosts[i]) On 2015/09/07 11:12:59, Marc Treib wrote: > I think here you also need to check that the old suggestions was a popular one? > There can be multiple personal suggestions with the same host. I do that by only populating the host field for popular suggestions. I explained this while populating the field, now I have duplicated the comment to be more clear. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:565: (*result_urls)[position] = std::move((*personal_urls)[i]); On 2015/09/07 11:12:59, Marc Treib wrote: > We're not allowed to use C++11 library features yet, so no std::move. On the > upside, that also means the inputs can be const& instead of * :) Done. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.h:86: // Adds the suggestions from |popular_sites_| into |titles| and |urls|. This On 2015/09/07 11:13:00, Marc Treib wrote: > Comment isn't accurate anymore now. Done. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.h:97: // Return values. On 2015/09/07 11:13:00, Marc Treib wrote: > nit: Order of arguments should be inputs, then outputs. Done. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... File chrome/browser/android/most_visited_sites_unittest.cc (right): https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites_unittest.cc:52: // - Removing blacklisted suggestions. On 2015/09/07 11:13:00, Marc Treib wrote: > Also, blacklisting suggestions in the first place. I don't think we particularly > care about the blacklisting case though :) Acknowledged. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites_unittest.cc:55: // - Correct Host extraction from the URL. On 2015/09/07 11:13:00, Marc Treib wrote: > This is just a feature of GURL and (hopefully) tested elsewhere? > I guess you actually mean the "duplicate" elimination? No that is below. Agreed it is tested elsewhere, just specifying scope. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites_unittest.cc:56: // - Ensuring popular suggestions don't contain personal ones. On 2015/09/07 11:13:00, Marc Treib wrote: > What does this mean? This is duplicate elimination. https://codereview.chromium.org/1330773002/diff/1/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/1330773002/diff/1/chrome/common/pref_names.cc... chrome/common/pref_names.cc:1498: const char kNTPSuggestionsSource[] = "ntp.suggestions_source"; On 2015/09/07 11:13:00, Marc Treib wrote: > This is actually a list of bools, so what does "source" mean? Maybe call it > "is_personal" or something? Done. https://codereview.chromium.org/1330773002/diff/20001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1330773002/diff/20001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:103: size_t num_personal_suggestions, On 2015/09/07 11:22:58, Bernhard Bauer wrote: > This is the number of elements in |personal_*| below, right? Why do we need to > pass it in again? Yes not required. https://codereview.chromium.org/1330773002/diff/20001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:105: // Passed as ptr for move semantics only. On 2015/09/07 11:22:58, Bernhard Bauer wrote: > I don't quite understand what you mean by move semantics here. To allow > modification (which would include moving elements out of the container)? Yes. https://codereview.chromium.org/1330773002/diff/20001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:106: std::vector<std::string>* personal_urls, On 2015/09/07 11:22:58, Bernhard Bauer wrote: > Could we put all of these into a ScopedVector of some struct? Something like: > > struct Suggestion { > base::string16 title; > string url; > }; > > And then extend as necessary and pass a ScopedVector for each of personal > suggestions, popular suggestions, and output... or something. Done.
Also to clarify, I am not storing all the popular suggestions in preferences so the json file still needs to be around.
Looking much better, thanks! Some more comments below... https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:454: void MostVisitedSites::AddPopularSites( On 2015/09/08 10:48:56, knn wrote: > On 2015/09/07 11:12:59, Marc Treib wrote: > > Any reason why this doesn't just update |titles| and |urls| in-place anymore? > > Would save the caller from having to create two extra vectors. > > Did you mean inplace? I did not want to complicate the logic too much. I just meant that the caller shouldn't have to pass in extra vectors, but you've changed that anyway in the latest PS :) https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites.cc:559: if (old_sites_host[position] == personal_hosts[i]) On 2015/09/08 10:48:55, knn wrote: > On 2015/09/07 11:12:59, Marc Treib wrote: > > I think here you also need to check that the old suggestions was a popular > one? > > There can be multiple personal suggestions with the same host. > > I do that by only populating the host field for popular suggestions. I explained > this while populating the field, now I have duplicated the comment to be more > clear. Acknowledged. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... File chrome/browser/android/most_visited_sites_unittest.cc (right): https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites_unittest.cc:56: // - Ensuring popular suggestions don't contain personal ones. On 2015/09/08 10:48:56, knn wrote: > On 2015/09/07 11:13:00, Marc Treib wrote: > > What does this mean? > > This is duplicate elimination. Then please say that :) I find the current comment confusing, since of course there can be overlap between personal and popular suggestions. https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:179: std::vector<size_t> InsertMatchingSuggestions( Please add a comment explaining what this function does, in particular how it modifies its inputs (see also below about src_suggestions), and what it returns. https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:202: std::swap((*dst_suggestions)[position], (*src_suggestions)[i]); This is the only reason why src_suggestions can't be const, right? But as far as I can tell, the caller actually doesn't care if src_suggestions is modified like this, is that correct? If so, please make src_suggestions const, and copy elements over instead of swapping. It's not gonna be any less efficient, and a lot more readable. (At least to me, functions that destroy their input are much harder to understand!) https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:211: ScopedVector<MostVisitedSites::Suggestion>* src_suggestions, Same here: Please make const if possible. https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:259: return kHistogramClientName; Maybe rename these constants, since they're now used for more than just histograms? https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:574: void MostVisitedSites::MergeSuggestions( Let this just return the result, rather than passing it out through a pointer param? https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:576: ScopedVector<Suggestion>* popular_suggestions, If InsertMatchingSuggestions and InsertAllSuggestions above take const inputs, then these can also be const& instead of * :) https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:616: current_suggestions_.swap(*suggestions); This is unexpected - I wouldn't expect a Notify method to change member state. In fact, can all the (non-static) methods operate directly on current_suggestions_, and pass one less ScopedVector around? https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:73: Suggestion(base::string16 title, std::string url, MostVisitedSource source); nit: Pass strings by const& ? https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:82: std::string GetSourceString(); const :) https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... File chrome/browser/android/most_visited_sites_unittest.cc (right): https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites_unittest.cc:27: MostVisitedSites::Suggestion* GetSuggestion(bool is_personal) const { nit: Return a scoped_ptr to make ownership clear? https://codereview.chromium.org/1330773002/diff/40001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/1330773002/diff/40001/chrome/common/pref_name... chrome/common/pref_names.cc:1501: // Whether the suggestion was derieved from personal data. nit: derived
https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:202: std::swap((*dst_suggestions)[position], (*src_suggestions)[i]); We need to do a swap as ScopedVector owns the member pointers, and I don't want to copy structs around (Hence ScopedVector). I'll add a lot of comments if that helps? https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:259: return kHistogramClientName; On 2015/09/08 11:39:49, Marc Treib wrote: > Maybe rename these constants, since they're now used for more than just > histograms? They are still used only in Histograms, I'll rename the function.
https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:202: std::swap((*dst_suggestions)[position], (*src_suggestions)[i]); On 2015/09/08 11:53:52, knn wrote: > We need to do a swap as ScopedVector owns the member pointers, and I don't want > to copy structs around (Hence ScopedVector). > I'll add a lot of comments if that helps? Hm. TBH I'd prefer just copying the structs (readability over performance, unless performance actually turns out to be problematic). But since Bernhard explicitly suggested ScopedVectors... oh well. Can you at least null out the corresponding entry in src_suggestions after the swap? (Maybe make a MoveElement helper function.) I think that'd make things a bit less confusing for me. (Would it also allow you to get rid of the vector<size_t>?)
On 2015/09/08 12:07:57, Marc Treib wrote: > https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... > File chrome/browser/android/most_visited_sites.cc (right): > > https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... > chrome/browser/android/most_visited_sites.cc:202: > std::swap((*dst_suggestions)[position], (*src_suggestions)[i]); > On 2015/09/08 11:53:52, knn wrote: > > We need to do a swap as ScopedVector owns the member pointers, and I don't > want > > to copy structs around (Hence ScopedVector). > > I'll add a lot of comments if that helps? > > Hm. TBH I'd prefer just copying the structs (readability over performance, > unless performance actually turns out to be problematic). But since Bernhard > explicitly suggested ScopedVectors... oh well. > Can you at least null out the corresponding entry in src_suggestions after the > swap? (Maybe make a MoveElement helper function.) I think that'd make things a > bit less confusing for me. (Would it also allow you to get rid of the > vector<size_t>?) It is nulled out already std::swap is the MoveHelper I am using. I am using the vector<size_t> to get the insert positions without checking for them, would you rather I have 2 loops instead?
On 2015/09/08 12:24:53, knn wrote: > On 2015/09/08 12:07:57, Marc Treib wrote: > > > https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... > > File chrome/browser/android/most_visited_sites.cc (right): > > > > > https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... > > chrome/browser/android/most_visited_sites.cc:202: > > std::swap((*dst_suggestions)[position], (*src_suggestions)[i]); > > On 2015/09/08 11:53:52, knn wrote: > > > We need to do a swap as ScopedVector owns the member pointers, and I don't > > want > > > to copy structs around (Hence ScopedVector). > > > I'll add a lot of comments if that helps? > > > > Hm. TBH I'd prefer just copying the structs (readability over performance, > > unless performance actually turns out to be problematic). But since Bernhard > > explicitly suggested ScopedVectors... oh well. > > Can you at least null out the corresponding entry in src_suggestions after the > > swap? (Maybe make a MoveElement helper function.) I think that'd make things a > > bit less confusing for me. (Would it also allow you to get rid of the > > vector<size_t>?) > > It is nulled out already std::swap is the MoveHelper I am using. Ah, right, because the dst_suggestions entry is null before the swap. I missed that, sorry. Maybe a comment would help, something like "move suggestion from src to dst (note that the dst entry is null before)"? > I am using the vector<size_t> to get the insert positions without checking for > them, would you rather I have 2 loops instead? I think that'd be simpler, yes. But I'll leave it up to you, I certainly won't block the CL over this :)
Thanks for the review so far! PTAL. https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... File chrome/browser/android/most_visited_sites_unittest.cc (right): https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites_unittest.cc:56: // - Ensuring popular suggestions don't contain personal ones. On 2015/09/08 11:39:49, Marc Treib wrote: > On 2015/09/08 10:48:56, knn wrote: > > On 2015/09/07 11:13:00, Marc Treib wrote: > > > What does this mean? > > > > This is duplicate elimination. > > Then please say that :) > I find the current comment confusing, since of course there can be overlap > between personal and popular suggestions. There shouldn't be overlap between the two right? I have rephrased it. https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:179: std::vector<size_t> InsertMatchingSuggestions( On 2015/09/08 11:39:49, Marc Treib wrote: > Please add a comment explaining what this function does, in particular how it > modifies its inputs (see also below about src_suggestions), and what it returns. Done. https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:574: void MostVisitedSites::MergeSuggestions( On 2015/09/08 11:39:49, Marc Treib wrote: > Let this just return the result, rather than passing it out through a pointer > param? ScopedVector is move only. But actually there is very little change either way since the actual work is done in Insert{Matching/All}Suggestions using pointers. https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:616: current_suggestions_.swap(*suggestions); On 2015/09/08 11:39:49, Marc Treib wrote: > This is unexpected - I wouldn't expect a Notify method to change member state. > In fact, can all the (non-static) methods operate directly on > current_suggestions_, and pass one less ScopedVector around? Done. https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:73: Suggestion(base::string16 title, std::string url, MostVisitedSource source); On 2015/09/08 11:39:49, Marc Treib wrote: > nit: Pass strings by const& ? Done. https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:82: std::string GetSourceString(); On 2015/09/08 11:39:49, Marc Treib wrote: > const :) Done. https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... File chrome/browser/android/most_visited_sites_unittest.cc (right): https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites_unittest.cc:27: MostVisitedSites::Suggestion* GetSuggestion(bool is_personal) const { On 2015/09/08 11:39:49, Marc Treib wrote: > nit: Return a scoped_ptr to make ownership clear? Done. https://codereview.chromium.org/1330773002/diff/40001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/1330773002/diff/40001/chrome/common/pref_name... chrome/common/pref_names.cc:1501: // Whether the suggestion was derieved from personal data. On 2015/09/08 11:39:49, Marc Treib wrote: > nit: derived Done.
https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... File chrome/browser/android/most_visited_sites_unittest.cc (right): https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites_unittest.cc:56: // - Ensuring popular suggestions don't contain personal ones. On 2015/09/08 13:42:00, knn wrote: > On 2015/09/08 11:39:49, Marc Treib wrote: > > On 2015/09/08 10:48:56, knn wrote: > > > On 2015/09/07 11:13:00, Marc Treib wrote: > > > > What does this mean? > > > > > > This is duplicate elimination. > > > > Then please say that :) > > I find the current comment confusing, since of course there can be overlap > > between personal and popular suggestions. > > There shouldn't be overlap between the two right? I have rephrased it. After MergeSuggestions, right. The comment IMO doesn't really make clear if you're talking about the "before" or "after" state. https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:574: void MostVisitedSites::MergeSuggestions( On 2015/09/08 13:42:00, knn wrote: > On 2015/09/08 11:39:49, Marc Treib wrote: > > Let this just return the result, rather than passing it out through a pointer > > param? > > ScopedVector is move only. But actually there is very little change either way > since the actual work is done in Insert{Matching/All}Suggestions using pointers. But it has a .Pass() method :) https://codereview.chromium.org/1330773002/diff/60001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1330773002/diff/60001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:186: const std::vector<std::string> match_urls, nit: Missing "&"
PTAL. Thanks! https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... File chrome/browser/android/most_visited_sites_unittest.cc (right): https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites_unittest.cc:56: // - Ensuring popular suggestions don't contain personal ones. On 2015/09/08 13:56:55, Marc Treib wrote: > On 2015/09/08 13:42:00, knn wrote: > > On 2015/09/08 11:39:49, Marc Treib wrote: > > > On 2015/09/08 10:48:56, knn wrote: > > > > On 2015/09/07 11:13:00, Marc Treib wrote: > > > > > What does this mean? > > > > > > > > This is duplicate elimination. > > > > > > Then please say that :) > > > I find the current comment confusing, since of course there can be overlap > > > between personal and popular suggestions. > > > > There shouldn't be overlap between the two right? I have rephrased it. > > After MergeSuggestions, right. The comment IMO doesn't really make clear if > you're talking about the "before" or "after" state. I'm not sure I understand, there is no "after" for personal and popular suggestions. Can you read my rephrased comment and suggest changes? https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1330773002/diff/40001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:574: void MostVisitedSites::MergeSuggestions( On 2015/09/08 13:56:55, Marc Treib wrote: > On 2015/09/08 13:42:00, knn wrote: > > On 2015/09/08 11:39:49, Marc Treib wrote: > > > Let this just return the result, rather than passing it out through a > pointer > > > param? > > > > ScopedVector is move only. But actually there is very little change either way > > since the actual work is done in Insert{Matching/All}Suggestions using > pointers. > > But it has a .Pass() method :) My bad missed that. https://codereview.chromium.org/1330773002/diff/60001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1330773002/diff/60001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:186: const std::vector<std::string> match_urls, On 2015/09/08 13:56:55, Marc Treib wrote: > nit: Missing "&" Oops! Thanks.
https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... File chrome/browser/android/most_visited_sites_unittest.cc (right): https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites_unittest.cc:56: // - Ensuring popular suggestions don't contain personal ones. On 2015/09/08 14:17:32, knn wrote: > On 2015/09/08 13:56:55, Marc Treib wrote: > > On 2015/09/08 13:42:00, knn wrote: > > > On 2015/09/08 11:39:49, Marc Treib wrote: > > > > On 2015/09/08 10:48:56, knn wrote: > > > > > On 2015/09/07 11:13:00, Marc Treib wrote: > > > > > > What does this mean? > > > > > > > > > > This is duplicate elimination. > > > > > > > > Then please say that :) > > > > I find the current comment confusing, since of course there can be overlap > > > > between personal and popular suggestions. > > > > > > There shouldn't be overlap between the two right? I have rephrased it. > > > > After MergeSuggestions, right. The comment IMO doesn't really make clear if > > you're talking about the "before" or "after" state. > > I'm not sure I understand, there is no "after" for personal and popular > suggestions. Can you read my rephrased comment and suggest changes? Or maybe I'm missing something? What I mean is: When you call MergeSuggestions(personal_suggestions, popular_suggestions, ...) below, then of course there can be duplicates between personal_suggestions and popular_suggestions, and it's MergeSuggestions' job to filter them out, and not put any duplicates into the result. (Right?) So maybe: "Ensure that MostVisitedSites::MergeSuggestions filters out duplicated between personal and popular suggestions"?
On 2015/09/08 14:24:27, Marc Treib wrote: > https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... > File chrome/browser/android/most_visited_sites_unittest.cc (right): > > https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... > chrome/browser/android/most_visited_sites_unittest.cc:56: // - Ensuring popular > suggestions don't contain personal ones. > On 2015/09/08 14:17:32, knn wrote: > > On 2015/09/08 13:56:55, Marc Treib wrote: > > > On 2015/09/08 13:42:00, knn wrote: > > > > On 2015/09/08 11:39:49, Marc Treib wrote: > > > > > On 2015/09/08 10:48:56, knn wrote: > > > > > > On 2015/09/07 11:13:00, Marc Treib wrote: > > > > > > > What does this mean? > > > > > > > > > > > > This is duplicate elimination. > > > > > > > > > > Then please say that :) > > > > > I find the current comment confusing, since of course there can be > overlap > > > > > between personal and popular suggestions. > > > > > > > > There shouldn't be overlap between the two right? I have rephrased it. > > > > > > After MergeSuggestions, right. The comment IMO doesn't really make clear if > > > you're talking about the "before" or "after" state. > > > > I'm not sure I understand, there is no "after" for personal and popular > > suggestions. Can you read my rephrased comment and suggest changes? > > Or maybe I'm missing something? What I mean is: When you call > MergeSuggestions(personal_suggestions, popular_suggestions, ...) > below, then of course there can be duplicates between personal_suggestions and > popular_suggestions, and it's MergeSuggestions' job to filter them out, and not > put any duplicates into the result. (Right?) > So maybe: "Ensure that MostVisitedSites::MergeSuggestions filters out duplicated > between personal and popular suggestions"? Ah no, that is precisely what is out of scope here, hence the documentation.
https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:140: std::string url_string; Move this to line 149? https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:163: DCHECK_EQ(num_tiles, old_sites_source->size()); If you DCHECK this, it implies you expect the check in line 159 to succeed every time... which means you should DCHECK that value instead of silently skipping it, then DCHECKing here. https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:171: for (const auto suggestion : suggestions) { Use a const reference (otherwise you'll make a copy), and consider using the explicit type (I don't think that would make it terribly unreadable). https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:192: size_t position; Move this into the loop header? https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:215: // into dst_suggestions where ever empty starting from |start_position|. Returns Nit: |dst_suggestions| in pipes. https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:226: size_t i = start_position; Can you move this into the loop header? https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:263: provider_index(provider_index) {} DCHECK that source == SUGGESTIONS_SERVICE? https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:551: for (const auto& suggestion : *personal_suggestions) Use the actual type :) https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:637: for (const auto suggestion : current_suggestions_) { And here as well :) https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:65: struct Suggestion { Can you make this private and only forward-declare in here in the header? https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:70: // Only for source == SUGGESTIONS_SERVICE Document that it's -1 otherwise? (Or at least that it's only guaranteed to be a valid index if source == SUGGESTIONS_SERVICE) https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:83: int provider_index); Add a destructor? https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:84: std::string GetSourceHistogramName() const; Add DISALLOW_COPY_AND_ASSIGN if you are not using those methods. If you are, add a comment that copying and assignment are allowed. https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... File chrome/browser/android/most_visited_sites_unittest.cc (right): https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites_unittest.cc:60: for (const auto& site : personal_sites) Use the actual type instead of auto?
Patchset #6 (id:100001) has been deleted
PTAL. Addressed all comments. Thanks! https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:140: std::string url_string; On 2015/09/08 15:44:25, Bernhard Bauer wrote: > Move this to line 149? Done. https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:163: DCHECK_EQ(num_tiles, old_sites_source->size()); On 2015/09/08 15:44:25, Bernhard Bauer wrote: > If you DCHECK this, it implies you expect the check in line 159 to succeed every > time... which means you should DCHECK that value instead of silently skipping > it, then DCHECKing here. Done. https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:171: for (const auto suggestion : suggestions) { On 2015/09/08 15:44:25, Bernhard Bauer wrote: > Use a const reference (otherwise you'll make a copy), and consider using the > explicit type (I don't think that would make it terribly unreadable). Done. https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:192: size_t position; On 2015/09/08 15:44:25, Bernhard Bauer wrote: > Move this into the loop header? I am using the result outside the loop. https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:215: // into dst_suggestions where ever empty starting from |start_position|. Returns On 2015/09/08 15:44:25, Bernhard Bauer wrote: > Nit: |dst_suggestions| in pipes. Done. Also moved to header. https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:226: size_t i = start_position; On 2015/09/08 15:44:25, Bernhard Bauer wrote: > Can you move this into the loop header? This is used as a return value outside the loop. https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:263: provider_index(provider_index) {} On 2015/09/08 15:44:25, Bernhard Bauer wrote: > DCHECK that source == SUGGESTIONS_SERVICE? Done. https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:551: for (const auto& suggestion : *personal_suggestions) On 2015/09/08 15:44:25, Bernhard Bauer wrote: > Use the actual type :) Done. https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.cc:637: for (const auto suggestion : current_suggestions_) { On 2015/09/08 15:44:25, Bernhard Bauer wrote: > And here as well :) Done. https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:65: struct Suggestion { On 2015/09/08 15:44:25, Bernhard Bauer wrote: > Can you make this private and only forward-declare in here in the header? Made private but can't forward declare as it is used in tests. https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:70: // Only for source == SUGGESTIONS_SERVICE On 2015/09/08 15:44:25, Bernhard Bauer wrote: > Document that it's -1 otherwise? (Or at least that it's only guaranteed to be a > valid index if source == SUGGESTIONS_SERVICE) Done. https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:83: int provider_index); On 2015/09/08 15:44:25, Bernhard Bauer wrote: > Add a destructor? Done. Just curious, whether this serves any purpose other than document that I haven't forgotten? https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:84: std::string GetSourceHistogramName() const; On 2015/09/08 15:44:25, Bernhard Bauer wrote: > Add DISALLOW_COPY_AND_ASSIGN if you are not using those methods. If you are, add > a comment that copying and assignment are allowed. Done. https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... File chrome/browser/android/most_visited_sites_unittest.cc (right): https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites_unittest.cc:60: for (const auto& site : personal_sites) On 2015/09/08 15:44:25, Bernhard Bauer wrote: > Use the actual type instead of auto? Done.
https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... chrome/browser/android/most_visited_sites.h:83: int provider_index); On 2015/09/09 09:35:38, knn wrote: > On 2015/09/08 15:44:25, Bernhard Bauer wrote: > > Add a destructor? > > Done. Just curious, whether this serves any purpose other than document that I > haven't forgotten? Some trybots check that "non-trivial" structs have a destructor. I ran into that recently, see https://codereview.chromium.org/1314493013/#ps120001
Patchset #6 (id:120001) has been deleted
On 2015/09/09 09:58:42, Marc Treib wrote: > https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... > File chrome/browser/android/most_visited_sites.h (right): > > https://codereview.chromium.org/1330773002/diff/80001/chrome/browser/android/... > chrome/browser/android/most_visited_sites.h:83: int provider_index); > On 2015/09/09 09:35:38, knn wrote: > > On 2015/09/08 15:44:25, Bernhard Bauer wrote: > > > Add a destructor? > > > > Done. Just curious, whether this serves any purpose other than document that I > > haven't forgotten? > > Some trybots check that "non-trivial" structs have a destructor. I ran into that > recently, see https://codereview.chromium.org/1314493013/#ps120001 Ah, right! I forget too soon. Thanks!
Some (hopefully) last comments :) https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... File chrome/browser/android/most_visited_sites_unittest.cc (left): https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites_unittest.cc:168: TEST_F(MostVisitedSitesTest, PopularSitesComplex) { On 2015/09/07 11:13:00, Marc Treib wrote: > Why did you remove this test? I don't think your current tests cover the > overflow case in particular (i.e. more total suggestions than we have tiles). I think this hasn't been addressed yet - do we have a test that covers the overflow case, i.e. where we have more total suggestions than will fit on the page? https://codereview.chromium.org/1330773002/diff/140001/chrome/browser/android... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1330773002/diff/140001/chrome/browser/android... chrome/browser/android/most_visited_sites.h:71: std::string host; I think it'd make sense to combine |url| and |host| into a single GURL. https://codereview.chromium.org/1330773002/diff/140001/chrome/browser/android... chrome/browser/android/most_visited_sites.h:125: std::vector<bool>* old_sites_source); nit: const https://codereview.chromium.org/1330773002/diff/140001/chrome/browser/android... chrome/browser/android/most_visited_sites.h:133: static std::vector<size_t> InsertMatchingSuggestions( nit: I think you could move this and also InsertAllSuggestions out of the class, into a namespace in the .cc file.
https://codereview.chromium.org/1330773002/diff/140001/chrome/browser/android... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1330773002/diff/140001/chrome/browser/android... chrome/browser/android/most_visited_sites.cc:161: DCHECK(source == MostVisitedSites::SUGGESTIONS_SERVICE); Use DCHECK_EQ for nicer error messages.
https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... File chrome/browser/android/most_visited_sites_unittest.cc (left): https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites_unittest.cc:168: TEST_F(MostVisitedSitesTest, PopularSitesComplex) { On 2015/09/09 12:15:01, Marc Treib wrote: > On 2015/09/07 11:13:00, Marc Treib wrote: > > Why did you remove this test? I don't think your current tests cover the > > overflow case in particular (i.e. more total suggestions than we have tiles). > > I think this hasn't been addressed yet - do we have a test that covers the > overflow case, i.e. where we have more total suggestions than will fit on the > page? Oops missed this. I did not remove any particular test, I rewrote all the cases that are within the scope of the changed function. We presently make sure that we only take the required number of suggestions so this will never happen. This along with restoring the previous suggestions from preferences will have to be tested elsewhere. https://codereview.chromium.org/1330773002/diff/140001/chrome/browser/android... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1330773002/diff/140001/chrome/browser/android... chrome/browser/android/most_visited_sites.cc:161: DCHECK(source == MostVisitedSites::SUGGESTIONS_SERVICE); On 2015/09/09 12:52:15, Bernhard Bauer wrote: > Use DCHECK_EQ for nicer error messages. Done. https://codereview.chromium.org/1330773002/diff/140001/chrome/browser/android... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1330773002/diff/140001/chrome/browser/android... chrome/browser/android/most_visited_sites.h:71: std::string host; On 2015/09/09 12:15:01, Marc Treib wrote: > I think it'd make sense to combine |url| and |host| into a single GURL. Yeah, I though GURL parses the host on fly, I stand corrected, will do. https://codereview.chromium.org/1330773002/diff/140001/chrome/browser/android... chrome/browser/android/most_visited_sites.h:125: std::vector<bool>* old_sites_source); On 2015/09/09 12:15:01, Marc Treib wrote: > nit: const Done. https://codereview.chromium.org/1330773002/diff/140001/chrome/browser/android... chrome/browser/android/most_visited_sites.h:133: static std::vector<size_t> InsertMatchingSuggestions( On 2015/09/09 12:15:01, Marc Treib wrote: > nit: I think you could move this and also InsertAllSuggestions out of the class, > into a namespace in the .cc file. Except Suggestions can't be private anymore which is why I moved it here.
LGTM % remaining nits :) https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... File chrome/browser/android/most_visited_sites_unittest.cc (left): https://codereview.chromium.org/1330773002/diff/1/chrome/browser/android/most... chrome/browser/android/most_visited_sites_unittest.cc:168: TEST_F(MostVisitedSitesTest, PopularSitesComplex) { On 2015/09/09 13:54:30, knn wrote: > On 2015/09/09 12:15:01, Marc Treib wrote: > > On 2015/09/07 11:13:00, Marc Treib wrote: > > > Why did you remove this test? I don't think your current tests cover the > > > overflow case in particular (i.e. more total suggestions than we have > tiles). > > > > I think this hasn't been addressed yet - do we have a test that covers the > > overflow case, i.e. where we have more total suggestions than will fit on the > > page? > > Oops missed this. > I did not remove any particular test, I rewrote all the cases that are within > the scope of the changed function. We presently make sure that we only take the > required number of suggestions so this will never happen. > This along with restoring the previous suggestions from preferences will have to > be tested elsewhere. Ah, right. Okay then, I guess. https://codereview.chromium.org/1330773002/diff/140001/chrome/browser/android... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/1330773002/diff/140001/chrome/browser/android... chrome/browser/android/most_visited_sites.h:125: std::vector<bool>* old_sites_source); On 2015/09/09 13:54:31, knn wrote: > On 2015/09/09 12:15:01, Marc Treib wrote: > > nit: const > > Done. I meant the method itself should be const. Making the pointers const doesn't actually do anything :) https://codereview.chromium.org/1330773002/diff/140001/chrome/browser/android... chrome/browser/android/most_visited_sites.h:133: static std::vector<size_t> InsertMatchingSuggestions( On 2015/09/09 13:54:31, knn wrote: > On 2015/09/09 12:15:01, Marc Treib wrote: > > nit: I think you could move this and also InsertAllSuggestions out of the > class, > > into a namespace in the .cc file. > > Except Suggestions can't be private anymore which is why I moved it here. Ah, right, missed that, sorry. https://codereview.chromium.org/1330773002/diff/140001/chrome/browser/android... chrome/browser/android/most_visited_sites.h:134: ScopedVector<MostVisitedSites::Suggestion>* src_suggestions, nit: "MostVisitedSites::" is not necessary anymore then; also below.
LGTM with Marc's nits and one additional one from me addressed. https://codereview.chromium.org/1330773002/diff/180001/chrome/browser/android... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1330773002/diff/180001/chrome/browser/android... chrome/browser/android/most_visited_sites.cc:152: DCHECK_EQ(source, MostVisitedSites::SUGGESTIONS_SERVICE); And put the expected value first (sorry, should have mentioned that straight away).
Done. Thanks for the review! https://codereview.chromium.org/1330773002/diff/180001/chrome/browser/android... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/1330773002/diff/180001/chrome/browser/android... chrome/browser/android/most_visited_sites.cc:152: DCHECK_EQ(source, MostVisitedSites::SUGGESTIONS_SERVICE); On 2015/09/09 14:48:56, Bernhard Bauer wrote: > And put the expected value first (sorry, should have mentioned that straight > away). Done.
The CQ bit was checked by knn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/1330773002/#ps200001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1330773002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1330773002/200001
Message was sent while issue was closed.
Committed patchset #9 (id:200001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/cbbd754d90f22a594d5c10c2f6c2e447cfabb5b5 Cr-Commit-Position: refs/heads/master@{#347938}
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/cbbd754d90f22a594d5c10c2f6c2e447cfabb5b5 Cr-Commit-Position: refs/heads/master@{#347938} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
