Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2232473002:
Register all CSuggestionsProviders in IOSChromeContentSuggestionsServiceFactory (Closed)
|
Created:
4 years, 4 months ago by Philipp Keck Modified:
4 years, 3 months ago CC:
chromium-reviews, ntp-dev+reviews_chromium.org, sdefresne+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRegister all providers in IOSChromeContentSuggestionsServiceFactory
Create the articles provider (NTPSnippetsService) and the
BookmarkSuggestionsProvider in the
IOSChromeContentSuggestionsServiceFactory and register them with
the ContentSuggestionsService.
This makes the IOSChromeContentSuggestionsServiceFactory equivalent
to the ContentSuggestionsServiceFactory again (apart from the
OfflinePageSuggestionsProvider, which is Android-only).
BUG=633139
Committed: https://crrev.com/c9f49e317107c0a827f559f4a2da1050dd207358
Cr-Commit-Position: refs/heads/master@{#414415}
Patch Set 1 #Patch Set 2 : Fix compiler errors #Patch Set 3 : Fix another compile error #
Total comments: 8
Patch Set 4 : Marc's comments #Patch Set 5 : Update the factory #Patch Set 6 : Add check for kArticleSuggestionsFeature #Patch Set 7 : Fix compiler errors #Patch Set 8 : Fix another compiler error #Messages
Total messages: 17 (6 generated)
Description was changed from ========== Register all CSuggestionsProviders in IOSChromeContentSuggestionsFactory BUG=633139 ========== to ========== Register all providers in IOSChromeContentSuggestionsServiceFactory Create the articles provider (NTPSnippetsService) and the BookmarkSuggestionsProvider in the IOSChromeContentSuggestionsServiceFactory and register them with the ContentSuggestionsService. This makes the IOSChromeContentSuggestionsServiceFactory equivalent to the ContentSuggestionsServiceFactory again (apart from the OfflinePageSuggestionsProvider, which is Android-only). BUG=633139 ==========
pke@google.com changed reviewers: + noyau@chromium.org, treib@chromium.org
PTAL In a following CL, I will change both the Android and the iOS factory to use ServiceIsCreatedWithBrowserContext/ServiceIsCreatedWithBrowserState.
https://codereview.chromium.org/2232473002/diff/40001/ios/chrome/browser/ntp_... File ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2232473002/diff/40001/ios/chrome/browser/ntp_... ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc:111: State enabled = State::DISABLED; // Currently always disabled. Hm. We could also check the feature, like we do on other platforms. It'll still always be disabled in practice, but it might make testing easier. Let's see what Eric thinks :) https://codereview.chromium.org/2232473002/diff/40001/ios/chrome/browser/ntp_... ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc:127: // Create the NTPSnippetsService (articles provider). You should check the new kArticleSuggestionsFeature from ntp_snippets/features.h https://codereview.chromium.org/2232473002/diff/40001/ios/chrome/browser/ntp_... ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc:146: // implemented on iOS. crbug.com/609127 I think this comment is outdated - we are injecting an ImageDecoder. (There's more stuff to do in the image_fetcher component, but I don't think that warrants a comment here anymore)
Are all those classes used in this factory unittested on iOS? Just to be sure, thanks. https://codereview.chromium.org/2232473002/diff/40001/ios/chrome/browser/ntp_... File ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2232473002/diff/40001/ios/chrome/browser/ntp_... ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc:111: State enabled = State::DISABLED; // Currently always disabled. On 2016/08/10 08:39:51, Marc Treib wrote: > Hm. We could also check the feature, like we do on other platforms. It'll still > always be disabled in practice, but it might make testing easier. Let's see what > Eric thinks :) I do not understand what you mean by "check the feature". In general, if we can get the code as close as possible on all platforms, that's better, but in that case I'm missing information to voice an opinion.
https://codereview.chromium.org/2232473002/diff/40001/ios/chrome/browser/ntp_... File ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2232473002/diff/40001/ios/chrome/browser/ntp_... ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc:111: State enabled = State::DISABLED; // Currently always disabled. On 2016/08/19 21:16:03, noyau wrote: > On 2016/08/10 08:39:51, Marc Treib wrote: > > Hm. We could also check the feature, like we do on other platforms. It'll > still > > always be disabled in practice, but it might make testing easier. Let's see > what > > Eric thinks :) > > I do not understand what you mean by "check the feature". In general, if we can > get the code as close as possible on all platforms, that's better, but in that > case I'm missing information to voice an opinion. In the non-ios factory, this reads: State state = base::FeatureList::IsEnabled(ntp_snippets::kContentSuggestionsFeature) ? State::ENABLED : State::DISABLED; I.e. set state to ENABLED iff ntp_snippets::kContentSuggestionsFeature is enabled. I'm proposing we do the same on ios, even though that feature will always be disabled right now.
On 2016/08/22 08:15:00, Marc Treib wrote: > https://codereview.chromium.org/2232473002/diff/40001/ios/chrome/browser/ntp_... > File > ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc > (right): > > https://codereview.chromium.org/2232473002/diff/40001/ios/chrome/browser/ntp_... > ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc:111: > State enabled = State::DISABLED; // Currently always disabled. > On 2016/08/19 21:16:03, noyau wrote: > > On 2016/08/10 08:39:51, Marc Treib wrote: > > > Hm. We could also check the feature, like we do on other platforms. It'll > > still > > > always be disabled in practice, but it might make testing easier. Let's see > > what > > > Eric thinks :) > > > > I do not understand what you mean by "check the feature". In general, if we > can > > get the code as close as possible on all platforms, that's better, but in that > > case I'm missing information to voice an opinion. > > In the non-ios factory, this reads: > State state = > base::FeatureList::IsEnabled(ntp_snippets::kContentSuggestionsFeature) > ? State::ENABLED > : State::DISABLED; > > I.e. set state to ENABLED iff ntp_snippets::kContentSuggestionsFeature is > enabled. I'm proposing we do the same on ios, even though that feature will > always be disabled right now. Sounds reasonable.
lgtm
https://codereview.chromium.org/2232473002/diff/40001/ios/chrome/browser/ntp_... File ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2232473002/diff/40001/ios/chrome/browser/ntp_... ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc:111: State enabled = State::DISABLED; // Currently always disabled. On 2016/08/22 08:15:00, Marc Treib wrote: > On 2016/08/19 21:16:03, noyau wrote: > > On 2016/08/10 08:39:51, Marc Treib wrote: > > > Hm. We could also check the feature, like we do on other platforms. It'll > > still > > > always be disabled in practice, but it might make testing easier. Let's see > > what > > > Eric thinks :) > > > > I do not understand what you mean by "check the feature". In general, if we > can > > get the code as close as possible on all platforms, that's better, but in that > > case I'm missing information to voice an opinion. > > In the non-ios factory, this reads: > State state = > base::FeatureList::IsEnabled(ntp_snippets::kContentSuggestionsFeature) > ? State::ENABLED > : State::DISABLED; > > I.e. set state to ENABLED iff ntp_snippets::kContentSuggestionsFeature is > enabled. I'm proposing we do the same on ios, even though that feature will > always be disabled right now. Done. https://codereview.chromium.org/2232473002/diff/40001/ios/chrome/browser/ntp_... ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc:127: // Create the NTPSnippetsService (articles provider). On 2016/08/10 08:39:51, Marc Treib wrote: > You should check the new kArticleSuggestionsFeature from ntp_snippets/features.h Done. https://codereview.chromium.org/2232473002/diff/40001/ios/chrome/browser/ntp_... ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc:146: // implemented on iOS. crbug.com/609127 On 2016/08/10 08:39:51, Marc Treib wrote: > I think this comment is outdated - we are injecting an ImageDecoder. (There's > more stuff to do in the image_fetcher component, but I don't think that warrants > a comment here anymore) Done.
lgtm
The CQ bit was checked by pke@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from noyau@chromium.org Link to the patchset: https://codereview.chromium.org/2232473002/#ps140001 (title: "Fix another compiler error")
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 ========== Register all providers in IOSChromeContentSuggestionsServiceFactory Create the articles provider (NTPSnippetsService) and the BookmarkSuggestionsProvider in the IOSChromeContentSuggestionsServiceFactory and register them with the ContentSuggestionsService. This makes the IOSChromeContentSuggestionsServiceFactory equivalent to the ContentSuggestionsServiceFactory again (apart from the OfflinePageSuggestionsProvider, which is Android-only). BUG=633139 ========== to ========== Register all providers in IOSChromeContentSuggestionsServiceFactory Create the articles provider (NTPSnippetsService) and the BookmarkSuggestionsProvider in the IOSChromeContentSuggestionsServiceFactory and register them with the ContentSuggestionsService. This makes the IOSChromeContentSuggestionsServiceFactory equivalent to the ContentSuggestionsServiceFactory again (apart from the OfflinePageSuggestionsProvider, which is Android-only). BUG=633139 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Register all providers in IOSChromeContentSuggestionsServiceFactory Create the articles provider (NTPSnippetsService) and the BookmarkSuggestionsProvider in the IOSChromeContentSuggestionsServiceFactory and register them with the ContentSuggestionsService. This makes the IOSChromeContentSuggestionsServiceFactory equivalent to the ContentSuggestionsServiceFactory again (apart from the OfflinePageSuggestionsProvider, which is Android-only). BUG=633139 ========== to ========== Register all providers in IOSChromeContentSuggestionsServiceFactory Create the articles provider (NTPSnippetsService) and the BookmarkSuggestionsProvider in the IOSChromeContentSuggestionsServiceFactory and register them with the ContentSuggestionsService. This makes the IOSChromeContentSuggestionsServiceFactory equivalent to the ContentSuggestionsServiceFactory again (apart from the OfflinePageSuggestionsProvider, which is Android-only). BUG=633139 Committed: https://crrev.com/c9f49e317107c0a827f559f4a2da1050dd207358 Cr-Commit-Position: refs/heads/master@{#414415} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/c9f49e317107c0a827f559f4a2da1050dd207358 Cr-Commit-Position: refs/heads/master@{#414415} |
