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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

(can't ask review from bz atm)

Comment 3

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Oops, https://bugzilla.mozilla.org/show_bug.cgi?id=1268077#c2 is the context here.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5583f4e00aa7
EventListenerWasAdded/Remove doesn't work in JS implemented webidl because of missing 'override', r=bz

Comment 8

[Andrew Swan [:aswan]]

(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!

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5583f4e00aa7

Comment 10

[Sylvestre Ledru [:Sylvestre]]

we would need an uplift request too :)
thank

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c6c037a5f3bd

Comment 12

[Andrew Swan [:aswan]]

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

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8757622 [details] [diff] [review]
patch

Thanks, approving for posterity