Bug 43919

Summary: [GTK] Provide unit tests for AtkText's text selection functions
Product: WebKit Reporter: Mario Sanchez Prada <mario>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED INVALID    
Severity: Normal CC: apinheiro, jdiggs, mrobinson, xan.lopez
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 25673, 26991    
Bug Blocks:    
Attachments:
Description Flags
Patch proposal mrobinson: review-

Description Mario Sanchez Prada 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...
Comment 1 Mario Sanchez Prada 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
Comment 2 Mario Sanchez Prada 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.
Comment 3 Martin Robinson 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.
Comment 4 Mario Sanchez Prada 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?
Comment 5 Mario Sanchez Prada 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.