[webkit-reviews] review denied: [Bug 26991] [Gtk] get_n_selections and get_selection fail when selecting text across object boundaries : [Attachment 64234] Patch proposal

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Mario Sanchez Prada
<msanchez at igalia.com>'s request for review:
Bug 26991: [Gtk] get_n_selections and get_selection fail when selecting text
across object boundaries
https://bugs.webkit.org/show_bug.cgi?id=26991

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

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
>	  [Gtk] get_n_selections and get_selection fail when selecting text
across object boundaries
>	  https://bugs.webkit.org/show_bug.cgi?id=26991
> 
>	  Fix AtkText getNSelections() and getSelection() to work properly
> 
>	  Changed the implementation of the selectionBelongsToObject()
>	  function in the Atk wrapper to return a reliable value by
>	  carefully checking that the selection belongs to the selected
>	  object or some of its descendants.
> 
>	  Implemented a new function to get the start and end offsets of a
>	  selection for a given accessible object.
> 
>	  Made it so webkit_accessible_text_get_selection returns zero when
>	  both start and end offsets are equal, following GAIL's lead.
> 
>	  * accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:
>	  (selectionBelongsToObject):
>	  (getSelectionOffsetsForObject): New.
>	  (webkit_accessible_text_get_selection):
You should put your descriptions of the functions inline with the
ChangeLog file and method names. It makes the result more readable.

Take a look at the existing ChangeLog entries in WebCore.

> +	   * accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:
> +	   (selectionBelongsToObject):
> +	   (getSelectionOffsetsForObject): New.
> +	   (webkit_accessible_text_get_selection):

Same here.

> +    // Make range for the given selection
> +    RefPtr<Range> range = makeRange(selection.visibleStart(),
selection.visibleEnd());
> +    if (!range)
> +	   return false;

Comments like this really don't add anything. Your comments should aways
answer 'Why?' and not just 'What?'

> +    // Check whether selected node is the selection's owner

For instance here, I'm unsure why a node owns a selection if it is
inside the selection range.

> +    Node* node = coreObject->node();
> +    for (TextIterator it(range.get()); !it.atEnd(); it.advance()) {
> +	   Node* nodeInRange = it.range()->startContainer();
> +	   if (nodeInRange && nodeInRange->isDescendantOf(node))
> +	       return true;
> +    }
> +
> +    // Default
> +    return false;
> +}

> +    // Get the offsets of the selection for the selected object
> +    AccessibilityObject* coreObject = core(text);
> +    VisibleSelection selection = coreObject->selection();
> +    getSelectionOffsetsForObject(coreObject, selection, *start_offset,
*end_offset);

Perhaps I simply do not understand this section of code, but couldn't
you simply call core(text)->doAXStringForRange(coreObject->selectedTextRange())
and
return that? That would allow you to get rid of most of the code above.

> +    // Return the text selected between the offsets if different or zero
otherwise
> +    if (*start_offset != *end_offset)
> +	   return webkit_accessible_text_get_text(text, *start_offset,
*end_offset);
> -    return webkit_accessible_text_get_text(text, *start_offset,
*end_offset);
> +    return 0;

It is my preference that the failure case "return 0" is the early return.
Also, the comment doesn't add much and doesn't explain why NULL is returned
instead of "".