📰 Move metrics and scheduling events out of SnippetsBridge

[Suggestions] Move metrics and scheduling events out of SnippetsBridge

Extracts a SuggestionsEventReporter out of SnippetsBridge, that is
responsible for recording backend metrics and sending events used by
the fetch scheduler.

BUG=710268
Review-Url: https://codereview.chromium.org/2844033002
Cr-Commit-Position: refs/heads/master@{#469305}
Committed: https://chromium.googlesource.com/chromium/src/+/ba0a51b986fd26385a2e1009a8e2ec974bd43639

[dgn, 2017-04-27 09:35:20]

Description was changed from

==========
[Suggestions] Move metrics and scheduling events out of SnippetsBridge

BUG=710268
==========

to

==========
[Suggestions] Move metrics and scheduling events out of SnippetsBridge

Extracts a SuggestionsEventReporter out of SnippetsBridge, that is
responsible for recording backend metrics and sending events used by
the fetch scheduler.

BUG=710268
==========

[dgn, 2017-04-27 09:35:20]

dgn@chromium.org changed reviewers:
+ bauerb@chromium.org, mvanouwerkerk@google.com, treib@chromium.org

[dgn, 2017-04-27 09:53:19]

Early draft, can you please have a quick look for sanity checkin and design
opinions? Thanks! I'll send for proper review after I have a draft for
https://crbug.com/693593

https://codereview.chromium.org/2844033002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2844033002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:319:
SuggestionsEventReporter eventReporter = new SuggestionsEventReporterBridge();
Not sure if it should be an instance like this since all the methods could be
static. Using an instance makes it possible to spy metrics calls, but we don't
currently make use of that. I can also make it a singleton and change the calls
to SuggestionsEventReporter.getInstance().onFooHappened()

[Marc Treib, 2017-04-27 09:54:28]

+markusheintz FYI since we talked about this in the past.

Looking at the CL now...

[Marc Treib, 2017-04-27 10:06:47]

Sanity check passed! Let me know when you want a full review.

https://codereview.chromium.org/2844033002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2844033002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:178:
SuggestionsEventReporter metricsReporter,
s/metricsReporter/eventReporter/, in lots of places :)

https://codereview.chromium.org/2844033002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:319:
SuggestionsEventReporter eventReporter = new SuggestionsEventReporterBridge();
On 2017/04/27 09:53:19, dgn wrote:
> Not sure if it should be an instance like this since all the methods could be
> static. Using an instance makes it possible to spy metrics calls, but we don't
> currently make use of that. I can also make it a singleton and change the
calls
> to SuggestionsEventReporter.getInstance().onFooHappened()

I'd probably go for making it static, but I think you need the interface/impl
split for tests? In which case IMO the current thing is fine.

https://codereview.chromium.org/2844033002/diff/1/chrome/browser/android/ntp/...
File chrome/browser/android/ntp/suggestions_event_reporter_bridge.cc (right):

https://codereview.chromium.org/2844033002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/suggestions_event_reporter_bridge.cc:1: // Copyright
2016 The Chromium Authors. All rights reserved.
2017 :)

I haven't looked at this file in detail, I assume it's basically all copy-pasted
from ntp_snippets_bridge.cc?

[Bernhard Bauer, 2017-04-27 11:55:16]

Looks pretty good to me.

https://codereview.chromium.org/2844033002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(left):

https://codereview.chromium.org/2844033002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:378: if
(mSnippetsBridge != null) {
Huh, I think that check was unnecessary anyway.

[dgn, 2017-04-28 14:29:28]

PTAL

https://codereview.chromium.org/2844033002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2844033002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:178:
SuggestionsEventReporter metricsReporter,
On 2017/04/27 10:06:47, Marc Treib wrote:
> s/metricsReporter/eventReporter/, in lots of places :)

Done.

https://codereview.chromium.org/2844033002/diff/1/chrome/browser/android/ntp/...
File chrome/browser/android/ntp/suggestions_event_reporter_bridge.cc (right):

https://codereview.chromium.org/2844033002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/suggestions_event_reporter_bridge.cc:1: // Copyright
2016 The Chromium Authors. All rights reserved.
On 2017/04/27 10:06:47, Marc Treib wrote:
> 2017 :)
Oops thanks!

> I haven't looked at this file in detail, I assume it's basically all
copy-pasted
> from ntp_snippets_bridge.cc?

Basically yes. I extracted GetUserClassifier() to get it from the service I pull
from the factory since it was using the member reference before. I'll try to
lower the similarity threshold to see if I can get Rietveld to show a diff

[Marc Treib, 2017-04-28 15:06:34]

C++ LGTM with one comment

https://codereview.chromium.org/2844033002/diff/60001/chrome/browser/android/...
File chrome/browser/android/ntp/suggestions_event_reporter_bridge.cc (right):

https://codereview.chromium.org/2844033002/diff/60001/chrome/browser/android/...
chrome/browser/android/ntp/suggestions_event_reporter_bridge.cc:46:
DCHECK(content_suggestions_service);  // TODO(dgn): remove
:)

[dgn, 2017-05-02 13:42:42]

Thanks! PTAL

https://codereview.chromium.org/2844033002/diff/60001/chrome/browser/android/...
File chrome/browser/android/ntp/suggestions_event_reporter_bridge.cc (right):

https://codereview.chromium.org/2844033002/diff/60001/chrome/browser/android/...
chrome/browser/android/ntp/suggestions_event_reporter_bridge.cc:46:
DCHECK(content_suggestions_service);  // TODO(dgn): remove
On 2017/04/28 15:06:34, Marc Treib wrote:
> :)

Done.

[Bernhard Bauer, 2017-05-03 13:17:42]

LGTM!

[dgn, 2017-05-03 13:43:52]

The CQ bit was checked by dgn@chromium.org

[dgn, 2017-05-03 13:43:53]

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

[commit-bot: I haz the power, 2017-05-03 13:44:10]

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-05-03 13:50:47]

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

[commit-bot: I haz the power, 2017-05-03 13:50:48]

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...)

[dgn, 2017-05-03 13:57:03]

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

[dgn, 2017-05-03 13:57:03]

skyostil@chromium.org: Please review changes in //chrome/test/android/BUILD.gn

[Sami, 2017-05-04 00:36:11]

chrome/test/android/BUILD.gn lgtm.

[dgn, 2017-05-04 09:43:55]

The CQ bit was checked by dgn@chromium.org

[commit-bot: I haz the power, 2017-05-04 09:44: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-05-04 10:23:29]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493891035733180,
"parent_rev": "ba0dacfc73d208fc779907890bc94dbd09ef1aad", "commit_rev":
"ba0a51b986fd26385a2e1009a8e2ec974bd43639"}

[commit-bot: I haz the power, 2017-05-04 10:23:41]

Description was changed from

==========
[Suggestions] Move metrics and scheduling events out of SnippetsBridge

Extracts a SuggestionsEventReporter out of SnippetsBridge, that is
responsible for recording backend metrics and sending events used by
the fetch scheduler.

BUG=710268
==========

to

==========
[Suggestions] Move metrics and scheduling events out of SnippetsBridge

Extracts a SuggestionsEventReporter out of SnippetsBridge, that is
responsible for recording backend metrics and sending events used by
the fetch scheduler.

BUG=710268

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

[commit-bot: I haz the power, 2017-05-04 10:23:42]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/ba0a51b986fd26385a2e1009a8e2...