Make context paint (context-fill, context-stroke, etc.) work for SVG-as-an-image

RESOLVED FIXED in Firefox 55

Status

()

Core
SVG
P1
normal
RESOLVED FIXED
3 years ago
4 days ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

(Blocks: 6 bugs, {dev-doc-complete})

Trunk
mozilla55
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [photon-visual] [only supported for a limited number of elements][pref for content: svg.context-properties.content.enabled])

Attachments

(18 attachments, 14 obsolete attachments)

20.86 KB, patch
dholbert
: review+
dholbert
: checkin+
Details | Diff | Splinter Review
39.82 KB, patch
dholbert
: review+
dholbert
: checkin+
Details | Diff | Splinter Review
8.09 KB, patch
dholbert
: review+
dholbert
: checkin+
Details | Diff | Splinter Review
18.59 KB, patch
dholbert
: review+
dholbert
: checkin+
Details | Diff | Splinter Review
17.34 KB, patch
dholbert
: review+
dholbert
: checkin+
Details | Diff | Splinter Review
4.59 KB, patch
jwatt
: review+
jwatt
: checkin+
Details | Diff | Splinter Review
5.70 KB, patch
jwatt
: review+
jwatt
: checkin+
Details | Diff | Splinter Review
4.10 KB, patch
dholbert
: review+
jwatt
: checkin+
Details | Diff | Splinter Review
4.09 KB, patch
jwatt
: review+
jwatt
: checkin+
Details | Diff | Splinter Review
8.11 KB, patch
dholbert
: review+
jwatt
: checkin+
Details | Diff | Splinter Review
21.18 KB, patch
dholbert
: review+
jwatt
: checkin+
Details | Diff | Splinter Review
14.57 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
1.94 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
1.06 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
1.19 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
10.70 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
4.15 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
5.31 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
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.)
(Assignee)

Updated

3 years ago
Depends on: 1094247
(Assignee)

Updated

a year ago
Summary: Make context-fill and context-stroke work for SVG-as-an-image → Make context paint (context-fill, context-stroke, etc.) work for SVG-as-an-image
(Assignee)

Updated

11 months ago
Depends on: 1290781
(Assignee)

Comment 4

11 months ago
Created attachment 8777116 [details] [diff] [review]
part 1 - Move gfxTextContextPaint to a separate file to enable use in imagelib

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.
Assignee: nobody → jwatt
Attachment #8777116 - Flags: review?(dholbert)
(Assignee)

Comment 5

11 months ago
Created attachment 8777117 [details] [diff] [review]
part 2 - Rename gfxTextContextPaint to SVGContextPaint and add some code comments
Attachment #8777117 - Flags: review?(dholbert)
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).
Attachment #8777116 - Flags: review?(dholbert) → review+
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)
Attachment #8777117 - Flags: review?(dholbert) → review+

Comment 8

11 months ago
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
(Assignee)

Comment 9

11 months ago
Created attachment 8777161 [details] [diff] [review]
part 3 - Move SVGTextContextPaint into SVGContextPaint.h
Attachment #8777161 - Flags: review?(dholbert)
(Assignee)

Comment 10

11 months ago
Created attachment 8777162 [details] [diff] [review]
part 4 - Rename SVGTextContextPaint to SVGContextPaintImpl

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.
Attachment #8777162 - Flags: review?(dholbert)
(Assignee)

Comment 11

11 months ago
Created attachment 8777165 [details] [diff] [review]
part 5 - Move the code from nsSVGUtils::SetupContextPaint and nsSVGUtils::GetContextPaint into SVGContextPaint.

There is really no reason for this code to live in nsSVGUtils.
Attachment #8777165 - Flags: review?(dholbert)
(Assignee)

Updated

11 months ago
Keywords: leave-open

Comment 12

11 months ago
bugherder
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.)
Attachment #8777161 - Flags: review?(dholbert) → review+
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.)
Attachment #8777162 - Flags: review?(dholbert) → review+
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?
Attachment #8777161 - Flags: review+ → review-
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?
Attachment #8777165 - Flags: review?(dholbert) → review+
(Assignee)

Comment 18

11 months ago
Created attachment 8777695 [details] [diff] [review]
part 3 - Move SVGTextContextPaint into SVGContextPaint.h/.cpp

(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.
Attachment #8777161 - Attachment is obsolete: true
Attachment #8777695 - Flags: review?(dholbert)
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.
Attachment #8777695 - Flags: review?(dholbert) → review+

Comment 20

11 months ago
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

Comment 21

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9bc431d8e922
https://hg.mozilla.org/mozilla-central/rev/17233e72c546
https://hg.mozilla.org/mozilla-central/rev/6a38b96af140
Keywords: dev-doc-needed
Do these patches add the context-fill keyword or are they just groundwork?
Flags: needinfo?(dholbert)
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.)
Flags: needinfo?(dholbert)

Comment 24

10 months ago
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. :-)
Flags: needinfo?(dholbert)
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)
Flags: needinfo?(dholbert)
Attachment #8777116 - Flags: checkin+
Attachment #8777117 - Flags: checkin+
Attachment #8777162 - Flags: checkin+
Attachment #8777165 - Flags: checkin+
Attachment #8777695 - Flags: checkin+
Hi Jonathan, if you are back do you know when you might be able to look at this again? Thanks!
Flags: needinfo?(jwatt)
(Assignee)

Comment 27

7 months ago
Replied on IRC, but should have replied here too. I can look at this again during the All Hands. Let's talk then.
Flags: needinfo?(jwatt)
(Assignee)

Comment 28

5 months ago
Created attachment 8828472 [details] [diff] [review]
part 6 - When copying SVGImageContexts use the copy ctor and override individual members
Attachment #8828472 - Flags: review?(dholbert)
(Assignee)

Comment 29

5 months ago
Created attachment 8828473 [details] [diff] [review]
part 7 - Support lazy initialization of AutoSetRestoreSVGContextPaint
Attachment #8828473 - Flags: review?(dholbert)
(Assignee)

Comment 30

5 months ago
Created attachment 8828474 [details] [diff] [review]
part 8 - Make SVGContextPaint a ref counted class
Attachment #8828474 - Flags: review?(dholbert)
(Assignee)

Comment 31

5 months ago
Created attachment 8828475 [details] [diff] [review]
part 9 - Make AutoSetRestoreSVGContextPaint::Init's SVGContextPaint argument const
Attachment #8828475 - Flags: review?(dholbert)
(Assignee)

Comment 32

5 months ago
Created attachment 8828476 [details] [diff] [review]
part 10 - Make SVGImageContext's ctor's aViewportSize argument optional
Attachment #8828476 - Flags: review?(dholbert)
(Assignee)

Comment 33

5 months ago
Created attachment 8828477 [details] [diff] [review]
part 11 - Add an SVGContextPaint subclass for inheriting context paint into SVG-as-an-image
Attachment #8828477 - Flags: review?(dholbert)
(Assignee)

Comment 34

5 months ago
Created attachment 8828478 [details] [diff] [review]
part 12 - Allow SVGImageContext to store an SVGContextPaint and have VectorImage propagate it
Attachment #8828478 - Flags: review?(dholbert)
(Assignee)

Comment 35

5 months ago
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.
(Assignee)

Comment 36

5 months ago
Created attachment 8828481 [details] [diff] [review]
part 13 - Allow nsImageBoxFrame to pass context paint to VectorImage

For now I'm only implementing this for nsImageBoxFrame since that's what the front-end code needs.
Attachment #8828481 - Flags: review?(dholbert)
(Assignee)

Comment 37

5 months ago
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<..."
Attachment #8828472 - Flags: review?(dholbert) → review+
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(...);
   }
Attachment #8828473 - Flags: review?(dholbert) → review-
[ni for htoughts on comment 39 & whether we actually need part 7]
Flags: needinfo?(jwatt)
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.
Attachment #8828474 - Flags: review?(dholbert) → review+
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.
Attachment #8828475 - Flags: review?(dholbert) → review-
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".
Attachment #8828477 - Flags: review?(dholbert) → review+
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),
                     ...);
Attachment #8828478 - Flags: review?(dholbert) → review-
(Assignee)

Comment 46

5 months ago
(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.
Flags: needinfo?(jwatt)
(Assignee)

Comment 47

5 months ago
(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|.
(Assignee)

Comment 48

5 months ago
Created attachment 8832398 [details] [diff] [review]
part 6 - When copying SVGImageContexts use the copy ctor. r=dholbert
Attachment #8828472 - Attachment is obsolete: true
Attachment #8828473 - Attachment is obsolete: true
Attachment #8832398 - Flags: review+
(Assignee)

Comment 49

5 months ago
Created attachment 8832399 [details] [diff] [review]
part 7 - Make SVGContextPaint a ref counted class. r=dholbert
Attachment #8828474 - Attachment is obsolete: true
Attachment #8832399 - Flags: review+
(Assignee)

Comment 50

5 months ago
Created attachment 8832400 [details] [diff] [review]
part 8 - Make AutoSetRestoreSVGContextPaint::Init's SVGContextPaint argument const
Attachment #8832400 - Flags: review?(dholbert)
(Assignee)

Comment 51

5 months ago
Created attachment 8832402 [details] [diff] [review]
part 9 - Add an SVGContextPaint subclass for inheriting context paint into SVG-as-an-image. r=dholbert
Attachment #8828475 - Attachment is obsolete: true
Attachment #8828476 - Attachment is obsolete: true
Attachment #8828477 - Attachment is obsolete: true
Attachment #8828476 - Flags: review?(dholbert)
Attachment #8832402 - Flags: review+
(Assignee)

Comment 52

5 months ago
Created attachment 8832403 [details] [diff] [review]
part 10 - Allow SVGImageContext to store an SVGContextPaint and have VectorImage propagate it
Attachment #8828478 - Attachment is obsolete: true
Attachment #8832403 - Flags: review?(dholbert)
(Assignee)

Comment 53

5 months ago
Created attachment 8832440 [details] [diff] [review]
part 11 - Have nsImageBoxFrame pass context paint to VectorImage
Attachment #8828481 - Attachment is obsolete: true
Attachment #8828481 - Flags: review?(dholbert)
Attachment #8832440 - Flags: review?(dholbert)
(Assignee)

Comment 54

5 months ago
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.
Attachment #8832440 - Attachment is obsolete: true
Attachment #8832440 - Flags: review?(dholbert)
(Assignee)

Updated

5 months ago
Depends on: 1335780
(Assignee)

Comment 55

5 months ago
Created attachment 8832652 [details] [diff] [review]
part 11 - Feed a Maybe<SVGImageContext> through to DrawImageInternal
Attachment #8832652 - Flags: review?(dholbert)
(Assignee)

Comment 56

5 months ago
Created attachment 8832653 [details] [diff] [review]
part 13 - Have nsImageBoxFrame pass context paint to VectorImage
Attachment #8832653 - Flags: review?(dholbert)
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.
Attachment #8832400 - Flags: review?(dholbert) → review+
Comment hidden (obsolete)
er, sorry! ignore comment 58 - I was looking at the wrong version of the patch somehow. Those comments were indeed addressed.
Attachment #8832402 - Flags: feedback?(jwatt)
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.)
Attachment #8832403 - Flags: review?(dholbert) → review+
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.)
Attachment #8832652 - Flags: review?(dholbert) → review+
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.
Attachment #8832653 - Flags: review?(dholbert) → review+
(Assignee)

Comment 64

5 months ago
(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.

Comment 65

5 months ago
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
(Assignee)

Comment 66

5 months ago
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.

Comment 67

5 months ago
bugherder
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
(Assignee)

Comment 68

4 months ago
(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.
(Assignee)

Updated

4 months ago
Attachment #8832653 - Attachment description: part 12 - Have nsImageBoxFrame pass context paint to VectorImage → part 13 - Have nsImageBoxFrame pass context paint to VectorImage
(Assignee)

Comment 69

4 months ago
Created attachment 8839503 [details] [diff] [review]
part 12 - Make SVGImageContext's ctor's aViewportSize parameter optional
Attachment #8839503 - Flags: review?(dholbert)
(Assignee)

Updated

4 months ago
Attachment #8832652 - Flags: checkin+
(Assignee)

Updated

4 months ago
Attachment #8832403 - Flags: checkin+
(Assignee)

Updated

4 months ago
Attachment #8832402 - Flags: checkin+
(Assignee)

Updated

4 months ago
Attachment #8832400 - Flags: checkin+
(Assignee)

Updated

4 months ago
Attachment #8832399 - Flags: checkin+
(Assignee)

Updated

4 months ago
Attachment #8832398 - Flags: checkin+
(Assignee)

Comment 70

4 months ago
Created attachment 8839508 [details] [diff] [review]
part 12 - Make SVGImageContext's ctor's aViewportSize parameter optional

Update to tip.
Attachment #8839503 - Attachment is obsolete: true
Attachment #8839503 - Flags: review?(dholbert)
Attachment #8839508 - Flags: review?(dholbert)
(Assignee)

Comment 71

4 months ago
Created attachment 8839509 [details] [diff] [review]
part 14 - Have nsImageBoxFrame pass context paint to VectorImage [r=dholbert]

Update to tip.
Attachment #8832653 - Attachment is obsolete: true
Attachment #8839509 - Flags: review+
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.
Attachment #8839508 - Flags: review?(dholbert) → review+
(Assignee)

Comment 73

4 months ago
(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.

Comment 74

4 months ago
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

Comment 75

4 months ago
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
Flags: needinfo?(jwatt)
(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.
(Assignee)

Comment 77

4 months ago
Created attachment 8845174 [details] [diff] [review]
part 13 - Fix an uninitialized memory bug in SVGImageContext's default ctor
Flags: needinfo?(jwatt)
Attachment #8845174 - Flags: review?(dholbert)
(Assignee)

Updated

4 months ago
Attachment #8845174 - Attachment description: part 10b - Account for the new SVGContextPaint member in SVGImageContext::operator== → part 13 - Fix an uninitialized memory bug in SVGImageContext's default ctor
(Assignee)

Comment 78

4 months ago
Created attachment 8845176 [details] [diff] [review]
part 10b - Account for the new SVGContextPaint member in SVGImageContext::operator==
Attachment #8845176 - Flags: review?(dholbert)
(Assignee)

Comment 79

4 months ago
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.)
(Assignee)

Comment 80

4 months ago
(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
Attachment #8845174 - Flags: review?(dholbert) → review+
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.
Attachment #8845176 - Flags: review?(dholbert) → review-

Comment 83

4 months ago
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

Comment 84

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/db0ae4feb40a
https://hg.mozilla.org/mozilla-central/rev/62859e9f131a
https://hg.mozilla.org/mozilla-central/rev/f622c2236747
See Also: → bug 1347543
(Assignee)

Updated

3 months ago
Blocks: 1347543

Updated

3 months ago
Blocks: 759252
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?
Flags: needinfo?(jwatt)

Updated

3 months ago
Blocks: 1325171

Updated

3 months ago
See Also: bug 1347543

Updated

3 months ago
Blocks: 1346488
Whiteboard: [photon]

Updated

3 months ago
No longer blocks: 1346488
Or maybe dholbert can weigh in on comment 85?
Flags: needinfo?(dholbert)
I chatted with jwatt yesterday about fill-opacity not working in combination with context-fill, he said he'd look into it.
(Assignee)

Updated

3 months ago
Blocks: 1350010
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.
(Assignee)

Updated

3 months ago
Blocks: 1350015
(Assignee)

Comment 89

3 months ago
(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.)
Flags: needinfo?(jwatt)
Flags: needinfo?(dholbert)
(Assignee)

Comment 90

3 months ago
Created attachment 8850795 [details] [diff] [review]
part 15 - Have nsImageFrame pass context paint to VectorImage
Attachment #8850795 - Flags: review?(dholbert)
(Assignee)

Updated

3 months ago
Whiteboard: [photon] → [photon][only supported for a limited number of elements][pref for content: svg.context-properties.content.enabled]
(Assignee)

Comment 91

3 months ago
(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.)
(Assignee)

Updated

3 months ago
Attachment #8839509 - Attachment description: part 13 - Have nsImageBoxFrame pass context paint to VectorImage [r=dholbert] → part 14 - Have nsImageBoxFrame pass context paint to VectorImage [r=dholbert]
(Assignee)

Comment 92

3 months ago
Created attachment 8851580 [details] [diff] [review]
part 10b - Account for the SVGContextPaint member in SVGImageContext::operator==
Attachment #8851580 - Flags: review?(dholbert)
(Assignee)

Comment 93

3 months ago
Created attachment 8851647 [details] [diff] [review]
part 17 - Add a bunch of SVG-as-an-image context paint reftests.
Attachment #8851647 - Flags: review?(dholbert)
Attachment #8845176 - Attachment is obsolete: true
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.
Attachment #8851580 - Flags: review?(dholbert) → review-
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.
Attachment #8850795 - Flags: review?(dholbert) → review+
(Assignee)

Comment 96

3 months ago
Created attachment 8851733 [details] [diff] [review]
part 10b - Account for the new SVGContextPaint member in SVGImageContext::operator==

(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.
Attachment #8851580 - Attachment is obsolete: true
Attachment #8851733 - Flags: review?(dholbert)
(Assignee)

Comment 97

3 months ago
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?)
(Assignee)

Comment 98

3 months ago
(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.)
Attachment #8851647 - Flags: review?(dholbert) → review+
(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
Attachment #8851733 - Flags: review?(dholbert) → review+
(Assignee)

Comment 102

3 months ago
Created attachment 8851775 [details] [diff] [review]
part 18 - Minimize the cost of context paint when it is not available

I wonder what you think about maybe doing it this way.
Attachment #8851775 - Flags: review?(dholbert)
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.
Attachment #8851775 - Flags: review?(dholbert) → review+
(Assignee)

Comment 104

3 months ago
(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.
(Assignee)

Updated

3 months ago
Blocks: 1351236
(Assignee)

Updated

3 months ago
Blocks: 1351243

Comment 105

3 months ago
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
(Assignee)

Comment 106

3 months ago
I appreciate all your work on the reviews here, Daniel. Your thoroughness really helped improve things!
No longer depends on: 1094247
Keywords: leave-open
(Assignee)

Updated

3 months ago
Attachment #8851775 - Attachment description: part 17 - Minimize the cost of context paint when it is not available → part 18 - Minimize the cost of context paint when it is not available
(Assignee)

Updated

3 months ago
Attachment #8851647 - Attachment description: part 16 - Add a bunch of SVG-as-an-image context paint reftests. → part 17 - Add a bunch of SVG-as-an-image context paint reftests.

Comment 107

3 months ago
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

Comment 108

3 months ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Updated

3 months ago
Depends on: 1351990

Updated

3 months ago
Depends on: 1352258

Updated

2 months ago
Iteration: --- → 55.2 - Apr 3
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [photon][only supported for a limited number of elements][pref for content: svg.context-properties.content.enabled] → [photon-visual] [only supported for a limited number of elements][pref for content: svg.context-properties.content.enabled]

Updated

2 months ago
Flags: qe-verify? → qe-verify-
(Assignee)

Updated

2 months ago
Blocks: 1359073
(Assignee)

Updated

a month ago
Blocks: 1365926
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.
Flags: needinfo?(jwatt)
(Assignee)

Comment 110

a month ago
(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?
Flags: needinfo?(jwatt)
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.
Flags: needinfo?(jwatt)
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 1370797
(Assignee)

Comment 112

9 days ago
(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.
Flags: needinfo?(jwatt)
(Assignee)

Updated

9 days ago
Flags: needinfo?(cmills)
Thanks jwatt!

For the record, the comments have been answered (and fixes made) in https://github.com/mdn/data/pull/91
Flags: needinfo?(cmills)
You need to log in before you can comment on or make changes to this bug.