Is there any reason not to?

http://dxr.mozilla.org/mozilla-central/source/widget/nsGUIEvent.h#l113 http://dxr.mozilla.org/mozilla-central/source/widget/nsGUIEvent.h#l677 Currently defined as a uint32_t

Enum should probably work. In theory that limits certain kinds of binary addons which might add new types of event structs, but I haven't heard of any such addon.

Several places still use static_cast. One is the mId in EventNameMapping. I'm not sure how is it used. The others are in the interfaces defined in XPIDL, they accept uint32_t instead of the enum. The name nsGUIMessage should be improved since in many places the variable are named aEventType but message.

Comment on attachment 778361 [details] [diff] [review] WIP patch The name of the enum should not be nsGUIMessage. Perhaps mozilla::EventMessageType or mozilla::EventMessage. Couldn't you just change mapping's mId to be type of this new thing? We really shouldn't be static_casting. The naming should change. All thos NS_* event messages should become eFooBar or eEventType_FooBar. what is the sizeof a member variable which has type of this new enum? nsEventListenerManager has an optimization and the sizeof mNoListenerForEvent must not be more than 23 bits.

Many of the ns*Event types are not in the mozilla namespace. Should we fix this? Otherwise we have to use mozilla::EventMessageType in those declarations :(

Is there something wrong with using mozilla:: in those ns*Events?

Looks like that nobody works on this. Can I take this when I have time to fix this? Making event messages an enum help debug on Visual Studio.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #7) > Looks like that nobody works on this. Can I take this when I have time to > fix this? Making event messages an enum help debug on Visual Studio. Please take it :) I thought this as part of the Events refactoring effort but didn't find time to finish it.

Okay. Taking.

Renaming WidgetEvent::message -> WidgetEvent::mMessage. # I'd like to land patches separately because there will be a lot of patches to rename NS_* to eEventMessage_*.

I'll make the enum named enum, and rename each message name in following patches (next week).

And the patch breaks the 80 column per line rule, but please ignore that because each line will be modified when I rename it.

Comment on attachment 8651003 [details] [diff] [review] part.2 Define event messages as anonymous enum with EventMessageList.h The patch is a bit odd looking. For some reason first tons of stuff is removed, then the same thing copied and modified... I guess hg isn't clever enough to create better looking patch. >+enum >+{ >+ >+#define NS_EVENT_MESSAGE(aMessage, aValue) aMessage = aValue, >+ >+#include "mozilla/EventMessageList.h" >+ >+#undef NS_EVENT_MESSAGE >+ >+ // For preventing bustage due to "," after the last item. >+ eEventMessage_MaxValue >+}; It would be nice if we could assert here or in ELM that aValue is always <= ((1 << 22) - 1) so that mNoListenerForEvent can handle the value. But that can be done in a separate patch.

url: https://hg.mozilla.org/integration/mozilla-inbound/rev/bb91cf98a42b08db96677dd8bc3ebe5218945318 changeset: bb91cf98a42b08db96677dd8bc3ebe5218945318 user: Masayuki Nakano <masayuki@d-toybox.com> date: Sat Aug 22 10:34:51 2015 +0900 description: Bug 895274 part.1 Rename WidgetEvent::message to WidgetEvent::mMessage r=smaug url: https://hg.mozilla.org/integration/mozilla-inbound/rev/756c006944ab770d06841c63a5d0f00616d3d41e changeset: 756c006944ab770d06841c63a5d0f00616d3d41e user: Masayuki Nakano <masayuki@d-toybox.com> date: Sat Aug 22 10:34:51 2015 +0900 description: Bug 895274 part.2 Define event messages as anonymous enum with EventMessageList.h r=smaug

https://hg.mozilla.org/mozilla-central/rev/bb91cf98a42b https://hg.mozilla.org/mozilla-central/rev/756c006944ab

It seems that we should use enum class instead of named enum. However, for using it, we need to replace all items of the enum. It's difficult to do it for now. Even if you like enum class better, we should use named enum for now. After we will add unique prefix like "eEventMessage_" at renaming each item, we can replace all items easier. So, if you like enum class better, please let me know. I'll do that after renaming all event messages. Therefore, this patch just makes the enum of event messages named enum, "mozilla::EventMessage". And the type of this enum can be uint16_t rather than uint32_t. This patch also checks the size of EventListenerManager's size at build time with static_assert(). For dosing that, I separated the integer members to EventListenerManagerBase. And also this patch names all variables of EventMessage as "EventMesage", "Message" or "Msg". Additionally, this patch adds a lot of "default: break;" to the switch statements since they are necessary for avoiding warnings. I'll post this patch with -w.

FYI: I don't change nsBaseDragService::FireDragEventAtSource()'s argument since it's a scriptable interface method. We should make it non-scriptable or expose the concrete class later.

Now, we shouldn't call the event message as ID.

url: https://hg.mozilla.org/integration/mozilla-inbound/rev/cd06f5f5641c3dfb7f07db878584d389fb4df1b7 changeset: cd06f5f5641c3dfb7f07db878584d389fb4df1b7 user: Masayuki Nakano <masayuki@d-toybox.com> date: Wed Aug 26 21:56:59 2015 +0900 description: Bug 895274 part.3 Make the enum of event messages a named enum IGNORE IDL r=smaug url: https://hg.mozilla.org/integration/mozilla-inbound/rev/5dc1087883b0c3685006722dbee0995c815ac4c1 changeset: 5dc1087883b0c3685006722dbee0995c815ac4c1 user: Masayuki Nakano <masayuki@d-toybox.com> date: Wed Aug 26 21:56:59 2015 +0900 description: Bug 895274 part.4 Clean up the macro names of EventNameList.h r=smaug

What's name do you like for each event message? I think normal event messages should be: eEventMessage_KeyDown, eEventMessage_MouseClick, eEventMessage_MouseEnterWidget, eEventMessage_MouseEnter, On the other hand, the "start" and "end" messages should be "first" and "last" like the messages of Windows. start and end could be used by explaining operations, e.g., "drag start". So, I think they should be: eEventMessage_KeyFirst, eEventMessage_KeyLast, eEventMessage_MouseFirst, eEventMessage_MouseLast, Do you have some better ideas?

Do we need eEventMessage_* ? Why not just eKeyDown etc? first and last sound good.

(In reply to Olli Pettay [:smaug] (high review load) from comment #30) > Do we need eEventMessage_* ? > Why not just eKeyDown etc? Oh, does it OK? You don't think that we should make them enum class for stricter type check? If we should do that, common prefix helps me to replace with EventMessage::eKeyDown or something. (But if we don't need to do that, shorter name is better!)

Well, does the longer name really help with type check? I think not. So I prefer shorter names, since those are still very clear.

(In reply to Olli Pettay [:smaug] (high review load) from comment #32) > Well, does the longer name really help with type check? I think not. So I > prefer shorter names, since those are still very clear. Okay, then, we should keep using named enum and shorter item names. I'll create some patches tomorrow.

https://hg.mozilla.org/mozilla-central/rev/cd06f5f5641c https://hg.mozilla.org/mozilla-central/rev/5dc1087883b0

FYI: I realized that we don't have any specific rule of naming enum items... https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

(In reply to Olli Pettay [:smaug] (high review load) from comment #32) > Well, does the longer name really help with type check? Oops, I missed this point. If we use enum class, we have following merits: We can reduce "e" prefix because each item name is valid only under the class name. So, e.g., EventMessage::Null won't conflict with anything. enum class allows to use forward declaration. Therefore, header files don't need to include the defined header file only for a method which has EventMessage as a result type or an argument. And enum class doesn't allow following code: uint32_t foo = enumclass::bar; // error at build time So, when we need to use to set to/get from integer, we need to cast explicitly.

In other words, shorter names are used for us, but they may conflict with other enum items if we name some of them with too generic name.

ah, conflicts are possible, though with e-prefix hopefully unlikely.

Hmm, I feel following names are odd: NS_KEY_BEFORE_DOWN NS_KEY_AFTER_DOWN NS_KEY_BEFORE_UP NS_KEY_AFTER_UP They are named as "mozbrowser(before|after)key(down|up). And the class name is InternalBeforeAfterKeyboardEvent. So, in the current idea, they should be e(Before|After)Key(Down|Up)...

I guess that if we change the enum to enum class, the longest name will be probably EventMessage::QuerySelectionAsTransferable. It's 8 characters longer than NS_QUERY_SELECTION_AS_TRANSFERABLE.

Hmm, I was hoping e-prefixed names, and without EventMessage:: everywhere. And yes, those before/after names could be changed.

Okay, then, I'll keep yesterday's rules.

Er, *was* hoping?

I think that message indicates the meaning of message. Then, void is better than null.

Looks like that this is an event fired at closing window (not DOM window). Therefore, I don't think "XUL" is necessary keyword for this.