Creating the NTP snippet service.

Creating the NTP snippet service.

This service will provide content that will be displayed on the NTP
initially on mobile platforms (iOS and Android). This CL just creates
the necessary scaffolding, the API will be added and refined in
subsequent CLs.

BUG=547046
Committed: https://crrev.com/b0888e833f21b7a496482b96b90293f3d433e334
Cr-Commit-Position: refs/heads/master@{#360610}

[noyau (Ping after 24h), 2015-11-13 16:58:39]

The CQ bit was checked by noyau@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-13 16:59:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1438043008/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1438043008/1

[commit-bot: I haz the power, 2015-11-13 17:07:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-13 17:07:13]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[noyau (Ping after 24h), 2015-11-13 17:53:51]

Patchset #1 (id:1) has been deleted

[noyau (Ping after 24h), 2015-11-13 18:09:33]

Patchset #1 (id:20001) has been deleted

[noyau (Ping after 24h), 2015-11-13 18:18:45]

The CQ bit was checked by noyau@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-13 18:19:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1438043008/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1438043008/40001

[commit-bot: I haz the power, 2015-11-13 18:34:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-13 18:34:58]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[noyau (Ping after 24h), 2015-11-16 09:17:05]

noyau@chromium.org changed reviewers:
+ blundell@chromium.org

[noyau (Ping after 24h), 2015-11-16 09:17:05]

To blundell@ for initial review.

[Bernhard Bauer, 2015-11-16 11:46:38]

Description was changed from

==========
Creating the NTP content service.

This service will provide content that will be displayed on the NTP
initially on mobile platforms (iOS and Android). This CL just creates
the necessary scafolding, the API will be added and refined in
subsequent CLs.

BUG=None
==========

to

==========
Creating the NTP content service.

This service will provide content that will be displayed on the NTP
initially on mobile platforms (iOS and Android). This CL just creates
the necessary scafolding, the API will be added and refined in
subsequent CLs.

BUG=None
==========

[noyau (Ping after 24h), 2015-11-16 14:21:22]

Patchset #2 (id:60001) has been deleted

[blundell, 2015-11-16 16:00:53]

As discussed offline, I like your suggestion of ntp_suggestions as an
alternative name that avoids potential confusion around the overleading of the
term "content".

Do you want to instantiate the factories in this CL too so that they're just
ready to use going forward?

https://codereview.chromium.org/1438043008/diff/80001/chrome/browser/ntp_cont...
File chrome/browser/ntp_content/OWNERS (right):

https://codereview.chromium.org/1438043008/diff/80001/chrome/browser/ntp_cont...
chrome/browser/ntp_content/OWNERS:1: noyau@chromium.org
Add corresponding per-file OWNERS for the gypi file in //componetns/OWNERS.

https://codereview.chromium.org/1438043008/diff/80001/chrome/browser/ntp_cont...
File chrome/browser/ntp_content/ntp_content_service_factory.h (right):

https://codereview.chromium.org/1438043008/diff/80001/chrome/browser/ntp_cont...
chrome/browser/ntp_content/ntp_content_service_factory.h:17: namespace
ntp_content {
nit: Don't use a namespace in //chrome.

https://codereview.chromium.org/1438043008/diff/80001/ios/chrome/browser/ntp_...
File ios/chrome/browser/ntp_content/ntp_content_service_factory.h (right):

https://codereview.chromium.org/1438043008/diff/80001/ios/chrome/browser/ntp_...
ios/chrome/browser/ntp_content/ntp_content_service_factory.h:21: namespace
ntp_content {
nit: After some amount of thrashing, the naming convention we've settled on for
//ios/chrome factories of //components-level KeyedServices is
IOSChrome<Foo>Factory. i.e., IOSChromeNTPContentServiceFactory in this instance.

[noyau (Ping after 24h), 2015-11-17 10:20:09]

Patchset #6 (id:160001) has been deleted

[noyau (Ping after 24h), 2015-11-17 10:30:34]

Patchset #4 (id:120001) has been deleted

[noyau (Ping after 24h), 2015-11-17 10:36:10]

PTAL

> As discussed offline, I like your suggestion of ntp_suggestions as an
> alternative name that avoids potential confusion around the overleading
> of the term "content".
>
Done.

> Do you want to instantiate the factories in this CL too so that they're
>  just ready to use going forward?
>
Good idea, done.

https://codereview.chromium.org/1438043008/diff/80001/chrome/browser/ntp_cont...
File chrome/browser/ntp_content/OWNERS (right):

https://codereview.chromium.org/1438043008/diff/80001/chrome/browser/ntp_cont...
chrome/browser/ntp_content/OWNERS:1: noyau@chromium.org
On 2015/11/16 16:00:53, blundell wrote:
> Add corresponding per-file OWNERS for the gypi file in //componetns/OWNERS.

Done.

https://codereview.chromium.org/1438043008/diff/80001/chrome/browser/ntp_cont...
File chrome/browser/ntp_content/ntp_content_service_factory.h (right):

https://codereview.chromium.org/1438043008/diff/80001/chrome/browser/ntp_cont...
chrome/browser/ntp_content/ntp_content_service_factory.h:17: namespace
ntp_content {
On 2015/11/16 16:00:53, blundell wrote:
> nit: Don't use a namespace in //chrome.

Done.

https://codereview.chromium.org/1438043008/diff/80001/ios/chrome/browser/ntp_...
File ios/chrome/browser/ntp_content/ntp_content_service_factory.h (right):

https://codereview.chromium.org/1438043008/diff/80001/ios/chrome/browser/ntp_...
ios/chrome/browser/ntp_content/ntp_content_service_factory.h:21: namespace
ntp_content {
On 2015/11/16 16:00:53, blundell wrote:
> nit: After some amount of thrashing, the naming convention we've settled on
for
> //ios/chrome factories of //components-level KeyedServices is
> IOSChrome<Foo>Factory. i.e., IOSChromeNTPContentServiceFactory in this
instance.

Done.

[noyau (Ping after 24h), 2015-11-17 10:36:10]

PTAL

> As discussed offline, I like your suggestion of ntp_suggestions as an
> alternative name that avoids potential confusion around the overleading
> of the term "content".
>
Done.

> Do you want to instantiate the factories in this CL too so that they're
>  just ready to use going forward?
>
Good idea, done.

https://codereview.chromium.org/1438043008/diff/80001/chrome/browser/ntp_cont...
File chrome/browser/ntp_content/OWNERS (right):

https://codereview.chromium.org/1438043008/diff/80001/chrome/browser/ntp_cont...
chrome/browser/ntp_content/OWNERS:1: noyau@chromium.org
On 2015/11/16 16:00:53, blundell wrote:
> Add corresponding per-file OWNERS for the gypi file in //componetns/OWNERS.

Done.

https://codereview.chromium.org/1438043008/diff/80001/chrome/browser/ntp_cont...
File chrome/browser/ntp_content/ntp_content_service_factory.h (right):

https://codereview.chromium.org/1438043008/diff/80001/chrome/browser/ntp_cont...
chrome/browser/ntp_content/ntp_content_service_factory.h:17: namespace
ntp_content {
On 2015/11/16 16:00:53, blundell wrote:
> nit: Don't use a namespace in //chrome.

Done.

https://codereview.chromium.org/1438043008/diff/80001/ios/chrome/browser/ntp_...
File ios/chrome/browser/ntp_content/ntp_content_service_factory.h (right):

https://codereview.chromium.org/1438043008/diff/80001/ios/chrome/browser/ntp_...
ios/chrome/browser/ntp_content/ntp_content_service_factory.h:21: namespace
ntp_content {
On 2015/11/16 16:00:53, blundell wrote:
> nit: After some amount of thrashing, the naming convention we've settled on
for
> //ios/chrome factories of //components-level KeyedServices is
> IOSChrome<Foo>Factory. i.e., IOSChromeNTPContentServiceFactory in this
instance.

Done.

[blundell, 2015-11-17 11:28:24]

LGTM!

[noyau (Ping after 24h), 2015-11-17 11:43:45]

noyau@chromium.org changed reviewers:
+ jochen@chromium.org, phajdan.jr@chromium.org, sdefresne@chromium.org

[noyau (Ping after 24h), 2015-11-17 11:43:45]

jochen: For the addition in chrome/browser
sdefresne: For the addition in ios/chrome/browser
phajdan: For the new dependency on keyed_service

An All euro reviewer panel :)

Thanks,
-- Éric

[jochen (gone - plz use gerrit), 2015-11-17 13:42:26]

please file a tracking bug and reference it from the CL description

[jochen (gone - plz use gerrit), 2015-11-17 13:45:27]

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
File components/ntp_suggestions/DEPS (right):

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
components/ntp_suggestions/DEPS:2: "+components/keyed_service",
maybe restrict to .../core ?

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
File components/ntp_suggestions/OWNERS (right):

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
components/ntp_suggestions/OWNERS:2: maybelle@chromium.org
only chromium committers can be in owners files

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
File components/ntp_suggestions/ntp_suggestions_service.cc (right):

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
components/ntp_suggestions/ntp_suggestions_service.cc:4: #include
"components/ntp_suggestions/ntp_suggestions_service.h"
empty line before this one

[sdefresne, 2015-11-17 13:57:29]

https://codereview.chromium.org/1438043008/diff/200001/chrome/browser/ntp_sug...
File chrome/browser/ntp_suggestions/ntp_suggestions_service_factory.cc (right):

https://codereview.chromium.org/1438043008/diff/200001/chrome/browser/ntp_sug...
chrome/browser/ntp_suggestions/ntp_suggestions_service_factory.cc:24:
DCHECK(!context->IsOffTheRecord());
nit: The default implementation of
BrowserContextKeyedService::GetServiceForBrowserContext() will return NULL if
the passed context is off-the-record, thus I think this DCHECK is not required
(i.e. the caller will crash if it try to use the service without checking that
the context is not off-the-record).

https://codereview.chromium.org/1438043008/diff/200001/chrome/browser/ntp_sug...
chrome/browser/ntp_suggestions/ntp_suggestions_service_factory.cc:38:
DCHECK(!context->IsOffTheRecord());
Remove this DCHECK, it is useless.

https://codereview.chromium.org/1438043008/diff/200001/chrome/browser/ntp_sug...
chrome/browser/ntp_suggestions/ntp_suggestions_service_factory.cc:45: return
chrome::GetBrowserContextRedirectedInIncognito(context);
By default BrowserContextKeyedServiceFactory will return NULL if context is
off-the-record. Since you DCHECK that GetForBrowserContext is not called with an
off-the-record context, there is no need to override this method.

https://codereview.chromium.org/1438043008/diff/200001/chrome/browser/ntp_sug...
File chrome/browser/ntp_suggestions/ntp_suggestions_service_factory.h (right):

https://codereview.chromium.org/1438043008/diff/200001/chrome/browser/ntp_sug...
chrome/browser/ntp_suggestions/ntp_suggestions_service_factory.h:26: static
ntp_suggestions::NTPSuggestionsService* GetForBrowserContext(
Usually the BrowserContextKeyedServiceFactory do define a
"GetForProfile(Profile* profile)" method. This is recommended if you ever cast
the context to a Profile* in the implementation (which you indirectly do by
calling chrome::GetBrowserContextRedirectedInIncognito).

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
File components/ntp_suggestions/DEPS (right):

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
components/ntp_suggestions/DEPS:2: "+components/keyed_service",
This should be "+components/keyed_service/core" as
components/keyed_service/{content,ios} should only be used from the embedder.

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
File components/ntp_suggestions/ntp_suggestions_service.h (right):

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
components/ntp_suggestions/ntp_suggestions_service.h:4: #ifndef
COMPONENTS_NTP_SUGGESTIONS_NTP_SUGGESTIONS_SERVICE_H_
nit: blank line before include guards seems to be the norm in new files (at
least tools/boilerplate.py put a blank line there)

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
components/ntp_suggestions/ntp_suggestions_service.h:21:
NTPSuggestionsService(const std::string& application_language_code);
nit:

NTPSuggestionsService(const std::string& application_language_code);
~NTPSuggestionsService() override;

// KeyedService implementation.
void Shutdown() override;

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
components/ntp_suggestions/ntp_suggestions_service.h:25: // TODO(noyau): Add
API.
nit: TODO(crbug/xxxxx): Add API.

https://codereview.chromium.org/1438043008/diff/200001/ios/chrome/browser/ntp...
File
ios/chrome/browser/ntp_suggestions/ios_chrome_ntp_suggestions_service_factory.cc
(right):

https://codereview.chromium.org/1438043008/diff/200001/ios/chrome/browser/ntp...
ios/chrome/browser/ntp_suggestions/ios_chrome_ntp_suggestions_service_factory.cc:25:
return static_cast<ntp_suggestions::NTPSuggestionsService*>(
Implementation is different from the one in //chrome/browser/ntp_suggestions? Is
this intentional (here off-the-record context is supported, but it is not
supported in //chrome). If this is intentional, please document.

[noyau (Ping after 24h), 2015-11-17 15:04:46]

Description was changed from

==========
Creating the NTP content service.

This service will provide content that will be displayed on the NTP
initially on mobile platforms (iOS and Android). This CL just creates
the necessary scafolding, the API will be added and refined in
subsequent CLs.

BUG=None
==========

to

==========
Creating the NTP content service.

This service will provide content that will be displayed on the NTP
initially on mobile platforms (iOS and Android). This CL just creates
the necessary scafolding, the API will be added and refined in
subsequent CLs.

BUG=547046
==========

[noyau (Ping after 24h), 2015-11-17 15:57:01]

On 2015/11/17 13:42:26, jochen wrote:
> please file a tracking bug and reference it from the CL description

Done.

[Marc Treib, 2015-11-17 16:10:23]

There is already a class called SuggestionsService, which is also used by the
NTP:
https://code.google.com/p/chromium/codesearch#chromium/src/components/suggest...

To avoid confusion, we should think of another name here IMO...

[noyau (Ping after 24h), 2015-11-17 17:00:34]

Fixed all review comment.

Note to all reviewers, I'm going to do another name change, as pointed out by
treib@ the name is too close to an existing one (and for the record we are
considering using SuggestionService as one of the possible back end back end for
this new serve).

I will rename ntp_suggestions and NTPSuggestions to snippet_service and
SnippetsService.

https://codereview.chromium.org/1438043008/diff/200001/chrome/browser/ntp_sug...
File chrome/browser/ntp_suggestions/ntp_suggestions_service_factory.cc (right):

https://codereview.chromium.org/1438043008/diff/200001/chrome/browser/ntp_sug...
chrome/browser/ntp_suggestions/ntp_suggestions_service_factory.cc:24:
DCHECK(!context->IsOffTheRecord());
On 2015/11/17 13:57:29, sdefresne wrote:
> nit: The default implementation of
> BrowserContextKeyedService::GetServiceForBrowserContext() will return NULL if
> the passed context is off-the-record, thus I think this DCHECK is not required
> (i.e. the caller will crash if it try to use the service without checking that
> the context is not off-the-record).

I prefer a crash rather than a null that needs to be checked everywhere.

https://codereview.chromium.org/1438043008/diff/200001/chrome/browser/ntp_sug...
chrome/browser/ntp_suggestions/ntp_suggestions_service_factory.cc:38:
DCHECK(!context->IsOffTheRecord());
On 2015/11/17 13:57:29, sdefresne wrote:
> Remove this DCHECK, it is useless.

Done.

https://codereview.chromium.org/1438043008/diff/200001/chrome/browser/ntp_sug...
chrome/browser/ntp_suggestions/ntp_suggestions_service_factory.cc:45: return
chrome::GetBrowserContextRedirectedInIncognito(context);
On 2015/11/17 13:57:29, sdefresne wrote:
> By default BrowserContextKeyedServiceFactory will return NULL if context is
> off-the-record. Since you DCHECK that GetForBrowserContext is not called with
an
> off-the-record context, there is no need to override this method.

Done.

https://codereview.chromium.org/1438043008/diff/200001/chrome/browser/ntp_sug...
File chrome/browser/ntp_suggestions/ntp_suggestions_service_factory.h (right):

https://codereview.chromium.org/1438043008/diff/200001/chrome/browser/ntp_sug...
chrome/browser/ntp_suggestions/ntp_suggestions_service_factory.h:26: static
ntp_suggestions::NTPSuggestionsService* GetForBrowserContext(
On 2015/11/17 13:57:29, sdefresne wrote:
> Usually the BrowserContextKeyedServiceFactory do define a
> "GetForProfile(Profile* profile)" method. This is recommended if you ever cast
> the context to a Profile* in the implementation (which you indirectly do by
> calling chrome::GetBrowserContextRedirectedInIncognito).

Done.

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
File components/ntp_suggestions/DEPS (right):

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
components/ntp_suggestions/DEPS:2: "+components/keyed_service",
On 2015/11/17 13:57:29, sdefresne wrote:
> This should be "+components/keyed_service/core" as
> components/keyed_service/{content,ios} should only be used from the embedder.

Done.

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
components/ntp_suggestions/DEPS:2: "+components/keyed_service",
On 2015/11/17 13:45:27, jochen wrote:
> maybe restrict to .../core ?

Done.

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
File components/ntp_suggestions/OWNERS (right):

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
components/ntp_suggestions/OWNERS:2: maybelle@chromium.org
On 2015/11/17 13:45:27, jochen wrote:
> only chromium committers can be in owners files

Good catch. How do you check that someone is a chromium committer? Checking in
groups is quite cumbersome, is there another tool I'm not aware of?

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
File components/ntp_suggestions/ntp_suggestions_service.cc (right):

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
components/ntp_suggestions/ntp_suggestions_service.cc:4: #include
"components/ntp_suggestions/ntp_suggestions_service.h"
On 2015/11/17 13:45:27, jochen wrote:
> empty line before this one

Done.

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
File components/ntp_suggestions/ntp_suggestions_service.h (right):

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
components/ntp_suggestions/ntp_suggestions_service.h:4: #ifndef
COMPONENTS_NTP_SUGGESTIONS_NTP_SUGGESTIONS_SERVICE_H_
On 2015/11/17 13:57:29, sdefresne wrote:
> nit: blank line before include guards seems to be the norm in new files (at
> least tools/boilerplate.py put a blank line there)

Done.

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
components/ntp_suggestions/ntp_suggestions_service.h:21:
NTPSuggestionsService(const std::string& application_language_code);
On 2015/11/17 13:57:29, sdefresne wrote:
> nit:
> 
> NTPSuggestionsService(const std::string& application_language_code);
> ~NTPSuggestionsService() override;
> 
> // KeyedService implementation.
> void Shutdown() override;

I used the same order as the superclass on purpose, and the comment is not
necessary for an override.

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
components/ntp_suggestions/ntp_suggestions_service.h:25: // TODO(noyau): Add
API.
On 2015/11/17 13:57:29, sdefresne wrote:
> nit: TODO(crbug/xxxxx): Add API.

Done.

https://codereview.chromium.org/1438043008/diff/200001/ios/chrome/browser/ntp...
File
ios/chrome/browser/ntp_suggestions/ios_chrome_ntp_suggestions_service_factory.cc
(right):

https://codereview.chromium.org/1438043008/diff/200001/ios/chrome/browser/ntp...
ios/chrome/browser/ntp_suggestions/ios_chrome_ntp_suggestions_service_factory.cc:25:
return static_cast<ntp_suggestions::NTPSuggestionsService*>(
On 2015/11/17 13:57:29, sdefresne wrote:
> Implementation is different from the one in //chrome/browser/ntp_suggestions?
Is
> this intentional (here off-the-record context is supported, but it is not
> supported in //chrome). If this is intentional, please document.

Made consistent with a DCHECK and removal of GetBrowserStatetoUse.

[Bernhard Bauer, 2015-11-17 17:10:43]

bauerb@chromium.org changed reviewers:
+ bauerb@chromium.org

[Bernhard Bauer, 2015-11-17 17:10:44]

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
File components/ntp_suggestions/OWNERS (right):

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
components/ntp_suggestions/OWNERS:2: maybelle@chromium.org
On 2015/11/17 13:45:27, jochen wrote:
> only chromium committers can be in owners files

Could you add me in lieu?

[noyau (Ping after 24h), 2015-11-18 10:24:06]

Description was changed from

==========
Creating the NTP content service.

This service will provide content that will be displayed on the NTP
initially on mobile platforms (iOS and Android). This CL just creates
the necessary scafolding, the API will be added and refined in
subsequent CLs.

BUG=547046
==========

to

==========
Creating the NTP snippet service.

This service will provide content that will be displayed on the NTP
initially on mobile platforms (iOS and Android). This CL just creates
the necessary scaffolding, the API will be added and refined in
subsequent CLs.

BUG=547046
==========

[noyau (Ping after 24h), 2015-11-18 10:42:03]

PTAL. Thanks.

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
File components/ntp_suggestions/OWNERS (right):

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
components/ntp_suggestions/OWNERS:2: maybelle@chromium.org
On 2015/11/17 17:10:44, Bernhard Bauer wrote:
> On 2015/11/17 13:45:27, jochen wrote:
> > only chromium committers can be in owners files
> 
> Could you add me in lieu?

Done.

[noyau (Ping after 24h), 2015-11-18 10:48:43]

Patchset #11 (id:300001) has been deleted

[noyau (Ping after 24h), 2015-11-18 10:54:33]

Patchset #11 (id:320001) has been deleted

[jochen (gone - plz use gerrit), 2015-11-18 16:22:48]

lgtm with comments

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
File components/ntp_suggestions/OWNERS (right):

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
components/ntp_suggestions/OWNERS:2: maybelle@chromium.org
On 2015/11/17 at 17:00:34, noyau wrote:
> On 2015/11/17 13:45:27, jochen wrote:
> > only chromium committers can be in owners files
> 
> Good catch. How do you check that someone is a chromium committer? Checking in
groups is quite cumbersome, is there another tool I'm not aware of?

check https://chromium-committers.appspot.com/lists/committers@chromium.org

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

https://codereview.chromium.org/1438043008/diff/340001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.h:22: NTPSnippetsService(const
std::string& application_language_code);
explicit

[Bernhard Bauer, 2015-11-18 16:42:47]

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
File components/ntp_suggestions/OWNERS (right):

https://codereview.chromium.org/1438043008/diff/200001/components/ntp_suggest...
components/ntp_suggestions/OWNERS:2: maybelle@chromium.org
On 2015/11/18 16:22:48, jochen wrote:
> On 2015/11/17 at 17:00:34, noyau wrote:
> > On 2015/11/17 13:45:27, jochen wrote:
> > > only chromium committers can be in owners files
> > 
> > Good catch. How do you check that someone is a chromium committer? Checking
in
> groups is quite cumbersome, is there another tool I'm not aware of?
> 
> check https://chromium-committers.appspot.com/lists/committers@chromium.org

(If you get a 404, go to the root page first and sign in with your @chromium.org
account.)

[noyau (Ping after 24h), 2015-11-18 17:39:40]

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

https://codereview.chromium.org/1438043008/diff/340001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.h:22: NTPSnippetsService(const
std::string& application_language_code);
On 2015/11/18 16:22:48, jochen wrote:
> explicit

Done.

[sdefresne, 2015-11-18 17:59:07]

https://codereview.chromium.org/1438043008/diff/360001/components/ntp_snippet...
File components/ntp_snippets/BUILD.gn (right):

https://codereview.chromium.org/1438043008/diff/360001/components/ntp_snippet...
components/ntp_snippets/BUILD.gn:1: # GYP version:
components/ntp_snippets.gypi:ntp_snippets
This file also needs a copyright notice.

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

https://codereview.chromium.org/1438043008/diff/360001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.h:23: void Shutdown() override;
style: the constructor and destructor must come before other methods, so the
order should be NTPSnippetsService(), ~ NTPSnippetsService(), Shutdown()

https://google.github.io/styleguide/cppguide.html#Declaration_Order

nit: the usage is generally to prefix method overrides from super-class by a
comment like this "// KeyedService implementation.".

https://codereview.chromium.org/1438043008/diff/360001/ios/chrome/browser/ntp...
File ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.h
(right):

https://codereview.chromium.org/1438043008/diff/360001/ios/chrome/browser/ntp...
ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.h:19: }
nit: "}  // namespace ios" for consistency.

[noyau (Ping after 24h), 2015-11-19 09:08:21]

PTAL.

https://codereview.chromium.org/1438043008/diff/360001/components/ntp_snippet...
File components/ntp_snippets/BUILD.gn (right):

https://codereview.chromium.org/1438043008/diff/360001/components/ntp_snippet...
components/ntp_snippets/BUILD.gn:1: # GYP version:
components/ntp_snippets.gypi:ntp_snippets
On 2015/11/18 17:59:06, sdefresne wrote:
> This file also needs a copyright notice.

Done.

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

https://codereview.chromium.org/1438043008/diff/360001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.h:23: void Shutdown() override;
On 2015/11/18 17:59:06, sdefresne wrote:
> style: the constructor and destructor must come before other methods, so the
> order should be NTPSnippetsService(), ~ NTPSnippetsService(), Shutdown()
> 
> https://google.github.io/styleguide/cppguide.html#Declaration_Order
> 
> nit: the usage is generally to prefix method overrides from super-class by a
> comment like this "// KeyedService implementation.". 

Again, this is the same order as in the superclass, and I think this is here for
a reason: showing that shutdown should be called before the destructor. If you
look at the superclass this was done on purpose, and I'm just following the lead
there. And I already explained this in a previous comment you made.

https://codereview.chromium.org/1438043008/diff/360001/ios/chrome/browser/ntp...
File ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.h
(right):

https://codereview.chromium.org/1438043008/diff/360001/ios/chrome/browser/ntp...
ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.h:19: }
On 2015/11/18 17:59:06, sdefresne wrote:
> nit: "}  // namespace ios" for consistency.

Done.

[sdefresne, 2015-11-19 11:34:43]

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

https://codereview.chromium.org/1438043008/diff/360001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.h:23: void Shutdown() override;
On 2015/11/19 at 09:08:21, noyau wrote:
> On 2015/11/18 17:59:06, sdefresne wrote:
> > style: the constructor and destructor must come before other methods, so the
> > order should be NTPSnippetsService(), ~ NTPSnippetsService(), Shutdown()
> > 
> > https://google.github.io/styleguide/cppguide.html#Declaration_Order
> > 
> > nit: the usage is generally to prefix method overrides from super-class by a
> > comment like this "// KeyedService implementation.". 
> 
> Again, this is the same order as in the superclass, and I think this is here
for a reason: showing that shutdown should be called before the destructor. If
you look at the superclass this was done on purpose, and I'm just following the
lead there. And I already explained this in a previous comment you made.

This does not seems to be what other classes inheriting from KeyedService do:
https://code.google.com/p/chromium/codesearch#search/&q=//%5C%20KeyedService&...

[sdefresne, 2015-11-19 11:46:50]

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

https://codereview.chromium.org/1438043008/diff/360001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.h:23: void Shutdown() override;
On 2015/11/19 at 11:34:43, sdefresne wrote:
> On 2015/11/19 at 09:08:21, noyau wrote:
> > On 2015/11/18 17:59:06, sdefresne wrote:
> > > style: the constructor and destructor must come before other methods, so
the
> > > order should be NTPSnippetsService(), ~ NTPSnippetsService(), Shutdown()
> > > 
> > > https://google.github.io/styleguide/cppguide.html#Declaration_Order
> > > 
> > > nit: the usage is generally to prefix method overrides from super-class by
a
> > > comment like this "// KeyedService implementation.". 
> > 
> > Again, this is the same order as in the superclass, and I think this is here
for a reason: showing that shutdown should be called before the destructor. If
you look at the superclass this was done on purpose, and I'm just following the
lead there. And I already explained this in a previous comment you made.
> 
> This does not seems to be what other classes inheriting from KeyedService do:
>
https://code.google.com/p/chromium/codesearch#search/&q=//%5C%20KeyedService&...

Sent https://codereview.chromium.org/1459843002 to fix the style guide violation
in the super class.

[noyau (Ping after 24h), 2015-11-19 12:36:14]

I repent, I repent! PTAL :)

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

https://codereview.chromium.org/1438043008/diff/360001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.h:23: void Shutdown() override;
On 2015/11/18 17:59:06, sdefresne wrote:
> style: the constructor and destructor must come before other methods, so the
> order should be NTPSnippetsService(), ~ NTPSnippetsService(), Shutdown()
> 
> https://google.github.io/styleguide/cppguide.html#Declaration_Order
> 
> nit: the usage is generally to prefix method overrides from super-class by a
> comment like this "// KeyedService implementation.". 

Done.

[sdefresne, 2015-11-19 13:23:56]

lgtm

[noyau (Ping after 24h), 2015-11-19 14:20:27]

The CQ bit was checked by noyau@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-19 14:20:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1438043008/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1438043008/380001

[commit-bot: I haz the power, 2015-11-19 14:22:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-19 14:22:35]

Dry run: Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[noyau (Ping after 24h), 2015-11-19 15:51:56]

The CQ bit was checked by noyau@chromium.org

[noyau (Ping after 24h), 2015-11-19 15:51:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
blundell@chromium.org, jochen@chromium.org, sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/1438043008/#ps420001
(title: "Rebase fix: Don't trust git.")

[commit-bot: I haz the power, 2015-11-19 15:52:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1438043008/420001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1438043008/420001

[commit-bot: I haz the power, 2015-11-19 17:13:33]

Committed patchset #16 (id:420001)

[commit-bot: I haz the power, 2015-11-19 17:14:34]

Patchset 16 (id:??) landed as
https://crrev.com/b0888e833f21b7a496482b96b90293f3d433e334
Cr-Commit-Position: refs/heads/master@{#360610}