Bug 156645

Summary: New SPI to export a dictionary of runtime features
Product: WebKit Reporter: Dean Jackson <dino>
Component: New BugsAssignee: Dean Jackson <dino>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, eoconnor, mitz, ossy
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch darin: review+

Description Dean Jackson 2016-04-15 14:22:15 PDT
New SPI to export a dictionary of runtime features
Comment 1 Dean Jackson 2016-04-15 14:26:34 PDT
Created attachment 276511 [details]
Patch
Comment 2 Dean Jackson 2016-04-15 14:27:03 PDT
Comment on attachment 276511 [details]
Patch

Not for review yet.
Comment 3 Anders Carlsson 2016-04-15 15:02:33 PDT
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.
Comment 4 Dean Jackson 2016-04-15 15:45:24 PDT
<rdar://problem/23621666>
Comment 5 Dean Jackson 2016-04-15 16:43:44 PDT
Created attachment 276524 [details]
Patch
Comment 6 Dean Jackson 2016-04-15 19:03:25 PDT
Created attachment 276538 [details]
Patch
Comment 7 Dean Jackson 2016-04-15 19:03:46 PDT
Updated after comments from Anders
Comment 8 Dean Jackson 2016-04-15 19:07:22 PDT
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 9 Darin Adler 2016-04-16 10:40:31 PDT
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.
Comment 10 Dean Jackson 2016-04-18 18:47:59 PDT
Committed r199700: <http://trac.webkit.org/changeset/199700>
Comment 11 Dean Jackson 2016-04-18 18:51:25 PDT
Committed with most of Darin's comments addressed. Will still need a followup about the function table.
Comment 12 mitz 2016-04-18 18:51:39 PDT
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?
Comment 14 mitz 2016-04-18 21:05:46 PDT
(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.
Comment 15 Csaba Osztrogonác 2016-04-18 23:06:14 PDT
(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.
Comment 16 mitz 2016-04-25 10:33:16 PDT
(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>.