EventListenerWasAdded/Remove doesn't work in JS implemented webidl because of missing 'override'

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

36 Branch
mozilla49
Points:
---

Firefox Tracking Flags

(firefox48 fixed, firefox49 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(1 attachment)

This used to work just fine, but looks like we've become more strict with 'override' usage, and so the generated code isn't working anymore.
Created attachment 8757622 [details] [diff] [review]
patch

bz, is this too ugly approach?
This is so special case, for now at least, that adding a new annotation or anything doesn't quite make sense.


My test interface 
[JSImplementation="@mozilla.org/test/event-target;1"]
interface TestEventTarget : EventTarget {
  [ChromeOnly]
  void eventListenerWasAdded(DOMString aType);
  [ChromeOnly]
  void eventListenerWasRemoved(DOMString aType);

  void fooBarMethod();
};

get the following methods
  virtual void EventListenerWasAdded(const nsAString& aType, ErrorResult& aRv, JSCompartment* aCompartment = nullptr) override;

  virtual void EventListenerWasRemoved(const nsAString& aType, ErrorResult& aRv, JSCompartment* aCompartment = nullptr) override;

  void FooBarMethod(ErrorResult& aRv, JSCompartment* aCompartment = nullptr);





aswan, want to try this patch?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(aswan)
(can't ask review from bz atm)
> and so the generated code isn't working anymore

When you say "not working" does that mean "doesn't compile" or something else?  I'm assuming it doesn't compile because it overrides without saying so?
Whiteboard: btpp-active
Comment on attachment 8757622 [details] [diff] [review]
patch

This is probably fine.  If we wanted to, we could add an extended attribute for this, just to prevent issues with other interfaces that use these method names, unlikely as that is.

r=me, in any case.
Flags: needinfo?(bzbarsky)
Attachment #8757622 - Flags: review+
well, that should be very unlikely, which is why I took this quick and dirty approach.
If we'll add more this kind of methods, then sure, some annotation in webidl hinting that the js implementation method should have virtual and override.

Comment 7

3 years ago
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5583f4e00aa7
EventListenerWasAdded/Remove doesn't work in JS implemented webidl because of missing 'override', r=bz
(In reply to Olli Pettay [:smaug] from comment #1)
> aswan, want to try this patch?

confirmed that it appears to fix the problem I ran into, thanks!
Flags: needinfo?(aswan)

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5583f4e00aa7
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
status-firefox48: --- → affected
we would need an uplift request too :)
thank

Comment 11

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/c6c037a5f3bd
status-firefox48: affected → fixed
Comment on attachment 8757622 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
This bug blocks bug 1268077 which was approved for uplift, there is a discussion there about the impact.

[Describe test coverage new/current, TreeHerder]:
try run for this bug plus 1268077 against beta: https://treeherder.mozilla.org/#/jobs?repo=try&revision=09967b7d9658

[Risks and why]: 
see discussion in bug 1268077

[String/UUID change made/needed]:
none
Attachment #8757622 - Flags: approval-mozilla-beta?
Comment on attachment 8757622 [details] [diff] [review]
patch

Thanks, approving for posterity
Attachment #8757622 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.