Persist setup metrics and have Chrome report them during UMA upload.

components/metrics/file_metrics_provider.h

+// Copyright 2015 The Chromium Authors. All rights reserved.

[grt (UTC plus 2), 2016/02/04 15:36:44]

2016 :-)

[bcwhite, 2016/02/04 21:51:15]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_METRICS_FILE_METRICS_PROVIDER_H_
+#define COMPONENTS_METRICS_FILE_METRICS_PROVIDER_H_
+
+#include <atomic>

[grt (UTC plus 2), 2016/02/04 15:36:44]

unused

[bcwhite, 2016/02/04 21:51:15]

Done.

+#include <list>
+
+#include "base/callback.h"
+#include "base/files/file_path.h"
+#include "base/gtest_prod_util.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/synchronization/lock.h"
+#include "components/metrics/metrics_provider.h"
+
+class PrefRegistrySimple;
+class PrefService;
+
+namespace base {
+class MemoryMappedFile;
+class PersistentMemoryAllocator;
+}
+
+namespace metrics {
+
+class MetricsService;
+
+// FileMetricsProvider gathers and logs histograms written to files on disk.

[grt (UTC plus 2), 2016/02/04 15:36:44]

Please add some more details. For example, how often does it check for file(s),
what file(s) does it look for? Does it poll for them? It looks like a scan is
made for files each time the metrics service opens a new log. How often is that?
Also good to explain what RegisterFile does and how preferences are used to
track things.

[bcwhite, 2016/02/04 21:51:14]

Done.

+class FileMetricsProvider
+    : public metrics::MetricsProvider {
+ public:
+  enum FileType {
+    FILE_HISTOGRAMS_ATOMIC,  // Written atomically and never updated.
+    // TODO(bcwhite): Enable when read/write mem-mapped files are supported.
+    //FILE_HISTOGRAMS_ACTIVE,  // Metrics actively updated by other processes.
+  };
+
+ private:
+  // This is fully defined in the header file (rather than just a forward
+  // declaration) so it can be known to tests.
+  struct FileInformation {
+    base::FilePath path;
+    FileType type;
+    std::string prefs_key;

[grt (UTC plus 2), 2016/02/04 15:36:43]

#include <string> above

[bcwhite, 2016/02/04 21:51:14]

Done.

+    base::Time last_seen;

[grt (UTC plus 2), 2016/02/04 15:36:44]

#include "base/time/time.h"

[bcwhite, 2016/02/04 21:51:15]

Done.

+    scoped_ptr<base::MemoryMappedFile> mapped;
+    scoped_ptr<base::PersistentMemoryAllocator> allocator;
+
+    FileInformation();

[grt (UTC plus 2), 2016/02/04 15:36:43]

the ctor and dtor should come before the data members as per
https://google.github.io/styleguide/cppguide.html#Declaration_Order

[bcwhite, 2016/02/04 21:51:15]

Done.

+    ~FileInformation();
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(FileInformation);
+  };
+
+ public:
+  typedef std::list<scoped_ptr<FileInformation>> FileInformationList;

[grt (UTC plus 2), 2016/02/04 15:36:44]

nit: using FileInformationList = ...;

[grt (UTC plus 2), 2016/02/08 18:09:18]

ping (as per http://chromium-cpp.appspot.com/#core-whitelist)

[bcwhite, 2016/02/09 21:08:45]

Right.  I had wanted to talk with you about the reasons for that in person. 
Thanks for the link.  Done.

+
+  FileMetricsProvider(metrics::MetricsService* metrics_service,
+                      PrefService* local_state);
+  ~FileMetricsProvider() override;
+
+  void RegisterFile(const base::FilePath& path, FileType type,

[grt (UTC plus 2), 2016/02/04 15:36:43]

please document

[bcwhite, 2016/02/04 21:51:14]

Done.

+                    const std::string& prefs_key);

[grt (UTC plus 2), 2016/02/04 15:36:44]

nit: base::StringPiece prefs_key

[bcwhite, 2016/02/04 21:51:15]

Done.

+
+  static void RegisterPrefs(PrefRegistrySimple* prefs, const std::string& key);

[grt (UTC plus 2), 2016/02/04 15:36:43]

nit: base::StringPiece key

[bcwhite, 2016/02/04 21:51:14]

Done.

+
+  // metrics::MetricsDataProvider:
+  void OnDidCreateMetricsLog() override;

[grt (UTC plus 2), 2016/02/04 15:36:43]

prefer placing the inherited methods in the private: section unless they need to
be part of this class's public API (see
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/CwzjcWd9cYM/TcUUl...
and
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/MkBEHBx3g_4/_wP74...
for various discussions).

[bcwhite, 2016/02/04 21:51:14]

Done.

+  void RecordHistogramSnapshots(base::HistogramSnapshotManager* hsm) override;
+
+ private:
+  friend class FileMetricsProviderTest;
+  FRIEND_TEST_ALL_PREFIXES(FileMetricsProviderTest, AccessMetrics);
+
+  static void CheckAndMapNewMetricFiles(
+      FileInformationList* files,
+      base::Callback<void(FileInformationList*)> callback);

[grt (UTC plus 2), 2016/02/04 15:36:44]

note: pass callbacks by constref, although this callback should go away; see
comment in ScheduleFilesCheck in .cc file

[bcwhite, 2016/02/09 21:08:45]

Done.

+  static bool CheckAndMapNewMetrics(FileMetricsProvider::FileInformation* file);

[grt (UTC plus 2), 2016/02/04 15:36:43]

"FileMetricsProvider::" not needed here or in definition

[bcwhite, 2016/02/04 21:51:15]

Done.

+
+  void ScheduleFilesCheck();
+  void RecordFileInformation(FileInformationList* files);
+  void RecordFileAsSeen(FileInformation* file);
+
+  base::Lock lock_;

[grt (UTC plus 2), 2016/02/04 15:36:44]

Is this lock needed? It looks like everything would happen on the UI thread once
the call to RecordFileInformation is moved there. Or is there something about
the interaction with the metrics service that makes this need to be threadsafe?

+  FileInformationList metrics_files_;

[grt (UTC plus 2), 2016/02/04 15:36:44]

what's the advantage of having two lists? could the loop in
RecordHistogramSnapshots just begin with:
    if (!file->mapped)
      continue;  // Another attempt will be made on the next ScheduleFilesCheck.

[bcwhite, 2016/02/04 21:51:14]

Unmapped files (i.e. those to be checked for something new) are extracted from
this object and passed to the worker task so as to avoid having that "anonymous"
task point back to data owned by another object.  If they're mixed in the same
vector with mapped files, the separation would have to happen at every check.

All this multi-thread stuff is a pain.  It would be nice if metric collection
wasn't on the UI thread in the first place, but that's a battle for a later
date...

+  FileInformationList metrics_found_;
+  metrics::MetricsService* metrics_service_;
+  PrefService* pref_service_;
+
+  DISALLOW_COPY_AND_ASSIGN(FileMetricsProvider);
+};
+
+}  // namespace metrics
+
+#endif  // COMPONENTS_METRICS_FILE_METRICS_PROVIDER_H_