Move NewTabPage.LogoShownTime metrics recording into LogoBridge

chrome/browser/android/logo_bridge.h

 // Copyright 2014 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_ANDROID_LOGO_BRIDGE_H_
 #define CHROME_BROWSER_ANDROID_LOGO_BRIDGE_H_
 
 #include <jni.h>
 
 #include <memory>
@@ skipping 17 matching lines @@
 
 namespace image_fetcher {
 class ImageFetcher;
 struct RequestMetadata;
 }  // namespace image_fetcher
 
 // The C++ counterpart to LogoBridge.java. Enables Java code to access the
 // default search provider's logo.
 class LogoBridge : public doodle::DoodleService::Observer {
  public:
+  // Stores its creation time and writes it to a UMA histogram. Each instance
+  // writes the metric once or never.
+  class LoadTimeMetricRecorder {

[tschumann, 2017/04/27 06:09:17]

this seems overly complicated to me. Why do we need to maintain so much state?
What are we abstracting here?

I guess the main question is why do we have a long-lived metric-recorder (as a
member) and not one per request. In the end, it's all about capturing a start
time and deciding whether to log it or not, based on conditions we already
know... (at the places where we now call the recorder).

The more state we can make request-scoped (i.e. not outlive the request), the
simpler the solution would get (e.g., one-time callbacks instead of registered
observers; per-request objects (passed through parameters) instead of members.
Happy to discuss offline.

[fhorschig, 2017/04/27 11:16:13]

I moved the state out of the class. 
Although this CL should be abandoned soon as we discussed to do this in the UI.

+   public:
+    // Writes the diff between |Now| and the recorders creation time to a UMA
+    // metric. If the recorder was aborted/finished before, nothing happens.
+    virtual void Finish() = 0;
+
+    // The recorder instance immediately stops to record.
+    virtual void Abort() = 0;
+
+    // The recorder waits for a pending result.
+    virtual void WaitForResult() = 0;
+
+    // Unless the recorder is waiting for a result, |Abort| will be called.
+    virtual void AbortIfNotWaiting() = 0;
+  };
+
   explicit LogoBridge(jobject j_profile);
   void Destroy(JNIEnv* env, const base::android::JavaParamRef<jobject>& obj);
 
   // Gets the current non-animated logo (downloading it if necessary) and passes
   // it to the observer.
   void GetCurrentLogo(
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& obj,
       const base::android::JavaParamRef<jobject>& j_logo_observer);
 
@@ skipping 20 matching lines @@
       bool from_cache);
 
   void DoodleImageFetched(bool config_from_cache,
                           const GURL& on_click_url,
                           const std::string& alt_text,
                           const GURL& animated_image_url,
                           const std::string& image_fetch_id,
                           const gfx::Image& image,
                           const image_fetcher::RequestMetadata& metadata);
 
+  // Returns the currently running LoadTimeRecorder. If there is none, a null
+  // object is returned.
+  LoadTimeMetricRecorder* load_time_recorder();
+
   // Only valid if UseNewDoodleApi is disabled.
   LogoService* logo_service_;
 
   // Only valid if UseNewDoodleApi is enabled.
   doodle::DoodleService* doodle_service_;
   std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher_;
   base::android::ScopedJavaGlobalRef<jobject> j_logo_observer_;
   ScopedObserver<doodle::DoodleService, doodle::DoodleService::Observer>
       doodle_observer_;
 
   std::unique_ptr<AnimatedLogoFetcher> animated_logo_fetcher_;
 
+  std::unique_ptr<LoadTimeMetricRecorder> load_time_recorder_;
+
   base::WeakPtrFactory<LogoBridge> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(LogoBridge);
 };
 
 bool RegisterLogoBridge(JNIEnv* env);
 
 #endif  // CHROME_BROWSER_ANDROID_LOGO_BRIDGE_H_