Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1487763003:
Expose NotificationEvent.action (Closed)
DescriptionExpose NotificationEvent.action
This is needed to complete the shipping of action buttons.
Seems like it was accidentally not included in #358304.
BUG=513671
Committed: https://crrev.com/ea938a33d6fc21a8d39764ef335031a4a2f87615
Cr-Commit-Position: refs/heads/master@{#362414}
Patch Set 1 #Patch Set 2 : with -expected.txt #
Dependent Patchsets: Messages
Total messages: 22 (8 generated)
b.kelemen@samsung.com changed reviewers: + peter@chromium.org
I think this is missing for action buttons.
johnme@chromium.org changed reviewers: + johnme@chromium.org
lgtm, thanks for spotting this! I'll take care of cherry-picking it to m48.
The CQ bit was checked by johnme@chromium.org
peter@chromium.org changed reviewers: - johnme@chromium.org
Thank you so much for finding this!! This is a really bad one we missed out on :/ lgtm I think you'll need to modify an -expected.txt for Service Worker interface listings as well. Could you please run layout tests and run/update the following test? LayoutTests/http/tests/serviceworker/webexposed/global-interface-listing-service-worker.html
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1487763003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1487763003/1
johnme@chromium.org changed reviewers: + johnme@chromium.org, philipj@opera.com
Hi Philip, this already passed intent to ship[1], but got accidentally omitted when the rest of the feature was enabled[2]. Please review quickly, so we can merge this to m48 - thanks :) [1]: https://groups.google.com/a/chromium.org/d/topic/blink-dev/8iSlPpwokbY/discus... [2]: https://codereview.chromium.org/1412223007
There's also NotificationEventInit.action. Looking at that IDL file I see that notification is not required, which means that the resulting NotificationEvent's notification attribute can be null, even though the attribute isn't nullable in the IDL. That's for a separate CL, but better fix that ASAP as well. lgtm to land this with NotificationEventInit.action fix.
On 2015/12/01 12:25:42, philipj wrote: > There's also NotificationEventInit.action. > > Looking at that IDL file I see that notification is not required, which means > that the resulting NotificationEvent's notification attribute can be null, even > though the attribute isn't nullable in the IDL. That's for a separate CL, but > better fix that ASAP as well. > > lgtm to land this with NotificationEventInit.action fix. Thanks! Yes, I also noticed NotificationEventInit, and am fixing that in https://codereview.chromium.org/1489773003/. Though RuntimeEnabled is actually ignored on dictionaries (see https://crbug.com/546006), so the one on NotificationEventInit never had any effect anyway (so doesn't need to be merged).
The CQ bit was checked by johnme@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, johnme@chromium.org Link to the patchset: https://codereview.chromium.org/1487763003/#ps20001 (title: "with -expected.txt")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1487763003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1487763003/20001
On 2015/12/01 12:36:24, johnme wrote: > On 2015/12/01 12:25:42, philipj wrote: > > There's also NotificationEventInit.action. > > > > Looking at that IDL file I see that notification is not required, which means > > that the resulting NotificationEvent's notification attribute can be null, > even > > though the attribute isn't nullable in the IDL. That's for a separate CL, but > > better fix that ASAP as well. > > > > lgtm to land this with NotificationEventInit.action fix. > > Thanks! Yes, I also noticed NotificationEventInit, and am fixing that in > https://codereview.chromium.org/1489773003/. Though RuntimeEnabled is actually > ignored on dictionaries (see https://crbug.com/546006), so the one on > NotificationEventInit never had any effect anyway (so doesn't need to be > merged). https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin... tl;dr - that's not the case. NotificationEventInit.hasAction() will always be false.
On 2015/12/01 12:39:22, Peter Beverloo wrote: > On 2015/12/01 12:36:24, johnme wrote: > > On 2015/12/01 12:25:42, philipj wrote: > > > There's also NotificationEventInit.action. > > > > > > Looking at that IDL file I see that notification is not required, which > means > > > that the resulting NotificationEvent's notification attribute can be null, > > even > > > though the attribute isn't nullable in the IDL. That's for a separate CL, > but > > > better fix that ASAP as well. > > > > > > lgtm to land this with NotificationEventInit.action fix. > > > > Thanks! Yes, I also noticed NotificationEventInit, and am fixing that in > > https://codereview.chromium.org/1489773003/. Though RuntimeEnabled is actually > > ignored on dictionaries (see https://crbug.com/546006), so the one on > > NotificationEventInit never had any effect anyway (so doesn't need to be > > merged). > > https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin... > > tl;dr - that's not the case. NotificationEventInit.hasAction() will always be > false. Looks like [RuntimeEnabled=NotificationExperimental] in NotificationEventInit.idl actually does change the generated code. johnme@, will you drop [RuntimeEnabled=NotificationExperimental] from there as well?
On 2015/12/01 13:15:26, philipj wrote: > On 2015/12/01 12:39:22, Peter Beverloo wrote: > > On 2015/12/01 12:36:24, johnme wrote: > > > On 2015/12/01 12:25:42, philipj wrote: > > > > There's also NotificationEventInit.action. > > > > > > > > Looking at that IDL file I see that notification is not required, which > > means > > > > that the resulting NotificationEvent's notification attribute can be null, > > > even > > > > though the attribute isn't nullable in the IDL. That's for a separate CL, > > but > > > > better fix that ASAP as well. > > > > > > > > lgtm to land this with NotificationEventInit.action fix. > > > > > > Thanks! Yes, I also noticed NotificationEventInit, and am fixing that in > > > https://codereview.chromium.org/1489773003/. Though RuntimeEnabled is > actually > > > ignored on dictionaries (see https://crbug.com/546006), so the one on > > > NotificationEventInit never had any effect anyway (so doesn't need to be > > > merged). > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin... > > > > tl;dr - that's not the case. NotificationEventInit.hasAction() will always be > > false. > > Looks like [RuntimeEnabled=NotificationExperimental] in > NotificationEventInit.idl actually does change the generated code. > > johnme@, will you drop [RuntimeEnabled=NotificationExperimental] from there as > well?
On 2015/12/01 13:34:36, johnme wrote: > On 2015/12/01 13:15:26, philipj wrote: > > On 2015/12/01 12:39:22, Peter Beverloo wrote: > > > On 2015/12/01 12:36:24, johnme wrote: > > > > On 2015/12/01 12:25:42, philipj wrote: > > > > > There's also NotificationEventInit.action. > > > > > > > > > > Looking at that IDL file I see that notification is not required, which > > > means > > > > > that the resulting NotificationEvent's notification attribute can be > null, > > > > even > > > > > though the attribute isn't nullable in the IDL. That's for a separate > CL, > > > but > > > > > better fix that ASAP as well. > > > > > > > > > > lgtm to land this with NotificationEventInit.action fix. > > > > > > > > Thanks! Yes, I also noticed NotificationEventInit, and am fixing that in > > > > https://codereview.chromium.org/1489773003/. Though RuntimeEnabled is > > actually > > > > ignored on dictionaries (see https://crbug.com/546006), so the one on > > > > NotificationEventInit never had any effect anyway (so doesn't need to be > > > > merged). > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin... > > > > > > tl;dr - that's not the case. NotificationEventInit.hasAction() will always > be > > > false. > > > > Looks like [RuntimeEnabled=NotificationExperimental] in > > NotificationEventInit.idl actually does change the generated code. > > > > johnme@, will you drop [RuntimeEnabled=NotificationExperimental] from there as > > well? Yes, that's what https://codereview.chromium.org/1489773003/ is for. Based on what Peter pointed out, I'll merge that to m48 as well.
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Expose NotificationEvent.action This is needed to complete the shipping of action buttons. Seems like it was accidentally not included in #358304. BUG=513671 ========== to ========== Expose NotificationEvent.action This is needed to complete the shipping of action buttons. Seems like it was accidentally not included in #358304. BUG=513671 Committed: https://crrev.com/ea938a33d6fc21a8d39764ef335031a4a2f87615 Cr-Commit-Position: refs/heads/master@{#362414} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ea938a33d6fc21a8d39764ef335031a4a2f87615 Cr-Commit-Position: refs/heads/master@{#362414} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
