Closed Bug 1276424 Opened 3 years ago Closed 3 years ago

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

Categories

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

36 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

Details

(Whiteboard: btpp-active)

Attachments

(1 file)

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.
Attached patch patchSplinter Review
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.
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)
https://hg.mozilla.org/mozilla-central/rev/5583f4e00aa7
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
we would need an uplift request too :)
thank
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+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.