[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


Mario Sanchez Prada <msanchez at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #64234|0                           |1
        is obsolete|                            |
  Attachment #66035|                            |review?
               Flag|                            |




--- Comment #6 from Mario Sanchez Prada <msanchez at igalia.com>  2010-08-31 04:56:28 PST ---
Created an attachment (id=66035)
 --> (https://bugs.webkit.org/attachment.cgi?id=66035)
Patch proposal

Attaching new patch addressing the issues pointed out in comment #5.

See my comments below:

(In reply to comment #5)
> [...]
> 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.

Done.

> > +    // 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.

I agree with you, sorry for the confusion. Changed :-)

> > +    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.

I know what you mean, but in this case I think that wouldn't be enough, since we need both to return the selected text and the offsets *relative to the AtkObject* where we are asking for a text selection. That's the reason behind the getSelectionOffsetsForObject() method.

> > +    // 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 "".

Changed.

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