[Webkit-unassigned] [Bug 26991] [Gtk] get_n_selections and get_selection fail when selecting text across object boundaries

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=26991


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #64234|review?                     |review-
               Flag|                            |




--- Comment #5 from Martin Robinson <mrobinson at webkit.org>  2010-08-16 08:51:29 PST ---
(From update of attachment 64234)
>         [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 "".

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.