Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2031213004:
Upstream: Create WebAPK when user selects "Add to Home screen" (Closed)
DescriptionUpstream: Create WebAPK when user selects "Add to Home screen"
This CL makes "Add to Home Screen" create a WebAPK when a user runs Chrome with
the --enable-webapk command line flag and:
- Selects "Add to Home screen" from the app menu for a web app compatible app
OR
- Selects "Add to Home screen" from "Add to Home screen" infobar
BUG=609122
Committed: https://crrev.com/671478be7e3f071f73cb790d74d5484ff4ddce2c
Cr-Commit-Position: refs/heads/master@{#398706}
Patch Set 1 #
Total comments: 5
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Messages
Total messages: 29 (11 generated)
pkotwicz@chromium.org changed reviewers: + hanxi@chromium.org
Xi, can you please take a look? This CL depends on https://codereview.chromium.org/2038983002/
https://codereview.chromium.org/2031213004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2031213004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:157: apkBuilder.buildWebApkAsync(url, GURLUtils.getOrigin(url), shortName, icon); We could do the encoding for the "icon" here, and just pass in a String.
Xi, can you please take another look? https://codereview.chromium.org/2031213004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2031213004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:157: apkBuilder.buildWebApkAsync(url, GURLUtils.getOrigin(url), shortName, icon); We could. I think that the fact that buildWebApkAsync() sends an intent in an implementation detail so I would rather not. In the final version, buildWebApkAsync() will send data to a server and not send an intent
I am thinking that the WebAPK Minting Server will need the web manifest file, so eventually we need to plumb the URL of web manifest to the ShortcutHelper#addShourtcut. It could be in a following up CL. lgtm https://codereview.chromium.org/2031213004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2031213004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:157: apkBuilder.buildWebApkAsync(url, GURLUtils.getOrigin(url), shortName, icon); On 2016/06/06 21:29:24, pkotwicz wrote: > We could. I think that the fact that buildWebApkAsync() sends an intent in an > implementation detail so I would rather not. In the final version, > buildWebApkAsync() will send data to a server and not send an intent You are right, we should keep the Bitmap.
pkotwicz@chromium.org changed reviewers: + dfalcantara@chromium.org
dfalcantara@ can you please take a look?
1) Is this always supposed to hijack adding URLs to the home screen? 2) Can you guys please start filling out your CL descriptions? Giving a one-liner saying that you're upstreaming something doesn't help me understand what you've already done in your private repo.
Talked with Sam to figure out what you're doing. Your CL description doesn't reflect what your code is doing. 1) This only creates WebAPKs when users try to add a web app capable site to their home screen. 2) This doesn't affect _just_ the menu, it affects app install banners, too. They go through the exact same pathway. https://chromiumcodereview.appspot.com/2031213004/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkBuilder.java (right): https://chromiumcodereview.appspot.com/2031213004/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkBuilder.java:16: * @param scope: Navigations from the WebAPK to URLs with sub-origin {@link scope} will stay indent this so that "within" is aligned with "Navigations" https://chromiumcodereview.appspot.com/2031213004/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkBuilder.java:21: public void buildWebApkAsync(String startUrl, String scope, String appName, Bitmap appIcon); interface methods are automatically public. Remove that.
Description was changed from ========== Upstream: Create WebAPK when user selects "Add to Home screen" from app menu BUG=609122 ========== to ========== Upstream: Create WebAPK when user selects "Add to Home screen" from app menu This CL makes "Add to Home Screen" create a WebAPK when a user runs Chrome with the --enable-webapk command line flag and: - Selects "Add to Home screen" from the app menu for a web app compatible app OR - Selects "Add to Home screen" from "Add to Home screen" infobar BUG=609122 ==========
dfalcantara@ can you please take another look? I will try to make CL descriptions more descriptive in the future. The purpose of this CL is to enable the "add to homescreen" menu item in the app menu to create web apks. There is followup to this CL. In particular: - There are additional checks to determine whether a web-app compatible website is WebAPK compatible. For instance, for a Web Manifest to be WebAPK compatible, the start_url field in the Web Manifest must be defined and must https - The "Add to Homescreen" in the webapp "Add to Homescreen" infobar should be replaced with "Install" if the site is WebAPK compatible
Thanks for updating the description, but the subject is still misleading. Update that and I'll l-g-t-m. https://codereview.chromium.org/2031213004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkBuilder.java (right): https://codereview.chromium.org/2031213004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkBuilder.java:15: * @param startUrl: The url that the WebAPK should navigate to when launched from the app list. Javadocs don't add colons after argument names
dfalcantara@: What do you suggest that I change the title to? I am having trouble coming up with a good short title https://codereview.chromium.org/2031213004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkBuilder.java (right): https://codereview.chromium.org/2031213004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkBuilder.java:15: * @param startUrl: The url that the WebAPK should navigate to when launched from the app list. On 2016/06/07 22:59:43, dfalcantara wrote: > Javadocs don't add colons after argument names Done.
Create WebAPK when user selects "Add to Home screen"? I'm objecting to the specificity if the menu being mentioned. On Jun 7, 2016 7:06 PM, <pkotwicz@chromium.org> wrote: > dfalcantara@: What do you suggest that I change the title to? I am having > trouble coming up with a good short title > > > > https://codereview.chromium.org/2031213004/diff/20001/chrome/android/java/src... > File > > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkBuilder.java > (right): > > > https://codereview.chromium.org/2031213004/diff/20001/chrome/android/java/src... > > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkBuilder.java:15: > * @param startUrl: The url that the WebAPK should navigate to when > launched from the app list. > On 2016/06/07 22:59:43, dfalcantara wrote: > > Javadocs don't add colons after argument names > > Done. > > https://codereview.chromium.org/2031213004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== Upstream: Create WebAPK when user selects "Add to Home screen" from app menu This CL makes "Add to Home Screen" create a WebAPK when a user runs Chrome with the --enable-webapk command line flag and: - Selects "Add to Home screen" from the app menu for a web app compatible app OR - Selects "Add to Home screen" from "Add to Home screen" infobar BUG=609122 ========== to ========== Upstream: Create WebAPK when user selects "Add to Home screen" This CL makes "Add to Home Screen" create a WebAPK when a user runs Chrome with the --enable-webapk command line flag and: - Selects "Add to Home screen" from the app menu for a web app compatible app OR - Selects "Add to Home screen" from "Add to Home screen" infobar BUG=609122 ==========
dfalcantara@ can you please take another look? I have changed the CL title as requested
You missed the "Subject" in the form, so it was still the original name. Did it for you. lgtm.
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by pkotwicz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hanxi@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2031213004/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2031213004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_clang on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by pkotwicz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2031213004/60001
Message was sent while issue was closed.
Description was changed from ========== Upstream: Create WebAPK when user selects "Add to Home screen" This CL makes "Add to Home Screen" create a WebAPK when a user runs Chrome with the --enable-webapk command line flag and: - Selects "Add to Home screen" from the app menu for a web app compatible app OR - Selects "Add to Home screen" from "Add to Home screen" infobar BUG=609122 ========== to ========== Upstream: Create WebAPK when user selects "Add to Home screen" This CL makes "Add to Home Screen" create a WebAPK when a user runs Chrome with the --enable-webapk command line flag and: - Selects "Add to Home screen" from the app menu for a web app compatible app OR - Selects "Add to Home screen" from "Add to Home screen" infobar BUG=609122 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Upstream: Create WebAPK when user selects "Add to Home screen" This CL makes "Add to Home Screen" create a WebAPK when a user runs Chrome with the --enable-webapk command line flag and: - Selects "Add to Home screen" from the app menu for a web app compatible app OR - Selects "Add to Home screen" from "Add to Home screen" infobar BUG=609122 ========== to ========== Upstream: Create WebAPK when user selects "Add to Home screen" This CL makes "Add to Home Screen" create a WebAPK when a user runs Chrome with the --enable-webapk command line flag and: - Selects "Add to Home screen" from the app menu for a web app compatible app OR - Selects "Add to Home screen" from "Add to Home screen" infobar BUG=609122 Committed: https://crrev.com/671478be7e3f071f73cb790d74d5484ff4ddce2c Cr-Commit-Position: refs/heads/master@{#398706} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/671478be7e3f071f73cb790d74d5484ff4ddce2c Cr-Commit-Position: refs/heads/master@{#398706} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
