The SVG WG want to allow SVG-as-an-image in <img> (and probably other SVG-as-an-image) to get color from the embedding document. In the longer term we want something much more general than just a mechanism to get color, which may be some combination of: http://www.w3.org/TR/2009/WD-SVGParamPrimer-20090616/ CSS variables and other things. For now though it was suggested that I go ahead and add context-fill and context-stroke support for SVG-as-an-image in <img> and see what people think. This would be useful for the gaia guys if they are to perhaps replace their icon font files with SVG icon set files.

We'd want a same origin restriction on this. (We discussed various opt-in options that could be used on the embedding document side to say that CSS properties (such as fill/stroke) can be accessed by the embedded document, but that's not nailed down yet.)

We can probably bundle this into the SVGImageContext. This is what we use for <svg:image> with a preserveAspectRatio attribute, to communicate that attribute's value to the "guest" image document, so that it can draw properly.

MXR link: http://mxr.mozilla.org/mozilla-central/source/layout/svg/SVGImageContext.h?rev=d997e5accc3f&mark=13-15 (It would no longer be <image> specific, of course.)

Right now the context paint stuff is text specific, at least in terms of where the classes live and what they're called. It needs to be generalized for use in SVG-as-an-image, markers and use.

Comment on attachment 8777116 [details] [diff] [review] part 1 - Move gfxTextContextPaint to a separate file to enable use in imagelib Review of attachment 8777116 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits addressed. If you'd like, it might be worth spinning off these first two parts into their own bug, so that they can land ASAP (as pure refactoring patches) and avoid risk of bitrot. (Or I suppose you could just land them soonihs anyway, as long as you're sure the rest of this bug will be ready to land very soon.) ::: dom/svg/SVGContentUtils.cpp @@ +15,5 @@ > #include "gfxSVGGlyphs.h" > #include "mozilla/gfx/2D.h" > #include "mozilla/dom/SVGSVGElement.h" > #include "mozilla/RefPtr.h" > +#include "mozilla/SVGContextPaint.h" I think you can get rid of the gfxSVGGlyphs.h include here, right? (4 lines above this new #include) The |#include "gfxSVGGlyphs.h"| was only here so that this file could use code that you've now moved to SVGContextPaint, I think. So this file's gfxSVGGlyphs.h include is now stale/unnecessary, I'm pretty sure. After this patch, it looks like gfxSVGGlyphs.h only defines 3 main things: - gfxSVGGlyphsDocument - gfxSVGGlyphs - SimpleTextContextPaint ...none of which are mentioned in this file (SVGContentUtils.cpp). ::: gfx/thebes/gfxSVGGlyphs.h @@ +163,5 @@ > static int CompareIndexEntries(const void *_a, const void *_b); > }; > > /** > + * XXX This is a complete hack and should die. This class is used when code Could you file a bug on removing this code (e.g. "Get rid of SimpleTextContextPaint")? Ideally it'd be great to mention that bug number in this comment, too. ::: layout/svg/SVGContextPaint.h @@ +23,5 @@ > + * > + * https://www.w3.org/TR/SVG2/painting.html#context-paint > + * > + * This feature allows the color in an SVG-in-OpenType glyph to come from the > + * computed style for the text that is being draws, for example, or for color Typo: s/being draws/being drawn/ ::: layout/svg/SVGTextFrame.h @@ +13,3 @@ > #include "gfxMatrix.h" > #include "gfxRect.h" > #include "gfxSVGGlyphs.h" As above, I believe you can get rid of the "gfxSVGGlyphs.h" include here; this file only used pieces that you've now moved to SVGContextPaint.h (which you're including separately). ::: layout/svg/nsSVGPathGeometryFrame.cpp @@ +10,4 @@ > #include "gfx2DGlue.h" > #include "gfxContext.h" > #include "gfxPlatform.h" > #include "gfxSVGGlyphs.h" As above, I'm pretty sure we should get rid of this "gfxSVGGlyphs.h" include. ::: layout/svg/nsSVGUtils.cpp @@ +17,5 @@ > #include "gfxUtils.h" > #include "mozilla/gfx/2D.h" > #include "mozilla/gfx/PatternHelpers.h" > #include "mozilla/Preferences.h" > +#include "mozilla/SVGContextPaint.h" As above, we should get rid of the "gfxSVGGlyphs.h" include (which isn't shown in this patch's context, but is present in this file -- it's 21 lines below this).

Comment on attachment 8777117 [details] [diff] [review] part 2 - Rename gfxTextContextPaint to SVGContextPaint and add some code comments Review of attachment 8777117 [details] [diff] [review]: ----------------------------------------------------------------- r=me on part 2; one optional nit: ::: layout/svg/nsSVGUtils.h @@ +526,5 @@ > /* > * @return false if there is no stroke > */ > static bool HasStroke(nsIFrame* aFrame, > + SVGContextPaint *aContextPaint = nullptr); Nit: shift the "*" to the left here. (You're fixing that pretty much everywhere else, it seems, but you missed this one)

Pushed by jwatt@jwatt.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/0a536af3ebfa part 1 - Move gfxTextContextPaint to a separate file to enable use in imagelib. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/c0404d1f3b47 part 2 - Rename gfxTextContextPaint to SVGContextPaint and add some code comments. r=dholbert

SVGContextPaintImpl is the thing we'll use in the imagelib code. I'd like to have a better name than SVGContextPaintImpl, but I can't think of one. The code from this class implements what SVGContextPaint should really do, but we can't really put that code there until we've killed off the other SVGContextPaint subclass in bug 1291494.

There is really no reason for this code to live in nsSVGUtils.

https://hg.mozilla.org/mozilla-central/rev/0a536af3ebfa https://hg.mozilla.org/mozilla-central/rev/c0404d1f3b47

Comment on attachment 8777161 [details] [diff] [review] part 3 - Move SVGTextContextPaint into SVGContextPaint.h Review of attachment 8777161 [details] [diff] [review]: ----------------------------------------------------------------- r=me on part 3, with one nit on now-stale code-removal: ::: layout/svg/SVGTextFrame.h @@ -159,5 @@ > - struct Paint { > - Paint() : mPaintType(eStyleSVGPaintType_None) {} > - > - void SetPaintServer(nsIFrame *aFrame, const gfxMatrix& aContextMatrix, > - nsSVGPaintServerFrame *aPaintServerFrame) { This file currently has two includes that I think are becoming stale, now that you're moving the code that they're supporting: #include "nsSVGPaintServerFrame.h" #include "mozilla/SVGContextPaint.h" Please remove both of those, if you can. Also, please remove this from the "class SVGTextFrame" scope: typedef mozilla::SVGTextContextPaint SVGTextContextPaint; (That typedef seems to already be stale/unused before this patch, but it probably makes sense to remove it as part of this patch, since you'll be removing the associated #include here.)

Comment on attachment 8777162 [details] [diff] [review] part 4 - Rename SVGTextContextPaint to SVGContextPaintImpl Review of attachment 8777162 [details] [diff] [review]: ----------------------------------------------------------------- r=me on part 4; just two observations: ::: layout/svg/SVGTextFrame.h @@ +190,5 @@ > typedef gfxTextRun::Range Range; > typedef mozilla::gfx::DrawTarget DrawTarget; > typedef mozilla::gfx::Path Path; > typedef mozilla::gfx::Point Point; > + typedef mozilla::SVGContextPaintImpl SVGContextPaintImpl; Per comment 13, this typedef should be removed in that earlier patch, I think. That'll mean this part of this patch (tweaking the typedef) won't be needed. ::: layout/svg/nsSVGUtils.cpp @@ +1328,5 @@ > const gfxMatrix& aContextMatrix, > nsIFrame* aFrame, > float& aOpacity, > SVGContextPaint* aOuterContextPaint, > + SVGContextPaintImpl::Paint& aTargetPaint, (Heads-up: this change doesn't apply cleanly, but it does apply with fuzz. One of the contextual lines was changed in https://hg.mozilla.org/mozilla-central/rev/685de09e5481 , in a way that patch -F can figure out.)

The "this change doesn't apply cleanly" thing affects part 5, too (in a way that patch -F can't work around, since part 5 is moving the code). You can fix it with search-and-replace in the patch files themselves (parts 4 and 5) -- just replace "&(style->*aFillOrStroke)" with "aFillOrStroke", it looks like.

Comment on attachment 8777161 [details] [diff] [review] part 3 - Move SVGTextContextPaint into SVGContextPaint.h Review of attachment 8777161 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, just realized one more thing about "part 3", which probably merits another round of review once you've added it (hence, changing r+ to r-): ::: layout/svg/SVGContextPaint.h @@ +108,5 @@ > +/** > + * This class should be flattened into SVGContextPaint once we get rid of the > + * other sub-class (SimpleTextContextPaint). > + */ > +struct SVGTextContextPaint : public SVGContextPaint This patch is moving the *struct definition* from SVGTextFrame.h to SVGContextPaint.h here, but all of its *method implementations* are left behind in SVGTextFrame.cpp! Shouldn't those be moved to SVGContextPaint.cpp as part of this change?

Comment on attachment 8777165 [details] [diff] [review] part 5 - Move the code from nsSVGUtils::SetupContextPaint and nsSVGUtils::GetContextPaint into SVGContextPaint. Review of attachment 8777165 [details] [diff] [review]: ----------------------------------------------------------------- r=me on part 5 with the following addressed. ::: layout/svg/SVGContextPaint.cpp @@ +94,5 @@ > + opacity, aOuterContextPaint, > + mFillPaint, &nsStyleSVG::mFill, > + nsSVGEffects::FillProperty()); > + > + SetFillOpacity(opacity); Nit: this function is the resting place of the *only* existing calls to the (public) methods SetFillOpacity & SetStrokeOpacity, I'm pretty sure. Since this code is now internal to the class itself here, those setters don't need to be public, nor do they really need to exist. We might as well just assign mFillOpacity etc. directly and get rid of the setters. Could you do make that change, either in a subsequent patch (rs=me in advance on that patch, if it's as trivial as I think) or as part of this patch here if you prefer? @@ +116,5 @@ > + SetStrokeOpacity(opacity); > + > + toDraw |= DrawMode::GLYPH_STROKE; > + } > + Stray whitespace here -- delete. ::: layout/svg/SVGTextFrame.cpp @@ +3740,5 @@ > // when they use context-fill etc. > aContext.SetMatrix(initialMatrix); > > SVGContextPaintImpl contextPaint; > + DrawMode drawMode = contextPaint.Init(&aDrawTarget, Now that all the setup code lives inside of SVGContextPaintImpl (rather than in nsSVGUtils), it seems hard to argue for the value of this Init method. It should probably just be reshaped into a SVGContextPaintImpl constructor. It seems like the only reason it exists is so it can return a DrawMode, which is a bit awkward for an Init method to do anyway; normally they'd return a success/failure code, which is what justifies their existence. We could just as easily store the DrawMode as a member-var on SVGContextPaintImpl (and we'd compute the value of that member-var in the constructor, using the same logic that currently lives in Init()). And then we could expose a GetDrawMode() accessor to return its value, which we'd use at this callsite.) Could you maybe add a "part 6" (or whatever number) patch to do that refactoring, so we aren't left with a hard-to-justify Init() method here, and to reduce the likelihood that we end up with accidentally-uninitialized SVGContextPaintImpl objects floating around?

(In reply to Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) from comment #16) > This patch is moving the *struct definition* from SVGTextFrame.h to > SVGContextPaint.h here, but all of its *method implementations* are left > behind in SVGTextFrame.cpp! Shouldn't those be moved to SVGContextPaint.cpp > as part of this change? They certainly should.

Comment on attachment 8777695 [details] [diff] [review] part 3 - Move SVGTextContextPaint into SVGContextPaint.h/.cpp Review of attachment 8777695 [details] [diff] [review]: ----------------------------------------------------------------- Thanks -- r=me on the updated part 3.

Pushed by jwatt@jwatt.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/9bc431d8e922 part 3 - Move SVGTextContextPaint into SVGContextPaint.h/.cpp. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/17233e72c546 part 4 - Rename SVGTextContextPaint to SVGContextPaintImpl. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/6a38b96af140 part 5 - Move the code from nsSVGUtils::SetupContextPaint and nsSVGUtils::GetContextPaint into SVGContextPaint. r=dholbert

https://hg.mozilla.org/mozilla-central/rev/9bc431d8e922 https://hg.mozilla.org/mozilla-central/rev/17233e72c546 https://hg.mozilla.org/mozilla-central/rev/6a38b96af140

Do these patches add the context-fill keyword or are they just groundwork?

I believe everything that's landed so far has just been groundwork - this isn't done yet, per the "leave-open" keyword. It looks like jwatt's out until October, so unless he says otherwise, I expect that's when the rest will be coming. (That would also mean this bug will have patches that land across multiple releases, which is a bit awkward & unfortunate from a tracking perspective. :-/ That's what I was getting at in comment 6, regarding doing these helper-patches in a separate helper-bug (and my implication was that we should only land patches on this bug once we've got everything approximately ready-to-land). The "leave-open" keyword is a hack & isn't really meant to be a long-term thing, generally; release/regression-tracking & backouts get very cumbersome when a single bug has multiple patches land over a long period of time. Not a huge deal, but worth avoiding in the future if possible.)

Dan, is there anybody else who could pick this up before October? Unblocking the use of SVG in firefox frontend for the primary UI would be pretty useful from a sanity / code maintenance / footprint / design perspective. :-)

I'm backlogged on reviews & coding-that-I-need-to-do, so I personally can't volunteer to pick this up, and I'm not sure who else to suggest. (jet would be a better person to answer that, if we really need someone here.) Also: - I'm not sure exactly what the high-level plan of attack / next-steps were here. (so far this has just been refactoring) - I'm not sure how much work remains. - So, anyone who'd be picking this up would have a bit of a research/spin-up burden which might eat up some of the time that we'd be trying to win by reassigning it away from jwatt. jwatt's back at the very beginning of October, which isn't *too* far out -- so unless there's a serious need to take people off of other tasks & put them onto this, I'd tend to say we should just wait for him. (even if it means we can't use custom-colored SVG in Firefox frontend code quite yet)

Hi Jonathan, if you are back do you know when you might be able to look at this again? Thanks!

Replied on IRC, but should have replied here too. I can look at this again during the All Hands. Let's talk then.

Since this functionality isn't in a spec currently, context color can only be propagated by chrome code or if the pref svg.context-properties.content.enabled is set to true to allow content devs to experiment with the feature.

For now I'm only implementing this for nsImageBoxFrame since that's what the front-end code needs.

I have two other things that I've still to finish. First I'm working on implementing a new '-moz-context-properties' property which will take a list of other property names which are the properties an embedding element wishes to expose to the embedded SVG image. This is so that (a) the propagation is opt-in by both the embedding and embedded content (possibly to avoid any security/privacy issues) and (b) so that we can avoid propagating an SVGImageContext and SVGEmbeddingContextPaint in the 99.99999% of the time that we paint an image when it is not needed. Second, tests.

Comment on attachment 8828472 [details] [diff] [review] part 6 - When copying SVGImageContexts use the copy ctor and override individual members Review of attachment 8828472 [details] [diff] [review]: ----------------------------------------------------------------- Extended-commit-message nit: > Prior to this patch whenever we wanted to pass on a modified SVGImageContexts Drop the trailing "s" here. (Should be singular, "a modified SVGImageContext") > to modify (because of default arguements). This patch fixes that by making us Typo -- there's a stray 'e' in "arguements" here. Should be "arguments". ::: image/VectorImage.cpp @@ +854,5 @@ > if ((aFlags & FLAG_FORCE_PRESERVEASPECTRATIO_NONE) && aSVGContext.isSome()) { > Maybe<SVGPreserveAspectRatio> aspectRatio = > Some(SVGPreserveAspectRatio(SVG_PRESERVEASPECTRATIO_NONE, > SVG_MEETORSLICE_UNKNOWN)); > + svgContext = Some(SVGImageContext(aSVGContext.ref())); // copy Don't use .ref() here -- "*aSVGContext" (with an asterisk) is equivalent and preferred, I think. (Semantically, we try to treat Maybe variables as if they were pointers.) ::: layout/svg/SVGImageContext.h @@ +39,5 @@ > const CSSIntSize& GetViewportSize() const { > return mViewportSize; > } > > + void SetViewportSize(CSSIntSize& aSize) { add 'const' to the parameter, please -- "const CSSIntSize&" @@ +48,5 @@ > return mPreserveAspectRatio; > } > > + void SetPreserveAspectRatio(Maybe<SVGPreserveAspectRatio>& aPAR) { > + mPreserveAspectRatio = aPAR; As above, the param should be "const Maybe<..."

Comment on attachment 8828473 [details] [diff] [review] part 7 - Support lazy initialization of AutoSetRestoreSVGContextPaint Review of attachment 8828473 [details] [diff] [review]: ----------------------------------------------------------------- > Bug 1058040, part 7 - Support lazy initialization of AutoSetRestoreSVGContextPaint. r=dholbert This seems like it might be adding unnecessary complexity. Can we achieve the same result (with better separation-of-concerns) by composing Maybe<> with the existing AutoSetRestoreSVGContextPaint class (and only doing that in whichever places need this lazy behavior)? e.g. Rather than the following: AutoSetRestoreSVGContextPaint foo; ... if (...) { foo.Init() } ...we'd instead have the following: Maybe<AutoSetRestoreSVGContextPaint> foo; ... if (...) { foo.emplace(...); }

[ni for htoughts on comment 39 & whether we actually need part 7]

Comment on attachment 8828474 [details] [diff] [review] part 8 - Make SVGContextPaint a ref counted class Review of attachment 8828474 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the SVGContextPaint comment clarified as described below (rewording & adding a "XXX Note" which then gets removed in a later patch) ::: layout/svg/SVGContextPaint.h @@ +32,5 @@ > * computed style for the text that is being drawn, for example, or for color > * in an SVG embedded by an <img> element to come from the embedding <img> > * element. > + * > + * This class is generally created on the stack and deleted when the function "created on the stack and deleted" sounds contradictory. Stack variables don't need to be deleted. (I have some suggested replacement-text for this whole paragraph below.) @@ +37,5 @@ > + * returns. The reason this class is reference counted is because > + * SVGImageContext needs to hold an object of this type, but SVGImageContext > + * can be stored in the heap. SVGImageContext objects are also copied a lot > + * which would make holding an object of this type tricky it if wasn't ref- > + * counted (UniquePtr members and copying of objects don't play nicely). The last couple sentences here are a bit too hand-wavy -- in particular: - There's an implied connection between "SVGImageContext can be stored on the heap" and the need for refcounting, but it's unclear what that connection is. - And the stuff about copying & UniquePtr "don't play nicely" is similarly unclear. How about we replace this whole paragraph with something like the following (which IMO is clearer & more direct) (please edit as-needed if any of this is incorrect): ==== This class is reference counted so that it can be shared among many similar SVGImageContext objects. (SVGImageContext objects are frequently copy-constructed with small modifications, and we'd like for those copies to be able to share their context-paint data cheaply.) However, in most cases, SVGContextPaint instances are stored in a local RefPtr and only last for the duration of a function call. ==== One additional tangent here: from a hg-archeology perspective, it's confusing that a comment *in this patch* is talking about SVGImageContext as if it owns instances of this class. This is confusing because SVGImageContext does NOT own instances of this class -- not until several patches later [part 12?]. To make this clearer to us & other-hg-archeologists in the future, it'd be nice to drop some sort of breadcrumb about this here, which you then remove in part 12. E.g. perhaps add a note like this at the bottom of this comment: XXX Note: SVGImageContext doesn't actually have a SVGContextPaint member yet, but it will in a later patch in this series. ...and then remove that in comment 12.

Comment on attachment 8828475 [details] [diff] [review] part 9 - Make AutoSetRestoreSVGContextPaint::Init's SVGContextPaint argument const Review of attachment 8828475 [details] [diff] [review]: ----------------------------------------------------------------- Per comment 39, I'm not sure this "Init" method should exist in the first place. But I think this patch still makes sense, as applied to the constructor that Init() supplanted. Anyway, I'll r- this patch for now, since it'll likely look different after you've rebased for comment 39 (assuming I'm not mistaken there), and it'll be trivial to re-review after that point. ::: layout/svg/SVGContextPaint.cpp @@ +144,5 @@ > > + // XXX The SVGContextPaint that was passed in to SetProperty was const. > + // Ideally we could and should re-apply that constness to the > + // SVGContextPaint we get here (SVGImageContext is never changed after it is > + // initialized). , Unfortunately lazy initialization of SVGContextPaint You've got a stray comma near the beginning of this line.

Comment on attachment 8828476 [details] [diff] [review] part 10 - Make SVGImageContext's ctor's aViewportSize argument optional Review of attachment 8828476 [details] [diff] [review]: ----------------------------------------------------------------- Can you provide more context (ideally in the extended commit message, though here as well) about *why* we're making SVGImageContext's aViewportSize arg optional in this patch? IIRC, it's one of the main pieces of context we need for drawing SVG-as-an-image -- so that we can resolve percent sizes, honor "viewBox", etc. I'm not clear on what circumstances we're allowing it to be unspecified (and what that means for painting with that SVGImageContext).

Comment on attachment 8828477 [details] [diff] [review] part 11 - Add an SVGContextPaint subclass for inheriting context paint into SVG-as-an-image Review of attachment 8828477 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits addressed: ::: layout/svg/SVGContextPaint.h @@ +213,5 @@ > }; > > +/** > + * This class is for use when an element draws an SVG image (by embedding it, > + * or by referencing it from a CSS property such as 'background-image') and The phrase "embedding it" (first line here) is slightly problematic, because it sounds like you're talking about <embed>. (And you're not -- <embed> does *not* use the SVG-as-an-image codepath.) How about we start that parenthetical out like so: "(e.g. in HTML <img> or SVG <image>, or by referencing ...etc...") @@ +229,5 @@ > + void SetStroke(Color aStroke); > + > + virtual already_AddRefed<gfxPattern> GetFillPattern(const DrawTarget* aDrawTarget, > + float aOpacity, > + const gfxMatrix& aCTM) override { Please drop the "virtual" keyword here & on all methods in this class. The coding style guide says "use *at most* one of the following keywords: virtual, override, or final" (emphasis added), in part because it's redundant to combine 'virtual' with 'override'. (We have a lot of old code that doesn't adhere to this yet, but we should honor it in new code as best we can.) https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Methods @@ +240,5 @@ > + return do_AddRef(mStroke); > + } > + > + virtual float GetFillOpacity() const override { > + // Always 1.0f since we don't currently allow 'context-fill-opacity' Please copy this comment the GetStrokeOpacity impl below this (with s/fill/stroke/ in the comment text), if the same explanation applies there. @@ +252,5 @@ > + virtual uint32_t Hash() const override; > + > +private: > + RefPtr<gfxPattern> mFill; > + RefPtr<gfxPattern> mStroke; Consider adding a comment above these member-vars, saying e.g. // Note: if these are set at all, they'll have type PatternType::COLOR. Otherwise, these members (which look like paint servers) superficially seem to contradict the final line of documentation for this class, where you say we "only support context colors and not paint servers".

Comment on attachment 8828478 [details] [diff] [review] part 12 - Allow SVGImageContext to store an SVGContextPaint and have VectorImage propagate it Review of attachment 8828478 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/VectorImage.cpp @@ +881,5 @@ > return DrawResult::SUCCESS; > } > > + AutoSetRestoreSVGContextPaint autoContextPaint; > + if (aSVGContext.isSome() && s/.isSome()// (Just treat Maybe<> variables as if they're pointers, including for "null-check"-equivalent operations) ::: layout/svg/SVGImageContext.cpp @@ +18,5 @@ > +SVGImageContext::MaybeStoreContextPaint(nsIFrame* aFromFrame) > +{ > + // If the embedding frame is not chrome, disallow unless the pref is enabled. > + if (!aFromFrame->PresContext()->IsChrome() && > + !Preferences::GetBool("svg.context-properties.content.enabled", false)) { For symmetry & the ability to quickly disable this feature, perhaps we should have an equivalent property for chrome code? (e.g. "svg.context-properties.chrome.enabled", instead of "content.enabled") (And we'd check one pref or the other, depending on whether IsChrome() returns true.) Also: I assume this happens during paint, and we probably don't want to be doing pref lookups while we're painting, for perf reasons. Could you add a local static boolean variable cache for this pref here? @@ +28,5 @@ > + RefPtr<SVGEmbeddingContextPaint> contextPaint = new SVGEmbeddingContextPaint(); > + > + const nsStyleSVG* style = aFromFrame->StyleSVG(); > + > + // XXX don't set values for properties not listed in 'context-properties'. There are 3 XXX comments in this function -- it looks like maybe you were intending to address those before landing...? (though I'm not sure) Or maybe in a future patch on this bug, per comment 37? If they're being addressed here, great! If they're destined for a followup bug, then please file that followup bug & mention the bug number here. @@ +37,5 @@ > + if (style->mStroke.Type() == eStyleSVGPaintType_Color) { > + contextPaint->SetStroke(ToDeviceColor(style->mStroke.GetColor())); > + } > + > + // XXX don't set mContextPaint if nothing was set on contextPaint. (This comment, at least, seems like it'd be pretty simple to address in this patch, I think; no need for a followup for this one.) ::: layout/svg/SVGImageContext.h @@ +85,5 @@ > + mPreserveAspectRatio.map(HashPAR).valueOr(0), > + HashBytes(&mGlobalOpacity, sizeof(gfxFloat)), > + mIsPaintingSVGImageElement); > + if (mContextPaint) { > + hash = HashGeneric(hash, mContextPaint->Hash()); Nit: in the interests of being able to verify that all member-vars are indeed being hashed, it might be best to hash the member variables in order here. So since mContextPaint is the first-listed member var (as of this patch), it should be the first thing to be hashed as well. So, this should probably be: uint32_t hash = 0; if (mContextPaint) { hash = HashGeneric(hash, ...); } return HashGeneric(hash, mViewportSize.map(HashSize).valueOr(0), ...);

(In reply to Daniel Holbert [:dholbert] from comment #43) > Can you provide more context (ideally in the extended commit message, though > here as well) about *why* we're making SVGImageContext's aViewportSize arg > optional in this patch? > > IIRC, it's one of the main pieces of context we need for drawing > SVG-as-an-image -- so that we can resolve percent sizes, honor "viewBox", > etc. I'm not clear on what circumstances we're allowing it to be > unspecified (and what that means for painting with that SVGImageContext). The SVGImageContext itself is optional. I was basically trying to keep that aspect of the code constant (i.e. the paint path from nsImageBoxFrame won't provide a viewportSize). It does seem like we should be providing this though (maybe our limited use in this case means we haven't encountered the bugs) so I'll do that and drop this patch.

(In reply to Daniel Holbert [:dholbert] from comment #45) > For symmetry & the ability to quickly disable this feature, perhaps we > should have an equivalent property for chrome code? I don't think a pref makes sense in the chrome case. Flipping such a pref would make all UI icons that use this feature render blank. If we want to stop using the feature we're going to need to make more invasive changes to actually change the icon files to avoid breaking the UI. > Also: I assume this happens during paint, and we probably don't want to be > doing pref lookups while we're painting, for perf reasons. Could you add a > local static boolean variable cache for this pref here? It would be one hash table lookup per image instance so not a big hit, but sure. > There are 3 XXX comments in this function -- it looks like maybe you were > intending to address those before landing...? (though I'm not sure) Yes, addressed in the patch to add |context-properties|.

Comment on attachment 8832440 [details] [diff] [review] part 11 - Have nsImageBoxFrame pass context paint to VectorImage Cancelling review on this patch since I've reworked it to use Maybe. That depends on my fix to bug 1335780 though. I'll upload the new patch shortly.

Comment on attachment 8832400 [details] [diff] [review] part 8 - Make AutoSetRestoreSVGContextPaint::Init's SVGContextPaint argument const Review of attachment 8832400 [details] [diff] [review]: ----------------------------------------------------------------- I'll admit to not fully-grokking why this const hackery is needed yet, but I'll provisionally r+ it and circle back if I have reservations when reviewing the later patch that needs it.

er, sorry! ignore comment 58 - I was looking at the wrong version of the patch somehow. Those comments were indeed addressed.

Comment on attachment 8832402 [details] [diff] [review] part 9 - Add an SVGContextPaint subclass for inheriting context paint into SVG-as-an-image. r=dholbert Review of attachment 8832402 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/svg/SVGContextPaint.cpp @@ +280,5 @@ > + > +void > +SVGEmbeddingContextPaint::SetFill(Color aFill) > +{ > + mFill = new gfxPattern(aFill); Could you change these SetFill/SetStroke functions to accept a "nscolor" arg (instead of gfx::Color), and just convert to gfx::Color internally? Because: (1) This is a SVGContextPaint subclass, and it looks like other SVGContextPaint APIs deal with nscolor rather than gfx::Color. (e.g. "SetColor" in https://bugzilla.mozilla.org/attachment.cgi?id=8777695&action=diff ) (2) I believe all the callsites for these functions (added later in part 10) are dealing with nscolor values -- so they have to awkwardly convert to Color to satisfy this signature, like so: contextPaint->SetFill(ToDeviceColor(style->mFill.GetColor())); This callsite would be easier to read if we could drop the ToDeviceColor(...) cruft at the callsites (and bury it as an implementation detail inside of these setters. So this would improve SVGContextPaint API consistency per (1), and it would let the callsites be cleaner per (2).

Comment on attachment 8832403 [details] [diff] [review] part 10 - Allow SVGImageContext to store an SVGContextPaint and have VectorImage propagate it Review of attachment 8832403 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/svg/SVGImageContext.cpp @@ +27,5 @@ > + } > + > + if (!sEnabledForContent && > + !aFromFrame->PresContext()->IsChrome()) { > + return false; Your previous version of this patch had a code-comment here that was quite helpful in explaining the intent here, IMO -- could you add back some version of that comment? (This logic is hard to immediately grok without that intent-hint, for me at least, due to the compound negation.) Maybe something like: // Bail if this is a content doc & the feature is preffed off for content. @@ +30,5 @@ > + !aFromFrame->PresContext()->IsChrome()) { > + return false; > + } > + > + // XXX return early if the 'context-properties' property is not set. (You said above this will be "addressed in the patch to add |context-properties|". I assume you're referring to a later patch on this bug, but I'm not seeing it here. I suspect it's still coming?) @@ +42,5 @@ > + // XXX don't set values for properties not listed in 'context-properties'. > + > + if (style->mFill.Type() == eStyleSVGPaintType_Color) { > + haveContextPaint = true; > + contextPaint->SetFill(ToDeviceColor(style->mFill.GetColor())); Per comment 60, I think we should push the ToDeviceColor() conversion down into the SetFill/SetStroke implementations. So hopefully we won't need it here anymore. ::: layout/svg/SVGImageContext.h @@ +90,4 @@ > mViewportSize.height, > mPreserveAspectRatio.map(HashPAR).valueOr(0), > + HashBytes(&mGlobalOpacity, > + sizeof(decltype(mGlobalOpacity))), I think you can just use "sizeof(mGlobalOpacity)" here, right? (mGlobalOpacity has type gfxFloat, and the old code uses "sizeof(gfxFloat)" and you're fixing that to replace gfxFloat with decltype(mGlobalOpacity) as a tangential improvement here -- but I think you can just as easily/correctly drop the decltype() wrapper.)

Comment on attachment 8832652 [details] [diff] [review] part 11 - Feed a Maybe<SVGImageContext> through to DrawImageInternal Review of attachment 8832652 [details] [diff] [review]: ----------------------------------------------------------------- Commit message nit: > Bug 1058040, part 11 - Feed a Maybe<SVGImageContext> through to DrawImageInternal. r=dholbert This sounds like you're adding a *new* arg to DrawImageInternal. But really you're just changing the type of an existing arg. how about something like: Bug 1058040, part 11 - Convert DrawImage/DrawImageInternal's SVGImageContext param from pointer to Maybe<>. r=dholbert r=me regardless, with one nsLayoutUtils.h nit addressed: ::: layout/base/nsLayoutUtils.h @@ +1859,5 @@ > * @param aDirty Pixels outside this area may be skipped. > + * @param aSVGContext Optionally provides an SVGImageContext. > + * Callers should pass an SVGImageContext with at > + * least the viewport size set if aImage is of > + * type imgIContainer::TYPE_VECTOR, or pass This recommendation ("callers should set...") is perhaps a bit too prescriptive. It sounds like it's *requiring* that callers must check imgIContainer::GetType (a virtual function) in order to decide what they should pass for this parameter. And that's not really a requirement we need to make. (And it might actually be slower to make the virtual GetType function call than to simply construct a temporary SVGImageContext.) I'd walk this back a bit and say: s/if aImage is of type...VECTOR/if aImage might be...VECTOR/ ("is of type" --> "might be of type") Something like that -- to make it clearer that, if a caller *doesn't know* the image's type, it doesn't necessarily need to check. (It can just err on the safe side and create a temporary SVGImageContext.)

Comment on attachment 8832653 [details] [diff] [review] part 13 - Have nsImageBoxFrame pass context paint to VectorImage Review of attachment 8832653 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/xul/nsImageBoxFrame.cpp @@ +408,5 @@ > > + Maybe<SVGImageContext> svgContext; > + if (imgCon->GetType() == imgIContainer::TYPE_VECTOR) { > + // We avoid this overhead for raster images > + svgContext.emplace(CSSPixel::FromAppUnitsRounded(dest.Size())); (1) nit: add period at the end of the comment. (2) Observation (probably no action needed): I'm a little uneasy about the fact that we'll now be skipping all the complicated nsLayoutUtils math for doing pixel-snapping/etc. to compute the correct default viewport size. I'll bet this FromAppUnitsRounded() call might produce subtly different behavior in some cases, though I'm not sure if it'll be noticeable. So I guess this is fine for now -- but we might need to circle back and create a "SVGContext-with-lazily-initialized-viewport" concept at some point, for use here, and let nsLayoutUtils fill in the viewport.

(In reply to Daniel Holbert [:dholbert] from comment #61) > (You said above this will be "addressed in the patch to add > |context-properties|". I assume you're referring to a later patch on this > bug, but I'm not seeing it here. I suspect it's still coming?) That's correct, it will be a patch on this bug. Sorry that wasn't clear.

Pushed by jwatt@jwatt.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/b6959c93a4f7 part 6 - When copying SVGImageContexts use the copy ctor. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/d3db704e690e part 7 - Make SVGContextPaint a ref counted class. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/244eaae6e4d6 part 8 - Make AutoSetRestoreSVGContextPaint::Init's SVGContextPaint argument const. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/2f5e1de5444a part 9 - Add an SVGContextPaint subclass for inheriting context paint into SVG-as-an-image. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/8c98153f74aa part 10 - Allow SVGImageContext to store an SVGContextPaint and have VectorImage propagate it. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/76ab9a7a714a part 11 - Convert DrawImage/DrawImageInternal's SVGImageContext param from pointer to Maybe<>. r=dholbert

I've pushed some of the patches to make it easier for shorlander to run his tests. However, I haven't pushed "part 12 - Have nsImageBoxFrame pass context paint to VectorImage" since after addressing the review comments in comment 43 things can break in some cases. (In reply to Jonathan Watt [:jwatt] from comment #46) > (In reply to Daniel Holbert [:dholbert] from comment #43) > > Can you provide more context (ideally in the extended commit message, though > > here as well) about *why* we're making SVGImageContext's aViewportSize arg > > optional in this patch? > > > > IIRC, it's one of the main pieces of context we need for drawing > > SVG-as-an-image -- so that we can resolve percent sizes, honor "viewBox", > > etc. I'm not clear on what circumstances we're allowing it to be > > unspecified (and what that means for painting with that SVGImageContext). > > The SVGImageContext itself is optional. I was basically trying to keep that > aspect of the code constant (i.e. the paint path from nsImageBoxFrame won't > provide a viewportSize). It does seem like we should be providing this > though (maybe our limited use in this case means we haven't encountered the > bugs) so I'll do that and drop this patch. Before addressing this review comment we would end up using ComputeSnappedImageDrawingParameters when an SVGImageContext (and thus the "viewport size") were not provided. The current code in attachment 8832653 [details] [diff] [review] to use CSSPixel::FromAppUnitsRounded(dest.Size()) does not give the same results and breaks -moz-image-region. This might not matter to shorlander if he's replacing the code that uses -moz-image-region, but there are other parts of the code that still use -moz-image-region so this needs a proper fix.

https://hg.mozilla.org/mozilla-central/rev/b6959c93a4f7 https://hg.mozilla.org/mozilla-central/rev/d3db704e690e https://hg.mozilla.org/mozilla-central/rev/244eaae6e4d6 https://hg.mozilla.org/mozilla-central/rev/2f5e1de5444a https://hg.mozilla.org/mozilla-central/rev/8c98153f74aa https://hg.mozilla.org/mozilla-central/rev/76ab9a7a714a

(In reply to Daniel Holbert [:dholbert] from comment #63) > we might need to circle back and create a > "SVGContext-with-lazily-initialized-viewport" concept at some point, for use > here, and let nsLayoutUtils fill in the viewport. I should also have mentioned in comment 66 that this is basically what the patch to make SVGImageContext's aViewportSize argument optional did (until I dropped that patch in comment 46). Anyway, I've been trying various ways to fix this without resurrecting that patch, but the pixel snapping code in nsLayoutUtils and elsewhere is gnarly and buggy. Even my local "fixes" to parts of that code cause weird failures in UI tests. Sometimes that seems to be because tests are working around those bugs, but in most cases it seems to be because I need to fix other (unclear) places to be compatible, or perhaps because I simply don't understand all the layers of this code well enough. Either way I've spent way more time than is justified to try and get things working in a nicer way. Instead I've resurrected the patch to make SVGImageContext's aViewportSize argument optional.

Update to tip.

Update to tip.

Comment on attachment 8839508 [details] [diff] [review] part 12 - Make SVGImageContext's ctor's aViewportSize parameter optional Review of attachment 8839508 [details] [diff] [review]: ----------------------------------------------------------------- r=me, with one nit: ::: image/VectorImage.cpp @@ +846,5 @@ > > AutoRestore<bool> autoRestoreIsDrawing(mIsDrawing); > mIsDrawing = true; > > + // If FLAG_FORCE_PRESERVEASPECTRATIO_NONE bit is set, that means we should All the tweaks in VectorImage::Draw (i.e. all changes to this file from here on down) seem like an in-place refactoring that doesn't affect behavior and isn't related to the rest of this patch. (No mentions of the viewportsize here.) The changes seem fine, but they probably belong in their own separate patch, to keep the bulk of this patch focused on what it's really doing. rs=me on that as its own separate patch, assuming you agree & I'm not missing something.

(In reply to Daniel Holbert [:dholbert] from comment #72) > All the tweaks in VectorImage::Draw (i.e. all changes to this file from here > on down) seem like an in-place refactoring that doesn't affect behavior and > isn't related to the rest of this patch. I'll file a separate bug and assume your r+ there.

Pushed by jwatt@jwatt.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/588c44c7a966 part 12 - Make SVGImageContext's ctor's aViewportSize parameter optional. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/e77029e04814 part 13 - Have nsImageBoxFrame pass context paint to VectorImage. r=dholbert

Sorry had to back this out for valgrind bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=80996169&repo=mozilla-inbound&lineNumber=23194 https://hg.mozilla.org/integration/mozilla-inbound/rev/296647884376 https://hg.mozilla.org/integration/mozilla-inbound/rev/8c7dbec36fa7

(In reply to Jonathan Watt [:jwatt] from comment #73) > (In reply to Daniel Holbert [:dholbert] from comment #72) > > All the tweaks in VectorImage::Draw (i.e. all changes to this file from here > > on down) seem like an in-place refactoring that doesn't affect behavior and > > isn't related to the rest of this patch. > > I'll file a separate bug and assume your r+ there. Fine by me! thanks.

Comment on attachment 8845174 [details] [diff] [review] part 13 - Fix an uninitialized memory bug in SVGImageContext's default ctor This fixes the Valgrind failures, and was a real pain to debug. The error Valgrind reported was under: PLDHashTable::EntryIsFree PLDHashTable::SearchTable PLDHashTable::Add nsTHashtable<nsBaseHashtableET<nsGenericHashKey<SurfaceKey>, RefPtr<CachedSurface>>>::PutEntry nsBaseHashtable<nsGenericHashKey<SurfaceKey>, RefPtr<CachedSurface>, CachedSurface*>::Put nsBaseHashtable<nsGenericHashKey<SurfaceKey>, RefPtr<CachedSurface>, CachedSurface*>::Put ImageSurfaceCache::Insert SurfaceCacheImpl::Insert SurfaceCache::Insert VectorImage::CreateSurfaceAndShow VectorImage::Draw ClippedImage::DrawSingleTile ClippedImage::Draw DrawImageInternal nsLayoutUtils::DrawSingleImage nsImageBoxFrame::PaintImage I traced the memory that it was accessing to where it was allocated and convinced myself that this memory was always memset to zero and therefore valid, so this Valgrind made no sense for quite a while. It turns out that I was right, and the issue is actually that *which* aEntry is passed to EntryIsFree depends on uninitialized memory. Unfortunately there was a rather long and indirect chain of calculations (all with *unreported*, undefined values!) that lead up to the eventual error report in a far off unrelated part of the code. Specifically what happens is that aEntry depends on the value that was computed by the Hash1() call in PLDHashTable::SearchTable, which depends on the ComputeKeyHash() call in PLDHashTable::Search, which depends on the aProvider->GetImageKey() call in SurfaceCacheImpl::Insert, which depends on the VectorSurfaceKey() call in VectorImage::CreateSurfaceAndShow, which depends on the SVGImageContent::Hash call under that, which depends on the SVGImageContent's mIsPaintingSVGImageElement, which is uninitialized because when we create the Maybe<SVGImageContent> in nsImageBoxFrame::PaintImage it uses the SVGImageContent default ctor which has a bug. (Big thanks to jseward for putting me on the right path with regards to Valgrind's behavior.)

(In reply to Daniel Holbert [:dholbert] from comment #39) > Comment on attachment 8828473 [details] [diff] [review] > part 7 - Support lazy initialization of AutoSetRestoreSVGContextPaint > > Can we achieve the same result (with better separation-of-concerns) by > composing Maybe<> with the existing AutoSetRestoreSVGContextPaint class (and > only doing that in whichever places need this lazy behavior)? Of course now that we've done that, it occurs to me that this approach is less than ideal since we would want double emplace() to assert, which it won't. :/

Comment on attachment 8845174 [details] [diff] [review] part 13 - Fix an uninitialized memory bug in SVGImageContext's default ctor Review of attachment 8845174 [details] [diff] [review]: ----------------------------------------------------------------- r=me

Comment on attachment 8845176 [details] [diff] [review] part 10b - Account for the new SVGContextPaint member in SVGImageContext::operator== Review of attachment 8845176 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/svg/SVGContextPaint.cpp @@ +312,5 @@ > +SVGEmbeddingContextPaint::operator==(const SVGContextPaint& aOther) const > +{ > + // It would be nice to compare the paint directly, but for that we'd need > + // to be able to cast aOther to SVGEmbeddingContextPaint. > + return Hash() == aOther.Hash(); Per IRC discussion, Hash() comparisons aren't a reliable way of determining equality. (there can totally be collisions) So this seems like it could return "true" even when the two things aren't equal.

Pushed by jwatt@jwatt.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/db0ae4feb40a part 12 - Make SVGImageContext's ctor's aViewportSize parameter optional. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/62859e9f131a part 13 - Fix an uninitialized memory bug in SVGImageContext's default ctor. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/f622c2236747 part 14 - Have nsImageBoxFrame pass context paint to VectorImage. r=dholbert

https://hg.mozilla.org/mozilla-central/rev/db0ae4feb40a https://hg.mozilla.org/mozilla-central/rev/62859e9f131a https://hg.mozilla.org/mozilla-central/rev/f622c2236747

Hey jwatt, lots of impressive work in here - but (as an outsider to this region of the code) it's hard for me to get a sense of what's remaining before we can close this one out. Are we close on this one?

Or maybe dholbert can weigh in on comment 85?

I chatted with jwatt yesterday about fill-opacity not working in combination with context-fill, he said he'd look into it.

Comment on attachment 8832402 [details] [diff] [review] part 9 - Add an SVGContextPaint subclass for inheriting context paint into SVG-as-an-image. r=dholbert >+public: >+ SVGEmbeddingContextPaint() {} >+ >+ void SetFill(Color aFill); >+ void SetStroke(Color aStroke); >+ >+ already_AddRefed<gfxPattern> GetFillPattern(const DrawTarget* aDrawTarget, >+ float aOpacity, >+ const gfxMatrix& aCTM) override { >+ return do_AddRef(mFill); >+ } >+ >+ already_AddRefed<gfxPattern> GetStrokePattern(const DrawTarget* aDrawTarget, >+ float aOpacity, >+ const gfxMatrix& aCTM) override { >+ return do_AddRef(mStroke); >+ } >+ >+ float GetFillOpacity() const override { >+ // Always 1.0f since we don't currently allow 'context-fill-opacity' >+ return 1.0f; This might be relevant, maybe? Just to be clear again: I wasn't trying to use context-fill-opacity, just fill-opacity:x directly in the SVG but in combination with fill: context-fill.

(In reply to Mike Conley (:mconley) (Offsite until March 27) from comment #85) > it's hard for me to get a sense of what's remaining > before we can close this one out. Are we close on this one? Yeah, sorry. You guys can use this functionality already (shorlander already is over in bug 1347543). There are some caveats though: 1) Only nsImageBoxFrame currently passes through context color. If you're trying to use this functionality in a way that doesn't use nsImageBoxFrame (which, if it doesn't work for you, will almost certainly be what's happening) then please fill a follow-up bug. It's trivial to pass the context properties through, but there are a lot of places where we might want to. 2) In bug 1350010 I plan to require the context element to have a new property called 'context-properties' set on it, listing which properties should be exposed to the SVG image. Wherever you set 'fill' and 'stroke' in your CSS with the intend of using it in your SVG image you should set this property too so that your code doesn't break if/when we do that. 3) As Dao says, there's the issue that when you use the 'context-fill' value it causes the 'fill-opacity' property on the element to be ignored. I've filed bug 1350015 for that. As to what needs to happen for me to close this, I just need to figure out a better way to do part 10b, and I should write a test or two. (Although I may leave the tests for bug 1350010.)

(In reply to Dão Gottwald [::dao] from comment #88) > >+ float GetFillOpacity() const override { > >+ // Always 1.0f since we don't currently allow 'context-fill-opacity' > >+ return 1.0f; > > This might be relevant, maybe? Just to be clear again: I wasn't trying to > use context-fill-opacity Not to your issue, no. This function is only called if you *do* use 'context-fill-opacity' which gets the value from this context object. (Since I haven't implemented the pass through of the 'fill-opacity' value from the context, this has nothing sensible to return.)

Comment on attachment 8851580 [details] [diff] [review] part 10b - Account for the SVGContextPaint member in SVGImageContext::operator== Review of attachment 8851580 [details] [diff] [review]: ----------------------------------------------------------------- The SVGImageContext.h changes look good, but I'm not sure about the SVGEmbeddingContextPaint changes. ::: layout/svg/SVGContextPaint.h @@ +225,5 @@ > public: > SVGEmbeddingContextPaint() {} > > + bool operator==(const SVGEmbeddingContextPaint& aOther) const { > + return mFill == aOther.mFill && mStroke == aOther.mStroke; This implementation is only comparing two of this class's 5 member vars. It inherits 3 member-vars from its superclass (from SVGContextPaint: mDashes, mDashOffset, mStrokeWidth), and it seems like a straightforward operator==() implementation should either compare *all* of the member-vars, or should include a comment explaining why it's excluding some specific ones. (Note that the inherited member-vars are declared as "private" in the superclass, so technically we can't see them; though if we need to, we could just change that to "protected".) Anyway -- please consider whether the inherited members need to be compared here. And either add them, or add a comment explaining why we're excluding them from equality consideration.

Comment on attachment 8850795 [details] [diff] [review] part 15 - Have nsImageFrame pass context paint to VectorImage Review of attachment 8850795 [details] [diff] [review]: ----------------------------------------------------------------- r=me though you should probably either file a followup or add another patch here to make the following optimization: ::: layout/generic/nsImageFrame.cpp @@ +1701,5 @@ > + Maybe<SVGImageContext> svgContext; > + if (aImage->GetType() == imgIContainer::TYPE_VECTOR) { > + // We avoid this overhead for raster images. > + svgContext.emplace(); > + svgContext->MaybeStoreContextPaint(this); Hmm -- so this is adding a new virtual function call to all PaintImage() invocations, and we happen to know it's entirely unnecessary for the content process for actual users (since MaybeStoreContextPaint will be a no-op). As an optimization, we might want to wrap blocks like this in an additional cheaper layer of "if"-check -- something like: if (SVGImageContext::IsContextPaintSupported()) { ... } ...which would just read the result of the bool-var-cache lookups. That would let us skip the virtual imgIContainer::GetType() function call in the extremely-common case.

(In reply to Daniel Holbert [:dholbert] from comment #94) > > + bool operator==(const SVGEmbeddingContextPaint& aOther) const { > > + return mFill == aOther.mFill && mStroke == aOther.mStroke; > > This implementation is only comparing two of this class's 5 member vars. We (currently at least) don't pass those through as part of the context information from an image context so we know we don't need to compare them. As discussed on IRC, I'll assert that they're always equal in debug builds. I needed to mark a few SVGContextPaint methods as const to do that.

Comment on attachment 8851647 [details] [diff] [review] part 17 - Add a bunch of SVG-as-an-image context paint reftests. Review of attachment 8851647 [details] [diff] [review]: ----------------------------------------------------------------- ::: modules/libpref/init/all.js @@ +2972,5 @@ > > pref("svg.transform-box.enabled", true); > > +# This feature is currently not part of any spec. > +pref("svg.context-properties.content.enabled", false); I needed to add this due to bug 1350948. (Maybe I should make it a separate commit?)

(In reply to Daniel Holbert [:dholbert] from comment #95) > Hmm -- so this is adding a new virtual function call to all PaintImage() > invocations, and we happen to know it's entirely unnecessary for the content > process for actual users (since MaybeStoreContextPaint will be a no-op). Given the amount of work done under PaintImage() is that something to worry about?

Comment on attachment 8851647 [details] [diff] [review] part 17 - Add a bunch of SVG-as-an-image context paint reftests. Review of attachment 8851647 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the tests, with feedback addressed. I'm happy to take another look at the final patch if you like, too -- I'll leave that up to you. Commit message nit: > Bug 1058040, part 16 - Add a bunch of SVG-as-an-image context paint reftests. r=dholbert This patch isn't just tests -- it also makes an all.js tweak (to explicitly disable an already-implicitly-disabled feature). For full disclosure, the commit message should hint at that. So maybe add to the end: "& explicitly pref off context paint for web content" ::: layout/reftests/svg/as-image/context-fill-01.html @@ +12,5 @@ > + > + </style> > + </head> > + <body> > + <img src="data:image/svg+xml,<svg xmlns='http://www.w3.org/2000/svg'><rect width='100%' height='100%' fill='context-fill red'/></svg>"> s/red/blue/ here, since I think we'll want to load this testcase in two different modes. (See my review feedback for reftest.list) ::: layout/reftests/svg/as-image/context-fill-02.html @@ +1,4 @@ > +<!DOCTYPE html> > +<html> > + <head> > + <title>Test context-fill where fill is transparent</title> s/transparent/semi-transparent/ ("transparent" is an actual color keyword, i.e. the name of an actual CSS color which would be valid as a value for the "fill" property. But it's not the value you're actually using for "fill" here. So, better to not use that term ambiguously here.) ::: layout/reftests/svg/as-image/context-fill-04.html @@ +1,4 @@ > +<!DOCTYPE html> > +<html> > + <head> > + <title>Test context-fill with different fill values (test image caching correct)</title> The parenthesized clause, "test image caching correct", is grammatically awkward. (Brevity is good, but too many words are missing from this sentence for it to make sense.) Maybe s/correct/correctness/, I think? Or s/correct/is correct/? ::: layout/reftests/svg/as-image/context-stroke-02.html @@ +1,4 @@ > +<!DOCTYPE html> > +<html> > + <head> > + <title>Test context-stroke where stroke is transparent</title> As above: s/transparent/semi-transparent/ ::: layout/reftests/svg/as-image/context-stroke-04.html @@ +1,4 @@ > +<!DOCTYPE html> > +<html> > + <head> > + <title>Test context-stroke with different stroke values (test image caching correct)</title> As above: s/correct/correctness/ (or "is correct") ::: layout/reftests/svg/as-image/context-stroke-05.html @@ +1,4 @@ > +<!DOCTYPE html> > +<html> > + <head> > + <title>Test context-stroke works as a fill value</title> This test does not currently do what the title says it does. Needs tweaking. (Read on...) @@ +6,5 @@ > + > +img { > + width: 100px; > + height: 100px; > + fill: lime; I think this should be "stroke", not "fill" @@ +12,5 @@ > + > + </style> > + </head> > + <body> > + <img src="data:image/svg+xml,<svg xmlns='http://www.w3.org/2000/svg'><rect width='100%' height='100%' fill='context-fill red'/></svg>"> And this should have "context-stroke", not "context-fill". ::: layout/reftests/svg/as-image/reftest.list @@ +56,5 @@ > > +# Context paint tests (this feature is currently not part of any spec.) > +pref(svg.context-properties.content.enabled,true) == context-fill-01.html lime100x100-ref.html > +pref(svg.context-properties.content.enabled,true) == context-fill-02.html lime100x100-50pct-ref.html > +pref(svg.context-properties.content.enabled,true) == context-fill-03.html lime100x100-50pct-ref.html Technically these should all use "test-pref", not "pref", since you only need the feature enabled for the testcase (not for the reference case). @@ +63,5 @@ > +pref(svg.context-properties.content.enabled,true) == context-stroke-01.html lime100x100-ref.html > +pref(svg.context-properties.content.enabled,true) == context-stroke-02.html lime100x100-50pct-ref.html > +pref(svg.context-properties.content.enabled,true) == context-stroke-03.html lime100x100-50pct-ref.html > +pref(svg.context-properties.content.enabled,true) == context-stroke-04.html context-fill-or-stroke-04-ref.html > +pref(svg.context-properties.content.enabled,true) == context-stroke-05.html lime100x100-ref.html Three more things that you should test here: (1) What happens when this feature is disabled? (Put another more-important way: can we prove/assert that this feature *is in fact disabled*, using a reftest? * Suggested way of testing this: - Add an additional line for context-fill-01.html to reftest.list, but WITHOUT the pref/test-pref annotation. - Change the test's fallback color to "blue" instead of "red", so that it's got less of a "fail" stigma when we render with that color (as we should when the pref isn't set). - Create a reference case blue-100x100-ref.html to use for this scenario with the pref disabled (and the new reftest.list line) (2) What happens when the embedding page doesn't specify a value for "fill"? * Suggested way of testing this: - add another <img> to context-fill-04.html, WITHOUT any explicit style rule to set its "fill". - I suspect that <img> will render entirely blank (since it'll be passing through a context-fill of "none"). So maybe add a border to the img elements (and the divs in the reference case) to make it clearer that there is actually an element there whose rendering we're comparing. (3) What happens when the embedded content just uses "context-fill" *without* providing a fallback color? (Please test this both when the feature is enabled -- in which case the value should still make it through I think -- and *also* test it when the feature is disabled, in which case we fall back... to what?) In all of the above, I'm discussing "context-fill" but we should test them for "context-stroke" as well. ::: modules/libpref/init/all.js @@ +2972,5 @@ > > pref("svg.transform-box.enabled", true); > > +# This feature is currently not part of any spec. > +pref("svg.context-properties.content.enabled", false); This comment should start with a line that explains what the pref is doing, like we do for most other CSS/layout features in this file. And the "not part of any spec" comment needs a bit more explanation about what point it's trying to get across (I think it's intending to explain why the pref should probably stay "false", yes?) Maybe replace the comment with something like: # Are the context-fill & context-stroke keywords enabled in content processes? # (These keywords are enabled in chrome processes, regardless of this pref. # Also, these keywords are currently not part of any spec, which is partly why # we disable them for web content.)

(In reply to Jonathan Watt [:jwatt] from comment #98) > (In reply to Daniel Holbert [:dholbert] from comment #95) > > Hmm -- so this is adding a new virtual function call to all PaintImage() > > invocations, and we happen to know it's entirely unnecessary for the content > > process for actual users > > Given the amount of work done under PaintImage() is that something to worry > about? Maybe not? I'm not sure. My perspective is: - Painting is a performance-critical codepath (though perhaps already-slow). - This patch is adding a call with a known (small) perf cost to that codepath -- a virtual function call. - And, we *know* we *actually don't need to make that call* 99.9% of the time. (100% of the time, in content processes for actual users.) - And, we could easily spin out a helper-function (the first 1/3 of SVGImageContext::MaybeStoreContextPaint) to let us avoid making this call in cases where we can tell up-front that it's unnecessary. (And that helper-function would be cheaper and branch-predictable, since it'd just be checking a cached bool and are-we-content-process, which I expect is effectively also a trivial bool-check). Anyway, I'm happy punting on it for now, and perhaps treating it as a low-priority mentored bug. Seems trivial & likely-to-take-effect enough to be a worthwhile optimization to make, though, IMO.

Comment on attachment 8851733 [details] [diff] [review] part 10b - Account for the new SVGContextPaint member in SVGImageContext::operator== Review of attachment 8851733 [details] [diff] [review]: ----------------------------------------------------------------- r=me

I wonder what you think about maybe doing it this way.

Comment on attachment 8851775 [details] [diff] [review] part 18 - Minimize the cost of context paint when it is not available Review of attachment 8851775 [details] [diff] [review]: ----------------------------------------------------------------- r=me, with two nits: ::: layout/svg/SVGImageContext.cpp @@ +22,5 @@ > { > static bool sEnabledForContent = false; > static bool sEnabledForContentCached = false; > > + MOZ_ASSERT(!aContext, "The emplace() call below with overwrite this object"); s/with/will/ @@ +32,5 @@ > } > > if (!sEnabledForContent && > + (XRE_IsContentProcess() || > + !aFromFrame->PresContext()->IsChrome())) { Per IRC, I don't think we need to add this XRE_IsContentProcess() call.

(In reply to Daniel Holbert [:dholbert] from comment #99) > This patch isn't just tests -- it also makes an all.js tweak (to explicitly > disable an already-implicitly-disabled feature). For full disclosure, the > commit message should hint at that. So maybe add to the end: "& explicitly > pref off context paint for web content" That would make the commit message summary line quite long, and burying it below the summary line is not great either. This could logically be a separate commit, so I'll break it out. > (2) What happens when the embedding page doesn't specify a value for "fill"? > * Suggested way of testing this: > - add another <img> to context-fill-04.html, WITHOUT any explicit style > rule to set its "fill". > - I suspect that <img> will render entirely blank (since it'll be passing > through a context-fill of "none"). So maybe add a border to the img > elements (and the divs in the reference case) to make it clearer that there > is actually an element there whose rendering we're comparing. context-fill-04.html is about testing that our caching doesn't fail to take account of context color when fetching a cached rendering, so I don't think testing the lack of a context color fits well here. I've created *-07.html tests for this. > (3) What happens when the embedded content just uses "context-fill" > *without* providing a fallback color? (Please test this both when the > feature is enabled -- in which case the value should still make it through I > think -- and *also* test it when the feature is disabled, in which case we > fall back... to what?) > > > In all of the above, I'm discussing "context-fill" but we should test them > for "context-stroke" as well. I incremented the test numbers of *-02 and above and slotted this in as *-02.

Pushed by jwatt@jwatt.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/826d5695cc8d part 10b - Account for the SVGContextPaint member in SVGImageContext::operator==. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/17138504d367 part 15 - Have nsImageFrame pass context paint to VectorImage. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/c39b6cbdda77 part 16 - Set the pref svg.context-properties.content.enabled in all.js so reftests can use it. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/df0af8f286b7 part 17 - Add a bunch of SVG-as-an-image context paint reftests. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/a039ec3178a2 part 18 - Minimize the cost of context paint when it is not available. r=dholbert

I appreciate all your work on the reviews here, Daniel. Your thoroughness really helped improve things!

Pushed by jwatt@jwatt.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/302913bd58be part 17 follow-up - Fuzz context paint reftest 04 for slight color difference on Win8 due to opacity. r=me

https://hg.mozilla.org/mozilla-central/rev/826d5695cc8d https://hg.mozilla.org/mozilla-central/rev/17138504d367 https://hg.mozilla.org/mozilla-central/rev/c39b6cbdda77 https://hg.mozilla.org/mozilla-central/rev/df0af8f286b7 https://hg.mozilla.org/mozilla-central/rev/a039ec3178a2 https://hg.mozilla.org/mozilla-central/rev/302913bd58be

I've been trying to document this, but am having trouble getting what's gong on here. I looked at your tests and put together my own minimal example: <!DOCTYPE html> <html> <head> <title>Basic context-fill/fill-opacity test</title> <style> img { width: 100px; height: 100px; } .img1 { fill: lime; } .img2 { fill: blue; } .img3 { fill: yellow; } </style> </head> <body> <img class="img1" src="data:image/svg+xml;utf8,<svg xmlns='http://www.w3.org/2000/svg'><rect width='100%' height='100%' fill='context-fill red' fill-opacity='0.9'/></svg>"> <img class="img2" src="data:image/svg+xml;utf8,<svg xmlns='http://www.w3.org/2000/svg'><rect width='100%' height='100%' fill='context-fill yellow' fill-opacity='0.9'/></svg>"> <img class="img3" src="data:image/svg+xml,<svg xmlns='http://www.w3.org/2000/svg'><line x1='0' y1='50%' x2='100%' y2='50%' stroke-width='120%' stroke='context-fill red'/></svg>"> </body> </html> But I don't really get: * what context-fill does - the squares seem to be colored the same as the color set in SVG, regardless of whether the context-fill keyword is included, and regardless of whether there is a fill color set in the CSS or not. * what is going on with the stroke color - the third square seems to be filled red, even though the stroke is set to red. Can you give me a very quick rundown of what changes need to be documented as a result of this bug, and wat gives withj the above apparent problems? Apologies if these seem like stupid questions, but I'm not very experienced with SVG. Thanks in advance for any help you can give.

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #109) > img { > width: 100px; > height: 100px; > } Since bug 1350010 landed, you need to specify a value for -moz-context-properties. Add the following to the rule above: -moz-context-properties: fill; This declares that the value of the 'fill' property on the 'img' elements (the "context" in which the images are embedded) may be exposed to the embedded images. > * what context-fill does - the squares seem to be colored the same as the > color set in SVG, regardless of whether the context-fill keyword is > included, and regardless of whether there is a fill color set in the CSS or > not. In addition to specifying -moz-context-properties, also note that you need to set the svg.context-properties.content.enabled pref to true if the images aren't referenced via a chrome:// or resource:// URL. > * what is going on with the stroke color - the third square seems to be > filled red, even though the stroke is set to red. There is no fill in the third image. It uses a line, which can only be stroked. The stroke is simply very wide. > Apologies if these seem like stupid questions, but I'm not very experienced > with SVG. Thanks in advance for any help you can give. They're not stupid questions. If the above isn't clear, perhaps email me directly or ping me on IRC to discuss?

Cool, thanks for the help jwatt! I think has cleared up my confusion. I've gone on to document it, by first updating the example, and the reference page for the property that sfoster had already started: https://mdn.github.io/css-examples/moz-context-properties/ https://developer.mozilla.org/en-US/docs/Web/CSS/-moz-context-properties I also added some details to the SVG stroke and fill attribute pages: https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/fill https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/stroke I added a note to the Fx55 release notes: https://developer.mozilla.org/en-US/Firefox/Releases/55#CSS Last but not least, I submitted a pull request to add the formal syntax into the MDN CSS data: https://github.com/mdn/data/pull/84/files I'm really not sure if I've got the syntax values right though. Can you check these and let me know if anything needs improving? Thanks again.

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #111) > I think has cleared up my confusion. I've gone on to document it, by first > updating the example, and the reference page for the property that sfoster > had already started: > > https://mdn.github.io/css-examples/moz-context-properties/ > https://developer.mozilla.org/en-US/docs/Web/CSS/-moz-context-properties Thanks, Chris. I've corrected and clarified some things on that page. Of particular note is that this feature is NOT available to web content, and there are no immediate plans to support that. > I also added some details to the SVG stroke and fill attribute pages: > > https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/fill > https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/stroke Since this feature is not currently available to web content, and because the examples you added duplicated what's on the -moz-context-properties page, I've removed the examples you added and referred readers to the -moz-context-properties page. > https://developer.mozilla.org/en-US/Firefox/Releases/55#CSS I don't think we should mention this in the release notes since we aren't shipping this to web content authors right now. You'll likely know better though so I'll leave it up to you to change this page. > Last but not least, I submitted a pull request to add the formal syntax into > the MDN CSS data: > > https://github.com/mdn/data/pull/84/files > > I'm really not sure if I've got the syntax values right though. > > Can you check these and let me know if anything needs improving? Commented over there.

Thanks jwatt! For the record, the comments have been answered (and fixes made) in https://github.com/mdn/data/pull/91