Closed Bug 1046101 Opened 10 years ago Closed 10 years ago

Redesign nsEventStructType

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(33 files, 1 obsolete file)

107.56 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.77 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.67 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.53 KB, patch
smaug
: review+
Details | Diff | Splinter Review
15.59 KB, patch
smaug
: review+
Details | Diff | Splinter Review
8.47 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.76 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.08 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.65 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.52 KB, patch
smaug
: review+
Details | Diff | Splinter Review
42.32 KB, patch
smaug
: review+
Details | Diff | Splinter Review
22.42 KB, patch
smaug
: review+
Details | Diff | Splinter Review
20.25 KB, patch
smaug
: review+
Details | Diff | Splinter Review
24.38 KB, patch
smaug
: review+
Details | Diff | Splinter Review
20.60 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.80 KB, patch
smaug
: review+
Details | Diff | Splinter Review
21.33 KB, patch
smaug
: review+
Details | Diff | Splinter Review
25.59 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.81 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.06 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.06 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.96 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.92 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.15 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.18 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.21 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.90 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.57 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.26 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.08 KB, patch
smaug
: review+
Details | Diff | Splinter Review
8.57 KB, patch
smaug
: review+
Details | Diff | Splinter Review
24.60 KB, patch
smaug
: review+
Details | Diff | Splinter Review
11.27 KB, patch
smaug
: review+
Details | Diff | Splinter Review
nsEventStructType should be generated from macro of EventClassList.h automatically.

Then, the values will be like:

KeyboardEventClass,
MouseEventClass,

Then, I think nsEventStructType should be renamed to mozilla::EventClassID and also WidgetEvent::eventStructType should be renamed to mClassID.

Any ideas?
I roughly created 33 patches for this bug.
https://tbpl.mozilla.org/?tree=Try&rev=286cd58b2335
https://tbpl.mozilla.org/?tree=Try&rev=93200e41c32e

However, part.2 - part.32 are not so important they are just separated for renaming each NS_FOO_BAR_EVENT to FooBarEventClass.

So, first, part.1 and part.33 should be reviewed for preventing bug spams.

This patch renames:
nsEventStructType -> mozilla::EventClassID
WidgetEvent::eventStructType -> WidgetEvent::mClass

If you have other idea about the new names, let me know. I'll just replace them with your suggestion.

# It might be better to make mClass private and make friend class ParamTraits and create |EventClassID WidgetEvent::Class() const| for protecting the value.
Attachment #8466162 - Flags: review?(bugs)
All NS_FOO_BAR_EVENT are renamed to FooBarEvent. But NS_EVENT is renamed to BasicEventClass. Perhaps, it could be GenericEventClass or SimpleEventClass too.

And it should be defined in EventForwards.h because the EventClassID should be used in the related methods of nsContentUtils but BasicEvents.h shouldn't be included by nsContentUtils.h for avoiding include hell.
Attachment #8466164 - Flags: review?(bugs)
Comment on attachment 8466162 [details] [diff] [review]
part.1 Rename nsEventStructType to mozilla::EventClassID

I wonder if mClass could be const.

Why you need
+    WriteParam(aMsg,
+      static_cast<mozilla::EventClassIDType>(aParam.mClass));
?
Attachment #8466162 - Flags: review?(bugs) → review+
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #2)
> All NS_FOO_BAR_EVENT are renamed to FooBarEvent. But NS_EVENT is renamed to
> BasicEventClass. Perhaps, it could be GenericEventClass or SimpleEventClass
> too.
Specs talk about simple events, so perhaps SimpleEventClass
Comment on attachment 8466164 [details] [diff] [review]
part.33 Generate EventClassID with EventClassList.h in EventForwards.h and nsContentUtils should use it instead of uint32_t

But I think there is some patch missing here.
Like part 2?
Attachment #8466164 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #3)
> Comment on attachment 8466162 [details] [diff] [review]
> part.1 Rename nsEventStructType to mozilla::EventClassID
> 
> I wonder if mClass could be const.

I have no idea to do that because nsGUIEventIPC.h needs default constructor. However, then, we cannot initialize mClass with proper value without calling a lot of virtual calls of AsFooEvent().
(In reply to Olli Pettay [:smaug] from comment #3)
> Why you need
> +    WriteParam(aMsg,
> +      static_cast<mozilla::EventClassIDType>(aParam.mClass));
> ?

I confirmed that if we use enum type here, we need to create struct ParamTraits<mozilla::EventClassID>. It's redundant.
Okay, let's go.
Attachment #8466164 - Attachment is obsolete: true
Attachment #8466573 - Flags: review?(bugs)
Attachment #8466573 - Flags: review?(bugs) → review+
Attachment #8466574 - Flags: review?(bugs) → review+
Attachment #8466575 - Flags: review?(bugs) → review+
Attachment #8466576 - Flags: review?(bugs) → review+
I actually think all the FooBarClass enum values should be
eFooBarClass. That would make the code easier to read, since it becomes clear what
FooBarClass thing is.
(And per coding style enum values should have e-prefix.)
Attachment #8466577 - Flags: review?(bugs) → review+
Attachment #8466578 - Flags: review?(bugs) → review+
Attachment #8466579 - Flags: review?(bugs) → review+
Attachment #8466580 - Flags: review?(bugs) → review+
Attachment #8466581 - Flags: review?(bugs) → review+
Attachment #8466582 - Flags: review?(bugs) → review+
Attachment #8466583 - Flags: review?(bugs) → review+
Attachment #8466584 - Flags: review?(bugs) → review+
Attachment #8466586 - Flags: review?(bugs) → review+
Attachment #8466587 - Flags: review?(bugs) → review+
Attachment #8466588 - Flags: review?(bugs) → review+
Attachment #8466589 - Flags: review?(bugs) → review+
Attachment #8466590 - Flags: review?(bugs) → review+
Attachment #8466591 - Flags: review?(bugs) → review+
Attachment #8466592 - Flags: review?(bugs) → review+
Attachment #8466593 - Flags: review?(bugs) → review+
Attachment #8466594 - Flags: review?(bugs) → review+
Attachment #8466595 - Flags: review?(bugs) → review+
Attachment #8466596 - Flags: review?(bugs) → review+
Attachment #8466597 - Flags: review?(bugs) → review+
Attachment #8466598 - Flags: review?(bugs) → review+
Attachment #8466599 - Flags: review?(bugs) → review+
Attachment #8466600 - Flags: review?(bugs) → review+
Attachment #8466601 - Flags: review?(bugs) → review+
Attachment #8466602 - Flags: review?(bugs) → review+
Attachment #8466603 - Flags: review?(bugs) → review+
Comment on attachment 8466604 [details] [diff] [review]
part.32 Rename NS_EVENT to SimpleEventClass

Oh, my mistake. Since we can have non-simple events using the basic struct,
we probably should call it something else. Sorry!
BaseEventClass perhaps. That fixed, r+
Attachment #8466604 - Flags: review?(bugs) → review+
Comment on attachment 8466605 [details] [diff] [review]
part.33 Generate EventClassID with EventClassList.h in EventForwards.h and nsContentUtils should use it instead of uint32_t

So I think e-prefix might make code easier to read.
With that, r+.

(or feel free to explain if you don't want e-prefix)
Attachment #8466605 - Flags: review?(bugs) → review+
I added e prefix. Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6e08b99faf8
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0783cbdb8d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/32faf084d418
https://hg.mozilla.org/integration/mozilla-inbound/rev/17cfb4ff5a86
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e94b93c2c62
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b9d077a13aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e0084fdadf1
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9d1559b5320
https://hg.mozilla.org/integration/mozilla-inbound/rev/090872caa52c
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1866b96921b
https://hg.mozilla.org/integration/mozilla-inbound/rev/cddf9f550571
https://hg.mozilla.org/integration/mozilla-inbound/rev/df76962b3683
https://hg.mozilla.org/integration/mozilla-inbound/rev/955ed4eff9c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/52e95550adbf
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa613254b2f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/489398167822
https://hg.mozilla.org/integration/mozilla-inbound/rev/eba87b3656a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf0c9fbd6cb2
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad442dc11b05
https://hg.mozilla.org/integration/mozilla-inbound/rev/0dcfa42108aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/e45296e548c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d500741f3e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/b55f3c2894ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/8afadb6c396c
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f548e273185
https://hg.mozilla.org/integration/mozilla-inbound/rev/fca0bb35087e
https://hg.mozilla.org/integration/mozilla-inbound/rev/016c799315bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d873dd5eea0
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed8b6fe7bc44
https://hg.mozilla.org/integration/mozilla-inbound/rev/17473a495afb
https://hg.mozilla.org/integration/mozilla-inbound/rev/878f57134601
https://hg.mozilla.org/integration/mozilla-inbound/rev/a00fe3dedf67
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0cfff4ee625
https://hg.mozilla.org/mozilla-central/rev/b6e08b99faf8
https://hg.mozilla.org/mozilla-central/rev/d0783cbdb8d8
https://hg.mozilla.org/mozilla-central/rev/32faf084d418
https://hg.mozilla.org/mozilla-central/rev/17cfb4ff5a86
https://hg.mozilla.org/mozilla-central/rev/3e94b93c2c62
https://hg.mozilla.org/mozilla-central/rev/2b9d077a13aa
https://hg.mozilla.org/mozilla-central/rev/4e0084fdadf1
https://hg.mozilla.org/mozilla-central/rev/c9d1559b5320
https://hg.mozilla.org/mozilla-central/rev/090872caa52c
https://hg.mozilla.org/mozilla-central/rev/d1866b96921b
https://hg.mozilla.org/mozilla-central/rev/cddf9f550571
https://hg.mozilla.org/mozilla-central/rev/df76962b3683
https://hg.mozilla.org/mozilla-central/rev/955ed4eff9c9
https://hg.mozilla.org/mozilla-central/rev/52e95550adbf
https://hg.mozilla.org/mozilla-central/rev/aa613254b2f3
https://hg.mozilla.org/mozilla-central/rev/489398167822
https://hg.mozilla.org/mozilla-central/rev/eba87b3656a5
https://hg.mozilla.org/mozilla-central/rev/cf0c9fbd6cb2
https://hg.mozilla.org/mozilla-central/rev/ad442dc11b05
https://hg.mozilla.org/mozilla-central/rev/0dcfa42108aa
https://hg.mozilla.org/mozilla-central/rev/e45296e548c4
https://hg.mozilla.org/mozilla-central/rev/7d500741f3e9
https://hg.mozilla.org/mozilla-central/rev/b55f3c2894ff
https://hg.mozilla.org/mozilla-central/rev/8afadb6c396c
https://hg.mozilla.org/mozilla-central/rev/6f548e273185
https://hg.mozilla.org/mozilla-central/rev/fca0bb35087e
https://hg.mozilla.org/mozilla-central/rev/016c799315bb
https://hg.mozilla.org/mozilla-central/rev/9d873dd5eea0
https://hg.mozilla.org/mozilla-central/rev/ed8b6fe7bc44
https://hg.mozilla.org/mozilla-central/rev/17473a495afb
https://hg.mozilla.org/mozilla-central/rev/878f57134601
https://hg.mozilla.org/mozilla-central/rev/a00fe3dedf67
https://hg.mozilla.org/mozilla-central/rev/c0cfff4ee625
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: qe-verify-
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.