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

RESOLVED FIXED in mozilla27

Status

()

Core
Graphics
RESOLVED FIXED
4 years ago
4 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)

Created attachment 801169 [details] [diff] [review]
gfxTypes

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)
(Assignee)

Comment 1

4 years ago
Created attachment 801170 [details] [diff] [review]
Move enums to gfxTypes.h

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)
(Assignee)

Comment 2

4 years ago
Created attachment 801171 [details] [diff] [review]
Adapt the codebase to these changes (generated by a regex)

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)
(Assignee)

Comment 3

4 years ago
Created attachment 801172 [details] [diff] [review]
Adapt the codebase to these changes (generated by a regex)

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)
(Assignee)

Comment 4

4 years ago
Argh, there is already a mozilla::gfx::SurfaceType in gfx/2d/Types.h... sadness. Cancelling reviews for now.
(Assignee)

Updated

4 years ago
Attachment #801170 - Flags: review?(jmuizelaar)
(Assignee)

Updated

4 years ago
Attachment #801172 - Flags: review?(jmuizelaar)
(Assignee)

Comment 5

4 years ago
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: 4 years ago
Resolution: --- → WONTFIX
(Assignee)

Comment 6

4 years ago
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 → ---
(Assignee)

Comment 7

4 years ago
Created attachment 808540 [details] [diff] [review]
The automatic changes (only review the regexes)

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)
(Assignee)

Comment 8

4 years ago
Created attachment 808541 [details] [diff] [review]
The manual changes (mostly moving the enums out of gfxASurface and prefixing the values)
Attachment #801170 - Attachment is obsolete: true
Attachment #801172 - Attachment is obsolete: true
Attachment #808541 - Flags: review?(jmuizelaar)
(Assignee)

Comment 9

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=f4efab822ecf
Created attachment 808546 [details] [diff] [review]
The manual changes (mostly moving the enums out of gfxASurface and prefixing the values)

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)
(Assignee)

Updated

4 years ago
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
Created attachment 808567 [details] [diff] [review]
Remove useless inclusions of gfxASurface.h
Attachment #808567 - Flags: review?(jmuizelaar)
Attachment #808540 - Flags: review?(jmuizelaar) → review+
Attachment #808546 - Flags: review?(jmuizelaar) → review+
Attachment #808567 - Flags: review?(jmuizelaar) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/1892aa2a6de8
http://hg.mozilla.org/integration/mozilla-inbound/rev/0f86bf03656b
http://hg.mozilla.org/integration/mozilla-inbound/rev/7588ab535671
Assignee: nobody → bjacob
Target Milestone: --- → mozilla27
(Assignee)

Updated

4 years ago
Blocks: 785103
https://hg.mozilla.org/mozilla-central/rev/1892aa2a6de8
https://hg.mozilla.org/mozilla-central/rev/0f86bf03656b
https://hg.mozilla.org/mozilla-central/rev/7588ab535671
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.