Bug 88698

Summary: [GTK] Add unit tests for GtkInputMethodFilter
Product: WebKit Reporter: Martin Robinson <mrobinson>
Component: WebKitGTKAssignee: Martin Robinson <mrobinson>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 88458    
Bug Blocks: 65093    
Attachments:
Description Flags
Patch cgarcia: review+

Description Martin Robinson 2012-06-08 19:34:11 PDT
GtkInputMethodFilter is a good candidate for unit testing, because it's very hard to do functional tests for GTK+ input methods.
Comment 1 Martin Robinson 2012-06-08 20:08:01 PDT
Created attachment 146681 [details]
Patch
Comment 2 Carlos Garcia Campos 2012-06-20 09:42:48 PDT
Comment on attachment 146681 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146681&action=review

> Tools/TestWebKitAPI/Tests/gtk/InputMethodFilter.cpp:43
> +        m_testWindow = gtk_window_new(GTK_WINDOW_POPUP);

Can we do this using the class initializer syntax? 

TestInputMethodFilter()
    : m_testWindow(gtk_window_new(GTK_WINDOW_POPUP))

> Tools/TestWebKitAPI/Tests/gtk/InputMethodFilter.cpp:55
> +        GdkEvent* event = gdk_event_new(type);

GOwnPtr?

> Tools/TestWebKitAPI/Tests/gtk/InputMethodFilter.cpp:60
> +

You should probably set the event time too.

> Tools/TestWebKitAPI/Tests/gtk/InputMethodFilter.cpp:117
> +        m_events.append(String::format("confirmCurrentcomposition"));

Do you need to use String::format() even if the string is just a literal?

> Tools/TestWebKitAPI/Tests/gtk/InputMethodFilter.cpp:122
> +        m_events.append(String::format("cancelCurrentComposition"));

Ditto.

> Tools/TestWebKitAPI/Tests/gtk/InputMethodFilter.cpp:131
> +    GRefPtr<GtkWidget> m_testWindow;

I don't like using GRefPtr for widgets, because it consumes the floating reference, but in this case, the widget is not going to be added to any containser, so I guess it's not a problem.

> Tools/TestWebKitAPI/Tests/gtk/InputMethodFilter.cpp:217
> +typedef void (*GetPreeditStringCallback) (GtkIMContext*, gchar **, PangoAttrList **, int*);

gchar ** -> gchar**
PangoAttrList ** -> PangoAttrList**

> Tools/TestWebKitAPI/Tests/gtk/InputMethodFilter.cpp:229
> +    // This is a bit of a hack to avoid mocking out the entire InputMethodContext, by
> +    // simply replacing the get_preedit_string virtual method for the length of this test.

Indeed ;-)

> Tools/TestWebKitAPI/Tests/gtk/InputMethodFilter.cpp:231
> +    GtkIMContextClass* contextClass = GTK_IM_CONTEXT_CLASS(G_OBJECT_GET_CLASS(context));

You can use GTK_IM_CONTEXT_GET_CLASS() directly with the GtkIMContext instance.

> Tools/TestWebKitAPI/Tests/gtk/InputMethodFilter.cpp:273
> +    GtkIMContextClass* contextClass = GTK_IM_CONTEXT_CLASS(G_OBJECT_GET_CLASS(context));

Ditto.

> Tools/TestWebKitAPI/Tests/gtk/InputMethodFilter.cpp:293
> +    GtkIMContextClass* contextClass = GTK_IM_CONTEXT_CLASS(G_OBJECT_GET_CLASS(context));

Ditto.
Comment 3 Martin Robinson 2012-06-20 09:45:50 PDT
(In reply to comment #2)

> > Tools/TestWebKitAPI/Tests/gtk/InputMethodFilter.cpp:55
> > +        GdkEvent* event = gdk_event_new(type);
> 
> GOwnPtr?

I didn't use GOwnPtr here because I'd have to add it to the source list of the test, I think. I can do that though.
Comment 4 Martin Robinson 2012-06-20 09:46:53 PDT
(In reply to comment #2)
> (From update of attachment 146681 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146681&action=review
> 
> > Tools/TestWebKitAPI/Tests/gtk/InputMethodFilter.cpp:43
> > +        m_testWindow = gtk_window_new(GTK_WINDOW_POPUP);
> 
> Can we do this using the class initializer syntax? 

Yep! I'll make this change.

>
> > Tools/TestWebKitAPI/Tests/gtk/InputMethodFilter.cpp:117
> > +        m_events.append(String::format("confirmCurrentcomposition"));
> 
> Do you need to use String::format() even if the string is just a literal?
> 
> > Tools/TestWebKitAPI/Tests/gtk/InputMethodFilter.cpp:122
> > +        m_events.append(String::format("cancelCurrentComposition"));
> 
> Ditto.

Nope, this is just left over from a previous iteration. I'll remove it.
Comment 5 Carlos Garcia Campos 2012-06-20 09:56:12 PDT
Comment on attachment 146681 [details]
Patch

Looks good to me, just change the minor things I commented before landing.
Comment 6 Martin Robinson 2012-06-28 15:34:33 PDT
Committed r121475: <http://trac.webkit.org/changeset/121475>