276431 - external SVG not loaded from img tag

Status: RESOLVED FIXED

(Core :: SVG, defect, P2)

Description

[fantasai]

Mozilla doesn't seem to support loading external SVG images embedded with <img>
tags. Probably because nsImageFrame isn't set up to handle SVG whereas
nsObjectFrame is.

It is important that the nsImageFrame use the intrinsic size of the SVG image
when no explicit width/height is given. Once that works, it might be possible to
have <object>-embedded SVG to use nsImageFrame, too, so those get sized
correctly as well. (See e.g. bug 70978.)

Tested on SVG-enabled Mozilla, build id 2004122709

To whoever knows the answer:
  What image-renderer interfaces would need to be implemented for this to work
  (or would nsImageFrame need special-cased code?), and what core SVG interfaces
  would they be hooking into?

Comment 1

[fantasai]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

So to get this to work we'd need an nsIImage implementation for SVG, and an
imgIDecoder.  These will have to allow pixel-by-pixel access to the SVG image
data (presumably cached, not computed on every pixel access) and provide some
metadata about the image (notably the intrinsic size).

If that's done, there will need to be no changes to nsImageFrame, since SVG will
just look like any other image.  Note that this will not allow interactivity and
such the way <object> does.

Comment 3

[Stuart Parmenter]

I don't think it makes sense to "decode" an SVG image in to an nsIImage.  Things
like animation, etc won't work if you do this (without a huge amount of extra
work).  If we really want this to work, i'd guess that changes similar to those
in the object frame would have to go in to image frame.  That kinda sucks though
and isn't very flexable...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Frankly, we want to have SVG working in list-style-image, background-image, etc,
etc, not just in <img>.

So either all images work the same way (and all these places use the image
apis), or we don't support SVG images (just SVG documents) or we hack every
single one of these places somehow, probably implementing the equivalent of
SVG-to-nsIImage glue in 3-4 places.

This is complicated by the fact that for <img> we won't know we're dealing with
SVG until we get it....  For <object>, if it claims to be SVG and comes out HTML
we just render the HTML, but for <img> that would seem to be somewhat
undesirable.  404 pages in <img> tags, anyone?  ;)

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

note bug 272288 comment 1 and (especially) bug 231179...

Comment 6

[Jeff Schiller]

Any disposition on this bug?  It would definitely be an improvement to support
SVG wherever a raster image is allowed (<img> tag, background images, etc). 
This could turn out to be a killer feature for a browser to have.  

What makes this killer is if the image is a "live" SVG image 
(though I will agree with Tim Rowley in Bug 272288, that it is ambiguous in the
SVG spec).  Imagine being able to animate your background image while displaying
your HTML over top.  Currently this is only possible with a compound XHTML
document or some form of foreignObject within a SVG root document.

Comment 7

[fantasai]

I am sure there was a Gecko demo at some point with animated GIF backgrounds.

Comment 8

[Erik Fabert]

*** Bug 298682 has been marked as a duplicate of this bug. ***

Comment 9

[Steffen Macke]

Would it make things much easier to leave out the animation part for the time
being? I guess that most users are interested in simply replacing unanimated
images with SVG ones (e.g. to improve quality).

Comment 10

[Holger Will]

when using <image xlink:href="example.svg"/> in ASV the result is always a
static SVG, no animation no scriting. i think this is the desired behaviour for
<html:img> as well. the interactivity and animation feature for background
images would be cool, but can wait, given that there is currently no animation
support, and i think user events dont make sense on backround images, so
enabling scrips in image is questionable. if you need script access you got to
use <object> because image does not have any scripting interfaces (
contentDocument,contentWindow)
maybe just enable the onload event. 
so i guess the solution Boris Zbarsky proposed in comment #2 would be the right
way to go for now.

Comment 11

[Jeff Schiller]

I agree that the simple solution right now would be to do what Holger is
suggesting (a frozen and isolated view of the SVG document) if it will mean
quicker implemention.  And while I agree with Holger's interpretations, I would
recommend contacting the W3C CDF working group as they are the definitive source
of answers (even if they don't have all the use cases yet) .  

At least in my initial emails with Chris Lilley he felt that scripting and
animation could still happen in a background-image or a <img> (which would be
nice).  He did agree that events would not be able to flow from HTML DOM to SVG
DOM through background-image (i.e. SVG is "underneath") but that events might be
able to go through HTML DOM to SVG DOM.  The bottom line is that all these use
cases need to be thought out and described unambiguously in the CDF spec.

See http://www.w3.org/TR/2005/WD-CDRReqs-20050404/ and
http://lists.w3.org/Archives/Public/public-cdf/ for more and let's get that
clarified soon so Mozilla has clear direction on implementation.

Comment 12

[Jeff Schiller]

correction:

"...but that events might be able to go through HTML DOM to SVG DOM through an
html:img tag"

Comment 13

[Chris Hubick]

A note on perspective and intrinsic sizing, I just wanted to leave a quick
pointer to bug 70978 (and maybe bug 70976).  That is to say, it would be (more
than) nice for the ability to include an SVG graphic, say only specifying an 80%
width, and have the height automatically scaled to maintain the image's
intrinsic perspective.  If I want to include an SVG banner for a website that
will scale properly when displayed on a 2" Cell Phone screen or 20" Desktop LCD
- I can specify a width of 80% of the page, but there is then no way to specify
a height that will maintain the image's perspective. Em's could be used, but
that's a less than ideal workaround.

Comment 14

[petr.pisar]

This missing function really suck. Now when we have native SVG support in stable version of FF. In the bug 320875 is testcase with nodefault object size but with size defined in the SVG file in contrast to the PNG.  

Comment 15

[Hixie (not reading bugmail)]

(In reply to comment #4)
> This is complicated by the fact that for <img> we won't know we're dealing 
> with SVG until we get it....  For <object>, if it claims to be SVG and comes 
> out HTML we just render the HTML, but for <img> that would seem to be somewhat
> undesirable.  404 pages in <img> tags, anyone?  ;)

If we support SVG in <img> then I see no reason we wouldn't support HTML in <img> (with 404s being handled as failed loads just like for <object> elements -- <iframe> is the only one of these element that ignores errors and doesn't fall back to its fallback for them). In fact, I don't see how we couldn't. Anything that supports SVG automatically supports HTML in our architecture. They're the same thing, for all intents and purposes.

The question is whether we want to support vector graphics in <img> at all. One could argue that <img> (just like <svg:image> in SVG Tiny) should be only for raster images. But then one could also argue the opposite...

Comment 16

[Hixie (not reading bugmail)]

I suppose one option (which reading the comments again may have been what you had in mind anyway) is to say that any |Document|s that end up rendered by an <img> are rendered as static bitmaps, much like if they were used in CSS, so that they are image |Frame|s and so we sidestep the problem hyatt would have with all this (namely that <img> should always have the same kind of render frame, and that it shouldn't be able to change from an image render frame to an iframe render frame, for example). Hmm...

Comment 17

[Matthew Raymond]

I we should have the following guidelines when using SVG as a graphics format:

1) Scripts should not be executed.
2) Elements in non-SVG namespaces should generally be ignored.
3) Any elements unrelated to presentation should be ignored.
4) CSS should be ignored???

I do not think rendering a one-time static image is a good option, because the image can't be resized. Wasn't that the whole point of vector graphics to begin with? Also, we already allow stuff like animated GIF images (and MNG when using MNGZilla), so I don't see a justification for using a static image for SVG unless animation in SVG cannot be accomplished without scripting. (I admit that my knowledge of SVG is to limited to make a determination on the latter issue.)

Comment 18

[Daniel Pope]

Allowing SVG in <img>s is bad news. Many web applications differentiate <img> as fairly innocuous allowing it even from third parties, but not <object>, because <object> holds power.

SVG is incredibly powerful. It contains scripting, hyperlinks, <svg:image> even allows possibly third-party images to make HTTP requests. I also suspect that a fairly small SVG can DoS the renderer by requiring exponential time to render. That is a huge paradigm shift.

<img> tags would require a neutered implementation of SVG to match the security model of most sites, even more so than just dropping scripting and animation - it would need at least to disallow <svg:image> links to third and fourth parties, and have a render timeout.

I would recommend this is not fixed until that stuff can be thoroughly looked at.

Comment 19

[Jeff Schiller]

Agreed.  As per comment #10...

Comment 20

[Jeff Schiller]

Btw, I consider this a low-priority bug compared to the SVG CSS one (Bug 231179).  The CSS bug prevents us from doing really cool things.  This bug would just be a "nice to have"...

You can already do most things you need to do here with the object tag, so it doesn't hurt developers as much.  As developers, we can just use <object> anywhere you wanted to use an <img> tag (unless I'm missing something).

Comment 21

[Boris Zbarsky [:bzbarsky]]

The SVG CSS bug is really the same bug as this, basically...  That is, the functionality needed from the point of view of Gecko is the same.  Some of the security concerns are different, of course.

Comment 22

[Holger Will]

(In reply to comment #17)

> 1) Scripts should not be executed.

maybe enable only the onload event, like the batik-rasterizer does.

> 2) Elements in non-SVG namespaces should generally be ignored.

if we could have the onload event and/or XSLT transformations (or even xbl), elements from other namespaces might be usefull.

> 3) Any elements unrelated to presentation should be ignored.
> 4) CSS should be ignored???

no, css stylable images are cool !

> 
> I do not think rendering a one-time static image is a good option, because the
> image can't be resized. Wasn't that the whole point of vector graphics to begin
> with?

well, i guess the svg should be re-rasterized when the size changes.

> Also, we already allow stuff like animated GIF images (and MNG when using
> MNGZilla), so I don't see a justification for using a static image for SVG
> unless animation in SVG cannot be accomplished without scripting.
> 

yes you could prerender some frames and then display them in a loop,
the problem is that there is no way to determin how long the animation takes.

Comment 23

[Boris Zbarsky [:bzbarsky]]

> maybe enable only the onload event, like the batik-rasterizer does.

From an XSS/security point of view, this is equivalent to allowing all scripting.

Comment 24

[Holger Will]

(In reply to comment #23)

> From an XSS/security point of view, this is equivalent to allowing all
> scripting.

i see :-( , so what about xslt/xbl then ?

Comment 25

[Boris Zbarsky [:bzbarsky]]

Probably OK, though most of XBL would not work (no scripting, so no constructors, etc).  You could still create anonymous content, though.  That seems reasonably safe.

Comment 26

[Millennium]

What if the img tag supported only SVG Tiny, with the object tag required to use full SVG? This seems to (at least theoretically) answer the security concerns, while still allowing a well-documented subset of SVG in the img tag. I can see only two clarifications which would need to be added: foreign namespaces are not recognized when included through the img tag, and svg:image would not support SVG (but would support, for example, JPEG and PNG, and possibly other bitmap formats) when included through the img tag.

SVG Tiny doesn't support scripting, it doesn't necessarily have to support elements from other namespaces, and it doesn't necessarily have to support SVG in the SVG:image tag. Would an implementation of SVG Tiny which didn't support these two optional things be safe? Obviously you couldn't do everything with this that you could with full SVG, but I'm not sure this is a Bad Thing, since the object tag still exists to handle full SVG.

It seems to me that if this method can be shown safe then it would provide, if not necessarily the best of both worlds, then at least a decent compromise. The full power (and danger) of SVG would not be available through the img tag, but a well-documented standard subset would be. Tiny is, by itself, powerful enough to handle most aspects of vector graphics, and what it can't handle could be dealt with through the object tag.

It's just a thought, anyway. I might even be wrong about Tiny being safe, in which case the whole thing is moot. But if it would be safe, then might it be satisfactory?

Comment 27

[Boris Zbarsky [:bzbarsky]]

> SVG Tiny doesn't support scripting

You must mean SVG Tiny 1.1 or something? SVG Tiny 1.2 most certainly does.

More to the point, implementing both SVG profiles in the same code is a lot of work -- about twice as much work as only implementing one and putting some restrictions on it.

Comment 28

[Holger Will]

(In reply to comment #26)
> What if the img tag supported only SVG Tiny, with the object tag required to
> use full SVG? 

i would object to that, i think this feature would be nice to have especially where you need a complex graphic, but you dont need a dom and stuff.
especially the most expensive features of SVG like filters would benefit most from solving this bug, because then i could pack tons of filters in a file,
without loosing performance of the browser, but filters are not part of SVGT. using SVGT would cost to many features, like clippathes, masks, opacity, gradients, filters.
i recently had a little project where this would come in handy, i wrote an XSLT to create a barchart on the clientside. the largest benefit here is
the saved serverload and bandwidth. so only a small chart description is send to the browser. with xslt, i turn that description into a complex barchart graphic with 100s of objects with grandients  , opacity, strokes and filters
this works great, but try embeding this chart in a html file; firefox nearly
dies when you try to scroll. as i dont want to change the chart via script, a one shot still image would be enough for me. having this bug fixed would gain me the best of two worlds: decreased server load and bandwith, and good performance on the client while still providing graphicly rich content.
(here is an example: http://www.treebuilder.de/default.asp?file=172558.xml i had to revert to a prerendered image because of the performance issues, and this is only a small example )

also i think it is not neccesarry to revert to SVGT because SVG allows for sub profiling via modules. so we could have an SVG full implementation without the scripting module.

Comment 29

[Arpad Borsos [:Swatinem]]

What exactly are the security concerns? I mean what can be exploited via scripting that can harm the user or steal his data? An SVG embedded via img does not have any relation to the parent document so it can't steal any data. And XmlHttpRequest  does only work on the same domain name anyway so there is no way to send any stolen data.
And how do these security concerns relate to svg images in css? (#231179) CSS is used by the site author and is not meant to harm anybody, whereas the img tag can be used by and 3rd person in message boards for example.

Comment 30

[Christian :Biesinger (don't email me, ping me on IRC)]

there are other ways to send data than XMLHttpRequest... (forms, <img src="....?data=whatever"> come to mind)

Comment 31

[Boris Zbarsky [:bzbarsky]]

> I mean what can be exploited via scripting that can harm the user or steal
> his data?

Simple example:  an SVG image posted as part of a MySpace (or LiveJournal, or whatever) page that steals other MySpace users' MySpace cookies and sends them to some other server by any of the various means that can be used to send text data (biesi mentioned some, there are others).

The same-domain restrictions of XMLHttpRequest are not so much there to prevent info being sent to a site that wants it, but rather to prevent attacks against web services via sending them XML they do not want and do not expect.

In general, I suggest that anyone planning to comment on this bug take a few hours to read up on the current state of XSS exploits on the web before doing so.  It would help make the comments much more constructive.

> CSS is used by the site author and is not meant to harm anybody

Many sites of the sort we're talking about here allow users to upload CSS to style their page.

Comment 32

[dolphinling]

Hmm... what about foreignObject? Could it (reasonably, by a human) be implemented so that it would be safe, or would it open social engineering attacks that couldn't be stopped by simply blocking script? I'm thinking here of a full-page SVG with embedded HTML that looks like a login screen, complete with form submitting to the outside. Would it work to just block forms in <img> as well as script? Would that be reasonably doable? If not, would other content types be safe (audio, video)?

Note that some sort of descending into referenced objects to block script will have to be done, as when bug 231179 and bug 272288 are fixed, otherwise one could just include script in the svg-referenced-by-svg. So that descending should also work to block scripts in other file formats. But would that be enough?

Comment 33

[Holger Will]

i think a static snapshot image of the svg, with all scripts ignored, coud not
be any more or less secure than any other image format. in the end its all just pixeldata, even the forms in foreignObject. or am i missing something ? 

Comment 34

[leffeman]

The only scripting support I can see a need for in simple images (img and CSS) is simple DOM manipulation (deletion, creation and read-write access to objects, no events).  Why not simply block anything else, and not allow the SVG to access anything outside its own document (no cookie access, I can't see an use for it either in simple images)?

For SVG applications that will take user input, ordain the use of the object element.

Comment 35

[Boris Zbarsky [:bzbarsky]]

> Why not simply block anything else

Because that involves making an exhaustive list of "everything else" and adding checks to all of it.  It's very very hard to get this right without leaving any holes, and places a burden on implementations of every single DOM method to check for this case.

I'm not really sure what the discussion is about.  There's pretty much agreement that SVG in <img> means no script, last I checked.  The remaining questions are how to actually best do the rendering.

Comment 36

[David Finch]

Being able to display SVG from untrusted sources without the risk of running embedded scripts sounds to me like a good idea in general, and shouldn't be limited to the <img> tag. Maybe some requirement like 'scripted="yes"' could be added to both the <object> and <img> tags.

Comment 37

[Toni Ruottu]

I'm experiencing the same problem on Ubuntu 7.04. Please add Ubuntu into the OS field. I don't seem to have permissions required for doing that.

Comment 38

[jonathan chetwynd]

Attachment: thunder and rain

slightly better formed html

Comment 39

[jonathan chetwynd]

What's the 3 year delay on this guys?

Opera already implements this. 
OperaMail displays svg on receipt, but not in compose mode [e-mail]

Safari Webkit claims fixed and landed in trunk tomorrow?
http://bugs.webkit.org/show_bug.cgi?id=5971

email applications that allow the author to include svg symbols will significantly help many. This is just one recent email:

"The largest single group of pupils with additional support needs
in Scotland are those with moderate/severe learning difficulties (25%), who
are likely to benefit from symbol support."

When will Thunderbird default to displaying SVG?
https://bugzilla.mozilla.org/show_bug.cgi?id=366126

Comment 40

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

So, is there any reason that this should work other than that some spec says so?  Is there a reason it would be useful to authors?  Is there a good reason that the spec asks for it other than somebody's idea of theoretical purity?  Some use case that it satisifies, that using object doesn't?

SVG, in reality, is a document format, not an image format, and I don't see why we shouldn't just treat it like other document formats.

Comment 41

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

(I'm asking because fixing this seems like a good bit of work and a good bit of extra security exposure, and doing that requires a good reason.)

Comment 42

[Jeff Walden [:Waldo]]

While SVG may in reality have been designed as a document format, its uses in practice are primarily as an image/graphics format, whose natural home in the mind of a web developer has always been in <img>.  I don't think you can reasonably fight this intuition, particularly without support from other browsers.

Comment 43

[Guilherme Fonseca]

Let's look, for example, at the following wikipedia image page:

http://en.wikipedia.org/wiki/Image:Hexahedron.svg

It contains a PNG preview of an SVG image and a link to it. I believe that it would be cleaner if the preview used the svg image itself, and left rendering of the SVG for the web browser.

Comment 44

[Jeff Walden [:Waldo]]

(In reply to comment #43)
> I believe that it would be cleaner if the preview used the svg image itself,
> and left rendering of the SVG for the web browser.

It would indeed, but I don't think that's quite the distinction being made.

The question is whether it's worthwhile to support <img> when perfectly adequate support already exists in <iframe>, <object>, etc. with understood and effective security mechanisms in place to prevent the SVG (or any other document format loaded instead) from escaping its prison.  (There's no reason Wikipedia couldn't do that now, with Accept or user-agent detection.)  Those security mechanisms would have to be modified or reinvented for supporting SVG in <img>, or the document-support code would have to be modified to provide an <img>-style context; implementing either is a decent-sized task with lots of potential for regressions (and security ones, at that).

Comment 45

[Chris Hubick]

As a web author who has basically been waiting for this for 6 years now (see bug 70978), and since this blocks bug 231179, I will comment...

I don't author many pages with bar charts or anything where you would put the graphics as actual content of the page... like most authors, I use graphics mainly for logos/themes/styling via CSS image replacement - I write device independent semantic pages, and *all* the style goes in the CSS.

I want to author a *single* web page which will display on both my cell phone browser and my 30" LCD - and right now such a page can only contain text.  I can author one version of a page, along with a separate CSS theme for each media type, guessing at suitably sized bitmap images for each type, but this is, simply put, a hack, and a lot of extra work, and can never properly scale for all devices.  Without support for referencing SVG graphics from CSS, I can never create a truly device independent theme for my pages without compromising them semantically (which I am not willing to do).  

Comment 46

[Dão Gottwald [:dao]]

There's quite a difference between IMG and OBJECT/IFRAME. As with SVG loaded from CSS, IMG should limit graphics to be non-interactive, whereas OBJECT would always support the full set of SVG features (scripting in particular). Intrinsic sizing is another point.

Comment 47

[Dão Gottwald [:dao]]

See also http://blog.vlad1.com/archives/2006/10/07/125/

Comment 48

[Toni Ruottu]

 (a general comment)

 - img -

img-tag is an old design error in html,
reminding us from times when other media types
were not widely used. Sometimes it is very
annoying that only text can be used as
alternative representation. alt-attribute is also
often used for something else than an actual
alternative representation for the idea that the
image is used for. Correct alt for a flag picture
in language selector would be something like...

<img src="fr-flag.png" alt="display the page in french" />

 - object -

While it is possible to use object tag for the
same purpose and thus create greater alternative
representations. It doesn't always work because
of lacking browser support. If img-tags had
never been invented this would of course not
be a problem. Anyway, if a web designer wishes
to go for object tags he can do e.g.

<object data="fr-flag.png">
  display the page in <em>french</em>
</object>

 - xhtml 2 -

When it comes to the future, I think xhtml 2
team has finally dropped img-tag completely.

Iirc the idea is that videos, images, etc.
would be themselves alternative representations
for something. The code would look something
along these lines... (pardon me for not checking
the actual syntax)

<p src="map.png">
Germany is located at the center of Europe.
</p>

In the light of this (imo sane) notation paradigm,
even the object tag is questioned, as it might
be by people as a null-tag just to have some tag
to attach the media to. There are already general
null-tags (div and span) and the question is whether
or not use of object adds any useful semantics.

Comment 49

[Martin Eyles]

(In reply to comment #45)
> As a web author who has basically been waiting for this for 6 years now (see
> bug 70978), and since this blocks bug 231179, I will comment...
> 
> I don't author many pages with bar charts or anything where you would put the
> graphics as actual content of the page... like most authors, I use graphics
> mainly for logos/themes/styling via CSS image replacement - I write device
> independent semantic pages, and *all* the style goes in the CSS.

<snip>

I agree with the above, I want bug 231179 to be implemented, which is why I want this blocking bug also resolving. Like Chris Hubick, I would like to have svg as background images. Unlike Chris, I would also like to use svg for graphs etc. - however, for this purpose I find the object tag to be appropriate, using something like a table to represent results.

Therefore, I would be happy to drop this bug *IF* 231179 could be achieved a different way that did not require this.

Comment 50

[Martin Eyles]

using something like a table to represent results. 

should have been

using something like a table to represent results when the svg of the graph cannot be handled, rather than being limited to the img tag's alt keyword and a link to tabulated data.

Comment 51

[Jeff Schiller]

If this means anything - I agree with Martin - SVG as background-image is more important to me than using img

Comment 52

[Joe Drew (not getting mail)]

This is required for bug 435299, and so it should be on the wanted-1.9.1 list (at least).

Comment 53

[Andreas Schildbach]

I'd like to add that Opera 9.5 supports this properly.
Using <object> as an alternative is not an option, because an object is not sensitive to a surrounding <a> tag (<a href...><object .../></a> is not clickable).

Comment 54

[Brett Zamir]

I wonder if this could also be made to work in XUL, if that's not made inevitable when this may be fixed. For example, it'd be great to have (localizable) SVG available within <toolbar image=>, etc.

Comment 55

[Tim Besard]

Is there any news about this bug? Because I'm looking for a way to make SVG files clickable, and as comment 53 describes: <object> tags doesn't seem to be sensitive to surrounding <a> tags.

Comment 56

[Jeff Schiller]

Questions about this.  

I think there is agreement that no scripts will be run in SVG files included with HTML:img elements.  But what if you have:

<a href="blah.html"><img src="foo.svg" /></a>

and foo.svg has clickable regions in it (svg:a links of its own).  What should happen?  What does Opera do?  Is the whole region just treated as a rectangular blob of pixels?  This would seem to make sense as that is what web developers expect from the HTML <img> tag.

What does the HTML5 spec say?

Comment 57

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

The image is treated as a unit. I seem to recall some spec saying this, but I don't know where. Anyway it's the only reasonable thing to do.

Comment 58

[Tomasz Kaźmierczak]

Hello,

I've investigated how Opera handles the SVG inside <img> tag and I think they do this quite reasonably. If this would be helpful, here is a short summary (when refering to "foo.svg" I mean a "foo.svg" inside HTML img: <img src="foo.svg" />):


-First of all, as already decided about the eventual Firefox implementation - all scripts in "foo.svg" are ignored.

-In response to comment #56 - internal links in "foo.svg" are always ignored, whether <img> is surrounded by <a> or not.

-Animation - of course, scripted animation is not being executed, but the SMIL animation is, so if "foo.svg" contains declarative animation, then it's being animated (in my opinion this is something that web developers would be very glad to see once the bug #216462 is resolved).

-The <svg:image> tag - all images inside "foo.svg" are being rendered, whether this is "bar.png" or "bar.svg". But animations in "bar.svg" are ignored (even the declarative ones). So if "foo.svg" is included using HTML <img> then all internal images are treated as rasters (contrary to the situation when "foo.svg" is included using HTML <object> - then the "bar.svg" is animated, as expected).

Comment 59

[Tomasz Kaźmierczak]

I've checked also WebKit (527). It behaves the same way as I described in previous comment, except for the last case - if the foo.svg is embedded into HTML using <img> tag, then WebKit doesn't display the internal bar.svg. Perhaps it's a bug, because bar.svg is being displayed (and animated) if foo.svg is embedded using <object> tag.

However, I've also repeated the tests in Opera, and this time the bar.svg is being animated (SMIL, of course) even if foo.svg is embedded using <img> tag. Don't know, maybe the last time I was checking I made some mistake, but I'm pretty sure that the bar.svg is being animated in both cases (foo.svg embedded using both <img> and <object>). Checked with Opera 9.50 and 9.60.

And there's a weird thing in Opera 10 beta: also in the last case, in this version of Opera, bar.svg is not being animated at all (neither for foo.svg in <img> nor for foo.svg in <object>). Maybe it's a bug, but maybe they decided to change the behaviour for some reason?


So summarizing, Opera and WebKit behave the same way for cases 1, 2 and 3, as described in comment #58. For case 4, Opera (9.5, 9.6) does animate the bar.svg embedded in foo.svg (using <svg:image>) in all cases (foo.svg in <img> and in <object>). However, Opera 10 beta does not. As for WebKit, it has problem with displaying the bar.svg when foo.svg is embedded using <img>, but for foo.svg in <object> everything works fine (the animation also).

Hope I didn't complicate it too much;)

Comment 60

[Tomasz Kaźmierczak]

Attachment: testcases for comments 58 and 59

attached testcases that I used to check what's described in comments 58 and 59. It consists of an html file and several svg files, thus I've put it into an archive.

Comment 61

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

Not making it.

Comment 62

[adeveria]

Does "Not making it" mean it won't be in 1.9.2a1 (the target milestone) or is the feature not expected to make it for 1.9.2 at all?

Comment 63

[Joe Drew (not getting mail)]

This will not make it for 1.9.2.

Comment 64

[worms]

Would it be possible to know, roughly, what prevented this bug, together with Bug 231179 and Bug 272288, to make it in 1.9.2, as we were expecting it so much, and when would its new estimated time of implementation be, if it's already known?
 
Thank you!

Comment 65

[Joe Drew (not getting mail)]

The changes in bug 753 and bug 435296 were being made at the time that svg-in-img development would have been happening, and the two pieces of work would conflict with each other (or, more specifically, depend on each other), so we opted to delay implementation until the other imagelib work was done.

Comment 66

[mackyle]

Is there a parity-chrome tag for whiteboard?

Chrome supports .svg images using img tags and appears to have more market share at this time than either Safari or Opera.

Comment 67

[adeveria]

According to the IE blog [1], SVG in img tags is planned for IE9. Like SVG-in-CSS (see bug 231179), it is also already supported in Safari and Chrome, so I really think it's time Mozilla starts prioritizing this. 

[1] http://blogs.msdn.com/ie/archive/2010/03/18/svg-in-ie9-roadmap.aspx

Comment 68

[Chan Tsz Kin]

Mozilla won't support it. Please use Chrome.

Comment 69

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

You might notice that this bug was assigned to Daniel a little under a month ago, and I believe he's been working on it during that time.

Comment 70

[d]

(In reply to comment #68)
> Mozilla won't support it. Please use Chrome.

How do you know that?

Comment 71

[Daniel Holbert [:dholbert]]

(In reply to comment #69)
> You might notice that this bug was assigned to Daniel a little under a month
> ago

Yeah - I've got some work-in-progress patches that I'll be posting soon that already provide basic support for this.

Comment 72

[Tiago Sá]

(In reply to comment #70)
> (In reply to comment #68)
> > Mozilla won't support it. Please use Chrome.
> 
> How do you know that?

He doesn't, because he's obviously a troll. Chrome doesn't support SVG in favicon. In fact, they barely support anything at all! Last I checked, they only support ICO formats, just like Internet Explorer.

Comment 73

[Tiago Sá]

I'm sorry, I correct myself: Chrome added support for non-animated GIF, JPEG and PNG with Chrome 4.0. Still, no APNG or animated GIF, which are supported in Firefox. And, of course, no SVG either. The only browser that supports it today is Opera.

Of the major browsers, of course.

Comment 74

[WulfTheSaxon [:Wulf]]

(In reply to comment #73)
I actually prefer not to have animated favicons (in fact, that's bug 111373). I imagine the Chrome team had to go out of their way to disable them.

Comment 75

[d]

(In reply to comment #74)
> (In reply to comment #73)
> I actually prefer not to have animated favicons (in fact, that's bug 111373). I
> imagine the Chrome team had to go out of their way to disable them.

The possibility should always be there, for those who use it responsibly. If a site uses it in an annoying way, they could be contacted and asked to stop, and if they still do it, well, then they are going to lose visitors.

Comment 76

[Jonathan Watt [:jwatt]]

Guys, please limit bugzilla chatter to comments that are directly focused on helping fix the bug at hand. That helps devs and reviewers make more efficient use of their time.

Comment 77

[Daniel Holbert [:dholbert]]

For any who are interested, I'm developing my patches for this in a public patch-queue, here:  http://hg.mozilla.org/users/dholbert_mozilla.com/svg-patches/file/

The patches in the queue are still in a very WIP state, but they now "work" in the simple cases I've tested, on SVG documents that specify an explicit px-valued height & width on their <svg> element.  For any who want to give it a spin, I've generated a build with the patches from the queue here:
  http://build.mozilla.org/tryserver-builds/[e-mail]-try-19fe2e4f24ee

It still doesn't work with the testcases attached to this bug (because they don't have height/width specified on their SVG documents' <svg> element), but I've posted some testcases here that it works for here:
http://people.mozilla.org/~dholbert/tests/276431/test_svg_img.html
http://people.mozilla.org/~dholbert/tests/276431/test_svg_background.html

Note that background tiling doesn't work correctly yet.  And I make no stability guarantees. :) (though it hasn't crashed on my machine during recent testing)

Comment 78

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #77)
> For any who are interested, I'm developing my patches for this in a public
> patch-queue, here: 
> http://hg.mozilla.org/users/dholbert_mozilla.com/svg-patches/file/
> 
> The patches in the queue are still in a very WIP state, but they now "work" in
> the simple cases I've tested, on SVG documents that specify an explicit
> px-valued height & width on their <svg> element.  For any who want to give it a
> spin, I've generated a build with the patches from the queue here:
>  
> http://build.mozilla.org/tryserver-builds/[e-mail]-try-19fe2e4f24ee
> 
> It still doesn't work with the testcases attached to this bug (because they
> don't have height/width specified on their SVG documents' <svg> element), but
> I've posted some testcases here that it works for here:
> http://people.mozilla.org/~dholbert/tests/276431/test_svg_img.html
> http://people.mozilla.org/~dholbert/tests/276431/test_svg_background.html
> 
> Note that background tiling doesn't work correctly yet.  And I make no
> stability guarantees. :) (though it hasn't crashed on my machine during recent
> testing)

Are you also going to be adding the feature to use SVG in favicons or would that be considered separately, if at all?

Good work on this. It works great here.

Comment 79

[Daniel Holbert [:dholbert]]

Thanks for the feedback! Yes, we're intending to support SVG favicons -- basically, the intention is to allow you to use SVG in *any* context where you could use a raster image.

Comment 80

[Sebastiaan Dammann]

Indeed, good work.
You said:
> The patches in the queue are still in a very WIP state, but they now "work" in
> the simple cases I've tested, on SVG documents that specify an explicit
> px-valued height & width on their <svg> element. 
Is this done deliberate, or is it just because scaling based on a width and height of an img tag isn't implemented yet?

Comment 81

[Daniel Holbert [:dholbert]]

> > px-valued height & width on their <svg> element. 
> Is this done deliberate

It's just because that's how far I've gotten. :)  I don't intend for that limitation to persist in the final patch.

>  or is it just because scaling based on a width and
> height of an img tag isn't implemented yet?

Just to be clear: scaling based on width/height of the <img> tag *is* actually implemented. That only gives us the size of the <img> viewport, though -- it doesn't tell us what region of the SVG content we should scale to fit that viewport.  

The SVG document can give us this information in a very nuanced way, using any subset of the "width", "height", "viewBox", and "preserveAspectRatio" attributes.  Currently, I just use width & height to get something working, though I'm not sure what I'm doing is the Right Thing yet.

Comment 82

[d]

(In reply to comment #78)
> Are you also going to be adding the feature to use SVG in favicons or would
> that be considered separately, if at all?
> 
> Good work on this. It works great here.

The implementation of SVG in favicons is tracked in bug 366324. And, it's looking really good Daniel, keep it up :-)

Comment 83

[adeveria]

Awesome work, thanks so much for doing this! :)

By the way, here's an (experimental) test page I created a while back:
http://a.deveria.com/svg/css_svg_test.html

Your implementation does pretty well, possibly better than webkit already! I'm not sure if the correct behavior is in any spec (yet), but it certainly should be. Until then I'd try to follow Opera's implementation, it seems the most intuitive IMO.

Keep it up, would be great if this made it to Firefox 4.0.

Comment 84

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

The behavior should be covered by existing specs, though it's spread between a bunch of places:

http://www.w3.org/TR/SVG11/coords.html
http://www.w3.org/TR/CSS21/visudet.html#inline-replaced-width (and other relevant sections of that chapter)
http://www.w3.org/TR/CSS21/colors.html#propdef-background-image
http://www.w3.org/TR/CSS21/generate.html#propdef-list-style-image
http://www.w3.org/TR/css3-background/#the-background-size

Comment 85

[Robert Longson [:longsonr]]

> +    // XXXdholbert double --> int conversion on next line.  Need to apply
> +    // ceiling or anything?
> +    gfxIntSize surfaceSize(size.width, size.height);

Use nsSVGUtils::ConvertToSurfaceSize, see nsSVGPatternFrame or nsSVGFilterInstance

Comment 86

[Daniel Holbert [:dholbert]]

(In reply to comment #85)
> Use nsSVGUtils::ConvertToSurfaceSize, see nsSVGPatternFrame or
> nsSVGFilterInstance

Thanks for the suggestion -- that looks like what I want, except that I can't call nsSVGUtils methods from imglib, since nsSVGUtils lives in gklayout.  (I get linker errors if I try.)  I filed bug 567848 on splitting up nsSVGUtils to fix this.

Comment 87

[Robert Longson [:longsonr]]

Other associated things that may need to change...

I think nsSVGFeaturesList.h can mark http://www.w3.org/TR/SVG11/feature#Image as supported with this patch.

Consider whether nsSVGImageElement needs to support a complete set of mapped attributes now rather than the restricted set it currently supports.

Comment 88

[Daniel Holbert [:dholbert]]

(In reply to comment #86)
> Thanks for the suggestion -- that looks like what I want, except that I can't
> call nsSVGUtils methods from imglib, since nsSVGUtils lives in gklayout.

(I actually ended up fixing this by moving imgContainerSVG to /layout/, BTW, at bz's suggestion in bug 567848.)

About to post a series of mostly-done patches here.  They layer on top of the patch in bug 574529.

Comment 89

[Daniel Holbert [:dholbert]]

Attachment: patch 1: Create imgIContainerRaster

This patch splits off a bunch of the "imgIContainer" interface into a sub-interface, imgIContainerRaster.  The moved methods are generally things relevant to raster images, but not to SVG images.

Comment 90

[Daniel Holbert [:dholbert]]

Attachment: patch 2: Add imgIContainer::GetImageType

This patch adds a method to query an imgIContainer for whether it's an SVG image or a raster image. (There are cases where we'll need different client behavior, depending on the type.)

Comment 91

[Daniel Holbert [:dholbert]]

Attachment: patch 3: Make imgIContainer receive data as an nsIStreamListener

This patch makes imgIContainer implementations (only one so far -- imgContainer) promise to implement the nsIStreamListener interface, and receive data via that interface's methods.

i.e. this replaces the methods "newSourceData", "sourceDataComplete", and a call to inStream->ReadSegments() with their pseudo-equivalents, "OnStartRequest", "OnDataAvailable", and "OnStopRequest".

(We need to have the nsIStreamListener versions of these methods for svg images, because we need all of their arguments in order to pass them through to our SVG parser, which is also an nsIStreamListener.)

Comment 92

[Daniel Holbert [:dholbert]]

Attachment: patch 4: Add method imgIContainer::GetRootLayoutFrame

This adds a method "GetRootLayoutFrame" to let us query an imgIContainer for its root layout frame  (its nsSVGOuterSVGFrame).

This allows a "host" nsImageFrame to query its "guest" SVG image for its outermost frame's intrinsic size[1] & intrinsic ratio[2] and then delegate sizing decisions to nsLayoutUtils::ComputeSizeWithIntrinsicDimensions().

(Note: This new "GetRootLayoutFrame" method doesn't have a useful implementation or any callers yet -- that comes in a later patch.  This patch just extends the interface & adds a stub impl to the imgContainer class.)

[1] via nsIFrame::GetIntrinsicSize() (note that this method supports percent heights/widths, *unlike* imgIContainer::GetHeight/Width)
[2] via nsIFrame::GetIntrinsicRatio()

Comment 93

[Daniel Holbert [:dholbert]]

Attachment: patch 5: nsImageFrame stores intrinsic size & ratio

This patch adds client code for GetRootLayoutFrame(), in nsImageFrame:
 - Makes nsImageFrame store its image's intrinsic size as an nsIFrame::IntrinsicSize, rather than as a nsSize.  This lets it support percent-valued intrinsic sizes, and (as mentioned above) pass those directly to nsLayoutUtils::ComputeSizeWithIntrinsicDimensions (which expects an nsIFrame::IntrinsicSize as its argument)
 - Makes nsImageFrame store its image's intrinsic ratio, too. (which ComputeSizeWithIntrinsicDimensions also expects)

(The above only matters if we've got an implementation for GetRootLayoutFrame that returns something non-null -- which we don't yet, but we'll get that in a later patch)

Comment 94

[Daniel Holbert [:dholbert]]

Attachment: pre-patch: make args to imgIContainer::Draw const [landed]

This patch is trivial -- it just adds 'const' to some input args that are passed-by-reference to imgIContainer::Draw.

Comment 95

[Daniel Holbert [:dholbert]]

Attachment: patch 7: make imgIContainer::Draw take viewport size

This patch makes imgIContainer::Draw take an additional argument -- the "viewport size" of the image.  (the dimensions that the client wants us to use for drawing, basically)

We need this for SVG images to draw correctly, particularly when they have the |viewBox| and optionally |preserveAspectRatio| attributes on their outermost <svg> element. (those attributes specify how SVG content should be positioned & scaled to fit an arbitrary viewbox)

Comment 96

[Boris Zbarsky [:bzbarsky]]

> This adds a method "GetRootLayoutFrame" to let us query an imgIContainer

Why wouldn't we just have methods for getting the intrinsic size and ratio on the container instead?  Under the hood those could still talk to the nsIFrame involved, as needed.

Comment 97

[Daniel Holbert [:dholbert]]

Attachment: patch 8: add viewport-size argument to clients of imgIContainer::Draw

This patch fixes callers of imgIContainer::Draw to pass the viewport-size as an argument. (the argument added in the previous patch)

NOTE: In some cases (in particular nsLayoutUtils::DrawSingleUnscaledImage and ::DrawImage), the 'viewport size' is purely based on what the imgIContainer returns from GetHeight & GetWidth.  This is a bit of a problem for that method, when we call it on an SVG image that has a percent height and/or width, because then its GetHeight/GetWidth method(s) will fail and return a height/width value of 0, which makes us bail out in an NS_ENSURE_TRUE(imageSize.width > 0 ...); call.

I've added XXX comments to highlight these issues for now -- I'm not worrying about it very much right now, since I think this problem will be easier to fix after bug 506826 & friends land -- I think that bug has to work around this issue, too, for -moz-element clients.

This means that (for now) this bug's patches don't support SVG images for CSS backgrounds *unless* the <svg> element has a non-percent-valued height & width on it. (so that imgIContainer::GetHeight & imgIContainer::GetWidth can both return meaningful values, as nsLayoutUtils::Draw[SingleUnscaledImage] expect them to)

Comment 98

[Daniel Holbert [:dholbert]]

Attachment: patch 9: main patch -- add imgContainerSVG

Finally, here's the "main" patch. :)  This is the last of this patch-series.

This adds a class "imgContainerSVG", which is a subclass of imgIContainer. The imgContainerSVG class outsources a lot of its work to another new class, "nsSVGDocumentWrapper", which basically encapsulates our helper SVG document and exposes just the things that imgContainerSVG needs access to.

WHAT WORKS (AFAIK):
 - <img> (I've tested this the most, and I think its support is pretty robust).
 - CSS background & list-style-image & border-image *only if* you have a px height/width on the <svg> element (see comment 97)
 - SVG favicons (though you probably want to provide a viewBox to tell the favicon how to scale -- otherwise, not much SVG content fits in a 16x16 viewport. :))

Note: <svg:image xlink:href="foo.svg"> does *not* work right now -- need to look into that more.

CAVEATS:
There are a number of things in this patch that aren't quite done -- generally they're flagged with // XXXdholbert comments.  Some of these are:
  - Currently in OnDataComplete(), I make the SVG parser finish parsing synchronously, for simplicity. (This lets us immediately notify our image-decoder-observers that we're done and they can draw whenever they want.)  This works, it'd be better to let the parsing finish asynchronously.
  - Animated SVG documents use a helper thread for "animation update, repaint me!" callbacks.  Right now that's just on a timer, but ideally it should plug in to the helper-document's nsRefreshDriver, I think.  In the meantime, the helper-thread paints somewhat infrequently (80ms) so as not to hog the CPU too much.
  - The patch includes a workaround for a shutdown null-crash in nsJARChannel.cpp that I hit on one of my machines, when I quit a build with this patch.  For some reason, we try to release-and-null-out a pointer there twice -- probably due to me not cleaning something up correctly.  (I work around it by changing the NS_RELEASE to NS_IF_RELEASE, so the second release gets skipped.  Need to debug this further.)

Just kicked off a TryServer build with these patches applied -- builds should appear at this URL (404 right now, but shouldn't be in an hour or so):
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-d1fd3e7bcdc1/

Comment 99

[Daniel Holbert [:dholbert]]

(In reply to comment #96)
> > This adds a method "GetRootLayoutFrame" to let us query an imgIContainer
> Why wouldn't we just have methods for getting the intrinsic size and ratio on
> the container instead?  Under the hood those could still talk to the nsIFrame
> involved, as needed.

That was my initial instinct too, but there are complications with adding a "GetIntrinsicSize" accessor to imgIContainer (see below).  roc & joe & I talked this over in #gfx a week or so ago, and settled on the nsIFrame getter as a reasonable solution.

So the simplest solution would be to have imgIContainer::GetIntrinsicSize, which would return an nsIFrame::IntrinsicSize (which, importantly, can contain both percent and px values).  But that doesn't work, because imglib doesn't know about nsIFrame, so it can't use nsIFrame::IntrinsicSize (or about nsStyleCoord, which is what IntrinsicSize uses under the hood for its component parts).  So, to get around that, we could create a new "nsStyleCoord-like" structure (which would need to support both px & percent-values) and an "IntrinsicSize-like" wrapper for it, somewhere in /gfx/.  And we'd use that to replace nsIFrame::IntrinsicSize.  But that would result in semi-duplicated code, which isn't great either -- we already have a lot of px-or-percent-[or-something-else] value types, e.g. nsCSSValue, nsStyleCoord, nsStyleAnimation::Value, and probably a few more -- and adding yet another one for just this one purpose isn't ideal.

Ultimately, we should probably refactor one or more of the existing "enumerated-meaning" value types to live in GFX and be suitable for this purpose -- but for now, the nsIFrame* accessor is something simple that avoids all that hassle.

Comment 100

[Daniel Holbert [:dholbert]]

(In reply to comment #99)
> But that doesn't work, because imglib doesn't
> know about nsIFrame

Sorry, I misspoke there -- imglib may know about nsIFrame (at least enough to be able to return an nsIFrame* :)) -- but it does *not* know about /layout/style/ (where nsStyleCoord lives), which is what prevents it from directly using nsIFrame::IntrinsicSize.

Comment 101

[Boris Zbarsky [:bzbarsky]]

> Ultimately, we should probably refactor one or more of the existing
> "enumerated-meaning" value types to live in GFX and be suitable for this
> purpose

OK, that I buy.  File the followup bug?

Comment 102

[Daniel Holbert [:dholbert]]

Filed bug 576202.

Comment 103

[Daniel Holbert [:dholbert]]

Attachment: patch 9: main patch -- add imgContainerSVG

Here's an updated version of patch 9 -- this fixes a bug I just caught when testing the TryServer build. (I was missing didn't the member variable 'mHaveRestrictedRegion' from the imgContainerSVG initializer list, which then led to crashes from requesting too large of a surface.)  I never saw that in my local debug build for some reason -- perhaps one of my build flags made member-data initialize to 0 by default or something, which saved me from the crash.

In any case, I pushed a new less-crashy try-server build, with this fixed:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-6baee70a294b/

Comment 104

[fantasai]

I'm a little concerned about adding support for SVG images but requiring that they have an explicit 'width' and 'height'. Using an SVG image with a viewport (aspect ratio) but no fixed 'width' and 'height' strikes me as an important use-case. Even if we can't deal with explicit percents, can we at least handle missing dimensions?

Comment 105

[Boris Zbarsky [:bzbarsky]]

> but requiring that they have an explicit 'width' and 'height'

This patch only requires that for images used as CSS backgrounds, right?

> Using an SVG image with a viewport (aspect ratio) but no fixed 'width' and
> 'height' strikes me as an important use-case.

It is, but such use requires that a size be specified, no?  So there are various cases (generated content images in CSS, <img> with no size specified, and so forth) where it just can't work, as far as I can tell.

Comment 106

[Daniel Holbert [:dholbert]]

(In reply to comment #104)
> I'm a little concerned about adding support for SVG images but requiring that
> they have an explicit 'width' and 'height'.

Sorry for not being clearer about that -- that's a limitation of the *current* (WIP) patches, but I don't think that limitation will remain in the final version (once I merge with changes in bug 506826, which I believe increases our awareness of viewport-size when painting).

I expect there will be edge cases where we can't reasonably paint anything, but there's generally something sensible we can fall back on.

Note also that there's *always* specified dimensions on the <svg> document -- it's just that the default values are "100%", and percent units are only meaningful if we have a percent basis (which is hard to get, with the wall of separation at the imgIContainer interface-barrier).

If all else fails, though, as long as we end up with a nonempty region to paint into, we can just apply the percent values to that region.  The problem right now is that existing code (for e.g. CSS backgrounds) makes assumptions that the pixel-unitted "GetHeight()" & "GetWidth()" methods are always useful, and unfortunately that's not the case for an SVG image with %-valued dimensions.

(In reply to comment #105)
> This patch only requires that for images used as CSS backgrounds, right?

Yes, and I think for border-background and list-style-image too -- I haven't tested those thoroughly.

> <img> with no size specified [...]  where it just can't work

That one actually works fine with the current patch-stack, FWIW -- I matched our behavior for <embed> & used our existing code to follow the CSS rules for a replaced CSS element.  The logic for that exists already in nsLayoutUtils::ComputeSizeWithIntrinsicDimensions() -- I just needed to pass it the nsSVGOuterSVGFrame's IntrinsicSize (allowed to be %-valued) & intrinsic ratio. (That's where having the root nsIFrame* comes in handy. :))

Comment 107

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

Once the patches in bug 506826 have all landed, we'll have a default image size for CSS background painting.

But there are still places in CSS where you don't have a default image size. For example, list item bullets and content:url(...). IIRC for list item bullets, Webkit just uses something based on the font size. No idea what to do for content:url(...) though.

Comment 108

[Markus Stange [:mstange]]

(In reply to comment #106)
> (once I merge with changes in bug 506826, which I believe increases our
> awareness of viewport-size when painting).

It doesn't really. The only part that touches CSS background sizing is the change to ImageRenderer::ComputeSize in attachment 451963 [details] [diff] [review], but that's special-cased to -moz-element. Filling the whole element (or the whole background-size rect) is what already happens for CSS gradients, and I'm just doing the same for SVG patterns and gradients by setting mSize = aDefault. And then in ImageRenderer::Draw I'm passing mSize to nsSVGIntegrationUtils::DrawPaintServer. The eStyleImageType_Image branch ignores mSize, so maybe that's what you need to change.

Comment 109

[fantasai]

> But there are still places in CSS where you don't have a default image size.

I believe that is no longer true. If you find a case where it's not specified, let me know, but the ones you mention are specified. There's a set of rules in the CSS2.1 Lists chapter for list item bullets, and content:url() should be treated the same as any other replaced element.

Comment 110

[Joe Drew (not getting mail)]

Comment on attachment 455344 [details] [diff] [review]
pre-patch: make args to imgIContainer::Draw const [landed]

Change the iid of imgIContainer.

Comment 111

[Daniel Holbert [:dholbert]]

Comment on attachment 455344 [details] [diff] [review]
pre-patch: make args to imgIContainer::Draw const [landed]

Landed (trivial) patch 6, with UUID rev per joe's comment:
http://hg.mozilla.org/mozilla-central/rev/e0d080bd0762

Comment 112

[Damon Sicore (:damons)]

Any more progress here?

Comment 113

[Daniel Holbert [:dholbert]]

Yup -- I've got <svg:image> working, and I've addressed the caveats that I brought up in comment 98, except for the first one[1]. (plus other misc cleanup / fixes)

I'm currently merging in changes from the recently-landed Bug 572520, which shifted towards using imgContainer instead of imgIContainer in some places (breaking this bug's patches) and which changed a bunch of other code out from under this bug's patches.

FWIW, this bug also requires the final patch that ends up on Bug 359608 (which lets us detect when images are only used in bfcache-frozen pages, so we can pause their animations and save on CPU & battery).  It looks like that bug's almost done -- once it is, I'll use its functionality in one of this bug's patches.  Without that, this can get pretty CPU-heavy if you end up with multiple SMIL-animated SVG images in your bfcache.

[1] We'll still need to synchronously finish parsing the SVG document in OnDataComplete for now -- I tried making it async, but after wrestling with that & talking to bholley, I don't think that can work, due to assumptions made by imagelib about height/width being available as soon as we've received all the data. (might be able to optimize that later, but not worth worrying about right now)

Comment 114

[Emil Ivanov]

(In reply to comment #113)

> FWIW, this bug also requires the final patch that ends up on Bug 359608 (which
> lets us detect when images are only used in bfcache-frozen pages, so we can
> pause their animations and save on CPU & battery). 

One question: Will it be possible to pause the animations with escape key like animated GIFs?

Comment 115

[Daniel Holbert [:dholbert]]

Yes.

Comment 116

[Joe Drew (not getting mail)]

This has to block at latest beta 5, because that's the feature freeze.

Comment 117

[Jess Holle]

When is this going to be fixed?  In Firefox 3.6.x?  In Firefox 4?

This bug has succeeded in making Firefox's SVG support utterly useless for me (as for practical reasons I need to use an img tag to present an image across all browsers and then selectively produce SVG for browsers that have reasonable SVG handling).

Comment 118

[d]

(In reply to comment #117)
> When is this going to be fixed?  In Firefox 3.6.x?  In Firefox 4?

New features for web developers can't be introduced in minor versions for obvious reasons. If resolved in time, it will be in Firefox 4.

Comment 119

[Jess Holle]

One could easily argue that this is not a new feature -- as Firefox gives every appearance that SVG in an img tag should work until it doesn't.  That said, I can understand if this has to wait until Firefox 4.  I *cannot* understand if this is not addressed in Firefox 4 -- that would scream to me that Firefox is not going to keep up in the browser arena, address longstanding gaps, etc, i.e. that we should start emphasizing Chrome -- and maybe even MSIE 9 (!?!)

Comment 120

[Daniel Holbert [:dholbert]]

The plan is to have this implemented in Firefox 4, yes.  (see comment 116 -- "beta 5" there means Firefox 4 beta 5)

Comment 121

[Grégoire Colbert]

(In reply to comment #120)
> The plan is to have this implemented in Firefox 4, yes.  (see comment 116 --
> "beta 5" there means Firefox 4 beta 5)

Hi! I just downloaded Firefox 4 beta 3, and <img src="myfile.svg"/> still doesn't seem to work. However, I read at :

https://developer.mozilla.org/En/Firefox_4_for_developers

that it is supported. Is there something to be done in my tags, or will it ship only in beta 5? Thanks!

Comment 122

[Daniel Holbert [:dholbert]]

It'll ship in beta 5. (That's why this bug here is still open.)

Comment 123

[Daniel Holbert [:dholbert]]

Comment on attachment 455335 [details] [diff] [review]
patch 1: Create imgIContainerRaster

Obsoleting old/bitrotted patches, to post a new patch-stack. (Note that I've actually landed variants of old-patches 1 and 2 -- imgIContainerRaster & Get[Image]Type -- as parts of bug 584841.)

Comment 124

[Daniel Holbert [:dholbert]]

Note: future patches here all layer on top of the patches for bug 587371, bug 574529, bug 587779, bug 587800, and bug 587902. (all of which are in this bug's depends-list, and all of which are ready or nearly-ready to land, I think)

Comment 125

[Daniel Holbert [:dholbert]]

Attachment: Patch 1: Pass through imgRequest::OnStartRequest/OnDataAvailable/OnStopRequest for Vector-type images

This patch makes us pass imgRequest::OnStartRequest/OnDataAvailable/OnStopRequest calls on through to the image, for TYPE_VECTOR images.

Comment 126

[Daniel Holbert [:dholbert]]

Attachment: Patch 2: Add method imgIContainer::GetRootLayoutFrame

This adds the method imgIContainer::GetRootLayoutFrame, with a no-op impl for RasterImage.

(We'll need this in layout code in order to get percentage heights/widths and intrinsic ratios from VectorImages' internal SVG documents.)

I made this [notxpcom] because it never needs to fail (it can just return null in failure-cases), and because checking nsresult return-values is clunky (and unnecessary in this case since there are no special failure codes).

Comment 127

[Daniel Holbert [:dholbert]]

Attachment: Patch 3: Make nsImageFrame store an intrinsic ratio & use an nsIFrame::IntrinsicSize for its intrinsic size (to allow % vals)

Comment 128

[Daniel Holbert [:dholbert]]

Attachment: Patch 4: Add nsIDocument::IsBeingUsedAsImage

This lets us check if our document is being used as an image, which lets us take the right path in DocumentViewer setup (to get & set up a presentation context).

Comment 129

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

       if (aPresContext->CompatibilityMode() == eCompatibility_NavQuirks) {
-        mIntrinsicSize.SizeTo(nsPresContext::CSSPixelsToAppUnits(ICON_SIZE+(2*(ICON_PADDING+ALT_BORDER_WIDTH))),
-                              nsPresContext::CSSPixelsToAppUnits(ICON_SIZE+(2*(ICON_PADDING+ALT_BORDER_WIDTH))));
+        nscoord edgeLengthToUse =
+          nsPresContext::CSSPixelsToAppUnits(
+            ICON_SIZE+(2*(ICON_PADDING+ALT_BORDER_WIDTH)));
+        mIntrinsicSize.width.SetCoordValue(edgeLengthToUse);
+        mIntrinsicSize.height.SetCoordValue(edgeLengthToUse);

Shouldn't we be setting the ratio here too? And maybe nsImageFrame::EnsureIntrinsicSize should be renamed to EnsureIntrinsicSizeAndRatio?

       // Have to size to 0,0 so that GetDesiredSize recalculates the size
-      mIntrinsicSize.SizeTo(0, 0);
+      mIntrinsicSize.width.SetCoordValue(0);
+      mIntrinsicSize.height.SetCoordValue(0);

And we should set the intrinsic ratio here too?

Regarding nsImageFrame::RecalculateTransform, we shouldn't be caching a transform in the first place, we should just have a CalculateTransform method that returns the transform whenever we need it. And it should be able to return failure if we can't, in which case the two users of mTransform can just invalidate the entire frame.

+  // XXXdholbert The caller expects a px-valued width & height, but our
+  // intrinsic width and/or height has a percentage value.  Just fail for now.
+  // (Might eventually want to make this method return nsIFrame::IntrinsicSize,
+  // though.)  NOTE: There are only two clients of this method right now:
+  // nsVideoFrame::GetVideoIntrinsicSize and
+  // nsHTMLReflowState.cpp:GetIntrinsicSizeFor().

So what happens when this failure occurs, in those two callers?

Comment 130

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

BTW, one thing I just thought of: if someone calls canvas.drawImage on an SVG image, we need to mark the canvas as tainted for getImageData, since that SVG image might be same-origin with the canvas document but refer to external resources (images, IFRAMEs, etc) from different origins, and we must avoid creating a cross-origin information leak here.

Comment 131

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

Comment on attachment 467866 [details] [diff] [review]
Patch 4: Add nsIDocument::IsBeingUsedAsImage

Needs content peer sr

Comment 132

[Daniel Holbert [:dholbert]]

Attachment: Patch 5: Move nsSVGUtils::ConvertToSurfaceSize & ClampToInt into header file

This patch moves two nsSVGUtils methods into the header file, so they can be inlined for (soon-to-be-added) callers in imagelib. (Otherwise, non-libxul builds fail to link, since nsSVGUtils.cpp is part of gklayout, and imagelib is linked *by* gklayout, and can't link *with* gklayout.)

This includes an XXX comment to indicate that ConvertToSurfaceSize should move back to the .cpp file once we move to a libxul-only world (when the linking issues will go away.)

Comment 133

[Daniel Holbert [:dholbert]]

Attachment: Patch 5: Move nsSVGUtils::ConvertToSurfaceSize & ClampToInt into header file

(oops, attached wrong patch-version before)

Comment 134

[Daniel Holbert [:dholbert]]

(In reply to comment #129) (RE patch 3 review comments):
> Shouldn't we be setting the ratio here too?

Ah, good catch.  Fixed (in patch queue).

> And maybe
> nsImageFrame::EnsureIntrinsicSize should be renamed to
> EnsureIntrinsicSizeAndRatio?

Yup. Changed.

> And we should set the intrinsic ratio here too?

Yes - fixed.

> Regarding nsImageFrame::RecalculateTransform, we shouldn't be caching a
> transform in the first place, we should just have a CalculateTransform method
> that returns the transform whenever we need it. And it should be able to return
> failure if we can't, in which case the two users of mTransform can just
> invalidate the entire frame.

That sounds good. (Still need to do that - might file as a separate bug.)

RE GetIntrinsicImageSize:
> +  // though.)  NOTE: There are only two clients of this method right now:
> +  // nsVideoFrame::GetVideoIntrinsicSize and
> +  // nsHTMLReflowState.cpp:GetIntrinsicSizeFor().
> So what happens when this failure occurs, in those two callers?

Right now, they'll allocate 0 space for it (which would affect a video element with a SVG poster-image, in the former case, and I think for a floated SVG image, in the latter case -- whatever nsHTMLReflowState::CalculateHypotheticalBox is used for).  That's probably not what we want.

It'd be better to explicitly fail, rather than returning a zero-size here.  Both callers have fallback code nearby that would be sensible to use, but they don't actually check the return-status of GetIntrinsicImageSize() yet -- I'll just make them check the return status and then use the relevant fallback code.

Comment 135

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

Sounds good. So I'm waiting for a new part 3?

Comment 136

[Daniel Holbert [:dholbert]]

Comment on attachment 467864 [details] [diff] [review]
Patch 3: Make nsImageFrame store an intrinsic ratio & use an nsIFrame::IntrinsicSize for its intrinsic size (to allow % vals)

Yup. Coming up.

Comment 137

[Daniel Holbert [:dholbert]]

Attachment: Patch 3 v2: Make nsImageFrame store an intrinsic ratio & use nsIFrame::IntrinsicSize

Updated version of Patch 3, to address comment 129:
 - Makes nsImageFrame::GetIntrinsicImageSize simply fail if we don't have coord-unit dimensions.
 - Adds nsresult-failure-checking code in clients of GetIntrinsicImageSize (in nsHTMLReflowState & nsVideoFrame), using existing fallback code.
 - s/EnsureIntrinsicSize/EnsureIntrinsicSizeAndRatio/
 - Set mIntrinsicRatio alongside mIntrinsicSize where roc suggested - to 1,1 in the first case (where we're setting ourselves to be a square icon) -- and 0,0 in the second (where we're clearing our intrinsic size).

nsImageFrame::RecalculateTransform is the same as in the previous patch, except I removed the XXX comment there. I'll replace it with dynamic transform-calculation in a followup patch.

Comment 138

[Daniel Holbert [:dholbert]]

Attachment: Patch 3b: nsImageFrame computes its transform on-demand

This patch has us regenerate the nsImageFrame's transform on-demand. If we fail, it makes SourceRectToDest just return GetInnerArea(), which I think is the relevant nscoord-space rect to return, since that's what we used in RecalculateTransform.

(caveat: haven't tested this yet (though it does compile) -- I'll run it through tryserver to make sure it doesn't break anything)

Comment 139

[Daniel Holbert [:dholbert]]

(to follow up on the end of comment 138: the patch-stack up through 3b was fine on tryserver)

Comment 140

[Daniel Holbert [:dholbert]]

Attachment: Patch 6: Add nsSMILAnimationController::HasRegisteredAnimations

This is a trivial patch to let us check whether our document's animation controller has any registered animations. (This lets us know whether to bother scheduling periodic repaints of SVG images, for animation updates.)

Comment 141

[Daniel Holbert [:dholbert]]

Attachment: Patch 7: Add special nsIntRect value "kFullImageSpaceRect"

This patch defines a special INT_MAX-sized nsIntRect, called "kFullImageSpaceRect", in imgIContainer.idl.

This rect is to be used to represent "the whole (unbounded) image space", in places where SVG Images need to pass a dirty-rect but don't have a rect that makes sense to patch.

I initially tried *declaring* this variable in imgIContainer.idl and then *defining* it in Image.cpp, but that gives me linking errors when I try to refer to it from layout code.  The existing solution (defining it in the interface) compiles & links, but I think at the cost of giving each compilation unit its own (static const) copy of kFullImageSpaceRect...  If there's something better I could be doing (to define this variable such that it can be referred to from gklayout & imagelib), I'd love to know.

ALSO: I don't actually use kFullImageSpaceRect in any imgIContainer.idl APIs, but rather in imgIDecoderObserver and imgIContainerObserver. (patches to add that code is coming up next)  Nonetheless, I'm putting it in imgIContainer.idl because it's a single common file that all clients of both imgIDecoderObserver and imgIContainerObserver will already have to #include.

Comment 142

[Daniel Holbert [:dholbert]]

Attachment: (not patch 8; attached wrong file)

This patch checks for the special "full image space" rect (added in Patch 7) in the only non-trivial* imgIDecoderObserver::OnDataAvailable implementation.  If there's a match, we invalidate GetInnerArea(), like in patch 3b.

(This is needed because my VectorImage class will call this method with kFullImageSpaceRect as the dirty-rect.)

This patch also shifts the early-return cases in that method to be a bit earlier, so we don't waste time working with rects for no reason.

* Note that all other imgIDecoderObserver::OnDataAvailable implemenations are either no-ops ("return NS_OK;"), or forwarders (just passing the message on to another imgIDecoderObserver).

Comment 143

[Daniel Holbert [:dholbert]]

Attachment: Patch 8: check for kFullImageSpaceRect in imgIDecoderObserver::OnDataAvailable

(oops, attached wrong file; here's the real patch 8)

Comment 144

[Daniel Holbert [:dholbert]]

Attachment: Patch 9: check for kFullImageSpaceRect in imgIContainerObserver::OnFrameChanged

Same idea as previous patch, but now in imgIContainerObserver::OnFrameChanged. (This is the method that's used for e.g. notifying observers that there's been an animation update.)

Comment 145

[Daniel Holbert [:dholbert]]

Attachment: Patch 10: Make imgContainer::Draw take image-viewport-size, and improve nsLayoutUtils drawing wrappers

This patch does two related things:
 - Adds a new argument to imgIContainer::Draw -- a rect which corresponds to the full image viewport size -- which VectorImage needs in order to be able to size & position its contents correctly when it's got a |viewBox| attribute.
 - Adds a new method "nsLayoutUtils::ComputeSizeForDrawing" which just uses the intrinsic size & intrinsic ratio to make a best guess about an intrinsic size to use for our image.

Comment 146

[Joe Drew (not getting mail)]

Comment on attachment 468703 [details] [diff] [review]
Patch 7: Add special nsIntRect value "kFullImageSpaceRect"

Put kFullImageSpaceRect in mozilla::imagelib and r=me.

Comment 147

[Daniel Holbert [:dholbert]]

(In reply to comment #146)
> Put kFullImageSpaceRect in mozilla::imagelib and r=me.

Ok - thanks!

(In reply to comment #145)
>  - Adds a new method "nsLayoutUtils::ComputeSizeForDrawing" which just uses the
> intrinsic size & intrinsic ratio to make a best guess about an intrinsic size
> to use for our image.

(sorry, to clarify, I meant "to make a best guess about a **px-valued** intrinsic size to use for our image", since GetHeight / GetWidth may fail for SVG images that have percent heights/widths)

Comment 148

[Joe Drew (not getting mail)]

Comment on attachment 468735 [details] [diff] [review]
Patch 10: Make imgContainer::Draw take image-viewport-size, and improve nsLayoutUtils drawing wrappers

It is very odd to me that we set the draw size to the destination size in DrawSingleImage, but I guess it makes sense since SVG images don't always have an intrinsic size.

Comment 149

[Daniel Holbert [:dholbert]]

(In reply to comment #148)
> It is very odd to me that we set the draw size to the destination size in
> DrawSingleImage, but I guess it makes sense since SVG images don't always have
> an intrinsic size.

Right -- or more imporantly, an SVG image may have an intrinsic size and/or ratio, but it's not tied to that size and/or ratio so much that it's willing to be deformed to maintain it.[1]

IIRC, image-size & destination-size in DrawSingleImage are used to calculate how much we'll have to stretch the [raster] image along each dimension to get it to fit our destination-area.  But we don't want or need to do that for SVG.  We can just give our SVG content the viewport-size that we'd like it to draw into, and it'll adapt to that, scaling itself if necessary (if it has a |viewBox| attribute on the <svg> node).[2]

[1] Unless they have preserveAspectRatio="none" -- but even in that case, the helper-document will manage the stretching for us, based on the viewport we give it.


[2] If our SVG content doesn't have a viewBox attribute, it won't auto-scale -- it'll just crop out the region of SVG-space covered by the image -- but I think that's reasonable behavior, given that it's what happens with <embed> and with svg-viewed-directly-in-a-browser

Comment 150

[Daniel Holbert [:dholbert]]

Attachment: (stale version of Patch 11)

Here's a patch with a stub implementation of VectorImage, with only the very basic:
 - changes to Makefile
 - VectorImage class definition
 - VectorImage construction at the right spot in imgRequest
 - Both versions of VectorImage::GetType implemented (since they're one-liners and imgRequest calls one of them right away)
 - All the rest of the methods stubbed out. (to be filled in by a subsequent patch).

Comment 151

[Daniel Holbert [:dholbert]]

Attachment: Patch 11: Stub for VectorImage class

(here's patch 11 again - first version had one minor piece of stale state from some patch queue reordering I'd done)

Comment 152

[Daniel Holbert [:dholbert]]

Attachment: Patch 4b: Refuse to create a widget for documents that are actually images

Here's a followup to go with patch 4 (nsIDocument::IsBeingUsedAsImage), which I needed to add last night, when merging in changes from bug 582057's landing.

This patch just keeps us from creating a widget during DocumentViewer setup[1], for documents that are being used as images. (If we try to create a widget, we end failing, since we have no connection to our parent widget, and the widget-creation code is much more strict about that.)  We don't need a dedicated widget anyway, since we always end up being provided with a context to draw into.

[1] Note: documentviewer setup happens in the next patch I'll attach (#12)

Comment 153

[Daniel Holbert [:dholbert]]

(In reply to comment #152)
> the widget-creation code is much more strict about that.
er, I meant "is now more strict about that" (since bug 582057)

Comment 154

[Daniel Holbert [:dholbert]]

Attachment: Patch 12: SVGDocumentWrapper class

This patch adds mozilla::imagelib::SVGDocumentWrapper, a helper-class to let VectorImage be a little less complex.

The idea is that VectorImage doesn't directly set up, tear down, or interact with our helper SVG document -- it lets SVGDocumentWrapper manage that.

Requesting r?joe since it's in imagelib, and r?roc since a lot of the code is calling methods from SVG and content.

Comment 155

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

Comment on attachment 468809 [details] [diff] [review]
Patch 4b: Refuse to create a widget for documents that are actually images

Maybe instead of IsBeingUsedAsImage, we should have called that method IsHeadless or something...

Comment 156

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

+    SVGDocumentWrapper::kSVGAtom =
+#ifdef MOZ_ENABLE_LIBXUL
+      nsGkAtoms::svg;
+#else
+      NS_NewPermanentAtom(NS_LITERAL_STRING("svg"));
+#endif

I think we should just take the NS_NewPermanentAtom path in both cases. less fragile.

+PRBool
+SVGDocumentWrapper::GetWidthOrHeight(PRBool   aIsWidth,
+                                     PRInt32& aResult)

Weird API. Why not just make it GetIntrinsicSize() and return an nsIntSize, with -1 for width/height if we don't know them?

+  nsSVGSVGElement* rootElem = GetRootSVGElem();
+  NS_ABORT_IF_FALSE(rootElem, "root elem missing or of wrong type");

Can't this happen if someone serves image/svg+xml with a non-<svg> root?

+  if (presShell)
+    presShell->FlushPendingNotifications(Flush_Layout);

{}

Comment 157

[Daniel Holbert [:dholbert]]

Attachment: Patch 12 v2: SVGDocumentWrapper class

(In reply to comment #156)
> I think we should just take the NS_NewPermanentAtom path in both cases. less
> fragile.

Sounds good - fixed.

> +PRBool
> +SVGDocumentWrapper::GetWidthOrHeight(PRBool   aIsWidth,
> +                                     PRInt32& aResult)
> 
> Weird API. Why not just make it GetIntrinsicSize() and return an nsIntSize,
> with -1 for width/height if we don't know them?

Hopefully that's illuminated by the next patch (posting soon).

This method is primarily to service VectorImage's implementations of imgIContainer::GetWidth and GetHeight APIs. Those methods' clients almost always call both methods back-to-back. Given that, it seemed wasteful to look up *both* width and height within each of those calls.

Also, the required code for looking up width/height is basically the same (with just s/Width/Height/), so I think it makes sense to share code for them at some level.

I'd be open to changing this if you still think I should, though.

> +  nsSVGSVGElement* rootElem = GetRootSVGElem();
> +  NS_ABORT_IF_FALSE(rootElem, "root elem missing or of wrong type");
> 
> Can't this happen if someone serves image/svg+xml with a non-<svg> root?

[this is inside of SVGDocumentWrapper::GetWidthOrHeight]

Good question, but no -- that problem would get caught by the VectorImage (in the next patch) before it'd call this method.  (It checks an mError flag before invoking its SVGDocumentWrapper, and that error flag gets set if we receive XML content without a SVG root)

> +  if (presShell)
> +    presShell->FlushPendingNotifications(Flush_Layout);
> 
> {}

Fixed.

Comment 158

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

(In reply to comment #157)
> I'd be open to changing this if you still think I should, though.

No, that makes sense.

Comment 159

[Daniel Holbert [:dholbert]]

Attachment: Patch 13: VectorImage impl

Here's the final patch, with the "VectorImage" class.

A few notes:
 - I want/need to clean up "Draw" & its helper function "RenderToSurface" before landing.  (Right now it always creates a helper gfxASurface, but we really only need to do that if we're tiling.  It also can still benefit from mstange's work in bug 572680 - I haven't incorporated that yet)

 - I leave imgIContainer::CopyFrame unimplemented in this patch. It's not used at all for <img> or for CSS backgrounds, though it is used for <svg:image>.  I have a patch that implements it & fixes up SVG's <svg:image> client-code, but I haven't tested it as much yet, and I'd prefer to do that work in a followup bug.

Comment 160

[Daniel Holbert [:dholbert]]

(In reply to comment #159)
>  - I leave imgIContainer::CopyFrame unimplemented in this patch. It's not used
> at all for <img> or for CSS backgrounds, though it is used for <svg:image>.

The same applies to imgIContainer::GetFrame -- that's what canvas's "drawImage" uses, actually, so comment 130 isn't an issue until that's implemented.

Comment 161

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

+  // XXXdholbert This method also doesn't actually freeze animation in the
+  // returned imgIContainer, because it shares our helper-document. If that's
+  // important, we'll need to duplicate the helper-document, or rasterize to a
+  // RasterImage and return that instead.

We'll need a bug on fixing this. I think cloning the document would be feasible. We do it for printing already. Expensive though!

In Draw, you're using the imageSpaceToUserSpace translation and scalefactors but not the rest of the matrix. That can't be right. I think we need to actually put the whole imageSpaceToUserSpace into the matrix plus an adjustment that scales the temporary surface to the right size for the image. Or something like that.

Why do we need mLockCount? Seems to me we don't.

+  mAnimationMode = aAnimationMode;
+
+  switch (mAnimationMode = aAnimationMode) {
+    case kDontAnimMode:
+      StopAnimation();
+      break;
+    default:
+      StartAnimation();
+      break;
+  }

Redundant assignment of mAnimationMode. Might as well do
if (aAnimationMode == kDontAnimMode)

+    // XXXdholbert Maybe we should be listening for BeforePaint instead? (to
+    // make sure we're getting callbacks *after* SMIL samples, rather than
+    // just before the SMIL sample, which I think we might, depending on
+    // nsRefreshDriver's iteration order)

Flush_Display observers always run after Flush_Style observers (which nsSMILAnimationController is), so I think you're good.

+  // If we've got a non-1.0 scale factor (e.g. in cases of full-page zoom),
+  // need to tell the our context the scale to draw at.
+  tmpCtx->Scale(aScale.width, aScale.height);

This isn't right. You need to recalculate the ratio between the nominal image size and the surface you actually created. nsSVGUtils::ConvertToSurfaceSize might have chosen a different size to expected.

+  presShell->RenderDocument(svgRect,
+                            nsIPresShell::RENDER_IGNORE_VIEWPORT_SCROLLING,
+                            NS_RGBA(0, 0, 0, 0), // transparent
+                            tmpCtx);

Probably should add RENDER_ASYNC_DECODE_IMAGES

I definitely want to see a direct-render path that bypasses the surface when tiling is not required. That might require some restructuring, so I'd like to see it now.

One thing I expected to see but I don't is some way of capturing invalidations performed in the SVG document. For example if the SVG document progressively loads an image, we need to invalidate in the outer document. I think you should be adding a rendering observer to the root element using nsSVGEffects::AddRenderingObserver and your own subclass of nsSVGRenderingObserver. Or maybe we could have an alternative interface in nsSVGEffects that manages the nsSVGRenderingObserver for you and just calls back a function whenever invalidation happens. If we do that, we probably don't need NotifyObserverOfAnimationSample, nor do we need to hook into the refresh driver. Instead our invalidation observer will be able to call FrameChanged directly and will even be able to pass an accurate rectangle.

Comment 162

[Daniel Holbert [:dholbert]]

(In reply to comment #161)
> We'll need a bug on fixing this. I think cloning the document would be
> feasible. We do it for printing already. Expensive though!

Ok - filed bug 590792 on that. Also filed bug 590790 on another XXX comment (for making GetDataSize smarter).

> In Draw, you're using the imageSpaceToUserSpace translation and scalefactors
> but not the rest of the matrix. That can't be right. I think we need to
> actually put the whole imageSpaceToUserSpace into the matrix plus an adjustment
> that scales the temporary surface to the right size for the image. Or something
> like that.

Ok, I'll look into that as I refactor Draw().

> Why do we need mLockCount? Seems to me we don't.

You're right, we don't. I'd thought bug 359608 was going to depend on LockImage/UnlockImage, but based on the latest patch, it doesn't.  bholley says in #gfx that the locking methods are just used for discarding decoded images (which we can't do for SVG images), so these methods can be changed to no-ops.  I'll fix that in my patch-queue in a minute.

> Redundant assignment of mAnimationMode. Might as well do
> if (aAnimationMode == kDontAnimMode)

Thanks, fixed in patch queue.

> +    // XXXdholbert Maybe we should be listening for BeforePaint instead?
> Flush_Display observers always run after Flush_Style observers (which
> nsSMILAnimationController is), so I think you're good.

Thanks, good to know - I'd suspected that was the case, but hadn't verified it yet.

> +  // If we've got a non-1.0 scale factor (e.g. in cases of full-page zoom),
> +  // need to tell the our context the scale to draw at.
> +  tmpCtx->Scale(aScale.width, aScale.height);
> 
> This isn't right. You need to recalculate the ratio [etc]

Ok, I'll keep in mind while refactoring Draw().
 
> +  presShell->RenderDocument(svgRect,
> +                            nsIPresShell::RENDER_IGNORE_VIEWPORT_SCROLLING,
> Probably should add RENDER_ASYNC_DECODE_IMAGES

Ok, fixed in patch-queue.

> I definitely want to see a direct-render path that bypasses the surface when
> tiling is not required.

Yup, working on it.

> One thing I expected to see but I don't is some way of capturing invalidations
> performed in the SVG document. For example if the SVG document progressively
> loads an image, [etc]

Ok -- I think this use-case (progressive loading of an image embedded in SVG) is the only situation where this would cause problems right now, right?

> If we do that, we probably don't
> need NotifyObserverOfAnimationSample, nor do we need to hook into the refresh
> driver.

Nice!

> Instead our invalidation observer will be able to call FrameChanged
> directly and will even be able to pass an accurate rectangle.

I think we still can't provide an accurate rectangle to FrameChanged, at least for now -- the rectangle is in "image space", which depends on the client's perspective & dimensions. (For example, if we have an SVG image that has a viewBox attr, then a tall-and-skinny client will have a very different concept of image-space vs. a squat-and-wide client, and there's no rect we can return (aside from 'everything!') that would invalidate the correct region of both clients.)

Comment 163

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

> > +  presShell->RenderDocument(svgRect,
> > +                            nsIPresShell::RENDER_IGNORE_VIEWPORT_SCROLLING,
> > Probably should add RENDER_ASYNC_DECODE_IMAGES
> 
> Ok, fixed in patch-queue.

Actually, we want to pass RENDER_ASYNC_DECODE_IMAGES if and only if the drawing of *this* image allowed async decode. I'm not sure how easy that would be to implement.

> > One thing I expected to see but I don't is some way of capturing invalidations
> > performed in the SVG document. For example if the SVG document progressively
> > loads an image, [etc]
> 
> Ok -- I think this use-case (progressive loading of an image embedded in SVG)
> is the only situation where this would cause problems right now, right?

Well, there's loading of external resource documents as well. Also, right now you don't support progressive image display as the SVG document loads (right?) But I think we easily could, if we could invalidate as new content arrives.

> > Instead our invalidation observer will be able to call FrameChanged
> > directly and will even be able to pass an accurate rectangle.
> 
> I think we still can't provide an accurate rectangle to FrameChanged, at least
> for now -- the rectangle is in "image space", which depends on the client's
> perspective & dimensions. (For example, if we have an SVG image that has a
> viewBox attr, then a tall-and-skinny client will have a very different concept
> of image-space vs. a squat-and-wide client, and there's no rect we can return
> (aside from 'everything!') that would invalidate the correct region of both
> clients.)

I see. Fair enough. It could be done with some interface enhancements, but it's not worth it right now.

Comment 164

[Daniel Holbert [:dholbert]]

Attachment: Patch 13 v2: VectorImage impl

Here's a patch that lets a gfxCallbackDrawable do our drawing for us, using a custom implementation of gfxDrawingCallback, called "SVGDrawingCallback".

This patch differs from the previous version in that it removes VectorImage::RenderToSurface completely & replaces it with SVGDrawingCallback and gfxUtils::DrawPixelSnapped. (called by VectorImage::Draw)

I believe this change will automagically gets us the direct-render path where applicable (thanks to mstange's gfxDrawable awesomeness).

This seems to pass my tests as well as the previous patch did, so I think it's good...  I'll check it more thoroughly for quirks in the morning.  Tentatively requesting feedback on it, though.

Comment 165

[Daniel Holbert [:dholbert]]

(This version doesn't use RENDER_ASYNC_DECODE_IMAGE yet, per the first part of comment 163 - I don't think that should be tricky to get working, though. Will look at that as well as the invalidation-capturing in the morning.)

Comment 166

[Joe Drew (not getting mail)]

Comment on attachment 469183 [details] [diff] [review]
Patch 12 v2: SVGDocumentWrapper class


>+SVGDocumentWrapper::SVGDocumentWrapper()
>+{
>+  // Lazy-initialize our "svg" atom.  (It'd be nicer to just use nsGkAtoms::svg
>+  // directly, but we can't access it from here in non-libxul builds.)
>+  if (!SVGDocumentWrapper::kSVGAtom) {
>+    SVGDocumentWrapper::kSVGAtom =
>+      NS_NewPermanentAtom(NS_LITERAL_STRING("svg"));
>+  }
>+}

Aren't non-libxul builds going away?

>+PRBool
>+SVGDocumentWrapper::GetWidthOrHeight(PRBool   aIsWidth,
>+                                     PRInt32& aResult)

Why not just have a GetWidth/GetHeight pair that internally use this method?

(Alternately, create an enumerated type so you can say GetWidthOrHeight(SVGDocumentWrapper::WIDTH, &width))


>+/** nsIRequestObserver methods **/
>+
>+/* void onStartRequest (in nsIRequest request, in nsISupports ctxt); */
>+NS_IMETHODIMP
>+SVGDocumentWrapper::OnStartRequest(nsIRequest* aRequest, nsISupports* ctxt)
>+{
>+  nsresult rv = SetupViewer(aRequest,
>+                            getter_AddRefs(mViewer),
>+                            getter_AddRefs(mLoadGroup));
>+
>+  if (NS_SUCCEEDED(rv)) {
>+    mViewer->GetDocument()->SetIsBeingUsedAsImage();
>+    rv = mViewer->Init(nsnull, nsIntRect(0, 0, 0, 0));
>+    if (NS_SUCCEEDED(rv)) {
>+      rv = mViewer->Open(nsnull, nsnull);
>+    }
>+  }
>+  return rv;

Is there a reason that OnStartRequest doesn't call mListener->OnStartRequest? OnDataAvailable, OnStopRequest both chain to mListener.

Comment 167

[Daniel Holbert [:dholbert]]

(In reply to comment #166)
> Aren't non-libxul builds going away?

Not until the post-Firefox4 timeframe.  So, they still need to work for now.

> >+SVGDocumentWrapper::GetWidthOrHeight(PRBool   aIsWidth,
> Why not just have a GetWidth/GetHeight pair that internally use this method?

I do, actually -- VectorImage (in patch 13) has GetWidth/GetHeight, and SVGDocumentWrapper's only role is to help out VectorImage.

> (Alternately, create an enumerated type so you can say
> GetWidthOrHeight(SVGDocumentWrapper::WIDTH, &width))

Ok, I'll replace aIsWidth with an enum.

> Is there a reason that OnStartRequest doesn't call mListener->OnStartRequest?

It does actually call mListener->OnStartRequest, actually -- that just happens within its call to SetupViewer (the last step of SetupViewer).

You're right, though -- it'd be clearer if that call were up one level, within SVGDocumentWrapper::OnStartRequest.  I'll change that.

Comment 168

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

Comment on attachment 469400 [details] [diff] [review]
Patch 13 v2: VectorImage impl

+  gfxMatrix savedMatrix(aContext->CurrentMatrix());

Use gfxAutoSaveRestoreMatrix. (Should have used that in nsSVGIntegrationUtils too, bad me.) Otherwise looks great!

Comment 169

[Daniel Holbert [:dholbert]]

Still working on this - I have a patch that watches for invalidation, here:
http://hg.mozilla.org/users/dholbert_mozilla.com/svg-patches/file/38f54f9cf705/svgRootRenderingObserver
...which layers on top of a patch to make nsSVGRenderingObserver a generic superclass..
http://hg.mozilla.org/users/dholbert_mozilla.com/svg-patches/file/38f54f9cf705/invalidation_observer
but it causes hangs in a few cases (maybe because I'm re-registering as an invalidation observer too frequently / at the wrong times), which I'm working on fixing. (I also have to look into some linking issues that non-libxul builds have with those patches.)

This is very close to done, but it's not going to make beta5 (since any feature work for that beta was supposed to have landed yesterday) -- and since the Firefox 4 feature-freeze is now at beta6, this doesn't actually need to block beta5.

Can we change the blocking status here to beta6+?

Comment 170

[Mike Beltzner [:beltzner, not reading bugmail]]

Yes, we can.

Comment 171

[Daniel Holbert [:dholbert]]

Attachment: Patch 4c: Add method "IsResourceDoc"

This patch just adds a helper-method "IsResourceDoc" on nsIDocument, so we can just call that one method for if() conditions that apply to both external resource documents and svg-as-image helper-documents.

This patch changes one such if() condition, and the next patch will add another new one.

Comment 172

[Daniel Holbert [:dholbert]]

Attachment: Patch 12b: Make "UnsuppressPainting" call work for resource docs

This patch fixes a bug that was preventing me from receiving invalidation notifications from the svg helper-document.  In current mozilla-central, we  bail out of UnsuppressPainting calls (leaving painting suppressed) if the document fails an EnsureVisible check.  But we want to go through with the UnsuppressPainting call for resource documents.

Comment 173

[Daniel Holbert [:dholbert]]

Attachment: Patch 12c: Make nsSVGRenderingObserver into a generic interface

This makes nsSVGRenderingObserver into a more generic interface (for which I can add a new concrete implementation).  The existing implementation is renamed to nsSVGIDRenderingObserver.

Comment 174

[Daniel Holbert [:dholbert]]

Attachment: Patch 13 v3: VectorImage impl

Here's patch 13 again (the main VectorImage implementation). I've removed all of the refresh driver stuff, since we won't need that anymore, as roc mentioned at the end of comment 161.

This patch doesn't add the SVGRenderingObserver stuff yet, for simplicity, I'm adding that in a followup patch.  (This patch still works on its own, though -- just not for animated SVG content, and not for SVG content with progressively-loaded images.)

Comment 175

[Daniel Holbert [:dholbert]]

Attachment: Patch 13 v4: VectorImage impl

oops - I missed two refresh-driver-related changes that I no longer need (visibility tweaks to nsPresContext.h and nsRefreshDriver.h).  I've removed those from the patch now.

Comment 176

[Daniel Holbert [:dholbert]]

Attachment: Patch 14: Add "SVGRootRenderingObserver" helper-class

This patch...
 - Adds a nsSVGRenderingObserver impl, so that VectorImage can listen for rendering in the helper SVG document.
 - Adds a flag on SVGDocumentWrapper to let us ignore invalidations while we're resizing the viewport in Draw().
 - Adds some more null-checking in SVGDocumentWrapper::GetRootSVGElem, to avoid shutdown crashes. (Our renderingobserver will sometimes get a callback during shutdown, after we've gotten a call to DestroyViewer.  This patch makes GetRootSVGElem just return null in that situation, instead of shutdown-crashing.)

Comment 177

[Daniel Holbert [:dholbert]]

Oh, also -- patch 14 doesn't work with non-libxul builds, due to some linking issue from SVGRootRenderingObserver inheriting from a class in another module.   I've wrestled with it a bit*, but I haven't been able to get it to link correctly yet, in a non-libxul configuration.

My current plan for that is to just wrap most of patch 14's code in "#ifdef MOZ_LIBXUL" guards, so that non-libxul configurations will still build successfully (but won't get automatic invalidations for svg-image animations), and then fix it in a followup bug, since libxul is what we really care most about (for our release builds).

Comment 178

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

Comment on attachment 472032 [details] [diff] [review]
Patch 14: Add "SVGRootRenderingObserver" helper-class

+  virtual mozilla::dom::Element* GetTarget()

you don't need mozilla::dom::

+    observer->FrameChanged(this, &mozilla::imagelib::kFullImageSpaceRect);

using namespace mozilla::imagelib!

Looks great!

Comment 179

[Daniel Holbert [:dholbert]]

Attachment: Patch 12d: respect containing-block size, in nsSVGOuterSVGFrame::ComputeSize

This tweak fixes a reftest of mine that I'd been meaning to debug for a little while (and fixed today).  Basically, when an <img> element has a specified width, we communicate that to our DocumentViewerImpl correctly (via "SetBounds") -- but we also need to tell nsSVGOuterSVGFrame::ComputeSize to respect that size, too, or else it will recalculate its size on its own.

We do this for <embed> already, via a protected helper-method that lets us check whether we're in <embed>.  This patch just adds another check in that same spot to see if we're the root of an SVG document that's being used as an image.

Comment 180

[Daniel Holbert [:dholbert]]

Attachment: Patch 15: reftests

This patch includes:
 - A variety of simple <img> reftests (img-simple-*)
 - A variety of background-image reftests (background-*) including one with moz-image-rect
 - One list-style-image reftest (list-simple-1.html)
 - A subdirectory "zoom" containing two reftests at non-default full-page-zoom levels.
 - img-[width|height|widthAndHeight]-[meet|slice]-[1|2].html: A set of 12 tests each of which dynamically generates a grid of 40 subtests, to verify that our <img> behavior matches our existing <embed> behavior across a variety of viewBox / preserveAspectRatio / container-size / svg-element-size values.  In each testcase/reference-case pair, the only difference is "img" vs. "embed".

These tests all pass on my local machine. I'm pushing them to TryServer to be sure they still pass on other platforms. (They've passed TryServer on other platforms in past weeks, and I expect they still should.)

Comment 181

[Daniel Holbert [:dholbert]]

Attachment: Patch 15 v2: reftests

(fixed a typo in a reftest comment)

Comment 182

[Joe Drew (not getting mail)]

Comment on attachment 472026 [details] [diff] [review]
Patch 13 v4: VectorImage impl


> NS_IMETHODIMP
> VectorImage::GetWidth(PRInt32* aWidth)

>+  return mSVGDocumentWrapper->GetWidthOrHeight(SVGDocumentWrapper::eWidth,
>+                                               *aWidth) ?
>+    NS_OK : NS_ERROR_FAILURE;
> }

I'm not super-in love with the ternary operator; if it's all the same to you, could you do if (NS_FAILED(...))\n return NS_ERROR_FAILURE; return NS_OK? (Same below in the Height getter.)

> VectorImage::OnStopRequest(nsIRequest* aRequest, nsISupports* aCtxt,
>                            nsresult aStatus)

>+  nsresult rv = mSVGDocumentWrapper->OnStopRequest(aRequest, aCtxt, aStatus);
>+  if (!mSVGDocumentWrapper->ParsedSuccessfully()) {
>+    // XXXdholbert Need to do something more here -- right now, this just
>+    // makes us draw the "object" icon, rather than the (jagged) "broken image"
>+    // icon.
>+    mError = PR_TRUE;
>+    return rv;
>+  }

If the status trackers' GetImageStatus returns an error, the broken image icon should automatically be set by nsImageFrame. Calling Cancel with an error status should probably do this for you, I think?

Fixing this isn't a blocker for this bug, though. Feel free to file a followup.

Comment 183

[Daniel Holbert [:dholbert]]

(In reply to comment #182)
> I'm not super-in love with the ternary operator; if it's all the same to you,
> could you do if (NS_FAILED(...))\n return NS_ERROR_FAILURE; return NS_OK? 

Fixed in patch queue. (but without NS_FAILED, since the return-type we're checking is PRBool, not nsresult)

> If the status trackers' GetImageStatus returns an error, the broken image icon
> should automatically be set by nsImageFrame. Calling Cancel with an error
> status should probably do this for you, I think?
> 
> Fixing this isn't a blocker for this bug, though. Feel free to file a followup.

I'm not sure it's that simple -- in any case, I filed bug 594505 as a followup to investigate.  Thanks for the review!

Comment 184

[Daniel Holbert [:dholbert]]

Attachment: Patch 15 v3: reftests (now with reftest-wait)

This reftests revision adds "reftest-wait" to the dynamically generated reftests (the last category mentioned in Comment 180).  I used to have that in there, but I'd thought I could get rid of it, since it didn't seem to be required on my local machine.  TryServer says it is needed on some platforms, though.

Comment 185

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

Comment on attachment 473180 [details] [diff] [review]
Patch 15 v3: reftests (now with reftest-wait)

go go go!

Comment 186

[Daniel Holbert [:dholbert]]

Landed - yay!

http://hg.mozilla.org/mozilla-central/rev/c3cde26e0195 
http://hg.mozilla.org/mozilla-central/rev/6f9b870337ba
http://hg.mozilla.org/mozilla-central/rev/f3874d6012b6
http://hg.mozilla.org/mozilla-central/rev/53aad62ff820
http://hg.mozilla.org/mozilla-central/rev/996dd51c979a
http://hg.mozilla.org/mozilla-central/rev/7706c06bdf6f
http://hg.mozilla.org/mozilla-central/rev/58df4c112b6d
http://hg.mozilla.org/mozilla-central/rev/827cba9e2671
http://hg.mozilla.org/mozilla-central/rev/9711d082d0ed
http://hg.mozilla.org/mozilla-central/rev/c05b5937306d
http://hg.mozilla.org/mozilla-central/rev/2ef2faec3e30
http://hg.mozilla.org/mozilla-central/rev/bf93d8c0a869
http://hg.mozilla.org/mozilla-central/rev/6eb8d33411ab
http://hg.mozilla.org/mozilla-central/rev/f65c60f33425
http://hg.mozilla.org/mozilla-central/rev/153aab554926
http://hg.mozilla.org/mozilla-central/rev/2515b8ed7160
http://hg.mozilla.org/mozilla-central/rev/e4b11cd1b34c
http://hg.mozilla.org/mozilla-central/rev/81bb4ca2a4a2
http://hg.mozilla.org/mozilla-central/rev/c0270004efac
http://hg.mozilla.org/mozilla-central/rev/77a3ee888b8f
http://hg.mozilla.org/mozilla-central/rev/ec53c1b41f3c

Comment 187

[Daniel Holbert [:dholbert]]

(/me removes launchpad's auto-added-but-actually-unrelated "See also" bug, https://launchpad.net/bugs/168261.  That bug is about SVG in <object> behaving in some way the user doesn't like. Unrelated to SVG-as-an-image)

Comment 188

[Daniel Holbert [:dholbert]]

(In reply to comment #183)
> (In reply to comment #182)
> > I'm not super-in love with the ternary operator; if it's all the same to you,
> > could you do if (NS_FAILED(...))\n return NS_ERROR_FAILURE; return NS_OK? 
> 
> Fixed in patch queue. (but without NS_FAILED, since the return-type we're
> checking is PRBool, not nsresult)

dbaron added a missing "!" that I'd left out in the GetHeight version of this fix:
http://hg.mozilla.org/mozilla-central/rev/9f5e12404215

Comment 189

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #141)
> I initially tried *declaring* this variable in imgIContainer.idl and then
> *defining* it in Image.cpp, but that gives me linking errors when I try to
> refer to it from layout code.  The existing solution (defining it in the
> interface) compiles & links, but I think at the cost of giving each compilation
> unit its own (static const) copy of kFullImageSpaceRect...  If there's
> something better I could be doing (to define this variable such that it can be
> referred to from gklayout & imagelib), I'd love to know.

You're correct that the cost here is giving each compilation unit its own static const copy, which is initialized at runtime.  Apparently gcc isn't smart enough to figure out that kFullImageSpaceRect is POD or to merge identical pieces of generated code, because this landing almost *doubled* the number of static initializers that we have (from 1100 to a bit over 2000).

See http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/abf60ad5fd03a708?pli=1 for more information about why static initializers are bad.

Comment 190

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

From the codesighs log

+52	global constructors keyed to BasicTableLayoutStrategy.cpp
+52	global constructors keyed to CanvasUtils.cpp
+52	global constructors keyed to Decoder.cpp
+52	global constructors keyed to DiscardTracker.cpp
+52	global constructors keyed to DocumentRendererChild.cpp
+52	global constructors keyed to DocumentRendererNativeIDChild.cpp
+52	global constructors keyed to DocumentRendererShmemChild.cpp
+52	global constructors keyed to FixedTableLayoutStrategy.cpp
+52	global constructors keyed to FrameLayerBuilder.cpp
+52	global constructors keyed to Image.cpp
... for hundreds of lines

Comment 191

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

Yeah, we'd better fix that immediately.

Comment 192

[Daniel Holbert [:dholbert]]

Working on it in Bug 594650.

Comment 193

[Eric Shepherd [:sheppy]]

This is documented, both through a mention on Fx4 for developers and as a mention on the <image> tag page.

Comment 194

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to comment #133)
> Created attachment 468460 [details] [diff] [review]
> Patch 5: Move nsSVGUtils::ConvertToSurfaceSize & ClampToInt into header file

Did you file a followup?

Comment 195

[Daniel Holbert [:dholbert]]

You mean, a followup on eventually reverting part of that patch, to move ConvertToSurfaceSize back to the .cpp file once we're in a libxul-only world?

I hadn't, since the libxul-only world is still a little ways off.  But since you mentioned it, I filed Bug 595734.