[webkit-reviews] review denied: [Bug 45156] Send webkit accessibility notifications to Chromium : [Attachment 66504] Send accessibility notifications and update chromium test expectations

[bugzilla-daemon at webkit.org]

chris fleizach <cfleizach at apple.com> has denied chris.guillory at google.com's
request for review:
Bug 45156: Send webkit accessibility notifications to Chromium
https://bugs.webkit.org/show_bug.cgi?id=45156

Attachment 66504: Send accessibility notifications and update chromium test
expectations
https://bugs.webkit.org/attachment.cgi?id=66504&action=review

------- Additional Comments from chris fleizach <cfleizach at apple.com>

>  
>	   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