[Suggestions] Moving suggestions code to a new component

[Suggestions] Moving suggestions code to a new component

Adding base::StatisticsRecorder::Initialize() to run_all_unittests to mirror another change we had made to the content test suite in https://codereview.chromium.org/330473003/

BUG=387751
TEST=Suggestions*,Blacklist*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285598

[Mathieu, 2014-07-22 14:34:52]

Hello, 

This is ready for review. Thanks!

[blundell, 2014-07-22 15:55:19]

https://codereview.chromium.org/410673002/diff/40001/components/suggestions/DEPS
File components/suggestions/DEPS (right):

https://codereview.chromium.org/410673002/diff/40001/components/suggestions/D...
components/suggestions/DEPS:2: "+content/public",
If this component is intended to be shared by iOS (as it looks like from
components.gyp), then it shouldn't use the content API. What is it using the
content API for? If it's just BrowserThread, it's fairly trivial to refactor.

[Mathieu, 2014-07-22 16:56:31]

Thanks!

https://codereview.chromium.org/410673002/diff/40001/components/suggestions/DEPS
File components/suggestions/DEPS (right):

https://codereview.chromium.org/410673002/diff/40001/components/suggestions/D...
components/suggestions/DEPS:2: "+content/public",
On 2014/07/22 15:55:19, blundell wrote:
> If this component is intended to be shared by iOS (as it looks like from
> components.gyp), then it shouldn't use the content API. What is it using the
> content API for? If it's just BrowserThread, it's fairly trivial to refactor.

Yes, it's for BrowserThread. Does iOS have the concept of threads? Could we
remove dependence on content/ in a follow-up CL?

[Mathieu, 2014-07-22 17:12:42]

See comment

https://codereview.chromium.org/410673002/diff/40001/components/suggestions/DEPS
File components/suggestions/DEPS (right):

https://codereview.chromium.org/410673002/diff/40001/components/suggestions/D...
components/suggestions/DEPS:2: "+content/public",
On 2014/07/22 16:56:31, Mathieu Perreault wrote:
> On 2014/07/22 15:55:19, blundell wrote:
> > If this component is intended to be shared by iOS (as it looks like from
> > components.gyp), then it shouldn't use the content API. What is it using the
> > content API for? If it's just BrowserThread, it's fairly trivial to
refactor.
> 
> Yes, it's for BrowserThread. Does iOS have the concept of threads? Could we
> remove dependence on content/ in a follow-up CL?

If not possible, I can put this CL on hold and try to remove dependence on
content/ in my code. Not sure how to remove the support for threads though.

[blundell, 2014-07-23 12:39:11]

On 2014/07/22 17:12:42, Mathieu Perreault wrote:
> See comment
> 
>
https://codereview.chromium.org/410673002/diff/40001/components/suggestions/DEPS
> File components/suggestions/DEPS (right):
> 
>
https://codereview.chromium.org/410673002/diff/40001/components/suggestions/D...
> components/suggestions/DEPS:2: "+content/public",
> On 2014/07/22 16:56:31, Mathieu Perreault wrote:
> > On 2014/07/22 15:55:19, blundell wrote:
> > > If this component is intended to be shared by iOS (as it looks like from
> > > components.gyp), then it shouldn't use the content API. What is it using
the
> > > content API for? If it's just BrowserThread, it's fairly trivial to
> refactor.
> > 
> > Yes, it's for BrowserThread. Does iOS have the concept of threads? Could we
> > remove dependence on content/ in a follow-up CL?
> 
> If not possible, I can put this CL on hold and try to remove dependence on
> content/ in my code. Not sure how to remove the support for threads though.

Ideally the BrowserThread stuff would be factored out as you put it in the
component. It's not factoring out threads but rather factoring out
content::BrowserThread, i.e., replacing it with usage of base-level primitives.
This can be SingleThreadTaskRunners that get injected by the embedder where a
call to e.g. BrowserThread::IO was previously used, or can be usage of
ThreadChecker where a DCHECK that you were running on BrowserThread::UI was
previously used.

[Mathieu, 2014-07-23 14:53:06]

On 2014/07/23 12:39:11, blundell wrote:
> On 2014/07/22 17:12:42, Mathieu Perreault wrote:
> > See comment
> > 
> >
>
https://codereview.chromium.org/410673002/diff/40001/components/suggestions/DEPS
> > File components/suggestions/DEPS (right):
> > 
> >
>
https://codereview.chromium.org/410673002/diff/40001/components/suggestions/D...
> > components/suggestions/DEPS:2: "+content/public",
> > On 2014/07/22 16:56:31, Mathieu Perreault wrote:
> > > On 2014/07/22 15:55:19, blundell wrote:
> > > > If this component is intended to be shared by iOS (as it looks like from
> > > > components.gyp), then it shouldn't use the content API. What is it using
> the
> > > > content API for? If it's just BrowserThread, it's fairly trivial to
> > refactor.
> > > 
> > > Yes, it's for BrowserThread. Does iOS have the concept of threads? Could
we
> > > remove dependence on content/ in a follow-up CL?
> > 
> > If not possible, I can put this CL on hold and try to remove dependence on
> > content/ in my code. Not sure how to remove the support for threads though.
> 
> Ideally the BrowserThread stuff would be factored out as you put it in the
> component. It's not factoring out threads but rather factoring out
> content::BrowserThread, i.e., replacing it with usage of base-level
primitives.
> This can be SingleThreadTaskRunners that get injected by the embedder where a
> call to e.g. BrowserThread::IO was previously used, or can be usage of
> ThreadChecker where a DCHECK that you were running on BrowserThread::UI was
> previously used.

Okay, removed dependence on content/. I think it should be good now, thanks!

[blundell, 2014-07-24 08:31:45]

https://codereview.chromium.org/410673002/diff/100001/components/suggestions/...
File components/suggestions/suggestions_service.cc (right):

https://codereview.chromium.org/410673002/diff/100001/components/suggestions/...
components/suggestions/suggestions_service.cc:123:
scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner)
It looks like this object is created and used entirely on the UI thread...is
that right? If so, you don't even need to inject this...you could post the task
to MessageLoopProxy::Current().

[Mathieu, 2014-07-24 11:51:37]

On 2014/07/24 08:31:45, blundell wrote:
>
https://codereview.chromium.org/410673002/diff/100001/components/suggestions/...
> File components/suggestions/suggestions_service.cc (right):
> 
>
https://codereview.chromium.org/410673002/diff/100001/components/suggestions/...
> components/suggestions/suggestions_service.cc:123:
> scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner)
> It looks like this object is created and used entirely on the UI thread...is
> that right? If so, you don't even need to inject this...you could post the
task
> to MessageLoopProxy::Current().

Thanks, fixed. Also fixed a gn issue. Have a look!

[Mathieu, 2014-07-24 14:00:28]

Hello jam, 

Can you review for OWNERS of chrome/ (mostly renaming).

Thanks!

[jam, 2014-07-25 00:13:25]

On 2014/07/24 14:00:28, Mathieu Perreault wrote:
> Hello jam, 
> 
> Can you review for OWNERS of chrome/ (mostly renaming).
> 
> Thanks!

i'm very overloaded with the CY, please find another reviewer

[Mathieu, 2014-07-25 00:26:46]

-jam, +darin

No problem. Darin, this is simply a move with modified tests. chrome/ owner
needed. 

Thanks!

[darin (slow to review), 2014-07-25 03:19:10]

LGTM

[blundell, 2014-07-25 07:23:42]

LGTM, thanks

https://codereview.chromium.org/410673002/diff/140001/components/suggestions/...
File components/suggestions/DEPS (right):

https://codereview.chromium.org/410673002/diff/140001/components/suggestions/...
components/suggestions/DEPS:2: "+components/keyed_service",
Please restrict this to components/keyed_service/core.

[Mathieu, 2014-07-25 14:07:51]

Thanks, submitting.

https://codereview.chromium.org/410673002/diff/140001/components/suggestions/...
File components/suggestions/DEPS (right):

https://codereview.chromium.org/410673002/diff/140001/components/suggestions/...
components/suggestions/DEPS:2: "+components/keyed_service",
On 2014/07/25 07:23:42, blundell (OOO until August 6) wrote:
> Please restrict this to components/keyed_service/core.

Done.

[Mathieu, 2014-07-25 14:07:57]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2014-07-25 14:08:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mathp@chromium.org/410673002/160001

[commit-bot: I haz the power, 2014-07-25 16:47:53]

Change committed as 285598