[webkit-reviews] review denied: [Bug 34464] [Chromium] Chromium should receive checkbox state changes : [Attachment 47945] Notify Chromium of state change notifications.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Feb 2 22:45:48 PST 2010


Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied
chris.guillory at google.com's request for review:
Bug 34464: [Chromium] Chromium should receive checkbox state changes
https://bugs.webkit.org/show_bug.cgi?id=34464

Attachment 47945: Notify Chromium of state change notifications. 
https://bugs.webkit.org/attachment.cgi?id=47945&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> 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.


More information about the webkit-reviews mailing list