Dao tells me that when he uses the 'context-fill' value on an element it causes the 'fill-opacity' property on the element to be ignored.

So the aOpacity that gets passed to SVGContextPaint's (or in our case SVGEmbeddingContextPaint's) GetFillPattern/GetStrokePattern methods is the fill/stroke opacity: https://hg.mozilla.org/mozilla-central/annotate/e03e0c60462c775c7558a1dc9d5cf2076c3cd1f9/layout/svg/nsSVGUtils.cpp#l1496 The current code mistakenly ignores that argument.

Comment on attachment 8850819 [details] [diff] [review] patch Review of attachment 8850819 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/svg/SVGContextPaint.h @@ +238,4 @@ > } > > + /** > + * Always of type PatternType::COLOR. I changed these comments to: * Returns a pattern of type PatternType::COLOR, or else nullptr.

Comment on attachment 8850819 [details] [diff] [review] patch Review of attachment 8850819 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/svg/SVGContextPaint.cpp @@ +321,1 @@ > } It looks to me like this implementation will have unwanted/unnecessary collisions, between e.g. an object that represents "context-fill: red" and an object that represents "context-stroke: red". Both would produce HashGeneric(0, "red".ToABGR()). So they'd collide in any hash-map they end up in, resulting in suboptimal hash table usage. This problem is pretty trivial to avoid (and it'll provide improved hash lookup performance) so we might as well. I *think* we can work around this by changing the first clause to e.g.: if (mFillIsSet) { hash = HashGeneric(hash, mFill.ToABGR()); } else { // Arbitrary number, just to avoid trivial hash collisions between pairs of // instances with a.mFill == b.mStroke & with the other fields unset. hash = 1; } ::: layout/svg/SVGContextPaint.h @@ +266,5 @@ > private: > + Color mFill; > + Color mStroke; > + bool mFillIsSet; > + bool mStrokeIsSet; Could you make these Maybe<Color>, for better encapsulation & for built-in assertion checking that we only use the color when the boolean actually says it's been set? Minor downside of Maybe<> is that it won't pack as well, but that doesn't really matter here because - this struct is temporary anyway, and we won't ever have tons of them at once I think - both strategies work out to 64 bytes in practice (36 bytes the way you've got it, 40 with Maybe<>, according to local sizeof() measurements -- and both of those round up to 64 with word alignment).

Comment on attachment 8850819 [details] [diff] [review] patch Review of attachment 8850819 [details] [diff] [review]: ----------------------------------------------------------------- Two more final nits. And then, this is basically r+-with-nits-addressed, but it's probably worth a final look after you've converted to Maybe<> -- assuming you're cool with that -- since that will tweak a decent amount of the patch. Hence, r- for the moment, but I'll do my best to grant swift r+ when you post an updated version. First nit: the commit message: > Bug 1350015 - Fix the 'fill-opacity' property work with fill="context-fill". r=dholbert This doesn't quite make grammatical sense. I think maybe you're missing a word -- s/work/to work/, maybe? (Anyway, please reword to make sense.) ::: layout/svg/SVGContextPaint.cpp @@ +288,5 @@ > + return nullptr; > + } > + // The gfxPattern that we create below depends on aFillOpacity, and since > + // different elements in the SVG image may pass in different values for > + // fill opacities we don't try to cache the gfxContexts that we create. Second nit: s/gfxContexts/gfxPatterns/

Good catch on the hashing issue!

Comment on attachment 8851184 [details] [diff] [review] patch Review of attachment 8851184 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Is it possible to add a reftest for this? If so, it'd be great to land one with this patch (or soon afterwards if folks like Dao need the fix to be in ASAP). r=me

Pushed by jwatt@jwatt.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/c7f3f26cc004 Fix the 'fill-opacity' property to work with fill="context-fill". r=dholbert