Bug 171914

Created attachment 309587 [details] Patch

@Simon: I had two questions about this: 1) Maybe (as suggested by Antonio) the settings should be called FrameFlatteningExtraHeuristicsEnabled in case more heuristics are added? 2) Is there a convenient way to enable/disable the option during development? Is it necessary to add it to the context menu of experimental features for that purpose?

(In reply to Frédéric Wang (:fredw) from comment #2) > @Simon: I had two questions about this: > > 1) Maybe (as suggested by Antonio) the settings should be called > FrameFlatteningExtraHeuristicsEnabled in case more heuristics are added? I don't like "FrameFlatteningExtraHeuristicsEnabled" because it doesn't indicate that frames are NOT flattened according to the heuristics. Maybe we should have: frameFlatteningEnabled partialFrameFlatteningEnabled where frameFlatteningEnabled is the current behavior, and partialFrameFlatteningEnabled is new behavior with heuristics. frameFlatteningEnabled would trump partialFrameFlatteningEnabled. > 2) Is there a convenient way to enable/disable the option during > development? Is it necessary to add it to the context menu of experimental > features for that purpose? It would be nice to have this an an Experimental Feature (in the WebKit2 sense, like WebGPU).

Created attachment 309897 [details] Patch (use an enum for FrameFlattening)

Created attachment 309898 [details] Patch (use an enum for FrameFlattening)

Attachment 309898 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/API/C/WKAPICast.h:344: Non-label code inside switch statements should be indented. [whitespace/indent] [4] ERROR: Source/WebKit2/UIProcess/API/C/WKAPICast.h:344: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit2/UIProcess/API/C/WKAPICast.h:346: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit2/UIProcess/API/C/WKAPICast.h:348: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 4 in 59 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 309898 [details] Patch (use an enum for FrameFlattening) Attachment 309898 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3726675 New failing tests: fast/frames/flattening/iframe-flattening-crash.html

Created attachment 309904 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Comment on attachment 309898 [details] Patch (use an enum for FrameFlattening) Attachment 309898 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3726732 New failing tests: platform/mac/fast/frames/flattening/set-preference.html fast/frames/flattening/iframe-flattening-crash.html

Created attachment 309906 [details] Archive of layout-test-results from ews103 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 309898 [details] Patch (use an enum for FrameFlattening) Attachment 309898 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3726786 New failing tests: platform/mac/fast/frames/flattening/set-preference.html fast/frames/flattening/iframe-flattening-crash.html

Created attachment 309910 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 309898 [details] Patch (use an enum for FrameFlattening) Attachment 309898 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3726797 New failing tests: fast/frames/flattening/iframe-flattening-crash.html

Created attachment 309912 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

Comment on attachment 309898 [details] Patch (use an enum for FrameFlattening) View in context: https://bugs.webkit.org/attachment.cgi?id=309898&action=review > LayoutTests/plugins/frameset-with-plugin-frame.html:6 > + internals.settings.setFrameFlattening(2); // Full Frame Flattening Can we put some consts in the IDL, or use an IDL enum?

Created attachment 310019 [details] Patch (use an enum for FrameFlattening)

Created attachment 310083 [details] Patch (use an enum for FrameFlattening)

Created attachment 310112 [details] Patch

Comment on attachment 310112 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310112&action=review > LayoutTests/fast/frames/flattening/iframe-flattening-fullscreen.html:22 > + test(function() { assert_true(!isUnflatten("iframe2")); }, "iframe with vw/vh units"); note to self: these should probably be assert_false(isUnflatten)

Comment on attachment 310112 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310112&action=review > Source/WebCore/page/Settings.h:93 > + FrameFlatteningPartiallyEnabled, Can we qualify "partially"? What kind of frames still get flattened? > Source/WebCore/rendering/RenderIFrame.cpp:89 > + if (style().hasOutOfFlowPosition() && style().hasViewportUnits()) > + return false; Maybe move this code into a separate function, since I imagine these heuristics will evolve. The name of the function should reflect the enum value that is currently "FrameFlatteningPartiallyEnabled". > Source/WebKit/mac/WebView/WebPreferencesPrivate.h:163 > +- (unsigned)frameFlattening; > +- (void)setFrameFlattening:(unsigned)flag; unsigned -> WebKitFrameFlattening. Actually I think you need to leave the existing functions alone, for API compatibility, but still add the new ones. > Tools/DumpRenderTree/mac/DumpRenderTree.mm:906 > + [preferences setFrameFlattening:NO]; This should use the enum, not a BOOL.

Created attachment 313265 [details] Patch for landing

Comment on attachment 313265 [details] Patch for landing Clearing flags on attachment: 313265 Committed r218480: <http://trac.webkit.org/changeset/218480>

All reviewed patches have been landed. Closing bug.