Closed Bug 1338059 Opened 7 years ago Closed 7 years ago

implement PromiseRejectionEvent

Categories

(Core :: DOM: Events, defect, P2)

Product:
Core
Component:
DOM: Events
Version:
50 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: 709922234, Assigned: ben.tian)

References

(
URL
)

Details

(Keywords: dev-doc-complete)

Attachments

(3 files)

Bug 1338059 - Part 1: Generate cycle collection related code for WebIDL type Promise,
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1338059 - Part 2: Implement PromiseRejectionEvent,
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
WIP patch w/ test case changes
6.87 KB, patch
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161209095719
Component: Untriaged → DOM: Events
Product: Firefox → Core
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Take this bug.
Assignee: nobody → btian
Hit following TypeError during codegen. Checking whether to revise codegen or it's inapplicable for eventInit dictionary w/ promise.

  File "/Users/bentian/Workspace/central/dom/bindings/Codegen.py", line 96, in idlTypeNeedsCycleCollection
   raise TypeError("Don't know whether to cycle-collect type %s" % type)
TypeError: Don't know whether to cycle-collect type AnyPromise
Comment 3 revises Codegen.py to generate cycle collection code for Promise.
Comment 3 patch causes following build failure on Linux. Checking.

https://treeherder.mozilla.org/logviewer.html#?job_id=93238981&repo=try&lineNumber=8310
(In reply to Ben Tian [:btian] from comment #5)
> Comment 3 patch causes following build failure on Linux. Checking.
> 
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=93238981&repo=try&lineNumber=8310

The attribute getter of |PromiseRejectionEvent.promise| in auto generated header conflicts existing Promise definition as below. The error occurs on Linux and Android only, and disappears if I rename attribute |PromiseRejectionEvent.promise|. Checking for method to avoid such naming problem.

/home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/PromiseRejectionEvent.h:46:22: error: declaration of 'mozilla::dom::Promise* mozilla::dom::PromiseRejectionEvent::Promise() const' [-fpermissive]
  INFO -     Promise* Promise() const;
  INFO -                        ^
  ...
  INFO -  /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/Promise.h:40:7: error: changes meaning of 'Promise' from 'class mozilla::dom::Promise' [-fpermissive]
  INFO -   class Promise : public nsISupports,
(In reply to Ben Tian [:btian] from comment #6)
> The attribute getter of |PromiseRejectionEvent.promise| in auto generated
> header conflicts existing Promise definition as below.

A hacky way that adds "_" prefix if getter name is "Promise" in [1] works. Still looking for better solution.

[1] http://searchfox.org/mozilla-central/source/dom/bindings/Codegen.py#9146
Tried to declare getter as following but still hit the same error:(
-         class Promise* Promise() const;
- mozilla::dom::Promise* Promise() const;

Will work on Codegen.py revision for renaming first.
Will refer to a similar method [1] that avoids method name collision with cpp keywords.

[1] http://searchfox.org/mozilla-central/source/dom/bindings/Codegen.py#8922
Attachment #8860293 - Flags: review?(bzbarsky)
Comment on attachment 8860293 [details]
Bug 1338059 - Part 1: Generate cycle collection related code for WebIDL type Promise,

https://reviewboard.mozilla.org/r/132320/#review138632

::: dom/bindings/Codegen.py:83
(Diff revision 5)
>          type.isString() or
>          type.isAny() or
>          type.isObject() or
>          type.isSpiderMonkeyInterface()):
>          return False
> -    elif type.isCallback() or type.isGeckoInterface():
> +    elif type.isCallback() or type.isPromise() or type.isGeckoInterface():

This should probably be in a separate changeset from the other changes in this patch.

::: dom/bindings/Codegen.py:9149
(Diff revision 5)
>          extendedAttrs = descriptor.getExtendedAttributes(attr, getter=True)
>          canFail = ('infallible' not in extendedAttrs or
>                     'canOOM' in extendedAttrs)
>          if resultOutParam or attr.type.nullable() or canFail:
>              nativeName = "Get" + nativeName
> +        # Avoid name conflict between getter and class Promise (bug 1338059)

Why should this be hardcoded in codegen as opposed to people using [BinaryName] to deal if they need to?  That seems like the right solution in this auto-generation case, for sure.

::: dom/events/test/test_all_synthetic_events.html:291
(Diff revision 5)
>    ProgressEvent:                             { create: function (aName, aProps) {
>                                                           return new ProgressEvent(aName, aProps);
>                                                         },
>                                               },
> +  PromiseRejectionEvent:                     {
> +                                               // Difficult to test required arguments.

Why?  We can just Object.create a new dictionary with aProps as its proto and whatever additional required args we need on it.

::: dom/tests/mochitest/general/test_interfaces.js:784
(Diff revision 5)
>  // IMPORTANT: Do not change this list without review from a DOM peer!
>      "ProcessingInstruction",
>  // IMPORTANT: Do not change this list without review from a DOM peer!
>      "ProgressEvent",
>  // IMPORTANT: Do not change this list without review from a DOM peer!
> +    "PromiseRejectionEvent",

We shouldn't be exposing this interface until we're actually firing the events.

::: dom/workers/test/serviceworkers/test_serviceworker_interfaces.js:189
(Diff revision 5)
>  // IMPORTANT: Do not change this list without review from a DOM peer!
>      { name: "PerformanceObserverEntryList", nightly: true },
>  // IMPORTANT: Do not change this list without review from a DOM peer!
>      "ProgressEvent",
>  // IMPORTANT: Do not change this list without review from a DOM peer!
> +    "PromiseRejectionEvent",

Again, shouldn't be exposed until it's actually implemented.

::: dom/workers/test/test_worker_interfaces.js:183
(Diff revision 5)
>  // IMPORTANT: Do not change this list without review from a DOM peer!
>      { name: "PerformanceObserverEntryList", nightly: true },
>  // IMPORTANT: Do not change this list without review from a DOM peer!
>      "ProgressEvent",
>  // IMPORTANT: Do not change this list without review from a DOM peer!
> +    "PromiseRejectionEvent",

And here.
Attachment #8860293 - Flags: review?(bzbarsky) → review-
[Pref] is required but cannot apply to non-Window-only interface. Need to check whether to implement the event manually (instead of auto-gen) to insert pref function for [Func]. If so I'll discard the Codegen.py change.

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #14)
> ::: dom/tests/mochitest/general/test_interfaces.js:784
> (Diff revision 5)
> >  // IMPORTANT: Do not change this list without review from a DOM peer!
> >      "ProcessingInstruction",
> >  // IMPORTANT: Do not change this list without review from a DOM peer!
> >      "ProgressEvent",
> >  // IMPORTANT: Do not change this list without review from a DOM peer!
> > +    "PromiseRejectionEvent",
> 
> We shouldn't be exposing this interface until we're actually firing the
> events.
> Need to check whether to implement the event manually (instead of auto-gen) to insert pref function for [Func].

You can use [Func] together with autogen.  You just need to put your function somewhere like nsContentUtils.
Or in some other place where it makes sense.  Nothing requires the Func function to live in the implementation class for the interface.
Attachment #8860293 - Flags: review?(bzbarsky)
Will request review for both patches after verifying with try.
(In reply to Ben Tian [:btian] from comment #18)
> Created attachment 8864378 [details]
> Bug 1338059 - Part 2: Implement PromiseRejectionEvent
> 
> Add PromiseRejectionEvent interface, enabled by pref
> "dom.promise_rejection_events.enabled".
> 
> Review commit: https://reviewboard.mozilla.org/r/136058/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/136058/

The latest patch discards related test case revision in comment 13, since interface PromiseRejectionEvent is by default not exposed. We should enable it once actually firing the events.

Attach WIP patch with test case changes as reference for later interface enabling.
Attachment #8864378 - Flags: review?(bzbarsky)
Comment on attachment 8860293 [details]
Bug 1338059 - Part 1: Generate cycle collection related code for WebIDL type Promise,

https://reviewboard.mozilla.org/r/132320/#review139358

r=me
Attachment #8860293 - Flags: review?(bzbarsky) → review+
Comment on attachment 8864378 [details]
Bug 1338059 - Part 2: Implement PromiseRejectionEvent,

https://reviewboard.mozilla.org/r/136058/#review139360

r=me

::: modules/libpref/init/all.js:4948
(Diff revision 2)
>  #endif
>  
>  // W3C pointer events draft
>  pref("dom.w3c_pointer_events.implicit_capture", false);
>  
> +// WHATWG promise rejection events. See

Might be worth a comment about why it's disabled for now, along with a bug number for the remaining work.
Attachment #8864378 - Flags: review?(bzbarsky) → review+
Blocks: 1362272
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #25)
> Might be worth a comment about why it's disabled for now, along with a bug
> number for the remaining work.

Opened bug 1362272 to track remaining work and updated patch in comment 27.
Hi Ben, can you take a look, seems there are open issues so we can't land this via autolander
Flags: needinfo?(btian)
Keywords: checkin-needed
(In reply to Carsten Book [:Tomcat] from comment #30)
> Hi Ben, can you take a look, seems there are open issues so we can't land
> this via autolander

Marked review comments as fixed since they've been addressed in the latest patches (comment 26 and comment 27).
Flags: needinfo?(btian)
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f44e06221afa
Part 1: Generate cycle collection related code for WebIDL type Promise, r=bz
https://hg.mozilla.org/integration/autoland/rev/61af8ff774fe
Part 2: Implement PromiseRejectionEvent, r=bz
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f44e06221afa
https://hg.mozilla.org/mozilla-central/rev/61af8ff774fe
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Chris, PromiseRejectionEvent interface is implemented but now disabled by pref since firing part is not implemented yet (bug 1362272). Should we hold MDN and release note until the firing part is done and pref on?

Also one correction on [1]: PromiseRejectionEvent.promise should be "The Promise which was rejected" as in [2].

[1] https://developer.mozilla.org/en-US/docs/Web/API/PromiseRejectionEvent
[2] https://developer.mozilla.org/en-US/docs/Web/API/PromiseRejectionEvent/PromiseRejectionEvent
Flags: needinfo?(cmills)
(In reply to Ben Tian [:btian] from comment #35)
> Chris, PromiseRejectionEvent interface is implemented but now disabled by
> pref since firing part is not implemented yet (bug 1362272). Should we hold
> MDN and release note until the firing part is done and pref on?
> 
> Also one correction on [1]: PromiseRejectionEvent.promise should be "The
> Promise which was rejected" as in [2].
> 
> [1] https://developer.mozilla.org/en-US/docs/Web/API/PromiseRejectionEvent
> [2]
> https://developer.mozilla.org/en-US/docs/Web/API/PromiseRejectionEvent/
> PromiseRejectionEvent

Thanks for the review Ben.

I've made the fix you highlight here, changed all the pages to say that it is not yet supported in Firefox (disabled behind a pref), removed the release note, and added an entry to our experimental features page:

https://developer.mozilla.org/en-US/Firefox/Experimental_features
Flags: needinfo?(cmills)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: