[webkit-reviews] review granted: [Bug 222828] Update ApplicationCacheStorage::originsWithCache() to return a HashSet<SecurityOriginData> instead of a Vector<Ref<SecurityOrigin>> : [Attachment 422443] 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 222828: Update ApplicationCacheStorage::originsWithCache() to return a
HashSet<SecurityOriginData> instead of a Vector<Ref<SecurityOrigin>>
https://bugs.webkit.org/show_bug.cgi?id=222828

Attachment 422443: Patch

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




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

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

> Source/WebCore/ChangeLog:11
> +	   Update ApplicationCacheStorage::originsWithCache() to return a
HashSet<SecurityOriginData> instead of a Vector<Ref<SecurityOrigin>>.
> +	   Using a HashSet makes sure we do not return duplicates (which do
occur as per ApplicationCacheStorage implementation) and makes typing
> +	   more consistent with other similar storage functions. Using
SecurityOriginData is more lightweight and sufficient for our use cases.
> +	   It also takes care of a FIXME comment in
WebsiteDataStore::fetchDataAndApply about switching to SecurityOriginData.

The downside of switching to HashSet is that any algorithms that involve
iterating through the origins now iterate in an unpredictable order. As long as
that’s not an issue, this will be great.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:536
> +	       for (auto& origin : storage->originsWithCache()) {

This is a situation where we might want to consider some kind of sorting so we
have predictable behavior. I worry about creating a vector that preserves the
randomness of hash table order and doing anything with that vector.

In the early days of Safari some of our most difficult to diagnose performance
and correctness bugs were hard to reproduce because they depended on hash
ordering. Although the biggest one had a property that this does not, which is
that the hash was almost global (I think it was used by NSNotificationCenter)
so seemingly unrelated code to add something was having an effect on something
seemingly unrelated.