Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(115)

Issue 2707993003: [Android]: Hide add-to-homescreen app menu item when WebAPK is installed (Closed)

Created:
3 years, 10 months ago by gonzalon
Modified:
3 years, 9 months ago
CC:
chromium-reviews, zpeng+watch_chromium.org, srahim+watch_chromium.org, agrieve+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+184 lines, -19 lines) Patch
M chrome/android/java/res/menu/custom_tabs_menu.xml View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +4 lines, -1 line 0 comments Download
M chrome/android/java/res/menu/main_menu.xml View 11 12 13 2 chunks +2 lines, -1 line 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 5 chunks +27 lines, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +44 lines, -6 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java View 1 2 3 4 5 6 7 8 9 6 chunks +7 lines, -6 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java View 1 2 3 4 5 6 7 8 10 11 12 13 2 chunks +15 lines, -0 lines 0 comments Download
M chrome/android/java/strings/android_chrome_strings.grd View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java View 1 2 3 4 11 12 13 4 chunks +45 lines, -5 lines 0 comments Download
M tools/metrics/actions/actions.xml View 1 2 3 4 5 6 7 1 chunk +8 lines, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +26 lines, -0 lines 0 comments Download

Messages

Total messages: 55 (16 generated)
gonzalon
Hi! This is an implementation of the 'Open WebAPK' menu item as discussed on the ...
3 years, 10 months ago (2017-02-21 16:38:15 UTC) #2
pkotwicz
Your CL looks good. Thank you for bearing with me while we argued what the ...
3 years, 10 months ago (2017-02-21 22:52:32 UTC) #3
dominickn
https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java#newcode1783 chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1783: Log.e(TAG, "Failed to open app : %s!", packageName, e); ...
3 years, 10 months ago (2017-02-21 23:04:42 UTC) #4
dominickn
Also: change the title and first line of the description to be something like, "Replace ...
3 years, 10 months ago (2017-02-21 23:07:21 UTC) #5
pkotwicz
https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java (right): https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java#newcode174 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. ...
3 years, 10 months ago (2017-02-22 15:04:51 UTC) #6
gonzalon
Thanks for your time reviewing! https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2707993003/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java#newcode1783 chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1783: Log.e(TAG, "Failed to open ...
3 years, 10 months ago (2017-02-22 15:50:50 UTC) #8
pkotwicz
Looks good. A few comments https://codereview.chromium.org/2707993003/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2707993003/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java#newcode1782 chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1782: RecordUserAction.record("MobileMenuOpenWebAPK"); You need to ...
3 years, 10 months ago (2017-02-22 16:50:20 UTC) #9
gonzalon
https://codereview.chromium.org/2707993003/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2707993003/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java#newcode1782 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 ...
3 years, 10 months ago (2017-02-22 17:17:40 UTC) #10
pkotwicz
L-G-T-M once you address these comments :) https://codereview.chromium.org/2707993003/diff/60001/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java 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/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java#newcode43 chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:43: public static ...
3 years, 10 months ago (2017-02-22 20:26:10 UTC) #11
gonzalon
https://codereview.chromium.org/2707993003/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2707993003/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java#newcode1777 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 ...
3 years, 10 months ago (2017-02-22 20:45:05 UTC) #12
dominickn
Looks pretty good. I've just thought of another metric we should add (sorry for not ...
3 years, 10 months ago (2017-02-23 05:53:55 UTC) #13
gonzalon
Thanks for your time reviewing. Please don't hesitate to mention any other changes/additions, even if ...
3 years, 10 months ago (2017-02-23 16:49:57 UTC) #14
pkotwicz
A few final comments about the user action recroded in ChromeActivity.java https://codereview.chromium.org/2707993003/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java (right): ...
3 years, 10 months ago (2017-02-23 20:46:18 UTC) #15
dominickn
https://codereview.chromium.org/2707993003/diff/120001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2707993003/diff/120001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java#newcode1790 chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1790: } +1, except maybe call it LAUNCH_SUCCESS instead of ...
3 years, 10 months ago (2017-02-24 05:21:25 UTC) #16
gonzalon
https://codereview.chromium.org/2707993003/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java (right): https://codereview.chromium.org/2707993003/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java#newcode225 chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:225: Menu menu, String url, boolean homeScreenVisible) { On 2017/02/23 ...
3 years, 10 months ago (2017-02-24 16:02:58 UTC) #17
pkotwicz
LGTM! with nits Thank you for bearing with me https://codereview.chromium.org/2707993003/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java (right): https://codereview.chromium.org/2707993003/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java#newcode62 chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:62: ...
3 years, 10 months ago (2017-02-24 16:29:41 UTC) #18
pkotwicz
Nit: Can you please change the CL description [Android]: Hide add-to-homescreen app menu item when ...
3 years, 10 months ago (2017-02-24 16:33:28 UTC) #19
gonzalon
On 2017/02/24 16:33:28, pkotwicz wrote: > Nit: Can you please change the CL description > ...
3 years, 10 months ago (2017-02-24 16:41:09 UTC) #20
gonzalon
https://codereview.chromium.org/2707993003/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java (right): https://codereview.chromium.org/2707993003/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java#newcode62 chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:62: */ On 2017/02/24 16:29:40, pkotwicz wrote: > Nits: > ...
3 years, 10 months ago (2017-02-24 16:41:29 UTC) #22
gonzalon
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 ...
3 years, 10 months ago (2017-02-24 16:52:10 UTC) #24
Lei Zhang
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 ...
3 years, 10 months ago (2017-02-24 18:54:00 UTC) #25
gonzalon
Thanks Lei for pointing me in the right direction. nyquist@, would you mind taking a ...
3 years, 10 months ago (2017-02-24 18:59:40 UTC) #28
Steven Holte
lgtm
3 years, 10 months ago (2017-02-25 01:35:06 UTC) #29
pkotwicz
https://codereview.chromium.org/2707993003/diff/160001/chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java (right): https://codereview.chromium.org/2707993003/diff/160001/chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java#newcode232 chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:232: Context context = ContextUtils.getApplicationContext(); One final nit: You should ...
3 years, 10 months ago (2017-02-25 02:36:05 UTC) #30
dominickn
lgtm https://codereview.chromium.org/2707993003/diff/160001/chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java (right): https://codereview.chromium.org/2707993003/diff/160001/chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java#newcode32 chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:32: private static final int WEBAPK_OPEN_MAX = 3; By ...
3 years, 9 months ago (2017-02-27 02:21:22 UTC) #31
gonzalon
https://codereview.chromium.org/2707993003/diff/160001/chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java (right): https://codereview.chromium.org/2707993003/diff/160001/chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java#newcode232 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: ...
3 years, 9 months ago (2017-02-27 18:37:35 UTC) #32
nyquist
I really appreciate you adding the UMA histogram for how long it takes to query ...
3 years, 9 months ago (2017-02-28 07:04:19 UTC) #33
gonzalon
https://codereview.chromium.org/2707993003/diff/180001/chrome/android/java/res/menu/main_menu.xml File chrome/android/java/res/menu/main_menu.xml (left): https://codereview.chromium.org/2707993003/diff/180001/chrome/android/java/res/menu/main_menu.xml#oldcode103 chrome/android/java/res/menu/main_menu.xml:103: On 2017/02/28 07:04:19, nyquist wrote: > Nit: Why is ...
3 years, 9 months ago (2017-02-28 16:12:24 UTC) #34
nyquist
Just a couple of style things left, and then I think you're good to go! ...
3 years, 9 months ago (2017-02-28 17:57:17 UTC) #35
gonzalon
Thanks for the review! https://codereview.chromium.org/2707993003/diff/200001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2707993003/diff/200001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java#newcode1818 chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1818: if (launchIntent != null) { ...
3 years, 9 months ago (2017-02-28 18:05:09 UTC) #36
nyquist
lgtm https://codereview.chromium.org/2707993003/diff/220001/chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java (right): https://codereview.chromium.org/2707993003/diff/220001/chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java#newcode47 chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:47: public static void recordUpdateRequestSent(int type) { While you're ...
3 years, 9 months ago (2017-02-28 18:19:17 UTC) #37
Theresa
chrome/android/java/res/menu/main_menu.xml lgtm
3 years, 9 months ago (2017-02-28 18:19:40 UTC) #38
gonzalon
https://codereview.chromium.org/2707993003/diff/220001/chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java (right): https://codereview.chromium.org/2707993003/diff/220001/chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java#newcode47 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, ...
3 years, 9 months ago (2017-02-28 20:46:04 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2707993003/240001
3 years, 9 months ago (2017-02-28 20:46:52 UTC) #42
commit-bot: I haz the power
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_android_rel_ng/builds/241219)
3 years, 9 months ago (2017-02-28 22:19:23 UTC) #44
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2707993003/260001
3 years, 9 months ago (2017-03-01 15:32:29 UTC) #47
commit-bot: I haz the power
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_android_rel_ng/builds/241902)
3 years, 9 months ago (2017-03-01 17:14:44 UTC) #49
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2707993003/280001
3 years, 9 months ago (2017-03-01 18:07:29 UTC) #52
commit-bot: I haz the power
3 years, 9 months ago (2017-03-01 19:36:08 UTC) #55
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/569d20972fa8cd57b3f6b666c0c4...

Powered by Google App Engine
This is Rietveld 408576698