Create and integrate a metrics service client into Blimp engine.

blimp/engine/app/blimp_metrics_service_client.h

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BLIMP_ENGINE_APP_BLIMP_METRICS_SERVICE_CLIENT_H_
+#define BLIMP_ENGINE_APP_BLIMP_METRICS_SERVICE_CLIENT_H_
+
+#include <stdint.h>
+
+#include <string>
+
+#include "base/lazy_instance.h"
+#include "base/macros.h"
+#include "components/metrics/metrics_service_client.h"
+
+class PrefService;
+
+namespace metrics {
+struct ClientInfo;
+class MetricsStateManager;
+}  // namespace metrics
+
+namespace net {
+class URLRequestContextGetter;
+}
+
+namespace blimp {
+namespace engine {
+
+// BlimpMetricsServiceClient provides an implementation of MetricsServiceClient
+// that depends on blimp. This singleton manages metrics for an app.
+// Metrics are always turned on. This class should always be used on the main
+// thread because MetricsService is single-threaded.
+// Based on AwMetricsServiceClient.

[Wez, 2016/04/26 01:38:19]

This comment is somewhat cryptic, IMO!

- It's not clear what "depends on blimp" means; are we saying that this is a
MetricsServiceClient tailored to the Blimp Engine?
- Not sure what you mean by "This singleton...for an app".
- Not clear why metrics being always-on is remarkable; might be clearer to
instead override the GetDefaultOptIn() method to always return ON, with a
suitable comment? Presumably we also need a TODO w/ bug filed to change the
behaviour before this is released?
- Comment talks about threads, but doesn't itself have threads, nor any
immediate mention of threads; in general it's best to avoid referring to details
of the surrounding system in comments describing "leaf" classes of this sort,
and instead document the assumptions their interface makes. In this case the
MetricsServiceClient _owns_ the MetricsService, and it seems that the
MetricsService expects only to be called from a single thread, so the constraint
is just that Initialize and Finalize must be called on the same thread - the
MetricsService takes care of calling to the MetricsServiceClient APIs on that
same thread itself?
- That this is based on AwMetricsServiceClient is only worth noting if there is
some particular pattern that that class used that you wanted to copy - if it's
just an example of another implementation then remove the comment; reader can
use code-search to find other examples if need be!

[Jess, 2016/04/26 21:24:11]

Inlining since various sub points require own responses.

On 2016/04/26 01:38:19, Wez wrote:
Rewritten PTAL.  I was operating under the assumption that key points in the Aw
comments reflected common comment patterns that would be generally relevant.
Are we already filing bugs for v0.6+?  I know this is included in some of our
documents of required work there, and was unsure if we had a preferred way of
tracking such items yet.

Also right now this is doing more than just setting the opt in but instead
ignoring that setting entirely and always reporting.
Sounds right to me.

Alexei can you confirm?
Done.

+class BlimpMetricsServiceClient : public metrics::MetricsServiceClient {
+  friend struct base::DefaultLazyInstanceTraits<BlimpMetricsServiceClient>;
+ public:
+  static BlimpMetricsServiceClient* GetInstance();

[Wez, 2016/04/26 01:38:18]

Document this API - e.g. does it return a process-global singleton? Does/should
the caller take a reference to it? Does it ever get deleted?

Also, since this is a singleton and the only APIs it really has are Initialize()
and Finalize(), did you consider making those stand alone methods in the blimp
namespace and moving the class declaration into the .cc file?  You'd then be
able to safely get rid of the private ctor/dtor and friending, and the
is_initialized bool (since you'd only create the instance in Initialize, so it'd
always be initialized).

[Jess, 2016/04/26 21:24:10]

I see what you mean about moving the class definition into the .cc and have done
that. However I am having a harder time following how you are propose to update
the interface.

+
+  void Initialize(PrefService* pref_service,
+                  net::URLRequestContextGetter* request_context);
+  void Finalize();
+
+  // metrics::MetricsServiceClient implementation:

[Wez, 2016/04/26 01:38:18]

nit: End comment with . not :

[Jess, 2016/04/26 21:24:11]

Done.

+  metrics::MetricsService* GetMetricsService() override;
+  void SetMetricsClientId(const std::string& client_id) override;
+  void OnRecordingDisabled() override;
+  bool IsOffTheRecordSessionActive() override;
+  int32_t GetProduct() override;
+  std::string GetApplicationLocale() override;
+  bool GetBrand(std::string* brand_code) override;
+  metrics::SystemProfileProto::Channel GetChannel() override;
+  std::string GetVersionString() override;
+  void OnLogUploadComplete() override;
+  void InitializeSystemProfileMetrics(
+      const base::Closure& done_callback) override;
+  void CollectFinalMetricsForLog(const base::Closure& done_callback) override;
+  std::unique_ptr<metrics::MetricsLogUploader> CreateUploader(
+      const base::Callback<void(int)>& on_upload_complete) override;
+  base::TimeDelta GetStandardUploadInterval() override;
+
+ private:
+  BlimpMetricsServiceClient();
+  ~BlimpMetricsServiceClient() override;
+
+  bool is_initialized_;

[Wez, 2016/04/26 01:38:19]

Suggest initializing this and other POD members in-line.

[Jess, 2016/04/26 21:24:11]

Done.

+  PrefService* pref_service_;
+  // URLRequestContextGetter* that remains valid for class lifetime for use by
+  // NetMetricsLogUploader in place of
+  // g_browser_process->system_request_context()

[Wez, 2016/04/26 01:38:18]

I'd suggest replacing this entire comment with just "Used by
NetMetricsLogUploader to create log-upload requests."

[Jess, 2016/04/26 21:24:11]

Done.

+  // In engine use subclass BlimpURLRequestContextGetter.

[Wez, 2016/04/26 01:38:18]

If this detail is important for the caller to know then comment on the
Initialize() call, where they provide the getter. If not then remove the
comment, since it's not relevant to the implementation.

[Jess, 2016/04/26 21:24:11]

Done.

+  // Member of g/e/common/blimp_browser_context.

[Wez, 2016/04/26 01:38:18]

nit:What is g/e/?

[Jess, 2016/04/26 21:24:10]

Comment cleaned up.

+  net::URLRequestContextGetter* request_context_;

[Wez, 2016/04/26 01:38:18]

This class is ref-counted - take a lovely safe reference to it, please :D

nit: This is the getter, not a context.

[Jess, 2016/04/26 21:24:11]

Done.

+  std::unique_ptr<metrics::MetricsStateManager> metrics_state_manager_;
+  std::unique_ptr<metrics::MetricsService> metrics_service_;
+
+  DISALLOW_COPY_AND_ASSIGN(BlimpMetricsServiceClient);
+};
+
+}  // namespace engine
+}  // namespace blimp
+
+#endif  // BLIMP_ENGINE_APP_BLIMP_METRICS_SERVICE_CLIENT_H_