Bug 192395

Summary: Allow control over child order when adding nodes to the scrolling tree
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: New BugsAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: ews, fred.wang, rniwa, simon.fraser
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=176914
Bug Depends on: 192398    
Bug Blocks:    
Attachments:
Description Flags
Patch
fred.wang: review-, ews: commit-queue-
Archive of layout-test-results from ews105 for mac-sierra-wk2
none
Patch (same as 356581)
ews: commit-queue-
Archive of layout-test-results from ews107 for mac-sierra-wk2
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Attachment 356581, rebased on top of bug 192398 none

Description Simon Fraser (smfr) 2018-12-04 21:57:41 PST
Allow control over child order when adding nodes to the scrolling tree
Comment 1 Simon Fraser (smfr) 2018-12-04 21:59:54 PST
Created attachment 356581 [details]
Patch
Comment 2 Build Bot 2018-12-04 23:13:26 PST
Comment on attachment 356581 [details]
Patch

Attachment 356581 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10274669

New failing tests:
fast/visual-viewport/tiled-drawing/zoomed-fixed-scrolling-layers-state.html
Comment 3 Build Bot 2018-12-04 23:13:27 PST
Created attachment 356584 [details]
Archive of layout-test-results from ews105 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 4 Frédéric Wang (:fredw) 2018-12-05 00:17:11 PST
This seems similar to what I tried in https://bugs.webkit.org/attachment.cgi?id=327061&action=review
Comment 5 Frédéric Wang (:fredw) 2018-12-05 00:27:01 PST
Trying to fix the EWS build error on iOS:

https://trac.webkit.org/changeset/238889/webkit
Comment 6 Frédéric Wang (:fredw) 2018-12-05 00:30:38 PST
Created attachment 356589 [details]
Patch (same as 356581)

resending to EWS
Comment 7 Build Bot 2018-12-05 01:46:09 PST
Comment on attachment 356589 [details]
Patch (same as 356581)

Attachment 356589 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10275650

New failing tests:
fast/visual-viewport/tiled-drawing/zoomed-fixed-scrolling-layers-state.html
Comment 8 Build Bot 2018-12-05 01:46:11 PST
Created attachment 356591 [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 9 Build Bot 2018-12-05 02:07:59 PST
Comment on attachment 356589 [details]
Patch (same as 356581)

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

Number of test failures exceeded the failure limit.
Comment 10 Build Bot 2018-12-05 02:08:01 PST
Created attachment 356592 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 11 Frédéric Wang (:fredw) 2018-12-05 04:49:37 PST
Comment on attachment 356589 [details]
Patch (same as 356581)

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

> Source/WebCore/page/scrolling/ScrollingStateTree.h:-30
> -#include "ScrollingStateFrameScrollingNode.h"

OK, I thought what I fixed in comment 5 was again due to rotating unified build but it seems it was actually this change. I guess several cleanup changes in this patch could be handled outside to facilitate review.

> Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp:416
> +        m_scrollingStateTree->attachNode(nodeType, nodeID, parentNodeID, i);

This seems wrong since numNodes is the size of the whole scrolling state tree not of the child list. I guess that's why I had tried to encode the child index in https://bugs.webkit.org/attachment.cgi?id=327061&action=review
Comment 12 Frédéric Wang (:fredw) 2018-12-05 06:00:46 PST
Created attachment 356599 [details]
Attachment 356581 [details], rebased on top of bug 192398

This is the patch rebased on top of bug 192398, but I think my comment 11 regarding RemoteScrollingCoordinatorTransaction still holds.
Comment 13 Simon Fraser (smfr) 2018-12-05 08:27:08 PST
Sorry for the duplicate patch; let's work on your version.

*** This bug has been marked as a duplicate of bug 176914 ***
Comment 14 Frédéric Wang (:fredw) 2018-12-05 08:41:42 PST
(In reply to Simon Fraser (smfr) from comment #13)
> Sorry for the duplicate patch; let's work on your version.
> 
> *** This bug has been marked as a duplicate of bug 176914 ***

OK, my patch is from one year ago though so probably we could just work from your version. BTW, I extracted the minor refactoring parts in bug 192398.