Bug 25673

Created attachment 64235 [details] Patch proposal Asking for review on this patch. More details in the ChangeLog entry inside the patch. Note: Unit tests for this new stuff are provided by the patch for bug 43919 (which was filed just with the purpose of providing those unittests) because they couldn't be properly implemented without the fix for bug 26991, which is closely related to this one. That's why bug 43919 was set as dependent on these other two bugs (so patches get applied in order once reviewed+). See comments on bug 26991 and bug 43919 for more details

Attachment 64235 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3762092

(In reply to comment #2) > Attachment 64235 [details] did not build on gtk: > Build output: http://queues.webkit.org/results/3762092 Hmm... true, but that is because it uses a new static function defined while writing the patch for bug 26991. It would be great if we could set this bug as depending on bug 26991 then, but I can't do it. Perhaps Joanmarie could (as she's the reporter). If not possible, I can write a new patch not assuming the function defined in the other patch and later on we could do the merge, or just provide a new -updated- patch in the bug being fixed in second place. Opinions?

(In reply to comment #3) > [...] > It would be great if we could set this bug as depending on bug 26991 then, but > I can't do it. Perhaps Joanmarie could (as she's the reporter). Thanks Joanie! Now let's wait for 26991 to get reviewed+ :-)

Created attachment 66040 [details] Patch proposal Updating patch after coming back from holidays, based on comment #5 for bug 26991 (guess the same rationale applies here). (Remember that this patch depends on patch bug 26991 to work)

Attachment 66040 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3855184

Created attachment 66735 [details] Path proposal + Unit tests Attaching new up-to-date version now we (Martin and I) decided to bundle unit tests for this stuff along with this patch (instead of using a the new bug 43919 for that, which should be closed/invalidated now). Hope this helps to get this stuff in soon O:-) PS: Still, note this patch won't work if you don't apply patch for bug 26991 first, as it uses a new function defined there

Attachment 66735 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3961245

Comment on attachment 66735 [details] Path proposal + Unit tests 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.

Created attachment 67206 [details] Path proposal + Unit tests (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.

Comment on attachment 67206 [details] Path proposal + Unit tests View in context: https://bugs.webkit.org/attachment.cgi?id=67206&action=prettypatch > WebCore/accessibility/gtk/AccessibilityObjectAtk.cpp:112 > + if (!textLength) { This should probably be an early return. > WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:346 > +{ I like this a lot better, but I'd rather see early returns. :/ Looks good! r=me, but please switch to using early returns before landing.

Created attachment 67208 [details] Path proposal + Unit tests (In reply to comment #11) > (From update of attachment 67206 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=67206&action=prettypatch > > > WebCore/accessibility/gtk/AccessibilityObjectAtk.cpp:112 > > + if (!textLength) { > This should probably be an early return. > > > WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:346 > > +{ > I like this a lot better, but I'd rather see early returns. :/ > > Looks good! r=me, but please switch to using early returns before landing. Here you have a new version. BTw, I'm not asking for cq+ for now because we must make sure patch for 26991 are landed first (hence, I'll ask for cq+ later)

Comment on attachment 67208 [details] Path proposal + Unit tests View in context: https://bugs.webkit.org/attachment.cgi?id=67208&action=prettypatch Thanks! LGTM.

Created attachment 67214 [details] Path proposal + Unit tests (In reply to comment #13) > (From update of attachment 67208 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=67208&action=prettypatch > > Thanks! LGTM. This is *the right patch*, sorry for the noise. Guess you'll need to review+ it again (but do not cq+ it yet, please)

Attachment 67214 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WARNING: File exempt from style guide. Skipping: "WebKit/gtk/tests/testatk.c" WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1390: webkit_accessible_text_set_selection is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1411: webkit_accessible_text_remove_selection is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 67214 [details] Path proposal + Unit tests Ok.

Comment on attachment 67206 [details] Path proposal + Unit tests Cleared Martin Robinson's review+ from obsolete attachment 67206 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Comment on attachment 67208 [details] Path proposal + Unit tests Cleared Martin Robinson's review+ from obsolete attachment 67208 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Comment on attachment 67214 [details] Path proposal + Unit tests Now that the patch for bug 26991 has already landed in the WK repository, it's time to ask for the cq+ for this patch. Thanks!

Comment on attachment 67214 [details] Path proposal + Unit tests Bang.

Comment on attachment 67214 [details] Path proposal + Unit tests Clearing flags on attachment: 67214 Committed r67383: <http://trac.webkit.org/changeset/67383>

All reviewed patches have been landed. Closing bug.