Implement embedded SVG glyphs in OpenType fonts

RESOLVED FIXED in mozilla18

Status

()

--
enhancement
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: eflores, Assigned: eflores)

Tracking

(Depends on: 2 bugs, Blocks: 2 bugs, {dev-doc-needed})

unspecified
mozilla18
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(25 attachments, 113 obsolete attachments)

3.14 KB, patch
roc
: review+
Details | Diff | Splinter Review
329.31 KB, patch
eflores
: review+
Details | Diff | Splinter Review
3.00 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
9.12 KB, patch
eflores
: review+
Details | Diff | Splinter Review
121.28 KB, patch
roc
: review+
Details | Diff | Splinter Review
112.34 KB, patch
roc
: review+
Details | Diff | Splinter Review
118.77 KB, patch
eflores
: review+
Details | Diff | Splinter Review
5.73 KB, patch
eflores
: review+
Details | Diff | Splinter Review
41.92 KB, patch
eflores
: review+
Details | Diff | Splinter Review
7.32 KB, patch
eflores
: review+
Details | Diff | Splinter Review
14.48 KB, patch
eflores
: review+
Details | Diff | Splinter Review
47.71 KB, patch
eflores
: review+
Details | Diff | Splinter Review
1.85 KB, patch
eflores
: review+
Details | Diff | Splinter Review
17.58 KB, patch
eflores
: review+
Details | Diff | Splinter Review
856 bytes, patch
eflores
: review+
Details | Diff | Splinter Review
772 bytes, patch
eflores
: review+
Details | Diff | Splinter Review
10.25 KB, patch
eflores
: review+
Details | Diff | Splinter Review
17.95 KB, patch
eflores
: review+
Details | Diff | Splinter Review
16.94 KB, patch
eflores
: review+
Details | Diff | Splinter Review
19.23 KB, patch
eflores
: review+
Details | Diff | Splinter Review
13.90 KB, patch
eflores
: review+
Details | Diff | Splinter Review
18.61 KB, patch
eflores
: review+
Details | Diff | Splinter Review
2.39 KB, patch
eflores
: review+
Details | Diff | Splinter Review
14.71 KB, patch
eflores
: review+
Details | Diff | Splinter Review
2.11 KB, patch
roc
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
*Very* basic implementation. Examples to come.
Attachment #589713 - Flags: review?(roc)
This is where it starts to look quite cool.
Attachment #589716 - Flags: review?(roc)
NOTE: There is still a horrific memory leak somewhere I need to track down
Gah. Stupid machines. *Actually* attached this time.
Attachment #589722 - Attachment is obsolete: true
Attachment #589722 - Flags: review?(roc)
Posted patch Reftest for Patch 1 (obsolete) — Splinter Review
Super basic test
Attachment #589734 - Flags: review?(roc)
The OTS changes should be broken out into their own patch and be reviewed by Jonathan Kew. They look OK to me but will probably need license boilerplate in the files, and maybe other changes to make them acceptable upstream.
The nsDocument.cpp change can go into its own patch, with a suitable commit message and in-code comment describing why we're doing this.
Nice progress, Edwin!

FWIW I think we should use something other than id="<glyphid>" as the means to identify the glyphs in the SVG document.  For one, a plain number in there is invalid (as IDs must be XML Names), and two I think it's probably best to leave semantics out of user controlled attributes like id="" (just like class="").  Sairus Patel on the SVG-in-OpenType mailing list (http://www.w3.org/community/svgopentype/, http://lists.w3.org/Archives/Public/public-svgopentype/) preferred id="<glyphid>" but I'd rather come up with a new attribute like glyphid="".
Are you concerned about space taken up in the font with lots of 'glyphid="..."' strings?  (Can tables be compressed?  If not, it might be worth allowing the table to be gzipped at some point in the future.)  I don't mind whether we require the use of <glyph> around the glyph data or whether we allow any element to act as a glyph.  The space taken up by

  <glyph n="100">...</glyph>

versus

  <g glyphid="100">...</g>

is nearly the same.  n="" just seems not descriptive enough to me, especially if allowed on any element.  So I would still prefer <g glyphid="100">.
<g glyphid="..."> is fine I guess.
Comment on attachment 589713 [details] [diff] [review]
Patch 1 rev 1 - Basic OpenType SVG functionality

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

Probably in automation.py we should set the pref for feature to be true so the tests will run...

::: gfx/thebes/Makefile.in
@@ +90,5 @@
> +                  -I$(topsrcdir)/layout/generic \
> +                  -I$(topsrcdir)/layout/svg/base/src \
> +                  -I$(topsrcdir)/content/xml/document/src \
> +                  -I$(topsrcdir)/layout/xul/base/src \
> +                  $(NULL)

Eek, this is ugly!

I don't have any better ideas right now though :-(.

@@ +216,5 @@
>  	gfxUnicodeProperties.cpp \
>  	gfxScriptItemizer.cpp \
>  	gfxHarfBuzzShaper.cpp \
>  	gfxSharedImageSurface.cpp \
> +    gfxSVGGlyphsWrapper.cpp \

tab, not spaces

::: gfx/thebes/gfxFont.cpp
@@ +1424,5 @@
> +    // Multiply the stroke matrix by the context matrix since cairo multiplies
> +    // the pattern matrix by the inverse of the context matrix whenever the
> +    // pattern is set
> +    gfxMatrix strokeMatrix;
> +    gfxMatrix strokeMatrixInv;

Inv sounds like inverse, which this isn't. I'd probably just not use a temporary variable here.

@@ -1503,5 @@
>                                    glyphs.Flush(cr, strokePattern, aDrawMode, isRTL);
>                                } while (--strikeCount > 0);
>                            }
>                        }
> -                      x += direction*advance;

Moving this code means you're not advancing x in the "if (glyphData->IsMissing())" case anymore. That seems like a bug.

@@ +1760,5 @@
> +bool
> +gfxFont::RenderSVGGlyph(gfxContext *aContext, gfxPoint aPoint, DrawMode aDrawMode,
> +                        PRUint32 aGlyphId)
> +{
> +    enum { SVG_UNITS_PER_EM = 1000 };

static const float is probably better. Could be either inside or outside this function.

@@ +1765,5 @@
> +
> +    const float devUnitsPerSVGUnit = GetStyle()->size / (float)SVG_UNITS_PER_EM;
> +    gfxContextMatrixAutoSaveRestore matrixRestore(aContext);
> +
> +    aContext->Translate(gfxPoint(aPoint.x, aPoint.y - GetMetrics().maxAscent));

Using maxAscent here is probably not right.

I think y=0 in the SVG coordinate space should be the baseline. Then you just need to translate by aPoint.y here (aPoint.y is the baseline).

::: gfx/thebes/gfxFont.h
@@ +286,5 @@
> +    bool TryGetSVGData();
> +    bool HasSVGGlyph(PRUint32 id) {
> +        if (!mSVGInitialized) {
> +            TryGetSVGData();
> +        }

Can we just assert mSVGInitialized? We should simply not call this unless TryGetSVGData has already returned true.

@@ +293,5 @@
> +
> +    bool RenderSVGGlyph(gfxContext* context, PRUint32 id, int drawMode) {
> +        if (!mSVGInitialized) {
> +            TryGetSVGData();
> +        }

Can we just assert mSVGInitialized? We should simply not call this unless TryGetSVGData has already returned true.

@@ +364,5 @@
>      gfxUserFontData* mUserFontData;
>  
> +    bool             mSVGInitialized;
> +    bool             mHasSVG;
> +    nsRefPtr<gfxSVGGlyphsWrapper>  mSVGGlyphs;

Can we remove mHasSVG and just check mSVGGlyphs != null?

Only need one space before mSVGGlyphs

::: gfx/thebes/gfxSVGGlyphsWrapper.cpp
@@ +1,1 @@
> +#include "gfxSVGGlyphsWrapper.h"

Need license boilterplate

@@ +34,5 @@
> +
> +typedef mozilla::dom::Element Element;
> +
> +
> +/* static */ nsresult

1-line spacing not 2

@@ +36,5 @@
> +
> +
> +/* static */ nsresult
> +gfxSVGGlyphsWrapper::ParseFromBuffer(PRUint8* aBuffer, PRUint32 aBufLen,
> +                                     nsRefPtr<gfxSVGGlyphsWrapper>& aResult)

This should just return already_AddRefed<gfxSVGGlyphsWrapper> directly, returning null for failure.

@@ +51,5 @@
> +    nsXPIDLCString contractId;
> +    rv = catMan->GetCategoryEntry("Gecko-Content-Viewers", "image/svg+xml", getter_Copies(contractId));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    nsCOMPtr<nsIDocumentLoaderFactory> docLoaderFactory = do_GetService(contractId);

probaly should have a null check after this

@@ +73,5 @@
> +        rv = presShell->InitialReflow(rect.width, rect.height);
> +        NS_ENSURE_SUCCESS(rv, rv);
> +    }
> +
> +    NS_ASSERTION(presShell, "GetPresShell succeeded but returned nothing.");

we'd have already crashed if this assertion failed. Better just remove it :-)

@@ +98,5 @@
> +    }
> +
> +    gfxContextAutoSaveRestore contextRestorer(context);
> +
> +    mDocument->FlushPendingNotifications(Flush_Layout);

We don't need the mDocument flush, just take it out. The document is already completely parsed.

@@ +109,5 @@
> +
> +    nsIFrame* frame = glyphElem->GetPrimaryFrame();
> +
> +    if (!frame) {
> +        NS_WARNING("Couldn't get frame");

"No frame for SVG glyph"

::: gfx/thebes/gfxSVGGlyphsWrapper.h
@@ +1,2 @@
> +#ifndef GFX_SVG_GLYPHS_WRAPPER_H
> +#define GFX_SVG_GLYPHS_WRAPPER_H

Need boilerplate

@@ +30,5 @@
> +
> +protected:
> +
> +
> +private:

Useless 'protected'.

Also, too many blank lines

@@ +48,5 @@
> +    nsCOMPtr<nsIContentViewer>  mViewer;
> +    nsCOMPtr<nsIPresShell>      mPresShell;
> +
> +    gfxSparseBitSet         mSearchedMap;
> +    gfxSparseBitSet         mHasGlyph;

Unnecessary spaces before m...

I think here we should have an nsTHashtable of nsISupportsHashKey mapping to the nsIContent* for the glyph. You'd build this by walking the DOM after you've parsed it. We shouldn't need mSearchedMap at all if we do it that way.
Comment on attachment 589727 [details] [diff] [review]
Patch 2 rev 1 - Allow SVG glyphs to inherit fill/stroke pattern from object

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

I think we should  split up this patch into at least "add objectFill and objectStroke support" and "set up objectFill and objectStroke values for SVG fonts".

::: layout/style/nsStyleStruct.h
@@ +2117,5 @@
>  {
>    union {
>      nscolor mColor;
>      nsIURI *mPaintServer;
> +    bool mFill;

What does this mean? I think it might make more sense to add eStyleSVGPaintType_ObjectFill and eStyleSVGPaintType_ObjectStroke to nsStyleSVGPaintType. What do you think?

::: layout/svg/base/src/svg.css
@@ +52,5 @@
>  
> +svg {
> + fill: objectfill;
> + stroke: objectstroke;
> +}

This should only apply to glyphs!

Maybe use selector *[glyphid]? Although that might be slow.

Faster-matching UA style sheet selector argues for using a specific SVG <glyph> element instead of using glyphid on any element.
Comment on attachment 589713 [details] [diff] [review]
Patch 1 rev 1 - Basic OpenType SVG functionality

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

::: gfx/ots/src/svg.cc
@@ +8,5 @@
> +bool ots_svg_parse(OpenTypeFile *file, const uint8_t *data, size_t length) {
> +  Buffer table(data, length);
> +
> +  OpenTypeSVG *svg = new OpenTypeSVG;
> +  svg->svg = std::string((const char*)table.buffer());

Is the 'SVG ' table necessarily going to be NUL-terminated in the sfnt package? (That would seem counter-intuitive, as sfnt tables have an explicit length recorded, so no terminator is needed.) If not, you'd need to pass the length to the std::string constructor rather than relying on the table being a NUL-terminated C string.

However, in any case you shouldn't use std::string here; you don't need to copy the data, just remember the pointer and length. See the handling of the Graphite tables in ots/src/graphite.[h,cc] for how you can do this.

::: gfx/ots/src/svg.h
@@ +7,5 @@
> +
> +namespace ots {
> +
> +struct OpenTypeSVG {
> +  std::string svg;

This should just record a pointer and a length, not make a copy of the entire table as a string.

::: gfx/thebes/Makefile.in
@@ +90,5 @@
> +                  -I$(topsrcdir)/layout/generic \
> +                  -I$(topsrcdir)/layout/svg/base/src \
> +                  -I$(topsrcdir)/content/xml/document/src \
> +                  -I$(topsrcdir)/layout/xul/base/src \
> +                  $(NULL)

At least sort the paths so that all the content/ ones are together, then all the layout/ ones.

::: gfx/thebes/gfxFont.cpp
@@ +228,5 @@
> +
> +        bool svgEnabled;
> +        nsresult rv =
> +            Preferences::GetBool("gfx.font_rendering.opentype_svg.enabled",
> +                                 &svgEnabled);

It'd be nice to support toggling this pref on-the-fly, particularly during testing, but also as a convenience for font developers and page authors who want to check how a page looks both with and without the feature.

To do this, I think you want to follow the pattern used for other gfx.font_rendering.* options, using gfxPlatform::FontsPrefsChanged to notice when it changes; at that point, you can flush the font cache so that fresh instances will be created and see the new setting.

With the preference "live", you can also use the pref(...) option in a reftest manifest to do reftesting both with and without the feature enabled. E.g. with two versions of a test font, where only one has SVG glyphs present, you can confirm that they render identically with OT-SVG disabled, but differently when it's enabled.

::: gfx/thebes/gfxSVGGlyphsWrapper.cpp
@@ +18,5 @@
> +#include "nsIFrame.h"
> +#include "nsQueryFrame.h"
> +#include "nsSVGContainerFrame.h"
> +#include "nsIContentSink.h"
> +#include "nsIContentSink.h"

I don't think we really need this twice. :)

In fact, given that you're about to include nsXMLContentSink.h, I'd guess you don't need to explicitly mention this at all, as it'll get pulled in below anyway.

@@ +90,5 @@
> + * @param id The glyph id
> + * @return true iff rendering succeeded
> + */
> +bool
> +gfxSVGGlyphsWrapper::RenderGlyph(gfxContext* context, PRUint32 id, int drawMode)

Our convention is for arguments to have an 'a' prefix, so gfxContext* aContext, etc. (Here and in following functions.)

@@ +217,5 @@
> +
> +    channel->SetOwner(principal);
> +
> +    nsCOMPtr<nsIDocument> document(do_QueryInterface(domDoc));
> +    if (!document) return NS_ERROR_FAILURE;

Braces and newline, to conform to standard style guidelines.

::: gfx/thebes/gfxSVGGlyphsWrapper.h
@@ +18,5 @@
> +
> +class gfxSVGGlyphsWrapper {
> +public:
> +
> +    NS_INLINE_DECL_REFCOUNTING(gfxSVGGlyphsWrapper);

There shouldn't be a semicolon on the end of this; the macro provides all that's needed, and some fussy compilers will complain about the extra one.
(In reply to Cameron McCormack (:heycam) from comment #13)
> Are you concerned about space taken up in the font with lots of
> 'glyphid="..."' strings?  (Can tables be compressed?  If not, it might be
> worth allowing the table to be gzipped at some point in the future.) 

If the font is packaged as WOFF for deployment, the SVG data will be gzipped and should compress pretty well.

> I
> don't mind whether we require the use of <glyph> around the glyph data or
> whether we allow any element to act as a glyph.

Would it make sense to follow the structure used in SVG Fonts (as per http://www.w3.org/TR/SVG/fonts.html, for example) as closely as possible? Obviously we wouldn't need the attributes of the <font> element that contains the <glyph>s, as OpenType is providing that structure, as well as the char-to-glyph mapping, contextual shaping behavior (rather than the very limited Arabic shaping defined for SVG fonts); but it'd be nice if the actual glyph artwork could in general be interoperable between OT-SVG and standard SVG fonts.

I think that would argue for having a collection of <glyph> elements as the containers for each glyph's graphic content. We could either use the 'glyph-name' attribute to hold glyph IDs, using a convention for names such as "g###" where ### is the desired numeric ID, or else add a new 'gid' attribute for this purpose.

(Disclaimer: I don't really know much about SVG or SVG Fonts, having just briefly skimmed some specs.)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)
> I think here we should have an nsTHashtable of nsISupportsHashKey mapping to
> the nsIContent* for the glyph. You'd build this by walking the DOM after
> you've parsed it. We shouldn't need mSearchedMap at all if we do it that way.

Actually, of course you need the key to be the PRInt32 glyph ID and have a value part that's nsCOMPtr<nsIContent>.

(In reply to Jonathan Kew (:jfkthame) from comment #18)
> Would it make sense to follow the structure used in SVG Fonts (as per
> http://www.w3.org/TR/SVG/fonts.html, for example) as closely as possible?
> Obviously we wouldn't need the attributes of the <font> element that
> contains the <glyph>s, as OpenType is providing that structure, as well as
> the char-to-glyph mapping, contextual shaping behavior (rather than the very
> limited Arabic shaping defined for SVG fonts); but it'd be nice if the
> actual glyph artwork could in general be interoperable between OT-SVG and
> standard SVG fonts.

I thought this would be a good idea, but it turns out that there's practically nothing that can be shared from the SVG Fonts specs. Maybe only the <glyph> element (but none of its custom attributes). So I don't think it matters much.
Comment on attachment 589713 [details] [diff] [review]
Patch 1 rev 1 - Basic OpenType SVG functionality

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

A few nits and a couple of questions.

::: content/base/src/nsDocument.cpp
@@ +3177,5 @@
>  
> +  if (!mAttrStyleSheet) {
> +      ResetStylesheetsToURI(mDocumentURI);
> +  }
> +

Why was this necessary?

::: gfx/ots/src/svg.cc
@@ +1,1 @@
> +#include "ots.h"

License here too, and in svg.h.

@@ +14,5 @@
> +}
> +
> +bool ots_svg_serialise(OTSStream *out, OpenTypeFile *file) {
> +  OpenTypeSVG *svg = file->svg;
> +  std::string & svgString = svg->svg;

No space after "&".

::: gfx/thebes/Makefile.in
@@ +79,5 @@
>  	gfxUtils.h \
>  	gfxUserFontSet.h \
>  	nsCoreAnimationSupport.h \
>  	gfxSharedImageSurface.h \
> +    gfxSVGGlyphsWrapper.h \

Tab instead of spaces.

::: gfx/thebes/gfxFont.cpp
@@ +235,5 @@
> +            return false;
> +        }
> +
> +        FallibleTArray<PRUint8> svgTable;
> +        rv = GetFontTable(TRUETYPE_TAG('S','V','G',' '), svgTable);

Spaces after commas.

@@ +1397,5 @@
>      const double devUnitsPerAppUnit = 1.0/double(appUnitsPerDevUnit);
>      bool isRTL = aTextRun->IsRightToLeft();
>      double direction = aTextRun->GetDirection();
>  
> +    bool haveSvgGlyphs = GetFontEntry()->TryGetSVGData();

I'd use "haveSVGGlyphs", since "SVG" rather than "Svg" is used elsewhere in camel case names.

@@ +1525,5 @@
>                                                                       details->mGlyphID);
>                            }
>                        } else {
> +                          double glyphX = x + details->mXOffset;
> +                          x += direction*advance;

If you are moving this line can you put spaces around "*".

::: gfx/thebes/gfxFont.h
@@ +290,5 @@
> +        }
> +        return mSVGGlyphs->HasSVGGlyph(id);
> +    }
> +
> +    bool RenderSVGGlyph(gfxContext* context, PRUint32 id, int drawMode) {

Can drawMode be a real enum instead of an int?  Same in gfxSVGGlyphsWrapper::RenderGlyph.

::: gfx/thebes/gfxSVGGlyphsWrapper.cpp
@@ +57,5 @@
> +    nsCOMPtr<nsIContentViewer> viewer;
> +    rv = docLoaderFactory->CreateInstanceForDocument(nsnull, doc, nsnull, getter_AddRefs(viewer));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    rv = viewer->Init(nsnull, nsIntRect(0, 0, 1000, 1000));

Is it at all possible that the 1000x1000 size you use can affect the SVG content?  What about when you have a root <svg> with no viewBox="" and percentage width="" and height="" (like their default values 100%)?  If so, I guess we'll have to spec this arbitrary size?

@@ +117,5 @@
> +    nsSVGDisplayContainerFrame* displayFrame =
> +        do_QueryFrame(glyphElem->GetPrimaryFrame());
> +
> +    if (!displayFrame) {
> +        NS_WARNING("Couldn't cast to an nsSVGContainerFrame!");

Do we want to restrict glyphs to be only defined by container elements?  If we're not going to use <glyph>, then I think things like

  <svg ...>
    <path glyphid="10" d="M ..."/>
  </svg>

should be allowed.
Comment on attachment 589727 [details] [diff] [review]
Patch 2 rev 1 - Allow SVG glyphs to inherit fill/stroke pattern from object

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

::: gfx/thebes/gfxSVGGlyphsWrapper.cpp
@@ +87,5 @@
>   * If there exists an SVG glyph with the specified glyph id, render it and return true
>   * If no such glyph exists, or in the case of an error return false
>   * @param context The thebes context to draw to
>   * @param id The glyph id
>   * @return true iff rendering succeeded

Add docs for the last three parameters.

@@ +126,5 @@
>  
>      nsSVGRenderState* svgContext = new nsSVGRenderState(context);
>  
> +    nsRefPtr<nsSVGObjectRenderState> objectState =
> +        new nsSVGObjectRenderState (fillPattern, strokePattern,

Remove space before "(".

@@ +136,5 @@
> +    if (strokePattern) {
> +        objectState->mStrokeMatrix = strokePattern->GetMatrix();
> +    } else {
> +        objectState->mStrokePattern = fillPattern;
> +        objectState->mStrokeMatrix = fillPattern->GetMatrix();

Why do we use the stroke here when there is no fill on the referencing <text> element?  If you had

  <text fill="none" stroke="red">A</text>

and the glyph for A was

  <g glyphid="..."><path d="..." fill="objectfill"/></g>

would it be a red filled path?

::: layout/base/nsStyleConsts.h
@@ +245,1 @@
>  

Does making objectfill and objectstroke color values mean that you could write color:objectfill?  Does that make sense, given objectfill and objectstroke refer to paints?

::: layout/style/nsRuleNode.cpp
@@ +6686,5 @@
> +      case NS_STYLE_OBJECT_STROKE_OPACITY:
> +        aField = aObjectStrokeValue;
> +        break;
> +      default:
> +        NS_NOTREACHED("SetSVGObjectOpacity : Unknown keyword");

Remove space before ":".

@@ +6731,5 @@
>  
>    // fill-opacity: factor, inherit, initial
> +  if (!SetSVGObjectOpacity(*aRuleData->ValueForFillOpacity(),
> +                           svg->mFillOpacity, canStoreInRuleTree,
> +                           0.0f, 0.0f)) {

I guess this means objectfillopacity/objectstrokeopacity always computes to 0.  If you're handling this in a later patch, can you add a comment here in the meantime?

::: layout/svg/base/src/nsSVGGeometryFrame.h
@@ +84,5 @@
> +   * Set up cairo context with an object pattern
> +   */
> +  bool SetupObjectPaint(gfxContext *aContext,
> +                        nsSVGObjectRenderState* aObjectState,
> +                        const nsStyleSVGPaint& aPaint);

Inconsistent whitespace around "*"s and "&"s, and in the SetupCairo{Fill,Stroke} declarations below too.
(In reply to Cameron McCormack (:heycam) from comment #20)
> Is it at all possible that the 1000x1000 size you use can affect the SVG
> content?  What about when you have a root <svg> with no viewBox="" and
> percentage width="" and height="" (like their default values 100%)?  If so,
> I guess we'll have to spec this arbitrary size?

I think we definitely have to spec it.

I think making the viewport be the em-square and making the em-square be 0,0,1000,1000 is a fine approach.

> Do we want to restrict glyphs to be only defined by container elements?  If
> we're not going to use <glyph>, then I think things like
> 
>   <svg ...>
>     <path glyphid="10" d="M ..."/>
>   </svg>
> 
> should be allowed.

One issue is what the UA rule for setting objectFill/objectStroke is going to be. Currently the patch has
+svg {
+ fill: objectfill;
+ stroke: objectstroke;
+}
which I don't think is right. Currently the spec says the initial value of fill is "black" and this would effectively break that.

If we have glyph elements, we can simply say
glyph { fill:objectFill; stroke:objectStroke; }
and that'll be good enough.

If we allow glyphid anywhere we'll need to say
*[glyphid] { fill:objectFill; stroke:objectStroke; }
which would be worse for performance. Maybe. I don't know. Would have to be checked. Seems a little weirder anyway.

Or we could make the UA rule use <svg> but only have it apply for the root of a font document (by introducing a new media query value, say? That sounds good.)

OTOH being able to say
<path glyphid="..." .../>
<path glyphid="..." .../>
<path glyphid="..." .../>
would be more compact in the DOM.

I think I prefer the approach of adding a new CSS media query feature for font documents and using that to set the UA rule on the root element of font documents.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> (In reply to Cameron McCormack (:heycam) from comment #20)
> > Is it at all possible that the 1000x1000 size you use can affect the SVG
> > content?  What about when you have a root <svg> with no viewBox="" and
> > percentage width="" and height="" (like their default values 100%)?  If so,
> > I guess we'll have to spec this arbitrary size?
> 
> I think we definitely have to spec it.
> 
> I think making the viewport be the em-square and making the em-square be
> 0,0,1000,1000 is a fine approach.

I still wonder whether there's value in "borrowing" from SVG Fonts here, rather than either speccing an arbitrary value (which will doubtless end up annoying some designer who has existing data or tools that assume a different em-square) or inventing a new way to control it.

It's true that there is very little of SVG Fonts that we'd actually care about, as virtually all the <font> and <glyph>-specific attributes will be handled from the OpenType side, but still... why not an 'SVG ' table that contains something like:

  <svg>
    <font>
      <font-face units-per-em="2048" />
      <!-- we might want to define a new "gid" attribute -->
      <glyph gid="314">
          ...glyph content...
      </glyph>
      <!-- or alternatively, encode glyph IDs in the glyph-name -->
      <glyph glyph-name="g271" d="...path data..." />
      ...etc...
    </font>
  </svg>

If <font-face> is missing (or lacks units-per-em) the default would be 1000, as per the SVG spec, but this allows the designer to work with other units if desired.

I suppose what I'm aiming for here is a situation where the content of the 'SVG ' table can in fact be a valid SVG font, except for the addition of the gid attribute if we decide this is better than requiring a specific form of glyph-name. This could facilitate interoperability for people who want to share glyphs between a standalone-SVG-Font environment and our SVG-in-OpenType implementation.
(In reply to Jonathan Kew (:jfkthame) from comment #23)
> I still wonder whether there's value in "borrowing" from SVG Fonts here,
> rather than either speccing an arbitrary value (which will doubtless end up
> annoying some designer who has existing data or tools that assume a
> different em-square) or inventing a new way to control it.

You can add a transform or child <svg viewbox> to use content in a different design coordinate space. I don't see the value in having new syntax for that.

> If <font-face> is missing (or lacks units-per-em) the default would be 1000,
> as per the SVG spec, but this allows the designer to work with other units
> if desired.

The <font> and <font-face> just seem superfluous.

> I suppose what I'm aiming for here is a situation where the content of the
> 'SVG ' table can in fact be a valid SVG font, except for the addition of the
> gid attribute if we decide this is better than requiring a specific form of
> glyph-name. This could facilitate interoperability for people who want to
> share glyphs between a standalone-SVG-Font environment and our
> SVG-in-OpenType implementation.

Given they'd have to wrap their SVG Font in an OpenType envelope anyway (with generated CMAP etc), I think they could also generate a transform or viewbox to account for their units-per-em.

Plus I think this is a low-priority use-case. There's not a lot of SVG fonts out there, and of those that exist, how many are we expecting to actually be converted to be compatible with this new approach (vs being regenerated from some other source)? And of those, how many use non-default em-square?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
> (In reply to Jonathan Kew (:jfkthame) from comment #23)
> > I still wonder whether there's value in "borrowing" from SVG Fonts here,
> > rather than either speccing an arbitrary value (which will doubtless end up
> > annoying some designer who has existing data or tools that assume a
> > different em-square) or inventing a new way to control it.
> 
> You can add a transform or child <svg viewbox> to use content in a different
> design coordinate space. I don't see the value in having new syntax for that.
> 
> > If <font-face> is missing (or lacks units-per-em) the default would be 1000,
> > as per the SVG spec, but this allows the designer to work with other units
> > if desired.
> 
> The <font> and <font-face> just seem superfluous.

<font-face> could be omitted if the designer wants to use the default 1000-unit em-square. If a different em-square is wanted, then *something* has to be added, whether it's a transform or a viewbox or whatever.... there'd' be other ways to handle this, but the "standard" svg fonts way would be to set the desired units-per-em using <font-face>, why not support that rather than requiring the designer to work around it in a different way?

<font> is superfluous here, I grant, except as a container that groups <font-face> (if present) with the <glyph>s that it relates to; this structure would easily extend to allow multiple <font>s within the SVG table, each of which could use a different em-square.

> 
> > I suppose what I'm aiming for here is a situation where the content of the
> > 'SVG ' table can in fact be a valid SVG font, except for the addition of the
> > gid attribute if we decide this is better than requiring a specific form of
> > glyph-name. This could facilitate interoperability for people who want to
> > share glyphs between a standalone-SVG-Font environment and our
> > SVG-in-OpenType implementation.
> 
> Given they'd have to wrap their SVG Font in an OpenType envelope anyway
> (with generated CMAP etc), I think they could also generate a transform or
> viewbox to account for their units-per-em.

Why is this better than accepting <font-face units-per-em="..."/>? 

> Plus I think this is a low-priority use-case. There's not a lot of SVG fonts
> out there, and of those that exist, how many are we expecting to actually be
> converted to be compatible with this new approach (vs being regenerated from
> some other source)? And of those, how many use non-default em-square?

Probably not many, but I don't see what we gain by breaking with the SVG Fonts way of doing things, for aspects that make sense in both environments. Maybe we can save a handful of bytes of XML here and there, but that's going to be insignificant in comparison to the overall verbosity of SVG glyph data - and compression will deal with it anyway.
(In reply to Jonathan Kew (:jfkthame) from comment #25)
> the "standard" svg fonts way
> would be to set the desired units-per-em using <font-face>, why not support
> that rather than requiring the designer to work around it in a different way?

Because we'd be adding a feature solely for the sake of a slight reduction in the work required to port legacy SVG fonts, which I don't think is a significant benefit.

> <font> is superfluous here, I grant, except as a container that groups
> <font-face> (if present) with the <glyph>s that it relates to; this
> structure would easily extend to allow multiple <font>s within the SVG
> table, each of which could use a different em-square.

I don't know if there's a use-case for that, and anyway it would be awfully confusing since in legacy SVG fonts each <font> is a different font that's independently referenced, and here they're all the same font.

> > Given they'd have to wrap their SVG Font in an OpenType envelope anyway
> > (with generated CMAP etc), I think they could also generate a transform or
> > viewbox to account for their units-per-em.
> 
> Why is this better than accepting <font-face units-per-em="..."/>?

Because it's less legacy cruft to support.
So. Many. Comments. To address the bigger/more broken things:

On <g id="...">, I went with that because it's pretty much already valid SVG (other than the numeric id). I don't like having to say "yeah, it's SVG except for [...]". Further, does it really matter that much if it isn't semantically sound? You've found the document in an OpenType font table---surely that should be enough to hint at what it might be.

> ::: layout/svg/base/src/svg.css
> @@ +52,5 @@
> >  
> > +svg {
> > + fill: objectfill;
> > + stroke: objectstroke;
> > +}
> 
> This should only apply to glyphs!
> 
> Maybe use selector *[glyphid]? Although that might be slow.
> 
> Faster-matching UA style sheet selector argues for using a specific SVG
> <glyph> element instead of using glyphid on any element.

Only SVG glyphs will have an object context, so for anything but SVG glyphs it'll fall back to the default style.

> Using maxAscent here is probably not right.
> 
> I think y=0 in the SVG coordinate space should be the baseline. Then you
> just need to translate by aPoint.y here (aPoint.y is the baseline).

I did have it like this, but found that the top of the SVG box was higher than the top of the text bounding box, which seemed wrong. If we offset it by maxAscent it lines up perfectly. Also it means that constructing reftests is slightly less of a pain. I guess we should stick to however TrueType glyphs do it. I'll look it up. (or if somebody already knows and would like to save me some reading...)

> ::: layout/style/nsStyleStruct.h
> @@ +2117,5 @@
> >  {
> >    union {
> >      nscolor mColor;
> >      nsIURI *mPaintServer;
> > +    bool mFill;
> 
> What does this mean? I think it might make more sense to add
> eStyleSVGPaintType_ObjectFill and eStyleSVGPaintType_ObjectStroke to
> nsStyleSVGPaintType. What do you think?

Makes sense. In fact, I had it like that initially and there's no technical reason it shouldn't be that way, but _Object{Fill,Stroke} seemed to be a bit more specific than the other members of that enum. I guess that is a rather pedantic argument though, and it would be a bit clearer to do it this way.

> @@ +136,5 @@
> > +    if (strokePattern) {
> > +        objectState->mStrokeMatrix = strokePattern->GetMatrix();
> > +    } else {
> > +        objectState->mStrokePattern = fillPattern;
> > +        objectState->mStrokeMatrix = fillPattern->GetMatrix();
> 
> Why do we use the stroke here when there is no fill on the referencing
> <text> element?  If you had
> 
>   <text fill="none" stroke="red">A</text>
> 
> and the glyph for A was
> 
>   <g glyphid="..."><path d="..." fill="objectfill"/></g>
> 
> would it be a red filled path?

I'm not sure what I was thinking here; I'm fairly certain it's a mistake. Though I think your example is the wrong way around; that is, <text fill="red" stroke="none">A</text> would leak "red" into objectstroke.

> ::: layout/base/nsStyleConsts.h
> @@ +245,1 @@
> >  
> 
> Does making objectfill and objectstroke color values mean that you could
> write color:objectfill?  Does that make sense, given objectfill and
> objectstroke refer to paints?

Good point. Paints should have their own lookup table. As it stands, I don't think it'd apply a color:objectfill but indeed just hit that NS_NOTREACHED and do something undefined. I should fix that.

> @@ +6731,5 @@
> >  
> >    // fill-opacity: factor, inherit, initial
> > +  if (!SetSVGObjectOpacity(*aRuleData->ValueForFillOpacity(),
> > +                           svg->mFillOpacity, canStoreInRuleTree,
> > +                           0.0f, 0.0f)) {
> 
> I guess this means objectfillopacity/objectstrokeopacity always computes to
> 0.  If you're handling this in a later patch, can you add a comment here in
> the meantime?

Will do.
(In reply to Edwin Flores from comment #27)
> On <g id="...">, I went with that because it's pretty much already valid SVG
> (other than the numeric id). I don't like having to say "yeah, it's SVG
> except for [...]". Further, does it really matter that much if it isn't
> semantically sound? You've found the document in an OpenType font
> table---surely that should be enough to hint at what it might be.

We shouldn't use "id" because of that dumb XML limitation. Let's use glyphid or gid instead.

> > ::: layout/svg/base/src/svg.css
> > @@ +52,5 @@
> > >  
> > > +svg {
> > > + fill: objectfill;
> > > + stroke: objectstroke;
> > > +}
> > 
> > This should only apply to glyphs!
> > 
> > Maybe use selector *[glyphid]? Although that might be slow.
> > 
> > Faster-matching UA style sheet selector argues for using a specific SVG
> > <glyph> element instead of using glyphid on any element.
> 
> Only SVG glyphs will have an object context, so for anything but SVG glyphs
> it'll fall back to the default style.

Yeah, things will render correctly, but getComputedStyle will return "objectfill"/"objectstroke" instead of "rgb(0,0,0)" or "none". That's bad.

Making this rule conditional on a media query test (@media font?) for being in a font seems like the right way to go. That media value could be useful for other things too.

> Makes sense. In fact, I had it like that initially and there's no technical
> reason it shouldn't be that way, but _Object{Fill,Stroke} seemed to be a bit
> more specific than the other members of that enum. I guess that is a rather
> pedantic argument though, and it would be a bit clearer to do it this way.

Thanks :-).
(In reply to Jonathan Kew (:jfkthame) from comment #23)
> I suppose what I'm aiming for here is a situation where the content of the
> 'SVG ' table can in fact be a valid SVG font, except for the addition of the
> gid attribute if we decide this is better than requiring a specific form of
> glyph-name. This could facilitate interoperability for people who want to
> share glyphs between a standalone-SVG-Font environment and our
> SVG-in-OpenType implementation.

There are other differences though, the most significant of which is probably that the coordinate space of the <glyph> element from SVG Fonts proper being y up (unlike what I assume we are doing here, where we have a y down coordinate system, like the rest of SVG).  Some non-trivial amount of messing with the SVG Font is going to be required to convert it to an SVG-in-OpenType font.
(In reply to Cameron McCormack (:heycam) from comment #29)
> There are other differences though, the most significant of which is
> probably that the coordinate space of the <glyph> element from SVG Fonts
> proper being y up (unlike what I assume we are doing here, where we have a y
> down coordinate system, like the rest of SVG).

We do? Ouch.... offhand, wearing my font-designer hat, I think that's unfortunate and will probably irritate the people who we'd like to see using this facility.

At least, I think we should try to have some discussion in the font community before deciding things like this. If my guess (that the great majority of font designers would prefer to work with y-up coordinates, being the expectation in both SVG and OpenType fonts) is correct, I think it behooves us to implement it that way rather than requiring them all to handle the mismatch of expectations by adding a transform within the font, or whatever.
Thinking a bit more about glyph IDs....

I'd guess that the main use-case for this feature will be for "fancy" (multi-coloured, etc) glyphs such as emoji, company logos and other dingbats, which will typically be "simple glyphs" from a text-shaping point of view; providing fancy SVG glyphs for an entire complex-shaping script such as Arabic or Indic will probably be rare.

As such, I suggest that by far the most convenient way for a font developer to identify glyphs will usually be via Unicode values, not glyph IDs. This is both simpler for the author when originally adding such glyphs to a font, and more robust as the font is updated - revisions to the basic OpenType glyph collection, which may arbitrarily shuffle the numeric glyph IDs, don't require revising the "gid" attributes of the SVG "override" glyphs being provided for certain emoji or PUA codepoints.

So I propose that each glyph (which I still think ought to be an SVG <glyph> element, but whatever....) _may_ have a "unicode" attribute specifying the Unicode character it should represent. If this is present, we'd look up that character in the font's cmap to get the actual glyph ID that it represents. A "gid" or "glyphid" attribute, or a standardized form of glyph name, would only be needed for glyphs that are intended to be unencoded and only accessed via GSUB lookups, etc.

(What if both are present? Either we pick and document which one takes priority - probably glyphid - or else we could allow a single <glyph> to serve as the rendering for multiple glyph ids. Which is actually a perfectly reasonable thing to do.)
(In reply to Jonathan Kew (:jfkthame) from comment #30)
> At least, I think we should try to have some discussion in the font
> community before deciding things like this. If my guess (that the great
> majority of font designers would prefer to work with y-up coordinates, being
> the expectation in both SVG and OpenType fonts) is correct, I think it
> behooves us to implement it that way rather than requiring them all to
> handle the mismatch of expectations by adding a transform within the font,
> or whatever.

That sounds reasonable. We can just specify that the glyph is rendered with an extra transformation that flips it vertically.

(In reply to Jonathan Kew (:jfkthame) from comment #31)
> So I propose that each glyph (which I still think ought to be an SVG <glyph>
> element, but whatever....) _may_ have a "unicode" attribute specifying the
> Unicode character it should represent. If this is present, we'd look up that
> character in the font's cmap to get the actual glyph ID that it represents.
> A "gid" or "glyphid" attribute, or a standardized form of glyph name, would
> only be needed for glyphs that are intended to be unencoded and only
> accessed via GSUB lookups, etc.

That sounds like a good idea. But I wouldn't use "unicode" since in SVG Fonts, "unicode" can match multi-character strings and we don't want to support that here (I assume). Maybe "glyphchar"?

> (What if both are present? Either we pick and document which one takes
> priority - probably glyphid - or else we could allow a single <glyph> to
> serve as the rendering for multiple glyph ids. Which is actually a perfectly
> reasonable thing to do.)

Yes. However, we do have to specify what happens if a glyph ID is matched by multiple SVG elements via glyphid or glyphchar. I think the first element wins (matching what happens with getElementById when multiple elements have the same id).
One argument to make in favour of a "y down" coordinate system for glyphs is that authors will quite likely want to design their glyphs in a standalone document first and then encapsulate them in a font.  I think it would be a bit confusing for authors to have to put a transform="scale(1,-1)" in their content when converting from a "normal" SVG document to a "font" one.  I've experienced this myself when playing around with SVG Fonts; once you know that <glyph>s establish a y up coordinate system them it is clear how to fix your content, but it is the only element in SVG where this is the case.  I had thought that eliminating this difference would be an advantage.

On the other hand, if we keep y down glyphs and have them on a baseline of y = 0, then most of the glyph content is going to fall off the top of the canvas when designing/viewing as a normal document anyway.
Attachment #589713 - Attachment is obsolete: true
Attachment #589713 - Flags: review?(roc)
Attachment #593289 - Flags: review?(roc)
Attachment #593290 - Attachment is obsolete: true
Attachment #593290 - Flags: review?(roc)
Attachment #593293 - Flags: review?(roc)
Attachment #593289 - Flags: review?(roc) → review?(jfkthame)
Comment on attachment 593293 [details] [diff] [review]
Patch 1-2 rev 3 Basic OpenType SVG functionality

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

::: gfx/thebes/Makefile.in
@@ +85,5 @@
>  
> +# gfxSVGGlyphsWrapper needs nsDOMParser.h
> +LOCAL_INCLUDES += -I$(topsrcdir)/content/base/public \
> +				  -I$(topsrcdir)/content/base/src \
> +                  -I$(topsrcdir)/content/svg/content/src \

Fix indent

::: gfx/thebes/gfxFont.cpp
@@ +241,5 @@
> +            return false;
> +        }
> +
> +        mSVGGlyphs = gfxSVGGlyphsWrapper::ParseFromBuffer(svgTable.Elements(),
> +                                                          svgTable.Length());

If this returns null, we crash. Let's not.

@@ +1457,5 @@
>                    glyphX = x;
>                    x += advance;
>                }
> +
> +              if (haveSVGGlyphs && GetFontEntry()->HasSVGGlyph(glyphData->GetSimpleGlyph())) {

We're checking HasSVGGlyph twice per glyph, here and in RenderSVGGlyph. I think you can just remove this check.

@@ +1462,5 @@
> +                  gfxPoint point(ToDeviceUnits(glyphX, devUnitsPerAppUnit),
> +                                 ToDeviceUnits(y, devUnitsPerAppUnit));
> +                  if (RenderSVGGlyph(aContext, point, aDrawMode,
> +                                     glyphData->GetSimpleGlyph())) {
> +                      continue;

Shouldn't RenderSVGGlyph be taking the stroke pattern as a parameter?

::: gfx/thebes/gfxFont.h
@@ +358,5 @@
>      PRUint32         mUVSOffset;
>      nsAutoArrayPtr<PRUint8> mUVSData;
>      gfxUserFontData* mUserFontData;
>  
> +    bool             mSVGInitialized;

move this bool up near the others to avoid padding issues

::: gfx/thebes/gfxSVGGlyphsWrapper.cpp
@@ +72,5 @@
> +gfxSVGGlyphsWrapper::ParseFromBuffer(PRUint8 *aBuffer, PRUint32 aBufLen)
> +{
> +    nsCOMPtr<nsIDocument> doc;
> +    nsresult rv = ParseDocument(aBuffer, aBufLen, getter_AddRefs(doc));
> +    NS_ENSURE_SUCCESS(rv, nsnull);

What happens if the XML is invalid? Do we get an error rv and exit with no glyphs here?

We'd better have a test for that to at least make sure we're exercising the failure path.

@@ +115,5 @@
> +}
> +
> +/**
> + * Initialise the glyphid |-> glyph map. |aContext| and |aFont| params optional
> + * but needed to map glyphchar to glyph

This comment could be more literate :-). Document what aContext and aFont are for.

@@ +127,5 @@
> +        return false;
> +    }
> +
> +    nsCOMPtr<nsIContent> svgRoot = mDocument->GetRootElement();
> +    NS_ASSERTION(svgRoot->IsSVG(), "OpenType SVG table not SVG");

We shouldn't assert this since someone could give us a document with non-SVG root.

But that doesn't actually matter, right? We don't need this assertion.

Also, svgRoot could be null since you can have a document with no elements. That might be a parse error, but let's not rely on that.

@@ +142,5 @@
> +        nsIAtom *tag = child->Tag();
> +
> +        if (!tag->Equals(NS_LITERAL_STRING("defs"))) {
> +            continue;
> +        }

I actually think we should ignore the structure of the document and simply check every element that's in the SVG namespace for a glyphid or glyphchar attribute. This means recursively walking the document.

@@ +155,5 @@
> +            }
> +        }
> +        for (int j = 0; j < nGlyphElems; j++) {
> +            InsertGlyphIds(glyphElems[j]);
> +        }

As written this seems to overwrite existing glyphids, so later elements win. That's not what we want.

I think we should just process the elements in tree order and say that earlier elements win. We don't need to define a precedence between glyphchar and glyphid, since if they're on the same element and map to the same glyph id, you get the same result whichever one wins.

@@ +179,5 @@
> +    }
> +
> +    gfxContextAutoSaveRestore aContextRestorer(aContext);
> +
> +    nsCOMPtr<nsIContent> glyph = mGlyphIdMap.Get(aGlyphId);

Don't make this a strong reference, that's not needed.

@@ +181,5 @@
> +    gfxContextAutoSaveRestore aContextRestorer(aContext);
> +
> +    nsCOMPtr<nsIContent> glyph = mGlyphIdMap.Get(aGlyphId);
> +    if (!glyph) {
> +        return false;

We can just require that the caller has checked HasGlyph previously, so assert instead of checking.

@@ +196,5 @@
> +        NS_WARNING("Non SVG frame for SVG glyph");
> +        return false;
> +    }
> +
> +    nsSVGRenderState* svgContext = new nsSVGRenderState(aContext);

This is leaking.

I think you can just use an on-stack local variable here.

Maybe we can reduce the set of dirs you need to add to LOCAL_INCLUDES by moving most of this method to a new method in nsContentUtils, which you can call from here.

@@ +313,5 @@
> +    return NS_OK;
> +}
> +
> +void
> +gfxSVGGlyphsWrapper::InsertGlyphIds(nsIContent *aGlyph)

aGlyphElement. in fact, should use Element instead of nsIContent* here and in the hashtable

@@ +315,5 @@
> +
> +void
> +gfxSVGGlyphsWrapper::InsertGlyphIds(nsIContent *aGlyph)
> +{
> +    nsCOMPtr<nsIAtom> glyphIdAtom = do_GetAtom("glyphid");

add to nsGkAtomList and use nsGkAtoms

@@ +316,5 @@
> +void
> +gfxSVGGlyphsWrapper::InsertGlyphIds(nsIContent *aGlyph)
> +{
> +    nsCOMPtr<nsIAtom> glyphIdAtom = do_GetAtom("glyphid");
> +    nsString glyphIds;

nsAutoString

@@ +335,5 @@
> +            break;
> +        }
> +
> +        mGlyphIdMap.Put(glyphId, aGlyph);
> +        offset = glyphIds.Find(",", false, offset + 1);

do we really need to support comma-separated IDs? what's the use-case for this?

@@ +341,5 @@
> +}
> +
> +void
> +gfxSVGGlyphsWrapper::InsertGlyphChar(gfxContext *aContext, gfxFont *aFont,
> +                                     nsIContent *aGlyph)

aGlyphElement

@@ +343,5 @@
> +void
> +gfxSVGGlyphsWrapper::InsertGlyphChar(gfxContext *aContext, gfxFont *aFont,
> +                                     nsIContent *aGlyph)
> +{
> +    nsCOMPtr<nsIAtom> glyphCharAtom = do_GetAtom("glyphchar");

nsGkAtoms

@@ +344,5 @@
> +gfxSVGGlyphsWrapper::InsertGlyphChar(gfxContext *aContext, gfxFont *aFont,
> +                                     nsIContent *aGlyph)
> +{
> +    nsCOMPtr<nsIAtom> glyphCharAtom = do_GetAtom("glyphchar");
> +    nsString glyphChar;

nsAutoString

@@ +351,5 @@
> +    }
> +
> +    if (!glyphChar.Length()) {
> +        NS_WARNING("glyphchar is empty");
> +        return;

Don't need to check this here. We'll get an empty length for 'shaped' and exit below.

@@ +357,5 @@
> +
> +    gfxShapedWord *shaped =
> +        aFont->GetShapedWord(aContext, glyphChar.get(), glyphChar.Length(),
> +                             glyphChar.CharAt(0), 0, 100,
> +                             0);

I think we're leaking 'shaped'

@@ +362,5 @@
> +
> +    if (!shaped->Length()) {
> +        NS_WARNING("glyphchar resolution failed");
> +        return;
> +    }

I think we should exit if shaped->Length() > 1 as well. Otherwise it's ambiguous what should happen.

::: gfx/thebes/gfxSVGGlyphsWrapper.h
@@ +48,5 @@
> +#include "nsIDocument.h"
> +#include "gfxPattern.h"
> +
> +
> +typedef mozilla::dom::Element Element;

We can't use a global typedef like this. You can put it in the gfxSVGGlyphsWrapper class though.

@@ +58,5 @@
> +
> +    NS_INLINE_DECL_REFCOUNTING(gfxSVGGlyphsWrapper)
> +
> +    static already_AddRefed<gfxSVGGlyphsWrapper> ParseFromBuffer(
> +                                    PRUint8 *aBuffer, PRUint32 aBufLen);

put ParseFromBuffer at the start of the next line.

@@ +60,5 @@
> +
> +    static already_AddRefed<gfxSVGGlyphsWrapper> ParseFromBuffer(
> +                                    PRUint8 *aBuffer, PRUint32 aBufLen);
> +
> +    bool HasSVGGlyph(PRUint32 aGlyph);

aGlyphId

@@ +62,5 @@
> +                                    PRUint8 *aBuffer, PRUint32 aBufLen);
> +
> +    bool HasSVGGlyph(PRUint32 aGlyph);
> +
> +    bool RenderGlyph(gfxContext *aContext, PRUint32 aGlyphId, int aDrawMode);

Don't we have an enum for aDrawMode?
Comment on attachment 593291 [details] [diff] [review]
Patch 2-1 rev 2 Add support for CSS objectfill and objectstroke values

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

You probably should separate out the fill-opacity and stroke-opacity changes to their own patch.

You'll need to add something to property_database.js for testing.

This will need style-system review; which by default means dbaron but he might be able to delegate to someone else.

::: layout/base/nsPresContext.h
@@ +980,5 @@
> +      return mDrawingSVGGlyph;
> +  }
> +
> +  void SetDrawingSVGGlyph(bool aValue) {
> +      mDrawingSVGGlyph = aValue;

I'd call these SetIsFont/IsFont.

::: layout/svg/base/src/svg.css
@@ +50,5 @@
>   overflow: hidden;
>  }
>  
> +@media all and (-moz-font) {
> + svg {

Make this :root
> Shouldn't RenderSVGGlyph be taking the stroke pattern as a parameter?

Never mind, I see you add this later.
Comment on attachment 593292 [details] [diff] [review]
Patch 2-2 rev 2 Let OpenType SVG glyphs inherit object fill and stroke paints

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

::: gfx/thebes/gfxFont.cpp
@@ +1755,5 @@
>  
>  bool
>  gfxFont::RenderSVGGlyph(gfxContext *aContext, gfxPoint aPoint, DrawMode aDrawMode,
> +                        PRUint32 aGlyphId, gfxPattern *aStrokePattern,
> +                        gfxMatrix aStrokeMatrix)

const gfxMatrix&. Copying those sucks.

@@ +1784,5 @@
> +
> +    if (aStrokePattern) {
> +        aStrokePattern->SetMatrix(strokeMatrixOrig);
> +    }
> +    fillPattern->SetMatrix(fillMatrix);

Why do you need to save and restore this matrix?

Or rather, why don't you need to set the matrix on fillPattern?

::: layout/svg/base/src/nsSVGGeometryFrame.cpp
@@ +200,5 @@
> +  if (!pattern) {
> +    return false;
> +  }
> +
> +  pattern->SetMatrix(aContext->CurrentMatrix().Multiply(*matrix));

Is this Multiply correct? Why not just set the matrix directly?

::: layout/svg/base/src/nsSVGUtils.h
@@ +150,5 @@
>  #undef CLIP_MASK
>  
> +struct nsSVGObjectRenderState
> +{
> +  NS_INLINE_DECL_REFCOUNTING(nsSVGObjectRenderState);

Why does this need to be refcounted?

Seems to me this stuff could go directly on nsSVGRenderState, or on a struct that goes directly on nsSVGRenderState.

@@ +163,5 @@
> +  gfxPattern *mStrokePattern;
> +  gfxMatrix mFillMatrix;
> +  gfxMatrix mStrokeMatrix;
> +  bool mDoFill;
> +  bool mDoStroke;

Do we need these booleans? Surely a null pattern can be used to indicate false?
Don't forget to attach the complete set of reftest patches (the first one is still missing).
Comment on attachment 593289 [details] [diff] [review]
Patch 1-1 rev 2 Add OTS support for OpenType SVG table

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

r=jfkthame, with a couple of notes as below.

::: gfx/ots/src/svg.cc
@@ +4,5 @@
> +
> +#include "ots.h"
> +
> +#include "svg.h"
> +#include <string>

Not needed, AFAICS.

Also, please add a comment noting that this just _preserves_ the SVG glyph table, it does not _sanitize_ it. (In other words, we are relying on the consumer of that data to safely handle any arbitrary junk that may be present - OTS is not ensuring that it is well-formed, valid, or anything.)

::: gfx/ots/src/svg.h
@@ +4,5 @@
> +
> +#ifndef OTS_SVG_H
> +#define OTS_SVG_H
> +
> +#include <string>

Not needed.
Attachment #593289 - Flags: review?(jfkthame) → review+
Comment on attachment 593293 [details] [diff] [review]
Patch 1-2 rev 3 Basic OpenType SVG functionality

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

::: gfx/thebes/gfxSVGGlyphsWrapper.cpp
@@ +357,5 @@
> +
> +    gfxShapedWord *shaped =
> +        aFont->GetShapedWord(aContext, glyphChar.get(), glyphChar.Length(),
> +                             glyphChar.CharAt(0), 0, 100,
> +                             0);

We don't want to be calling GetShapedWord here, just to map a Unicode character to a glyph ID. What we want is to map the char to a glyph ID using the font's cmap table, which you can do using gfxFontUtils::MapCharToGlyph.

A complication, though, is that we really need to handle Unicode Variation Selector sequences, where glyphChar contains two Unicode characters, a base character followed by a variation selector. gfxFontUtils::MapCharToGlyph doesn't support that, so you have to locate the UVS mapping table (if present) and pass it to MapUVSToGlyphFormat14. There's currently code in gfxHarfBuzzShaper that does this, but no convenient utility you can use here. It would probably make sense for us to factor this out into a simple method on gfxFont instead, and then both this code and gfxHarfBuzzShaper could call that.

I was initially going to just mention the UVS issue, and suggest it could be a followup, but it's actually quite significant, I think: one of the new features of Unicode 6.1, released yesterday (http://www.unicode.org/versions/Unicode6.1.0/), is the addition of a whole bunch of UVS sequences to distinguish "text-style" vs. "emoji-style" display of emoji characters - which is exactly the sort of use-case where I'd expect people to be interested in using SVG glyphs.
This adds UVS support to gfxFontUtils::MapCharToGlyph. This should be sufficient to make it fairly easy to handle the glyphChar attribute for SVG glyphs - just get the font's cmap table, and call this.

Calling gfxFontUtils::MapCharToGlyph repeatedly for the same font is somewhat inefficient, which is why the harfbuzz shaper doesn't simply do that, but performance is less critical here - the cmap lookup will be overshadowed by the rest of SVG parsing, etc, anyway. Still, as a further step, we might want to take the table & subtable-offset management that's done in gfxHarfBuzzShaper and move it all into a method on gfxFont, and then just use that instead.
Attachment #593367 - Flags: review?(roc)
Why not just use GetShapedWord? It's even less efficient, but that almost certainly won't ever matter, and it will handle everything we might ever want to handle, even stuff like two letters that form a ligature glyph. And we don't have to extend MapCharToGlyph.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #47)
> Why not just use GetShapedWord? It's even less efficient, but that almost
> certainly won't ever matter, and it will handle everything we might ever
> want to handle, even stuff like two letters that form a ligature glyph. And
> we don't have to extend MapCharToGlyph.

It's also potentially more confusing for the author, as it may apply GSUB (or Graphite, or AAT...) substitutions - but we're not exposing any control to let the author choose _which_ such rules it would be using. A font might map character U+xxxx to various different glyphs depending on the script and/or language in use, as well as contextual variants, etc. For clarity, we should say that glyphChar means precisely the glyph ID that is provided by the cmap, not the result of some further application of certain shaping rules to that.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #42)
> ::: layout/svg/base/src/nsSVGGeometryFrame.cpp
> @@ +200,5 @@
> > +  if (!pattern) {
> > +    return false;
> > +  }
> > +
> > +  pattern->SetMatrix(aContext->CurrentMatrix().Multiply(*matrix));
> 
> Is this Multiply correct? Why not just set the matrix directly?

When the pattern is set, cairo sets the pattern context matrix to be the inverse of the context matrix which is lame and wrong because the pattern already has the correct transform. Multiplying by the context matrix here nulls out the multiplication that happens after setting the pattern.

> @@ +1784,5 @@
> > +
> > +    if (aStrokePattern) {
> > +        aStrokePattern->SetMatrix(strokeMatrixOrig);
> > +    }
> > +    fillPattern->SetMatrix(fillMatrix);
> 
> Why do you need to save and restore this matrix?
> 
> Or rather, why don't you need to set the matrix on fillPattern?

The above multiplication is also done at the start of gfxFont::Draw for the same reason. Its original state is restored here so it doesn't happen for a second time down in SetupObjectState. I suppose it could be moved so it's a bit clearer.

> ::: layout/svg/base/src/nsSVGUtils.h
> @@ +150,5 @@
> >  #undef CLIP_MASK
> >  
> > +struct nsSVGObjectRenderState
> > +{
> > +  NS_INLINE_DECL_REFCOUNTING(nsSVGObjectRenderState);
> 
> Why does this need to be refcounted?
> 
> Seems to me this stuff could go directly on nsSVGRenderState, or on a struct
> that goes directly on nsSVGRenderState.

Yeah, refcounting is unnecessary, I guess. I do like just holding a pointer to it, though, then we have objectstate = nsnull iff there is no outer object.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #39)> @@ +62,5 @@
> > +                                    PRUint8 *aBuffer, PRUint32 aBufLen);
> > +
> > +    bool HasSVGGlyph(PRUint32 aGlyph);
> > +
> > +    bool RenderGlyph(gfxContext *aContext, PRUint32 aGlyphId, int aDrawMode);
> 
> Don't we have an enum for aDrawMode?

Circular dependency -- gfxFont.h includes gfxSVGGlyphsWrapper.h includes gfxFont.h...
(In reply to Edwin Flores from comment #50)
> Yeah, refcounting is unnecessary, I guess. I do like just holding a pointer
> to it, though, then we have objectstate = nsnull iff there is no outer
> object.

Generally speaking creating extra heap-allocated objects should be avoided if the data has a 1:1 relationship with an existing object. In this case the size of the object is small and nsSVGRenderState lives on the stack anyway, so there's only a few around a a time, so adding some fields is no problem.

> Circular dependency -- gfxFont.h includes gfxSVGGlyphsWrapper.h includes
> gfxFont.h...

Can we break that somehow? forward-declare gfxSVGGlyphsWrapper in some way?

BTW gfxSVGGlyphsWrapper should maybe just be called gfxSVGGlyphs.
Attachment #593289 - Attachment is obsolete: true
Attachment #594936 - Flags: review?(jfkthame)
Still have to add UVS support and redo reftests since the document format has changed.
Attachment #594936 - Flags: review?(jfkthame) → review+
Comment on attachment 594937 [details] [diff] [review]
Patch 1-2 rev 4 Basic OpenType SVG functionality

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

Pretty good, it's all nits now.

::: gfx/thebes/gfxFont.h
@@ +350,5 @@
>      gfxSparseBitSet  mCharacterMap;
>      PRUint32         mUVSOffset;
>      nsAutoArrayPtr<PRUint8> mUVSData;
>      gfxUserFontData* mUserFontData;
> +    bool             mSVGInitialized;

move this bool up near the others to avoid padding issues

::: gfx/thebes/gfxSVGGlyphs.cpp
@@ +114,5 @@
> +    return result.forget();
> +}
> +
> +/**
> + * Initialise the glyphid -> glyph map.

Initialize. We're all Americans here.

@@ +199,5 @@
> +    }
> +
> +    nsSVGRenderState svgContext(aContext);
> +
> +    displayFrame->PaintSVG(&svgContext, nsnull);

> Maybe we can reduce the set of dirs you need to add to LOCAL_INCLUDES by moving most of this method to a new method in nsContentUtils,
> which you can call from here.

Did you try this?

@@ +320,5 @@
> +    if (!aGlyphElement->GetAttr(kNameSpaceID_None, nsGkAtoms::glyphid, glyphIdStr)) {
> +        return;
> +    }
> +
> +	nsresult rv;

fix indent.

@@ +324,5 @@
> +	nsresult rv;
> +	PRUint32 glyphId = glyphIdStr.ToInteger(&rv, kRadix10);
> +
> +	if (NS_FAILED(rv)) {
> +		return;

and here!

@@ +347,5 @@
> +    }
> +
> +    PRUint32 glyphId = gfxFontUtils::MapCharToGlyph(aCmapTable.Elements(),
> +                                                    aCmapTable.Length(),
> +                                                    glyphChar.CharAt(0));

This needs to pass the UVS selector parameter. You can do that in a separate patch for UVS support if you want.

However, we do need to check the string length. We shouldn't just ignore any characters, so without UVS support, we should ignore glyphchar if there's more than one character.

::: gfx/thebes/gfxSVGGlyphs.h
@@ +52,5 @@
> +
> +class gfxSVGGlyphs {
> +private:
> +	typedef mozilla::dom::Element Element;
> +	typedef gfxFont::DrawMode DrawMode;

Fix indent!

@@ +56,5 @@
> +	typedef gfxFont::DrawMode DrawMode;
> +
> +public:
> +
> +    NS_INLINE_DECL_REFCOUNTING(gfxSVGGlyphs)

Remove blank line above

@@ +59,5 @@
> +
> +    NS_INLINE_DECL_REFCOUNTING(gfxSVGGlyphs)
> +
> +    static already_AddRefed<gfxSVGGlyphs>
> +                ParseFromBuffer(PRUint8 *aBuffer, PRUint32 aBufLen);

Put it at the start of the line, right below "static"
Comment on attachment 594938 [details] [diff] [review]
Patch 2-1 rev 3 Add support for CSS objectfill and objectstroke values

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

The -moz-font media feature could probably be broken out into its own patch. But both of these patches will need review from a style system person, preferably dbaron.
Attachment #594938 - Flags: review?(roc) → review?(dbaron)
Comment on attachment 594939 [details] [diff] [review]
Patch 2-2 rev 3 Let OpenType SVG glyphs inherit object fill and stroke paints

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

::: gfx/thebes/gfxFont.cpp
@@ +1785,5 @@
>  
>  bool
>  gfxFont::RenderSVGGlyph(gfxContext *aContext, gfxPoint aPoint, DrawMode aDrawMode,
> +                        PRUint32 aGlyphId, gfxPattern *aStrokePattern,
> +                        gfxMatrix aStrokeMatrix)

const gfxMatrix&, per my previous comment

::: layout/svg/base/src/nsSVGUtils.h
@@ +182,5 @@
>    bool IsPaintingToWindow() { return mPaintingToWindow; }
>  
> +  // Outer object properties for when we are drawing an SVG glyph
> +  void SetIsFont(bool aIsFont) { mIsFont = aIsFont; }
> +  bool IsFont() { return mIsFont; }

You don't use these anywhere.

@@ +207,5 @@
> +  bool mIsFont;
> +  gfxPattern *mFillPattern;
> +  gfxPattern *mStrokePattern;
> +  gfxMatrix mFillMatrix;
> +  gfxMatrix mStrokeMatrix;

Let's call these mOuterObjectFillPattern, etc, and rename the methods accordingly.

Updated

7 years ago
Keywords: sec-review-needed
Attachment #594937 - Attachment is obsolete: true
Attachment #594937 - Flags: review?(roc)
Attachment #595680 - Flags: review?(roc)
These patch numbers are really getting out of hand.
Attachment #594938 - Attachment is obsolete: true
Attachment #594938 - Flags: review?(dbaron)
Attachment #595682 - Flags: review?(dbaron)
Attachment #595682 - Attachment is obsolete: true
Attachment #595682 - Flags: review?(dbaron)
Attachment #595684 - Flags: review?(dbaron)
Writing reftests proving to be a bit more of a pain than I had thought, so those are to come tomorrow hopefully.

Also still to do: track down memory leak. Bleh.
Comment on attachment 595680 [details] [diff] [review]
Patch 1-2 rev 5 Basic OpenType SVG functionality

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

r+ with that.

::: gfx/thebes/gfxSVGGlyphs.cpp
@@ +339,5 @@
> +        case 0:
> +            NS_WARNING("glyphchar is empty");
> +            return;
> +        default:
> +            NS_WARNING("glyphchar contains more than one character");

"more than two characters"

::: gfx/thebes/gfxSVGGlyphs.h
@@ +49,5 @@
> +#include "gfxPattern.h"
> +#include "gfxFont.h"
> +
> +
> +class gfxSVGGlyphs {

Add a comment explaining what this class is for.
Attachment #595680 - Flags: review?(roc) → review+
Comment on attachment 595937 [details] [diff] [review]
Patch 2-2 rev 4 Let OpenType SVG glyphs inherit object fill and stroke paints

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

r+ with that

::: layout/svg/base/src/nsSVGUtils.h
@@ +190,5 @@
> +
> +  void SetOuterObjectStrokePattern(gfxPattern *aPattern) { mOuterObjectStrokePattern = aPattern; }
> +  gfxPattern *GetOuterObjectStrokePattern() { return mOuterObjectStrokePattern; }
> +
> +  void SetOuterObjectFillMatrix(gfxMatrix aMatrix) { mOuterObjectFillMatrix = aMatrix; }

const gfxMatrix&

@@ +191,5 @@
> +  void SetOuterObjectStrokePattern(gfxPattern *aPattern) { mOuterObjectStrokePattern = aPattern; }
> +  gfxPattern *GetOuterObjectStrokePattern() { return mOuterObjectStrokePattern; }
> +
> +  void SetOuterObjectFillMatrix(gfxMatrix aMatrix) { mOuterObjectFillMatrix = aMatrix; }
> +  gfxMatrix &GetOuterObjectFillMatrix() { return mOuterObjectFillMatrix; }

return a const gfxMatrix&

@@ +193,5 @@
> +
> +  void SetOuterObjectFillMatrix(gfxMatrix aMatrix) { mOuterObjectFillMatrix = aMatrix; }
> +  gfxMatrix &GetOuterObjectFillMatrix() { return mOuterObjectFillMatrix; }
> +
> +  void SetOuterObjectStrokeMatrix(gfxMatrix aMatrix) { mOuterObjectStrokeMatrix = aMatrix; }

const gfxMatrix&

@@ +194,5 @@
> +  void SetOuterObjectFillMatrix(gfxMatrix aMatrix) { mOuterObjectFillMatrix = aMatrix; }
> +  gfxMatrix &GetOuterObjectFillMatrix() { return mOuterObjectFillMatrix; }
> +
> +  void SetOuterObjectStrokeMatrix(gfxMatrix aMatrix) { mOuterObjectStrokeMatrix = aMatrix; }
> +  gfxMatrix &GetOuterObjectStrokeMatrix() { return mOuterObjectStrokeMatrix; }

return a const gfxMatrix&
Attachment #595937 - Flags: review?(roc) → review+
Finally.
Attachment #589734 - Attachment is obsolete: true
Attachment #589734 - Flags: review?(roc)
Attachment #596550 - Flags: review?(roc)
Comment on attachment 596550 [details] [diff] [review]
Reftest Patch 1 - Basic tests for OpenType SVG glyphs

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

Seems to be the best testing strategy here would be to have the references not use SVG fonts at all. Can we do that? For example, could we have just an SVG or SVG+HTML file as reference?

Also, can you document somewhere what SVG glyphs the fonts contain, and what those glyphs render as? And document what each test is testing?

::: layout/reftests/text-svgglyphs/svg-glyph-positioning.html
@@ +13,5 @@
> +}
> +</style>
> +</head>
> +<body>
> +<div style="position:relative;">

why position:relative?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #70)
> Seems to be the best testing strategy here would be to have the references
> not use SVG fonts at all. Can we do that? For example, could we have just an
> SVG or SVG+HTML file as reference?

The references only use svg.ttf to figure out where to place things. For the actual graphics it uses the SVG images in resources/. It would be (and indeed was) a bit tedious to place them by hand so I decided to do it this way.
Comment on attachment 595680 [details] [diff] [review]
Patch 1-2 rev 5 Basic OpenType SVG functionality

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

Looks very cool :) - just a few questions in passing...

::: gfx/thebes/gfxFont.cpp
@@ +260,5 @@
> +            return false;
> +        }
> +
> +        nsRefPtr<gfxSVGGlyphs> svgGlyphs =
> +            gfxSVGGlyphs::ParseFromBuffer(svgTable.Elements(), svgTable.Length());

Does the gfxSVGGlyphs object need refcounting at all? Offhand, it looks like it could simply be owned by an nsAutoPtr in the font entry. Which would avoid the need for any manual refcount-munging.

@@ +269,5 @@
> +            NS_ADDREF(mSVGGlyphs);
> +
> +            FallibleTArray<PRUint8> cmapTable;
> +            nsresult rv = GetFontTable(TRUETYPE_TAG('c', 'm', 'a', 'p'), cmapTable);
> +            NS_ENSURE_SUCCESS(rv, false);

If we fail to get the cmap table here, we should discard the svgGlyphs altogether rather than leave it hanging around uselessly. Just don't set mSVGGlyphs until after it has been successfully Init()ed?

@@ +1492,5 @@
> +                  if (RenderSVGGlyph(aContext, point, aDrawMode,
> +                                     glyphData->GetSimpleGlyph())) {
> +                      continue;
> +                  }
> +              }

Should SVG glyphs be affected by synthetic italic and bold styling? (I think so, for consistency with how regular OpenType glyphs are handled.) It doesn't look like this will currently work.

::: gfx/thebes/gfxPlatform.cpp
@@ +1353,5 @@
>          gfxFontCache::GetCache()->AgeAllGenerations();
>      } else if (!strcmp(BIDI_NUMERAL_PREF, aPref)) {
>          mBidiNumeralOption = UNINITIALIZED_VALUE;
> +    } else if (!strcmp(GFX_PREF_OPENTYPE_SVG, aPref)) {
> +        gfxPlatformFontList::PlatformFontList()->ClearPrefFonts();

Why do we need to call ClearPrefFonts() here?

::: gfx/thebes/gfxSVGGlyphs.cpp
@@ +120,5 @@
> + * @param aCmapTable Buffer containing the raw cmap table data
> + */
> +bool
> +gfxSVGGlyphs::Init(const gfxFontEntry *aFontEntry,
> +                   FallibleTArray<PRUint8> &aCmapTable)

aCmapTable can be declared const, I think (here and in following functions that it's passed to).

@@ +297,5 @@
> +    return NS_OK;
> +}
> +
> +void
> +gfxSVGGlyphs::InsertGlyphIds(Element *aGlyphElement)

This only accepts a single ID in the glyphid attribute, right? In that case it should be "InsertGlyphId" (singular).

@@ +332,5 @@
> +            varSelector = 0;
> +            break;
> +        case 2:
> +            // Assume the second character is a UVS selector -- if it isn't,
> +            // MapCharToGlyph will just quietly fail anyway

It won't "fail", IIRC, it'll return the default glyph for the first character, as the "variation sequence" will not be recognized. I think you need to check gfxFontUtils::IsVarSelector(glyphChar.CharAt(1)); if not true, issue a warning and return.

@@ +339,5 @@
> +        case 0:
> +            NS_WARNING("glyphchar is empty");
> +            return;
> +        default:
> +            NS_WARNING("glyphchar contains more than one character");

Need to add "return" here, so as to ignore the invalid attribute.
(In reply to Edwin Flores from comment #71)
> The references only use svg.ttf to figure out where to place things. For the
> actual graphics it uses the SVG images in resources/. It would be (and
> indeed was) a bit tedious to place them by hand so I decided to do it this
> way.

I think it's better to not use fonts at all. Just make sure your test font uses simple metrics (like the Ahem font does) and then we should be able to easily figure out where each character should be rendered.
Comment on attachment 595683 [details] [diff] [review]
Patch 2-1-2 rev 4 Add support for objectfill and objectstroke values to CSS parser

># HG changeset patch
># Parent ae73a88fa53127845c1c32895776507f1f652dbf
>Bug 719286 - Add support for objectfill and objectstroke values to CSS parser r=dbaron

Is there a description of what these values do that I can read somewhere?

>diff --git a/gfx/thebes/gfxSVGGlyphs.cpp b/gfx/thebes/gfxSVGGlyphs.cpp

This seems like it belongs in a different patch.

>diff --git a/layout/style/nsCSSKeywordList.h b/layout/style/nsCSSKeywordList.h
>+CSS_KEY(objectfill, objectfill)
>+CSS_KEY(objectstroke, objectstroke)

These names aren't very CSS-like.  In CSS they should be hyphenated as object-fill and object-stroke.


>diff --git a/layout/style/nsCSSParser.cpp b/layout/style/nsCSSParser.cpp
>+  if (!ParseVariant(x, VARIANT_HCK | VARIANT_NONE | VARIANT_URL,
>+                    nsCSSProps::kObjectPatternKTable)) {
>+    return false;
>+  }

Given this, you should have a comment in nsStyleConsts.h explaining that the two values you're adding are only valid for SVG paint, and not for other color values.

>+const PRInt32 nsCSSProps::kObjectPatternKTable[] = {
>+  eCSSKeyword_objectfill, NS_COLOR_OBJECTFILL,
>+  eCSSKeyword_objectstroke, NS_COLOR_OBJECTSTROKE,
>+  eCSSKeyword_UNKNOWN,-1
>+};

You should add a comment to kColorKTable mentioning the existence of kObjectPatternKTable.

>diff --git a/layout/style/nsRuleNode.cpp b/layout/style/nsRuleNode.cpp
>--- a/layout/style/nsRuleNode.cpp
>+++ b/layout/style/nsRuleNode.cpp
>@@ -6586,16 +6586,34 @@ nsRuleNode::ComputeColumnData(void* aSta
>               column->mColumnFill, canStoreInRuleTree,
>               SETDSC_ENUMERATED, parent->mColumnFill,
>               NS_STYLE_COLUMN_FILL_BALANCE,
>               0, 0, 0, 0);
> 
>   COMPUTE_END_RESET(Column, column)
> }
> 
>+static bool
>+SetSVGObjectPaint(const nsCSSValue& aValue, nsStyleSVGPaint& aResult) {

Local style has the { on its own line, and has 2-space indent (throughout the file).

>diff --git a/layout/svg/base/src/svg.css b/layout/svg/base/src/svg.css
>--- a/layout/svg/base/src/svg.css
>+++ b/layout/svg/base/src/svg.css
>@@ -45,16 +45,23 @@ style, script, symbol {
> switch {
>  -moz-binding: none !important;
> }
> 
> svg:not(:root), symbol, image, marker, pattern, foreignObject {
>  overflow: hidden;
> }
> 
>+@media all and (-moz-font) {
>+ :root {
>+   fill: objectfill;
>+   stroke: objectstroke;
>+ }
>+}
>+
> foreignObject {
>   margin: 0 ! important;
>   padding: 0 ! important;
>   border-width: 0 ! important;
> }
> 
> @media all and (-moz-is-resource-document) {
>  foreignObject *|* {

This also seems like it ought to be part of a different patch.


Also, please add uses of the new values to layout/style/test/property_database.js and make sure that style system mochitests pass.

Could you address those comments (and answer the question at the top) and post a new patch for review?
Attachment #595683 - Flags: review?(dbaron) → review-
Comment on attachment 595684 [details] [diff] [review]
Patch 2-1-1 rev 4 Add -moz-font media feature for SVG Glyphs

># HG changeset patch
># Parent c8e95a5c0acb470ccb56b14a239eaec12f208500
>Bug 719286 - Add support for -moz-font media feature r=dbaron

Based on what I think this patch is doing, -moz-font doesn't seem like a great name for this feature -- in the context of media queries it seems like it's a query for a device that supports fonts.

Could you also explain in the commit message whether the feature is intended to be standardized and used on the Web or whether it's for internal use only.

>diff --git a/layout/base/nsPresContext.h b/layout/base/nsPresContext.h

This file uses 2 space indent.

>diff --git a/layout/style/test/test_media_queries.html b/layout/style/test/test_media_queries.html
>--- a/layout/style/test/test_media_queries.html
>+++ b/layout/style/test/test_media_queries.html
>@@ -623,16 +623,24 @@ function run() {
>   expression_should_be_parseable("-moz-windows-theme: luna-silver");
>   expression_should_be_parseable("-moz-windows-theme: royale");
>   expression_should_be_parseable("-moz-windows-theme: generic");
>   expression_should_be_parseable("-moz-windows-theme: zune");
>   expression_should_be_parseable("-moz-windows-theme: garbage");
>   expression_should_not_be_parseable("-moz-windows-theme: ''");
>   expression_should_not_be_parseable("-moz-windows-theme: ");
> 
>+  // OpenType SVG media features
>+  query_should_be_parseable("(-moz-font)");
>+  query_should_not_be_parseable("not (-moz-font)");
>+  query_should_not_be_parseable("only (-moz-font)");
>+  query_should_be_parseable("all and (-moz-font)");
>+  query_should_be_parseable("not all and (-moz-font)");
>+  query_should_be_parseable("only all and (-moz-font)");

How about testing that this document isn't a font?  And test with both the (-moz-font) syntax and the (-moz-font: 1) / (-moz-font: 0) syntax?

Could you address the comments/questions and post a new patch?
Attachment #595684 - Flags: review?(dbaron) → review-
(In reply to David Baron [:dbaron] (busy through 12 Feb) from comment #74)
> Comment on attachment 595683 [details] [diff] [review]
> Patch 2-1-2 rev 4 Add support for objectfill and objectstroke values to CSS
> parser
> 
> ># HG changeset patch
> ># Parent ae73a88fa53127845c1c32895776507f1f652dbf
> >Bug 719286 - Add support for objectfill and objectstroke values to CSS parser r=dbaron
> 
> Is there a description of what these values do that I can read somewhere?

Not currently; I should really update the wiki page at some point to reflect where the current implementation is/is going. Basically these values are used to let SVG glyphs inherit paints from its context.

That is, if one were to have:

<text style="font-family: 'Some font with SVG'" fill="red" stroke="blue">
L
</text>

then the SVG glyph for L could use the objectFill keyword instead of a paint and have it resolve to red. Similar for objectStroke. Of course it also works for gradients and patterns.

> >diff --git a/layout/style/nsCSSKeywordList.h b/layout/style/nsCSSKeywordList.h
> >+CSS_KEY(objectfill, objectfill)
> >+CSS_KEY(objectstroke, objectstroke)
> 
> These names aren't very CSS-like.  In CSS they should be hyphenated as
> object-fill and object-stroke.

It seems that the SVG convention for values is to use camelCase rather than hyphenation.

(In reply to David Baron [:dbaron] (busy through 12 Feb) from comment #75)
> Based on what I think this patch is doing, -moz-font doesn't seem like a
> great name for this feature -- in the context of media queries it seems like
> it's a query for a device that supports fonts.

True. Would something like is-glyph or is-svg-glyph be better?
(In reply to Jonathan Kew (:jfkthame) from comment #72)
> @@ +1492,5 @@
> > +                  if (RenderSVGGlyph(aContext, point, aDrawMode,
> > +                                     glyphData->GetSimpleGlyph())) {
> > +                      continue;
> > +                  }
> > +              }
> 
> Should SVG glyphs be affected by synthetic italic and bold styling? (I think
> so, for consistency with how regular OpenType glyphs are handled.) It
> doesn't look like this will currently work.

I'm not sure how synthetic bold would work. Since we're allowing arbitrary SVG, simply drawing the glyph at a few different offsets like we're doing now won't really work. The simplest case I can think of is partially transparent glyphs: they'll just look blurry rather than bold.

Synthetic italic we can probably do quite easily: it'd just be a case of modifying the context matrix, as far as I can tell.
I think we should skip synthetic bold and italic for now. We can add them later if someone can figure out how synthetic bold should work.
Attachment #595680 - Attachment is obsolete: true
Attachment #596898 - Flags: review?(roc)
Attachment #596898 - Flags: feedback?(jfkthame)
Comment on attachment 596898 [details] [diff] [review]
Patch 1-2 rev 6 Basic OpenType SVG functionality

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

r+ with that

::: gfx/thebes/gfxSVGGlyphs.cpp
@@ +153,5 @@
> +                                const FallibleTArray<PRUint8> &aCmapTable)
> +{
> +    PRUint32 nChildren;
> +    nsIContent * const *children = aElem->GetChildArray(&nChildren);
> +    for (int i = nChildren - 1; i >= 0; i--) {

Actually instead of GetChildArray, it would be better to use GetFirstChild()/GetNextSibling(). GetChildArray needs to go away at some point.
Attachment #596898 - Flags: review?(roc) → review+
Carrying over r=roc from comment #81
Attachment #596898 - Attachment is obsolete: true
Attachment #596898 - Flags: feedback?(jfkthame)
Attachment #596930 - Flags: review+
Attachment #596550 - Attachment is obsolete: true
Attachment #596550 - Flags: review?(roc)
Attachment #596933 - Flags: review?(roc)
Attachment #589739 - Attachment is obsolete: true
Attachment #589740 - Attachment is obsolete: true
Comment on attachment 596933 [details] [diff] [review]
Reftest Patch 1 rev 2 - Basic tests for OpenType SVG glyphs

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

If you turn the TTF files into WOFF fonts, they'll probably be smaller.

r+ with that.
Attachment #596933 - Flags: review?(roc) → review+
Comment on attachment 596934 [details] [diff] [review]
Reftest Patch 2 - Tests for objectfill/objectstroke

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

You should probably have a test for solid objectStroke as well.
Attachment #596934 - Flags: review?(roc) → review+
Carrying over r=roc from comment #85
Attachment #596933 - Attachment is obsolete: true
Attachment #597199 - Flags: review+

Updated

7 years ago
Blocks: 490986
Talking with roc on irc, we don't currently have a spec for this work.  In lieu of a formal spec proposal, it would be nice to have a wiki page that describes the functionality, how it differs from Adobe's draft proposal [1] and what extensions we've added (e.g. objectfill/objectstroke).  A more formal spec proposal can expand on this in the future, a wiki page would allow others to get a better understanding of the functionality included in these patches.

[1] http://lists.w3.org/Archives/Public/public-svgopentype/2011Oct/0000.html
(In reply to John Daggett (:jtd) from comment #91)
> Talking with roc on irc, we don't currently have a spec for this work.  In
> lieu of a formal spec proposal, it would be nice to have a wiki page that
> describes the functionality, how it differs from Adobe's draft proposal [1]
> and what extensions we've added (e.g. objectfill/objectstroke).

Boom. https://wiki.mozilla.org/SVGOpenTypeFonts#Prototype_Implementation
(In reply to Edwin Flores from comment #92) 
> Boom. https://wiki.mozilla.org/SVGOpenTypeFonts#Prototype_Implementation

A short answer to this assertion : "Also, they have two glyph definitions per actual glyph -- one static, one animated. I think we should just allow the author to specify a specific frame to use when not animating; I can't really think of any use case where the static version wouldn't be a frame of the animated one."

There is some use cases where an author could want a static version of an animated glyph which is not a frame of the animated on. For example, if he applied a motion blur (i.e. through a filter) to its animated glyph all along the animation, it would be better to have a non-blury glyph when not animated. It's like having a poster frame with video, it is useful.
I think the non-animated version of the glyphs should not just be a specific point in the animation timeline, but the glyph rendered without any animations applied.  That way, you can have arbitrarily different graphics when not animated and you also get something useful for engines that happen not to support SMIL yet.
Ah, writing that I was under the impression that the first frame of an animated SVG drawing == the SVG drawing without animations applied. Edited.
(In reply to Edwin Flores from comment #92)
> Boom. https://wiki.mozilla.org/SVGOpenTypeFonts#Prototype_Implementation

Great, thanks!  One quick comment, I think you need to say "*declarative* animation wil be allowed" since scriptable animation isn't allowed.
(In reply to Cameron McCormack (:heycam) from comment #94)
> I think the non-animated version of the glyphs should not just be a specific
> point in the animation timeline, but the glyph rendered without any
> animations applied.
Mmmh... I'm not sure it's good enough. Declarative animation can be quite messy and having a different alternative glyph for its non-animated version could be useful. It's easier for author to recognize which is what during the editing process. And it's allow them to build a nicer and simpler variant. For example, what if an author want to draw deformed shapes for the animated version and having a diferent shape for the static version ? IMO it's the same as having real italic (drawn the right way) or false italic (a skew version of the regular glyph). In the first case you let the author control precisely what he want, in the second, you let the machine guess. Fonts authors can be quite picky on such a matter :)
You can have alternative glyphs without adding extra features by writing something like this:

<g glyphid="...">
  <g class="animated" style="opacity:0">
    <set begin="0" attributeName="opacity" to="1"/>
    ...
  </g>
  <g class="static">
    <set begin="0" attributeName="opacity" to="0"/>
    ...
  </g>
</g>
Specifically:
 * Clipping to TrueType bounding box
 * Making fill="none" and stroke="none" do what they should
 * Fixed a problem with some glyphs not drawing initially
Attachment #598985 - Flags: review?(roc)
Just updated to account for clipping change in Patch 3. Carry over r=roc from comment #85
Attachment #597199 - Attachment is obsolete: true
Attachment #598989 - Flags: review+
Just updated to account for clipping change in Patch 3. Carry over r=roc from Attachment #597201 [details] [diff]
Attachment #597201 - Attachment is obsolete: true
Attachment #598992 - Flags: review+
Comment on attachment 598985 [details] [diff] [review]
Patch 3 - Assorted small fixes

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

You need a better checking comment, similar to what you wrote in the bug. (But you should elucidate "do what they should".)

Split this patch up so that the independent fixes are separate patches.

::: gfx/thebes/gfxFont.cpp
@@ +1805,5 @@
> +    gfxContextAutoSaveRestore contextRestore(aContext);
> +
> +    gfxFont::Metrics metrics = GetMetrics();
> +    aContext->Clip(gfxRect(aPoint.x, aPoint.y - metrics.maxAscent,
> +                           advance, metrics.maxHeight));

No, you can't clip to the font-box. You need to use the glyph overflow area. Call GetOrCreateGlyphExtents and then GetTightGlyphExtentsAppUnits.

We could try another approach where we get the SVG bounding box for the glyph element and add that to the overflow area. That would be a bit more author-friendly, but it would be bad for animated glyphs because we'd have to do expensive stuff to recompute the bboxes and update text (and possibly text layout) for every frame.

::: layout/svg/base/src/nsSVGUtils.h
@@ +185,5 @@
>    void SetIsFont(bool aIsFont) { mIsFont = aIsFont; }
>    bool IsFont() { return mIsFont; }
>  
> +  bool ShouldFill() {
> +    return !IsFont() || !!GetOuterObjectFillPattern();

This isn't really right. Seems like if you have no outer object fill pattern, we won't fill anything ever. But someone might want to render "stroke" by filling stuff.

Instead of doing this, I think we should go with the approach we talked about before, where objectFillOpacity is set to 0 if the object fill pattern is "none".
Whiteboard: [secr:cdiehl]
Attachment #598992 - Attachment is obsolete: true
Attachment #597201 - Attachment is obsolete: false
Woo, one line patch.
Attachment #598985 - Attachment is obsolete: true
Attachment #598985 - Flags: review?(roc)
Attachment #599819 - Flags: review?(jwatt)
Minor change to basic reftests after patch 3. Carry over r=roc from comment #87
Attachment #598989 - Attachment is obsolete: true
Attachment #599821 - Flags: review+
Comment on attachment 599820 [details] [diff] [review]
Patch 4 - Include SVG glyph when calculating glyph extents

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

::: content/base/public/nsContentUtils.h
@@ +1953,5 @@
>                              gfxFont::DrawMode aDrawMode,
>                              gfxPattern *aFillPattern,
>                              gfxPattern *aStrokePattern);
>  
> +  static bool GetSVGGlyphExtents(Element *aElement, const gfxMatrix &aSVGToAppSpace,

For reference parameters, I think our usual style would be "const gfxMatrix& aSVGToAppSpace" - i.e., attach the & to the type, not the parameter. (Here and in the .cpp file.)

::: gfx/thebes/gfxFont.cpp
@@ +235,5 @@
>  bool
> +gfxFontEntry::GetSVGGlyphExtents(gfxContext *aContext, PRUint32 aGlyphId,
> +                                 gfxRect *aResult)
> +{
> +    NS_ASSERTION(mSVGInitialized, "SVG data has not yet been loaded. TryGetSVGData() first.");

Given that our NS_ASSERTIONs are normally non-fatal, I think cases like this (where failure of the assertion really means there's been a basic programmer error, not an unexpected runtime condition such as resource exhaustion) should use NS_ABORT_IF_FALSE so that there's no way we can continue executing if our code is broken in this way.

@@ +1813,5 @@
>      if (!GetFontEntry()->HasSVGGlyph(aGlyphId)) {
>          return false;
>      }
>  
> +    const float devUnitsPerSVGUnit = GetStyle()->size / gfxSVGGlyphs::SVG_UNITS_PER_EM;

It'd probably make sense to declare this as gfxFloat, as that's the type gfxContext::Scale expects.

@@ +2376,5 @@
>          ++gGlyphExtentsSetupFallBackToTight;
>      }
>  #endif
>  
>      double d2a = appUnitsPerDevUnit;

While we're here, let's make this a gfxFloat, too, for consistency (even though that's currently just a typedef for double).

@@ +2383,5 @@
> +
> +    gfxRect svgBounds;
> +    if (mFontEntry->TryGetSVGData() &&
> +        mFontEntry->HasSVGGlyph(aGlyphID) &&
> +        GetFontEntry()->GetSVGGlyphExtents(aContext, aGlyphID, &svgBounds)) {

It seems confusing to mix the use of mFontEntry and GetFontEntry() here - please go with one or the other!

::: gfx/thebes/gfxSVGGlyphs.cpp
@@ +195,5 @@
>                                           aFillPattern, aStrokePattern);
>  }
>  
>  bool
> +gfxSVGGlyphs::GetGlyphExtents(PRUint32 aGlyphId, const gfxMatrix &aSVGToAppSpace,

Move the & to the end of "gfxMatrix", here and in the .h file. (I know we're a bit inconsistent about the placement of "*", but I think "&" is virtually always appended to the type.)

@@ +202,5 @@
> +    Element *glyph = mGlyphIdMap.Get(aGlyphId);
> +    NS_ASSERTION(glyph, "No glyph element. Should check with HasSVGGlyph() first!");
> +
> +    gfxRect svgResult;
> +    bool success = nsContentUtils::GetSVGGlyphExtents(glyph, aSVGToAppSpace, &svgResult);

You can just pass aResult directly in to GetSVGGlyphExtents here, can't you?

::: gfx/thebes/gfxSVGGlyphs.h
@@ +59,5 @@
>      typedef mozilla::dom::Element Element;
>      typedef gfxFont::DrawMode DrawMode;
>  
>  public:
> +    static const float SVG_UNITS_PER_EM = 1000.0f;

Sorry, but this isn't legal C++ (although gcc accepts it, I believe it'll warn if you use the --pedantic option). Only integral and enum constants can be initialized within the class declaration; for a float, it needs to be initialized using a separate definition.
Comment on attachment 597278 [details] [diff] [review]
Patch 2-1-1 rev 5 Add -moz-is-glyph media feature for SVG Glyphs

># HG changeset patch
># Parent a52f73d2bc01085bcaa13256fe427a1b32d24b7f

You should configure hg so that it puts your username in patches in your patch queue.  See https://developer.mozilla.org/en/Installing_Mercurial#Configuring_mq .  (I'm assuming you're using mq.)

>Bug 719286 - Add support for -moz-is-glyph media feature for internal use with SVG glyphs r=dbaron

Isn't the goal to standardize the whole set of features?  Why is it internal use?

>+  bool IsFont() const {

Could you search-replace all occurrences of IsFont that you're adding and make them IsGlyph?  (That includes mIsFont, SetIsFont.)

r=dbaron with those fixed, though if we want this to be a standard feature we're going to need a written description of it at some point (sooner rather than later)
Attachment #597278 - Flags: review?(dbaron) → review+
Comment on attachment 597664 [details] [diff] [review]
Patch 2-1-2 rev 5 Add support for objectfill and objectstroke values to CSS parser

># HG changeset patch
># Parent ed4689dd1d57d31177bb7adc0e37a6a44e97881b
>Bug 719286 - Add support for objectfill and objectstroke values to CSS parser r=dbaron

Again, you should have a username in the patch header.

>diff --git a/layout/style/nsCSSKeywordList.h b/layout/style/nsCSSKeywordList.h
>--- a/layout/style/nsCSSKeywordList.h
>+++ b/layout/style/nsCSSKeywordList.h
>@@ -354,16 +354,18 @@ CSS_KEY(no-open-quote, no_open_quote)
> CSS_KEY(no-repeat, no_repeat)
> CSS_KEY(none, none)
> CSS_KEY(normal, normal)
> CSS_KEY(not-allowed, not_allowed)
> CSS_KEY(nowrap, nowrap)
> CSS_KEY(ns-resize, ns_resize)
> CSS_KEY(nw-resize, nw_resize)
> CSS_KEY(nwse-resize, nwse_resize)
>+CSS_KEY(objectfill, objectfill)
>+CSS_KEY(objectstroke, objectstroke)

These should be -moz- prefixed / _moz_ prefixed.  (The latter will require changing the eCSSKeyword_* values through the rest of the patch.)

>diff --git a/layout/style/nsCSSParser.cpp b/layout/style/nsCSSParser.cpp
>--- a/layout/style/nsCSSParser.cpp
>+++ b/layout/style/nsCSSParser.cpp
>@@ -8975,18 +8975,20 @@ CSSParserImpl::SetDefaultNamespaceOnSele
>     aSelector.SetNameSpace(kNameSpaceID_Unknown); // wildcard
>   }
> }
> 
> bool
> CSSParserImpl::ParsePaint(nsCSSProperty aPropID)
> {
>   nsCSSValue x, y;
>-  if (!ParseVariant(x, VARIANT_HC | VARIANT_NONE | VARIANT_URL, nsnull))
>-    return false;
>+  if (!ParseVariant(x, VARIANT_HCK | VARIANT_NONE | VARIANT_URL,
>+                    nsCSSProps::kObjectPatternKTable)) {
>+    return false;
>+  }
>   if (x.GetUnit() == eCSSUnit_URL) {
>     if (!ParseVariant(y, VARIANT_COLOR | VARIANT_NONE, nsnull))

So here you don't parse fallback colors when you have one of your new values, but... (see below, skipping a comment)

>diff --git a/layout/style/nsRuleNode.cpp b/layout/style/nsRuleNode.cpp
>+  } else if (SetSVGObjectPaint(aValue, aResult)) {
>+    aCanStoreInRuleTree = false;

Why do you set this?  I don't think you should.  It should be set only when the style data that you're computing depends on something other than the CSS rules used to compute it.

>diff --git a/layout/svg/base/src/nsSVGUtils.cpp b/layout/svg/base/src/nsSVGUtils.cpp
>--- a/layout/svg/base/src/nsSVGUtils.cpp
>+++ b/layout/svg/base/src/nsSVGUtils.cpp
>@@ -1588,17 +1588,19 @@ nsSVGUtils::RootSVGElementHasViewbox(con
> 
> /* static */ void
> nsSVGUtils::GetFallbackOrPaintColor(gfxContext *aContext, nsStyleContext *aStyleContext,
>                                     nsStyleSVGPaint nsStyleSVG::*aFillOrStroke,
>                                     float *aOpacity, nscolor *color)
> {
>   const nsStyleSVGPaint &paint = aStyleContext->GetStyleSVG()->*aFillOrStroke;
>   nsStyleContext *styleIfVisited = aStyleContext->GetStyleIfVisited();
>-  bool isServer = paint.mType == eStyleSVGPaintType_Server;
>+  bool isServer = paint.mType == eStyleSVGPaintType_Server ||
>+                  paint.mType == eStyleSVGPaintType_ObjectFill ||
>+                  paint.mType == eStyleSVGPaintType_ObjectStroke;
>   *color = isServer ? paint.mFallbackColor : paint.mPaint.mColor;

... yet here (see above, skipping one comment), you assume that the fallback color is meaningful.

review- since I'd like to find out about the fallback color thing.  I think it actually might be desirable to have the fallback color parsed (so that something intended as a glyph can be viewed in other ways), though I'd like to hear the opinions of others.
Attachment #597664 - Flags: review?(dbaron) → review-
Comment on attachment 599819 [details] [diff] [review]
Patch 3 - Fixed bug where SVG glyphs outside of defs tag weren't being drawn

Edwin, I'm not familiar with this code, so you should probably get someone who is to review it.
Attachment #599819 - Flags: review?(jwatt) → review?(roc)
(In reply to David Baron [:dbaron] from comment #107)
> Comment on attachment 597278 [details] [diff] [review]
> Patch 2-1-1 rev 5 Add -moz-is-glyph media feature for SVG Glyphs
> 
> >Bug 719286 - Add support for -moz-is-glyph media feature for internal use with SVG glyphs r=dbaron
> 
> Isn't the goal to standardize the whole set of features?  Why is it internal
> use?

I don't really think this part is worth standardising -- the only place I can see it being useful is in the user agent itself. There isn't much value in requiring that they implement it if they aren't going to use it.

I suppose it *might* come in handy for font designers in that they would be able to edit the document directly in a different style from what would be used when rendered as a glyph (e.g. edit as pink, display as objectfill), but editing it as a whole would be a mess. They would likely work with each glyph individually and then merge it into one big document at the end.

Might be missing a use case here, though.

(In reply to David Baron [:dbaron] from comment #108)
> Comment on attachment 597664 [details] [diff] [review]
> Patch 2-1-2 rev 5 Add support for objectfill and objectstroke values to CSS
> parser
> 
> review- since I'd like to find out about the fallback color thing.  I think
> it actually might be desirable to have the fallback color parsed (so that
> something intended as a glyph can be viewed in other ways), though I'd like
> to hear the opinions of others.

Agreed.
Blargh, bitrot. Carry over r=roc from comment #81.
Attachment #596930 - Attachment is obsolete: true
Attachment #604831 - Flags: review+
Carry over r=dbaron from comment #107
Attachment #597278 - Attachment is obsolete: true
Attachment #604832 - Flags: review+
(In reply to Edwin Flores from comment #110)
> (In reply to David Baron [:dbaron] from comment #107)
> > Comment on attachment 597278 [details] [diff] [review]
> > Patch 2-1-1 rev 5 Add -moz-is-glyph media feature for SVG Glyphs
> > 
> > >Bug 719286 - Add support for -moz-is-glyph media feature for internal use with SVG glyphs r=dbaron
> > 
> > Isn't the goal to standardize the whole set of features?  Why is it internal
> > use?
> 
> I don't really think this part is worth standardising -- the only place I
> can see it being useful is in the user agent itself. There isn't much value
> in requiring that they implement it if they aren't going to use it.
> 
> I suppose it *might* come in handy for font designers in that they would be
> able to edit the document directly in a different style from what would be
> used when rendered as a glyph (e.g. edit as pink, display as objectfill),
> but editing it as a whole would be a mess. They would likely work with each
> glyph individually and then merge it into one big document at the end.

ah, ok.  Could you explain that in the commit message -- or, actually, even better, in a code comment?

> (In reply to David Baron [:dbaron] from comment #108)
> > Comment on attachment 597664 [details] [diff] [review]
> > Patch 2-1-2 rev 5 Add support for objectfill and objectstroke values to CSS
> > parser
> > 
> > review- since I'd like to find out about the fallback color thing.  I think
> > it actually might be desirable to have the fallback color parsed (so that
> > something intended as a glyph can be viewed in other ways), though I'd like
> > to hear the opinions of others.
> 
> Agreed.

roc/jwatt/heycam -- any opinions on this one?  (See comment 108 -- the question is whether these objectFill and objectStroke keywords should have fallback colors (like paint servers) or not.)
(In reply to Jonathan Kew (:jfkthame) from comment #106)
> Given that our NS_ASSERTIONs are normally non-fatal, I think cases like this
> (where failure of the assertion really means there's been a basic programmer
> error, not an unexpected runtime condition such as resource exhaustion)

To be clear, an "unexpected runtime condition such as resource exhaustion" should not trigger assertions, only warnings. Assertions are only for code bugs.

I don't really care if this one is fatal.
Yes, we should support fallback colors.
New patch that doesn't break everything.
Attachment #604833 - Attachment is obsolete: true
Attachment #604833 - Flags: review?(dbaron)
Attachment #605515 - Flags: review?(dbaron)
Updated tests to use -moz prefixed attributes. Carry over r=roc from comment #87
Attachment #599821 - Attachment is obsolete: true
Attachment #605519 - Flags: review+
Grrr, epic bitrot. Pretty much had to redo the whole thing. At least it's now implemented in a way that doing object{Fill,Stroke}Opacity should be a lot easier.
Attachment #596899 - Attachment is obsolete: true
Attachment #605539 - Flags: review?(roc)
Attachment #599820 - Attachment is obsolete: true
Attachment #599820 - Flags: review?(jfkthame)
Attachment #605578 - Flags: review?(jfkthame)
Attachment #605578 - Flags: review?(jfkthame) → review+
Comment on attachment 605581 [details] [diff] [review]
Reftest Patch 3 - Test for SVG glyph clipping

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

r=me, assuming you verified that this does in fact fail without the glyph-extents patch, and then passes with it. (I didn't actually build and test it locally.)
Attachment #605581 - Flags: review?(jfkthame) → review+
Comment on attachment 605539 [details] [diff] [review]
Patch 2-2 rev 6 Let OpenType SVG glyphs inherit object fill and stroke paints

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

::: gfx/src/nsFontMetrics.cpp
@@ +339,5 @@
>          pt.x += textRun->GetAdvanceWidth(0, aLength, &provider);
>      }
> +
> +    nsRefPtr<gfxPattern> pattern = aContext->ThebesContext()->GetPattern();
> +    nsAutoPtr<gfxTextObjectPaint> objectPaint(new SimpleTextObjectPaint(pattern, nsnull));

Can't we just stack-allocate a gfxTextObjectPaint? I don't see a need to heap-allocate.

::: gfx/thebes/gfxFont.h
@@ +3134,5 @@
> +    }
> +
> +    already_AddRefed<gfxPattern> GetStrokePattern(float aOpacity) {
> +        if (mStrokePattern) {
> +            mStrokePattern->SetMatrix(mStrokeMatrix);

Remind me, why are the matrices in these patterns being changed? Surely a SetSource call on a pattern doesn't change its matrix?

::: xpcom/glue/nsHashKeys.h
@@ +218,1 @@
>    const PRUint64 mValue;

How come a float has a PRUint64 mValue?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #124)
> ::: gfx/thebes/gfxFont.h
> @@ +3134,5 @@
> > +    }
> > +
> > +    already_AddRefed<gfxPattern> GetStrokePattern(float aOpacity) {
> > +        if (mStrokePattern) {
> > +            mStrokePattern->SetMatrix(mStrokeMatrix);
> 
> Remind me, why are the matrices in these patterns being changed? Surely a
> SetSource call on a pattern doesn't change its matrix?

This one's ugly. Because we mess with the pattern matrix later, and because it's such a pain to copy patterns, it's using the same pattern over and over again so we restore it every time.

> ::: xpcom/glue/nsHashKeys.h
> @@ +218,1 @@
> >    const PRUint64 mValue;
> 
> How come a float has a PRUint64 mValue?

Well. That's embarrassing.
Rebase. Carry over r=roc from comment #81.
Attachment #604831 - Attachment is obsolete: true
Attachment #606056 - Flags: review+
Rebase.
Attachment #605596 - Attachment is obsolete: true
Attachment #605596 - Flags: review?(dbaron)
Attachment #606058 - Flags: review?(dbaron)
Comment on attachment 606058 [details] [diff] [review]
Patch 2-1-1 rev 7 Add -moz-is-glyph media feature for SVG Glyphs

r=dbaron with the comment from comment 114.
Attachment #606058 - Flags: review?(dbaron) → review+
Comment on attachment 605515 [details] [diff] [review]
Patch 2-1-2 rev 7 Add support for objectfill and objectstroke values to CSS parser

In CSSParserImpl::ParsePaint, could you declare:
  bool canHaveFallback =
    x.GetUnit() == eCSSUnit_URL || x.GetUnit() == eCSSUnit_Enumerated;
and then use it in the conditions (once as if (canHaveFallback) and then
once a few lines below with a !).


The comments in your changes to nsCSSPropList.h don't make sense; 
there's no function called ParseSVGPaint, and SetSVGPaint doesn't do
parsing.  But those changes are fine without comments, so just remove
the comments.

In nsRuleNode.cpp, you've now duplicated a good bit of code for 
fallback.  Could you merge SetSVGObjectPaint back into SetSVGPaint so 
that you can share the code for fallbacks.

r=dbaron with those things fixed
Attachment #605515 - Flags: review?(dbaron) → review+
Comment on attachment 606059 [details] [diff] [review]
Patch 2-2 rev 7 Let OpenType SVG glyphs inherit object fill and stroke paints

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

I think this patch can be broken up a bit.

::: gfx/src/nsFontMetrics.cpp
@@ +340,5 @@
>          pt.x += textRun->GetAdvanceWidth(0, aLength, &provider);
>      }
> +
> +    nsRefPtr<gfxPattern> pattern = aContext->ThebesContext()->GetPattern();
> +    SimpleTextObjectPaint objectPaint(pattern, nsnull);

This creates a new wrapper object that we don't really need.

Can we say that if we pass null for the gfxTextObjectPaint parameter, the context's current source is used for the fill pattern and there is no stroke? Then we can just pass null here.

::: gfx/thebes/gfxSVGGlyphs.h
@@ +105,5 @@
> +    gfxTextObjectPaint() { }
> +
> +public:
> +    virtual already_AddRefed<gfxPattern> GetFillPattern(float aOpacity = 1.0f)=0;
> +    virtual already_AddRefed<gfxPattern> GetStrokePattern(float aOpacity = 1.0f)=0;

spaces around =

You need to document what the aOpacity parameters mean.

You need to document that these can return null and what that means.

@@ +126,5 @@
> +        nsRefPtr<gfxPattern> fillPattern = mFillPattern;
> +        return fillPattern.forget();
> +    }
> +
> +    already_AddRefed<gfxPattern> GetStrokePattern (float aOpacity) {

no space before (

::: layout/svg/base/src/nsSVGGlyphFrame.cpp
@@ +944,5 @@
> +  if (style->mFillRule == NS_STYLE_FILL_RULE_EVENODD) {
> +    aContext->SetFillRule(gfxContext::FILL_RULE_EVEN_ODD);
> +  } else {
> +    aContext->SetFillRule(gfxContext::FILL_RULE_WINDING);
> +  }

Do glyph frames really need to honor the fill rule? Seems to me it should be ignored for glyphs.

@@ +1006,5 @@
> +    pattern->SetMatrix(aContext->CurrentMatrix().Multiply(pattern->GetMatrix()));
> +    aContext->SetPattern(pattern);
> +  } else {
> +    return false;
> +  }

if (!pattern)
  return false;

.. then do the rest.

::: layout/svg/base/src/nsSVGGlyphFrame.h
@@ +278,5 @@
> +
> +  // Slightly horrible callback for deferring application of opacity
> +  struct SVGTextObjectPaint : public gfxTextObjectPaint {
> +    already_AddRefed<gfxPattern> GetFillPattern(float opacity);
> +    already_AddRefed<gfxPattern> GetStrokePattern (float opacity);

No space before (

::: xpcom/glue/nsHashKeys.h
@@ +267,5 @@
> +  KeyType GetKey() const { return mValue; }
> +  bool KeyEquals(KeyTypePointer aKey) const { return *aKey == mValue; }
> +
> +  static KeyTypePointer KeyToPointer(KeyType aKey) { return &aKey; }
> +  static PLDHashNumber HashKey(KeyTypePointer aKey) { return PLDHashNumber(*aKey); }

This is not a good hash value for floats, since I think this just rounds. How about *reinterpret_cast<PRUint32*>(aKey) instead? But are there float values that compare equal but have different bit patterns?
Carry over r=dbaron from comment 132
Attachment #606058 - Attachment is obsolete: true
Attachment #607030 - Flags: review+
> ::: gfx/src/nsFontMetrics.cpp
> @@ +340,5 @@
> >          pt.x += textRun->GetAdvanceWidth(0, aLength, &provider);
> >      }
> > +
> > +    nsRefPtr<gfxPattern> pattern = aContext->ThebesContext()->GetPattern();
> > +    SimpleTextObjectPaint objectPaint(pattern, nsnull);
> 
> This creates a new wrapper object that we don't really need.
> 
> Can we say that if we pass null for the gfxTextObjectPaint parameter, the
> context's current source is used for the fill pattern and there is no
> stroke? Then we can just pass null here.

It's passed all the way through to the SVG glyphs part where the current source won't necessarily be the fill pattern, so it's going to have to be created at some point anyway. May as well just leave it?
It's only going to have to be created when we actually render with SVG glyphs, which will be almost never.
Can you break the patch up a bit? Seems like one patch could add support for passing around gfxTextObjectPaint, then another patch actually consults it when drawing, and another patch sets up the gfxTextObjectPaint to pass in when drawing glyphs.
Comment on attachment 607476 [details] [diff] [review]
Patch 2-2-1 rev 9 Add new gfxTextObjectPaint paint wrapper for SVG glyphs to inherit outer text object paints

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

Is there a reason not to make the gfxTextObjectPaint* user-data on nsRenderingContext?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #144)
> Is there a reason not to make the gfxTextObjectPaint* user-data on
> nsRenderingContext?

...because I didn't know that was a thing. D'oh.
Comment on attachment 607813 [details] [diff] [review]
Patch 2-2-1 rev 10 Add new gfxTextObjectPaint paint wrapper for SVG glyphs to inherit outer text object paints

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

The rest looks good.

::: gfx/thebes/gfxFont.cpp
@@ +1320,5 @@
>      }
>  
> +    void Flush(cairo_t *aCR, gfxFont::DrawMode aDrawMode, bool aReverse,
> +               gfxTextObjectPaint *aObjectPaint,
> +               gfxMatrix aGlobalMatrix, bool aFinish = false) {

const gfxMatrix&

But why not leave Flush() the way it is, and keep passing in aStrokePattern? I don't know why you changed this.
Comment on attachment 607814 [details] [diff] [review]
Patch 2-2-2 rev 10 - Actually have text objects construct and pass in a gfxTextObjectPaint

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

::: gfx/thebes/gfxFont.cpp
@@ +1459,5 @@
> +        objectPaint = nsAutoPtr<gfxTextObjectPaint>(
> +            new SimpleTextObjectPaint(fillPattern, nsnull)
> +        );
> +        aObjectPaint = objectPaint;
> +    }

So when gfxFont::Draw draws SVG glyphs, we use the fill and stroke patterns from aObjectPaint, but when it draws normal glyphs, we only use the stroke pattern from aObjectPaint?

It seems to me that when aObjectPaint is non-null, we should always use its fill parameters for filling and its stroke parameters for stroking even when drawing normal glyphs.

::: layout/svg/base/src/nsSVGGlyphFrame.cpp
@@ +942,5 @@
> +  if (style->mFill.mType == eStyleSVGPaintType_None) {
> +    return false;
> +  }
> +
> +  float opacity = MaybeOptimizeOpacity(style->mFillOpacity);

This optimization isn't correct for text with SVG glyphs, is it? Let's just take it out now.

@@ +1039,5 @@
> +  case eStyleSVGPaintType_None:
> +    pattern = new gfxPattern(gfxRGBA(0.0f, 0.0f, 0.0f, 0.0f));
> +    break;
> +  case eStyleSVGPaintType_Color:
> +    pattern = new gfxPattern(mPaintDefinition.mColor);

Shouldn't you be using aOpacity here?

::: layout/svg/base/src/nsSVGGlyphFrame.h
@@ +341,5 @@
> +                             float aOpacity,
> +                             gfxTextObjectPaint *aOuterObjectPaint,
> +                             SVGTextObjectPaint::Paint& aTargetPaint,
> +                             nsStyleSVGPaint nsStyleSVG::*aFillOrStroke,
> +                             const FramePropertyDescriptor *aProperty);

These new methods all need to be documented.
Comment on attachment 608208 [details] [diff] [review]
Patch 2-2-1 rev 11 Add new gfxTextObjectPaint paint wrapper for SVG glyphs to inherit outer text object paints

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

::: gfx/thebes/gfxFont.h
@@ +2418,5 @@
>      void Draw(gfxContext *aContext, gfxPoint aPt,
>                gfxFont::DrawMode aDrawMode,
>                PRUint32 aStart, PRUint32 aLength,
>                PropertyProvider *aProvider,
> +              gfxFloat *aAdvanceWidth, gfxTextObjectPaint *aObjectPaint);

Please add a comment to indicate that if aObjectPaint is null, then aDrawMode must not include GLYPH_STROKE and aContext's current source will be used as the fill pattern, and that if aObjectPaint is non-null then its fill pattern must be non-null if aDrawMode includes GLYPH_FILL and its stroke pattern must be non-null if aDrawMode includes GLYPH_STROKE.
Attachment #608208 - Flags: review?(roc) → review+
Comment on attachment 608209 [details] [diff] [review]
Patch 2-2-2 rev 11 - Actually have text objects construct and pass in a gfxTextObjectPaint

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

::: layout/generic/nsTextFrameThebes.cpp
@@ +5576,2 @@
>    mTextRun->Draw(aCtx, aTextBaselinePt, gfxFont::GLYPH_FILL, aOffset, aLength,
> +                 &aProvider, &aAdvanceWidth, &objectPaint);

Why do we have to do this? We should be able to pass null here and draw with the current pattern.

@@ +5586,5 @@
>        gfxFloat hyphenBaselineX = aTextBaselinePt.x + mTextRun->GetDirection() * aAdvanceWidth -
>          (mTextRun->IsRightToLeft() ? hyphenTextRun->GetAdvanceWidth(0, hyphenTextRun->GetLength(), nsnull) : 0);
>        hyphenTextRun->Draw(aCtx, gfxPoint(hyphenBaselineX, aTextBaselinePt.y),
>                            gfxFont::GLYPH_FILL, 0, hyphenTextRun->GetLength(),
> +                          nsnull, nsnull, &objectPaint);

Same here.

::: layout/svg/base/src/nsSVGGlyphFrame.h
@@ +349,5 @@
> +                        gfxTextObjectPaint *aObjectPaint);
> +
> +  /**
> +   * Stores in |aTargetPaint| information on how to reconstruct the current
> +   * fill or stroke pattern.

We need a bit more information in this comment. Explain what 'aProperty' is, at least.
Comment on attachment 608556 [details] [diff] [review]
Patch 2-2-2 rev 12 - Actually have text objects construct and pass in a gfxTextObjectPaint

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

When you land this, split out the nsHashKeys.h change into its own patch.

::: layout/generic/nsTextFrameThebes.cpp
@@ +112,5 @@
>  
>  #include "gfxFont.h"
>  #include "gfxContext.h"
>  #include "gfxImageSurface.h"
> +#include "gfxSVGGlyphs.h"

Remove this patch hunk, you no longer need it.
Attachment #608556 - Flags: review?(roc) → review+
Rebase; carry over r=jfkthame
Attachment #605578 - Attachment is obsolete: true
Attachment #608571 - Flags: review+
Was missing commit message. Carry over r=roc.
Attachment #597201 - Attachment is obsolete: true
Attachment #608576 - Flags: review+
Comment on attachment 608573 [details] [diff] [review]
Patch 5-2 rev 2 - Let SVG glyphs inherit fill-opacity and stroke-opacity through objectFillOpacity and objectStrokeOpacity

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

Something needs to set objectFillOpacity to 0 when gfxFont::Draw has no gfxTextObjectPaint and is not filling, and set objectStrokeOpacity to 0 when gfxFont::Draw has no gfxTextObjectPaint and is not stroking. I don't see anything for that.

::: layout/svg/base/src/nsSVGGlyphFrame.cpp
@@ +913,5 @@
>                                    gfxTextObjectPaint *aOuterObjectPaint,
>                                    SVGTextObjectPaint *aThisObjectPaint)
>  {
> +  if (!HasStroke()) {
> +    aThisObjectPaint->SetStrokeOpacity(0.0f);

Seems to me it would be most logical to set objectStrokeOpacity to 0.0 only for "stroke:none" and not for zero stroke width.

@@ +924,5 @@
>    aContext->IdentityMatrix();
>  
>    SetupCairoStrokeHitGeometry(aContext);
> +  float opacity = GetOpacity(style->mObjectStrokeOpacity, style->mStrokeOpacity,
> +                             aOuterObjectPaint);

GetOpacity is doing MaybeOptimizeOpacity. We really don't want to be doing that for glyphs.

@@ +1015,5 @@
>  
> +  gfxRGBA color;
> +  if (pattern->GetSolidColor(color) && color == zero) {
> +    aOpacity = 0.0f;
> +  }

Do we really need to do this?

fill:none sets objectFillOpacity to 0 in the glyphs. That's good. I don't think we need rgba(0,0,0,0) to do it too.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #165)
> @@ +1015,5 @@
> >  
> > +  gfxRGBA color;
> > +  if (pattern->GetSolidColor(color) && color == zero) {
> > +    aOpacity = 0.0f;
> > +  }
> 
> Do we really need to do this?
> 
> fill:none sets objectFillOpacity to 0 in the glyphs. That's good. I don't
> think we need rgba(0,0,0,0) to do it too.

I think so. It works without if it's just fill:none---this path is for when we have fill:objectFill inheriting fill:none.

...you know, for when the author puts an SVGOT font inside another one. =\
Comment on attachment 608572 [details] [diff] [review]
Patch 5-1 rev 2 - Add support for objectFillOpacity and objectStrokeOpacity values to CSS parser

>diff --git a/layout/style/nsRuleNode.cpp b/layout/style/nsRuleNode.cpp
> // flags for SetFactor
> #define SETFCT_POSITIVE 0x01        // assert value is >= 0.0f
> #define SETFCT_OPACITY  0x02        // clamp value to [0.0f .. 1.0f]
> #define SETFCT_NONE     0x04        // allow _None (uses aInitialValue).
>+#define SETFCT_OBJECT   0x08        // allow object{Fill,Stroke}Opacity

This is unused.  Remove it (and the one place you set it).

>+static nsStyleSVGOpacityType
>+GetSVGObjectOpacity(const nsCSSValue& aValue)
>+{
>+  if (aValue.GetUnit() != eCSSUnit_Enumerated) {
>+    return eStyleSVGOpacityType_Normal;
>+  }
>+
>+  switch (aValue.GetIntValue()) {
>+  case NS_STYLE_OBJECT_FILL_OPACITY:
>+    return eStyleSVGOpacityType_ObjectFillOpacity;
>+    break;

Remove the break; the return is enough.

>+  case NS_STYLE_OBJECT_STROKE_OPACITY:
>+    return eStyleSVGOpacityType_ObjectStrokeOpacity;
>+    break;

Same here.

>+  default:
>+    NS_NOTREACHED("SetSVGObjectOpacity: Unknown keyword");
>+  }
>+  return eStyleSVGOpacityType_Normal;

And move this return inside the default:.


This patch has the more serious bug that these values don't inherit correctly when explicit 'inherit' is used (and there's another value declared for than struct+element+calculation step).  This is the case since you depend on SetFactor to handle inheritance, and it only looks at one of the two variables representing the property.  You should fix this and add a test for it.

You also need to patch nsStyleSVG::CalcDifference.
Attachment #608572 - Flags: review?(dbaron) → review-
Comment on attachment 607032 [details] [diff] [review]
Patch 2-1-2 rev 8 Add support for objectfill and objectstroke values to CSS parser

Actually, I just noticed another bug here:  you need to patch nsStyleSVGPaint::operator== -- it's currently returning false sometimes when it should return true.  (It's relatively straightforward -- you should just swap the last two returns and change the condition to == eStyleSVGPaintType_Color.)
Attachment #607032 - Flags: review-
(In reply to David Baron [:dbaron] from comment #168)
> Comment on attachment 607032 [details] [diff] [review]
> Patch 2-1-2 rev 8 Add support for objectfill and objectstroke values to CSS
> parser
> 
> Actually, I just noticed another bug here:  you need to patch
> nsStyleSVGPaint::operator== -- it's currently returning false sometimes when
> it should return true.  (It's relatively straightforward -- you should just
> swap the last two returns and change the condition to ==
> eStyleSVGPaintType_Color.)

Note that this bug isn't easily testable, but I think it is a performance problem since it will cause extra repaints following style changes (depending on the results of uninitialized memory), including in cases unrelated to this patch.
Comment on attachment 609159 [details] [diff] [review]
Patch 2-1-2 rev 9 Add support for objectfill and objectstroke values to CSS parser

r=dbaron
Attachment #609159 - Flags: review?(dbaron) → review+
Added test for explicit inheritance of objectFillOpacity/objectStrokeOpacity.
Attachment #606425 - Attachment is obsolete: true
Attachment #609169 - Flags: review?(dbaron)
Comment on attachment 609170 [details] [diff] [review]
Patch 5-1 rev 3 - Add support for objectFillOpacity and objectStrokeOpacity values to CSS parser

>+static void
>+SetSVGOpacity(const nsCSSValue& aValue,
>+              float& aOpacityField, nsStyleSVGOpacityType& aOpacityTypeField,
>+              bool& aCanStoreInRuleTree,
>+              float aParentOpacity, nsStyleSVGOpacityType aParentOpacityType)
>+{
>+  aOpacityTypeField = eStyleSVGOpacityType_Normal;

This is wrong, and testably so (but the test requires a little setup); this code must not touch the style struct values corresponding to a value whose GetUnit() is eCSSUnit_Null.  (This is important because we sometimes build on top of partial results that were cached in the rule tree.  For example, if we have a bunch of style rules whose resulting computed style can be cached in the rule tree, and has been, and then we compute style for an element that matches those rules and an additional one, the specified values we have will only be the delta between the rules, and we might have eCSSUnit_Null for things that are specified in a rule in the cached struct that we're starting from.)

Say, something like:

  g.foo { fill-opacity: -moz-objectFillOpacity }
  g.foo#bar { color-interpolation: sRGB }

  <g class="foo" />
  <g class="foo" id="bar" />

You should put the SetFactor call inside if (aValue.GetUnit() != eCSSUnit_Null), and then move this into that check.

> const void*
> nsRuleNode::ComputeSVGData(void* aStartStruct,

>+  // fill-opacity: factor, inherit, initial, objectFillOpacity, objectStrokeOpacity
>+  nsStyleSVGOpacityType objectFillOpacity;

you need to initialize this to svg->mObjectFillOpacity, for the same reason as above

>+  nsStyleSVGOpacityType objectStrokeOpacity;

same here, but svg->mObjectStrokeOpacity

>diff --git a/layout/style/nsStyleStruct.cpp b/layout/style/nsStyleStruct.cpp
>@@ -997,17 +1003,20 @@ nsChangeHint nsStyleSVG::CalcDifference(
>        mColorInterpolation    != aOther.mColorInterpolation    ||
>        mColorInterpolationFilters != aOther.mColorInterpolationFilters ||
>        mFillRule              != aOther.mFillRule              ||
>        mImageRendering        != aOther.mImageRendering        ||
>        mShapeRendering        != aOther.mShapeRendering        ||
>        mStrokeDasharrayLength != aOther.mStrokeDasharrayLength ||
>        mStrokeLinecap         != aOther.mStrokeLinecap         ||
>        mStrokeLinejoin        != aOther.mStrokeLinejoin        ||
>-       mTextAnchor            != aOther.mTextAnchor) {
>+       mTextAnchor            != aOther.mTextAnchor            ||
>+
>+       mObjectFillOpacity     != aOther.mObjectFillOpacity     ||
>+       mObjectStrokeOpacity   != aOther.mObjectStrokeOpacity) {
>     NS_UpdateHint(hint, nsChangeHint_RepaintFrame);

Don't add the blank line here.  (Maybe not in the other spots either.)


r=dbaron with that
Attachment #609170 - Flags: review?(dbaron) → review+
Comment on attachment 609169 [details] [diff] [review]
Reftest Patch 4 rev 2 - Tests for objectFillOpacity/objectStrokeOpacity

How did you generate the woff file?

And could you also add tests for the issue in comment 175 (and confirm they fail with the current patch)?
Added test for comment 175.

As for how the WOFF file is generated, I started with the Liberation Serif font, then in FontForge:
 * Added a glyph with a Unicode Variation Selector so that could be tested
 * Changed some metrics so that the bounding boxes were grid-aligned to ease testing

This results in the font at [1]. I then create a new `SVG ' table in that font containing the svg-woff.svg file using the tool I've written at [2].

Finally, I use Jonathan Kew's sfnt2woff tool [3] to turn it into a WOFF.

[1] https://github.com/edf825/SVG-OpenType-Utils/blob/master/insertsvg/UVSLiberation.otf
[2] https://github.com/edf825/SVG-OpenType-Utils/blob/master/insertsvg/insertsvg.cpp
[3] http://people.mozilla.org/~jkew/woff/
Attachment #609169 - Attachment is obsolete: true
Attachment #609169 - Flags: review?(dbaron)
Attachment #610327 - Flags: review?(dbaron)
Rebase. Carry over r=roc from comment 81.
Attachment #606056 - Attachment is obsolete: true
Attachment #610335 - Flags: review+
(In reply to Edwin Flores from comment #179)
> Created attachment 610335 [details] [diff] [review]
> Patch 1-2 rev 8 Basic OpenType SVG functionality
> 
> Rebase. Carry over r=roc from comment 81.

Oops, not just rebase but also fixes a failing reftest testing RTL text.
Minor fix for failing reftest where pattern was not respecting CSS transforms. Carry over r=roc from comment 156.
Attachment #608568 - Attachment is obsolete: true
Attachment #610339 - Flags: review+
Small fix---just made -moz-objectFillOpacity and -moz-objectStrokeOpacity the default opacity values for SVG glyphs. Carry over r=roc.
Attachment #609171 - Attachment is obsolete: true
Attachment #610342 - Flags: review+
Actually does some sanitisation now for new SVG table format:
https://wiki.mozilla.org/SVGOpenTypeFonts#The_SVG_table
Attachment #594936 - Attachment is obsolete: true
Attachment #615242 - Flags: review?(jfkthame)
Comment on attachment 615241 [details] [diff] [review]
Reftest Patch 6 rev 1 - Update tests to use the new SVG table format

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

r+ if you add some comments explaining what ranges and documents are stored in the WOFF file.
Attachment #615241 - Flags: review?(roc) → review+
Comment on attachment 615237 [details] [diff] [review]
Patch 6-2 rev 1 - Use objectValue keyword in SVG glyphs

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

Mostly fine...

::: gfx/thebes/gfxSVGGlyphs.cpp
@@ +376,5 @@
> +{
> +    mStrokeWidth = aContext->CurrentLineWidth() / devUnitsPerSVGUnit;
> +    aContext->CurrentDash(mDashes, &mDashOffset);
> +    for (PRUint32 i = 0; i < mDashes.Length(); i++) {
> +        mDashes.Elements()[i] /= devUnitsPerSVGUnit;

mDashes[i]

::: gfx/thebes/gfxSVGGlyphs.h
@@ +126,5 @@
>  
> +    void InitStrokeGeometry(gfxContext *aContext,
> +                            float devUnitsPerSVGUnit);
> +
> +    FallibleTArray<gfxFloat>& GetStrokeDasharray() {

DashArray

@@ +130,5 @@
> +    FallibleTArray<gfxFloat>& GetStrokeDasharray() {
> +        return mDashes;
> +    }
> +
> +    gfxFloat GetStrokeDashoffset() {

DashOffset

::: layout/svg/base/src/nsSVGGeometryFrame.h
@@ +86,5 @@
>    // nsSVGGeometryFrame methods:
>    virtual gfxMatrix GetCanvasTM() = 0;
>    PRUint16 GetClipRule();
>  
> +  virtual float GetStrokeWidth(gfxTextObjectPaint *aObjectPaint = nsnull);

Why does this need to be virtual?

@@ +114,5 @@
>    /*
>     * Set up a cairo context for hit testing a stroked path
>     */
> +  void SetupCairoStrokeHitGeometry(gfxContext *aContext,
> +                                   gfxTextObjectPaint *aObjectPaint = nsnull);

Maybe you should pass in the rendering context and let these methods get the gfxTextObjectPaint only if it's needed. More efficient.
Comment on attachment 615242 [details] [diff] [review]
Patch 1-1 rev 4 - OTS support for SVG table

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

Clearing r? on this for now, pending clarification of how we want the indices to work; if we decide the glyph ranges are to be non-overlapping, then we should also require them to be sorted (to allow binary search), and the sanitizer should check this.

On a general level, though, I think the SVG table sanitizer shouldn't do OTS_FAILURE if it detects an error; it should instead issue an OTS_WARNING, do DROP_THIS_TABLE, and return true. (Compare the gsub/gpos/gdef sanitizers, which only do a hard OTS_FAILURE on a grossly malformed table, but handle most failures by doing DROP_THIS_TABLE.)
Attachment #615242 - Flags: review?(jfkthame)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #192)
> @@ +114,5 @@
> >    /*
> >     * Set up a cairo context for hit testing a stroked path
> >     */
> > +  void SetupCairoStrokeHitGeometry(gfxContext *aContext,
> > +                                   gfxTextObjectPaint *aObjectPaint = nsnull);
> 
> Maybe you should pass in the rendering context and let these methods get the
> gfxTextObjectPaint only if it's needed. More efficient.

Innocently tried this only to find I had stepped into a world of hate and build failure due to everything that uses these methods. Will try to address in a cleanup patch.
Attachment #615242 - Attachment is obsolete: true
Attachment #616472 - Flags: review?(jfkthame)
Attachment #615240 - Attachment is obsolete: true
Attachment #615240 - Flags: review?(roc)
Attachment #616475 - Flags: review?(roc)
Carry over r=roc from comment #192
Attachment #615237 - Attachment is obsolete: true
Attachment #615237 - Flags: review?(roc)
Attachment #616476 - Flags: review+
Comment on attachment 616475 [details] [diff] [review]
Patch 7 rev 2 - Support new SVG table format

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

::: gfx/thebes/gfxSVGGlyphs.cpp
@@ +69,5 @@
> +                              (reinterpret_cast<PRUint8*>(&(x))[1] << 16) | \
> +                              (reinterpret_cast<PRUint8*>(&(x))[2] << 8) | \
> +                               reinterpret_cast<PRUint8*>(&(x))[3])
> +#define FROM_BIG_ENDIAN16(x) ((reinterpret_cast<PRUint8*>(&(x))[0] << 8) | \
> +                               reinterpret_cast<PRUint8*>(&(x))[1])

Instead of these, try using the AutoSwap classes from gfxUtils.h

@@ +86,2 @@
>  {
> +    mSVGData = aSVGTable;

You make a copy here. We should be able to just adopt the array already allocated, e.g. using SwapElements.

@@ +252,5 @@
> +
> +    if (!mGlyphIdMap.Get(aGlyphId, &elem)) {
> +        elem = nsnull;
> +        PRUint32 setIndex;
> +        if (FindGlyphSet(aGlyphId, &setIndex)) {

Why not have FindGlyphSet return a gfxSVGGlyphSet*?

@@ +255,5 @@
> +        PRUint32 setIndex;
> +        if (FindGlyphSet(aGlyphId, &setIndex)) {
> +
> +            nsAutoPtr<gfxSVGGlyphSet>& set = mGlyphSets[setIndex];
> +            if (!set) {

Just write this as gfxSVGGlyphSet* set =

@@ +257,5 @@
> +
> +            nsAutoPtr<gfxSVGGlyphSet>& set = mGlyphSets[setIndex];
> +            if (!set) {
> +                set = new gfxSVGGlyphSet(mSVGData.Elements() + mDocHeaders[setIndex].mOffset,
> +                                         mDocHeaders[setIndex].mLength,

Aren't you supposed to add this to mGlyphSets?

::: gfx/thebes/gfxSVGGlyphs.h
@@ +50,5 @@
>  #include "gfxFont.h"
>  #include "mozilla/gfx/UserData.h"
>  
>  
> +class gfxSVGGlyphSet

call this gfxSVGGlyphsDocument?

@@ +53,5 @@
>  
> +class gfxSVGGlyphSet
> +{
> +    typedef mozilla::dom::Element Element;
> +    typedef gfxFont::DrawMode DrawMode;

Needs documentation explaining what this class is for.

@@ +59,5 @@
> +public:
> +    gfxSVGGlyphSet(PRUint8 *aBuffer, PRUint32 aBufLen,
> +                   const FallibleTArray<PRUint8>& aCmapTable);
> +
> +    Element *GetGlyphElement(PRUint32 aGlyphId);

These need documentation too.

@@ +90,3 @@
>  /**
>   * Used by gfxFontEntry for OpenType fonts which contains SVG tables to parse
>   * the SVG document and store and render SVG glyphs

This comment could be better. At least say there's one of these per gfxFontEntry.

@@ +109,2 @@
>  
>      bool HasSVGGlyph(PRUint32 aGlyphId);

These could all use documentation, especially UnmangleHeaders
For new new (new) SVG table format
Attachment #616472 - Attachment is obsolete: true
Attachment #616472 - Flags: review?(jfkthame)
Attachment #617690 - Flags: review?(jfkthame)
Attachment #616475 - Attachment is obsolete: true
Attachment #616475 - Flags: review?(roc)
Attachment #617691 - Flags: review?(roc)
For new^3 SVG table format. Carry over r=roc
Attachment #616480 - Attachment is obsolete: true
Attachment #617693 - Flags: review+
Comment on attachment 617690 [details] [diff] [review]
Patch 1-1 rev 6 - OTS support for SVG table

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

Looking good... I'm suggesting a couple of small tweaks/tightening-up of the table format, which would be reflected in adjustments here, but nothing major.

::: gfx/ots/src/svg.cc
@@ +30,5 @@
> +  if (!table.ReadU16(&version_major) ||
> +      !table.ReadU16(&version_minor) ||
> +      !table.ReadU16(&index_length)) {
> +    NONFATAL_FAILURE("Couldn't read SVG table header");
> +  }

Hmm. This header is now 6 bytes, which means that the 32-bit values in the index will be misaligned. In keeping with most (though sadly not quite all) parts of the 'sfnt' format, I think we should try to avoid that - the simplest thing would be to make the version field a single 16-bit value (I don't think there's any real need for "major.minor" numbering - it's just a convention some tables have used, but others have simple integer versions, and we can do the same here).

@@ +60,5 @@
> +
> +    if (last_end_glyph && start_glyph <= last_end_glyph) {
> +      NONFATAL_FAILURE("SVG table index range is not sorted");
> +    }
> +

One more sanity-check: doc_offset must not be less than the end of the index (which is computable based on the known index_length and header size). Though that could be verified later, see next comment.

@@ +85,5 @@
> +  for (lociter_t iter = doc_locations.begin();
> +       iter != doc_locations.end(); ++iter) {
> +    if (iter->first < last_end) {
> +      NONFATAL_FAILURE("SVG table contains overlapping document range");
> +    }

I think we should tighten up the table spec to prohibit "dead space" between the SVG documents - there's no reason anyone should need it - and then this loop can easily check that they're strictly consecutive with no gaps. (Initialize last_end to the end of the index, and then check that iter->first == last_end for each document.)

@@ +91,5 @@
> +  }
> +
> +  if (max_address > length) {
> +    NONFATAL_FAILURE("Bad SVG document length");
> +  }

Similarly, check that max_address == length; no reason to permit junk after the SVG documents.
Whiteboard: [secr:cdiehl] → [sec-assigned:cdiehl]
Attachment #617690 - Attachment is obsolete: true
Attachment #617690 - Flags: review?(jfkthame)
Attachment #620216 - Flags: review?(jfkthame)
Attachment #617691 - Attachment is obsolete: true
Attachment #617691 - Flags: review?(roc)
Attachment #620217 - Flags: review?(roc)
For new^4 table format. Carry over r=roc
Attachment #617693 - Attachment is obsolete: true
Attachment #620219 - Flags: review+
Comment on attachment 620216 [details] [diff] [review]
Patch 1-1 rev 7 - OTS support for SVG table

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

Looks OK, except for last_end_glyph issues; r=me with that fixed.

::: gfx/ots/src/svg.cc
@@ +55,5 @@
> +    if (end_glyph < start_glyph) {
> +      NONFATAL_FAILURE("Bad SVG table index range");
> +    }
> +
> +    if (last_end_glyph && start_glyph <= last_end_glyph) {

AFAICS, last_end_glyph never gets initialized before the first iteration, nor updated by the loop. (Oops!)
Attachment #620216 - Flags: review?(jfkthame) → review+
Comment on attachment 620217 [details] [diff] [review]
Patch 7 rev 4 - Support new SVG table format

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

::: gfx/thebes/gfxSVGGlyphs.cpp
@@ +110,5 @@
> +
> +    do {
> +        mid = (high + low) / 2;
> +        if (aGlyphId <= mIndex[mid].mEndGlyph) {
> +            high = mid;

This seems wrong. 'high' is exclusive. Shouldn't you be comparing aGlyphId < mIndex[mid].mStartGlyph here?

@@ +123,5 @@
> +        mIndex[high].mEndGlyph >= aGlyphId) {
> +        index = high;
> +    } else if (mIndex[low].mStartGlyph <= aGlyphId &&
> +               mIndex[low].mEndGlyph >= aGlyphId) {
> +        index = low;

I suspect we don't need to check both low and high. If there is a match at this point, "low" must be it.
Comment on attachment 620217 [details] [diff] [review]
Patch 7 rev 4 - Support new SVG table format

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

::: gfx/thebes/gfxSVGGlyphs.cpp
@@ +126,5 @@
> +               mIndex[low].mEndGlyph >= aGlyphId) {
> +        index = low;
> +    } else {
> +        return nsnull;
> +    }

Couldn't we just use bsearch() to simplify all the above?
> ::: gfx/thebes/gfxSVGGlyphs.cpp
> @@ +126,5 @@
> > +               mIndex[low].mEndGlyph >= aGlyphId) {
> > +        index = low;
> > +    } else {
> > +        return nsnull;
> > +    }
> 
> Couldn't we just use bsearch() to simplify all the above?

Probably. I was under the impression that we were a bit STL-shy here for some reason...
(In reply to Edwin Flores from comment #210)
> > ::: gfx/thebes/gfxSVGGlyphs.cpp
> > @@ +126,5 @@
> > > +               mIndex[low].mEndGlyph >= aGlyphId) {
> > > +        index = low;
> > > +    } else {
> > > +        return nsnull;
> > > +    }
> > 
> > Couldn't we just use bsearch() to simplify all the above?
> 
> Probably. I was under the impression that we were a bit STL-shy here for
> some reason...

/me actually checks man page and sees that it's actually in stdlib. D'oh.