Summary: | Modernize lambda usage for all callers of RunLoop::dispatch() (take 2) | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||||||||
Component: | WebCore Misc. | Assignee: | Brady Eidson <beidson> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | buildbot, cdumez, cgarcia, commit-queue, rniwa | ||||||||||
Priority: | P2 | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=158265 | ||||||||||||
Attachments: |
|
Description
Brady Eidson
2016-06-01 15:16:53 PDT
Created attachment 280277 [details]
Patch
Obviously missed something here. 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. 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 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. 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
(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 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. *** Bug 158010 has been marked as a duplicate of this bug. *** 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. I Ref<>'ed something that was allowed to be nullptr (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 Created attachment 280297 [details]
Patch
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() &&; (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! |