Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
298703009:
SuggestionsService blacklist handling. (Closed)
|
Created:
6 years, 7 months ago by manzagop (departed) Modified:
6 years, 6 months ago CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, dhollowa+watch_chromium.org, dougw+watch_chromium.org, donnd+watch_chromium.org, dominich, jfweitz+watch_chromium.org, David Black, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionSuggestionsService blacklist handling.
- MostVisitedSites keeps track of which source the sites come from
- Blacklist requests are sent to the source corresponding to the current
suggestions
- SuggestionsService now handles blacklist requests
BUG=None
TEST=SuggestionsService
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274284
Patch Set 1 #
Total comments: 16
Patch Set 2 : Address first round of comments. Avoid QueryMostVisited loop. #
Total comments: 20
Patch Set 3 : Address Jered and Ted's first round of comments. #Patch Set 4 : Touchups for Ted's second round (if -> DCHECK in MostVisitedSites::BlacklistUrl). #Patch Set 5 : Sync / rebase. #
Total comments: 4
Patch Set 6 : Rename AddBlacklistedURL to BlacklistURL. #Patch Set 7 : Missed renaming. #
Messages
Total messages: 15 (0 generated)
Hey Math, Here's the discussed change! Thanks, Pierre
See note about GURL. https://codereview.chromium.org/298703009/diff/1/chrome/browser/android/most_... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/298703009/diff/1/chrome/browser/android/most_... chrome/browser/android/most_visited_sites.cc:187: GURL url(url_string); I guess you could move this in top_sites->AddBlacklistURL as before https://codereview.chromium.org/298703009/diff/1/chrome/browser/android/most_... chrome/browser/android/most_visited_sites.cc:201: if (suggestions_service) { Aren't we certain at this point that the service exists, since it put the suggestions there in the first place? It won't become null within the same session. https://codereview.chromium.org/298703009/diff/1/chrome/browser/android/most_... chrome/browser/android/most_visited_sites.cc:203: url_string, This should probably take a GURL as well. https://codereview.chromium.org/298703009/diff/1/chrome/browser/android/most_... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/298703009/diff/1/chrome/browser/android/most_... chrome/browser/android/most_visited_sites.h:78: // The source of the most visited sites. *Most Visited https://codereview.chromium.org/298703009/diff/1/chrome/browser/search/sugges... File chrome/browser/search/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/298703009/diff/1/chrome/browser/search/sugges... chrome/browser/search/suggestions/suggestions_service.cc:126: // issueing a blacklist request. *issuing https://codereview.chromium.org/298703009/diff/1/chrome/browser/search/sugges... File chrome/browser/search/suggestions/suggestions_service_unittest.cc (right): https://codereview.chromium.org/298703009/diff/1/chrome/browser/search/sugges... chrome/browser/search/suggestions/suggestions_service_unittest.cc:244: LOG(INFO) << expected_url; fix https://codereview.chromium.org/298703009/diff/1/chrome/browser/search/sugges... chrome/browser/search/suggestions/suggestions_service_unittest.cc:258: // (Testing only) wait until suggestion fetch is complete. blacklist request https://codereview.chromium.org/298703009/diff/1/chrome/browser/search/sugges... chrome/browser/search/suggestions/suggestions_service_unittest.cc:261: // Ensure that CheckSuggestionsData() ran twice. Once?
Addressed comments. Also only calling QueryMostVisitedURLs from Observe when the currently displayed recommendations are from TopSites (this fixes a pre-existing issue with the code). https://codereview.chromium.org/298703009/diff/1/chrome/browser/android/most_... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/298703009/diff/1/chrome/browser/android/most_... chrome/browser/android/most_visited_sites.cc:187: GURL url(url_string); On 2014/05/22 21:50:49, Mathieu Perreault wrote: > I guess you could move this in top_sites->AddBlacklistURL as before Done. https://codereview.chromium.org/298703009/diff/1/chrome/browser/android/most_... chrome/browser/android/most_visited_sites.cc:201: if (suggestions_service) { On 2014/05/22 21:50:49, Mathieu Perreault wrote: > Aren't we certain at this point that the service exists, since it put the > suggestions there in the first place? It won't become null within the same > session. If services can't be turned off, then yes. Still, checking doesn't seem to hurt. Call it defensive? https://codereview.chromium.org/298703009/diff/1/chrome/browser/android/most_... chrome/browser/android/most_visited_sites.cc:203: url_string, On 2014/05/22 21:50:49, Mathieu Perreault wrote: > This should probably take a GURL as well. Done. https://codereview.chromium.org/298703009/diff/1/chrome/browser/android/most_... File chrome/browser/android/most_visited_sites.h (right): https://codereview.chromium.org/298703009/diff/1/chrome/browser/android/most_... chrome/browser/android/most_visited_sites.h:78: // The source of the most visited sites. On 2014/05/22 21:50:49, Mathieu Perreault wrote: > *Most Visited Done. https://codereview.chromium.org/298703009/diff/1/chrome/browser/search/sugges... File chrome/browser/search/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/298703009/diff/1/chrome/browser/search/sugges... chrome/browser/search/suggestions/suggestions_service.cc:126: // issueing a blacklist request. On 2014/05/22 21:50:49, Mathieu Perreault wrote: > *issuing Done. https://codereview.chromium.org/298703009/diff/1/chrome/browser/search/sugges... File chrome/browser/search/suggestions/suggestions_service_unittest.cc (right): https://codereview.chromium.org/298703009/diff/1/chrome/browser/search/sugges... chrome/browser/search/suggestions/suggestions_service_unittest.cc:244: LOG(INFO) << expected_url; On 2014/05/22 21:50:49, Mathieu Perreault wrote: > fix Done. https://codereview.chromium.org/298703009/diff/1/chrome/browser/search/sugges... chrome/browser/search/suggestions/suggestions_service_unittest.cc:258: // (Testing only) wait until suggestion fetch is complete. On 2014/05/22 21:50:49, Mathieu Perreault wrote: > blacklist request Done. https://codereview.chromium.org/298703009/diff/1/chrome/browser/search/sugges... chrome/browser/search/suggestions/suggestions_service_unittest.cc:261: // Ensure that CheckSuggestionsData() ran twice. On 2014/05/22 21:50:49, Mathieu Perreault wrote: > Once? Done.
lgtm but you'll have to ask others :) jered: search/ ted: most_visited_sites
Thanks, will wait for Ted and Jered's lgtm.
https://codereview.chromium.org/298703009/diff/10001/chrome/browser/search/su... File chrome/browser/search/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/298703009/diff/10001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.cc:97: if (pending_request_.get()) { What if the pending request is a blacklist request? https://codereview.chromium.org/298703009/diff/10001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.cc:134: pending_request_.reset(CreateSuggestionsRequest(url)); nit: To me this makes more sense as a PUT request than a GET request. https://codereview.chromium.org/298703009/diff/10001/chrome/browser/search/su... File chrome/browser/search/suggestions/suggestions_service.h (right): https://codereview.chromium.org/298703009/diff/10001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.h:51: // in-flight, the new blacklist request is ignored. nit: omit the - between in and flight
https://codereview.chromium.org/298703009/diff/10001/chrome/browser/android/m... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/298703009/diff/10001/chrome/browser/android/m... chrome/browser/android/most_visited_sites.cc:152: history::TopSites* top_sites = profile_->GetTopSites(); should we not be doing this block if we are using the suggestions service? Since we check the source in the observer, it seems like we shouldn't be registering this block at all. Also, as these start diverging more based on source, I think we should consider splitting these bits out of this class and have the source be something this delegates to. https://codereview.chromium.org/298703009/diff/10001/chrome/browser/android/m... chrome/browser/android/most_visited_sites.cc:191: if (top_sites) { braces aren't needed...and should we be dchecking the existence of top_sites and suggestion_service below as you shouldn't have been able to fetch any data without these being valid. https://codereview.chromium.org/298703009/diff/10001/chrome/browser/android/m... chrome/browser/android/most_visited_sites.cc:204: &MostVisitedSites::OnSuggestionsProfileAvailable, +2 indent on these lines https://codereview.chromium.org/298703009/diff/10001/chrome/browser/android/m... chrome/browser/android/most_visited_sites.cc:231: SuggestionsServiceFactory::GetForProfile(profile_); The data source seems to be entirely determined off the existence of this suggestions_service. Will this service exist during construction of the most visited sites object? Just wondering if we should just set the source in the constructor instead of in the callbacks. https://codereview.chromium.org/298703009/diff/10001/chrome/browser/android/m... chrome/browser/android/most_visited_sites.cc:266: mv_source_ = TOP_SITES; if you set the source in the constructor, then this method can move back to the empty namespace
Thanks for the review (and appologies for taking so long to address). Please have another look. https://codereview.chromium.org/298703009/diff/10001/chrome/browser/android/m... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/298703009/diff/10001/chrome/browser/android/m... chrome/browser/android/most_visited_sites.cc:152: history::TopSites* top_sites = profile_->GetTopSites(); On 2014/05/27 23:59:09, Ted C wrote: > should we not be doing this block if we are using the suggestions service? > > Since we check the source in the observer, it seems like we shouldn't be > registering this block at all. The TopSites notification actually does get used in the fallback scenario: if the suggestion service can't get recommendations, it calls OnSuggestionsProfileAvailable with an empty profile, which triggers a InitiateTopSitesQuery which gets TopSites urls to show and changes the source to TOP_SITES. At this point, the user might blacklist something and we rely on the notification. Though we could register / unregister when we change the source. Does that seem like a good idea? > Also, as these start diverging more based on source, I think we should consider > splitting these bits out of this class and have the source be something this > delegates to. I agree that what we have now is messy. Would you be ok leaving that for a subsequent CL? I don't think the current cl makes things worse, and separating the blacklist change from the refactoring may be a good idea. https://codereview.chromium.org/298703009/diff/10001/chrome/browser/android/m... chrome/browser/android/most_visited_sites.cc:191: if (top_sites) { On 2014/05/27 23:59:09, Ted C wrote: > braces aren't needed...and should we be dchecking the existence of top_sites and > suggestion_service below as you shouldn't have been able to fetch any data > without these being valid. Removed braces. (Btw is it frowned upon in chromium? I know google c++ is fine either way.) Wrt DCHECKing instead of ifs: could top_sites / suggestion_service become null after being non-null (something shuts the service down, the browser shuts down)? https://codereview.chromium.org/298703009/diff/10001/chrome/browser/android/m... chrome/browser/android/most_visited_sites.cc:204: &MostVisitedSites::OnSuggestionsProfileAvailable, On 2014/05/27 23:59:09, Ted C wrote: > +2 indent on these lines Done. https://codereview.chromium.org/298703009/diff/10001/chrome/browser/android/m... chrome/browser/android/most_visited_sites.cc:231: SuggestionsServiceFactory::GetForProfile(profile_); On 2014/05/27 23:59:09, Ted C wrote: > The data source seems to be entirely determined off the existence of this > suggestions_service. Will this service exist during construction of the most > visited sites object? Just wondering if we should just set the source in the > constructor instead of in the callbacks. The source may change in case of fallback (eg becoming offline offline). https://codereview.chromium.org/298703009/diff/10001/chrome/browser/android/m... chrome/browser/android/most_visited_sites.cc:266: mv_source_ = TOP_SITES; On 2014/05/27 23:59:09, Ted C wrote: > if you set the source in the constructor, then this method can move back to the > empty namespace As mentioned above, this is because of the fallback case which can switch the source back to topsites. https://codereview.chromium.org/298703009/diff/10001/chrome/browser/search/su... File chrome/browser/search/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/298703009/diff/10001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.cc:97: if (pending_request_.get()) { On 2014/05/27 22:33:17, Jered wrote: > What if the pending request is a blacklist request? That's fine as the blacklist request returns updated suggestions. https://codereview.chromium.org/298703009/diff/10001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.cc:134: pending_request_.reset(CreateSuggestionsRequest(url)); On 2014/05/27 22:33:17, Jered wrote: > nit: To me this makes more sense as a PUT request than a GET request. You're quite right: this is a request with server-side side-effects. Would you be ok leaving this as a TODO, until we come up with a story for the server accepting PUT requests? https://codereview.chromium.org/298703009/diff/10001/chrome/browser/search/su... File chrome/browser/search/suggestions/suggestions_service.h (right): https://codereview.chromium.org/298703009/diff/10001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.h:51: // in-flight, the new blacklist request is ignored. On 2014/05/27 22:33:17, Jered wrote: > nit: omit the - between in and flight Done.
most_visited_sites - lgtm https://codereview.chromium.org/298703009/diff/10001/chrome/browser/android/m... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/298703009/diff/10001/chrome/browser/android/m... chrome/browser/android/most_visited_sites.cc:152: history::TopSites* top_sites = profile_->GetTopSites(); On 2014/05/29 21:40:31, manzagop wrote: > On 2014/05/27 23:59:09, Ted C wrote: > > should we not be doing this block if we are using the suggestions service? > > > > Since we check the source in the observer, it seems like we shouldn't be > > registering this block at all. > > The TopSites notification actually does get used in the fallback scenario: if > the suggestion service can't get recommendations, it calls > OnSuggestionsProfileAvailable with an empty profile, which triggers a > InitiateTopSitesQuery which gets TopSites urls to show and changes the source to > TOP_SITES. At this point, the user might blacklist something and we rely on the > notification. Though we could register / unregister when we change the source. > Does that seem like a good idea? Ah, I missed the InitiateTopSitesQuery call in the OnSuggestionsProfileAvailable function. I do think unregistering is slightly cleaner, but since you are guarding the call in the observer I don't think it really matters too much. > > > Also, as these start diverging more based on source, I think we should > consider > > splitting these bits out of this class and have the source be something this > > delegates to. > > I agree that what we have now is messy. Would you be ok leaving that for a > subsequent CL? I don't think the current cl makes things worse, and separating > the blacklist change from the refactoring may be a good idea. Follow up CL is definitely fine. https://codereview.chromium.org/298703009/diff/10001/chrome/browser/android/m... chrome/browser/android/most_visited_sites.cc:191: if (top_sites) { On 2014/05/29 21:40:31, manzagop wrote: > On 2014/05/27 23:59:09, Ted C wrote: > > braces aren't needed...and should we be dchecking the existence of top_sites > and > > suggestion_service below as you shouldn't have been able to fetch any data > > without these being valid. > > Removed braces. (Btw is it frowned upon in chromium? I know google c++ is fine > either way.) It's more about consistency. I think most places don't use braces so I generally avoid them for one liners as well, but again it's better to follow the code around it. > > Wrt DCHECKing instead of ifs: could top_sites / suggestion_service become null > after being non-null (something shuts the service down, the browser shuts down)? > My hope is that this object would be destroyed before these objects could become null. Otherwise people will think they are blacklisting a URL successfully only to find out that it didn't work because something is terribly wrong in the underbelly of chrome.
Thanks for the review Ted! Jered: friendly ping! https://codereview.chromium.org/298703009/diff/10001/chrome/browser/android/m... File chrome/browser/android/most_visited_sites.cc (right): https://codereview.chromium.org/298703009/diff/10001/chrome/browser/android/m... chrome/browser/android/most_visited_sites.cc:152: history::TopSites* top_sites = profile_->GetTopSites(); On 2014/05/30 16:37:13, Ted C wrote: > On 2014/05/29 21:40:31, manzagop wrote: > > On 2014/05/27 23:59:09, Ted C wrote: > > > should we not be doing this block if we are using the suggestions service? > > > > > > Since we check the source in the observer, it seems like we shouldn't be > > > registering this block at all. > > > > The TopSites notification actually does get used in the fallback scenario: if > > the suggestion service can't get recommendations, it calls > > OnSuggestionsProfileAvailable with an empty profile, which triggers a > > InitiateTopSitesQuery which gets TopSites urls to show and changes the source > to > > TOP_SITES. At this point, the user might blacklist something and we rely on > the > > notification. Though we could register / unregister when we change the source. > > Does that seem like a good idea? > > Ah, I missed the InitiateTopSitesQuery call in the OnSuggestionsProfileAvailable > function. > > I do think unregistering is slightly cleaner, but since you are guarding the > call in the observer I don't think it really matters too much. > > > > > Also, as these start diverging more based on source, I think we should > > consider > > > splitting these bits out of this class and have the source be something this > > > delegates to. > > > > I agree that what we have now is messy. Would you be ok leaving that for a > > subsequent CL? I don't think the current cl makes things worse, and separating > > the blacklist change from the refactoring may be a good idea. > > Follow up CL is definitely fine. Acknowledged. https://codereview.chromium.org/298703009/diff/10001/chrome/browser/android/m... chrome/browser/android/most_visited_sites.cc:191: if (top_sites) { On 2014/05/30 16:37:13, Ted C wrote: > On 2014/05/29 21:40:31, manzagop wrote: > > On 2014/05/27 23:59:09, Ted C wrote: > > > braces aren't needed...and should we be dchecking the existence of top_sites > > and > > > suggestion_service below as you shouldn't have been able to fetch any data > > > without these being valid. > > > > Removed braces. (Btw is it frowned upon in chromium? I know google c++ is fine > > either way.) > > It's more about consistency. I think most places don't use braces so I > generally avoid them for one liners as well, but again it's better to follow the > code around it. Got it. > > Wrt DCHECKing instead of ifs: could top_sites / suggestion_service become null > > after being non-null (something shuts the service down, the browser shuts > down)? > > > > My hope is that this object would be destroyed before these objects could become > null. Otherwise people will think they are blacklisting a URL successfully only > to find out that it didn't work because something is terribly wrong in the > underbelly of chrome. Sgtm. Switched to DCHECK.
lgtm for search https://codereview.chromium.org/298703009/diff/70001/chrome/browser/search/su... File chrome/browser/search/suggestions/suggestions_service.h (right): https://codereview.chromium.org/298703009/diff/70001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.h:8: #include <deque> revert? I don't see a deque in this file. https://codereview.chromium.org/298703009/diff/70001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.h:60: void AddBlacklistedURL(const GURL& candidate_url, nit: just BlacklistURL would be clearer
Thanks for the review. Will go ahead with submission. https://codereview.chromium.org/298703009/diff/70001/chrome/browser/search/su... File chrome/browser/search/suggestions/suggestions_service.h (right): https://codereview.chromium.org/298703009/diff/70001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.h:8: #include <deque> On 2014/06/02 15:27:48, Jered wrote: > revert? I don't see a deque in this file. Done. https://codereview.chromium.org/298703009/diff/70001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.h:60: void AddBlacklistedURL(const GURL& candidate_url, On 2014/06/02 15:27:48, Jered wrote: > nit: just BlacklistURL would be clearer This was to be consistent with TopSites, but agreed it's ugly. Done.
The CQ bit was checked by manzagop@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/manzagop@chromium.org/298703009/100001
Message was sent while issue was closed.
Change committed as 274284 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
