Bug 183908 - Stop using dispatch_set_target_queue()
Summary: Stop using dispatch_set_target_queue()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-03-22 11:53 PDT by David Kilzer (:ddkilzer)
Modified: 2018-03-23 12:07 PDT (History)
7 users (show)

See Also:


Attachments
Patch v1 (12.39 KB, patch)
2018-03-22 13:16 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v2 (12.74 KB, patch)
2018-03-23 10:53 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2018-03-22 11:53:06 PDT
WebKit should stop using dispatch_set_target_queue() and replace it with dispatch_queue_create_with_target() where needed.
Comment 1 David Kilzer (:ddkilzer) 2018-03-22 11:53:18 PDT
<rdar://problem/33553533>
Comment 2 David Kilzer (:ddkilzer) 2018-03-22 13:08:43 PDT
Filed this bug to upstream libwebrtc changes:
https://bugs.chromium.org/p/webrtc/issues/detail?id=9055
Comment 3 David Kilzer (:ddkilzer) 2018-03-22 13:16:43 PDT
Created attachment 336300 [details]
Patch v1
Comment 4 youenn fablet 2018-03-22 13:37:28 PDT
(In reply to David Kilzer (:ddkilzer) from comment #2)
> Filed this bug to upstream libwebrtc changes:
> https://bugs.chromium.org/p/webrtc/issues/detail?id=9055

Nice.
In case they do not take it, would it be possible to add a patch for just that change in Source/ThirdParty/libwebrtc/WebKit/
When updating libwebrtc, that will give a chance to not loose this change by reapplying it.
Comment 5 David Kilzer (:ddkilzer) 2018-03-22 14:09:05 PDT
(In reply to youenn fablet from comment #4)
> (In reply to David Kilzer (:ddkilzer) from comment #2)
> > Filed this bug to upstream libwebrtc changes:
> > https://bugs.chromium.org/p/webrtc/issues/detail?id=9055
> 
> Nice.
> In case they do not take it, would it be possible to add a patch for just
> that change in Source/ThirdParty/libwebrtc/WebKit/
> When updating libwebrtc, that will give a chance to not loose this change by
> reapplying it.

It's already included in Source/ThirdParty/libwebrtc/WebKit/patch-libwebrtc in the posted patch.  Is that not the correct place to put it?
Comment 6 youenn fablet 2018-03-22 14:19:38 PDT
(In reply to David Kilzer (:ddkilzer) from comment #5)
> (In reply to youenn fablet from comment #4)
> > (In reply to David Kilzer (:ddkilzer) from comment #2)
> > > Filed this bug to upstream libwebrtc changes:
> > > https://bugs.chromium.org/p/webrtc/issues/detail?id=9055
> > 
> > Nice.
> > In case they do not take it, would it be possible to add a patch for just
> > that change in Source/ThirdParty/libwebrtc/WebKit/
> > When updating libwebrtc, that will give a chance to not loose this change by
> > reapplying it.
> 
> It's already included in Source/ThirdParty/libwebrtc/WebKit/patch-libwebrtc
> in the posted patch.  Is that not the correct place to put it?

Oh, I did not see it.
I moved away from one big diff patch to smaller commit-like diffs (see 001-XXX...).
Could patch-libwebrtc be renamed to something more explicit?

LGTM otherwise.
Comment 7 Daniel Bates 2018-03-23 09:37:48 PDT
Comment on attachment 336300 [details]
Patch v1

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

> Tools/Scripts/webkitpy/style/checkers/cpp.py:3229
> +              'Never use dispatch_set_target_queue.  Use dispatch_queue_create_with_target instead.')

Out of curiosity, why?
Comment 8 Daniel Bates 2018-03-23 09:39:38 PDT
(In reply to Daniel Bates from comment #7)
> Comment on attachment 336300 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336300&action=review
> 
> > Tools/Scripts/webkitpy/style/checkers/cpp.py:3229
> > +              'Never use dispatch_set_target_queue.  Use dispatch_queue_create_with_target instead.')
> 
> Out of curiosity, why?

You explained why in bug description for <https://bugs.chromium.org/p/webrtc/issues/detail?id=9055>, repeated below:

[[
1. The dispatch_set_target_queue() method is not officially deprecated, but it should be avoided because it causes mutation of the dispatch queue after it has been created, which can cause a number of issues covered in this WWDC17 talk (starting at the 40m30s mark):
<https://developer.apple.com/videos/play/wwdc2017/706/>
]]
Comment 9 David Kilzer (:ddkilzer) 2018-03-23 10:53:45 PDT
Created attachment 336389 [details]
Patch v2
Comment 10 David Kilzer (:ddkilzer) 2018-03-23 12:03:20 PDT
(In reply to David Kilzer (:ddkilzer) from comment #9)
> Created attachment 336389 [details]
> Patch v2

Will land this manually since Dan Bates gave r+, and the only change I made was to rename the libwebrtc patch from WebKit/patch-libwebrtc to WebKit/0009-Remove-dispatch_set_target_queue.patch.
Comment 11 David Kilzer (:ddkilzer) 2018-03-23 12:07:27 PDT
Committed r229913: <https://trac.webkit.org/changeset/229913>