[webkit-reviews] review requested: [Bug 12499] External <use> xlink:href references do not work : [Attachment 129471] Proposed second part

[bugzilla-daemon at webkit.org]

Renata Hodovan <reni at webkit.org> has asked  for review:
Bug 12499: External <use> xlink:href references do not work
https://bugs.webkit.org/show_bug.cgi?id=12499

Attachment 129471: Proposed second part
https://bugs.webkit.org/attachment.cgi?id=129471&action=review

------- Additional Comments from Renata Hodovan <reni at webkit.org>
(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())->markShadowTreeForRe
creation();
> > +	 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.