[Webkit-unassigned] [Bug 25673] [GTK] ATs should be able to select/unselect text

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=25673


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #66735|review?                     |review-
               Flag|                            |




--- Comment #9 from Martin Robinson <mrobinson at webkit.org>  2010-09-07 15:06:32 PST ---
(From update of attachment 66735)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.