Bug 158277 – Modernize lambda usage for all callers of RunLoop::dispatch() (take 2)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

Modernize lambda usage for all callers of RunLoop::dispatch() (take 2)

In https://bugs.webkit.org/show_bug.cgi?id=158265 I'd searched for ".dispatch("
Now I'm searching for ">dispatch(" and making the same types of changes.

[Brady Eidson]

Created attachment 280277 [details]
Patch

[Brady Eidson]

Obviously missed something here.

[Build Bot]

Comment on attachment 280277 [details]
Patch

Attachment 280277 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1420330

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 280283 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 280277 [details]
Patch

Attachment 280277 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1420336

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 280285 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Brady Eidson]

(In reply to comment #2)
> Obviously missed something here.

This comment was based on the fact that I saw the tests failing. Exploring locally in a bit.

[Chris Dumez]

Comment on attachment 280277 [details]
Patch

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

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:619
> +    WorkQueue::create("com.apple.WebKit.Cache.delete")->dispatch([path = dumpFilePath()] {

Why doesn't this call isolatedCopy()?

> Source/WebKit2/UIProcess/Storage/StorageManager.cpp:493
> +    m_queue->dispatch([this, protectedThis = Ref<StorageManager>(*this), connection = Ref<IPC::Connection>(*allowedConnection), storageNamespaceID]() mutable {

allowedConnection can be null :(

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:261
>  #if ENABLE(VIDEO)

Not strictly related to your change but the #ifdefs in this file should be outside the if() scope, not inside.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:546
>  #if ENABLE(VIDEO)

Not strictly related to your change but the #ifdefs in this file should be outside the if() scope, not inside.

[Chris Dumez]

*** Bug 158010 has been marked as a duplicate of this bug. ***

[Chris Dumez]

Comment on attachment 280277 [details]
Patch

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

> Source/WebKit2/UIProcess/Storage/StorageManager.cpp:527
>      RefPtr<IPC::Connection> protectedConnection(&connection);

I think you meant to drop this line as well.

[Brady Eidson]

I Ref<>'ed something that was allowed to be nullptr

[Brady Eidson]

(In reply to comment #8)
> Comment on attachment 280277 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280277&action=review
> 
> > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:619
> > +    WorkQueue::create("com.apple.WebKit.Cache.delete")->dispatch([path = dumpFilePath()] {
> 
> Why doesn't this call isolatedCopy()?

I double checked what dumpFilePath() does - It always creates a brand new String, therefore new StringImpl, therefore isolatedCopy not needing.

> 
> > Source/WebKit2/UIProcess/Storage/StorageManager.cpp:493
> > +    m_queue->dispatch([this, protectedThis = Ref<StorageManager>(*this), connection = Ref<IPC::Connection>(*allowedConnection), storageNamespaceID]() mutable {
> 
> allowedConnection can be null :(

Yup, discovered that. https://bugs.webkit.org/show_bug.cgi?id=158277#c11

[Brady Eidson]

Created attachment 280297 [details]
Patch

[Chris Dumez]

Comment on attachment 280297 [details]
Patch

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

R=me with a comment.

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:619
> +    WorkQueue::create("com.apple.WebKit.Cache.delete")->dispatch([path = dumpFilePath()] {

I still think it would be less fragile to call isolatedCopy() here. I don't believe this is more efficient either because isolatedCopy() when called on a temporary already takes care of avoiding the copy in cases that are safe. See
String isolatedCopy() &&;

[Brady Eidson]

(In reply to comment #14)
> Comment on attachment 280297 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280297&action=review
> 
> R=me with a comment.
> 
> > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:619
> > +    WorkQueue::create("com.apple.WebKit.Cache.delete")->dispatch([path = dumpFilePath()] {
> 
> I still think it would be less fragile to call isolatedCopy() here. I don't
> believe this is more efficient either because isolatedCopy() when called on
> a temporary already takes care of avoiding the copy in cases that are safe.
> See
> String isolatedCopy() &&;

Fair enough. I didn't know about that optimization!

[Brady Eidson]

http://trac.webkit.org/changeset/201587