920425 - WidgetEvent should have As*Event() methods for capsuling event struct check and static cast

Status: RESOLVED FIXED

(Core :: Widget, enhancement)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Currently, there are a lot of code which do:

if (aEvent->eventStructType == NS_FOO_EVENT) {
  nsFooEvent* fooEvent = static_cast<nsFooEvent>(aEvent);
  fooEvent->bar = baz;
}

Especially, if the checking type is a common event class such as WidgetInputEvent, then, the condition becomes complicated. Additionally, static_cast<>() isn't useful for our coding rules (<= 80 chars per line).

I'd like to suggest that WidgetEvent should have As*Event() methods. For example,

WidgetInputEvent* AsInputEvent() const
{
  switch (eventStructType) {
    case NS_INPUT_EVENT:
    case NS_KEY_EVENT:
    case NS_MOUSE_EVENT:
    ...
      return this;
    default:
      return nullptr;
  }
}

So, this can get rid of all struct type check methods of WidgetEvent because e.g.,

if (aEvent->AsKeyboardEvent()) {}

can check the event is WidgetKeyboardEvent (or its derived class) by this method.

Any ideas?

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sounds good. Best to implement these as virtual methods that return nullptr in the base class and are overridden in subclasses to return 'this'.

Comment 2

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

Hmm, virtualness would make it a bit slower, and more important would add virtual table to
event structs. Not quite what I'd like to see.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's not clear a single virtual call would be slower than a lot of conditional branches or a switch on eventStructType.

What's the concern about virtual tables on event structs?

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Since there is no virtual methods in any event classes, I believed that it's a rule about event classes.

I don't think that virtual call makes damage to the performance because it shouldn't be called so many times.

However, I'm not sure if creating vtable is a problem or not.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, but override methods need to call its super class's same method until the most super class.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Okay, I think that we should discard event struct IDs. Instead of that, we should manage it with bit field. Then, we can manage/maintain it easier.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oops, there is nsEventNameList.h, so, we need constant values for each class.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Create widget/EventClassList.h

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Perhaps, this approach is the best.

https://hg.mozilla.org/try/rev/7455563185be

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Draft patches:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=c01ae89d65ab

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Create widget/EventClassList.h

First, let's making a class event list for defining a lot of enumerators and methods which needs for all event classes.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 Rename nsEventStructType to mozilla::EventClassID and create mozilla::WidgetEvent::IsDerivedFrom() for checking if the event class is derived from a class ID specified by the argument

Let's define mozilla::EventClassID which replaces nsEventStructType. Then, let's use ClassEventList.h. I'll rename each value of it in next bug.

Then, we can create super class database with ClassEventList.h (See gEventSuperClassID in WidgetEventImpl.cpp).

Finally, we can check if an event class instance has which super class. See WidgetEvent::IsDerivedFrom(). The for loop isn't long loop. It may climb the super class database 5 times in the worst case (WidgetDragEvent). Therefore, I believe that this cost is enough cheap.

If this would make damage to the performance, we should create the database with bitfield.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.3 Implement mozilla::WidgetEvent::As*Event() methods

This patch implements As*Event() with EventClassList.h.

The methods have optional argument aStrictMatch. It's default value is false. If it's true, they don't check super class database. This argument is necessary in some cases actually.

I've already created patches which replaces all static_cast with As*Event() methods. However, it might be broken during these 3 patches' review and I need to fix bug 920377 first (I'm waiting the blocker patch's landing since it breaks my patch in local repository will be broken).

After that, I'll post 27 patches this bug. And they should be reviewed by sumaug, I think.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't understand why we're doing it this way instead of using virtual methods like the Layer class has. This way is more complicated and I don't believe it's faster.

Also the "aStrictMatch" feature is odd. Please explain why it is needed.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> I don't understand why we're doing it this way instead of using virtual
> methods like the Layer class has. This way is more complicated and I don't
> believe it's faster.

I don't familiar with layar class. I guess that you think that WidgetEvent should have all As*Event() methods which return false and each sub class should have an As*Event() which returns true?

If nobody don't mind the classes become having vtable, I don't mind it.

FYI: In a lot of cases of call of As*Event() don't climb the array. In that case, it's faster than using virtual method. I guess the cost isn't problem, tough.

> Also the "aStrictMatch" feature is odd. Please explain why it is needed.

In some points, I see like following code.

if (aEvent->eventStructType == NS_MOUSE_EVENT) {
  foo = static_cast<WidgetMouseEvent*>(aEvent)->foo.
}

WidgetMouseEvent is inherited by WidgetDragEvent. So, in this case, As*Event(true) is necessary. If you don't like the bool param because it's not clear what the value means if you see such callers, it might be better to create separated methods. (I have no idea of good name for them, though)

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

And this case:

if (aEvent->eventStructType == NS_MOUSE_EVENT) {
  delete static_cast<WidgetMouseEvent*>(mEvent);
}

However, the later case should be managed by a method later such as WidgetEvent::Destroy().

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #16)
> And this case:
> 
> if (aEvent->eventStructType == NS_MOUSE_EVENT) {
>   delete static_cast<WidgetMouseEvent*>(mEvent);
> }
> 
> However, the later case should be managed by a method later such as
> WidgetEvent::Destroy().

Oops, this can be gone since if WidgetEvent becomes having vtable (i.e., we can do |delete mEvent;|).

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Create widget/EventClassList.h

I removed NS_EVENT_RELATED_STRUCT() from EventClassList.h because it's only necessary in EventForwards.h.

Instead, this defines NS_ROOT_EVENT_CLASS() for WidgetEvent. In following patches, we often don't want to do anything for WidgetEvent. Then, it's useful.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 Implement mozilla::WidgetEvent::As*Event() methods

This patch implements As*Event() as virtual methods.

By this, WidgetEvent now has vtable. Therefore, DOM event classes don't need to check the event class of mEvent at deleting it. Therefore, this patch also removes the unnecessary destructors of DOM event classes.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.3 Add aStrictMatch argument to As*Event()

This patch adds a param to As*Events(). This is necessary on 6 points in current m-c. So, if you really don't like this, and you don't have any idea for good name for creating new methods instead of adding the param, we can drop this change.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #15)
> I don't familiar with layar class. I guess that you think that WidgetEvent
> should have all As*Event() methods which return false and each sub class
> should have an As*Event() which returns true?

Right.

> In some points, I see like following code.
> 
> if (aEvent->eventStructType == NS_MOUSE_EVENT) {
>   foo = static_cast<WidgetMouseEvent*>(aEvent)->foo.
> }
> 
> WidgetMouseEvent is inherited by WidgetDragEvent. So, in this case,
> As*Event(true) is necessary. If you don't like the bool param because it's
> not clear what the value means if you see such callers, it might be better
> to create separated methods. (I have no idea of good name for them, though)

For this case, wouldn't it make more sense to check the event message value and then call AsMouseEvent()? Or possibly in the other order? Basically the code is trying to process some mouse events and ignore some other mouse events.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 814863 [details] [diff] [review]
part.1 Create widget/EventClassList.h

Review of attachment 814863 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/EventClassList.h
@@ +5,5 @@
> +
> +/**
> + * This header file lists up all event classes and related structs.
> + * Define NS_EVENT_CLASS(aPrefix, aName) and NS_ROOT_EVENT_CLASS(aPrefix, aName)
> + * before inluding this.

"including"

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 814865 [details] [diff] [review]
part.2 Implement mozilla::WidgetEvent::As*Event() methods

Review of attachment 814865 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/BasicEvents.h
@@ +680,5 @@
> +   */
> +#define NS_ROOT_EVENT_CLASS(aPrefix, aName)
> +#define NS_EVENT_CLASS(aPrefix, aName) \
> +  virtual aPrefix##aName* As##aName(); \
> +  virtual const aPrefix##aName* As##aName() const;

Instead of defining a duplicate set of const methods here, I suggest having these methods be non-virtual, cast away const and call the non-const version.

@@ +805,5 @@
> +
> +#define NS_DECL_COMMON_EVENT_CLASS_METHODS(aPrefix, aName) \
> +public: \
> +  virtual const aPrefix##aName* As##aName() const; \
> +  virtual aPrefix##aName* As##aName();

With the above change, I don't think we need this macro anymore. It's easy enough to just declare and implement the As method in one go:
  virtual WidgetGUIEvent* AsWidgetGUIEvent() MOZ_OVERRIDE { return this; }

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 Implement mozilla::WidgetEvent::As*Event() methods

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23)
> Comment on attachment 814865 [details] [diff] [review]
> part.2 Implement mozilla::WidgetEvent::As*Event() methods
> 
> Review of attachment 814865 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/BasicEvents.h
> @@ +680,5 @@
> > +   */
> > +#define NS_ROOT_EVENT_CLASS(aPrefix, aName)
> > +#define NS_EVENT_CLASS(aPrefix, aName) \
> > +  virtual aPrefix##aName* As##aName(); \
> > +  virtual const aPrefix##aName* As##aName() const;
> 
> Instead of defining a duplicate set of const methods here, I suggest having
> these methods be non-virtual, cast away const and call the non-const version.

Nice idea!

> @@ +805,5 @@
> > +
> > +#define NS_DECL_COMMON_EVENT_CLASS_METHODS(aPrefix, aName) \
> > +public: \
> > +  virtual const aPrefix##aName* As##aName() const; \
> > +  virtual aPrefix##aName* As##aName();
> 
> With the above change, I don't think we need this macro anymore. It's easy
> enough to just declare and implement the As method in one go:
>   virtual WidgetGUIEvent* AsWidgetGUIEvent() MOZ_OVERRIDE { return this; }

Hmm, I worry about that it's possible to take simple mistake in the future especially at implementing new event class. And also it might be useful at adding new methods in the future.

However, at least for now, it's not matter.

I separated the previous part.2 patch to:

new part.2: Implementing As*Event() methods.
new part.3: Sorting out the code which delete event class instance

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.3 We don't need to use static_cast at releasing event class instance since WidgetEvent has virtual destructor now

Now, we can remove a lot of |delete static_cast<Widget*Event*>(mEvent);| because WidgetEvent and its derived classes have virtual destructor. So, e.g.,:

WidgetEvent* event = new WidgetMouseEvent();
delete event;

should be safe.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.4 Use mozilla::WidgetEvent::AsScriptErrorEvent()

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.5 Use mozilla::WidgetEvent::AsScrollPortEvent()

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.6 Use mozilla::WidgetEvent::AsScrollAreaEvent()

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.7 Use mozilla::WidgetEvent::AsFormEvent()

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.8 Use mozilla::WidgetEvent::AsClipboardEvent()

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.6 Use mozilla::WidgetEvent::AsScrollAreaEvent()

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.9 Use mozilla::WidgetEvent::AsFocusEvent()

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.10 Use mozilla::WidgetEvent::AsTransitionEvent()

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.11 Use mozilla::WidgetEvent::AsAnimationEvent()

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.12 Use mozilla::WidgetEvent::AsMutationEvent()

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.13 Use mozilla::WidgetEvent::AsContentCommandEvent()

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.14 Use mozilla::WidgetEvent::AsCommandEvent()

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.15 Use mozilla::WidgetEvent::AsTouchEvent()

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.16 Use mozilla::WidgetEvent::AsSimpleGestureEvent()

Comment 40

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.17 Use mozilla::WidgetEvent::AsGestureNotifyEvent()

Comment 41

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.18 Use mozilla::WidgetEvent::AsKeyboardEvent()

Comment 42

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.19 Use mozilla::WidgetEvent::AsTextEvent()

Comment 43

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.20 Use mozilla::WidgetEvent::AsCompositionEvent()

Comment 44

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.21 Use mozilla::WidgetEvent::AsQueryContentEvent()

Comment 45

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.22 Use mozilla::WidgetEvent::AsSelectionEvent()

Comment 46

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.23 Use mozilla::WidgetEvent::AsUIEvent()

Comment 47

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.24 Use mozilla::WidgetEvent::AsMouseScrollEvent()

Comment 48

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.25 Use mozilla::WidgetEvent::AsDragEvent()

Comment 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.26 Use mozilla::WidgetEvent::AsMouseEventBase()

Comment 50

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.27 Use mozilla::WidgetEvent::AsInputEvent()

Comment 51

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.28 Use mozilla::WidgetEvent::AsMouseEvent()

Comment 52

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.29 Use mozilla::WidgetEvent::AsGUIEvent()

Comment 53

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.30 Use mozilla::WidgetEvent::AsWheelEvent()

Comment 54

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #21)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #15)

> > In some points, I see like following code.
> > 
> > if (aEvent->eventStructType == NS_MOUSE_EVENT) {
> >   foo = static_cast<WidgetMouseEvent*>(aEvent)->foo.
> > }
> > 
> > WidgetMouseEvent is inherited by WidgetDragEvent. So, in this case,
> > As*Event(true) is necessary. If you don't like the bool param because it's
> > not clear what the value means if you see such callers, it might be better
> > to create separated methods. (I have no idea of good name for them, though)
> 
> For this case, wouldn't it make more sense to check the event message value
> and then call AsMouseEvent()? Or possibly in the other order? Basically the
> code is trying to process some mouse events and ignore some other mouse
> events.

Hmm, I'm not sure... Checking message might be better in some points, but might not be so in the other points...

Anyway, creating Is*Event() method might be useful if we make shorter code and hide the struct type check (it's too raw, I don't like there are such code). Even if so, it should be done in another bug.

Comment 55

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

Comment on attachment 818287 [details] [diff] [review]
part.18 Use mozilla::WidgetEvent::AsKeyboardEvent()

s/defualt/default/

Comment 56

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

Comment on attachment 818289 [details] [diff] [review]
part.20 Use mozilla::WidgetEvent::AsCompositionEvent()

While you're there:

WidgetCompositionEvent *compositionEvent = aEvent->AsCompositionEvent();
->
idgetCompositionEvent* compositionEvent = aEvent->AsCompositionEvent();

Comment 57

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

Comment on attachment 818292 [details] [diff] [review]
part.22 Use mozilla::WidgetEvent::AsSelectionEvent()

WidgetSelectionEvent *selectionEvent ->
WidgetSelectionEvent* selectionEvent

Comment 58

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

Comment on attachment 818297 [details] [diff] [review]
part.26 Use mozilla::WidgetEvent::AsMouseEventBase()

s/mosueEventBase/mouseEventBase/

Comment 59

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

Comment on attachment 818299 [details] [diff] [review]
part.28 Use mozilla::WidgetEvent::AsMouseEvent()

Really odd to see IsLeftMouseButtonDownEvent etc in WidgetEvent.
Those methods should be in the WidgetMouseEvent

Comment 60

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #59)
> Comment on attachment 818299 [details] [diff] [review]
> part.28 Use mozilla::WidgetEvent::AsMouseEvent()
> 
> Really odd to see IsLeftMouseButtonDownEvent etc in WidgetEvent.
> Those methods should be in the WidgetMouseEvent

I think it's not odd since they return false when event class instance isn't a WidgetMouseEvent.

If we move them to WidgetMouseEvent, the callers need to do:

WidgetMouseEvent* mouseEvent = aEvent->AsMouseEvent();
if (mouseEvent && mouseEvent->IsFooEvent() && ... )

these simple work by themselves. Is it OK?

I think that implementing them in WidgetEvent is smarter especially for callers.

Anyway, thanks a lot for your quick review!

Comment 61

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I feel that the code looks redundant.

Comment 62

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

I'm just worried that we end up making the base Event class a monster API containing
all the getter methods and they will just return false if the event type isn't correct.
I don't think we want that.

Comment 63

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Okay, I see.

Comment 64

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ee916cacd47
https://hg.mozilla.org/integration/mozilla-inbound/rev/f20b704a6727
https://hg.mozilla.org/integration/mozilla-inbound/rev/081e788525f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/582c33cea601
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f96402dc1d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c2180e88728
https://hg.mozilla.org/integration/mozilla-inbound/rev/72b0df413e3b
https://hg.mozilla.org/integration/mozilla-inbound/rev/45a99edd7297
https://hg.mozilla.org/integration/mozilla-inbound/rev/9080e47ab89d
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a65548a1556
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a8ad5e27738
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d1be2be411c
https://hg.mozilla.org/integration/mozilla-inbound/rev/30cfb3f452b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/233e5e2db7c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b3805db4d2c
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cdc23bb6bc4
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea4c8fcc6d8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ac236b84f07
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8e43d3e128d
https://hg.mozilla.org/integration/mozilla-inbound/rev/c311f205e805
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc070ecf1a0d
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f95a3a1873c
https://hg.mozilla.org/integration/mozilla-inbound/rev/c607e72bfe0c
https://hg.mozilla.org/integration/mozilla-inbound/rev/31bcb9ec1eff
https://hg.mozilla.org/integration/mozilla-inbound/rev/64764bf05ac2
https://hg.mozilla.org/integration/mozilla-inbound/rev/42dda6ce6ec8
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc72b42bcf76

landed part.1 - part.27.

Comment 65

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

FYI:

If you find a crash bug which is a regression of this bug, it may detect wrong pointer cast before landing the patches. So, if so, we need to fix it on branch too if it's a security bug.

So, please move the bug to security group and cc me.

Comment 66

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.28 Use mozilla::WidgetEvent::AsMouseEvent()

For focusing the original purpose of this bug, I removed the new methods.

Comment 67

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ee916cacd47
https://hg.mozilla.org/mozilla-central/rev/f20b704a6727
https://hg.mozilla.org/mozilla-central/rev/081e788525f9
https://hg.mozilla.org/mozilla-central/rev/582c33cea601
https://hg.mozilla.org/mozilla-central/rev/9f96402dc1d4
https://hg.mozilla.org/mozilla-central/rev/6c2180e88728
https://hg.mozilla.org/mozilla-central/rev/72b0df413e3b
https://hg.mozilla.org/mozilla-central/rev/45a99edd7297
https://hg.mozilla.org/mozilla-central/rev/9080e47ab89d
https://hg.mozilla.org/mozilla-central/rev/5a65548a1556
https://hg.mozilla.org/mozilla-central/rev/2a8ad5e27738
https://hg.mozilla.org/mozilla-central/rev/8d1be2be411c
https://hg.mozilla.org/mozilla-central/rev/30cfb3f452b5
https://hg.mozilla.org/mozilla-central/rev/233e5e2db7c7
https://hg.mozilla.org/mozilla-central/rev/3b3805db4d2c
https://hg.mozilla.org/mozilla-central/rev/8cdc23bb6bc4
https://hg.mozilla.org/mozilla-central/rev/ea4c8fcc6d8f
https://hg.mozilla.org/mozilla-central/rev/1ac236b84f07
https://hg.mozilla.org/mozilla-central/rev/c8e43d3e128d
https://hg.mozilla.org/mozilla-central/rev/c311f205e805
https://hg.mozilla.org/mozilla-central/rev/fc070ecf1a0d
https://hg.mozilla.org/mozilla-central/rev/3f95a3a1873c
https://hg.mozilla.org/mozilla-central/rev/c607e72bfe0c
https://hg.mozilla.org/mozilla-central/rev/31bcb9ec1eff
https://hg.mozilla.org/mozilla-central/rev/64764bf05ac2
https://hg.mozilla.org/mozilla-central/rev/42dda6ce6ec8
https://hg.mozilla.org/mozilla-central/rev/bc72b42bcf76

Comment 68

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

Comment on attachment 819002 [details] [diff] [review]
part.28 Use mozilla::WidgetEvent::AsMouseEvent()


> nsresult
> HTMLImageElement::PreHandleEvent(nsEventChainPreVisitor& aVisitor)
> {
>   // If we are a map and get a mouse click, don't let it be handled by
>   // the Generic Element as this could cause a click event to fire
>   // twice, once by the image frame for the map and once by the Anchor
>   // element. (bug 39723)
>-  if (aVisitor.mEvent->eventStructType == NS_MOUSE_EVENT &&
>-      aVisitor.mEvent->message == NS_MOUSE_CLICK &&
>-      static_cast<WidgetMouseEvent*>(aVisitor.mEvent)->button ==
>-        WidgetMouseEvent::eLeftButton) {
>+  if (aVisitor.mEvent->IsLeftClickEvent()) {
Uh, when this got added to basic Event struct :/
The API is getting odd. Some random stuff is in the base struct, and some
other stuff is not.
Why IsLeftClickEvent() is in the base class, but IsAlt() or IsMeta() aren't?
Some consistency is needed. But not about this bug, please file a new one to sort out the API we want.


>-        const WidgetMouseEvent& mouseEvent =
>-          static_cast<const WidgetMouseEvent&>(anEvent);
>+        const WidgetMouseEvent* mouseEvent = anEvent.AsMouseEvent();
Could you not make this
const WidgetMouseEvent& mouseEvent = *anEvent.AsMouseEvent();

>-      const WidgetMouseEvent& mouseEvent =
>-        static_cast<const WidgetMouseEvent&>(aEvent);
>-      WidgetMouseEvent* outEvent = static_cast<WidgetMouseEvent*>(aOutEvent);
>-      return ProcessMouseEvent(mouseEvent, outEvent);
>+      const WidgetMouseEvent* mouseEvent = aEvent.AsMouseEvent();
ditto

Comment 69

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a03c584f9874
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd05abee0fca
https://hg.mozilla.org/integration/mozilla-inbound/rev/0dc6eccf8c77

(In reply to Olli Pettay [:smaug] from comment #68)
> Comment on attachment 819002 [details] [diff] [review]
> part.28 Use mozilla::WidgetEvent::AsMouseEvent()
> 
> 
> > nsresult
> > HTMLImageElement::PreHandleEvent(nsEventChainPreVisitor& aVisitor)
> > {
> >   // If we are a map and get a mouse click, don't let it be handled by
> >   // the Generic Element as this could cause a click event to fire
> >   // twice, once by the image frame for the map and once by the Anchor
> >   // element. (bug 39723)
> >-  if (aVisitor.mEvent->eventStructType == NS_MOUSE_EVENT &&
> >-      aVisitor.mEvent->message == NS_MOUSE_CLICK &&
> >-      static_cast<WidgetMouseEvent*>(aVisitor.mEvent)->button ==
> >-        WidgetMouseEvent::eLeftButton) {
> >+  if (aVisitor.mEvent->IsLeftClickEvent()) {
> Uh, when this got added to basic Event struct :/
> The API is getting odd. Some random stuff is in the base struct, and some
> other stuff is not.

All macros are moved to WidgetEvent at bug 912956 (part.12, r=roc).

> Why IsLeftClickEvent() is in the base class, but IsAlt() or IsMeta() aren't?

I think that a lot of methods which handle Widget*Event take WidgetEvent, WidgetGUIEvent or WidgetInputEvent for their argument even when they only handles one of their subclass. Therefore, the macro was designed for checking the eventStructType and being able to pass WidgetEvent (nsEvent).

> Some consistency is needed. But not about this bug, please file a new one to
> sort out the API we want.

Well, I *guess* we can sort out the callers' argument. If so, we can move the methods to subclass. I'll check it and file bugs.

Comment 70

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

https://hg.mozilla.org/mozilla-central/rev/a03c584f9874
https://hg.mozilla.org/mozilla-central/rev/cd05abee0fca
https://hg.mozilla.org/mozilla-central/rev/0dc6eccf8c77