|
|
Description[Android]: Hide add-to-homescreen app menu item when WebAPK is installed
If the web page being shown has a related Web APK installed on the
phone, rather than saying 'Add to Homescreen' the menu item now says
'Open {app_name}'. (ex. 'Open Telegram').
For a more on-depth discussion on this please see the attached bug.
BUG=668258
Review-Url: https://codereview.chromium.org/2707993003
Cr-Commit-Position: refs/heads/master@{#453997}
Committed: https://chromium.googlesource.com/chromium/src/+/569d20972fa8cd57b3f6b666c0c4e596ada50640
Patch Set 1 #Patch Set 2 : Add new state for 'Add to Homescreen' Menu item #Patch Set 3 : Add new state for 'Add to Homescreen' Menu item #
Total comments: 18
Patch Set 4 : Add new state for 'Add to Homescreen' Menu item #
Total comments: 22
Patch Set 5 : Add new state for 'Add to Homescreen' Menu item #
Total comments: 13
Patch Set 6 : Add new state for 'Add to Homescreen' Menu item #
Total comments: 4
Patch Set 7 : Add new state for 'Add to Homescreen' Menu item #
Total comments: 5
Patch Set 8 : Add new state for 'Add to Homescreen' Menu item #
Total comments: 6
Patch Set 9 : Add new state for 'Add to Homescreen' Menu item #
Total comments: 4
Patch Set 10 : Add new state for 'Add to Homescreen' Menu item #
Total comments: 24
Patch Set 11 : Add new state for 'Add to Homescreen' Menu item #
Total comments: 4
Patch Set 12 : Add new state for 'Add to Homescreen' Menu item #
Total comments: 4
Patch Set 13 : Add new state for 'Add to Homescreen' Menu item #Patch Set 14 : Add new state for 'Add to Homescreen' Menu item #Patch Set 15 : Add new state for 'Add to Homescreen' Menu item #Messages
Total messages: 55 (16 generated)
gonzalon@chromium.org changed reviewers: + dominickn@chromium.org, hartmanng@chromium.org, pkotwicz@chromium.org
Hi! This is an implementation of the 'Open WebAPK' menu item as discussed on the bug 668258. Would you mind taking a look please?
Your CL looks good. Thank you for bearing with me while we argued what the right way of doing things was https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java (right): https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:174: MenuItem openWebApkItem = menu.findItem(R.id.open_webapk_id); 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 https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:190: RecordHistogram.recordTimesHistogram("AndroidPrepareMenu.OpenWebAPK", 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 https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:191: System.currentTimeMillis() - addToHomeScreenStart, TimeUnit.MILLISECONDS); From looking at the codebase, it looks like we prefer SystemClock.elapsedRealtime()
https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1783: Log.e(TAG, "Failed to open app : %s!", packageName, e); Nit: clarify this to be "open WebAPK". Should there be a toast or something to tell the user that it didn't work? Seems a bad experience to just no-op on failure. https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java (right): https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:42: public class AppMenuPropertiesDelegate { You also need to change CustomTabAppMenuPropertiesDelegate.java, which is used for CCT (and also has add to homescreen in the menu). https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:172: long addToHomeScreenStart = System.currentTimeMillis(); 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. https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:174: MenuItem openWebApkItem = menu.findItem(R.id.open_webapk_id); On 2017/02/21 22:52:32, pkotwicz wrote: > 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 I thought that we were going to display "Open appname" like this CL implements? https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:181: if (canBeAddedToHomeScreen && !openWebApkItemVisible) { 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. https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:190: RecordHistogram.recordTimesHistogram("AndroidPrepareMenu.OpenWebAPK", +1 to all Peter's comments https://codereview.chromium.org/2707993003/diff/40001/chrome/android/webapk/l... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2707993003/diff/40001/chrome/android/webapk/l... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:84: private static List<ResolveInfo> resolveInfosForUrl(Context context, String url) { It would be nice if this method never returned null, and gave back an empty list when an exception happened. That way you can remove a bunch of null checks because they'll just work (iteration over an empty list). E.g. PackageManager#queryIntentActivities returns an empty list when it doesn't find anything.
Also: change the title and first line of the description to be something like, "Replace A2HS menu item with Open WebAPK when installed"
https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java (right): https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:174: MenuItem openWebApkItem = menu.findItem(R.id.open_webapk_id); Yes Dominick you are right. I somehow missed Comment #8
Description was changed from ========== Add new state for 'Add to Homescreen' Menu item. If the web page being shown has a related Web APK installed on the phone, rather than saying 'Add to Homescreen' the menu item now says 'Open {app_name}'. (ex. 'Open Telegram'). For a more on-depth discussion on this please see the attached bug. BUG=668258 ========== to ========== Replace A2HS menu item with Open WebAPK when installed If the web page being shown has a related Web APK installed on the phone, rather than saying 'Add to Homescreen' the menu item now says 'Open {app_name}'. (ex. 'Open Telegram'). For a more on-depth discussion on this please see the attached bug. BUG=668258 ==========
Thanks for your time reviewing! https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1783: Log.e(TAG, "Failed to open app : %s!", packageName, e); On 2017/02/21 23:04:42, dominickn wrote: > Nit: clarify this to be "open WebAPK". > > Should there be a toast or something to tell the user that it didn't work? Seems > a bad experience to just no-op on failure. Done, adn added the toast. https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java (right): https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:42: public class AppMenuPropertiesDelegate { On 2017/02/21 23:04:42, dominickn wrote: > You also need to change CustomTabAppMenuPropertiesDelegate.java, which is used > for CCT (and also has add to homescreen in the menu). Done. https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:172: long addToHomeScreenStart = System.currentTimeMillis(); On 2017/02/21 23:04:42, dominickn wrote: > 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. Done. https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:181: if (canBeAddedToHomeScreen && !openWebApkItemVisible) { On 2017/02/21 23:04:42, dominickn wrote: > 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. Done. https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:190: RecordHistogram.recordTimesHistogram("AndroidPrepareMenu.OpenWebAPK", On 2017/02/21 22:52:32, pkotwicz wrote: > 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 Done. https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:191: System.currentTimeMillis() - addToHomeScreenStart, TimeUnit.MILLISECONDS); On 2017/02/21 22:52:32, pkotwicz wrote: > From looking at the codebase, it looks like we prefer > SystemClock.elapsedRealtime() Done. https://codereview.chromium.org/2707993003/diff/40001/chrome/android/webapk/l... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2707993003/diff/40001/chrome/android/webapk/l... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:84: private static List<ResolveInfo> resolveInfosForUrl(Context context, String url) { On 2017/02/21 23:04:42, dominickn wrote: > It would be nice if this method never returned null, and gave back an empty list > when an exception happened. That way you can remove a bunch of null checks > because they'll just work (iteration over an empty list). > > E.g. PackageManager#queryIntentActivities returns an empty list when it doesn't > find anything. Done.
Looks good. A few comments https://codereview.chromium.org/2707993003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2707993003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1782: RecordUserAction.record("MobileMenuOpenWebAPK"); You need to add "MobileMenuOpenWebAPK" in tools/metrics/actions/actions.xml You might get it added for you automatically via the extract_actions.py script https://codereview.chromium.org/2707993003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1784: Log.e(TAG, "Failed to open WebAPK: %s", packageName, e); I don't think that this log is useful. https://codereview.chromium.org/2707993003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1787: } Shouldn't we show a toast in the else case? https://codereview.chromium.org/2707993003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java (right): https://codereview.chromium.org/2707993003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:229: RecordHistogram.recordTimesHistogram("Android.PrepareMenu.DisableAddToHomescreenCheck", Nit: Rename this histogram to "Android.PrepareMenu.OpenWebApkVisibilityCheck" to better indicate what the histogram is for (since you are not disabling the add-to-homescreen menu item when the WebAPK is already installed) https://codereview.chromium.org/2707993003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:237: if (homeScreenVisible) { I'd move the fetching of |resolveInfo| and the computation of |openWebApkItemVisible| here https://codereview.chromium.org/2707993003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:240: openWebApkItem.setTitle(context.getString(R.string.menu_open_webapk, appName)); Nit: Can you please add a new line? https://codereview.chromium.org/2707993003/diff/60001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2707993003/diff/60001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1590: <message name="IDS_OPEN_WEBAPK_FAILED" desc="Opening of a WebAPK fails. [CHAR-LIMIT=27]"> Nit: "fails" -> "failed" https://codereview.chromium.org/2707993003/diff/60001/chrome/android/webapk/l... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2707993003/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:43: public static String queryWebApkPackage(Context context, String url) { Can you make this function use queryResolveInfo()? https://codereview.chromium.org/2707993003/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:59: public static ResolveInfo queryWebApk(Context context, String url) { Can you rename this function to queryResolveInfo() ? https://codereview.chromium.org/2707993003/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:101: public static String findWebApkPackage(Context context, List<ResolveInfo> infos) { Can you create a new function findResolveInfo() and make findWebApkPackage() use it?
https://codereview.chromium.org/2707993003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2707993003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1782: RecordUserAction.record("MobileMenuOpenWebAPK"); On 2017/02/22 16:50:19, pkotwicz wrote: > You need to add "MobileMenuOpenWebAPK" in tools/metrics/actions/actions.xml You > might get it added for you automatically via the extract_actions.py script Done. https://codereview.chromium.org/2707993003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1784: Log.e(TAG, "Failed to open WebAPK: %s", packageName, e); On 2017/02/22 16:50:19, pkotwicz wrote: > I don't think that this log is useful. Done. https://codereview.chromium.org/2707993003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1787: } On 2017/02/22 16:50:19, pkotwicz wrote: > Shouldn't we show a toast in the else case? Done. https://codereview.chromium.org/2707993003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java (right): https://codereview.chromium.org/2707993003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:229: RecordHistogram.recordTimesHistogram("Android.PrepareMenu.DisableAddToHomescreenCheck", On 2017/02/22 16:50:19, pkotwicz wrote: > Nit: Rename this histogram to "Android.PrepareMenu.OpenWebApkVisibilityCheck" to > better indicate what the histogram is for (since you are not disabling the > add-to-homescreen menu item when the WebAPK is already installed) Done. https://codereview.chromium.org/2707993003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:237: if (homeScreenVisible) { On 2017/02/22 16:50:19, pkotwicz wrote: > I'd move the fetching of |resolveInfo| and the computation of > |openWebApkItemVisible| here Done. https://codereview.chromium.org/2707993003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:240: openWebApkItem.setTitle(context.getString(R.string.menu_open_webapk, appName)); On 2017/02/22 16:50:19, pkotwicz wrote: > Nit: Can you please add a new line? Done. https://codereview.chromium.org/2707993003/diff/60001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2707993003/diff/60001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:1590: <message name="IDS_OPEN_WEBAPK_FAILED" desc="Opening of a WebAPK fails. [CHAR-LIMIT=27]"> On 2017/02/22 16:50:19, pkotwicz wrote: > Nit: "fails" -> "failed" Done. https://codereview.chromium.org/2707993003/diff/60001/chrome/android/webapk/l... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2707993003/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:43: public static String queryWebApkPackage(Context context, String url) { On 2017/02/22 16:50:20, pkotwicz wrote: > Can you make this function use queryResolveInfo()? I don't understand why. Isn't that what findWebApkPackage is doing? We'd be repeating code, wouldn't we? https://codereview.chromium.org/2707993003/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:59: public static ResolveInfo queryWebApk(Context context, String url) { On 2017/02/22 16:50:20, pkotwicz wrote: > Can you rename this function to queryResolveInfo() ? Done. https://codereview.chromium.org/2707993003/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:101: public static String findWebApkPackage(Context context, List<ResolveInfo> infos) { On 2017/02/22 16:50:20, pkotwicz wrote: > Can you create a new function findResolveInfo() and make findWebApkPackage() use > it? Done.
L-G-T-M once you address these comments :) https://codereview.chromium.org/2707993003/diff/60001/chrome/android/webapk/l... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2707993003/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:43: public static String queryWebApkPackage(Context context, String url) { I wanted to merge resolveInfosForUrl() and queryResolveInfo() https://codereview.chromium.org/2707993003/diff/60001/chrome/android/webapk/l... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:56: * @return Resolve Info of a WebAPK which can handle the URL. Null if the url should not be Nit: "Resolve Info of a WebAPK" -> "ResolveInfo of WebAPK" https://codereview.chromium.org/2707993003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2707993003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1777: context.getPackageManager().getLaunchIntentForPackage(packageName); Nit: Maybe add a boolean variable "launched" that you set after Context#startActivity() is called. That way you don't call Toast#makeToast() from two locations https://codereview.chromium.org/2707993003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java (right): https://codereview.chromium.org/2707993003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:223: Can you add a comment to this function? How about: "Sets the visibility and labels of the "Add to Home screen" and "Open WebAPK" menu items." https://codereview.chromium.org/2707993003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:225: Menu menu, String url, boolean homeScreenVisible) { Perhaps rename |homeScreenVisible| to |homeScreenHidden| (and fix up the appropriate logic) because: - the homescreen item is not always visible when |homescreenVisible| is passed in - the homescreen item is always hidden when not |homescreenVisible| https://codereview.chromium.org/2707993003/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2707993003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:757: +<histogram name="Android.PrepareMenu.OpenWebApkVisibilityCheck" units="ms"> Can you please update the histogram name? https://codereview.chromium.org/2707993003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:761: + Measures the amount of time spent querying for a WebAPK name while preparing Nit: "querying for a WebAPK name" -> "querying for whether a WebAPK is already installed"
https://codereview.chromium.org/2707993003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2707993003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1777: context.getPackageManager().getLaunchIntentForPackage(packageName); On 2017/02/22 20:26:10, pkotwicz wrote: > Nit: Maybe add a boolean variable "launched" that you set after > Context#startActivity() is called. That way you don't call Toast#makeToast() > from two locations Done. https://codereview.chromium.org/2707993003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java (right): https://codereview.chromium.org/2707993003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:223: On 2017/02/22 20:26:10, pkotwicz wrote: > Can you add a comment to this function? > > How about: "Sets the visibility and labels of the "Add to Home screen" and "Open > WebAPK" menu items." Done. https://codereview.chromium.org/2707993003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:225: Menu menu, String url, boolean homeScreenVisible) { On 2017/02/22 20:26:10, pkotwicz wrote: > Perhaps rename |homeScreenVisible| to |homeScreenHidden| (and fix up the > appropriate logic) because: > - the homescreen item is not always visible when |homescreenVisible| is passed > in > - the homescreen item is always hidden when not |homescreenVisible| Well, if I call it homeScreenHidden and it's false, it may be hidden anyways, so we have the same problem. Also, it would make the code a bit more difficult to follow, because we'd have to think in the negative rather than the positive case. How about canShowHomeScreenItem? I think that better reflects what the variable means. Let me know what you think. https://codereview.chromium.org/2707993003/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2707993003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:757: +<histogram name="Android.PrepareMenu.OpenWebApkVisibilityCheck" units="ms"> On 2017/02/22 20:26:10, pkotwicz wrote: > Can you please update the histogram name? I'm not sure I understand, this is the name you suggested, isn't it? https://codereview.chromium.org/2707993003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:761: + Measures the amount of time spent querying for a WebAPK name while preparing On 2017/02/22 20:26:10, pkotwicz wrote: > Nit: "querying for a WebAPK name" -> "querying for whether a WebAPK is already > installed" Done.
Looks pretty good. I've just thought of another metric we should add (sorry for not having it in my earlier review :( ) https://codereview.chromium.org/2707993003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2707993003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1784: launchFailed = true; Can you add another metric here to record the failure reason? It would be good to keep an eye on that. I imagine it would have: 1. Open WebAPK from menu successfully 2. Could not get launch intent for WebAPK 3. WebAPK activity not found Ideally (2) and (3) should be zero, since it would be pretty bad if we got to here and failed to open (given that getting to here involves the check for a WebAPK succeeding when we built the menu, and that uses basically the same code). Hopefully there's not some quirky or weird navigation pattern where you could trigger the Open <appname> menu item and manage to fail this check when you tapped it. https://codereview.chromium.org/2707993003/diff/100001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2707993003/diff/100001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:1591: Failed to open Ideally this would be: Failed to open <appname> if we have an appname Otherwise something like "Could not launch app" if we don't have an app name.
Thanks for your time reviewing. Please don't hesitate to mention any other changes/additions, even if you didn't mention them before, I don't mind :) https://codereview.chromium.org/2707993003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2707993003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1784: launchFailed = true; On 2017/02/23 05:53:55, dominickn wrote: > Can you add another metric here to record the failure reason? It would be good > to keep an eye on that. I imagine it would have: > > 1. Open WebAPK from menu successfully > 2. Could not get launch intent for WebAPK > 3. WebAPK activity not found > > Ideally (2) and (3) should be zero, since it would be pretty bad if we got to > here and failed to open (given that getting to here involves the check for a > WebAPK succeeding when we built the menu, and that uses basically the same > code). Hopefully there's not some quirky or weird navigation pattern where you > could trigger the Open <appname> menu item and manage to fail this check when > you tapped it. I added the actions. I'm unsure if the way I did it is the correct one in this codebase, please correct me if this is not what you meant. https://codereview.chromium.org/2707993003/diff/100001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2707993003/diff/100001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:1591: Failed to open On 2017/02/23 05:53:55, dominickn wrote: > Ideally this would be: > > Failed to open <appname> if we have an appname > > Otherwise something like "Could not launch app" if we don't have an app name. We could or could not have the app name, depending on the cause of the error. I went with "Could not launch app" but let me know if you would prefer the other message in the cases that the app failed to launch but we do have access to its name. I can't think of a case where that would happen though, if this fails it would most likely be because the package name could not be found for the url, and even that shouldn't happen as you said in the other comment.
A few final comments about the user action recroded in ChromeActivity.java https://codereview.chromium.org/2707993003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java (right): https://codereview.chromium.org/2707993003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:225: Menu menu, String url, boolean homeScreenVisible) { Perhaps canShowHomeScreenMenuItem() to be consistent with prepareAddToHomescreenMenuItem() https://codereview.chromium.org/2707993003/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2707993003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:757: +<histogram name="Android.PrepareMenu.OpenWebApkVisibilityCheck" units="ms"> Sorry. For some reason I thought that the histogram name differed from the name passed to RecordHistogram.recordTimesHistogram() in Java code https://codereview.chromium.org/2707993003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2707993003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1790: } There are two different metrics: 1) The user action (The user tapped the menu item) 2) Whether Chrome was able to open the WebAPK as a result of the user's tap We want to use RecordUserAction.record() for #1 We want to use RecordHistogram.recordEnumeratedHistogram() for #2 I suggest naming the action "MobileMenuOpenWebApk" (instead of MobileMenuOpenWebAPK). I don't have an opinion as to whether the action should be recorded "all of the time" or "only when the WebAPK is successfully launched" I suggest naming the histogram "WebApk.OpenFromMenu" and the states SUCCESS = 0 NO_LAUNCH_INTENT = 1 ACTIVITY_NOT_FOUND = 2 https://codereview.chromium.org/2707993003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java (right): https://codereview.chromium.org/2707993003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:173: boolean homeScreenVisible = ShortcutHelper.isAddToHomeIntentSupported(mActivity) Nit: Rename this boolean to canShowHomeScreenMenuItem to match variable name in prepareAddToHomescreenMenuItem()
https://codereview.chromium.org/2707993003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2707993003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1790: } +1, except maybe call it LAUNCH_SUCCESS instead of just SUCCESS
https://codereview.chromium.org/2707993003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java (right): https://codereview.chromium.org/2707993003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:225: Menu menu, String url, boolean homeScreenVisible) { On 2017/02/23 20:46:17, pkotwicz wrote: > Perhaps canShowHomeScreenMenuItem() to be consistent with > prepareAddToHomescreenMenuItem() Done. https://codereview.chromium.org/2707993003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2707993003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1790: } On 2017/02/23 20:46:17, pkotwicz wrote: > There are two different metrics: > 1) The user action (The user tapped the menu item) > 2) Whether Chrome was able to open the WebAPK as a result of the user's tap > > We want to use RecordUserAction.record() for #1 > We want to use RecordHistogram.recordEnumeratedHistogram() for #2 > > I suggest naming the action "MobileMenuOpenWebApk" (instead of > MobileMenuOpenWebAPK). I don't have an opinion as to whether the action should > be recorded "all of the time" or "only when the WebAPK is successfully launched" > > I suggest naming the histogram "WebApk.OpenFromMenu" and the states > SUCCESS = 0 > NO_LAUNCH_INTENT = 1 > ACTIVITY_NOT_FOUND = 2 Done. https://codereview.chromium.org/2707993003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java (right): https://codereview.chromium.org/2707993003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:173: boolean homeScreenVisible = ShortcutHelper.isAddToHomeIntentSupported(mActivity) On 2017/02/23 20:46:17, pkotwicz wrote: > Nit: Rename this boolean to canShowHomeScreenMenuItem to match variable name in > prepareAddToHomescreenMenuItem() Done.
LGTM! with nits Thank you for bearing with me https://codereview.chromium.org/2707993003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java (right): https://codereview.chromium.org/2707993003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:62: */ Nits: "When pressing" -> "When a user presses" "on the Open WebAPK menu item" -> "on the "Open WebAPK" menu item" "representing the result of trying to open a WebAPK" -> "Result of trying to open WebAPK" https://codereview.chromium.org/2707993003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:65: RecordHistogram.recordEnumeratedHistogram(WEBAPK_OPEN_NAME, type, WEBAPK_OPEN_MAX); Nit: Just inline "WebApk.OpenFromMenu" https://codereview.chromium.org/2707993003/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2707993003/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:74351: + whether the opening of the WebAPK was successful when this menu item is Nit: "was successful" -> "is successful" Either both "was successful" and "was clicked" or "is successful" and "is clicked"
Nit: Can you please change the CL description [Android]: Hide add-to-homescreen app menu item when WebAPK is installed - Its nice to make it clear which platform is targetted by a CL - A2HS is not a commonly used acronym so I try to avoid it in CL titles and CL descriptions
On 2017/02/24 16:33:28, pkotwicz wrote: > Nit: Can you please change the CL description > > [Android]: Hide add-to-homescreen app menu item when WebAPK is installed > > - Its nice to make it clear which platform is targetted by a CL > - A2HS is not a commonly used acronym so I try to avoid it in CL titles and CL > descriptions That was the title suggested previously in this CL, I had written another one at first. I'll change it.
Description was changed from ========== Replace A2HS menu item with Open WebAPK when installed If the web page being shown has a related Web APK installed on the phone, rather than saying 'Add to Homescreen' the menu item now says 'Open {app_name}'. (ex. 'Open Telegram'). For a more on-depth discussion on this please see the attached bug. BUG=668258 ========== to ========== [Android]: Hide add-to-homescreen app menu item when WebAPK is installed If the web page being shown has a related Web APK installed on the phone, rather than saying 'Add to Homescreen' the menu item now says 'Open {app_name}'. (ex. 'Open Telegram'). For a more on-depth discussion on this please see the attached bug. BUG=668258 ==========
https://codereview.chromium.org/2707993003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java (right): https://codereview.chromium.org/2707993003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:62: */ On 2017/02/24 16:29:40, pkotwicz wrote: > Nits: > "When pressing" -> "When a user presses" > "on the Open WebAPK menu item" -> "on the "Open WebAPK" menu item" > > "representing the result of trying to open a WebAPK" -> "Result of trying to > open WebAPK" Done, but the last change goes against the wording used in the rest of this file. https://codereview.chromium.org/2707993003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:65: RecordHistogram.recordEnumeratedHistogram(WEBAPK_OPEN_NAME, type, WEBAPK_OPEN_MAX); On 2017/02/24 16:29:40, pkotwicz wrote: > Nit: Just inline "WebApk.OpenFromMenu" Done, but this goes against the way it's done on the rest of this file. https://codereview.chromium.org/2707993003/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2707993003/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:74351: + whether the opening of the WebAPK was successful when this menu item is On 2017/02/24 16:29:40, pkotwicz wrote: > Nit: "was successful" -> "is successful" > > Either both > "was successful" and "was clicked" > or > "is successful" and "is clicked" Done.
gonzalon@chromium.org changed reviewers: + holte@chromium.org, thestig@chromium.org, twellington@chromium.org
twellington@chromium.org: Please review changes in chrome/android/java/res/menu/main_menu.xml holte@chromium.org: Please review changes in tools/metrics thestig@chromium.org: Please review changes in chrome/android/java/src/org/chromium/chrome/browser/ and strings Hi! Would you mind taking a look at the files you have ownership over please? Thanks a lot for your time and please let me know if you are too busy.
On 2017/02/24 16:52:10, gonzalon wrote: > mailto:thestig@chromium.org: Please review changes in > chrome/android/java/src/org/chromium/chrome/browser/ and strings Please ask someone from chrome/android/OWNERS instead.
gonzalon@chromium.org changed reviewers: - thestig@chromium.org
gonzalon@chromium.org changed reviewers: + nyquist@chromium.org
Thanks Lei for pointing me in the right direction. nyquist@, would you mind taking a look at the chrome/android files on this CL please? Thanks for your time!
lgtm
https://codereview.chromium.org/2707993003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java (right): https://codereview.chromium.org/2707993003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:232: Context context = ContextUtils.getApplicationContext(); One final nit: You should only call this if WebAPKs are enabled (ChromeWebApkHost#isEnabled()) We don't want to give users to option of opening a WebAPK even if one is installed if WebAPKs are disabled (e.g. due to some aggregious bug)
lgtm https://codereview.chromium.org/2707993003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java (right): https://codereview.chromium.org/2707993003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:32: private static final int WEBAPK_OPEN_MAX = 3; By convention this should go at the end and probably have the same access modifier as the others.
https://codereview.chromium.org/2707993003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java (right): https://codereview.chromium.org/2707993003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:232: Context context = ContextUtils.getApplicationContext(); On 2017/02/25 02:36:04, pkotwicz wrote: > One final nit: You should only call this if WebAPKs are enabled > (ChromeWebApkHost#isEnabled()) > > We don't want to give users to option of opening a WebAPK even if one is > installed if WebAPKs are disabled (e.g. due to some aggregious bug) Done. https://codereview.chromium.org/2707993003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java (right): https://codereview.chromium.org/2707993003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:32: private static final int WEBAPK_OPEN_MAX = 3; On 2017/02/27 02:21:21, dominickn wrote: > By convention this should go at the end and probably have the same access > modifier as the others. Done.
I really appreciate you adding the UMA histogram for how long it takes to query the ResolveInfos. https://codereview.chromium.org/2707993003/diff/180001/chrome/android/java/re... File chrome/android/java/res/menu/main_menu.xml (left): https://codereview.chromium.org/2707993003/diff/180001/chrome/android/java/re... chrome/android/java/res/menu/main_menu.xml:103: Nit: Why is this line removed? https://codereview.chromium.org/2707993003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2707993003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1785: Context context = ContextUtils.getApplicationContext(); Is there a reason why you don't use |this| as the context? If something you call needs the application context instead of an activity context, shouldn't they be the ones to get the application context from the activity? https://codereview.chromium.org/2707993003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1786: String packageName = WebApkValidator.queryWebApkPackage(context, currentTab.getUrl()); This ended up being a very long section of the menu. Could you either extract this into its own method? It seems like the only thing it really needs is the URL from the current tab, which you could pass in, i.e. it could even be a private static method if you use the app context, or just an instance method if you end up using |this|. I guess you could even keep the Toast here if you want, doing something like: boolean launchFailed = openWebApkFromUrl(currentTab.getUrl()); if (launchFailed) { Toast.makeText(...); } That would at least make this part of the method so much easier to read. WDYT? https://codereview.chromium.org/2707993003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java (right): https://codereview.chromium.org/2707993003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:245: resolveInfo != null && resolveInfo.activityInfo.packageName != null; The null-check of packageName seems unnecessary here. If that is something that can happen, we would have already crashed in WebApkValidator#isValidWebApk(...): if (!webappPackageName.startsWith(WEBAPK_PACKAGE_PREFIX)) { ... Maybe you could move this null-check of the package name there instead, and just check if the resolveInfo is null here? At that point openWebApkItemVisible == resolveInfo != null, so we conceptually don't need both, but I find the variable name very helpful so I'd keep it to ensure it's easy to understand for new readers. https://codereview.chromium.org/2707993003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java (right): https://codereview.chromium.org/2707993003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:32: public static final int WEBAPK_OPEN_LAUNCH_SUCCESS = 0; Could you add a comment about this enum that refers to the UMA stat? https://codereview.chromium.org/2707993003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:62: public static void recordWebApkOpenAttempt(int type) { Any reason this isn't documented using @IntDef? Seems like that could be helpful here. https://codereview.chromium.org/2707993003/diff/180001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2707993003/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:52: * NOTE: This can fail if multiple WebAPKs can match the supplied url. What does fail mean in this scenario? From the code it looks like we will just be returning the first one? https://codereview.chromium.org/2707993003/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:55: * @param url The url to check. Nit: Comments are written in normal English, so URL should be all uppercase. I would be OK with |url| though, if you want to refer to the URL parameter specifically. Update the rest of the comment to URL too. https://codereview.chromium.org/2707993003/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:64: * Fetches the list of resolve infos from the PackageManager that can handle the URL. {@link ResolveInfo}s https://codereview.chromium.org/2707993003/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:70: private static List<ResolveInfo> resolveInfosForUrl(Context context, String url) { Would you want to annotate this with @Nonnull and/or update the @return statement? https://codereview.chromium.org/2707993003/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:139: e.printStackTrace(); While you're at it, could you pleas remove this line? If it's supposed to stay, add it as the third param to the log-statement below.
https://codereview.chromium.org/2707993003/diff/180001/chrome/android/java/re... File chrome/android/java/res/menu/main_menu.xml (left): https://codereview.chromium.org/2707993003/diff/180001/chrome/android/java/re... chrome/android/java/res/menu/main_menu.xml:103: On 2017/02/28 07:04:19, nyquist wrote: > Nit: Why is this line removed? There were two empty lines at the end of the file. There's only one now. I checked with a different text editor to make sure it wasn't just an error, not sure why this is happening, I can see one empty line after </menu> on my local machine. https://codereview.chromium.org/2707993003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2707993003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1785: Context context = ContextUtils.getApplicationContext(); On 2017/02/28 07:04:19, nyquist wrote: > Is there a reason why you don't use |this| as the context? If something you call > needs the application context instead of an activity context, shouldn't they be > the ones to get the application context from the activity? Not sure why I used the application context, we can definitely use |this|. https://codereview.chromium.org/2707993003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1786: String packageName = WebApkValidator.queryWebApkPackage(context, currentTab.getUrl()); On 2017/02/28 07:04:19, nyquist wrote: > This ended up being a very long section of the menu. Could you either extract > this into its own method? > It seems like the only thing it really needs is the URL from the current tab, > which you could pass in, i.e. it could even be a private static method if you > use the app context, or just an instance method if you end up using |this|. > > I guess you could even keep the Toast here if you want, doing something like: > > boolean launchFailed = openWebApkFromUrl(currentTab.getUrl()); > if (launchFailed) { > Toast.makeText(...); > } > > That would at least make this part of the method so much easier to read. > > WDYT? Sounds great, it did get quite big. Only thing I would slightly change is for the openWebApkFromUrl method to return true if it was opened successfully rather than if it failed, since it seems to make more semantic sense. Are you all right with that? https://codereview.chromium.org/2707993003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java (right): https://codereview.chromium.org/2707993003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:245: resolveInfo != null && resolveInfo.activityInfo.packageName != null; On 2017/02/28 07:04:19, nyquist wrote: > The null-check of packageName seems unnecessary here. If that is something that > can happen, we would have already crashed in WebApkValidator#isValidWebApk(...): > > if (!webappPackageName.startsWith(WEBAPK_PACKAGE_PREFIX)) { > ... > > Maybe you could move this null-check of the package name there instead, and just > check if the resolveInfo is null here? At that point openWebApkItemVisible == > resolveInfo != null, so we conceptually don't need both, but I find the variable > name very helpful so I'd keep it to ensure it's easy to understand for new > readers. Great catch. Thanks! https://codereview.chromium.org/2707993003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java (right): https://codereview.chromium.org/2707993003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:32: public static final int WEBAPK_OPEN_LAUNCH_SUCCESS = 0; On 2017/02/28 07:04:19, nyquist wrote: > Could you add a comment about this enum that refers to the UMA stat? Done. https://codereview.chromium.org/2707993003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:62: public static void recordWebApkOpenAttempt(int type) { On 2017/02/28 07:04:19, nyquist wrote: > Any reason this isn't documented using @IntDef? Seems like that could be helpful > here. I just followed the style on this file, I added the @IntDef annotation, but please let me know if I missed something since this is the first time I've used it. It does look like a good alternative to an enum, but what advantage does it have over using one and calling it's #ordinal method? Is it performance reasons? https://codereview.chromium.org/2707993003/diff/180001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2707993003/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:52: * NOTE: This can fail if multiple WebAPKs can match the supplied url. On 2017/02/28 07:04:19, nyquist wrote: > What does fail mean in this scenario? From the code it looks like we will just > be returning the first one? Yes, I added a clarification on the comment about the behavior. https://codereview.chromium.org/2707993003/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:55: * @param url The url to check. On 2017/02/28 07:04:19, nyquist wrote: > Nit: Comments are written in normal English, so URL should be all uppercase. I > would be OK with |url| though, if you want to refer to the URL parameter > specifically. Update the rest of the comment to URL too. Done. https://codereview.chromium.org/2707993003/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:64: * Fetches the list of resolve infos from the PackageManager that can handle the URL. On 2017/02/28 07:04:19, nyquist wrote: > {@link ResolveInfo}s Done. https://codereview.chromium.org/2707993003/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:70: private static List<ResolveInfo> resolveInfosForUrl(Context context, String url) { On 2017/02/28 07:04:19, nyquist wrote: > Would you want to annotate this with @Nonnull and/or update the @return > statement? Done. https://codereview.chromium.org/2707993003/diff/180001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:139: e.printStackTrace(); On 2017/02/28 07:04:19, nyquist wrote: > While you're at it, could you pleas remove this line? If it's supposed to stay, > add it as the third param to the log-statement below. Done.
Just a couple of style things left, and then I think you're good to go! https://codereview.chromium.org/2707993003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2707993003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1786: String packageName = WebApkValidator.queryWebApkPackage(context, currentTab.getUrl()); On 2017/02/28 16:12:23, gonzalon wrote: > On 2017/02/28 07:04:19, nyquist wrote: > > This ended up being a very long section of the menu. Could you either extract > > this into its own method? > > It seems like the only thing it really needs is the URL from the current tab, > > which you could pass in, i.e. it could even be a private static method if you > > use the app context, or just an instance method if you end up using |this|. > > > > I guess you could even keep the Toast here if you want, doing something like: > > > > boolean launchFailed = openWebApkFromUrl(currentTab.getUrl()); > > if (launchFailed) { > > Toast.makeText(...); > > } > > > > That would at least make this part of the method so much easier to read. > > > > WDYT? > > Sounds great, it did get quite big. Only thing I would slightly change is for > the openWebApkFromUrl method to return true if it was opened successfully rather > than if it failed, since it seems to make more semantic sense. Are you all right > with that? Yeah, that's totally fine. https://codereview.chromium.org/2707993003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java (right): https://codereview.chromium.org/2707993003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:62: public static void recordWebApkOpenAttempt(int type) { On 2017/02/28 16:12:23, gonzalon wrote: > On 2017/02/28 07:04:19, nyquist wrote: > > Any reason this isn't documented using @IntDef? Seems like that could be > helpful > > here. > > I just followed the style on this file, I added the @IntDef annotation, but > please let me know if I missed something since this is the first time I've used > it. It does look like a good alternative to an enum, but what advantage does it > have over using one and calling it's #ordinal method? Is it performance reasons? Yeah, it's just for performance reasons. https://codereview.chromium.org/2707993003/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2707993003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1818: if (launchIntent != null) { I think I'd flip this around (i.e. == null). That way you get an early return too, and don't have to indent the try-catch. https://codereview.chromium.org/2707993003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1831: return true; I think I'd move this up to the end of the try-statement for clarity.
Thanks for the review! https://codereview.chromium.org/2707993003/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2707993003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1818: if (launchIntent != null) { On 2017/02/28 17:57:17, nyquist wrote: > I think I'd flip this around (i.e. == null). That way you get an early return > too, and don't have to indent the try-catch. Done. https://codereview.chromium.org/2707993003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1831: return true; On 2017/02/28 17:57:17, nyquist wrote: > I think I'd move this up to the end of the try-statement for clarity. Done.
lgtm https://codereview.chromium.org/2707993003/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java (right): https://codereview.chromium.org/2707993003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:47: public static void recordUpdateRequestSent(int type) { While you're at it, would you want to do the @IntDef trick here too? https://codereview.chromium.org/2707993003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:69: assert type >= 0 && type < WEBAPK_OPEN_MAX; I'm not sure this is technically necessary now, but feel free to keep it for your own sanity, and clarity :-)
chrome/android/java/res/menu/main_menu.xml lgtm
https://codereview.chromium.org/2707993003/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java (right): https://codereview.chromium.org/2707993003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:47: public static void recordUpdateRequestSent(int type) { On 2017/02/28 18:19:17, nyquist wrote: > While you're at it, would you want to do the @IntDef trick here too? Done. https://codereview.chromium.org/2707993003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:69: assert type >= 0 && type < WEBAPK_OPEN_MAX; On 2017/02/28 18:19:17, nyquist wrote: > I'm not sure this is technically necessary now, but feel free to keep it for > your own sanity, and clarity :-) Done.
The CQ bit was checked by gonzalon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org, dominickn@chromium.org, holte@chromium.org, twellington@chromium.org, nyquist@chromium.org Link to the patchset: https://codereview.chromium.org/2707993003/#ps240001 (title: "Add new state for 'Add to Homescreen' Menu item")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by gonzalon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, twellington@chromium.org, holte@chromium.org, pkotwicz@chromium.org, nyquist@chromium.org Link to the patchset: https://codereview.chromium.org/2707993003/#ps260001 (title: "Add new state for 'Add to Homescreen' Menu item")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by gonzalon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, twellington@chromium.org, holte@chromium.org, pkotwicz@chromium.org, nyquist@chromium.org Link to the patchset: https://codereview.chromium.org/2707993003/#ps280001 (title: "Add new state for 'Add to Homescreen' Menu item")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 280001, "attempt_start_ts": 1488391635644400, "parent_rev": "2684ef71713dde49b5708d3944eb674ca8d4f24f", "commit_rev": "569d20972fa8cd57b3f6b666c0c4e596ada50640"}
Message was sent while issue was closed.
Description was changed from ========== [Android]: Hide add-to-homescreen app menu item when WebAPK is installed If the web page being shown has a related Web APK installed on the phone, rather than saying 'Add to Homescreen' the menu item now says 'Open {app_name}'. (ex. 'Open Telegram'). For a more on-depth discussion on this please see the attached bug. BUG=668258 ========== to ========== [Android]: Hide add-to-homescreen app menu item when WebAPK is installed If the web page being shown has a related Web APK installed on the phone, rather than saying 'Add to Homescreen' the menu item now says 'Open {app_name}'. (ex. 'Open Telegram'). For a more on-depth discussion on this please see the attached bug. BUG=668258 Review-Url: https://codereview.chromium.org/2707993003 Cr-Commit-Position: refs/heads/master@{#453997} Committed: https://chromium.googlesource.com/chromium/src/+/569d20972fa8cd57b3f6b666c0c4... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://chromium.googlesource.com/chromium/src/+/569d20972fa8cd57b3f6b666c0c4... |