[Webkit-unassigned] [Bug 127447] Web Inspector: AX: Accessibility Node Inspection

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=127447





--- Comment #3 from Joseph Pecoraro <joepeck at webkit.org>  2014-01-22 15:12:36 PST ---
(From update of attachment 221905)
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);

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.