Create and integrate a metrics service client into Blimp engine.

Create and integrate a metrics service client into Blimp engine.

This is a reland of https://codereview.chromium.org/1885673003/

This will allow Blimp to collect and upload metrics to aid in
development.

BUG=592757
Committed: https://crrev.com/53edab71d14e3dbd4b9b767d6708e661bf329648
Cr-Commit-Position: refs/heads/master@{#393063}

[Jess, 2016-05-04 16:38:09]

Description was changed from

==========
Create and integrate a metrics service client into Blimp engine.

This is a reland of https://codereview.chromium.org/1885673003/

This will allow Blimp to collect and upload metrics to aid in
development.

BUG=592757

Committed: https://crrev.com/0be9daea216b9144830da20bcb3e2a56591207e6
Cr-Commit-Position: refs/heads/master@{#391360}

patch from issue 1885673003 at patchset 240001
(http://crrev.com/1885673003#ps240001)
==========

to

==========
Create and integrate a metrics service client into Blimp engine.

This is a reland of https://codereview.chromium.org/1885673003/

This will allow Blimp to collect and upload metrics to aid in
development.

BUG=592757
==========

[Jess, 2016-05-09 21:12:25]

jessicag@chromium.org changed reviewers:
+ asvitkine@chromium.org, battre@chromium.org, bauerb@chromium.org,
marshallk@google.com, wez@chromium.org

[Jess, 2016-05-09 21:12:26]

This is a reland of the previous review.  The first patch here is equivalent to
the failed committed patch from that review.

bauerb@, asvitkine@, battre@: Please review for DEP additions. There should not
significant change from before.

wez@, marshallk@: There has been refactoring for test support. The new test for
blimp_browser_context is limited to issues on construction and destruction, but
would have caught last weeks failure.

[Bernhard Bauer, 2016-05-10 08:51:42]

https://codereview.chromium.org/1945393002/diff/100001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_metrics_service_client.cc (right):

https://codereview.chromium.org/1945393002/diff/100001/blimp/engine/app/blimp...
blimp/engine/app/blimp_metrics_service_client.cc:62:
DCHECK(metrics_state_manager_.get());
Nit: std::unique_ptr has a bool conversion operator, so you don't need the
.get() if you just want to test for non-null.

https://codereview.chromium.org/1945393002/diff/100001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_metrics_service_client_unittest.cc (right):

https://codereview.chromium.org/1945393002/diff/100001/blimp/engine/app/blimp...
blimp/engine/app/blimp_metrics_service_client_unittest.cc:70:
BlimpMetricsServiceClient* sc = client_.release();
Just call client_.reset()?

https://codereview.chromium.org/1945393002/diff/100001/blimp/engine/common/bl...
File blimp/engine/common/blimp_browser_context_unittest.cc (right):

https://codereview.chromium.org/1945393002/diff/100001/blimp/engine/common/bl...
blimp/engine/common/blimp_browser_context_unittest.cc:20:
BlimpBrowserContextTest() {}
You can just leave out the constructor if it's empty.

[Jess, 2016-05-10 17:30:03]

https://codereview.chromium.org/1945393002/diff/100001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_metrics_service_client.cc (right):

https://codereview.chromium.org/1945393002/diff/100001/blimp/engine/app/blimp...
blimp/engine/app/blimp_metrics_service_client.cc:62:
DCHECK(metrics_state_manager_.get());
On 2016/05/10 08:51:42, Bernhard Bauer wrote:
> Nit: std::unique_ptr has a bool conversion operator, so you don't need the
> .get() if you just want to test for non-null.

Done.

https://codereview.chromium.org/1945393002/diff/100001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_metrics_service_client_unittest.cc (right):

https://codereview.chromium.org/1945393002/diff/100001/blimp/engine/app/blimp...
blimp/engine/app/blimp_metrics_service_client_unittest.cc:70:
BlimpMetricsServiceClient* sc = client_.release();
On 2016/05/10 08:51:42, Bernhard Bauer wrote:
> Just call client_.reset()?

Done.

https://codereview.chromium.org/1945393002/diff/100001/blimp/engine/common/bl...
File blimp/engine/common/blimp_browser_context_unittest.cc (right):

https://codereview.chromium.org/1945393002/diff/100001/blimp/engine/common/bl...
blimp/engine/common/blimp_browser_context_unittest.cc:20:
BlimpBrowserContextTest() {}
On 2016/05/10 08:51:42, Bernhard Bauer wrote:
> You can just leave out the constructor if it's empty.

Done.

[Alexei Svitkine (slow), 2016-05-10 17:38:07]

lgtm

still lgtm with some new nits

https://codereview.chromium.org/1945393002/diff/120001/blimp/engine/common/bl...
File blimp/engine/common/blimp_browser_context_unittest.cc (right):

https://codereview.chromium.org/1945393002/diff/120001/blimp/engine/common/bl...
blimp/engine/common/blimp_browser_context_unittest.cc:19: protected:
Nit: Begin with a public section. Probably fine to have SetUp/TearDown be public
here.

https://codereview.chromium.org/1945393002/diff/120001/blimp/engine/common/bl...
blimp/engine/common/blimp_browser_context_unittest.cc:22: private:
Nit: Empty line above this.

https://codereview.chromium.org/1945393002/diff/120001/blimp/engine/common/bl...
blimp/engine/common/blimp_browser_context_unittest.cc:26: };
Nit: DISALLOW_COPY_AND_ASSIGN().

https://codereview.chromium.org/1945393002/diff/120001/blimp/engine/common/bl...
blimp/engine/common/blimp_browser_context_unittest.cc:28: void
BlimpBrowserContextTest::SetUp() {
Nit: You can probably declare these inline.

[Jess, 2016-05-10 18:23:49]

https://codereview.chromium.org/1945393002/diff/120001/blimp/engine/common/bl...
File blimp/engine/common/blimp_browser_context_unittest.cc (right):

https://codereview.chromium.org/1945393002/diff/120001/blimp/engine/common/bl...
blimp/engine/common/blimp_browser_context_unittest.cc:19: protected:
On 2016/05/10 17:38:07, Alexei Svitkine wrote:
> Nit: Begin with a public section. Probably fine to have SetUp/TearDown be
public
> here.

Created a public section for the default constructor that now must be included
explicitly due to the addition of DISALLOW_COPY_AND_ASSIGN.

https://codereview.chromium.org/1945393002/diff/120001/blimp/engine/common/bl...
blimp/engine/common/blimp_browser_context_unittest.cc:22: private:
On 2016/05/10 17:38:07, Alexei Svitkine wrote:
> Nit: Empty line above this.

Done.

https://codereview.chromium.org/1945393002/diff/120001/blimp/engine/common/bl...
blimp/engine/common/blimp_browser_context_unittest.cc:26: };
On 2016/05/10 17:38:07, Alexei Svitkine wrote:
> Nit: DISALLOW_COPY_AND_ASSIGN().

Done.

https://codereview.chromium.org/1945393002/diff/120001/blimp/engine/common/bl...
blimp/engine/common/blimp_browser_context_unittest.cc:28: void
BlimpBrowserContextTest::SetUp() {
On 2016/05/10 17:38:07, Alexei Svitkine wrote:
> Nit: You can probably declare these inline.

Done.

[Bernhard Bauer, 2016-05-11 08:53:14]

lgtm

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_metrics_service_client_unittest.cc (right):

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
blimp/engine/app/blimp_metrics_service_client_unittest.cc:28: void
HandleReadError(PersistentPrefStore::PrefReadError error) {
Put this into an anonymous namespace?

[Jess, 2016-05-11 17:07:25]

Thanks for the DEPs reviews!

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_metrics_service_client_unittest.cc (right):

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
blimp/engine/app/blimp_metrics_service_client_unittest.cc:28: void
HandleReadError(PersistentPrefStore::PrefReadError error) {
On 2016/05/11 08:53:14, Bernhard Bauer wrote:
> Put this into an anonymous namespace?

Done.

[Jess, 2016-05-11 17:07:26]

Thanks for the DEPs reviews!

[Kevin M, 2016-05-11 17:08:41]

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

[Kevin M, 2016-05-11 17:08:42]

Some minor-ish stuff...

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_metrics_service_client.cc (right):

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
blimp/engine/app/blimp_metrics_service_client.cc:29: // How often after initial
logging metrics results should be uploaded to the
nit: add whitespace above comment

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
blimp/engine/app/blimp_metrics_service_client.cc:31: const int
kStandardUploadIntervalMinutes = 30;
This seems quite long? Would this mean that we miss out on metrics telemetry
from short lived sessions with Blimp?

Since this is happening on the engine side, aren't we able to increase the
frequency without any battery/CPU cost to the user?

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
blimp/engine/app/blimp_metrics_service_client.cc:33: // Returns if the
MetricsService should be recording metrics information for
nit: returns true

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
blimp/engine/app/blimp_metrics_service_client.cc:66: 
nit: consider clumping all metrics_service_ creation/initialization into one
contiguous block

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
blimp/engine/app/blimp_metrics_service_client.cc:91: metrics_service_->Stop();
This isn't necessary; metrics_service_ stops itself on destruction

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_metrics_service_client.h (right):

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
blimp/engine/app/blimp_metrics_service_client.h:31: // BlimpMetricsServiceClient
provides an implementation of MetricsServiceClient
Can you add a comment explaining why we need this?

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_metrics_service_client_unittest.cc (right):

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
blimp/engine/app/blimp_metrics_service_client_unittest.cc:33: void SetUp()
override;
optional nit: since this is in a .cc file, you can inline the implementation in
the class definition if you prefer.

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
blimp/engine/app/blimp_metrics_service_client_unittest.cc:36: void
SetUpPrefRegistry();
nit: Can you just merge these into commented regions of SetUp()? SetUp is the
only caller of these functions.

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
blimp/engine/app/blimp_metrics_service_client_unittest.cc:44:
std::unique_ptr<BlimpMetricsServiceClient> client_;
DISALLOW_COPY_AND_ASSIGN(...);

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/common/bl...
File blimp/engine/common/blimp_browser_context_unittest.cc (right):

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/common/bl...
blimp/engine/common/blimp_browser_context_unittest.cc:9: #include
"base/run_loop.h"
nit: not used

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/common/bl...
blimp/engine/common/blimp_browser_context_unittest.cc:20: // Must have explict
constructor due to DISALLOW_COPY_AND_ASSIGN
nit: I don't think the comment is necessary; having a default constructor is
pretty mundane and doesn't warrant a justification :)

[Jess, 2016-05-11 20:17:30]

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_metrics_service_client.cc (right):

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
blimp/engine/app/blimp_metrics_service_client.cc:29: // How often after initial
logging metrics results should be uploaded to the
On 2016/05/11 17:08:42, Kevin M wrote:
> nit: add whitespace above comment

Done.

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
blimp/engine/app/blimp_metrics_service_client.cc:31: const int
kStandardUploadIntervalMinutes = 30;
On 2016/05/11 17:08:42, Kevin M wrote:
> This seems quite long? Would this mean that we miss out on metrics telemetry
> from short lived sessions with Blimp?
> 
> Since this is happening on the engine side, aren't we able to increase the
> frequency without any battery/CPU cost to the user?

This number is just taken from an existing use case.  I took a brief look at
updating it, but it became clear it we don't have the information yet to make
all that meaningful a selection.

Created http://crbug/611099 for tracking.

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
blimp/engine/app/blimp_metrics_service_client.cc:33: // Returns if the
MetricsService should be recording metrics information for
On 2016/05/11 17:08:42, Kevin M wrote:
> nit: returns true

Added note, but left longer explanation since the always true behavior is
temporary.

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
blimp/engine/app/blimp_metrics_service_client.cc:66: 
On 2016/05/11 17:08:42, Kevin M wrote:
> nit: consider clumping all metrics_service_ creation/initialization into one
> contiguous block

Done.

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
blimp/engine/app/blimp_metrics_service_client.cc:91: metrics_service_->Stop();
On 2016/05/11 17:08:42, Kevin M wrote:
> This isn't necessary; metrics_service_ stops itself on destruction

Done.

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_metrics_service_client.h (right):

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
blimp/engine/app/blimp_metrics_service_client.h:31: // BlimpMetricsServiceClient
provides an implementation of MetricsServiceClient
On 2016/05/11 17:08:42, Kevin M wrote:
> Can you add a comment explaining why we need this?

Added explanation of why.  Please take a look and let me know if I am assuming
too much context.

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_metrics_service_client_unittest.cc (right):

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
blimp/engine/app/blimp_metrics_service_client_unittest.cc:33: void SetUp()
override;
On 2016/05/11 17:08:42, Kevin M wrote:
> optional nit: since this is in a .cc file, you can inline the implementation
in
> the class definition if you prefer.

Done.

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
blimp/engine/app/blimp_metrics_service_client_unittest.cc:36: void
SetUpPrefRegistry();
On 2016/05/11 17:08:42, Kevin M wrote:
> nit: Can you just merge these into commented regions of SetUp()? SetUp is the
> only caller of these functions.

Done.

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/app/blimp...
blimp/engine/app/blimp_metrics_service_client_unittest.cc:44:
std::unique_ptr<BlimpMetricsServiceClient> client_;
On 2016/05/11 17:08:42, Kevin M wrote:
> DISALLOW_COPY_AND_ASSIGN(...);

Done.

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/common/bl...
File blimp/engine/common/blimp_browser_context_unittest.cc (right):

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/common/bl...
blimp/engine/common/blimp_browser_context_unittest.cc:9: #include
"base/run_loop.h"
On 2016/05/11 17:08:42, Kevin M wrote:
> nit: not used

Done.

https://codereview.chromium.org/1945393002/diff/140001/blimp/engine/common/bl...
blimp/engine/common/blimp_browser_context_unittest.cc:20: // Must have explict
constructor due to DISALLOW_COPY_AND_ASSIGN
On 2016/05/11 17:08:42, Kevin M wrote:
> nit: I don't think the comment is necessary; having a default constructor is
> pretty mundane and doesn't warrant a justification :)

Done.
Only included it because I had been asked to remove the unused default
constructor in an earlier set of comments. :)

[Kevin M, 2016-05-11 20:36:32]

lgtm

[Jess, 2016-05-11 21:00:36]

The CQ bit was checked by jessicag@chromium.org

[Jess, 2016-05-11 21:00:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1945393002/#ps180001
(title: "Follow up on comments.")

[commit-bot: I haz the power, 2016-05-11 21:01:02]

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

[commit-bot: I haz the power, 2016-05-11 21:34:57]

Description was changed from

==========
Create and integrate a metrics service client into Blimp engine.

This is a reland of https://codereview.chromium.org/1885673003/

This will allow Blimp to collect and upload metrics to aid in
development.

BUG=592757
==========

to

==========
Create and integrate a metrics service client into Blimp engine.

This is a reland of https://codereview.chromium.org/1885673003/

This will allow Blimp to collect and upload metrics to aid in
development.

BUG=592757
==========

[commit-bot: I haz the power, 2016-05-11 21:34:58]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-05-11 21:36:09]

Description was changed from

==========
Create and integrate a metrics service client into Blimp engine.

This is a reland of https://codereview.chromium.org/1885673003/

This will allow Blimp to collect and upload metrics to aid in
development.

BUG=592757
==========

to

==========
Create and integrate a metrics service client into Blimp engine.

This is a reland of https://codereview.chromium.org/1885673003/

This will allow Blimp to collect and upload metrics to aid in
development.

BUG=592757

Committed: https://crrev.com/53edab71d14e3dbd4b9b767d6708e661bf329648
Cr-Commit-Position: refs/heads/master@{#393063}
==========

[commit-bot: I haz the power, 2016-05-11 21:36:11]

Patchset 10 (id:??) landed as
https://crrev.com/53edab71d14e3dbd4b9b767d6708e661bf329648
Cr-Commit-Position: refs/heads/master@{#393063}