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

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=66734&action=prettypatch

Looks good, but I have a few concerns.

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

> 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?

> 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().

> 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?

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