Bug 183081 - Relayout frames after AsyncFrameScrolling or FrameFlattening option is changed
Summary: Relayout frames after AsyncFrameScrolling or FrameFlattening option is changed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords: InRadar
Depends on: 173704 183198
Blocks: 149264 171667
  Show dependency treegraph
 
Reported: 2018-02-23 06:09 PST by Frédéric Wang (:fredw)
Modified: 2018-03-07 09:11 PST (History)
12 users (show)

See Also:


Attachments
Patch (WIP) (1.96 KB, patch)
2018-02-23 06:09 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.68 MB, application/zip)
2018-02-23 07:21 PST, Build Bot
no flags Details
Patch (8.13 KB, patch)
2018-02-26 03:52 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.63 MB, application/zip)
2018-02-26 05:03 PST, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-sierra (2.98 MB, application/zip)
2018-02-26 05:21 PST, Build Bot
no flags Details
Patch for EWS (14.82 KB, patch)
2018-02-26 07:51 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.29 MB, application/zip)
2018-02-26 08:56 PST, Build Bot
no flags Details
Archive of layout-test-results from ews202 for win-future (11.63 MB, application/zip)
2018-02-26 09:33 PST, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-sierra (3.03 MB, application/zip)
2018-02-26 09:37 PST, Build Bot
no flags Details
Archive of layout-test-results from ews205 for win-future (11.54 MB, application/zip)
2018-02-26 09:51 PST, Build Bot
no flags Details
Patch (8.14 KB, patch)
2018-02-27 01:28 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (7.31 KB, patch)
2018-02-27 01:36 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (8.67 KB, patch)
2018-02-28 02:50 PST, Frédéric Wang (:fredw)
tonikitoo: review+
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) 2018-02-23 06:09:52 PST
Created attachment 334524 [details]
Patch (WIP)

Changing AsyncFrameScrolling may change frame flattening after bug 173704 and hence may require a relayout. 
Maybe we also need to update scrolling tree and related stuff when enabling/disabling async frame scrolling.
  
Note that disabling AsyncFrameScrolling via the iOS UI does not take effect immediately, because of bug 182485.
Comment 1 Build Bot 2018-02-23 07:21:43 PST
Comment on attachment 334524 [details]
Patch (WIP)

Attachment 334524 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6639860

New failing tests:
compositing/tiling/tiled-drawing-async-frame-scrolling.html
Comment 2 Build Bot 2018-02-23 07:21:44 PST
Created attachment 334529 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 3 Frédéric Wang (:fredw) 2018-02-23 09:47:55 PST
(In reply to Frédéric Wang (:fredw) from comment #0)
> Maybe we also need to update scrolling tree and related stuff when
> enabling/disabling async frame scrolling.

Testing with the iOS frame scrolling patches, forcing a relayout seems enough for that.
Comment 4 Frédéric Wang (:fredw) 2018-02-26 03:52:23 PST
Created attachment 334607 [details]
Patch
Comment 5 Build Bot 2018-02-26 05:03:50 PST
Comment on attachment 334607 [details]
Patch

Attachment 334607 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6674211

New failing tests:
compositing/tiling/tiled-drawing-async-frame-scrolling.html
fast/frames/flattening/frameset-flattening-simple.html
Comment 6 Build Bot 2018-02-26 05:03:51 PST
Created attachment 334613 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 7 Build Bot 2018-02-26 05:21:37 PST
Comment on attachment 334607 [details]
Patch

Attachment 334607 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6674227

New failing tests:
fast/frames/flattening/non-flattening-frame-inside-flattening-iframe-crash.html
Comment 8 Build Bot 2018-02-26 05:21:38 PST
Created attachment 334614 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 9 Frédéric Wang (:fredw) 2018-02-26 07:51:10 PST
Created attachment 334616 [details]
Patch for EWS
Comment 10 Build Bot 2018-02-26 07:54:03 PST
Attachment 334616 [details] did not pass style-queue:


ERROR: Source/WebCore/page/SettingsBase.cpp:278:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/page/SettingsBase.cpp:279:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/page/SettingsBase.cpp:280:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/page/SettingsBase.cpp:281:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/page/SettingsBase.cpp:283:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/page/SettingsBase.cpp:284:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Build Bot 2018-02-26 08:56:47 PST
Comment on attachment 334616 [details]
Patch for EWS

Attachment 334616 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6675969

New failing tests:
fast/frames/flattening/frameset-flattening-simple.html
Comment 12 Build Bot 2018-02-26 08:56:48 PST
Created attachment 334619 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 13 Build Bot 2018-02-26 09:33:00 PST
Comment on attachment 334616 [details]
Patch for EWS

Attachment 334616 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6676215

New failing tests:
fast/frames/flattening/frameset-flattening-simple.html
Comment 14 Build Bot 2018-02-26 09:33:11 PST
Created attachment 334622 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 15 Build Bot 2018-02-26 09:37:49 PST
Comment on attachment 334616 [details]
Patch for EWS

Attachment 334616 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6676120

New failing tests:
fast/frames/flattening/non-flattening-frame-inside-flattening-iframe-crash.html
fast/frames/flattening/frameset-flattening-simple.html
Comment 16 Build Bot 2018-02-26 09:37:50 PST
Created attachment 334624 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 17 Build Bot 2018-02-26 09:50:51 PST
Comment on attachment 334607 [details]
Patch

Attachment 334607 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6676388

New failing tests:
fast/frames/flattening/frameset-flattening-simple.html
Comment 18 Build Bot 2018-02-26 09:51:01 PST
Created attachment 334625 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 19 Frédéric Wang (:fredw) 2018-02-27 01:28:36 PST
Created attachment 334679 [details]
Patch
Comment 20 Frédéric Wang (:fredw) 2018-02-27 01:36:21 PST
Created attachment 334680 [details]
Patch
Comment 21 Frédéric Wang (:fredw) 2018-02-27 06:00:11 PST
Committed r229060: <https://trac.webkit.org/changeset/229060>
Comment 22 Radar WebKit Bug Importer 2018-02-27 06:03:07 PST
<rdar://problem/37940015>
Comment 23 Frédéric Wang (:fredw) 2018-02-27 06:30:41 PST
@Don: The WinCairo bot is complaining about Settings::setFrameFlattening https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/11231/steps/compile-webkit/logs/stdio but the EWS was building correctly. I wonder whether it's because you need to force a clean rebuild on the bot as that happened in a previous bug? Can you please check? Thanks!
Comment 24 Frédéric Wang (:fredw) 2018-02-27 08:45:32 PST
(In reply to Frédéric Wang (:fredw) from comment #23)
> @Don: The WinCairo bot is complaining about Settings::setFrameFlattening
> https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/11231/
> steps/compile-webkit/logs/stdio but the EWS was building correctly. I wonder
> whether it's because you need to force a clean rebuild on the bot as that
> happened in a previous bug? Can you please check? Thanks!

Please ignore this. The bot is green again.
Comment 26 Frédéric Wang (:fredw) 2018-02-27 23:43:58 PST
Thanks Alexey, I'll check this.
Comment 27 WebKit Commit Bot 2018-02-27 23:46:25 PST
Re-opened since this is blocked by bug 183198
Comment 28 Frédéric Wang (:fredw) 2018-02-28 02:50:24 PST
Created attachment 334734 [details]
Patch
Comment 29 Frédéric Wang (:fredw) 2018-02-28 02:55:51 PST
Comment on attachment 334680 [details]
Patch

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

> Source/WebCore/page/SettingsBase.cpp:285
> +            frameView->layoutContext().scheduleLayout();

This was not needed since setNeedsLayout actually takes care of scheduling the layout. It was also causing the assert for the two SVG tests.
Comment 30 Antonio Gomes 2018-03-07 05:01:46 PST
Comment on attachment 334734 [details]
Patch

r=me

lets please follow on to make sure the svn failures were related to the trailing ScheduleLayout or something we were leaving over set between test runner instances, affecting the next tests.
Comment 31 Antonio Gomes 2018-03-07 05:09:41 PST
(In reply to Antonio Gomes from comment #30)
> Comment on attachment 334734 [details]
> Patch
> 
> r=me
> 
> lets please follow on to make sure the svn failures were related to the
> trailing ScheduleLayout or something we were leaving over set between test
> runner instances, affecting the next tests.

s/svn/svg/g
Comment 32 Frédéric Wang (:fredw) 2018-03-07 09:06:33 PST
(In reply to Antonio Gomes from comment #30)
> lets please follow on to make sure the svn failures were related to the
> trailing ScheduleLayout or something we were leaving over set between test
> runner instances, affecting the next tests.

I had verified the ASSERT happened on these SVG tests, even when running only a single one so it's not something left between test runner instances.

The foreignObject in these tests are subframes, so frame->ownerRenderer()->setNeedsLayoutAndPrefWidthsRecalc were called on them. Then it was wrong to force a relayout on the main frame myself: In LayoutContext::layout(), RenderTreeNeedsLayoutChecker was complaining that the subframes layout remained dirty. Instead, the calls to setNeedsLayoutAndPrefWidthsRecalc already takes care of scheduling the correct relayout.

There is a separate issue, though: setNeedsRelayoutAllFrames is always called via the following stack:
 
WebCore::SettingsBase::setNeedsRelayoutAllFrames()
WebCore::Settings::setAsyncFrameScrollingEnabled(bool)
WebKit::InjectedBundle::setAsyncFrameScrollingEnabled(WebKit::WebPageGroupProxy*, bool)
WKBundleSetAsyncFrameScrollingEnabled
WTR::InjectedBundle::beginTesting

It's not clear to me why because async frame scrolling is disabled by default and WKBundleSetAsyncFrameScrollingEnabled forces it to be disable, so Settings::setAsyncFrameScrollingEnabled should just ignore the change. I'll try to check why later, maybe it's not specific to async frame scrolling.
Comment 33 Frédéric Wang (:fredw) 2018-03-07 09:11:25 PST
Committed r229361: <https://trac.webkit.org/changeset/229361>