Bug 156645 – New SPI to export a dictionary of runtime features

• Component: New Bugs
• Status: RESOLVED FIXED

[Dean Jackson]

New SPI to export a dictionary of runtime features

[Dean Jackson]

Created attachment 276511 [details]
Patch

[Dean Jackson]

Comment on attachment 276511 [details]
Patch

Not for review yet.

[Anders Carlsson]

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.

[Dean Jackson]

<rdar://problem/23621666>

[Dean Jackson]

Created attachment 276524 [details]
Patch

[Dean Jackson]

Created attachment 276538 [details]
Patch

[Dean Jackson]

Updated after comments from Anders

[Dean Jackson]

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?

[Darin Adler]

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.

[Dean Jackson]

Committed r199700: <http://trac.webkit.org/changeset/199700>

[Dean Jackson]

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?

[Alexey Proskuryakov]

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.

[Csaba Osztrogonác]

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