Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Description • 11 years ago

nsGUIEvent.h is now included by a lot of files. This causes very long rebuild when you modify nsGUIEvent.h.

For solving this issue, we should sort out the nsGUIEvent.h.

1. Move all constants to nsEvent.h
2. Make BasicEvent.h for nsEvent, nsGUIEvent, nsInputEvent, nsUIEvent
3. Make TextEvent.h for nsKeyEvent, nsTextEvent, nsCompositionEvent
4. Make MouseEvent.h for nsMouseEvent_base, nsMouseEvent, nsDragEvent, nsMouseScrollEvent, widget::WheelEvent
5. Make TouchEvent.h for nsGestureNotifyEvent, nsTouchEvent, nsSimpleGestureEvent
6. Make ContentEvent.h for nsScriptErrorEvent, nsScrollPortEvent, nsScrollAreaEvent, nsFormEvent, nsCommandEvent, nsClipboardEvent, nsFocusEvent, nsTransitionEvent, nsAnimationEvent
7. Make InternalEvent. for nsQueryContentEvent, nsSelectionEvent, nsContentCommandEvent, nsPluginEvent

And then, all events should be renamed to mozilla::widget::*Event and for compatibility, defining ns*Event with typedef.

If you have some suggestion, let me know.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 1 • 11 years ago

Sounds reasonable. However I'd put everything in the mozilla namespace. We don't really need a 'widget' namespace here.

I also think you should update all uses of ns*Event rather than leaving typedefs in.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 2 • 11 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #1)
> Sounds reasonable. However I'd put everything in the mozilla namespace. We
> don't really need a 'widget' namespace here.

Thank you for the feedback. Currently, only WheelEvent and some constants are in mozilla::widget. If all event stuff should be in mozilla namespace, I'll change them too.

> I also think you should update all uses of ns*Event rather than leaving
> typedefs in.

I also want to do it. However, it makes the patch too large. So, I'd like to do it in next bug. Do you agree with this idea?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 3 • 11 years ago

A separate bug would be OK, or a separate patch in this bug would be fine too.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 4 • 11 years ago

First of all, all mozilla::widget::* in nsGUIEvent.h and nsEvent.h are renamed to mozilla::*.

However, only widget::NotificationToIME is moved to nsIWidget.h because it's not an event stuff and it's used by nsIWidget::NotifyIME(). Other IME related structs and types are in mozilla::widget.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 5 • 11 years ago

Although, I like widget name space for internal event classes since it can distinguish the constants, types and class belonging in dom or widget easier.

However, it's not big matter for me. Let's land this first.

See comment 4 for the detail of this patch.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 6 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/5dbc32740161

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 7 • 11 years ago

I'll improve the EventForwards.h's enum with "enum class" later.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 8 • 11 years ago

workers/Events.cpp users Event class in anonymous namespace. Therefore, some code conflict with mozilla::Event.

Against for it, I changed *Event in workers/Events.cpp to Workers*Event. It might be better we rename nsEvent to mozilla::BasicEvent or something rather than mozilla::Event, tough.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 9 • 11 years ago

Roc:

Do you agree with exporting the new event header files as "mozilla/*Events.h"? Or they should be just "*Events.h"?

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/5dbc32740161

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 11 • 11 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #9)
> Do you agree with exporting the new event header files as
> "mozilla/*Events.h"? Or they should be just "*Events.h"?

I'm not sure, but I guess "mozilla/*Events.h".

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #8)
> workers/Events.cpp users Event class in anonymous namespace. Therefore, some
> code conflict with mozilla::Event.
> 
> Against for it, I changed *Event in workers/Events.cpp to Workers*Event. It
> might be better we rename nsEvent to mozilla::BasicEvent or something rather
> than mozilla::Event, tough.

I think the worker events stuff is going to change a lot, to use the same infrastructure as main-thread DOM events. So let's not worry about that.

Maybe we should name it something like mozilla::WidgetEvent or mozilla::NativeEvent though, in case we want to rename nsDOMEvent to mozilla::dom::Event? I think we might want to do the latter, and since a lot of code has to deal with nsDOMEvent and nsEvent, we don't really want to rely on namespaces to disambiguate.

Olli, what do you think?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 12 • 11 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> I think the worker events stuff is going to change a lot, to use the same
> infrastructure as main-thread DOM events. So let's not worry about that.

What I mean is, I think you've done the right thing to do whatever you have to do to get things working with workers.

I think mozilla::WidgetEvent is probably the best name for nsEvent.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 13 • 11 years ago

Worker events are indeed going away, so better to make other things look good, and worker events
can be a bit uglier.

mozilla::NativeEvent sounds ok.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 14 • 11 years ago

Thank you for the advice.

mozilla::NativeEvent sounds not good for widget developers because widget converts from "native event" to "Gecko event". In other words, ns*Event are abstract event information of each platform's native event. So, sounds mozilla::WidgetEvent is better to me.

Then, other events should also have Widget prefix? E.g., InputEvent and KeyEvent would conflict with B2G's native events in android:: namespace (http://mxr.mozilla.org/mozilla-central/source/widget/gonk/libui/Input.h) if we just drop "ns" prefix. mozilla::Widget*Event sounds very clear name to me in XP level code.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 15 • 11 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #14)
> Then, other events should also have Widget prefix? E.g., InputEvent and
> KeyEvent would conflict with B2G's native events in android:: namespace
> (http://mxr.mozilla.org/mozilla-central/source/widget/gonk/libui/Input.h) if
> we just drop "ns" prefix. mozilla::Widget*Event sounds very clear name to me
> in XP level code.

I agree.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 16 • 11 years ago

WidgetEvent is ok to me.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 18 • 11 years ago

Changes in BasicEvents.h will cause almost full build like nsGUIEvent.h change. This should be fixed in another bug.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 23 • 11 years ago

This patch removes all unnecessary forward declarations and including headers. This causes bustage on some platform, I'm still testing this on tryserver.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 25 • 11 years ago

Comment on attachment 807098 [details] [diff] [review]
part.9 Rename nsMutationEvent.h to mozilla/MutationEvent.h

Renaming mutation event like other widget events.

This patch moves the NS_MUTATION_* event message to BasicEvents.h because I'd like to change them as an enum in the future.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 26 • 11 years ago

I believe that this is just a mistake at creating this file.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 27 • 11 years ago

NS_EVENT_TYPE_* values are used as event message value. So, they should be merged in other message values.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 28 • 11 years ago

This patch is still being tested on tryserver.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 29 • 11 years ago

I'll post some patches which removes nsGUIEvent.h completely but I'd like to land them separately after landing part.2 - part.12.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 30 • 11 years ago

Comment on attachment 807097 [details] [diff] [review]
part.8 Create mozilla/InternalEvents.h

Okay, bustage has gone.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 31 • 11 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 32 • 11 years ago

Comment on attachment 807098 [details] [diff] [review]
part.9 Rename nsMutationEvent.h to mozilla/MutationEvent.h




>+// XXX Following values are used in nsINode.h.  So, change of this header or
>+//     BasicEvents.h causes rebuilding a lot of files.  We need to do something
>+//     for this issue.
This header shouldn't be changed too often, so perhaps no worth to have this comment




I'm not very happy with WidgetMutationEvent, but I don't know what would be a better name.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 33 • 11 years ago

Comment on attachment 807099 [details] [diff] [review]
part.10 Replace NS_EVENT_NULL in nsEventNameList.h with NS_EVENT since the argument specifies an event struct type

Should be ok since nsContentUtils::GetEventCategory defaults to NS_EVENT.

This does change the behavior of GetEventIdAndAtom, but looks ok to me.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 34 • 11 years ago

Comment on attachment 807092 [details] [diff] [review]
part.4 Create mozilla/KeyTextEvents.h

Review of attachment 807092 [details] [diff] [review]:
-----------------------------------------------------------------

Why not call the new header KeyEvents.h?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 35 • 11 years ago

Comment on attachment 807095 [details] [diff] [review]
part.7 Create mozilla/ContentEvents.h

Review of attachment 807095 [details] [diff] [review]:
-----------------------------------------------------------------

Actually, in this patch, I wonder if we should keep using the "Widget" prefix. Because these events are really unrelated to widgets.

How about, for the events that aren't relevant to widgets, we call them Internal*Event instead?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 36 • 11 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #34)
> Comment on attachment 807092 [details] [diff] [review]
> part.4 Create mozilla/KeyTextEvents.h
> 
> Review of attachment 807092 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why not call the new header KeyEvents.h?

Composition (and Text) events may be caused by non keyboard device such as hand writing system or voice input system.

I'd like to name it InputEvents.h. However, there is WidgetInputEvent in BasicEvents.h. So, do you have better idea for the new header file?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 37 • 11 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #35)
> Comment on attachment 807095 [details] [diff] [review]
> part.7 Create mozilla/ContentEvents.h
> 
> Review of attachment 807095 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Actually, in this patch, I wonder if we should keep using the "Widget"
> prefix. Because these events are really unrelated to widgets.
> 
> How about, for the events that aren't relevant to widgets, we call them
> Internal*Event instead?

Hmm, then, InternalEvents.h becomes strange name...

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 38 • 11 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #37)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #35)
> > Comment on attachment 807095 [details] [diff] [review]
> > part.7 Create mozilla/ContentEvents.h
> > 
> > Review of attachment 807095 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Actually, in this patch, I wonder if we should keep using the "Widget"
> > prefix. Because these events are really unrelated to widgets.
> > 
> > How about, for the events that aren't relevant to widgets, we call them
> > Internal*Event instead?
> 
> Hmm, then, InternalEvents.h becomes strange name...

How about Content*Event?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 39 • 11 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #36)
> Composition (and Text) events may be caused by non keyboard device such as
> hand writing system or voice input system.
> 
> I'd like to name it InputEvents.h. However, there is WidgetInputEvent in
> BasicEvents.h. So, do you have better idea for the new header file?

How about TextEvents.h?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 40 • 11 years ago

In InternalEvents.h, shouldn't WidgetQueryContentEvent belong to KeyTextEvents.h (or TextEvents.h)?

Then we could rename InternalEvents.h to MiscEvents.h.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 41 • 11 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #39)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #36)
> > Composition (and Text) events may be caused by non keyboard device such as
> > hand writing system or voice input system.
> > 
> > I'd like to name it InputEvents.h. However, there is WidgetInputEvent in
> > BasicEvents.h. So, do you have better idea for the new header file?
> 
> How about TextEvents.h?

Keyboard event may not cause text input, though. However, I was thinking it as a candidate name of it. If you do not mind this issue, let's use it.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 42 • 11 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #40)
> In InternalEvents.h, shouldn't WidgetQueryContentEvent belong to
> KeyTextEvents.h (or TextEvents.h)?
> 
> Then we could rename InternalEvents.h to MiscEvents.h.

Hmm, then, I'd like to move WidgetSelectionEvent too because it's also used by IME handling.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 43 • 11 years ago

KeyTextEvents.h -> TextEvents.h
WidgetQueryContentEvent and WidgetSelectionEvent belong to TextEvents.h
Name the events in ContentEvents.h (Or InternalEvents.h?)
InternalEvents.h -> MiscEvents.h

Then, WidgetMutationEvent should be renamed as InternalMutationEvent too?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 44 • 11 years ago

> Name the events in ContentEvents.h (Or InternalEvents.h?)

I meant they should be named Internal*Event and ContentEvents.h should be InternalEvents.h?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 45 • 11 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #41)
> Keyboard event may not cause text input, though. However, I was thinking it
> as a candidate name of it. If you do not mind this issue, let's use it.

Yeah I think that's fine. Keyboard events are still related to text even if they don't cause immediate input.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #43)
> KeyTextEvents.h -> TextEvents.h
> WidgetQueryContentEvent and WidgetSelectionEvent belong to TextEvents.h
> Name the events in ContentEvents.h (Or InternalEvents.h?)
> InternalEvents.h -> MiscEvents.h
> 
> Then, WidgetMutationEvent should be renamed as InternalMutationEvent too?

Yes.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #44)
> > Name the events in ContentEvents.h (Or InternalEvents.h?)
> 
> I meant they should be named Internal*Event and ContentEvents.h should be
> InternalEvents.h?

No. If we have a header file called InternalEvents.h then you'd expect all Internal*Events to be in it, which we don't want since there are a lot of Internal*Events. So let's have ContentEvents.h, MiscEvents.h and any other files we need, all containing various Internal*Events.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 46 • 11 years ago

Thanks, I agree all of your idea now!

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 47 • 11 years ago

Ah, then, WidgetUIEvent should be InternalUIEvent? It's internal event for nsDOMUIEvent. It's not used by widget directly.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 48 • 11 years ago

See the previous comment. Sorry for the spam.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 49 • 11 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #47)
> Ah, then, WidgetUIEvent should be InternalUIEvent? It's internal event for
> nsDOMUIEvent. It's not used by widget directly.

Yes.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 50 • 11 years ago

(I think that the fact we need to define nsEvent subclasses for events that have nothing to do with widgets --- i.e., that we need Internal*Events at all --- is a really annoying problem with our event system.)

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 51 • 11 years ago

Thank you, perhaps, I'll post new patches Tuesday or later (Monday is a holiday of Japan).

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 52 • 11 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #50)
> (I think that the fact we need to define nsEvent subclasses for events that
> have nothing to do with widgets
You don't *have* to define any subclass if you just need a DOM Event. Just define a new DOM Event 
interface using .webidl (and implement it hopefully using .webidl event codegen if the interface is rather simple).
But for certain cases when we don't necessarily need any heap event object, ns*Event structs are handy.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 53 • 11 years ago

Are you saying we don't need stuff like WidgetScriptErrorEvent?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 54 • 11 years ago

Technically we don't need it if someone implements the same functionality to some DOM event. But script error event is *very* special case, even in the spec. EventHandlers for script errors get different kinds of arguments than EventListeners.
http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#onerroreventhandler

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 57 • 11 years ago

Renamed to TextEvents.h

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 60 • 11 years ago

Widget*Event are Renamed to Internal*Event.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 61 • 11 years ago

The file is renamed to MiscEvents.h. And WidgetQueryContentEvent and WidgetSelectionEvent are moved to TextEvents.h.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 62 • 11 years ago

nsMutationEvent is now renamed to InternalMutationEvent.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 66 • 11 years ago

Comment on attachment 809033 [details] [diff] [review]
part.8 Create mozilla/MiscEvents.h

Review of attachment 809033 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/MiscEvents.h
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_InternalEvents_h__
> +#define mozilla_InternalEvents_h__

Change to MiscEvents

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 67 • 11 years ago

Comment on attachment 809040 [details] [diff] [review]
part.12 All event utils (macros and inline methods) should be methods of mozilla::WidgetEvent

Review of attachment 809040 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

::: widget/shared/WidgetEventImpl.cpp
@@ +36,5 @@
> +bool
> +WidgetEvent::IsMouseDerivedEvent() const
> +{
> +  return (eventStructType == NS_MOUSE_EVENT ||
> +          eventStructType == NS_DRAG_EVENT);

You don't need parentheses around these return values. (Here and many below.)

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 68 • 11 years ago

Thank you, I'll post patches which remove nsGUIEvent.h (i.e., including new header files directly).

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2fce6c8e493
https://hg.mozilla.org/integration/mozilla-inbound/rev/70bbbaa3c1d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff21dedcf8e7
https://hg.mozilla.org/integration/mozilla-inbound/rev/354ac52e5325
https://hg.mozilla.org/integration/mozilla-inbound/rev/93a96f509d9a
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4e2588531d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab54eba99d34
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba03fca314e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ed0d8104386
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f29ba660b50
https://hg.mozilla.org/integration/mozilla-inbound/rev/008ba688cd5a

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 69 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/f2fce6c8e493
https://hg.mozilla.org/mozilla-central/rev/70bbbaa3c1d0
https://hg.mozilla.org/mozilla-central/rev/ff21dedcf8e7
https://hg.mozilla.org/mozilla-central/rev/354ac52e5325
https://hg.mozilla.org/mozilla-central/rev/93a96f509d9a
https://hg.mozilla.org/mozilla-central/rev/e4e2588531d6
https://hg.mozilla.org/mozilla-central/rev/ab54eba99d34
https://hg.mozilla.org/mozilla-central/rev/ba03fca314e1
https://hg.mozilla.org/mozilla-central/rev/6ed0d8104386
https://hg.mozilla.org/mozilla-central/rev/2f29ba660b50
https://hg.mozilla.org/mozilla-central/rev/008ba688cd5a

	Masatoshi Kimura [:emk]
	Comment 70 • 11 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #66)
> > +#ifndef mozilla_InternalEvents_h__
> > +#define mozilla_InternalEvents_h__
> 
> Change to MiscEvents

And don't use double underscores in identifiers. C++ reserves all identifiers containing a double underscore.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 71 • 11 years ago

(In reply to Masatoshi Kimura [:emk] from comment #70)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #66)
> > > +#ifndef mozilla_InternalEvents_h__
> > > +#define mozilla_InternalEvents_h__
> > 
> > Change to MiscEvents
> 
> And don't use double underscores in identifiers. C++ reserves all
> identifiers containing a double underscore.

IIRC, there are a lot of header files using "__". Could you file a bug for it?

	Masatoshi Kimura [:emk]
	Comment 72 • 11 years ago

Filed 920412 about include guards.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 75 • 11 years ago

(In reply to Masatoshi Kimura [:emk] from comment #72)
> Filed 920412 about include guards.

Hey, don't close this bug without confirmation. There are some remaining work.

	Masatoshi Kimura [:emk]
	Comment 80 • 11 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #75)
> Hey, don't close this bug without confirmation. There are some remaining
> work.

Sorry, I thought the work was done because you removed [leave open].

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 81 • 11 years ago

(In reply to Masatoshi Kimura [:emk] from comment #80)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #75)
> > Hey, don't close this bug without confirmation. There are some remaining
> > work.
> 
> Sorry, I thought the work was done because you removed [leave open].

because next landing is the final work in this bug.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 82 • 11 years ago

Comment on attachment 809727 [details] [diff] [review]
part.15 mozilla/TextEvents.h should be included directly

Review of attachment 809727 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/xul/base/src/nsXULPopupManager.cpp
@@ +48,5 @@
> +              nsIDOMKeyEvent::DOM_VK_LEFT  == nsIDOMKeyEvent::DOM_VK_END + 2 &&
> +              nsIDOMKeyEvent::DOM_VK_UP    == nsIDOMKeyEvent::DOM_VK_END + 3 &&
> +              nsIDOMKeyEvent::DOM_VK_RIGHT == nsIDOMKeyEvent::DOM_VK_END + 4 &&
> +              nsIDOMKeyEvent::DOM_VK_DOWN  == nsIDOMKeyEvent::DOM_VK_END + 5,
> +              "nsXULPopupManager assumes some keyCode values are consinueous");

"consecutive"

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 83 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/de77a9248c04
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3d5df94a2fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc7aad8c3109
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd4c88679a79
https://hg.mozilla.org/integration/mozilla-inbound/rev/8486e298ef70
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e3017bef2c6

Thank you, roc and smaug! I'll work on related stuff of this bug in other bugs.

	Ryan VanderMeulen [:RyanVM]
	Comment 84 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/de77a9248c04
https://hg.mozilla.org/mozilla-central/rev/d3d5df94a2fc
https://hg.mozilla.org/mozilla-central/rev/cc7aad8c3109
https://hg.mozilla.org/mozilla-central/rev/bd4c88679a79
https://hg.mozilla.org/mozilla-central/rev/8486e298ef70
https://hg.mozilla.org/mozilla-central/rev/2e3017bef2c6

	Jim Mathies [:jimm]
	Comment 85 • 11 years ago

These changes caught me by surprise. You might want to post to dev.platform to give people an idea as to what moved where and what changed names.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 86 • 11 years ago

(In reply to Jim Mathies [:jimm] from comment #85)
> These changes caught me by surprise. You might want to post to dev.platform
> to give people an idea as to what moved where and what changed names.

Indeed. I posted it. Sorry for the delay to notify of this series of changes.
https://groups.google.com/forum/#!topic/mozilla.dev.platform/cHg_ExqEC_U