Bug 126435 – Move WebCore storage code to C++11 lambdas, std::function

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

[Zan Dobersek]

Move WebCore storage code to std::bind, std::function

[Zan Dobersek]

Created attachment 220316 [details]
Patch

[WebKit Commit Bot]

Attachment 220316 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/storage/StorageAreaSync.cpp', u'Source/WebCore/storage/StorageSyncManager.cpp', u'Source/WebCore/storage/StorageSyncManager.h', u'Source/WebCore/storage/StorageThread.cpp', u'Source/WebCore/storage/StorageThread.h', u'Source/WebCore/storage/StorageTracker.cpp', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/storage/StorageThread.cpp:79:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageThread.cpp:83:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageThread.cpp:95:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageThread.h:48:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageThread.h:61:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageTracker.cpp:305:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageSyncManager.h:44:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageSyncManager.cpp:74:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 8 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 220316 [details]
Patch

Attachment 220316 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5418391959502848

[Build Bot]

Comment on attachment 220316 [details]
Patch

Attachment 220316 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6403554377990144

[Anders Carlsson]

Comment on attachment 220316 [details]
Patch

I don't think this is correct. WTF::bind has some tricks to automatically ref/deref objects that are captured and I think that's lost in this conversion.

[Zan Dobersek]

Created attachment 220324 [details]
Patch

[WebKit Commit Bot]

Attachment 220324 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/storage/StorageAreaSync.cpp', u'Source/WebCore/storage/StorageSyncManager.cpp', u'Source/WebCore/storage/StorageSyncManager.h', u'Source/WebCore/storage/StorageThread.cpp', u'Source/WebCore/storage/StorageThread.h', u'Source/WebCore/storage/StorageTracker.cpp', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/storage/StorageThread.cpp:79:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageThread.cpp:83:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageThread.cpp:95:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageThread.h:49:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageThread.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageTracker.cpp:305:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageSyncManager.h:45:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageSyncManager.cpp:74:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 8 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

Comment on attachment 220324 [details]
Patch

Removing the r? flag, still has the same issue as the previous patch.

[Zan Dobersek]

Created attachment 220433 [details]
Patch

[WebKit Commit Bot]

Attachment 220433 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/storage/StorageAreaSync.cpp', u'Source/WebCore/storage/StorageSyncManager.cpp', u'Source/WebCore/storage/StorageSyncManager.h', u'Source/WebCore/storage/StorageThread.cpp', u'Source/WebCore/storage/StorageThread.h', u'Source/WebCore/storage/StorageTracker.cpp', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/storage/StorageThread.cpp:79:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageThread.cpp:83:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageThread.cpp:95:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageThread.h:49:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageThread.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageTracker.cpp:305:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageSyncManager.h:45:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageAreaSync.cpp:115:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/storage/StorageSyncManager.cpp:74:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 9 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

Comment on attachment 220433 [details]
Patch

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

> Source/WebCore/storage/StorageAreaSync.cpp:78
> -    m_syncManager->dispatch(bind(&StorageAreaSync::performImport, this));
> +    m_syncManager->dispatch(std::bind(&StorageAreaSync::performImport,
> +        std::bind([](const RefPtr<StorageAreaSync>& storageArea) { return storageArea.get(); }, RefPtr<StorageAreaSync>(this))));

To handle referencing and dereferencing of the StorageAreaSync object on which the member function should be called, a nested bind expression is used that holds onto a RefPtr<StorageAreaSync> object and returns a raw pointer to the StorageAreaSync object when called.

If this approach makes sense it can be abstracted into a helper template.

[Anders Carlsson]

I'd rather we moved away from std::bind to lambdas instead.

[Zan Dobersek]

Lambdas would work and look better, but capturing the this pointer wouldn't handle the (de)referencing, so a RefPtr<T> variable would have to be initalized and captured by value.

C++14 adds support for inlining lambda capture initializations. That's available in Clang 3.4 and GCC 4.9.

[Zan Dobersek]

Created attachment 220505 [details]
Patch

[WebKit Commit Bot]

Attachment 220505 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/storage/StorageAreaSync.cpp', u'Source/WebCore/storage/StorageSyncManager.cpp', u'Source/WebCore/storage/StorageSyncManager.h', u'Source/WebCore/storage/StorageThread.cpp', u'Source/WebCore/storage/StorageThread.h', u'Source/WebCore/storage/StorageTracker.cpp', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/storage/StorageThread.cpp:79:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageThread.cpp:83:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageThread.cpp:95:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageThread.h:49:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageThread.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageTracker.cpp:175:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/storage/StorageTracker.cpp:238:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/storage/StorageTracker.cpp:306:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageTracker.cpp:315:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/storage/StorageTracker.cpp:315:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageTracker.cpp:388:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/storage/StorageTracker.cpp:485:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/storage/StorageSyncManager.h:45:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageAreaSync.cpp:115:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/storage/StorageSyncManager.cpp:74:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 15 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

Comment on attachment 220505 [details]
Patch

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

> Source/WebCore/storage/StorageTracker.cpp:288
> -        callOnMainThread(bind(&StorageTracker::deleteOriginWithIdentifier, this, originIdentifier.isolatedCopy()));
> +        callOnMainThread(std::bind([this](const String& originIdentifier) { deleteOriginWithIdentifier(originIdentifier); },
> +            originIdentifier.isolatedCopy()));

std::bind() remains in use here and in similar places to avoid unnecessarily copying the isolated copies through lambda capture:

    String originIdentifierCopy = originIdentifier.isolatedCopy();
    callOnMainThread([this, originIdentifierCopy] { deleteOriginWithIdentifier(originIdentifier); });

Again, C++14 lambda improvements will allow just this:

    callOnMainThread([this, originIdentifierCopy = originIdentifier.copy()] { deleteOriginWithIdentifier(originIdentifier); });

[Zan Dobersek]

Created attachment 220636 [details]
Patch

Get rid of the multiple-commands-on-same-line style errors. The errors caused by the space between the return type and the leading parantheses of the wrapped function signature should be resolved in the style checker.

[WebKit Commit Bot]

Attachment 220636 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/storage/StorageAreaSync.cpp', u'Source/WebCore/storage/StorageSyncManager.cpp', u'Source/WebCore/storage/StorageSyncManager.h', u'Source/WebCore/storage/StorageThread.cpp', u'Source/WebCore/storage/StorageThread.h', u'Source/WebCore/storage/StorageTracker.cpp', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/storage/StorageThread.cpp:79:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageThread.cpp:83:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageThread.cpp:95:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageThread.h:49:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageThread.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageTracker.cpp:311:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageTracker.cpp:320:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageSyncManager.h:45:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageSyncManager.cpp:74:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 9 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

Comment on attachment 220636 [details]
Patch

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

> Source/WebCore/storage/StorageTracker.cpp:293
> +        callOnMainThread(std::bind([this](const String& originIdentifier) {
> +            deleteOriginWithIdentifier(originIdentifier);
> +        }, originIdentifier.isolatedCopy()));

In cases such as this we now mostly use a pure lambda that captures an isolated copy of the necessary string instead of relying on std::bind().

I'll update this patch to use that approach.

[Zan Dobersek]

Created attachment 237031 [details]
Patch

[WebKit Commit Bot]

Attachment 237031 [details] did not pass style-queue:


ERROR: Source/WebCore/storage/StorageThread.cpp:79:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageThread.cpp:83:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageThread.cpp:95:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageThread.h:49:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageThread.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageSyncManager.h:45:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/storage/StorageSyncManager.cpp:74:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 7 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 237031 [details]
Patch

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

> Source/WebCore/storage/StorageAreaSync.cpp:75
> +    m_syncManager->dispatch([=] {

Is there a good reason that in sites like this we have [=] instead of [protector]?

[Zan Dobersek]

Comment on attachment 237031 [details]
Patch

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

>> Source/WebCore/storage/StorageAreaSync.cpp:75
>> +    m_syncManager->dispatch([=] {
> 
> Is there a good reason that in sites like this we have [=] instead of [protector]?

Not really, explicitly listing the captured variables is generally a better idea since it prevents accidentally capturing unwanted objects.

I'll fix that before landing.

[Zan Dobersek]

Committed r172901: <http://trac.webkit.org/changeset/172901>