Expose notification info in ContentSuggestion

Expose notification info in ContentSuggestion

Add ContentSuggestion::notification_extra(), set when we should notify.
Set it based on notificationInfo from the JSON response. For the sake of
the test, move the code to convert NTPSnippet to ContentSuggestion in
NTPSnippet itself.

BUG=675961
Review-Url: https://codereview.chromium.org/2616543002
Cr-Commit-Position: refs/heads/master@{#441653}
Committed: https://chromium.googlesource.com/chromium/src/+/a588fa5a67e9f1c24326ae6f0b2475e1bb1e8d5f

[sfiera, 2017-01-03 22:40:12]

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

[commit-bot: I haz the power, 2017-01-03 22:40:29]

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

[sfiera, 2017-01-03 22:41:09]

sfiera@chromium.org changed reviewers:
+ jkrcal@chromium.org

[sfiera, 2017-01-03 22:41:10]

Howdy, owner!

[commit-bot: I haz the power, 2017-01-03 23:25:58]

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

[commit-bot: I haz the power, 2017-01-03 23:25:59]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2017-01-04 07:34:32]

lgtm

https://codereview.chromium.org/2616543002/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/ntp_snippet.cc (right):

https://codereview.chromium.org/2616543002/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippet.cc:263: snippet->should_notify_ =
false;
nit: is this needed? should_notify_ is initially false. Do you just make sure
DictionaryValue::GetBoolean does not misbehave?

https://codereview.chromium.org/2616543002/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/ntp_snippet_unittest.cc (right):

https://codereview.chromium.org/2616543002/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippet_unittest.cc:413:
std::unique_ptr<base::DictionaryValue> BaseJSON() {
nit: I am a bit lost in the various functions here that return Dictionary of a
snippet. What is the relation to SnippetWithTwo/ThreeSources? Can you unify the
naming or make this name more specific to make it clearer how it differs,
please?

https://codereview.chromium.org/2616543002/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippet_unittest.cc:475: TEST(NTPSnippetTest,
ToContentSuggestion) {
Thanks for increasing test coverage! :)

[sfiera, 2017-01-05 14:03:56]

https://codereview.chromium.org/2616543002/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/ntp_snippet.cc (right):

https://codereview.chromium.org/2616543002/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippet.cc:263: snippet->should_notify_ =
false;
On 2017/01/04 07:34:32, jkrcal wrote:
> nit: is this needed? should_notify_ is initially false. Do you just make sure
> DictionaryValue::GetBoolean does not misbehave?

I guess not. Removed.

https://codereview.chromium.org/2616543002/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/ntp_snippet_unittest.cc (right):

https://codereview.chromium.org/2616543002/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippet_unittest.cc:413:
std::unique_ptr<base::DictionaryValue> BaseJSON() {
On 2017/01/04 07:34:32, jkrcal wrote:
> nit: I am a bit lost in the various functions here that return Dictionary of a
> snippet. What is the relation to SnippetWithTwo/ThreeSources? Can you unify
the
> naming or make this name more specific to make it clearer how it differs,
> please?

Multiple sources is a ChromeReader thing. This one is ContentSuggestionsServer.
Named the functions accordingly.

[sfiera, 2017-01-05 14:04:30]

The CQ bit was checked by sfiera@chromium.org

[sfiera, 2017-01-05 14:04:31]

The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org
Link to the patchset: https://codereview.chromium.org/2616543002/#ps20001
(title: "Address comments")

[commit-bot: I haz the power, 2017-01-05 14:04:46]

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, 2017-01-05 14:47:41]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1483625070585200,
"parent_rev": "61d89346dfce65fc96f2b1ed17314c667cf6d332", "commit_rev":
"a588fa5a67e9f1c24326ae6f0b2475e1bb1e8d5f"}

[commit-bot: I haz the power, 2017-01-05 14:48:10]

Description was changed from

==========
Expose notification info in ContentSuggestion

Add ContentSuggestion::notification_extra(), set when we should notify.
Set it based on notificationInfo from the JSON response. For the sake of
the test, move the code to convert NTPSnippet to ContentSuggestion in
NTPSnippet itself.

BUG=675961
==========

to

==========
Expose notification info in ContentSuggestion

Add ContentSuggestion::notification_extra(), set when we should notify.
Set it based on notificationInfo from the JSON response. For the sake of
the test, move the code to convert NTPSnippet to ContentSuggestion in
NTPSnippet itself.

BUG=675961

Review-Url: https://codereview.chromium.org/2616543002
Cr-Commit-Position: refs/heads/master@{#441653}
Committed:
https://chromium.googlesource.com/chromium/src/+/a588fa5a67e9f1c24326ae6f0b24...
==========

[commit-bot: I haz the power, 2017-01-05 14:48:10]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/a588fa5a67e9f1c24326ae6f0b24...