[webkit-reviews] review denied: [Bug 44472] Add AX notification for childrenChanged : [Attachment 65193] Proposed patch

[bugzilla-daemon at webkit.org]

chris fleizach <cfleizach at apple.com> has denied chris.guillory at google.com's
request for review:
Bug 44472: Add AX notification for childrenChanged
https://bugs.webkit.org/show_bug.cgi?id=44472

Attachment 65193: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=65193&action=review

------- Additional Comments from chris fleizach <cfleizach at apple.com>
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 65849)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,18 @@
> +2010-08-23  Chris Guillory	<chris.guillory at google.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Add AX notification for childrenChanged.
> +	   https://bugs.webkit.org/show_bug.cgi?id=44472
> +
> +	   * accessibility/AXObjectCache.h:
> +	   (WebCore::AXObjectCache::):
> +	   * accessibility/AccessibilityRenderObject.cpp:
> +	   (WebCore::AccessibilityRenderObject::childrenChanged):
> +	   * accessibility/chromium/AXObjectCacheChromium.cpp:
> +	   (WebCore::AXObjectCache::postPlatformNotification):
> +	   * page/chromium/ChromeClientChromium.h:
> +
>  2010-08-23  Darin Adler  <darin at apple.com>
>  
>	   Reviewed by Geoff Garen.
> Index: WebCore/accessibility/AXObjectCache.h
> ===================================================================
> --- WebCore/accessibility/AXObjectCache.h	(revision 65848)
> +++ WebCore/accessibility/AXObjectCache.h	(working copy)
> @@ -107,6 +107,7 @@ public:
>      enum AXNotification {
>	   AXActiveDescendantChanged,
>	   AXCheckedStateChanged,
> +	   AXChildrenChanged,
>	   AXFocusedUIElementChanged,
>	   AXLayoutComplete,
>	   AXLoadComplete,
> Index: WebCore/accessibility/AccessibilityRenderObject.cpp
> ===================================================================
> --- WebCore/accessibility/AccessibilityRenderObject.cpp	(revision
65848)
> +++ WebCore/accessibility/AccessibilityRenderObject.cpp	(working copy)
> @@ -3236,6 +3236,8 @@ void AccessibilityRenderObject::children
>      if (!m_renderer)
>	   return;
>      
> +    BOOL sentChildrenChanged = false;
> +    
>      // Go up the accessibility parent chain, but only if the element already
exists. This method is
>      // called during render layouts, minimal work should be done. 
>      // If AX elements are created now, they could interrogate the render
tree while it's in a funky state.
> @@ -3245,6 +3247,12 @@ void AccessibilityRenderObject::children
>	       continue;
>	   
>	   AccessibilityRenderObject* axParent =
static_cast<AccessibilityRenderObject*>(parent);
> +	   
> +	   if (!sentChildrenChanged) {
> +	       axObjectCache()->postNotification(axParent->renderer(),
AXObjectCache::AXChildrenChanged, true);
> +	       sentChildrenChanged = true;
> +	   }
> +	   
>	   // Only do work if the children haven't been marked dirty. This has
the effect of blocking
>	   // future live region change notifications until the AX tree has
been accessed again. This
>	   // is a good performance win for all parties.
> Index: WebCore/accessibility/chromium/AXObjectCacheChromium.cpp
> ===================================================================
> --- WebCore/accessibility/chromium/AXObjectCacheChromium.cpp	(revision
65848)
> +++ WebCore/accessibility/chromium/AXObjectCacheChromium.cpp	(working copy)
> @@ -56,15 +56,23 @@ void AXObjectCache::attachWrapper(Access
>  
>  void AXObjectCache::postPlatformNotification(AccessibilityObject* obj,
AXNotification notification)
>  {
> -    if (notification != AXCheckedStateChanged)
> +    if (notification != AXCheckedStateChanged && notification !=
AXChildrenChanged)
>	   return;
>  
>      if (!obj || !obj->document() || !obj->documentFrameView())
>	   return;
>  
>      ChromeClientChromium* client =
toChromeClientChromium(obj->documentFrameView());
> -    if (client)
> -	   client->didChangeAccessibilityObjectState(obj);
> +    if (client) {
> +	   switch(notification) {
> +	       case AXCheckedStateChanged:
> +		   client->didChangeAccessibilityObjectState(obj);
> +		   break;
> +	       case AXChildrenChanged:
> +		   client->didChangeAccessibilityObjectChildren(obj);
> +		   break;
> +	   }
> +    }
>  }
>  
>  void AXObjectCache::handleFocusedUIElementChanged(RenderObject*,
RenderObject*)
> Index: WebCore/page/chromium/ChromeClientChromium.h
> ===================================================================
> --- WebCore/page/chromium/ChromeClientChromium.h	(revision 65848)
> +++ WebCore/page/chromium/ChromeClientChromium.h	(working copy)
> @@ -55,6 +55,9 @@ public:
>  
>      // Notifies embedder that the state of an accessibility object has
changed.
>      virtual void didChangeAccessibilityObjectState(AccessibilityObject*) =
0;
> +    
> +    // Notified embedder that the children of an accessibility object has
changed.
> +    virtual void didChangeAccessibilityObjectChildren(AccessibilityObject*)
= 0;
>  };
>  
>  } // namespace WebCore
> Index: WebKit/chromium/ChangeLog
> ===================================================================
> --- WebKit/chromium/ChangeLog (revision 65849)
> +++ WebKit/chromium/ChangeLog (working copy)
> @@ -1,3 +1,16 @@
> +2010-08-23  Chris Guillory	<chris.guillory at google.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Add AX notification for childrenChanged.
> +	   https://bugs.webkit.org/show_bug.cgi?id=44472
> +
> +	   * public/WebViewClient.h:
> +	   (WebKit::WebViewClient::didChangeAccessibilityObjectChildren):
> +	   * src/ChromeClientImpl.cpp:
> +	   (WebKit::ChromeClientImpl::didChangeAccessibilityObjectChildren):
> +	   * src/ChromeClientImpl.h:
> +
>  2010-08-23  Kenneth Russell	<kbr at google.com>
>  
>	   Reviewed by Dimitri Glazkov.
> Index: WebKit/chromium/public/WebViewClient.h
> ===================================================================
> --- WebKit/chromium/public/WebViewClient.h	(revision 65848)
> +++ WebKit/chromium/public/WebViewClient.h	(working copy)
> @@ -277,6 +277,9 @@ public:
>  
>      // Notifies embedder that the state of an accessibility object has
changed.
>      virtual void didChangeAccessibilityObjectState(const
WebAccessibilityObject&) { }
> +    
> +    // Notifies embedder that the children of an accessibility object has
changed.
> +    virtual void didChangeAccessibilityObjectChildren(const
WebAccessibilityObject&) { }
>  
>  
>      // Developer tools -----------------------------------------------------

> Index: WebKit/chromium/src/ChromeClientImpl.cpp
> ===================================================================
> --- WebKit/chromium/src/ChromeClientImpl.cpp	(revision 65848)
> +++ WebKit/chromium/src/ChromeClientImpl.cpp	(working copy)
> @@ -712,6 +712,13 @@ void ChromeClientImpl::didChangeAccessib
>	  
m_webView->client()->didChangeAccessibilityObjectState(WebAccessibilityObject(o
bj));
>  }
>  
> +void
ChromeClientImpl::didChangeAccessibilityObjectChildren(WebCore::AccessibilityOb
ject* obj)
> +{
> +    // Alert assistive technology about the accessibility object children
change
> +    if (obj)
> +	  
m_webView->client()->didChangeAccessibilityObjectChildren(WebAccessibilityObjec
t(obj));
> +}
> +
>  #if ENABLE(NOTIFICATIONS)
>  NotificationPresenter* ChromeClientImpl::notificationPresenter() const
>  {
> Index: WebKit/chromium/src/ChromeClientImpl.h
> ===================================================================
> --- WebKit/chromium/src/ChromeClientImpl.h	(revision 65848)
> +++ WebKit/chromium/src/ChromeClientImpl.h	(working copy)
> @@ -167,6 +167,7 @@ public:
>				bool handleExternally);
>      virtual void popupClosed(WebCore::PopupContainer* popupContainer);
>      virtual void
didChangeAccessibilityObjectState(WebCore::AccessibilityObject*);
> +    virtual void
didChangeAccessibilityObjectChildren(WebCore::AccessibilityObject*);
>  
>      // ChromeClientImpl:
>      void setCursor(const WebCursorInfo& cursor);

WebCore/accessibility/AccessibilityRenderObject.cpp:3240
 +	
bool should be lowercase

Can you write a layout test to make sure that you're getting this notification
on chrome?