[webkit-reviews] review granted: [Bug 32007] WAI-ARIA: implement support for ARIA drag and drop : [Attachment 44054] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 32007: WAI-ARIA: implement support for ARIA drag and drop
https://bugs.webkit.org/show_bug.cgi?id=32007

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +void AccessibilityRenderObject::determineARIADropEffects(Vector<String>&
effects)
> +{
> +    String dropEffects = getAttribute(aria_dropeffectAttr).string();
> +    if (dropEffects.isEmpty())
> +	   return;

Probably would be better to clear "effects" before returning.

> +	   unsigned length = dropEffects.size();

The best type to use here would be size_t rather than unsigned.

> +	   NSMutableArray* dropEffectsArray = [NSMutableArray array];

It's better for performance to use the arrayWithCapacity: method here.

> +	   for (unsigned i = 0; i < length; ++i)

Again, size_t.

> +    // A space concatentated string of all the drop effects.
> +    JSStringRef ariaDropEffects() const;

Could this be a RetainPtr<JSStringRef> rather than a JSStringRef?

> +    id value = [m_element
accessibilityAttributeValue:NSAccessibilityDropEffectsAttribute];
> +    if (![value isKindOfClass:[NSArray class]])
> +	   return 0;
> +
> +    NSMutableString* dropEffects = [NSMutableString string];
> +    NSInteger length = [value count];
> +    for (NSInteger k = 0; k < length; ++k) {
> +	   [dropEffects appendString:[value objectAtIndex:k]];
> +	   if (k < length - 1)
> +	       [dropEffects appendString:@" "];
> +    }
> +    
> +    return [dropEffects createJSStringRef];

Rebuilding the string with spaces makes it hard to detect bugs involving
mis-parsing the string. It would be better if the separator here was not the
same as the separator used in the syntax itself. Maybe ", " instead or " | " or
something like that.

r=me -- please consider some of my suggested improvements above.