[webkit-reviews] review requested: [Bug 25673] [GTK] ATs should be able to select/unselect text : [Attachment 67206] Path proposal + Unit tests

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <msanchez at igalia.com> has asked  for review:
Bug 25673: [GTK] ATs should be able to select/unselect text
https://bugs.webkit.org/show_bug.cgi?id=25673

Attachment 67206: Path proposal + Unit tests
https://bugs.webkit.org/attachment.cgi?id=67206&action=review

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

Attaching a new patch, hopefully better (still it depends on patch for 26991 so
don't expect this to compile without applying the other one first).

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1419
> >  static gboolean webkit_accessible_text_remove_selection(AtkText* text,
gint selection_num)
> Please convert non-WebKit-style variable names as you implement these
methods.

Done
 
> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1435
> > +	 if (startOffset != endOffset) {
> Please use early returns.

Done.

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1440
> > +	     VisiblePositionRange range =
coreObject->visiblePositionRangeForRange(textRange);
> Why not just use core(text)->setSelectedTextRange(PlainTextRange(caretOffset,
0)); ?

I removed this part now, and make removeSelection delegate on setSelection, so
no more a problem.

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1449
> >  static gboolean webkit_accessible_text_set_selection(AtkText* text, gint
selection_num, gint start_offset, gint end_offset)
> Please convert these variable names to WebKit-style as you implement these
methods.

Done.

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1455
> > +	 if (start_offset != end_offset) {
> Please use an early return here instead.
> if (start_offset == end_offset)
>     return FALSE;
> 
> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1458
> > +	     VisiblePositionRange range =
coreObject->visiblePositionRangeForRange(textRange);
> Why not just use core(text)->setSelectedTextRange(PlainTextRange(startOffset,
endOffset - startOffset)); ?

I tried that but didn't work fine, probably (wild guess) because it's not the
same than setting a VisibleSelection. Not sure, though.
Hence I kept this part as it is.
 
> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1472
> > +	 VisiblePositionRange range =
coreObject->visiblePositionRangeForRange(textRange);
> Why not just use core(text)->setSelectedTextRange(PlainTextRange(offset, 0));
?

I kept this part as it is, as that change didn't work (see rationale above)
 
> > WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:357
> > +	 if (gtk_widget_has_screen(GTK_WIDGET(m_webView))) {
> This change seems sane, but since we're getting into a condition of depth
three (if you count the #if), perhaps we should move this logic to a new method
called something like setSelectionPrimaryClipboardIfNeeded and use early
returns.

Done.