Summary: | [GTK] Add unit tests for GtkInputMethodFilter | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Martin Robinson <mrobinson> | ||||
Component: | WebKitGTK | Assignee: | 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
Martin Robinson
2012-06-08 19:34:11 PDT
Created attachment 146681 [details]
Patch
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. (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. (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 on attachment 146681 [details]
Patch
Looks good to me, just change the minor things I commented before landing.
Committed r121475: <http://trac.webkit.org/changeset/121475> |