New SPI to export a dictionary of runtime features
Created attachment 276511 [details] Patch
Comment on attachment 276511 [details] Patch Not for review yet.
Comment on attachment 276511 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276511&action=review > Source/WebKit2/Shared/WebPreferencesDefinitions.h:101 > // macro(KeyUpper, KeyLower, TypeNameUpper, TypeName, DefaultValue) Please update this comment. > Source/WebKit2/Shared/WebPreferencesKeys.cpp:34 > +#define DEFINE_KEY_GETTERS(KeyUpper, KeyLower, TypeName, Type, DefaultValue, Name, Details) \ I think name and Details need to be more descriptive.
<rdar://problem/23621666>
Created attachment 276524 [details] Patch
Created attachment 276538 [details] Patch
Updated after comments from Anders
Comment on attachment 276538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276538&action=review > Source/WebKit2/UIProcess/API/Cocoa/WKPreferences.mm:430 > + Vector<RefPtr<API::Object>> experimentalFeatures = WebKit::WebPreferences::experimentalFeatures(); > + Ref<API::Array> descriptions = API::Array::create(WTFMove(experimentalFeatures)); > + return [wrapper(descriptions.leakRef()) autorelease]; I really wanted WebKit::WebPreferences::experimentalFeatures() to return a Vector<RefPtr<API::ExperimentalFeature>>. However, the API::Array::create didn't accept such a type, and I was unable to cast from Vector<RefPtr<API::ExperimentalFeature>> to Vector<RefPtr<API::Object>>. Is there a way to do it?
Comment on attachment 276538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276538&action=review Patch looks fine as is, r=me without changes. I have some suggestions for making it better. >> Source/WebKit2/UIProcess/API/Cocoa/WKPreferences.mm:430 >> + return [wrapper(descriptions.leakRef()) autorelease]; > > I really wanted WebKit::WebPreferences::experimentalFeatures() to return a Vector<RefPtr<API::ExperimentalFeature>>. However, the API::Array::create didn't accept such a type, and I was unable to cast from Vector<RefPtr<API::ExperimentalFeature>> to Vector<RefPtr<API::Object>>. > > Is there a way to do it? There’s no way to cast. We’d have to copy the vector to change from a more specific type to a less specific type. And API::Array::create takes a specific type because it moves the passed-in vector in. If we added functions for API::Array::create that were more flexible about the vector type, they would have to copy the vector rather than moving it in. I suggest using "auto" when writing code like this, rather than writing out the types, and also not using as many local variables. I think I would have written this: auto features = WebKit::WebPreferences::experimentalFeatures(); return [wrapper(API::Array::create(WTFMove(features)).leakRef()) autorelease]; I’m keeping the local variable for the features because we’d probably want to change experimentalFeatures to return a const& to a global vector rather than returning a new one every time. If we kept it returning a Vector then we would probably want to eliminate that local just so we don’t have to explicitly do WTFMove. > Source/WebKit2/UIProcess/API/Cocoa/WKPreferencesPrivate.h:95 > +- (void)_setEnabled:(BOOL)value forFeature:(_WKExperimentalFeature *)feature; > + > + > @end Extra blank line here. Please remove one. > Source/WebKit2/UIProcess/WebPreferences.cpp:195 > + static NeverDestroyed<Vector<RefPtr<API::Object>>> features; I believe the GTK and EFL builds are failing because this file needs to include <wtf/NeverDestroyed.h>. In the other ports I am guessing it’s indirectly included by some header. > Source/WebKit2/UIProcess/WebPreferences.cpp:199 > + apiFeature = API::ExperimentalFeature::create(HumanReadableName, #KeyUpper, HumanReadableDescription); \ > + features.get().append(apiFeature); \ No need for the local variable here. Better to just write: features.get().append(API::ExperimentalFeature::create(HumanReadableName, #KeyUpper, HumanReadableDescription)); > Source/WebKit2/UIProcess/WebPreferences.cpp:201 > + if (!features.get().size()) { Nicer to write this as two functions so we don’t have do this "check the size" thing. I suggest: static Vector<RefPtr<API::Object>> createExperimentalFeaturesVector() { Vector<RefPtr<API::Object>> features; // ... return features; } Then below: const Vector<RefPtr<API::Object>>& WebPreferences::experimentalFeatures() { static NeverDestroyed<Vector<RefPtr<API::Object>>> features = createExperimentalFeaturesVector(); return features; } Annoying that every time someone calls WebPreferences::experimentalFeatures it creates a new vector. Instead return type should be const&. That does mean we’d have to explicitly copy the vector in -[WKPreferences _experimentalFeatures], but I think that’s a better place to do it, since the need for a new vector comes out of the way API::Array works. The way the code is currently written, the copying would happen automatically if the type was const Vector&. Given that there is not already a vector present to be adopted by API::Array, we could make this return Vector<RefPtr<API::ExperimentalFeature>> and then add a function, whether hand-written or template, external or built into API::Array that takes a const Vector& and makes a new vector and calls API::Array::create on that. But I’m not sure any of that is worth the effort. > Source/WebKit2/UIProcess/WebPreferences.cpp:225 > + if (key == #KeyUpper) \ > + return KeyLower(); \ Rather than having the code unrolled like this I think it’s better to use the macro to generate a global constant table with a string and a function pointer for each and use a loop, or even a hash table (loop for now, I think). > Source/WebKit2/UIProcess/WebPreferences.cpp:240 > + if (key == #KeyUpper) \ > + set##KeyUpper(value); \ Ditto.
Committed r199700: <http://trac.webkit.org/changeset/199700>
Committed with most of Darin's comments addressed. Will still need a followup about the function table.
Comment on attachment 276538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276538&action=review > Source/WebKit2/UIProcess/API/Cocoa/WKPreferencesPrivate.h:92 > ++ (WK_ARRAY(_WKExperimentalFeature *) *)_experimentalFeatures; > +- (BOOL)_isEnabledForFeature:(_WKExperimentalFeature *)feature; > +- (void)_setEnabled:(BOOL)value forFeature:(_WKExperimentalFeature *)feature; These need availability annotations. > Source/WebKit2/UIProcess/API/Cocoa/_WKExperimentalFeature.h:32 > +WK_CLASS_AVAILABLE(10_11, 9_0) This doesn’t seem right. Was this API available in OS X 10.11 and iOS 9.0?
This broke the build: https://build.webkit.org/builders/Apple%20Yosemite%20Release%20%28Build%29/builds/14411/steps/compile-webkit/logs/errors
(In reply to comment #13) > This broke the build: > > https://build.webkit.org/builders/Apple%20Yosemite%20Release%20%28Build%29/ > builds/14411/steps/compile-webkit/logs/errors I expect <http://trac.webkit.org/r199705> to have fixed this.
(In reply to comment #10) > Committed r199700: <http://trac.webkit.org/changeset/199700> It broke the Apple Mac cmake build, see build.webkit.org for details.
(In reply to comment #12) > Comment on attachment 276538 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=276538&action=review > > > Source/WebKit2/UIProcess/API/Cocoa/WKPreferencesPrivate.h:92 > > ++ (WK_ARRAY(_WKExperimentalFeature *) *)_experimentalFeatures; > > +- (BOOL)_isEnabledForFeature:(_WKExperimentalFeature *)feature; > > +- (void)_setEnabled:(BOOL)value forFeature:(_WKExperimentalFeature *)feature; > > These need availability annotations. Added in <http://trac.webkit.org/r200033>.