Bug 219252

Summary: Make worker RTC insertable streams constructs controlled by webrtc insertable streams feature setting
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebRTCAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, benjamin, calvaris, cdumez, eric.carlson, esprehn+autocc, ews-watchlist, glenn, hta, jer.noble, kangil.han, kondapallykalyan, philipj, sam, sergio, tommyw, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 219688    
Bug Blocks: 219890    
Attachments:
Description Flags
Patch
none
Rebasing none

youenn fablet
Reported 2020-11-23 01:41:37 PST
Make worker RTC insertable streams constructs controlled by webrtc insertable streams feature setting
Attachments
Patch (74.92 KB, patch)
2020-11-23 02:04 PST, youenn fablet 		no flags
DetailsFormatted DiffDiff
Rebasing (52.00 KB, patch)
2020-12-14 03:11 PST, youenn fablet 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
youenn fablet
Comment 1 2020-11-23 02:04:34 PST
Created attachment 414796 [details] Patch
Radar WebKit Bug Importer
Comment 2 2020-11-30 01:42:15 PST
<rdar://problem/71798249>
youenn fablet
Comment 3 2020-12-08 01:05:18 PST
Ping review.
Sam Weinig
Comment 4 2020-12-08 09:29:07 PST
Thank you so much for working on making this part of Settings better! Would you mind breaking out the change to Settings into its own patch? I think it would be good to look at it separately.
youenn fablet
Comment 5 2020-12-08 10:04:16 PST
(In reply to Sam Weinig from comment #4) > Thank you so much for working on making this part of Settings better! Would > you mind breaking out the change to Settings into its own patch? I think it > would be good to look at it separately. Sure. Do you have any preliminary comment on the Settings part of the patch? I for instance kept it as a subclass of Settings but contemplated to move it to its own class. Or maybe you have some other idea?
Sam Weinig
Comment 6 2020-12-08 10:29:13 PST
(In reply to youenn fablet from comment #5) > (In reply to Sam Weinig from comment #4) > > Thank you so much for working on making this part of Settings better! Would > > you mind breaking out the change to Settings into its own patch? I think it > > would be good to look at it separately. > > Sure. > Do you have any preliminary comment on the Settings part of the patch? > I for instance kept it as a subclass of Settings but contemplated to move it > to its own class. > Or maybe you have some other idea? Subobject seems fine, though I am not sure if it makes sense to constrain it to just the boolean values. I think I would prefer doing a deep (thread safe) copy of all the Settings if possible, just to keep things relatively normalized.
youenn fablet
Comment 7 2020-12-08 11:17:48 PST
(In reply to Sam Weinig from comment #6) > (In reply to youenn fablet from comment #5) > > (In reply to Sam Weinig from comment #4) > > > Thank you so much for working on making this part of Settings better! Would > > > you mind breaking out the change to Settings into its own patch? I think it > > > would be good to look at it separately. > > > > Sure. > > Do you have any preliminary comment on the Settings part of the patch? > > I for instance kept it as a subclass of Settings but contemplated to move it > > to its own class. > > Or maybe you have some other idea? > > Subobject seems fine, though I am not sure if it makes sense to constrain it > to just the boolean values. I think I would prefer doing a deep (thread > safe) copy of all the Settings if possible, just to keep things relatively > normalized. I used the boolean heuristic as a way to restrict to the feature flags that are necessary when creating the global scope. I thought a bit about extending that but it did not seem useful right now so ended up not doing it.
Sam Weinig
Comment 8 2020-12-08 11:38:14 PST
(In reply to youenn fablet from comment #7) > (In reply to Sam Weinig from comment #6) > > (In reply to youenn fablet from comment #5) > > > (In reply to Sam Weinig from comment #4) > > > > Thank you so much for working on making this part of Settings better! Would > > > > you mind breaking out the change to Settings into its own patch? I think it > > > > would be good to look at it separately. > > > > > > Sure. > > > Do you have any preliminary comment on the Settings part of the patch? > > > I for instance kept it as a subclass of Settings but contemplated to move it > > > to its own class. > > > Or maybe you have some other idea? > > > > Subobject seems fine, though I am not sure if it makes sense to constrain it > > to just the boolean values. I think I would prefer doing a deep (thread > > safe) copy of all the Settings if possible, just to keep things relatively > > normalized. > > I used the boolean heuristic as a way to restrict to the feature flags that > are necessary when creating the global scope. > I thought a bit about extending that but it did not seem useful right now so > ended up not doing it. There are a number of non-bool settings that will be useful in workers eventually, usually around maximum on minimum sizes (e.g. maximumAccelerated2dCanvasSize for offscreen canvas at some point, etc).
youenn fablet
Comment 9 2020-12-14 03:11:07 PST
Created attachment 416147 [details] Rebasing
Sam Weinig
Comment 10 2020-12-15 08:29:25 PST
Comment on attachment 416147 [details] Rebasing r=me too.
EWS
Comment 11 2020-12-15 08:46:00 PST
Committed r270842: <https://trac.webkit.org/changeset/270842> All reviewed patches have been landed. Closing bug and clearing flags on attachment 416147 [details].
Note You need to log in before you can comment on or make changes to this bug.