Bug 141382 – SVGUseElement follow-up improvements

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Darin Adler]

SVGUseElement follow-up improvements

[Darin Adler]

Created attachment 246261 [details]
Patch

[Darin Adler]

Created attachment 246272 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 246272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=246272&action=review

> Source/WebCore/svg/SVGElement.cpp:968
> +            return title;

I do not know any use for this code.  My understanding is the title() function is used mainly for hit testing and the UI tooltip text. The hit testing never hits an instance element inside an SVG.  This means for the tooltip text, we never call to this function with an instance element. We always walk the tree up starting from the use element till we find a non-empty title or we reach the the root of the svg. I do not think also we can hit this function with an instance element though DOM. There is nothing in the SVGElement which can return its UI title. 

But if there a way, which I am not aware of, to can call this function with an instance element, I think we should return the useElement->title() always regardless whether it is empty or not.  We should not return the title of the instance element ever.

> Source/WebCore/svg/SVGUseElement.cpp:402
>      }

Do not we need if-else here? If correspondingElement is not null then we need to search the original tree and we do not need to search for cycles in the instance tree.  In other words do we need to search for cycles in the instance tree?

> Source/WebCore/svg/SVGUseElement.cpp:519
> +    invalidateShadowTree();

Why do we need to move this statement here? My understanding is this function gets called when a <use> element is referencing an element in an external document. When the external document finishes loading, the <use> element gets notified by this function so it can build its shadow tree.  If the <use> element is removed from the document before it the external document finishes loading, we might need to do invalidateShadowTree() in this case since the element is going to be deleted anyway.

[Darin Adler]

Comment on attachment 246272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=246272&action=review

>> Source/WebCore/svg/SVGElement.cpp:968
>> +            return title;
> 
> I do not know any use for this code.  My understanding is the title() function is used mainly for hit testing and the UI tooltip text. The hit testing never hits an instance element inside an SVG.  This means for the tooltip text, we never call to this function with an instance element. We always walk the tree up starting from the use element till we find a non-empty title or we reach the the root of the svg. I do not think also we can hit this function with an instance element though DOM. There is nothing in the SVGElement which can return its UI title. 
> 
> But if there a way, which I am not aware of, to can call this function with an instance element, I think we should return the useElement->title() always regardless whether it is empty or not.  We should not return the title of the instance element ever.

I see your point. As you say, we won’t hit this through the DOM. If someone has a pointer to the instance and uses it to call title, they also could have called lots of other functions on that element, such as getAttribute or className. I don’t see what’s special about title. Normally what we do is to prevent them from having a pointer to the instance. I think all of this points to the fact that we should just remove this code.

>> Source/WebCore/svg/SVGUseElement.cpp:402
>>      }
> 
> Do not we need if-else here? If correspondingElement is not null then we need to search the original tree and we do not need to search for cycles in the instance tree.  In other words do we need to search for cycles in the instance tree?

I think you are saying we only need the second loop. I am having trouble thinking this through.

>> Source/WebCore/svg/SVGUseElement.cpp:519
>> +    invalidateShadowTree();
> 
> Why do we need to move this statement here? My understanding is this function gets called when a <use> element is referencing an element in an external document. When the external document finishes loading, the <use> element gets notified by this function so it can build its shadow tree.  If the <use> element is removed from the document before it the external document finishes loading, we might need to do invalidateShadowTree() in this case since the element is going to be deleted anyway.

We don’t really need to move it; but it also does no harm. I like it slightly better before the check, because the invalidation is always a good/safe idea and will do no harm if not needed. But I don’t think notifyFinished can ever be called when inDocument() will return false, because we remove ourselves as a client from the cached resource whenever the element is removed from the document.

[Darin Adler]

Comment on attachment 246272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=246272&action=review

>>> Source/WebCore/svg/SVGUseElement.cpp:402
>>>      }
>> 
>> Do not we need if-else here? If correspondingElement is not null then we need to search the original tree and we do not need to search for cycles in the instance tree.  In other words do we need to search for cycles in the instance tree?
> 
> I think you are saying we only need the second loop. I am having trouble thinking this through.

OK, I think I figured it out. The algorithm for preventing cycles is to check whether the target is something we already cloned to make one of our parents. There’s no need to prevent any other kind of cycle; they’ll all end up trying to clone the same target twice and that’s all we need to check for. Accordingly we only need the first loop, not the second one.

[Darin Adler]

Created attachment 246326 [details]
Patch

[Darin Adler]

New patch incorporates all the feedback from Said.

[Darin Adler]

Need to find a reviewer.

[Antti Koivisto]

Comment on attachment 246326 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=246326&action=review

> Source/WebCore/svg/SVGUseElement.cpp:102
>      else if (name == SVGNames::heightAttr)
>          setHeightBaseValue(SVGLength::construct(LengthModeHeight, value, parseError, ForbidNegativeLengths));
> -    else if (SVGLangSpace::parseAttribute(name, value)
> -             || SVGExternalResourcesRequired::parseAttribute(name, value)
> -             || SVGURIReference::parseAttribute(name, value)) {
> -    } else
> -        ASSERT_NOT_REACHED();
>  
>      reportAttributeParsingError(parseError, name, value);
> -}
>  
> -#if !ASSERT_DISABLED
> -static inline bool isWellFormedDocument(Document& document)
> -{
> -    if (document.isSVGDocument() || document.isXHTMLDocument())
> -        return static_cast<XMLDocumentParser*>(document.parser())->wellFormed();
> -    return true;
> +    SVGExternalResourcesRequired::parseAttribute(name, value);
> +    SVGGraphicsElement::parseAttribute(name, value);
> +    SVGLangSpace::parseAttribute(name, value);
> +    SVGURIReference::parseAttribute(name, value);

Looks like this could bail out when someone handles the attribute. Maybe not a worthwhile optimization.

> Source/WebCore/svg/SVGUseElement.cpp:376
> +    SVGElement* correspondingElement = this->correspondingElement();
> +    const SVGUseElement& original = correspondingElement ? downcast<SVGUseElement>(*correspondingElement) : *this;
> +
> +    Element* targetCandidate = targetElementFromIRIString(original.href(), original.document(), targetID, original.externalDocument());

I would use auto in all these.

> Source/WebCore/svg/SVGUseElement.cpp:387
> +    SVGElement& target = downcast<SVGElement>(*targetCandidate);

Here too.

> Source/WebCore/svg/SVGUseElement.cpp:-501
> -    if (cachedDocumentIsStillLoading())
> -        return;

Any idea why this check was here?

[Darin Adler]

Comment on attachment 246326 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=246326&action=review

>> Source/WebCore/svg/SVGUseElement.cpp:102
>> +    SVGURIReference::parseAttribute(name, value);
> 
> Looks like this could bail out when someone handles the attribute. Maybe not a worthwhile optimization.

This is a salient topic for all the SVG element code. There’s a non-trivial amount of code to implement “bail out when someone handles an attribute” including HashSets and such. I’m not sure we ever need to do that optimization. If the performance cost of more atomic string comparisons and function call overhead was really significant, then I’d think we’d want to make a bigger change, not just these early exits.

>> Source/WebCore/svg/SVGUseElement.cpp:376
>> +    Element* targetCandidate = targetElementFromIRIString(original.href(), original.document(), targetID, original.externalDocument());
> 
> I would use auto in all these.

OK, I’ll do some more.

>> Source/WebCore/svg/SVGUseElement.cpp:-501
>> -        return;
> 
> Any idea why this check was here?

Yes, I do understand it. The old code went out of its way to not build a shadow tree at all when any document was loading. But there’s no reason for that. We can build the partial tree at first, then rebuild the whole tree once the loading is done. I don’t think there’s a “flash of unsettled content” issue and the extra code to handle the loading state specially doesn’t seem to provide any significant benefit.

[Darin Adler]

Committed r179980: <http://trac.webkit.org/changeset/179980>

[Alex Christensen]

Something about those braces causes Visual Studio debug builds to have internal errors like this one:

1>c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\svg\svgsvgelement.cpp(540): fatal error C1001: An internal error has occurred in the compiler.

https://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/85500/steps/compile-webkit/logs/stdio

Fix committed to http://trac.webkit.org/changeset/179991