[webkit-reviews] review requested: [Bug 26991] [Gtk] get_n_selections and get_selection fail when selecting text across object boundaries : [Attachment 67204] 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 67204: Patch proposal
https://bugs.webkit.org/attachment.cgi?id=67204&action=review

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
(In reply to comment #10)
> (From update of attachment 66734 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=66734&action=prettypatch
> 
> Looks good, but I have a few concerns.

Thanks for the review, I've been working on this addressing these issues and I
have to say I learnt quite a lot of interesting things in the way, so here you
have a new patch, hopefully better :-)

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1336
> > +	     return false;
> Any reason you aren't using selection.firstRange() or
selection.toNormalizedRange() here?

I didn't know of those :-). selection.toNormalizedRange() works fine in this
case so I used that one in this new patch

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1341
> > +	 for (TextIterator it(range.get()); !it.atEnd(); it.advance()) {
> Would it be possible to replace this loop with Range::intersectsNode?

In this case intersectsNode() *almost* works ok but it fails when the selection
is right touching one of the boundaries of the selected node so that wouldn't
be enough. Anyway, I still could replace the loop with a combination of
intersectsNode() + 2 extra checks to make sure we don't return TRUE if the
situation with the boundaries happened. Thanks for the tip.

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1360
> > +	 RefPtr<Range> range = makeRange(selection.visibleStart(),
selection.visibleEnd());
> Again it seems like this should be selection.firstRange() or
selection.toNormalizedRange().

Changed to selection.toNormalizedRange().

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1376
> > +		 break;
> Tricky loop logic. :/ I admit I'm not sure exactly what the range return
value is supposed to represent, but could you take a non-loop approach, such as
the one used in AccessibilityRenderObject::ariaSelectedTextDOMRange in
WebCore/accessibility/AccessibilityRenderObject.cpp?

It is not exactly the same thing because we need to return the offsets of the
selection relative to the selected object, so some manual calculation, at least
for the endoffset is still needed.

However, I was able to improve the code (I think) making it more readable
(easier to understand) and efficient (still a loop but with far less
iterations), allowing to determine starOffset in a much more logical fashion,
and keeping a small loop just to compute the length of the intermediate nodes
to build the "effective" value for endOffset

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1397
> > +	 if (selection_num)
> There are lots of non-WebKit style variable names in this file, but we should
be converting them gradually to WebKit style. Please do this for all other
arguments of this method as well.

I fixed those as well in this new patch, compiled and tested against the very
vest version of trunk :-)