Bug 183081 – Relayout frames after AsyncFrameScrolling or FrameFlattening option is changed

• Status: RESOLVED
• Resolution: FIXED
• Component: Frames

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

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

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

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

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

Created attachment 334607 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

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

Created attachment 334616 [details]
Patch for EWS

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

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

Created attachment 334679 [details]
Patch

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

Created attachment 334680 [details]
Patch

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

Committed r229060: <https://trac.webkit.org/changeset/229060>

[Radar WebKit Bug Importer]

<rdar://problem/37940015>

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

@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!

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

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

[Alexey Proskuryakov]

This made some tests assert more than before, notably:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&showExpectations=true&revision=229045&tests=imported%2Fmozilla%2Fsvg%2Fmask-transformed-01.svg

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fmozilla%2Fsvg%2FforeignObject-ancestor-style-change-01.svg

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

Thanks Alexey, I'll check this.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 183198

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

Created attachment 334734 [details]
Patch

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

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.

[Antonio Gomes]

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.

[Antonio Gomes]

(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

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

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

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

Committed r229361: <https://trac.webkit.org/changeset/229361>