Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2406573002:
📰 Persist category dismissals (Closed)
Description[NTP Client] Persist category dismissals
- Persists the ids of dismissed categories to preferences as a
list of category ids
- Restores the dismissed categories from the list of dismissed
ids when the ContentSuggestionService is created, and matches
with providers when they are registered.
- Changes the way dismissals are cleared, from happening when
anything about the category changes to only happen when a
non empty list of suggestions is received.
BUG=638580
Committed: https://crrev.com/5291472f4339aca714470a1359ffb4e444de7960
Cr-Commit-Position: refs/heads/master@{#425939}
Patch Set 1 #Patch Set 2 : ready for review #
Total comments: 5
Patch Set 3 : fix tests #Patch Set 4 : fix test #
Total comments: 7
Patch Set 5 : rebase and rewrite #
Total comments: 34
Patch Set 6 : address comments #
Total comments: 11
Patch Set 7 : rebase, address comments, add test #
Total comments: 2
Patch Set 8 : address comment #Patch Set 9 : #
Total comments: 2
Patch Set 10 : fix nits #Messages
Total messages: 62 (39 generated)
Description was changed from ========== [NTP Client] Persist category dismissals [WIP] Should work, but for some reason the NTP still shows a status card BUG=638580 ========== to ========== [NTP Client] Persist category dismissals Persist the ids of dismissed categories to preferences. A dismissal will be cleared when: - The category is not returned anymore by the server. We assume that the category has been deprecated in that case and just stop tracking it - New suggestions are received and we want to show the new useful information to the user. BUG=638580 ==========
dgn@chromium.org changed reviewers: + bauerb@chromium.org, treib@chromium.org
dgn@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
PTAL. Tests are not completely working yet.
dgn@chromium.org changed reviewers: - bauerb@chromium.org
A quick drive-by comment... https://codereview.chromium.org/2406573002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2406573002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:64: virtual void OnNewSuggestionBatch(ContentSuggestionsProvider* provider, Do we really need this batching functionality? Why not just have n calls to OnNewSuggestions?
https://codereview.chromium.org/2406573002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2406573002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:64: virtual void OnNewSuggestionBatch(ContentSuggestionsProvider* provider, On 2016/10/10 16:50:54, tschumann wrote: > Do we really need this batching functionality? Why not just have n calls to > OnNewSuggestions? This is to have the full list of remote categories to clean up the dismissed one. Since the remote provider notifies for all the known categories, we consider the server deprecated a category when we don't get anything back about that one. We then can remove the data saved about it. I suppose I could have a separate UpdateDismissals(vector<Category>) instead
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2406573002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2406573002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:64: virtual void OnNewSuggestionBatch(ContentSuggestionsProvider* provider, On 2016/10/10 18:27:59, dgn wrote: > On 2016/10/10 16:50:54, tschumann wrote: > > Do we really need this batching functionality? Why not just have n calls to > > OnNewSuggestions? > > This is to have the full list of remote categories to clean up the dismissed > one. Since the remote provider notifies for all the known categories, we > consider the server deprecated a category when we don't get anything back about > that one. We then can remove the data saved about it. > > I suppose I could have a separate UpdateDismissals(vector<Category>) instead Actually, I still need the suggestions, or at least whether they are empty or not, so that won't be enough. Maybe I can rename the method and make it more obviously targeted at the SnippetService/RemoteSuggestionsProvider. Would that be enough?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tschumann@chromium.org changed reviewers: + tschumann@chromium.org
https://codereview.chromium.org/2406573002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2406573002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:64: virtual void OnNewSuggestionBatch(ContentSuggestionsProvider* provider, On 2016/10/10 19:54:53, dgn wrote: > On 2016/10/10 18:27:59, dgn wrote: > > On 2016/10/10 16:50:54, tschumann wrote: > > > Do we really need this batching functionality? Why not just have n calls to > > > OnNewSuggestions? > > > > This is to have the full list of remote categories to clean up the dismissed > > one. Since the remote provider notifies for all the known categories, we > > consider the server deprecated a category when we don't get anything back > about > > that one. We then can remove the data saved about it. How important is that cleanup? Does the UI need to worry about this? It feels like this change breaks a lot of abstractions... IMO it should not matter if suggestions from different catgories come from the same provider or not. What if a provider just stops delivering results (think of the PhysicalWeb integration). Would that be handled differently? > > > > I suppose I could have a separate UpdateDismissals(vector<Category>) instead > > Actually, I still need the suggestions, or at least whether they are empty or > not, so that won't be enough. > > Maybe I can rename the method and make it more obviously targeted at the > SnippetService/RemoteSuggestionsProvider. Would that be enough?
https://codereview.chromium.org/2406573002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2406573002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:64: virtual void OnNewSuggestionBatch(ContentSuggestionsProvider* provider, On 2016/10/10 20:40:23, tschumann wrote: > On 2016/10/10 19:54:53, dgn wrote: > > On 2016/10/10 18:27:59, dgn wrote: > > > On 2016/10/10 16:50:54, tschumann wrote: > > > > Do we really need this batching functionality? Why not just have n calls > to > > > > OnNewSuggestions? > > > > > > This is to have the full list of remote categories to clean up the dismissed > > > one. Since the remote provider notifies for all the known categories, we > > > consider the server deprecated a category when we don't get anything back > > about > > > that one. We then can remove the data saved about it. > How important is that cleanup? Does the UI need to worry about this? > It feels like this change breaks a lot of abstractions... IMO it should not > matter if suggestions from different catgories come from the same provider or > not. What if a provider just stops delivering results (think of the PhysicalWeb > integration). Would that be handled differently? > > > > > > I suppose I could have a separate UpdateDismissals(vector<Category>) instead > > > > Actually, I still need the suggestions, or at least whether they are empty or > > not, so that won't be enough. > > > > Maybe I can rename the method and make it more obviously targeted at the > > SnippetService/RemoteSuggestionsProvider. Would that be enough? The UI does not need to worry about it. The only place where this is a concern is between the provider for remote sections and the suggestions service. If we have orphaned dismissed categories it will stay in the preferences and never be removed. If we start syncing them they will be carried over, etc. I totally agree that where the categories come from should not matter. There is nothing in the CL to cleanup dismissed suggestions from local sources like PhysicalWeb, but since they can't be added dynamically if feels less of an issue. Another option could be to have the server report deprecated categories via a different request maybe? Or Finch? And we could purge them separately on launch. Then sure, I can remove all the category-absence based cleanup, and land that one later, when we would start needing it.
I'm looking now; sorry for the delay. Note that this will require a (probably non-trivial) rebase on top of Michael's https://codereview.chromium.org/2400783003/.
I haven't looked at everything in detail yet, but added a comment on the SuggestionBatch stuff. PTAL, or maybe a quick VC would be best? https://codereview.chromium.org/2406573002/diff/60001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2406573002/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:41: SuggestionBatch; nit: using SuggestionBatch = ... https://codereview.chromium.org/2406573002/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:65: SuggestionBatch suggestion_batch) = 0; Like Tim, I'm not happy with having two methods here which do almost (but not quite) exactly the same thing. IIUC, you need this so that you can detect when a category isn't there anymore. But shouldn't it be the responsibility of the provider to set that category's status to something unavailable? https://codereview.chromium.org/2406573002/diff/60001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2406573002/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:392: if (IsCategoryDismissed(category)) I thought changing the status should make the dismissal go away? https://codereview.chromium.org/2406573002/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:437: dismissed_categories_.insert(category_factory()->FromIDValue(id)); This might create the category, and the category order is atm defined by order of creation. That's probably not what we want...
https://codereview.chromium.org/2406573002/diff/60001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2406573002/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:65: SuggestionBatch suggestion_batch) = 0; On 2016/10/11 07:52:55, Marc Treib wrote: > Like Tim, I'm not happy with having two methods here which do almost (but not > quite) exactly the same thing. IIUC, you need this so that you can detect when a > category isn't there anymore. But shouldn't it be the responsibility of the > provider to set that category's status to something unavailable? As discussed offline, I'm going to remove this then, we'll figure out what to do about orphaned categories later, it's not a big concern. https://codereview.chromium.org/2406573002/diff/60001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2406573002/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:392: if (IsCategoryDismissed(category)) On 2016/10/11 07:52:55, Marc Treib wrote: > I thought changing the status should make the dismissal go away? It currently works just by the fact that when status changes or we received new suggestions, we run RegisterCategoryIfNecessary, which will make the category reappear. With this patch we have a more explicit way of controlling it. But I'll wait for Michael's patch to land, since it does mostly the same as this already. https://codereview.chromium.org/2406573002/diff/60001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:437: dismissed_categories_.insert(category_factory()->FromIDValue(id)); On 2016/10/11 07:52:55, Marc Treib wrote: > This might create the category, and the category order is atm defined by order > of creation. That's probably not what we want... To be addressed by registering the articles at the same time as the local categories.
tschumann@chromium.org changed reviewers: - tschumann@chromium.org
Description was changed from ========== [NTP Client] Persist category dismissals Persist the ids of dismissed categories to preferences. A dismissal will be cleared when: - The category is not returned anymore by the server. We assume that the category has been deprecated in that case and just stop tracking it - New suggestions are received and we want to show the new useful information to the user. BUG=638580 ========== to ========== [NTP Client] Persist category dismissals - Persists the ids of dismissed categories to preferences as a list of category ids - Restores the dismissed categories from the list of dismissed ids when the ContentSuggestionService is created, and matches with providers when they are registered. - Changes the way dismissals are cleared, from happening when anything about the category changes to only happen when a non empty list of suggestions is received. BUG=638580 ==========
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dgn@chromium.org changed reviewers: + bauerb@chromium.org
dgn@chromium.org changed reviewers: + bauerb@chromium.org
PTAL https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... File components/ntp_snippets/category_factory.cc (right): https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/category_factory.cc:26: AddKnownCategory(KnownCategories::ARTICLES); Added to keep the articles as the first remote section https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:165: suggestions_by_category_.erase(category); I'm not sure why we just cleared the entry before, since the rest of the time we erase it (e.g. when category not available.). Did I miss something?
PTAL https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... File components/ntp_snippets/category_factory.cc (right): https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/category_factory.cc:26: AddKnownCategory(KnownCategories::ARTICLES); Added to keep the articles as the first remote section https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:165: suggestions_by_category_.erase(category); I'm not sure why we just cleared the entry before, since the rest of the time we erase it (e.g. when category not available.). Did I miss something?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Thanks! Mostly LG now. https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... File components/ntp_snippets/category_factory.cc (right): https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/category_factory.cc:26: AddKnownCategory(KnownCategories::ARTICLES); On 2016/10/13 14:46:51, dgn wrote: > Added to keep the articles as the first remote section Please add a comment saying this :) https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:165: suggestions_by_category_.erase(category); On 2016/10/13 14:46:51, dgn wrote: > I'm not sure why we just cleared the entry before, since the rest of the time we > erase it (e.g. when category not available.). Did I miss something? No, I think I missed this in a previous CL - erase LG. But should this happen inside UnregisterCategory? https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:214: if (IsCategoryDismissed(category)) { else if? If the category was just successfully registered, it can't be dismissed, right? https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:315: auto it = providers_by_category_.find(category); Can we early-out here if |it != providers_by_category_.end()|? https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:323: (!is_dismissed || !dismissed_it->second); Maybe |category_is_dismissed| and |provider_is_new|? The two variables don't really refer to the same thing. https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:341: DCHECK(providers_by_category_.find(category) == providers_by_category_.end()); optional nit: base::ContainsKey https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:356: if (IsCategoryDismissed(category)) Why? https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:416: dismissed_providers_by_category_.end(); optional nit: base::ContainsKey https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:425: ContentSuggestionsProvider* provider = dismissed_it->second; Can provider be null here? If not, DCHECK that? https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:445: // When the provider is registered, it will be moved to this map. nit: s/moved to/stored in/ ? "moved to" sounds like it was already around somewhere. https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:26: #include "components/prefs/pref_service.h" Is this needed? Note that there's already a forward decl just below. https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:220: bool RegisterCategoryIfRequired(ContentSuggestionsProvider* provider, Would RegisterProviderForCategory be a better name for this? https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:225: ContentSuggestionsProvider* provider); nit: These two have the arguments the other way around from the first. https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:261: // RestoreDismissedCategories(). Add a comment that the providers may be null? https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:210: enabled, nullptr /* history_service */, pref_service_.get())); nit: /*history_service=*/nullptr (then clang-tidy can verify that the comment actually matches the parameter name) https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:681: ASSERT_THAT(service()->GetCategoryStatus(category1), EXPECT_THAT from here on? (Rough rule: ASSERT for preconditions to the test, EXPECT for the tested behavior itself) https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:702: } Could you also add a test for the actual persisting? I.e. recreate the service and make sure the dismissals are still there.
Java LGTM, C++ LGTM when Marc is happy :)
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
PTAL. New tests still TBD https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:165: suggestions_by_category_.erase(category); On 2016/10/13 15:59:00, Marc Treib wrote: > On 2016/10/13 14:46:51, dgn wrote: > > I'm not sure why we just cleared the entry before, since the rest of the time > we > > erase it (e.g. when category not available.). Did I miss something? > > No, I think I missed this in a previous CL - erase LG. > But should this happen inside UnregisterCategory? It's not done currently, but it would make sense to do it there. Moving the call. https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:214: if (IsCategoryDismissed(category)) { On 2016/10/13 15:59:00, Marc Treib wrote: > else if? > If the category was just successfully registered, it can't be dismissed, right? Done. https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:315: auto it = providers_by_category_.find(category); On 2016/10/13 15:59:00, Marc Treib wrote: > Can we early-out here if |it != providers_by_category_.end()|? Done, also rewrote the function since it removes branches below. https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:323: (!is_dismissed || !dismissed_it->second); On 2016/10/13 15:59:00, Marc Treib wrote: > Maybe |category_is_dismissed| and |provider_is_new|? The two variables don't > really refer to the same thing. Done. https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:341: DCHECK(providers_by_category_.find(category) == providers_by_category_.end()); On 2016/10/13 15:59:00, Marc Treib wrote: > optional nit: base::ContainsKey Thanks! I was annoyed the STL doesn't provide anything like that and didn't think about checking //base :D https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:356: if (IsCategoryDismissed(category)) On 2016/10/13 15:59:00, Marc Treib wrote: > Why? Because when OnCategoryStatusChanged() is called with a dismissed category, we would hit a DCHECK. Rewrote that bit to make it more clear. https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:416: dismissed_providers_by_category_.end(); On 2016/10/13 15:59:00, Marc Treib wrote: > optional nit: base::ContainsKey Done. https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:425: ContentSuggestionsProvider* provider = dismissed_it->second; On 2016/10/13 15:59:01, Marc Treib wrote: > Can provider be null here? If not, DCHECK that? Good catch, it's possible, yes. https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:445: // When the provider is registered, it will be moved to this map. On 2016/10/13 15:59:00, Marc Treib wrote: > nit: s/moved to/stored in/ ? "moved to" sounds like it was already around > somewhere. Done. https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:26: #include "components/prefs/pref_service.h" On 2016/10/13 15:59:01, Marc Treib wrote: > Is this needed? Note that there's already a forward decl just below. Done. https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:220: bool RegisterCategoryIfRequired(ContentSuggestionsProvider* provider, On 2016/10/13 15:59:01, Marc Treib wrote: > Would RegisterProviderForCategory be a better name for this? Done. https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:225: ContentSuggestionsProvider* provider); On 2016/10/13 15:59:01, Marc Treib wrote: > nit: These two have the arguments the other way around from the first. Different names now. The category is the main thing being processed here. https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:261: // RestoreDismissedCategories(). On 2016/10/13 15:59:01, Marc Treib wrote: > Add a comment that the providers may be null? Done. https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:210: enabled, nullptr /* history_service */, pref_service_.get())); On 2016/10/13 15:59:01, Marc Treib wrote: > nit: /*history_service=*/nullptr > (then clang-tidy can verify that the comment actually matches the parameter > name) Done. https://codereview.chromium.org/2406573002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service_unittest.cc:681: ASSERT_THAT(service()->GetCategoryStatus(category1), On 2016/10/13 15:59:01, Marc Treib wrote: > EXPECT_THAT from here on? > (Rough rule: ASSERT for preconditions to the test, EXPECT for the tested > behavior itself) Done.
Dry run: 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
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Thanks! LGTM with a few more nits https://codereview.chromium.org/2406573002/diff/100001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2406573002/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:182: dismissed_providers_by_category_.erase(category_provider_pair.first); I think RestoreDismissedCategory now handles a null provider correctly, so the check here shouldn't be required anymore? https://codereview.chromium.org/2406573002/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:242: suggestions_by_category_.erase(category); I think this can be removed, since there's now another "if (!IsCategoryStatusAvailable(new_status))" below? https://codereview.chromium.org/2406573002/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:421: DCHECK(dismissed_it != dismissed_providers_by_category_.end()); DCHECK_NE? Or alternatively, DCHECK(base::ContainsKey(..))? https://codereview.chromium.org/2406573002/diff/100001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2406573002/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service_unittest.cc:203: UserClassifier::RegisterProfilePrefs(pref_service_->registry()); Is this required now? If so, nit: #include it! https://codereview.chromium.org/2406573002/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service_unittest.cc:655: // Create and register provider nitty nit: Comment should end in "."
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL. Ready for submitting now. https://codereview.chromium.org/2406573002/diff/100001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2406573002/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:182: dismissed_providers_by_category_.erase(category_provider_pair.first); On 2016/10/17 08:44:46, Marc Treib wrote: > I think RestoreDismissedCategory now handles a null provider correctly, so the > check here shouldn't be required anymore? Done. https://codereview.chromium.org/2406573002/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:242: suggestions_by_category_.erase(category); On 2016/10/17 08:44:46, Marc Treib wrote: > I think this can be removed, since there's now another "if > (!IsCategoryStatusAvailable(new_status))" below? Done. https://codereview.chromium.org/2406573002/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:421: DCHECK(dismissed_it != dismissed_providers_by_category_.end()); On 2016/10/17 08:44:46, Marc Treib wrote: > DCHECK_NE? Or alternatively, DCHECK(base::ContainsKey(..))? Done.
lgtm https://codereview.chromium.org/2406573002/diff/120001/components/ntp_snippet... File components/ntp_snippets/category_factory_unittest.cc (right): https://codereview.chromium.org/2406573002/diff/120001/components/ntp_snippet... components/ntp_snippets/category_factory_unittest.cc:19: CategoryFactoryTest() : unused_remote_category_id_(2) {} Nit: You could just use ARTICLES + 1 :)
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2406573002/diff/120001/components/ntp_snippet... File components/ntp_snippets/category_factory_unittest.cc (right): https://codereview.chromium.org/2406573002/diff/120001/components/ntp_snippet... components/ntp_snippets/category_factory_unittest.cc:19: CategoryFactoryTest() : unused_remote_category_id_(2) {} On 2016/10/17 16:52:04, Bernhard Bauer wrote: > Nit: You could just use ARTICLES + 1 :) Done. Added LAST_KNOWN_REMOTE_CATEGORY for this.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
In case you were waiting for me: Still LGTM! :) https://codereview.chromium.org/2406573002/diff/100001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2406573002/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service_unittest.cc:203: UserClassifier::RegisterProfilePrefs(pref_service_->registry()); On 2016/10/17 08:44:46, Marc Treib wrote: > Is this required now? If so, nit: #include it! Still open https://codereview.chromium.org/2406573002/diff/160001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2406573002/diff/160001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service_unittest.cc:716: // For a regular initialisation, the category is not disabled. s/disabled/dismissed/
https://codereview.chromium.org/2406573002/diff/100001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2406573002/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service_unittest.cc:203: UserClassifier::RegisterProfilePrefs(pref_service_->registry()); On 2016/10/18 07:44:31, Marc Treib wrote: > On 2016/10/17 08:44:46, Marc Treib wrote: > > Is this required now? If so, nit: #include it! > > Still open Oops, done. Because we now provide a non null pref_service, the UserClassifier gets initialised, yes. https://codereview.chromium.org/2406573002/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service_unittest.cc:655: // Create and register provider On 2016/10/17 08:44:46, Marc Treib wrote: > nitty nit: Comment should end in "." Done. https://codereview.chromium.org/2406573002/diff/160001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2406573002/diff/160001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service_unittest.cc:716: // For a regular initialisation, the category is not disabled. On 2016/10/18 07:44:31, Marc Treib wrote: > s/disabled/dismissed/ Done.
The CQ bit was checked by dgn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, treib@chromium.org Link to the patchset: https://codereview.chromium.org/2406573002/#ps180001 (title: "fix nits")
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 ========== [NTP Client] Persist category dismissals - Persists the ids of dismissed categories to preferences as a list of category ids - Restores the dismissed categories from the list of dismissed ids when the ContentSuggestionService is created, and matches with providers when they are registered. - Changes the way dismissals are cleared, from happening when anything about the category changes to only happen when a non empty list of suggestions is received. BUG=638580 ========== to ========== [NTP Client] Persist category dismissals - Persists the ids of dismissed categories to preferences as a list of category ids - Restores the dismissed categories from the list of dismissed ids when the ContentSuggestionService is created, and matches with providers when they are registered. - Changes the way dismissals are cleared, from happening when anything about the category changes to only happen when a non empty list of suggestions is received. BUG=638580 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [NTP Client] Persist category dismissals - Persists the ids of dismissed categories to preferences as a list of category ids - Restores the dismissed categories from the list of dismissed ids when the ContentSuggestionService is created, and matches with providers when they are registered. - Changes the way dismissals are cleared, from happening when anything about the category changes to only happen when a non empty list of suggestions is received. BUG=638580 ========== to ========== [NTP Client] Persist category dismissals - Persists the ids of dismissed categories to preferences as a list of category ids - Restores the dismissed categories from the list of dismissed ids when the ContentSuggestionService is created, and matches with providers when they are registered. - Changes the way dismissals are cleared, from happening when anything about the category changes to only happen when a non empty list of suggestions is received. BUG=638580 Committed: https://crrev.com/5291472f4339aca714470a1359ffb4e444de7960 Cr-Commit-Position: refs/heads/master@{#425939} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/5291472f4339aca714470a1359ffb4e444de7960 Cr-Commit-Position: refs/heads/master@{#425939} |
