Open Bug 1295098 Opened 8 years ago Updated 2 years ago

WidgetMouseEventBase::buttonType should be an enum class in mozilla::

Categories

(Core :: DOM: UI Events & Focus Handling, defect, P3)

defect

Tracking

()

Tracking Status
firefox51 --- affected

People

(Reporter: masayuki, Unassigned)

References

(Blocks 1 open bug)

Details

WidgetMouseEventBase::buttonType is too long to use it in our coding rules (80 characters per line rule). And now, we can use enum class. So, I think that we can redefine it as enum class for type safer like: namespace mozilla { typdef uint8 MouseButtonType; enum class MouseButton { eLeft, eMiddle, eRight, e4th, e5th, eUnknown }; } // namespace mozilla in mozilla/EventForwards.h Additionally, we should make WidgetMouseEventBase::button a private member (but dom::MouseEvent class should be a friend class to access the raw value because JS can set its value to any value) and Create |MouseButton Button() const| which casts from integer to MouseButton. Then, any default event handlers in C++ can check the button value safer. Smaug, I'd like to use this bug in a hacker event in Mozilla Japan at the end of this month. Do you have any suggestion or objection for this bug?
Flags: needinfo?(bugs)
Hmm, how would this work in case JS initiates mouse button, say with number 100 ? Would we need to everywhere explicitly check whether casting type back to MouseButton is safe?
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #1) > Hmm, how would this work in case JS initiates mouse button, say with number > 100 ? > Would we need to everywhere explicitly check whether casting type back to > MouseButton is safe? No, I think that if the raw value isn't 0-4, WidgetMouseButtonBase::Button() should return MouseButton::eUnknown. When a default event handler of MouseEvent handles new button, the author should define new button with the enum class. (I assume that nobody handles unknown button in default event handlers.)
But what would MouseEvent.button return in JS then?
(In reply to Olli Pettay [:smaug] from comment #3) > But what would MouseEvent.button return in JS then? Then, dom::MouseEvent just returns its raw value which is stored in WidgetMouseEvent::button (int16_t).
Priority: -- → P2
Oh, I see. C++ callers would use MouseButton Button() const method, which hides all the casting. Ok, sounds reasonable.
Exactly. Thank you for your time.
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) from comment #0) > Smaug, I'd like to use this bug in a hacker event in Mozilla Japan at the > end of this month. Do you have any suggestion or objection for this bug? Did the event happen? If so, I guess this bug didn't get included :)
Flags: needinfo?(masayuki)
Priority: P2 → P3
Yeah, this required a lot of code change than as I expected. I'll take this when I have much time.
Flags: needinfo?(masayuki)
Component: Event Handling → User events and focus handling
Summary: WidgetMosueEventBase::buttonType should be an enum class in mozilla:: → WidgetMouseEventBase::buttonType should be an enum class in mozilla::
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.