Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2725813004:
Init WebApkUpdateManager with a WebappDataStorage to avoid null object. (Closed)
DescriptionInit WebApkUpdateManager with a WebappDataStorage to avoid null object.
WebappDataStorage may be invalid for two reasons:
1. Storage is not initialized yet. WebAPKs are registered with
WebappRegistry at postInflationStartup using an AsyncTask.
2. Browser data is cleared. Clearing Chrome browser data will affect
WebappDataStorage.
This CL refactors WebApkUpdateManager so that it takes in a
WebappDataStorage object upon construction. This CL also moves the
WebAPK registration into onDeferredStartup().
BUG=696418
Review-Url: https://codereview.chromium.org/2725813004
Cr-Commit-Position: refs/heads/master@{#455082}
Committed: https://chromium.googlesource.com/chromium/src/+/37402096ff8038dd57754bfd6f41fac15cff89df
Patch Set 1 #Patch Set 2 : Keep recordUpdate static, instead of making C++ class non-static. #
Total comments: 8
Patch Set 3 : Use overrides to simply storage update & splash screen init. #
Total comments: 10
Patch Set 4 : Fix errors #
Total comments: 8
Patch Set 5 : Split onDeferredStorage #
Messages
Total messages: 51 (35 generated)
Description was changed from ========== Add validity check for WebappDataStorage uses in WebapkUpdateManager BUG= ========== to ========== Add validity checks of WebappDataStorage in WebapkUpdateManager WebappDataStorage may be invalid for two reasons: 1. Storage is not initialized yet. WebAPKs are registered with WebappRegistry at postInflationStartup using an AsyncTask. 2. Browser data is cleared. Clearing Chrome browser data will affect WebappDataStorage. This Cl adds validity checks of WebappDataStorage in WebapkUpdateManager to be consistent with other classes that uses WebappDataStorage. BUG=696418 ==========
The CQ bit was checked by zpeng@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...
zpeng@chromium.org changed reviewers: + dominickn@chromium.org, pkotwicz@chromium.org
Hi Dominick & Peter, PTAL. Thanks!
I thought about this a bit I am wondering whether: - WebApkUpdateManager can take a WebappDataStorage parameter in its constructor. The data in WebappDataStorage might be cleared during the object's lifetime but this should be ok. - The WebApkUpdateManager be only created once WebApkActivity#onStorageIsNull() is run. I am thinking of having WebApkActivity#mOnStorageInitializedTasks ArrayList<Runnable>. When the WebApkActivity#onStorageIsNull() callback is called all of the tasks are run. - WebApkActivity#onDeferredStartup() would add a task to WebApkActivity#mOnStorageInitializedTasks if the storage that it fetches is null Felix, what do you think?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add validity checks of WebappDataStorage in WebapkUpdateManager WebappDataStorage may be invalid for two reasons: 1. Storage is not initialized yet. WebAPKs are registered with WebappRegistry at postInflationStartup using an AsyncTask. 2. Browser data is cleared. Clearing Chrome browser data will affect WebappDataStorage. This Cl adds validity checks of WebappDataStorage in WebapkUpdateManager to be consistent with other classes that uses WebappDataStorage. BUG=696418 ========== to ========== Init WebApkUpdateManager with a WebappDataStorage to avoid null object. WebappDataStorage may be invalid for two reasons: 1. Storage is not initialized yet. WebAPKs are registered with WebappRegistry at postInflationStartup using an AsyncTask. 2. Browser data is cleared. Clearing Chrome browser data will affect WebappDataStorage. This CL moves This Cl adds validity checks of WebappDataStorage in WebapkUpdateManager to be consistent with other classes that uses WebappDataStorage. BUG=696418 ==========
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Init WebApkUpdateManager with a WebappDataStorage to avoid null object. WebappDataStorage may be invalid for two reasons: 1. Storage is not initialized yet. WebAPKs are registered with WebappRegistry at postInflationStartup using an AsyncTask. 2. Browser data is cleared. Clearing Chrome browser data will affect WebappDataStorage. This CL moves This Cl adds validity checks of WebappDataStorage in WebapkUpdateManager to be consistent with other classes that uses WebappDataStorage. BUG=696418 ========== to ========== Init WebApkUpdateManager with a WebappDataStorage to avoid null object. WebappDataStorage may be invalid for two reasons: 1. Storage is not initialized yet. WebAPKs are registered with WebappRegistry at postInflationStartup using an AsyncTask. 2. Browser data is cleared. Clearing Chrome browser data will affect WebappDataStorage. This CL refactors WebApkUpdateManager so that it takes in a WebappDataStorage object upon construction. This CL also moves the WebAPK registration into onDeferredStartup(). This Cl adds validity checks of WebappDataStorage in WebapkUpdateManager to be consistent with other classes that uses WebappDataStorage. BUG=696418 ==========
Description was changed from ========== Init WebApkUpdateManager with a WebappDataStorage to avoid null object. WebappDataStorage may be invalid for two reasons: 1. Storage is not initialized yet. WebAPKs are registered with WebappRegistry at postInflationStartup using an AsyncTask. 2. Browser data is cleared. Clearing Chrome browser data will affect WebappDataStorage. This CL refactors WebApkUpdateManager so that it takes in a WebappDataStorage object upon construction. This CL also moves the WebAPK registration into onDeferredStartup(). This Cl adds validity checks of WebappDataStorage in WebapkUpdateManager to be consistent with other classes that uses WebappDataStorage. BUG=696418 ========== to ========== Init WebApkUpdateManager with a WebappDataStorage to avoid null object. WebappDataStorage may be invalid for two reasons: 1. Storage is not initialized yet. WebAPKs are registered with WebappRegistry at postInflationStartup using an AsyncTask. 2. Browser data is cleared. Clearing Chrome browser data will affect WebappDataStorage. This CL refactors WebApkUpdateManager so that it takes in a WebappDataStorage object upon construction. This CL also moves the WebAPK registration into onDeferredStartup(). BUG=696418 ==========
Thanks Peter! PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Patchset #2 (id:20001) has been deleted
This CL doesn't seem to do what the description says it does? Is it ready for review?
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by zpeng@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...
Thanks Dominick & Peter, PTAL! Sorry for the confusion caused by the deletion of last patch. It also caused Peter's comments to be blank. To summarize, in the beginning this CL adds checks before using WebappDataStorage in WebApkUpdateManager. By Peter's comments and offline discussion, we are refactoring WebApkUpdateManager to take a storage object during construction to avoid crashes caused by null storage objects.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2725813004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2725813004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:174: (WebApkInfo) mWebappInfo); We want to run the code on lines 167 - 174 when storage != null too https://codereview.chromium.org/2725813004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2725813004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:63: How about: "The WebappDataStorage with cached data about prior update requests." WebappDataStorage does have the WebAPK's cached data but we don't use this data. We get the WebAPK data from |mInfo| https://codereview.chromium.org/2725813004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2725813004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:350: if (storage == null) return; For the sake of understandability, maybe add a function: isNullStorageSupported() which returns whether the WebappActivity can be run if the storage is null - Maybe we can revisit this later and allow WebappActivities to run when storage is null. We only care about the files/webapp-authenticator file not being deleted Maybe add a onDeferredStartup() method to this class and call updateStorage() there. That way we are consistent across webapps and WebAPKs. It looks like WebappRegistry#getWebappDataStorage() just accesses data from a java.util.Map so it should be really fast. The change that I am suggesting might break some tests (Run the Webapp related tests in order to make sure that none of the tests break) This method would become: WebappDataStorage storage = WebappRegistry.getInstance().getWebappDataStorage(mWebappInfo.id()); if (storage == null && !isNullStorageSupported()) return; if (storage == null) { initializeSplashScreenWidgets(backgroundColor, null); return; } storage.getSplashScreenImage(..){..} https://codereview.chromium.org/2725813004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:351: For the WebApkActivity#isNullStorageSupported() comment you can put: "WebAPKs don't register with WebappDataStorage when they are created. They need to still be launchable after a user clear's Chrome's data."
The CQ bit was checked by zpeng@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...
Thanks Peter, PTAL! https://codereview.chromium.org/2725813004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2725813004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:174: (WebApkInfo) mWebappInfo); On 2017/03/02 20:26:37, pkotwicz wrote: > We want to run the code on lines 167 - 174 when storage != null too Done. https://codereview.chromium.org/2725813004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java (right): https://codereview.chromium.org/2725813004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:63: On 2017/03/02 20:26:37, pkotwicz wrote: > How about: "The WebappDataStorage with cached data about prior update requests." > > WebappDataStorage does have the WebAPK's cached data but we don't use this data. > We get the WebAPK data from |mInfo| Done. https://codereview.chromium.org/2725813004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2725813004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:350: if (storage == null) return; On 2017/03/02 20:26:37, pkotwicz wrote: > For the sake of understandability, maybe add a function: > isNullStorageSupported() which returns whether the WebappActivity can be run if > the storage is null > - Maybe we can revisit this later and allow WebappActivities to run when storage > is null. We only care about the files/webapp-authenticator file not being > deleted > > Maybe add a onDeferredStartup() method to this class and call updateStorage() > there. That way we are consistent across webapps and WebAPKs. It looks like > WebappRegistry#getWebappDataStorage() just accesses data from a java.util.Map so > it should be really fast. > The change that I am suggesting might break some tests (Run the Webapp related > tests in order to make sure that none of the tests break) > > This method would become: > > WebappDataStorage storage = > WebappRegistry.getInstance().getWebappDataStorage(mWebappInfo.id()); > if (storage == null && !isNullStorageSupported()) return; > > if (storage == null) { > initializeSplashScreenWidgets(backgroundColor, null); > return; > } > > storage.getSplashScreenImage(..){..} Done. https://codereview.chromium.org/2725813004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:351: On 2017/03/02 20:26:37, pkotwicz wrote: > For the WebApkActivity#isNullStorageSupported() comment you can put: > > "WebAPKs don't register with WebappDataStorage when they are created. They need > to still > be launchable after a user clear's Chrome's data." Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2725813004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2725813004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:152: // Initialize the time of the last is-update-needed check with the Nit: can you make this comment 2 lines only (the Java line limit is 100 chars) https://codereview.chromium.org/2725813004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:160: super.onDeferredStartup(); This is a bit strange because super.onDeferredStartup() will fetch the WebappDataStorage too. Perhaps you should remove this line and add a call to onDeferredStartupWithStorage(storage) at the end of this method? https://codereview.chromium.org/2725813004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:174: mUpdateManager = new WebApkUpdateManager(WebApkActivity.this, storage); Won't this only create mUpdateManager if the WebappDataStorage was initially null? The old behaviour created it every time. https://codereview.chromium.org/2725813004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2725813004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:368: if (!isNullStorageSupported()) return; if (isNullStorageSupported()) { initializeSplashScreenWidgets(backgroundColor, null); } return; https://codereview.chromium.org/2725813004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:374: // No need to update storage here as splash screen image would not be updated. This comment doesn't really make sense. The main reason for updating the storage is to keep the last used time up to date. Can you just remove this?
Thanks Dominick & Peter, PTAL! Apologies for the low-quality patch. This week my head hurt. Please be more careful with my CLs. Sorry, and thanks! https://codereview.chromium.org/2725813004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2725813004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:152: // Initialize the time of the last is-update-needed check with the On 2017/03/03 02:36:48, dominickn wrote: > Nit: can you make this comment 2 lines only (the Java line limit is 100 chars) Done. https://codereview.chromium.org/2725813004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:160: super.onDeferredStartup(); On 2017/03/03 02:36:48, dominickn wrote: > This is a bit strange because super.onDeferredStartup() will fetch the > WebappDataStorage too. Perhaps you should remove this line and add a call to > onDeferredStartupWithStorage(storage) at the end of this method? At the moment, fetching WebappDataStorage is fast because it's stored in a HashMap, so I think it is ok to fetch it twice. Also, I think not calling super class's method would be more difficult to maintain in the long run. WDUT? https://codereview.chromium.org/2725813004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:174: mUpdateManager = new WebApkUpdateManager(WebApkActivity.this, storage); On 2017/03/03 02:36:47, dominickn wrote: > Won't this only create mUpdateManager if the WebappDataStorage was initially > null? The old behaviour created it every time. Oops. Fixed by moving these into onDeferredStartupWithStorage https://codereview.chromium.org/2725813004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2725813004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:368: if (!isNullStorageSupported()) return; On 2017/03/03 02:36:48, dominickn wrote: > if (isNullStorageSupported()) { > initializeSplashScreenWidgets(backgroundColor, null); > } > return; Done. https://codereview.chromium.org/2725813004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:374: // No need to update storage here as splash screen image would not be updated. On 2017/03/03 02:36:48, dominickn wrote: > This comment doesn't really make sense. The main reason for updating the storage > is to keep the last used time up to date. Can you just remove this? Done.
LGTM! https://codereview.chromium.org/2725813004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2725813004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:170: // installed, so re-register it. How about: "Register the WebAPK. The WebAPK is not registered when it is created so we have to register it now. The WebAPK may also be unregistered if a user cleared Chrome's data." https://codereview.chromium.org/2725813004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2725813004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:296: protected void onDeferredStartupWithStorage(WebappDataStorage storage) { Nit: Reorder this function after onDeferredStartup() It is generally nice to have functions which occur later be closer to the end of the file than functions which occur earlier
There will be a bunch of conflicts with crrev.com/2641973003, so I'll leave it to you and Xi to decide who should land what first. :) https://codereview.chromium.org/2725813004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2725813004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:163: super.onDeferredStartup(); This is still a little confusing in the case where we have a null WebappDataStorage. We'll immediately try fetching one, it'll be null, so then we try fetching it again in here before re-registering. I think it's much clearer if you remove the super call and just add an else case after the if (storage == null) that calls onDeferredStartup(storage). That has the benefit of not needing to bounce to the super class to understand the control flow. https://codereview.chromium.org/2725813004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2725813004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:360: protected boolean isNullStorageSupported() { I actually don't understand why we don't initialize the splash screen with mWebappInfo.icon() if WebappDataStorage is null in WebappActivity (that's part of crbug.com/698354). I feel that that was a decision made by the person who first implemented splash screens (only show if WebappDataStorage is available). But initializeSplashScreenWidgets already falls back onto using mWebappInfo.icon() if you pass null into it. Can you try just removing isNullStorageSupported, and always calling initializeSplashScreenWidgets(backgroundColor, null) if the WebappDataStorage is null? It should have the effect that when adding a PWA (non-WebAPK), then clearing browsing data, the PWA should launch with the homescreen icon as the splash screen icon.
The CQ bit was checked by zpeng@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...
Thanks Dominick & Peter, PTAL! https://codereview.chromium.org/2725813004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2725813004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:163: super.onDeferredStartup(); On 2017/03/06 02:08:02, dominickn wrote: > This is still a little confusing in the case where we have a null > WebappDataStorage. We'll immediately try fetching one, it'll be null, so then we > try fetching it again in here before re-registering. > > I think it's much clearer if you remove the super call and just add an else case > after the if (storage == null) that calls onDeferredStartup(storage). That has > the benefit of not needing to bounce to the super class to understand the > control flow. Because onDeferredStartup is a function inherited Async Activity, I think it is better if we keep calling the super class. That way, if some superclass such as ChromeActivity changes something in onDeferredStartup, WebApkActivity can get the update as well. (Oops, forgot to call super in WebappActivity) After discussion with Peter, we propose adding onDeferredStartupWithNullStorage and onDeferredStartupWithStorage. WDUT? https://codereview.chromium.org/2725813004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:170: // installed, so re-register it. On 2017/03/03 20:49:26, pkotwicz wrote: > How about: "Register the WebAPK. The WebAPK is not registered when it is created > so we have to register it now. The WebAPK may also be unregistered if a user > cleared Chrome's data." Done. https://codereview.chromium.org/2725813004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2725813004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:296: protected void onDeferredStartupWithStorage(WebappDataStorage storage) { On 2017/03/03 20:49:26, pkotwicz wrote: > Nit: Reorder this function after onDeferredStartup() > > It is generally nice to have functions which occur later be closer to the end of > the file than functions which occur earlier Done. https://codereview.chromium.org/2725813004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:360: protected boolean isNullStorageSupported() { On 2017/03/06 02:08:02, dominickn wrote: > I actually don't understand why we don't initialize the splash screen with > mWebappInfo.icon() if WebappDataStorage is null in WebappActivity (that's part > of crbug.com/698354). I feel that that was a decision made by the person who > first implemented splash screens (only show if WebappDataStorage is available). > But initializeSplashScreenWidgets already falls back onto using > mWebappInfo.icon() if you pass null into it. > > Can you try just removing isNullStorageSupported, and always calling > initializeSplashScreenWidgets(backgroundColor, null) if the WebappDataStorage is > null? > > It should have the effect that when adding a PWA (non-WebAPK), then clearing > browsing data, the PWA should launch with the homescreen icon as the splash > screen icon. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, thanks. This looks a lot more straightforward control-flow wise.
The CQ bit was checked by zpeng@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/2725813004/#ps120001 (title: "Split onDeferredStorage")
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": 120001, "attempt_start_ts": 1488901058376530,
"parent_rev": "86e08ee7912c3c9bb87e98474ccb9ed21f86753a", "commit_rev":
"37402096ff8038dd57754bfd6f41fac15cff89df"}
Message was sent while issue was closed.
Description was changed from ========== Init WebApkUpdateManager with a WebappDataStorage to avoid null object. WebappDataStorage may be invalid for two reasons: 1. Storage is not initialized yet. WebAPKs are registered with WebappRegistry at postInflationStartup using an AsyncTask. 2. Browser data is cleared. Clearing Chrome browser data will affect WebappDataStorage. This CL refactors WebApkUpdateManager so that it takes in a WebappDataStorage object upon construction. This CL also moves the WebAPK registration into onDeferredStartup(). BUG=696418 ========== to ========== Init WebApkUpdateManager with a WebappDataStorage to avoid null object. WebappDataStorage may be invalid for two reasons: 1. Storage is not initialized yet. WebAPKs are registered with WebappRegistry at postInflationStartup using an AsyncTask. 2. Browser data is cleared. Clearing Chrome browser data will affect WebappDataStorage. This CL refactors WebApkUpdateManager so that it takes in a WebappDataStorage object upon construction. This CL also moves the WebAPK registration into onDeferredStartup(). BUG=696418 Review-Url: https://codereview.chromium.org/2725813004 Cr-Commit-Position: refs/heads/master@{#455082} Committed: https://chromium.googlesource.com/chromium/src/+/37402096ff8038dd57754bfd6f41... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://chromium.googlesource.com/chromium/src/+/37402096ff8038dd57754bfd6f41... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
