Combine all suggestions factories into ContentSuggestionsServiceFactory

Combine all suggestions factories into ContentSuggestionsServiceFactory

Remove the NTPSnippetsServiceObserver and its uses.
Change ContentSuggestionsProviders to be owned by the
ContentSuggestionsService instead of having their own lifecycle as a
KeyedService. Change RegisterProvider() and Shutdown() behavior for
the service and the providers.
Remove NTPSnippetsServiceFactory and its uses in the SnippetsBridge.
Remove OfflinePageSuggestionsProviderFactory.
Expose the NTPSnippetsService to the snippets-internals through the
ContentSuggestionsService.
Adjust unit tests.

BUG=633139
Committed: https://crrev.com/5728f0895e82f74b307d39db57099a5197ebfeb0
Cr-Commit-Position: refs/heads/master@{#409556}

[Philipp Keck, 2016-08-03 11:14:17]

pke@google.com changed reviewers:
+ treib@chromium.org

[Philipp Keck, 2016-08-03 11:14:17]

PTAL
Corresponding iOS factory will follow later.

[Marc Treib, 2016-08-03 11:53:23]

https://codereview.chromium.org/2205233002/diff/20001/chrome/browser/ntp_snip...
File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right):

https://codereview.chromium.org/2205233002/diff/20001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:50: using
suggestions::ImageDecoderImpl;
nit: keep these alphabetical

https://codereview.chromium.org/2205233002/diff/20001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:145:
base::MakeUnique<ntp_snippets::NTPSnippetsStatusService>(
nit: You already have a bunch of using declarations above, might as well add
these two.

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.cc (right):

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:32: providers_.clear();
Also reset ntp_snippets_service_

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:9: #include <string>
#include <memory>

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:192: // fetching and
debugging calls to it. This is a pointer into |providers_| or
It's not really a pointer into providers_, just providers_ will contain this one
too.

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:192: // fetching and
debugging calls to it. This is a pointer into |providers_| or
s/fetching/background fetching

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (left):

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:710:
snippets_status_service_.reset();
Why did you remove these two lines?

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:214: // In some cases, don't
even bother loading the database.
Not really appropriate anymore

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:230: expiry_timer_.Stop();
I think this isn't necessary, the timer will be destroyed anyway.

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:231: RescheduleFetching();
Why is this here? Note that we do NOT want to un-schedule just because the
service is being destroyed.

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:161: //  +------->  ERROR
This doesn't really express that any state can go to ERROR. How about:

//  +------- NOT_INITED
//  |        /       \
//  |   READY <--> DISABLED
//  |       \        /
//  +----->   ERROR

or maybe, if you want to express the direction of the downward arrows,

//  +------- NOT_INITED
//  |        /       \
//  |       v         v
//  |   READY <--> DISABLED
//  |       \        /
//  |        v      v
//  +----->   ERROR

WDYT?

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:164: // - DISABLED: if the
constructor was called with |enabled == false| . In
The enabled param doesn't exist anymore

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:165: //             that case the
service will stay disabled until it is shut
I believe this is also outdated - it can get enabled at runtime

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service_unittest.cc (right):

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service_unittest.cc:233:
std::vector<ContentSuggestion> suggestions) {}
Shouldn't this be marked override? Or maybe just defined as a MOCK_METHOD too?

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service_unittest.cc:245:
.WillByDefault(Invoke(this, &WaitForDBLoad::OnCategoryStatusChanged));
WillOnce?

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service_unittest.cc:316: service_.reset();
Is this still required now that there is an explicit Recreate? Should we DCHECK
that service_ is null?

[Philipp Keck, 2016-08-03 12:28:25]

Thank you.

All fixed except for the RescheduleFetching() on Shutdown/Destroy, where I'm not
sure.

https://codereview.chromium.org/2205233002/diff/20001/chrome/browser/ntp_snip...
File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right):

https://codereview.chromium.org/2205233002/diff/20001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:50: using
suggestions::ImageDecoderImpl;
On 2016/08/03 11:53:23, Marc Treib wrote:
> nit: keep these alphabetical

Done.

https://codereview.chromium.org/2205233002/diff/20001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:145:
base::MakeUnique<ntp_snippets::NTPSnippetsStatusService>(
On 2016/08/03 11:53:23, Marc Treib wrote:
> nit: You already have a bunch of using declarations above, might as well add
> these two.

Done, and a couple more from ntp_snippets.

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.cc (right):

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:32: providers_.clear();
On 2016/08/03 11:53:23, Marc Treib wrote:
> Also reset ntp_snippets_service_

Done.

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:9: #include <string>
On 2016/08/03 11:53:23, Marc Treib wrote:
> #include <memory>

Done.

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:192: // fetching and
debugging calls to it. This is a pointer into |providers_| or
On 2016/08/03 11:53:23, Marc Treib wrote:
> It's not really a pointer into providers_, just providers_ will contain this
one
> too.

Done.

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:192: // fetching and
debugging calls to it. This is a pointer into |providers_| or
On 2016/08/03 11:53:23, Marc Treib wrote:
> s/fetching/background fetching

Done.

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (left):

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:710:
snippets_status_service_.reset();
On 2016/08/03 11:53:23, Marc Treib wrote:
> Why did you remove these two lines?

That was unintentional, because I had removed the SHUT_DOWN state entirely,
along with this entire method, and moved stuff to the destructor, where the
resets are not necessary. Then I introduced the ERROR state and took the code
from the destructor.

Now they're back.

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:214: // In some cases, don't
even bother loading the database.
On 2016/08/03 11:53:23, Marc Treib wrote:
> Not really appropriate anymore

Done.

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:230: expiry_timer_.Stop();
On 2016/08/03 11:53:23, Marc Treib wrote:
> I think this isn't necessary, the timer will be destroyed anyway.

Indeed. Done.

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:231: RescheduleFetching();
On 2016/08/03 11:53:23, Marc Treib wrote:
> Why is this here? Note that we do NOT want to un-schedule just because the
> service is being destroyed.

Both of these lines are here because they previously were in
EnterStateShutdown().
So previously, when the KeyedService (which is now the
ContentSuggestionsService) got the Shutdown() signal, it would reschedule the
fetching. If I remove this from here, it probably changes the observable
behavior of the application, right? It might still be the right thing to do.

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:161: //  +------->  ERROR
On 2016/08/03 11:53:23, Marc Treib wrote:
> This doesn't really express that any state can go to ERROR. How about:
> 
> //  +------- NOT_INITED
> //  |        /       \
> //  |   READY <--> DISABLED
> //  |       \        /
> //  +----->   ERROR
> 
> or maybe, if you want to express the direction of the downward arrows,
> 
> //  +------- NOT_INITED
> //  |        /       \
> //  |       v         v
> //  |   READY <--> DISABLED
> //  |       \        /
> //  |        v      v
> //  +----->   ERROR
> 
> WDYT?

Right. I missed that OnDatabaseError can occur at any time.
+1 for advanced ASCII art

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:164: // - DISABLED: if the
constructor was called with |enabled == false| . In
On 2016/08/03 11:53:23, Marc Treib wrote:
> The enabled param doesn't exist anymore

Done.

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.h:165: //             that case the
service will stay disabled until it is shut
On 2016/08/03 11:53:23, Marc Treib wrote:
> I believe this is also outdated - it can get enabled at runtime

Done.

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service_unittest.cc (right):

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service_unittest.cc:245:
.WillByDefault(Invoke(this, &WaitForDBLoad::OnCategoryStatusChanged));
On 2016/08/03 11:53:23, Marc Treib wrote:
> WillOnce?

Not available for ON_CALL, unfortunately. That's also why I have to call
MOCK::[VerifyAnd]Clear below.

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service_unittest.cc:316: service_.reset();
On 2016/08/03 11:53:23, Marc Treib wrote:
> Is this still required now that there is an explicit Recreate? Should we
DCHECK
> that service_ is null?

Right. Done.

[Philipp Keck, 2016-08-03 12:31:14]

pke@google.com changed reviewers:
+ bauerb@chromium.org

[Philipp Keck, 2016-08-03 12:31:14]

bauerb@chromium.org: Please review changes in 
chrome/browser/ui/webui/snippets_internals_message_handler.*

[Philipp Keck, 2016-08-03 12:33:22]

pke@google.com changed reviewers:
+ anthonyvd@chromium.org

[Philipp Keck, 2016-08-03 12:33:22]

anthonyvd@chromium.org: Please review changes in 
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
chrome/browser/profiles/profile_manager.cc

We now use one factory for all the NTP Snippets / Content Suggestions services
and providers, so I removed the others.

[Marc Treib, 2016-08-03 12:37:56]

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:231: RescheduleFetching();
On 2016/08/03 12:28:24, Philipp Keck wrote:
> On 2016/08/03 11:53:23, Marc Treib wrote:
> > Why is this here? Note that we do NOT want to un-schedule just because the
> > service is being destroyed.
> 
> Both of these lines are here because they previously were in
> EnterStateShutdown().
> So previously, when the KeyedService (which is now the
> ContentSuggestionsService) got the Shutdown() signal, it would reschedule the
> fetching. If I remove this from here, it probably changes the observable
> behavior of the application, right? It might still be the right thing to do.

Huh, indeed. That line was added by dgn in
https://codereview.chromium.org/2061803002, and I'm guessing he just wrongly
assumed that we should unschedule on shutdown. Probably background fetching is
completely broken right now. :-/
Anyway, probably best not to change it in this CL. So leave it there for now,
but please add a TODO to verify it really should be there, because I strongly
suspect it shouldn't.

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service_unittest.cc (right):

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service_unittest.cc:245:
.WillByDefault(Invoke(this, &WaitForDBLoad::OnCategoryStatusChanged));
On 2016/08/03 12:28:25, Philipp Keck wrote:
> On 2016/08/03 11:53:23, Marc Treib wrote:
> > WillOnce?
> 
> Not available for ON_CALL, unfortunately. That's also why I have to call
> MOCK::[VerifyAnd]Clear below.

EXPECT_CALL then?

[Philipp Keck, 2016-08-03 12:57:23]

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:231: RescheduleFetching();
On 2016/08/03 12:37:56, Marc Treib wrote:
> On 2016/08/03 12:28:24, Philipp Keck wrote:
> > On 2016/08/03 11:53:23, Marc Treib wrote:
> > > Why is this here? Note that we do NOT want to un-schedule just because the
> > > service is being destroyed.
> > 
> > Both of these lines are here because they previously were in
> > EnterStateShutdown().
> > So previously, when the KeyedService (which is now the
> > ContentSuggestionsService) got the Shutdown() signal, it would reschedule
the
> > fetching. If I remove this from here, it probably changes the observable
> > behavior of the application, right? It might still be the right thing to do.
> 
> Huh, indeed. That line was added by dgn in
> https://codereview.chromium.org/2061803002, and I'm guessing he just wrongly
> assumed that we should unschedule on shutdown. Probably background fetching is
> completely broken right now. :-/
> Anyway, probably best not to change it in this CL. So leave it there for now,
> but please add a TODO to verify it really should be there, because I strongly
> suspect it shouldn't.

Ok. I just noticed that I did change the observable behavior, because
RescheduleFetching now doesn't unschedule anymore: there is no more SHUT_DOWN
state. So to achieve the same behavior, we would have to use "Unschedule()"
here.

Let's discuss this offline and then implement the right solution right away.

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service_unittest.cc (right):

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service_unittest.cc:233:
std::vector<ContentSuggestion> suggestions) {}
On 2016/08/03 11:53:23, Marc Treib wrote:
> Shouldn't this be marked override? Or maybe just defined as a MOCK_METHOD too?

Done.

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service_unittest.cc:245:
.WillByDefault(Invoke(this, &WaitForDBLoad::OnCategoryStatusChanged));
On 2016/08/03 12:37:56, Marc Treib wrote:
> On 2016/08/03 12:28:25, Philipp Keck wrote:
> > On 2016/08/03 11:53:23, Marc Treib wrote:
> > > WillOnce?
> > 
> > Not available for ON_CALL, unfortunately. That's also why I have to call
> > MOCK::[VerifyAnd]Clear below.
> 
> EXPECT_CALL then?

Ok, using EXPECT_CALL now. It's not really what it means here, but it works.

[Marc Treib, 2016-08-03 13:01:18]

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:231: RescheduleFetching();
On 2016/08/03 12:57:23, Philipp Keck wrote:
> On 2016/08/03 12:37:56, Marc Treib wrote:
> > On 2016/08/03 12:28:24, Philipp Keck wrote:
> > > On 2016/08/03 11:53:23, Marc Treib wrote:
> > > > Why is this here? Note that we do NOT want to un-schedule just because
the
> > > > service is being destroyed.
> > > 
> > > Both of these lines are here because they previously were in
> > > EnterStateShutdown().
> > > So previously, when the KeyedService (which is now the
> > > ContentSuggestionsService) got the Shutdown() signal, it would reschedule
> the
> > > fetching. If I remove this from here, it probably changes the observable
> > > behavior of the application, right? It might still be the right thing to
do.
> > 
> > Huh, indeed. That line was added by dgn in
> > https://codereview.chromium.org/2061803002, and I'm guessing he just wrongly
> > assumed that we should unschedule on shutdown. Probably background fetching
is
> > completely broken right now. :-/
> > Anyway, probably best not to change it in this CL. So leave it there for
now,
> > but please add a TODO to verify it really should be there, because I
strongly
> > suspect it shouldn't.
> 
> Ok. I just noticed that I did change the observable behavior, because
> RescheduleFetching now doesn't unschedule anymore: there is no more SHUT_DOWN
> state. So to achieve the same behavior, we would have to use "Unschedule()"
> here.
> 
> Let's discuss this offline and then implement the right solution right away.

New plan: I'll land a fix for this first, then maybe that will still merge
cleanly to M53.

[Philipp Keck, 2016-08-03 15:35:00]

On 2016/08/03 13:01:18, Marc Treib wrote:
>
https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
> File components/ntp_snippets/ntp_snippets_service.cc (right):
> 
>
https://codereview.chromium.org/2205233002/diff/20001/components/ntp_snippets...
> components/ntp_snippets/ntp_snippets_service.cc:231: RescheduleFetching();
> On 2016/08/03 12:57:23, Philipp Keck wrote:
> > On 2016/08/03 12:37:56, Marc Treib wrote:
> > > On 2016/08/03 12:28:24, Philipp Keck wrote:
> > > > On 2016/08/03 11:53:23, Marc Treib wrote:
> > > > > Why is this here? Note that we do NOT want to un-schedule just because
> the
> > > > > service is being destroyed.
> > > > 
> > > > Both of these lines are here because they previously were in
> > > > EnterStateShutdown().
> > > > So previously, when the KeyedService (which is now the
> > > > ContentSuggestionsService) got the Shutdown() signal, it would
reschedule
> > the
> > > > fetching. If I remove this from here, it probably changes the observable
> > > > behavior of the application, right? It might still be the right thing to
> do.
> > > 
> > > Huh, indeed. That line was added by dgn in
> > > https://codereview.chromium.org/2061803002, and I'm guessing he just
wrongly
> > > assumed that we should unschedule on shutdown. Probably background
fetching
> is
> > > completely broken right now. :-/
> > > Anyway, probably best not to change it in this CL. So leave it there for
> now,
> > > but please add a TODO to verify it really should be there, because I
> strongly
> > > suspect it shouldn't.
> > 
> > Ok. I just noticed that I did change the observable behavior, because
> > RescheduleFetching now doesn't unschedule anymore: there is no more
SHUT_DOWN
> > state. So to achieve the same behavior, we would have to use "Unschedule()"
> > here.
> > 
> > Let's discuss this offline and then implement the right solution right away.
> 
> New plan: I'll land a fix for this first, then maybe that will still merge
> cleanly to M53.

As discussed offline, I removed the RescheduleFetching() from the destructor. 
I also made a couple of other changes, so PTAL again.

[anthonyvd, 2016-08-03 15:40:15]

c/b/profiles/* lgtm

[Marc Treib, 2016-08-03 15:41:25]

lgtm

[Bernhard Bauer, 2016-08-03 16:30:51]

LGTM

https://codereview.chromium.org/2205233002/diff/140001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/2205233002/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1229:
ContentSuggestionsServiceFactory::GetForProfile(profile);
We could even get rid of this by overriding the factory's
ServiceIsCreatedWithBrowserContext() to return true.

https://codereview.chromium.org/2205233002/diff/140001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/2205233002/diff/140001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.h:171: // - ERROR: when an
unrecoverable error occurred.
ERROR_OCCURRED

[Philipp Keck, 2016-08-03 16:45:16]

Thank you.

https://codereview.chromium.org/2205233002/diff/140001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/2205233002/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1229:
ContentSuggestionsServiceFactory::GetForProfile(profile);
On 2016/08/03 16:30:51, Bernhard Bauer wrote:
> We could even get rid of this by overriding the factory's
> ServiceIsCreatedWithBrowserContext() to return true.

Will look into that and make another CL.

https://codereview.chromium.org/2205233002/diff/140001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/2205233002/diff/140001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.h:171: // - ERROR: when an
unrecoverable error occurred.
On 2016/08/03 16:30:51, Bernhard Bauer wrote:
> ERROR_OCCURRED

Done. Fixed all occurrences in this file.

[Philipp Keck, 2016-08-03 16:45:26]

The CQ bit was checked by pke@google.com

[Philipp Keck, 2016-08-03 16:45:27]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
treib@chromium.org, anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/2205233002/#ps160001
(title: "Bernhard's comments")

[commit-bot: I haz the power, 2016-08-03 16:46:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 17:27:51]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-08-03 17:29:48]

Description was changed from

==========
Combine all suggestions factories into ContentSuggestionsServiceFactory

Remove the NTPSnippetsServiceObserver and its uses.
Change ContentSuggestionsProviders to be owned by the
ContentSuggestionsService instead of having their own lifecycle as a
KeyedService. Change RegisterProvider() and Shutdown() behavior for
the service and the providers.
Remove NTPSnippetsServiceFactory and its uses in the SnippetsBridge.
Remove OfflinePageSuggestionsProviderFactory.
Expose the NTPSnippetsService to the snippets-internals through the
ContentSuggestionsService.
Adjust unit tests.

BUG=633139
==========

to

==========
Combine all suggestions factories into ContentSuggestionsServiceFactory

Remove the NTPSnippetsServiceObserver and its uses.
Change ContentSuggestionsProviders to be owned by the
ContentSuggestionsService instead of having their own lifecycle as a
KeyedService. Change RegisterProvider() and Shutdown() behavior for
the service and the providers.
Remove NTPSnippetsServiceFactory and its uses in the SnippetsBridge.
Remove OfflinePageSuggestionsProviderFactory.
Expose the NTPSnippetsService to the snippets-internals through the
ContentSuggestionsService.
Adjust unit tests.

BUG=633139

Committed: https://crrev.com/5728f0895e82f74b307d39db57099a5197ebfeb0
Cr-Commit-Position: refs/heads/master@{#409556}
==========

[commit-bot: I haz the power, 2016-08-03 17:29:50]

Patchset 9 (id:??) landed as
https://crrev.com/5728f0895e82f74b307d39db57099a5197ebfeb0
Cr-Commit-Position: refs/heads/master@{#409556}