Bug 45156 - Send webkit accessibility notifications to Chromium
Summary: Send webkit accessibility notifications to Chromium
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-02 18:34 PDT by Chris Guillory
Modified: 2010-09-24 00:52 PDT (History)
8 users (show)

See Also:


Attachments
Patch. (45.00 KB, patch)
2010-09-02 18:44 PDT, Chris Guillory
no flags Details | Formatted Diff | Diff
Patch (44.76 KB, patch)
2010-09-02 19:28 PDT, Chris Guillory
no flags Details | Formatted Diff | Diff
Patch (44.57 KB, patch)
2010-09-02 19:50 PDT, Chris Guillory
no flags Details | Formatted Diff | Diff
Update LayoutTests expectations (2.22 KB, patch)
2010-09-02 23:08 PDT, Chris Guillory
dglazkov: review-
dglazkov: commit-queue-
Details | Formatted Diff | Diff
Send accessibility notifications and update chromium test expectations (46.20 KB, patch)
2010-09-03 09:00 PDT, Chris Guillory
cfleizach: review-
cfleizach: commit-queue-
Details | Formatted Diff | Diff
Print id if available in dumpAccessibilityNotifications (45.88 KB, patch)
2010-09-08 13:53 PDT, Chris Guillory
no flags Details | Formatted Diff | Diff
Removed argument names in WebViewClient.h (45.86 KB, patch)
2010-09-08 14:00 PDT, Chris Guillory
no flags Details | Formatted Diff | Diff
Resolved conflict in drt_expectations.txt (46.80 KB, patch)
2010-09-08 14:43 PDT, Chris Guillory
no flags Details | Formatted Diff | Diff
Add AssertMatchingEnums.h to pass style check (54.66 KB, patch)
2010-09-09 12:06 PDT, Chris Guillory
no flags Details | Formatted Diff | Diff
Rebasing ChangeLog files (49.40 KB, patch)
2010-09-09 13:57 PDT, Chris Guillory
no flags Details | Formatted Diff | Diff
Rebased all non ChangeLog files - drt_expectations.txt conflict (46.20 KB, patch)
2010-09-10 11:36 PDT, Chris Guillory
cfleizach: review-
cfleizach: commit-queue-
Details | Formatted Diff | Diff
Not using AssertMatchingEnums. Rebased. (48.22 KB, patch)
2010-09-13 01:36 PDT, Chris Guillory
no flags Details | Formatted Diff | Diff
Some of the layout test results had an extra newline (48.21 KB, patch)
2010-09-13 01:46 PDT, Chris Guillory
no flags Details | Formatted Diff | Diff
Recreating patch to fix patch errors. (48.21 KB, patch)
2010-09-13 13:21 PDT, Chris Guillory
no flags Details | Formatted Diff | Diff
Patch (1.57 KB, patch)
2010-09-14 18:54 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff
Send remaining webkit notifications to Chromium (31.22 KB, patch)
2010-09-20 21:38 PDT, Chris Guillory
cfleizach: review-
Details | Formatted Diff | Diff
Update patch (31.37 KB, patch)
2010-09-21 10:34 PDT, Chris Guillory
no flags Details | Formatted Diff | Diff
Reland patch (30.55 KB, patch)
2010-09-22 10:20 PDT, Chris Guillory
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Guillory 2010-09-02 18:34:14 PDT
Chromium needs accessibility notifications to provide better accessibility support.
Comment 1 Chris Guillory 2010-09-02 18:44:01 PDT
Created attachment 66451 [details]
Patch.
Comment 2 WebKit Review Bot 2010-09-02 18:49:29 PDT
Attachment 66451 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
ERROR: Unexpected diff format when parsing a chunk: '</ul>'
WebKit/chromium/src/AssertMatchingEnums.cpp:39:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/chromium/src/AssertMatchingEnums.cpp:55:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/chromium/src/ChromeClientImpl.cpp:722:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebKitTools/DumpRenderTree/chromium/WebViewHost.cpp:492:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebKitTools/DumpRenderTree/chromium/WebViewHost.cpp:495:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 5 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Chris Guillory 2010-09-02 19:28:47 PDT
Created attachment 66455 [details]
Patch

I've cleaned up some style issues and fixed up the layout test files that had an extra "AccessibilityNotification - ChildrenChanged".
Comment 4 Chris Guillory 2010-09-02 19:50:51 PDT
Created attachment 66463 [details]
Patch

More fixes for ""AccessibilityNotification - ChildrenChanged". I've been having issues with getting expectations using new-run-webkit-tests.
Comment 5 Dimitri Glazkov (Google) 2010-09-02 20:59:04 PDT
Comment on attachment 66463 [details]
Patch

This looks great, but you'll need to somehow figure out how to make these test either not run or be WONTFIX in test_shell, but run and expect to pass in Chromium DRT.
Comment 6 Chris Guillory 2010-09-02 23:08:13 PDT
Created attachment 66472 [details]
Update LayoutTests expectations

Oh yeah. Here's another patch to update the test expectations. I can put it all in one patch if that's better otherwise this can land first.
Comment 7 Dimitri Glazkov (Google) 2010-09-03 06:59:31 PDT
Comment on attachment 66472 [details]
Update LayoutTests expectations

Yeah, one patch would be better for archeological reasons.
Comment 8 Chris Guillory 2010-09-03 09:00:39 PDT
Created attachment 66504 [details]
Send accessibility notifications and update chromium test expectations

Here it is in one patch.
Comment 9 Eric Seidel (no email) 2010-09-07 03:18:56 PDT
Comment on attachment 66463 [details]
Patch

Cleared Dimitri Glazkov's review+ from obsolete attachment 66463 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 10 chris fleizach 2010-09-07 23:20:46 PDT
Comment on attachment 66504 [details]
Send accessibility notifications and update chromium test expectations


>  
>          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
Comment 11 Chris Guillory 2010-09-08 00:06:47 PDT
I've made all the small changes.

(In reply to comment #10)
> 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?
I'm following the way enums are brought from WebCore to WebKit that I see in AssertMatchingEnums.cpp. I'm not sure how I'd avoid maintenance. Dimitri, do you have some guidance here?

> seems wrong to print the help text in dumpeAccessibilityNotifications.
I'm trying to unique identify obj of the notification. What do you think if I print the id of the obj's node if it's available?
Comment 12 Dimitri Glazkov (Google) 2010-09-08 09:31:03 PDT
(In reply to comment #11)
> I've made all the small changes.
> 
> (In reply to comment #10)
> > 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?
> I'm following the way enums are brought from WebCore to WebKit that I see in AssertMatchingEnums.cpp. I'm not sure how I'd avoid maintenance. Dimitri, do you have some guidance here?

Unfortunately, we have to do this to ensure integrity of the port. It's ugly, but it's what we've got.

> 
> > seems wrong to print the help text in dumpeAccessibilityNotifications.
> I'm trying to unique identify obj of the notification. What do you think if I print the id of the obj's node if it's available?
Comment 13 chris fleizach 2010-09-08 09:31:50 PDT
> > seems wrong to print the help text in dumpeAccessibilityNotifications.
> I'm trying to unique identify obj of the notification. What do you think if I print the id of the obj's node if it's available?

I think that's probably a good idea
Comment 14 Chris Guillory 2010-09-08 13:53:58 PDT
Created attachment 66936 [details]
Print id if available in dumpAccessibilityNotifications

Here's the updated patch.
Comment 15 WebKit Review Bot 2010-09-08 13:56:38 PDT
Attachment 66936 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/AssertMatchingEnums.cpp:36:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 46 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Chris Guillory 2010-09-08 14:00:13 PDT
Created attachment 66938 [details]
Removed argument names in WebViewClient.h
Comment 17 WebKit Review Bot 2010-09-08 14:04:48 PDT
Attachment 66938 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/AssertMatchingEnums.cpp:36:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 46 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Chris Guillory 2010-09-08 14:43:46 PDT
Created attachment 66944 [details]
Resolved conflict in drt_expectations.txt
Comment 19 WebKit Review Bot 2010-09-08 14:49:59 PDT
Attachment 66944 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/AssertMatchingEnums.cpp:36:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 chris fleizach 2010-09-08 14:50:55 PDT
(In reply to comment #19)
> Attachment 66944 [details] did not pass style-queue:
> 
> Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
> WebKit/chromium/src/AssertMatchingEnums.cpp:36:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
> Total errors found: 1 in 47 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

waiting for style queue to pass before r+
Comment 21 Chris Guillory 2010-09-08 15:23:55 PDT
AssertMatchingEnums.cpp does not pass style because it doesn't
#include "AssertMatchingEnums.h"
No such file exists.
Comment 22 chris fleizach 2010-09-08 15:29:44 PDT
Comment on attachment 66944 [details]
Resolved conflict in drt_expectations.txt

r=me
Comment 23 WebKit Commit Bot 2010-09-09 00:43:20 PDT
Comment on attachment 66944 [details]
Resolved conflict in drt_expectations.txt

Rejecting patch 66944 from commit-queue.

Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1
Last 500 characters of output:
romium/public/WebViewClient.h
	M	WebKit/chromium/src/AssertMatchingEnums.cpp
	M	WebKit/chromium/src/ChromeClientImpl.cpp
	M	WebKit/chromium/src/ChromeClientImpl.h
	M	WebKitTools/ChangeLog
	M	WebKitTools/DumpRenderTree/chromium/WebViewHost.cpp
	M	WebKitTools/DumpRenderTree/chromium/WebViewHost.h
RA layer request failed: MERGE request failed on '/repository/webkit/trunk': MERGE of '/repository/webkit/trunk': 409 Conflict (http://svn.webkit.org) at /usr/local/git/libexec/git-core/git-svn line 572


Full output: http://queues.webkit.org/results/3951292
Comment 24 Adam Barth 2010-09-09 06:49:00 PDT
Comment on attachment 66944 [details]
Resolved conflict in drt_expectations.txt

That's a new error.  Let's try that again.
Comment 25 WebKit Commit Bot 2010-09-09 07:38:26 PDT
Comment on attachment 66944 [details]
Resolved conflict in drt_expectations.txt

Rejecting patch 66944 from commit-queue.

Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1
Last 500 characters of output:
romium/public/WebViewClient.h
	M	WebKit/chromium/src/AssertMatchingEnums.cpp
	M	WebKit/chromium/src/ChromeClientImpl.cpp
	M	WebKit/chromium/src/ChromeClientImpl.h
	M	WebKitTools/ChangeLog
	M	WebKitTools/DumpRenderTree/chromium/WebViewHost.cpp
	M	WebKitTools/DumpRenderTree/chromium/WebViewHost.h
RA layer request failed: MERGE request failed on '/repository/webkit/trunk': MERGE of '/repository/webkit/trunk': 409 Conflict (http://svn.webkit.org) at /usr/local/git/libexec/git-core/git-svn line 572


Full output: http://queues.webkit.org/results/3935338
Comment 26 Chris Guillory 2010-09-09 12:06:28 PDT
Created attachment 67075 [details]
Add AssertMatchingEnums.h to pass style check

The previous patch failed to commit with the commit-queue. This patch passes style.
Comment 27 chris fleizach 2010-09-09 13:28:27 PDT
Comment on attachment 67075 [details]
Add AssertMatchingEnums.h to pass style check

WebKitTools changelog looks busted
Comment 28 Chris Guillory 2010-09-09 13:57:46 PDT
Created attachment 67093 [details]
Rebasing ChangeLog files
Comment 29 chris fleizach 2010-09-09 14:15:51 PDT
Comment on attachment 67093 [details]
Rebasing ChangeLog files

r=me
Comment 30 WebKit Commit Bot 2010-09-10 11:15:43 PDT
Comment on attachment 67093 [details]
Rebasing ChangeLog files

Rejecting patch 67093 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Chris Fleizach', u'--force']" exit_code: 1
Last 500 characters of output:
enChanged.html
patching file LayoutTests/platform/chromium/accessibility/post-notification-SelectedTextChanged-expected.txt
patching file LayoutTests/platform/chromium/accessibility/post-notification-SelectedTextChanged.html
patching file LayoutTests/platform/chromium/accessibility/post-notification-ValueChanged-expected.txt
patching file LayoutTests/platform/chromium/accessibility/post-notification-ValueChanged.html
patching file LayoutTests/platform/chromium/accessibility/post-notification.js

Full output: http://queues.webkit.org/results/3899372
Comment 31 Chris Guillory 2010-09-10 11:36:17 PDT
Created attachment 67209 [details]
Rebased all non ChangeLog files - drt_expectations.txt conflict

http://trac.webkit.org/changeset/67181 created conflict in drt_expectations.txt. Here's a rebased patch.
Comment 32 Eric Seidel (no email) 2010-09-10 23:03:43 PDT
Comment on attachment 66944 [details]
Resolved conflict in drt_expectations.txt

Cleared Chris Fleizach's review+ from obsolete attachment 66944 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 33 Eric Seidel (no email) 2010-09-10 23:03:48 PDT
Comment on attachment 67093 [details]
Rebasing ChangeLog files

Cleared Chris Fleizach's review+ from obsolete attachment 67093 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 34 chris fleizach 2010-09-12 20:08:56 PDT
Comment on attachment 67209 [details]
Rebased all non ChangeLog files - drt_expectations.txt conflict

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 66699)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,33 @@
> +2010-09-02  Chris Guillory   <chris.guillory@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@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@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@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?
Comment 35 Chris Guillory 2010-09-12 23:13:58 PDT
> 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;
> }
I see what you're saying now. We should be able to assign the values directly in the enum definition.

> didn't we fix this before?
We fixed a lot of this before. I must have uploaded an old patch by mistake.
Comment 36 Chris Guillory 2010-09-13 01:36:55 PDT
Created attachment 67375 [details]
Not using AssertMatchingEnums. Rebased.
Comment 37 Chris Guillory 2010-09-13 01:46:58 PDT
Created attachment 67378 [details]
Some of the layout test results had an extra newline
Comment 38 Chris Guillory 2010-09-13 13:21:10 PDT
Created attachment 67464 [details]
Recreating patch to fix patch errors.
Comment 39 chris fleizach 2010-09-14 12:34:30 PDT
Comment on attachment 67464 [details]
Recreating patch to fix patch errors.

r=me
Comment 40 WebKit Commit Bot 2010-09-14 13:41:01 PDT
Comment on attachment 67464 [details]
Recreating patch to fix patch errors.

Rejecting patch 67464 from commit-queue.

Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1
Last 500 characters of output:
eLog
	M	WebKit/chromium/WebKit.gyp
	M	WebKit/chromium/public/WebViewClient.h
	M	WebKit/chromium/src/ChromeClientImpl.cpp
	M	WebKit/chromium/src/ChromeClientImpl.h
	M	WebKitTools/ChangeLog
	M	WebKitTools/DumpRenderTree/chromium/WebViewHost.cpp
	M	WebKitTools/DumpRenderTree/chromium/WebViewHost.h
RA layer request failed: MERGE request failed on '/repository/webkit/trunk': MERGE of '/repository/webkit/trunk': 409 Conflict (http://svn.webkit.org) at /usr/local/git/libexec/git-core/git-svn line 572


Full output: http://queues.webkit.org/results/4002007
Comment 41 Chris Guillory 2010-09-14 13:55:20 PDT
Second time this commit-queue error has occurred with this patch. I get no conflict like this when I update my svn client. Can this patch be submitted manually?
Comment 42 Adam Barth 2010-09-14 14:28:11 PDT
(In reply to comment #41)
> Second time this commit-queue error has occurred with this patch. I get no conflict like this when I update my svn client. Can this patch be submitted manually?

    The following files contain tab characters:

        trunk/LayoutTests/platform/chromium/accessibility/post-notification-MenuListValueChanged.html

    Please use spaces instead to indent.
    If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
Comment 43 Adam Barth 2010-09-14 14:29:54 PDT
Committed r67498: <http://trac.webkit.org/changeset/67498>
Comment 44 Mihai Parparita 2010-09-14 18:38:33 PDT
This causes a lot of tests to fail with the Chromium DRT, for example editing/undo/5738768.html:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=editing%2Fundo%2F5738768.html&upstreamWebKit=true

It looks like a lot of AccessibilityNotification being printed out, caused by a stray print left in WebViewHost::postAccessibilityNotification.
Comment 45 Mihai Parparita 2010-09-14 18:54:32 PDT
Created attachment 67632 [details]
Patch
Comment 46 Adam Barth 2010-09-14 18:59:31 PDT
Comment on attachment 67632 [details]
Patch

Clearing flags on attachment: 67632

Committed r67527: <http://trac.webkit.org/changeset/67527>
Comment 47 Adam Barth 2010-09-14 18:59:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 48 Chris Guillory 2010-09-20 21:38:27 PDT
Created attachment 68187 [details]
Send remaining webkit notifications to Chromium
Comment 49 Chris Guillory 2010-09-20 21:44:54 PDT
Need remaining notifications.
Comment 50 chris fleizach 2010-09-21 09:57:09 PDT
Comment on attachment 68187 [details]
Send remaining webkit notifications to Chromium

View in context: https://bugs.webkit.org/attachment.cgi?id=68187&action=review

> WebCore/ChangeLog:6
> +        https://bugs.webkit.org/show_bug.cgi?id=45156

comment should end with a period

> WebCore/accessibility/chromium/AXObjectCacheChromium.cpp:88
> +    case AXRowExpanded:

these cases should be alphabetized

> WebCore/accessibility/chromium/AXObjectCacheChromium.cpp:109
> +

this ASSERT might not be correct. for example, this is possible

<div role="group" aria-hidden="true" tabindex="0">test</div>

where the div should be ignored, but it can be focused

> WebCore/editing/chromium/SelectionControllerChromium.cpp:42
> +

seems like you don't need to access document until it's inside the if block, so there's no need to declare out here
Comment 51 Chris Guillory 2010-09-21 10:34:33 PDT
Created attachment 68259 [details]
Update patch

Thanks. Here an updated patch.
Comment 52 chris fleizach 2010-09-21 10:49:43 PDT
Comment on attachment 68259 [details]
Update patch

r=me
Comment 53 WebKit Commit Bot 2010-09-21 13:22:29 PDT
Comment on attachment 68259 [details]
Update patch

Clearing flags on attachment: 68259

Committed r67982: <http://trac.webkit.org/changeset/67982>
Comment 54 WebKit Commit Bot 2010-09-21 13:22:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 55 Chris Guillory 2010-09-22 10:20:21 PDT
Created attachment 68389 [details]
Reland patch

The previous revision http://trac.webkit.org/changeset/67982 was reverted due to bug 46237.
Comment 56 Chris Guillory 2010-09-22 10:24:04 PDT
Marking unconfirmed.
Comment 57 chris fleizach 2010-09-23 22:46:29 PDT
Comment on attachment 68389 [details]
Reland patch

r=me
Comment 58 WebKit Commit Bot 2010-09-24 00:51:58 PDT
Comment on attachment 68389 [details]
Reland patch

Clearing flags on attachment: 68389

Committed r68240: <http://trac.webkit.org/changeset/68240>
Comment 59 WebKit Commit Bot 2010-09-24 00:52:08 PDT
All reviewed patches have been landed.  Closing bug.