Add runtime flag for partial frame flattening

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

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

In bug 171914, the frame flattening setting is changed to a three-states enum: disabled, partially enabled and fully enabled. In order to easily test partial frame flattening, we should expose a runtime flag that would force the value of frame flattening to "partially enabled".

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

Created attachment 310369 [details]
Patch

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

Created attachment 310372 [details]
Patch (171914+172107)

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

Comment on attachment 310369 [details]
Patch

This patch applies on to of bug 171914.

[Antonio Gomes]

Comment on attachment 310372 [details]
Patch (171914+172107)

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

A couple of comments, mostly about naming.

> LayoutTests/ChangeLog:29
> [phone] Frederic Wang  <[e-mail]>
> +
> +        Add heuristic to avoid flattening "fullscreen" iframes
> +        https://bugs.webkit.org/show_bug.cgi?id=171914
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        This commit adjusts tests to work when frame flattening is an enum.
> +        It also adds a test to check the new heuristic when "partial frame flattening" is enabled.
> +        set-preference.html is disabled for now, as the test suite does not support overridePreference()
> +        for non-boolean values (bug 128594).
> +
> +        * fast/forms/ios/delete-in-input-in-iframe.html: Use enum value "FullyEnabled".

nit: duplicated changelog entry

> LayoutTests/fast/frames/flattening/set-experimental-feature-force-partial-frame-flattening.html:11
> +        internals.settings.setFrameFlattening("FullyEnabled");
> +        internals.settings.setForcePartialFrameFlatteningEnabled(true);

for readers of these APIs, it might look a bit strange to see a call to "enable frame flattening fully" followed by a call to "force enabled partial frame flattening".

> LayoutTests/fast/frames/flattening/set-experimental-feature-force-partial-frame-flattening.html:22
> +        test(function() { assert_false(isUnflatten("iframe0")); }, "simple iframe");

nit: it seems more common to use word separators like "_" or "-", or camel case ID strings, rather than space separated words.

> Source/WebCore/page/RuntimeEnabledFeatures.h:202
> +    void setForcePartialFrameFlatteningEnabled(bool isEnabled) { m_forcePartialFrameFlatteningEnabled = isEnabled; }
> +    bool forcePartialFrameFlatteningEnabled() const { return m_forcePartialFrameFlatteningEnabled; }

I think a more widely used terminology for similar cases is 'override'?

so force -> override: OverrideFrameFlatteningPolicy(bool enable_by_heuristics); ?

and add a comment if this is a temporary API?

> Source/WebCore/page/Settings.h:91
> +enum FrameFlattening {

what about FrameFlatteningPolicy { FullyEnabled, PartiallyEnabled, Disabled };

e.g.:

settings->SetFrameFlatenningPolicy(FullyEnabled);

too verbose?

> Source/WebCore/page/Settings.h:335
> +    WEBCORE_EXPORT FrameFlattening actualFrameFlattening();

... and name the getter frameFlatteningPolicy?

[Simon Fraser (smfr)]

Comment on attachment 310369 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        In bug 171914, the frame flattening setting is changed to a three-states enum: disabled,
> +        partially enabled and fully enabled. In order to easily test partial frame flattening, this
> +        commit introduce a runtime flag forcing frame flattening to "partially enabled".

Why can't the tests just change the Settting? I don't see the new for a runtime enabled feature for this.

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

Comment on attachment 310369 [details]
Patch

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

>> Source/WebCore/ChangeLog:10
>> +        commit introduce a runtime flag forcing frame flattening to "partially enabled".
> 
> Why can't the tests just change the Settting? I don't see the new for a runtime enabled feature for this.

I think the idea was to have this feature for developers and beta testers to easily experiment the feature. I see runtime flags appear in a menu on macOS (and safari tech preview) but I'm not sure how it would work on iOS?

Anyway, maybe I did not understand your comment here:
https://bugs.webkit.org/show_bug.cgi?id=171914#c3
"It would be nice to have this an an Experimental Feature (in the WebKit2 sense, like WebGPU)."
What did you mean?

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

Comment on attachment 310369 [details]
Patch

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

>>> Source/WebCore/ChangeLog:10
>>> +        commit introduce a runtime flag forcing frame flattening to "partially enabled".
>> 
>> Why can't the tests just change the Settting? I don't see the new for a runtime enabled feature for this.
> 
> I think the idea was to have this feature for developers and beta testers to easily experiment the feature. I see runtime flags appear in a menu on macOS (and safari tech preview) but I'm not sure how it would work on iOS?
> 
> Anyway, maybe I did not understand your comment here:
> https://bugs.webkit.org/show_bug.cgi?id=171914#c3
> "It would be nice to have this an an Experimental Feature (in the WebKit2 sense, like WebGPU)."
> What did you mean?

After discussion with Simon, it seems we do not want "runtime flag" (deprecated) just "experimental feature" (similar to SpringTimingFunctionEnabled). So I'm going to WONTFIX this.