[webkit-reviews] review granted: [Bug 215382] Deferred WKUserScripts are exponentially injected on preloaded pages with frames : [Attachment 406390] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Timothy Hatcher
<timothy at apple.com>'s request for review:
Bug 215382: Deferred WKUserScripts are exponentially injected on preloaded
pages with frames
https://bugs.webkit.org/show_bug.cgi?id=215382

Attachment 406390: Patch

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




--- Comment #2 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 406390
  --> https://bugs.webkit.org/attachment.cgi?id=406390
Patch

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

r=me

> Source/WebCore/page/Frame.cpp:694
> +void Frame::injectUserScriptsAwaitingNotification()
> +{
> +    for (const auto& pair : m_userScriptsAwaitingNotification)
> +	   injectUserScriptImmediately(pair.first, pair.second.get());
> +
> +    m_userScriptsAwaitingNotification.clear();
> +}

I prefer the idiom where you take() or WTFMove() or exchange() before entering
the loop. This means the data member is always coherent, even during the loop.
One reason this might matter is re-entrancy. But even if we think re-entrancy
won't happen, I think the idiom is a bit clearer.

In this case, you even get to delete a line of code:

for (const auto& pair : std::exchange(m_userScriptsAwaitingNotification, { }))
    ...