Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2335843002:
Remove the logic of fetching metadata when launching WebAPKs. (Closed)
DescriptionRemove the logic of fetching metadata when launching WebAPKs.
The logic was introduced in CL (https://codereview.chromium.org/2126583002/).
Since all its metadata are stored in the WebAPK's AndroidManifest.xml, so
there is no need to fetch metadata like theme color etc. during launch
the WebAPK. Revert the logic back.
BUG=624834
Committed: https://crrev.com/b6c54b8210dae027b628164b8655c740b060e9b9
Cr-Commit-Position: refs/heads/master@{#418918}
Patch Set 1 #
Total comments: 4
Patch Set 2 : pkotwicz@'s comments. #
Total comments: 2
Patch Set 3 : Add comments #
Total comments: 4
Patch Set 4 : Update comment. #
Messages
Total messages: 32 (20 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Remove logic to use metadata for WebAPK. The logic was introduced in CL (https://codereview.chromium.org/2126583002/). Since we stores all its metadata in the WebAPK's AndroidManifest.xml, so we doesn't need to fetch metadata like background color etc. during launch the WebAPK. Revert the logic back. BUG=624834 ========== to ========== Remove logic to use metadata for WebAPK. The logic was introduced in CL (https://codereview.chromium.org/2126583002/). Since we stores all its metadata in the WebAPK's AndroidManifest.xml, so we doesn't need to fetch metadata like theme color etc. during launch the WebAPK. Revert the logic back. BUG=624834 ==========
Description was changed from ========== Remove logic to use metadata for WebAPK. The logic was introduced in CL (https://codereview.chromium.org/2126583002/). Since we stores all its metadata in the WebAPK's AndroidManifest.xml, so we doesn't need to fetch metadata like theme color etc. during launch the WebAPK. Revert the logic back. BUG=624834 ========== to ========== Remove logic to use metadata for WebAPK. The logic was introduced in CL (https://codereview.chromium.org/2126583002/). Since all its metadata are stored in the WebAPK's AndroidManifest.xml, so there is no need to fetch metadata like theme color etc. during launch the WebAPK. Revert the logic back. BUG=624834 ==========
Description was changed from ========== Remove logic to use metadata for WebAPK. The logic was introduced in CL (https://codereview.chromium.org/2126583002/). Since all its metadata are stored in the WebAPK's AndroidManifest.xml, so there is no need to fetch metadata like theme color etc. during launch the WebAPK. Revert the logic back. BUG=624834 ========== to ========== Remove the logic to fetch metadata when launching WebAPK. The logic was introduced in CL (https://codereview.chromium.org/2126583002/). Since all its metadata are stored in the WebAPK's AndroidManifest.xml, so there is no need to fetch metadata like theme color etc. during launch the WebAPK. Revert the logic back. BUG=624834 ==========
Description was changed from ========== Remove the logic to fetch metadata when launching WebAPK. The logic was introduced in CL (https://codereview.chromium.org/2126583002/). Since all its metadata are stored in the WebAPK's AndroidManifest.xml, so there is no need to fetch metadata like theme color etc. during launch the WebAPK. Revert the logic back. BUG=624834 ========== to ========== Remove the logic of fetching metadata when launching WebAPKs. The logic was introduced in CL (https://codereview.chromium.org/2126583002/). Since all its metadata are stored in the WebAPK's AndroidManifest.xml, so there is no need to fetch metadata like theme color etc. during launch the WebAPK. Revert the logic back. BUG=624834 ==========
hanxi@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi Peter, could you please take a look? Thanks!
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
LGTM with nits 🗻 https://codereview.chromium.org/2335843002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2335843002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:52: // cleared Chrome's data. When it is launching again, we know that the WebAPK is still Nit: launching -> launched https://codereview.chromium.org/2335843002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2335843002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:274: Nit: Can you please make this function private and rename it to: private void updateStorage(WebappDataStorage storage) and move lines 291 - 297 into the caller initializeWebappData() WebApkActivity#onStorageIsNull() can call WebappActivity#initializeSplashScreenWidgets() with a null bitmap
Patchset #2 (id:40001) has been deleted
hanxi@chromium.org changed reviewers: + dfalcantara@chromium.org, dominickn@chromium.org
Hi dominickn@ and dfalcantara@: Please take a look, thanks! https://codereview.chromium.org/2335843002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2335843002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:52: // cleared Chrome's data. When it is launching again, we know that the WebAPK is still On 2016/09/13 15:25:47, pkotwicz wrote: > Nit: launching -> launched Done. https://codereview.chromium.org/2335843002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2335843002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:274: On 2016/09/13 15:25:47, pkotwicz wrote: > Nit: Can you please make this function private and rename it to: > private void updateStorage(WebappDataStorage storage) > > and move lines 291 - 297 into the caller initializeWebappData() > > WebApkActivity#onStorageIsNull() can call > WebappActivity#initializeSplashScreenWidgets() with a null bitmap As discussed offline, move line 291 - 297 to initializeSplashScreenWidgets(int). The |updateStorage| has to be protected, since it is called by WebApkActivity too.
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2335843002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2335843002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:63: initializeSplashScreenWidgets(backgroundColor, null); It looks to me that WebAPKs no longer retrieve the splash screen image here, when they used to. Is that behaviour correct?
Hi Dominick, ptal, thanks! https://codereview.chromium.org/2335843002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2335843002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:63: initializeSplashScreenWidgets(backgroundColor, null); On 2016/09/15 05:20:43, dominickn wrote: > It looks to me that WebAPKs no longer retrieve the splash screen image here, > when they used to. Is that behaviour correct? Yes, that is a trade off that we made to simplify the registration of WebApk in WebappRegistry. The downloading of the splash screen image happens before the WebAPK's package name is available. If we want to use it in the first launch, we need to cache the image, register the WebAPK when it's installed (before the first launch) and delete the cached image if it's not installed. Lots of complexity is introduced. So we made a trade off that we don't cache the image and register WebAPK during the first launch.
On 2016/09/15 12:44:53, Xi Han wrote: > Hi Dominick, ptal, thanks! > > https://codereview.chromium.org/2335843002/diff/60001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java > (right): > > https://codereview.chromium.org/2335843002/diff/60001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:63: > initializeSplashScreenWidgets(backgroundColor, null); > On 2016/09/15 05:20:43, dominickn wrote: > > It looks to me that WebAPKs no longer retrieve the splash screen image here, > > when they used to. Is that behaviour correct? > > Yes, that is a trade off that we made to simplify the registration of WebApk in > WebappRegistry. The downloading of the splash screen image happens before the > WebAPK's package name is available. If we want to use it in the first launch, we > need to cache the image, register the WebAPK when it's installed (before the > first launch) and delete the cached image if it's not installed. Lots of > complexity is introduced. So we made a trade off that we don't cache the image > and register WebAPK during the first launch. Thanks for the explanation, that makes sense. lgtm if you add that comment in WebApkActivity#onStorageIsNull
Added the comments. PTAL, thanks!
lgtm https://codereview.chromium.org/2335843002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2335843002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:65: // image in the SharePreference when the WebAPK is installed SharedPreference https://codereview.chromium.org/2335843002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:67: // installed. Therefore, Lots of complexity will be introduced. To simplify lots
https://codereview.chromium.org/2335843002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2335843002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:65: // image in the SharePreference when the WebAPK is installed On 2016/09/15 17:23:05, dfalcantara wrote: > SharedPreference Done. https://codereview.chromium.org/2335843002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:67: // installed. Therefore, Lots of complexity will be introduced. To simplify On 2016/09/15 17:23:05, dfalcantara wrote: > lots Done.
The CQ bit was checked by hanxi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org, dominickn@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2335843002/#ps100001 (title: "Update comment.")
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 ========== Remove the logic of fetching metadata when launching WebAPKs. The logic was introduced in CL (https://codereview.chromium.org/2126583002/). Since all its metadata are stored in the WebAPK's AndroidManifest.xml, so there is no need to fetch metadata like theme color etc. during launch the WebAPK. Revert the logic back. BUG=624834 ========== to ========== Remove the logic of fetching metadata when launching WebAPKs. The logic was introduced in CL (https://codereview.chromium.org/2126583002/). Since all its metadata are stored in the WebAPK's AndroidManifest.xml, so there is no need to fetch metadata like theme color etc. during launch the WebAPK. Revert the logic back. BUG=624834 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Remove the logic of fetching metadata when launching WebAPKs. The logic was introduced in CL (https://codereview.chromium.org/2126583002/). Since all its metadata are stored in the WebAPK's AndroidManifest.xml, so there is no need to fetch metadata like theme color etc. during launch the WebAPK. Revert the logic back. BUG=624834 ========== to ========== Remove the logic of fetching metadata when launching WebAPKs. The logic was introduced in CL (https://codereview.chromium.org/2126583002/). Since all its metadata are stored in the WebAPK's AndroidManifest.xml, so there is no need to fetch metadata like theme color etc. during launch the WebAPK. Revert the logic back. BUG=624834 Committed: https://crrev.com/b6c54b8210dae027b628164b8655c740b060e9b9 Cr-Commit-Position: refs/heads/master@{#418918} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b6c54b8210dae027b628164b8655c740b060e9b9 Cr-Commit-Position: refs/heads/master@{#418918} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
