[webkit-reviews] review granted: [Bug 28200] ListMarker should be included as part of the text value to parse : [Attachment 34611] 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 28200: ListMarker should be included as part of the text value to parse
https://bugs.webkit.org/show_bug.cgi?id=28200

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   Node* stringNode = node;
> +	   while (stringNode) {

We'd normally write this as a for loop.

	   for (Node* stringNode = node; stringNode; stringNode =
stringNode->parent())

> +	       RenderObject* renderObject = stringNode->renderer();
> +	       if (renderObject->isListItem()) {

You need to check the renderObject for 0 here. There's not an ironclad
guarantee it will be non-zero. If you were walking up the render tree then
there would be, but a DOM parent of a renderer might not be rendered. I am not
even sure this will do the right thing in unusual circumstances, like when a
list is in a floating element. DOM parents might be far away from the element
in question, so walking up the DOM tree like this is not necessarily a good
idea.

I think the code would read better if you factored the code to find the
RenderListItem* apart from the code to extract the marker text, maybe even in a
separate function. It could help make the flow clearer to not have the marker
text code inside a loop.

> +    String listMarkerTextForRange(const VisiblePosition& , Node*) const;

Extra space here before the comma. Strange that this takes both a
VisiblePosition and a Node*. I'm not really sure I understand the nature and
relationship of the two arguments.

> +    if ([string isKindOfClass:[NSString class]])
> +	   return [string createJSStringRef];
> +    return 0;

We normally prefer to have the early exit be for the failure case.

> +JSStringRef AccessibilityUIElement::stringForRange(unsigned location,
unsigned length)

Typically we omit argument names if they are not used in a function. I guess we
don't have that warning turned on here, but it's worth following the style for
new code.

r=me