Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2292003002:
Move OnURLsDeleted from NTPSnippetsService to ContentSuggestionsService (Closed)
DescriptionMove OnURLsDeleted from NTPSnippetsService to ContentSuggestionsService
This CL makes ContentSuggestionsService an observer of HistoryService,
so that all providers can be notified about deleted URLs.
The call is redirected to ClearHistory().
NTPSnippetsService's HistoryService observer code is replaced with
the notification above.
BUG=643608
Committed: https://crrev.com/45941154e2a8e0766210a65d2c12bf40a2c95d9a
Cr-Commit-Position: refs/heads/master@{#416518}
Patch Set 1 #
Total comments: 8
Patch Set 2 #
Total comments: 15
Patch Set 3 : Marc's comments. #
Total comments: 10
Patch Set 4 : Marc's comments. #
Total comments: 4
Patch Set 5 : Marc's nits. #Messages
Total messages: 29 (12 generated)
vitaliii@chromium.org changed reviewers: + treib@chromium.org
Hi Marc, Would you mind checking the interface before I edit all the providers? https://codereview.chromium.org/2292003002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2292003002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:111: const history::URLRows& deleted_rows) = 0; Marc, would you mind checking the interface? I ignore |std::set<GURL>& favicon_urls|, because 1) we do not need them; 2) set of URLs does not look useful.
https://codereview.chromium.org/2292003002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2292003002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:105: // Removes given list of URLs and associated data (depends on a provider). Removes from where? s/a provider/the provider/ https://codereview.chromium.org/2292003002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:108: // then the deletion is due to expiration and providers may ignore it. Would any provider ever care about expired URLs? If not, let's just remove the parameter, and have the service not forward those events. https://codereview.chromium.org/2292003002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:111: const history::URLRows& deleted_rows) = 0; On 2016/08/30 08:57:31, vitaliii wrote: > Marc, would you mind checking the interface? > I ignore |std::set<GURL>& favicon_urls|, because > 1) we do not need them; > 2) set of URLs does not look useful. Yup, this sounds reasonable. https://codereview.chromium.org/2292003002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:111: const history::URLRows& deleted_rows) = 0; It might be nice not to use history:: types here, otherwise every provider has to include some history header. Do we need anything other than the actual URLs?
Description was changed from ========== Move OnURLsDeleted from NTPSnippetsService to ContentSuggestionsService This CL makes ContentSuggestionsService an observer of HistoryService, so that all providers can be notified about deleted URLs. NTPSnippetsService's HistoryService observer code is replaced with the notification above. BUG=641321,640972 ========== to ========== Move OnURLsDeleted from NTPSnippetsService to ContentSuggestionsService This CL makes ContentSuggestionsService an observer of HistoryService, so that all providers can be notified about deleted URLs. NTPSnippetsService's HistoryService observer code is replaced with the notification above. BUG=641321 ==========
Description was changed from ========== Move OnURLsDeleted from NTPSnippetsService to ContentSuggestionsService This CL makes ContentSuggestionsService an observer of HistoryService, so that all providers can be notified about deleted URLs. NTPSnippetsService's HistoryService observer code is replaced with the notification above. BUG=641321 ========== to ========== Move OnURLsDeleted from NTPSnippetsService to ContentSuggestionsService This CL makes ContentSuggestionsService an observer of HistoryService, so that all providers can be notified about deleted URLs. NTPSnippetsService's HistoryService observer code is replaced with the notification above. The providers reactions are implemented as well. BUG=643608 ==========
Patchset #2 (id:20001) has been deleted
Hi Marc, your attention is required. https://codereview.chromium.org/2292003002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2292003002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:105: // Removes given list of URLs and associated data (depends on a provider). On 2016/08/30 09:42:32, Marc Treib wrote: > Removes from where? > s/a provider/the provider/ Done. https://codereview.chromium.org/2292003002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:108: // then the deletion is due to expiration and providers may ignore it. On 2016/08/30 09:42:32, Marc Treib wrote: > Would any provider ever care about expired URLs? If not, let's just remove the > parameter, and have the service not forward those events. Done. https://codereview.chromium.org/2292003002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:111: const history::URLRows& deleted_rows) = 0; On 2016/08/30 09:42:32, Marc Treib wrote: > It might be nice not to use history:: types here, otherwise every provider has > to include some history header. Do we need anything other than the actual URLs? Done. https://codereview.chromium.org/2292003002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2292003002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:105: // provider) from all providers. Providers should immediately clear any I am not sure about the wording. Should it be "from all providers' internal storage"?
https://codereview.chromium.org/2292003002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (left): https://codereview.chromium.org/2292003002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:129: HistoryService* history_service, You'll have to do the same change in the ios factory. https://codereview.chromium.org/2292003002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2292003002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:105: // provider) from all providers. Providers should immediately clear any On 2016/09/02 12:38:30, vitaliii wrote: > I am not sure about the wording. > Should it be "from all providers' internal storage"? See below - if possible, I'd like to merge this with ClearHistory below, since it's essentially the same thing: "something got deleted from history, do whatever you have to do in response to that". Or am I missing something? If these two actually are significantly different, then the comments need to make very clear what the difference is, and what is expected of a provider in either case. https://codereview.chromium.org/2292003002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:108: virtual void RemoveURLsFromHistory(bool all_history, Hm, with all_history==true, isn't this essentially the same as a ClearHistory call with appropriate begin/end parameters? Can we maybe map one to the other? (e.g. the service could construct an appropriate filter function from deleted_urls, and pass that to ClearHistory). I'd prefer if we didn't have multiple history-clearing-related methods on this interface. https://codereview.chromium.org/2292003002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2292003002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:246: if (expired) Add a comment, like we had in NTPSnippetsService? https://codereview.chromium.org/2292003002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:251: deleted_urls.emplace_back(row.url()); nit: push_back, since it has to make a copy anyway. https://codereview.chromium.org/2292003002/diff/40001/components/ntp_snippets... File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc (right): https://codereview.chromium.org/2292003002/diff/40001/components/ntp_snippets... components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:156: // Ignored. Even if all_history is true?
Description was changed from ========== Move OnURLsDeleted from NTPSnippetsService to ContentSuggestionsService This CL makes ContentSuggestionsService an observer of HistoryService, so that all providers can be notified about deleted URLs. NTPSnippetsService's HistoryService observer code is replaced with the notification above. The providers reactions are implemented as well. BUG=643608 ========== to ========== Move OnURLsDeleted from NTPSnippetsService to ContentSuggestionsService This CL makes ContentSuggestionsService an observer of HistoryService, so that all providers can be notified about deleted URLs. The call is redirected to ClearHistory(). NTPSnippetsService's HistoryService observer code is replaced with the notification above. BUG=643608 ==========
Hey Marc, Please have a look at my fix. https://codereview.chromium.org/2292003002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (left): https://codereview.chromium.org/2292003002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:129: HistoryService* history_service, On 2016/09/02 12:57:10, Marc Treib wrote: > You'll have to do the same change in the ios factory. Should it be in different CL? https://codereview.chromium.org/2292003002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2292003002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:105: // provider) from all providers. Providers should immediately clear any On 2016/09/02 12:57:10, Marc Treib wrote: > On 2016/09/02 12:38:30, vitaliii wrote: > > I am not sure about the wording. > > Should it be "from all providers' internal storage"? > > See below - if possible, I'd like to merge this with ClearHistory below, since > it's essentially the same thing: "something got deleted from history, do > whatever you have to do in response to that". > Or am I missing something? If these two actually are significantly different, > then the comments need to make very clear what the difference is, and what is > expected of a provider in either case. Done. https://codereview.chromium.org/2292003002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:108: virtual void RemoveURLsFromHistory(bool all_history, On 2016/09/02 12:57:10, Marc Treib wrote: > Hm, with all_history==true, isn't this essentially the same as a ClearHistory > call with appropriate begin/end parameters? Can we maybe map one to the other? > (e.g. the service could construct an appropriate filter function from > deleted_urls, and pass that to ClearHistory). > I'd prefer if we didn't have multiple history-clearing-related methods on this > interface. Done. https://codereview.chromium.org/2292003002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2292003002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:246: if (expired) On 2016/09/02 12:57:10, Marc Treib wrote: > Add a comment, like we had in NTPSnippetsService? Done. https://codereview.chromium.org/2292003002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:251: deleted_urls.emplace_back(row.url()); On 2016/09/02 12:57:10, Marc Treib wrote: > nit: push_back, since it has to make a copy anyway. Acknowledged. https://codereview.chromium.org/2292003002/diff/40001/components/ntp_snippets... File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc (right): https://codereview.chromium.org/2292003002/diff/40001/components/ntp_snippets... components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:156: // Ignored. On 2016/09/02 12:57:10, Marc Treib wrote: > Even if all_history is true? Acknowledged.
https://codereview.chromium.org/2292003002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (left): https://codereview.chromium.org/2292003002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:129: HistoryService* history_service, On 2016/09/02 14:13:16, vitaliii wrote: > On 2016/09/02 12:57:10, Marc Treib wrote: > > You'll have to do the same change in the ios factory. > > Should it be in different CL? No, same CL. As it is, this won't build on iOS. https://codereview.chromium.org/2292003002/diff/60001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2292003002/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:256: auto set_contains = [](const std::set<GURL>& set, const GURL& url) { Just inline these two definitions into the respective Binds below? Or at least move their definitions closer to where they're used. https://codereview.chromium.org/2292003002/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:257: return set.count(url) != 0 ? true : false; remove the " ? true : false", it doesn't do anything https://codereview.chromium.org/2292003002/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:266: else { No else after return. Or, if you initialize begin/end to suitable values below, you might not need the early exit at all. https://codereview.chromium.org/2292003002/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:281: for (const auto& provider : providers_) { Just call this class's ClearHistory
Hi Marc, please have a look. https://codereview.chromium.org/2292003002/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (left): https://codereview.chromium.org/2292003002/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:129: HistoryService* history_service, On 2016/09/02 14:45:19, Marc Treib wrote: > On 2016/09/02 14:13:16, vitaliii wrote: > > On 2016/09/02 12:57:10, Marc Treib wrote: > > > You'll have to do the same change in the ios factory. > > > > Should it be in different CL? > > No, same CL. As it is, this won't build on iOS. Done. https://codereview.chromium.org/2292003002/diff/60001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2292003002/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:256: auto set_contains = [](const std::set<GURL>& set, const GURL& url) { On 2016/09/02 14:45:19, Marc Treib wrote: > Just inline these two definitions into the respective Binds below? > Or at least move their definitions closer to where they're used. Done. https://codereview.chromium.org/2292003002/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:257: return set.count(url) != 0 ? true : false; On 2016/09/02 14:45:19, Marc Treib wrote: > remove the " ? true : false", it doesn't do anything |count| returns |unsigned int|. https://codereview.chromium.org/2292003002/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:266: else { On 2016/09/02 14:45:19, Marc Treib wrote: > No else after return. > Or, if you initialize begin/end to suitable values below, you might not need the > early exit at all. Done. https://codereview.chromium.org/2292003002/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:281: for (const auto& provider : providers_) { On 2016/09/02 14:45:19, Marc Treib wrote: > Just call this class's ClearHistory Done.
LGTM with some more nits https://codereview.chromium.org/2292003002/diff/60001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2292003002/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:257: return set.count(url) != 0 ? true : false; On 2016/09/02 16:40:06, vitaliii wrote: > On 2016/09/02 14:45:19, Marc Treib wrote: > > remove the " ? true : false", it doesn't do anything > |count| returns |unsigned int|. Yes, you can keep the "!= 0" (that's nicer than the static_cast you have now), just the arithmetic if after that was pointless. https://codereview.chromium.org/2292003002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2292003002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:253: base::Time begin = base::Time::UnixEpoch(); base::Time() (default ctor) might be better. https://codereview.chromium.org/2292003002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:254: base::Time end = base::Time::Now(); And base::Time::Max() here.
Just a fix, no need to look. https://codereview.chromium.org/2292003002/diff/60001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2292003002/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:257: return set.count(url) != 0 ? true : false; On 2016/09/02 16:50:35, Marc Treib wrote: > On 2016/09/02 16:40:06, vitaliii wrote: > > On 2016/09/02 14:45:19, Marc Treib wrote: > > > remove the " ? true : false", it doesn't do anything > > |count| returns |unsigned int|. > > Yes, you can keep the "!= 0" (that's nicer than the static_cast you have now), > just the arithmetic if after that was pointless. Done. https://codereview.chromium.org/2292003002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2292003002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:253: base::Time begin = base::Time::UnixEpoch(); On 2016/09/02 16:50:35, Marc Treib wrote: > base::Time() (default ctor) might be better. Done. https://codereview.chromium.org/2292003002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:254: base::Time end = base::Time::Now(); On 2016/09/02 16:50:35, Marc Treib wrote: > And base::Time::Max() here. Done.
vitaliii@chromium.org changed reviewers: + noyau@chromium.org
Hi Eric, We needed to react to OnURLsDeleted in BookmarkSuggestionsProvider, so we moved it to ContentSuggestionsService. This means that we need to change NTPSnippetsService constructor, which leads to iOS code change and that is why your OWNER approval is needed.
lgtm On Monday, 5 September 2016, <vitaliii@chromium.org> wrote: > Hi Eric, > > We needed to react to OnURLsDeleted in BookmarkSuggestionsProvider, so we > moved > it to ContentSuggestionsService. This means that we need to change > NTPSnippetsService constructor, which leads to iOS code change and that is > why > your OWNER approval is needed. > > > > https://codereview.chromium.org/2292003002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by vitaliii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2292003002/#ps100001 (title: "Marc's nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by noyau@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Move OnURLsDeleted from NTPSnippetsService to ContentSuggestionsService This CL makes ContentSuggestionsService an observer of HistoryService, so that all providers can be notified about deleted URLs. The call is redirected to ClearHistory(). NTPSnippetsService's HistoryService observer code is replaced with the notification above. BUG=643608 ========== to ========== Move OnURLsDeleted from NTPSnippetsService to ContentSuggestionsService This CL makes ContentSuggestionsService an observer of HistoryService, so that all providers can be notified about deleted URLs. The call is redirected to ClearHistory(). NTPSnippetsService's HistoryService observer code is replaced with the notification above. BUG=643608 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Move OnURLsDeleted from NTPSnippetsService to ContentSuggestionsService This CL makes ContentSuggestionsService an observer of HistoryService, so that all providers can be notified about deleted URLs. The call is redirected to ClearHistory(). NTPSnippetsService's HistoryService observer code is replaced with the notification above. BUG=643608 ========== to ========== Move OnURLsDeleted from NTPSnippetsService to ContentSuggestionsService This CL makes ContentSuggestionsService an observer of HistoryService, so that all providers can be notified about deleted URLs. The call is redirected to ClearHistory(). NTPSnippetsService's HistoryService observer code is replaced with the notification above. BUG=643608 Committed: https://crrev.com/45941154e2a8e0766210a65d2c12bf40a2c95d9a Cr-Commit-Position: refs/heads/master@{#416518} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/45941154e2a8e0766210a65d2c12bf40a2c95d9a Cr-Commit-Position: refs/heads/master@{#416518} |
