Bug 88698 – [GTK] Add unit tests for GtkInputMethodFilter

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Martin Robinson]

GtkInputMethodFilter is a good candidate for unit testing, because it's very hard to do functional tests for GTK+ input methods.

[Martin Robinson]

Created attachment 146681 [details]
Patch

[Carlos Garcia Campos]

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.

[Martin Robinson]

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

[Martin Robinson]

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

[Carlos Garcia Campos]

Comment on attachment 146681 [details]
Patch

Looks good to me, just change the minor things I commented before landing.

[Martin Robinson]

Committed r121475: <http://trac.webkit.org/changeset/121475>