Bug 158277 - Modernize lambda usage for all callers of RunLoop::dispatch() (take 2)
Summary: Modernize lambda usage for all callers of RunLoop::dispatch() (take 2)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
: 158010 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-06-01 15:16 PDT by Brady Eidson
Modified: 2016-06-01 20:39 PDT (History)
5 users (show)

See Also:


Attachments
Patch (27.23 KB, patch)
2016-06-01 16:40 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (673.71 KB, application/zip)
2016-06-01 17:22 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (939.07 KB, application/zip)
2016-06-01 17:34 PDT, Build Bot
no flags Details
Patch (27.98 KB, patch)
2016-06-01 19:50 PDT, Brady Eidson
cdumez: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2016-06-01 15:16:53 PDT
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.
Comment 1 Brady Eidson 2016-06-01 16:40:38 PDT
Created attachment 280277 [details]
Patch
Comment 2 Brady Eidson 2016-06-01 17:17:47 PDT
Obviously missed something here.
Comment 3 Build Bot 2016-06-01 17:22:56 PDT
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.
Comment 4 Build Bot 2016-06-01 17:22:59 PDT
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
Comment 5 Build Bot 2016-06-01 17:33:58 PDT
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.
Comment 6 Build Bot 2016-06-01 17:34:01 PDT
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
Comment 7 Brady Eidson 2016-06-01 19:03:45 PDT
(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.
Comment 8 Chris Dumez 2016-06-01 19:15:38 PDT
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.
Comment 9 Chris Dumez 2016-06-01 19:18:15 PDT
*** Bug 158010 has been marked as a duplicate of this bug. ***
Comment 10 Chris Dumez 2016-06-01 19:22:09 PDT
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.
Comment 11 Brady Eidson 2016-06-01 19:47:45 PDT
I Ref<>'ed something that was allowed to be nullptr
Comment 12 Brady Eidson 2016-06-01 19:50:02 PDT
(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
Comment 13 Brady Eidson 2016-06-01 19:50:41 PDT
Created attachment 280297 [details]
Patch
Comment 14 Chris Dumez 2016-06-01 20:08:22 PDT
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() &&;
Comment 15 Brady Eidson 2016-06-01 20:13:07 PDT
(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!
Comment 16 Brady Eidson 2016-06-01 20:39:07 PDT
http://trac.webkit.org/changeset/201587