Bug 172235 – Bindings: Support runtime-enabled features in specific worlds

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Daniel Bates]

Currently an interface, function, or attribute can be annotated with either EnabledAtRuntime or EnabledForWorld, but not both. Even though we do not have any interfaces, functions, or attributes that are annotated with both at the time of writing, it seems reasonable to support such a combination of annotations. This will also generalize the CodeGenerator code that exposes/conceals an interface, function, or attribute so as to make it straightforward to support the extended attribute SecureContext.

[Daniel Bates]

Created attachment 310423 [details]
Patch and tests

[Sam Weinig]

I am not a fan of continuing to promote the use of RuntimeEnabledFeature, and instead think we should be use Settings for all "settings".  I'd rather not see more features being added to support RuntimeEnabledFeature.

[Daniel Bates]

(In reply to Sam Weinig from comment #2)
> I am not a fan of continuing to promote the use of RuntimeEnabledFeature,
> and instead think we should be use Settings for all "settings".  I'd rather
> not see more features being added to support RuntimeEnabledFeature.

We can likely remove the code generation logic for RuntimeEnabledFeature. I see this an an orthogonal issue to the purpose of this patch, generalizing code generation logic for arbitrary runtime enabled features, because the infrastructure that supports code generation of RuntimeEnabledFeature feature checks (e.g. GetRuntimeEnableFunctionName()) is also being used to support the extended attribute EnabledForWorld - to restrict interfaces, attributes, and functions to specific DOM worlds. I was planning to make use of this machinery to support the extended attribute SecureContext. We may be able to further refactor the CodeGeneratorJS machinery for features enabled via settings into the GetRuntimeEnableFunctionName() machinery.

[Daniel Bates]

Created attachment 310555 [details]
Patch and tests

Updated patch following the landing of the patch for bug #172252

[Chris Dumez]

Comment on attachment 310555 [details]
Patch and tests

This does not really add complexity and Daniel explains this is a first step towards supporting SecureContext, so OK.

[Chris Dumez]

(In reply to Sam Weinig from comment #2)
> I am not a fan of continuing to promote the use of RuntimeEnabledFeature,
> and instead think we should be use Settings for all "settings".  I'd rather
> not see more features being added to support RuntimeEnabledFeature.

I think this should be a webkit-dev thread then. There was a mail thread about using RuntimeEnabled more not too long ago. I even spent time improving our support for RuntimeEnabled in the bindings generator as a result.

[Daniel Bates]

Comment on attachment 310555 [details]
Patch and tests

Clearing flags on attachment: 310555

Committed r217146: <http://trac.webkit.org/changeset/217146>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Sam Weinig]

(In reply to Chris Dumez from comment #6)
> (In reply to Sam Weinig from comment #2)
> > I am not a fan of continuing to promote the use of RuntimeEnabledFeature,
> > and instead think we should be use Settings for all "settings".  I'd rather
> > not see more features being added to support RuntimeEnabledFeature.
> 
> I think this should be a webkit-dev thread then. There was a mail thread
> about using RuntimeEnabled more not too long ago. I even spent time
> improving our support for RuntimeEnabled in the bindings generator as a
> result.

Do you have a link to that thread in the archives, I'd be interested to go over it. 

By the way, in case it wasn't clear, I am all in favor of enabling features at runtime rather than compile time, I just think we should stick to one class for such configurations, and that class should be Settings. I've plumbed through support for using Settings in the bindings, so I don't think that should be the issue.

What are the virtues of having both RuntimeEnabledFeature and Settings?

[Chris Dumez]

(In reply to Sam Weinig from comment #9)
> (In reply to Chris Dumez from comment #6)
> > (In reply to Sam Weinig from comment #2)
> > > I am not a fan of continuing to promote the use of RuntimeEnabledFeature,
> > > and instead think we should be use Settings for all "settings".  I'd rather
> > > not see more features being added to support RuntimeEnabledFeature.
> > 
> > I think this should be a webkit-dev thread then. There was a mail thread
> > about using RuntimeEnabled more not too long ago. I even spent time
> > improving our support for RuntimeEnabled in the bindings generator as a
> > result.
> 
> Do you have a link to that thread in the archives, I'd be interested to go
> over it. 
> 
> By the way, in case it wasn't clear, I am all in favor of enabling features
> at runtime rather than compile time, I just think we should stick to one
> class for such configurations, and that class should be Settings. I've
> plumbed through support for using Settings in the bindings, so I don't think
> that should be the issue.
> 
> What are the virtues of having both RuntimeEnabledFeature and Settings?

Not much AFAICT. I did not even know the Settings version until you pointed it out. I have been using RuntimeEnabledFeature exclusively for a while, most people have (You can see how little EnabledBySetting is used right now compared to EnabledAtEuntime).

I am not saying EnabledAtEuntime is better. I am saying that if EnabledBySetting is better, then this needs to be discussed and we need to actively port code from one to another. We should also make sure EnabledBySetting works for all cases, like EnabledAtEuntime (e.g. On instance, on Prototype, on Window, ...).