[webkit-reviews] review granted: [Bug 236852] Clean up / optimize even more call sites constructing vectors : [Attachment 452870] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 236852: Clean up / optimize even more call sites constructing vectors
https://bugs.webkit.org/show_bug.cgi?id=236852

Attachment 452870: Patch

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




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

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

Hope I’m just not just writing the same comments again. I can’t remember what
is new here.

> Source/WTF/wtf/cf/LanguageCF.cpp:109
>      Vector<String> languages;
> +    languages.reserveInitialCapacity(platformLanguagesCount);
>      for (CFIndex i = 0; i < platformLanguagesCount; i++) {
>	   auto platformLanguage =
static_cast<CFStringRef>(CFArrayGetValueAtIndex(platformLanguages.get(), i));
> -	   languages.append(httpStyleLanguageCode(platformLanguage,
shouldMinimizeLanguages));
> +	   languages.uncheckedAppend(httpStyleLanguageCode(platformLanguage,
shouldMinimizeLanguages));
>      }

Makes me think we should have a makeVector for CF in a VectorCF.h that is like
VectorCocoa.h, because this would be nicer:

    auto languages = makeVector(platformLanguages.get(),
[shouldMinimizeLanguages](CFTypeRef language) {
	return httpStyleLanguageCode(checked_cf_cast<CFStringRef>(language),
shouldMinimizeLanguages);
    });

> Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:105
> +	   result.addressLines = Vector { String { street } };

I don’t think we need Vector here.

> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:182
> +	   // It is valid for javascript to pass in a list of object store
names with the same name listed twice,
> +	   // so we need to put them all in a set to get a unique list.
> +	   HashSet<String> objectStoreSet;
> +	   objectStoreSet.add(objectStores.begin(), objectStores.end());
> +	   objectStores = copyToVector(objectStoreSet);

I made some of these comments already, but four comments on this moved code:

1) To actually get the optimization pretty sure we need to call WTFMove on
storeNames rather than on the result of std::get.
2) JavaScript rather than javascript.
3) Why doesn’t HashSet have a constructor that takes a pair of iterators?
4) Building a set is not a great way to remove duplicates. A ListHashSet would
be better because it preserves ordering rather than accidentally scrambling
ordering using hashing resulting in a pseudo-random ordering. An algorithm
using std::sort and std::unique would be even better, significantly more
efficient, and yields a sorted results rather than a pseudo-random one.
5) Even if we don’t change the algorithm as suggested in (4), the operation of
"remove duplicates from a Vector<String>" seems like it should be broken out
into a separate function; maybe one that takes an rvalue reference to a vector
and return a vector.

    objectStores =
removeDuplicates(std::get<Vector<String>>(WTFMove(storeNames)));
    objectStores =
sortAndRemoveDuplicates(std::get<Vector<String>>(WTFMove(storeNames)));
    objectStores = sortUnique(std::get<Vector<String>>(WTFMove(storeNames)));

Note that it seems obvious once you have to name the function that "remove
duplicates and scramble the order" is not a good semantic.

> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:184
>	   objectStores.append(WTFMove(std::get<String>(storeNames)));

Calling append is not a great way to create a single-element vector. Maybe
this:

    objectStores = { std::get<String>(WTFMove(storeNames)) };

> Source/WebCore/Modules/indexeddb/IDBKeyData.h:168
> +    add(hasher, keyData.isNull());

Is the other data to hash when the data is null? Maybe we need an if statement
or early return?

> Source/WebCore/Modules/mediasource/SourceBufferList.h:63
>      Vector<RefPtr<SourceBuffer>>::iterator begin() { return m_list.begin();
}
>      Vector<RefPtr<SourceBuffer>>::iterator end() { return m_list.end(); }
> +    Vector<RefPtr<SourceBuffer>>::const_iterator begin() const { return
m_list.begin(); }
> +    Vector<RefPtr<SourceBuffer>>::const_iterator end() const { return
m_list.end(); }

I think we can use "auto" for these return types. And many other return types,
but these seem to particularly benefit from it.

> Source/WebCore/Modules/speech/cocoa/WebSpeechRecognizerTask.mm:132
> +    SpeechRecognitionResultData data { WTFMove(alternatives), !!isFinal };
> +    _delegateCallback(SpeechRecognitionUpdate::createResult(_identifier, {
WTFMove(data) }));

Might read better as a one liner since we would need one less WTFMove.

> Source/WebCore/Modules/speech/cocoa/WebSpeechRecognizerTaskMock.mm:68
> +    SpeechRecognitionAlternativeData alternative { "Test", 1.0 };
> +    SpeechRecognitionResultData data { { WTFMove(alternative) }, true };
> +    _delegateCallback(SpeechRecognitionUpdate::createResult(_identifier, {
WTFMove(data) }));

Similar thought.

> Source/WebCore/dom/DOMStringList.h:64
> +    { }

I usually format these braces vertically, when the part above is formatted
vertically. I suppose we should choose one style or the other for the future
here.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:214
> +	   records.reserveCapacity(records.size() + value.size());
>	   for (auto& record : value)
> -	       records.append(record);
> +	       records.uncheckedAppend(record);

I thought we discussed not making this change.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:688
> +	   keys.reserveCapacity(keys.size() + records.size());
>	   for (auto& record : records)
> -	       keys.append(record.key);
> +	       keys.uncheckedAppend(record.key);

Ditto.

> Source/WebKit/NetworkProcess/storage/SQLiteStorageArea.cpp:74
> +   
m_cachedStatements.reserveInitialCapacity(static_cast<size_t>(StatementType::In
valid));
>      for (size_t i = 0; i < static_cast<size_t>(StatementType::Invalid); ++i)
> -	   m_cachedStatements.append(nullptr);
> +	   m_cachedStatements.uncheckedAppend(nullptr);

Should be:

    m_cachedStatements =
Vector<std::unique_ptr<WebCore::SQLiteStatement>>(static_cast<size_t>(Statement
Type::Invalid), nullptr);

Or something roughly like that. No loop.

> Source/WebKit/Shared/ApplePay/WebPaymentCoordinatorProxy.cpp:97
> +	   return URL(URL(), linkIconURLString);

We really have to fix the URL class so this super-common operation can be
written in a less bizarre way!