Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2858563004:
Add support for webapk without runtimeHost (Closed)
DescriptionAdd support for webapk without runtimeHost
An Unbound webapk will not specify a "runtimeHost" as part of the
AndroidManifest. During the first launch of a WebAPK, we will use the default
browser as the host browser if it supports WebAPKs. If the default browser
doesn't support WebAPKs, a dialog will pop up to ask user to choose a browser
to launch the WebAPK. The UX of the dialog is:
https://docs.google.com/presentation/d/1ZC7Fc9Y7-vsGqvbEyiZmiJoWtsONkhPBUb47F0s4u4s/edit#slide=id.g1ca70aafc8_0_16
For existing WebAPKs which has specified "runtimeHost":
1) Launch the WebAPK by the specified "runtimeHost" if it is installed;
2) Show a dialog to ask for users to choose a host browser.
In any case, if user chooses a browser that isn't installed, we will redirect
to Play Store to install it.
The screenshots refer to:
https://drive.google.com/open?id=0B7zEF5GgyYmpVkowOUY4ZGFKTmM
BUG=714737
Review-Url: https://codereview.chromium.org/2858563004
Cr-Commit-Position: refs/heads/master@{#477360}
Committed: https://chromium.googlesource.com/chromium/src/+/25f15cec7b331804a3b884280d5701de16ef899a
Patch Set 1 #
Total comments: 21
Patch Set 2 : Reorder how to choose a host browser automatically. #
Total comments: 18
Patch Set 3 : Clean up. #
Total comments: 13
Patch Set 4 : Nits. #
Total comments: 17
Patch Set 5 : Add junit tests. #Patch Set 6 : Fix test failure. #
Total comments: 14
Patch Set 7 : Nits. #
Total comments: 2
Patch Set 8 : Add a dialog to choose host brwoser. #Patch Set 9 : Delete private data before switching host browser. #
Total comments: 36
Patch Set 10 : pkotwicz@'s comments #
Total comments: 18
Patch Set 11 : pkotwicz@'s comments. #Patch Set 12 : Use spy to partically mock. #
Total comments: 41
Patch Set 13 : pkotwicz@'s comments. #
Total comments: 2
Patch Set 14 : Remove the usage of DialogFragment to decrease the WebAPK size. #
Total comments: 9
Patch Set 15 : Update Unit tests. #
Total comments: 12
Patch Set 16 : Nits. #
Total comments: 6
Patch Set 17 : Add a padding. #Patch Set 18 : Use clean application data. #
Total comments: 16
Patch Set 19 : Delete internal storage. #Patch Set 20 : Rebase and update one string. #Patch Set 21 : Try to fix the compile error. #
Total comments: 2
Patch Set 22 : Try to fix the compile error again. #Patch Set 23 : Remove template. #Patch Set 24 : Sort the browser list to make the ones that don't support WebAPKs come later. #
Total comments: 6
Patch Set 25 : Nits. #
Total comments: 18
Patch Set 26 : Nits. #Patch Set 27 : Ted@'s comments and add "Unsupported by ..." strings for browser not supporting webapks. #
Total comments: 2
Patch Set 28 : Put suppressions inline. #
Total comments: 4
Patch Set 29 : Nits. #Messages
Total messages: 140 (75 generated)
Patchset #1 (id:1) has been deleted
hanxi@chromium.org changed reviewers: + pkotwicz@chromium.org, yfriedman@chromium.org
Hi Yaron & Peter, could you please take a look? Thanks!
https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:30: new String[] {"com.android.chrome", "com.google.android.apps.chrome", "com.chrome.beta", I don't think that Google Play enables WebAPK installs/updates for com.google.android.apps.chrome? https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:61: public static String getHostBrowserPackageName(final Context context) { If initially Firefox and Chrome are installed and Firefox is the default browser and the WebAPK is launched -> The SharedPreference will be set to the Firefox package name Then the user uninstalls Firefox -> The SharedPreference will still contain the Firefox package name and getHostBrowserPackageName() will return the Firefox package name even though Firefox is no longer installed. https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:154: private static String getInstalledWebApkSupportedBrowser(PackageManager packageManager) { We can probably use logic similar to CustomTabsHelper#getPackageNameToUse() (namely PackageManager#resolveActivity()) to prefer the user's default browser
https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:34: * Caches the value read from Application Metadata which specifies the host browser's package Update comment https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:37: private static String sHostPackage; This static could be cached incorrectly if you clear app data and don't restart the app. We don't care though but you could mention it in the comment. https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:61: public static String getHostBrowserPackageName(final Context context) { On 2017/05/04 18:22:56, pkotwicz wrote: > If initially Firefox and Chrome are installed and Firefox is the default browser > and the WebAPK is launched > -> The SharedPreference will be set to the Firefox package name > > Then the user uninstalls Firefox > -> The SharedPreference will still contain the Firefox package name and > getHostBrowserPackageName() will return the Firefox package name even though > Firefox is no longer installed. > What did we decide was the desired behaviour here? I feel like almost anything we do is magic. Note we can't switch hosts without deleting private data. Perhaps the only sensible option is to wipe data and prompt for a browser (as if it were never used) https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:64: // Step 1: retrieves the host package name from SharedPreference. hmm - I'm not sure about this. I don't think we want a shared pref to override runtimeHost if it exists - I *think* we only want to use shared pref if the manifest value isn't specified. wdyt? https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:67: // Step 2: retrieves the host package name from AndroidManifest.xml. avoid all this nesting by doing soemthing liek (extract inner fn if needed): // step 1 if () { return } // step 2 if () { return.. }
https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:61: public static String getHostBrowserPackageName(final Context context) { I vote for automatically picking the browser since currently the only choices are different channels of Chrome
Patchset #3 (id:60001) has been deleted
Patchset #2 (id:40001) has been deleted
Thank you both for the suggestions! PTAL, thanks! https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:30: new String[] {"com.android.chrome", "com.google.android.apps.chrome", "com.chrome.beta", On 2017/05/04 18:22:56, pkotwicz wrote: > I don't think that Google Play enables WebAPK installs/updates for > com.google.android.apps.chrome? Good catch, removed. https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:34: * Caches the value read from Application Metadata which specifies the host browser's package On 2017/05/05 14:50:29, Yaron (limited availability) wrote: > Update comment Done. https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:37: private static String sHostPackage; On 2017/05/05 14:50:30, Yaron (limited availability) wrote: > This static could be cached incorrectly if you clear app data and don't restart > the app. We don't care though but you could mention it in the comment. Why the cached value would be incorrectly? I am not sure that I understand. If don't restart the app, the host browser will remain the same, then the value is correct. Also, if the host browser is uninstalled, the WebAPK will be forced to close. https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:61: public static String getHostBrowserPackageName(final Context context) { On 2017/05/04 18:22:56, pkotwicz wrote: > If initially Firefox and Chrome are installed and Firefox is the default browser > and the WebAPK is launched > -> The SharedPreference will be set to the Firefox package name > > Then the user uninstalls Firefox > -> The SharedPreference will still contain the Firefox package name and > getHostBrowserPackageName() will return the Firefox package name even though > Firefox is no longer installed. > Good catch! Yes, we need to check whether the cached host browser is still installed. https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:61: public static String getHostBrowserPackageName(final Context context) { On 2017/05/05 14:50:29, Yaron (limited availability) wrote: > On 2017/05/04 18:22:56, pkotwicz wrote: > > If initially Firefox and Chrome are installed and Firefox is the default > browser > > and the WebAPK is launched > > -> The SharedPreference will be set to the Firefox package name > > > > Then the user uninstalls Firefox > > -> The SharedPreference will still contain the Firefox package name and > > getHostBrowserPackageName() will return the Firefox package name even though > > Firefox is no longer installed. > > > > What did we decide was the desired behaviour here? I feel like almost anything > we do is magic. Note we can't switch hosts without deleting private data. > Perhaps the only sensible option is to wipe data and prompt for a browser (as if > it were never used) Yes, we should give user an option to choose which browser they want to switch. However, this involves introducing a dialog and translation stuff. I am now working on another CL that generates translations for WebAPKs. How about making the dialog in a follow up CL? Thank you Peter for the suggestion of automatically picking browsers for now. https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:64: // Step 1: retrieves the host package name from SharedPreference. On 2017/05/05 14:50:30, Yaron (limited availability) wrote: > hmm - I'm not sure about this. I don't think we want a shared pref to override > runtimeHost if it exists - I *think* we only want to use shared pref if the > manifest value isn't specified. wdyt? sgtm. I made the check from metadata be the first one. https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:67: // Step 2: retrieves the host package name from AndroidManifest.xml. On 2017/05/05 14:50:30, Yaron (limited availability) wrote: > avoid all this nesting by doing soemthing liek (extract inner fn if needed): > // step 1 > if () { > return > } > // step 2 > if () { > return.. > } It is because we have 4 line of code (line 80 to 83) will be repeated each time, so I would like each if just sets a proper value for the |hostPackage|:( https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:154: private static String getInstalledWebApkSupportedBrowser(PackageManager packageManager) { On 2017/05/04 18:22:56, pkotwicz wrote: > We can probably use logic similar to CustomTabsHelper#getPackageNameToUse() > (namely PackageManager#resolveActivity()) to prefer the user's default browser Done.
https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:37: private static String sHostPackage; The WebApkActivity will be forced to close. The WebApkService might still be running (e.g. the WebApkService is used by multiple browsers once that is possible. See crbug.com/720077) Meta comment: Do we need both a SharedPreference and a static. We might be over engineering the solution https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:67: // Step 2: retrieves the host package name from AndroidManifest.xml. You can use a helper function to avoid duplicating code. In particular: public static String getHostBrowserPackageName(Context context) { if (sHostPackage == null) { String hostPackage = selectHostBrowserPackageName(context); sHostPackage = (hostPackage != null) ? hostPackage : ""; } return sHostPackage; } https://codereview.chromium.org/2858563004/diff/80001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/80001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:34: private static final String CHROME_CANARY_PACKAGE = "com.chrome.canary"; Personally, I would inline the package names in |sBrowsersSupportingWebApk| and remove the constants https://codereview.chromium.org/2858563004/diff/80001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:37: private static final List<String> sBrowsersSupportingWwebApk; Nit: sBrowsersSupportingWwebApk -> sBrowsersSupportingWebApk https://codereview.chromium.org/2858563004/diff/80001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:98: hostPackage = defaultBrowser; Can you change lines 99-107 into iterating over a for() loop? (Iterating over sBrowsersSupportingWebApk). This means that sBrowsersSupportingWebApk would need to be sorted with the most preferred browser first. https://codereview.chromium.org/2858563004/diff/80001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:150: PackageManager packageManager) { I don't know what to think about this function. In the context of lines 99 - 107 I want this function to return a "sorted list with the most preferred browser first". In the context of lines 82 and 89 I want this function to return a "list of installed browsers" https://codereview.chromium.org/2858563004/diff/80001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:165: private static String getHostBrowserFromAndroidManifest(Context context) { Should this function take a PackageManager as an argument for the sake of consistency? https://codereview.chromium.org/2858563004/diff/80001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:172: e.printStackTrace(); We probably remove e.printStackTrace() (I know that you copied it from the old code) https://codereview.chromium.org/2858563004/diff/80001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:202: }.execute(hostPackage); Is it worth to save to SharedPreferences in an AsyncTask? - How long does saving to SharedPreferences take? - How long do the PackageManager requests in getInstalledBrowsersSupportingWebApks() take? According to the documentation SharedPreferences.Editor#apply() is somewhat asynchronous
https://codereview.chromium.org/2858563004/diff/80001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/80001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:34: private static final String CHROME_CANARY_PACKAGE = "com.chrome.canary"; On 2017/05/09 21:38:28, pkotwicz wrote: > Personally, I would inline the package names in |sBrowsersSupportingWebApk| and > remove the constants +1 https://codereview.chromium.org/2858563004/diff/80001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:81: hostPackage = null; hostPackage is already null and just a local variable. https://codereview.chromium.org/2858563004/diff/80001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:202: }.execute(hostPackage); On 2017/05/09 21:38:27, pkotwicz wrote: > Is it worth to save to SharedPreferences in an AsyncTask? > - How long does saving to SharedPreferences take? > - How long do the PackageManager requests in > getInstalledBrowsersSupportingWebApks() take? > > According to the documentation SharedPreferences.Editor#apply() is somewhat > asynchronous Ya, it's not necessary for apply
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
PTAL, thanks! https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:37: private static String sHostPackage; On 2017/05/09 21:38:27, pkotwicz wrote: > The WebApkActivity will be forced to close. The WebApkService might still be > running (e.g. the WebApkService is used by multiple browsers once that is > possible. See crbug.com/720077) Once the MainActivity is created again, the |sHostPackage| will be re-selected, are you saying that it might be different than the one that the WebApkService was created from? The |sHostPackage| should be the correct one, but the WebApkService is the one out-of-date, right? I will add a comment saying that "The WebApkService may be still running and out-of-date when sHostPackage is changed after user clears the WebAPK's data." > Meta comment: Do we need both a SharedPreference and a static. We might be over > engineering the solution I think we'd better keep both. We need the SharedPreference any way, since we need to store the host browser there. Having a static could avoid reading from the SharedPreference or AndroidManifest every time that we query the host browser. https://codereview.chromium.org/2858563004/diff/20001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:67: // Step 2: retrieves the host package name from AndroidManifest.xml. On 2017/05/09 21:38:27, pkotwicz wrote: > You can use a helper function to avoid duplicating code. In particular: > > public static String getHostBrowserPackageName(Context context) { > if (sHostPackage == null) { > String hostPackage = selectHostBrowserPackageName(context); > sHostPackage = (hostPackage != null) ? hostPackage : ""; > } > return sHostPackage; > } SGTM, thanks! https://codereview.chromium.org/2858563004/diff/80001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/80001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:34: private static final String CHROME_CANARY_PACKAGE = "com.chrome.canary"; On 2017/05/10 17:47:57, Yaron (limited availability) wrote: > On 2017/05/09 21:38:28, pkotwicz wrote: > > Personally, I would inline the package names in |sBrowsersSupportingWebApk| > and > > remove the constants > > +1 Done. https://codereview.chromium.org/2858563004/diff/80001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:37: private static final List<String> sBrowsersSupportingWwebApk; On 2017/05/09 21:38:27, pkotwicz wrote: > Nit: sBrowsersSupportingWwebApk -> sBrowsersSupportingWebApk Done. https://codereview.chromium.org/2858563004/diff/80001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:81: hostPackage = null; On 2017/05/10 17:47:57, Yaron (limited availability) wrote: > hostPackage is already null and just a local variable. Acknowledged. https://codereview.chromium.org/2858563004/diff/80001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:98: hostPackage = defaultBrowser; On 2017/05/09 21:38:27, pkotwicz wrote: > Can you change lines 99-107 into iterating over a for() loop? (Iterating over > sBrowsersSupportingWebApk). This means that sBrowsersSupportingWebApk would need > to be sorted with the most preferred browser first. Good idea, thanks! https://codereview.chromium.org/2858563004/diff/80001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:150: PackageManager packageManager) { On 2017/05/09 21:38:27, pkotwicz wrote: > I don't know what to think about this function. > > In the context of lines 99 - 107 I want this function to return a "sorted list > with the most preferred browser first". > > In the context of lines 82 and 89 I want this function to return a "list of > installed browsers" How about changing the variable |packagesSupportingWebApks| to |installedBrowsersSupportingWebApks|? This function is most likely to be changed when implementing the host browser selection dialog. https://codereview.chromium.org/2858563004/diff/80001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:165: private static String getHostBrowserFromAndroidManifest(Context context) { On 2017/05/09 21:38:28, pkotwicz wrote: > Should this function take a PackageManager as an argument for the sake of > consistency? Done. https://codereview.chromium.org/2858563004/diff/80001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:172: e.printStackTrace(); On 2017/05/09 21:38:27, pkotwicz wrote: > We probably remove e.printStackTrace() (I know that you copied it from the old > code) Done. https://codereview.chromium.org/2858563004/diff/80001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:202: }.execute(hostPackage); On 2017/05/10 17:47:57, Yaron (limited availability) wrote: > On 2017/05/09 21:38:27, pkotwicz wrote: > > Is it worth to save to SharedPreferences in an AsyncTask? > > - How long does saving to SharedPreferences take? > > - How long do the PackageManager requests in > > getInstalledBrowsersSupportingWebApks() take? > > > > According to the documentation SharedPreferences.Editor#apply() is somewhat > > asynchronous > > Ya, it's not necessary for apply Ok, removed.
https://codereview.chromium.org/2858563004/diff/140001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:28: private static final String SHARED_PREF = "metadata"; Shouldn't you call this variable PREF_PACKAGE and set it to "org.chromium.webapk.shell_apk" to match HostBrowserClassLoader.java Otherwise I think we will end up with multiple SharedPreferences files https://codereview.chromium.org/2858563004/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:31: // The package names of different versions of Chromes that support WebAPKs. How about: "The package names of the channels of Chrome that support WebAPKs." My version is shorter :) Chromes -> Chrome https://codereview.chromium.org/2858563004/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:36: "com.android.chrome", "com.chrome.beta", "com.chrome.dev", "com.chrome.canary")); Not including "com.google.android.apps.chrome" in this list will make our lives harder because we will need to build Chrome with one of the white listed package names for testing. Just a heads up. https://codereview.chromium.org/2858563004/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:68: public static String getHostBrowserPackageName(Context context) { Nit: Can you handle the null return value in the caller instead of in this function? https://codereview.chromium.org/2858563004/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:155: PackageManager packageManager) { Can this function just return a list of installed browsers? On line 106, you will need to check: sBrowsersSupportingWebApk.contains(defaultBrowser) && installedBrowsers.contains(defaultBrowser) On line 93 && 100, you can just check installedBrowsers.contains(defaultBrowser)
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:160001) has been deleted
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
PTAL, thanks! https://codereview.chromium.org/2858563004/diff/140001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:28: private static final String SHARED_PREF = "metadata"; On 2017/05/10 20:40:59, pkotwicz wrote: > Shouldn't you call this variable PREF_PACKAGE and set it to > "org.chromium.webapk.shell_apk" to match HostBrowserClassLoader.java > > Otherwise I think we will end up with multiple SharedPreferences files Done. https://codereview.chromium.org/2858563004/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:31: // The package names of different versions of Chromes that support WebAPKs. On 2017/05/10 20:40:59, pkotwicz wrote: > How about: "The package names of the channels of Chrome that support WebAPKs." > > My version is shorter :) > > Chromes -> Chrome Done. https://codereview.chromium.org/2858563004/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:36: "com.android.chrome", "com.chrome.beta", "com.chrome.dev", "com.chrome.canary")); On 2017/05/10 20:40:59, pkotwicz wrote: > Not including "com.google.android.apps.chrome" in this list will make our lives > harder because we will need to build Chrome with one of the white listed package > names for testing. Just a heads up. Hmm, that is why I added it in the first place. Add it back now. I think we could prefer it as the first one, since it has the latest changes and probably used by developers only. https://codereview.chromium.org/2858563004/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:68: public static String getHostBrowserPackageName(Context context) { On 2017/05/10 20:40:59, pkotwicz wrote: > Nit: Can you handle the null return value in the caller instead of in this > function? Thanks for reminding me to add extra logic in the caller to handle the null or "" string case. Updated to have early exit for these corner cases. To this function, can you elaborate more? Do you mean do not set |sHostPackage| to ""? We have to, since "" means we try to find a host browser but failed; while null means we haven't tried to find a proper host browser yet. https://codereview.chromium.org/2858563004/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:155: PackageManager packageManager) { On 2017/05/10 20:40:59, pkotwicz wrote: > Can this function just return a list of installed browsers? > > On line 106, you will need to check: > sBrowsersSupportingWebApk.contains(defaultBrowser) && > installedBrowsers.contains(defaultBrowser) sBrowsersSupportingWebApk.contains(defaultBrowser) would be enough. > On line 93 && 100, you can just check > installedBrowsers.contains(defaultBrowser) Good point!
Dry run: 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
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
looking good https://codereview.chromium.org/2858563004/diff/140002/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2858563004/diff/140002/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:125: if (hostBrowserPackageName == null) { this conditional isn't necessary anymore https://codereview.chromium.org/2858563004/diff/140002/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/140002/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:35: sBrowsersSupportingWebApk = new ArrayList<String>( can you just inline this into the declaration above. Also, how about making it a Set? (same with other collections in this CL) https://codereview.chromium.org/2858563004/diff/140002/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:72: sHostPackage = (hostPackage != null) ? hostPackage : ""; Nit: you can simplify by having getHostBrowserPackageNameInternal just return the empty string for you (up to you) https://codereview.chromium.org/2858563004/diff/140002/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:79: private static String getHostBrowserPackageNameInternal(final Context context) { Can we add some unit tests for this fn? https://codereview.chromium.org/2858563004/diff/140002/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:87: return null; may as well do this first since it's your first early-out (and it brings the hostBrowserFromManifest code together)
https://codereview.chromium.org/2858563004/diff/140002/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/140002/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:34: static { Cool! I didn't know that static {} blocks were a thing https://codereview.chromium.org/2858563004/diff/140002/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:43: */ How about for the second sentence: "{@link sHostPackage} might refer to a browser which has been uninstalled. A notification can keep the WebAPK process alive after the host browser has been uninstalled." https://codereview.chromium.org/2858563004/diff/140002/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:80: // Gets the package name of the host browser if it is specified in AndroidManifest.xml. Nit: Remove the comments from this function. The comments are on "what the function does" not "why the function does what it does". I think that "what the function does" is obvious from the code.
LGTM with nits! https://codereview.chromium.org/2858563004/diff/140001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:36: "com.android.chrome", "com.chrome.beta", "com.chrome.dev", "com.chrome.canary")); That makes sense. Sorry about suggesting to remove it. https://codereview.chromium.org/2858563004/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:68: public static String getHostBrowserPackageName(Context context) { Yes, I was suggesting to not setting |sHostPackage| to "" If this function returns null: - MainActivity will finish (causing the WebAPK process to finish) - We will send the user to the Play Store to install Chrome stable The next time that the WebAPK is launched, |sHostPackage| will be null (regardless of what getHostBrowserPackageName() previously returned) because the WebAPK process would just have gotten launched https://codereview.chromium.org/2858563004/diff/140002/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/140002/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:35: sBrowsersSupportingWebApk = new ArrayList<String>( This one needs to be a list because the order of |sBrowsersSupportingWebApk| matters. See the loop in line 113. There should be a comment here saying that the order matters.
Patchset #5 (id:190001) has been deleted
Patchset #5 (id:210001) has been deleted
I add some junit tests, PTAL, thanks! https://codereview.chromium.org/2858563004/diff/140001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:68: public static String getHostBrowserPackageName(Context context) { On 2017/05/11 17:37:38, pkotwicz wrote: > Yes, I was suggesting to not setting |sHostPackage| to "" > > If this function returns null: > - MainActivity will finish (causing the WebAPK process to finish) > - We will send the user to the Play Store to install Chrome stable > > The next time that the WebAPK is launched, |sHostPackage| will be null > (regardless of what getHostBrowserPackageName() previously returned) because the > WebAPK process would just have gotten launched That makes sense to me. https://codereview.chromium.org/2858563004/diff/140002/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2858563004/diff/140002/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:125: if (hostBrowserPackageName == null) { On 2017/05/11 17:26:15, Yaron (limited availability) wrote: > this conditional isn't necessary anymore Good catch, removed! https://codereview.chromium.org/2858563004/diff/140002/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/140002/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:34: static { On 2017/05/11 17:29:03, pkotwicz wrote: > Cool! I didn't know that static {} blocks were a thing Android studio suggested:) https://codereview.chromium.org/2858563004/diff/140002/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:35: sBrowsersSupportingWebApk = new ArrayList<String>( On 2017/05/11 17:37:38, pkotwicz wrote: > This one needs to be a list because the order of |sBrowsersSupportingWebApk| > matters. See the loop in line 113. There should be a comment here saying that > the order matters. Thanks Peter for the explanation, I will add a comment. https://codereview.chromium.org/2858563004/diff/140002/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:43: */ On 2017/05/11 17:29:03, pkotwicz wrote: > How about for the second sentence: "{@link sHostPackage} might refer to a > browser which has been uninstalled. A notification can keep the WebAPK process > alive after the host browser has been uninstalled." SG, thanks! https://codereview.chromium.org/2858563004/diff/140002/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:72: sHostPackage = (hostPackage != null) ? hostPackage : ""; On 2017/05/11 17:26:16, Yaron (limited availability) wrote: > Nit: you can simplify by having getHostBrowserPackageNameInternal just return > the empty string for you > (up to you) SG, thanks! https://codereview.chromium.org/2858563004/diff/140002/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:79: private static String getHostBrowserPackageNameInternal(final Context context) { On 2017/05/11 17:26:16, Yaron (limited availability) wrote: > Can we add some unit tests for this fn? Done. https://codereview.chromium.org/2858563004/diff/140002/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:80: // Gets the package name of the host browser if it is specified in AndroidManifest.xml. On 2017/05/11 17:29:03, pkotwicz wrote: > Nit: Remove the comments from this function. The comments are on "what the > function does" not "why the function does what it does". I think that "what the > function does" is obvious from the code. It doesn't hurt to keep them:) https://codereview.chromium.org/2858563004/diff/140002/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:87: return null; On 2017/05/11 17:26:16, Yaron (limited availability) wrote: > may as well do this first since it's your first early-out (and it brings the > hostBrowserFromManifest code together) Done.
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I fixed the MainActivityTest failure, PTAL, thanks!
https://codereview.chromium.org/2858563004/diff/250001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java (right): https://codereview.chromium.org/2858563004/diff/250001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:69: public void testReturnsEmptyStringWhenNoBrowserInstalled() { testReturnsNull https://codereview.chromium.org/2858563004/diff/250001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:86: Assert.assertTrue(hostBrowser.equals(expectedHostBrowser)); use assertEquals (throughout) for better error messages on failures. https://codereview.chromium.org/2858563004/diff/250001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:107: mockWebApkPackageMetadata("browser.uninstalled"); so the idea is that this is referring to a webapk that's been uninstalled? can you add a comment like: // Now simulate the webapk specifying a host browser that isn't installed. However, shouldn't the browser have also been included in |setBrowsersSupportingWebApkForTesting| ? cause it *could* have been supported but happened to not be installed? https://codereview.chromium.org/2858563004/diff/250001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:194: Mockito.when(mPackageManager.getApplicationInfo(anyString(), anyInt())).thenReturn(ai); instead of anyString() you should be able to use hostBrowserPackage (or perhaps equals(hostBrowserPackage) - unsure exactly about the call) https://codereview.chromium.org/2858563004/diff/250001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/250001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:44: public static void resetCachedHostPackageForTesting() { Can you confirm whether these are removed for release builds after proguarding? If not, I think you'll want to follow what we do with RemovableInRelease https://codereview.chromium.org/2858563004/diff/250001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:159: private static List<String> getInstalledBrowsers(PackageManager packageManager) { Ok, so Peter was right that ordering matters for the other collection but this can still use Set (and would make for faster |contains| checks)
Patchset #7 (id:270001) has been deleted
Patchset #7 (id:290001) has been deleted
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Hi Yaron, PTAL, thanks! https://codereview.chromium.org/2858563004/diff/250001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java (right): https://codereview.chromium.org/2858563004/diff/250001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:69: public void testReturnsEmptyStringWhenNoBrowserInstalled() { On 2017/05/15 18:41:37, Yaron wrote: > testReturnsNull Done. https://codereview.chromium.org/2858563004/diff/250001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:86: Assert.assertTrue(hostBrowser.equals(expectedHostBrowser)); On 2017/05/15 18:41:37, Yaron wrote: > use assertEquals (throughout) for better error messages on failures. It is deprecated, that is why I try to void using it. Anyway, updated. https://codereview.chromium.org/2858563004/diff/250001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:107: mockWebApkPackageMetadata("browser.uninstalled"); On 2017/05/15 18:41:37, Yaron wrote: > so the idea is that this is referring to a webapk that's been uninstalled? can > you add a comment like: > // Now simulate the webapk specifying a host browser that isn't installed. Added, thanks! > However, shouldn't the browser have also been included in > |setBrowsersSupportingWebApkForTesting| ? cause it *could* have been supported > but happened to not be installed? Good point, added. https://codereview.chromium.org/2858563004/diff/250001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:194: Mockito.when(mPackageManager.getApplicationInfo(anyString(), anyInt())).thenReturn(ai); On 2017/05/15 18:41:37, Yaron wrote: > instead of anyString() you should be able to use hostBrowserPackage (or perhaps > equals(hostBrowserPackage) - unsure exactly about the call) Done. https://codereview.chromium.org/2858563004/diff/250001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/250001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:44: public static void resetCachedHostPackageForTesting() { On 2017/05/15 18:41:37, Yaron wrote: > Can you confirm whether these are removed for release builds after proguarding? > If not, I think you'll want to follow what we do with RemovableInRelease I have confirmed that these functions aren't in the release build, and they are in the debug build which doesn't use proguarding. https://codereview.chromium.org/2858563004/diff/250001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:159: private static List<String> getInstalledBrowsers(PackageManager packageManager) { On 2017/05/15 18:41:38, Yaron wrote: > Ok, so Peter was right that ordering matters for the other collection but this > can still use Set (and would make for faster |contains| checks) You are right, updated to set.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2858563004/diff/250001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java (right): https://codereview.chromium.org/2858563004/diff/250001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:86: Assert.assertTrue(hostBrowser.equals(expectedHostBrowser)); On 2017/05/16 13:50:39, Xi Han wrote: > On 2017/05/15 18:41:37, Yaron wrote: > > use assertEquals (throughout) for better error messages on failures. > > It is deprecated, that is why I try to void using it. Anyway, updated. Pretty sure it's just a bug in studio (or at least I thought I saw it false-positiving on the array version). assertEquals(boolean, boolean) isn't deprecated afaik. https://codereview.chromium.org/2858563004/diff/310001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/310001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:102: && installedBrowsers.contains(hostBrowserFromManifest)) { let's follow up on this line - as we're unsure if this is the desired behaviour or whether we should prompt to re-install?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #10 (id:370001) has been deleted
Patchset #8 (id:330001) has been deleted
Patchset #8 (id:350001) has been deleted
Description was changed from ========== Add support for webapk without runtimeHost An Unbound webapk will not specify a "runtimeHost" as part of the AndroidManifest. During the first launch of a WebAPK, we will use the default browser as the host browser if it supports WebAPKs. If the default browser hasn't supported WebAPKs yet, we will check if any Chrome version is installed and set it as the host browser if yes. If there isn't any proper host browser found, we will promot to install Chrome. BUG=714737 ========== to ========== Add support for webapk without runtimeHost An Unbound webapk will not specify a "runtimeHost" as part of the AndroidManifest. During the first launch of a WebAPK, we will use the default browser as the host browser if it supports WebAPKs. If the default browser hasn't supported WebAPKs yet, a dialog will pop up to ask user to choose a browser to launch the WebAPK. For exisint WebAPKs which has specified "runtimeHost": 1) Launch the WebAPK by the specified "runtimeHost" if it is installed; 2) Show a dialog to ask for users to choose a host browser. BUG=714737 ==========
Description was changed from ========== Add support for webapk without runtimeHost An Unbound webapk will not specify a "runtimeHost" as part of the AndroidManifest. During the first launch of a WebAPK, we will use the default browser as the host browser if it supports WebAPKs. If the default browser hasn't supported WebAPKs yet, a dialog will pop up to ask user to choose a browser to launch the WebAPK. For exisint WebAPKs which has specified "runtimeHost": 1) Launch the WebAPK by the specified "runtimeHost" if it is installed; 2) Show a dialog to ask for users to choose a host browser. BUG=714737 ========== to ========== Add support for webapk without runtimeHost An Unbound webapk will not specify a "runtimeHost" as part of the AndroidManifest. During the first launch of a WebAPK, we will use the default browser as the host browser if it supports WebAPKs. If the default browser hasn't supported WebAPKs yet, a dialog will pop up to ask user to choose a browser to launch the WebAPK. For exisint WebAPKs which has specified "runtimeHost": 1) Launch the WebAPK by the specified "runtimeHost" if it is installed; 2) Show a dialog to ask for users to choose a host browser. In any case, if user chooses a browser that isn't installed, we will redirect to Play Store to install it. BUG=714737 ==========
Description was changed from ========== Add support for webapk without runtimeHost An Unbound webapk will not specify a "runtimeHost" as part of the AndroidManifest. During the first launch of a WebAPK, we will use the default browser as the host browser if it supports WebAPKs. If the default browser hasn't supported WebAPKs yet, a dialog will pop up to ask user to choose a browser to launch the WebAPK. For exisint WebAPKs which has specified "runtimeHost": 1) Launch the WebAPK by the specified "runtimeHost" if it is installed; 2) Show a dialog to ask for users to choose a host browser. In any case, if user chooses a browser that isn't installed, we will redirect to Play Store to install it. BUG=714737 ========== to ========== Add support for webapk without runtimeHost An Unbound WebAPK will not specify a "runtimeHost" as part of the AndroidManifest. During the first launch of a WebAPK, we will use the default browser as the host browser if it supports WebAPKs. If the default browser hasn't supported WebAPKs yet, a dialog will pop up to ask user to choose a browser to launch the WebAPK. For exising WebAPKs which has specified "runtimeHost": 1) Launch the WebAPK by the specified "runtimeHost" if it is installed; 2) Show a dialog to ask for users to choose a host browser. In any case, if user chooses a browser that isn't installed, we will redirect to Play Store to install it. BUG=714737 ==========
Description was changed from ========== Add support for webapk without runtimeHost An Unbound WebAPK will not specify a "runtimeHost" as part of the AndroidManifest. During the first launch of a WebAPK, we will use the default browser as the host browser if it supports WebAPKs. If the default browser hasn't supported WebAPKs yet, a dialog will pop up to ask user to choose a browser to launch the WebAPK. For exising WebAPKs which has specified "runtimeHost": 1) Launch the WebAPK by the specified "runtimeHost" if it is installed; 2) Show a dialog to ask for users to choose a host browser. In any case, if user chooses a browser that isn't installed, we will redirect to Play Store to install it. BUG=714737 ========== to ========== Add support for webapk without runtimeHost An Unbound WebAPK will not specify a "runtimeHost" as part of the AndroidManifest. During the first launch of a WebAPK, we will use the default browser as the host browser if it supports WebAPKs. If the default browser hasn't supported WebAPKs yet, a dialog will pop up to ask user to choose a browser to launch the WebAPK. For exising WebAPKs which has specified "runtimeHost": 1) Launch the WebAPK by the specified "runtimeHost" if it is installed; 2) Show a dialog to ask for users to choose a host browser. In any case, if user chooses a browser that isn't installed, we will redirect to Play Store to install it. BUG=714737 ==========
Description was changed from ========== Add support for webapk without runtimeHost An Unbound WebAPK will not specify a "runtimeHost" as part of the AndroidManifest. During the first launch of a WebAPK, we will use the default browser as the host browser if it supports WebAPKs. If the default browser hasn't supported WebAPKs yet, a dialog will pop up to ask user to choose a browser to launch the WebAPK. For exising WebAPKs which has specified "runtimeHost": 1) Launch the WebAPK by the specified "runtimeHost" if it is installed; 2) Show a dialog to ask for users to choose a host browser. In any case, if user chooses a browser that isn't installed, we will redirect to Play Store to install it. BUG=714737 ========== to ========== Add support for webapk without runtimeHost An Unbound webapk will not specify a "runtimeHost" as part of the AndroidManifest. During the first launch of a WebAPK, we will use the default browser as the host browser if it supports WebAPKs. If the default browser doesn't support WebAPKs, a dialog will pop up to ask user to choose a browser to launch the WebAPK. For exising WebAPKs which has specified "runtimeHost": 1) Launch the WebAPK by the specified "runtimeHost" if it is installed; 2) Show a dialog to ask for users to choose a host browser. In any case, if user chooses a browser that isn't installed, we will redirect to Play Store to install it. BUG=714737 ==========
Patchset #9 (id:410001) has been deleted
Hi Peter and Yaron, I added the dialog to choose host browser. PTAL, thanks! https://codereview.chromium.org/2858563004/diff/250001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java (right): https://codereview.chromium.org/2858563004/diff/250001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:86: Assert.assertTrue(hostBrowser.equals(expectedHostBrowser)); On 2017/05/16 15:20:47, Yaron wrote: > On 2017/05/16 13:50:39, Xi Han wrote: > > On 2017/05/15 18:41:37, Yaron wrote: > > > use assertEquals (throughout) for better error messages on failures. > > > > It is deprecated, that is why I try to void using it. Anyway, updated. > > Pretty sure it's just a bug in studio (or at least I thought I saw it > false-positiving on the array version). assertEquals(boolean, boolean) isn't > deprecated afaik. Only the one with (object, object) is deprecated, and there isn't an API for string. https://codereview.chromium.org/2858563004/diff/310001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/310001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:102: && installedBrowsers.contains(hostBrowserFromManifest)) { On 2017/05/16 15:20:47, Yaron wrote: > let's follow up on this line - as we're unsure if this is the desired behaviour > or whether we should prompt to re-install? We will pop up the same dialog to ask user to choose a host browser.
Please hold off reviewing until I update it. Thanks!
Patchset #9 (id:430001) has been deleted
PTAL, thanks!
https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialogFragment.java (right): https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialogFragment.java:61: // The dialog content contains: Nit: "The dialog contains:" http://www.dictionary.com defines content as "something that is contained." :) https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialogFragment.java:62: // 1) a description of the dialog; Nit: ';' -> '.' https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialogFragment.java:102: public void onAttach(Context context) { Would it make more sense to pass a listener as an argument to ChooseHostBrowserDialogFragment#newInstance() https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialogFragment.java:138: } Random question: Does overriding ListAdapter#isEnabled() have the same effect as lines 158-165? https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialogFragment.java:152: holder = (BrowserListViewHolder) convertView.getTag(); For the sake of interest, why are you using View#getTag() instead of |convertView|? https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:72: launch(); I don't think that having a separate launch() function is useful anymore now that this function no longer calls finish(). https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:93: private void launchHostBrowserInWebApkMode() { I think that it now makes sense to merge this function back into onCreate(). I don't know what the function's new purpose is https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:101: } else { My preference is to make the try/catch statement as narrow as possible. String host = ""; try { host = new URL(mStartUrl).getHost(); } catch (MalformedURLException e) { ... } https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:43: */ How about: "Stores information about a potential host browser for the WebAPK." It is unnecessary to describe how the class is used. In my opinion that is an implementation detail of the caller https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:132: */ Nit: "or null if not find one." -> "or null if one is not found." OR -> "or null if we did not find one." "one" = "a host browser" or null if "a host browser" is not found. OR or null if we did not find "a host browser". https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:138: Nit: You can probably move this below line 155 like this and get rid of |hasHostBrowserSpecified| if (!TextUtils.isEmpty(cachedHostBrowser) && installedBrowsers.contains(cachedHostBrowser)) { return cachedHostBrowser; } String hostBrowserFromManifest = ... if (!TextUtils.isEmpty(hostBrowserFromManifest)) { if (installedBrowsers.contains(hostBrowserFromManifest)) { return hostBrowserFromManifest; } return null; } https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:159: deleteSharedPref(context); This omits the data cached as a result of DexOptimizer#load() in /app_dex I am ok if you do this in a follow up CL https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:173: * Returns a list of browsers to choose host browser. The list includes all the installed Nit: "to choose host browser" -> "to choose host browser from" https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:174: * browser, and if none of the installed browser supports WebAPKs, Chrome will be added to the "all of the installed browser" -> "all of the installed browsers" https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:188: supportWebApk = true; Nit: supportWebApk -> supportsWebApk The browser support*s* WebAPKs
Patchset #10 (id:470001) has been deleted
PTAL, thanks! https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialogFragment.java (right): https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialogFragment.java:61: // The dialog content contains: On 2017/05/24 04:19:29, pkotwicz wrote: > Nit: "The dialog contains:" > > http://www.dictionary.com defines content as "something that is contained." :) Thanks! https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialogFragment.java:62: // 1) a description of the dialog; On 2017/05/24 04:19:29, pkotwicz wrote: > Nit: ';' -> '.' Done. https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialogFragment.java:102: public void onAttach(Context context) { On 2017/05/24 04:19:29, pkotwicz wrote: > Would it make more sense to pass a listener as an argument to > ChooseHostBrowserDialogFragment#newInstance() The newInstance() can only interactive with the new created dialog via a Bundle by calling setArguments(). Besides, I don't think Android will call the customized constructor of ChooseHostBrowserDialogFragment when creating the dialog. I follow the suggested way of adding a listener to an alert dialog (https://developer.android.com/guide/topics/ui/dialogs.html#PassingEvents). https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialogFragment.java:138: } On 2017/05/24 04:19:29, pkotwicz wrote: > Random question: Does overriding ListAdapter#isEnabled() have the same effect as > lines 158-165? Ah, I don't know there is ListAdapter#isEnabled(int position). However, it is only for the entire item, so I still have to set enable/disable to the text to make the color correct. https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialogFragment.java:152: holder = (BrowserListViewHolder) convertView.getTag(); On 2017/05/24 04:19:29, pkotwicz wrote: > For the sake of interest, why are you using View#getTag() instead of > |convertView|? I do use convertView.getTag(). Can you elaborate more? https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:72: launch(); On 2017/05/24 04:19:29, pkotwicz wrote: > I don't think that having a separate launch() function is useful anymore now > that this function no longer calls finish(). Agree, deleted. https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:93: private void launchHostBrowserInWebApkMode() { On 2017/05/24 04:19:29, pkotwicz wrote: > I think that it now makes sense to merge this function back into onCreate(). > > I don't know what the function's new purpose is Acknowledged. https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:101: } else { On 2017/05/24 04:19:29, pkotwicz wrote: > My preference is to make the try/catch statement as narrow as possible. > > String host = ""; > try { > host = new URL(mStartUrl).getHost(); > } catch (MalformedURLException e) { > ... > } Done. https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:43: */ On 2017/05/24 04:19:30, pkotwicz wrote: > How about: "Stores information about a potential host browser for the WebAPK." > > It is unnecessary to describe how the class is used. In my opinion that is an > implementation detail of the caller Thanks! https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:132: */ On 2017/05/24 04:19:30, pkotwicz wrote: > Nit: "or null if not find one." -> "or null if one is not found." > OR > -> "or null if we did not find one." > > "one" = "a host browser" > or null if "a host browser" is not found. > OR > or null if we did not find "a host browser". Sounds better, thanks! https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:138: On 2017/05/24 04:19:30, pkotwicz wrote: > Nit: You can probably move this below line 155 like this and get rid of > |hasHostBrowserSpecified| > > if (!TextUtils.isEmpty(cachedHostBrowser) && > installedBrowsers.contains(cachedHostBrowser)) { > return cachedHostBrowser; > } > > String hostBrowserFromManifest = ... > if (!TextUtils.isEmpty(hostBrowserFromManifest)) { > if (installedBrowsers.contains(hostBrowserFromManifest)) { > return hostBrowserFromManifest; > } > return null; > } > I had a discussion with Yaron about the order, and we need to check the manifest first. This is because for existing-WebAPKs, the runtime host specified in the manifest is the first choice, and we only look for cached one if the specified one isn't installed. But it raise a question: 1) the specified browser is uninstalled, so we pop up the dialog to ask user to choose. 2) user chooses another browser. 3) user install the previous browser again. Then which browser do we want to use to launch the WebAPK? Should we keep using the chosen one in 2) and don't do any magic switch? Thoughts? https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:159: deleteSharedPref(context); On 2017/05/24 04:19:30, pkotwicz wrote: > This omits the data cached as a result of DexOptimizer#load() in /app_dex > > I am ok if you do this in a follow up CL I have been thinking about this. From my understanding, if WebAPK changes its host browser to non-Chrome, WebAPK no longer needs to know whether Chrome loads the baked WebAPK runtime dex file. Eventually, I guess we will end up to call load() again to load dex from the new host browser. It should be safe to delete the Shared pref here, right? https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:173: * Returns a list of browsers to choose host browser. The list includes all the installed On 2017/05/24 04:19:30, pkotwicz wrote: > Nit: "to choose host browser" -> "to choose host browser from" Done. https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:174: * browser, and if none of the installed browser supports WebAPKs, Chrome will be added to the On 2017/05/24 04:19:30, pkotwicz wrote: > "all of the installed browser" -> "all of the installed browsers" Acknowledged. https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:188: supportWebApk = true; On 2017/05/24 04:19:30, pkotwicz wrote: > Nit: supportWebApk -> supportsWebApk > > The browser support*s* WebAPKs Done.
https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialogFragment.java (right): https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialogFragment.java:102: public void onAttach(Context context) { Ok, I see now. Thank you for the explanation. https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialogFragment.java:138: } Thank you for verifying! https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:138: I don't have a strong preference. I would add an extra choice to consider: "Show the user the dialog again and have them select which browser they want to use." Regardless of the choice we should clear SharedPreferences when we do the switch. We should also unit test this choice. (I have not checked whether you have already added unit tests) https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:159: deleteSharedPref(context); Sorry for the confusion. Deleting the SharedPref is not sufficient. If the WebAPK fails to load the dex file from the host browser, it extracts the dex file from the host browser APK into its private data directory. This is currently always the case on Android Jellybean. We don't want the WebAPK to still be using the "Chrome runtime library" if the host browser is no longer Chrome https://codereview.chromium.org/2858563004/diff/490001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java (right): https://codereview.chromium.org/2858563004/diff/490001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:70: WebApkUtils.setBrowsersSupportingWebApkForTesting(sBrowsersSupportingWebApks); Is calling setBrowsersSupportingWebApkForTesting() necessary? Isn't it possible to use RobolectricPackageManager#addResolveInfoForIntent() to alter which packages robolectric thinks are installed? (and use real package names like com.chrome.canary) https://codereview.chromium.org/2858563004/diff/490001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:99: * 1) a host browser specified in the AndroidManifest.xml but isn't installed; "browser specified" -> "browser is specified" https://codereview.chromium.org/2858563004/diff/490001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:199: private void mockInstallBrowsers() { - Can you use RobolectricPackageManager#addResolveInfoForIntent() the way that WebApkValidatorTest.java does? - Can you add a |packageName| parameter? That way a test can vary how many packages are installed - Can this function make the browser the default browser if appropriate (perhaps via a boolean parameter |makeDefault|) https://codereview.chromium.org/2858563004/diff/490001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:217: Mockito.when(sharedPreferences.edit()).thenReturn(editor); Is this code necessary? Can you instead do: SharedPreferences sharedPref = mContext.getSharedPreferences(WebApkConstants.PREF_PACKAGE, Context.MODE_PRIVATE); SharedPreferences.Editor editor = sharedPref.edit(); editor.putString(SHARED_PREF_RUNTIME_HOST, hostBrowserPackage); editor.apply(); https://codereview.chromium.org/2858563004/diff/490001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:220: private void mockWebApkPackageMetadata(String hostBrowserPackage) { Can you use WebApkTestHelper#registerWebApkWithMetaData() here? https://codereview.chromium.org/2858563004/diff/490001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2858563004/diff/490001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:58: private DialogFragment mDialog; It looks like |mDialog| is only used in onCreate(). Can it be local to onCreate()? https://codereview.chromium.org/2858563004/diff/490001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:134: // show. Nits: - WebApkUtils#getHostBrowserPackageName -> WebApkUtils#getHostBrowserPackageName() - "can avoid the dialog to show." -> "can avoid showing the dialog." (I wasn't able to find a reference for why this is the case) https://codereview.chromium.org/2858563004/diff/490001/chrome/android/webapk/... File chrome/android/webapk/strings/android_webapk_strings.grd (right): https://codereview.chromium.org/2858563004/diff/490001/chrome/android/webapk/... chrome/android/webapk/strings/android_webapk_strings.grd:103: <message name="IDS_CHOOSE_HOST_BROWSER_DIALOG_CANCEL" desc="Text for the cancel button on the dialog. "> Maybe rename this to IDS_CHOOSE_HOST_BROWSER_DIALOG_QUIT to match ChooseHostBrowserDialogFragment#DialogListener
Patchset #11 (id:510001) has been deleted
Hi Peter, could you please take another look? Thanks! https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:138: On 2017/05/25 02:59:08, pkotwicz wrote: > I don't have a strong preference. > > I would add an extra choice to consider: > "Show the user the dialog again and have them select which browser they want to > use." > > Regardless of the choice we should clear SharedPreferences when we do the > switch. We should also unit test this choice. (I have not checked whether you > have already added unit tests) I updated to your suggested solution, since sticking to the host browser which was chosen before can avoid unnecessary switch of the host browser. I will update the tests accordingly. https://codereview.chromium.org/2858563004/diff/450001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:159: deleteSharedPref(context); On 2017/05/25 02:59:09, pkotwicz wrote: > Sorry for the confusion. Deleting the SharedPref is not sufficient. If the > WebAPK fails to load the dex file from the host browser, it extracts the dex > file from the host browser APK into its private data directory. This is > currently always the case on Android Jellybean. > > We don't want the WebAPK to still be using the "Chrome runtime library" if the > host browser is no longer Chrome > I see. Yes, that part will be in a follow up CL. https://codereview.chromium.org/2858563004/diff/490001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java (right): https://codereview.chromium.org/2858563004/diff/490001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:70: WebApkUtils.setBrowsersSupportingWebApkForTesting(sBrowsersSupportingWebApks); On 2017/05/25 02:59:09, pkotwicz wrote: > Is calling setBrowsersSupportingWebApkForTesting() necessary? Isn't it possible > to use RobolectricPackageManager#addResolveInfoForIntent() to alter which > packages robolectric thinks are installed? (and use real package names like > com.chrome.canary) Yes, it is necessary. The |sBrowsersSupportingWebApks| is hard coded in WebApkUtils, and it is not obtained from the PackageManager. So it has to be set via setBrowsersSupportingWebApkForTesting(). https://codereview.chromium.org/2858563004/diff/490001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:99: * 1) a host browser specified in the AndroidManifest.xml but isn't installed; On 2017/05/25 02:59:09, pkotwicz wrote: > "browser specified" -> "browser is specified" Done. https://codereview.chromium.org/2858563004/diff/490001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:199: private void mockInstallBrowsers() { On 2017/05/25 02:59:09, pkotwicz wrote: > - Can you use RobolectricPackageManager#addResolveInfoForIntent() the way that > WebApkValidatorTest.java does? > - Can you add a |packageName| parameter? That way a test can vary how many > packages are installed > - Can this function make the browser the default browser if appropriate (perhaps > via a boolean parameter |makeDefault|) I didn't find a way to set the default browser through RobolectricPackageManager's API, so can't use RobolectricPackageManager but have to mock everything. https://codereview.chromium.org/2858563004/diff/490001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:217: Mockito.when(sharedPreferences.edit()).thenReturn(editor); On 2017/05/25 02:59:09, pkotwicz wrote: > Is this code necessary? Can you instead do: > > SharedPreferences sharedPref = > mContext.getSharedPreferences(WebApkConstants.PREF_PACKAGE, > Context.MODE_PRIVATE); > SharedPreferences.Editor editor = sharedPref.edit(); > editor.putString(SHARED_PREF_RUNTIME_HOST, hostBrowserPackage); > editor.apply(); The context isn't a Robolectric RuntimeEnvironment, so it has to be mocked in this way to avoid a null pointer exception. https://codereview.chromium.org/2858563004/diff/490001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:220: private void mockWebApkPackageMetadata(String hostBrowserPackage) { On 2017/05/25 02:59:09, pkotwicz wrote: > Can you use WebApkTestHelper#registerWebApkWithMetaData() here? Same reason as above. https://codereview.chromium.org/2858563004/diff/490001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2858563004/diff/490001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:58: private DialogFragment mDialog; On 2017/05/25 02:59:09, pkotwicz wrote: > It looks like |mDialog| is only used in onCreate(). Can it be local to > onCreate()? Done. https://codereview.chromium.org/2858563004/diff/490001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:134: // show. On 2017/05/25 02:59:09, pkotwicz wrote: > Nits: > - WebApkUtils#getHostBrowserPackageName -> > WebApkUtils#getHostBrowserPackageName() > - "can avoid the dialog to show." -> "can avoid showing the dialog." (I wasn't > able to find a reference for why this is the case) Thanks! https://codereview.chromium.org/2858563004/diff/490001/chrome/android/webapk/... File chrome/android/webapk/strings/android_webapk_strings.grd (right): https://codereview.chromium.org/2858563004/diff/490001/chrome/android/webapk/... chrome/android/webapk/strings/android_webapk_strings.grd:103: <message name="IDS_CHOOSE_HOST_BROWSER_DIALOG_CANCEL" desc="Text for the cancel button on the dialog. "> On 2017/05/25 02:59:09, pkotwicz wrote: > Maybe rename this to IDS_CHOOSE_HOST_BROWSER_DIALOG_QUIT to match > ChooseHostBrowserDialogFragment#DialogListener Done.
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add support for webapk without runtimeHost An Unbound webapk will not specify a "runtimeHost" as part of the AndroidManifest. During the first launch of a WebAPK, we will use the default browser as the host browser if it supports WebAPKs. If the default browser doesn't support WebAPKs, a dialog will pop up to ask user to choose a browser to launch the WebAPK. For exising WebAPKs which has specified "runtimeHost": 1) Launch the WebAPK by the specified "runtimeHost" if it is installed; 2) Show a dialog to ask for users to choose a host browser. In any case, if user chooses a browser that isn't installed, we will redirect to Play Store to install it. BUG=714737 ========== to ========== Add support for webapk without runtimeHost An Unbound webapk will not specify a "runtimeHost" as part of the AndroidManifest. During the first launch of a WebAPK, we will use the default browser as the host browser if it supports WebAPKs. If the default browser doesn't support WebAPKs, a dialog will pop up to ask user to choose a browser to launch the WebAPK. The UX of the dialog is: https://docs.google.com/presentation/d/1ZC7Fc9Y7-vsGqvbEyiZmiJoWtsONkhPBUb47F... For existing WebAPKs which has specified "runtimeHost": 1) Launch the WebAPK by the specified "runtimeHost" if it is installed; 2) Show a dialog to ask for users to choose a host browser. In any case, if user chooses a browser that isn't installed, we will redirect to Play Store to install it. BUG=714737 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2858563004/diff/490001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java (right): https://codereview.chromium.org/2858563004/diff/490001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:70: WebApkUtils.setBrowsersSupportingWebApkForTesting(sBrowsersSupportingWebApks); I was thinking of using the hard coded values in WebApkUtils in the tests https://codereview.chromium.org/2858563004/diff/490001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:199: private void mockInstallBrowsers() { This seems to work (Partial mocks for the win!) public void setUp() { mContext = RuntimeEnvironment.application; mPackageManager = Mockito.spy((RobolectricPackageManager) RuntimeEnvironment.getPackageManager()); RuntimeEnvironment.setRobolectricPackageManager(mPackageManager); WebApkUtils.resetCachedHostPackageForTesting(); WebApkUtils.setBrowsersSupportingWebApkForTesting(sBrowsersSupportingWebApks); } private void installBrowser(String packageName, boolean isDefault) { ResolveInfo resolveInfo = newResolveInfo(packageName); Intent intent = null; try { intent = Intent.parseUri("http://", Intent.URI_INTENT_SCHEME); } catch (Exception e) { } finally { } mPackageManager.addResolveInfoForIntent(intent, resolveInfo); if (isDefault) { Mockito.doReturn(resolveInfo).when(mPackageManager).resolveActivity(any(Intent.class), anyInt()); ResolveInfo info = ((PackageManager) mPackageManager) .resolveActivity(intent, PackageManager.MATCH_DEFAULT_ONLY); } }
Patchset #12 (id:550001) has been deleted
PTAL, thanks!
https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java (right): https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:44: "browser.installed.supporting.webapks"; Can this be com.chrome.canary? https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:46: "browser.uninstalled.supporting.webapks"; Can this be com.chrome.dev? https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:50: "another.browser.installed.supporting.webapks"; Can this be com.chrome.beta? https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:87: * not. How about: "no matter whether a host browser is specified in the AndroidManifest.xml or not" -> "even if a host browser is specified in the AndroidManifest.xml." You don't test of the "host browser" not being specified in AndroidManifest.xml (I think that it is unnecessary) https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:92: mockInstallBrowsers(); The default browser should be a parameter to mockInstallBrowsers(). If mockDefaultBrowser() is not called the default browser is BROWSER_INSTALLED_NOT_SUPPORTING_WEBAPK because PackageManager#addResolveInfoForIntent() is called for BROWSER_INSTALLED_NOT_SUPPORTING_WEBAPK first. https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:104: * uninstalled; Nit: Make the comment reflect the test. You don't test the scenario of the SharedPreference containing the package name of an uninstalled browser. I don't think you need to test the scenario. If an "uninstalled browser which supports WebAPKs" in SharedPreferences did anything WebApkUtils#getHostBrowserPackageName() would not return null in testReturnsNullIfHostBrowserSpecifiedInManifestIsUninstalled() https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:126: public void testReturnsNullIfHostBrowserSpecifiedInManifestIsUninstalled() { This test is identical to testReturnsNullWhenDefaultBrowserDoesNotSupportWebApks() The default browser for this test is BROWSER_INSTALLED_NOT_SUPPORTING_WEBAPK https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:135: // Simulates that the host browser stored in the SharedPreference has been uninstalled. Perhaps this should be its own test. We can simplify the test to not specify a host browser in the WebAPK meta data. https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:162: // Simulates that the host browser stored in the SharedPreference isn't installed. I don't think that you need to test this case. If an "uninstalled browser which supports WebAPKs" in SharedPreferences did anything WebApkUtils#getHostBrowserPackageName() would not return null in testReturnsNullIfHostBrowserSpecifiedInManifestIsUninstalled() https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:211: private void mockSharedPreferences(String hostBrowserPackage) { Maybe rename this function to setHostBrowserInSharedPreferences() I think that my suggestion better describes what the function does and the role of |hostBrowserPackage| https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:219: private void mockWebApkPackageMetadata(String hostBrowserPackage) { Maybe rename this function to setHostBrowserInMetadata() https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:225: private void mockDefaultBrowser(String defaultBrowser) { Maybe rename this function to setDefaultBrowser() https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialogFragment.java (right): https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialogFragment.java:27: * Shows the dialog to choose a host browser to launch WebAPKs. Calls the listener callback when the Nit: WebAPKs -> WebAPK https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialogFragment.java:42: // Listens to which browser is chosen by the user to launch WebAPKs. Nit: WebAPKs -> WebAPK https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:94: } Don't you need to call WebApkUtils#deleteSharedPref() here? For instance if: - WebApkUtils#getHostBrowserPackageName() returns a value but - launchInHostBrowser() returns false I am tempted to recommend making launchInHostBrowser() return void to make our life easier https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:127: } Don't you need to call WebApkUtils#deleteSharedPref() here? For instance if: - WebApkUtils#getHostBrowserPackageName() returns a value but - launchInHostBrowser() returns false https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:35: // comes first. Nit: use block commenting style https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:47: private boolean mIsEnabled; WebApkUtils should not know about the UI in which BrowserItem is used. Maybe rename this variable to mSupportsWebApks https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:57: // Returns the package name of a browser. Nit: Use /** */ for comments in this class. https://google.github.io/styleguide/javaguide.html#s7.1.1-javadoc-multi-line https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:123: } The Shared Preferences should be cleared if: 1) The user installs a WebAPK from Chrome Canary. 2) User launches WebAPK. SharedPreferences are populated with com.chrome.canary 3) User uninstalls Chrome Canary and installs Chrome Dev 4) User launches WebAPK. SharedPreferences are populated with com.chrome.dev The browser selection dialog might show. 5) User uninstalls Chrome Dev and installs Chrome Canary. 6) User launches WebAPK. Shared preferences are populated with com.chrome.canary. The browser selection dialog will not show because the WebAPK was created for Chrome Canary. In (6) I would expect deleteSharedPref() to be called. However, it is not called.
Patchset #14 (id:610001) has been deleted
Patchset #13 (id:590001) has been deleted
Hi Peter, PTAL, thanks for your comments! https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java (right): https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:44: "browser.installed.supporting.webapks"; On 2017/05/26 22:38:38, pkotwicz wrote: > Can this be com.chrome.canary? I would prefer this naming, since it reflects what the browser is supposed to be. https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:87: * not. On 2017/05/26 22:38:38, pkotwicz wrote: > How about: "no matter whether a host browser is specified in the > AndroidManifest.xml or not" -> "even if a host browser is specified in the > AndroidManifest.xml." > > You don't test of the "host browser" not being specified in AndroidManifest.xml > (I think that it is unnecessary) Sound good, thanks! https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:92: mockInstallBrowsers(); On 2017/05/26 22:38:38, pkotwicz wrote: > The default browser should be a parameter to mockInstallBrowsers(). > > If mockDefaultBrowser() > is not called the default browser is BROWSER_INSTALLED_NOT_SUPPORTING_WEBAPK > because PackageManager#addResolveInfoForIntent() is called for > BROWSER_INSTALLED_NOT_SUPPORTING_WEBAPK > first. Yes, adding the default browser as a parameter is a good idea. It guarantees that the default browser is in the installed browser list. Right, the first one in the |sInstalledBrowser| is the default one when nothing is set. Since both of them are used as default browser once, putting ANOTHER_BROWSER_INSTALLED_SUPPORTING_WEBAPKS as the first one in the |sInstalledBrowser|. https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:104: * uninstalled; On 2017/05/26 22:38:38, pkotwicz wrote: > Nit: Make the comment reflect the test. You don't test the scenario of the > SharedPreference containing the package name of an uninstalled browser. I don't > think you need to test the scenario. If an "uninstalled browser which supports > WebAPKs" in SharedPreferences did anything > WebApkUtils#getHostBrowserPackageName() would not return null in > testReturnsNullIfHostBrowserSpecifiedInManifestIsUninstalled() Good catch! I add the missing part to the test to make the test including all the possible cases. Even though it might be covered by other tests, but not very obvious to me. https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:126: public void testReturnsNullIfHostBrowserSpecifiedInManifestIsUninstalled() { On 2017/05/26 22:38:38, pkotwicz wrote: > This test is identical to > testReturnsNullWhenDefaultBrowserDoesNotSupportWebApks() > > The default browser for this test is BROWSER_INSTALLED_NOT_SUPPORTING_WEBAPK Sorry for the confusion. They are different tests, this one is for browser-WebAPK (has specified runtime host); the other is for play-WebAPK (without runtime host). I added a comment to specify the situation. https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:135: // Simulates that the host browser stored in the SharedPreference has been uninstalled. On 2017/05/26 22:38:38, pkotwicz wrote: > Perhaps this should be its own test. We can simplify the test to not specify a > host browser in the WebAPK meta data. This is a test for browser-WebAPK, which has a host browser in the meta data. https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:162: // Simulates that the host browser stored in the SharedPreference isn't installed. On 2017/05/26 22:38:38, pkotwicz wrote: > I don't think that you need to test this case. If an "uninstalled browser which > supports WebAPKs" in SharedPreferences did anything > WebApkUtils#getHostBrowserPackageName() would not return null in > testReturnsNullIfHostBrowserSpecifiedInManifestIsUninstalled() That test is for browser-webapk, while this is for play-webapk. Only in the case of no host browser in the meta data will lead to a default browser check. https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:211: private void mockSharedPreferences(String hostBrowserPackage) { On 2017/05/26 22:38:38, pkotwicz wrote: > Maybe rename this function to setHostBrowserInSharedPreferences() > > I think that my suggestion better describes what the function does and the role > of |hostBrowserPackage| SG https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:219: private void mockWebApkPackageMetadata(String hostBrowserPackage) { On 2017/05/26 22:38:38, pkotwicz wrote: > Maybe rename this function to setHostBrowserInMetadata() Done. https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:225: private void mockDefaultBrowser(String defaultBrowser) { On 2017/05/26 22:38:38, pkotwicz wrote: > Maybe rename this function to setDefaultBrowser() Move the logic to |mockInstalledBrowsers()|. https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialogFragment.java (right): https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialogFragment.java:27: * Shows the dialog to choose a host browser to launch WebAPKs. Calls the listener callback when the On 2017/05/26 22:38:39, pkotwicz wrote: > Nit: WebAPKs -> WebAPK Done. https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialogFragment.java:42: // Listens to which browser is chosen by the user to launch WebAPKs. On 2017/05/26 22:38:39, pkotwicz wrote: > Nit: WebAPKs -> WebAPK Done. https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:94: } On 2017/05/26 22:38:39, pkotwicz wrote: > Don't you need to call WebApkUtils#deleteSharedPref() here? > > For instance if: > - WebApkUtils#getHostBrowserPackageName() returns a value > but > - launchInHostBrowser() returns false > > I am tempted to recommend making launchInHostBrowser() return void to make our > life easier You are right. Call it inside launchiInHostBrowser to avoid delete the shared pref more than once. https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:127: } On 2017/05/26 22:38:39, pkotwicz wrote: > Don't you need to call WebApkUtils#deleteSharedPref() here? > > For instance if: > - WebApkUtils#getHostBrowserPackageName() returns a value > but > - launchInHostBrowser() returns false Add a call in |launchInHostBrowser|. https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:35: // comes first. On 2017/05/26 22:38:39, pkotwicz wrote: > Nit: use block commenting style Done. https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:47: private boolean mIsEnabled; On 2017/05/26 22:38:39, pkotwicz wrote: > WebApkUtils should not know about the UI in which BrowserItem is used. Maybe > rename this variable to mSupportsWebApks Done. https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:57: // Returns the package name of a browser. On 2017/05/26 22:38:39, pkotwicz wrote: > Nit: Use /** */ for comments in this class. > https://google.github.io/styleguide/javaguide.html#s7.1.1-javadoc-multi-line Thanks! https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:123: } On 2017/05/26 22:38:39, pkotwicz wrote: > The Shared Preferences should be cleared if: > 1) The user installs a WebAPK from Chrome Canary. > 2) User launches WebAPK. SharedPreferences are populated with com.chrome.canary > 3) User uninstalls Chrome Canary and installs Chrome Dev > 4) User launches WebAPK. SharedPreferences are populated with com.chrome.dev The > browser selection dialog might show. > 5) User uninstalls Chrome Dev and installs Chrome Canary. > 6) User launches WebAPK. Shared preferences are populated with > com.chrome.canary. The browser selection dialog will not show because the WebAPK > was created for Chrome Canary. > > In (6) I would expect deleteSharedPref() to be called. However, it is not > called. I moved the call of deleteSharedPref() in getHostBrowserPackageNameInternal() to above check the metadata, so I believe that will solve this issue.
https://codereview.chromium.org/2858563004/diff/630001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2858563004/diff/630001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:80: Personally, I would structure the code this way: String runtimeHostInPreferences = WebApkUtils.getHostBrowserFromSharedPreferences(); String runtimeHost = WebApkUtils.getHostBrowserPackageName(this); if (!TextUtils.isEmpty(runtimeHostInPreferences) && !runtimeHostInPreferences.equals(runtimeHost)) { WebApkUtils.deleteSharedPref(this); } This way the deletion of the shared preferences in only one place. Querying "runtime_host" from SharedPreferences multiple times should be quick.
Patchset #14 (id:650001) has been deleted
Patchset #14 (id:670001) has been deleted
PTAL, thanks! https://codereview.chromium.org/2858563004/diff/630001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2858563004/diff/630001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:80: On 2017/05/30 02:15:29, pkotwicz wrote: > Personally, I would structure the code this way: > > String runtimeHostInPreferences = > WebApkUtils.getHostBrowserFromSharedPreferences(); > String runtimeHost = WebApkUtils.getHostBrowserPackageName(this); > > if (!TextUtils.isEmpty(runtimeHostInPreferences) && > !runtimeHostInPreferences.equals(runtimeHost)) { > WebApkUtils.deleteSharedPref(this); > } > > This way the deletion of the shared preferences in only one place. Querying > "runtime_host" from SharedPreferences multiple times should be quick. It makes sense to move the part of "deleting the shared pref" out of the |getHostBrowserPackageName()|, since it isn't related to the naming of that function at all. Besides, remove the call of WebApkUtils.deleteSharedPref() in launchInHostBrowser() and change the return value of launchInHostBrowser() to be void. As discussed offline, it is very rare to fail in launchInHostBrowser() for a webapk installed from the WAM server.
Only test related comments! I think you can send the CL for UI review :) https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java (right): https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:44: "browser.installed.supporting.webapks"; I am suggesting this naming because it enables getting rid of WebApkUtils#setBrowsersSupportingWebApkForTesting() https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:162: // Simulates that the host browser stored in the SharedPreference isn't installed. I think of WebApkUtils#getHostBrowserPackageName() mostly as String getHostBrowserPackageName() { if (...) { return not-null; } if (...) { return not-null; } if (...) { return not-null; } return null; } with the exception that the default browser is ignored if the Android Manifest contains a runtime host. For this reason, I think that you only need a single test case to check the behavior when the browser listed in the SharedPreference has been uninstalled The test cases to care about are: S=Shared Preferences M=Manifest D=Default Browser I=Installed U=Uninstalled N=Null Q=Not WebAPK Compatible SI SN MN DQ SN MN DI SN MU DQ --> we do not need this test because the test case for "SN MU DI" returns null SN MU DI SN MI SU MN DQ SU MN DI --> we do not need this test SU MU DQ --> we do not need this test SU MU DI --> we do not need this test SU MI --> we do not need this test https://codereview.chromium.org/2858563004/diff/690001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java (right): https://codereview.chromium.org/2858563004/diff/690001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:91: mockInstallBrowsers(null); Please include the name of a browser. I think that null means ANOTHER_BROWSER_INSTALLED_SUPPORTING_WEBAPKS https://codereview.chromium.org/2858563004/diff/690001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:180: * been uninstalled; Nit: Remove "or the specified one has been uninstalled" https://codereview.chromium.org/2858563004/diff/690001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:182: * 2. there isn't a host browser specified in the AndroidManifest.xml; Nit: ';' -> '.' https://codereview.chromium.org/2858563004/diff/690001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:222: if (defaultBrowser != null && defaultBrowserInfo == null) { - Shouldn't defaultBrowser also != null. In this case you can just get rid of the if() statement entirely. - Tests don't have to be efficient. You can recreate the ResolveInfo https://codereview.chromium.org/2858563004/diff/690001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2858563004/diff/690001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:177: private String getStartUrl() { Nit: You will need to merge with https://chromium-review.googlesource.com/c/517563/
Patchset #15 (id:710001) has been deleted
hanxi@chromium.org changed reviewers: + tedchoc@chromium.org
Hi Peter, PTAL. +Ted: could you please do a UI review for: - webapk/shell_apk/ChooseHostBrowserDialog.java Thanks! https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java (right): https://codereview.chromium.org/2858563004/diff/570001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:44: "browser.installed.supporting.webapks"; On 2017/05/31 01:06:21, pkotwicz wrote: > I am suggesting this naming because it enables getting rid of > WebApkUtils#setBrowsersSupportingWebApkForTesting() > Got it. Updated the naming. https://codereview.chromium.org/2858563004/diff/690001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java (right): https://codereview.chromium.org/2858563004/diff/690001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:91: mockInstallBrowsers(null); On 2017/05/31 01:06:22, pkotwicz wrote: > Please include the name of a browser. I think that null means > ANOTHER_BROWSER_INSTALLED_SUPPORTING_WEBAPKS All right. I change the order, and make the BROWSER_INSTALLED_NOT_SUPPORTING_WEBAPKS as the default. https://codereview.chromium.org/2858563004/diff/690001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:180: * been uninstalled; On 2017/05/31 01:06:22, pkotwicz wrote: > Nit: Remove "or the specified one has been uninstalled" It is a possible condition but not part of the test. I would prefer to keep it, but add a note in the comment to indicate it. https://codereview.chromium.org/2858563004/diff/690001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:182: * 2. there isn't a host browser specified in the AndroidManifest.xml; On 2017/05/31 01:06:22, pkotwicz wrote: > Nit: ';' -> '.' Done. https://codereview.chromium.org/2858563004/diff/690001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:222: if (defaultBrowser != null && defaultBrowserInfo == null) { On 2017/05/31 01:06:22, pkotwicz wrote: > - Shouldn't defaultBrowser also != null. In this case you can just get rid of > the if() statement entirely. > - Tests don't have to be efficient. You can recreate the ResolveInfo Good catch, I shouldn't put complex logic in tests. Updated.
LGTM!!! Thank you very much for bearing with me and my nit pickiness https://codereview.chromium.org/2858563004/diff/730001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java (right): https://codereview.chromium.org/2858563004/diff/730001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:38: Nit: Remove new line. You can make the comment a single line too. /** Tests for WebApkUtils. */ https://codereview.chromium.org/2858563004/diff/730001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:82: mockInstallBrowsers(BROWSER_INSTALLED_NOT_SUPPORTING_WEBAPKS); Might as well make the default browser: ANOTHER_BROWSER_INSTALLED_SUPPORTING_WEBAPKS That way you test precedence as well https://codereview.chromium.org/2858563004/diff/730001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:127: mockInstallBrowsers(BROWSER_INSTALLED_NOT_SUPPORTING_WEBAPKS); I think that ANOTHER_BROWSER_INSTALLED_SUPPORTING_WEBAPKS is a better value for the default browser. That way you test that the default browser is ignored. https://codereview.chromium.org/2858563004/diff/730001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:163: * 2. there isn't a host browser specified in the AndroidManifest.xml. Isn't this condition redundant? You mention it in this part: "This is a test for the WebAPK WITHOUT any runtime host specified in its AndroidManifest.xml" https://codereview.chromium.org/2858563004/diff/730001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:191: e.printStackTrace(); Calling fail() would probably be more appropriate https://codereview.chromium.org/2858563004/diff/730001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:195: ResolveInfo defaultBrowserInfo = null; Can you move declaration of |defaultBrowserInfo| to line 204?
Patchset #17 (id:770001) has been deleted
Patchset #16 (id:750001) has been deleted
cool! This is looking pretty solid with one question on data clearing and me not knowing about the UI stuff https://codereview.chromium.org/2858563004/diff/790001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2858563004/diff/790001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:52: private String mOverrideUrl; This is risky unless we always reset them. I see they're helpful cause of the dialog now though so I guess we can keep them. Should have a comment for member variables https://codereview.chromium.org/2858563004/diff/790001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:83: WebApkUtils.deleteSharedPref(this); should we not clear all data: https://developer.android.com/reference/android/app/ActivityManager.html#clea... Only works on API 19+ so we could fallback to deleting shared data. https://codereview.chromium.org/2858563004/diff/790001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:106: public void onHostBrowserSelected(String runtimeHost) { nit: rename |selectedHost| This confused me for a bit cause we're in the process making this the runtimeHost
oh as discussed off-thread, since we've deviated from mocks, please upload some screenshots to drive and link from the bug
Description was changed from ========== Add support for webapk without runtimeHost An Unbound webapk will not specify a "runtimeHost" as part of the AndroidManifest. During the first launch of a WebAPK, we will use the default browser as the host browser if it supports WebAPKs. If the default browser doesn't support WebAPKs, a dialog will pop up to ask user to choose a browser to launch the WebAPK. The UX of the dialog is: https://docs.google.com/presentation/d/1ZC7Fc9Y7-vsGqvbEyiZmiJoWtsONkhPBUb47F... For existing WebAPKs which has specified "runtimeHost": 1) Launch the WebAPK by the specified "runtimeHost" if it is installed; 2) Show a dialog to ask for users to choose a host browser. In any case, if user chooses a browser that isn't installed, we will redirect to Play Store to install it. BUG=714737 ========== to ========== Add support for webapk without runtimeHost An Unbound webapk will not specify a "runtimeHost" as part of the AndroidManifest. During the first launch of a WebAPK, we will use the default browser as the host browser if it supports WebAPKs. If the default browser doesn't support WebAPKs, a dialog will pop up to ask user to choose a browser to launch the WebAPK. The UX of the dialog is: https://docs.google.com/presentation/d/1ZC7Fc9Y7-vsGqvbEyiZmiJoWtsONkhPBUb47F... For existing WebAPKs which has specified "runtimeHost": 1) Launch the WebAPK by the specified "runtimeHost" if it is installed; 2) Show a dialog to ask for users to choose a host browser. In any case, if user chooses a browser that isn't installed, we will redirect to Play Store to install it. The screenshots refer to: https://drive.google.com/open?id=0B7zEF5GgyYmpVkowOUY4ZGFKTmM BUG=714737 ==========
Patchset #17 (id:730002) has been deleted
Patchset #17 (id:820001) has been deleted
Thanks Yaron. I add a link for screenshots, and will update my patch soon. https://codereview.chromium.org/2858563004/diff/730001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java (right): https://codereview.chromium.org/2858563004/diff/730001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:38: On 2017/05/31 17:05:57, pkotwicz wrote: > Nit: Remove new line. You can make the comment a single line too. > > /** Tests for WebApkUtils. */ Done. https://codereview.chromium.org/2858563004/diff/730001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:82: mockInstallBrowsers(BROWSER_INSTALLED_NOT_SUPPORTING_WEBAPKS); On 2017/05/31 17:05:57, pkotwicz wrote: > Might as well make the default browser: > ANOTHER_BROWSER_INSTALLED_SUPPORTING_WEBAPKS > > That way you test precedence as well Done. https://codereview.chromium.org/2858563004/diff/730001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:127: mockInstallBrowsers(BROWSER_INSTALLED_NOT_SUPPORTING_WEBAPKS); On 2017/05/31 17:05:57, pkotwicz wrote: > I think that ANOTHER_BROWSER_INSTALLED_SUPPORTING_WEBAPKS is a better value for > the default browser. That way you test that the default browser is ignored. Good idea, thanks! https://codereview.chromium.org/2858563004/diff/730001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:163: * 2. there isn't a host browser specified in the AndroidManifest.xml. On 2017/05/31 17:05:57, pkotwicz wrote: > Isn't this condition redundant? You mention it in this part: "This is a test for > the WebAPK WITHOUT any runtime host specified in its AndroidManifest.xml" Removed. https://codereview.chromium.org/2858563004/diff/730001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:191: e.printStackTrace(); On 2017/05/31 17:05:57, pkotwicz wrote: > Calling fail() would probably be more appropriate Done. https://codereview.chromium.org/2858563004/diff/730001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:195: ResolveInfo defaultBrowserInfo = null; On 2017/05/31 17:05:57, pkotwicz wrote: > Can you move declaration of |defaultBrowserInfo| to line 204? Thanks. Also remove line 199 - 201 since it isn't necessary.
PTAL, thanks! https://codereview.chromium.org/2858563004/diff/790001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2858563004/diff/790001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:52: private String mOverrideUrl; On 2017/05/31 18:56:04, Yaron wrote: > This is risky unless we always reset them. I see they're helpful cause of the > dialog now though so I guess we can keep them. Should have a comment for member > variables Done. https://codereview.chromium.org/2858563004/diff/790001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:83: WebApkUtils.deleteSharedPref(this); On 2017/05/31 18:56:04, Yaron wrote: > should we not clear all data: > https://developer.android.com/reference/android/app/ActivityManager.html#clea... > Only works on API 19+ so we could fallback to deleting shared data. Thanks! https://codereview.chromium.org/2858563004/diff/790001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:106: public void onHostBrowserSelected(String runtimeHost) { On 2017/05/31 18:56:04, Yaron wrote: > nit: rename |selectedHost| > This confused me for a bit cause we're in the process making this the > runtimeHost Done.
https://codereview.chromium.org/2858563004/diff/860001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2858563004/diff/860001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:55: /** The URL to launch the WebAPK. */ Used in the case of deep-links (intents from other apps) that fall into the WebApk scope. https://codereview.chromium.org/2858563004/diff/860001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:92: activityManager.clearApplicationUserData(); Err, I think this kills the app so you need some verification that this doesn't break eerything. https://codereview.chromium.org/2858563004/diff/860001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:94: WebApkUtils.deleteSharedPref(this); Err I should've said that this isn't quite equivalent to clearApp.. so we may want to delete some other things that aren't shared prefs (e.g. runtime dex that's extracted and really anything in the private directory)
https://codereview.chromium.org/2858563004/diff/860001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java (right): https://codereview.chromium.org/2858563004/diff/860001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:28: public class ChooseHostBrowserDialog { Sorry for being slow to review, but any thoughts on using the ItemChooserDialog already in chromium? https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... Ideally, it was designed for exactly this type of use case. https://codereview.chromium.org/2858563004/diff/860001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:46: public void show(Activity activity, String url) { I don't recall my generics well enough, but would this work instead of the subsequent cast public <T extends Activity & DialogListener> void show(T activity, String url) https://codereview.chromium.org/2858563004/diff/860001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:49: } catch (ClassCastException e) { instead of catching the exception, I would do: if (!(activity instanceof DialogListener)) { throw new IllegalArgumentException("..."); } I think IllegalArgumentException makes more sense here because you have argument requirements that were not met. https://codereview.chromium.org/2858563004/diff/860001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:67: activity, android.R.style.Theme_DeviceDefault_Light_Dialog)); Why do you need the ContextThemeWrapper? Does the constructor that takes the theme not work? https://developer.android.com/reference/android/app/AlertDialog.Builder.html#..., int) Is the goal of this dialog to not look like it was generated from Chrome? Normally, we would use one of our existing themes that don't try to mimic the system dialog.
Patchset #19 (id:870001) has been deleted
Patchset #19 (id:890001) has been deleted
PTAL, thanks! https://codereview.chromium.org/2858563004/diff/860001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java (right): https://codereview.chromium.org/2858563004/diff/860001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:28: public class ChooseHostBrowserDialog { On 2017/06/01 15:18:44, Ted C wrote: > Sorry for being slow to review, but any thoughts on using the ItemChooserDialog > already in chromium? > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > Ideally, it was designed for exactly this type of use case. Thanks for pointing out this class, I didn't know before. However, WebAPK can't depends on code of Chrome, we'd better to have a simpler version of such a dialog. https://codereview.chromium.org/2858563004/diff/860001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:46: public void show(Activity activity, String url) { On 2017/06/01 15:18:44, Ted C wrote: > I don't recall my generics well enough, but would this work instead of the > subsequent cast > > public <T extends Activity & DialogListener> void show(T activity, String url) SG, thanks! https://codereview.chromium.org/2858563004/diff/860001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:49: } catch (ClassCastException e) { On 2017/06/01 15:18:43, Ted C wrote: > instead of catching the exception, I would do: > > if (!(activity instanceof DialogListener)) { > throw new IllegalArgumentException("..."); > } > > I think IllegalArgumentException makes more sense here because you have argument > requirements that were not met. Done. https://codereview.chromium.org/2858563004/diff/860001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:67: activity, android.R.style.Theme_DeviceDefault_Light_Dialog)); On 2017/06/01 15:18:44, Ted C wrote: > Why do you need the ContextThemeWrapper? Does the constructor that takes the > theme not work? > https://developer.android.com/reference/android/app/AlertDialog.Builder.html#..., > int) > > Is the goal of this dialog to not look like it was generated from Chrome? > Normally, we would use one of our existing themes that don't try to mimic the > system dialog. No, on KK (probably pre-L), the UX looks like two dialog overlapped together if I don't use ContextThemeWrapper: (https://drive.google.com/a/google.com/file/d/0B7zEF5GgyYmpbWcyUFRrOUlFWWM/vie...). With this change, everything looks normal: (https://drive.google.com/open?id=0B7zEF5GgyYmpOGdmekMyVElsbDA) Agree, I don't want to rely on the system dialog, since the UX looks different depends on Andriod version. However, adding more styles will definitely increase the APK size, so we need some balance. I think it should be fine for the initial look of the dialog, and we can add more styles in the future:) https://codereview.chromium.org/2858563004/diff/860001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2858563004/diff/860001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:55: /** The URL to launch the WebAPK. */ On 2017/06/01 14:12:43, Yaron wrote: > Used in the case of deep-links (intents from other apps) that fall into the > WebApk scope. Done. https://codereview.chromium.org/2858563004/diff/860001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:92: activityManager.clearApplicationUserData(); On 2017/06/01 14:12:43, Yaron wrote: > Err, I think this kills the app so you need some verification that this doesn't > break eerything. Since this call will stop the app, change it to delete the shared prefs and internal storage. https://codereview.chromium.org/2858563004/diff/860001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:94: WebApkUtils.deleteSharedPref(this); On 2017/06/01 14:12:43, Yaron wrote: > Err I should've said that this isn't quite equivalent to clearApp.. so we may > want to delete some other things that aren't shared prefs (e.g. runtime dex > that's extracted and really anything in the private directory) Done.
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #20 (id:930001) has been deleted
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Patchset #21 (id:970001) has been deleted
https://codereview.chromium.org/2858563004/diff/990001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java (right): https://codereview.chromium.org/2858563004/diff/990001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:49: activity.toString() + " must implement DialogListener"); Drive by: - Isn't it impossible for this if() statement to be hit now? - Another option would be to change the method signature to: public void show(Activity activity, DialogListener listener, String url) {}
PTAL, thanks! https://codereview.chromium.org/2858563004/diff/990001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java (right): https://codereview.chromium.org/2858563004/diff/990001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:49: activity.toString() + " must implement DialogListener"); On 2017/06/01 20:59:34, pkotwicz wrote: > Drive by: > - Isn't it impossible for this if() statement to be hit now? > - Another option would be to change the method signature to: > public void show(Activity activity, DialogListener listener, String url) {} Hmm, I don't prefer to have both parameters. Remove the template, and still cast activity to DialogListener.
Friendly Ping!
Xi, I am assuming that you are pinging Ted?
@Peter: I am pinging everyone, and I thought you might have more comments too:)
No more comments on my part
No more comments on my part
Thanks Peter! Yaron & Ted, PTAL, thanks!
As discussed offline with Yaron, it's better to list the browsers that don't support WebAPKs after the ones that support. So I add a compactor to sort the list. Since this change is small but needs a new shell APK version push to server, so I would prefer to do it in this CL. PTAL, thanks!
A few drive by comments https://codereview.chromium.org/2858563004/diff/1050001/chrome/android/webapk... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2858563004/diff/1050001/chrome/android/webapk... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:146: private void deleteDir(File dir) { Maybe rename this function to deletePath() since it deletes both files and directories https://codereview.chromium.org/2858563004/diff/1050001/chrome/android/webapk... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:159: File file = new File(dir, name); Can you call deleteDir() here (since deleteDir() handles deleting non directories as well?) https://codereview.chromium.org/2858563004/diff/1050001/chrome/android/webapk... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:166: } For simplicity you could do the following: if (dir.isDirectory()) { String[] children = dir.list(); ... } if (!dir.delete()) { ... }
PTAL, thanks! https://codereview.chromium.org/2858563004/diff/1050001/chrome/android/webapk... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2858563004/diff/1050001/chrome/android/webapk... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:146: private void deleteDir(File dir) { On 2017/06/05 17:27:55, pkotwicz wrote: > Maybe rename this function to deletePath() since it deletes both files and > directories Done. https://codereview.chromium.org/2858563004/diff/1050001/chrome/android/webapk... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:159: File file = new File(dir, name); On 2017/06/05 17:27:55, pkotwicz wrote: > Can you call deleteDir() here (since deleteDir() handles deleting non > directories as well?) Done. https://codereview.chromium.org/2858563004/diff/1050001/chrome/android/webapk... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:166: } On 2017/06/05 17:27:55, pkotwicz wrote: > For simplicity you could do the following: > > if (dir.isDirectory()) { > String[] children = dir.list(); > ... > } > if (!dir.delete()) { > ... > } Thanks! The code looks much nicer!
Still LGTM https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:150: String[] children = file.list(); Nit: You can further simplify things if you do: File[] children = file.listFiles();
Thanks! https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:150: String[] children = file.list(); On 2017/06/05 17:56:03, pkotwicz wrote: > Nit: You can further simplify things if you do: > > File[] children = file.listFiles(); Done.
lgtm...nits and small suggestions https://codereview.chromium.org/2858563004/diff/860001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java (right): https://codereview.chromium.org/2858563004/diff/860001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:46: public void show(Activity activity, String url) { On 2017/06/01 17:14:59, Xi Han wrote: > On 2017/06/01 15:18:44, Ted C wrote: > > I don't recall my generics well enough, but would this work instead of the > > subsequent cast > > > > public <T extends Activity & DialogListener> void show(T activity, String url) > > SG, thanks! From pkotwicz's comment here (https://codereview.chromium.org/2858563004/#msg108), I thought they were suggesting to either keep my suggestion or pass in both. Any reason you don't like the template param? I think we were both providing suggestions where the check below (for instanceof) was not needed. With the template, you should be able to remove it entirely and just rely on the compilation failure to prevent it from being passed in in the first place. https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... File chrome/android/webapk/shell_apk/res/color/item_selection_text_color.xml (right): https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... chrome/android/webapk/shell_apk/res/color/item_selection_text_color.xml:1: <?xml version="1.0" encoding="utf-8"?> did you try: https://developer.android.com/reference/android/R.color.html#primary_text_dark or https://developer.android.com/reference/android/R.color.html#primary_text_light They might be indistinguishable if you want to eke a tiny bit of improvement. https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... File chrome/android/webapk/shell_apk/res/layout/choose_host_browser_dialog.xml (right): https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... chrome/android/webapk/shell_apk/res/layout/choose_host_browser_dialog.xml:9: android:layout_width="fill_parent" s/fill/match https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... chrome/android/webapk/shell_apk/res/layout/choose_host_browser_dialog.xml:14: android:textColor="#000000" @android:color/black https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... chrome/android/webapk/shell_apk/res/layout/choose_host_browser_dialog.xml:16: android:paddingStart="20dip" dp is more common for us than dip, so i'd drop the i here and the next line https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... File chrome/android/webapk/shell_apk/res/layout/choose_host_browser_dialog_list.xml (right): https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... chrome/android/webapk/shell_apk/res/layout/choose_host_browser_dialog_list.xml:6: <LinearLayout FYI, you could likely use a framelayout here if you wanted to be a tiny bit more performant, but it probably makes this slightly harder to read so not super important (for horizontal layouts where you know the exact width of the view, you could do layout_marginStart="68dp" on the browser_name field because you know exactly how the image is...not needed here again, just fyi). https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... chrome/android/webapk/shell_apk/res/layout/choose_host_browser_dialog_list.xml:22: android:paddingStart="20dip" s/dip/dp https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java (right): https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:27: remove blank line https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:66: AlertDialog.Builder builder = new AlertDialog.Builder(new ContextThemeWrapper( let's add a comment about the context theme wrapper here being needed for pre-L. From the internets, it looks like we could either use this or set android:windowBackground to transparent / @null in our own custom theme. Neither one is clearly better than the other, so I think a comment is sufficient.
lgtm
When I changed the text color, I noticed that the string "Unsupported by ..." wasn't added for browsers that don't support WebAPKs. So I added this missing part back. Sorry for the additional pass of review. +agreive@: Could you please take a look: - build/android/lint/suppressions.xml Thanks all! https://codereview.chromium.org/2858563004/diff/860001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java (right): https://codereview.chromium.org/2858563004/diff/860001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:46: public void show(Activity activity, String url) { On 2017/06/05 18:24:48, Ted C wrote: > On 2017/06/01 17:14:59, Xi Han wrote: > > On 2017/06/01 15:18:44, Ted C wrote: > > > I don't recall my generics well enough, but would this work instead of the > > > subsequent cast > > > > > > public <T extends Activity & DialogListener> void show(T activity, String > url) > > > > SG, thanks! > > From pkotwicz's comment here > (https://codereview.chromium.org/2858563004/#msg108), I thought they were > suggesting to either keep my suggestion or pass in both. Any reason you don't > like the template param? Sorry for the misunderstanding. > I think we were both providing suggestions where the check below (for > instanceof) was not needed. With the template, you should be able to remove it > entirely and just rely on the compilation failure to prevent it from being > passed in in the first place. Yes, I adopted Peter's suggestion in the follow up CL and update this part too (https://codereview.chromium.org/2912393005/diff/140001/chrome/android/webapk/...). So both "context" and "DialogListener" are passed in. One advantage is the MainActivity doesn't implement the DialogListener interface. https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... File chrome/android/webapk/shell_apk/res/color/item_selection_text_color.xml (right): https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... chrome/android/webapk/shell_apk/res/color/item_selection_text_color.xml:1: <?xml version="1.0" encoding="utf-8"?> On 2017/06/05 18:24:48, Ted C wrote: > did you try: > https://developer.android.com/reference/android/R.color.html#primary_text_dark > or > https://developer.android.com/reference/android/R.color.html#primary_text_light > > They might be indistinguishable if you want to eke a tiny bit of improvement. The primary_text_dark looks too light for the enabled text, and black looks closer to the color in the UI review(https://docs.google.com/presentation/d/1ZC7Fc9Y7-vsGqvbEyiZmiJoWtsONkhPBUb47F0s4u4s/edit#slide=id.g1ca70aafc8_0_16). However, it seems using @android:color/primary_text_light can't change the color of the disabled text which is still black. So I remove this xml, and just set the text color programmatically. https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... File chrome/android/webapk/shell_apk/res/layout/choose_host_browser_dialog.xml (right): https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... chrome/android/webapk/shell_apk/res/layout/choose_host_browser_dialog.xml:9: android:layout_width="fill_parent" On 2017/06/05 18:24:48, Ted C wrote: > s/fill/match Thanks! https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... chrome/android/webapk/shell_apk/res/layout/choose_host_browser_dialog.xml:14: android:textColor="#000000" On 2017/06/05 18:24:48, Ted C wrote: > @android:color/black Done. https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... chrome/android/webapk/shell_apk/res/layout/choose_host_browser_dialog.xml:16: android:paddingStart="20dip" On 2017/06/05 18:24:48, Ted C wrote: > dp is more common for us than dip, so i'd drop the i here and the next line Done. https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... File chrome/android/webapk/shell_apk/res/layout/choose_host_browser_dialog_list.xml (right): https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... chrome/android/webapk/shell_apk/res/layout/choose_host_browser_dialog_list.xml:6: <LinearLayout On 2017/06/05 18:24:48, Ted C wrote: > FYI, you could likely use a framelayout here if you wanted to be a tiny bit more > performant, but it probably makes this slightly harder to read so not super > important > > (for horizontal layouts where you know the exact width of the view, you could do > layout_marginStart="68dp" on the browser_name field because you know exactly how > the image is...not needed here again, just fyi). Agree, I feel having two views is more easy to read and change. Thanks for the details anyway. https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... chrome/android/webapk/shell_apk/res/layout/choose_host_browser_dialog_list.xml:22: android:paddingStart="20dip" On 2017/06/05 18:24:48, Ted C wrote: > s/dip/dp Done. https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java (right): https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:27: On 2017/06/05 18:24:49, Ted C wrote: > remove blank line Done. https://codereview.chromium.org/2858563004/diff/1070001/chrome/android/webapk... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:66: AlertDialog.Builder builder = new AlertDialog.Builder(new ContextThemeWrapper( On 2017/06/05 18:24:48, Ted C wrote: > let's add a comment about the context theme wrapper here being needed for pre-L. > > From the internets, it looks like we could either use this or set > android:windowBackground to transparent / @null in our own custom theme. > Neither one is clearly better than the other, so I think a comment is > sufficient. Done.
hanxi@chromium.org changed reviewers: + agrieve@chromium.org
+agreive@: Could you please take a look: - build/android/lint/suppressions.xml Thanks!
https://codereview.chromium.org/2858563004/diff/1110001/build/android/lint/su... File build/android/lint/suppressions.xml (right): https://codereview.chromium.org/2858563004/diff/1110001/build/android/lint/su... build/android/lint/suppressions.xml:436: <ignore regexp="chrome/android/webapk/shell_apk/res/layout/choose_host_browser_dialog_list.xml"/> Would using tools:ignore= work for suppressing this? http://tools.android.com/tips/lint/suppressing-lint-warnings Generally better to put the suppressions inline
PTAL, thanks! https://codereview.chromium.org/2858563004/diff/1110001/build/android/lint/su... File build/android/lint/suppressions.xml (right): https://codereview.chromium.org/2858563004/diff/1110001/build/android/lint/su... build/android/lint/suppressions.xml:436: <ignore regexp="chrome/android/webapk/shell_apk/res/layout/choose_host_browser_dialog_list.xml"/> On 2017/06/05 20:34:20, agrieve wrote: > Would using tools:ignore= work for suppressing this? > http://tools.android.com/tips/lint/suppressing-lint-warnings > > Generally better to put the suppressions inline Thanks! Didn't know this before.
On 2017/06/05 20:43:50, Xi Han wrote: > PTAL, thanks! > > https://codereview.chromium.org/2858563004/diff/1110001/build/android/lint/su... > File build/android/lint/suppressions.xml (right): > > https://codereview.chromium.org/2858563004/diff/1110001/build/android/lint/su... > build/android/lint/suppressions.xml:436: <ignore > regexp="chrome/android/webapk/shell_apk/res/layout/choose_host_browser_dialog_list.xml"/> > On 2017/06/05 20:34:20, agrieve wrote: > > Would using tools:ignore= work for suppressing this? > > http://tools.android.com/tips/lint/suppressing-lint-warnings > > > > Generally better to put the suppressions inline > > Thanks! Didn't know this before. lgtm
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2858563004/diff/1130001/chrome/android/webapk... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java (right): https://codereview.chromium.org/2858563004/diff/1130001/chrome/android/webapk... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:120: if (item.supportWebApks()) { supportsWebApks() https://codereview.chromium.org/2858563004/diff/1130001/chrome/android/webapk... File chrome/android/webapk/strings/android_webapk_strings.grd (right): https://codereview.chromium.org/2858563004/diff/1130001/chrome/android/webapk... chrome/android/webapk/strings/android_webapk_strings.grd:106: <message name="IDS_HOST_BROWSER_ITEM_UNSUPPORTING_WEBAPK" desc="Text for the host browser item that doesn't support WebAPKs on the choose host browser dialog. "> IDS_HOST_BROWSER_ITEM_NOT_SUPPORTING_WEBAPKS
https://codereview.chromium.org/2858563004/diff/1130001/chrome/android/webapk... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java (right): https://codereview.chromium.org/2858563004/diff/1130001/chrome/android/webapk... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/ChooseHostBrowserDialog.java:120: if (item.supportWebApks()) { On 2017/06/06 17:16:22, pkotwicz wrote: > supportsWebApks() Done. https://codereview.chromium.org/2858563004/diff/1130001/chrome/android/webapk... File chrome/android/webapk/strings/android_webapk_strings.grd (right): https://codereview.chromium.org/2858563004/diff/1130001/chrome/android/webapk... chrome/android/webapk/strings/android_webapk_strings.grd:106: <message name="IDS_HOST_BROWSER_ITEM_UNSUPPORTING_WEBAPK" desc="Text for the host browser item that doesn't support WebAPKs on the choose host browser dialog. "> On 2017/06/06 17:16:22, pkotwicz wrote: > IDS_HOST_BROWSER_ITEM_NOT_SUPPORTING_WEBAPKS Done.
The CQ bit was checked by hanxi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org, tedchoc@chromium.org, yfriedman@chromium.org, agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2858563004/#ps1150001 (title: "Nits.")
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": 1150001, "attempt_start_ts": 1496769835390580,
"parent_rev": "b25f007b5254eaa159ed79aab8258211a622fb5d", "commit_rev":
"25f15cec7b331804a3b884280d5701de16ef899a"}
Message was sent while issue was closed.
Description was changed from ========== Add support for webapk without runtimeHost An Unbound webapk will not specify a "runtimeHost" as part of the AndroidManifest. During the first launch of a WebAPK, we will use the default browser as the host browser if it supports WebAPKs. If the default browser doesn't support WebAPKs, a dialog will pop up to ask user to choose a browser to launch the WebAPK. The UX of the dialog is: https://docs.google.com/presentation/d/1ZC7Fc9Y7-vsGqvbEyiZmiJoWtsONkhPBUb47F... For existing WebAPKs which has specified "runtimeHost": 1) Launch the WebAPK by the specified "runtimeHost" if it is installed; 2) Show a dialog to ask for users to choose a host browser. In any case, if user chooses a browser that isn't installed, we will redirect to Play Store to install it. The screenshots refer to: https://drive.google.com/open?id=0B7zEF5GgyYmpVkowOUY4ZGFKTmM BUG=714737 ========== to ========== Add support for webapk without runtimeHost An Unbound webapk will not specify a "runtimeHost" as part of the AndroidManifest. During the first launch of a WebAPK, we will use the default browser as the host browser if it supports WebAPKs. If the default browser doesn't support WebAPKs, a dialog will pop up to ask user to choose a browser to launch the WebAPK. The UX of the dialog is: https://docs.google.com/presentation/d/1ZC7Fc9Y7-vsGqvbEyiZmiJoWtsONkhPBUb47F... For existing WebAPKs which has specified "runtimeHost": 1) Launch the WebAPK by the specified "runtimeHost" if it is installed; 2) Show a dialog to ask for users to choose a host browser. In any case, if user chooses a browser that isn't installed, we will redirect to Play Store to install it. The screenshots refer to: https://drive.google.com/open?id=0B7zEF5GgyYmpVkowOUY4ZGFKTmM BUG=714737 Review-Url: https://codereview.chromium.org/2858563004 Cr-Commit-Position: refs/heads/master@{#477360} Committed: https://chromium.googlesource.com/chromium/src/+/25f15cec7b331804a3b884280d57... ==========
Message was sent while issue was closed.
Committed patchset #29 (id:1150001) as https://chromium.googlesource.com/chromium/src/+/25f15cec7b331804a3b884280d57... |
