Take nested enums out of gfxASurface, into gfxTypes.h, and remove useless includes of gfxASurface.h

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

(Blocks 1 bug)

Trunk
mozilla27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

Posted patch gfxTypes (obsolete) — Splinter Review
gfxASurface.h is not currently a cheap header to include. For example it includes nsThreadUtils.h because it defines a nsRunnable subclass.

In order to allow other headers to avoid including it, let's take some nested enums out of class gfxASurface, into the cheap gfxTypes.h header.

The rest of this patch is just generated by these commands:

$ find . -name '*.h' | xargs sed -i 's|gfxASurface\:\:gfxSurfaceType|gfxSurfaceType|g'
$ find . -name '*.cpp' | xargs sed -i 's|gfxASurface\:\:gfxSurfaceType|gfxSurfaceType|g'
$ find . -name '*.h' | xargs sed -i 's|gfxASurface\:\:gfxImageFormat|gfxImageFormat|g'
$ find . -name '*.cpp' | xargs sed -i 's|gfxASurface\:\:gfxImageFormat|gfxImageFormat|g'
$ find . -name '*.h' | xargs sed -i 's|gfxASurface\:\:gfxContentType|gfxContentType|g'
$ find . -name '*.cpp' | xargs sed -i 's|gfxASurface\:\:gfxContentType|gfxContentType|g'

I promise.
Attachment #801169 - Flags: review?(jmuizelaar)
Posted patch Move enums to gfxTypes.h (obsolete) — Splinter Review
Let's try again. This new patch:
 - only moves the enum, doesn't adapt the whole codebase (that will be the next patch). So it doesn't build by itself.
 - puts them into namespace mozilla::gfx and removes the gfx prefixes. Indeed, the enum values didn't have a prefix, and we don't want to running into silly bugs with conflicting enums.
Attachment #801169 - Attachment is obsolete: true
Attachment #801169 - Flags: review?(jmuizelaar)
Attachment #801170 - Flags: review?(jmuizelaar)
You probably want to only review the command that generated this diff:

 $ find gfx -name '*.h' -o -name '*.cpp' | xargs sed -i 's/gfxASurface\:\:\(\(gfx\)\?\(CONTENT_\|ImageFormat\|SurfaceType\|ContentType\)[0-9A-Za-z_]*\)/gfx\:\:\1/g'
Attachment #801171 - Flags: review?(jmuizelaar)
Argh, the previous command ran only on the gfx/ directory... was my fast debugging setup.

This one runs over all the code:

 $ find . -name '*.h' -o -name '*.cpp' | xargs sed -i 's/gfxASurface\:\:\(\(gfx\)\?\(CONTENT_\|ImageFormat\|SurfaceType\|ContentType\)[0-9A-Za-z_]*\)/gfx\:\:\1/g'
Attachment #801171 - Attachment is obsolete: true
Attachment #801171 - Flags: review?(jmuizelaar)
Attachment #801172 - Flags: review?(jmuizelaar)
Argh, there is already a mozilla::gfx::SurfaceType in gfx/2d/Types.h... sadness. Cancelling reviews for now.
Attachment #801170 - Flags: review?(jmuizelaar)
Attachment #801172 - Flags: review?(jmuizelaar)
I think for now this is going to be a WONTFIX. I'm taking a different approach, which is to trim gfxASurface to make it cheap to include, rather than to avoid including it at all costs.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX
Reopening! Found a way out.

Let's keep these silly gfx prefixes so we don't run into any conflicts. Let's even add gfx prefixes to the enum values (so far, only the enum _types_ had gfx prefixes).

So

class gfxASurface {
  enum gfxImageFormat {
    ImageFormatJeff
  };
};

becomes

// gfxTypes.h
enum gfxImageFormat {
  gfxImageFormatJeff
};

That's actually also a good thing for another reason: there is already a ImageFormat enum (the thing that can be PLANAR_YCBCR) so it was actually very confusing to have the present enum values start in ImageFormatXXX without gfx.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
I promise that this is generated by the following commands:

\(ImageFormat\|SurfaceType\|ContentType\|MemoryLocation\)[0-9A-Za-z_]*\)/gfx\1/g'

find . -name '*.h' -o -name '*.cpp' -o -name '*.mm' | grep -v '^\.\/obj' | xargs sed -i 's/gfx[A-Za-z0-9_]*Surface\:\:[a-z]*\(\(CONTENT_\|MEMORY_\)[0-9A-Za-z_]*\)/GFX_\1/g'

find . -name '*.h' -o -name '*.cpp' -o -name '*.mm' | grep -v '^\.\/obj' | xargs sed -i 's/\(^\|[^A-Za-z0-9_]\)\(CONTENT_COLOR\|CONTENT_ALPHA\|CONTENT_COLOR_ALPHA\|CONTENT_SENTINEL\|MEMORY_IN_PROCESS_HEAP\|MEMORY_IN_PROCESS_NONHEAP\|MEMORY_OUT_OF_PROCESS\)\($\|[^A-Za-z0-9_]\)/\1GFX_\2\3/g'

find . -name '*.h' -o -name '*.cpp' -o -name '*.mm' | grep -v '^\.\/obj' | xargs sed -i 's/\(^\|[^A-Za-z0-9_]\)\(ImageFormatARGB32\|ImageFormatRGB24\|ImageFormatA8\|ImageFormatA1\|ImageFormatRGB16_565\|ImageFormatUnknown\|SurfaceTypeImage\|SurfaceTypePDF\|SurfaceTypePS\|SurfaceTypeXlib\|SurfaceTypeXcb\|SurfaceTypeGlitz\|SurfaceTypeQuartz\|SurfaceTypeWin32\|SurfaceTypeBeOS\|SurfaceTypeDirectFB\|SurfaceTypeSVG\|SurfaceTypeOS2\|SurfaceTypeWin32Printing\|SurfaceTypeQuartzImage\|SurfaceTypeScript\|SurfaceTypeQPainter\|SurfaceTypeRecording\|SurfaceTypeVG\|SurfaceTypeGL\|SurfaceTypeDRM\|SurfaceTypeTee\|SurfaceTypeXML\|SurfaceTypeSkia\|SurfaceTypeSubsurface\|SurfaceTypeD2D\|SurfaceTypeMax\)\($\|[^A-Za-z0-9_]\)/\1gfx\2\3/g'
Attachment #808540 - Flags: review?(jmuizelaar)
Attachment #801170 - Attachment is obsolete: true
Attachment #801172 - Attachment is obsolete: true
Attachment #808541 - Flags: review?(jmuizelaar)
Forgot to remove a couple of tautological typedefs. 

https://tbpl.mozilla.org/?tree=Try&rev=d4a6992fa472
Attachment #808541 - Attachment is obsolete: true
Attachment #808541 - Flags: review?(jmuizelaar)
Attachment #808546 - Flags: review?(jmuizelaar)
Summary: Take nested enums out of gfxASurface, into gfxTypes.h → Take nested enums out of gfxASurface, into gfxTypes.h, and remove useless includes of gfxASurface.h
Attachment #808540 - Flags: review?(jmuizelaar) → review+
Attachment #808546 - Flags: review?(jmuizelaar) → review+
Attachment #808567 - Flags: review?(jmuizelaar) → review+
You need to log in before you can comment on or make changes to this bug.