Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2747353002:
Make BeforeInstallPromptEvent cancelable (Closed)
|
Created:
3 years, 9 months ago by dominickn Modified:
3 years, 7 months ago Reviewers:
Matt Giuca CC:
chromium-reviews, dominickn+watch_chromium.org, mlamouri+watch-blink_chromium.org, haraken, pkotwicz+watch_chromium.org, blink-reviews, zpeng+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis aligns the event to the spec at
https://www.w3.org/TR/appmanifest/#dfn-steps-to-notify-before-an-automated-install-prompt
BUG=658640
Review-Url: https://codereview.chromium.org/2747353002
Cr-Commit-Position: refs/heads/master@{#470854}
Committed: https://chromium.googlesource.com/chromium/src/+/d864c36cdca36cf097210057df806da44d98291a
Patch Set 1 #
Total comments: 3
Patch Set 2 : Comments #
Messages
Total messages: 25 (17 generated)
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
dominickn@chromium.org changed reviewers: + mgiuca@chromium.org
PTAL (sanity check), thanks!
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.
Note: I am deliberately waiting until the updates to the spec are pushed out before we bother to change the Chrome implementation (to avoid churn if there are changes to the spec draft).
https://codereview.chromium.org/2747353002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp (right): https://codereview.chromium.org/2747353002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp:62: DOMException::create(NotAllowedError, This is technically a breaking change (though NOBODY should be relying on it). I've sent an email to the team asking whether we can make such a change without an Intent email.
With the following change removed, we can finally land this. Thanks for your patience. There are probably more changes incoming to bring Chrome in line with the spec (see https://docs.google.com/document/d/1ONRGrtTqZj0mKWhVWJRWic9UfmsAvuZmy76s4-75y...), but we don't really have an action plan, so let's not block on this. Go ahead and land. Update the title of this CL to not say "align to spec" since it's far from matching the spec. Just say made the event cancelable. https://codereview.chromium.org/2747353002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp (right): https://codereview.chromium.org/2747353002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp:62: DOMException::create(NotAllowedError, On 2017/04/26 05:28:50, Matt Giuca wrote: > This is technically a breaking change (though NOBODY should be relying on it). > > I've sent an email to the team asking whether we can make such a change without > an Intent email. Per https://crbug.com/658639, this bug was a mistake (it was originally speced as InvalidStateError) so you can remove this change. Actually the spec no longer calls for ANY exception here. I've asked if it's a mistake on https://github.com/w3c/manifest/issues/577, but in the mean time, let's just keep the Chrome behaviour as-is. This means you should update the CL description to not mention changing the exception type, and remove the reference to bug 658639.
Description was changed from ========== Align the BeforeInstallPromptEvent to the proposed spec. This CL makes the BeforeInstallPromptEvent: - cancelable - throw NotAllowedError instead of InvalidStateError if untrusted This aligns the event with the proposed spec at https://github.com/w3c/manifest/pull/520/files. BUG=658639,658640 ========== to ========== BUG=658640 ==========
Description was changed from ========== BUG=658640 ========== to ========== This aligns the event to the just-landed spec at https://www.w3.org/TR/appmanifest/#beforeinstallpromptevent-interface BUG=658640 ==========
dominickn@chromium.org changed reviewers: + haraken@chromium.org
Thanks! https://codereview.chromium.org/2747353002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp (right): https://codereview.chromium.org/2747353002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/app_banner/BeforeInstallPromptEvent.cpp:62: DOMException::create(NotAllowedError, On 2017/05/11 01:42:05, Matt Giuca wrote: > On 2017/04/26 05:28:50, Matt Giuca wrote: > > This is technically a breaking change (though NOBODY should be relying on it). > > > > I've sent an email to the team asking whether we can make such a change > without > > an Intent email. > > Per https://crbug.com/658639, this bug was a mistake (it was originally speced > as InvalidStateError) so you can remove this change. > > Actually the spec no longer calls for ANY exception here. I've asked if it's a > mistake on https://github.com/w3c/manifest/issues/577, but in the mean time, > let's just keep the Chrome behaviour as-is. > > This means you should update the CL description to not mention changing the > exception type, and remove the reference to bug 658639. Done.
Your CL includes a gigantic rebase (Blink rename) along with the changes... please upload those separately in the future. CL description: Maybe drop "just-landed", and change the URL to: https://www.w3.org/TR/appmanifest/#dfn-steps-to-notify-before-an-automated-in... (which is the section that contains the cancelable requirement). LGTM
Description was changed from ========== This aligns the event to the just-landed spec at https://www.w3.org/TR/appmanifest/#beforeinstallpromptevent-interface BUG=658640 ========== to ========== This aligns the event to the spec at https://www.w3.org/TR/appmanifest/#dfn-steps-to-notify-before-an-automated-in... BUG=658640 ==========
dominickn@chromium.org changed reviewers: - haraken@chromium.org
The CQ bit was checked by dominickn@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_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 dominickn@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": 20001, "attempt_start_ts": 1494479262072710,
"parent_rev": "32a2d84e17b0b63b284f259260accc8dd765d26b", "commit_rev":
"d864c36cdca36cf097210057df806da44d98291a"}
Message was sent while issue was closed.
Description was changed from ========== This aligns the event to the spec at https://www.w3.org/TR/appmanifest/#dfn-steps-to-notify-before-an-automated-in... BUG=658640 ========== to ========== This aligns the event to the spec at https://www.w3.org/TR/appmanifest/#dfn-steps-to-notify-before-an-automated-in... BUG=658640 Review-Url: https://codereview.chromium.org/2747353002 Cr-Commit-Position: refs/heads/master@{#470854} Committed: https://chromium.googlesource.com/chromium/src/+/d864c36cdca36cf097210057df80... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/d864c36cdca36cf097210057df80... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
