317628 - Add "Web page, complete" to the Save File As... dialogue for SVG files

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

I'd really want this. Appart from being something end users will want, it would make it a lot easier to save complex SVG files (with lots of external JavaScript or raster graphics) so we can make changes, debug and test them.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch - seems to work well in FF1.5 and SeaMonkey

Comment 2

[Jonathan Watt [:jwatt]]

Comment on attachment 204092 [details] [diff] [review]
patch - seems to work well in FF1.5 and SeaMonkey

It also works fine in SeaMonkey.

Comment 3

[Jonathan Watt [:jwatt]]

Comment on attachment 204092 [details] [diff] [review]
patch - seems to work well in FF1.5 and SeaMonkey

I was holding off on requesting review until I could check that the following assertion wasn't caused by my changes (it's not).

###!!! ASSERTION: Did you know you were calling |do_GetInterface()| on an object that doesn't support the |nsIInterfaceRequestor| interface?: 'factoryPtr', file d:/mozilla/trees/trunk2/obj-firefox-debug/xpcom/build/nsIInterfaceRequestorUtils.cpp, line 50

Summary of patch:

To support "Web page, complete" for SVG, nsWebBrowserPersist needs to know which attributes of which SVG elements are URIs to external files used by the SVG (thanks biesi!). The two cases I deal with in this patch are external JavaScript and raster images included using xlink:href on <script> and <image>. Since this attribute belongs to the XLink namespace I had to make nsWebBrowserPersist::StoreURIAttribute and nsWebBrowserPersist::FixupNodeAttribute namespace aware.

My only concern is whether the line marked by the XXXjwatt is required, and if so, why?

Comment 4

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

Comment on attachment 204092 [details] [diff] [review]
patch - seems to work well in FF1.5 and SeaMonkey

r=biesi

those are the only SVG elements needing fixup?

I assume you tested that this doesn't break HTML saving?

Comment 5

[Darin Fisher]

Comment on attachment 204092 [details] [diff] [review]
patch - seems to work well in FF1.5 and SeaMonkey

>Index: mozilla/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp

>-        StoreURIAttribute(aNode, "src");
>+        StoreURIAttributeNS(aNode, "", "src");

I'm curious.  Why don't you just define StoreURIAttribute as an
inline method call ot StoreURIAttributeNS?  Then you will not 
have to touch all of the callers, and you can keep the rest of
the code simpler by not having to code the empty namespace value
everywhere.

sr=darin

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to comment #4)
> those are the only SVG elements needing fixup?

For now, yes.

> I assume you tested that this doesn't break HTML saving?

I did. I didn't see any obvious problems.

(In reply to comment #5)
> I'm curious.  Why don't you just define StoreURIAttribute as an
> inline method call ot StoreURIAttributeNS?  Then you will not 
> have to touch all of the callers, and you can keep the rest of
> the code simpler by not having to code the empty namespace value
> everywhere.

You're right, it's better that way. I've done just that for the commit.

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: actual commit diff

Comment 8

[Frank Wein [:mcsmurf]]

This checkin seems to have broken a compiler on the Sun platform, the compiler does not like the way inline is used here ;). See Bug 322110.

Comment 9

[Jonathan Watt [:jwatt]]

Thanks Frank. I've checked in the fix.