Bug 204226 – Run AccessibilityController::rootElement on secondary thread to simulate HIServices during LayoutTests.

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

[Andres Gonzalez]

Run AccessibilityController::rootElement on secondary thread to simulate HIServices during LayoutTests.

[Andres Gonzalez]

Created attachment 383623 [details]
Patch

[chris fleizach]

Comment on attachment 383623 [details]
Patch

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

> Tools/ChangeLog:17
> +        served in the main thread in order to build the AXIsolatedTree.

How do we enable the secondary thread mode?

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:79
> +    void* root = nullptr;

is this really a void* still? can we change type to PlatformUIElement?

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:82
> +        dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);

it looks like there a WTF::BinarySemaphore that might be more platform independent

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:95
> +        m_useAXThread = true;

we don't want to set this for all requests do we?

[EWS Watchlist]

Comment on attachment 383623 [details]
Patch

Attachment 383623 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13258982

New failing tests:
animations/no-style-recalc-during-accelerated-animation.html

[EWS Watchlist]

Created attachment 383642 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Andres Gonzalez]

(In reply to chris fleizach from comment #2)
> Comment on attachment 383623 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383623&action=review
> 
> > Tools/ChangeLog:17
> > +        served in the main thread in order to build the AXIsolatedTree.
> 
> How do we enable the secondary thread mode?

The first time it runs, it will always be main thread. the second time and on, it would be secondary thread.
> 
> > Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:79
> > +    void* root = nullptr;
> 
> is this really a void* still? can we change type to PlatformUIElement?

Done.
> 
> > Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:82
> > +        dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);
> 
> it looks like there a WTF::BinarySemaphore that might be more platform
> independent

Done.
> 
> > Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:95
> > +        m_useAXThread = true;
> 
> we don't want to set this for all requests do we?

It is set to true after the first request is served in the main thread and the isolated tree is generated, so that all subsequent requests are served in the secondary thread.

[Andres Gonzalez]

Created attachment 383653 [details]
Patch

[chris fleizach]

Comment on attachment 383653 [details]
Patch

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

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:89
> +        root = static_cast<PlatformUIElement>(WKAccessibilityRootObject(page));

I think we'll need a way to distinguish from the test (or some other process, since ultimately we want to run every test in this mode and regular mode).

we won't be able to call into regular AccessibilityObject off the main thread (lots of main thread asserts)

[Andres Gonzalez]

Created attachment 383701 [details]
Patch

[Andres Gonzalez]

Added canUseSecondaryAXThread. It will return true when isolated tree is enabled and the client supports the isolated tree. Accessibility controller checks for this condition in order to spawn the secondary thread.

[Andres Gonzalez]

Created attachment 383702 [details]
Patch

[chris fleizach]

Comment on attachment 383702 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:717
> +    return clientSupportsIsolatedTree();

will clientSupportsIsolatedTree be true when running tests?

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:92
> +        m_useAXThread = true;

seems like we should only set this true (m_useAXThread) if WKAccessibilityCanUseSecondaryAXThread(page) is true

[Andres Gonzalez]

Created attachment 383782 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #11)
> Comment on attachment 383702 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383702&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:717
> > +    return clientSupportsIsolatedTree();
> 
> will clientSupportsIsolatedTree be true when running tests?

Yes, the AXClientType during LayoutTests is kAXClientTypeNoActiveRequestFound. So the assumption is no client request found, it is LayoutTests.

> 
> > Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:92
> > +        m_useAXThread = true;
> 
> seems like we should only set this true (m_useAXThread) if
> WKAccessibilityCanUseSecondaryAXThread(page) is true

Yes, fixed it. thanks.

[chris fleizach]

Comment on attachment 383782 [details]
Patch

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

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:92
> +            // Set m_useAXThread to true for next request.

if you put this comment above the if, then you can get rid of the braces

[WebKit Commit Bot]

Comment on attachment 383782 [details]
Patch

Clearing flags on attachment: 383782

Committed r252609: <https://trac.webkit.org/changeset/252609>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57305440>