Log some information about the state of the homepage on Android.

chrome/browser/prefs/pref_metrics_service.h

 // Copyright 2013 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 CHROME_BROWSER_PREFS_PREF_METRICS_SERVICE_H_
 #define CHROME_BROWSER_PREFS_PREF_METRICS_SERVICE_H_
 
 #include <map>
 #include <memory>
 #include <string>
 
 #include "base/macros.h"
 #include "base/memory/singleton.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/keyed_service/content/browser_context_keyed_service_factory.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/syncable_prefs/synced_pref_change_registrar.h"
 
 class PrefRegistrySimple;
 
 // PrefMetricsService is responsible for recording prefs-related UMA stats.
 class PrefMetricsService : public KeyedService {
  public:
   explicit PrefMetricsService(Profile* profile);
   ~PrefMetricsService() override;
 
+  // Records metrics about the state of the homepage on launch.  On all
+  // platforms except Android this will be triggered automatically as part of
+  // initializing the PrefMetricsService.
+  //
+  // On Android, determining whether the homepage is enabled requires waiting
+  // for a response from a third party provider installed on the device.  Thus,
+  // this functionality is exposed to allow it to be called once all the
+  // necessary information is available.

[gab, 2016/05/25 14:03:24]

This comment should state what this method does, not how it's called.

The caller that omits OS_ANDROID is a good place to document how it's done
differently on Android (ref. other comment about changing if-def semantics).

[Mark P, 2016/05/25 17:01:53]

I agree with gab@ on this one; you can move everything but the first sentence of
this comment into the implementation.

[Ted C, 2016/05/25 18:13:07]

I personally think the comment about this being triggered during initialization
on all ports except android is relevant in the header.  Then readers will
understand quickly whether this is something they should be worrying about (w/o
having to look into the code).  I agree reasoning why android is different is
not important here and should be/is already in the code.

But if you still disagree strongly then I'll remove it as I don't want that to
be the thing that holds this up.

[gab, 2016/05/25 18:23:40]

If you want to mention where it's triggered, that mention should be on the
constructor of PrefMetricsService (where the trigger is).

Also, if you're going to state that it's not triggered on Android, keeping the
comment of what else is done on Android alongside makes sense too I'd say?

[Ted C, 2016/05/25 18:28:31]

I can't say I agree, but I'll just remove it entirely.

+  static void RecordHomePageLaunchMetrics(bool show_home_button,
+                                          bool homepage_is_ntp,
+                                          const GURL& homepage_url);
+
   class Factory : public BrowserContextKeyedServiceFactory {
    public:
     static Factory* GetInstance();
     static PrefMetricsService* GetForProfile(Profile* profile);
    private:
     friend struct base::DefaultSingletonTraits<Factory>;
 
     Factory();
     ~Factory() override;
 
@@ skipping 48 matching lines @@
 
   std::unique_ptr<syncable_prefs::SyncedPrefChangeRegistrar>
       synced_pref_change_registrar_;
 
   base::WeakPtrFactory<PrefMetricsService> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(PrefMetricsService);
 };
 
 #endif  // CHROME_BROWSER_PREFS_PREF_METRICS_SERVICE_H_