Bug 81109 - Make SVGUseElement respect & support externalResourcesRequired
Summary: Make SVGUseElement respect & support externalResourcesRequired
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks: 12499
  Show dependency treegraph
 
Reported: 2012-03-14 08:01 PDT by Nikolas Zimmermann
Modified: 2012-03-15 03:22 PDT (History)
3 users (show)

See Also:


Attachments
Patch (114.06 KB, patch)
2012-03-14 08:23 PDT, Nikolas Zimmermann
rwlbuis: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2012-03-14 08:01:40 PDT
Make SVGUseElement respect & support externalResourcesRequired.
Without externalResourcesRequired support there's no way to delay SVGLoad events until the external resource is actually needed. This currently makes 4 svg/dynamic-updates/SVGUseElement* tests flaky, that reni introduced in bug 12499.

I'll have a fix for this, generalizing the existing support for that in SVGScriptElement.
Comment 1 Nikolas Zimmermann 2012-03-14 08:23:43 PDT
Created attachment 131855 [details]
Patch
Comment 2 Rob Buis 2012-03-14 09:06:17 PDT
Comment on attachment 131855 [details]
Patch

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

Looks great, please fix my concerns before landing.

> Source/WebCore/svg/SVGExternalResourcesRequired.cpp:79
> +        // If we've already fired an load event and externalResourcesRequired is set to 'true'

fire *a* load

> Source/WebCore/svg/SVGExternalResourcesRequired.cpp:85
> +    // HTML and SVG differ completly in the 'onload' event handling of <script> elements.

Typo: completely.

> Source/WebCore/svg/SVGExternalResourcesRequired.cpp:86
> +    // HTML fires the 'load' event after it sucessfully loaded a remote resource, otherwhise an error event.

Typo: otherwise.

> Source/WebCore/svg/SVGExternalResourcesRequired.cpp:88
> +    // is set to 'false', otherwhise it dispatches the 'SVGLoad' event just after loading the remote resource.

Ditto.

> Source/WebCore/svg/SVGExternalResourcesRequired.cpp:89
> +    if (!externalResourcesRequired)

The externalResourcesRequired logic does not make sense for isParserInserted = false and externalResourcesRequired=true, as discussed on irc.

> Source/WebCore/svg/SVGExternalResourcesRequired.cpp:106
> +    // Eventually send SVGLoad event now for the dynamically inserted script element

Lacks period.
Comment 3 WebKit Review Bot 2012-03-14 09:43:41 PDT
Comment on attachment 131855 [details]
Patch

Attachment 131855 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11957084

New failing tests:
svg/dynamic-updates/SVGUseElement-dom-href2-attr.html
svg/dynamic-updates/SVGUseElement-svgdom-href2-prop.html
Comment 4 Nikolas Zimmermann 2012-03-15 03:22:35 PDT
(In reply to comment #3)
> svg/dynamic-updates/SVGUseElement-dom-href2-attr.html
> svg/dynamic-updates/SVGUseElement-svgdom-href2-prop.html

Oh I thought those were still listed as failing in the expectations, someone already rebaselined them. Now they need another rebaseline. I'll talk to the gardener.

Closing this bug, as the patch landed in r110711.