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

• Status: RESOLVED
• Resolution: INVALID
• Component: Accessibility

[Mario Sanchez Prada]

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

[Mario Sanchez Prada]

(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]

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

(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]

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