[webkit-reviews] review denied: [Bug 54230] [GTK] Implement WebPage class for WebKit2 : [Attachment 86926] Proposed patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Alejandro G. Castro
<alex at igalia.com>'s request for review:
Bug 54230: [GTK] Implement WebPage class for WebKit2
https://bugs.webkit.org/show_bug.cgi?id=54230

Attachment 86926: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=86926&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=86926&action=review

Looks good, but I think you should switch to making m_pendingEditorCommands an
object and just use append(). See below.

> Source/JavaScriptCore/wtf/StdLibExtras.h:48
> +// WEBKIT_COMMA: Required when using macros like DEFINE_STATIC_LOCAL and you

> +// need to define a template with multiple arguments i.e.
> +// DEFINE_STATIC_LOCAL(HasMap<type1 COMMA type2>, ...)
> +#define WEBKIT_COMMA ,

Hrm. I'm unsure about this change. Let's hold off on this or address it in
another bug.

> Source/WebKit2/UIProcess/gtk/WebView.cpp:305
> +void WebView::getEditorCommandsForKeyEvent(const NativeWebKeyboardEvent&
event, Vector<WTF::String>& commandsList)

commandsList -> commandList

> Source/WebKit2/UIProcess/gtk/WebView.cpp:309
> +    m_transientPendingEditorCommands = &commandsList;

Instead of making this change, maybe it would be better to turn the
m_pendingEditorCommands member into an object from a pointer and just do:
m_pendingEditorCommands.clear() here.

> Source/WebKit2/UIProcess/gtk/WebView.cpp:314
> +    gtk_bindings_activate_event(GTK_OBJECT(m_nativeWidget.get()),
reinterpret_cast<GdkEventKey*>(const_cast<GdkEvent*>(event.nativeEvent())));
> +#else
> +    gtk_bindings_activate_event(G_OBJECT(m_nativeWidget.get()),
reinterpret_cast<GdkEventKey*>(const_cast<GdkEvent*>(event.nativeEvent())));

Here you should use event.nativeEvent()->key to get the key event.

> Source/WebKit2/UIProcess/gtk/WebView.cpp:321
> +    if (m_transientPendingEditorCommands->isEmpty()) {
> +	   m_transientPendingEditorCommands = 0;
> +	   return;
> +    }
> +

Here you can just do commandsList.append(m_pendingEditorCommands);

> Source/WebKit2/UIProcess/gtk/WebView.cpp:323
> +    DEFINE_STATIC_LOCAL(HashMap<int WEBKIT_COMMA const char*>,
keyDownCommandsMap, ());
> +    DEFINE_STATIC_LOCAL(HashMap<int WEBKIT_COMMA const char*>,
keyPressCommandsMap, ());

For now instead of using WEBKIT_COMMA, just make a typedef like this at the top
of the file:

typedef HashMap<int, const char*> IntConstCharHashMap;

and use that here. I believe it should fix the issues with DEFINE_STATIC_LOCAL.