Closed
Bug 1046101
Opened 10 years ago
Closed 10 years ago
Redesign nsEventStructType
Categories
(Core :: DOM: UI Events & Focus Handling, defect)
Core
DOM: UI Events & Focus Handling
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: masayuki, Assigned: masayuki)
References
Details
Attachments
(33 files, 1 obsolete file)
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?
Assignee | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
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 3•10 years ago
|
||
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+
Comment 4•10 years ago
|
||
(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 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
(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().
Assignee | ||
Comment 7•10 years ago
|
||
(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.
Assignee | ||
Comment 8•10 years ago
|
||
Okay, let's go.
Attachment #8466164 -
Attachment is obsolete: true
Attachment #8466573 -
Flags: review?(bugs)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8466574 -
Flags: review?(bugs)
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8466575 -
Flags: review?(bugs)
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8466576 -
Flags: review?(bugs)
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8466577 -
Flags: review?(bugs)
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8466578 -
Flags: review?(bugs)
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8466579 -
Flags: review?(bugs)
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8466580 -
Flags: review?(bugs)
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8466581 -
Flags: review?(bugs)
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8466582 -
Flags: review?(bugs)
Assignee | ||
Comment 18•10 years ago
|
||
Attachment #8466583 -
Flags: review?(bugs)
Assignee | ||
Comment 19•10 years ago
|
||
Attachment #8466584 -
Flags: review?(bugs)
Assignee | ||
Comment 20•10 years ago
|
||
Attachment #8466586 -
Flags: review?(bugs)
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8466587 -
Flags: review?(bugs)
Assignee | ||
Comment 22•10 years ago
|
||
Attachment #8466588 -
Flags: review?(bugs)
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #8466589 -
Flags: review?(bugs)
Assignee | ||
Comment 24•10 years ago
|
||
Attachment #8466590 -
Flags: review?(bugs)
Assignee | ||
Comment 25•10 years ago
|
||
Attachment #8466591 -
Flags: review?(bugs)
Assignee | ||
Comment 26•10 years ago
|
||
Attachment #8466592 -
Flags: review?(bugs)
Assignee | ||
Comment 27•10 years ago
|
||
Attachment #8466593 -
Flags: review?(bugs)
Assignee | ||
Comment 28•10 years ago
|
||
Attachment #8466594 -
Flags: review?(bugs)
Assignee | ||
Comment 29•10 years ago
|
||
Attachment #8466595 -
Flags: review?(bugs)
Assignee | ||
Comment 30•10 years ago
|
||
Attachment #8466596 -
Flags: review?(bugs)
Assignee | ||
Comment 31•10 years ago
|
||
Attachment #8466597 -
Flags: review?(bugs)
Assignee | ||
Comment 32•10 years ago
|
||
Attachment #8466598 -
Flags: review?(bugs)
Assignee | ||
Comment 33•10 years ago
|
||
Attachment #8466599 -
Flags: review?(bugs)
Assignee | ||
Comment 34•10 years ago
|
||
Attachment #8466600 -
Flags: review?(bugs)
Assignee | ||
Comment 35•10 years ago
|
||
Attachment #8466601 -
Flags: review?(bugs)
Assignee | ||
Comment 36•10 years ago
|
||
Attachment #8466602 -
Flags: review?(bugs)
Assignee | ||
Comment 37•10 years ago
|
||
Attachment #8466603 -
Flags: review?(bugs)
Assignee | ||
Comment 38•10 years ago
|
||
Attachment #8466604 -
Flags: review?(bugs)
Assignee | ||
Comment 39•10 years ago
|
||
Attachment #8466605 -
Flags: review?(bugs)
Updated•10 years ago
|
Attachment #8466573 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466574 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466575 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466576 -
Flags: review?(bugs) → review+
Comment 40•10 years ago
|
||
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.)
Updated•10 years ago
|
Attachment #8466577 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466578 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466579 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466580 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466581 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466582 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466583 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466584 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466586 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466587 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466588 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466589 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466590 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466591 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466592 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466593 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466594 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466595 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466596 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466597 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466598 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466599 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466600 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466601 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466602 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8466603 -
Flags: review?(bugs) → review+
Comment 41•10 years ago
|
||
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 42•10 years ago
|
||
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+
Assignee | ||
Comment 43•10 years ago
|
||
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
Comment 44•10 years ago
|
||
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
Updated•10 years ago
|
Flags: qe-verify-
Updated•5 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•