Closed Bug 920425 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Widget, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Whiteboard: See comment 65 if you find regression of this.)

Attachments

(30 files, 8 obsolete files)

4.55 KB, patch
roc
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
26.06 KB, patch
roc
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
26.36 KB, patch
smaug
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
2.15 KB, patch
smaug
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
1.85 KB, patch
smaug
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
2.31 KB, patch
smaug
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
7.02 KB, patch
smaug
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
2.24 KB, patch
smaug
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
7.97 KB, patch
smaug
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
5.82 KB, patch
smaug
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
5.71 KB, patch
smaug
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
6.72 KB, patch
smaug
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
1.34 KB, patch
smaug
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
3.85 KB, patch
smaug
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
19.94 KB, patch
smaug
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
7.25 KB, patch
smaug
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
1.06 KB, patch
smaug
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
41.73 KB, patch
smaug
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
5.43 KB, patch
smaug
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
7.28 KB, patch
smaug
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
4.05 KB, patch
smaug
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
2.18 KB, patch
smaug
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
3.21 KB, patch
smaug
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
7.63 KB, patch
smaug
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
12.25 KB, patch
smaug
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
16.46 KB, patch
smaug
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
30.31 KB, patch
smaug
: review+
masayuki
: checkin+
Details | Diff | Splinter Review
22.06 KB, patch
smaug
: review+
Details | Diff | Splinter Review
10.64 KB, patch
smaug
: review+
Details | Diff | Splinter Review
106.61 KB, patch
smaug
: review+
Details | Diff | Splinter Review
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?
Sounds good. Best to implement these as virtual methods that return nullptr in the base class and are overridden in subclasses to return 'this'.
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.
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?
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.
Ah, but override methods need to call its super class's same method until the most super class.
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.
Oops, there is nsEventNameList.h, so, we need constant values for each class.
First, let's making a class event list for defining a lot of enumerators and methods which needs for all event classes.
Attachment #810565 - Attachment is obsolete: true
Attachment #813947 - Flags: review?(roc)
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.
Attachment #813949 - Flags: review?(roc)
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.
Attachment #813952 - Flags: review?(roc)
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.
(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)
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().
(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;|).
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.
Attachment #813947 - Attachment is obsolete: true
Attachment #813949 - Attachment is obsolete: true
Attachment #813952 - Attachment is obsolete: true
Attachment #813947 - Flags: review?(roc)
Attachment #813949 - Flags: review?(roc)
Attachment #813952 - Flags: review?(roc)
Attachment #814863 - Flags: review?(roc)
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.
Attachment #814865 - Flags: review?(roc)
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.
Attachment #814866 - Flags: review?(roc)
Whiteboard: [leave open]
(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 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"
Attachment #814863 - Flags: review?(roc) → review+
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; }
Attachment #814865 - Flags: review?(roc) → review-
(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
Attachment #814865 - Attachment is obsolete: true
Attachment #814866 - Attachment is obsolete: true
Attachment #814866 - Flags: review?(roc)
Attachment #818263 - Flags: review?(roc)
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.
Attachment #818266 - Flags: review?(bugs)
Attachment #818270 - Attachment is obsolete: true
Attachment #818270 - Flags: review?(bugs)
Attachment #818274 - Flags: review?(bugs)
(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.
Attachment #818266 - Flags: review?(bugs) → review+
Attachment #818267 - Flags: review?(bugs) → review+
Attachment #818269 - Flags: review?(bugs) → review+
Attachment #818271 - Flags: review?(bugs) → review+
Attachment #818272 - Flags: review?(bugs) → review+
Attachment #818274 - Flags: review?(bugs) → review+
Attachment #818275 - Flags: review?(bugs) → review+
Attachment #818276 - Flags: review?(bugs) → review+
Attachment #818277 - Flags: review?(bugs) → review+
Attachment #818278 - Flags: review?(bugs) → review+
Attachment #818279 - Flags: review?(bugs) → review+
Attachment #818280 - Flags: review?(bugs) → review+
Attachment #818281 - Flags: review?(bugs) → review+
Attachment #818284 - Flags: review?(bugs) → review+
Attachment #818285 - Flags: review?(bugs) → review+
Comment on attachment 818287 [details] [diff] [review]
part.18 Use mozilla::WidgetEvent::AsKeyboardEvent()

s/defualt/default/
Attachment #818287 - Flags: review?(bugs) → review+
Attachment #818288 - Flags: review?(bugs) → review+
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();
Attachment #818289 - Flags: review?(bugs) → review+
Attachment #818290 - Flags: review?(bugs) → review+
Comment on attachment 818292 [details] [diff] [review]
part.22 Use mozilla::WidgetEvent::AsSelectionEvent()

WidgetSelectionEvent *selectionEvent ->
WidgetSelectionEvent* selectionEvent
Attachment #818292 - Flags: review?(bugs) → review+
Attachment #818293 - Flags: review?(bugs) → review+
Attachment #818294 - Flags: review?(bugs) → review+
Attachment #818295 - Flags: review?(bugs) → review+
Comment on attachment 818297 [details] [diff] [review]
part.26 Use mozilla::WidgetEvent::AsMouseEventBase()

s/mosueEventBase/mouseEventBase/
Attachment #818297 - Flags: review?(bugs) → review+
Attachment #818298 - Flags: review?(bugs) → review+
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
Attachment #818299 - Flags: review?(bugs) → review-
Attachment #818300 - Flags: review?(bugs) → review+
Attachment #818301 - Flags: review?(bugs) → review+
(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!
Flags: needinfo?(bugs)
I feel that the code looks redundant.
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.
Flags: needinfo?(bugs)
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.
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.
Whiteboard: [leave open] → [leave open] See comment 65 if you find regression of this.
For focusing the original purpose of this bug, I removed the new methods.
Attachment #818299 - Attachment is obsolete: true
Attachment #819002 - Flags: review?(bugs)
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 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
Attachment #819002 - Flags: review?(bugs) → review+
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.
Whiteboard: [leave open] See comment 65 if you find regression of this. → See comment 65 if you find regression of this.
You need to log in before you can comment on or make changes to this bug.