Bug 195254 - Share more code for updating the state of frame scrolling nodes
Summary: Share more code for updating the state of frame scrolling nodes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks: 195258
  Show dependency treegraph
 
Reported: 2019-03-02 20:32 PST by Simon Fraser (smfr)
Modified: 2019-04-03 13:54 PDT (History)
8 users (show)

See Also:


Attachments
Patch (63.67 KB, patch)
2019-03-02 20:37 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.56 MB, application/zip)
2019-03-02 21:58 PST, Build Bot
no flags Details
Patch (65.67 KB, patch)
2019-03-03 09:12 PST, Simon Fraser (smfr)
koivisto: review+
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (18.07 MB, application/zip)
2019-03-03 11:30 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2019-03-02 20:32:12 PST
Share more code for updating the state of frame scrolling nodes
Comment 1 Simon Fraser (smfr) 2019-03-02 20:37:43 PST
Created attachment 363440 [details]
Patch
Comment 2 Build Bot 2019-03-02 21:58:23 PST Comment hidden (obsolete)
Comment 3 Build Bot 2019-03-02 21:58:24 PST Comment hidden (obsolete)
Comment 4 Simon Fraser (smfr) 2019-03-03 09:12:56 PST
Created attachment 363458 [details]
Patch
Comment 5 Build Bot 2019-03-03 11:30:36 PST
Comment on attachment 363458 [details]
Patch

Attachment 363458 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11353961

New failing tests:
scrollingcoordinator/scrolling-tree/fixed-inside-frame.html
Comment 6 Build Bot 2019-03-03 11:30:38 PST
Created attachment 363461 [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.13.6
Comment 7 Antti Koivisto 2019-03-03 11:46:31 PST
Comment on attachment 363458 [details]
Patch

r=me with the iOS test fixed
Comment 8 Simon Fraser (smfr) 2019-03-04 08:22:06 PST
http://trac.webkit.org/r242333
Comment 9 Radar WebKit Bug Importer 2019-03-04 08:25:36 PST
<rdar://problem/48562627>
Comment 10 Frédéric Wang (:fredw) 2019-03-05 06:15:43 PST
Comment on attachment 363458 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        ScrollingStateOverflowScrollingNodews, since both can be updated from a ScrollableArea.

too late, but s/ScrollingStateOverflowScrollingNodews/ScrollingStateOverflowScrollingNodes/

> Source/WebCore/ChangeLog:18
> +        on macOS.

Does this patch add rubber-stamping support on macOS or only prepares the support? In the former case, I think we need a test.
Comment 11 Frédéric Wang (:fredw) 2019-03-05 06:47:56 PST
Comment on attachment 363458 [details]
Patch

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

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:133
>          return;

frameView is no longer used on other platforms, so we should add ASSERT_UNUSED.
Comment 12 Frédéric Wang (:fredw) 2019-03-05 06:54:23 PST
Committed r242465: <https://trac.webkit.org/changeset/242465>
Comment 13 Simon Fraser (smfr) 2019-03-05 09:33:26 PST
(In reply to Frédéric Wang (:fredw) from comment #12)
> Committed r242465: <https://trac.webkit.org/changeset/242465>

Thanks for the fix.

I agree a test would be good (also a test that shows that scrollbars become visible), but overflow scrolling on macOS isn't my focus now, and we're debating whether we'll enable the threaded overflow scroll on macOS, so I didn't want to invest a lot of time in it.
Comment 14 Shawn Roberts 2019-04-03 13:54:07 PDT
This change caused scrollingcoordinator/scrolling-tree/fixed-inside-frame.html to fail.

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=scrollingcoordinator%2Fscrolling-tree%2Ffixed-inside-frame.html

Did not notice before because I had marked this test flaky prior to this change. 

I rebaselined the test in https://trac.webkit.org/changeset/243825/webkit to fix it.