Bug 138015 – [WK2] Use C++11 lambdas instead of helper methods in StorageManager

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: New Bugs

[Zan Dobersek]

[WK2] Remove uses of WTF::bind() in StorageManager

[Zan Dobersek]

Created attachment 240362 [details]
Patch

[Darin Adler]

Comment on attachment 240362 [details]
Patch

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

> Source/WebKit2/UIProcess/Storage/StorageManager.cpp:392
> +    RefPtr<StorageManager> protector(this);
> +    RefPtr<IPC::Connection> allowedConnectionProtector(allowedConnection);

We normally use the word “protector” for something that only protects and is not used directly. For a protected value, I think “protected” would be better:

    protectedThis
    protectedAllowedConnection

[Darin Adler]

Comment on attachment 240362 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        [WK2] Remove uses of WTF::bind() in StorageManager

I don’t like the title here. There’s nothing wrong with WTF::bind, it’s just that we prefer lambdas instead of separate functions for this kind of work.

>> Source/WebKit2/UIProcess/Storage/StorageManager.cpp:392
>> +    RefPtr<IPC::Connection> allowedConnectionProtector(allowedConnection);
> 
> We normally use the word “protector” for something that only protects and is not used directly. For a protected value, I think “protected” would be better:
> 
>     protectedThis
>     protectedAllowedConnection

I’d like you to consider this name change throughout.

Also wondering if we could use Ref instead of RefPtr. Probably not.

> Source/WebKit2/UIProcess/Storage/StorageManager.cpp:445
> +        HashMap<std::pair<RefPtr<IPC::Connection>, uint64_t>, RefPtr<StorageArea>> storageAreasByConnection = protector->m_storageAreasByConnection;

Can use auto here instead of reciting the HashMap class name.

Might be worth adding a comment about why we need to copy the map here and naming the local variable to indicate that as well.

[Zan Dobersek]

Comment on attachment 240362 [details]
Patch

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

>> Source/WebKit2/ChangeLog:3
>> +        [WK2] Remove uses of WTF::bind() in StorageManager
> 
> I don’t like the title here. There’s nothing wrong with WTF::bind, it’s just that we prefer lambdas instead of separate functions for this kind of work.

I'll come up with something more fitting.

>>> Source/WebKit2/UIProcess/Storage/StorageManager.cpp:392
>>> +    RefPtr<IPC::Connection> allowedConnectionProtector(allowedConnection);
>> 
>> We normally use the word “protector” for something that only protects and is not used directly. For a protected value, I think “protected” would be better:
>> 
>>     protectedThis
>>     protectedAllowedConnection
> 
> I’d like you to consider this name change throughout.
> 
> Also wondering if we could use Ref instead of RefPtr. Probably not.

I'll adopt the naming.

Using Ref isn't possible because it's not copyable.

>> Source/WebKit2/UIProcess/Storage/StorageManager.cpp:445
>> +        HashMap<std::pair<RefPtr<IPC::Connection>, uint64_t>, RefPtr<StorageArea>> storageAreasByConnection = protector->m_storageAreasByConnection;
> 
> Can use auto here instead of reciting the HashMap class name.
> 
> Might be worth adding a comment about why we need to copy the map here and naming the local variable to indicate that as well.

Copying the HashMap isn't necessary.

In fact, using HashMap::removeIf() would work best here.

[Zan Dobersek]

Created attachment 240597 [details]
Patch

Asking for another review, especially regarding the use of removeIf() in StorageManager::processWillCloseConnection().

[Zan Dobersek]

Fixed in bug #140086.

*** This bug has been marked as a duplicate of bug 140086 ***