Bug 34464

Created attachment 47942 [details] [Chromium] Notify Chromium of state change notifications.

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.

Created attachment 47945 [details] Notify Chromium of state change notifications. Code in WebKit\WebCore\platform\chromium\ChromiumBridge.h was indented inside of a namespace.

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.

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

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.

Created attachment 48059 [details] Patch updated with comments from Darin

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.

Created attachment 48063 [details] Fixing style - line endings.

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

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

Created attachment 48159 [details] Removed unneeded AXObjectCache:: Fixed the copy paste error.

Landed as http://trac.webkit.org/changeset/54364

Comment on attachment 48159 [details] Removed unneeded AXObjectCache:: Thanks for updating the patch. I actually made the change locally and committed. Same result :-)

Committed r54466: <http://trac.webkit.org/changeset/54466>

Rolled out, because it introduced ASSERTs. Needs more lovin'.

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 on attachment 48159 [details] Removed unneeded AXObjectCache:: Looks like this got rolled out? Marking r-.

Created attachment 48374 [details] Added the !obj->document() to fix debug ASSERTS Verified by running chromium webkit layout tests in debug mode.

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>

All reviewed patches have been landed. Closing bug.