Bug 216586 - Make Settings generated from Settings.yaml work with Workers
Summary: Make Settings generated from Settings.yaml work with Workers
Status: RESOLVED CONFIGURATION CHANGED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-09-15 15:22 PDT by Sam Weinig
Modified: 2021-02-27 18:55 PST (History)
6 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2020-09-15 15:22:57 PDT
The biggest hurdle to deal with for moving off of RuntimeEnabledFeatures is making the settings generated from Settings.yaml work with Workers. Specifically, making it so that IDL interfaces exposed to workers can use the EnabledBySetting=Foo syntax to indicate that an interface/operation/attribute is conditionally available based on Settings. (Currently, interfaces/operations/attribute that need this use EnabledAtRuntime, which uses RuntimeEnabledFeatures in a thread unsafe way).

I am not sure what the best way to go about this would be. 


Regardless of the technique we end up going, the end goal for the bindings will probably look pretty much the same. We will go from checks that look like:

downcast<Document>(jsCast<JSDOMGlobalObject*>(globalObject)->scriptExecutionContext())->settings()->isFooEnabled()

to:

jsCast<JSDOMGlobalObject*>(globalObject)->scriptExecutionContext()->settings()->isFooEnabled();


What exactly ScriptExecutionContext::settings() returns, and how it is populated is what we need to figure out.
Comment 1 Sam Weinig 2020-09-15 15:50:44 PDT
The most straightforward approach would be to move the 'const Ref<Settings>' member from Document to ScriptExecutionContext, and adding a lock around all getting/setting. (I don't think the overhead of the lock should matter, hopefully nothing is checking a setting in any perf sensitive code, but any settings returning Strings will also have to be switched to always copy the string in and out, so it's not so great).

An alternative strategy would be taking a copy of the Settings (or perhaps the subset needed by Workers) when spawning a Worker, and then serializing mutations to the Settings to each currently active Worker as necessary,
Comment 2 Sam Weinig 2020-09-15 15:51:25 PDT
cc'ing folks who might have interest in the topic to see if they have opinions.
Comment 3 Alex Christensen 2020-09-15 17:32:52 PDT
Would it be possible to make the settings object use std::atomic so we don't need any locks?  Most of them would be std::atomic<bool> and we could figure out something for those that aren't.
Comment 4 Sam Weinig 2020-09-15 17:45:06 PDT
(In reply to Alex Christensen from comment #3)
> Would it be possible to make the settings object use std::atomic so we don't
> need any locks?  Most of them would be std::atomic<bool> and we could figure
> out something for those that aren't.

Yeah, we could do that for the bool / integer ones.
Comment 5 Radar WebKit Bug Importer 2020-09-22 15:39:07 PDT
<rdar://problem/69396170>
Comment 6 Sam Weinig 2021-02-27 18:55:35 PST
With SettingValues, this is now complete.