Bug 45156 – Send webkit accessibility notifications to Chromium

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Chris Guillory]

Chromium needs accessibility notifications to provide better accessibility support.

[Chris Guillory]

Created attachment 66451 [details]
Patch.

[WebKit Review Bot]

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.

[Chris Guillory]

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

[Chris Guillory]

Created attachment 66463 [details]
Patch

More fixes for ""AccessibilityNotification - ChildrenChanged". I've been having issues with getting expectations using new-run-webkit-tests.

[Dimitri Glazkov (Google)]

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.

[Chris Guillory]

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.

[Dimitri Glazkov (Google)]

Comment on attachment 66472 [details]
Update LayoutTests expectations

Yeah, one patch would be better for archeological reasons.

[Chris Guillory]

Created attachment 66504 [details]
Send accessibility notifications and update chromium test expectations

Here it is in one patch.

[Eric Seidel (no email)]

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.

[chris fleizach]

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

[Chris Guillory]

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?

[Dimitri Glazkov (Google)]

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

[chris fleizach]

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

[Chris Guillory]

Created attachment 66936 [details]
Print id if available in dumpAccessibilityNotifications

Here's the updated patch.

[WebKit Review Bot]

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.

[Chris Guillory]

Created attachment 66938 [details]
Removed argument names in WebViewClient.h

[WebKit Review Bot]

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.

[Chris Guillory]

Created attachment 66944 [details]
Resolved conflict in drt_expectations.txt

[WebKit Review Bot]

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.

[chris fleizach]

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

[Chris Guillory]

AssertMatchingEnums.cpp does not pass style because it doesn't
#include "AssertMatchingEnums.h"
No such file exists.

[chris fleizach]

Comment on attachment 66944 [details]
Resolved conflict in drt_expectations.txt

r=me

[WebKit Commit Bot]

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

[Adam Barth]

Comment on attachment 66944 [details]
Resolved conflict in drt_expectations.txt

That's a new error.  Let's try that again.

[WebKit Commit Bot]

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

[Chris Guillory]

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.

[chris fleizach]

Comment on attachment 67075 [details]
Add AssertMatchingEnums.h to pass style check

WebKitTools changelog looks busted

[Chris Guillory]

Created attachment 67093 [details]
Rebasing ChangeLog files

[chris fleizach]

Comment on attachment 67093 [details]
Rebasing ChangeLog files

r=me

[WebKit Commit Bot]

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

[Chris Guillory]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[chris fleizach]

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 @@
> [phone]  Chris Guillory   <[e-mail]>
> +
> +        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  <[e-mail]>
>  
>          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 @@
> [phone]  Chris Guillory   <[e-mail]>
> +
> +        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  <[e-mail]>
>  
>          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?

[Chris Guillory]

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

[Chris Guillory]

Created attachment 67375 [details]
Not using AssertMatchingEnums. Rebased.

[Chris Guillory]

Created attachment 67378 [details]
Some of the layout test results had an extra newline

[Chris Guillory]

Created attachment 67464 [details]
Recreating patch to fix patch errors.

[chris fleizach]

Comment on attachment 67464 [details]
Recreating patch to fix patch errors.

r=me

[WebKit Commit Bot]

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

[Chris Guillory]

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?

[Adam Barth]

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

[Adam Barth]

Committed r67498: <http://trac.webkit.org/changeset/67498>

[Mihai Parparita]

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.

[Mihai Parparita]

Created attachment 67632 [details]
Patch

[Adam Barth]

Comment on attachment 67632 [details]
Patch

Clearing flags on attachment: 67632

Committed r67527: <http://trac.webkit.org/changeset/67527>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.

[Chris Guillory]

Created attachment 68187 [details]
Send remaining webkit notifications to Chromium

[Chris Guillory]

Need remaining notifications.

[chris fleizach]

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

[Chris Guillory]

Created attachment 68259 [details]
Update patch

Thanks. Here an updated patch.

[chris fleizach]

Comment on attachment 68259 [details]
Update patch

r=me

[WebKit Commit Bot]

Comment on attachment 68259 [details]
Update patch

Clearing flags on attachment: 68259

Committed r67982: <http://trac.webkit.org/changeset/67982>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Guillory]

Created attachment 68389 [details]
Reland patch

The previous revision http://trac.webkit.org/changeset/67982 was reverted due to bug 46237.

[Chris Guillory]

Marking unconfirmed.

[chris fleizach]

Comment on attachment 68389 [details]
Reland patch

r=me

[WebKit Commit Bot]

Comment on attachment 68389 [details]
Reland patch

Clearing flags on attachment: 68389

Committed r68240: <http://trac.webkit.org/changeset/68240>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.