Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1972923002:
MostVisitedSites: Remove unused order-persisting code (Closed)
|
Created:
4 years, 7 months ago by Marc Treib Modified:
4 years, 7 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMostVisitedSites: Remove unused order-persisting code
plus some related cleanup
This is part 1 of N, there's more cleanup to be done here.
BUG=601734
Committed: https://crrev.com/5a59f543c995af94830d79a8ca3865d7fc19b7fa
Cr-Commit-Position: refs/heads/master@{#394389}
Patch Set 1 #
Total comments: 8
Patch Set 2 : better TODOs #
Total comments: 4
Patch Set 3 : one more TODO, and rebase #
Messages
Total messages: 25 (9 generated)
Description was changed from ========== MostVisitedSites: Remove unused order-persisting code plus some related cleanup BUG=601734 ========== to ========== MostVisitedSites: Remove unused order-persisting code plus some related cleanup This is part 1 of N, there's more cleanup to be done here. BUG=601734 ==========
treib@chromium.org changed reviewers: + sfiera@chromium.org
PTAL! (I'll probably have to rebase this after your pending changes land, but merging should be easy enough, so IMO this is already worth a look.)
https://codereview.chromium.org/1972923002/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1972923002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.cc:361: // TODO(treib): Remove both of these. What prevents us from doing that now? https://codereview.chromium.org/1972923002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.cc:575: SuggestionsPtrVector merged_suggestions = MergeSuggestions( Creating three vectors and them merging them into a fourth seems convoluted. How about changing Create{WhitelistEntryPoint,PopularSites}Suggestions() to append to personal_suggestions instead? It'll obviate a lot of the checking you do on the sizes of those vectors too.
https://codereview.chromium.org/1972923002/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1972923002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.cc:361: // TODO(treib): Remove both of these. On 2016/05/12 14:24:05, sfiera wrote: > What prevents us from doing that now? The second one is still used, for determining if we need popular sites at all. The first one: I guess nothing, except that I wasn't sure if we need "migration" code that will delete existing values for it. So I decided to just postpone the whole prefs cleanup. https://codereview.chromium.org/1972923002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.cc:575: SuggestionsPtrVector merged_suggestions = MergeSuggestions( On 2016/05/12 14:24:05, sfiera wrote: > Creating three vectors and them merging them into a fourth seems convoluted. How > about changing Create{WhitelistEntryPoint,PopularSites}Suggestions() to append > to personal_suggestions instead? It'll obviate a lot of the checking you do on > the sizes of those vectors too. The reason I left it like this is that we have some tests that call MergeSuggestions, these won't be easy to "port". OTOH, they aren't really all that useful IMO... I could also just remove them. WDYT?
https://codereview.chromium.org/1972923002/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1972923002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.cc:361: // TODO(treib): Remove both of these. On 2016/05/12 14:30:33, Marc Treib wrote: > On 2016/05/12 14:24:05, sfiera wrote: > > What prevents us from doing that now? > > The second one is still used, for determining if we need popular sites at all. > The first one: I guess nothing, except that I wasn't sure if we need "migration" > code that will delete existing values for it. So I decided to just postpone the > whole prefs cleanup. OK. It would be nice to have more of that detail in the TODO :) https://codereview.chromium.org/1972923002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.cc:575: SuggestionsPtrVector merged_suggestions = MergeSuggestions( On 2016/05/12 14:30:33, Marc Treib wrote: > On 2016/05/12 14:24:05, sfiera wrote: > > Creating three vectors and them merging them into a fourth seems convoluted. > How > > about changing Create{WhitelistEntryPoint,PopularSites}Suggestions() to append > > to personal_suggestions instead? It'll obviate a lot of the checking you do on > > the sizes of those vectors too. > > The reason I left it like this is that we have some tests that call > MergeSuggestions, these won't be easy to "port". OTOH, they aren't really all > that useful IMO... I could also just remove them. WDYT? Removing testing in this directory is a sure way to get me to dislike this CL. I agree that they are not very useful, especially now that MergeSuggestions() doesn't do anything interesting, but I'd want them to stay.
https://codereview.chromium.org/1972923002/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1972923002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.cc:361: // TODO(treib): Remove both of these. On 2016/05/12 14:44:36, sfiera wrote: > On 2016/05/12 14:30:33, Marc Treib wrote: > > On 2016/05/12 14:24:05, sfiera wrote: > > > What prevents us from doing that now? > > > > The second one is still used, for determining if we need popular sites at all. > > The first one: I guess nothing, except that I wasn't sure if we need > "migration" > > code that will delete existing values for it. So I decided to just postpone > the > > whole prefs cleanup. > > OK. It would be nice to have more of that detail in the TODO :) Done. https://codereview.chromium.org/1972923002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/most_visited_sites.cc:575: SuggestionsPtrVector merged_suggestions = MergeSuggestions( On 2016/05/12 14:44:36, sfiera wrote: > On 2016/05/12 14:30:33, Marc Treib wrote: > > On 2016/05/12 14:24:05, sfiera wrote: > > > Creating three vectors and them merging them into a fourth seems convoluted. > > How > > > about changing Create{WhitelistEntryPoint,PopularSites}Suggestions() to > append > > > to personal_suggestions instead? It'll obviate a lot of the checking you do > on > > > the sizes of those vectors too. > > > > The reason I left it like this is that we have some tests that call > > MergeSuggestions, these won't be easy to "port". OTOH, they aren't really all > > that useful IMO... I could also just remove them. WDYT? > > Removing testing in this directory is a sure way to get me to dislike this CL. I > agree that they are not very useful, especially now that MergeSuggestions() > doesn't do anything interesting, but I'd want them to stay. Alright. Well, if the MergeSuggestions tests stay, then MergeSuggestions needs to stay too.
LGTM
treib@chromium.org changed reviewers: + bauerb@chromium.org
+bauerb for approval (since sfiera isn't a committer yet)
https://codereview.chromium.org/1972923002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1972923002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:611: void MostVisitedSites::AppendSuggestions(SuggestionsPtrVector* src, Can we move this to an anonymous namespace? Heck, we could even templatize it and move it to stl_util.h or something.
https://codereview.chromium.org/1972923002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1972923002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:611: void MostVisitedSites::AppendSuggestions(SuggestionsPtrVector* src, On 2016/05/18 10:13:31, Bernhard Bauer wrote: > Can we move this to an anonymous namespace? Heck, we could even templatize it > and move it to stl_util.h or something. Moving it into stl_util.h isn't worth it IMO. We can't directly move it into an anonymous namespace, since SuggestionsPtrVector is private in MostVisitedSites. Of course, I could either make it public, or use the underlying type directly, but.. bleargh. Given that we're planning to switch from SuggestionsPtrVector to SuggestionsVector (which is public) anyway, can we leave it like this for now, and move it into an anonymous namespace after the switch to non-Ptr?
lgtm https://codereview.chromium.org/1972923002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1972923002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:611: void MostVisitedSites::AppendSuggestions(SuggestionsPtrVector* src, On 2016/05/18 10:47:59, Marc Treib wrote: > On 2016/05/18 10:13:31, Bernhard Bauer wrote: > > Can we move this to an anonymous namespace? Heck, we could even templatize it > > and move it to stl_util.h or something. > > Moving it into stl_util.h isn't worth it IMO. > > We can't directly move it into an anonymous namespace, since > SuggestionsPtrVector is private in MostVisitedSites. Of course, I could either > make it public, or use the underlying type directly, but.. bleargh. > Given that we're planning to switch from SuggestionsPtrVector to > SuggestionsVector (which is public) anyway, can we leave it like this for now, > and move it into an anonymous namespace after the switch to non-Ptr? Ok, that's the next thing I'd have suggested 😉 Add a TODO for that?
https://codereview.chromium.org/1972923002/diff/20001/chrome/browser/android/... File chrome/browser/android/ntp/most_visited_sites.cc (right): https://codereview.chromium.org/1972923002/diff/20001/chrome/browser/android/... chrome/browser/android/ntp/most_visited_sites.cc:611: void MostVisitedSites::AppendSuggestions(SuggestionsPtrVector* src, On 2016/05/18 10:59:28, Bernhard Bauer wrote: > On 2016/05/18 10:47:59, Marc Treib wrote: > > On 2016/05/18 10:13:31, Bernhard Bauer wrote: > > > Can we move this to an anonymous namespace? Heck, we could even templatize > it > > > and move it to stl_util.h or something. > > > > Moving it into stl_util.h isn't worth it IMO. > > > > We can't directly move it into an anonymous namespace, since > > SuggestionsPtrVector is private in MostVisitedSites. Of course, I could either > > make it public, or use the underlying type directly, but.. bleargh. > > Given that we're planning to switch from SuggestionsPtrVector to > > SuggestionsVector (which is public) anyway, can we leave it like this for now, > > and move it into an anonymous namespace after the switch to non-Ptr? > > Ok, that's the next thing I'd have suggested 😉 Add a TODO for that? There is already a TODO in the header for the switch to SuggestionsVector. I've also added one here, to move this into an anonymous namespace.
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1972923002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1972923002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by treib@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, sfiera@chromium.org Link to the patchset: https://codereview.chromium.org/1972923002/#ps40001 (title: "one more TODO, and rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1972923002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1972923002/40001
Message was sent while issue was closed.
Description was changed from ========== MostVisitedSites: Remove unused order-persisting code plus some related cleanup This is part 1 of N, there's more cleanup to be done here. BUG=601734 ========== to ========== MostVisitedSites: Remove unused order-persisting code plus some related cleanup This is part 1 of N, there's more cleanup to be done here. BUG=601734 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== MostVisitedSites: Remove unused order-persisting code plus some related cleanup This is part 1 of N, there's more cleanup to be done here. BUG=601734 ========== to ========== MostVisitedSites: Remove unused order-persisting code plus some related cleanup This is part 1 of N, there's more cleanup to be done here. BUG=601734 Committed: https://crrev.com/5a59f543c995af94830d79a8ca3865d7fc19b7fa Cr-Commit-Position: refs/heads/master@{#394389} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5a59f543c995af94830d79a8ca3865d7fc19b7fa Cr-Commit-Position: refs/heads/master@{#394389} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
