[Webkit-unassigned] [Bug 45156] Send webkit accessibility notifications to Chromium

[bugzilla-daemon at webkit.org]

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


chris fleizach <cfleizach at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #66504|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #10 from chris fleizach <cfleizach at apple.com>  2010-09-07 23:20:46 PST ---
(From update of attachment 66504)

>  
>          Unreviewed, updating binding-tests expectations (for changeset 66521).
> Index: WebCore/accessibility/chromium/AXObjectCacheChromium.cpp
> ===================================================================
> --- WebCore/accessibility/chromium/AXObjectCacheChromium.cpp	(revision 66453)
> +++ WebCore/accessibility/chromium/AXObjectCacheChromium.cpp	(working copy)
> @@ -61,28 +61,13 @@ void AXObjectCache::postPlatformNotifica
>  
>      ChromeClientChromium* client = toChromeClientChromium(obj->documentFrameView());
>      if (client) {

You should use the early return idiom here if client == null.

> -        switch (notification) {
> -        case AXCheckedStateChanged:
> +        client->postAccessibilityNotification(obj, notification);
> +
> +        // TODO: Remove after the new postAccessibilityNotification is used downstream.
> +        if (notification == AXCheckedStateChanged)
>              client->didChangeAccessibilityObjectState(obj);
> -            break;
> -        case AXChildrenChanged:
> +        if (notification == AXChildrenChanged)
>              client->didChangeAccessibilityObjectChildren(obj);
> -            break;

looks like this should still use the switch () until its removed.


> +    
> +    // Notifies embedder about an accessibility notification.
> +    virtual void postAccessibilityNotification(AccessibilityObject* obj, AXObjectCache::AXNotification notification) = 0;
>  };

argument names not necessary here.

> +
> +// These values must match WebCore::AXObjectCache::AXNotification values
> +enum WebAccessibilityNotification {
> +    ActiveDescendantChanged,
> +    CheckedStateChanged,
> +    ChildrenChanged,
> +    FocusedUIElementChanged,
> +    LayoutComplete,
> +    LoadComplete,
> +    SelectedChildrenChanged,
> +    SelectedTextChanged,
> +    ValueChanged,
> +    ScrolledToAnchor,
> +    LiveRegionChanged,
> +    MenuListValueChanged,
> +    RowCountChanged,
> +    RowCollapsed,
> +    RowExpanded,
> +};
> +

is there a way to avoid this duplication? it seems a hackish to have two copies that have to be kept up to date. can you export the AX notifications or the header so that you can reference those directly?

> +    
> +    // Notifies embedder about an accessibility notification.
> +    virtual void postAccessibilityNotification(const WebAccessibilityObject& obj, WebAccessibilityNotification notification) { }

argument names not necessary

>  
> --- WebKit/chromium/src/ChromeClientImpl.h	(revision 66453)
> +++ WebKit/chromium/src/ChromeClientImpl.h	(working copy)
> @@ -163,6 +163,8 @@ public:
>      virtual void popupClosed(WebCore::PopupContainer* popupContainer);
>      virtual void didChangeAccessibilityObjectState(WebCore::AccessibilityObject*);
>      virtual void didChangeAccessibilityObjectChildren(WebCore::AccessibilityObject*);
> +    virtual void postAccessibilityNotification(
> +        WebCore::AccessibilityObject* obj, WebCore::AXObjectCache::AXNotification notification);
>  

argument names not necessary, same with the line breaks



> +    if (m_shell->accessibilityController()->shouldDumpAccessibilityNotifications()) {
> +        switch (notification) {
> +        case ActiveDescendantChanged:
> +            printf("AccessibilityNotification - ActiveDescendantChanged");

seems like you should printf "AccessibilityNotification - ") first, then just the name within the switch

> +
> +        if (!obj.helpText().isNull())
> +            printf(" - helpText:%s", obj.helpText().utf8().data());
> +

seems wrong to print the help text in dumpeAccessibilityNotifications.


> +    virtual void postAccessibilityNotification(const WebKit::WebAccessibilityObject& obj, WebKit::WebAccessibilityNotification notification);

no argument names

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