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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Sep 7 15:06:31 PDT 2010


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

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

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

Looks good, but I have a few concerns.

> WebCore/accessibility/AccessibilityObject.cpp:-377
> -#if PLATFORM(GTK)
Nice!

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

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

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

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

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

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


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


More information about the webkit-reviews mailing list