[webkit-reviews] review granted: [Bug 170925] Using StringView.split() instead of String.split() in some places : [Attachment 308626] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Daniel Bates <dbates at webkit.org>'s
request for review:
Bug 170925: Using StringView.split() instead of String.split() in some places
https://bugs.webkit.org/show_bug.cgi?id=170925

Attachment 308626: Patch

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




--- Comment #18 from Darin Adler <darin at apple.com> ---
Comment on attachment 308626
  --> https://bugs.webkit.org/attachment.cgi?id=308626
Patch

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

> Source/WTF/wtf/HashMap.h:72
> +    using IdentityTranslatorType = typename
HashTableType::IdentityTranslatorType;

Seems fine to have this. Not 100% sure it should be public; would probably be
fine to have it private.

> Source/WebCore/accessibility/AccessibilityObject.cpp:2131
> +// FIXME: Find a way to incorporate this functionality into
ASCIICaseInsensitiveHash and allow
> +// a HashMap whose keys are type String to perform operations when given a
key of type StringView.
> +struct ASCIICaseInsensitiveStringViewHashTranslator {
> +    static unsigned hash(StringView key)
> +    {
> +	   if (key.is8Bit())
> +	       return ASCIICaseInsensitiveHash::hash(key.characters8(),
key.length());
> +	   return ASCIICaseInsensitiveHash::hash(key.characters16(),
key.length());
> +    }
> +
> +    static bool equal(const String& a, StringView b)
> +    {
> +	   return equalIgnoringASCIICaseCommon(a, b);
> +    }
> +};

That comment seems fine, but it’s unclear why even in its current form this is
here and not in the the header with ASCIICaseSensitiveHash.

> Source/WebCore/dom/TreeScope.cpp:122
> +    if (RefPtr<AtomicStringImpl> atomicElementId =
elementId.toExistingAtomicString())

I would use auto here instead of writing out RefPtr.

> Source/WebCore/html/parser/XSSAuditor.cpp:242
> +static bool semicolonSeparatedValueContainsJavaScriptURL(const String&
semicolonSeparatedValue)

Should change argument type to StringView.

The one call site for this function calls stripLeadingAndTrailingHTMLSpaces
before calling this function. This is peculiar because it means that leading
HTML spaces on the entire value are ignored, but not spaces after semicolons. I
am not sure that is actually the desired behavior. Also, stripping trailing
spaces is pointless; such spaces will be ignored anyway. We might want to do
that stripping of leading spaces on each substring before calling
protocolIsJavaScript. Note that stripLeadingAndTrailingHTMLSpaces or just
stripping leading spaces are natural operations to support on a StringView
rather than a String when the result is going to inspected rather than stored.
There is no need to allocate a new String for the substring computed after
trimming off possible spaces. Generally speaking StringView is excellent for
substring operations like this one as long as the substring is not going to be
turned back into a String for storage. Worth coming back to and tidying up
later.