907798 - Do not include TimeStamp.h in files that do not need it

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Gabriele Svelto [:gsvelto] (PTO)]

While working on the TimeStamp class I noticed that touching the header file would clobber half of my build; further investigation showed to me that TimeStamp.h is being included by a number of source files (and headers) that do not need it or could live with forward declarations of the TimeStamp and TimeDuration types.

The files from which the header can be removed seem to be the following:

content/media/plugins/MediaPluginHost.cpp
dom/plugins/base/nsPluginTags.cpp
gfx/layers/composite/LayerManagerComposite.cpp
gfx/layers/composite/LayerManagerComposite.h
gfx/layers/ipc/PLayerTransaction.ipdl
gfx/layers/opengl/LayerManagerOGL.h
image/src/FrameBlender.h
image/src/VectorImage.h
netwerk/protocol/http/nsHttpConnection.h
widget/windows/winrt/FrameworkView.h
widget/windows/winrt/MetroAppShell.h
xpcom/build/XPCOM.h

The files that need forward declarations are:

gfx/layers/opengl/CompositorOGL.h
tools/profiler/GeckoProfiler.h
tools/profiler/GeckoProfilerFunc.h
tools/profiler/GeckoProfilerImpl.h
xpcom/threads/TimerThread.h

Comment 1

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH] Remove TimeStamp.h includes from source files that do not need it

Tentative patch which builds correctly under Linux; I still have to do some more rigorous testing of this.

Comment 2

[(no longer active)]

Comment on attachment 793539 [details] [diff] [review]
[PATCH] Remove TimeStamp.h includes from source files that do not need it

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

r=me when you get this to a stage where it builds everywhere!

Please request for review again if you make any non-trivial changes in the process.  Thanks!

Comment 3

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH v2] Remove TimeStamp.h includes from source files that do not need it

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> r=me when you get this to a stage where it builds everywhere!
> 
> Please request for review again if you make any non-trivial changes in the
> process.  Thanks!

I had to refresh the patch because a couple of headers have already been removed in previous commits but for the rest this is identical. I've launched a try-run of the build on all our major architectures:

https://tbpl.mozilla.org/?tree=Try&rev=98e2a2947b68

I had done one already with the previous patch and it turned out green everywhere:

https://tbpl.mozilla.org/?tree=Try&rev=6cc7656c161e

Is this enough or shall I build on more platforms?

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Gabriele Svelto [:gsvelto] from comment #3)
> Is this enough or shall I build on more platforms?

You may want to build for B2G just to make sure you're not going to mysteriously shoot yourself in the foot.  Otherwise, this is sufficient.

Comment 5

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH v3] Remove TimeStamp.h includes from source files that do not need it

Refreshed the patch again as it's bit-rotting real fast; this time I've pushed to try with all platforms including B2G just in case:

https://tbpl.mozilla.org/?tree=Try&rev=034cbda04980

If this sticks I'll ask for check-in.

Comment 6

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH v4] Remove TimeStamp.h includes from source files that do not need it

Fourth refresh and this time the patch hasn't bit-rotted before the try run completed :)

https://tbpl.mozilla.org/?tree=Try&rev=71ad2984a615

This is r=ehsan as per comment 2, I've only refreshed the patch since then but no other changes were made.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22d54c058e2a

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Reverted to clear the tree:

https://hg.mozilla.org/integration/mozilla-inbound/rev/48a3aeb26ed6

and relanded:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f85745c5f34c

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/22d54c058e2a
https://hg.mozilla.org/mozilla-central/rev/f85745c5f34c