Bug 193911 – AX: IsolatedTree: Implement more attributes

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

Round 2 for AXIsolatedTree work
  - Make use of new HIServices SPI
  - Store root node/focused node status in IsolatedTree rather than the element
  - Implement the following attributes: children, parent, isIgnored, isTree, relativeFrame, speechHint, description methods.
  - Implement hit-testing using relative-frames
  - Ensure that WKAccessibilityWebPageObject queries happen on main thread when they need to

[Radar WebKit Bug Importer]

<rdar://problem/47599217>

[chris fleizach]

Created attachment 360489 [details]
patch

[EWS Watchlist]

Attachment 360489 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/mac/AccessibilityPrivSPI.h:52:  _AXGetClientForCurrentRequestUntrusted is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:84:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:198:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.h:76:  The parameter name "parent" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.h:84:  The parameter name "treeIdentifier" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:118:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1820:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4400:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/accessibility/mac/AccessibilityObjectBase.mm:74:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
ERROR: Source/WebCore/accessibility/mac/AccessibilityObjectBase.mm:73:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/accessibility/mac/AccessibilityObjectBase.mm:148:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
ERROR: Source/WebCore/accessibility/AccessibilityObject.h:373:  "virtual" is redundant since function is already declared as "override"  [readability/inheritance] [4]
ERROR: Source/WebCore/accessibility/AccessibilityObject.h:374:  "virtual" is redundant since function is already declared as "override"  [readability/inheritance] [4]
Total errors found: 13 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Created attachment 360491 [details]
patch

[EWS Watchlist]

Attachment 360491 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/mac/AccessibilityPrivSPI.h:52:  _AXGetClientForCurrentRequestUntrusted is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:199:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:118:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Created attachment 360492 [details]
patch

[EWS Watchlist]

Attachment 360492 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/mac/AccessibilityPrivSPI.h:52:  _AXGetClientForCurrentRequestUntrusted is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:118:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Created attachment 360501 [details]
patch

[EWS Watchlist]

Attachment 360501 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/mac/AccessibilityPrivSPI.h:52:  _AXGetClientForCurrentRequestUntrusted is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:118:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 360501 [details]
patch

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

New failing tests:
accessibility/aria-hidden-false-works-in-subtrees.html
accessibility/mac/iframe-aria-hidden.html
accessibility/unknown-roles-not-exposed.html
accessibility/presentation-role-iframe.html
accessibility/iframe-bastardization.html
accessibility/loading-iframe-sends-notification.html
accessibility/mac/ordered-textmarker-crash.html
accessibility/inline-block-assertion.html
accessibility/deleting-iframe-destroys-axcache.html
accessibility/mac/stale-textmarker-crash.html
accessibility/mac/iframe-with-title-correct-hierarchy.html
accessibility/loading-iframe-updates-axtree.html
accessibility/frame-disconnect-textmarker-cache-crash.html

[EWS Watchlist]

Created attachment 360512 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360501 [details]
patch

Attachment 360501 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10943675

New failing tests:
accessibility/aria-hidden-false-works-in-subtrees.html
accessibility/mac/iframe-aria-hidden.html
accessibility/unknown-roles-not-exposed.html
accessibility/presentation-role-iframe.html
accessibility/iframe-bastardization.html
accessibility/loading-iframe-sends-notification.html
accessibility/mac/ordered-textmarker-crash.html
accessibility/inline-block-assertion.html
accessibility/deleting-iframe-destroys-axcache.html
accessibility/mac/stale-textmarker-crash.html
accessibility/mac/iframe-with-title-correct-hierarchy.html
accessibility/loading-iframe-updates-axtree.html
accessibility/frame-disconnect-textmarker-cache-crash.html

[EWS Watchlist]

Created attachment 360520 [details]
Archive of layout-test-results from ews113 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[chris fleizach]

Created attachment 360527 [details]
patch

[chris fleizach]

Created attachment 360528 [details]
patch

[EWS Watchlist]

Attachment 360528 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/mac/AccessibilityPrivSPI.h:52:  _AXGetClientForCurrentRequestUntrusted is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:118:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Created attachment 360529 [details]
patch

[EWS Watchlist]

Attachment 360529 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/mac/AccessibilityPrivSPI.h:52:  _AXGetClientForCurrentRequestUntrusted is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/accessibility/AccessibilityObjectInterface.h:29:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:118:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Created attachment 360551 [details]
patch

[EWS Watchlist]

Attachment 360551 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/mac/AccessibilityPrivSPI.h:52:  _AXGetClientForCurrentRequestUntrusted is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:118:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 360551 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:487
> +FloatRect AccessibilityObject::convertFrameToSpace(FloatRect& rect, AccessibilityConversionSpace space) const

This is only called in the main thread, right? It's definitely not safe to touch ScrollView / Page / Chrome in a non-main thread.

> Source/WebCore/accessibility/AccessibilityObject.cpp:489
> +    // Find the appropriate scroll view to use to convert the contents to the window.

Let's assert that we're in the main thread here?

> Source/WebCore/accessibility/AccessibilityObjectInterface.h:41
> +typedef struct _AtkObject AccessibilityObjectWrapper;

How could this possibly be safe? I don't think it is. I think we're better off leaving other ports in tact,
and just announce on webkit-dev that they need to move to this new model.

> Source/WebCore/accessibility/AccessibilityObjectInterface.h:43
> +class AccessibilityObjectWrapper : public RefCounted<AccessibilityObjectWrapper> { };

Ditto. This is definitely not going to work if AccessibilityObjectWrapper can be accessed from multiple threads.

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:97
> +        _AXUIElementUseSecondaryAXThread(true);

How does it make sense that we end up calling this function for every WebCore page object?

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:121
>      if ((tree = AXIsolatedTree::treeForPageID(m_pageID))) {

I think we should create a new function like initializeTreeForPageID
which is only called in the main thread instead of generalizing this code path.

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:122
> +        tree->setInitialRequestInProgress(isInitialRequest);

Is there any guarantee that AX thread would never access the AX tree while the initialization is taking place? That’s extremely important.

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:123
> +        ASSERT(isInitialRequest || !isMainThread());

This is basically just ASSERT(true) since we're setting isInitialRequest to true whenever isMainThread() is true.
I don't think we should do that. We need to have two different code paths
one for the main thread and another for AX thread.

Each code path should assert the thread type, and the main thread one should assert that
it's only ever called on the tree exactly once, and the AX thread one should assert that
the initialization had been finished instead of allowing it to take place concurrently.

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:81
> +#define accessibilityRetrieveResultFromMainThread(lambda) { \

Why does this need to be a macro? Just use an inline template function which takes lambda instead.

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:199
> +    auto dispatchBlock = [&self, &value] {
> +        const IntSize& s = m_page->size();
> +        value = [NSValue valueWithSize:NSMakeSize(s.width(), s.height())];
> +    };
> +    
> +    accessibilityRetrieveResultFromMainThread(dispatchBlock);

This will look better if we wrote it as:
return retrieveAccessibilityValueFromMainThread([&self, &value] {
    ~
});

[chris fleizach]

Comment on attachment 360551 [details]
patch

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

>> Source/WebCore/accessibility/AccessibilityObject.cpp:487
>> +FloatRect AccessibilityObject::convertFrameToSpace(FloatRect& rect, AccessibilityConversionSpace space) const
> 
> This is only called in the main thread, right? It's definitely not safe to touch ScrollView / Page / Chrome in a non-main thread.

yes definitely

>> Source/WebCore/accessibility/AccessibilityObject.cpp:489
>> +    // Find the appropriate scroll view to use to convert the contents to the window.
> 
> Let's assert that we're in the main thread here?

ok

>> Source/WebCore/accessibility/AccessibilityObjectInterface.h:41
>> +typedef struct _AtkObject AccessibilityObjectWrapper;
> 
> How could this possibly be safe? I don't think it is. I think we're better off leaving other ports in tact,
> and just announce on webkit-dev that they need to move to this new model.

this is not really touching other ports.

AXIsolatedTree and AccessibilityObject both descend from AccessibilityObjectInterface.  They implement many of the same methods to try to keep things similar. 

This is just moving code around so it compiles

>> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:97
>> +        _AXUIElementUseSecondaryAXThread(true);
> 
> How does it make sense that we end up calling this function for every WebCore page object?

inside the method it makes sure it only does this once. I can add some other methods like "isRunningOnSecondaryThread()" in HIServices and call that first, but technically not needed

>> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:121
>>      if ((tree = AXIsolatedTree::treeForPageID(m_pageID))) {
> 
> I think we should create a new function like initializeTreeForPageID
> which is only called in the main thread instead of generalizing this code path.

ok

>> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:122
>> +        tree->setInitialRequestInProgress(isInitialRequest);
> 
> Is there any guarantee that AX thread would never access the AX tree while the initialization is taking place? That’s extremely important.

good point. it could be possible if you had another AX client there would be a small window they could get in on the second thread.

I can move the secondary thread initialization until after initialization and that should help

>> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:123
>> +        ASSERT(isInitialRequest || !isMainThread());
> 
> This is basically just ASSERT(true) since we're setting isInitialRequest to true whenever isMainThread() is true.
> I don't think we should do that. We need to have two different code paths
> one for the main thread and another for AX thread.
> 
> Each code path should assert the thread type, and the main thread one should assert that
> it's only ever called on the tree exactly once, and the AX thread one should assert that
> the initialization had been finished instead of allowing it to take place concurrently.

ok will try to sort that out

[chris fleizach]

Created attachment 360730 [details]
patch

[EWS Watchlist]

Attachment 360730 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/mac/AccessibilityPrivSPI.h:52:  _AXGetClientForCurrentRequestUntrusted is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:90:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:91:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:118:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Created attachment 360732 [details]
patch

[EWS Watchlist]

Attachment 360732 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/mac/AccessibilityPrivSPI.h:52:  _AXGetClientForCurrentRequestUntrusted is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:118:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Comment on attachment 360732 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:487
> +FloatRect AccessibilityObject::convertFrameToSpace(FloatRect& rect, AccessibilityConversionSpace space) const

Something about this function doesn’t feel right. I think it could be the use of state variables instead of using an early returns. Name of first parameter doesn’t match with the name in this function name. “frame” seems more appropriate.

> Source/WebCore/accessibility/AccessibilityObject.cpp:493
> +    const AccessibilityObject* parent = AccessibilityObject::matchedParent(*this, false, [] (const AccessibilityObject& object) {

False? What does this do? Inline comment or, according to some people, const local variable please.

Also, use const auto*?

> Source/WebCore/accessibility/AccessibilityObject.cpp:499
> +    auto intRect = snappedIntRect(IntRect(rect));

Explicit constructor necessary? Maybe because we have a loss of precision, but I can’t remember and I don’t have access to a checkout. This variable name has a lot to be desired. “snappedFrameRect” or “snappedRect” would be more descriptive.

> Source/WebCore/accessibility/AccessibilityObject.cpp:512
> +#if PLATFORM(IOS_FAMILY)

Extract this block of code into a function to avoid clotting this function with a macro guard?

> Source/WebCore/accessibility/AccessibilityObject.cpp:525
> +    FloatRect rect = FloatRect(elementRect());

Inline this variable as its name provides no value. Is FloatRect an explicit constructor? If not, I don’t think we need to call the constructor explicitly.

> Source/WebCore/accessibility/AccessibilityObject.h:328
> +enum class AccessibilityConversionSpace { ScreenSpace, PageSpace };

“Space” suffix seems redundant. This is a enum class. So we’re scoped.

> Source/WebCore/accessibility/AccessibilityObject.h:363
> +    bool isLink() const override { return false; }

Nice!

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:89
> +    // TODO: Remove unknown client before enabling ACCESSIBILITY_ISOLATED_TREE.

TODO: Change TODO to FIXME. :D

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:187
> +    return retrieveAccessibilityValueFromMainThread<id>([&self]() -> id {

Missing space before ‘(‘

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:188
> +        const IntSize& s = m_page->size();

Variable name is poorly named. See style guide.

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:195
> +    return retrieveAccessibilityValueFromMainThread<id>([&self]() -> id {

Missing space before ‘(‘

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:196
> +        const WebCore::FloatPoint& point = m_page->accessibilityPosition();

Point => position

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:244
> +        if (!m_page)

We have to invoke the callback in order to do this early return? Really? It’s not possible to move this check outside the callback? Why not?

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:260
> +        if (applyContentOffset) {

Early return, please

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:261
> +            if (WebCore::FrameView* frameView = m_page->mainFrameView())

If we get a nullptr here then m_page->corePage() is nullptr, right? (Or how is mainFrameView() implemented?) Something about this if and the one below seem sub-optimal. I think there might be a relationship with m_page->mainFrame() as well :/

> Source/WebKitLegacy/win/AccessibleBase.cpp:687
> +    AccessibilityObject* focusedObj = downcast<AccessibilityObject>(m_object->focusedUIElement());

Auto?
(Because the downcast<> documents the type).

> Source/WebKitLegacy/win/AccessibleBase.cpp:813
> +    AccessibilityObject* childObj = downcast<AccessibilityObject>(m_object->accessibilityHitTest(point));

Ditto.

[chris fleizach]

Comment on attachment 360732 [details]
patch

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

>> Source/WebCore/accessibility/AccessibilityObject.cpp:493
>> +    const AccessibilityObject* parent = AccessibilityObject::matchedParent(*this, false, [] (const AccessibilityObject& object) {
> 
> False? What does this do? Inline comment or, according to some people, const local variable please.
> 
> Also, use const auto*?

re: "false" comment -- this is existing code. It looks like its too big a change for this patch to use an enum, I'll tackle that in another patch

>> Source/WebCore/accessibility/AccessibilityObject.cpp:525
>> +    FloatRect rect = FloatRect(elementRect());
> 
> Inline this variable as its name provides no value. Is FloatRect an explicit constructor? If not, I don’t think we need to call the constructor explicitly.

convertFrameToSpace takes a & parameter, so I can't use a temporary return value from elementRect(). I'll check if I can remove FloatRect() constructor

>> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:244
>> +        if (!m_page)
> 
> We have to invoke the callback in order to do this early return? Really? It’s not possible to move this check outside the callback? Why not?

we can't access m_page off the main thread

>> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:261
>> +            if (WebCore::FrameView* frameView = m_page->mainFrameView())
> 
> If we get a nullptr here then m_page->corePage() is nullptr, right? (Or how is mainFrameView() implemented?) Something about this if and the one below seem sub-optimal. I think there might be a relationship with m_page->mainFrame() as well :/

This one looks like

 if (Frame* frame = mainFrame())
        return frame->view();
    
    return nullptr;
}

[chris fleizach]

Created attachment 361059 [details]
patch

[EWS Watchlist]

Attachment 361059 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/mac/AccessibilityPrivSPI.h:52:  _AXGetClientForCurrentRequestUntrusted is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:118:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Comment on attachment 361059 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:487
> +FloatRect AccessibilityObject::convertFrameToSpace(FloatRect& frame, AccessibilityConversionSpace space) const

We are underutilizing the first param being an lvalue reference. Make it a const lvalue reference. Also, I don't like the name of the first param, frame, or the use of "Frame" in the name of this function because it coincides with the WebCore::Frame object. Is frame standard terminology here? Frame rect or rect is standard terminology in Cocoa and I think one of those is what you are going for here, right? So, rect or frameRect and update the name of this function accordingly. Something bothers me about the second param name, space. Maybe its because I would have chosen conversionSpace, but this doesn't bother me as much as the param name of "frame".

By the way, this function resembles - (CGRect)convertRectToSpace:(WebCore::FloatRect &)rect space:(ConversionSpace)space. We should try to share more code if possible.

> Source/WebCore/accessibility/AccessibilityObject.cpp:495
> +    const auto* parent = AccessibilityObject::matchedParent(*this, false, [] (const AccessibilityObject& object) {
> +        return is<AccessibilityScrollView>(object);
> +    });

Quickly looked at this file and we do something similar in AccessibilityObject::scrollViewAncestor(). By the way, figured out what the "false" param is....includeSelf! So, with that, extract method this code (or even extract to non-member, static-local function if you want):

const AccessibilityScrollView* ancestorAccessibilityScrollView(bool includeSelf)
{
    return AccessibilityObject::matchedParent(*this, includeSelf, [] (const auto& object) { return is<AccessibilityScrollView>(object); });
}

(I chose to extract to *private* method here. So, you will need to a decl for ancestorAccessibilityScrollView() in the header; not shown)

Then implement AccessibilityObject::scrollViewAncestor() in terms of ancestorAccessibilityScrollView()

ScrollView* AccessibilityObject::scrollViewAncestor() const
{
    if (auto* parentScrollView = ancestorAccessibilityScrollView(true /* includeSelf */))
        return downcast<AccessibilityScrollView>(*parentScrollView).scrollView();
    return nullptr;
}

(BTW, took the liberty to rename the local scrollParent to parentScrollView as the former starts with a verb and reads like a command - "scroll the parent", but we are using the var to get a handle to the parent scroll view, not scroll it).

Okay... back to this function, we can implement in terms of ancestorAccessibilityScrollView(false /* includeSelf */):

auto* parentAccessibilityScrollView = ancestorAccessibilityScrollView(false /* includeSelf */);

> Source/WebCore/accessibility/AccessibilityObject.cpp:497
> +    if (parent)
> +        scrollView = downcast<AccessibilityScrollView>(*parent).scrollView();

And then write this in one line to avoid a dumb compiler that initializes the local then overwrites it:

auto* parentScrollView = parentAccessibilityScrollView ? downcast<AccessibilityScrollView>(*parentAccessibilityScrollView).scrollView() : nullptr;

> Source/WebCore/accessibility/AccessibilityObject.cpp:509
> +    if (space == AccessibilityConversionSpace::Screen) {
> +        auto page = this->page();
> +        
> +        // If we have an empty chrome client (like SVG) then we should use the page
> +        // of the scroll view parent to help us get to the screen rect.
> +        if (parent && page && page->chrome().client().isEmptyChromeClient())
> +            page = parent->page();

Simplify this code by using an early return style on page. If we don't have a page, just return snappedFrameRect (read: NO need to call FloatRect constructor explicitly as IntRect to FloatRect is non-lossy conversion). Then...

> Source/WebCore/accessibility/AccessibilityObject.cpp:511
> +        if (page) {

... we can get rid of this if-statement.

> Source/WebCore/accessibility/AccessibilityObject.cpp:516
> +#if PLATFORM(IOS_FAMILY)
> +            snappedFrameRect = page->chrome().rootViewToAccessibilityScreen(snappedFrameRect);
> +#else
> +            snappedFrameRect = page->chrome().rootViewToScreen(snappedFrameRect);
> +#endif

Extract into a static, non-member, non-friend function (declare in header) and share with  -(CGRect)convertRectToSpace:(WebCore::FloatRect &)rect space:(ConversionSpace)space. Call it rootViewToAccessibilityScreen (or something else), have it take a Page& and a const IntRect& and it returns an IntRect. OR, maybe even better, add a Chrome::rootViewToAccessibilityScreen() for Mac in Chrome.h with inlined impl that returns Chrome::rootViewToScreen(). This smashes platform-difference into rootViewToAccessibilityScreen() and no more macro guard.... this code becomes something like:

snappedFrameRect = page->chrome().rootViewToAccessibilityScreen(snappedFrameRect);

> Source/WebCore/accessibility/AccessibilityObject.cpp:520
> +    return FloatRect(snappedFrameRect);

return snappedFrameRect;

(see comment for lines 503-509)

> Source/WebCore/accessibility/AccessibilityObject.cpp:526
> +    FloatRect rect = elementRect();
> +    return convertFrameToSpace(rect, AccessibilityConversionSpace::Page);

See comments above. First argument does *not* need to be lvalue reference....now we can inline 525 into 526!

> Source/WebCore/accessibility/AccessibilityObject.cpp:2686
> +AccessibilityObjectInterface* AccessibilityObject::elementAccessibilityHitTest(const IntPoint& point) const

Return a reference instead of a pointer? Or can this return nullptr? Doesn't look like it.

> Source/WebCore/accessibility/AccessibilityObject.cpp:2717
>      Document* doc = document();

Not part of patch.... doc => document.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1816
> +        Vector<RefPtr<AXIsolatedTreeNode>> children(nodeChildren.size());

Sub-optimal as we default initialize a bunch of nullptrs. Why do this? Call default Vector constructor then Vector::reserverInitialCapacity(nodeChildren.size()).

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1818
> +        for (auto child : nodeChildren)

auto => auto*, I think.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1820
> +

No need for the empty line.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1824
> +

No need for the empty line.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2574
> +            AccessibilityObject* focusedObject = static_cast<AccessibilityObject*>(m_object->focusedUIElement());

auto?

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4357
> +    for (size_t k = 0; k < count; k++) {

size_t => NSUInteger
k => i
k++ => ++k

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:4430
> +            if ([wrapper isKindOfClass:[WebAccessibilityObjectWrapper class]]
> +                && wrapper.axBackingObject->isAttachment() && [wrapper attachmentView])

Extract if-condition into a BOOL, say isAttachment, then write [subarray addObject:(isAttachment ? [wrapper attachmentView] : wrapper)]?

[Daniel Bates]

Comment on attachment 361059 [details]
patch

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1819
> +            children.append(tree->nodeForID(child));

UncheckedAppend(), if you reserve capacity.

[Daniel Bates]

Comment on attachment 361059 [details]
patch

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

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1816
>> +        Vector<RefPtr<AXIsolatedTreeNode>> children(nodeChildren.size());
> 
> Sub-optimal as we default initialize a bunch of nullptrs. Why do this? Call default Vector constructor then Vector::reserverInitialCapacity(nodeChildren.size()).

Actually wheat you wrote could  also be okay as-is, but if you keep as is then use operator[] as using append() does not seem correct. I think we are at over increasing the vector such that we have nodeChildren.size() nullptrs + nodeChildren.size() non-nullptrs (assuming nodeForID() always returns non-nullptr, not sure if it does;  if true,
then should change to return Ref<>).

[chris fleizach]

Comment on attachment 361059 [details]
patch

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

>> Source/WebCore/accessibility/AccessibilityObject.cpp:487
>> +FloatRect AccessibilityObject::convertFrameToSpace(FloatRect& frame, AccessibilityConversionSpace space) const
> 
> We are underutilizing the first param being an lvalue reference. Make it a const lvalue reference. Also, I don't like the name of the first param, frame, or the use of "Frame" in the name of this function because it coincides with the WebCore::Frame object. Is frame standard terminology here? Frame rect or rect is standard terminology in Cocoa and I think one of those is what you are going for here, right? So, rect or frameRect and update the name of this function accordingly. Something bothers me about the second param name, space. Maybe its because I would have chosen conversionSpace, but this doesn't bother me as much as the param name of "frame".
> 
> By the way, this function resembles - (CGRect)convertRectToSpace:(WebCore::FloatRect &)rect space:(ConversionSpace)space. We should try to share more code if possible.

frame is standard terminology. -- we have accessibilityFrame as a property on both Mac and iOS. 

What do you think the name of this function should be called? It's converting a rect to a different space, so I don't know what I would call it

(CGRect)convertRectToSpace:(WebCore::FloatRect &)rect space:(ConversionSpace)space. are tied to together. That one does the platform specific stuff first then calls into this one so that we can share code

>> Source/WebCore/accessibility/AccessibilityObject.cpp:2686
>> +AccessibilityObjectInterface* AccessibilityObject::elementAccessibilityHitTest(const IntPoint& point) const
> 
> Return a reference instead of a pointer? Or can this return nullptr? Doesn't look like it.

this one can return a nullptr. when calling into the children of an object there are a bunch of overrides and it is possible to get a nil back from some of them. it might be possible to figure out a path to change this to referent but it will explode this patch

>>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1816
>>> +        Vector<RefPtr<AXIsolatedTreeNode>> children(nodeChildren.size());
>> 
>> Sub-optimal as we default initialize a bunch of nullptrs. Why do this? Call default Vector constructor then Vector::reserverInitialCapacity(nodeChildren.size()).
> 
> Actually wheat you wrote could  also be okay as-is, but if you keep as is then use operator[] as using append() does not seem correct. I think we are at over increasing the vector such that we have nodeChildren.size() nullptrs + nodeChildren.size() non-nullptrs (assuming nodeForID() always returns non-nullptr, not sure if it does;  if true,
> then should change to return Ref<>).

nodeForID() can return null (in the case where an element becomes an orphan, but an accessibility client still has a reference to it) so we can't easily change to Ref

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1818
>> +        for (auto child : nodeChildren)
> 
> auto => auto*, I think.

here child is a RefPtr so I think this is correct as is

[Ryosuke Niwa]

Comment on attachment 361059 [details]
patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:58
> +    : m_pendingFocusedNodeID(InvalidAXID)

We should initialize that in the class declaration instead.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:60
> +    , m_rootNodeID(InvalidAXID)

Ditto.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:74
> +    ASSERT(isMainThread());

Let's use RELEASE_ASSERT here.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:115
> +    ASSERT(m_initialRequestInProgress || !isMainThread());

We should assert that while m_initialRequestInProgress is true, we're always on the main thread.
So it should be something like: (isMainThread() && m_initialRequestInProgress) || !isMainThread().
We should probably cache the result of isMainThread to a local variable though.
Perhaps we should have an inline helper function for this.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:163
> -    RELEASE_ASSERT(!isMainThread());
> +    RELEASE_ASSERT(m_initialRequestInProgress || !isMainThread());

Ditto about asserting (isMainThread() && m_initialRequestInProgress) || !isMainThread().

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:89
> +    // TODO: Remove unknown client before enabling ACCESSIBILITY_ISOLATED_TREE.

Nit: Use ""FIXME" instead.

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:97
> +            auto tree = AXIsolatedTree::intiializeTreeForPageId(m_pageID, *cache);

Typo: *intiialize*TreeForPageId

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:88
> +        value = lambda();

We may want to do WTFMove(lambda()) so as to avoid copying.

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:99
> +        Vector<String> result = m_page->corePage()->pageOverlayController().copyAccessibilityAttributesNames(true);

Use auto?

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:188
> +        const IntSize& s = m_page->size();

Use auto?

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:196
> +        const WebCore::FloatPoint& position = m_page->accessibilityPosition();

Ditto.

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:243
> +    IntPoint convertedPoint = retrieveAccessibilityValueFromMainThread<IntPoint>([&self, &point]() -> IntPoint {

Use auto?

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:247
> +        IntPoint convertedPoint = m_page->screenToRootView(IntPoint(point));

Ditto.

[chris fleizach]

Comment on attachment 361059 [details]
patch

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

>> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:88
>> +        value = lambda();
> 
> We may want to do WTFMove(lambda()) so as to avoid copying.

hitting this when trying to do the WTFMove

/data/web/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/StdLibExtras.h:511:5: error: static_assert failed due to requirement
      'is_lvalue_reference<id>::value' "T is not an lvalue reference; move() is unnecessary."
    static_assert(is_lvalue_reference<T>::value, "T is not an lvalue reference; move() is unnecessary.");
    ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /data/web/OpenSource/WebKitBuild/Debug/DerivedSources/WebKit2/unified-sources/UnifiedSource48-mm.mm:8:
/data/web/OpenSource/Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:88:17: note: in instantiation of function
      template specialization 'std::move<WTF::CheckMoveParameter, id>' requested here
        value = WTFMove(lambda());
                ^

[Ryosuke Niwa]

(In reply to chris fleizach from comment #36)
> Comment on attachment 361059 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361059&action=review
> 
> >> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:88
> >> +        value = lambda();
> > 
> > We may want to do WTFMove(lambda()) so as to avoid copying.
> 
> hitting this when trying to do the WTFMove
> 
> /data/web/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/StdLibExtras.h:
> 511:5: error: static_assert failed due to requirement
>       'is_lvalue_reference<id>::value' "T is not an lvalue reference; move()
> is unnecessary."
>     static_assert(is_lvalue_reference<T>::value, "T is not an lvalue
> reference; move() is unnecessary.");
>     ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from
> /data/web/OpenSource/WebKitBuild/Debug/DerivedSources/WebKit2/unified-
> sources/UnifiedSource48-mm.mm:8:
> /data/web/OpenSource/Source/WebKit/WebProcess/WebPage/mac/
> WKAccessibilityWebPageObjectMac.mm:88:17: note: in instantiation of function
>       template specialization 'std::move<WTF::CheckMoveParameter, id>'
> requested here
>         value = WTFMove(lambda());
>                 ^

Ugh... that sucks. I guess we'd have to use some template type traits for this. Let's just leave it as is (without WTFMove) for now.

[Daniel Bates]

(In reply to chris fleizach from comment #34)
> Comment on attachment 361059 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361059&action=review
> 
> >> Source/WebCore/accessibility/AccessibilityObject.cpp:487
> >> +FloatRect AccessibilityObject::convertFrameToSpace(FloatRect& frame, AccessibilityConversionSpace space) const
> > 
> > We are underutilizing the first param being an lvalue reference. Make it a const lvalue reference. Also, I don't like the name of the first param, frame, or the use of "Frame" in the name of this function because it coincides with the WebCore::Frame object. Is frame standard terminology here? Frame rect or rect is standard terminology in Cocoa and I think one of those is what you are going for here, right? So, rect or frameRect and update the name of this function accordingly. Something bothers me about the second param name, space. Maybe its because I would have chosen conversionSpace, but this doesn't bother me as much as the param name of "frame".
> > 
> > By the way, this function resembles - (CGRect)convertRectToSpace:(WebCore::FloatRect &)rect space:(ConversionSpace)space. We should try to share more code if possible.
> 
> frame is standard terminology. -- we have accessibilityFrame as a property
> on both Mac and iOS. 
> 
> What do you think the name of this function should be called? It's
> converting a rect to a different space, so I don't know what I would call it
> 

convertRectToSpace? <--- kinda like this
convertAccessibilityFrameToSpace?
rectConvertingFromSpace?
adjustRectForSpace? <--- kinda like this

Not going to harp on the name anymore. If "frame" is in accessibility lingo and you understand it then feel free to run with it. You're in this code more than I am.


> > Actually wheat you wrote could  also be okay as-is, but if you keep as is then use operator[] as using append() does not seem correct. I think we are at over increasing the vector such that we have nodeChildren.size() nullptrs + nodeChildren.size() non-nullptrs (assuming nodeForID() always returns non-nullptr, not sure if it does;  if true,
> > then should change to return Ref<>).
> 
> nodeForID() can return null (in the case where an element becomes an orphan,
> but an accessibility client still has a reference to it) so we can't easily
> change to Ref
> 

Ok.

> >> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1818
> >> +        for (auto child : nodeChildren)
> > 
> > auto => auto*, I think.
> 
> here child is a RefPtr so I think this is correct as is

Yep, you're right!

[Ryosuke Niwa]

Comment on attachment 361059 [details]
patch

I think there are enough review feedbacks that I don't think we want to land this patch as is. r- for now.

[Daniel Bates]

(In reply to Ryosuke Niwa from comment #37)
> (In reply to chris fleizach from comment #36)
> > Comment on attachment 361059 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=361059&action=review
> > 
> > >> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:88
> > >> +        value = lambda();
> > > 
> > > We may want to do WTFMove(lambda()) so as to avoid copying.
> > 
> > hitting this when trying to do the WTFMove
> > 
> > /data/web/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/StdLibExtras.h:
> > 511:5: error: static_assert failed due to requirement
> >       'is_lvalue_reference<id>::value' "T is not an lvalue reference; move()
> > is unnecessary."
> >     static_assert(is_lvalue_reference<T>::value, "T is not an lvalue
> > reference; move() is unnecessary.");
> >     ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > In file included from
> > /data/web/OpenSource/WebKitBuild/Debug/DerivedSources/WebKit2/unified-
> > sources/UnifiedSource48-mm.mm:8:
> > /data/web/OpenSource/Source/WebKit/WebProcess/WebPage/mac/
> > WKAccessibilityWebPageObjectMac.mm:88:17: note: in instantiation of function
> >       template specialization 'std::move<WTF::CheckMoveParameter, id>'
> > requested here
> >         value = WTFMove(lambda());
> >                 ^
> 
> Ugh... that sucks. I guess we'd have to use some template type traits for
> this. Let's just leave it as is (without WTFMove) for now.

What am I missing? Static assert is clearly right. No need to move. Probably should read the thread to see what you are trying to accomplish :/

[chris fleizach]

Created attachment 361380 [details]
patch

[EWS Watchlist]

Attachment 361380 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/mac/AccessibilityPrivSPI.h:52:  _AXGetClientForCurrentRequestUntrusted is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:118:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 48 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Created attachment 361382 [details]
patch

[chris fleizach]

Created attachment 361384 [details]
patch

[chris fleizach]

Created attachment 361385 [details]
patch

[EWS Watchlist]

Attachment 361385 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/mac/AccessibilityPrivSPI.h:52:  _AXGetClientForCurrentRequestUntrusted is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:118:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 59 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Comment on attachment 361059 [details]
patch

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

>>>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1818
>>>> +        for (auto child : nodeChildren)
>>> 
>>> auto => auto*, I think.
>> 
>> here child is a RefPtr so I think this is correct as is
> 
> Yep, you're right!

err, you're right, but for the wrong reason! child is an AXID, which is an unsigned, not a RefPtr<>.

[chris fleizach]

Created attachment 361406 [details]
patch

[EWS Watchlist]

Attachment 361406 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/mac/AccessibilityPrivSPI.h:52:  _AXGetClientForCurrentRequestUntrusted is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:118:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 55 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Comment on attachment 361406 [details]
patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:80
> +
> +    

Too many empty lines.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:138
> +    LockHolder locker(m_changeLogLock);

Old style. New hotness is uniform initializer syntax (UIS).

LockHolder locker { m_changeLogLock }; (MIND the spaces).

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:144
> +    LockHolder locker(m_changeLogLock);

Ditto.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:161
> +void AXIsolatedTree::setInitialRequestInProgress(bool initialRequest)

Not conforming to style.

initialRequest => initialRequestInProgress

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:176
> +    // We don't clear the pending IDs so that if the next round of updates does not modify these, then they stay the same

No need for comma though not a grammar expert...

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:177
> +    // value without extra bookkeeping

...but we need a period!

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:86
> +    AXID m_rootNodeID = InvalidAXID;

UIS?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:87
> +    AXID m_focusedNodeID = InvalidAXID;

UIS?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:37
>      , m_initialized(false)

Not part of patch, but this is the old style. New hotness is define in header using UIS.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:38
> +    , m_wrapper(nullptr)

Not necessary. Default constructor does not for us.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:99
> +    

Whitespace! Remove the whole line in my opinion.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:116
> +AccessibilityObjectInterface* AXIsolatedTreeNode::accessibilityHitTest(const IntPoint& pt) const

pt?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:120
> +        

Whitespace! Remove the whole line in my opinion.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:121
> +    for (auto childID : children()) {

I like the name of this local. Use this name for similar locals throughout this patch, please.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:123
> +        if (child && child->relativeFrame().contains(pt))

How can child ever be nullptr? If it can why is its id still in children()? If this in theory can never happen then add ASSERT(child) instead.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:126
> +    

Whitespace! Remove the whole line in my opinion.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:139
> +    return WTF::switchOn(value, [&zeroRect] (Optional<FloatRect> typedValue) {

Formatting looks off. Search the code for other examples.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:197
> -    
> +

Nice!

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:246
> +    

Whitespace! Remove the line in my opinion.

> Source/WebCore/accessibility/mac/AccessibilityObjectBase.mm:42
> +    if (speak & SpeakAs::SpellOut)
> +        [hints addObject:@"spell-out"];
> +    else
> +        [hints addObject:@"normal"];

Could write in one line using ternary? Maybe the way you wrote it is okay. It's growing on me as I type this....

> Source/WebCore/accessibility/mac/AccessibilityObjectBase.mm:43
> +    

Whitespace! Remove the line in my opinion.

> Source/WebCore/accessibility/mac/AccessibilityObjectBase.mm:50
> +    

Whitespace! Remove the line in my opinion.

> Source/WebCore/accessibility/mac/AccessibilityObjectBase.mm:60
> +    

Whitespace! But I like an empty line.

> Source/WebCore/accessibility/mac/AccessibilityObjectBase.mm:63
> +    

Whitespace! I don't have an empty line preference here. Err, kinda leaning towards no empty line. Not going to ruin my day either way. :/ But the whitespace will.

> Source/WebCore/accessibility/mac/AccessibilityObjectBase.mm:71
> +        

Whitespace!

> Source/WebCore/accessibility/mac/AccessibilityObjectBase.mm:156
> +        switch (text.textSource) {
> +        case AccessibilityTextSource::Alternative:
> +        case AccessibilityTextSource::Visible:
> +        case AccessibilityTextSource::Children:
> +        case AccessibilityTextSource::LabelByElement:
> +            descriptiveTextAvailable = true;
> +            break;
> +        default:
> +            break;
> +        }

Extract into static, non-member, file-local function, maybe called isDescriptiveText()?, defined above AccessibilityObject::descriptionAttributeValue() (see where I am going? If not keep reading...), takes a decl(text.textSource) and returns a bool. Share this with AccessibilityObject::descriptionAttributeValue().

I don't like tracking variables like descriptiveTextAvailable. Doing the above solves two birds with one stone.

> Source/WebCore/accessibility/mac/AccessibilityObjectBase.mm:161
> +    

What is with your editor? Whitespace!

> Source/WebCore/accessibility/mac/AccessibilityObjectBase.mm:162
> +    return String();

Old style. New hotness, return { }; (MIND the space).

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:263
> +

No whitespace here! I like it! What is going on?

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:273
> +NSArray *convertToNSArray(const Vector<RefPtr<WebCore::AXIsolatedTreeNode>>& vector)

I am not a computer. Better param name please.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:275
>      NSMutableArray *array = [NSMutableArray arrayWithCapacity:vector.size()];

Maybe call this result. I don't like Hungarian naming.

Old style. New hotness is wrap this in a RetainPtr<> and this also fixes the leak!

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:276
> +    for (auto child : vector)

auto&?

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:288
> +NSArray *convertToNSArray(const WebCore::AccessibilityObject::AccessibilityChildrenVector& vector)
> +{
> +    NSMutableArray *array = [NSMutableArray arrayWithCapacity:vector.size()];
> +    for (auto child : vector)
> +        addChildToArray(*child.get(), array);
> +    return [[array copy] autorelease];
> +}

Everything I said about the above function applies here too AND fix the leak!

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:372
> +    if (_AXUIElementRequestServicedBySecondaryAXThread())
> +        ASSERT_NOT_REACHED();

ASSERT(!...)

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:515
> +        cgRect = (CGRect)m_object->convertFrameToSpace(rect, space);

static_cast<>?

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1824
> +        for (auto child : nodeChildren)

child => childID

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3231
> +        auto rect = (CGRect)_axBackingObject->relativeFrame();

static_cast<>?

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3298
> +    auto focusedObj = _axBackingObject->focusedUIElement();

focusedObj => focusedObject (like _axBacking*Object* <- emphasis mine)

[Daniel Bates]

Comment on attachment 361406 [details]
patch

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

> Source/WebKitLegacy/win/AccessibleBase.cpp:687
> +    auto focusedObj = downcast<AccessibilityObject>(m_object->focusedUIElement());

focusedObj => focusedObject

> Source/WebKitLegacy/win/AccessibleBase.cpp:813
> +    auto childObj = downcast<AccessibilityObject>(m_object->accessibilityHitTest(point));

childObj => childObject

[Daniel Bates]

Comment on attachment 361406 [details]
patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:111
> +static inline assertThreadCorrectness(BOOL initialRequest)

No need to cache isMainThread() if you swap the disjuncts below. So, no need for this function. Just adds noise. Please remove.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:114
> +    RELEASE_ASSERT((mainThread && initialRequest) || !mainThread);

Swap the disjuncts and simplify.

[Ryosuke Niwa]

Comment on attachment 361406 [details]
patch

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

>> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:114
>> +    RELEASE_ASSERT((mainThread && initialRequest) || !mainThread);
> 
> Swap the disjuncts and simplify.

Oh, you're right. We can just do !mainThread() || initialRequest.

[Daniel Bates]

Comment on attachment 361406 [details]
patch

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

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:275
>>      NSMutableArray *array = [NSMutableArray arrayWithCapacity:vector.size()];
> 
> Maybe call this result. I don't like Hungarian naming.
> 
> Old style. New hotness is wrap this in a RetainPtr<> and this also fixes the leak!

Err,  no leak here since this function autorealease. Still this is the old style. RetainPtr<> and -init...  and the we don’t need to worry about autorelease pools and keep our mem footprint larger for longer.

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:288
>> +}
> 
> Everything I said about the above function applies here too AND fix the leak!

Err,  no leak here since we use autorealease helper function. Still this is the old style. RetainPtr<> and -init...  and the we don’t need to worry about autorelease pools and keep our mem footprint larger for longer.

[chris fleizach]

Created attachment 361475 [details]
patch

All comments addressed (to best of my knowledge)

[EWS Watchlist]

Attachment 361475 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/mac/AccessibilityPrivSPI.h:52:  _AXGetClientForCurrentRequestUntrusted is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 361475 [details]
patch

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

New failing tests:
accessibility/help-text.html
accessibility/mac/progress-with-label-element.html
accessibility/mac/figure-element.html
accessibility/mac/title-attribute-used-as-last-resort.html
accessibility/img-fallsback-to-title.html
accessibility/mac/title-attribute-not-used-as-axtitle.html
accessibility/mac/link-with-title.html

[EWS Watchlist]

Created attachment 361485 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 361475 [details]
patch

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

New failing tests:
accessibility/help-text.html
accessibility/mac/progress-with-label-element.html
accessibility/mac/figure-element.html
accessibility/mac/title-attribute-used-as-last-resort.html
accessibility/img-fallsback-to-title.html
accessibility/mac/title-attribute-not-used-as-axtitle.html
accessibility/mac/link-with-title.html

[EWS Watchlist]

Created attachment 361486 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 361475 [details]
patch

Attachment 361475 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11074644

New failing tests:
accessibility/help-text.html
accessibility/mac/progress-with-label-element.html
accessibility/mac/figure-element.html
accessibility/mac/title-attribute-used-as-last-resort.html
accessibility/img-fallsback-to-title.html
accessibility/mac/title-attribute-not-used-as-axtitle.html
accessibility/mac/link-with-title.html

[EWS Watchlist]

Created attachment 361487 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Daniel Bates]

Comment on attachment 361475 [details]
patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:110
> +static inline assertThreadCorrectness(BOOL initialRequest)

i don't like this function and would prefer you inline it into each call site. The only reason you added this function is because It was wrongly believed that we had to cache isMainThread(). We didn't have to, so we don't need this function. But maybe you like this function?! Or maybe you're tired of this review? If you kept the code like this out of review exhaustion I wouldn't be mad. But if you have an ounce or two left in you then please remove it.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:133
> +    FloatRect zeroRect;

Get rid of this. What dumb compiler are we working around? Just inline the constructor. See below.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:135
> +    return WTF::switchOn(value,

This looks better. I like it.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:138
> +                return zeroRect;

What are we saving here? Don't capture zeroRect, just return FloatRect { } (mind the space) and be done with it. No need to play this game of capturing zeroRect.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:141
> +        [&] (auto&) { return zeroRect; }

I get it. I get it, but this polymorphic design for a map seems weird. No need to change in this patch... having said that, no need to capture, just return FloatRect { }.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:148
> +    return WTF::switchOn(value,

Same here. I can read it!

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:149
> +        [&] (double& typedValue) { return typedValue; },

No need to capture anything.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:150
> +        [&] (auto&) { return 0.0; }

Not style guide conformant. 0.0  => 0

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:151
> +    );

No need to capture anything in the line above.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:158
> +        [&] (unsigned& typedValue) { return typedValue; },

No need to capture anything.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:159
> +        [&] (auto&) { return 0; }

Here, you did it! You returned 0. I like it. Now fix up the case in the above function.

Also, no need to capture anything.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:167
> +        [&] (bool& typedValue) { return typedValue; },

Stop capturing things.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:168
> +        [&] (auto&) { return false; }

Again, stop.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:176
> +        [&] (String& typedValue) { return typedValue; },

Stop capturing.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:177
> +        [&] (auto&) { return emptyString(); }

Ditto.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:185
> +        [&] (int& typedValue) { return typedValue; },

Ditto.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:186
> +        [&] (auto&) { return 0; }

Ditto.

> Source/WebCore/accessibility/mac/AccessibilityObjectBase.mm:39
> +    [hints addObject:(speak & SpeakAs::SpellOut) ? @"spell-out" : @"normal"];

I'm going to cry. This is beautiful.

> Source/WebCore/accessibility/mac/AccessibilityObjectBase.mm:59
> +        break;

Tears dried up... no break. Just return false and remove line 61. clang isn't that dumb.

Even better, if we can, get rid of the default case and let the compiler enforce we have all enumerators.

> Source/WebCore/accessibility/mac/AccessibilityObjectBase.mm:72
> +    accessibilityText(textOrder);

What is this? This function sounds like it returns something? But we don't use the retun value? What is going on here?

> Source/WebCore/accessibility/mac/AccessibilityObjectBase.mm:142
> +    accessibilityText(textOrder);

Weird named function? See above comments.

> Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm:64
> +bool AccessibilityObject::fileUploadButtonReturnsValueInTitle() const

Ok as-is, but unless we have a binary compatibility need. I would make this inline function (in header) instead of being out-of-line.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:274
> +    NSMutableArray *result = [NSMutableArray arrayWithCapacity:children.size()];

RetainPtr<> and -init....Capacity:here to avoid ref churn.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:276
> +        addChildToArray(*child.get(), result)

No need for .get(). I am repeating myself :/

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:283
> +    NSMutableArray *result = [NSMutableArray arrayWithCapacity:children.size()];

RetainPtr<> and -init....Capacity:here to avoid ref churn.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:285
> +        addChildToArray(*child.get(), result);

Ditto.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:509
>          cgRect = NSRectToCGRect(nsRect);

Just return here. No need to update a tracking variable. Return here please.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:512
> +    else

If you do what I said abov then remove this else and just return on the line below.

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:103
> +            // Now that we have created our tree, initialize the secondary thread so future requests come in on the other thread.

Line too long.

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:87
> +    callOnMainThreadAndWait([&value, &lambda] {

Doesn't this function do what the outer function does already? Or does callOn...Wait() not support return value? If the latter, why not fix it OR rename add a variant that does. Why is this an accessibility function? Why is it named so poorly in my opinion? I like thee name callOnMain...Wait() either fix it or add new function like callOnMain...WaitReturningValue().

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:97
> +    return retrieveAccessibilityValueFromMainThread<id>([&self]() -> id {

Missing space after ]

[chris fleizach]

Comment on attachment 361475 [details]
patch

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

>> Source/WebCore/accessibility/mac/AccessibilityObjectBase.mm:72
>> +    accessibilityText(textOrder);
> 
> What is this? This function sounds like it returns something? But we don't use the retun value? What is going on here?

you pass in the AXText vector and get back the various forms of accessibility text so that each platform can process how it wants to process

>> Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm:64
>> +bool AccessibilityObject::fileUploadButtonReturnsValueInTitle() const
> 
> Ok as-is, but unless we have a binary compatibility need. I would make this inline function (in header) instead of being out-of-line.

yea in this case we have a different return for iOS and for Mac, so we need different versions

>> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:87
>> +    callOnMainThreadAndWait([&value, &lambda] {
> 
> Doesn't this function do what the outer function does already? Or does callOn...Wait() not support return value? If the latter, why not fix it OR rename add a variant that does. Why is this an accessibility function? Why is it named so poorly in my opinion? I like thee name callOnMain...Wait() either fix it or add new function like callOnMain...WaitReturningValue().

callOnAndWait() does not support a return value. It's in WTF. Do you think I need to add a new WTF method for this purpose? Is anyone else trying to do this kind of thing?

callOn..Wait is sort of long. not looking forward to refactoring and consolidating into two methods

void callOnMainThreadAndWait(WTF::Function<void()>&& function)
{
    if (isMainThread()) {
        function();
        return;
    }

    Lock mutex;
    Condition conditionVariable;

    bool isFinished = false;

    callOnMainThread([&, function = WTFMove(function)] {
        function();

        std::lock_guard<Lock> lock(mutex);
        isFinished = true;
        conditionVariable.notifyOne();
    });

    std::unique_lock<Lock> lock(mutex);
    conditionVariable.wait(lock, [&] {
        return isFinished;
    });
}

[chris fleizach]

Created attachment 361513 [details]
patch

[EWS Watchlist]

Attachment 361513 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/mac/AccessibilityPrivSPI.h:52:  _AXGetClientForCurrentRequestUntrusted is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 58 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Created attachment 361525 [details]
patch

[EWS Watchlist]

Attachment 361525 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/mac/AccessibilityPrivSPI.h:52:  _AXGetClientForCurrentRequestUntrusted is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Created attachment 361526 [details]
patch

[EWS Watchlist]

Attachment 361526 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/mac/AccessibilityPrivSPI.h:52:  _AXGetClientForCurrentRequestUntrusted is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 361526 [details]
patch

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

New failing tests:
accessibility/help-text.html
accessibility/mac/progress-with-label-element.html
accessibility/mac/figure-element.html
accessibility/mac/title-attribute-used-as-last-resort.html
accessibility/img-fallsback-to-title.html
accessibility/mac/title-attribute-not-used-as-axtitle.html
accessibility/mac/link-with-title.html

[EWS Watchlist]

Created attachment 361576 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 361526 [details]
patch

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

New failing tests:
accessibility/help-text.html
accessibility/mac/progress-with-label-element.html
accessibility/mac/figure-element.html
accessibility/mac/title-attribute-used-as-last-resort.html
accessibility/img-fallsback-to-title.html
accessibility/mac/title-attribute-not-used-as-axtitle.html
accessibility/mac/link-with-title.html

[EWS Watchlist]

Created attachment 361583 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 361526 [details]
patch

Attachment 361526 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11086580

New failing tests:
accessibility/help-text.html
accessibility/mac/progress-with-label-element.html
accessibility/mac/figure-element.html
accessibility/mac/title-attribute-used-as-last-resort.html
accessibility/img-fallsback-to-title.html
accessibility/mac/title-attribute-not-used-as-axtitle.html
accessibility/mac/link-with-title.html

[EWS Watchlist]

Created attachment 361588 [details]
Archive of layout-test-results from ews112 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[chris fleizach]

Created attachment 361602 [details]
patch

[EWS Watchlist]

Attachment 361602 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/mac/AccessibilityPrivSPI.h:52:  _AXGetClientForCurrentRequestUntrusted is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Comment on attachment 361602 [details]
patch

Thank you, Chris, for iterating on this!

[WebKit Commit Bot]

Comment on attachment 361602 [details]
patch

Rejecting attachment 361602 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 361602, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 5000 characters of output:
y/AccessibilityAttachment.h
patching file Source/WebCore/accessibility/AccessibilityImageMapLink.cpp
patching file Source/WebCore/accessibility/AccessibilityImageMapLink.h
patching file Source/WebCore/accessibility/AccessibilityMediaControls.cpp
patching file Source/WebCore/accessibility/AccessibilityMediaControls.h
patching file Source/WebCore/accessibility/AccessibilityNodeObject.cpp
patching file Source/WebCore/accessibility/AccessibilityNodeObject.h
patching file Source/WebCore/accessibility/AccessibilityObject.cpp
Hunk #3 succeeded at 1828 (offset 4 lines).
Hunk #4 succeeded at 2683 (offset 4 lines).
Hunk #5 succeeded at 2712 (offset 4 lines).
patching file Source/WebCore/accessibility/AccessibilityObject.h
patching file Source/WebCore/accessibility/AccessibilityObjectInterface.h
patching file Source/WebCore/accessibility/AccessibilityRenderObject.cpp
patching file Source/WebCore/accessibility/AccessibilityRenderObject.h
patching file Source/WebCore/accessibility/AccessibilitySVGElement.cpp
patching file Source/WebCore/accessibility/AccessibilitySVGElement.h
patching file Source/WebCore/accessibility/AccessibilityScrollView.cpp
patching file Source/WebCore/accessibility/AccessibilityScrollView.h
patching file Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceComponent.cpp
patching file Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm
patching file Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm
patching file Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
patching file Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
patching file Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp
patching file Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.h
patching file Source/WebCore/accessibility/mac/AXObjectCacheMac.mm
patching file Source/WebCore/accessibility/mac/AccessibilityObjectBase.mm
patching file Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm
patching file Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.h
patching file Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm
Hunk #3 FAILED at 256.
1 out of 15 hunks FAILED -- saving rejects to file Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm.rej
patching file Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
patching file Source/WebCore/loader/EmptyClients.h
patching file Source/WebCore/page/Chrome.cpp
patching file Source/WebCore/page/Chrome.h
patching file Source/WebCore/page/ChromeClient.h
patching file Source/WebCore/platform/HostWindow.h
patching file Source/WebKit/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/Platform/spi/mac/AccessibilityPrivSPI.h
patching file Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp
Hunk #1 succeeded at 203 (offset 11 lines).
patching file Source/WebKit/UIProcess/API/gtk/PageClientImpl.h
Hunk #1 succeeded at 81 (offset 1 line).
patching file Source/WebKit/UIProcess/API/wpe/PageClientImpl.cpp
Hunk #1 succeeded at 182 (offset 5 lines).
patching file Source/WebKit/UIProcess/API/wpe/PageClientImpl.h
patching file Source/WebKit/UIProcess/PageClient.h
patching file Source/WebKit/UIProcess/WebPageProxy.cpp
Hunk #1 succeeded at 5062 (offset 3 lines).
Hunk #2 succeeded at 5071 (offset 3 lines).
patching file Source/WebKit/UIProcess/WebPageProxy.h
Hunk #1 succeeded at 1587 (offset 7 lines).
patching file Source/WebKit/UIProcess/WebPageProxy.messages.in
patching file Source/WebKit/UIProcess/mac/PageClientImplMac.h
patching file Source/WebKit/UIProcess/mac/PageClientImplMac.mm
patching file Source/WebKit/UIProcess/win/PageClientImpl.cpp
patching file Source/WebKit/UIProcess/win/PageClientImpl.h
patching file Source/WebKit/WebKit.xcodeproj/project.pbxproj
Hunk #2 succeeded at 7106 (offset -18 lines).
patching file Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp
patching file Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h
patching file Source/WebKit/WebProcess/WebPage/WebPage.cpp
Hunk #1 succeeded at 3185 (offset 3 lines).
Hunk #2 succeeded at 3198 (offset 3 lines).
patching file Source/WebKit/WebProcess/WebPage/WebPage.h
Hunk #1 succeeded at 561 (offset 1 line).
patching file Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.h
patching file Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm
patching file Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm
patching file Source/WebKitLegacy/mac/WebCoreSupport/WebChromeClient.h
patching file Source/WebKitLegacy/mac/WebCoreSupport/WebChromeClient.mm
patching file Source/WebKitLegacy/win/AccessibleBase.cpp
patching file Source/WebKitLegacy/win/WebCoreSupport/WebChromeClient.cpp
patching file Source/WebKitLegacy/win/WebCoreSupport/WebChromeClient.h

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Daniel Bates']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/11122693

[chris fleizach]

Created attachment 361802 [details]
landing patch

[chris fleizach]

Created attachment 361810 [details]
landing patch

[EWS Watchlist]

Attachment 361810 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/mac/AccessibilityPrivSPI.h:52:  _AXGetClientForCurrentRequestUntrusted is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 361810 [details]
landing patch

Clearing flags on attachment: 361810

Committed r241321: <https://trac.webkit.org/changeset/241321>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dean Jackson]

This breaks a build where HAVE_ACCESSIBILITY is true but the ApplicationServices/ApplicationServicesPriv.h file isn't found (on internal builds).

See https://build-safari.apple.com/#/builders/40/builds/21862/steps/19/logs/errors