[Android]: Hide add-to-homescreen app menu item when WebAPK is installed

chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java

Index: chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java b/chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java
index d72051fda974fcb1a8dde4b2718dfd4b07361938..061745e450927a521c97a68f0f64275753b2585b 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java
@@ -4,6 +4,8 @@
 
 package org.chromium.chrome.browser.appmenu;
 
+import android.content.Context;
+import android.content.pm.ResolveInfo;
 import android.graphics.PorterDuff;
 import android.graphics.drawable.Drawable;
 import android.view.Menu;
@@ -11,6 +13,8 @@ import android.view.MenuItem;
 
 import org.chromium.base.ApiCompatibilityUtils;
 import org.chromium.base.CommandLine;
+import org.chromium.base.ContextUtils;
+import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.ChromeActivity;
 import org.chromium.chrome.browser.ChromeFeatureList;
@@ -28,6 +32,9 @@ import org.chromium.chrome.browser.share.ShareHelper;
 import org.chromium.chrome.browser.tab.Tab;
 import org.chromium.components.dom_distiller.core.DomDistillerUrlUtils;
 import org.chromium.ui.base.DeviceFormFactor;
+import org.chromium.webapk.lib.client.WebApkValidator;
+
+import java.util.concurrent.TimeUnit;
 
 /**
  * App Menu helper that handles hiding and showing menu items based on activity state.
@@ -162,13 +169,26 @@ public class AppMenuPropertiesDelegate {
             //                access to the resource via FLAG_GRANT_READ_URI_PERMISSION, and that
             //                is not persisted when adding to the homescreen.
             // * If creating shortcuts it not supported by the current home screen.
+            long addToHomeScreenStart = System.currentTimeMillis();

[dominickn, 2017/02/21 23:04:42]

This timing is currently mixing in the work needed to compute the add to
homescreen menu item as well as the open WebAPK item. Can this be untangled?

Ideally you should have:

1. get current time
2. query webapk
3. record time histogram
4. work out whether to show add to homescreen or open WebAPK

Because what we really care about is how long step 2 takes.

[gonzalon, 2017/02/22 15:50:49]

Done.

             MenuItem homescreenItem = menu.findItem(R.id.add_to_homescreen_id);
-            boolean homescreenItemVisible = ShortcutHelper.isAddToHomeIntentSupported(mActivity)
+            MenuItem openWebApkItem = menu.findItem(R.id.open_webapk_id);

[pkotwicz, 2017/02/21 22:52:32]

Based on reading the discussion on the bug (and comment #5), I think that
agreement was reached w.r.t graying out the menu item if the WebAPK is already
installed

[dominickn, 2017/02/21 23:04:42]

I thought that we were going to display "Open appname" like this CL implements?

[pkotwicz, 2017/02/22 15:04:51]

Yes Dominick you are right. I somehow missed Comment #8

+            boolean canBeAddedToHomeScreen = ShortcutHelper.isAddToHomeIntentSupported(mActivity)
                     && !isChromeScheme && !isFileScheme && !isContentScheme && !isIncognito;
-            if (homescreenItemVisible) {
+            Context context = ContextUtils.getApplicationContext();
+            ResolveInfo resolveInfo = WebApkValidator.queryWebApk(context, currentTab.getUrl());
+            boolean openWebApkItemVisible = canBeAddedToHomeScreen && resolveInfo != null
+                    && resolveInfo.activityInfo.packageName != null;
+            if (canBeAddedToHomeScreen && !openWebApkItemVisible) {

[dominickn, 2017/02/21 23:04:42]

I think the logic here is more clearly expressed like this:

if (homescreenItemVisible) {
  if (openWebApkItemVisible) {
    openWebApkItem.setTitle()
    openWebApkItem.setVisible(true);
    homescreenItem.setVisible(false);
  } else {
    homescreenItem.setTitle()
    homescreenItem.setVisible(true);
    openWebApkItem.setVisible(false);
  }
}

That way we're not continually (and redundantly) computing booleans so much, and
it's exceedingly clear that only one or the other of homescreenItem and
openWebApkItem can be visible at once.

[gonzalon, 2017/02/22 15:50:49]

Done.

                 homescreenItem.setTitle(AppBannerManager.getHomescreenLanguageOption());
             }
-            homescreenItem.setVisible(homescreenItemVisible);
+            if (openWebApkItemVisible) {
+                String appName = resolveInfo.loadLabel(context.getPackageManager()).toString();
+                openWebApkItem.setTitle(context.getString(R.string.menu_open_webapk, appName));
+            }
+            homescreenItem.setVisible(canBeAddedToHomeScreen && !openWebApkItemVisible);
+            openWebApkItem.setVisible(openWebApkItemVisible);
+            RecordHistogram.recordTimesHistogram("AndroidPrepareMenu.OpenWebAPK",

[pkotwicz, 2017/02/21 22:52:32]

How about "Android.PrepareMenu.DisableAddToHomescreenCheck"

- I think that the OWNERS of tools/metrics/histograms/histograms.xml don't like
introducing new namespaces

You also need to add an entry to tools/metrics/histograms/histograms.xml 

[dominickn, 2017/02/21 23:04:42]

+1 to all Peter's comments

[gonzalon, 2017/02/22 15:50:49]

Done.

+                    System.currentTimeMillis() - addToHomeScreenStart, TimeUnit.MILLISECONDS);

[pkotwicz, 2017/02/21 22:52:32]

From looking at the codebase, it looks like we prefer
SystemClock.elapsedRealtime() 

[gonzalon, 2017/02/22 15:50:49]

Done.

 
             // Hide request desktop site on all chrome:// pages except for the NTP. Check request
             // desktop site if it's activated on this page.