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 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.

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.

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.

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

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

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

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

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

KeyboardLayout.h will be fixed by later patch.

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

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 on attachment 819009 [details] [diff] [review] part.1 Don't implement PresShell::Delayed*Event class in nsPresShell.h ># HG changeset patch ># User Masayuki Nakano <masayuki@d-toybox.com> ># 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:

(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.

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

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

(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. :-)

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

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...

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

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

(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.

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

(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. :-)

(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.

It is ok to un-inline nsDOMEventTargetHelper ctors.

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.

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 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?

(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.

(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.

(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.

(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.