Bug 34464

Summary: [Chromium] Chromium should receive checkbox state changes
Product: WebKit Reporter: Chris Guillory <ctguil>
Component: AccessibilityAssignee: Chris Guillory <ctguil>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ctguil, dglazkov, dmazzoni, fishd, klinktech, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
[Chromium] Notify Chromium of state change notifications.
none
Notify Chromium of state change notifications.
fishd: review-
Patch updated with comments from Darin
none
Fixing style - line endings.
fishd: review-
Updated with more comments from Darin
none
Removed unneeded AXObjectCache::
eric: review-, fishd: commit-queue-
Added the !obj->document() to fix debug ASSERTS none

Description Chris Guillory 2010-02-01 21:02:30 PST
Ensure that chromium gets notified of AXCheckedStateChanged events so it can send the MSAA event EVENT_OBJECT_STATECHANGE.
Comment 1 Chris Guillory 2010-02-02 09:40:30 PST
Created attachment 47942 [details]
[Chromium] Notify Chromium of state change notifications.
Comment 2 WebKit Review Bot 2010-02-02 09:45:33 PST
Attachment 47942 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/chromium/ChromiumBridge.h:53:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Chris Guillory 2010-02-02 09:56:25 PST
Created attachment 47945 [details]
Notify Chromium of state change notifications. 

Code in WebKit\WebCore\platform\chromium\ChromiumBridge.h was indented inside of a namespace.
Comment 4 Chris Guillory 2010-02-02 11:52:55 PST
This change needs to be committed with a parallel change in Chromium. As I understand it this should NOT be added to the commit-queue as both changes are needed.
Comment 5 Darin Fisher (:fishd, Google) 2010-02-02 22:30:44 PST
(In reply to comment #4)
> This change needs to be committed with a parallel change in Chromium. As I
> understand it this should NOT be added to the commit-queue as both changes are
> needed.

Hi Chris,

Is it possible to break this change up into two parts so that you can avoid the need for a two-sided patch landing?  Generally speaking, we try to preserve the old API while introducing the new side-by-side.  Then, once Chromium adopts the new API, we can go back to WebKit and remove the old API.  Is that possible in this case?
Comment 6 Darin Fisher (:fishd, Google) 2010-02-02 22:45:49 PST
Comment on attachment 47945 [details]
Notify Chromium of state change notifications. 

> Index: WebCore/ChangeLog

> +        [Chromium] Notify ChromiumBridge of state change notifications.
> +        Bug: https://bugs.webkit.org/show_bug.cgi?id=34464

nit: people generally do not add the "Bug: " label like that.  Just the URL is enough.


> +
> +        No new tests needed. Added virtual function with empty default implementation.
> +
> +        * Makefile:

^^^ This patch doesn't seem to include any changes to Makefile


> +        * accessibility/chromium/AXObjectCacheChromium.cpp:
> +        (WebCore::AXObjectCache::postPlatformNotification):
> +        * platform/chromium/ChromiumBridge.h:

^^^ This ChangeLog seems inconsistent with the patch.


> Index: WebCore/accessibility/chromium/AXObjectCacheChromium.cpp
...
> -void AXObjectCache::postPlatformNotification(AccessibilityObject*, AXNotification)
> +void AXObjectCache::postPlatformNotification(AccessibilityObject* obj, AXNotification notification)
>  {
> +    ChromiumBridge::postPlatformNotification(obj, notification);

Since your implementation of this ChromiumBridge method involves simply
calling back up to the WebViewClient, I think it would be better to put
that code here and skip defining a new ChromiumBridge method.

I would consider adding a method to ChromeClientChromium for this.  I
also think that it should have a more descriptive name than
postPlatformNotification.  "changeStateAccessibilityObject" sounds a
bit awkward to me.  How about didChangeAccessibilityObjectState?

In general, ChromiumBridge methods should only exist to help with the
implementation of code under WebCore/platform.  (There are numerous
exceptions that violate this rule and should be cleaned up.)

For example, if you find yourself dealing with Document or Frame in
ChromiumBridge, then you know it is the wrong place for the code.
Comment 7 Chris Guillory 2010-02-03 11:20:07 PST
Hi Darin,

Thanks for the review. I believe I incorrectly stated that this change required two-sided patch landing. It should just require one change in WebKit followed by one change in Chromium. Chromium will build fine with just the change in WebKit since the virtual method added to WebViewClient provides a default implementation.

I've moved the new function from ChromiumBridge to ChromeClientChromium and given it a better name. I'll send out the new patch.
Comment 8 Chris Guillory 2010-02-03 12:29:59 PST
Created attachment 48059 [details]
Patch updated with comments from Darin
Comment 9 WebKit Review Bot 2010-02-03 12:33:46 PST
Attachment 48059 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/ChromeClientImpl.cpp:662:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
WebCore/accessibility/chromium/AXObjectCacheChromium.cpp:30:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 27


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Chris Guillory 2010-02-03 13:13:01 PST
Created attachment 48063 [details]
Fixing style - line endings.
Comment 11 Darin Fisher (:fishd, Google) 2010-02-03 22:55:32 PST
Comment on attachment 48063 [details]
Fixing style - line endings.

> Index: WebCore/ChangeLog
...
> +
> +        No new tests.

^^^ please delete the line that says "No new tests."

That is in the template to provide a suggestion to you that you
should consider creating new tests :-)

(For this change, I don't think a new test is needed.)


> Index: WebCore/accessibility/chromium/AXObjectCacheChromium.cpp

> +static ChromeClientChromium* toChromeClientChromium(Widget* widget)

This function should be changed to take a FrameView* as a parameter.
Then you can avoid having to check if the widget is a FrameView :-)


> Index: WebKit/chromium/public/WebViewClient.h
...
> +    // Notifies embedder that the state of an accessibility object has changed.
> +    virtual void accessibilityObjectStateDidChange(const WebAccessibilityObject&) { }

I previously suggested didChangeAccessibilityObjectState to match
the naming conventions of other methods found in WebViewClient.h.

Unless you feel strongly about this, I think we should go with
that instead (for consistency).
Comment 12 Chris Guillory 2010-02-04 11:22:46 PST
Created attachment 48157 [details]
Updated with more comments from Darin

Removed no new tests.

toChromeClientChromium now takes a FrameView*. :)

I don't feel strongly about the name. Changed to didChangeAccessibilityObjectState.
Comment 13 Darin Fisher (:fishd, Google) 2010-02-04 11:40:06 PST
Comment on attachment 48157 [details]
Updated with more comments from Darin

> Index: WebCore/accessibility/chromium/AXObjectCacheChromium.cpp
...
> +void AXObjectCache::postPlatformNotification(AccessibilityObject* obj, AXNotification notification)
>  {
> +    if (!obj || !obj->documentFrameView() || notification != AXObjectCache::AXCheckedStateChanged)

nit: since you are within the namespace of AXObjectCache, there is no need
to use AXObjectCache::


otherwise, r=me
Comment 14 Chris Guillory 2010-02-04 12:10:54 PST
Created attachment 48159 [details]
Removed unneeded AXObjectCache::

Fixed the copy paste error.
Comment 15 Darin Fisher (:fishd, Google) 2010-02-04 12:17:11 PST
Landed as http://trac.webkit.org/changeset/54364
Comment 16 Darin Fisher (:fishd, Google) 2010-02-04 12:17:58 PST
Comment on attachment 48159 [details]
Removed unneeded AXObjectCache::

Thanks for updating the patch.  I actually made the change
locally and committed.  Same result :-)
Comment 17 Dimitri Glazkov (Google) 2010-02-06 12:18:59 PST
Committed r54466: <http://trac.webkit.org/changeset/54466>
Comment 18 Dimitri Glazkov (Google) 2010-02-06 12:20:52 PST
Rolled out, because it introduced ASSERTs. Needs more lovin'.
Comment 19 Eric Seidel (no email) 2010-02-08 15:12:49 PST
Comment on attachment 48157 [details]
Updated with more comments from Darin

Cleared Darin Fisher's review+ from obsolete attachment 48157 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 20 Eric Seidel (no email) 2010-02-08 15:28:56 PST
Comment on attachment 48159 [details]
Removed unneeded AXObjectCache::

Looks like this got rolled out?  Marking r-.
Comment 21 Chris Guillory 2010-02-08 16:18:09 PST
Created attachment 48374 [details]
Added the !obj->document() to fix debug ASSERTS

Verified by running chromium webkit layout tests in debug mode.
Comment 22 WebKit Commit Bot 2010-02-09 20:01:54 PST
Comment on attachment 48374 [details]
Added the !obj->document() to fix debug ASSERTS

Clearing flags on attachment: 48374

Committed r54584: <http://trac.webkit.org/changeset/54584>
Comment 23 WebKit Commit Bot 2010-02-09 20:02:07 PST
All reviewed patches have been landed.  Closing bug.