Bug 189442 – Add and expose Internal features from WebKit

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dean Jackson]

Add and expose Internal features from WebKit

[Dean Jackson]

<rdar://problem/44243404>

[Dean Jackson]

Experimental features have become a mess. People are using them for anything that they want to be easily toggled from a host app (e.g. Safari), which means the user-facing menu has become large and confusing.

Introduce the idea of Internal features, that will be exposed in a way that end-users are not expected to ever see (unless they really want to).

[Dean Jackson]

Created attachment 349215 [details]
Patch

[Dean Jackson]

Created attachment 349218 [details]
Patch

[Tim Horton]

Comment on attachment 349215 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349215&action=review

> Source/WebKit/Shared/WebPreferences.yaml:1162
> -  humanReadableName: "VisualViewportAPI"
> +  humanReadableName: "VisualViewport API"

THANK YOU maybe a space between visual and viewport too?

> Source/WebKit/Sources.txt:291
> +UIProcess/API/APIInternalDebugFeature.cpp @no-unify

Sort

> Source/WebKit/SourcesCocoa.txt:237
> +UIProcess/API/Cocoa/_WKInternalDebugFeature.mm @no-unify

Sort

> Source/WebKit/UIProcess/API/Cocoa/WKPreferencesPrivate.h:57
> +@class _WKInternalDebugFeature;

Sort

> Source/WebKit/UIProcess/API/Cocoa/_WKInternalDebugFeature.h:32
> +WK_CLASS_AVAILABLE(macosx(10.12), ios(10.0))

These don't seem like accurate versions.

[Sam Weinig]

Comment on attachment 349218 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349218&action=review

Very cool!

> Source/WebKit/Shared/WebPreferences.yaml:1246
> +  humanReadableDescription: "Modern Encrypted Media API"

This humanReadableDescription does not begin with "Enable" like at least ServerTimingEnabled and WebAPIStatisticsEnabled do. I am not sure which is better (or if we have any consistency here, but I thought I would point it out.

> Source/WebKit/Shared/WebPreferences.yaml:1261
> +# You must provide a humanReadableName and humanReadableName for all debug features. They

You repeat humanReadableName twice here. I think you mean humanReadableDescription for one of them. Also, I think you mean internal rather than debug.

> Source/WebKit/UIProcess/API/Cocoa/WKPreferences.mm:38
> +#import "_WKInternalDebugFeature.h"
> +#import "_WKInternalDebugFeatureInternal.h"

I don't think you need to include both of these since _WKInternalDebugFeatureInternal.h includes _WKInternalDebugFeature.h already.

[Dean Jackson]

Created attachment 349455 [details]
Patch

[Dean Jackson]

OOPS. Hadn't seen review comments. Will upload again.

[Dean Jackson]

Created attachment 349458 [details]
Patch

[Dean Jackson]

Created attachment 349461 [details]
Build Test

[Simon Fraser (smfr)]

Comment on attachment 349458 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349458&action=review

> Source/WebKit/Scripts/PreferencesTemplates/WebPreferencesInternalDebugFeatures.cpp.erb:2
> +* THIS FILE WAS AUTOMATICALLY GENERATED, DO NOT EDIT.

Lies.

> Source/WebKit/UIProcess/API/Cocoa/WKPreferences.mm:552
> +// FIXME: Remove this once Safari has adopted the new API.

What new API?

> Source/WebKit/UIProcess/API/Cocoa/WKPreferences.mm:558
> +// FIXME: Remove this once Safari has adopted the new API.

What new API?

> Source/WebKit/UIProcess/API/Cocoa/_WKInternalDebugFeature.mm:35
> +    _internalDebugFeature->API::InternalDebugFeature::~InternalDebugFeature();

Doesn't this happen automatically yet?

> Source/WebKit/UIProcess/WebPreferences.h:68
> +    bool isEnabledForFeature(const API::InternalDebugFeature&) const;

This could be isFeatureEnabled().

> Source/WebKit/UIProcess/WebPreferences.h:69
> +    void setEnabledForFeature(bool, const API::InternalDebugFeature&);

This could be setFeatureEnabled()

> Tools/MiniBrowser/mac/SettingsController.m:251
> +    [internalDebugFeaturesSubmenuItem release];
> +    [internalDebugFeaturesMenu release];

I guess this isn't ARC'ed yet?

[Dean Jackson]

Comment on attachment 349458 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349458&action=review

>> Source/WebKit/Scripts/PreferencesTemplates/WebPreferencesInternalDebugFeatures.cpp.erb:2
>> +* THIS FILE WAS AUTOMATICALLY GENERATED, DO NOT EDIT.
> 
> Lies.

Yeah. It's a bit silly that the ruby script doesn't insert this text, rather than have it in the template.

>> Source/WebKit/UIProcess/API/Cocoa/WKPreferences.mm:552
>> +// FIXME: Remove this once Safari has adopted the new API.
> 
> What new API?

EnabledForFeature is not EnabledForExperimentalFeature. I have a patch coming that updates Safari. Once that lands, these implementations can be removed.

>> Source/WebKit/UIProcess/API/Cocoa/_WKInternalDebugFeature.mm:35
>> +    _internalDebugFeature->API::InternalDebugFeature::~InternalDebugFeature();
> 
> Doesn't this happen automatically yet?

Nope.

>> Source/WebKit/UIProcess/WebPreferences.h:68
>> +    bool isEnabledForFeature(const API::InternalDebugFeature&) const;
> 
> This could be isFeatureEnabled().

Yep. I'd have to stage the changes again.

[Simon Fraser (smfr)]

(In reply to Dean Jackson from comment #12)
> Comment on attachment 349458 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349458&action=review
> 
> >> Source/WebKit/Scripts/PreferencesTemplates/WebPreferencesInternalDebugFeatures.cpp.erb:2
> >> +* THIS FILE WAS AUTOMATICALLY GENERATED, DO NOT EDIT.
> > 
> > Lies.
> 
> Yeah. It's a bit silly that the ruby script doesn't insert this text, rather
> than have it in the template.

It us.

> >> Source/WebKit/UIProcess/API/Cocoa/WKPreferences.mm:552
> >> +// FIXME: Remove this once Safari has adopted the new API.
> > 
> > What new API?
> 
> EnabledForFeature is not EnabledForExperimentalFeature. I have a patch
> coming that updates Safari. Once that lands, these implementations can be
> removed.

I'd like to propose a new API. The current one is really cumbersome, since it requires enumeration of all the features to switch any of them.

New API:

- (NSArray<NSString *> *)experimentalFeatureKeys; // Or maybe return an NSSet
- (NSDictionary*)featureDetailsForKey:(NSString *)key; // Dict with some specified keys
- (BOOL)isExperimentalFeatureWithKeyEnabled:(NSString *)key;
- (void)setExperimentalFeatureWithKey:(NSString *)key enabled:(BOOL)enabled;

[Simon Fraser (smfr)]

Comment on attachment 349461 [details]
Build Test

View in context: https://bugs.webkit.org/attachment.cgi?id=349461&action=review

> Source/WebKit/Shared/WebPreferences.yaml:1257
> +  category: experimental

I think this should be internal.

> Source/WebKit/UIProcess/API/APIInternalDebugFeature.cpp:45
> +InternalDebugFeature::~InternalDebugFeature()

= default

[Dean Jackson]

Committed r235921: <https://trac.webkit.org/changeset/235921>

[Truitt Savell]

Looks like https://trac.webkit.org/changeset/235921

has caused these 4 tests to fail continuously:

http/tests/webAPIStatistics/navigator-functions-accessed-data-collection.html
http/tests/webAPIStatistics/canvas-read-and-write-data-collection.html
http/tests/webAPIStatistics/font-load-data-collection.html
http/tests/webAPIStatistics/screen-functions-accessed-data-collection.html

Combined test history:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2FwebAPIStatistics%2Fnavigator-functions-accessed-data-collection.html%20http%2Ftests%2FwebAPIStatistics%2Fcanvas-read-and-write-data-collection.html%20http%2Ftests%2FwebAPIStatistics%2Ffont-load-data-collection.html%20http%2Ftests%2FwebAPIStatistics%2Fscreen-functions-accessed-data-collection.html

[Chris Dumez]

Comment on attachment 349461 [details]
Build Test

View in context: https://bugs.webkit.org/attachment.cgi?id=349461&action=review

> Source/WebKit/Shared/WebPreferences.yaml:1278
> +  category: internal

How can I turn on my feature now?

[Chris Dumez]

Comment on attachment 349461 [details]
Build Test

View in context: https://bugs.webkit.org/attachment.cgi?id=349461&action=review

>> Source/WebKit/Shared/WebPreferences.yaml:1278
>> +  category: internal
> 
> How can I turn on my feature now?

And why was this removed from experimental given that we'd like people to try it out with STP?

[Anders Carlsson]

Did you intend to keep this first patch maked review?

[Frédéric Wang (:fredw)]

Comment on attachment 349461 [details]
Build Test

View in context: https://bugs.webkit.org/attachment.cgi?id=349461&action=review

> Source/WebKit/Shared/WebPreferences.yaml:1294
> +  category: internal

This was moved to experimental, but is exposed in the iOS experimental WebKit features menu (iOS 12.1.1 beta). Is it intentional?

> Source/WebKit/Shared/WebPreferences.yaml:1302
> +  category: internal

Ditto.

> Source/WebKit/Shared/WebPreferences.yaml:1312
> +  category: internal

Ditto.

> Source/WebKit/Shared/WebPreferences.yaml:1328
> +  category: internal

Ditto.

> Source/WebKit/Shared/WebPreferences.yaml:1337
> +  category: internal

Ditto.

> Source/WebKit/Shared/WebPreferences.yaml:1347
> +  category: internal

Ditto.

[Frédéric Wang (:fredw)]

Comment on attachment 349461 [details]
Build Test

View in context: https://bugs.webkit.org/attachment.cgi?id=349461&action=review

>> Source/WebKit/Shared/WebPreferences.yaml:1294
>> +  category: internal
> 
> This was moved to experimental, but is exposed in the iOS experimental WebKit features menu (iOS 12.1.1 beta). Is it intentional?

Moved to *internal*

[Tim Horton]

(In reply to Frédéric Wang (:fredw) from comment #20)
> Comment on attachment 349461 [details]
> Build Test
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349461&action=review
> 
> > Source/WebKit/Shared/WebPreferences.yaml:1294
> > +  category: internal
> 
> This was moved to experimental, but is exposed in the iOS experimental
> WebKit features menu (iOS 12.1.1 beta). Is it intentional?

Insert usual spiel about Apple not commenting on timing/content of future releases. But no, this change isn’t in that beta.