Bug 12499

Another example of external references: http://www.w3.org/Graphics/SVG/Test/20061213/htmlEmbedHarness/full-text-tref-01-b.html

We are lowering the priority because this is not heavily used.

Parity Opera: http://files.myopera.com/MacDev_ed/sarpsborg2007/external-use.svg does this bug need a testcase or description? #2 Rob, this is not heavily used, because support is not widespread... ubiquitous support makes a huge difference. please raise priority. symbol languages are used by people with low literacy. just one example: they purchase symbol libraries at high expense, which are stored locally. email applications then only transmit text which has symbols added. there are very similar requirements for the web... regards

Parity Amaya SVG microformat for icons: http://www.gnote.org/svgSearch/requirements.html Authoring Tool Guidelines to follow shortly

parity Batik, openclipart.org and inkscape output are stymied because this has not been implemented. It's pretty close to impossible to search svg as graphic unless one understands and implements use. http://www.openicon.org/icon-ark/weather-icons.svgz 18 icons in 2.3K

Would be nice to have this, and also external tref. Any security issues to consider? Cheers, Rob.

Created attachment 21102 [details] Initial patch This patch makes external trefs work. External use is a bit harder, I did not succeed yet in making the external document references like paintserver work yet. Still there is a lot of code already that can be reviewed I think. Maybe we can even land the tref and use stuff separately. Cheers, Rob.

#7 if this bug needs to be taken to bits, the bit I'm voting for is <symbols> that are externally referenced.. test case in #5

What are the security implications of external references? Are they allowed cross-site?

Comment on attachment 21102 [details] Initial patch + This file is part of the KDE libraries I don't think it's helpful to include this in WebKit source files. #include "CachedXSLStyleSheet.h" +#if ENABLE(SVG) +#include "CachedSVGDocument.h" +#endif #include "CString.h" We normally put includes that require an #if in a separate paragraph, after all the unconditional includes. class CachedXSLStyleSheet; +#if ENABLE(SVG) +class CachedSVGDocument; +#endif class Document; Same with class forward declarations. + CachedSVGDocument* requestSVGDocument(const String &url); Please put the & next to the typename. #include "XLinkNames.h" +#include "DocLoader.h" +#include "CachedSVGDocument.h" Please keep includes sorted alphabetically. + , m_cachedDoc(0) Why abbreviate? Why not "m_cachedDocument"? + if (url.find('#') > -1) { // format is #target + unsigned int end = url.find('#'); It doesn't make sense to call find twice here. Since this is URL parsing, I think this should be done with a helper function in KURL.h. It would go along with the others, like equalIgnoringRef and protocolIs. No need to do that in this patch, but a good idea in the end. I really don't think that getDocUrl is a good name for this function. The abbreviation is unclear, for one thing. + if (docUrl.isEmpty()) + return document(); + else We normally don't do else after return. + const Document* doc = referencedDocument(); Why const? You need a test case for this new feature! I'm setting review- because of the lack of a test case mainly.

Created attachment 21329 [details] use #carrot

apologies, reduced testcase now attached..

nb, this bug covers other cases than <use:symbol> which is the reduction test case.

practical example: http://www.openicon.org/irc/ircwithIcon.svg follows irc://irc.freenode.net/#svg and appends an icon for each word, where available.

#14 please use http://www.openicon.org and follow the links wfm Opera

I would now like to purchase an itouch or iphone with safari and SVG support. however I'm waiting on this bug....

IIUC this bugs also has big impact on the usability of external stylesheets in SVG. Suppose SVG file 'a.svg' uses stylesheet file 'style.css'. 'style.css', on the other hand, uses 'b.svg' to define gradients, patterns etc (.myfill { fill:url(b.svg#fooGrad)} ). The effect of this bug is that 'myfill' style would be undefined.

Created attachment 27315 [details] external stylesheet referencing svg defs Attached testcase: * test8.svg - actual image to test, uses prologo.css * prologo.css - CSS for test8.svg, uses prologoDefs.svg * prologoDefs.svg - SVG file with definitions of gradients and patterns * test8.png - rendering of what seems to be the correct implementation (batik 1.7)

(In reply to comment #18) Alexay thanks for raising the issue of css, however please file a separate bug. also a simple example might work better. eg red for fail, green for success might be easier to interpret filters, gradients and patterns are not necessarily core to your bug... they may need to be filed as separate bugs at a later date. are you sure your attachment is standards based? prologoDefs.svg did not render in recent mozilla or opera builds using os x similarly test8.svg was not easy to interpret. regards

(In reply to comment #19) Thanks for quick reply. I'll try to submit the more specific bug with simplified test case later. I've submitted that one just to demonstrate, that external fragment referencing has one important usecase. I believe the case is based on standards; but, unfortunately, mozilla, webkit and opera have (different) issues with their implementations.

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

one not so simple workaround is to: Parse XML to js objects then use DOM manipulation. why so slow to fix this 'n' xslt bugs? this sure aint a web 2.0 world...

crash this has now become a crash bug

oops apologies needs new bug, slightly different issue. however if there is one webkit bug that I would vote to be fixed This is it!

maybe related to bug 27872

The ability to use external references is actually a primary use case for the <use> element, and is very important to SVG's usability. With external references, gradients and other paint servers, filters, and symbols can be stored in a "library" of resources, and easily reused. This functionality works in Firefox 3.5 and Opera, and should be supported in WebKit. If there are concerns about security issues, a sensible restriction would be to allow only same-domain resources to be referenced. If there is anything the SVG WG can do to clarify the specification, improve tests, or help coordinate between implementers to ensure interoperability of this feature, please send an email to www-svg@w3.org. We'll be happy to help.

Perhaps an actual example of SVG in use could convince this bug to be fixed. http://beta.pumpproedits.com/chart/quick/79/rhythm This is one of the many SVG generated stepcharts for edits for the game Pump It Up Pro. I wish to use SVG due to scalability and better file sizes. The format also compresses well due to the repeating tags. I have my core SVG files stored elsewhere on the server, and I link to them to try to help with caching, similar to how CSS does its magic.

Any update on this bug...?

Kicking this case because it's still broken and is a MAJOR problem. As said, it is imperative that <use> work externally, otherwise it is close to useless to have in the first place. Example: http://hackthetruth.org/index-svg.php Works in FF and O nightly, but not Webkit nightly.

Created attachment 62037 [details] Patch reworked against trunk This is the original patch reworked against master. The main difference is that KURL is better reused now. It is not up for review since a lot needs to be done still: - <use> referring an external fragment that uses an external paintserver does not work - there is a <use> bug when referring to <symbol>, when not as outer use width/height are not transferred, see batikLogo.svg - could still crash with certain testcases - cycles are not detected yet - needs lots more tests Hopefully in the next few weeks I can work on it and it should also get lots of attention during the upcoming ksvg hack weekend.

(In reply to comment #30) > Created an attachment (id=62037) [details] > Patch reworked against trunk > > This is the original patch reworked against master. The main difference is that KURL is better reused now. It is not up for review since a lot needs to be done still: > > - <use> referring an external fragment that uses an external paintserver does not work > - there is a <use> bug when referring to <symbol>, when not as outer use width/height are not transferred, see batikLogo.svg > - could still crash with certain testcases > - cycles are not detected yet > - needs lots more tests Hi Rob, good to see you working again on it. A general comment: I'd propose to refactor the CachedXBLDocument code into a shared CachedDocument class, so that both CachedSVGDocument & CachedXBLDocument can inherit from it. Cheers, Niko

Desperately wanting this patch. Thanks Rob for working on this for us all.

I looked at Rob Buis' patch, and I discussed it with Nate Chapin. The patch no longer applies to today's trunk, because of the move of WebCore into Source/, as well as other structural changes that happened since Rob's last submission in July 2010. But more importantly, there seem to be semantic issues. In the WebKit bug 15443 "SVGImage does not support sub-resource loading" a couple of problems are outlined, one of which is the fact that subresources have a bunch of limitations when compared to the main resources. They cannot further load other subresources, etc. I believe that Niko's suggestion in comment #31 to introduce the CachedDocument class is (partially) related to the issue of having <use>'d resources act as main resources. We (i.e. Nate and I) believe that Rob's patch that modifies classes from WebCore/loader/cache/ (CachedResource, CachedResourceClient, CachedResourceLoader, etc.) can only deal with subresources, just like anything else that is accessing these classes. (Anything that goes through this cache is a subresource -- CachedResourceRequest is the loader client for subresources.) We might therefore get a strange behavior, because we're loading something that should be a main resource, as a subresource; but subresources are not designed to carry themselves other subresources. Instead, a solution that resembles handling of frames in HTML would be the better way to go. Load <use>'d elements as real frames, not as subresources. Loading HTML frames is handled by the means of the class HTMLFrameOwnerElement. We could have a similar class, say, SVGAssetOwnerElement, that could perhaps share a common ancestor with HTMLFrameOwnerElement, (let's call it Owner), and then split the semantics: the HTML-specific semantics would go to HTMLFrameOwnerElement, while the SVG-specific semantics would go to SVGAssetOwnerElement. (And "SVG asset" could be <svg>, <g>, <symbol>, or anything else that could be <use>'d.) class Owner class HTMLFrameOwnerElement : public Owner, public HTMLElement class SVGAssetOwnerElement : public Owner, public SVGElement class SVGTextAssetOwnerElement : public SVGAssetElement // used by <tref> Perhaps, to avoid confusion that might arise because these classes do not correspond directly to actual elements, alternative names like HTMLFrameOwner (instead of HTMLFrameOwnerElement) and SVGAssetOwner (instead of SVGAssetOwnerElement) might be better. Security-wise, just as with the current HTMLFrameOwnerElement, we must be careful about the newly-introduced SVGAssetOwnerElement. As far as I know, SVG has no SandboxFlags, although, I think it should. At least in the beginning, SVGAssetOwnerElement's SandboxFlags should be set to SandboxNone (and tested for that).

(In reply to comment #33) s/SVGAssetOwner/SVGResourceOwner/

Created attachment 83784 [details] Draft patch, asking for a preliminary review This is an incomplete patch. I'm asking for a preliminary review only. I split HTMLFrameOwnerElement in three, with the parent Owner, and the children HTMLFrameOwnerElement (same name and behavior as the old one) and SVGResourceOwnerElement. Then I tried to adapt Rob's patch to this design. I modified SVGUseElement accordingly. But two essential things are still missing: loading the resource, and using it. See the "TODO(ctruta)" placeholders, in SVGUseElement::parseMappedAttribute, and in SVGUseElement::referencedDocument. The bulk of the work will be in parseMappedAttribute; finishing referencedDocument should be trivial. Care must be taken when "file.svg#FOO" and "file.svg#BAR" appear together in the tree (but not necessarily in the same document). Niko pointed out the following: How would the "frame-like" concept integrate into the existing <use> shadow tree? Whenever a <use> element is created, it will clone it's target into it's own shadow tree (a child of the <use> itself), and deep-clone that target tree into the <use> shadow tree. That would remove the need for 'subresource' loading. It's just as if the content has been textually included in the original document. So, if we have <use "file.svg#FOO">, then later, we encounter <use "file.svg#BAR">, we construct the whole tree for "file.svg", then clone the shadow subtree for "foo.svg#FOO", then do the same for "foo.svg#BAR". In summary, here are the steps: 1) Import first external reference, include into shadow tree. Don't build shadow tree renderers yet, only the DOM. 2) Walk the DOM, resolve any external uses, etc. Don't do any renderer creation until the whole resolving is a) done or b) abort if it cycles. In addition, we'll need to find a certain class/object that handles cycles. Cycles are to be identified by URIs that match all the way to the #fragment component (excluding the #fragment component). I'll worry about cycles later, though. I'd be happy to see the basics done.

Attachment 83784 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8019133

Attachment 83784 [details] did not build on win: Build output: http://queues.webkit.org/results/8032158

Attachment 83784 [details] did not build on win: Build output: http://queues.webkit.org/results/8035137

Attachment 83784 [details] did not build on qt: Build output: http://queues.webkit.org/results/8035152

Comment on attachment 83784 [details] Draft patch, asking for a preliminary review svg/SVGResourceOwnerElement.cpp and .h are missing from this patch.

Attachment 83784 [details] did not build on gtk: Build output: http://queues.webkit.org/results/8032255

Created attachment 83812 [details] Draft patch, asking for a preliminary review (In reply to comment #40) > svg/SVGResourceOwnerElement.cpp and .h are missing from this patch. Oops. Resubmitted.

Attachment 83812 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/GNUmakefile.am', u'Source/W..." exit_code: 1 Source/WebCore/svg/SVGResourceOwnerElement.cpp:31: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/svg/SVGResourceOwnerElement.h:26: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 83814 [details] Draft patch, asking for a preliminary review

Attachment 83812 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8031281

Attachment 83814 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8032286

Attachment 83814 [details] did not build on qt: Build output: http://queues.webkit.org/results/8031310

(In reply to comment #33) > This is an incomplete patch. I'm asking for a preliminary review only. > > I split HTMLFrameOwnerElement in three, with the parent Owner, and the children HTMLFrameOwnerElement (same name and behavior as the old one) and SVGResourceOwnerElement. I fail to see the need to reuse anything of the frame logic. It boils down to the question how loading is now handled? My first attempt would be to make SVGUseElement a CachedResourceClient, and let it load a CachedDocument, async, then process it. How is loading handled for frames? How do you want to implement it, using this frame-like concept? > 1) Import first external reference, include into shadow tree. Don't build shadow tree renderers yet, only the DOM. > 2) Walk the DOM, resolve any external uses, etc. Don't do any renderer creation until the whole resolving is a) done or b) abort if it cycles. > > In addition, we'll need to find a certain class/object that handles cycles. Cycles are to be identified by URIs that match all the way to the #fragment component (excluding the #fragment component). > I'll worry about cycles later, though. I'd be happy to see the basics done. Sure. Let's get a simple version working first :-)

Created attachment 86746 [details] Draft patch, asking for review I am abandoning the direction taken in the previous patch that involved SVGResourceOwnerElement, in response to Niko's suggestion in comment #48. This is a heavily-reworked version of the old patch submitted by Rob Buis in 2010-07-19. Rob's CachedSVGDocument is replaced with the more generic CachedDocument. Although CachedDocument is not meant to be SVG-specific, it is conditionally compiled with #if ENABLED(SVG), because, currently, only SVG needs it. The ChangeLog entry needs more details, and the external <tref> element is yet to be handled, perhaps in a different bug.

Attachment 86746 [details] did not build on win: Build output: http://queues.webkit.org/results/8237189

This is just an informal drive-by on my part, but it looks like you're going to be left with a stale m_cachedDocument in the case where the USE element is moved to a new document and the original document is destroyed. It seems like you should factor out the logic from parseMappedattribute into it's own method that you call from insertedIntoDocument as well. It also looks like you'll need a similar call in svgAttributeChanged for when SVGURIReference::isKnownAttribute returns true.

Attachment 86746 [details] did not build on win: Build output: http://queues.webkit.org/results/8234353

(In reply to comment #48) > (In reply to comment #33) > > This is an incomplete patch. I'm asking for a preliminary review only. > > > > I split HTMLFrameOwnerElement in three, with the parent Owner, and the children HTMLFrameOwnerElement (same name and behavior as the old one) and SVGResourceOwnerElement. > I fail to see the need to reuse anything of the frame logic. It boils down to the question how loading is now handled? My first attempt would be to make SVGUseElement a CachedResourceClient, and let it load a CachedDocument, async, then process it. How is loading handled for frames? How do you want to implement it, using this frame-like concept? I'm really sorry for not jumping in sooner on this. The reason for implementing this with a FrameOwner concept is that Subresources can't load if they're in a Document with a null frame. Note that we exit early in http://trac.webkit.org/browser/trunk/Source/WebCore/loader/SubresourceLoader.cpp#L65 if frame is null, and that Frame will be the Document's Frame. I'm not an svg expert, but my understanding is that external use references should be able to pull in, e.g., images from the external document, and for that you need the Document to have a Frame.

In addition to Justin's remarks in comment #51, I can also see that SVGUseElement::parseMappedAttribute is currently broken. If I have the following three files: ** file "main.svg" <svg> <use href="level1.svg#TOP"> </svg> ** file "level1.svg" <svg> <circle id="RECT"> <!-- deliberate mismatch to show error --> <use href="level2.svg#RECT"> </svg> ** file "level2.svg" <svg> <rect id="RECT"> </svg> Then, with the currently-submitted patch, I am seeing the <circle> from "level1.svg", instead of the <rect> from "level2.svg". That's because, while processing "level1.svg", the cachedDocument being loaded in parseMappedAttribute is "level1.svg" instead of "level2.svg".

Comment on attachment 86746 [details] Draft patch, asking for review View in context: https://bugs.webkit.org/attachment.cgi?id=86746&action=review Overall the code looks good. Some of the asserts and returns on document testing may need good studying, but I think it is done correct. Apart from the minor gripes above I think a few more tests are needed. > Source/WebCore/ChangeLog:9 > + (Currently it only needs to handle SVG documents.) Don't forget the overall goal. I'd personally say "Support external references on <use> by introducing CachedDocument, which handles document subresources." > Source/WebCore/ChangeLog:10 > + An instance of this class is a member in SVGUseElement. This line is not very helpful and can be discarded. Better may be to add such info here: * svg/SVGUseElement.h: store CachedDocument > Source/WebCore/loader/cache/CachedDocument.cpp:73 > + // FIXME: this should not be SVG-specific. Try Document::create. Is the FIXME still valid? > Source/WebCore/svg/SVGUseElement.cpp:130 > + KURL kurl(document()->baseURI(), href()); Better just use url instead of kurl.

Comment on attachment 86746 [details] Draft patch, asking for review Oops, forgot to r-

Comment on attachment 86746 [details] Draft patch, asking for review Cosmin, can you update the patch with Robs suggestions? Btw, I'm sure we have lots of batik tests that embed an external <use> file. You need to run all layout tests, I'm sure there are changes.

(In reply to comment #57) > (From update of attachment 86746 [details]) > Cosmin, can you update the patch with Robs suggestions? Yes, I'm fixing the problem that I mentioned in comment #54, and while at it, I am also addressing Rob's concerns. Then I'll go and address Justin's comment #51. > Btw, I'm sure we have lots of batik tests that embed an external <use> file. You need to run all layout tests, I'm sure there are changes. I had run the layout tests before I submitted my patch, and I saw no regressions.

(In reply to comment #58) > (In reply to comment #57) > > (From update of attachment 86746 [details] [details]) > > Cosmin, can you update the patch with Robs suggestions? > > Yes, I'm fixing the problem that I mentioned in comment #54, and while at it, I am also addressing Rob's concerns. > > Then I'll go and address Justin's comment #51. > > > Btw, I'm sure we have lots of batik tests that embed an external <use> file. You need to run all layout tests, I'm sure there are changes. > > I had run the layout tests before I submitted my patch, and I saw no regressions. Cosmin, are you still on this bug? :-)

Created attachment 90851 [details] Draft patch Here is a patch in which I'm addressing Rob's comments, and making other necessary changes in order to make it build with the WebKit du jour. Notably, I accounted for CachedResource::DocumentResource inside CachedResourceLoader::canRequest, in the light of the newly-added ContentSecurityPolicy class. There are no significant fixes, though, as I haven't had sufficient time for that. (I'm only working on this in my spare time, which, nowadays, is very little.) I noticed that, inside SVGUseElement::parseMappedAttribute, document()->baseURI() is invalid when the <use> element is inside a cached document. This is the reason behind the failure that I described in the example from comment #54. The cached document's m_baseURL needs to be initialized, and the place to do that (I think) is right at the point where it's created, i.e. inside CachedDocument::data(). I didn't get to make it work yet, though. A simple call to m_document->setDocumentURI(m_url) doesn't seem to be sufficient, and I'm looking into that.

I've read all of the comments here and hope that my comment is right in here: We really desperately need support for loading external style sheets into svg images referenced by html's "img"-tag! Every other browser does this today, even Internet Explorer. Why not WebKit? I'm very frustrated with the actual situation, with WebKit being the only one not being able to read external stylesheets or other external ressources, when used as image! Any news on that issue? Please let us use external stylesheets! That would make svg's really usable at last, even if there are more issues open! I'm not completely sure if my request is right here, but others have also mentioned external css ressources (see comments 17, 18, 19, 27).

This is the wrong bug for your comments. :) I suspect you meant to vent in bug 15443. Then again, venting in bugs is not helpful. Patches are. There are folks in #webkit on irc.freenode.org who are happy to help you hack webkit.

I'm awfully sorry, but I can no longer work on this bug (nor on any other WebKit bug), due to a conflict of interests. I wish good luck to the people who will take over.

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

<<BUMP.>> Is there ANY update on this? This bug is YEARS OLD! Webkit is the ONLY browser not supporting this VITAL feature that would FINALLY allow SVG to be of some discernible purpose in this world. PLEASE MAKE PRIORITY. If there is confusion as to why this is needed... take the following example... On a website that uses "icons"... They could all be defined as SVG symbols in a simple XML document and then referenced by ID in the main document. Its basically a free database for graphics on any site. This is so essential, I can't even state...

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

due to a conflict of interests I am no longer filing bugs on webkit. please ensure I do not receive emails regarding this bug. my email doesnt seem to be in the cc list?

(In reply to comment #67) > due to a conflict of interests I am no longer filing bugs on webkit. > please ensure I do not receive emails regarding this bug. > > my email doesnt seem to be in the cc list? You added yourself to this bug again - see history. Make sure that you remove your email from this bug and that you are not following someone on this bug.

Is there anyone working on this bug? It’s a vital SVG feature.

(In reply to comment #69) > Is there anyone working on this bug? It’s a vital SVG feature. Yeah, I try to continue Cosmin's work. Wish me luck :P

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

(In reply to comment #70) > (In reply to comment #69) > > Is there anyone working on this bug? It’s a vital SVG feature. > > Yeah, I try to continue Cosmin's work. Wish me luck :P I wish you all luck on it.

Created attachment 120886 [details] Draft patch This is a draft patch which based on Cosmin's previous patch and adapted to the current repo. It doesn't support recursive references and sandboxing yet.

Comment on attachment 120886 [details] Draft patch Attachment 120886 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11073037

Comment on attachment 120886 [details] Draft patch Attachment 120886 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11076048

Comment on attachment 120886 [details] Draft patch You should definitely write some security tests with self referencing, self referencing across documents. Referencing of resources over documents (patterns, gradients, masks, clipper, filter). Also resources that reference resources (with self referencing as well).

Comment on attachment 120886 [details] Draft patch View in context: https://bugs.webkit.org/attachment.cgi?id=120886&action=review Thanks Reni for taking this over, it obviously needs lots of new testcases, as Dirk already pointed out, but here's a first round of code comments: > LayoutTests/ChangeLog:8 > + * svg/custom/resources/rgb.svg: Wrapped <rect> elements in <g> and added id attributes. This will affect lots of more testcases, all test cases in svg/batik import external files, and thus will change. I'd like to see the full set of changes, even when generated on non-mac platforms, to see how it affects those tests. > Source/WebCore/ChangeLog:9 > + Support external references on <use> by introducing CachedSVGDocument, > + which handles document subresources. In a final version of this patch, this ChangeLog needs to be much more verbose, as this is an important new feature. > Source/WebCore/GNUmakefile.list.am:2456 > + Source/WebCore/loader/cache/CachedSVGDocument.cpp \ Formatting. > Source/WebCore/loader/cache/CachedResourceClient.h:42 > + SVGDocumentType, Why is this not conditionalized, or rather why is it done in Cachedresource.h at all? It should be consistent. > Source/WebCore/loader/cache/CachedSVGDocument.cpp:41 > + setAccept("image/svg+xml"); Hm, can you avoid the temp strings. Or is this a char*, I didn't check? > Source/WebCore/loader/cache/CachedSVGDocument.cpp:80 > + String decodedText; > + if (m_data) { > + decodedText = m_decoder->decode(m_data->data(), m_data->size()); > + decodedText += m_decoder->flush(); > + } > + That can be rewritten using StringBuilder, to avoid reallocations when using operator+=. if (m_data) { StringBuilder builder; builder->append(....); m_document->setContent(builder.toString()); } else m_document->setContent(emptyString()); > Source/WebCore/svg/SVGUseElement.cpp:155 > + if (!href().startsWith("#")) { There should be better ways to detect that, or at least a central implementation like "static inline bool isExternalURIReference", maybe even in SVGURIReference. > Source/WebCore/svg/SVGUseElement.cpp:193 > + if (href().startsWith("#")) Ditto. > Source/WebCore/svg/SVGUseElement.cpp:482 > + Document* doc = referencedDocument(); s/doc/document. > Source/WebCore/svg/SVGUseElement.cpp:543 > +// targetElement = doc->getElementById(m_resourceId); Leftover? > Source/WebCore/svg/SVGUseElement.cpp:866 > + Document* doc = referencedDocument(); s/doc/document/ > Source/WebCore/svg/SVGUseElement.cpp:925 > + Document* doc = referencedDocument(); Ditto. > Source/WebCore/svg/SVGUseElement.cpp:1100 > +void SVGUseElement::setSVGDocument() This needs a better name, here and in CachedSVGDocument.

The previous suggestions from Krit and Niko are done. Even so I don't want to upload the patch till it doesn't work recursively. But now I faced a conceptual problem and I need a little help how it should work. The issue described below: +---- SVGCachedDocument -----+ +-------+ | +------ Document ------- + | | USE | ----> | | | | +-------+ | +------------------------+ | +----------------------------+ | | +---- SVGCachedDocument -----+ +-------+ | +------ Document ------- + | | USE | ----> | | | | +-------+ | +------------------------+ | +----------------------------+ | | +-------+ | RECT | +-------+ The test consist of three steps/files. The first one has a use element which points out the second file with an other use element. This second file refers to the third one with a rect element. Processing the first file the algoritm creates a document using a visible frame and a CacedSVGDocument for the second svg. To be able to insert the proper instance based on this CachedSVGDocument to the instance tree later, the cached documents should have the same frame like their anchestor. At the same time this cached document doesn't have any information about it. One solution could be if we pass around this anchestor's frame. I don't know if it is correct. On the other hand if I don't do this way and the Document is created w/o a frame then it's security origin will be "unique" and CachedResourceLoader::canRequest() in requestResource() will return with false and the request is rejected. We can avoid it if we create an own SecurityOrigin for the cached document, but I don't know again how rude hack is it. Whatever... my questions are the following: 1) Could I propagate a frame around the documents or is it a layer violation? 2) Shall I maintain a list inside the SVGCachedDocument about the USE elements, who refers to this particular SVGCachedDocument? 3) How should been bounded the new document to the UseElement what it belongs to? I CC-d Abarth 'cos I'm sure he is one of the most competent developers in security related questions.

Woah, this bug has is long and complicated. I'm happy to help, but it's going to take me a while to sort through everything here.

Crazy. Nate should really look at this patch. We'd like to load all sorts of documents through the CachedResourceLoader. It's unclear to me why SVGDocuments should be handled specially, but maybe there's a good reason.

Comment on attachment 120886 [details] Draft patch View in context: https://bugs.webkit.org/attachment.cgi?id=120886&action=review This patch isn't a crazy as I thought at first. You've still got a good number of bugs that you'll want to resolve before landing this. As I wrote above, Nat should really look at this patch before landing because he's more tuned into our long-term plans for created CachedDocument objects. > Source/WebCore/loader/cache/CachedSVGDocument.cpp:26 > +// Currently, we only need CachedSVGDocument for SVG documents. That's not really true. We can use them in HTML documents when using SVG-in-HTML. The guard is still fine, but we should remove the comment. > Source/WebCore/loader/cache/CachedSVGDocument.cpp:72 > + m_document = Document::create(0, KURL()); Why do we create a document with no URL? Shouldn't we use the URL from which we received the document? > Source/WebCore/loader/cache/CachedSVGDocument.cpp:81 > + m_document->setContent(decodedText); Do we want to wait for allDataReceived before calling setContent? It seems better to use functions that let us supply data incrementally rather than blowing away all the content for each network packet. >> Source/WebCore/svg/SVGUseElement.cpp:155 >> + if (!href().startsWith("#")) { > > There should be better ways to detect that, or at least a central implementation like "static inline bool isExternalURIReference", maybe even in SVGURIReference. This check seems wrong. Why do we need to special-case fragments here? > Source/WebCore/svg/SVGUseElement.h:135 > + CachedSVGDocument* m_cachedDocument; Don't we need to use a CachedResourceHandle rather than a raw pointer here?

Will this patch allow nested external references to work? I don't know enough about how <use> is supposed to work to be clear from this. In terms of the CachedResource piece of this: as far as I'm concerned, the design is fine as it stands. However, you should be warned that I'm working on getting a standardized way of loading Documents through the MemoryCache (see https://bugs.webkit.org/show_bug.cgi?id=49246), and I may rewrite and scrap CachedSVGDocument as a part of that refactoring. In the long run, a special CachedResource type for SVG documents should be unnecessary cruft, but as a temporary solution while I get the refactoring right, I don't object.

Created attachment 122648 [details] Proposed patch This patch is still a draft. It needs better changelog ofc and maybe more tests, etc... The following are contained in this version: * External resoruces are supported recursively too. * Self referencing is handled. * For some reason I needed to pass the ancestor's frame around (instead the security origin wouldn't be set to the proper value). I'm not sure wheter it's the correct solution. * There are two functions in CachedResourceLoader (checkInsecureContent() and canRequest()) which are dealing with security settings. I tried to understand what are they for but I guess it needs more refinement. * Tests are added to check self referencing, self referencing across documents (these come from w3.org) and referencing of resources over documents.

The previous patch is maybe a bit large because it contains a lot of batik tests rebase. But Niko would like to see their results. Furthermore I have to admit that LayoutTests/platform/mac-snowleopard/svg/hixie/intrinsic/003.html shouldn't be rebased. It was a mistake. Sorry.

Comment on attachment 122648 [details] Proposed patch Attachment 122648 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11252400

Comment on attachment 122648 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=122648&action=review Test results look very promising! I think there's a design flaw related to the SVGDocument creation, see below: > Source/WebCore/ChangeLog:10 > + You already said it: the ChangeLog is too short :-) > Source/WebCore/GNUmakefile.list.am:2486 > + Source/WebCore/loader/cache/CachedSVGDocument.cpp \ > + Source/WebCore/loader/cache/CachedSVGDocument.h \ Indentation. > Source/WebCore/loader/cache/CachedResourceClient.h:42 > + SVGDocumentType, You forgot about my last comment here: either conditionalize both SVGDocumentType & Resource or none, not a mixture. > Source/WebCore/loader/cache/CachedResourceLoader.cpp:420 > - > + Unnecessary. > Source/WebCore/loader/cache/CachedSVGDocument.cpp:26 > +// Currently, we only need CachedSVGDocument for SVG documents. This can be removed IMHO. > Source/WebCore/loader/cache/CachedSVGDocument.cpp:67 > + String decodedText; > + decodedText = m_decoder->decode(data->data(), data->size()); > + decodedText += m_decoder->flush(); > + m_document->setContent(decodedText); You should switch to StringBuilder here. I think I already mentioned that. > Source/WebCore/page/ContentSecurityPolicy.h:72 > + bool allowSVGDocumentFromSource(const KURL&) const; No SVG conditional here? Why? > Source/WebCore/svg/SVGURIReference.cpp:70 > + The newline can go as well. > Source/WebCore/svg/SVGUseElement.cpp:95 > - > + I'd remove the lines alltogether. > Source/WebCore/svg/SVGUseElement.cpp:162 > + if (!href().startsWith("#")) { This... > Source/WebCore/svg/SVGUseElement.cpp:167 > + m_cachedDocument->setFrame(document()->frame()); Aha, that's the suspicious part you found suspicious. You store the originating documents Frame in CachedSVGDocument::m_frame, and once CachedSVGDocument::data() is called, you pass it to the newly created SVGDocument. Let's assume the approach is right in general, you still have an implementation bug here: There doesn't seem to be any place that clears this Frame again, that means CachedSVGDocument will hold a RefPtr<Frame> keeping the original document alive. The approach can't be right though. Think of 1.svg referencing foo.svg, and 2.svg referncing foo.svg When I open Safari, browse to 1.svg, a CachedSVGDocument of foo.svg is created and its underlying SVGDocument is associated with 1.svg. Next I browse to 2.svg, which now doesn't load foo.svg from the network, but from the cache and sees the same CachedSVGDocument than 1.svg saw, but still associated with 1.svg. This implies you're always marrying the external document to the first originating document that references it. We need to discuss this on IRC I think - in the meanwhile we should check how other parts of WebCore solve this for HTML. > Source/WebCore/svg/SVGUseElement.cpp:201 > + if (href().startsWith("#")) ... should go in an extra helper function, either static inline here, or on SVGURIReference. > Source/WebCore/svg/SVGUseElement.cpp:339 > + SVGUseElement* correspondingUseElement = static_cast<SVGUseElement*>(correspondingElement); This needs a comment, I don't understand why its needed at present. > Source/WebCore/svg/SVGUseElement.cpp:341 > + if ((correspondingUseElement->m_cachedDocument > + && correspondingUseElement->m_cachedDocument->isLoading())) Outer braces can be omitted. > Source/WebCore/svg/SVGUseElement.cpp:495 > + Document* doc = referencedDocument(); s/doc/document/ No abbrev. pls ;-) > Source/WebCore/svg/SVGUseElement.cpp:538 > + Document* doc = referencedDocument(); Ditto. > Source/WebCore/svg/SVGUseElement.cpp:552 > - Element* targetElement = SVGURIReference::targetElementFromIRIString(href(), document()); > + String targetHref; > + if (!href().startsWith("#")) { > + KURL url(document()->baseURI(), href()); > + targetHref = url.string(); > + } else > + targetHref = href(); This should be documented, and reorderered: if (href().startsWith("#")) { .. } else { .. } to avoid one !. Also it should make use of the aforementioned new helper function, to avoid repeating the startsWith(..) logic in several places. > Source/WebCore/svg/SVGUseElement.cpp:797 > + Element* targetElement = SVGURIReference::targetElementFromIRIString(use->href(), doc); Why is the same logic regarding targetHref not needed here, as it is in buildShadowAndInstanceTree? This should be documented. > Source/WebCore/svg/SVGUseElement.cpp:877 > + if (use->m_cachedDocument && use->m_cachedDocument->isLoading()) I've already seen that above, should be refactored. Maybe a self-explainatory name for the function can avoid a comment for this alltogether. if (cachedDocumentIsStillLoading()) ? > Source/WebCore/svg/SVGUseElement.cpp:880 > + Document* doc = referencedDocument(); s/doc/document/ > Source/WebCore/svg/SVGUseElement.cpp:883 > + Element* targetElement = SVGURIReference::targetElementFromIRIString(use->href(), doc); Again the use->href() question. > Source/WebCore/svg/SVGUseElement.cpp:939 > + Document* doc = referencedDocument(); s/doc/document/ > Source/WebCore/svg/SVGUseElement.cpp:1015 > + && static_cast<SVGUseElement*>(target)->m_cachedDocument > + && static_cast<SVGUseElement*>(target)->m_cachedDocument->isLoading())); The function appears again :-) > Source/WebCore/svg/SVGUseElement.cpp:1122 > + if (renderer()) { If we have no renderer, why do we need to notify the parent->renderer()? Nothing changed in that case. I think you can early exit if (!renderer()) > Source/WebCore/svg/SVGUseElement.h:48 > + ~SVGUseElement(); This needs to be virtual! > Source/WebCore/svg/SVGUseElement.h:110 > + static void updateContainerOffset(SVGElementInstance* targetInstance); I don't think we need the targetInstance name here.

Created attachment 122933 [details] Proposed patch Yeah, I know... changelog :$ I'll summarize it tomorrow. This patch contains Niko's suggestions from his last review and from our IRC conversation.

Created attachment 122945 [details] Proposed patch I hope ews will like this version.

Comment on attachment 122945 [details] Proposed patch Attachment 122945 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11281256

Created attachment 123267 [details] Proposed patch This patch contains the missing changelogs and a speculative buildfix for the failing chromium ews.

Failing chromium tests need just rebaseline, like batik tests on all the other platforms.

Comment on attachment 123267 [details] Proposed patch Attachment 123267 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11173759 New failing tests: svg/batik/filters/filterRegions.svg svg/batik/text/textProperties.svg svg/batik/text/textAnchor.svg svg/batik/text/textLength.svg svg/batik/paints/patternRegions-positioned-objects.svg svg/batik/text/verticalText.svg svg/batik/text/textEffect2.svg svg/batik/filters/feTile.svg svg/batik/text/textLayout.svg svg/batik/text/textOnPathSpaces.svg svg/batik/text/textStyles.svg svg/batik/text/textPosition2.svg svg/batik/text/textOnPath.svg svg/batik/text/textLayout2.svg svg/batik/text/verticalTextOnPath.svg svg/batik/paints/gradientLimit.svg svg/batik/text/textProperties2.svg svg/batik/text/textFeatures.svg svg/batik/text/longTextOnPath.svg svg/batik/paints/patternRegionA.svg svg/batik/paints/patternRegions.svg svg/batik/text/textDecoration.svg svg/batik/paints/patternPreserveAspectRatioA.svg svg/batik/text/textPosition.svg fast/js/navigator-language.html

Comment on attachment 123267 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=123267&action=review > Source/WebCore/loader/cache/CachedSVGDocument.cpp:65 > + decodedText.append(m_decoder->flush()); Why do we always flush? Usually we flush only at the end. > Source/WebCore/loader/cache/CachedSVGDocument.cpp:90 > + m_document = SVGDocument::create(0, url()); > + m_document->setSecurityOrigin(SecurityOrigin::create(parentUrl)); Woah there. You shouldn't need to call setSecurityOrigin. That's bad news bears. Also, is url() the URL that we request or the one that we finally get the document from (e.g., at the end of the redirect chain)? It's very important that we set the URL of the document to the one at the end of the redirection chain. > Source/WebCore/page/ContentSecurityPolicy.cpp:686 > +bool ContentSecurityPolicy::allowSVGDocumentFromSource(const KURL& url) const > +{ > + DEFINE_STATIC_LOCAL(String, type, ("svg")); > + return checkSourceAndReportViolation(operativeDirective(m_svgSrc.get()), url, type); > +} We shouldn't just make up new Content-Security-Policy directives. Probably this should go under another directive... We can ask the working group which one. > Source/WebCore/svg/SVGUseElement.cpp:163 > + KURL url(document()->baseURI(), href()); Why not just use document()->completeURL(href()) ? > Source/WebCore/svg/SVGUseElement.cpp:168 > + m_cachedDocument->createDocument(document()->url()); You shouldn't need to pass in document()->url() here. A cached document can be re-used in many contexts. You shouldn't need to tell it about the context in which it is used. > Source/WebCore/svg/SVGUseElement.cpp:205 > + if (m_cachedDocument && m_cachedDocument->isLoaded()) > + return m_cachedDocument->document(); Is this document mutable? What happens when many SVGUseElements point to the same CachedSVGDocument ? > Source/WebCore/svg/SVGUseElement.h:134 > + CachedSVGDocument* m_cachedDocument; Should this be a CachedResourceHandle?

We really should break up this patch into many smaller patches that can be reviewed carefully.

Comment on attachment 123267 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=123267&action=review >> Source/WebCore/loader/cache/CachedSVGDocument.cpp:65 >> + decodedText.append(m_decoder->flush()); > > Why do we always flush? Usually we flush only at the end. Why do we always flush? Usually we flush only at the end. >> Source/WebCore/loader/cache/CachedSVGDocument.cpp:90 >> + m_document = SVGDocument::create(0, url()); >> + m_document->setSecurityOrigin(SecurityOrigin::create(parentUrl)); > > Woah there. You shouldn't need to call setSecurityOrigin. That's bad news bears. > > Also, is url() the URL that we request or the one that we finally get the document from (e.g., at the end of the redirect chain)? It's very important that we set the URL of the document to the one at the end of the redirection chain. Woah there. You shouldn't need to call setSecurityOrigin. That's bad news bears. Also, is url() the URL that we request or the one that we finally get the document from (e.g., at the end of the redirect chain)? It's very important that we set the URL of the document to the one at the end of the redirection chain. >> Source/WebCore/page/ContentSecurityPolicy.cpp:686 >> +bool ContentSecurityPolicy::allowSVGDocumentFromSource(const KURL& url) const >> +{ >> + DEFINE_STATIC_LOCAL(String, type, ("svg")); >> + return checkSourceAndReportViolation(operativeDirective(m_svgSrc.get()), url, type); >> +} > > We shouldn't just make up new Content-Security-Policy directives. Probably this should go under another directive... We can ask the working group which one. We shouldn't just make up new Content-Security-Policy directives. Probably this should go under another directive... We can ask the working group which one. >> Source/WebCore/svg/SVGUseElement.cpp:163 >> + KURL url(document()->baseURI(), href()); > > Why not just use document()->completeURL(href()) ? Why not just use document()->completeURL(href()) ? >> Source/WebCore/svg/SVGUseElement.cpp:168 >> + m_cachedDocument->createDocument(document()->url()); > > You shouldn't need to pass in document()->url() here. A cached document can be re-used in many contexts. You shouldn't need to tell it about the context in which it is used. You shouldn't need to pass in document()->url() here. A cached document can be re-used in many contexts. You shouldn't need to tell it about the context in which it is used. >> Source/WebCore/svg/SVGUseElement.cpp:205 >> + if (m_cachedDocument && m_cachedDocument->isLoaded()) >> + return m_cachedDocument->document(); > > Is this document mutable? What happens when many SVGUseElements point to the same CachedSVGDocument ? Is this document mutable? What happens when many SVGUseElements point to the same CachedSVGDocument ? >> Source/WebCore/svg/SVGUseElement.h:134 >> + CachedSVGDocument* m_cachedDocument; > > Should this be a CachedResourceHandle? Should this be a CachedResourceHandle?

> > Source/WebCore/loader/cache/CachedSVGDocument.cpp:65 > > + decodedText.append(m_decoder->flush()); > > Why do we always flush? Usually we flush only at the end. You are right. Updated. > > Source/WebCore/loader/cache/CachedSVGDocument.cpp:90 > > + m_document = SVGDocument::create(0, url()); > > + m_document->setSecurityOrigin(SecurityOrigin::create(parentUrl)); > > Woah there. You shouldn't need to call setSecurityOrigin. That's bad news bears. > > Also, is url() the URL that we request or the one that we finally get the document from (e.g., at the end of the redirect chain)? It's very important that we set the URL of the document to the one at the end of the redirection chain. This url is what we requested. I not really understand what else could it be. Perhaps do you think of the last piece of use->use->use->data chain? > > Source/WebCore/page/ContentSecurityPolicy.cpp:686 > > +bool ContentSecurityPolicy::allowSVGDocumentFromSource(const KURL& url) const > > +{ > > + DEFINE_STATIC_LOCAL(String, type, ("svg")); > > + return checkSourceAndReportViolation(operativeDirective(m_svgSrc.get()), url, type); > > +} > > We shouldn't just make up new Content-Security-Policy directives. Probably this should go under another directive... We can ask the working group which one. Which wg did you think of? Perhaps whatwg? Could you ask them pls? Or could you give me a contact who I can ask about this? > > Source/WebCore/svg/SVGUseElement.cpp:163 > > + KURL url(document()->baseURI(), href()); > > Why not just use document()->completeURL(href()) ? > > > Source/WebCore/svg/SVGUseElement.cpp:168 > > + m_cachedDocument->createDocument(document()->url()); > > You shouldn't need to pass in document()->url() here. A cached document can be re-used in many contexts. You shouldn't need to tell it about the context in which it is used. Okay. > > Source/WebCore/svg/SVGUseElement.cpp:205 > > + if (m_cachedDocument && m_cachedDocument->isLoaded()) > > + return m_cachedDocument->document(); > > Is this document mutable? What happens when many SVGUseElements point to the same CachedSVGDocument ? What does "mutable" mean in security context? It can be changed? According to the spec the referenced document inherits its ancestors attributes. So if the same source is referenced many times it could be displayed different. What should we do in this case? Should it be regenerated? But currently the cached document is available just until its inherited data are built into the shadow tree. Or should we regenerate it? But then it's not "mutable" anymore (at least IMHO). > > Source/WebCore/svg/SVGUseElement.h:134 > > + CachedSVGDocument* m_cachedDocument; > > Should this be a CachedResourceHandle? Yeah, it should be and it is already :)

Any comments would be appreciated :)

> > > Source/WebCore/loader/cache/CachedSVGDocument.cpp:90 > > > + m_document = SVGDocument::create(0, url()); > > > + m_document->setSecurityOrigin(SecurityOrigin::create(parentUrl)); > > > > Woah there. You shouldn't need to call setSecurityOrigin. That's bad news bears. > > > > Also, is url() the URL that we request or the one that we finally get the document from (e.g., at the end of the redirect chain)? It's very important that we set the URL of the document to the one at the end of the redirection chain. > > This url is what we requested. I not really understand what else could it be. Perhaps do you think of the last piece of use->use->use->data chain? In that case, we should be able to get the URL from the CachedSVGDocument object itself rather than needed to pipe it in from the caller. > > > Source/WebCore/page/ContentSecurityPolicy.cpp:686 > > > +bool ContentSecurityPolicy::allowSVGDocumentFromSource(const KURL& url) const > > > +{ > > > + DEFINE_STATIC_LOCAL(String, type, ("svg")); > > > + return checkSourceAndReportViolation(operativeDirective(m_svgSrc.get()), url, type); > > > +} > > > > We shouldn't just make up new Content-Security-Policy directives. Probably this should go under another directive... We can ask the working group which one. > > Which wg did you think of? Perhaps whatwg? Could you ask them pls? Or could you give me a contact who I can ask about this? http://www.w3.org/2011/webappsec/ http://lists.w3.org/Archives/Public/public-webappsec/ > > > Source/WebCore/svg/SVGUseElement.cpp:205 > > > + if (m_cachedDocument && m_cachedDocument->isLoaded()) > > > + return m_cachedDocument->document(); > > > > Is this document mutable? What happens when many SVGUseElements point to the same CachedSVGDocument ? > > What does "mutable" mean in security context? It can be changed? According to the spec the referenced document inherits its ancestors attributes. So if the same source is referenced many times it could be displayed different. What should we do in this case? Should it be regenerated? But currently the cached document is available just until its inherited data are built into the shadow tree. Or should we regenerate it? But then it's not "mutable" anymore (at least IMHO). What I meant to ask is whether the nodes in the document are exposed to JavaScript for modifications. The way you've written this patch, the document is shared by everyone who uses the cached object. That means if one person modifies the document's nodes, everyone else see the modifications, which seems very wrong. > > > Source/WebCore/svg/SVGUseElement.h:134 > > > + CachedSVGDocument* m_cachedDocument; > > > > Should this be a CachedResourceHandle? > > Yeah, it should be and it is already :) I'm not sure I understand. The declaration says CachedSVGDocument* rather than CachedResourceHandle<CachedSVGDocument>.

(In reply to comment #98) > In that case, we should be able to get the URL from the CachedSVGDocument object itself rather than needed to pipe it in from the caller. We both don't get this sentence. Anyhow, Reni made a new approach, that avoids calling setSecOrigin directly, maybe we should discuss that, once the patch is here. > http://www.w3.org/2011/webappsec/ > http://lists.w3.org/Archives/Public/public-webappsec/ Is this already decided? Shall Reni post a mail here to ask which directive to use? > > > > Source/WebCore/svg/SVGUseElement.cpp:205 > > > > + if (m_cachedDocument && m_cachedDocument->isLoaded()) > > > > + return m_cachedDocument->document(); > > > > > > Is this document mutable? What happens when many SVGUseElements point to the same CachedSVGDocument ? > > > > What does "mutable" mean in security context? It can be changed? According to the spec the referenced document inherits its ancestors attributes. So if the same source is referenced many times it could be displayed different. What should we do in this case? Should it be regenerated? But currently the cached document is available just until its inherited data are built into the shadow tree. Or should we regenerate it? But then it's not "mutable" anymore (at least IMHO). > > What I meant to ask is whether the nodes in the document are exposed to JavaScript for modifications. The way you've written this patch, the document is shared by everyone who uses the cached object. That means if one person modifies the document's nodes, everyone else see the modifications, which seems very wrong. That can not happen. The external CachedSVGDocument is always cloned into the target document, so if you script the shadow tree, or the <use> element it can never affect the CachedSVGDocument. Reni will include a test for this. > I'm not sure I understand. The declaration says CachedSVGDocument* rather than CachedResourceHandle<CachedSVGDocument>. She changed it locally I think :-)

Created attachment 124547 [details] Proposed patch > > http://www.w3.org/2011/webappsec/ > > http://lists.w3.org/Archives/Public/public-webappsec/ > Is this already decided? Shall Reni post a mail here to ask which directive to use? In this patch I rate the cached SVG document to the image directive. Lack of more precise source it's based on this conversation: http://old.nabble.com/preventing-SVG-script-from-running-td30014840.html > That can not happen. The external CachedSVGDocument is always cloned into the target document, so if you script the shadow tree, or the <use> element it can never affect the CachedSVGDocument. Reni will include a test for this. Yeah, a dynamic update test is attached. This test refers the same source two times via a use element. After an event (mouse click) the parent of the first one will change, but the second remains the same. > > I'm not sure I understand. The declaration says CachedSVGDocument* rather than CachedResourceHandle<CachedSVGDocument>. > She changed it locally I think :-) Exactly! Sorry if I was ambiguous :)

One more comment: I'll split the patch into smaller pieces ofc... but currently it's better to see the whole concept in the same block at least IMO :)

Comment on attachment 124547 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=124547&action=review > Source/WebCore/svg/SVGUseElement.h:135 > + RefPtr<Document> m_ownerDocument; Oops... this is unused... i forgot to remove it..

Comment on attachment 124547 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=124547&action=review This looks much better! Thanks. I have a few comments, but there's mostly just me trying to understand. The only real important one is to use response().url() rather than url() when creating the document. I think we're ready to split this up into manageable pieces and land it. > Source/WebCore/loader/cache/CachedResourceLoader.cpp:370 > +#if ENABLE(SVG) > + case CachedResource::SVGDocumentResource: > +#endif Treating this as an image seems ok. We should confirm with the working group (but we don't need to block this patch on that). Would you be willing to email public-webappsec about this question? > Source/WebCore/loader/cache/CachedSVGDocument.cpp:65 > + if (!allDataReceived) > + return; So, there's no incremental rendering. > Source/WebCore/loader/cache/CachedSVGDocument.cpp:67 > + if (data) { You've checked that |data| accumulates (and isn't just the last block)? I don't remember off-hand. > Source/WebCore/loader/cache/CachedSVGDocument.cpp:92 > +void CachedSVGDocument::createDocument() > +{ > + if (m_document) > + return; > + > + m_document = SVGDocument::create(0, url()); > +} Why do you create this document with this function? I would have expected you to create the document right before calling m_document->setContent(). Also, this should be response().url() so that we get the final URL, which is another reason we need to wait until after we've gotten a response before creating the document. > Source/WebCore/loader/cache/CachedSVGDocument.h:38 > + CachedSVGDocument(const ResourceRequest&); Please add the keyword explicit to one-argument constructors. > Source/WebCore/page/ContentSecurityPolicy.h:109 > + This change seem spurious. > Source/WebCore/platform/network/chromium/ResourceRequest.h:49 > + TargetIsSVGResource, I bet there's a matching enum in the Chromium WebKit API. I'm surprised there isn't a COMPILE_ASSERT failures with this patch. > Source/WebCore/svg/SVGUseElement.cpp:107 > + m_cachedDocument->removeClient(this); Bad indent.

Comment on attachment 124547 [details] Proposed patch Attachment 124547 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11366753 New failing tests: svg/batik/filters/filterRegions.svg svg/batik/text/textProperties.svg svg/batik/text/textAnchor.svg svg/batik/text/textLength.svg svg/batik/paints/patternRegions-positioned-objects.svg http/tests/inspector/inspect-element.html svg/batik/text/textEffect2.svg svg/batik/filters/feTile.svg fast/frames/lots-of-objects.html svg/batik/text/textLayout.svg svg/batik/text/textOnPathSpaces.svg css2.1/20110323/abspos-containing-block-initial-004d.htm svg/batik/text/textPosition2.svg svg/batik/text/textOnPath.svg svg/batik/text/textLayout2.svg css2.1/20110323/abspos-containing-block-initial-004b.htm svg/batik/paints/gradientLimit.svg svg/batik/text/textProperties2.svg svg/batik/text/textFeatures.svg svg/batik/text/longTextOnPath.svg svg/batik/paints/patternRegionA.svg svg/batik/paints/patternRegions.svg svg/batik/text/textDecoration.svg svg/batik/paints/patternPreserveAspectRatioA.svg svg/batik/text/textPosition.svg

> This looks much better! Thanks. I have a few comments, but there's mostly just me trying to understand. The only real important one is to use response().url() rather than url() when creating the document. I think we're ready to split this up into manageable pieces and land it. We were talking about the splitting with Niko on IRC. We thought that bisecting the patch would be good. The first part would contain only the new class (CachedSVGDocument) with the forthcoming changes. This is just for testing the new code on the different platforms w/o any functional changes. And the second patch would bind it into the use element and add the new tests. Is it okay for you? > > Source/WebCore/loader/cache/CachedResourceLoader.cpp:370 > > +#if ENABLE(SVG) > > + case CachedResource::SVGDocumentResource: > > +#endif > > Treating this as an image seems ok. We should confirm with the working group (but we don't need to block this patch on that). Would you be willing to email public-webappsec about this question? I've sent a mail. Waiting for reply... > > Source/WebCore/loader/cache/CachedSVGDocument.cpp:67 > > + if (data) { > > You've checked that |data| accumulates (and isn't just the last block)? I don't remember off-hand. Yeah, it contains all blocks already. > > Source/WebCore/loader/cache/CachedSVGDocument.cpp:92 > > +void CachedSVGDocument::createDocument() > > +{ > > + if (m_document) > > + return; > > + > > + m_document = SVGDocument::create(0, url()); > > +} > > Why do you create this document with this function? I would have expected you to create the document right before calling m_document->setContent(). Right. It was needed when I passed around the ancestor's url. This was the only way to get it... but never mind... it's useless already and I moved back into data(). > Also, this should be response().url() so that we get the final URL, which is another reason we need to wait until after we've gotten a response before creating the document. > > > Source/WebCore/loader/cache/CachedSVGDocument.h:38 > > + CachedSVGDocument(const ResourceRequest&); > > Please add the keyword explicit to one-argument constructors. Ok. > > Source/WebCore/page/ContentSecurityPolicy.h:109 > > + > > This change seem spurious. Ok. > > Source/WebCore/platform/network/chromium/ResourceRequest.h:49 > > + TargetIsSVGResource, > > I bet there's a matching enum in the Chromium WebKit API. I'm surprised there isn't a COMPILE_ASSERT failures with this patch. Right again. I added it to TargetIsImage group. Is it right? > > Source/WebCore/svg/SVGUseElement.cpp:107 > > + m_cachedDocument->removeClient(this); > > Bad indent. Ok.

Created attachment 124750 [details] Proposed patch This is how the first part would look like

Attachment 124750 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource First, rewinding head to replay your work on top of it... Applying: Fix compilation errors on build-webkit --debug --no-workers on mac. Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/qt/Skipped CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #103) > (From update of attachment 124547 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124547&action=review > > Treating this as an image seems ok. We should confirm with the working group (but we don't need to block this patch on that). Would you be willing to email public-webappsec about this question? As I wrote on the list we can be sure that treating these resources as an image is right indeed. For the others: after trying to display an external resource test with various directives I saw that only the following one blocked the load of the resource: X-WebKit-CSP: default-src *; img-src 'none'

Any comments would be welcomed ;)

Comment on attachment 124750 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=124750&action=review Sorry Reni for the long delay - I hoped someone else would look as well, anyhow almost there, but still some issues that lead to r-: You should mention in the ChangeLog - a bit more explicit - that this is not yet testable, as its still unused. > Source/WebCore/loader/cache/CachedResourceLoader.cpp:280 > + You can remove this. > Source/WebCore/loader/cache/CachedResourceLoader.cpp:380 > + Ditto. > Source/WebCore/loader/cache/CachedResourceLoader.cpp:412 > - > + Ditto. > Source/WebCore/loader/cache/CachedResourceLoader.cpp:414 > - > + Ditto. > Source/WebCore/loader/cache/CachedSVGDocument.cpp:61 > + ASSERT(m_document.get()); m_document is 0 initially, this will fail in debug builds! > Source/WebCore/loader/cache/CachedSVGDocument.cpp:67 > + if (data) { > + StringBuilder decodedText; Ok, this is just as CachedResource::data, but doesn't store m_data, but m_document - seems fine. > Source/WebCore/loader/cache/CachedSVGDocument.cpp:78 > +void CachedSVGDocument::error(CachedResource::Status status) Why is this custom error() impl needed? CachedResource::error only contains an additional m_data.clear() - that can't hurt, so why not leave it to the base-class?

Created attachment 127563 [details] Proposed patch > Why is this custom error() impl needed? CachedResource::error only contains an additional m_data.clear() - that can't hurt, so why not leave it to the base-class? Right, I removed it. Changelog is also updated. This first part contains just a minimal explanation about the newly added classes. The functionality and its description will be in the follow-up patch.

Comment on attachment 127563 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=127563&action=review Some nitpicks: > Source/WebCore/loader/cache/CachedResourceClient.h:-48 > - You should omit this change. > Source/WebCore/loader/cache/CachedSVGDocument.cpp:26 > + One newline too much :-) > Source/WebCore/loader/cache/CachedSVGDocument.cpp:31 > +#include "CachedResourceClient.h" > +#include "CachedResourceHandle.h" > +#include "SVGDocument.h" Already included by the header. > Source/WebCore/loader/cache/CachedSVGDocument.cpp:33 > +#include "TextResourceDecoder.h" To be safe, just move this into the header. > Source/WebCore/loader/cache/CachedSVGDocument.cpp:68 > + m_document = SVGDocument::create(0, response().url()); ah, the 0 parameter needs to be explained here first. > Source/WebCore/loader/cache/CachedSVGDocument.h:25 > + Oh this is missing #if ENABLE(SVG) guards! > Source/WebCore/loader/cache/CachedSVGDocument.h:33 > +class Document; Unused. > Source/WebCore/loader/cache/CachedSVGDocument.h:34 > +class TextResourceDecoder; Is this needed? IIRC. RefPtr<T> requires a T include, not only a forward. Maybe this works, because sth. else pulls in this header already? If so, you can remove the class forward. > Source/WebCore/loader/cache/CachedSVGDocument.h:47 > + virtual bool schedule() const { return true; } This seems like a left-over, no other class uses this, you can remove it. > Source/WebCore/loader/cache/CachedSVGDocument.h:52 > + RefPtr<SharedBuffer> m_data; Unused, please remove.

Created attachment 127971 [details] Proposed first part

Comment on attachment 127971 [details] Proposed first part Looks great! r=me.

Committed r108785: <http://trac.webkit.org/changeset/108785>

The commited patch was just the initial step of supporting external resources. Further patches are coming...

(In reply to comment #115) > Committed r108785: <http://trac.webkit.org/changeset/108785> I landed a speculative buildfix for Qt MIPS and Qt SH1 - http://trac.webkit.org/changeset/108804 (ENABLE(SVG) && !ENABLE(XSLT) platforms) The following code was incorrect, because the first case didn't have statement: ----------------- switch ... { ... case .... : #if 0 case .... : break; #endif } ----------------- My fix isn't the best, but in this kind of hacked code I couldn't find nicer fix.

Comment on attachment 127971 [details] Proposed first part Remove flags, because it is landed.

Created attachment 129054 [details] Proposed second part This patch binds the previously introduced CachedSVGDocument class into SVGUseElement caching mechanism. Patch only contains the code base of this modification. Since the test rebaseline will have bigger size I plan to upload it in a follow-up patch or to rebase them as an unreviewed gardening.

Comment on attachment 129054 [details] Proposed second part View in context: https://bugs.webkit.org/attachment.cgi?id=129054&action=review Looks good in general! Still needs an iteration, as ToT changed SVGUseElements shadow tree construction. > Source/WebCore/ChangeLog:20 > + of the size of that test refactor they will be commited in a follow-up patch. Okay, once the code is fine, you should still upload one big final patch, that shows that you modified the chromium expectations file correctly (aka. cr-linux turns green) > Source/WebCore/svg/SVGURIReference.cpp:59 > + else > + base = start ? KURL(document->baseURI(), url.substring(0, start)) : document->baseURI(); I'd prefer: else if (start) { base = KURL(d... else { base = document->baseURI(); Isn't there a generic function for that? Just guessing, but we should verify that. > Source/WebCore/svg/SVGUseElement.cpp:1109 > + static_cast<RenderSVGShadowTreeRootContainer*>(renderer())->markShadowTreeForRecreation(); > + renderer()->updateFromElement(); This needs to be updated, within the new SVGUseElement implementation that landed some hours ago. SVGUseElement is now properly integrated within the standard ShadowTree. You'll likely find that your code needs to be changed now, but the attachment and rebuilding should be much more straight-forward & logical.

Created attachment 129471 [details] Proposed second part (In reply to comment #120) > (From update of attachment 129054 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129054&action=review > > Looks good in general! Still needs an iteration, as ToT changed SVGUseElements shadow tree construction. > > > Source/WebCore/ChangeLog:20 > > + of the size of that test refactor they will be commited in a follow-up patch. > > Okay, once the code is fine, you should still upload one big final patch, that shows that you modified the chromium expectations file correctly (aka. cr-linux turns green) Expecteds are attached, but these are generated on Mac Lion. Unfortunately I only have access to this (besides linux ofc.). Btw the patch caused 0 regression on it. > > Source/WebCore/svg/SVGURIReference.cpp:59 > > + else > > + base = start ? KURL(document->baseURI(), url.substring(0, start)) : document->baseURI(); > > I'd prefer: > else if (start) { > base = KURL(d... > else { > base = document->baseURI(); > > Isn't there a generic function for that? Just guessing, but we should verify that. Done. I think this is the way how we should handle this. > > Source/WebCore/svg/SVGUseElement.cpp:1109 > > + static_cast<RenderSVGShadowTreeRootContainer*>(renderer())->markShadowTreeForRecreation(); > > + renderer()->updateFromElement(); > > This needs to be updated, within the new SVGUseElement implementation that landed some hours ago. > SVGUseElement is now properly integrated within the standard ShadowTree. > > You'll likely find that your code needs to be changed now, but the attachment and rebuilding should be much more straight-forward & logical. Nice patch :) I adopted my modification to that.

Comment on attachment 129471 [details] Proposed second part View in context: https://bugs.webkit.org/attachment.cgi?id=129471&action=review Nice patch, still another iteration needed. I've also noticed that some of the svg/batik/ tests, don't include the correct CSS style sheet, that's why the text is so large in eg. textSTyles-expected.png. You could fix these in one-shot, as the tests already need rebaselining anyways. > Source/WebCore/svg/SVGUseElement.cpp:90 > + , m_cachedDocument(0) This doesn't seem to be needed at all, is it? > Source/WebCore/svg/SVGUseElement.cpp:164 > + if (isExternalURIReference(href())) { You need to test transitions from xlink:href pointing to internal -> external resource, and the other way round. Currently if you change from an external href to an internal, m_cachedDocument won't be released, if I read the code correctly. This is important to get right! Also if the <use> element itself gets removed from the tree (removedFromDocument) shouldn't we release the document? I'm thinking of a <use> element which loads a slooow external resource (say over 1mb large). Now if we remove the <use> from the document, while the external document is still loading, we could abort the load, and release m_cachedDocument, no? Whatever you decide, write a test for it :-) > Source/WebCore/svg/SVGUseElement.cpp:647 > + if (use->cachedDocumentIsStillLoading()) > + return; Shouldn't we detect this earlier? We end up expanding parts of the tree - and then throw away the whole shadow tree, once the external document loaded. I'd rather ASSERT here that the document is not loading, and detect this earlier. (Walk whole instance tree to find instances corresponding to use elements, and check if those use elements are still loading..) > Source/WebCore/svg/SVGUseElement.cpp:770 > + || (target->nodeName() == SVGNames::useTag && (static_cast<SVGUseElement*>(target))->cachedDocumentIsStillLoading())); This assertion change could go away, if you'd stop expanding parts of the tree only. > Source/WebCore/svg/SVGUseElement.cpp:879 > + renderer()->updateFromElement(); This shouldn't be necessary anymore. > Source/WebCore/svg/SVGUseElement.cpp:882 > + ASSERT(parent); No need to check that, you already tested inDocument(). This is also not 100% correct, you need to mark the <use> itself as needing layout. So RenderSVGResource::markForLayoutAndParentResourceInvalidation(renderer()) is the way to go here.

Created attachment 130341 [details] Proposed second part I'm not pretty sure I understood everything correctly what you asked, but here is what i thought... > Also if the <use> element itself gets removed from the tree (removedFromDocument) shouldn't we release the document? > I'm thinking of a <use> element which loads a slooow external resource (say over 1mb large). Now if we remove the <use> from the document, while the external document is still loading, we could abort the load, and release m_cachedDocument, no? This is an ambiguous part for me. If i remove the cachedDocument here, then it broke the dynamic update by changing from external to internal. So I missed this part. The others are fulfilled.

Comment on attachment 130341 [details] Proposed second part View in context: https://bugs.webkit.org/attachment.cgi?id=130341&action=review Almost there, next round of comments: > LayoutTests/svg/custom/struct-use-recursion-02-t.svg:1 > +<?xml version="1.0" encoding="UTF-8"?> Where did you grab these from? If its SVG 1.1 2nd edition testsuite, these should go to svg/W3C-SVG-1.1-SE/ > LayoutTests/svg/dynamic-updates/SVGUseElement-dom-href2-attr-expected.txt:1 > +CONSOLE MESSAGE: line 10: ReferenceError: Can't find variable: repaintTest Oops. This testcase doesn't work. > LayoutTests/svg/dynamic-updates/script-tests/SVGUseElement-dom-href1-attr.js:1 > +// [Name] SVGFEUseElement-dom-href1-attr.js Description is wrong. > LayoutTests/svg/dynamic-updates/script-tests/SVGUseElement-dom-href1-attr.js:4 > +description("Tests dynamic updates of the 'href' attribute of the SVGFEUseElement object") Ditto. > LayoutTests/svg/dynamic-updates/script-tests/SVGUseElement-dom-href1-attr.js:32 > + useElement.setAttributeNS(xlinkNS, "xlink:href", "../custom/resources/rgb.svg#R"); How about referencing #G? Green is better as pass color, than red ;-) > LayoutTests/svg/dynamic-updates/script-tests/SVGUseElement-dom-href2-attr.js:1 > +// [Name] SVGFEUseElement-dom-href1-attr.js Description is wrong. > LayoutTests/svg/dynamic-updates/script-tests/SVGUseElement-dom-href2-attr.js:4 > +description("Tests dynamic updates of the 'href' attribute of the SVGFEUseElement object") Ditto. > LayoutTests/svg/dynamic-updates/script-tests/SVGUseElement-dom-href2-attr.js:9 > +rootSVGElement.appendChild( > +defsElement); Typo. > LayoutTests/svg/dynamic-updates/script-tests/SVGUseElement-dom-href2-attr.js:22 > +rectElement.setAttribute("fill", "#408067"); Why not green? > Source/WebCore/ChangeLog:16 > + SVGURIReference::targetElementFromIRIString() also need to be extended. The creation > + of baseURI should be based on the referenced document's URL instead of the actual one and "The baseURI computation needs to take the referenced documents URL into account, instead of the current documents." ? > Source/WebCore/svg/SVGUseElement.cpp:236 > + if (m_cachedDocument && !isExternalURIReference(href())) { Aha, you handle the cleanup here, when switching from internal <-> external. That seems fine. > Source/WebCore/svg/SVGUseElement.cpp:246 > + || SVGExternalResourcesRequired::isKnownAttribute(attrName)) { This should be left out. Whitespace only change. > Source/WebCore/svg/SVGUseElement.cpp:268 > + SVGUseElement* correspondingUseElement = static_cast<SVGUseElement*>(correspondingElement); > + if (correspondingUseElement->cachedDocumentIsStillLoading()) Huh, how can you be sure its a SVGUseElement? Even if this is debug code, this crashes :-) You need to check the hasTagName first, before casting. > Source/WebCore/svg/SVGUseElement.cpp:746 > + RefPtr<SVGSVGElement> svgElement = SVGSVGElement::create(SVGNames::svgTag, document); How about you move the assert(document) into referencedDocument. Then you could just do a s/document()/referencedDocument()/ and save introducing local variables here. > Source/WebCore/svg/SVGUseElement.cpp:921 > + Element* parent = parentElement(); > + if (!parent->renderer()) > + return; > + > + RenderSVGResource::markForLayoutAndParentResourceInvalidation(parent->renderer()); I asked this before I think: why the parent->renderer() only? Why not this? Is this needed at all, when doing invalidateShadowTree(). I would have expected that this is enough. If not, then markForLayoutAndParentResourceInvalidation(renderer()), but not parent->renderer(). > Source/WebCore/svg/SVGUseElement.cpp:934 > + if (node->correspondingUseElement()) { if (SVGUseElement* use = node->correspondingUseElement()) { .. Saves calling it twice.

Created attachment 130373 [details] Proposed second part > > LayoutTests/svg/custom/struct-use-recursion-02-t.svg:1 > > +<?xml version="1.0" encoding="UTF-8"?> > > Where did you grab these from? If its SVG 1.1 2nd edition testsuite, these should go to svg/W3C-SVG-1.1-SE/ They were moved into a new W3C-SVG-1.2-Tiny directory because they come from that testsuite. > > LayoutTests/svg/dynamic-updates/SVGUseElement-dom-href2-attr-expected.txt:1 > > +CONSOLE MESSAGE: line 10: ReferenceError: Can't find variable: repaintTest > > Oops. This testcase doesn't work. Sorry, updated. > > Source/WebCore/svg/SVGUseElement.cpp:268 > > + SVGUseElement* correspondingUseElement = static_cast<SVGUseElement*>(correspondingElement); > > + if (correspondingUseElement->cachedDocumentIsStillLoading()) > > Huh, how can you be sure its a SVGUseElement? Even if this is debug code, this crashes :-) You need to check the hasTagName first, before casting. Right. Fixed. > > Source/WebCore/svg/SVGUseElement.cpp:746 > > + RefPtr<SVGSVGElement> svgElement = SVGSVGElement::create(SVGNames::svgTag, document); > > How about you move the assert(document) into referencedDocument. Then you could just do a s/document()/referencedDocument()/ and save introducing local variables here. Good idea. Done. > > Source/WebCore/svg/SVGUseElement.cpp:921 > > + Element* parent = parentElement(); > > + if (!parent->renderer()) > > + return; > > + > > + RenderSVGResource::markForLayoutAndParentResourceInvalidation(parent->renderer()); > > I asked this before I think: why the parent->renderer() only? Why not this? > Is this needed at all, when doing invalidateShadowTree(). I would have expected that this is enough. If not, then markForLayoutAndParentResourceInvalidation(renderer()), but not parent->renderer(). Really, we don't need it anymore. It's thrown off. > > Source/WebCore/svg/SVGUseElement.cpp:934 > > + if (node->correspondingUseElement()) { > > if (SVGUseElement* use = node->correspondingUseElement()) { > .. > Saves calling it twice. Done.

Comment on attachment 130373 [details] Proposed second part View in context: https://bugs.webkit.org/attachment.cgi?id=130373&action=review Discussed some things with reni on IRC, for the record: > Source/WebCore/svg/SVGUseElement.cpp:163 > + if (isExternalURIReference(href())) { Oh, just noticing that this is in parseAttribute, not in svgAttributeChanged. You won't react properly to SVG DOM changes like myUseElement.href.baseVal.value = "some-external-reference"; it only works through setAttribute() using this way. Move it into svgAttributeChanged, and it should work - please add a testcase covering this. > Source/WebCore/svg/SVGUseElement.cpp:248 > + || SVGExternalResourcesRequired::isKnownAttribute(attrName)) { Please restore the formatting. > Source/WebCore/svg/SVGUseElement.cpp:270 > +// SVGUseElement* correspondingUseElement = static_cast<SVGUseElement*>(element); Commented code should go away. > Source/WebCore/svg/SVGUseElement.cpp:919 > + for (SVGElementInstance* node = targetElementInstance->firstChild(); node; node = node->nextSibling()) { I'd s/node/instance/ here otherwise this is confusing. You're walking a SVGElementInstance tree here, not a DOM tree. > Source/WebCore/svg/SVGUseElement.h:1 > +/* Copyright (C) 2004, 2005, 2006, 2007, 2008 Nikolas Zimmermann <zimmermann@kde.org> Hm? > Source/WebCore/svg/SVGUseElement.h:107 > + Document* referencedDocument() const; > + virtual void notifyFinished(CachedResource*); For beauty, reorder this :-)

Created attachment 130393 [details] Proposed second part > Discussed some things with reni on IRC, for the record: > > > Source/WebCore/svg/SVGUseElement.cpp:163 > > + if (isExternalURIReference(href())) { > > Oh, just noticing that this is in parseAttribute, not in svgAttributeChanged. > You won't react properly to SVG DOM changes like myUseElement.href.baseVal.value = "some-external-reference"; it only works through setAttribute() using this way. > Move it into svgAttributeChanged, and it should work - please add a testcase covering this. This and the other fixes are done. The old dyn-up tests are also updated to produce nicer expected pngs.

Comment on attachment 130393 [details] Proposed second part Attachment 130393 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11832444 New failing tests: svg/W3C-SVG-1.2-Tiny/struct-use-recursion-02-t.svg svg/W3C-SVG-1.2-Tiny/struct-use-recursion-01-t.svg svg/W3C-SVG-1.2-Tiny/struct-use-recursion-03-t.svg

Comment on attachment 130393 [details] Proposed second part Attachment 130393 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11836485 New failing tests: svg/W3C-SVG-1.2-Tiny/struct-use-recursion-02-t.svg svg/W3C-SVG-1.2-Tiny/struct-use-recursion-01-t.svg svg/W3C-SVG-1.2-Tiny/struct-use-recursion-03-t.svg

The only failing tests are the newly introduced recursive tests. The expecteds were created on Mac Lion. Furthermore they contain texts too what easily could cause mismatches. Unfortunately I cannot check it on chrome build but I'm sure that all we need is just a rebase.

(In reply to comment #130) > The only failing tests are the newly introduced recursive tests. The expecteds were created on Mac Lion. Furthermore they contain texts too what easily could cause mismatches. Unfortunately I cannot check it on chrome build but I'm sure that all we need is just a rebase. You should suppress those as IMAGE+TEXT IMAGE TEXT PASS in the expectations - this is the sledgehammer turning cr-linux green until those are rebaselined - I was told this is best practice.

I tried out what we talked on IRC about: leaving fragmentIdentifierFromIRIString() in its original state and rewriting targetElementFromIRIString() as follows: Element* SVGURIReference::targetElementFromIRIString(const String& iri, Document* document, Document* externalDocument, String* fragmentIdentifier) { String id = fragmentIdentifierFromIRIString(iri, document); if (fragmentIdentifier) *fragmentIdentifier = id; if (id.isEmpty()) return 0; if (externalDocument) return externalDocument->getElementById(id); else if (isExternalURIReference(iri)) return 0; // Non-existing external resource // Local reference return document->getElementById(id); } Furthermore externalResource parameter would come from a new externalDocument() function what only gives the external document or NULL. Well... i shall continue to think it isn't the good idea. Let's take an example: <use y="80" xlink:href="resources/rgb.svg#RGB"/> In this case: KURL base = start ? KURL(document->baseURI(), url.substring(0, start)) : document->baseURI(); will results: document->baseURI() = file://path/to/reference/resources/rgb.svg#RGB url.substring(0, start) = resources/rgb.svg#RGB KURL kurl(base, fragmentIdentifier); results: base = file://path/to/reference/resources/resources/rgb.svg#RGB This is obviously wrong and also will fail at: if (equalIgnoringFragmentIdentifier(kurl, document->url())) So I submit that we should pass information to fragmentIdentifierFromIRIString() about the location of resource (external or internal) and create the path according to it. And if we do this way then we don't need to extend targetElementFromIRIString(). Furthermore I agree that the current solution is not the cleanest approach. We could maybe keep the externalDocument() function and stick some comment to the resource path creating part to make the source more readable. What do you think?

(In reply to comment #132) > I tried out what we talked on IRC about: leaving fragmentIdentifierFromIRIString() in its original state and rewriting targetElementFromIRIString() as follows: ... > Furthermore externalResource parameter would come from a new externalDocument() function what only gives the external document or NULL. Sounds good so far, much cleaner. > Well... i shall continue to think it isn't the good idea. Let's take an example: > <use y="80" xlink:href="resources/rgb.svg#RGB"/> > In this case: I can't follow your example. First of all let's agree on a baseURI for the host document containing the <use>. "/home/reni/testcase.svg" contains this <use>, which is supposed to reference the "RGB" element from the "/home/reni/resources/rgb.svg" file). That means the host document has a baseURI like this: "file:///home/reni/testcase.svg". targetElementFromIRIString() should be called with the _host_ document() as parameter (not the external one), as you correctly to in the revised version from above. 'start' is named badly, and means the startOfTheFragmentIdentifier. > KURL base = start ? KURL(document->baseURI(), url.substring(0, start)) : document->baseURI(); > will results: document->baseURI() = file://path/to/reference/resources/rgb.svg#RGB XXX This baseURI is wrong, it's not the baseURI of the host document(). See beow. > url.substring(0, start) = resources/rgb.svg#RGB XXXX This doesn't include the fragment part, it's only resources/rgb.svg > KURL kurl(base, fragmentIdentifier); results: > base = file://path/to/reference/resources/resources/rgb.svg#RGB This is unfortunately wrong, I'll repaint your code with the correct baseURI. KURL base = KURL("file:///home/reni/testcase.svg", "resources/rgb.svg"); results to: base = "file:///home/reni/resources/rgb.svg". Now KURL kur(base, fragmentIdentifier) is called with: KURL kurl("file:///home/reni/resources/rgb.svg", "#RGB"); This should result in "file:///home/reni/resources/rgb.svg#RGB". > This is obviously wrong and also will fail at: > if (equalIgnoringFragmentIdentifier(kurl, document->url())) Yeah, this is of course wrong now. It would need to be verified against the _external_ document URI here. I'd still like to avoid having to pass both the internal & external document to fragmentIdentifierFromIRIString. This requires to decouple the KURL construction fully from the validation of the URL. Here's an untested sketch-up: static inline KURL urlFromIRIStringWithFragmentIdentifier(const String& url, Document* document, String& fragmentIdentifier) { ASSERT(document); size_t startOfFragmentIdentifier = url.find('#'); if (startOfFragmentIdentifier == notFound) return KURL(); // Exclude the '#' character when determining the fragmentIdentifier. fragmentIdentifier = url.substring(start + 1); if (startOfFragmentIdentifier) { KURL base(document->baseURI(), url.substring(0, start)); return KURL(base, url.substring(start)); } return KURL(document->baseURI(), url.substring(start)); } Element* SVGURIReference::targetElementFromIRIString(const String& iri, Document* document, Document* externalDocument, String* fragmentIdentifier) { // If there's no fragment identifier contained within the IRI string, we can't lookup an element. String id; KURL url = urlFromIRIStringWithFragmentIdentifier(iri, document, id); if (url == KURL()) return 0; // If we're requesting an external resources, and externalDocument is non-zero, the load already succeeded. // Go ahead and check if the externalDocuments URL matches the expected URL, that we resolved using the // host document before in urlFromIRIStringWithFragmentIdentifier(). For internal resources, the same // assumption must hold true, just with the host documents URL, not the external documents URL. if (!equalIgnoringFragmentIdentifier(url, externalDocument ? externalDocument->url() : document->url())) return 0; if (fragmentIdentifier) *fragmentIdentifier = id; if (id.isEmpty) return 0; if (externalDocument) return externalDocument->getElementById(id); else if (isExternalURIReference(iri)) return 0; // Non-existing external resource return document->getElementById(id); } I think its more obvious what's going on now.

Created attachment 131061 [details] Proposed second part

Created attachment 131597 [details] Proposed second part

Comment on attachment 131597 [details] Proposed second part View in context: https://bugs.webkit.org/attachment.cgi?id=131597&action=review Looks much better, still some minor things to clean up: > LayoutTests/platform/mac-lion/svg/W3C-SVG-1.2-Tiny/struct-use-recursion-01-t-expected.txt:1 > +layer at (0,0) size 800x600 The location of these test results is wrong. Lion results should be in platform/mac/svg, not in platform/mac-lion/svg. Also a ChangeLog seems to be missing for LayoutTests. > LayoutTests/svg/custom/use-referencing-an-image-expected.svg:4 > +<svg version="1.2" baseProfile="tiny" xml:id="svg-root" width="100%" height="100%" > + viewBox="0 0 480 360" xmlns="http://www.w3.org/2000/svg" > + xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:xe="http://www.w3.org/2001/xml-events"> You should strip off all unneeded variables, like xmlns:xlink, xmlns:xe, width/height xml:id/baseProfile/version here. > LayoutTests/svg/custom/use-referencing-an-image.svg:3 > +<svg version="1.2" baseProfile="tiny" xml:id="svg-root" width="100%" height="100%" > + viewBox="0 0 480 360" xmlns="http://www.w3.org/2000/svg" Ditto. > LayoutTests/svg/custom/use-referencing-indirectly-itself-expected.svg:3 > +<svg version="1.2" baseProfile="tiny" xml:id="svg-root" width="100%" height="100%" > + viewBox="0 0 480 360" xmlns="http://www.w3.org/2000/svg" Ditto. > LayoutTests/svg/custom/use-referencing-indirectly-itself-expected.svg:6 > + <rect id="greenRect" width="100" height="100" fill="green"/> id is not need here, please make those as minimal as possible. > LayoutTests/svg/custom/use-referencing-indirectly-itself.svg:4 > + viewBox="0 0 480 360" xmlns="http://www.w3.org/2000/svg" > + xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:xe="http://www.w3.org/2001/xml-events"> Ditto. > LayoutTests/svg/custom/use-referencing-itself-expected.svg:3 > +<svg version="1.2" baseProfile="tiny" xml:id="svg-root" width="100%" height="100%" > + viewBox="0 0 480 360" xmlns="http://www.w3.org/2000/svg" Ditto. > LayoutTests/svg/custom/use-referencing-itself.svg:4 > + viewBox="0 0 480 360" xmlns="http://www.w3.org/2000/svg" > + xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:xe="http://www.w3.org/2001/xml-events"> Ditto. > Source/WebCore/platform/KURL.cpp:1391 > + if (a.m_queryEnd != b.m_queryEnd) { > + return false;} Please revert this. > Source/WebCore/svg/SVGURIReference.cpp:85 > + String id = ""; Do you need an empty string here? If so use String id = emptyString(); But I don't think that's need here, String id; should do it. > Source/WebCore/svg/SVGURIReference.cpp:106 > + return 0; // Non-existing external resource trailing period missing. > Source/WebCore/svg/SVGURIReference.h:47 > + if (!uri.startsWith("#")) { Reverse the logic here. if (uri,startsWith('#')) return false; > Source/WebCore/svg/SVGUseElement.cpp:231 > + if (isExternalURIReference(href(), document())) { I'd cache the result of isExternalURIREference in a boolean, to avoid calling it twice.

Created attachment 131655 [details] Proposed second part Done :)

Comment on attachment 131655 [details] Proposed second part View in context: https://bugs.webkit.org/attachment.cgi?id=131655&action=review Excellent work Reni! Thanks for your patience, it took a while :-) r=me with some last comments. There are some "new mode 100755" changes to page/ContentSecurityPolicy.h and platform/network/chromium/ResourceRequest.h that should be avoided. > LayoutTests/svg/custom/use-referencing-itself-expected.svg:4 > + xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:xe="http://www.w3.org/2001/xml-events"> version/baseProfile/xm:id/width/height/xmlns:xinkxmns:xe are useless.

Committed r110676: <http://trac.webkit.org/changeset/110676>

I'm sorry for bothering you... but it looks this makes chromium to crash. svg/custom/use-mutation-event-crash.svg ---- [11795:11795:17234733813326:ERROR:process_util_posix.cc(142)] Received signal 11 base::debug::StackTrace::StackTrace() [0x851842] base::(anonymous namespace)::StackDumpSignalHandler() [0x80b425] 0x7fccc9956af0 WebCore::SVGUseElement::externalDocument() [0x1d2f11c] WebCore::SVGUseElement::referencedDocument() [0x1d2f070] WebCore::SVGUseElement::buildPendingResource() [0x1d2fc67] WebCore::SVGUseElement::willRecalcStyle() [0x1d2f65e] WebCore::Element::recalcStyle() [0x6e16a7] WebCore::Element::recalcStyle() [0x6e1f50] ----

Created attachment 131824 [details] Follow-up patch We've discussed this on IRC - I'm adding a patch fixing these problems.

Comment on attachment 131824 [details] Follow-up patch r=me View in context: https://bugs.webkit.org/attachment.cgi?id=131824&action=review > Source/WebCore/ChangeLog:7 > + Assertions are firing due last minute changes, and some general problems. A little more explanation please.

Comment on attachment 131824 [details] Follow-up patch Landed with improved ChangeLog ok'ed by Zoltan on IRC in r110692. (Sorry I couldn't upload a patch that applies, as webkit-patch upload is not working, due my bad line to git.webkit.org)

Comment on attachment 131824 [details] Follow-up patch Landed with improved ChangeLog ok'ed by Zoltan on IRC in r110692. (Sorry I couldn't upload a patch that applies, as webkit-patch upload is not working, due my bad line to git.webkit.org)

Renis new tests in svg/dynamic-updates are flaky. They switch the xlink:href attribute to an external resource and call completeTest() afterwards (which calls layoutTestController.notifyDone()). Currently SVGUseElement doesn't support dispatching SVGLoad events at all, nor does it respect externalResourcesRequired. The code is already present in SVGScriptElement, it only needs refactoring. Then we can specify <use externalResourcesRequired="true" xlink:href="foo.svg" onload="alert('blub');>, and the alert() only fires after the external resource finished loading, or an SVGError event gets dispatched. This framework is needed otherwise we can't reliable test dynamic changes from internal to external resources. I will look into this.

(In reply to comment #145) > This framework is needed otherwise we can't reliable test dynamic changes from internal to external resources. I will look into this. All fixed in trunk, see bug 81109. externalResourcesRequired support is now enabled for <use>, and the flakiness is gone, by making the new tests depend on that. Closing this bug finally!

It works on WebKit nightly, but not on Chromium. Reading the patch it should work for Chromium as well, no?

#147 dirke, neither testcase nor example from #3 wfm Version 5.1.7 (6534.57.2, r122160) OS X 10.6.8 given the bug was 'fixed in trunk' some 3 months ago, you may begin to understand why frankly i stopped filing webkit-applebugs some time ago.

(In reply to comment #148) > given the bug was 'fixed in trunk' some 3 months ago, > you may begin to understand why frankly i stopped filing webkit-applebugs some time ago. I'm tired of this. nzimmermann ~/Downloads/extsvgref > cat prologo.css ... .nodeCons { fill: url(prologoDefs.svg#consGrad); } ... This is not about external use elements, it's about referencing paint servers from external documents, which we do NOT support at present. As external use references work in WebKit at present, the bug is fixed. External fill paint servers aren't fixed/supported, and are still not working.

#147, Nikolas I didn't morph the bug, the description, and the reduced test cases provided, do not fit your supposed morph. how is anyone else supposed to recognise the morph? where is the bug that fits the description of #12499, and these test cases?

(In reply to comment #149) > (In reply to comment #148) > As external use references work in WebKit at present, the bug is fixed. External fill paint servers aren't fixed/supported, and are still not working. It seems to be a Chromium only bug. I opened bug 91237.