SuggestionsService support for Desktop

SuggestionsService support for Desktop

- support for ClearBlacklist
- support for favicon generation

BUG=514752
Committed: https://crrev.com/85bc9a7a9abd749b382aa28725f488484dec0836
Cr-Commit-Position: refs/heads/master@{#340757}

[fserb, 2015-07-27 21:20:41]

Patchset #1 (id:1) has been deleted

[fserb, 2015-07-27 21:21:24]

fserb@chromium.org changed reviewers:
+ mathp@chromium.org

[fserb, 2015-07-27 21:21:24]

fserb@chromium.org changed required reviewers:
+ mathp@chromium.org

[Mathieu, 2015-07-28 13:09:29]

Test coverage please!

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

https://codereview.chromium.org/1259123002/diff/20001/components/suggestions/...
components/suggestions/suggestions_service.cc:88: // TODO(mathp): Add logic to
decide the device type of the request.
nice one ;) mathp -> fserb 

Seriously though, can you make this a follow-up CL? I was thinking we could make
device type an argument at construction time... perhaps the
SuggestionsServiceFactory can decide which one it constructs (IFDEF could be in
there). 

I don't see IFDEF extending well here if we need to fork more than just
constants.

https://codereview.chromium.org/1259123002/diff/20001/components/suggestions/...
components/suggestions/suggestions_service.cc:89: #if defined(OS_ANDROID) ||
defined(OS_IOS) || defined(OS_CHROMEOS)
ChromeOS can be lumped in the desktop case.

https://codereview.chromium.org/1259123002/diff/20001/components/suggestions/...
components/suggestions/suggestions_service.cc:333:
base::StringPrintf("https://s2.googleusercontent.com/s2/"
bring into a constant at the top

https://codereview.chromium.org/1259123002/diff/20001/components/suggestions/...
File components/suggestions/suggestions_service.h (right):

https://codereview.chromium.org/1259123002/diff/20001/components/suggestions/...
components/suggestions/suggestions_service.h:144: void
PopulateFavicons(SuggestionsProfile* suggestions);
comment

https://codereview.chromium.org/1259123002/diff/20001/components/suggestions/...
components/suggestions/suggestions_service.h:144: void
PopulateFavicons(SuggestionsProfile* suggestions);
PopulateFaviconUrls?

[fserb, 2015-07-28 15:31:34]

Done tests too.

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

https://codereview.chromium.org/1259123002/diff/20001/components/suggestions/...
components/suggestions/suggestions_service.cc:88: // TODO(mathp): Add logic to
decide the device type of the request.
On 2015/07/28 13:09:29, Mathieu Perreault wrote:
> nice one ;) mathp -> fserb 
> 
> Seriously though, can you make this a follow-up CL? I was thinking we could
make
> device type an argument at construction time... perhaps the
> SuggestionsServiceFactory can decide which one it constructs (IFDEF could be
in
> there). 
> 
> I don't see IFDEF extending well here if we need to fork more than just
> constants.

I mean, there's going to be an IFDEF somewhere, but sure, we can do that on the
Factory.

https://codereview.chromium.org/1259123002/diff/20001/components/suggestions/...
components/suggestions/suggestions_service.cc:89: #if defined(OS_ANDROID) ||
defined(OS_IOS) || defined(OS_CHROMEOS)
On 2015/07/28 13:09:29, Mathieu Perreault wrote:
> ChromeOS can be lumped in the desktop case.

Done.

https://codereview.chromium.org/1259123002/diff/20001/components/suggestions/...
components/suggestions/suggestions_service.cc:333:
base::StringPrintf("https://s2.googleusercontent.com/s2/"
On 2015/07/28 13:09:29, Mathieu Perreault wrote:
> bring into a constant at the top

Done.

https://codereview.chromium.org/1259123002/diff/20001/components/suggestions/...
File components/suggestions/suggestions_service.h (right):

https://codereview.chromium.org/1259123002/diff/20001/components/suggestions/...
components/suggestions/suggestions_service.h:144: void
PopulateFavicons(SuggestionsProfile* suggestions);
On 2015/07/28 13:09:29, Mathieu Perreault wrote:
> PopulateFaviconUrls?

Done.

https://codereview.chromium.org/1259123002/diff/20001/components/suggestions/...
components/suggestions/suggestions_service.h:144: void
PopulateFavicons(SuggestionsProfile* suggestions);
On 2015/07/28 13:09:29, Mathieu Perreault wrote:
> comment

Done.

[Mathieu, 2015-07-28 18:46:02]

Looks great, can we add a quick test for ClearBlacklist?

[fserb, 2015-07-28 19:02:42]

DONE!

[Mathieu, 2015-07-28 19:11:43]

lgtm

[fserb, 2015-07-28 19:12:43]

The CQ bit was checked by fserb@chromium.org

[commit-bot: I haz the power, 2015-07-28 19:13:06]

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

[commit-bot: I haz the power, 2015-07-28 20:20:19]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-07-28 20:21:01]

Patchset 4 (id:??) landed as
https://crrev.com/85bc9a7a9abd749b382aa28725f488484dec0836
Cr-Commit-Position: refs/heads/master@{#340757}