Closed Bug 958375 Opened 6 years ago Closed 6 years ago

Make Moz2D enums typed

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: bjacob, Assigned: bjacob)

References

Details

(Whiteboard: [qa-])

Attachments

(14 files)

2.08 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
28.69 KB, patch
bas.schouten
: feedback+
Details | Diff | Splinter Review
1.58 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
2.31 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
3.51 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
172.58 KB, patch
bas.schouten
: feedback+
Details | Diff | Splinter Review
2.24 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
46.09 KB, patch
bas.schouten
: feedback+
Details | Diff | Splinter Review
12.58 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
2.20 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
92.46 KB, patch
bas.schouten
: feedback+
Details | Diff | Splinter Review
23.15 KB, patch
bas.schouten
: feedback+
Details | Diff | Splinter Review
306.64 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
1.18 KB, patch
roc
: review+
Details | Diff | Splinter Review
Recent evidence for this being useful includes:

 1. Early version of the patches on bug 877115 had buggy code like this:
      gfxImageFormat(aSurfaceFormat)
   which silently ran as these enums weren't typed.

 2.  nsBlockFrame::Reflow has a bug where NS_SIDE_TOP is being mis-interpreted as a bit-field (what is meant is 1<<NS_SIDE_TOP instead), and this runs silently because mozilla::css::Side isn't typed.  (Patch coming below).
The problem is that when T depends on template parameters (or is a template parameter itself), we have to write

  typename T::Enum

to get at the nested Enum type.
Attachment #8358137 - Flags: review?(jwalden+bmo)
Attachment #8358142 - Flags: review?(bas) → feedback?(bas)
Attachment #8358147 - Flags: review?(bas) → feedback?(bas)
Sorry, things went wrong and I failed to keep the automatic changes well separated from manual ones.

What you have to know is that the hairiest part, by far, was mozilla::css::Side, which is used heavily in layout code, where it is often reinterpreted, sometimes as indexing arrays, sometimes as indexing bitfields. So I had to replace 'side' by 'int(side)' or 'size_t(side)'  (hoping that size_t implicitly conveys the notion that this is an array index).

This was worth it, though: this discovered a real bug in layout code, which is the subject of the next patch.
Attachment #8358156 - Flags: review?(bas)
 accessible/src/base/StyleInfo.h                      |    8 +-
 content/canvas/src/CanvasGradient.h                  |    2 +-
 content/canvas/src/CanvasRenderingContext2D.cpp      |  104 ++--
 content/canvas/src/CanvasRenderingContext2D.h        |   10 +-
 content/canvas/src/WebGLContextGL.cpp                |    8 +-
 content/svg/content/src/SVGPathData.cpp              |    2 +-
 content/svg/content/src/nsSVGPathGeometryElement.cpp |    4 +-
 gfx/2d/2D.h                                          |   46 +-
 gfx/2d/BaseMargin.h                                  |    4 +-
 gfx/2d/DataSourceSurface.cpp                         |    2 +-
 gfx/2d/DataSourceSurfaceWrapper.h                    |    4 +-
 gfx/2d/DataSurfaceHelpers.h                          |    4 +-
 gfx/2d/DrawTargetCG.cpp                              |  176 ++++----
 gfx/2d/DrawTargetCG.h                                |   16 +-
 gfx/2d/DrawTargetCairo.cpp                           |   54 +-
 gfx/2d/DrawTargetCairo.h                             |    8 +-
 gfx/2d/DrawTargetD2D.cpp                             |  102 ++--
 gfx/2d/DrawTargetD2D.h                               |    8 +-
 gfx/2d/DrawTargetD2D1.cpp                            |   50 +-
 gfx/2d/DrawTargetD2D1.h                              |    6 +-
 gfx/2d/DrawTargetDual.cpp                            |    6 +-
 gfx/2d/DrawTargetDual.h                              |    4 +-
 gfx/2d/DrawTargetRecording.cpp                       |   20 +-
 gfx/2d/DrawTargetRecording.h                         |    4 +-
 gfx/2d/DrawTargetSkia.cpp                            |   56 +-
 gfx/2d/DrawTargetSkia.h                              |    6 +-
 gfx/2d/Factory.cpp                                   |   32 +-
 gfx/2d/FilterNodeD2D1.cpp                            |  168 ++++----
 gfx/2d/FilterNodeSoftware.cpp                        |  114 ++--
 gfx/2d/FilterNodeSoftware.h                          |    8 +-
 gfx/2d/FilterProcessing.cpp                          |   12 +-
 gfx/2d/FilterProcessingSIMD-inl.h                    |   18 +-
 gfx/2d/FilterProcessingScalar.cpp                    |    2 +-
 gfx/2d/GradientStopsD2D.h                            |    2 +-
 gfx/2d/HelpersCairo.h                                |  116 ++--
 gfx/2d/HelpersD2D.h                                  |   68 +-
 gfx/2d/HelpersSkia.h                                 |   96 ++--
 gfx/2d/MacIOSurface.cpp                              |    4 +-
 gfx/2d/PathCG.cpp                                    |    2 +-
 gfx/2d/PathCG.h                                      |   10 +-
 gfx/2d/PathCairo.cpp                                 |    2 +-
 gfx/2d/PathCairo.h                                   |    6 +-
 gfx/2d/PathD2D.cpp                                   |    2 +-
 gfx/2d/PathD2D.h                                     |    6 +-
 gfx/2d/PathRecording.h                               |    6 +-
 gfx/2d/PathSkia.cpp                                  |    2 +-
 gfx/2d/PathSkia.h                                    |    6 +-
 gfx/2d/RecordedEvent.cpp                             |   46 +-
 gfx/2d/SVGTurbulenceRenderer-inl.h                   |    2 +-
 gfx/2d/Scale.cpp                                     |    2 +-
 gfx/2d/ScaledFontBase.cpp                            |   12 +-
 gfx/2d/ScaledFontBase.h                              |    2 +-
 gfx/2d/ScaledFontCairo.h                             |    4 +-
 gfx/2d/ScaledFontDWrite.cpp                          |   16 +-
 gfx/2d/ScaledFontDWrite.h                            |    4 +-
 gfx/2d/ScaledFontMac.cpp                             |    6 +-
 gfx/2d/ScaledFontMac.h                               |    2 +-
 gfx/2d/ScaledFontWin.h                               |    2 +-
 gfx/2d/SourceSurfaceCG.cpp                           |   18 +-
 gfx/2d/SourceSurfaceCG.h                             |    8 +-
 gfx/2d/SourceSurfaceCairo.cpp                        |   10 +-
 gfx/2d/SourceSurfaceCairo.h                          |    4 +-
 gfx/2d/SourceSurfaceD2D.h                            |    2 +-
 gfx/2d/SourceSurfaceD2D1.h                           |    4 +-
 gfx/2d/SourceSurfaceD2DTarget.cpp                    |    2 +-
 gfx/2d/SourceSurfaceD2DTarget.h                      |    4 +-
 gfx/2d/SourceSurfaceDual.h                           |    2 +-
 gfx/2d/SourceSurfaceRawData.h                        |    4 +-
 gfx/2d/SourceSurfaceSkia.cpp                         |    2 +-
 gfx/2d/SourceSurfaceSkia.h                           |    2 +-
 gfx/2d/Tools.h                                       |   14 +-
 gfx/2d/Types.h                                       |  307 +++++++++-----
 gfx/2d/unittest/TestBugs.cpp                         |   12 +-
 gfx/2d/unittest/TestDrawTargetD2D.cpp                |    2 +-
 gfx/cairo/cairo/NEWS                                 |    4 +-
 gfx/cairo/cairo/src/cairo-qt-surface.cpp             |    4 +-
 gfx/cairo/cairo/src/cairo-script-surface.c           |    6 +-
 gfx/cairo/cairo/src/cairo-xml-surface.c              |    6 +-
 gfx/gl/GLReadTexImageHelper.cpp                      |    8 +-
 gfx/gl/GLSharedHandleHelpers.cpp                     |    4 +-
 gfx/gl/GLUploadHelpers.cpp                           |   22 +-
 gfx/gl/SharedSurfaceEGL.cpp                          |    4 +-
 gfx/gl/SharedSurfaceGL.cpp                           |    8 +-
 gfx/gl/TextureImageEGL.cpp                           |    6 +-
 gfx/ipc/GfxMessageUtils.h                            |    6 +-
 gfx/layers/Effects.h                                 |   18 +-
 gfx/layers/Layers.cpp                                |    2 +-
 gfx/layers/LayersLogging.cpp                         |   22 +-
 gfx/layers/RotatedBuffer.cpp                         |   28 +-
 gfx/layers/RotatedBuffer.h                           |    2 +-
 gfx/layers/YCbCrImageDataSerializer.cpp              |    2 +-
 gfx/layers/basic/BasicCompositor.cpp                 |   16 +-
 gfx/layers/basic/BasicCompositor.h                   |    2 +-
 gfx/layers/basic/BasicLayerManager.cpp               |    4 +-
 gfx/layers/basic/MacIOSurfaceTextureHostBasic.cpp    |    4 +-
 gfx/layers/client/ContentClient.cpp                  |    8 +-
 gfx/layers/client/ContentClient.h                    |    2 +-
 gfx/layers/client/ImageClient.cpp                    |    2 +-
 gfx/layers/client/TextureClient.cpp                  |    8 +-
 gfx/layers/client/TextureClient.h                    |    4 +-
 gfx/layers/composite/LayerManagerComposite.cpp       |    2 +-
 gfx/layers/composite/TextureHost.cpp                 |   20 +-
 gfx/layers/composite/TextureHost.h                   |    4 +-
 gfx/layers/composite/ThebesLayerComposite.cpp        |    4 +-
 gfx/layers/d3d10/CanvasLayerD3D10.cpp                |    6 +-
 gfx/layers/d3d10/LayerManagerD3D10.cpp               |    6 +-
 gfx/layers/d3d10/ThebesLayerD3D10.cpp                |    6 +-
 gfx/layers/d3d11/CompositorD3D11.cpp                 |   10 +-
 gfx/layers/d3d11/TextureD3D11.cpp                    |   34 +-
 gfx/layers/d3d11/TextureD3D11.h                      |    2 +-
 gfx/layers/d3d9/CanvasLayerD3D9.cpp                  |    4 +-
 gfx/layers/d3d9/CompositorD3D9.cpp                   |   10 +-
 gfx/layers/d3d9/TextureD3D9.cpp                      |   62 +-
 gfx/layers/d3d9/TextureD3D9.h                        |    4 +-
 gfx/layers/d3d9/ThebesLayerD3D9.cpp                  |    2 +-
 gfx/layers/ipc/SharedPlanarYCbCrImage.cpp            |    4 +-
 gfx/layers/opengl/CompositingRenderTargetOGL.h       |    2 +-
 gfx/layers/opengl/CompositorOGL.cpp                  |    6 +-
 gfx/layers/opengl/CompositorOGL.h                    |    2 +-
 gfx/layers/opengl/GrallocTextureClient.cpp           |   14 +-
 gfx/layers/opengl/GrallocTextureHost.cpp             |   20 +-
 gfx/layers/opengl/MacIOSurfaceTextureHostOGL.cpp     |    4 +-
 gfx/layers/opengl/OGLShaderProgram.h                 |   20 +-
 gfx/layers/opengl/TextureHostOGL.cpp                 |   38 +-
 gfx/layers/opengl/TextureHostOGL.h                   |   16 +-
 gfx/src/FilterSupport.cpp                            |   62 +-
 gfx/src/nsRect.cpp                                   |    5 +-
 gfx/tests/gtest/TestTextures.cpp                     |    6 +-
 gfx/tests/gtest/gfxFontSelectionTests.h              |    4 +-
 gfx/tests/gtest/gfxTextRunPerfTest.cpp               |    2 +-
 gfx/tests/gtest/gfxWordCacheTest.cpp                 |    2 +-
 gfx/thebes/gfx2DGlue.h                               |  170 ++++----
 gfx/thebes/gfxAndroidPlatform.cpp                    |    4 +-
 gfx/thebes/gfxBlur.cpp                               |    4 +-
 gfx/thebes/gfxContext.cpp                            |   44 +-
 gfx/thebes/gfxContext.h                              |    6 +-
 gfx/thebes/gfxDWriteFonts.cpp                        |    4 +-
 gfx/thebes/gfxFT2FontBase.cpp                        |    8 +-
 gfx/thebes/gfxFT2Fonts.cpp                           |    4 +-
 gfx/thebes/gfxFont.cpp                               |   18 +-
 gfx/thebes/gfxGradientCache.cpp                      |    4 +-
 gfx/thebes/gfxMacFont.cpp                            |    2 +-
 gfx/thebes/gfxPangoFonts.cpp                         |    8 +-
 gfx/thebes/gfxPattern.cpp                            |    6 +-
 gfx/thebes/gfxPlatform.cpp                           |   90 ++--
 gfx/thebes/gfxPlatform.h                             |   26 +-
 gfx/thebes/gfxPlatformGtk.cpp                        |   12 +-
 gfx/thebes/gfxPlatformGtk.h                          |    4 +-
 gfx/thebes/gfxPlatformMac.cpp                        |   20 +-
 gfx/thebes/gfxQuartzNativeDrawing.cpp                |    4 +-
 gfx/thebes/gfxWindowsNativeDrawing.cpp               |    2 +-
 gfx/thebes/gfxWindowsPlatform.cpp                    |   26 +-
 gfx/thebes/gfxXlibNativeRenderer.cpp                 |    4 +-
 gfx/ycbcr/YCbCrUtils.cpp                             |   14 +-
 image/src/ClippedImage.cpp                           |    4 +-
 image/src/OrientedImage.cpp                          |    4 +-
 image/src/VectorImage.cpp                            |    2 +-
 image/src/imgTools.cpp                               |    2 +-
 ipc/glue/IPCMessageUtils.h                           |   38 +
 layout/base/FrameLayerBuilder.cpp                    |    2 +-
 layout/base/nsCSSRendering.cpp                       |   36 +-
 layout/base/nsCSSRendering.h                         |    4 +-
 layout/base/nsCSSRenderingBorders.cpp                |  247 ++++++------
 layout/base/nsCSSRenderingBorders.h                  |    8 +-
 layout/base/nsLayoutUtils.cpp                        |   14 +-
 layout/generic/StickyScrollContainer.cpp             |    8 +-
 layout/generic/nsBlockFrame.cpp                      |    2 +-
 layout/generic/nsCanvasFrame.cpp                     |    2 +-
 layout/generic/nsColumnSetFrame.cpp                  |    2 +-
 layout/generic/nsFlexContainerFrame.cpp              |   26 +-
 layout/generic/nsFrame.cpp                           |   16 +-
 layout/generic/nsImageFrame.cpp                      |    4 +-
 layout/generic/nsInlineFrame.cpp                     |    8 +-
 layout/generic/nsSplittableFrame.cpp                 |    8 +-
 layout/style/nsCSSParser.cpp                         |   32 +-
 layout/style/nsCSSProps.cpp                          |    6 +-
 layout/style/nsCSSValue.cpp                          |    6 +-
 layout/style/nsComputedDOMStyle.cpp                  |    8 +-
 layout/style/nsRuleNode.cpp                          |   28 +-
 layout/style/nsStyleAnimation.cpp                    |    8 +-
 layout/style/nsStyleConsts.h                         |   19 +-
 layout/style/nsStyleCoord.cpp                        |    4 +-
 layout/style/nsStyleCoord.h                          |    8 +-
 layout/style/nsStyleStruct.cpp                       |   16 +-
 layout/style/nsStyleStruct.h                         |   42 +-
 layout/svg/nsSVGFilterInstance.cpp                   |    6 +-
 layout/tables/celldata.h                             |    4 +-
 layout/tables/nsCellMap.cpp                          |    8 +-
 layout/tables/nsCellMap.h                            |    2 +-
 layout/tables/nsTableCellFrame.cpp                   |    4 +-
 layout/tables/nsTableColFrame.cpp                    |    2 +-
 layout/tables/nsTableColFrame.h                      |    2 +-
 layout/tables/nsTableColGroupFrame.cpp               |    6 +-
 layout/tables/nsTableColGroupFrame.h                 |    2 +-
 layout/tables/nsTableFrame.cpp                       |   42 +-
 layout/tables/nsTableRowFrame.cpp                    |    8 +-
 layout/tables/nsTableRowFrame.h                      |    2 +-
 layout/tables/nsTableRowGroupFrame.cpp               |    6 +-
 layout/tables/nsTableRowGroupFrame.h                 |    2 +-
 layout/xul/nsMenuPopupFrame.cpp                      |    2 +-
 layout/xul/nsMenuPopupFrame.h                        |    2 +-
 layout/xul/nsStackLayout.cpp                         |    8 +-
 mfbt/TypedEnum.h                                     |    2 +
 widget/android/AndroidBridge.cpp                     |    2 +-
 widget/cocoa/nsChildView.mm                          |   16 +-
 widget/cocoa/nsNativeThemeCocoa.mm                   |    4 +-
 widget/gtk/nsWindow.cpp                              |   10 +-
 widget/windows/nsWindowGfx.cpp                       |    2 +-
 208 files changed, 1919 insertions(+), 1799 deletions(-)
(In reply to Benoit Jacob [:bjacob] from comment #13)
> Created attachment 8358156 [details] [diff] [review]
> Make the remaining enums typed (sorry, unique patch with both manual and
> automatic changes)

Some comments to help reviewing this big patch.

Notice how this sets the underlying type of all the enums in gfx/2d/Types.h to be int8_t. This is because I noticed that some of these enums were used in bit-fields, implying that sizeof() was a concern. By making these enums be backed by int8_t's (a feature which we gain from C++11 typed enums!) we gain a different way of controlling their sizeof(), which should make these bitfields unneeded.
Comment on attachment 8358157 [details] [diff] [review]
My first layout bug fix!

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

Nice catch

::: layout/generic/nsBlockFrame.cpp
@@ +966,5 @@
>    if (aReflowState.AvailableHeight() != NS_UNCONSTRAINEDSIZE &&
>        aReflowState.ComputedHeight() != NS_AUTOHEIGHT &&
>        ShouldApplyOverflowClipping(this, aReflowState.mStyleDisplay)) {
>      nsMargin heightExtras = aReflowState.ComputedPhysicalBorderPadding();
> +    if (GetSkipSides() & (1 << int(NS_SIDE_TOP))) {

Lose the explicit int() cast
Attachment #8358157 - Flags: review?(roc) → review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #17)
> Lose the explicit int() cast

Hmm, I see that with typed enums the cast is required. That's nasty :-(.

For a very long time enums were the only portable way to declare integer constants in class scope. Now we're going to convert them to typed enums and require casts all over the place. That's very unfortunate.

Can we just not make NS_SIDE_* be a typed enum?
Attachment #8358134 - Flags: review?(bas) → review+
Comment on attachment 8358156 [details] [diff] [review]
Make the remaining enums typed (sorry, unique patch with both manual and automatic changes)

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

I'm not going to review every single file. I'm going to assume you checked this compiles :).
Attachment #8358156 - Flags: review?(bas) → review+
Attachment #8358154 - Flags: feedback?(bas) → feedback+
Attachment #8358135 - Flags: feedback?(bas) → feedback+
Attachment #8358141 - Flags: review?(bas) → review+
Attachment #8358152 - Flags: feedback?(bas) → feedback+
Attachment #8358150 - Flags: review?(bas) → review+
Attachment #8358149 - Flags: review?(bas) → review+
Attachment #8358147 - Flags: feedback?(bas) → feedback+
Attachment #8358144 - Flags: review?(bas) → review+
Attachment #8358142 - Flags: feedback?(bas) → feedback+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #17)
> > Lose the explicit int() cast
> 
> Hmm, I see that with typed enums the cast is required. That's nasty :-(.
> 
> For a very long time enums were the only portable way to declare integer
> constants in class scope. Now we're going to convert them to typed enums and
> require casts all over the place. That's very unfortunate.
> 
> Can we just not make NS_SIDE_* be a typed enum?

OK. I reverted all the mozilla::css::Side / NS_SIDE_* changes for now.

https://tbpl.mozilla.org/?tree=Try&rev=092706257805
Comment on attachment 8358137 [details] [diff] [review]
Add a variant of MOZ_ENUM_CLASS_ENUM_TYPE that works on template parameters

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

::: mfbt/TypedEnum.h
@@ +250,5 @@
>  
>    /*
>     * MOZ_ENUM_CLASS_ENUM_TYPE allows using enum classes
>     * as template parameter types. For that, we need integer types.
>     * In the present case, the integer type is the Enum nested type.

This should have a comment like what's in attachment 8358138 [details] [diff] [review] to explain the need for MOZ_TEMPLATE_ENUM_CLASS_ENUM_TYPE:

"""
If the enum class's definition depends on a template parameter, the special macro MOZ_TEMPLATE_ENUM_CLASS_ENUM_TYPE must be used:

  // MOZ_ENUM_CLASS_ENUM_TYPE is a value expression in compilers not supporting
  // C++11 enum classes.  The typename this macro adds makes it be interpreted
  // as a type.
  template<typename T, MOZ_TEMPLATE_ENUM_CLASS_ENUM_TYPE(T) Value>
  struct S {};

  MOZ_BEGIN_ENUM_CLASS(E, int16_t)
    V = 0
  MOZ_END_ENUM_CLASS(E)

  S<E, 15> s;
"""
Attachment #8358137 - Flags: review?(jwalden+bmo) → review+
Attachment #8358138 - Flags: review?(bent.mozilla) → review+
Btw here was my last try push (build only):
https://tbpl.mozilla.org/?tree=Try&rev=8d8561717c0a

And a slightly older one with tests:
https://tbpl.mozilla.org/?tree=Try&rev=092706257805
Comment on attachment 8358138 [details] [diff] [review]
Add a variant of EnumSerializer that works on MFBT typed enums

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

bent suggested over IRC that this might be duplicating too much code.  I'm not sure this matters too much in practice, but would it be worthwhile to separate out something like:

namespace detail {
  template<typename T, typename IntegerType, IntegerType LowBound, IntegerType HighBound>
  class IntegerEnumSerializer<...> {
  };
}

template<typename E, ...>
class EnumSerializer : public IntegerEnumSerializer<E, E, ...>
{
};

template<typename E, ...>
class TypedEnumSerializer {
  typedef template<E, MOZ_TEMPLATE_ENUM_CLASS_ENUM_TYPE(E), ...> IntegerSerializer;

  // Read/Write just forward to IntegerSerializer::{Read,Write}
};

?  Does that look plausible?
This plausible, but note that TypedEnumSerializer should be only temporary: once we can drop support for GCC < 4.6 and thus assume support for C++11 typed enums, we will be able to drop it, as on C++11 typed enums, MOZ_BEGIN_ENUM_CLASS-generated enums do not require any special treatment i.e. MOZ_TEMPLATE_ENUM_CLASS_ENUM_TYPE(E) can be replaced by just E.  (Of course, once we can assume C++11 typed enums, we'll also be able to drop MOZ_BEGIN_ENUM_CLASS altogether and switch to using c++11 typed enums directly).
...and so, my point was, since TypedEnumSerializer is only temporary, maybe it is best to avoid making EnumSerializer's implementation more complicated and abstract because of TypedEnumSerializer.
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.