Bug 43919

Summary:

[GTK] Provide unit tests for AtkText's text selection functions

Product:

WebKit

Reporter:

Mario Sanchez Prada <mario>

Component:

Accessibility

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED INVALID

Severity:

Normal

CC:

apinheiro, jdiggs, mrobinson, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

Linux

Bug Depends on:

25673, 26991

Bug Blocks:

Attachments:

Description	Flags
Patch proposal	mrobinson: review-

Mario Sanchez Prada

Reported 2010-08-12 08:22:36 PDT

This is a bug created just to provide in a common point unit tests to check how the following implementations of AtkText interface's functions work: 1. atk_text_get_n_selections() 2. atk_text_get_selection() 3. atk_text_set_selection() 4. atk_text_remove_selection() Patch for (1) and (2) will be provided along with bug 26991, while patch for (3) and (4) will be provided along with bug 25673. However, to implement unit tests for this kind of stuff all the 4 functions are required so it would be a bit messy, IMHO, just to provide the unit tests with one of those bugs and leave the patch for the other bug "trusting" that unit tests will eventually appear on trunk :-) Also, it way it's easier to stick to the rule of "one patch per bug". Attaching the patch in some minutes then...

Attachments
Patch proposal (10.40 KB, patch) 2010-08-12 10:20 PDT, Mario Sanchez Prada	mrobinson: review-	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Mario Sanchez Prada

Comment 1 2010-08-12 08:24:07 PDT

(In reply to comment #0) > This is a bug created just to provide in a common point unit tests to check how > the following implementations of AtkText interface's functions work: > > 1. atk_text_get_n_selections() > 2. atk_text_get_selection() > 3. atk_text_set_selection() > 4. atk_text_remove_selection() > > Patch for (1) and (2) will be provided along with bug 26991, while patch for > (3) and (4) will be provided along with bug 25673. Marking this bug as dependent on 26991 and 25673 then

Mario Sanchez Prada

Comment 2 2010-08-12 10:20:39 PDT

Created attachment 64236 [details] Patch proposal (In reply to comment #0) > This is a bug created just to provide in a common point unit tests > to check how the following implementations of AtkText interface's > functions work: > > 1. atk_text_get_n_selections() > 2. atk_text_get_selection() > 3. atk_text_set_selection() > 4. atk_text_remove_selection() > > [...] > Attaching the patch in some minutes then... As promised, here is the patch to test all those 4 functions. Notice in the patch that it was needed to add an extra check inside EditorClientGtk.cpp to ensure that a GdkWindow is associated to the webview widget before trying to actually mark the selection on that window (which won't be possible when running the unittests as the webview won't have a GdkWindow, since it's not inside any toplevel window). Note that this bug was set as blocked by bug 26991 and bug 25673, since those are were the actual implementations of those 4 methods are done. Hence, now asking for review.

Martin Robinson

Comment 3 2010-08-25 11:19:45 PDT

Comment on attachment 64236 [details] Patch proposal I think I'd rather see these tests attached to the second patch. I'm uncomfortable checking in code and tests separately. Let me know if I can lend a hand here.

Mario Sanchez Prada

Comment 4 2010-08-31 03:18:40 PDT

(In reply to comment #3) > (From update of attachment 64236 [details]) > I think I'd rather see these tests attached to the second patch. I'm > uncomfortable checking in code and tests separately. Not sure I follow you, but if I did it that way you'd still face the same problem when checking in the patch for bug 26991, as tests for it could be introduced until patch for bug 25673 was checked in as well. Ok, you'd improve the situation in the way you'd be having the problem with just one bug instead of two, but I still think that's not a good enough reason to mess things up, IMHO. Perhaps, after all, best thing here would be to merge all these three bugs into one new bug (+wontfixing the former ones, or something like that) and then provide one single patch with both implementations for get/set methods + the unit tests. You know... "One Patch to rule them all, One Patch to bring them all and in the darkness bind them" :-) What do you think?

Mario Sanchez Prada

Comment 5 2010-09-07 10:19:20 PDT

(In reply to comment #3) > (From update of attachment 64236 [details]) > I think I'd rather see these tests attached to the second patch. I'm uncomfortable checking in > code and tests separately. Let me know if I can lend a hand here. After talking to Martin via IRC we decided to finally go for that option so now unit tests are attached to patch for bug 25673, as requested. See https://bugs.webkit.org/show_bug.cgi?id=25673#c7 for more details.

Note You need to log in before you can comment on or make changes to this bug.