[webkit-reviews] review denied: [Bug 45156] Send webkit accessibility notifications to Chromium : [Attachment 67209] Rebased all non ChangeLog files - drt_expectations.txt conflict

[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 67209: Rebased all non ChangeLog files - drt_expectations.txt
conflict
https://bugs.webkit.org/attachment.cgi?id=67209&action=review

------- Additional Comments from chris fleizach <cfleizach at apple.com>
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 66699)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,33 @@
> +2010-09-02  Chris Guillory	<chris.guillory at google.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Send all accessibility notifications to Chromium
> +	   https://bugs.webkit.org/show_bug.cgi?id=45156
> +	   
> +	   Use postAccessibilityNotification to pass accessibility notification

> +	   to chromium.
> +
> +	   Tests:
platform/chromium/accessibility/post-notification-ActiveDescendantChanged.html
> +		 
platform/chromium/accessibility/post-notification-CheckedStateChanged.html
> +		 
platform/chromium/accessibility/post-notification-ChildrenChanged.html
> +		 
platform/chromium/accessibility/post-notification-FocusedUIElementChanged.html
> +		 
platform/chromium/accessibility/post-notification-LayoutComplete.html
> +		 
platform/chromium/accessibility/post-notification-LiveRegionChanged.html
> +		 
platform/chromium/accessibility/post-notification-LoadComplete.html
> +		 
platform/chromium/accessibility/post-notification-MenuListValueChanged.html
> +		 
platform/chromium/accessibility/post-notification-RowCollapsed.html
> +		 
platform/chromium/accessibility/post-notification-RowCountChanged.html
> +		 
platform/chromium/accessibility/post-notification-RowExpanded.html
> +		 
platform/chromium/accessibility/post-notification-ScrolledToAnchor.html
> +		 
platform/chromium/accessibility/post-notification-SelectedChildrenChanged.html
> +		 
platform/chromium/accessibility/post-notification-SelectedTextChanged.html
> +		 
platform/chromium/accessibility/post-notification-ValueChanged.html
> +
> +	   * accessibility/chromium/AXObjectCacheChromium.cpp:
> +	   (WebCore::AXObjectCache::postPlatformNotification):
> +	   * page/chromium/ChromeClientChromium.h:
> +
>  2010-09-02  Kinuko Yasuda  <kinuko at chromium.org>
>  
>	   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) {

early return after null client is needed here.
> +    
> +    // Notifies embedder about an accessibility notification.
> +    virtual void postAccessibilityNotification(AccessibilityObject* obj,
AXObjectCache::AXNotification notification) = 0;
>  };

no argument names

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

no argument names

> +COMPILE_ASSERT_MATCHING_ENUM(ActiveDescendantChanged,
AXObjectCache::AXActiveDescendantChanged);
> +COMPILE_ASSERT_MATCHING_ENUM(CheckedStateChanged,
AXObjectCache::AXCheckedStateChanged);
> +COMPILE_ASSERT_MATCHING_ENUM(ChildrenChanged,
AXObjectCache::AXChildrenChanged);
> +COMPILE_ASSERT_MATCHING_ENUM(FocusedUIElementChanged,
AXObjectCache::AXFocusedUIElementChanged);
> +COMPILE_ASSERT_MATCHING_ENUM(LayoutComplete,
AXObjectCache::AXLayoutComplete);
> +COMPILE_ASSERT_MATCHING_ENUM(LoadComplete, AXObjectCache::AXLoadComplete);
> +COMPILE_ASSERT_MATCHING_ENUM(SelectedChildrenChanged,
AXObjectCache::AXSelectedChildrenChanged);
> +COMPILE_ASSERT_MATCHING_ENUM(SelectedTextChanged,
AXObjectCache::AXSelectedTextChanged);
> +COMPILE_ASSERT_MATCHING_ENUM(ValueChanged, AXObjectCache::AXValueChanged);
> +COMPILE_ASSERT_MATCHING_ENUM(ScrolledToAnchor,
AXObjectCache::AXScrolledToAnchor);
> +COMPILE_ASSERT_MATCHING_ENUM(LiveRegionChanged,
AXObjectCache::AXLiveRegionChanged);
> +COMPILE_ASSERT_MATCHING_ENUM(MenuListValueChanged,
AXObjectCache::AXMenuListValueChanged);
> +COMPILE_ASSERT_MATCHING_ENUM(RowCountChanged,
AXObjectCache::AXRowCountChanged);
> +COMPILE_ASSERT_MATCHING_ENUM(RowCollapsed, AXObjectCache::AXRowCollapsed);
> +COMPILE_ASSERT_MATCHING_ENUM(RowExpanded, AXObjectCache::AXRowExpanded);
> +

going to re-open this again....
can you not do a runtime lookup of these keys? that way as new ones are added,
other platforms aren't responsible for modifying this list and 
chromium only needs to lookup the ones it needs?

just something like
WebNotification noti = 0;
switch (WebCore::notification) {
   case RowExpanded:
      noti = WebNotificationRowExpanded;
      break;
}


>      virtual void
didChangeAccessibilityObjectState(WebCore::AccessibilityObject*);
>      virtual void
didChangeAccessibilityObjectChildren(WebCore::AccessibilityObject*);
> +    virtual void postAccessibilityNotification(
> +	   WebCore::AccessibilityObject* obj,
WebCore::AXObjectCache::AXNotification notification);
>  

no argument names

>      // ChromeClientImpl:
>      void setCursor(const WebCursorInfo& cursor);
> Index: WebKitTools/ChangeLog
> ===================================================================
> --- WebKitTools/ChangeLog	(revision 66699)
> +++ WebKitTools/ChangeLog	(working copy)
> @@ -1,3 +1,14 @@
> +2010-09-02  Chris Guillory	<chris.guillory at google.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Send all accessibility notifications to Chromium
> +	   https://bugs.webkit.org/show_bug.cgi?id=45156
> +	   
> +	   * DumpRenderTree/chromium/WebViewHost.cpp:
> +	   (WebViewHost::postAccessibilityNotification):
> +	   * DumpRenderTree/chromium/WebViewHost.h:
> +
>  2010-09-02  Steve Block  <steveblock at google.com>
>  
>	   Reviewed by Adam Barth.
> Index: WebKitTools/DumpRenderTree/chromium/WebViewHost.cpp
> ===================================================================
> --- WebKitTools/DumpRenderTree/chromium/WebViewHost.cpp	(revision
66453)
> +++ WebKitTools/DumpRenderTree/chromium/WebViewHost.cpp	(working copy)
> @@ -489,10 +489,62 @@ void WebViewHost::focusAccessibilityObje
>      m_shell->accessibilityController()->setFocusedElement(object);
>  }
>  
> -void WebViewHost::didChangeAccessibilityObjectChildren(const
WebAccessibilityObject& object)
> +void WebViewHost::postAccessibilityNotification(const
WebAccessibilityObject& obj, WebAccessibilityNotification notification)
>  {
> -    if
(m_shell->accessibilityController()->shouldDumpAccessibilityNotifications())
> -	   printf("didChangeAccessibilityObjectChildren - new count: %d\n",
object.childCount());
> +    if
(m_shell->accessibilityController()->shouldDumpAccessibilityNotifications()) {
> +	   switch (notification) {
> +	   case ActiveDescendantChanged:
> +	       printf("AccessibilityNotification - ActiveDescendantChanged");
> +	       break;
> +	   case CheckedStateChanged:
> +	       printf("AccessibilityNotification - CheckedStateChanged");
> +	       break;
> +	   case ChildrenChanged:
> +	       printf("AccessibilityNotification - ChildrenChanged");
> +	       break;
> +	   case FocusedUIElementChanged:
> +	       printf("AccessibilityNotification - FocusedUIElementChanged");
> +	       break;
> +	   case LayoutComplete:
> +	       printf("AccessibilityNotification - LayoutComplete");
> +	       break;
> +	   case LoadComplete:
> +	       printf("AccessibilityNotification - LoadComplete");
> +	       break;
> +	   case SelectedChildrenChanged:
> +	       printf("AccessibilityNotification - SelectedChildrenChanged");
> +	       break;
> +	   case SelectedTextChanged:
> +	       printf("AccessibilityNotification - SelectedTextChanged");
> +	       break;
> +	   case ValueChanged:
> +	       printf("AccessibilityNotification - ValueChanged");
> +	       break;
> +	   case ScrolledToAnchor:
> +	       printf("AccessibilityNotification - ScrolledToAnchor");
> +	       break;
> +	   case LiveRegionChanged:
> +	       printf("AccessibilityNotification - LiveRegionChanged");
> +	       break;
> +	   case MenuListValueChanged:
> +	       printf("AccessibilityNotification - MenuListValueChanged");
> +	       break;
> +	   case RowCountChanged:
> +	       printf("AccessibilityNotification - RowCountChanged");
> +	       break;
> +	   case RowCollapsed:
> +	       printf("AccessibilityNotification - RowCollapsed");
> +	       break;
> +	   case RowExpanded:
> +	       printf("AccessibilityNotification - RowExpanded");
> +	       break;
> +	   }
> +
> +	   if (!obj.helpText().isNull())
> +	       printf(" - helpText:%s", obj.helpText().utf8().data());
> +

didn't we fix this before?