|
|
DescriptionWebApk needs to be restarted to notice that host browser was uninstalled.
We cache a static variable for the WebAPK's runtime host, and it won't updated
when the runtime host is uninstalled. Therefore, the WebAPK won't launch until
it is forced to stop and relaunch. In this CL, we add an additional check of
whether the runtime host is installed. If not, continue the regular browser
selection process.
BUG=736036
Review-Url: https://codereview.chromium.org/2954933002
Cr-Commit-Position: refs/heads/master@{#483064}
Committed: https://chromium.googlesource.com/chromium/src/+/5b7fc7601292363b4b02869601b4b2c3010107c8
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebase. #
Total comments: 2
Patch Set 3 : Move the logic. #
Total comments: 4
Patch Set 4 : Nits. #
Messages
Total messages: 33 (20 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== WebApk needs to be restarted to notice that host browser was uninstalled. We cach a static variable for the WebAPK's runtime host, and it won't updated when the runtime host is uninstalled. Therefore, the WebAPK won't launch until it is forced to stop and relaunch. In this CL, we add an additional check of whether the runtime host is installed. If not, continue the regular browser selection process. BUG=736036 ========== to ========== WebApk needs to be restarted to notice that host browser was uninstalled. We cache a static variable for the WebAPK's runtime host, and it won't updated when the runtime host is uninstalled. Therefore, the WebAPK won't launch until it is forced to stop and relaunch. In this CL, we add an additional check of whether the runtime host is installed. If not, continue the regular browser selection process. BUG=736036 ==========
hanxi@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi Peter, could you please take a look? Thanks!
hartmanng@chromium.org changed reviewers: + hartmanng@chromium.org
https://codereview.chromium.org/2954933002/diff/80001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/shell_apk_version.gni (right): https://codereview.chromium.org/2954933002/diff/80001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/shell_apk_version.gni:9: template_shell_apk_version = 9 drive-by nit: https://codereview.chromium.org/2948653002/ already incremented this to 9, we'll need to change it to 10 now.
PTAL, thanks! https://codereview.chromium.org/2954933002/diff/80001/chrome/android/webapk/s... File chrome/android/webapk/shell_apk/shell_apk_version.gni (right): https://codereview.chromium.org/2954933002/diff/80001/chrome/android/webapk/s... chrome/android/webapk/shell_apk/shell_apk_version.gni:9: template_shell_apk_version = 9 On 2017/06/26 17:35:22, hartmanng wrote: > drive-by nit: https://codereview.chromium.org/2948653002/ already incremented > this to 9, we'll need to change it to 10 now. Thanks Glenn, I will rebase it soon.
https://codereview.chromium.org/2954933002/diff/100001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2954933002/diff/100001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:102: boolean isInstalled = isInstalled(getPackageManager(), runtimeHost); Can you fix this bug by changing the logic of WebApkUtils#getHostBrowserPackageName() Perhaps by querying WebApkUtils#getInstalledBrowsers() in WebApkUtils#getHostBrowserPackageName() and passing the result of WebApkUtils#getInstalledBrowsers() to WebApkUtils#getHostBrowserPackageNameInternal() Also, it should be possible to write a JUnit test for the scenario that you are testing.
Patchset #3 (id:120001) has been deleted
Hi Peter, PTAL, thanks! https://codereview.chromium.org/2954933002/diff/100001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java (right): https://codereview.chromium.org/2954933002/diff/100001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:102: boolean isInstalled = isInstalled(getPackageManager(), runtimeHost); On 2017/06/27 01:14:27, pkotwicz wrote: > Can you fix this bug by changing the logic of > WebApkUtils#getHostBrowserPackageName() > > Perhaps by querying WebApkUtils#getInstalledBrowsers() in > WebApkUtils#getHostBrowserPackageName() and passing the result of > WebApkUtils#getInstalledBrowsers() to > WebApkUtils#getHostBrowserPackageNameInternal() > > Also, it should be possible to write a JUnit test for the scenario that you are > testing. Yes, I can move the logic to |getHostBrowserPackageName|. Query the package manager to check whether the cached |sHostPackage| is installed would help. Also add a JUnit test.
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.
LGTM https://codereview.chromium.org/2954933002/diff/140001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java (right): https://codereview.chromium.org/2954933002/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:235: private void uninstallBrowser(String packageName) { Can you move this function underneath uninstallBrowser() Can you please add a comment that this function only works for uninstalling the non default browser https://codereview.chromium.org/2954933002/diff/140001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2954933002/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:220: private static boolean isInstalled(PackageManager packageManager, String packageName) { Can you move this function next to the the other install-checking functions?
LGTM
Patchset #4 (id:160001) has been deleted
hanxi@chromium.org changed reviewers: + yfriedman@chromium.org
Hi Yaron, could you please take a look? Thanks! https://codereview.chromium.org/2954933002/diff/140001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java (right): https://codereview.chromium.org/2954933002/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/WebApkUtilsTest.java:235: private void uninstallBrowser(String packageName) { On 2017/06/28 15:37:22, pkotwicz wrote: > Can you move this function underneath uninstallBrowser() > > Can you please add a comment that this function only works for uninstalling the > non default browser Done. https://codereview.chromium.org/2954933002/diff/140001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2954933002/diff/140001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:220: private static boolean isInstalled(PackageManager packageManager, String packageName) { On 2017/06/28 15:37:22, pkotwicz wrote: > Can you move this function next to the the other install-checking functions? Done.
lgtm
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 Link to the patchset: https://codereview.chromium.org/2954933002/#ps170001 (title: "Nits.")
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 hanxi@chromium.org
The CQ bit was checked by hanxi@chromium.org
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": 170001, "attempt_start_ts": 1498668374076980, "parent_rev": "aa5ea5a7134eef78d614541e209285e78705cd71", "commit_rev": "5b7fc7601292363b4b02869601b4b2c3010107c8"}
Message was sent while issue was closed.
Description was changed from ========== WebApk needs to be restarted to notice that host browser was uninstalled. We cache a static variable for the WebAPK's runtime host, and it won't updated when the runtime host is uninstalled. Therefore, the WebAPK won't launch until it is forced to stop and relaunch. In this CL, we add an additional check of whether the runtime host is installed. If not, continue the regular browser selection process. BUG=736036 ========== to ========== WebApk needs to be restarted to notice that host browser was uninstalled. We cache a static variable for the WebAPK's runtime host, and it won't updated when the runtime host is uninstalled. Therefore, the WebAPK won't launch until it is forced to stop and relaunch. In this CL, we add an additional check of whether the runtime host is installed. If not, continue the regular browser selection process. BUG=736036 Review-Url: https://codereview.chromium.org/2954933002 Cr-Commit-Position: refs/heads/master@{#483064} Committed: https://chromium.googlesource.com/chromium/src/+/5b7fc7601292363b4b02869601b4... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:170001) as https://chromium.googlesource.com/chromium/src/+/5b7fc7601292363b4b02869601b4... |