[webkit-reviews] review granted: [Bug 236985] Avoid having to iterate the whole frame tree(s) every time we need to dispatch storage events : [Attachment 452748] 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 236985: Avoid having to iterate the whole frame tree(s) every time we need
to dispatch storage events
https://bugs.webkit.org/show_bug.cgi?id=236985

Attachment 452748: Patch

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




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

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

> Source/WebCore/page/DOMWindow.cpp:168
> +    for (auto& window : windowsInterestedInStorageEvents())
> +	   apply(window);

Maybe the use of Vector<Ref<DOMWindow> should be inside this function rather
than in the functions that call it. I suppose that’s less efficient since we
will create an extra vector, but it seems dangerous to have a function that’s
so easy to use unsafely.

> Source/WebCore/page/DOMWindow.cpp:458
> +    windowsInterestedInStorageEvents().remove(*this);

How important is it to do this explicitly in a WeakHashSet? If we have this
code, it seems to make it safe to use HashSet instead.

> Source/WebCore/storage/StorageEventDispatcher.cpp:43
> +static void dispatchSessionStorageEventsToWindows(Page& page, const
Vector<Ref<DOMWindow>>& windows, const String& key, const String& oldValue,
const String& newValue, const String& url, const SecurityOrigin&
securityOrigin)

Can we factor these two functions so they share almost all the code? There are
only two differences here, StorageType and what function we use to get the
storage object.