719286 - Implement embedded SVG glyphs in OpenType fonts

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, enhancement)

Comment 1

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 1 rev 1 - Basic OpenType SVG functionality

*Very* basic implementation. Examples to come.

Comment 2

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2 rev 1 - Allow SVG glyphs to inherit fill/stroke pattern from object

This is where it starts to look quite cool.

Comment 3

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

NOTE: There is still a horrific memory leak somewhere I need to track down

Comment 4

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2 rev 1 - Allow SVG glyphs to inherit fill/stroke pattern from object

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2 rev 1 - Allow SVG glyphs to inherit fill/stroke pattern from object

Gah. Stupid machines. *Actually* attached this time.

Comment 6

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Reftest for Patch 1

Super basic test

Comment 7

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Nice example image. Needs the [also attached] font in the same directory.

Comment 8

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Font to go with example image

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 11

[Cameron McCormack (:heycam)]

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="".

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

how about <glyph n="100"> ?

Comment 13

[Cameron McCormack (:heycam)]

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">.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

<g glyphid="..."> is fine I guess.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 17

[Jonathan Kew [:jfkthame]]

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.

Comment 18

[Jonathan Kew [:jfkthame]]

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

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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 20

[Cameron McCormack (:heycam)]

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 21

[Cameron McCormack (:heycam)]

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.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 23

[Jonathan Kew [:jfkthame]]

(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.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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?

Comment 25

[Jonathan Kew [:jfkthame]]

(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.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 27

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

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.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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 :-).

Comment 29

[Cameron McCormack (:heycam)]

(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.

Comment 30

[Jonathan Kew [:jfkthame]]

(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.

Comment 31

[Jonathan Kew [:jfkthame]]

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.)

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 33

[Cameron McCormack (:heycam)]

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.

Comment 34

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 1-1 rev 2 Add OTS support for OpenType SVG table

Comment 35

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 1-2 rev 2 Basic OpenType SVG functionality

Comment 36

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-1 rev 2 Add support for CSS objectfill and objectstroke values

Comment 37

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2 rev 2 Let OpenType SVG glyphs inherit object fill and stroke paints

Comment 38

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 1-2 rev 3 Basic OpenType SVG functionality

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Never mind, I see you add this later.

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Don't forget to attach the complete set of reftest patches (the first one is still missing).

Comment 44

[Jonathan Kew [:jfkthame]]

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.

Comment 45

[Jonathan Kew [: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/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.

Comment 46

[Jonathan Kew [:jfkthame]]

Attachment: patch, extend gfxFontUtils::MapCharToGlyph to accept variation selectors

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.

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 48

[Jonathan Kew [:jfkthame]]

(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.

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK.

Comment 50

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(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...

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 52

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 1-1 rev 3 Add OTS support for OpenType SVG table

Comment 53

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 1-2 rev 4 Basic OpenType SVG functionality

Comment 54

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-1 rev 3 Add support for CSS objectfill and objectstroke values

Comment 55

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2 rev 3 Let OpenType SVG glyphs inherit object fill and stroke paints

Comment 56

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Still have to add UVS support and redo reftests since the document format has changed.

Comment 57

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 58

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 59

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 60

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 1-2 rev 5 Basic OpenType SVG functionality

Comment 61

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-1-1 rev 4 Add -moz-font media feature for SVG Glyphs

These patch numbers are really getting out of hand.

Comment 62

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-1-2 rev 4 Add support for objectfill and objectstroke values to CSS parser

Comment 63

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-1-1 rev 4 Add -moz-font media feature for SVG Glyphs

Comment 64

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2 rev 4 Let OpenType SVG glyphs inherit object fill and stroke paints

Comment 65

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

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 66

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2 rev 4 Let OpenType SVG glyphs inherit object fill and stroke paints

Oops

Comment 67

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 68

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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&

Comment 69

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Reftest Patch 1 - Basic tests for OpenType SVG glyphs

Finally.

Comment 70

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 71

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(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 72

[Jonathan Kew [:jfkthame]]

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.

Comment 73

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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 74

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 75

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 76

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(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?

Comment 77

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(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.

Comment 78

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 79

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 1-2 rev 6 Basic OpenType SVG functionality

Comment 80

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2 rev 5 Let OpenType SVG glyphs inherit object fill and stroke paints

Carrying over r=roc from comment #68

Comment 81

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 82

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 1-2 rev 7 Basic OpenType SVG functionality

Carrying over r=roc from comment #81

Comment 83

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Reftest Patch 1 rev 2 - Basic tests for OpenType SVG glyphs

Comment 84

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Reftest Patch 2 - Tests for objectfill/objectstroke

Comment 85

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 86

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 87

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Reftest Patch 1 rev 3 - Basic tests for OpenType SVG glyphs

Carrying over r=roc from comment #85

Comment 88

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Reftest Patch 2 rev 2 - Tests for objectfill/objectstroke

Comment 89

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-1-1 rev 5 Add -moz-is-glyph media feature for SVG Glyphs

Comment 90

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-1-2 rev 5 Add support for objectfill and objectstroke values to CSS parser

Comment 91

[John Daggett (:jtd)]

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

Comment 92

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(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

Comment 93

[Jeremie Patonnier :Jeremie]

(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.

Comment 94

[Cameron McCormack (:heycam)]

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.

Comment 95

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Ah, writing that I was under the impression that the first frame of an animated SVG drawing == the SVG drawing without animations applied. Edited.

Comment 96

[John Daggett (:jtd)]

(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.

Comment 97

[Jeremie Patonnier :Jeremie]

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

Comment 98

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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>

Comment 99

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 3 - Assorted small fixes

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

Comment 100

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Reftest Patch 1 rev 4 - Basic tests for OpenType SVG glyphs

Just updated to account for clipping change in Patch 3. Carry over r=roc from comment #85

Comment 101

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Reftest Patch 2 rev 3 - Tests for objectfill/objectstroke

Just updated to account for clipping change in Patch 3. Carry over r=roc from Attachment #597201 [details] [diff]

Comment 102

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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".

Comment 103

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 3 - Fixed bug where SVG glyphs outside of defs tag weren't being drawn

Woo, one line patch.

Comment 104

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 4 - Include SVG glyph when calculating glyph extents

Comment 105

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Reftest Patch 1 rev 5 - Basic tests for OpenType SVG glyphs

Minor change to basic reftests after patch 3. Carry over r=roc from comment #87

Comment 106

[Jonathan Kew [:jfkthame]]

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 107

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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)

Comment 108

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 109

[Jonathan Watt [:jwatt]]

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.

Comment 110

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(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.

Comment 111

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 1-2 rev 7 Basic OpenType SVG functionality

Blargh, bitrot. Carry over r=roc from comment #81.

Comment 112

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-1-1 rev 6 Add -moz-is-glyph media feature for SVG Glyphs

Carry over r=dbaron from comment #107

Comment 113

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-1-2 rev 6 Add support for objectfill and objectstroke values to CSS parser

Comment 114

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 115

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 116

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, we should support fallback colors.

Comment 117

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-1-2 rev 7 Add support for objectfill and objectstroke values to CSS parser

New patch that doesn't break everything.

Comment 118

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Reftest Patch 1 rev 6 - Basic tests for OpenType SVG glyphs

Updated tests to use -moz prefixed attributes. Carry over r=roc from comment #87

Comment 119

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2 rev 6 Let OpenType SVG glyphs inherit object fill and stroke paints

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.

Comment 120

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 4 rev 2 - Include SVG glyph when calculating glyph extents

Comment 121

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Reftest Patch 3 - Test for SVG glyph clipping

Comment 122

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-1-1 rev 7 Add -moz-is-glyph media feature for SVG Glyphs

Comment 123

[Jonathan Kew [:jfkthame]]

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.)

Comment 124

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 125

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(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.

Comment 126

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 1-2 rev 7 Basic OpenType SVG functionality

Rebase. Carry over r=roc from comment #81.

Comment 127

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-1-1 rev 7 Add -moz-is-glyph media feature for SVG Glyphs

Rebase.

Comment 128

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2 rev 7 Let OpenType SVG glyphs inherit object fill and stroke paints

Comment 129

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 5-1 - Add support for objectFillOpacity and objectStrokeOpacity values to CSS parser

Comment 130

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 5-2 - Let SVG glyphs inherit fill-opacity and stroke-opacity through objectFillOpacity and objectStrokeOpacity

Comment 131

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Reftest Patch 4 - Tests for objectFillOpacity/objectStrokeOpacity

Comment 132

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 133

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 134

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 135

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-1-1 rev 8 Add -moz-is-glyph media feature for SVG Glyphs

Carry over r=dbaron from comment 132

Comment 136

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-1-2 rev 8 Add support for objectfill and objectstroke values to CSS parser

Carry over r=dbaron from comment 133

Comment 137

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

> ::: 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?

Comment 138

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's only going to have to be created when we actually render with SVG glyphs, which will be almost never.

Comment 139

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2 rev 8 Let OpenType SVG glyphs inherit object fill and stroke paints

Comment 140

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 141

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2-1 rev 9 Add new gfxTextObjectPaint paint wrapper for SVG glyphs to inherit outer text object paints

Comment 142

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2-2 rev 9 - Make SVG glyphs use gfxTextObjectPaint

Comment 143

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2-3 - Actually have text objects construct and pass in a gfxTextObjectPaint

Comment 144

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 145

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(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 146

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2-1 rev 10 Add new gfxTextObjectPaint paint wrapper for SVG glyphs to inherit outer text object paints

Comment 147

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2-2 rev 10 - Actually have text objects construct and pass in a gfxTextObjectPaint

Comment 148

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 149

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 150

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2-1 rev 11 Add new gfxTextObjectPaint paint wrapper for SVG glyphs to inherit outer text object paints

Comment 151

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2-2 rev 11 - Actually have text objects construct and pass in a gfxTextObjectPaint

Comment 152

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 153

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 154

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2-1 rev 12 Add new gfxTextObjectPaint paint wrapper for SVG glyphs to inherit outer text object paints

Comment 155

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2-2 rev 12 - Actually have text objects construct and pass in a gfxTextObjectPaint

Comment 156

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 157

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2-3 rev 13 - Actually have text objects construct and pass in a gfxTextObjectPaint

Carry over r=roc from comment 156

Comment 158

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2-3 rev 13 - Actually have text objects construct and pass in a gfxTextObjectPaint

Carry over r=roc from comment 156

Comment 159

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2-2 rev 13 - Add float hashtable key type

Carry over r=roc from comment 156

Comment 160

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 3 - Fixed bug where SVG glyphs outside of defs tag weren't being drawn

Rebase; carry over r=roc

Comment 161

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 4 rev 3 - Include SVG glyph when calculating glyph extents

Rebase; carry over r=jfkthame

Comment 162

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 5-1 rev 2 - Add support for objectFillOpacity and objectStrokeOpacity values to CSS parser

Comment 163

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 5-2 rev 2 - Let SVG glyphs inherit fill-opacity and stroke-opacity through objectFillOpacity and objectStrokeOpacity

Comment 164

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Reftest Patch 2 rev 4 - Tests for objectfill/objectstroke

Was missing commit message. Carry over r=roc.

Comment 165

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 166

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(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 167

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 168

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 169

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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 170

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-1-2 rev 9 Add support for objectfill and objectstroke values to CSS parser

Hrm. Good catch.

Comment 171

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 172

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Reftest Patch 4 rev 2 - Tests for objectFillOpacity/objectStrokeOpacity

Added test for explicit inheritance of objectFillOpacity/objectStrokeOpacity.

Comment 173

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 5-1 rev 3 - Add support for objectFillOpacity and objectStrokeOpacity values to CSS parser

Comment 174

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 5-2 rev 3 - Let SVG glyphs inherit fill-opacity and stroke-opacity through objectFillOpacity and objectStrokeOpacity

Comment 175

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 176

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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)?

Comment 177

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Reftest Patch 4 rev 3 - Tests for objectFillOpacity/objectStrokeOpacity

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/

Comment 178

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2-1 rev 13 Add new gfxTextObjectPaint paint wrapper for SVG glyphs to inherit outer text object paints

Rebase. Carry over r=roc.

Comment 179

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 1-2 rev 8 Basic OpenType SVG functionality

Rebase. Carry over r=roc from comment 81.

Comment 180

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(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.

Comment 181

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2-3 rev 14 - Actually have text objects construct and pass in a gfxTextObjectPaint

Minor fix for failing reftest where pattern was not respecting CSS transforms. Carry over r=roc from comment 156.

Comment 182

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 5-1 rev 4 - Add support for objectFillOpacity and objectStrokeOpacity values to CSS parser

Comment 183

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 5-2 rev 4 - Let SVG glyphs inherit fill-opacity and stroke-opacity through objectFillOpacity and objectStrokeOpacity

Small fix---just made -moz-objectFillOpacity and -moz-objectStrokeOpacity the default opacity values for SVG glyphs. Carry over r=roc.

Comment 184

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

https://tbpl.mozilla.org/?tree=Try&rev=bad174cc96b6

Comment 185

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 6-1 rev 1 - Add support for objectValue values to CSS parser for some stroke properties in SVG glyphs

Comment 186

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 6-2 rev 1 - Use objectValue keyword in SVG glyphs

Comment 187

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Reftest Patch 5 rev 1 - Tests for objectValue keyword in SVG glyphs

Comment 188

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 7 rev 1 - Support new SVG table format

https://wiki.mozilla.org/SVGOpenTypeFonts#The_SVG_table

Comment 189

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Reftest Patch 6 rev 1 - Update tests to use the new SVG table format

Comment 190

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 1-1 rev 4 - OTS support for SVG table

Actually does some sanitisation now for new SVG table format:
https://wiki.mozilla.org/SVGOpenTypeFonts#The_SVG_table

Comment 191

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 192

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 193

[Jonathan Kew [:jfkthame]]

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.)

Comment 194

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(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.

Comment 195

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 1-1 rev 5 - OTS support for SVG table

Comment 196

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 7 rev 2 - Support new SVG table format

Comment 197

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 6-2 rev 2 - Use objectValue keyword in SVG glyphs

Carry over r=roc from comment #192

Comment 198

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Reftest Patch 6 rev 2 - Update tests to use the new SVG table format

Carry over r=roc from comment #191

Comment 199

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 200

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 1-1 rev 6 - OTS support for SVG table

For new new (new) SVG table format

Comment 201

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 7 rev 3 - Support new SVG table format

Comment 202

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Reftest Patch 6 rev 3 - Update tests to use the new SVG table format

For new^3 SVG table format. Carry over r=roc

Comment 203

[Jonathan Kew [:jfkthame]]

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.

Comment 204

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 1-1 rev 7 - OTS support for SVG table

Comment 205

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 7 rev 4 - Support new SVG table format

Comment 206

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Reftest Patch 6 rev 4 - Update tests to use the new SVG table format

For new^4 table format. Carry over r=roc

Comment 207

[Jonathan Kew [:jfkthame]]

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!)

Comment 208

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 209

[Jonathan Kew [:jfkthame]]

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?

Comment 210

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

> ::: 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...

Comment 211

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(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.

Comment 212

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 1-1 rev 8 - OTS support for SVG table

Carry over r=jfkthame from comment #207.

That bit of code had never been a problem until you pointed it out. Then it broke _everything_. Crazy.

Comment 213

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 7 rev 5 - Support new SVG table format

Comment 214

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 620523 [details] [diff] [review]
Patch 7 rev 5 - Support new SVG table format

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

::: gfx/thebes/gfxSVGGlyphs.cpp
@@ +111,5 @@
> +{
> +    const IndexEntry *a = (const IndexEntry*)_a;
> +    const IndexEntry *b = (const IndexEntry*)_b;
> +
> +    if (a->mEndGlyph < b->mStartGlyph) return -1;

<= I think?

@@ +112,5 @@
> +    const IndexEntry *a = (const IndexEntry*)_a;
> +    const IndexEntry *b = (const IndexEntry*)_b;
> +
> +    if (a->mEndGlyph < b->mStartGlyph) return -1;
> +    if (a->mStartGlyph > b->mEndGlyph) return 1;

>= I think?

@@ +119,5 @@
> +
> +gfxSVGGlyphsDocument *
> +gfxSVGGlyphs::FindOrCreateGlyphsDocument(PRUint32 aGlyphId)
> +{
> +    IndexEntry key = (IndexEntry){aGlyphId, aGlyphId, 0, 0};

The end should be aGlyphId + 1

@@ +123,5 @@
> +    IndexEntry key = (IndexEntry){aGlyphId, aGlyphId, 0, 0};
> +    IndexEntry *entry = (IndexEntry*)bsearch(&key, mIndex,
> +                                             mHeader->mIndexLength,
> +                                             sizeof(IndexEntry),
> +                                             CompareIndexEntries);

You definitely still need to check that aGlyphId is in the returned range.

To be honest, I'm not comfortable using bsearch this way. We're hoping it matches something which isn't really equal to any of the elements in the array.

Unless you can come up with a really watertight specification for what the elements are and what the ordering is, with all the expected properties, I think a handwritten binary search would be more clear.

Comment 215

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 1-1 rev 9 - OTS support for SVG table

Carry over r=jfkthame

Comment 216

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 7 rev 6 - Support new SVG table format

Comment 217

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

That last OTS patch is a trivial change to assume half-open index ranges rather than closed.

Comment 218

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Reftest Patch 6 rev 5 - Update tests to use the new SVG table format

Changed format to use half-open ranges. Carry over r=roc

Comment 219

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 620592 [details] [diff] [review]
Patch 7 rev 6 - Support new SVG table format

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

::: gfx/thebes/gfxSVGGlyphs.cpp
@@ +130,5 @@
> +    IndexEntry *entry = (IndexEntry*)bsearch(&key, mIndex,
> +                                             mHeader->mIndexLength,
> +                                             sizeof(IndexEntry),
> +                                             CompareIndexEntries);
> +    NS_ENSURE_TRUE(entry, nsnull);

This issues a warning if entry is null. It shouldn't, since that is perfectly OK and just means the font doesn't have an SVG glyph for this glyphID. so just return null if entry is false, no macro.

Comment 220

[Jonathan Kew [:jfkthame]]

Comment on attachment 620592 [details] [diff] [review]
Patch 7 rev 6 - Support new SVG table format

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

::: gfx/thebes/gfxSVGGlyphs.cpp
@@ +129,5 @@
> +    IndexEntry key = (IndexEntry){aGlyphId, aGlyphId + 1, 0, 0};
> +    IndexEntry *entry = (IndexEntry*)bsearch(&key, mIndex,
> +                                             mHeader->mIndexLength,
> +                                             sizeof(IndexEntry),
> +                                             CompareIndexEntries);

AIUI, there's no need to create a "key" IndexEntry at all. Just do

  IndexEntry *entry = (IndexEntry*)bsearch(&aGlyphId, mIndex, ....)

where CompareIndexEntries is modified to do

  const PRUint32 gid = *(const PRUnt32*)a;
  const IndexEntry *entry = (const IndexEntry*)b;
  if (gid < entry->mStartGlyph) return -1;
  if (gid >= entry->mEndGlyph) return 1;
  return 0;

Note that there's no requirement for the "key" used in bsearch to be an object of the same type as the array elements. It just needs to be a value that the comparison function knows how to compare with them.

(There was also no need to change to half-open ranges, AFAICS - the comparison function could equally well be written for closed ranges. I'd tend to think the closed version was easier for people to understand, though I don't have a strong opinion on which is used - as long as it's clearly documented!)

Comment 221

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(In reply to Jonathan Kew (:jfkthame) from comment #220)
> Note that there's no requirement for the "key" used in bsearch to be an
> object of the same type as the array elements. It just needs to be a value
> that the comparison function knows how to compare with them.
Ah, I should be reading docs more closely. Cool. Also means that it'll actually build on MSVC, which try tells me doesn't support that sort of compound literal.

> (There was also no need to change to half-open ranges, AFAICS - the
> comparison function could equally well be written for closed ranges. I'd
> tend to think the closed version was easier for people to understand, though
> I don't have a strong opinion on which is used - as long as it's clearly
> documented!)
Apparently it's a more common convention.

Comment 222

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 7 rev 7 - Support new SVG table format

MSVC, Y U NO SUPPORT STRUCT COMPOUND LITERALS?

Carry over r=roc.

Before: https://tbpl.mozilla.org/?tree=Try&rev=0a0a0ebcd3dc
After: https://tbpl.mozilla.org/?tree=Try&rev=ab299522c26e

Comment 223

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 4-1 - Add _moz_cairo_get_font_matrix to symbols.def.in

Comment 224

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 610340 [details] [diff] [review]
Patch 5-1 rev 4 - 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)
>+{
>+  if (eCSSUnit_Enumerated == aValue.GetUnit()) {
>+    switch (aValue.GetIntValue()) {
>+    case NS_STYLE_OBJECT_FILL_OPACITY:
>+      aOpacityTypeField = eStyleSVGOpacityType_ObjectFillOpacity;
>+      break;
>+    case NS_STYLE_OBJECT_STROKE_OPACITY:
>+      aOpacityTypeField = eStyleSVGOpacityType_ObjectStrokeOpacity;
>+      break;
>+    default:
>+      NS_NOTREACHED("SetSVGOpacity: Unknown keyword");
>+    }
>+    // Fall back on fully opaque
>+    aOpacityField = 1.0f;
>+  } else if (eCSSUnit_Inherit == aValue.GetUnit()) {
>+    aCanStoreInRuleTree = false;
>+    aOpacityField = aParentOpacity;
>+    aOpacityTypeField = aParentOpacityType;
>+  } else {
>+    SetFactor(aValue, aOpacityField, aCanStoreInRuleTree,
>+              aParentOpacity, 1.0f, SETFCT_OPACITY);

You need to set aOpacityTypeField here, as I said in comment 175.


>+  nsStyleSVGOpacityType objectStrokeOpacity = parentSVG->mObjectStrokeOpacity;

No, I actually meant what I wrote in comment 175.  (twice).  svg, not parentSVG.  You're just using a temporary here because svg->mObjectStrokeOpacity has a field width so you can't pass it to a method, and you need to not modify it in some cases.

>+  SetSVGOpacity(*aRuleData->ValueForStrokeOpacity(),
>+                svg->mStrokeOpacity, objectStrokeOpacity, canStoreInRuleTree,
>+                parentSVG->mStrokeOpacity, parentSVG->mObjectStrokeOpacity);
>+  svg->mObjectStrokeOpacity = objectStrokeOpacity;

I'd note that I previously granted review+ on this patch with the changes written in comment 175, so shouldn't have needed to ask for re-review.  However, at this point, I would like to look at the patch again.

Comment 225

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 1-1 rev 9 - OTS support for SVG table

Rebase. Carry over r=jfkthame.

Comment 226

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 1-2 rev 8 Basic OpenType SVG functionality

Rebase. Carry over r=roc.

Comment 227

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-1-1 rev 8 Add -moz-is-glyph media feature for SVG Glyphs

Rebase. Carry over r=dbaron

Comment 228

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-1-2 rev 9 Add support for objectfill and objectstroke values to CSS parser

Rebase. Carry over r=dbaron from comment 171.

Comment 229

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2-1 rev 13 Add new gfxTextObjectPaint paint wrapper for SVG glyphs to inherit outer text object paints

Rebase. Carry over r=roc.

Comment 230

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2-3 rev 14 - Actually have text objects construct and pass in a gfxTextObjectPaint

Rebase. Carry over r=roc.

Comment 231

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 4 rev 3 - Include SVG glyph when calculating glyph extents

Rebase. Carry over r=jfkthame.

Comment 232

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 5-1 rev 5 - Add support for objectFillOpacity and objectStrokeOpacity values to CSS parser

Comment 233

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 5-2 rev 4 - Let SVG glyphs inherit fill-opacity and stroke-opacity through objectFillOpacity and objectStrokeOpacity

Rebase. Carry over r=roc

Comment 234

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 6-1 rev 1 - Add support for objectValue values to CSS parser for some stroke properties in SVG glyphs

Rebase.

Comment 235

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 6-2 rev 2 - Use objectValue keyword in SVG glyphs

Rebase. Carry over r=roc

Comment 236

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 8-1 rev 1 - Implement GetFontTable for downloaded fonts on linux

Comment 237

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 8-2 rev 1 - Azure-ify SVG glyphs

Comment 238

[Karl Tomlinson (:karlt)]

Comment on attachment 629079 [details] [diff] [review]
Patch 8-1 rev 1 - Implement GetFontTable for downloaded fonts on linux

>+#include <freetype/tttables.h>

FreeType includes are usually pulled in with the #include FT_TRUETYPE_TABLES_H
form.  ("tttables.h" is named to fit DOS 8.3 filenames.)

>+    nsresult GetFontTable(PRUint32 aTableTag, FallibleTArray<PRUint8>& aBuffer);

Please use NS_OVERRIDE virtual here to maximize explicitness and catch
possible errors in future code changes.

>+    FT_ULong length = 0;
>+    FT_Error error = FT_Load_Sfnt_Table(mFace, aTableTag, 0, NULL, &length);
>+    if (error) {
>+        return NS_ERROR_NOT_AVAILABLE;
>+    }
>+
>+    if (!aBuffer.SetLength(length)) {

Should check for truncation from FT_ULong to
FallibleTArray<PRUint8>::size_type.

>+    error = FT_Load_Sfnt_Table(mFace, aTableTag, 0, aBuffer.Elements(), &length);
>+    if (error) {
>+        return NS_ERROR_FAILURE;

Would it make sense to free the memory in aBuffer at this point?

Comment 239

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 629075 [details] [diff] [review]
Patch 5-1 rev 5 - Add support for objectFillOpacity and objectStrokeOpacity values to CSS parser

This is followup on the reviews in comment 167, comment 175, and 
comment 224.  (This bug is really too big; I think some of these
pieces should have gone in other bugs to make this more 
manageable.  Most of my time doing this review was recovering
that context.)

In SetSVGOpacity:
  >+  } else {
  >+    SetFactor(aValue, aOpacityField, aCanStoreInRuleTree,
  >+              aParentOpacity, 1.0f, SETFCT_OPACITY);
  >+    aOpacityTypeField = eStyleSVGOpacityType_Normal;
  >+  }
you've still missed the first half of what I said in this quote 
in comment 175:
  >You should put the SetFactor call inside if (aValue.GetUnit() !=
  >eCSSUnit_Null), and then move this into that check.

So you need to change the "else" quoted above to an
"else if (eCSSUnit_Null != aValue.GetUnit())".

r=dbaron with that one change

(Adding tests for the issues in comment 175, as I suggested in comment 176, would have been nice, but I guess I'm not going to hold this up over it.)

Comment 240

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 629075 [details] [diff] [review]
Patch 5-1 rev 5 - Add support for objectFillOpacity and objectStrokeOpacity values to CSS parser

Actually, a few more changes here, given that I think these names are confusing, especially in the context of the next patch:

>diff --git a/layout/style/nsStyleStruct.h b/layout/style/nsStyleStruct.h

>+enum nsStyleSVGOpacityType {

Could you change the name of this enum to nsStyleSVGOpacitySource (Type -> Source)

>+  eStyleSVGOpacityType_Normal,
>+  eStyleSVGOpacityType_ObjectFillOpacity,
>+  eStyleSVGOpacityType_ObjectStrokeOpacity

>+  //  In SVG glyphs, whether we inherit fill or stroke opacity from the outer
>+  // text object
>+  nsStyleSVGOpacityType mObjectFillOpacity    : 2;

And rename this mObjectFillOpacity -> mFillOpacitySource

>+  nsStyleSVGOpacityType mObjectStrokeOpacity  : 2;

mObjectStrokeOpacity -> mStrokeOpacitySource

Comment 241

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 629077 [details] [diff] [review]
Patch 6-1 rev 1 - Add support for objectValue values to CSS parser for some stroke properties in SVG glyphs

nsStyleConsts.h:

>+#define NS_STYLE_STROKE_PROP_OBJECTVALUE        255

This isn't used in conjunction with any other lists, so use 0.
(If it were, you should have had a comment saying which ones.)

nsRuleNode.cpp:

>+template <typename FieldT, typename T>
>+static bool
>+SetTextObjectValue(const nsCSSValue& aValue, FieldT& aField, T aFallbackValue)

If this compiles with only a single template parameter (i.e.,
so you have (..., T& aField, T aFallbackValue)), you should do it
that way, but otherwise leave it.

Then again, given that there's only one use of SetTextObjectValue,
I think the case for having it be a function rather than just writing
it out at the one user is pretty weak; that might be preferable.

>   case eCSSUnit_Inherit:
>     canStoreInRuleTree = false;
>     // only do the copy if weren't already set up by the copy constructor
>     // FIXME Bug 389408: This is broken when aStartStruct is non-null!
>     if (!svg->mStrokeDasharray) {
>       svg->mStrokeDasharrayLength = parentSVG->mStrokeDasharrayLength;
>+      svg->mObjectStrokeDasharray = parentSVG->mObjectStrokeDasharray;

At the very least, put your new line outside the if (and probably
above the comment as well) to avoid making the bug worse.

>+  case eCSSUnit_Enumerated:
>+    NS_ABORT_IF_FALSE(strokeDasharrayValue->GetIntValue() == NS_STYLE_STROKE_PROP_OBJECTVALUE,
>+                      "Unknown keyword for stroke-dasharray");

Please wrap after the == to avoid 80th column violation.


>+  if (!(svg->mObjectStrokeDashoffset =
>+        SetTextObjectValue(*aRuleData->ValueForStrokeDashoffset(),
>+                           svg->mStrokeDashoffset,
>+                           parentSVG->mStrokeDashoffset))) {

I think your fallback value (the third param to SetTextObjectValue)
should be the initial value of mStrokeDashoffset, not
parentSVG->mStrokeDashoffset.  I think this for two reasons:
 (1) it's better to be consistent with stroke-dasharray and stroke-width
 (2) if you don't make this change, you'd need to add a CheckSVGCallback
     function to handle this properly in
     nsRuleNode::CheckSpecifiedProperties, like other pseudo-inherited
     values (see CheckTextCallback for an example)

+  switch (strokeWidthValue->GetUnit()) {
+  case eCSSUnit_Enumerated:
+    NS_ABORT_IF_FALSE(strokeWidthValue->GetIntValue() == NS_STYLE_STROKE_PROP_OBJECTVALUE,
+                      "Unrecognized keyword for stroke-width");

Again, wrap at the ==.

nsStyleStruct.h:

+  // SVG glyph outer object inheritance for other properties
+  bool mObjectStrokeDasharray   : 1;

rename to mStrokeDasharrayFromObject

+  bool mObjectStrokeDashoffset  : 1;

rename to mStrokeDashoffsetFromObject

+  bool mObjectStrokeWidth       : 1;

rename to mStrokeWidthFromObject


r=dbaron with those changes

Comment 242

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 1-1 rev 9 - OTS support for SVG table

Rebase; carry over r+

Comment 243

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 1-2 rev 8 Basic OpenType SVG functionality

Rebase; carry over r+

Comment 244

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-1-1 rev 8 Add -moz-is-glyph media feature for SVG Glyphs

Rebase all the things! Carry over r+

Comment 245

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-1-2 rev 9 Add support for objectfill and objectstroke values to CSS parser

Rebase. Carry over r+

Comment 246

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2-1 rev 13 Add new gfxTextObjectPaint paint wrapper for SVG glyphs to inherit outer text object paints

More rebasing. Carry over r+

Comment 247

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2-2 rev 13 - Add float hashtable key type

Bet you didn't see a REBASE coming. Nor carrying over r+.

Comment 248

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 2-2-3 rev 14 - Actually have text objects construct and pass in a gfxTextObjectPaint

Rebase. Carry over r+.

Comment 249

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 3 - Fixed bug where SVG glyphs outside of defs tag weren't being drawn

Rebase. Carry over r+

Comment 250

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 4-1 - Add _moz_cairo_get_font_matrix to symbols.def.in

Rebase. Carry over r+

Comment 251

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 4 rev 3 - Include SVG glyph when calculating glyph extents

Rebase rebase rebase. Carry over review+

Comment 252

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 5-1 rev 5 - Add support for objectFillOpacity and objectStrokeOpacity values to CSS parser

Rebase. Carry over r+

Comment 253

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 5-2 rev 4 - Let SVG glyphs inherit fill-opacity and stroke-opacity through objectFillOpacity and objectStrokeOpacity

Rebase; carry over r+

Comment 254

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 6-1 rev 1 - Add support for objectValue values to CSS parser for some stroke properties in SVG glyphs

Rebase; carry over r+

Comment 255

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 6-2 rev 2 - Use objectValue keyword in SVG glyphs

Rebase; carry over r+

Comment 256

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 7 rev 7 - Support new SVG table format

Rebase; carry over r+

Comment 257

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 8-1 rev 1 - Implement GetFontTable for downloaded fonts on linux

Rebase; carry over r+

Comment 258

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 8-2 rev 1 - Azure-ify SVG glyphs

Last rebase. Last r+ to carry over. Phew.

Comment 259

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch 9 rev 1 - Relax PaintSVG assertions for SVG glyphs

Comment 260

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

https://tbpl.mozilla.org/?tree=Try&rev=a2cd741f64bc

https://hg.mozilla.org/integration/mozilla-inbound/rev/95e68df32601
https://hg.mozilla.org/integration/mozilla-inbound/rev/a27c021626dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/286c5d489588
https://hg.mozilla.org/integration/mozilla-inbound/rev/585eba0ca2a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ea9c7730f97
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0c93525cfba
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4b0ce0436fd
https://hg.mozilla.org/integration/mozilla-inbound/rev/cce02d267317
https://hg.mozilla.org/integration/mozilla-inbound/rev/3afb7682d000
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec84c72f3c5e
https://hg.mozilla.org/integration/mozilla-inbound/rev/781783d7bc18
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0a81d96a7ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/1acd3ee53ca7
https://hg.mozilla.org/integration/mozilla-inbound/rev/f99b3f8583f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/c83b07f9fbe8
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ec5e5f77023
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d6c5037f28c
https://hg.mozilla.org/integration/mozilla-inbound/rev/a41a90fab545
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1b07cfa6e47
https://hg.mozilla.org/integration/mozilla-inbound/rev/fff4143b479e
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb39545e33d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/61361e32327c
https://hg.mozilla.org/integration/mozilla-inbound/rev/137972e35454
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f65a2820a8d
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5e3428f33fd

Comment 261

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/95e68df32601
https://hg.mozilla.org/mozilla-central/rev/a27c021626dc
https://hg.mozilla.org/mozilla-central/rev/286c5d489588
https://hg.mozilla.org/mozilla-central/rev/585eba0ca2a7
https://hg.mozilla.org/mozilla-central/rev/1ea9c7730f97
https://hg.mozilla.org/mozilla-central/rev/a0c93525cfba
https://hg.mozilla.org/mozilla-central/rev/c4b0ce0436fd
https://hg.mozilla.org/mozilla-central/rev/cce02d267317
https://hg.mozilla.org/mozilla-central/rev/3afb7682d000
https://hg.mozilla.org/mozilla-central/rev/ec84c72f3c5e
https://hg.mozilla.org/mozilla-central/rev/781783d7bc18
https://hg.mozilla.org/mozilla-central/rev/f0a81d96a7ba
https://hg.mozilla.org/mozilla-central/rev/1acd3ee53ca7
https://hg.mozilla.org/mozilla-central/rev/f99b3f8583f8
https://hg.mozilla.org/mozilla-central/rev/c83b07f9fbe8
https://hg.mozilla.org/mozilla-central/rev/7ec5e5f77023
https://hg.mozilla.org/mozilla-central/rev/5d6c5037f28c
https://hg.mozilla.org/mozilla-central/rev/a41a90fab545
https://hg.mozilla.org/mozilla-central/rev/d1b07cfa6e47
https://hg.mozilla.org/mozilla-central/rev/fff4143b479e
https://hg.mozilla.org/mozilla-central/rev/fb39545e33d5
https://hg.mozilla.org/mozilla-central/rev/61361e32327c
https://hg.mozilla.org/mozilla-central/rev/137972e35454
https://hg.mozilla.org/mozilla-central/rev/4f65a2820a8d
https://hg.mozilla.org/mozilla-central/rev/d5e3428f33fd

Comment 262

[Karl Tomlinson (:karlt)]

> https://hg.mozilla.org/mozilla-central/rev/5d6c5037f28c

(In reply to Karl Tomlinson (:karlt) from comment #238)
> Comment on attachment 629079 [details] [diff] [review]
> >+    FT_ULong length = 0;
> >+    FT_Error error = FT_Load_Sfnt_Table(mFace, aTableTag, 0, NULL, &length);
> >+    if (error) {
> >+        return NS_ERROR_NOT_AVAILABLE;
> >+    }
> >+
> >+    if (!aBuffer.SetLength(length)) {
> 
> Should check for truncation from FT_ULong to
> FallibleTArray<PRUint8>::size_type.

Forgot this, it seems.

Comment 263

[Christoph Diehl [:posidron]]

@dveditz,
What I can do and what I did is mutating the XML table inside the font file but for fuzzing layout - which I believe is the real attack vector here - Jesse is better suited here.

Comment 264

[Terrell Kelley]

I would like to test this new feature in action, but I don't see any attached OpenType-SVG fonts or links to the same. Google has been no help in finding them, either. As you had to have at least one font to test this feature, could you provide a link?

Comment 265

[Mardeg]

(In reply to Terrell Kelley from comment #264)
> As you had to have at least one font to test this
> feature, could you provide a link?
From http://pixelambacht.nl/2014/compyx-a-multicolor-8bit-font/#seeing-compyx-in-action
"we’re going to create a SVG-in-OpenType font, as per Adobe and Mozilla’s spec"
That page goes a step further to make a multicolor font, so you don't need that step.