Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2791923005:
Fails InstallableManager if a selected badge icon cannot be fetched. (Closed)
DescriptionFails InstallableManager if a selected badge icon cannot be fetched.
This CL makes InstallableManager fail when an icon is selected as the
badge icon yet cannot be fetched. For an icon to be selected as a badge
icon, the web developer needs to explicitly add
manifest.icon.purpose.BADGE to the icon's purpose field. Providing a
badge icon is still optional; however, the WebAPK installation will not
proceed if the provided badge icon cannot be downloaded.
This CL gets rid of Clank's manifest protocol buffer's dependency on
the network condition.
BUG=649771
Review-Url: https://codereview.chromium.org/2791923005
Cr-Commit-Position: refs/heads/master@{#464452}
Committed: https://chromium.googlesource.com/chromium/src/+/6f3ee4aef9982d1c5579bd78ac12895ffc161f45
Patch Set 1 #Patch Set 2 : Correct the error of making badge icon required #
Total comments: 4
Patch Set 3 : Addressing comments #
Total comments: 2
Patch Set 4 : Rebase #Patch Set 5 : Addressing comments #
Messages
Total messages: 44 (32 generated)
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 ========== Fails InstallableManager if a selected badge icon cannot be fetched. BUG=649771 ========== to ========== Fails InstallableManager if a selected badge icon cannot be fetched. This CL makes InstallableManager fail when an icon is selected as the badge icon yet cannot be fetched. For an icon to be selected as a badge icon, the web developer needs to explicitly add manifest.icon.purpose.BADGE to the icon's purpose field. Providing a badge icon is still optional; however, the WebAPK installation will not proceed if the provided badge icon cannot be downloaded. This CL gets rid of Clank's manifest protocol buffer's dependency on the network condition. BUG=649771 ==========
zpeng@chromium.org changed reviewers: + dominickn@chromium.org
Hi Dominick, PTAL. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
pkotwicz@chromium.org changed reviewers: + pkotwicz@chromium.org
Does this mean that when WebAPKs are enabled, sites *must* specify a badge icon to pass the installability test? Both AppBannerManagerAndroid and AddToHomescreenDataFetcher set |fetch_valid_badge_icon| to true when WebAPKs are enabled, and that means InstallableManager explicitly looks for an icon with IconPurpose::BADGE. Icons with no purpose default to ANY, so nothing will pass unless explicitly mandated. Changing requirements like this means that we need to have this check in the PWA checklist and in Lighthouse before adding them in the client to ensure that we're clear about what the goalposts are for being a PWA / WebAPKable. If the badge icon is going to be mandatory, we should also modify the logic so that there are badge-specific error codes in installable_logging.h/cc so that developers don't get confused.
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...
Sorry for uploading a CL that does not do what the CL description says. I've corrected this and please take a look. Previous code change does make the badge icon required and is not the right change to send out.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Pretty close now. :) https://codereview.chromium.org/2791923005/diff/20001/chrome/browser/installa... File chrome/browser/installable/installable_manager.cc (right): https://codereview.chromium.org/2791923005/diff/20001/chrome/browser/installa... chrome/browser/installable/installable_manager.cc:183: return icon.error; Can you edit the following error messages in installable_logging so they're more generic? kCannotDownloadIconMessage -> "could not download a required icon from the manifest" kNoIconAvailableMessage -> "downloaded manifest icon was empty or corrupted" https://codereview.chromium.org/2791923005/diff/20001/chrome/browser/installa... chrome/browser/installable/installable_manager.cc:459: if (is_required) I misinterpreted why you did it this way the first time I read this. Instead of having is_required here, can you amend the conditional in GetErrorCode() to be: // If the error is NO_ACCEPTABLE_ICON, there is no icon suitable as a badge in the // manifest. Ignore this case since we only want to fail the check if there was a // suitable badge icon specified and we couldn't fetch it. if (icon.error != NO_ERROR_DETECTED && icon.error != NO_ACCEPTABLE_ICON) return icon.error; That way, we don't have the misleading NO_ERROR_DETECTED - we just ignore the error when it's caused by a suitable badge icon not being present.
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! PTAL https://codereview.chromium.org/2791923005/diff/20001/chrome/browser/installa... File chrome/browser/installable/installable_manager.cc (right): https://codereview.chromium.org/2791923005/diff/20001/chrome/browser/installa... chrome/browser/installable/installable_manager.cc:183: return icon.error; On 2017/04/05 02:57:24, dominickn wrote: > Can you edit the following error messages in installable_logging so they're more > generic? > > kCannotDownloadIconMessage -> "could not download a required icon from the > manifest" > > kNoIconAvailableMessage -> "downloaded manifest icon was empty or corrupted" Done. https://codereview.chromium.org/2791923005/diff/20001/chrome/browser/installa... chrome/browser/installable/installable_manager.cc:459: if (is_required) On 2017/04/05 02:57:24, dominickn wrote: > I misinterpreted why you did it this way the first time I read this. > > Instead of having is_required here, can you amend the conditional in > GetErrorCode() to be: > > // If the error is NO_ACCEPTABLE_ICON, there is no icon suitable as a badge in > the > // manifest. Ignore this case since we only want to fail the check if there was > a > // suitable badge icon specified and we couldn't fetch it. > if (icon.error != NO_ERROR_DETECTED && icon.error != NO_ACCEPTABLE_ICON) > return icon.error; > > That way, we don't have the misleading NO_ERROR_DETECTED - we just ignore the > error when it's caused by a suitable badge icon not being present. Done. This is a much better solution!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
lgtm, thanks! https://codereview.chromium.org/2791923005/diff/40001/chrome/browser/installa... File chrome/browser/installable/installable_logging.cc (right): https://codereview.chromium.org/2791923005/diff/40001/chrome/browser/installa... chrome/browser/installable/installable_logging.cc:53: "could not download a required icon from manifest"; Nit: "the manifest"
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Thanks Dominick! PTAL https://codereview.chromium.org/2791923005/diff/40001/chrome/browser/installa... File chrome/browser/installable/installable_logging.cc (right): https://codereview.chromium.org/2791923005/diff/40001/chrome/browser/installa... chrome/browser/installable/installable_logging.cc:53: "could not download a required icon from manifest"; On 2017/04/07 00:02:34, dominickn wrote: > Nit: "the manifest" Done.
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.
still lgtm
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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.
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": 80001, "attempt_start_ts": 1492102462670520,
"parent_rev": "c2da0d044f43d8edd68fba266ed0a63f39c0ce4a", "commit_rev":
"6f3ee4aef9982d1c5579bd78ac12895ffc161f45"}
Message was sent while issue was closed.
Description was changed from ========== Fails InstallableManager if a selected badge icon cannot be fetched. This CL makes InstallableManager fail when an icon is selected as the badge icon yet cannot be fetched. For an icon to be selected as a badge icon, the web developer needs to explicitly add manifest.icon.purpose.BADGE to the icon's purpose field. Providing a badge icon is still optional; however, the WebAPK installation will not proceed if the provided badge icon cannot be downloaded. This CL gets rid of Clank's manifest protocol buffer's dependency on the network condition. BUG=649771 ========== to ========== Fails InstallableManager if a selected badge icon cannot be fetched. This CL makes InstallableManager fail when an icon is selected as the badge icon yet cannot be fetched. For an icon to be selected as a badge icon, the web developer needs to explicitly add manifest.icon.purpose.BADGE to the icon's purpose field. Providing a badge icon is still optional; however, the WebAPK installation will not proceed if the provided badge icon cannot be downloaded. This CL gets rid of Clank's manifest protocol buffer's dependency on the network condition. BUG=649771 Review-Url: https://codereview.chromium.org/2791923005 Cr-Commit-Position: refs/heads/master@{#464452} Committed: https://chromium.googlesource.com/chromium/src/+/6f3ee4aef9982d1c5579bd78ac12... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/6f3ee4aef9982d1c5579bd78ac12... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
