386743 - Default event bubbling/cancelable flags should be set in the nsEvent constructor

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

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

Comment 1

[Andrew Overholt [:overholt]]

Is this still valid, Olli?

Comment 2

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

Trying to recall what this was about.
Probably about having a switch-case which then checks event type and based on that
sets the right flags. That way dispatching different events in different places would get consistent handling. So yes, I think still valid.

Comment 3

[Ming-Chou Shih [:stone]]

(In reply to Olli Pettay [:smaug] from comment #2)
> Trying to recall what this was about.
> Probably about having a switch-case which then checks event type and based
> on that
> sets the right flags. That way dispatching different events in different
> places would get consistent handling. So yes, I think still valid.

We're planning to have an enum for all event types and use switch-case to set the right flags, which centralize the logic to set all types of events in the event base class. Is my understanding correct?

Comment 4

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

Yeah, something like that. And this is needed for those event types only which are dispatched using Widget*Event classes on stack. Higher level DOM events need explicit initialization anyhow.

Comment 5

[Ming-Chou Shih [:stone]]

Attachment: Bug 386743: Set default event bubbling/cancelable flags in the WidgetEvent constructor.

Comment 6

[Ming-Chou Shih [:stone]]

Comment on attachment 8741675 [details] [diff] [review]
Bug 386743: Set default event bubbling/cancelable flags in the WidgetEvent constructor.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b390bdb6747f8e5f8acf65f162cb68ad837c0355

Comment 7

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

Sorry about delay, have had high review load. I'll try to get to this tomorrow.

Comment 8

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

Comment on attachment 8741675 [details] [diff] [review]
Bug 386743: Set default event bubbling/cancelable flags in the WidgetEvent constructor.

So I was hoping we could set all the default cancalable/bubbles flags in one place, since currently we do set them in many places where we (widget) events.
But this is the right start for simplifying the setup and making it easier to maintain.

>+  void UpdateFlags() {
Nit, { goes to its own line

And could we call the method something else, since this is only about cancelable and bubbles...
perhaps call it SetDefaultCancelableAndBubbles().

>+    switch (mClass) {
>+      case eEditorInputEventClass:
>+        mFlags.mCancelable = false;
>+        mFlags.mBubbles = mFlags.mIsTrusted;
>+        break;
(not related to this bug, but it is odd that mBubbles depends on mIsTrusted.)

Comment 9

[Ming-Chou Shih [:stone]]

Attachment: Bug 386743: Set default event bubbling/cancelable flags in the WidgetEvent constructor. r=smaug

Follow reviewer's feedbacks to refine codes and change summary.

Comment 10

[Ming-Chou Shih [:stone]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b390bdb6747f8e5f8acf65f162cb68ad837c0355

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfd6e945e298

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dfd6e945e298