Bug 191898 - Modernize SVGURIReference::targetElementFromIRIString
Summary: Modernize SVGURIReference::targetElementFromIRIString
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 174977
  Show dependency treegraph
 
Reported: 2018-11-21 16:24 PST by Ryosuke Niwa
Modified: 2018-11-23 21:05 PST (History)
9 users (show)

See Also:


Attachments
Cleanup (24.03 KB, patch)
2018-11-21 16:43 PST, Ryosuke Niwa
dbates: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.58 MB, application/zip)
2018-11-22 08:30 PST, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2018-11-21 16:24:13 PST
Simplify & modernize SVGURIReference::targetElementFromIRIString. e.g. stop taking raw pointers.
Comment 1 Ryosuke Niwa 2018-11-21 16:43:24 PST
Created attachment 355444 [details]
Cleanup
Comment 2 Daniel Bates 2018-11-21 23:00:11 PST
Comment on attachment 355444 [details]
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.
Comment 3 Daniel Bates 2018-11-21 23:04:16 PST
(In reply to Daniel Bates from comment #2)
> > 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.

I meant to say,

We can make this slightly more optimal and remove a ref() by WTFMove()ing id into the second argument. Obviously, we can't move it into the getElementById().
Comment 4 EWS Watchlist 2018-11-22 08:30:04 PST
Comment on attachment 355444 [details]
Cleanup

Attachment 355444 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10109974

New failing tests:
media/no-fullscreen-when-hidden.html
Comment 5 EWS Watchlist 2018-11-22 08:30:06 PST
Created attachment 355476 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 6 Ryosuke Niwa 2018-11-22 15:23:22 PST
Thanks for the review!

(In reply to Daniel Bates from comment #2)
> Comment on attachment 355444 [details]
> 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?

Fixed.

> > 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.

Done.

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

Ditto.

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

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?

That's a good point. We don't really need a null check here. The first thing isExternalURIReference does is:
if (uri.startsWith('#'))
    return false;
so that should bail out immediately if uri given is a null string.

> > Source/WebCore/svg/SVGUseElement.cpp:421
> >              *targetID = String();
> 
> For you consideration, I would modernize this line to use uniform
> initializer syntax:
> 
> *targetID = String { };

Fixed.

> > 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.

Good catch. Done that.
Comment 7 Ryosuke Niwa 2018-11-22 15:26:41 PST
(In reply to Build Bot from comment #4)
> Comment on attachment 355444 [details]
> Cleanup
> 
> Attachment 355444 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: https://webkit-queues.webkit.org/results/10109974
> 
> New failing tests:
> media/no-fullscreen-when-hidden.html

I'm pretty sure this failure is nothing to do with my patch since there is no SVG involved in the test.
Comment 8 Ryosuke Niwa 2018-11-22 15:47:35 PST
Committed r238452: <https://trac.webkit.org/changeset/238452>
Comment 9 Radar WebKit Bug Importer 2018-11-22 15:48:29 PST
<rdar://problem/46215764>
Comment 10 Darin Adler 2018-11-23 10:03:15 PST
Comment on attachment 355444 [details]
Cleanup

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

>>> 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.
> 
> Done.

Is order of execution guaranteed? I am worried about using a variable and moving the value of of that variable in the same argument list. I know this is undefined behavior when calling a function; typical we have to do the operation (getElementById in this case) in a separate statement and put the result into a local variable to avoid the problem (meaning we end up calling move twice).
Comment 11 Ryosuke Niwa 2018-11-23 20:16:27 PST
(In reply to Darin Adler from comment #10)
> Comment on attachment 355444 [details]
> Cleanup
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355444&action=review
> 
> >>> 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.
> > 
> > Done.
> 
> Is order of execution guaranteed? I am worried about using a variable and
> moving the value of of that variable in the same argument list. I know this
> is undefined behavior when calling a function; typical we have to do the
> operation (getElementById in this case) in a separate statement and put the
> result into a local variable to avoid the problem (meaning we end up calling
> move twice).

At least the order of initialization list is deterministic: https://stackoverflow.com/questions/4037219/order-of-execution-in-constructor-initialization-list
Comment 12 Daniel Bates 2018-11-23 20:51:01 PST
(In reply to Darin Adler from comment #10)
> Comment on attachment 355444 [details]
> Cleanup
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355444&action=review
> 
> >>> 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.
> > 
> > Done.
> 
> Is order of execution guaranteed? I am worried about using a variable and
> moving the value of of that variable in the same argument list. I know this
> is undefined behavior when calling a function; typical we have to do the
> operation (getElementById in this case) in a separate statement and put the
> result into a local variable to avoid the problem (meaning we end up calling
> move twice).

Since you are bringing this up we may want to reconsider how we write this code. To answer your question, yes, the order of evalauation is guaranteed here because this is aggregate initialization as TargetElementResult satisfies the criterion for it. A property of aggregate initialization is that order of initialization is well-defined. See the first bullet on <https://en.cppreference.com/w/cpp/language/aggregate_initialization>, repeated here for convenience: "Each direct public base, (since C++17) array element, or non-static class member, in order of array subscript/appearance in the class definition, is copy-initialized from the corresponding clause of the initializer list." And move construction is a form of copy-initialization.
Comment 13 Daniel Bates 2018-11-23 21:05:42 PST
(In reply to Daniel Bates from comment #12)
> (In reply to Darin Adler from comment #10)
> > Comment on attachment 355444 [details]
> > Cleanup
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=355444&action=review
> > 
> > >>> 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.
> > > 
> > > Done.
> > 
> > Is order of execution guaranteed? I am worried about using a variable and
> > moving the value of of that variable in the same argument list. I know this
> > is undefined behavior when calling a function; typical we have to do the
> > operation (getElementById in this case) in a separate statement and put the
> > result into a local variable to avoid the problem (meaning we end up calling
> > move twice).
> 
> Since you are bringing this up we may want to reconsider how we write this
> code.

This sentence sounds negative. I didn't mean to come across like that. What I meant to say was: Your concern about this code is likely shared by other people. We should strive for code that is easy to read and is unambiguous. I will leave it to the author, Ryosuke, (or anyone else) to determine whether or not to change the code or leave it as-is.