Bug 172107 - Add runtime flag for partial frame flattening
Summary: Add runtime flag for partial frame flattening
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords:
Depends on: 171914
Blocks: 149264
  Show dependency treegraph
 
Reported: 2017-05-15 01:05 PDT by Frédéric Wang (:fredw)
Modified: 2017-06-19 00:14 PDT (History)
3 users (show)

See Also:


Attachments
Patch (32.01 KB, patch)
2017-05-17 03:21 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (171914+172107) (80.97 KB, patch)
2017-05-17 03:22 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2017-05-15 01:05:08 PDT
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".
Comment 1 Frédéric Wang (:fredw) 2017-05-17 03:21:17 PDT
Created attachment 310369 [details]
Patch
Comment 2 Frédéric Wang (:fredw) 2017-05-17 03:22:56 PDT
Created attachment 310372 [details]
Patch (171914+172107)
Comment 3 Frédéric Wang (:fredw) 2017-05-17 05:53:01 PDT
Comment on attachment 310369 [details]
Patch

This patch applies on to of bug 171914.
Comment 4 Antonio Gomes 2017-05-19 08:35:42 PDT
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
> +2017-05-14 Frederic Wang  <fwang@igalia.com>
> +
> +        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?
Comment 5 Simon Fraser (smfr) 2017-06-15 10:50:10 PDT
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.
Comment 6 Frédéric Wang (:fredw) 2017-06-15 23:47:59 PDT
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?
Comment 7 Frédéric Wang (:fredw) 2017-06-19 00:13:48 PDT
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.