Closed Bug 975688 Opened 6 years ago Closed 6 years ago

Move all nsDOM*Event classes into mozilla::dom namespace and rename them to same as standard spec's name (i.e., nsDOM*Event -> mozilla::dom::*Event)

Categories

(Core :: DOM: Events, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(28 files)

11.59 KB, patch
smaug
: review+
Details | Diff | Splinter Review
8.43 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.93 KB, patch
smaug
: review+
Details | Diff | Splinter Review
9.25 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.07 KB, patch
smaug
: review+
Details | Diff | Splinter Review
8.34 KB, patch
smaug
: review+
Details | Diff | Splinter Review
9.24 KB, patch
smaug
: review+
Details | Diff | Splinter Review
21.58 KB, patch
smaug
: review+
Details | Diff | Splinter Review
11.74 KB, patch
smaug
: review+
Details | Diff | Splinter Review
9.33 KB, patch
smaug
: review+
Details | Diff | Splinter Review
11.97 KB, patch
smaug
: review+
Details | Diff | Splinter Review
17.39 KB, patch
smaug
: review+
Details | Diff | Splinter Review
10.09 KB, patch
smaug
: review+
Details | Diff | Splinter Review
13.78 KB, patch
smaug
: review+
Details | Diff | Splinter Review
47.37 KB, patch
smaug
: review+
Details | Diff | Splinter Review
6.58 KB, patch
smaug
: review+
Details | Diff | Splinter Review
15.23 KB, patch
smaug
: review+
roc
: review+
Details | Diff | Splinter Review
10.20 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.68 KB, patch
smaug
: review+
Details | Diff | Splinter Review
38.84 KB, patch
smaug
: review+
Details | Diff | Splinter Review
16.80 KB, patch
smaug
: review+
Details | Diff | Splinter Review
9.28 KB, patch
smaug
: review+
Details | Diff | Splinter Review
10.97 KB, patch
smaug
: review+
Details | Diff | Splinter Review
13.00 KB, patch
smaug
: review+
Details | Diff | Splinter Review
11.48 KB, patch
smaug
: review+
Details | Diff | Splinter Review
64.95 KB, patch
smaug
: review+
Details | Diff | Splinter Review
8.27 KB, patch
smaug
: review+
Details | Diff | Splinter Review
191.76 KB, patch
smaug
: review+
Details | Diff | Splinter Review
If you don't disagree with the summary, please comment here as soon as possible.
I don't disagree.
Oops, I meant that you don't agree with the summary :-(
Whiteboard: [leave open]
It might be better to move NS_NewDOM*Event into mozilla::dom, but anyway, we should focus to renaming nsDOM*Event for now.
Comment on attachment 8381358 [details] [diff] [review]
part.1 Rename DOMWheelEvent to WheelEvent


> 'WheelEvent': {
>-    'headerFile': 'DOMWheelEvent.h',
>-    'nativeType': 'mozilla::dom::DOMWheelEvent',
>+    'headerFile': 'WheelEvent.h',
>+    'nativeType': 'mozilla::dom::WheelEvent',
> },
I don't think we need this entry in Bindings.conf anymore, since headerFile name and nativeType are now 
the defaults.
Attachment #8381358 - Flags: review?(bugs) → review+
Or, I guess you need to export WheelEvent.h to mozilla/dom so that
bindings can use mozilla/dom/WheelEvent.h
Comment on attachment 8381359 [details] [diff] [review]
part.2 Rename nsDOMAnimationEvent to mozilla::dom::AnimationEvent

Same with this, and I assume with others.
You could upload a new patch to remove stuff from Bindings.conf
and make moz.build to export headers using EXPORTS.mozilla.dom
Attachment #8381359 - Flags: review?(bugs) → review+
Attachment #8381360 - Flags: review?(bugs) → review+
Attachment #8381361 - Flags: review?(bugs) → review+
Attachment #8381362 - Flags: review?(bugs) → review+
Attachment #8381363 - Flags: review?(bugs) → review+
Attachment #8381364 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #12)
> Or, I guess you need to export WheelEvent.h to mozilla/dom so that
> bindings can use mozilla/dom/WheelEvent.h

(In reply to Olli Pettay [:smaug] from comment #13)
> Comment on attachment 8381359 [details] [diff] [review]
> part.2 Rename nsDOMAnimationEvent to mozilla::dom::AnimationEvent
> 
> Same with this, and I assume with others.
> You could upload a new patch to remove stuff from Bindings.conf
> and make moz.build to export headers using EXPORTS.mozilla.dom

Yes, I like this idea better too!
Attachment #8382202 - Flags: review?(bugs)
Attachment #8382205 - Flags: review?(bugs)
Attachment #8382206 - Flags: review?(bugs)
Attachment #8382208 - Flags: review?(bugs)
Attachment #8382209 - Flags: review?(bugs)
Attachment #8382210 - Flags: review?(bugs)
Attachment #8382211 - Flags: review?(bugs)
Attachment #8382212 - Flags: review?(bugs)
Comment on attachment 8382214 [details] [diff] [review]
part.16 Clean up dom::PointerEvent code

Just cleaning up the PointerEvent implementation code for using same coding style with other DOM event classes.
Attachment #8382214 - Flags: review?(bugs)
Comment on attachment 8382215 [details] [diff] [review]
part.17 Rename dom/events/MutationEvent.h to dom/Events/InternalMutationEvent.h

Renaming MutationEvent.h to InternalMutationEvent.h because it will conflict with renamed nsDOMMutationEvent.h.

If we move this MutationEvent.h to widget/, we can distinguish the fine as mozilla/MutationEvent.h vs mozilla/dom/MutationEvent.h. But it may cause developers confused. Therefore, I like renaming as this patch better.
Attachment #8382215 - Flags: review?(roc)
Attachment #8382215 - Flags: review?(bugs)
Attachment #8382216 - Flags: review?(bugs)
Comment on attachment 8382202 [details] [diff] [review]
part.8 Rename nsDOMDeviceMotionEvent, nsDOMDeviceAcceleration and nsDOMDeviceRotationRate to mozilla::dom::DeviceMotionEvent, DeviceAcceleration and DeviceRotationRate

> 'DeviceAcceleration': {
>-    'nativeType': 'nsDOMDeviceAcceleration',
>-    'headerFile': 'nsDOMDeviceMotionEvent.h',
>+    'headerFile': 'mozilla/dom/DeviceMotionEvent.h',
>+    'nativeType': 'mozilla::dom::DeviceAcceleration',
> },
I think you don't need nativeType, since it is the default


> 
> 'DeviceMotionEvent': {
>-    'nativeType': 'nsDOMDeviceMotionEvent',
>+    'nativeType': 'mozilla::dom::DeviceMotionEvent',
> },
I think you don't need this entry in .conf at all since nativeType is now the default one.


> 
> 'DeviceRotationRate': {
>-    'nativeType': 'nsDOMDeviceRotationRate',
>-    'headerFile': 'nsDOMDeviceMotionEvent.h',
>+    'headerFile': 'mozilla/dom/DeviceMotionEvent.h',
>+    'nativeType': 'mozilla::dom::DeviceRotationRate',
> },
I think you don't need nativeType, since it is the default
Attachment #8382202 - Flags: review?(bugs) → review+
Comment on attachment 8382205 [details] [diff] [review]
part.9 Rename nsDOMDragEvent to mozilla::dom::DragEvent


> 'DragEvent': {
>-    'nativeType': 'nsDOMDragEvent',
>+    'nativeType': 'mozilla::dom::DragEvent',
> },
I think you can just remove the whole 'DragEvent' entry
Attachment #8382205 - Flags: review?(bugs) → review+
Comment on attachment 8382206 [details] [diff] [review]
part.10 Rename nsDOMFocusEvent to mozilla::dom::FocusEvent

> 'FocusEvent': {
>-    'nativeType': 'nsDOMFocusEvent',
>+    'nativeType': 'mozilla::dom::FocusEvent',
> },
Again, I think the whole entry is now useless.
Attachment #8382206 - Flags: review?(bugs) → review+
Comment on attachment 8382208 [details] [diff] [review]
part.11 Rename nsDOMKeyboardEvent to mozilla::dom::KeyboardEvent

> 'KeyboardEvent': {
>-    'nativeType': 'nsDOMKeyboardEvent',
>+    'nativeType': 'mozilla::dom::KeyboardEvent',
> },
Same thing here
Attachment #8382208 - Flags: review?(bugs) → review+
Comment on attachment 8382209 [details] [diff] [review]
part.12 Rename nsDOMMessageEvent to mozilla::dom::MessageEvent

> 'MessageEvent': {
>-    'nativeType': 'nsDOMMessageEvent',
>+    'nativeType': 'mozilla::dom::MessageEvent',
> },
again the same thing
Attachment #8382209 - Flags: review?(bugs) → review+
Comment on attachment 8382210 [details] [diff] [review]
part.13 Rename nsDOMMouseScrollEvent to mozilla::dom::MouseScrollEvent

> 'MouseScrollEvent': {
>-    'nativeType': 'nsDOMMouseScrollEvent',
>+    'nativeType': 'mozilla::dom::MouseScrollEvent',
> },
Again the same thing.
Attachment #8382210 - Flags: review?(bugs) → review+
Comment on attachment 8382211 [details] [diff] [review]
part.14 Rename nsDOMSimpleGestureEvent to mozilla::dom::SimpleGestureEvent


> 'SimpleGestureEvent': {
>-    'nativeType': 'nsDOMSimpleGestureEvent',
>+    'nativeType': 'mozilla::dom::SimpleGestureEvent',
> },
ditto
Attachment #8382211 - Flags: review?(bugs) → review+
Comment on attachment 8382212 [details] [diff] [review]
part.15 Rename nsDOMMouseEvent to mozilla::dom::MouseEvent

> 'MouseEvent': {
>-    'nativeType': 'nsDOMMouseEvent',
>+    'nativeType': 'mozilla::dom::MouseEvent',
> },
ditto
Attachment #8382212 - Flags: review?(bugs) → review+
Attachment #8382214 - Flags: review?(bugs) → review+
Comment on attachment 8382216 [details] [diff] [review]
part.18 Rename nsDOMMutationEvent to mozilla::dom::MutationEvent


> 'MutationEvent': {
>-    'nativeType': 'nsDOMMutationEvent',
>+    'nativeType': 'mozilla::dom::MutationEvent',
> },
Same case as elsewhere.
Attachment #8382216 - Flags: review?(bugs) → review+
Attachment #8382215 - Flags: review?(bugs) → review+
According to your review, we can remove them now.
Attachment #8383557 - Flags: review?(bugs)
Although, nsDOMTimeEvent is under dom/smil, but this is the last derived event (except nsDOMTextEvent which will be removed by bug 974318) which is not in mozilla::dom namespace.
Attachment #8383567 - Flags: review?(bugs)
Comment on attachment 8383557 [details] [diff] [review]
part.19 Remove unnecessary items in Bindings.conf

Just make sure to push these patches to try.
Attachment #8383557 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #50)
> Comment on attachment 8383557 [details] [diff] [review]
> part.19 Remove unnecessary items in Bindings.conf
> 
> Just make sure to push these patches to try.

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=aeb468ff33c8

Yeah, I'm a big fan of tryserver.
Attachment #8383558 - Flags: review?(bugs) → review+
Attachment #8383559 - Flags: review?(bugs) → review+
Attachment #8383560 - Flags: review?(bugs) → review+
Attachment #8383561 - Flags: review?(bugs) → review+
Comment on attachment 8383562 [details] [diff] [review]
part.24 Rename nsDOMNotifyPaintEvent to mozilla::dom::NotifyPaintEvent

The comments in moz.build are odd, but not your fault.
Attachment #8383562 - Flags: review?(bugs) → review+
Attachment #8383563 - Flags: review?(bugs) → review+
Attachment #8383564 - Flags: review?(bugs) → review+
Comment on attachment 8383567 [details] [diff] [review]
part.27 Rename nsDOMTimeEvent to mozilla::dom::TimeEvent

Hmm, one could actually implement TimeEvent using Event codegenerator (either the old one, if support for nsIDOMTimeEvent is needed, or the new one if webidl only is enough), I think.

But we can do that change in a different bug.
Attachment #8383567 - Flags: review?(bugs) → review+
I'm working on renaming nsDOMEvent but it's a big patch. I'm going to post it next Monday.

It may be the last patch of this bug. However, I wonder, should I rename NS_NewDOM*Event() too? If so, perhaps, mozilla::dom::Create*Event()? Although, it might be conflict with CreateEvent() in some places.
Flags: needinfo?(bugs)
mozilla::dom::CreateNew*Event()?
If we need a factor method at all, how about mozilla::dom::New*Event ?
Sounds good to me. How about Smaug?
Please don't rename NS_New* since I think we should make all the events to use
similar way to construct what codegenerated events, at least, support.

FooEventInit init;
init.bubbles = true;
...

nsRefPtr<FooEvent> event = FooEvent::Construct(someTarget, NS_LITERAL_STRING("footype"), init);

Once everything uses that, we could perhaps just remove NS_New*
Flags: needinfo?(bugs)
Okay. I'll keep them.
The last patch of this bug.
Attachment #8385189 - Flags: review?(bugs)
Attachment #8385189 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/8b872592e441
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.