[webkit-reviews] review granted: [Bug 170925] Using StringView.split() instead of String.split() in some places : [Attachment 307434] 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 307434: Patch

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




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

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

> Source/WTF/wtf/HashMap.h:301
> +    KeyValuePairType* entry = const_cast<HashTableType&>(m_impl).template
lookup<HashMapTranslatorAdapter<KeyValuePairTraits, HashTranslator>>(value);

Seems like this should be auto or auto* instead of KeyValuePairType*.

> Source/WTF/wtf/HashMap.h:406
> +    return get<typename HashTableType::IdentityTranslatorType>(key);

Why doesn’t this use IdentityTranslatorType instead of typename
HashTableType::IdentityTranslatorType?

> Source/WebCore/accessibility/AccessibilityObject.cpp:2242
> +	   if (AccessibilityRole role = ariaRoleMap().get<typename
ARIARoleMap::IdentityTranslatorType>(roleName)) // Identity translator supports
StringView; get() does not.

That is so peculiar. Even with the comment it’s so non-obvious that an
"identity" translator would allow things that the get function would not allow
itself. The comment just makes me wonder why! I wonder if there is some more
elegant way to handle this. Or some clearer way to explain why explicit use of
the identity translator makes this work.

> Source/WebCore/accessibility/AccessibilityObject.cpp:3080
> +	   if (Element* idElement =
treeScope.getElementById(idName.toAtomicString()))

Could consider optimizing this by using toExistingAtomicString; if that
function exists and we would also have to make sure null string is handled
properly.

Could also use auto* here instead of Element*.

> Source/WebCore/platform/URL.cpp:1149
> +    return protocolIs(url.toStringWithoutCopying(), "javascript");

This is an inefficient algorithm, allocating memory every time. We can do this
much more efficiently by either copying and pasting the body of the
protocolIs(const String&, const char*) function or using templates to make a
StringView version.

Also there is a very strange FIXME above protocolIs(const String&, const char*)
that says: "Why is this different than protocolIs(StringView, const char*)?'
That seems to imply that there was a protocolIs function that took a StringView
at one time. Perhaps it was deleted? The FIXME should go.

>
Source/WebCore/platform/graphics/avfoundation/CDMPrivateMediaSourceAVFObjC.mm:1
07
> +    StringView keySystem = m_cdm->keySystem();

This looks unsafe. What type does m_cdm->keySystem() return? If it returns
String then this is unsafe. If it returns const String& then this is safe. I
worry that someone will change it from returning const String& to returning
String and then make this code invalid. Hard to write this so it’s obviously
safe. Think of StringView like a raw pointer. There’s always the question of
why the StringView is still good after you initialize it from the result of an
expression that might involve a temporary String.

Generally we should avoid local variables of type StringView that are
initialized from a temporary String or look like they might be. Arguments of
type StringView don’t have this problem, many cases of local variable
StringView that are not initialized from temporary Strings are fine, and
various other idioms are OK.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:1476
> +	   auto segments =
StringView(m_fontFaceElement->attributeWithoutSynchronization(SVGNames::font_we
ightAttr)).split(' ');
> +	   for (auto segment : segments) {

I would have written this;

    auto& weight =
m_fontFaceElement->attributeWithoutSynchronization(SVGNames::font_weightAttr);
    for (auto segment: StringView(weight).split(' ')) {

Makes the object lifetimes more obviously correct. Otherwise, you have the
puzzle of why it’s OK for the StringView to outlive the result of
attributeWithoutSynchronization.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:1489
> +	   segments =
StringView(m_fontFaceElement->attributeWithoutSynchronization(SVGNames::font_st
yleAttr)).split(' ');
> +	   for (auto segment : segments) {

Same issue here.

> Source/WebCore/testing/Internals.cpp:3509
> +void Internals::setPageMuted(const String& statesString)

I think you can just change the argument type to StringView instead.