Created attachment 355444 [details] Cleanup

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.

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

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

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.

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

Committed r238452: <https://trac.webkit.org/changeset/238452>

<rdar://problem/46215764>

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

(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

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

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