Bug 164905

Summary: Make it possible to test non-stable-state scrolling on iOS
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Tools / TestsAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: koivisto, lforschler, ossy, simon.fraser, thorton
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 164967    
Bug Blocks: 164855    
Attachments:
Description Flags
Patch
none
Patch
none
Patch mitz: review+

Description Simon Fraser (smfr) 2016-11-17 19:49:45 PST
I need to be able to test scrolling behavior (tile coverage, scrolling tree state) when the viewport is not in a stable state, to simulate what happens in the middle of scrolling or zooming.
Comment 1 Simon Fraser (smfr) 2016-11-17 19:50:20 PST
I have a patch but the associated layer tree dump, with visible and coverage rects, is flakey.
Comment 2 Simon Fraser (smfr) 2016-11-17 19:59:43 PST
The source of the flakiness is that sometimes TileController::adjustTileCoverageRect() has CoverageForVisibleArea and sometimes CoverageForVerticalScrolling. That's affected by FrameView::speculativeTilingEnabled() which is in turn affected by FrameView::wasScrolledByUser(). FrameView:: wasScrolledByUser() is being set via EventHandler::sendScrollEvent() (why?). This is called from FrameView::scrollPositionChanged(), which uses the eventThrottlingDelay() from WebKit2.

There are also timers at various points in this chain. It's all very hard to get stable for testing.
Comment 3 Simon Fraser (smfr) 2016-11-17 20:04:05 PST
In my particular case m_estimatedLatency is 16ms so we decide to send a scroll event, setting FrameView's scrolledByUser, which then triggers additional tile coverage.
Comment 4 Simon Fraser (smfr) 2016-11-17 21:04:21 PST
Created attachment 295133 [details]
Patch
Comment 5 Simon Fraser (smfr) 2016-11-17 22:16:48 PST
Created attachment 295135 [details]
Patch
Comment 6 Simon Fraser (smfr) 2016-11-18 19:01:14 PST
Need to redo parts of this patch after bug 164980.
Comment 7 Simon Fraser (smfr) 2016-11-19 10:01:00 PST
Created attachment 295264 [details]
Patch
Comment 8 mitz 2016-11-19 10:22:49 PST
Comment on attachment 295264 [details]
Patch

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

> Source/WebKit2/ChangeLog:11
> +        scrolling and layer trees in these transients states.

“transients”?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:293
> +@property (nonatomic, setter=_setStableStateOverride:) NSNumber *_stableStateOverride WK_API_AVAILABLE(ios(WK_IOS_TBA));

Can be readonly in this class, with subclasses redeclaring as readwrite.

> Tools/WebKitTestRunner/cocoa/TestRunnerWKWebView.mm:50
> +@property (nonatomic, copy) NSNumber *testRunnerStableStateOverride;

Strange to add this level of indirection. Can just declare a RetainPtr<NSNumber> ivar to back the existing property.

> Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm:63
> +    [webView _doAfterNextPresentationUpdate:^ {

Extra space after the ^, and I thought we preferred C++ lambdas with explicit capture over Objective-C blocks.

> Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm:423
> +    if (overrideValue)
> +        webView._stableStateOverride = @(overrideValue.value());
> +    else
> +        webView._stableStateOverride = nil;

Can be written more succinctly with a ternary operator.
Comment 9 Simon Fraser (smfr) 2016-11-19 11:02:34 PST
https://trac.webkit.org/changeset/208926
Comment 10 Csaba Osztrogonác 2016-11-19 12:46:15 PST
(In reply to comment #9)
> https://trac.webkit.org/changeset/208926

It broke the Apple Mac build on all buildbots, see build.webkit.org for details.
Comment 11 Simon Fraser (smfr) 2016-11-19 12:51:00 PST
Fixing.