602787 - widget/*Events.h shouldn't be included by other *.h files as far as possible

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

When I change nsGUIEvent.h, that needs about 20mins to rebuild firefox on my Win7 system.

The one of the causes is other .h files include it. And if the header is included by another header file, that causes unnecessary recompiling.

The patch reduces the rebuild time about 2mins on my system but it can not change any behavior.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 481763 [details] [diff] [review]
Patch v1.0

Oops, canceling the review.

I forget to build test on non-Windows system. I'm sorry for the spam.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Don't implement PresShell::Delayed*Event class in nsPresShell.h

Moving the DelayedEvent implementation to PresShell.cpp, we can avoid not to include event headers from PresShell.h.

I removed "ns" prefix from DelayedEvent since it's not necessary for nested class.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 Don't implement nsIWidgetListener in its header file

nsIWidgetListener isn't an abstract class (interface).

It should be implemented in new .cpp file. Then, we can minimize the header files which are included by nsIWidgetListener.h.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.3 Don't implement some methods which use WidgetEvent in nsEventListenerManager.h (WONTFIX for now)

nsEventListenerManager.h is included from a lot of files. So, this is really big improvement for fixing include hell.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.4 Don't implement nsAutoHandlingUserInputStatePusher class in nsEventStateManager.h

nsEventStateManager.h is also included from a lot of files.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.5 Use EventForwards.h in TabParent.h

TabParent.h doesn't need to include event header file...

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.6 Use EventForwards.h in gtk/nsWindow.h

gtk/nsWindow.h has not already needed to include event header file.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.7 Don't implement methods which use Widget*Event in qt/nsWindow.h

I've not tested this yet because I don't succeed to build with Qt on Fedora x64.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.8 Don't implement nsWindowBase::DispatchPluginEvent() in windows/nsWindowBase.h

KeyboardLayout.h will be fixed by later patch.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.9 Don't implement methods which need BasicEvents.h or TextEvents.h in windows/KeyboardLayout.h

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.10 Create TextRange.h for separating TextEvents.h

Separating TextRange* definition from TextEvents.h improves the include hell of BasicEvents.h which is included by TextEvents.h.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 819013 [details] [diff] [review]
part.3 Don't implement some methods which use WidgetEvent in nsEventListenerManager.h (WONTFIX for now)

::HandleEvent is inline for performance reasons, and we should ensure
IsListening stays inlined too.
So if you can find some proper MOZ_ALWAYS_INLINE macro magic to
do the inlining when possible, then we can move those methods to .cpp

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 819009 [details] [diff] [review]
part.1 Don't implement PresShell::Delayed*Event class in nsPresShell.h

># HG changeset patch
># User Masayuki Nakano <[e-mail]>
># Parent a5bc90bb0f8f8d23cde0b69a779bbcaa2e591f08
>Bug 602787 part.1 Don't implement PresShell::Delayed*Event class in nsPresShell.h r=
>
>diff --git a/layout/base/nsPresShell.cpp b/layout/base/nsPresShell.cpp
>--- a/layout/base/nsPresShell.cpp
>+++ b/layout/base/nsPresShell.cpp
>@@ -6086,18 +6086,17 @@ PresShell::HandleEvent(nsIFrame* aFrame,
>     }
>   }
> 
>   if (aEvent->eventStructType == NS_KEY_EVENT &&
>       mDocument && mDocument->EventHandlingSuppressed()) {
>     if (aEvent->message == NS_KEY_DOWN) {
>       mNoDelayedKeyEvents = true;
>     } else if (!mNoDelayedKeyEvents) {
>-      nsDelayedEvent* event =
>-        new nsDelayedKeyEvent(aEvent->AsKeyboardEvent());
>+      DelayedEvent* event = new DelayedKeyEvent(aEvent->AsKeyboardEvent());
>       if (!mDelayedEvents.AppendElement(event)) {
>         delete event;
>       }
>     }
>     return NS_OK;
>   }
> 
>   nsIFrame* frame = aFrame;
>@@ -6307,17 +6306,17 @@ PresShell::HandleEvent(nsIFrame* aFrame,
> 
>     // Suppress mouse event if it's being targeted at an element inside
>     // a document which needs events suppressed
>     if (aEvent->eventStructType == NS_MOUSE_EVENT &&
>         frame->PresContext()->Document()->EventHandlingSuppressed()) {
>       if (aEvent->message == NS_MOUSE_BUTTON_DOWN) {
>         mNoDelayedMouseEvents = true;
>       } else if (!mNoDelayedMouseEvents && aEvent->message == NS_MOUSE_BUTTON_UP) {
>-        nsDelayedEvent* event = new nsDelayedMouseEvent(aEvent->AsMouseEvent());
>+        DelayedEvent* event = new DelayedMouseEvent(aEvent->AsMouseEvent());
>         if (!mDelayedEvents.AppendElement(event)) {
>           delete event;
>         }
>       }
> 
>       return NS_OK;
>     }
> 
>@@ -7600,19 +7599,19 @@ PresShell::FireOrClearDelayedEvents(bool
>     mDelayedEvents.Clear();
>     return;
>   }
> 
>   if (mDocument) {
>     nsCOMPtr<nsIDocument> doc = mDocument;
>     while (!mIsDestroying && mDelayedEvents.Length() &&
>            !doc->EventHandlingSuppressed()) {
>-      nsAutoPtr<nsDelayedEvent> ev(mDelayedEvents[0].forget());
>+      nsAutoPtr<DelayedEvent> ev(mDelayedEvents[0].forget());
>       mDelayedEvents.RemoveElementAt(0);
>-      ev->Dispatch(this);
>+      ev->Dispatch();
>     }
>     if (!doc->EventHandlingSuppressed()) {
>       mDelayedEvents.Clear();
>     }
>   }
> }
> 
> static void
>@@ -8313,16 +8312,66 @@ nsIPresShell::RemovePostRefreshObserver(
>   presContext->RefreshDriver()->RemovePostRefreshObserver(aObserver);
>   return true;
> }
> 
> //------------------------------------------------------
> // End of protected and private methods on the PresShell
> //------------------------------------------------------
> 
>+//------------------------------------------------------------------
>+//-- Delayed event Classes Impls
>+//------------------------------------------------------------------
>+
>+PresShell::DelayedInputEvent::DelayedInputEvent() :
>+  DelayedEvent(),
>+  mEvent(nullptr)
>+{
>+}
>+
>+PresShell::DelayedInputEvent::~DelayedInputEvent()
>+{
>+  delete mEvent;
>+}
>+
>+void
>+PresShell::DelayedInputEvent::Dispatch()
>+{
>+  if (!mEvent || !mEvent->widget) {
>+    return;
>+  }
>+  nsCOMPtr<nsIWidget> widget = mEvent->widget;
>+  nsEventStatus status;
>+  widget->DispatchEvent(mEvent, status);
>+}
>+
>+PresShell::DelayedMouseEvent::DelayedMouseEvent(WidgetMouseEvent* aEvent) :
>+  DelayedInputEvent()
>+{
>+  WidgetMouseEvent* mouseEvent =
>+    new WidgetMouseEvent(aEvent->mFlags.mIsTrusted,
>+                         aEvent->message,
>+                         aEvent->widget,
>+                         aEvent->reason,
>+                         aEvent->context);
>+  mouseEvent->AssignMouseEventData(*aEvent, false);
>+  mEvent = mouseEvent;
>+}
>+
>+PresShell::DelayedKeyEvent::DelayedKeyEvent(WidgetKeyboardEvent* aEvent) :
>+  DelayedInputEvent()
>+{
>+  WidgetKeyboardEvent* keyEvent =
>+    new WidgetKeyboardEvent(aEvent->mFlags.mIsTrusted,
>+                            aEvent->message,
>+                            aEvent->widget);
>+  keyEvent->AssignKeyEventData(*aEvent, false);
>+  mEvent = keyEvent;
>+}
>+
> // Start of DEBUG only code
> 
> #ifdef DEBUG
> 
> static void
> LogVerifyMessage(nsIFrame* k1, nsIFrame* k2, const char* aMsg)
> {
>   nsAutoString n1, n2;
>diff --git a/layout/base/nsPresShell.h b/layout/base/nsPresShell.h
>--- a/layout/base/nsPresShell.h
>+++ b/layout/base/nsPresShell.h
>@@ -29,19 +29,18 @@
> #include "nsCRT.h"
> #include "nsAutoPtr.h"
> #include "nsIWidget.h"
> #include "nsStyleSet.h"
> #include "nsFrameSelection.h"
> #include "nsContentUtils.h" // For AddScriptBlocker().
> #include "nsRefreshDriver.h"
> #include "mozilla/Attributes.h"
>+#include "mozilla/EventForwards.h"
> #include "mozilla/MemoryReporting.h"
>-#include "mozilla/MouseEvents.h"
>-#include "mozilla/TextEvents.h"
> 
> class nsRange;
> class nsIDragService;
> class nsCSSStyleSheet;
> 
> struct RangePaintInfo;
> struct nsCallbackEventRequest;
> #ifdef MOZ_REFLOW_PERF
>@@ -542,77 +541,45 @@ protected:
>     nsresult rv = NS_OK;
>     if (GetCurrentEventFrame()) {
>       rv = HandleEventInternal(aEvent, aStatus);
>     }
>     PopCurrentEventInfo();
>     return rv;
>   }
> 
>-  class nsDelayedEvent
>+  class DelayedEvent
>   {
>   public:
>-    virtual ~nsDelayedEvent() {};
>-    virtual void Dispatch(PresShell* aShell) {}
>+    virtual ~DelayedEvent() { }
>+    virtual void Dispatch() { }
>   };
> 
>-  class nsDelayedInputEvent : public nsDelayedEvent
>+  class DelayedInputEvent : public DelayedEvent
>   {
>   public:
>-    virtual void Dispatch(PresShell* aShell)
>-    {
>-      if (mEvent && mEvent->widget) {
>-        nsCOMPtr<nsIWidget> w = mEvent->widget;
>-        nsEventStatus status;
>-        w->DispatchEvent(mEvent, status);
>-      }
>-    }
>+    virtual void Dispatch() MOZ_OVERRIDE;
> 
>   protected:
>-    nsDelayedInputEvent()
>-    : nsDelayedEvent(), mEvent(nullptr) {}
>-
>-    virtual ~nsDelayedInputEvent()
>-    {
>-      delete mEvent;
>-    }
>+    DelayedInputEvent();
>+    virtual ~DelayedInputEvent();
> 
>     mozilla::WidgetInputEvent* mEvent;
>   };
> 
>-  class nsDelayedMouseEvent : public nsDelayedInputEvent
>+  class DelayedMouseEvent : public DelayedInputEvent
>   {
>   public:
>-    nsDelayedMouseEvent(mozilla::WidgetMouseEvent* aEvent) :
>-      nsDelayedInputEvent()
>-    {
>-      mozilla::WidgetMouseEvent* mouseEvent =
>-        new mozilla::WidgetMouseEvent(aEvent->mFlags.mIsTrusted,
>-                                      aEvent->message,
>-                                      aEvent->widget,
>-                                      aEvent->reason,
>-                                      aEvent->context);
>-      mouseEvent->AssignMouseEventData(*aEvent, false);
>-      mEvent = mouseEvent;
>-    }
>+    DelayedMouseEvent(mozilla::WidgetMouseEvent* aEvent);
>   };
> 
>-  class nsDelayedKeyEvent : public nsDelayedInputEvent
>+  class DelayedKeyEvent : public DelayedInputEvent
>   {
>   public:
>-    nsDelayedKeyEvent(mozilla::WidgetKeyboardEvent* aEvent) :
>-      nsDelayedInputEvent()
>-    {
>-      mozilla::WidgetKeyboardEvent* keyEvent =
>-        new mozilla::WidgetKeyboardEvent(aEvent->mFlags.mIsTrusted,
>-                                         aEvent->message,
>-                                         aEvent->widget);
>-      keyEvent->AssignKeyEventData(*aEvent, false);
>-      mEvent = keyEvent;
>-    }
>+    DelayedKeyEvent(mozilla::WidgetKeyboardEvent* aEvent);
>   };
> 
>   // Check if aEvent is a mouse event and record the mouse location for later
>   // synth mouse moves.
>   void RecordMouseLocation(mozilla::WidgetGUIEvent* aEvent);
>   class nsSynthMouseMoveEvent MOZ_FINAL : public nsARefreshObserver {
>   public:
>     nsSynthMouseMoveEvent(PresShell* aPresShell, bool aFromScroll)
>@@ -754,17 +721,17 @@ protected:
> 
>   // Set of frames that we should mark with NS_FRAME_HAS_DIRTY_CHILDREN after
>   // we finish reflowing mCurrentReflowRoot.
>   nsTHashtable<nsPtrHashKey<nsIFrame> > mFramesToDirty;
> 
>   // Reflow roots that need to be reflowed.
>   nsTArray<nsIFrame*>       mDirtyRoots;
> 
>-  nsTArray<nsAutoPtr<nsDelayedEvent> > mDelayedEvents;
>+  nsTArray<nsAutoPtr<DelayedEvent> > mDelayedEvents;
>   nsRevocableEventPtr<nsRunnableMethod<PresShell> > mResizeEvent;
>   nsCOMPtr<nsITimer>        mAsyncResizeEventTimer;
> private:
>   nsIFrame*                 mCurrentEventFrame;
>   nsCOMPtr<nsIContent>      mCurrentEventContent;
>   nsTArray<nsIFrame*>       mCurrentEventFrameStack;
>   nsCOMArray<nsIContent>    mCurrentEventContentStack;
> protected:

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #12)
> Comment on attachment 819013 [details] [diff] [review]
> part.3 Don't implement some methods which use WidgetEvent in
> nsEventListenerManager.h
> 
> ::HandleEvent is inline for performance reasons, and we should ensure
> IsListening stays inlined too.

Okay, I'll add MOZ_ALWAYS_INLINE.

> So if you can find some proper MOZ_ALWAYS_INLINE macro magic to
> do the inlining when possible, then we can move those methods to .cpp

I've believed that when we put whole body of a method in class declaration, it implicitly specifies just "inline" to the method. I.e., I understand as that using MOZ_ALWAYS_INLINE may be inlined even when the methods have not been inlined with current code.

Ehasn, do you know more details about this? You worked on MOZ_ALWAYS_INLINE at bug 800106.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, making inline nsEventListenerManager::HandleEvent() causes link error...
https://tbpl.mozilla.org/php/getParsedLog.php?id=29484374&tree=Try#error0

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa0f355e7871
https://hg.mozilla.org/integration/mozilla-inbound/rev/4827bdae97fd
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef095c3aef98
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c35693be3d3
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ee56eacc84b
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b9f1062d915
https://hg.mozilla.org/integration/mozilla-inbound/rev/57ca2861ea30
https://hg.mozilla.org/integration/mozilla-inbound/rev/70606b9b1e42
https://hg.mozilla.org/integration/mozilla-inbound/rev/1730bcae2c45

Comment 17

[(no longer active)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #14)
> > So if you can find some proper MOZ_ALWAYS_INLINE macro magic to
> > do the inlining when possible, then we can move those methods to .cpp
> 
> I've believed that when we put whole body of a method in class declaration,
> it implicitly specifies just "inline" to the method. I.e., I understand as
> that using MOZ_ALWAYS_INLINE may be inlined even when the methods have not
> been inlined with current code.

First of all, compilers most often completely ignore the inline keyword, as they often have better information to make inlining decisions than programmers do.  MOZ_ALWAYS_INLINE does indeed cause the function to be force-inlined, but you should use it with caution since sometimes it can screw up other inlining decisions that the compiler makes.  The other thing to note is that if this code is examined during the PGO instrumentation phase, then the compiler may choose to inline the function even if its body lives in a different translation unit.

The gist of the story here is that it's very difficult to predict the compiler's inlining decisions.  When in doubt, just look at the generated code.  :-)

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed it all out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3c5efc154d58 under suspicion of breaking mochitest-metro's text selection tests: https://tbpl.mozilla.org/php/getParsedLog.php?id=29502085&tree=Mozilla-Inbound&full=1

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Hrm, a PGO run prior to this landing just failed that test, so these patches are probably safe to reland as-is after the tree reopens...

Comment 20

[Matt Brubeck (:mbrubeck)]

Re-landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2607bb7e0a30
https://hg.mozilla.org/integration/mozilla-inbound/rev/64a6e6020d05
https://hg.mozilla.org/integration/mozilla-inbound/rev/b122fa9825ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/e47912877f80
https://hg.mozilla.org/integration/mozilla-inbound/rev/574ba6718462
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8263a460d24
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f497027e202
https://hg.mozilla.org/integration/mozilla-inbound/rev/55c6ec50d580
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a8a8a7e9583
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a22cfe165fa

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2607bb7e0a30
https://hg.mozilla.org/mozilla-central/rev/64a6e6020d05
https://hg.mozilla.org/mozilla-central/rev/b122fa9825ee
https://hg.mozilla.org/mozilla-central/rev/e47912877f80
https://hg.mozilla.org/mozilla-central/rev/574ba6718462
https://hg.mozilla.org/mozilla-central/rev/b8263a460d24
https://hg.mozilla.org/mozilla-central/rev/8f497027e202
https://hg.mozilla.org/mozilla-central/rev/55c6ec50d580
https://hg.mozilla.org/mozilla-central/rev/3a8a8a7e9583

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #17)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #14)
> > > So if you can find some proper MOZ_ALWAYS_INLINE macro magic to
> > > do the inlining when possible, then we can move those methods to .cpp
> > 
> > I've believed that when we put whole body of a method in class declaration,
> > it implicitly specifies just "inline" to the method. I.e., I understand as
> > that using MOZ_ALWAYS_INLINE may be inlined even when the methods have not
> > been inlined with current code.
> 
> First of all, compilers most often completely ignore the inline keyword, as
> they often have better information to make inlining decisions than
> programmers do.  MOZ_ALWAYS_INLINE does indeed cause the function to be
> force-inlined, but you should use it with caution since sometimes it can
> screw up other inlining decisions that the compiler makes.  The other thing
> to note is that if this code is examined during the PGO instrumentation
> phase, then the compiler may choose to inline the function even if its body
> lives in a different translation unit.
> 
> The gist of the story here is that it's very difficult to predict the
> compiler's inlining decisions.  When in doubt, just look at the generated
> code.  :-)

Thank you for your reply. I research about this case but my conclusion of this case is, the method is actually inlined and it's impossible to move the method body to the .cpp because nsEventListenerManager.h is exported.

Therefore, I researched if we can fix nsEventListenerManager.h's include hell. However, nsEventListenerManager is stored with nsRefPtr in 4 classes (nsDocument.h, nsDOMEventTargetHelper.h, nsGlobalWindow.h and nsWindowRoot.h). So, this is one of include hell :(

Anyway, I give up to fix this bug in nsEventListenerManager.h. I think that we need to sort out around this class for fixing this include hell.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Well, HandleEvent should be called only by nsEventDispatcher.cpp.
You could in theory put HandleEvent implementation to some other header file and include
that in nsEventDispatcher.cpp

Comment 24

[(no longer active)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #22)
> (In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment
> #17)
> > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #14)
> > > > So if you can find some proper MOZ_ALWAYS_INLINE macro magic to
> > > > do the inlining when possible, then we can move those methods to .cpp
> > > 
> > > I've believed that when we put whole body of a method in class declaration,
> > > it implicitly specifies just "inline" to the method. I.e., I understand as
> > > that using MOZ_ALWAYS_INLINE may be inlined even when the methods have not
> > > been inlined with current code.
> > 
> > First of all, compilers most often completely ignore the inline keyword, as
> > they often have better information to make inlining decisions than
> > programmers do.  MOZ_ALWAYS_INLINE does indeed cause the function to be
> > force-inlined, but you should use it with caution since sometimes it can
> > screw up other inlining decisions that the compiler makes.  The other thing
> > to note is that if this code is examined during the PGO instrumentation
> > phase, then the compiler may choose to inline the function even if its body
> > lives in a different translation unit.
> > 
> > The gist of the story here is that it's very difficult to predict the
> > compiler's inlining decisions.  When in doubt, just look at the generated
> > code.  :-)
> 
> Thank you for your reply. I research about this case but my conclusion of
> this case is, the method is actually inlined and it's impossible to move the
> method body to the .cpp because nsEventListenerManager.h is exported.
> 
> Therefore, I researched if we can fix nsEventListenerManager.h's include
> hell. However, nsEventListenerManager is stored with nsRefPtr in 4 classes
> (nsDocument.h, nsDOMEventTargetHelper.h, nsGlobalWindow.h and
> nsWindowRoot.h). So, this is one of include hell :(

Two of those four #includes are very easy to remove, see bug 930583.  :-)

Comment 25

[Karl Tomlinson (:karlt)]

(In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #24)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #22)
> > (In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment
> > #17)
> > > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #14)
> > > > > So if you can find some proper MOZ_ALWAYS_INLINE macro magic to
> > > > > do the inlining when possible, then we can move those methods to .cpp
> > > > 
> > > > I've believed that when we put whole body of a method in class declaration,
> > > > it implicitly specifies just "inline" to the method. I.e., I understand as
> > > > that using MOZ_ALWAYS_INLINE may be inlined even when the methods have not
> > > > been inlined with current code.
> > > 
> > > First of all, compilers most often completely ignore the inline keyword, as
> > > they often have better information to make inlining decisions than
> > > programmers do.  MOZ_ALWAYS_INLINE does indeed cause the function to be
> > > force-inlined, but you should use it with caution since sometimes it can
> > > screw up other inlining decisions that the compiler makes.  The other thing
> > > to note is that if this code is examined during the PGO instrumentation
> > > phase, then the compiler may choose to inline the function even if its body
> > > lives in a different translation unit.
> > > 
> > > The gist of the story here is that it's very difficult to predict the
> > > compiler's inlining decisions.  When in doubt, just look at the generated
> > > code.  :-)
> > 
> > Thank you for your reply. I research about this case but my conclusion of
> > this case is, the method is actually inlined and it's impossible to move the
> > method body to the .cpp because nsEventListenerManager.h is exported.
> > 
> > Therefore, I researched if we can fix nsEventListenerManager.h's include
> > hell. However, nsEventListenerManager is stored with nsRefPtr in 4 classes
> > (nsDocument.h, nsDOMEventTargetHelper.h, nsGlobalWindow.h and
> > nsWindowRoot.h). So, this is one of include hell :(
> 
> Two of those four #includes are very easy to remove, see bug 930583.  :-)

If it is OK to uninline the nsDOMEventTargetHelper constructors, then the nsRefPtr there won't be a problem.  I don't know what the blocker is in nsGlobalWindow.h.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

It is ok to un-inline nsDOMEventTargetHelper ctors.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.11 nsDOMEvent.h should not include BasicEvents.h directly

Some methods of nsIDOMEvent needs WidgetEvent for implementing them. If we don't use NS_FORWARD_TO_NSIDOMEVENT and implement them in .cpp, we can improve the include hell around DOM*Event.h.

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Remaining headers are:

widget/cocoa/nsChildView.h
widget/os2/nsWindow.h
widget/windows/nsWindow.h
widget/windows/nsWinGesture.h

They are using enums defined in WidgetMouseEvent and WidgetGetstureNotifyEvent for arguments of some methods. So, we need to move them to separated header file or using enum class and forward declaration. I don't like to create new header file since these headers cause include hell not so seriously. Additionally, we cannot use enum class without macro magic. Therefore, it needs additional ifdef in nsGUIEventIPC.h. I don't want to do it. So, it is not time to fix this bug in them.

layout/style/nsAnimationManager.h

AnimationEventInfo has InternalAnimationEvent member. So, it really need to include ContentEvents.h.

So, the part.11 is the last patch of this bug for now.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 822097 [details] [diff] [review]
part.11 nsDOMEvent.h should not include BasicEvents.h directly

It is unfortunate that various getters in nsDOMEvent are un-inlined.
But I guess they aren't that performance critical.
And unfortunate to make implementing events more complicated.
How much does this actually help?

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #29)
> Comment on attachment 822097 [details] [diff] [review]
> part.11 nsDOMEvent.h should not include BasicEvents.h directly
> 
> It is unfortunate that various getters in nsDOMEvent are un-inlined.
> But I guess they aren't that performance critical.

If you meant that "various getters" are non-virtual methods which come from webidl, unfortunately, we can use neither MOZ_INLINE nor MOZ_ALWAYS_INLINE in nsDOMEvent.h and nsDOMUIEvent.h since they are exported. But as you say, they must not cause harming the performance.

If you meant that they are virtual methods which are come from nsIDOMEvent, they must be never inlined because they are virtual methods which are solved at run-time.

> And unfortunate to make implementing events more complicated.
> How much does this actually help?

nsDOM*Event.h are included by about 70 files (except nsDOM*Event.cpp). And about 10 files of them are header file. So, I believe that this increases the rebuild cost.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #30)
> If you meant that "various getters" are non-virtual methods which come from
> webidl, unfortunately, we can use neither MOZ_INLINE nor MOZ_ALWAYS_INLINE
> in nsDOMEvent.h and nsDOMUIEvent.h since they are exported. But as you say,
> they must not cause harming the performance.
Yeah, I meant webidl case


> nsDOM*Event.h are included by about 70 files (except nsDOM*Event.cpp). And
> about 10 files of them are header file. So, I believe that this increases
> the rebuild cost.
Do we actually need to include nsDOMEvent in those headers? That doesn't sounds right.

Have you done any measurements how much the patch helps with rebuild?
I know fast rebuilds are important but so is easy-to-read-and-write code.

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #31)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #30)
> > nsDOM*Event.h are included by about 70 files (except nsDOM*Event.cpp). And
> > about 10 files of them are header file. So, I believe that this increases
> > the rebuild cost.
> Do we actually need to include nsDOMEvent in those headers? That doesn't
> sounds right.

Okay, I'll check it.

> Have you done any measurements how much the patch helps with rebuild?

Actually, no. I'll check the actual build time difference on slow machine.

> I know fast rebuilds are important but so is easy-to-read-and-write code.

I don't think that my patch makes them more difficult, though... because the patch just moves the forward declaration and inline methods into cpp files.

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #31)
> > nsDOM*Event.h are included by about 70 files (except nsDOM*Event.cpp). And
> > about 10 files of them are header file. So, I believe that this increases
> > the rebuild cost.
> Do we actually need to include nsDOMEvent in those headers? That doesn't
> sounds right.
> 
> Have you done any measurements how much the patch helps with rebuild?
> I know fast rebuilds are important but so is easy-to-read-and-write code.

I confirmed that the build time difference with/without the patch part.11.

On one of my machines which is the slowest one takes about 50 min at changing BasicEvents.h. When I applied part.11, sometimes the build time is shorter than without the patch. However, even with the patch, the improvement only less than 5 min. So, it isn't worthwhile to land it for now.

I'm marking this bug as FIXED.