Bug 127447 – Web Inspector: AXI: Accessibility Node Inspection

• Component: Web Inspector
• Status: RESOLVED FIXED

[James Craig]

Add accessibility node inspection to the Web Inspector. Some properties (like computed role and computer label) are not available through DOM methods, so ask WebCore for these and expose them to the page author.

<rdar://problem/14065212>

[James Craig]

Created attachment 221905 [details]
first draft (works, but still needs substantial cleanup)

[James Craig]

Note: the excess whitespace diffs are b/c my branch is a month or two out of date. I've been working on this in my spare time and was tired of resolving conflicts along the way. Will merge as part of the other cleanup.

[Joseph Pecoraro]

Comment on attachment 221905 [details]
first draft (works, but still needs substantial cleanup)

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

Some drive by comments to help.

> Source/WebCore/accessibility/AccessibilityObject.cpp:1508
> +typedef HashMap<int, String> ARIAReverseRoleMap;

Nit: int => AccessibilityRole

> Source/WebCore/accessibility/AccessibilityObject.cpp:1520
> -static ARIARoleMap* createARIARoleMap()
> +static void initializeRoleMap()
>  {
> +    if (gAriaRoleMap)

Does this need to be initialized in a thread safe manner? Or is it only ever accessed from the WebCore thread?

> Source/WebCore/inspector/protocol/DOM.json:206
> +                { "name": "objectGroup", "type": "string", "optional": true, "description": "todo" }

I see "objectGroup" is listed as a Todo item. I don't think you need this, because you aren't creating any RemoteObjects or explicitly keeping any JavaScript objects alive on the inspected page. You can remove this.

EventListeners uses the objectGroup to wrap the event listener handler function from the inspected page. You're just sending a bunch of strings over to the frontend, none of which are objects that could be garbage collected.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1
> -þÿvar localizedStrings = new Object;
> +ÿþvar localizedStrings = new Object;

Yikes, the changes to this file look weird.

> Source/WebInspectorUI/UserInterface/DOMNode.js:446
> +	/**
> +     * @param {function(?Protocol.Error)=} callback
> +     */

No need to add these comments unless they help you. We no longer use them but we haven't gotten through and stripped them out.

> Source/WebInspectorUI/UserInterface/DOMNodeDetailsSidebarPanel.js:257
> +            if (label === "" && domNode.getAttribute('aria-label'))
> +                label = domNode.getAttribute('aria-label');

Style: Always use double quoted strings in the inspector.

  'aria-label' => "aria-label"

> Source/WebInspectorUI/UserInterface/DOMNodeDetailsSidebarPanel.js:260
> +                WebInspector.UIString("%s (computed)").replace("%s", label);

You should not use .replace() on a localized string. We provide String.prototype.format, which is like printf style filling in of %x format strings with .format()'s parameters. So:

    WebInspector.UIString("%s (computed)").format(label);

[James Craig]

(In reply to comment #3)
> (From update of attachment 221905 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221905&action=review

> > Source/WebCore/accessibility/AccessibilityObject.cpp:1508
> > +typedef HashMap<int, String> ARIAReverseRoleMap;
> 
> Nit: int => AccessibilityRole

The compiler actually chokes on this change, believe it or not. Could not get it to build until changing AccessibilityRole to int. I was planning to at that in a comment. Asumption is that it doesn't want an enum as the hashmap key. Would love a better solution if you know the fix.

[Joseph Pecoraro]

Comment on attachment 221905 [details]
first draft (works, but still needs substantial cleanup)

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

>>> Source/WebCore/accessibility/AccessibilityObject.cpp:1508
>>> +typedef HashMap<int, String> ARIAReverseRoleMap;
>> 
>> Nit: int => AccessibilityRole
> 
> The compiler actually chokes on this change, believe it or not. Could not get it to build until changing AccessibilityRole to int. I was planning to at that in a comment. Asumption is that it doesn't want an enum as the hashmap key. Would love a better solution if you know the fix.

Ahh, it is an enum. I see. Is the error you get something like DefaultHash:
/usr/local/include/wtf/HashMap.h:41:79: error: implicit instantiation of undefined template 'WTF::DefaultHash<Foo>'

I'm sure there is a way to fix this but for now jus leave it int. I'll ask the C++ gurus over here if they have a solution.

[James Craig]

Created attachment 223290 [details]
Patch fixes last crash and reconciles TOT merge; still needs some cleanup: comment/todo removals.

[chris fleizach]

Comment on attachment 223290 [details]
Patch fixes last crash and reconciles TOT merge; still needs some cleanup: comment/todo removals.

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

> Source/WebCore/accessibility/AccessibilityObject.h:786
> +    virtual String computedRoleString() const;

this doesn't need to be virtual since no one will override

> Source/WebCore/accessibility/AccessibilityObject.h:788
> +    // MSAA

no need to add this comment in this patch

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1427
> +    if (node){

space after )

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1429
> +        if (axObjectCache) {

this can be written as
if (AXObjectCache* cache = node->document()...)

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1431
> +            if (axObject)

This can be written as
if (AXObject* axObject = axObjectCache->getO...)

> Source/WebCore/inspector/InspectorDOMAgent.h:134
> +    virtual void getAccessibilityPropertiesForNode(ErrorString*, int in_nodeId, RefPtr<Inspector::TypeBuilder::DOM::AccessibilityProperties>& out_properties) override;

these probably don't need to be virtual

[James Craig]

Created attachment 223315 [details]
progress

Found one more crash (on page reload when displaying the Accessibility Node Inspector) and I still need to get the hashmap template working.

[James Craig]

Not sure what's going on with all the diffs in the loc strings file (my local diff looks fine). I guess I'll have to revert that one and add back in those new strings manually.

[James Craig]

Created attachment 223316 [details]
patch

seeing if this one fixes the unicode oddities in the loc strings file

[Timothy Hatcher]

localizedStrings.js is a binary file. You can use "svn-create-patch" or "webkit-patch upload".

[James Craig]

Created attachment 223506 [details]
patch with test coverage

[chris fleizach]

Comment on attachment 223506 [details]
patch with test coverage

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

> LayoutTests/accessibility/roles-computedRoleString-expected.txt:2
> +This tests that native elements and ARIA overrides result in the same ARIA computed role, regardless of platform.

You should add an entry in TestExpectation in GTK marking this as failing until computedRoleString is implemented

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2067
> +- (NSString*)computedRoleString

need a space before the *

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2113
> +    if ([attributeName isEqualToString: @"AXAriaRole"])

This should be AXARIARole and no space after the ":" even though other lines are doing that.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:879
> +    Element* element = assertElement(errorString, nodeId);

Is there a reason you need to get the element here? you don't see to use it.
It seems just getting the node, and checking if that's nil should be good enough

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1412
> +PassRefPtr<TypeBuilder::DOM::AccessibilityProperties> InspectorDOMAgent::buildObjectForAccessibilityProperties(Node* node)

you should probably ASSERT(node) in here, and 
if (!node)
return nullptr

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1424
> +    String label = "";

i don't think you need to initialize these strings to anything

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1429
> +                isAccessibilityObject = true;

this should probably check accessibilityIsIgnored() to determine true/false

> Source/WebCore/inspector/protocol/DOM.json:55
> +            "description": "A structure holding EventListener properties."

this seems unrelated to this patch

> Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:598
> +    NSString* computedRoleString = descriptionOfValue([m_element accessibilityAttributeValue:@"AXAriaRole"], m_element);

* on the wrong side

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:642
> +    NSString* computedRoleString = descriptionOfValue([m_element accessibilityAttributeValue:@"AXAriaRole"], m_element);

* on wrong side

[James Craig]

(In reply to comment #13)
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2067
> > +- (NSString*)computedRoleString
> 
> need a space before the *

Just following the file conventions. Could you raise a separate bug to fix all of those?

- (NSString*)role
- (NSString*)subrole
- (NSString*)roleDescription

> > Source/WebCore/inspector/InspectorDOMAgent.cpp:879
> > +    Element* element = assertElement(errorString, nodeId);
> 
> Is there a reason you need to get the element here? you don't see to use it.
> It seems just getting the node, and checking if that's nil should be good enough

Without it, there is a condition where I'll crash on page reload in between the time when node becomes valid, before element becomes valid. A few of the other InspectorDOMAgent methods use the same fix.

I work on the rest of your feedback.

[chris fleizach]

(In reply to comment #14)
> (In reply to comment #13)
> > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2067
> > > +- (NSString*)computedRoleString
> > 
> > need a space before the *
> 
> Just following the file conventions. Could you raise a separate bug to fix all of those?
> 
> - (NSString*)role
> - (NSString*)subrole
> - (NSString*)roleDescription
> 

Yep those are wrong and need to be fixed at some point, but we shouldn't keep making the same mistake

> > > Source/WebCore/inspector/InspectorDOMAgent.cpp:879
> > > +    Element* element = assertElement(errorString, nodeId);
> > 
> > Is there a reason you need to get the element here? you don't see to use it.
> > It seems just getting the node, and checking if that's nil should be good enough
> 
> Without it, there is a condition where I'll crash on page reload in between the time when node becomes valid, before element becomes valid. A few of the other InspectorDOMAgent methods use the same fix.

So it seem you could just use Element then, since it's a subclass of Node

> 
> I work on the rest of your feedback.

[James Craig]

(In reply to comment #15)

> So it seem you could just use Element then, since it's a subclass of Node

Actually, your ASSERT(node) suggestion seems to be a better solution to the problem.

[chris fleizach]

(In reply to comment #16)
> (In reply to comment #15)
> 
> > So it seem you could just use Element then, since it's a subclass of Node
> 
> Actually, your ASSERT(node) suggestion seems to be a better solution to the problem.

Assert only covers debug cases, so you'll still need to plan if this happens in production code

[James Craig]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > 
> > > So it seem you could just use Element then, since it's a subclass of Node
> > 
> > Actually, your ASSERT(node) suggestion seems to be a better solution to the problem.
> 
> Assert only covers debug cases, so you'll still need to plan if this happens in production code

Included this too.

if (!node)
        return nullptr;

I just noticed element won't cover the cases of accessible text nodes, which have a "text" role and computed label value, so it'd be nice to keep those visible in the inspector.

[James Craig]

Created attachment 223524 [details]
patch with test coverage and first round review feedback

Given that this feature patch is pushing 60k, I'd also like final review from a member of the Web Inspector team once Chris gives it the thumbs up.

[Joseph Pecoraro]

Comment on attachment 223524 [details]
patch with test coverage and first round review feedback

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

Included some comments. Looks good overall.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2112
> +    // this is used only by DumpRenderTree for testing (so far)

Style: WebKit Style is for comments to be full sentences. Start with a capital, end with a period.

> Source/WebCore/inspector/protocol/DOM.json:210
> +            "name": "getAccessibilityPropertiesForNode",
> +            "parameters": [
> +                { "name": "nodeId", "$ref": "NodeId", "description": "Id of the node for which to get accessibility properties." }
> +            ],
> +            "returns": [
> +                { "name": "properties", "$ref": "AccessibilityProperties", "description": "Dictionary of relevant accessibility properties." }
> +            ],
> +            "description": "Returns a dictionary of accessibility properties for the node."
> +        },

Nit: I prefer the ordering name, description, parameters, returns. Easier on the eyes.

Also, it would be nice to have a small protocol test for this:
LayoutTests/inspector-protocol/dom/getAccessibilityPropertiesForDebugger.html

We prefer any new protocol methods to include a test. But since there are probably no good tests for the DOM domain right now I think we can avoid it since you have such a nice LayoutTest for the AX portion of this.

> Source/WebInspectorUI/UserInterface/DOMNode.js:446
>      /**
> +     * @param {function(?Protocol.Error)=} callback
> +     */

Might as well remove this comment. The existing ones are legacy. And this comment is only partially accurate, the callback returns more then just an error.

> Source/WebInspectorUI/UserInterface/DOMNodeDetailsSidebarPanel.js:252
> +                    role = WebInspector.UIString("No exact recognized role match.");

Hmm, as a user I wouldn't quite understand what this string means.

> Source/WebInspectorUI/UserInterface/InspectorWebBackendCommands.js:105
> +InspectorBackend.registerCommand("DOM.getAccessibilityPropertiesForNode", [{"name": "nodeId", "type": "number", "optional": false}, {"name": "objectGroup", "type": "string", "optional": true}], ["properties"]);

This is out of date. This includes "objectGroup" which doesn't exist in the DOM.json protocol definition. Regenerate this file:

    shell> ./Source/WebInspectorUI/Scripts/update-InspectorBackendCommands.rb

[James Craig]

(In reply to comment #20)

> > Source/WebInspectorUI/UserInterface/DOMNodeDetailsSidebarPanel.js:252
> > +                    role = WebInspector.UIString("No exact recognized role match.");
> 
> Hmm, as a user I wouldn't quite understand what this string means.

I think I originally called it, "No exact ARIA role match." Do you think that would be more understandable?

[James Craig]

(In reply to comment #20)
> (From update of attachment 223524 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223524&action=review
> 
> Included some comments. Looks good overall.

Thanks. I'll put up a new patch with all the suggested changes.

> Nit: I prefer the ordering name, description, parameters, returns. Easier on the eyes.

Done, but if this bugs you, you might want to raise another bug to rearrange the rest of that file, as almost none of the rest order it that.

> Also, it would be nice to have a small protocol test for this:
> LayoutTests/inspector-protocol/dom/getAccessibilityPropertiesForDebugger.html
> 
> We prefer any new protocol methods to include a test. But since there are probably no good tests for the DOM domain right now I think we can avoid it since you have such a nice LayoutTest for the AX portion of this.

Okay. I went ahead and raised another bug <http://webkit.org/b/128493> to track the additional protocol test. I have some more features in mind for the Accessibility Node Inspector that should be tested in the protocol, even though the 1.0 version has good coverage in the accessibility layout test.

[James Craig]

Created attachment 223619 [details]
patch with review feedback

[Timothy Hatcher]

Comment on attachment 223619 [details]
patch with review feedback

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:1582
> +static ARIARoleMap* ariaRoleMap()

This should be: static ARIARoleMap& ariaRoleMap().

> Source/WebCore/accessibility/AccessibilityObject.cpp:1588
> +static ARIAReverseRoleMap* reverseAriaRoleMap()

This should be: static ARIAReverseRoleMap& reverseAriaRoleMap()

> Source/WebCore/accessibility/AccessibilityObject.cpp:1598
> +    ARIARoleMap* roleMap = ariaRoleMap();

Would use a reference here then. We are trying to remove pointers where possible.

> Source/WebCore/accessibility/AccessibilityObject.cpp:1618
> +    ARIAReverseRoleMap* roleMap = reverseAriaRoleMap();
> +    return roleMap->get(roleValue());

No need for the local, put on one line.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2114
> +    // AXARIARole is only used by DumpRenderTree (so far).
> +    if ([attributeName isEqualToString:@"AXARIARole"])
> +        return [self computedRoleString];

Is this exposed to the web?

> Source/WebCore/inspector/protocol/DOM.json:61
> +                { "name": "accessibilityIsIgnored", "type": "boolean", "description": "Returns whether the DOM node is exposed to the accessibility API." },

accessibilityIsIgnored is kind of wordy given that it is in AccessibilityProperties object. This could just be "ignored" or "hidden"?

> Source/WebInspectorUI/UserInterface/DOMNode.js:447
> +    accessibilityProperties: function(callback)
> +    {
> +        DOMAgent.getAccessibilityPropertiesForNode(this.id, callback);
> +    },

This leaks the protocol callback and result object into the Inspector. We try to insulate the other classes by keeping all the protocol objects and callbacks in the Manager classes. Some of the older DOM code still does not do this, but new code should.

This should have a local callback that gets passed to DOMAgent.getAccessibilityPropertiesForNode and that local callback checks for errors, and makes a new object for the properties before calling the client callback. This allows us to insulate the calls from future compatibility changes.

> Source/WebInspectorUI/UserInterface/DOMNodeDetailsSidebarPanel.js:255
> +                        role = WebInspector.UIString("%s (default)").replace("%s", role);

Use: WebInspector.UIString("%s (default)").format(role);

> Source/WebInspectorUI/UserInterface/DOMNodeDetailsSidebarPanel.js:257
> +                        role = WebInspector.UIString("%s (computed)").replace("%s", role);

Ditto.

> Source/WebInspectorUI/UserInterface/DOMNodeDetailsSidebarPanel.js:262
> +                if (label && label != domNode.getAttribute("aria-label"))

!==

> Source/WebInspectorUI/UserInterface/DetailsSection.css:126
> -    table-layout: fixed;
> +    /* Removed 'table-layout: fixed;' because of WebKit CSS bug: http://webkit.org/b/128294 */

Just remove the whole line. We don't commit commented out code. We might not even need to add it back, layout seems fine without it.

[James Craig]

(In reply to comment #24)
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2114
> > +    // AXARIARole is only used by DumpRenderTree (so far).
> > +    if ([attributeName isEqualToString:@"AXARIARole"])
> > +        return [self computedRoleString];
> 
> Is this exposed to the web?

Do you mean through a public DOM interface on Element? It's possible this will be added in ARIA 1.1, but nothing public yet. 

> > Source/WebInspectorUI/UserInterface/DetailsSection.css:126
> > -    table-layout: fixed;
> > +    /* Removed 'table-layout: fixed;' because of WebKit CSS bug: http://webkit.org/b/128294 */
> 
> Just remove the whole line. We don't commit commented out code. We might not even need to add it back, layout seems fine without it.

I'm worried that, without the comment, someone will add it back and regress this. I don't really consider this commented code. It's just a comment that references code. What about this?

/* Removed fixed table layout because of WebKit CSS bug: http://webkit.org/b/128294 */

[Timothy Hatcher]

(In reply to comment #25)
> (In reply to comment #24)
> > > Source/WebInspectorUI/UserInterface/DetailsSection.css:126
> > > -    table-layout: fixed;
> > > +    /* Removed 'table-layout: fixed;' because of WebKit CSS bug: http://webkit.org/b/128294 */
> > 
> > Just remove the whole line. We don't commit commented out code. We might not even need to add it back, layout seems fine without it.
> 
> I'm worried that, without the comment, someone will add it back and regress this. I don't really consider this commented code. It's just a comment that references code. What about this?
> 
> /* Removed fixed table layout because of WebKit CSS bug: http://webkit.org/b/128294 */

I would just remove it altogether. I don't think we need table-layout: fixed even if that bug is fixed.

[James Craig]

(In reply to comment #24)

> > Source/WebCore/inspector/protocol/DOM.json:61
> > +                { "name": "accessibilityIsIgnored", "type": "boolean", "description": "Returns whether the DOM node is exposed to the accessibility API." },
> 
> accessibilityIsIgnored is kind of wordy given that it is in AccessibilityProperties object. This could just be "ignored" or "hidden"?

I want to leave this one as-is for now. I don't think "presentational", "hidden", or "ignored" are particularly accurate given all the overlapping platform APIs that expose these nodes in different ways. I don't even think accessibilityIsIgnored is particularly accurate, but I kept that name b/c that's what WebCore calls it. A more accurate term might be heuristicallyDeterminedAsIrrelevantForAccessibility, but that's even more verbose, and "irrelevant" may be a little too vague.

I'm sure a better term will present itself at some point, but I'd like to err on the side of the common terminology used in the accessibility part of WebCore.

[James Craig]

Created attachment 223738 [details]
patch with review feedback

addresses remaining review feedback

[Timothy Hatcher]

(In reply to comment #27)
> (In reply to comment #24)
> 
> > > Source/WebCore/inspector/protocol/DOM.json:61
> > > +                { "name": "accessibilityIsIgnored", "type": "boolean", "description": "Returns whether the DOM node is exposed to the accessibility API." },
> > 
> > accessibilityIsIgnored is kind of wordy given that it is in AccessibilityProperties object. This could just be "ignored" or "hidden"?
> 
> I want to leave this one as-is for now. I don't think "presentational", "hidden", or "ignored" are particularly accurate given all the overlapping platform APIs that expose these nodes in different ways. I don't even think accessibilityIsIgnored is particularly accurate, but I kept that name b/c that's what WebCore calls it. A more accurate term might be heuristicallyDeterminedAsIrrelevantForAccessibility, but that's even more verbose, and "irrelevant" may be a little too vague.
> 
> I'm sure a better term will present itself at some point, but I'd like to err on the side of the common terminology used in the accessibility part of WebCore.

We try to lee the wording terse, yet accurate. Repeating the word "accessibility" in an object named AccessibilityProperties is redundant. Think of the protocol as API too, we try not to change it later.

[Samuel White]

(In reply to comment #27)
> (In reply to comment #24)
> 
> > > Source/WebCore/inspector/protocol/DOM.json:61
> > > +                { "name": "accessibilityIsIgnored", "type": "boolean", "description": "Returns whether the DOM node is exposed to the accessibility API." },
> > 
> > accessibilityIsIgnored is kind of wordy given that it is in AccessibilityProperties object. This could just be "ignored" or "hidden"?
> 
> I want to leave this one as-is for now. I don't think "presentational", "hidden", or "ignored" are particularly accurate given all the overlapping platform APIs that expose these nodes in different ways. I don't even think accessibilityIsIgnored is particularly accurate, but I kept that name b/c that's what WebCore calls it. A more accurate term might be heuristicallyDeterminedAsIrrelevantForAccessibility, but that's even more verbose, and "irrelevant" may be a little too vague.
> 
> I'm sure a better term will present itself at some point, but I'd like to err on the side of the common terminology used in the accessibility part of WebCore.

Not to pile on, but I agree with removing the "accessibility" prefix from "accessibilityIsIgnored". The context is enough.

While many WebCore AccessibilityObject variables and methods do use the "accessibility" prefix as you mention, I think most are a mistake and could (should) be removed as we continue to cleanup.

[James Craig]

(In reply to comment #30)

> Not to pile on, but I agree with removing the "accessibility" prefix from "accessibilityIsIgnored". The context is enough.

Okay. Final patch on its way soon.

[James Craig]

Created attachment 223782 [details]
patch with review feedback

Differences in this patch:

- Found one more stubbed method needed for AccessibilityUIElement::computedRoleString() in Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementAtk.mm
- Review: property name changed from "accessibilityIsIgnored" to "ignored"
- Review: modified the following callback mod in Source/WebInspectorUI/UserInterface/DOMNode.js

    accessibilityProperties: function(callback)
    {
        function accessibilityPropertiesCallback(error, accessibilityProperties)
        {
            if (!error && callback) {
                callback({
                    ignored: accessibilityProperties.ignored,
                    role: accessibilityProperties.role,
                    label: accessibilityProperties.label
                });
            }
        }
        DOMAgent.getAccessibilityPropertiesForNode(this.id, accessibilityPropertiesCallback.bind(this));
    },

[WebKit Commit Bot]

Comment on attachment 223782 [details]
patch with review feedback

Clearing flags on attachment: 223782

Committed r163891: <http://trac.webkit.org/changeset/163891>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[James Craig]

Created attachment 223897 [details]
build fix

[Joseph Pecoraro]

I don't think the Commit Queue won't land patches on an already closed bug. I'll land this build fix for you.

[Joseph Pecoraro]

Comment on attachment 223897 [details]
build fix

iOS Build Fix landed in: <http://trac.webkit.org/changeset/163913>

[Darin Adler]

Comment on attachment 223897 [details]
build fix

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

> Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:259
> -JSStringRef AccessibilityUIElement::computedRoleString()
> +JSRetainPtr<JSStringRef> AccessibilityUIElement::computedRoleString()
>  {
>      // FIXME: implement
>      return JSStringCreateWithCharacters(0, 0);

Writing it like this will result in a leak of every single one of these strings; needs an adopt since JSStringCreateWithCharacters needs to be balanced by a release.

[Darin Adler]

Comment on attachment 223897 [details]
build fix

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

>> Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:259
>>      return JSStringCreateWithCharacters(0, 0);
> 
> Writing it like this will result in a leak of every single one of these strings; needs an adopt since JSStringCreateWithCharacters needs to be balanced by a release.

Needs to be:

    return adopt(JSStringCreateWithCharacters(nullptr, 0));

[James Craig]

(In reply to comment #39)
> (From update of attachment 223897 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223897&action=review
> 
> >> Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:259
> >>      return JSStringCreateWithCharacters(0, 0);
> > 
> > Writing it like this will result in a leak of every single one of these strings; needs an adopt since JSStringCreateWithCharacters needs to be balanced by a release.
> 
> Needs to be:
> 
>     return adopt(JSStringCreateWithCharacters(nullptr, 0));

Since the entire file (and others) are done that way, I'll file another bug for that fix.

[James Craig]

Bug 128630 - AX: AccessibilityUIElement leaks in DumpRenderTree and WebKitTestRunner