WIP -- separate out Parts from SkPaint

include/core/SkPaintParts.h

Index: include/core/SkPaintParts.h
diff --git a/include/core/SkPaintParts.h b/include/core/SkPaintParts.h
new file mode 100644
index 0000000000000000000000000000000000000000..1f06ad48fb250e6c628771d90e38fd7ab5ff5ef8
--- /dev/null
+++ b/include/core/SkPaintParts.h
@@ -0,0 +1,201 @@
+/*
+ * Copyright 2010 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#ifndef SkPaintParts_DEFINED
+#define SkPaintParts_DEFINED
+
+#include "SkColor.h"
+
+#ifdef SK_BUILD_FOR_ANDROID
+#include "SkPaintOptionsAndroid.h"
+#endif
+
+class SkAnnotation;
+class SkAutoGlyphCache;
+class SkColorFilter;
+class SkDescriptor;
+struct SkDeviceProperties;
+class SkDrawLooper;
+class SkReadBuffer;
+class SkWriteBuffer;
+struct SkGlyph;
+struct SkRect;
+class SkGlyphCache;
+class SkImageFilter;
+class SkMaskFilter;
+class SkPath;
+class SkPathEffect;
+struct SkPoint;
+class SkRasterizer;
+class SkShader;
+class SkTypeface;
+class SkXfermode;
+
+class SkPaintParts {

[scroggo, 2014/04/15 17:21:07]

Comments would be nice. If I understand correctly, a goal of this class is to
hold on to these parts without reffing and unreffing them. Further, a client
could use an SkPaintParts instead of an SkPaint, to get all of the fields. (Are
there other reasons for this class?)

[tomhudson, 2014/04/16 11:41:31]

...and if this is somehow avoiding touching the refcount, it isn't obvious how.

[reed1, 2014/04/18 14:04:31]

Absolutely this will need to be documented. (but maybe not until it is more
fleshed out).

The end-goal (in my mind) is to make SkPaint purely an optional helper library,
and move it (logically) out of core.

+public:
+    SkPaintParts() {
+        sk_bzero(this, sizeof(*this));

[mtklein, 2014/04/15 14:32:44]

Tsk tsk.  We know this bzero/memcpy stuff is bad news now...

[reed1, 2014/04/18 14:04:31]

Tsk tsk... premature optimization until the fields of this class settle down :)

+    }
+
+    SkPaintParts(const SkPaintParts& other) {

[mtklein, 2014/04/15 14:32:44]

Maybe we should just write these two as
  // Intentionally using the default copy constructor and operator=.
  // SkPaintParts(const SkPaintParts&) = default;
  // SkPaintParts& operator=(const SkPaintParts&) = default;
?  Those defaults are actually what we'd want here.

[reed1, 2014/04/18 14:04:31]

Agreed, will remove these.

+        memcpy(this, &other, sizeof(other));
+    }
+
+    SkPaintParts& operator=(const SkPaintParts& other) {
+        memcpy(this, &other, sizeof(other));
+        return *this;
+    }
+
+    bool operator==(const SkPaintParts& other) const;
+    bool operator!=(const SkPaintParts& other) const {
+        return !(*this == other);
+    }
+
+    /** Specifies the bit values that are stored in the paint's flags.
+     */
+    enum Flags {

[scroggo, 2014/04/15 17:21:07]

I'm pretty opposed to having all these enums duplicated here. It seems like it
would be very easy to get the two classes out of sync - especially for Flags; I
expect the others won't change very often, but I don't see the advantage to the
duplication.

+        kAntiAlias_Flag       = 0x01,   //!< mask to enable antialiasing

[mtklein, 2014/04/15 14:32:44]

Can you note if 0x02 available for future use or somehow verboten?

[scroggo, 2014/04/17 15:27:54]

0x02 used to be kFilterBitmap_Flag, and Android treats 0x02 as their analogous
FILTER_BITMAP_FLAG (in Paint.java). That said, we already have to special case
that (at least, with recent revs to Skia - you can see this by the CTS failures
in Android's master-skia branch), so if we put a new flag there, Android will
just need to continue special casing it.

[reed1, 2014/04/18 14:04:31]

Will do that also in a diff CL for the current SkPaint.

+        kDither_Flag          = 0x04,   //!< mask to enable dithering
+        kUnderlineText_Flag   = 0x08,   //!< mask to enable underline text
+        kStrikeThruText_Flag  = 0x10,   //!< mask to enable strike-thru text
+        kFakeBoldText_Flag    = 0x20,   //!< mask to enable fake-bold text
+        kLinearText_Flag      = 0x40,   //!< mask to enable linear-text
+        kSubpixelText_Flag    = 0x80,   //!< mask to enable subpixel text positioning
+        kDevKernText_Flag     = 0x100,  //!< mask to enable device kerning text
+        kLCDRenderText_Flag   = 0x200,  //!< mask to enable subpixel glyph renderering
+        kEmbeddedBitmapText_Flag = 0x400, //!< mask to enable embedded bitmap strikes
+        kAutoHinting_Flag     = 0x800,  //!< mask to force Freetype's autohinter
+        kVerticalText_Flag    = 0x1000,
+        kGenA8FromLCD_Flag    = 0x2000, // hack for GDI -- do not use if you can help it
+        kDistanceFieldTextTEMP_Flag = 0x4000, //!< TEMPORARY mask to enable distance fields

[mtklein, 2014/04/15 14:32:44]

Can't hurt to align everyone to the widest =?

+        // currently overrides LCD and subpixel rendering
+        // when adding extra flags, note that the fFlags member is specified
+        // with a bit-width and you'll have to expand it.
+        
+        kAllFlags = 0xFFFF
+    };
+    
+    uint32_t getFlags() const { return fFlags; }
+    void setFlags(uint32_t flags);
+
+    enum Hinting {
+        kNo_Hinting            = 0,
+        kSlight_Hinting        = 1,
+        kNormal_Hinting        = 2,     //!< this is the default

[mtklein, 2014/04/15 14:32:44]

Doesn't bzero make this comment wrong?

If the idea is to make SkPaint enforce all the semantics and have this just
store the data dumbly, could be the default constructor (that is, leave
everything uninitialized) is what we want.

+        kFull_Hinting          = 3
+    };
+    
+    Hinting getHinting() const {
+        return static_cast<Hinting>(fHinting);

[mtklein, 2014/04/15 14:32:44]

Might take the opportunity to make some things consistent in the file:
  - Hinting is explicitly enumerated, the rest of the enums aren't
  - Hinting is static_cast<Hinting>, the rest are (Class)

[scroggo, 2014/04/15 17:21:07]

I vote for C++ style casts.

[reed1, 2014/04/18 14:04:31]

Will do (before committing)

+    }
+    void setHinting(Hinting hinting) { fHinting = hinting; }
+
+    enum Style {
+        kFill_Style,            //!< fill the geometry
+        kStroke_Style,          //!< stroke the geometry
+        kStrokeAndFill_Style,   //!< fill and stroke the geometry
+    };
+    
+    Style getStyle() const { return (Style)fStyle; }
+    void setStyle(Style style) { fStyle = style; }
+
+    SkColor getColor() const { return fColor; }
+    void setColor(SkColor color) { fColor = color; }
+
+    enum TextEncoding {
+        kUTF8_TextEncoding,     //!< the text parameters are UTF8
+        kUTF16_TextEncoding,    //!< the text parameters are UTF16
+        kUTF32_TextEncoding,    //!< the text parameters are UTF32
+        kGlyphID_TextEncoding   //!< the text parameters are glyph indices
+    };
+    
+    TextEncoding getTextEncoding() const { return (TextEncoding)fTextEncoding; }
+    void setTextEncoding(TextEncoding encoding);
+
+    enum Align {
+        kLeft_Align,
+        kCenter_Align,
+        kRight_Align,
+    };
+    
+    Align getTextAlign() const { return (Align)fTextAlign; }
+    void setTextAlign(Align align) { fTextAlign = align; }
+
+    SkScalar getTextSize() const { return fTextSize; }
+    void setTextSize(SkScalar size) { fTextSize = size; }
+
+    SkScalar getTextScaleX() const { return fTextScaleX; }
+    void setTextScaleX(SkScalar scale) { fTextScaleX = scale; }
+    
+    SkScalar getTextSkewX() const { return fTextSkewX; }
+    void setTextSkewX(SkScalar skew) { fTextSkewX = skew; }
+   
+    SkScalar getStrokeWidth() const { return fWidth; }
+    void setStrokeWidth(SkScalar width) { fWidth = width; }
+    
+    SkScalar getStrokeMiter() const { return fMiterLimit; }
+    void setStrokeMiter(SkScalar miter) { fMiterLimit = miter; }
+
+    enum Cap {
+        kButt_Cap,      //!< begin/end contours with no extension
+        kRound_Cap,     //!< begin/end contours with a semi-circle extension
+        kSquare_Cap,    //!< begin/end contours with a half square extension
+        
+        kDefault_Cap = kButt_Cap
+    };
+
+    Cap getStrokeCap() const { return (Cap)fCapType; }
+    void setStrokeCap(Cap cap) { fCapType = cap; }
+
+    enum Join {
+        kMiter_Join,    //!< connect path segments with a sharp join
+        kRound_Join,    //!< connect path segments with a round join
+        kBevel_Join,    //!< connect path segments with a flat bevel join
+        
+        kDefault_Join = kMiter_Join
+    };
+
+    Join getStrokeJoin() const { return (Join)fJoinType; }
+    void setStrokeJoin(Join join) { fJoinType = join; }
+
+    SkTypeface*     fTypeface;

[mtklein, 2014/04/15 14:32:44]

Shall we go whole-hog here and make everything just a public field (and tag the
thing as a struct)?  You ought to be able to write the fields as their proper
enum types too:

struct SkPaintParts {
  SkTypeface* fTypeface;
  ...
  SkAnnotation* fAnnotation;

  SkScalar fTextSize;
  ...
  SkScalar fMiterLimit;

  union {
    struct {
        uint32_t fFlags     : 16;
        Align    fTextAlign : 2;
        Cap      fCapType   : 2;
        ...
        // unsigned fFreeBits : 4;
    };
    uint32_t fAllBitfields;
  };
};

bool operator==(const SkPaintParts&, const SkPaintParts&);
bool operator!=(const SkPaintParts&, const SkPaintParts&) {
  return !(a==b);
}

[reed1, 2014/04/18 14:04:31]

This is meant to be our new PUBLIC api. History has amply shown us that we
evolve how/where we store values in Paint. It is convenient perhaps to just
expose everything, but makes future refactoring much harder.

+    SkPathEffect*   fPathEffect;
+    SkShader*       fShader;
+    SkXfermode*     fXfermode;
+    SkMaskFilter*   fMaskFilter;
+    SkColorFilter*  fColorFilter;
+    SkRasterizer*   fRasterizer;
+    SkDrawLooper*   fLooper;
+    SkImageFilter*  fImageFilter;
+    SkAnnotation*   fAnnotation;
+    
+private:
+    friend class SkPaint;
+
+    SkScalar        fTextSize;
+    SkScalar        fTextScaleX;
+    SkScalar        fTextSkewX;
+    SkColor         fColor;
+    SkScalar        fWidth;
+    SkScalar        fMiterLimit;
+    union {
+        struct {
+            // all of these bitfields should add up to 32
+            unsigned        fFlags : 16;
+            unsigned        fTextAlign : 2;
+            unsigned        fCapType : 2;
+            unsigned        fJoinType : 2;
+            unsigned        fStyle : 2;
+            unsigned        fTextEncoding : 2;  // 3 values
+            unsigned        fHinting : 2;
+            //unsigned      fFreeBits : 4;
+        };
+        uint32_t fBitfields;
+    };
+};
+
+#endif