[webkit-reviews] review denied: [Bug 72866] Accessibility: AccessibilityController should support listening to notifications on all elements. : [Attachment 116320] Patch

[bugzilla-daemon at webkit.org]

chris fleizach <cfleizach at apple.com> has denied Dominic Mazzoni
<dmazzoni at google.com>'s request for review:
Bug 72866: Accessibility: AccessibilityController should support listening to
notifications on all elements.
https://bugs.webkit.org/show_bug.cgi?id=72866

Attachment 116320: Patch
https://bugs.webkit.org/attachment.cgi?id=116320&action=review

------- Additional Comments from chris fleizach <cfleizach at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=116320&action=review


You also need to make this work in WebKitTestRunner, which will basically
duplicate a bunch of this code

> Tools/DumpRenderTree/AccessibilityController.h:65
> +#endif

Can these methods be reconciled with windows so we don't need two
addNotificationListeners

> Tools/DumpRenderTree/AccessibilityController.h:82
> +#endif

this should probably be a RetainPtr<NotificationHandler>

> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.h:34
> +#import <Cocoa/Cocoa.h>

i doubt you need to import Cocoa here. I believe the prefix header handles this


> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.h:43
> +- (id)initAndListenToAllElements;

i think you should add a setter to set the platform element if you want (so you
don't need two different init's)
and i think you should have to call startObserving explicitly.
that way you can remove both the init methods, and add a setPlatformElement:()
and make startObserving public

> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.h:47
> +

if these are private they should not be in the header. if they are not private
they should not be prefixed with_

> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:36
> +#import <Foundation/Foundation.h>

unlikely that you need Foundation here

> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:66
> +    m_platformElement = platformElement;

if [super init] returns nil then you should also return nil right away

> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:93
> +    // Release the old callback.

comment is not needed.

> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:106
> +    [NSClassFromString(@"WebAccessibilityObjectWrapper")
accessibilitySetShouldRepostNotifications:YES];

you should assert that NSClassFromString(@"WebAccessibilityObjectWrapper")
returns something  (in case the name changes like it did recently)

> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:122
> +	   JSObjectCallAsFunction([mainFrame globalContext],
m_notificationFunctionCallback, 0, 1, &notificationNameArgument, 0);

if you have an if block with a comment (making it more than one line), i
believe you should be using brackets