[Webkit-unassigned] [Bug 54230] [GTK] Implement WebPage class for WebKit2

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=54230


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #86926|review?                     |review-
               Flag|                            |




--- Comment #19 from Martin Robinson <mrobinson at webkit.org>  2011-03-25 09:11:33 PST ---
(From update of attachment 86926)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.