Bug 215382

Created attachment 406390 [details]
Patch

Comment on attachment 406390 [details]
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, { }))
    ...

Comment on attachment 406390 [details]
Patch

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

> Source/WebCore/page/Frame.cpp:690
> +    for (const auto& pair : m_userScriptsAwaitingNotification)

You can use structured bindings to make this a bit nicer:

for (const auto& [world, script] : m_userScriptsAwaitingNotification)

Comment on attachment 406390 [details]
Patch

Eek, sorry, did not meant to change the review flag state.

Created attachment 406402 [details]
Patch

Comment on attachment 406390 [details]
Patch

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

>> Source/WebCore/page/Frame.cpp:690
>> +    for (const auto& pair : m_userScriptsAwaitingNotification)
> 
> You can use structured bindings to make this a bit nicer:
> 
> for (const auto& [world, script] : m_userScriptsAwaitingNotification)

Cool. Did this.

>> Source/WebCore/page/Frame.cpp:694
>> +}
> 
> 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, { }))
>     ...

Done.

Committed r265510: <https://trac.webkit.org/changeset/265510>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406402 [details].