Correctly record WebAPK install source for Launch.HomescreenSource

chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java

 // 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.
 
 package org.chromium.chrome.browser.webapps;
 
 import android.content.Intent;
 import android.os.Bundle;
 import android.os.SystemClock;
 
 import org.chromium.base.ContextUtils;
 import org.chromium.base.library_loader.LibraryProcessType;
 import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.base.process_launcher.ChildProcessCreationParams;
+import org.chromium.chrome.browser.ShortcutSource;
 import org.chromium.chrome.browser.externalnav.ExternalNavigationParams;
+import org.chromium.chrome.browser.metrics.LaunchMetrics;
 import org.chromium.chrome.browser.metrics.WebApkUma;
 import org.chromium.chrome.browser.tab.BrowserControlsVisibilityDelegate;
 import org.chromium.chrome.browser.tab.InterceptNavigationDelegateImpl;
 import org.chromium.chrome.browser.tab.Tab;
 import org.chromium.chrome.browser.tab.TabDelegateFactory;
 import org.chromium.chrome.browser.tab.TabRedirectHandler;
 import org.chromium.components.navigation_interception.NavigationParams;
 import org.chromium.webapk.lib.client.WebApkServiceConnectionManager;
 
 import java.util.concurrent.TimeUnit;
 
 /**
  * An Activity is designed for WebAPKs (native Android apps) and displays a webapp in a nearly
  * UI-less Chrome.
  */
 public class WebApkActivity extends WebappActivity {
     /** Manages whether to check update for the WebAPK, and starts update check if needed. */
     private WebApkUpdateManager mUpdateManager;
 
     /** Indicates whether launching renderer in WebAPK process is enabled. */
     private boolean mCanLaunchRendererInWebApkProcess;
 
     private final ChildProcessCreationParams mDefaultParams =
             ChildProcessCreationParams.getDefault();
 
     /** The start time that the activity becomes focused. */
     private long mStartTime;
 
+    /** Indicates whether has committed the source info to the launch metrics. */
+    private boolean mHasCommitedLaunchMetrics;
+
     @Override
     protected WebappInfo createWebappInfo(Intent intent) {
         return (intent == null) ? WebApkInfo.createEmpty() : WebApkInfo.create(intent);
     }
 
     @Override
     protected void initializeUI(Bundle savedInstance) {
         super.initializeUI(savedInstance);
         getActivityTab().setWebappManifestScope(mWebappInfo.scopeUri().toString());
     }
@@ skipping 64 matching lines @@
 
     @Override
     public void onResumeWithNative() {
         super.onResumeWithNative();
 
         // When launching Chrome renderer in WebAPK process is enabled, WebAPK hosts Chrome's
         // renderer processes by declaring the Chrome's renderer service in its AndroidManifest.xml
         // and sets {@link ChildProcessCreationParams} for WebAPK's renderer process so the
         // {@link ChildProcessLauncher} knows which application's renderer service to connect to.
         initializeChildProcessCreationParams(mCanLaunchRendererInWebApkProcess);
+
+        // Records launch metrics for the WebAPK.
+        if (!mHasCommitedLaunchMetrics) {
+            mHasCommitedLaunchMetrics = true;
+            int source = mWebappInfo.source();
+            if (mWebappInfo.isLaunchedFromHomescreen()) {

[dominickn, 2017/05/10 06:04:46]

I'm not sure this works. isLaunchedFromHomescreen() just checks
mWebappInfo.source(), which is taken from the intent used to launch the WebAPK.
That source is unknown right - we have to get it from WebappDataStorage?

[Xi Han, 2017/05/10 20:49:58]

If the WebAPK is resumed by external Intent or notification, this check could
make sure we don't get the source from the WebappDataStorage, right?

+                WebappDataStorage storage =
+                        WebappRegistry.getInstance().getWebappDataStorage(mWebappInfo.id());
+                source = storage != null ? storage.getSource(ShortcutSource.WEBAPK_UNKNOWN)
+                                         : ShortcutSource.WEBAPK_UNKNOWN;
+                if (source == ShortcutSource.UNKNOWN) {
+                    source = ShortcutSource.WEBAPK_UNKNOWN;
+                    storage.updateSource(source);
+                }
+            }

[pkotwicz, 2017/05/09 18:37:36]

I think (Dominick might disagree) that we should determine whether the source is
ShortcutSource.UNKNOWN or ShortcutSource.WEBAPK_UNKNOWN prior to calling
LaunchMetrics#recordHomeScreenLaunchIntoStandaloneActivity()
In particular, we would not store WEBAPK_UNKNOWN into WebappDataStorage

[dominickn, 2017/05/10 06:04:46]

I agree that if possible, we shouldn't explicitly store UNKNOWN - we should just
default to WEBAPK_UNKNOWN if we don't get back anything from WebappDataStorage
(or just return WEBAPK_UNKNOWN as the default value out of WebappDataStorage).
If it's not possible I'm fine with explicitly storing it though.

[Xi Han, 2017/05/10 20:49:58]

Agree, it is neat to have WEBAPK_UNKNOWN as the default source for both Webapp
and WebAPKs.

[pkotwicz, 2017/05/10 21:08:59]

I wasn't suggesting changing the default source in WebappDataStorage. Not sure
if Dominick was suggesting it.

I was thinking of something like this in WebappLauncherActivity.java

if (validWebApk && webappSource == ShortcutSource.UNKNOWN) {
    WebappDataStorage storage =
        WebappRegistry.getInstance().getWebappDataStorage(webappInfo.id());
    if (storage != null) {
        webappSource = storage.getSource();
        if (webappSource == ShortcutSource.UNKNOWN) {
            webappSource = ShortcutSource.WEBAPK_UNKNOWN;
        }
    }
}
LaunchMetrics.recordHomeScreenLaunchIntoStandaloneActivity(webappUrl,
webappSource);

[Xi Han, 2017/05/11 17:14:28]

Ok, I must misunderstood. Revert the default source for Webapps to
ShortcutSource.UNKNOWN.

+            LaunchMetrics.recordHomeScreenLaunchIntoStandaloneActivity(
+                    mWebappInfo.uri().toString(), source);
+            LaunchMetrics.commitLaunchMetrics(getActivityTab().getWebContents());
+        }
     }
 
     @Override
     public void onResume() {
         super.onResume();
         mStartTime = SystemClock.elapsedRealtime();
     }
 
     @Override
     protected void recordIntentToCreationTime(long timeMs) {
         super.recordIntentToCreationTime(timeMs);
 
         RecordHistogram.recordTimesHistogram(
                 "MobileStartup.IntentToCreationTime.WebApk", timeMs, TimeUnit.MILLISECONDS);
     }
 
     @Override
     protected void onDeferredStartupWithStorage(WebappDataStorage storage) {
         super.onDeferredStartupWithStorage(storage);
 
         mUpdateManager = new WebApkUpdateManager(WebApkActivity.this, storage);
         mUpdateManager.updateIfNeeded(getActivityTab(),
                 (WebApkInfo) mWebappInfo);
     }
 
     @Override
     protected void onDeferredStartupWithNullStorage() {
         super.onDeferredStartupWithNullStorage();
 
-        // Register the WebAPK. The WebAPK is not registered when it is created so it has to be
-        // registered now. The WebAPK may also become unregistered after a user clears Chrome's
-        // data.
+        // Register the WebAPK. The WebAPK was registered when it was created, but may also become
+        // unregistered after a user clears Chrome's data.
         WebappRegistry.getInstance().register(
                 mWebappInfo.id(), new WebappRegistry.FetchWebappDataStorageCallback() {
                     @Override
                     public void onWebappDataStorageRetrieved(WebappDataStorage storage) {
                         // Initialize the time of the last is-update-needed check with the
                         // registration time. This prevents checking for updates on the first run.
                         storage.updateTimeOfLastCheckForUpdatedWebManifest();
 
+                        storage.updateSource(ShortcutSource.WEBAPK_UNKNOWN);

[dominickn, 2017/05/10 06:04:46]

As noted above, I agree with Peter that we shouldn't explicitly store UNKNOWN,
but default to it in the WebAPK path if possible.

[Xi Han, 2017/05/10 20:49:58]

Removed.

                         onDeferredStartupWithStorage(storage);
                     }
                 });
     }
 
     @Override
     public void onPause() {
         super.onPause();
         initializeChildProcessCreationParams(false);
     }
@@ skipping 23 matching lines @@
     }
 
     @Override
     protected void onDestroyInternal() {
         if (mUpdateManager != null) {
             mUpdateManager.destroy();
         }
         super.onDestroyInternal();
     }
 }