| Summary: | [WK2] Use C++11 lambdas instead of helper methods in StorageManager | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Zan Dobersek <zan> | ||||||
| Component: | New Bugs | Assignee: | Zan Dobersek <zan> | ||||||
| Status: | RESOLVED DUPLICATE | ||||||||
| Severity: | Normal | CC: | darin | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Zan Dobersek
2014-10-23 12:59:15 PDT
Created attachment 240362 [details]
Patch
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 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 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. Created attachment 240597 [details]
Patch
Asking for another review, especially regarding the use of removeIf() in StorageManager::processWillCloseConnection().
Fixed in bug #140086. *** This bug has been marked as a duplicate of bug 140086 *** |