Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2071213005:
Use metadata when launching WebAPKs. (Closed)
DescriptionUse metadata when launching WebAPKs.
WebAPKs use WebappDataStorage to store the metadata too.
Some changes in the web app manifest, like themeColor,
orientation etc. don't request re-minting a WebAPK,
but will update the metadata in the SharedPreference
(WebappDataStorage). The metadata will be retrieved
and used for launching a WebAPK.
Registering a WebAPK will be in a following up CL.
BUG=624834
Committed: https://crrev.com/a6fe748fda4067575facddcea0e99ac41139670a
Cr-Commit-Position: refs/heads/master@{#403677}
Patch Set 1 : Add WebAPK's metadata in WebappDataStorage. #
Total comments: 23
Patch Set 2 : pkotwicz@'s comments. #
Total comments: 17
Patch Set 3 : Register WebAPK when storage is null. #
Total comments: 4
Patch Set 4 : Always init webapp screen with background color before fetching the SharedPreference. #Patch Set 5 : Always use backgroundColor from launching Intent for WebAPK. #
Total comments: 6
Patch Set 6 : Nits. #
Total comments: 23
Patch Set 7 : Add more tests and nits. #
Total comments: 6
Patch Set 8 : Nits. #
Total comments: 34
Patch Set 9 : dominickn@'s comments and rebase. #
Total comments: 4
Patch Set 10 : Nits. #
Total comments: 3
Patch Set 11 : Nits. #Messages
Total messages: 60 (32 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Add WebAPK's metadata in WebappDataStorage. BUG=609122 ========== to ========== Add WebAPK's metadata in WebappDataStorage. WebAPKs use WebappDataStorage to store the metadata too. Some changes in the web app manifest, like backgroundColor, themeColor, oritation etc. don't request ree-minting a WebAPK, but will update the metadata in the SharedPreference (WebappDataStorage). The metadata will be retrieved and used for launching a WebAPK. Besides, some WebAPK updates related timestamp(s) are stored in the SharedPreference too. BUG=609122 ==========
Description was changed from ========== Add WebAPK's metadata in WebappDataStorage. WebAPKs use WebappDataStorage to store the metadata too. Some changes in the web app manifest, like backgroundColor, themeColor, oritation etc. don't request ree-minting a WebAPK, but will update the metadata in the SharedPreference (WebappDataStorage). The metadata will be retrieved and used for launching a WebAPK. Besides, some WebAPK updates related timestamp(s) are stored in the SharedPreference too. BUG=609122 ========== to ========== Add WebAPK's metadata in WebappDataStorage. WebAPKs use WebappDataStorage to store the metadata too. Some changes in the web app manifest, like backgroundColor, themeColor, orientation etc. don't request ree-minting a WebAPK, but will update the metadata in the SharedPreference (WebappDataStorage). The metadata will be retrieved and used for launching a WebAPK. Besides, some WebAPK updates related timestamp(s) are stored in the SharedPreference too. BUG=609122 ==========
Description was changed from ========== Add WebAPK's metadata in WebappDataStorage. WebAPKs use WebappDataStorage to store the metadata too. Some changes in the web app manifest, like backgroundColor, themeColor, orientation etc. don't request ree-minting a WebAPK, but will update the metadata in the SharedPreference (WebappDataStorage). The metadata will be retrieved and used for launching a WebAPK. Besides, some WebAPK updates related timestamp(s) are stored in the SharedPreference too. BUG=609122 ========== to ========== Add WebAPK's metadata in WebappDataStorage. WebAPKs use WebappDataStorage to store the metadata too. Some changes in the web app manifest, like backgroundColor, themeColor, orientation etc. don't request ree-minting a WebAPK, but will update the metadata in the SharedPreference (WebappDataStorage). The metadata will be retrieved and used for launching a WebAPK. Besides, some WebAPK updates related timestamp(s) are stored in the SharedPreference too. BUG=609122 ==========
Description was changed from ========== Add WebAPK's metadata in WebappDataStorage. WebAPKs use WebappDataStorage to store the metadata too. Some changes in the web app manifest, like backgroundColor, themeColor, orientation etc. don't request ree-minting a WebAPK, but will update the metadata in the SharedPreference (WebappDataStorage). The metadata will be retrieved and used for launching a WebAPK. Besides, some WebAPK updates related timestamp(s) are stored in the SharedPreference too. BUG=609122 ========== to ========== Add WebAPK's metadata in WebappDataStorage. WebAPKs use WebappDataStorage to store the metadata too. Some changes in the web app manifest, like backgroundColor, themeColor, orientation etc. don't request ree-minting a WebAPK, but will update the metadata in the SharedPreference (WebappDataStorage). The metadata will be retrieved and used for launching a WebAPK. Besides, some update related timestamp(s) are stored in the SharedPreference too. BUG=609122 ==========
Description was changed from ========== Add WebAPK's metadata in WebappDataStorage. WebAPKs use WebappDataStorage to store the metadata too. Some changes in the web app manifest, like backgroundColor, themeColor, orientation etc. don't request ree-minting a WebAPK, but will update the metadata in the SharedPreference (WebappDataStorage). The metadata will be retrieved and used for launching a WebAPK. Besides, some update related timestamp(s) are stored in the SharedPreference too. BUG=609122 ========== to ========== Add WebAPK's metadata in WebappDataStorage. WebAPKs use WebappDataStorage to store the metadata too. Some changes in the web app manifest, like backgroundColor, themeColor, orientation etc. don't request ree-minting a WebAPK, but will update the metadata in the SharedPreference (WebappDataStorage). The metadata will be retrieved and used for launching a WebAPK. Besides, some update related timestamp(s) are stored in the SharedPreference too. Registering a WebAPK will be in a following up CL. BUG=609122 ==========
hanxi@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi Peter, could you please take a look? Thanks!
Some initial review comments. Thank you for undertaking this task ⛱ https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:52: protected void initializeWebappData() { - Can this function be in WebappActivity instead of WebapkActivity? I think that updating the splash screen as the Web Manifest changes is useful for generic Web apps not just WebAPKs If it is not, then we should only update the splashscreen for WebAPKs if we get a new WebAPK from the server. We should get approval from dfalcantara@ before merging this code into WebappActivity. I have been bad at guessing "what the real OWNERS think" lately - I am not a fan of all of the WebappInfo setters. Can we rename this function to resolveWebappInfo() and have it call onWebappInfoResolved(WebappInfo info, Bitmap bitmap) when it has retrieved the data from WebappDataStorage https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:57: if (storage == null) { Currently, WebappRegistry#registerWebapp() is not called for WebAPKs in ShorcutHelper. Don't we currently always go through the early return? Can you post the CL which calls WebappRegistry#registerWebapp() as prerequisite to this one. My preference is to land CLs in an order so that each CL produces a visible change. (and it is easy to test the changes caused by each CL) https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:78: Shouldn't we also update WebappInfo#name()? I am unsure whether we should update WebappInfo#shortName(). That's the one which shows up in the app list and in recents https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:267: : WebappUma.SPLASHSCREEN_COLOR_STATUS_DEFAULT); This seems out of place. We should: 1) fetch the theme color from data storage 2) Log UMA with the theme color that was fetched from data storage https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:315: || (displayIcon == mWebappInfo.icon() && mWebappInfo.isIconGenerated())) { I am not sure whether we should get "icon" and "is icon generated" from WebappDataStorage or always get the properties from the WebAPK manifest https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:473: private void updateTaskDescription() { Is this guaranteed to be called after getting the information from WebappDataStorage? https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:401: void updateLastRequestForNewShellApkVersionTime() { Nit: Can you do this change as part of a separate CL? My preference would be to wait on adding this function till we are ready to use it. I am not a fan of adding unused code to the code base.
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:140001) has been deleted
Hi Peter, I am thinking the changes since our last discussion, but I still believe some of changes are better to leave in WebApkActivity. PTAL, thanks! https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:52: protected void initializeWebappData() { On 2016/06/20 18:26:14, pkotwicz wrote: > - Can this function be in WebappActivity instead of WebapkActivity? I think that > updating the splash screen as the Web Manifest changes is useful for generic Web > apps not just WebAPKs If it is not, then we should only update the splashscreen > for WebAPKs if we get a new WebAPK from the server. We should get approval from > dfalcantara@ before merging this code into WebappActivity. I have been bad at > guessing "what the real OWNERS think" lately I am not sure whether this suits Webapps, because they also update the meta data from the new launching Intent, while WebAPK prefer to use the metadata. Webapp could benefit from this code, but after the efforts to keep the meta data updated. So now isn't the right time to change Webapp's behavior, we'd better keep the changes within WebApkActivity. > - I am not a fan of all of the WebappInfo setters. Can we rename this function > to resolveWebappInfo() and have it call onWebappInfoResolved(WebappInfo info, > Bitmap bitmap) when it has retrieved the data from WebappDataStorage Add WebappDataStorage#createWebappInfo() to create a webappInfo from the storage, but we still use the URL of the current mWebappInfo since that is the latest URL to navigate. https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:57: if (storage == null) { On 2016/06/20 18:26:14, pkotwicz wrote: > Currently, WebappRegistry#registerWebapp() is not called for WebAPKs in > ShorcutHelper. Don't we currently always go through the early return? Yes, we always go through the early return for now. > Can you post the CL which calls WebappRegistry#registerWebapp() as prerequisite > to this one. My preference is to land CLs in an order so that each CL produces a > visible change. (and it is easy to test the changes caused by each CL) I haven't figured out the best place to call WebappRegistry#registerWebapp(), but it should be somewhere when we find the storage is null. https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:78: On 2016/06/20 18:26:14, pkotwicz wrote: > Shouldn't we also update WebappInfo#name()? > I am unsure whether we should update WebappInfo#shortName(). That's the one > which shows up in the app list and in recents Good point. Creating a new WebappInfo will have the updated values. https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:267: : WebappUma.SPLASHSCREEN_COLOR_STATUS_DEFAULT); On 2016/06/20 18:26:14, pkotwicz wrote: > This seems out of place. We should: > 1) fetch the theme color from data storage > 2) Log UMA with the theme color that was fetched from data storage Good catch. Move the setThemeColor() before setBackgroundColor(). https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:315: || (displayIcon == mWebappInfo.icon() && mWebappInfo.isIconGenerated())) { On 2016/06/20 18:26:14, pkotwicz wrote: > I am not sure whether we should get "icon" and "is icon generated" from > WebappDataStorage or always get the properties from the WebAPK manifest I think "icon" should come from WebAPK manifest, since icon's change will request re-mint (if the change doesn't happen frequently). The "is icon generated" isn't part of the manifest spec at all, so we should use the metadata in the WebappDataStorage. However, this is the tricky part I neglected, and we don't know what the value should be at all. https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:473: private void updateTaskDescription() { On 2016/06/20 18:26:14, pkotwicz wrote: > Is this guaranteed to be called after getting the information from > WebappDataStorage? I think yes. The onCreate() calls to get the information from the WebappDataStorage, and onResume() calls updateTaskDescription(). https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:401: void updateLastRequestForNewShellApkVersionTime() { On 2016/06/20 18:26:15, pkotwicz wrote: > Nit: Can you do this change as part of a separate CL? My preference would be to > wait on adding this function till we are ready to use it. I am not a fan of > adding unused code to the code base. Done.
https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:52: protected void initializeWebappData() { I don't mind waiting till the "fetch updated Web Manifest data" is implemented for WebAPKs before we port the feature to regular Web apps too. https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:57: if (storage == null) { I think that there will be multiple places which will call WebappRegistry#registerWebapp(). One of those places should be ShortcutHelper#addShortcut(). Otherwise there will be no splashscreen image the first time that the WebAPK is launched. I am OK if we guess what the WebAPK's package name is for now (ShortcutHelper#guessWebApkPackageName()) I think that we should add a place which calls WebappRegistry#registerWebapp() *somewhere* even if that place is suboptimal either in this CL or in a CL which preceeds this one https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:473: private void updateTaskDescription() { I think that it is likely that updateTaskDescription() is called after getting data from WebappDataStorage but not guaranteed. (I think it is likely because TabObserver#onTitleUpdated() is called often) https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:259: protected int initializeWebappDataInternal() { Nits: - Rename this function to initializeSolidColorSplashscreen() - It is confusing that this function returns an int. - Can initializeSolidColorSplashscreen() take a backgroundColor parameter? - Can we add a new function: static int adjustSplashScreenBackgroundColor(WebappInfo info) {} - Move the call to WebappUma#recordSplashscreenThemeColor() to onWebappInfoResolved() https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:279: private void initializeSplashScreenWidgets(final int backgroundColor) { Can this method be renamed to: protected static void fetchWebAppDataStorage(Callback<Pair<WebappDataStorage,Bitmap>> callback) WebappDataStorage#updateFromShortcutIntent() and WebappDataStorage#updateLastUsedTime() would be moved to the two arg version of initializeSplashScreenWidgets() My goal is to use fetchWebAppDataStorage() in both WebappActivity and WebapkActivity We could add SourceOfTruth WebappActivity#getWebappInfoSourceOfTruth() {} where SourceOfTruth would be enum SourceOfTruth { LAUNCH_INTENT, WEBAPP_DATA_STORAGE } and use getWebappInfoSourceOfTruth() in order to determine whether WebappDataStorage#updateFromShortcutIntent() should be called https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:285: if (storage == null) return; Nit: For the sake of consistency, shouldn't we treat null storage the same way here as in WebApkActivity? In WebApkActivity we call initializeSplashScreenWidgets() with a null bitmap https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:52: "last_request_new_version_shell_apk_time"; Can you remove KEY_LAST_REQUEST_NEW_VERSION_SHELL_APK_TIME for the sake of this CL? https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java (right): https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:200: for (String id : currentWebapps) { You added KEY_WEBAPK_PACKAGE_NAME in WebappDataStorage. We should either: Use KEY_WEBAPK_PACKAGE_NAME to get the package name everywhere OR Extract the package name from the id everywhere https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:223: private static boolean isWebApkInstalled(PackageManager pm, String webApkPakcage) { Nit: webApkPakcage -> webApkPackage
Patchset #3 (id:180001) has been deleted
Hi Peter, thanks for your comments. I modified some of the suggested solutions and hopefully made code look better. PTAL, thanks! https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:473: private void updateTaskDescription() { On 2016/06/23 01:16:37, pkotwicz wrote: > I think that it is likely that updateTaskDescription() is called after getting > data from WebappDataStorage but not guaranteed. (I think it is likely because > TabObserver#onTitleUpdated() is called often) Hmmm, now I understand your concern. It might not be guaranteed, but I don't feel necessary to check metadata every time when this function is called. Instead, we can update the mWebappInfo as long as we find web manifest changes and update metadata. https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:259: protected int initializeWebappDataInternal() { On 2016/06/23 01:16:37, pkotwicz wrote: > Nits: > - Rename this function to initializeSolidColorSplashscreen() > - It is confusing that this function returns an int. > - Can initializeSolidColorSplashscreen() take a backgroundColor parameter? > - Can we add a new function: > static int adjustSplashScreenBackgroundColor(WebappInfo info) {} Make sense to me. Renamed. > - Move the call to WebappUma#recordSplashscreenThemeColor() to > onWebappInfoResolved() I understand why you want to move this part, but it's better to keep them together. https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:279: private void initializeSplashScreenWidgets(final int backgroundColor) { On 2016/06/23 01:16:37, pkotwicz wrote: > Can this method be renamed to: > protected static void > fetchWebAppDataStorage(Callback<Pair<WebappDataStorage,Bitmap>> callback) > > WebappDataStorage#updateFromShortcutIntent() and > WebappDataStorage#updateLastUsedTime() would be moved to the two arg version of > initializeSplashScreenWidgets() > > My goal is to use fetchWebAppDataStorage() in both WebappActivity and > WebapkActivity > > We could add > SourceOfTruth WebappActivity#getWebappInfoSourceOfTruth() {} > where SourceOfTruth would be > enum SourceOfTruth { > LAUNCH_INTENT, > WEBAPP_DATA_STORAGE > } > and use getWebappInfoSourceOfTruth() in order to determine whether > WebappDataStorage#updateFromShortcutIntent() should be called Thanks for the suggestions, but it is too complicated. Instead, I did a refactoring: always retrieve storage before initialize the splash screen widgets. WebappActivity and WebApkActivity could have their own onWebappInfoResolved. https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:285: if (storage == null) return; On 2016/06/23 01:16:37, pkotwicz wrote: > Nit: For the sake of consistency, shouldn't we treat null storage the same way > here as in WebApkActivity? > In WebApkActivity we call initializeSplashScreenWidgets() with a null bitmap I think for this case, it prefers to open in a tab, not the Webapp. For WebAPK, we may want to call both initializeSplashScreenWidgets() and WebappRegister#registerWebapp() as well in the future. We can leave this question to dominickn@. https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:52: "last_request_new_version_shell_apk_time"; On 2016/06/23 01:16:37, pkotwicz wrote: > Can you remove KEY_LAST_REQUEST_NEW_VERSION_SHELL_APK_TIME for the sake of this > CL? Introduce KEY_LAST_REQUEST_NEW_VERSION_SHELL_APK_TIME is first goal of this CL, and using metadata when launching WebAPKs is the second goal. Since there aren't changes in this class, I would prefer to keep them. After all, this is the metadata for WebView only. https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java (right): https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:200: for (String id : currentWebapps) { On 2016/06/23 01:16:37, pkotwicz wrote: > You added KEY_WEBAPK_PACKAGE_NAME in WebappDataStorage. We should either: > Use KEY_WEBAPK_PACKAGE_NAME to get the package name everywhere > OR > Extract the package name from the id everywhere Ideally, we should use KEY_WEBAPK_PACKAGE_NAME as long as it is possible. It is more straightforward, and can void unnecessary checks in WebappStorage. However, we cannot get the package name directly without reading the SharedParences again for the webppId here. It isn't worthy reading them especially when there are many ids to check. https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:223: private static boolean isWebApkInstalled(PackageManager pm, String webApkPakcage) { On 2016/06/23 01:16:37, pkotwicz wrote: > Nit: webApkPakcage -> webApkPackage Done.
https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:57: if (storage == null) { It looks like you missed this comment https://codereview.chromium.org/2071213005/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2071213005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:248: private void initializeWebappData() { Based on crbug.com/522042 it looks like a splash screen with just the background color is intentionally displayed first. Can getting data from SharedPreferences take a long time? It seems logical that we would want the same behavior for WebAPKs and web apps (load background only once we get data from SharedPreferences). However, the current design seems to be intentional.
Patchset #4 (id:220001) has been deleted
Hi Peter, could you please take another look? Thanks! https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:57: if (storage == null) { On 2016/06/27 19:23:38, pkotwicz wrote: > It looks like you missed this comment Sorry, just forgot to reply in here. I add the registerWebapp() call when the storage is null, since this is a place that we don't need to guess the package name. https://codereview.chromium.org/2071213005/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2071213005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:248: private void initializeWebappData() { On 2016/06/27 19:23:38, pkotwicz wrote: > Based on crbug.com/522042 it looks like a splash screen with just the background > color is intentionally displayed first. Can getting data from SharedPreferences > take a long time? > > It seems logical that we would want the same behavior for WebAPKs and web apps > (load background only once we get data from SharedPreferences). However, the > current design seems to be intentional. That is a good point. We should keep their old way to display the background color before loading the SharedPreference. Moved that part before fetching the WebappDataStorage. For WebAPKs, we have to wait to fetch the metadata, since we can't start with one color and switch to another one if we find the background color is different.
pkotwicz@chromium.org changed reviewers: + yfriedman@chromium.org
https://codereview.chromium.org/2071213005/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2071213005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:248: private void initializeWebappData() { +yfriedman@ for his opinion on whether we should delay when non-WebAPK-webapps set the splash screen background color in order to be consistent with WebAPKs
Patchset #5 (id:260001) has been deleted
Description was changed from ========== Add WebAPK's metadata in WebappDataStorage. WebAPKs use WebappDataStorage to store the metadata too. Some changes in the web app manifest, like backgroundColor, themeColor, orientation etc. don't request ree-minting a WebAPK, but will update the metadata in the SharedPreference (WebappDataStorage). The metadata will be retrieved and used for launching a WebAPK. Besides, some update related timestamp(s) are stored in the SharedPreference too. Registering a WebAPK will be in a following up CL. BUG=609122 ========== to ========== Add WebAPK's metadata in WebappDataStorage. WebAPKs use WebappDataStorage to store the metadata too. Some changes in the web app manifest, like themeColor, orientation etc. don't request ree-minting a WebAPK, but will update the metadata in the SharedPreference (WebappDataStorage). The metadata will be retrieved and used for launching a WebAPK. Besides, some update related timestamp(s) are stored in the SharedPreference too. Registering a WebAPK will be in a following up CL. BUG=609122 ==========
Description was changed from ========== Add WebAPK's metadata in WebappDataStorage. WebAPKs use WebappDataStorage to store the metadata too. Some changes in the web app manifest, like themeColor, orientation etc. don't request ree-minting a WebAPK, but will update the metadata in the SharedPreference (WebappDataStorage). The metadata will be retrieved and used for launching a WebAPK. Besides, some update related timestamp(s) are stored in the SharedPreference too. Registering a WebAPK will be in a following up CL. BUG=609122 ========== to ========== Add WebAPK's metadata in WebappDataStorage. WebAPKs use WebappDataStorage to store the metadata too. Some changes in the web app manifest, like themeColor, orientation etc. don't request re-minting a WebAPK, but will update the metadata in the SharedPreference (WebappDataStorage). The metadata will be retrieved and used for launching a WebAPK. Besides, some update related timestamp(s) are stored in the SharedPreference too. Registering a WebAPK will be in a following up CL. BUG=609122 ==========
Hi Peter, could you please take another look? Thanks! https://codereview.chromium.org/2071213005/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2071213005/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:248: private void initializeWebappData() { On 2016/06/27 21:03:33, pkotwicz wrote: > +yfriedman@ for his opinion on whether we should delay when non-WebAPK-webapps > set the splash screen background color in order to be consistent with WebAPKs As discussed offline, we won't rely on the backgroundColor in metadata when launching the WebAPK. Background color is always from WebAPK's manifest, and its change will require re-mint WebAPK.
https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:57: if (storage == null) { I see the call to registerWebapp() now. Sorry that I missed it https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:473: private void updateTaskDescription() { I think that after we update WebappActivity#mWebappInfo we should call updateTaskDescription() just to be safe https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:285: if (storage == null) return; Ok https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:52: "last_request_new_version_shell_apk_time"; In my opinion, the goal of the CL is to give priority to data stored in WebappDataStorage over the extras passed in the WebAPK launch intent in determining how the splash screen should be rendered. https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java (right): https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:200: for (String id : currentWebapps) { I don't think it is bad to query SharedPreferences. WebappDataStorage#getLastUsedTime() queries SharedPreferences in the non-WebAPK case I prefer consistency over a marginal speed up. https://codereview.chromium.org/2071213005/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2071213005/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:64: storage.updateFromShortcutIntent(getIntent()); Nit: WebappRegistry#registerWebapp() registers the last used time so you do not need to do it here https://codereview.chromium.org/2071213005/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2071213005/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:280: if (storage == null) return; Can you rename resolveWebappInfo() to fetchDataStorage() and onWebappInfoResolved() to onFetchedDataStorage() If you move the early exit on line 280 to onFetchedDataStorage() you do not need to override fetchDataStorage() in WebApkActivity My goal is for WebappActivity and WebApkActivity to share as much code as possible. https://codereview.chromium.org/2071213005/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:297: // with the heuristic. Shouldn't this be called for WebAPKs too?
Patchset #6 (id:300001) has been deleted
Description was changed from ========== Add WebAPK's metadata in WebappDataStorage. WebAPKs use WebappDataStorage to store the metadata too. Some changes in the web app manifest, like themeColor, orientation etc. don't request re-minting a WebAPK, but will update the metadata in the SharedPreference (WebappDataStorage). The metadata will be retrieved and used for launching a WebAPK. Besides, some update related timestamp(s) are stored in the SharedPreference too. Registering a WebAPK will be in a following up CL. BUG=609122 ========== to ========== Add WebAPK's metadata in WebappDataStorage. WebAPKs use WebappDataStorage to store the metadata too. Some changes in the web app manifest, like themeColor, orientation etc. don't request re-minting a WebAPK, but will update the metadata in the SharedPreference (WebappDataStorage). The metadata will be retrieved and used for launching a WebAPK. Registering a WebAPK will be in a following up CL. BUG=609122 ==========
Description was changed from ========== Add WebAPK's metadata in WebappDataStorage. WebAPKs use WebappDataStorage to store the metadata too. Some changes in the web app manifest, like themeColor, orientation etc. don't request re-minting a WebAPK, but will update the metadata in the SharedPreference (WebappDataStorage). The metadata will be retrieved and used for launching a WebAPK. Registering a WebAPK will be in a following up CL. BUG=609122 ========== to ========== Use metadata when launching WebAPKs. WebAPKs use WebappDataStorage to store the metadata too. Some changes in the web app manifest, like themeColor, orientation etc. don't request re-minting a WebAPK, but will update the metadata in the SharedPreference (WebappDataStorage). The metadata will be retrieved and used for launching a WebAPK. Registering a WebAPK will be in a following up CL. BUG=609122 ==========
Hi Peter, PTAL, thanks! https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2071213005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:473: private void updateTaskDescription() { On 2016/06/28 19:00:11, pkotwicz wrote: > I think that after we update WebappActivity#mWebappInfo we should call > updateTaskDescription() just to be safe Done. https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:52: "last_request_new_version_shell_apk_time"; On 2016/06/28 19:00:11, pkotwicz wrote: > In my opinion, the goal of the CL is to give priority to data stored in > WebappDataStorage over the extras passed in the WebAPK launch intent in > determining how the splash screen should be rendered. Removed. https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java (right): https://codereview.chromium.org/2071213005/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:200: for (String id : currentWebapps) { On 2016/06/28 19:00:11, pkotwicz wrote: > I don't think it is bad to query SharedPreferences. > WebappDataStorage#getLastUsedTime() queries SharedPreferences in the non-WebAPK > case > > I prefer consistency over a marginal speed up. Done. https://codereview.chromium.org/2071213005/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2071213005/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:64: storage.updateFromShortcutIntent(getIntent()); On 2016/06/28 19:00:11, pkotwicz wrote: > Nit: WebappRegistry#registerWebapp() registers the last used time so you do not > need to do it here Good catch! https://codereview.chromium.org/2071213005/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2071213005/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:280: if (storage == null) return; On 2016/06/28 19:00:11, pkotwicz wrote: > Can you rename resolveWebappInfo() to fetchDataStorage() and > onWebappInfoResolved() to onFetchedDataStorage() > > If you move the early exit on line 280 to onFetchedDataStorage() you do not need > to override fetchDataStorage() in WebApkActivity > > My goal is for WebappActivity and WebApkActivity to share as much code as > possible. Good idea. I reorganized the code and removed the fecthDataStorage(). https://codereview.chromium.org/2071213005/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:297: // with the heuristic. On 2016/06/28 19:00:11, pkotwicz wrote: > Shouldn't this be called for WebAPKs too? For WebAPK: 1) we don't update the storage from Intent for normal cases, since all the data in the Intent are coming from the AndroidManifest. We do need to call storage.updateFromShortcutIntent(getIntent()) when WebAPK is updated or first registered. After we know the client/server communications better, we will find the best time to update the storage. 2) WebAPK always call the other functions: storage.updateLastUsedTime() and retrieveSplashScreenImage(getBackgroundColor(), storage) in onDataStorageFetched().
L-G-T-M after these changes. Thank you for bearing with me https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:65: // Update WebappInfo for the WebAPK. Changes in fields like themeColor, orientation How about: "Update WebappInfo from WebappDataStorage because the information in WebappDataStorage is more up to date than the information in the launch intent. Whenever the Web Manifest changes, WebappDataStorage is updated. Depending on which of the Web Manifest's fields change a new WebAPK may or may not be downloaded from the WebAPK server." - I think that my comment is slightly clearer https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:71: boolean isOrientationChange = Nit: isOrientationChange -> orientationChanged https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:81: retrieveSplashScreenImage(getBackgroundColor(), storage); Nit: Can you make the call to retrieveSplashScreenImage() the last one in this function. I think it makes more sense because retrieveSplashScreenImage() does asynchronous processing https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:276: protected void updateRemainingUma() { Nit: Rename function to recordSplashScreenThemeColorUma() https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:283: updateRemainingUma(); Can you move the call to recordSplashScreenThemeColorUma() after the "storage == null" check? I know that this "changes the behavior" but I think that it makes things more understandable. https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:316: protected void initializeSplashScreenWidgets(int backgroundColor, Bitmap splashImage) { Can you remove the backgroundColor parameter and call getBackgroundColor() where needed? https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:285: public WebappInfo createWebappInfo(WebappInfo info) { Nit: Rename info to fallbackInfo https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:371: if (webApkPackage != null) { Nits: - Use IntentUtils.safeGetStringExtra - Can you remove the if() statement and call putString() even if the package name is null? https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java (right): https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:222: 📝 Please add a comment! https://codereview.chromium.org/2071213005/diff/320001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java (right): https://codereview.chromium.org/2071213005/diff/320001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:359: String webappId = "webapk:uninstalledWebApk"; I do not think that this test exercises the WebAPK path anymore (because of the changes that I asked you to make to unregisterOldWebapps()) https://codereview.chromium.org/2071213005/diff/320001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:373: } Can you please add a test checking that the WebappInfo for a still-installed-WebAPK is not deleted by unregisterOldWebapps() ? I think that you can use Mockito to mock the PackageManager. Yaron did this in WebApkServiceImplTest.java
Patchset #7 (id:340001) has been deleted
Hi Peter, I addressed all of your comments, except the one that causes behavior changes. PTAL, thanks! https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:65: // Update WebappInfo for the WebAPK. Changes in fields like themeColor, orientation On 2016/06/29 00:42:19, pkotwicz wrote: > How about: "Update WebappInfo from WebappDataStorage because the information in > WebappDataStorage is more up to date than the information in the launch intent. > Whenever the Web Manifest changes, WebappDataStorage is updated. Depending on > which of the Web Manifest's fields change a new WebAPK may or may not be > downloaded from the WebAPK server." > > - I think that my comment is slightly clearer Done. https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:71: boolean isOrientationChange = On 2016/06/29 00:42:19, pkotwicz wrote: > Nit: isOrientationChange -> orientationChanged Done. https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:81: retrieveSplashScreenImage(getBackgroundColor(), storage); On 2016/06/29 00:42:19, pkotwicz wrote: > Nit: Can you make the call to retrieveSplashScreenImage() the last one in this > function. I think it makes more sense because retrieveSplashScreenImage() does > asynchronous processing Done. https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:276: protected void updateRemainingUma() { On 2016/06/29 00:42:19, pkotwicz wrote: > Nit: Rename function to recordSplashScreenThemeColorUma() Done. https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:283: updateRemainingUma(); On 2016/06/29 00:42:19, pkotwicz wrote: > Can you move the call to recordSplashScreenThemeColorUma() after the "storage == > null" check? > > I know that this "changes the behavior" but I think that it makes things more > understandable. I would prefer to leave it here, since it's better not to change the behavior. How about leaving it here and see the OWNER's opinions? https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:316: protected void initializeSplashScreenWidgets(int backgroundColor, Bitmap splashImage) { On 2016/06/29 00:42:19, pkotwicz wrote: > Can you remove the backgroundColor parameter and call getBackgroundColor() where > needed? Done. https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:285: public WebappInfo createWebappInfo(WebappInfo info) { On 2016/06/29 00:42:20, pkotwicz wrote: > Nit: Rename info to fallbackInfo Done. https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:371: if (webApkPackage != null) { On 2016/06/29 00:42:19, pkotwicz wrote: > Nits: > - Use IntentUtils.safeGetStringExtra > - Can you remove the if() statement and call putString() even if the package > name is null? Done. https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java (right): https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:222: On 2016/06/29 00:42:20, pkotwicz wrote: > 📝 Please add a comment! Done. https://codereview.chromium.org/2071213005/diff/320001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java (right): https://codereview.chromium.org/2071213005/diff/320001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:359: String webappId = "webapk:uninstalledWebApk"; On 2016/06/29 00:42:20, pkotwicz wrote: > I do not think that this test exercises the WebAPK path anymore (because of the > changes that I asked you to make to unregisterOldWebapps()) Good catch, updated this test accordingly. https://codereview.chromium.org/2071213005/diff/320001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:373: } On 2016/06/29 00:42:20, pkotwicz wrote: > Can you please add a test checking that the WebappInfo for a > still-installed-WebAPK is not deleted by unregisterOldWebapps() ? > > I think that you can use Mockito to mock the PackageManager. Yaron did this in > WebApkServiceImplTest.java Done.
LGTM!!!!!!!!!! https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2071213005/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:283: updateRemainingUma(); Ok let's get OWNER's opinion https://codereview.chromium.org/2071213005/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2071213005/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:317: int backgroundColor = getBackgroundColor(); Nit: Move backgroundColor variable initialization to line 366 https://codereview.chromium.org/2071213005/diff/360001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java (right): https://codereview.chromium.org/2071213005/diff/360001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:365: // Put the current time such that the task runs. Nit: Put -> Set https://codereview.chromium.org/2071213005/diff/360001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:397: // Put the current time such that the task runs. Nit: Put -> Set
hanxi@chromium.org changed reviewers: + dominickn@chromium.org
Hi dominickn@: could you please take a look? Thanks! https://codereview.chromium.org/2071213005/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2071213005/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:317: int backgroundColor = getBackgroundColor(); On 2016/06/29 21:26:18, pkotwicz wrote: > Nit: Move backgroundColor variable initialization to line 366 Done. https://codereview.chromium.org/2071213005/diff/360001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java (right): https://codereview.chromium.org/2071213005/diff/360001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:365: // Put the current time such that the task runs. On 2016/06/29 21:26:18, pkotwicz wrote: > Nit: Put -> Set Done. https://codereview.chromium.org/2071213005/diff/360001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:397: // Put the current time such that the task runs. On 2016/06/29 21:26:18, pkotwicz wrote: > Nit: Put -> Set Done.
https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:54: // Register the WebAPK. Can you add a comment here explaining why you need to register WebAPKs specifically and not other web apps? (I think the reason is because WebAPKs persist when Chrome's data is cleared?) https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:60: storage.updateFromShortcutIntent(getIntent()); At this point, where is this intent coming from? Is it from the home screen? https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:66: // is more up to date than the information in the launch intent. Whenever the Web Manifest Right now, this change doesn't make sense because of the TODO (the storage will never be more up to date than mWebappInfo right?). Can we leave this part out and add it when you implement the manifest update listening? https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:81: storage.updateLastUsedTime(); The parent method only updates last used time when the web app is launched from the home screen. Should this be consistent? https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:82: updateTaskDescription(); Is this call necessary? AFAIK all of this work happens before onResume(), which calls updateTaskDescription() (postInflationStartup() runs in onCreate() in the Android activity lifecycle). https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:283: if (storage == null) { Nit: remove the braces and inline "return" as it was previously. https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:291: return WebappInfo.create(mId, How many manifest properties don't trigger a re-minting when they change? If it's only background_color and orientation, then just add update*() methods in WebappInfo. It's not worth adding all of this extra code (which should also be tested) for just two properties. https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:405: * Returns the package name if the data is for a WebAPK, NULL otherwise. Nit: "null", not "NULL". Also, is it safer for this to return an empty string rather than null? https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:407: String getWebApkPakcageName() { Spelling: "getWebApkPackageName" https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java (right): https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:182: * 2. Deletes the data for all WebAPKs that have been uninstalled in the last a month. Nit: "last a month" -> "last month" https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:201: String webApkPackage = storage.getWebApkPakcageName(); getWebApkPackageName https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:202: if (webApkPackage != null) { Nit: if ((webApkPackage != null && isWebApkInstalled(pm, webApkPackage)) || ((currentTime - storage.getLastUsedTime()) < WEBAPP_UNOPENED_CLEANUP_DURATION)) { continue } Also, do you need the null check? What does PackageManager.getPackageInfo do if you pass in null? If it throws NameNotFoundException, then you can skip the null check and this becomes nicer. https://codereview.chromium.org/2071213005/diff/380001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java (right): https://codereview.chromium.org/2071213005/diff/380001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:355: public void testCleanupRemovesUninstalledWebApks() throws Exception { This test should: a) add a second webapk, and; b) verify that both are registered before running the cleanup https://codereview.chromium.org/2071213005/diff/380001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:385: public void testCleanupDoNotRemovesInstalledWebApks() throws Exception { Spelling: testCleanupDoesNotRemoveInstalledWebApks https://codereview.chromium.org/2071213005/diff/380001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:389: createWebApkIntent(webappId, webApkPackage)); This test should add a second webapk that isn't installed, and make sure that it gets cleaned up but the installed one is left behind.
Patchset #9 (id:400001) has been deleted
Hi Dominick, I addressed all of your comments, PTAL, thanks! https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:54: // Register the WebAPK. On 2016/06/30 06:53:51, dominickn wrote: > Can you add a comment here explaining why you need to register WebAPKs > specifically and not other web apps? (I think the reason is because WebAPKs > persist when Chrome's data is cleared?) Yes, when Chrome receives such an Intent from WebAPK, it means the WebAPK is still installed. https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:60: storage.updateFromShortcutIntent(getIntent()); On 2016/06/30 06:53:51, dominickn wrote: > At this point, where is this intent coming from? Is it from the home screen? Yes, it is the intent sent by a WebAPK to launch Chrome in full screen. https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:66: // is more up to date than the information in the launch intent. Whenever the Web Manifest On 2016/06/30 06:53:51, dominickn wrote: > Right now, this change doesn't make sense because of the TODO (the storage will > never be more up to date than mWebappInfo right?). Can we leave this part out > and add it when you implement the manifest update listening? There are only two fields to update, so it doesn't hurt to keep this change if you are ok with the refactoring. Besides, we need to fetch the storage to update the "last launching time" anyway. What do you think? https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:81: storage.updateLastUsedTime(); On 2016/06/30 06:53:51, dominickn wrote: > The parent method only updates last used time when the web app is launched from > the home screen. Should this be consistent? It is not supper important to know whether the WebAPK is launched from home screen or from navigation (i.e., user click a link) etc., and actually WebAPK doesn't know how it is launched. It just sends an intent to open Chrome in full screen, and the intent is the one that we get here. We want to have a timestamp for last launching time of WebAPK. So we treat the "last used time" as the "last launch time". I think it is better to reuse the existing one than introducing a new one. Does it make sense? https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:82: updateTaskDescription(); On 2016/06/30 06:53:51, dominickn wrote: > Is this call necessary? AFAIK all of this work happens before onResume(), which > calls updateTaskDescription() (postInflationStartup() runs in onCreate() in the > Android activity lifecycle). Removed. https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:283: if (storage == null) { On 2016/06/30 06:53:51, dominickn wrote: > Nit: remove the braces and inline "return" as it was previously. Done. https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:291: return WebappInfo.create(mId, On 2016/06/30 06:53:51, dominickn wrote: > How many manifest properties don't trigger a re-minting when they change? If > it's only background_color and orientation, then just add update*() methods in > WebappInfo. It's not worth adding all of this extra code (which should also be > tested) for just two properties. This was my initial thought. Revert back to use update*() in WebappInfo. Another test is added. https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:405: * Returns the package name if the data is for a WebAPK, NULL otherwise. On 2016/06/30 06:53:51, dominickn wrote: > Nit: "null", not "NULL". Also, is it safer for this to return an empty string > rather than null? Done. https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:407: String getWebApkPakcageName() { On 2016/06/30 06:53:52, dominickn wrote: > Spelling: "getWebApkPackageName" Done. https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java (right): https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:182: * 2. Deletes the data for all WebAPKs that have been uninstalled in the last a month. On 2016/06/30 06:53:52, dominickn wrote: > Nit: "last a month" -> "last month" Done. https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:201: String webApkPackage = storage.getWebApkPakcageName(); On 2016/06/30 06:53:52, dominickn wrote: > getWebApkPackageName Sorry for the typo. https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:202: if (webApkPackage != null) { On 2016/06/30 06:53:52, dominickn wrote: > Nit: > > if ((webApkPackage != null && isWebApkInstalled(pm, webApkPackage)) > || ((currentTime - storage.getLastUsedTime()) < > WEBAPP_UNOPENED_CLEANUP_DURATION)) { > continue > } > > Also, do you need the null check? What does PackageManager.getPackageInfo do if > you pass in null? If it throws NameNotFoundException, then you can skip the null > check and this becomes nicer. The suggested check will delay the removing of an uninstalled WebAPK until it meets the time limit. I think we'd better unregister it as soon as we know it was uninstalled. The "webApkPackage != null" is checking whether it is a WebAPK or not. https://codereview.chromium.org/2071213005/diff/380001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java (right): https://codereview.chromium.org/2071213005/diff/380001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:355: public void testCleanupRemovesUninstalledWebApks() throws Exception { On 2016/06/30 06:53:52, dominickn wrote: > This test should: > a) add a second webapk, and; > b) verify that both are registered > > before running the cleanup Done. https://codereview.chromium.org/2071213005/diff/380001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:385: public void testCleanupDoNotRemovesInstalledWebApks() throws Exception { On 2016/06/30 06:53:52, dominickn wrote: > Spelling: testCleanupDoesNotRemoveInstalledWebApks Done. https://codereview.chromium.org/2071213005/diff/380001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:389: createWebApkIntent(webappId, webApkPackage)); On 2016/06/30 06:53:52, dominickn wrote: > This test should add a second webapk that isn't installed, and make sure that it > gets cleaned up but the installed one is left behind. Done.
Patchset #9 (id:420001) has been deleted
Description was changed from ========== Use metadata when launching WebAPKs. WebAPKs use WebappDataStorage to store the metadata too. Some changes in the web app manifest, like themeColor, orientation etc. don't request re-minting a WebAPK, but will update the metadata in the SharedPreference (WebappDataStorage). The metadata will be retrieved and used for launching a WebAPK. Registering a WebAPK will be in a following up CL. BUG=609122 ========== to ========== Use metadata when launching WebAPKs. WebAPKs use WebappDataStorage to store the metadata too. Some changes in the web app manifest, like themeColor, orientation etc. don't request re-minting a WebAPK, but will update the metadata in the SharedPreference (WebappDataStorage). The metadata will be retrieved and used for launching a WebAPK. Registering a WebAPK will be in a following up CL. BUG=624834 ==========
https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java (right): https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:202: if (webApkPackage != null) { On 2016/06/30 15:51:18, Xi Han wrote: > On 2016/06/30 06:53:52, dominickn wrote: > > Nit: > > > > if ((webApkPackage != null && isWebApkInstalled(pm, webApkPackage)) > > || ((currentTime - storage.getLastUsedTime()) < > > WEBAPP_UNOPENED_CLEANUP_DURATION)) { > > continue > > } > > > > Also, do you need the null check? What does PackageManager.getPackageInfo do > if > > you pass in null? If it throws NameNotFoundException, then you can skip the > null > > check and this becomes nicer. > > The suggested check will delay the removing of an uninstalled WebAPK until it > meets the time limit. I think we'd better unregister it as soon as we know it > was uninstalled. The "webApkPackage != null" is checking whether it is a WebAPK > or not. The check I suggested is exactly the same as what you have, right? It's just formatted into a single if statement - it's something of an antipattern to do: if (foo) continue else if (bar) continue You should do: if (foo || bar) continue which is what I suggested. https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:203: if (isWebApkInstalled(pm, webApkPackage)) { My question stands regarding webApkPackage - I really prefer not having to test nulls if at all possible. If PackageManager.getPackageInfo() throws when you pass it null instead of a string, then you don't need the null check, right? Because the only time webApkPackage is null is when it's not a WebAPK, which is what you want? https://codereview.chromium.org/2071213005/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2071213005/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:482: protected void updateTaskDescription() { Nit: this no longer needs to be protected. https://codereview.chromium.org/2071213005/diff/440001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java (right): https://codereview.chromium.org/2071213005/diff/440001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:402: public void testCleanupDoesNotRemovesInstalledWebApks() throws Exception { Removes -> Remove
https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java (right): https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:202: if (webApkPackage != null) { On 2016/07/01 00:27:53, dominickn wrote: > On 2016/06/30 15:51:18, Xi Han wrote: > > On 2016/06/30 06:53:52, dominickn wrote: > > > Nit: > > > > > > if ((webApkPackage != null && isWebApkInstalled(pm, webApkPackage)) > > > || ((currentTime - storage.getLastUsedTime()) < > > > WEBAPP_UNOPENED_CLEANUP_DURATION)) { > > > continue > > > } > > > > > > Also, do you need the null check? What does PackageManager.getPackageInfo do > > if > > > you pass in null? If it throws NameNotFoundException, then you can skip the > > null > > > check and this becomes nicer. > > > > The suggested check will delay the removing of an uninstalled WebAPK until it > > meets the time limit. I think we'd better unregister it as soon as we know it > > was uninstalled. The "webApkPackage != null" is checking whether it is a > WebAPK > > or not. > > The check I suggested is exactly the same as what you have, right? It's just > formatted into a single if statement - it's something of an antipattern to do: > > if (foo) > continue > else if (bar) > continue > > You should do: > > if (foo || bar) > continue > > which is what I suggested. No, it isn't. My check is: if (a) { if (b) { } else if (c) { } So the combined logic would be: if (a && b || !a && c) { } The combined logic doesn't look nicer, unfortunately, and I hesitated to use it replace the current one. https://codereview.chromium.org/2071213005/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:203: if (isWebApkInstalled(pm, webApkPackage)) { On 2016/07/01 00:27:53, dominickn wrote: > My question stands regarding webApkPackage - I really prefer not having to test > nulls if at all possible. If PackageManager.getPackageInfo() throws when you > pass it null instead of a string, then you don't need the null check, right? > Because the only time webApkPackage is null is when it's not a WebAPK, which is > what you want? As the logic above, it is not about how the null package name is handled within in isWebApkInstalled(), it is because we have to keep the null check to see whether it is a WebAPK or not.
I don't want to hold up this CL any longer, though I'm still not convinced that checking webApkPackage is necessary (i.e. I don't understand why getWebApkPackageName() would ever return anything relevant for a non-WebAPK, and there that value should be perfectly safe to pass into PackageManager without checking it). If you fix up the other nits I pointed out that will do.
If you look at WebappRegistry at patch5, you will see that I used webappId to check the package name, not the getWebApkPackageName(). They are just different ways to check whether it is a WebAPK or a regular webapp. The isWebApkInstalled() will return the same for either an uninstalled WebAPK or a regular webapp, so we need the package name to know which one was WebAPK. If it was a WebAPK, we want to remove it from the SharedPreference right away; but if it is a regular webapp, we will still wait it passes the time limit and then remove it. I updated this CL according to your comments. PTAL, and thank you for all the suggestions! https://codereview.chromium.org/2071213005/diff/440001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2071213005/diff/440001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:482: protected void updateTaskDescription() { On 2016/07/01 00:27:53, dominickn wrote: > Nit: this no longer needs to be protected. Done. https://codereview.chromium.org/2071213005/diff/440001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java (right): https://codereview.chromium.org/2071213005/diff/440001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:402: public void testCleanupDoesNotRemovesInstalledWebApks() throws Exception { On 2016/07/01 00:27:53, dominickn wrote: > Removes -> Remove Done.
lgtm - thanks for the explanation, additional tests and nit fixes. :)
hanxi@chromium.org changed reviewers: + dfalcantara@chromium.org
Hi Dan, I need OWNER review for WebApkActivity.java. Could you please take a look? Thanks!
lgtm https://chromiumcodereview.appspot.com/2071213005/diff/460001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://chromiumcodereview.appspot.com/2071213005/diff/460001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:66: } nit: newline here https://chromiumcodereview.appspot.com/2071213005/diff/460001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:72: // update SharedPreference for WebAPKs. nit: indent so that "update" lines up with "Introduces" above. https://chromiumcodereview.appspot.com/2071213005/diff/460001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java (right): https://chromiumcodereview.appspot.com/2071213005/diff/460001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:203: if (isWebApkInstalled(pm, webApkPackage)) { nit: Be consistent. Either put the continues on the same line as the if statement (like below) or put them in the block (like here).
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, dominickn@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2071213005/#ps480001 (title: "Nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Use metadata when launching WebAPKs. WebAPKs use WebappDataStorage to store the metadata too. Some changes in the web app manifest, like themeColor, orientation etc. don't request re-minting a WebAPK, but will update the metadata in the SharedPreference (WebappDataStorage). The metadata will be retrieved and used for launching a WebAPK. Registering a WebAPK will be in a following up CL. BUG=624834 ========== to ========== Use metadata when launching WebAPKs. WebAPKs use WebappDataStorage to store the metadata too. Some changes in the web app manifest, like themeColor, orientation etc. don't request re-minting a WebAPK, but will update the metadata in the SharedPreference (WebappDataStorage). The metadata will be retrieved and used for launching a WebAPK. Registering a WebAPK will be in a following up CL. BUG=624834 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:480001)
Message was sent while issue was closed.
Description was changed from ========== Use metadata when launching WebAPKs. WebAPKs use WebappDataStorage to store the metadata too. Some changes in the web app manifest, like themeColor, orientation etc. don't request re-minting a WebAPK, but will update the metadata in the SharedPreference (WebappDataStorage). The metadata will be retrieved and used for launching a WebAPK. Registering a WebAPK will be in a following up CL. BUG=624834 ========== to ========== Use metadata when launching WebAPKs. WebAPKs use WebappDataStorage to store the metadata too. Some changes in the web app manifest, like themeColor, orientation etc. don't request re-minting a WebAPK, but will update the metadata in the SharedPreference (WebappDataStorage). The metadata will be retrieved and used for launching a WebAPK. Registering a WebAPK will be in a following up CL. BUG=624834 Committed: https://crrev.com/a6fe748fda4067575facddcea0e99ac41139670a Cr-Commit-Position: refs/heads/master@{#403677} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/a6fe748fda4067575facddcea0e99ac41139670a Cr-Commit-Position: refs/heads/master@{#403677}
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:480001) has been created in https://codereview.chromium.org/2122813002/ by ksakamoto@chromium.org. The reason for reverting is: Broke chrome_public_test_apk in Android Tests (dbg). https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg... . |
