[webkit-reviews] review granted: [Bug 191898] Modernize SVGURIReference::targetElementFromIRIString : [Attachment 355444] Cleanup

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 191898: Modernize SVGURIReference::targetElementFromIRIString
https://bugs.webkit.org/show_bug.cgi?id=191898

Attachment 355444: Cleanup

https://bugs.webkit.org/attachment.cgi?id=355444&action=review




--- Comment #2 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 355444
  --> https://bugs.webkit.org/attachment.cgi?id=355444
Cleanup

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

r=me

> Source/WebCore/svg/SVGURIReference.cpp:102
> +    URL url = document.completeURL(iri);

auto?

> Source/WebCore/svg/SVGURIReference.cpp:106
> +	   return { externalDocument->getElementById(id), id };

We can make this slightly more optimal and remove a ref() by WTFMove()ing id.

> Source/WebCore/svg/SVGURIReference.cpp:111
> +	   return { nullptr, id };

Ditto.

> Source/WebCore/svg/SVGURIReference.cpp:113
> +    return { document.getElementById(id), id };

Ditto.

> Source/WebCore/svg/SVGUseElement.cpp:420
> +	   if (!targetID->isNull() && isExternalURIReference(original.href(),
original.document()))

How did you come to the decision to move the null string check here? Currently
this section of the code has one branch (targetID && targetID->isNull()). Now
it has two. How often is targetID a null string in practice? If not often then
would bad things happen if we didn't check for a null string? If not, have you
considered removing the null string check and just making this code slower for
the null string case?

> Source/WebCore/svg/SVGUseElement.cpp:421
>	       *targetID = String();

For you consideration, I would modernize this line to use uniform initializer
syntax:

*targetID = String { };

> Source/WebCore/svg/graphics/filters/SVGFEImage.cpp:84
>	   return 0;

For your consideration I suggest modernizing this line and line 87 to return
nullptr instead of 0 since you have effectively rewritten every line in this
function.