Bug 138015 - [WK2] Use C++11 lambdas instead of helper methods in StorageManager
Summary: [WK2] Use C++11 lambdas instead of helper methods in StorageManager
Status: RESOLVED DUPLICATE of bug 140086
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-23 12:59 PDT by Zan Dobersek
Modified: 2015-02-22 08:25 PST (History)
1 user (show)

See Also:


Attachments
Patch (16.45 KB, patch)
2014-10-23 13:05 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (16.32 KB, patch)
2014-10-29 04:59 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2014-10-23 12:59:15 PDT
[WK2] Remove uses of WTF::bind() in StorageManager
Comment 1 Zan Dobersek 2014-10-23 13:05:05 PDT
Created attachment 240362 [details]
Patch
Comment 2 Darin Adler 2014-10-25 17:24:19 PDT
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
Comment 3 Darin Adler 2014-10-28 09:37:21 PDT
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.
Comment 4 Zan Dobersek 2014-10-29 04:44:41 PDT
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.
Comment 5 Zan Dobersek 2014-10-29 04:59:37 PDT
Created attachment 240597 [details]
Patch

Asking for another review, especially regarding the use of removeIf() in StorageManager::processWillCloseConnection().
Comment 6 Zan Dobersek 2015-02-22 08:24:58 PST
Fixed in bug #140086.

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