Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2759753002:
Fetch manifest icon prior to checking eligibility in InstallableManager (Closed)
DescriptionFetch manifest icon prior to checking eligibility in InstallableManager
Previously primary icons are not fetched for web sites that do not pass
the installability tests. As a result, web sites such as
sensor-compass.appspot.com and superbowl.com, when added to home
screen, have generated icons instead of primary icons provided in their
Web manifests.
This CL makes InstallableManager to fetch manifest icon prior to
checking eligibility for PWA in InstallableManager.
BUG=702233
Review-Url: https://codereview.chromium.org/2759753002
Cr-Commit-Position: refs/heads/master@{#463134}
Committed: https://chromium.googlesource.com/chromium/src/+/f7338b32ab404c8dca728c4b8dabb8b660d5b4ec
Patch Set 1 : Add tests #
Total comments: 2
Patch Set 2 : Fetch manifest icon prior to checking eligibility in InstallableManager #
Messages
Total messages: 31 (24 generated)
Description was changed from ========== Always attempt to fetch primary icon if Web manifest is present. BUG=702233 ========== to ========== Always attempt to fetch primary icon if Web manifest is present. Previously primary icons are not fetched for web sites that do not pass the installability tests. As a result, web sites such as sensor-compass.appspot.com and superbowl.com, when added to home screen, have generated icons instead of primary icons provided in their Web manifests. This CL makes InstallableManager to always attemp to fetch primary icon if Web manifest exists. BUG=702233 ==========
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #1 (id:1) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
zpeng@chromium.org changed reviewers: + dominickn@chromium.org
Hi Dominick, PTAL. Thanks!
Hmmm, right now we deliberately don't fetch the icon if we don't pass the installability check to save the network request and bandwidth. I'd like to talk this over with PMs before approving - will get back to you asap.
Can you clarify the description and subject to be: "Fetch manifest icon prior to checking eligibility in InstallableManager" Strictly speaking, we're not always attempting to fetch the primary icon: we're changing the order in which the test is early-exited such that the icon will be completed before the installable check. https://codereview.chromium.org/2759753002/diff/20001/chrome/browser/installa... File chrome/browser/installable/installable_manager_browsertest.cc (left): https://codereview.chromium.org/2759753002/diff/20001/chrome/browser/installa... chrome/browser/installable/installable_manager_browsertest.cc:381: // START_URL_NOT_VALID since we won't have a cached icon error. Not sure why you removed this test?
Description was changed from ========== Always attempt to fetch primary icon if Web manifest is present. Previously primary icons are not fetched for web sites that do not pass the installability tests. As a result, web sites such as sensor-compass.appspot.com and superbowl.com, when added to home screen, have generated icons instead of primary icons provided in their Web manifests. This CL makes InstallableManager to always attemp to fetch primary icon if Web manifest exists. BUG=702233 ========== to ========== Fetch manifest icon prior to checking eligibility in InstallableManager Previously primary icons are not fetched for web sites that do not pass the installability tests. As a result, web sites such as sensor-compass.appspot.com and superbowl.com, when added to home screen, have generated icons instead of primary icons provided in their Web manifests. This CL makes InstallableManager to fetch manifest icon prior to checking eligibility for PWA in InstallableManager. BUG=702233 ==========
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Thanks Dominick! PTAL https://codereview.chromium.org/2759753002/diff/20001/chrome/browser/installa... File chrome/browser/installable/installable_manager_browsertest.cc (left): https://codereview.chromium.org/2759753002/diff/20001/chrome/browser/installa... chrome/browser/installable/installable_manager_browsertest.cc:381: // START_URL_NOT_VALID since we won't have a cached icon error. On 2017/04/07 04:42:54, dominickn wrote: > Not sure why you removed this test? I removed this because now that primary icon is fetched before the installability check, this test no longer applies. As an alternative, I tweaked this test so it still reports the original error. WDUT?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, thanks
The CQ bit was checked by zpeng@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491664061937390,
"parent_rev": "67f22396e43ea69024f3f158bfcd5ec411042dee", "commit_rev":
"f7338b32ab404c8dca728c4b8dabb8b660d5b4ec"}
Message was sent while issue was closed.
Description was changed from ========== Fetch manifest icon prior to checking eligibility in InstallableManager Previously primary icons are not fetched for web sites that do not pass the installability tests. As a result, web sites such as sensor-compass.appspot.com and superbowl.com, when added to home screen, have generated icons instead of primary icons provided in their Web manifests. This CL makes InstallableManager to fetch manifest icon prior to checking eligibility for PWA in InstallableManager. BUG=702233 ========== to ========== Fetch manifest icon prior to checking eligibility in InstallableManager Previously primary icons are not fetched for web sites that do not pass the installability tests. As a result, web sites such as sensor-compass.appspot.com and superbowl.com, when added to home screen, have generated icons instead of primary icons provided in their Web manifests. This CL makes InstallableManager to fetch manifest icon prior to checking eligibility for PWA in InstallableManager. BUG=702233 Review-Url: https://codereview.chromium.org/2759753002 Cr-Commit-Position: refs/heads/master@{#463134} Committed: https://chromium.googlesource.com/chromium/src/+/f7338b32ab404c8dca728c4b8dab... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001) as https://chromium.googlesource.com/chromium/src/+/f7338b32ab404c8dca728c4b8dab... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
