Export stability metrics to UMA from Blimp engine

Export stability metrics to UMA from Blimp engine

BUG=612169
Committed: https://crrev.com/d3655e825ef57ce4543f96959523713d84a3b077
Cr-Commit-Position: refs/heads/master@{#401072}

[xyzzyz, 2016-06-08 23:08:15]

xyzzyz@chromium.org changed reviewers:
+ jessicag@chromium.org, wez@chromium.org

[Jess, 2016-06-09 18:09:59]

https://codereview.chromium.org/2045073002/diff/40001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_metrics_service_client_unittest.cc (right):

https://codereview.chromium.org/2045073002/diff/40001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_metrics_service_client_unittest.cc:102: // Needed for
stability metrics provider.
Actually it is needed by the client for several other reasons making this
comment misleading.  Please remove.

https://codereview.chromium.org/2045073002/diff/40001/blimp/engine/common/bli...
File blimp/engine/common/blimp_browser_context.cc (right):

https://codereview.chromium.org/2045073002/diff/40001/blimp/engine/common/bli...
blimp/engine/common/blimp_browser_context.cc:108: // Create an in memory
preferences store to hold metrics logs.
New line before comments that are not at the start of a code block.

[xyzzyz, 2016-06-09 20:01:25]

https://codereview.chromium.org/2045073002/diff/40001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_metrics_service_client_unittest.cc (right):

https://codereview.chromium.org/2045073002/diff/40001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_metrics_service_client_unittest.cc:102: // Needed for
stability metrics provider.
On 2016/06/09 18:09:59, Jess wrote:
> Actually it is needed by the client for several other reasons making this
> comment misleading.  Please remove.

This comment was meant to refer to NotificationService, but I forgot to move it
along with the notification_service_ member, but yeah, the comment is probably
not necessary.

https://codereview.chromium.org/2045073002/diff/40001/blimp/engine/common/bli...
File blimp/engine/common/blimp_browser_context.cc (right):

https://codereview.chromium.org/2045073002/diff/40001/blimp/engine/common/bli...
blimp/engine/common/blimp_browser_context.cc:108: // Create an in memory
preferences store to hold metrics logs.
On 2016/06/09 18:09:59, Jess wrote:
> New line before comments that are not at the start of a code block.

Done.

[Jess, 2016-06-09 22:52:50]

lgtm

[Wez, 2016-06-10 23:50:10]

https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_metrics_service_client_unittest.cc (right):

https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_metrics_service_client_unittest.cc:42: // Set up
NotificationService
nit: This and the other comments in this function are missing the . at the end
of the sentence!

nit: It'd be good to clarify _why_ we're setting this up, e.g. "Set up
NotificationService for the StabilityMetricsHelper to use."

https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_metrics_service_client_unittest.cc:63:
base::MessageLoop::current()->task_runner());
nit: Please use base::ThreadTaskRunnerHandle::Get() for this.

https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_stability_metrics_provider_unittest.cc (right):

https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_stability_metrics_provider_unittest.cc:67:
content::Source<std::nullptr_t>(nullptr);
nit: This looks strange - we need to specify nullptr_t explicitly because it
won't be inferred from nullptr?

https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/common/bli...
File blimp/engine/common/blimp_browser_context_unittest.cc (right):

https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/common/bli...
blimp/engine/common/blimp_browser_context_unittest.cc:24: // Set up
NotificationService
nit: Punctuation at end of line, and clarify _why_ we're doing this.

[Wez, 2016-06-10 23:50:10]

https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_metrics_service_client_unittest.cc (right):

https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_metrics_service_client_unittest.cc:42: // Set up
NotificationService
nit: This and the other comments in this function are missing the . at the end
of the sentence!

nit: It'd be good to clarify _why_ we're setting this up, e.g. "Set up
NotificationService for the StabilityMetricsHelper to use."

https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_metrics_service_client_unittest.cc:63:
base::MessageLoop::current()->task_runner());
nit: Please use base::ThreadTaskRunnerHandle::Get() for this.

https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_stability_metrics_provider_unittest.cc (right):

https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_stability_metrics_provider_unittest.cc:67:
content::Source<std::nullptr_t>(nullptr);
nit: This looks strange - we need to specify nullptr_t explicitly because it
won't be inferred from nullptr?

https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/common/bli...
File blimp/engine/common/blimp_browser_context_unittest.cc (right):

https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/common/bli...
blimp/engine/common/blimp_browser_context_unittest.cc:24: // Set up
NotificationService
nit: Punctuation at end of line, and clarify _why_ we're doing this.

[xyzzyz, 2016-06-17 20:16:58]

https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_metrics_service_client_unittest.cc (right):

https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_metrics_service_client_unittest.cc:42: // Set up
NotificationService
On 2016/06/10 23:50:10, Wez wrote:
> nit: This and the other comments in this function are missing the . at the end
> of the sentence!
> 
> nit: It'd be good to clarify _why_ we're setting this up, e.g. "Set up
> NotificationService for the StabilityMetricsHelper to use."

Done.

https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_metrics_service_client_unittest.cc:63:
base::MessageLoop::current()->task_runner());
On 2016/06/10 23:50:10, Wez wrote:
> nit: Please use base::ThreadTaskRunnerHandle::Get() for this.

Done.

https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_stability_metrics_provider_unittest.cc (right):

https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_stability_metrics_provider_unittest.cc:67:
content::Source<std::nullptr_t>(nullptr);
On 2016/06/10 23:50:10, Wez wrote:
> nit: This looks strange - we need to specify nullptr_t explicitly because it
> won't be inferred from nullptr?

Exactly:

../../blimp/engine/app/blimp_stability_metrics_provider_unittest.cc:67:16:
error: cannot refer to class template 'Source' without a template argument list
      content::Source(nullptr);
      ~~~~~~~~~^

https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/common/bli...
File blimp/engine/common/blimp_browser_context_unittest.cc (right):

https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/common/bli...
blimp/engine/common/blimp_browser_context_unittest.cc:24: // Set up
NotificationService
On 2016/06/10 23:50:10, Wez wrote:
> nit: Punctuation at end of line, and clarify _why_ we're doing this.

Done.

[maniscalco, 2016-06-17 20:45:42]

maniscalco@chromium.org changed reviewers:
+ maniscalco@chromium.org

[maniscalco, 2016-06-17 20:45:43]

Drive-by question...

https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_stability_metrics_provider_unittest.cc (right):

https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_stability_metrics_provider_unittest.cc:67:
content::Source<std::nullptr_t>(nullptr);
On 2016/06/17 20:16:58, xyzzyz wrote:
> On 2016/06/10 23:50:10, Wez wrote:
> > nit: This looks strange - we need to specify nullptr_t explicitly because it
> > won't be inferred from nullptr?
> 
> Exactly:
> 
> ../../blimp/engine/app/blimp_stability_metrics_provider_unittest.cc:67:16:
> error: cannot refer to class template 'Source' without a template argument
list
>       content::Source(nullptr);
>       ~~~~~~~~~^

Should we just use AllSources here?

https://cs.chromium.org/chromium/src/content/public/browser/notification_serv...

https://cs.chromium.org/chromium/src/content/public/browser/notification_sour...

[xyzzyz, 2016-06-17 21:25:15]

On 2016/06/17 20:45:43, maniscalco wrote:
> Drive-by question...
> 
>
https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/app/blimp_...
> File blimp/engine/app/blimp_stability_metrics_provider_unittest.cc (right):
> 
>
https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/app/blimp_...
> blimp/engine/app/blimp_stability_metrics_provider_unittest.cc:67:
> content::Source<std::nullptr_t>(nullptr);
> On 2016/06/17 20:16:58, xyzzyz wrote:
> > On 2016/06/10 23:50:10, Wez wrote:
> > > nit: This looks strange - we need to specify nullptr_t explicitly because
it
> > > won't be inferred from nullptr?
> > 
> > Exactly:
> > 
> > ../../blimp/engine/app/blimp_stability_metrics_provider_unittest.cc:67:16:
> > error: cannot refer to class template 'Source' without a template argument
> list
> >       content::Source(nullptr);
> >       ~~~~~~~~~^
> 
> Should we just use AllSources here?
> 
>
https://cs.chromium.org/chromium/src/content/public/browser/notification_serv...
> 
>
https://cs.chromium.org/chromium/src/content/public/browser/notification_sour...

From what I understand, AllSources is used to represent all possible sources of
notifications, when you are registering for them. Here, we need to use a
"concrete" source from which our notification came from. In the end, it doesn't
make a difference though, as we don't use the source anyway (that's why the
variable is called irrelevant_source).

It's worth comparing it to the original test of the
ChromeStabilityMetricsProvider, which goes to great lengths to set up the proper
source:
https://cs.chromium.org/chromium/src/chrome/browser/metrics/chrome_stability_...

[Wez, 2016-06-21 00:14:10]

lgtm

https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_stability_metrics_provider_unittest.cc (right):

https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_stability_metrics_provider_unittest.cc:67:
content::Source<std::nullptr_t>(nullptr);
On 2016/06/17 20:45:43, maniscalco wrote:
> On 2016/06/17 20:16:58, xyzzyz wrote:
> > On 2016/06/10 23:50:10, Wez wrote:
> > > nit: This looks strange - we need to specify nullptr_t explicitly because
it
> > > won't be inferred from nullptr?
> > 
> > Exactly:
> > 
> > ../../blimp/engine/app/blimp_stability_metrics_provider_unittest.cc:67:16:
> > error: cannot refer to class template 'Source' without a template argument
> list
> >       content::Source(nullptr);
> >       ~~~~~~~~~^
> 
> Should we just use AllSources here?
> 
>
https://cs.chromium.org/chromium/src/content/public/browser/notification_serv...
> 
>
https://cs.chromium.org/chromium/src/content/public/browser/notification_sour...

AllSources is just content::Source<void>(nullptr), so presumably the only way in
which it could differ from the test as-is is if T=void has special handling that
T=nullptr_t won't?

[xyzzyz, 2016-06-21 18:18:11]

On 2016/06/21 00:14:10, Wez wrote:
> lgtm
> 
>
https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/app/blimp_...
> File blimp/engine/app/blimp_stability_metrics_provider_unittest.cc (right):
> 
>
https://codereview.chromium.org/2045073002/diff/80001/blimp/engine/app/blimp_...
> blimp/engine/app/blimp_stability_metrics_provider_unittest.cc:67:
> content::Source<std::nullptr_t>(nullptr);
> On 2016/06/17 20:45:43, maniscalco wrote:
> > On 2016/06/17 20:16:58, xyzzyz wrote:
> > > On 2016/06/10 23:50:10, Wez wrote:
> > > > nit: This looks strange - we need to specify nullptr_t explicitly
because
> it
> > > > won't be inferred from nullptr?
> > > 
> > > Exactly:
> > > 
> > > ../../blimp/engine/app/blimp_stability_metrics_provider_unittest.cc:67:16:
> > > error: cannot refer to class template 'Source' without a template argument
> > list
> > >       content::Source(nullptr);
> > >       ~~~~~~~~~^
> > 
> > Should we just use AllSources here?
> > 
> >
>
https://cs.chromium.org/chromium/src/content/public/browser/notification_serv...
> > 
> >
>
https://cs.chromium.org/chromium/src/content/public/browser/notification_sour...
> 
> AllSources is just content::Source<void>(nullptr), so presumably the only way
in
> which it could differ from the test as-is is if T=void has special handling
that
> T=nullptr_t won't?

Yes. The test doesn't care about the source value though, so it doesn't make any
difference. My point is that AllSources() is not semantically correct, as it's
not a source, but just a pattern for sources.

[xyzzyz, 2016-06-21 18:18:26]

The CQ bit was checked by xyzzyz@chromium.org

[xyzzyz, 2016-06-21 18:18:27]

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

[commit-bot: I haz the power, 2016-06-21 18:19:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2045073002/100001

[commit-bot: I haz the power, 2016-06-21 19:28:38]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-21 19:32:21]

Description was changed from

==========
Export stability metrics to UMA from Blimp engine

BUG=612169
==========

to

==========
Export stability metrics to UMA from Blimp engine

BUG=612169

Committed: https://crrev.com/d3655e825ef57ce4543f96959523713d84a3b077
Cr-Commit-Position: refs/heads/master@{#401072}
==========

[commit-bot: I haz the power, 2016-06-21 19:32:23]

Patchset 6 (id:??) landed as
https://crrev.com/d3655e825ef57ce4543f96959523713d84a3b077
Cr-Commit-Position: refs/heads/master@{#401072}