Closed Bug 912042 Opened 7 years ago Closed 7 years ago

Avoid including GLContext.h in headers that don't need it

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: bjacob, Assigned: bjacob)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

GLContext.h is a big header, itself including many headers.

It's worth avoiding #including it where we don't actually need it, especially in other headers.

After this patch, here are the only remaining headers #including GLContext.h:

  $ find . -type f -name '*.h' | xargs egrep '\#include.*[^a-zA-Z0-9_]GLContext\.h' | cut -d ':' -f 1 | sort | uniq | sed 's|\.\/||g'

  gfx/layers/CopyableCanvasLayer.h
  gfx/layers/d3d9/CanvasLayerD3D9.h
  gfx/layers/SharedTextureImage.h

These 3 headers are included only once or a few times each.

Now, touching GLContext.h and rebuilding Gecko only rebuilds 58 object files, 23 of which are WebGL.

Speaking of WebGL, it currently has 45 object files; prior to this patch, they were all including GLContext.h, now only 23 of them are (as just mentioned above).

A few changes were made to and around GLContext.h along the way:

* ExtensionBitSet had a comment explaining it should be a std::bitset but that used to give compile errors on Mac, referencing an old bug. Hopefully that's resolved now. Killed ExtensionBitSet, now using std::bitset, and the function ExtensionBitSet::Load is now a static GLContext method, InitializeExtensionsBitSet.

* GLContext contained a few nested enum types. Nested enum types are impossible (AFAIK) to forward-declare. Moved them out of GLContext, still in namespace gl.

* These enums were moved to GLContextTypes.h which also gained a forward-decl of class GLContext, making it 'the right header to include' for most people.

* Of course, lots of code was moved from inline functions in headers into cpp files. A couple cpp files were created as some classes were entirely defined in headers.

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=fbe08467d685
Attachment #798851 - Flags: review?(jmuizelaar)
Errors caused by -Werror and double #defines, not worth updating the patch for review, but updated try push:
https://tbpl.mozilla.org/?tree=Try&rev=d2b6c836ec23
As long as there are tryservers, there is hope.
https://tbpl.mozilla.org/?tree=Try&rev=1d5aba99f1f6
New push for Win and Mac, which were still failing:
https://tbpl.mozilla.org/?tree=Try&rev=dd7c30c8fad3
Attachment #798851 - Flags: review?(jmuizelaar) → review+
Hopefully last try run, with some tests:
https://tbpl.mozilla.org/?tree=Try&rev=30661fa2a517
Green at last! landing!
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c04fad82d85
Assignee: nobody → bjacob
Target Milestone: --- → mozilla26
Blocks: includehell
There is, after all, no reason to stop half-way! There remained a few headers including GLContext.h, and on second look fixing it was easy too.

Now we are able to give people a very simple message: do not include GLContext.h in headers; instead, include GLContextTypes.h.

On try:
https://tbpl.mozilla.org/?tree=Try&rev=7ec397814580
Attachment #800125 - Flags: review?(jmuizelaar)
Blocks: 912974
Forgot to include GLContext.h in CanvasLayerD3D9.cpp, new try:
https://tbpl.mozilla.org/?tree=Try&rev=3b7aa76f1778
Attachment #800125 - Flags: review?(jmuizelaar) → review+
You need to log in before you can comment on or make changes to this bug.