Bug 147791 – Simplify WKTR's view options

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Anders Carlsson]

Simplify WKTR's view options

[Anders Carlsson]

Created attachment 258519 [details]
Patch

[Anders Carlsson]

Created attachment 258523 [details]
Patch

[Anders Carlsson]

Committed r188157: <http://trac.webkit.org/changeset/188157>

[Alexey Proskuryakov]

Comment on attachment 258523 [details]
Patch

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

> Tools/WebKitTestRunner/mac/PlatformWebViewMac.mm:247
> -    WKRetainPtr<WKStringRef> useThreadedScrollingKey(AdoptWK, WKStringCreateWithUTF8CString("ThreadedScrolling"));
> -    WKTypeRef useThreadedScrollingValue = WKDictionaryGetItemForKey(options, useThreadedScrollingKey.get());
> -    bool useThreadedScrolling = useThreadedScrollingValue && WKBooleanGetValue(static_cast<WKBooleanRef>(useThreadedScrollingValue));
> +    if (m_options.useThreadedScrolling != options.useThreadedScrolling)
> +        return false;
>  
> -    return useThreadedScrolling == [(TestRunnerWKView *)m_view useThreadedScrolling];
> +    return true;

This changed the behavior, because the options structure doesn't have optional values any more, but it's used as if it did.

See e.g. here:

void TestController::updateLayoutTypeForTest(const TestInvocation& test)
{
    ViewOptions viewOptions;

    viewOptions.useFixedLayout = shouldUseFixedLayout(test);

    ensureViewSupportsOptions(viewOptions);
}

This results in WebKitTestRunner re-creating its view multiple times when running tests in tiled-drawing directory (which makes attaching to the process to debug super hard), ad likely in other misbehaviors.

This code turned out to be super fragile. Another example (pre-existing) is that TestController::configureViewForTest() indirectly calls ensureViewSupportsOptions multiple times, and if any of those decides to re-create the view, then any prior adjustments are lost.

While I agree with the premise of this patch, it seems more appropriate to start fresh than to further mutate from current state. I'll see if this can be easily reverted now.

[Alexey Proskuryakov]

Bummer, the rollout doesn't apply cleanly any more.

[Alexey Proskuryakov]

Anders, could you please take a look at this?

The known symptom is that when running any tiled-drawing test, we end up re-creating the view in TestController::ensureViewSupportsOptions. There may be other consequences, too.