Bug 219252 – Make worker RTC insertable streams constructs controlled by webrtc insertable streams feature setting

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

Make worker RTC insertable streams constructs controlled by webrtc insertable streams feature setting

[youenn fablet]

Created attachment 414796 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/71798249>

[youenn fablet]

Ping review.

[Sam Weinig]

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]

(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]

(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]

(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]

(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]

Created attachment 416147 [details]
Rebasing

[Sam Weinig]

Comment on attachment 416147 [details]
Rebasing

r=me too.

[EWS]

Committed r270842: <https://trac.webkit.org/changeset/270842>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416147 [details].