[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