[webkit-reviews] review granted: [Bug 81109] Make SVGUseElement respect & support externalResourcesRequired : [Attachment 131855] Patch

[bugzilla-daemon at webkit.org]

Rob Buis <rwlbuis at gmail.com> has granted Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 81109: Make SVGUseElement respect & support externalResourcesRequired
https://bugs.webkit.org/show_bug.cgi?id=81109

Attachment 131855: Patch
https://bugs.webkit.org/attachment.cgi?id=131855&action=review

------- Additional Comments from Rob Buis <rwlbuis at gmail.com>
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.