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

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <msanchez at igalia.com> has asked  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 66035: Patch proposal
https://bugs.webkit.org/attachment.cgi?id=66035&action=review

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
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.