Created attachment 310369 [details] Patch

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

Comment on attachment 310369 [details] Patch This patch applies on to of bug 171914.

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