[remote suggestions] Attach the fetch time to RemoteSnippets, ContentSnippets and SnippetArticle

[remote suggestions] Attach the fetch time to RemoteSnippets, ContentSnippets and SnippetArticle

Attach the fetch time to RemoteSnippets, ContentSnippets and SnippetArticle and use it for calculating metrics. This allows to mix suggestions from multiples background fetches on the fly while still beeing able to compute currect freshness metrics for each individual snippet.

BUG=690514
Review-Url: https://codereview.chromium.org/2686063003
Cr-Commit-Position: refs/heads/master@{#450692}
Committed: https://chromium.googlesource.com/chromium/src/+/05b1e88e39e1f54d14a3cc272be365ef203ea18f

[markusheintz_, 2017-02-09 16:36:27]

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

[commit-bot: I haz the power, 2017-02-09 16:37:00]

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

[markusheintz_, 2017-02-09 16:44:36]

Description was changed from

==========
Attach the fetch time to RemoteSnippets, ContentSnippets and SnippetArticle and
use it for calculating metrics. This allows to mix suggestions from multiples
background fetches on the fly while still beeing able to compute currect
freshness metrics for each individual snippet.

BUG=TBD
==========

to

==========
Attach the fetch time to RemoteSnippets, ContentSnippets and SnippetArticle and
use it for calculating metrics. This allows to mix suggestions from multiples
background fetches on the fly while still beeing able to compute currect
freshness metrics for each individual snippet.

BUG=690514
==========

[commit-bot: I haz the power, 2017-02-09 17:04:41]

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

[commit-bot: I haz the power, 2017-02-09 17:04:42]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[markusheintz_, 2017-02-10 13:37:27]

Update comments

[markusheintz_, 2017-02-10 13:38:32]

markusheintz@chromium.org changed reviewers:
+ jkrcal@chromium.org, mvanouwerkerk@google.com, treib@chromium.org

[markusheintz_, 2017-02-10 13:39:10]

markusheintz@chromium.org changed reviewers:
+ mvanouwerkerk@chromium.org
- mvanouwerkerk@google.com

[markusheintz_, 2017-02-10 16:06:56]

PTAL

[jkrcal, 2017-02-10 16:14:42]

Quick comment: 

I am not happy to move data to UI that it does not need and that is only sent
back to c++ for metrics. Especially if it's only for one provider.

I see no simple solution, let's discuss on Monday.

https://codereview.chromium.org/2686063003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestion.cc (right):

https://codereview.chromium.org/2686063003/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestion.cc:342: // TODO(markusheintz):
can you please fill in the todo?

[Marc Treib, 2017-02-10 16:16:56]

Please add a title line to the description

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

https://codereview.chromium.org/2686063003/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestion.h:171: // The time when the remote
suggestion was fetched from the server. This filed
s/filed/field/

https://codereview.chromium.org/2686063003/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestion.h:174: base::Time fetch_time_;
fetch_date_, to be consistent with publish_date_, and expiry_date in
RemoteSuggestion?

https://codereview.chromium.org/2686063003/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestion.h:175:
DISALLOW_COPY_AND_ASSIGN(ContentSuggestion);
nit: empty line before

https://codereview.chromium.org/2686063003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/proto/ntp_snippets.proto (right):

https://codereview.chromium.org/2686063003/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/proto/ntp_snippets.proto:29: optional int64
fetch_time = 11;
fetch_date ?

https://codereview.chromium.org/2686063003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestion.cc (right):

https://codereview.chromium.org/2686063003/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestion.cc:12: #include
"base/time/time.h"
nit: not required, it's already included in the header

https://codereview.chromium.org/2686063003/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestion.cc:342: // TODO(markusheintz):
?

https://codereview.chromium.org/2686063003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestion.h (right):

https://codereview.chromium.org/2686063003/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestion.h:68: // TODO(markusheintz):
Move this method to the ContentSuggestion class.
No, this belongs here IMO. The general ContentSuggestion class shouldn't know
about the provider-specific suggestion types.

https://codereview.chromium.org/2686063003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_fetcher.cc (right):

https://codereview.chromium.org/2686063003/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_fetcher.cc:448: const
base::Time fetch_time = base::Time::Now();
This should use tick_clock_ (so it's injectable in tests)

[markusheintz_, 2017-02-13 09:16:54]

Description was changed from

==========
Attach the fetch time to RemoteSnippets, ContentSnippets and SnippetArticle and
use it for calculating metrics. This allows to mix suggestions from multiples
background fetches on the fly while still beeing able to compute currect
freshness metrics for each individual snippet.

BUG=690514
==========

to

==========
[remote suggestions] Attach the fetch time to RemoteSnippets, ContentSnippets
and SnippetArticle

Attach the fetch time to RemoteSnippets, ContentSnippets and SnippetArticle and
use it for calculating metrics. This allows to mix suggestions from multiples
background fetches on the fly while still beeing able to compute currect
freshness metrics for each individual snippet.

BUG=690514
==========

[markusheintz_, 2017-02-13 09:55:43]

Address comments (execpt the time related once)

[markusheintz_, 2017-02-13 09:59:56]

@Jan: I'm aware that this is not optimal and I'm unhappy about it. But this
requires quite some substantial changes

Let's please take that on a separate track so that I can come up with some
proper design proposal first that we can then discuss.

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

https://codereview.chromium.org/2686063003/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestion.h:171: // The time when the remote
suggestion was fetched from the server. This filed
On 2017/02/10 16:16:56, Marc Treib wrote:
> s/filed/field/

Done.

https://codereview.chromium.org/2686063003/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestion.h:174: base::Time fetch_time_;
On 2017/02/10 16:16:56, Marc Treib wrote:
> fetch_date_, to be consistent with publish_date_, and expiry_date in
> RemoteSuggestion?

Done.

https://codereview.chromium.org/2686063003/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestion.h:175:
DISALLOW_COPY_AND_ASSIGN(ContentSuggestion);
On 2017/02/10 16:16:56, Marc Treib wrote:
> nit: empty line before

Done.

https://codereview.chromium.org/2686063003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/proto/ntp_snippets.proto (right):

https://codereview.chromium.org/2686063003/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/proto/ntp_snippets.proto:29: optional int64
fetch_time = 11;
On 2017/02/10 16:16:56, Marc Treib wrote:
> fetch_date ?

Done.

https://codereview.chromium.org/2686063003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestion.cc (right):

https://codereview.chromium.org/2686063003/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestion.cc:12: #include
"base/time/time.h"
On 2017/02/10 16:16:56, Marc Treib wrote:
> nit: not required, it's already included in the header

Done.

https://codereview.chromium.org/2686063003/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestion.cc:342: // TODO(markusheintz):
On 2017/02/10 16:16:56, Marc Treib wrote:
> ?

sorry forgot to remove :-(

Removed now.

https://codereview.chromium.org/2686063003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestion.h (right):

https://codereview.chromium.org/2686063003/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestion.h:68: // TODO(markusheintz):
Move this method to the ContentSuggestion class.
On 2017/02/10 16:16:56, Marc Treib wrote:
> No, this belongs here IMO. The general ContentSuggestion class shouldn't know
> about the provider-specific suggestion types.

I'll remove the TODO. Not feeling strongly about it.

[markusheintz_, 2017-02-13 10:10:48]

Address time related comments

[markusheintz_, 2017-02-13 10:11:04]

https://codereview.chromium.org/2686063003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_fetcher.cc (right):

https://codereview.chromium.org/2686063003/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_fetcher.cc:448: const
base::Time fetch_time = base::Time::Now();
On 2017/02/10 16:16:56, Marc Treib wrote:
> This should use tick_clock_ (so it's injectable in tests)

Done.

[Marc Treib, 2017-02-13 10:24:49]

https://codereview.chromium.org/2686063003/diff/60001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestion.h (right):

https://codereview.chromium.org/2686063003/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestion.h:159: // The time when the
remote suggestions was fetched from the server.
nit: s/suggestions/suggestion/

https://codereview.chromium.org/2686063003/diff/60001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_fetcher.cc (right):

https://codereview.chromium.org/2686063003/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_fetcher.cc:449:
tick_clock_->NowTicks().ToInternalValue());
Hrm, this is a bit ugly, and I'm not entirely convinced it's safe...
I guess the "proper" way would be to also inject a (non-Tick) Clock and use that
here, but I'm not sure it's worth the effort.
WDYT?

[markusheintz_, 2017-02-13 11:19:16]

https://codereview.chromium.org/2686063003/diff/60001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestion.h (right):

https://codereview.chromium.org/2686063003/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestion.h:159: // The time when the
remote suggestions was fetched from the server.
On 2017/02/13 10:24:49, Marc Treib wrote:
> nit: s/suggestions/suggestion/

Done.

https://codereview.chromium.org/2686063003/diff/60001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_fetcher.cc (right):

https://codereview.chromium.org/2686063003/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_fetcher.cc:449:
tick_clock_->NowTicks().ToInternalValue());
On 2017/02/13 10:24:49, Marc Treib wrote:
> Hrm, this is a bit ugly, and I'm not entirely convinced it's safe...
> I guess the "proper" way would be to also inject a (non-Tick) Clock and use
that
> here, but I'm not sure it's worth the effort.
> WDYT?

I share your concerns and I fully agree. I'm actually already working at it. I
was only side tracked by reviewing another CL.

[jkrcal, 2017-02-13 12:34:09]

Markus, I am fine with proceeding with this CL and relaying a redesign for
later.

[markusheintz_, 2017-02-13 14:12:30]

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

[commit-bot: I haz the power, 2017-02-13 14:12:43]

Dry run: 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-02-13 14:39:11]

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

[commit-bot: I haz the power, 2017-02-13 14:39:12]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[markusheintz_, 2017-02-13 15:05:26]

Update RemoteSuggestionTest

[markusheintz_, 2017-02-13 15:05:33]

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

[commit-bot: I haz the power, 2017-02-13 15:05:50]

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

[markusheintz_, 2017-02-13 15:06:00]

On 2017/02/13 15:05:26, markusheintz_ wrote:
> Update RemoteSuggestionTest

Thanks a lot Jan

Marc PTAL

[Marc Treib, 2017-02-13 15:20:32]

https://codereview.chromium.org/2686063003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java
(right):

https://codereview.chromium.org/2686063003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:43:
public final Long mFetchTimestampMilliseconds;
s/Long/long/ ?

https://codereview.chromium.org/2686063003/diff/100001/components/ntp_snippet...
File components/ntp_snippets/remote/json_request.h (right):

https://codereview.chromium.org/2686063003/diff/100001/components/ntp_snippet...
components/ntp_snippets/remote/json_request.h:168: base::Time
creation_time_time_;
Accidental edit? No need to say "time" twice :)

https://codereview.chromium.org/2686063003/diff/100001/components/ntp_snippet...
File components/ntp_snippets/remote/remote_suggestion_unittest.cc (right):

https://codereview.chromium.org/2686063003/diff/100001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestion_unittest.cc:28: const
base::Time fetch_date) {
nit: &

https://codereview.chromium.org/2686063003/diff/100001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestion_unittest.cc:57: const
base::Time fetch_date = base::Time::Now();
This feels wrong... Can we use a hardcoded fixed date instead?

https://codereview.chromium.org/2686063003/diff/100001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestion_unittest.cc:133:
ChromeReaderSnippetWithTwoSources(), base::Time::Now());
Also here and everywhere below: I'd prefer to avoid Now() in tests.

[Marc Treib, 2017-02-13 15:20:32]

https://codereview.chromium.org/2686063003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java
(right):

https://codereview.chromium.org/2686063003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:43:
public final Long mFetchTimestampMilliseconds;
s/Long/long/ ?

https://codereview.chromium.org/2686063003/diff/100001/components/ntp_snippet...
File components/ntp_snippets/remote/json_request.h (right):

https://codereview.chromium.org/2686063003/diff/100001/components/ntp_snippet...
components/ntp_snippets/remote/json_request.h:168: base::Time
creation_time_time_;
Accidental edit? No need to say "time" twice :)

https://codereview.chromium.org/2686063003/diff/100001/components/ntp_snippet...
File components/ntp_snippets/remote/remote_suggestion_unittest.cc (right):

https://codereview.chromium.org/2686063003/diff/100001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestion_unittest.cc:28: const
base::Time fetch_date) {
nit: &

https://codereview.chromium.org/2686063003/diff/100001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestion_unittest.cc:57: const
base::Time fetch_date = base::Time::Now();
This feels wrong... Can we use a hardcoded fixed date instead?

https://codereview.chromium.org/2686063003/diff/100001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestion_unittest.cc:133:
ChromeReaderSnippetWithTwoSources(), base::Time::Now());
Also here and everywhere below: I'd prefer to avoid Now() in tests.

[commit-bot: I haz the power, 2017-02-13 15:34:04]

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

[commit-bot: I haz the power, 2017-02-13 15:34:05]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[markusheintz_, 2017-02-13 15:43:28]

Fix android tests

[markusheintz_, 2017-02-13 15:43:33]

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

[commit-bot: I haz the power, 2017-02-13 15:43:56]

Dry run: 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-02-13 15:45:59]

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

[commit-bot: I haz the power, 2017-02-13 15:46:00]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[markusheintz_, 2017-02-13 15:54:30]

Address comments

[markusheintz_, 2017-02-13 15:55:01]

https://codereview.chromium.org/2686063003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java
(right):

https://codereview.chromium.org/2686063003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:43:
public final Long mFetchTimestampMilliseconds;
On 2017/02/13 15:20:31, Marc Treib wrote:
> s/Long/long/ ?

Done.

https://codereview.chromium.org/2686063003/diff/100001/components/ntp_snippet...
File components/ntp_snippets/remote/json_request.h (right):

https://codereview.chromium.org/2686063003/diff/100001/components/ntp_snippet...
components/ntp_snippets/remote/json_request.h:168: base::Time
creation_time_time_;
On 2017/02/13 15:20:31, Marc Treib wrote:
> Accidental edit? No need to say "time" twice :)

*arrrg* sorry forgot to remove
removed

https://codereview.chromium.org/2686063003/diff/100001/components/ntp_snippet...
File components/ntp_snippets/remote/remote_suggestion_unittest.cc (right):

https://codereview.chromium.org/2686063003/diff/100001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestion_unittest.cc:28: const
base::Time fetch_date) {
On 2017/02/13 15:20:31, Marc Treib wrote:
> nit: &

Done.

https://codereview.chromium.org/2686063003/diff/100001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestion_unittest.cc:57: const
base::Time fetch_date = base::Time::Now();
On 2017/02/13 15:20:31, Marc Treib wrote:
> This feels wrong... Can we use a hardcoded fixed date instead?

Done.

https://codereview.chromium.org/2686063003/diff/100001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestion_unittest.cc:133:
ChromeReaderSnippetWithTwoSources(), base::Time::Now());
On 2017/02/13 15:20:32, Marc Treib wrote:
> Also here and everywhere below: I'd prefer to avoid Now() in tests.

Done.

[markusheintz_, 2017-02-13 16:35:33]

rebase

[markusheintz_, 2017-02-13 16:35:37]

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

[commit-bot: I haz the power, 2017-02-13 16:36:07]

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

[Marc Treib, 2017-02-13 16:58:43]

lgtm

https://codereview.chromium.org/2686063003/diff/160001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java
(right):

https://codereview.chromium.org/2686063003/diff/160001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java:139:
1466634774); // Timestamp
nit: "Publish timestamp" and "Fetch timestamp"?

[commit-bot: I haz the power, 2017-02-13 17:22:49]

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

[commit-bot: I haz the power, 2017-02-13 17:22:50]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2017-02-14 08:40:46]

Markus, Marc, thanks for doing the hard work!

lgtm % one further change:

please change OnSuggestionShown() in content_suggestions_metrics.cc so that
_every_ impression is recorded, not only the first in the category

[Marc Treib, 2017-02-14 08:54:46]

On 2017/02/14 08:40:46, jkrcal wrote:
> Markus, Marc, thanks for doing the hard work!
> 
> lgtm % one further change:
> 
> please change OnSuggestionShown() in content_suggestions_metrics.cc so that
> _every_ impression is recorded, not only the first in the category

Jan, that's a very good catch!
However, if we change the semantics of the histogram (by recording
per-suggestion rather than per-category), then we should also rename it.

[jkrcal, 2017-02-14 09:02:33]

On 2017/02/14 08:54:46, Marc Treib wrote:
> On 2017/02/14 08:40:46, jkrcal wrote:
> > Markus, Marc, thanks for doing the hard work!
> > 
> > lgtm % one further change:
> > 
> > please change OnSuggestionShown() in content_suggestions_metrics.cc so that
> > _every_ impression is recorded, not only the first in the category
> 
> Jan, that's a very good catch!
> However, if we change the semantics of the histogram (by recording
> per-suggestion rather than per-category), then we should also rename it.

Agreed, let's change it and rename the histogram.

[markusheintz_, 2017-02-14 11:43:21]

On 2017/02/14 09:02:33, jkrcal wrote:
> On 2017/02/14 08:54:46, Marc Treib wrote:
> > On 2017/02/14 08:40:46, jkrcal wrote:
> > > Markus, Marc, thanks for doing the hard work!
> > > 
> > > lgtm % one further change:
> > > 
> > > please change OnSuggestionShown() in content_suggestions_metrics.cc so
that
> > > _every_ impression is recorded, not only the first in the category
> > 
> > Jan, that's a very good catch!
> > However, if we change the semantics of the histogram (by recording
> > per-suggestion rather than per-category), then we should also rename it.
> 
> Agreed, let's change it and rename the histogram.

I'll do the histogram Changes in a separate CL.

[markusheintz_, 2017-02-14 11:47:58]

Addresse comment treib@

[Michael van Ouwerkerk, 2017-02-14 12:08:58]

lgtm with nits

https://codereview.chromium.org/2686063003/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java
(right):

https://codereview.chromium.org/2686063003/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:76:
String previewText, String url, long publishTimestamp, float score,
Note on order: long publishTimestamp, float score, long fetchTimestamp

https://codereview.chromium.org/2686063003/diff/180001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java
(right):

https://codereview.chromium.org/2686063003/diff/180001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java:308:
"publisher" + i, "previewText" + i, url, 1466614774 + i, 10f, 1466634774));
What is this magic number? Can we have a constant for it, and if the number is
significant, document what it means? Should it get +" i" like the other
timestamp?

https://codereview.chromium.org/2686063003/diff/180001/chrome/browser/android...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2686063003/diff/180001/chrome/browser/android...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:332:
base::Time::FromJavaTime(fetch_timestamp_ms), score);
Note on argument order: publish_timestamp_ms, fetch_timestamp_ms, score. Could
ntp_snippets::metrics::OnSuggestionShown, NTPSnippetsBridge::OnSuggestionShown,
and the SnippetsArticle constructor be made consistent?

[jkrcal, 2017-02-14 12:42:29]

On 2017/02/14 11:43:21, markusheintz_ wrote:
> On 2017/02/14 09:02:33, jkrcal wrote:
> > On 2017/02/14 08:54:46, Marc Treib wrote:
> > > On 2017/02/14 08:40:46, jkrcal wrote:
> > > > Markus, Marc, thanks for doing the hard work!
> > > > 
> > > > lgtm % one further change:
> > > > 
> > > > please change OnSuggestionShown() in content_suggestions_metrics.cc so
> that
> > > > _every_ impression is recorded, not only the first in the category
> > > 
> > > Jan, that's a very good catch!
> > > However, if we change the semantics of the histogram (by recording
> > > per-suggestion rather than per-category), then we should also rename it.
> > 
> > Agreed, let's change it and rename the histogram.
> 
> I'll do the histogram Changes in a separate CL.

Fine for me, thanks!

[markusheintz_, 2017-02-14 13:09:35]

Address comments

[markusheintz_, 2017-02-14 13:09:59]

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

[commit-bot: I haz the power, 2017-02-14 13:10:25]

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

[markusheintz_, 2017-02-14 13:11:11]

https://codereview.chromium.org/2686063003/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java
(right):

https://codereview.chromium.org/2686063003/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:76:
String previewText, String url, long publishTimestamp, float score,
On 2017/02/14 12:08:58, Michael van Ouwerkerk wrote:
> Note on order: long publishTimestamp, float score, long fetchTimestamp

sorry i'm not sure what you want me to change: Should I change the order of the
arguments?

https://codereview.chromium.org/2686063003/diff/180001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java
(right):

https://codereview.chromium.org/2686063003/diff/180001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java:308:
"publisher" + i, "previewText" + i, url, 1466614774 + i, 10f, 1466634774));
On 2017/02/14 12:08:58, Michael van Ouwerkerk wrote:
> What is this magic number? Can we have a constant for it, and if the number is
> significant, document what it means? Should it get +" i" like the other
> timestamp?

I added consts for both timestamps and the score.
The +i is not needed for the fetch timestamp since it is more common that the
snippets were fetched in a single batch.

https://codereview.chromium.org/2686063003/diff/180001/chrome/browser/android...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2686063003/diff/180001/chrome/browser/android...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:332:
base::Time::FromJavaTime(fetch_timestamp_ms), score);
On 2017/02/14 12:08:58, Michael van Ouwerkerk wrote:
> Note on argument order: publish_timestamp_ms, fetch_timestamp_ms, score. Could
> ntp_snippets::metrics::OnSuggestionShown,
NTPSnippetsBridge::OnSuggestionShown,
> and the SnippetsArticle constructor be made consistent?

Done.

[commit-bot: I haz the power, 2017-02-14 13:17:41]

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

[commit-bot: I haz the power, 2017-02-14 13:17:42]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[markusheintz_, 2017-02-14 13:38:57]

Fix test after reording params in a method

[markusheintz_, 2017-02-14 13:43:29]

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

[commit-bot: I haz the power, 2017-02-14 13:43:43]

Dry run: 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-02-14 15:08:27]

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

[commit-bot: I haz the power, 2017-02-14 15:08:28]

Dry run: This issue passed the CQ dry run.

[markusheintz_, 2017-02-15 10:37:58]

The CQ bit was checked by markusheintz@chromium.org

[markusheintz_, 2017-02-15 10:37:58]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
jkrcal@chromium.org, mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/2686063003/#ps220001
(title: "Fix test after reording params in a method")

[commit-bot: I haz the power, 2017-02-15 10:38:18]

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-02-15 10:44:59]

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

[commit-bot: I haz the power, 2017-02-15 10:45:01]

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

[markusheintz_, 2017-02-15 10:50:39]

rebase

[markusheintz_, 2017-02-15 10:57:14]

The CQ bit was checked by markusheintz@chromium.org

[markusheintz_, 2017-02-15 10:57:15]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
jkrcal@chromium.org, mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/2686063003/#ps190020
(title: "rebase")

[commit-bot: I haz the power, 2017-02-15 10:57:28]

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-02-15 10:59:42]

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

[commit-bot: I haz the power, 2017-02-15 10:59:45]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[markusheintz_, 2017-02-15 11:19:56]

rebase

[markusheintz_, 2017-02-15 11:20:02]

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

[commit-bot: I haz the power, 2017-02-15 11:20:23]

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

[markusheintz_, 2017-02-15 11:35:59]

markusheintz@chromium.org changed reviewers:
+ skyostil@chromium.org

[markusheintz_, 2017-02-15 11:36:49]

Sami would you mind reviewing the single line change in 

chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/suggestions/ContentSuggestionsTestUtils.java
?

Thanks a lot

[commit-bot: I haz the power, 2017-02-15 12:05:55]

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

[commit-bot: I haz the power, 2017-02-15 12:05:56]

Dry run: This issue passed the CQ dry run.

[markusheintz_, 2017-02-15 14:28:27]

markusheintz@chromium.org changed reviewers:
- skyostil@chromium.org

[markusheintz_, 2017-02-15 14:30:27]

Remove Sami from the list of reviewers now that an OWNERS file was added which
makes the existing reviewers cover all files.

[markusheintz_, 2017-02-15 14:30:42]

The CQ bit was checked by markusheintz@chromium.org

[markusheintz_, 2017-02-15 14:30:43]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
jkrcal@chromium.org, mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/2686063003/#ps250001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-15 14:31:01]

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-02-15 14:38:23]

CQ is committing da patch.

Bot data: {"patchset_id": 250001, "attempt_start_ts": 1487169042854630,
"parent_rev": "1377231e00ed45ba28f3649c39aef7c7f47a33b2", "commit_rev":
"05b1e88e39e1f54d14a3cc272be365ef203ea18f"}

[commit-bot: I haz the power, 2017-02-15 14:38:59]

Description was changed from

==========
[remote suggestions] Attach the fetch time to RemoteSnippets, ContentSnippets
and SnippetArticle

Attach the fetch time to RemoteSnippets, ContentSnippets and SnippetArticle and
use it for calculating metrics. This allows to mix suggestions from multiples
background fetches on the fly while still beeing able to compute currect
freshness metrics for each individual snippet.

BUG=690514
==========

to

==========
[remote suggestions] Attach the fetch time to RemoteSnippets, ContentSnippets
and SnippetArticle

Attach the fetch time to RemoteSnippets, ContentSnippets and SnippetArticle and
use it for calculating metrics. This allows to mix suggestions from multiples
background fetches on the fly while still beeing able to compute currect
freshness metrics for each individual snippet.

BUG=690514

Review-Url: https://codereview.chromium.org/2686063003
Cr-Commit-Position: refs/heads/master@{#450692}
Committed:
https://chromium.googlesource.com/chromium/src/+/05b1e88e39e1f54d14a3cc272be3...
==========

[commit-bot: I haz the power, 2017-02-15 14:39:01]

Committed patchset #14 (id:250001) as
https://chromium.googlesource.com/chromium/src/+/05b1e88e39e1f54d14a3cc272be3...