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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Mar 13 16:41:40 PDT 2011


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 85059: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=85059&action=review

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

r- because of some really minor things but this looks good to me otherwise! I'd
really like to have an Apple person look over the IPC changes, since I'm not
particularly familiar with that code.

> Source/WebKit2/UIProcess/WebPageProxy.messages.in:139
> +    # Keyboard support messages

Might want to make this slightly more descriptive. "Support for GTK+ platform
keybindings" or something like that.

> Source/WebKit2/UIProcess/WebPageProxy.messages.in:140
> +    GenerateEditorCommands() -> (Vector<WTF::String> commandsList)

Likewise, this might be better named in a way to describe that it converts a
keystroke into a editor commands.

> Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:39
> +    // First try to interpret the command in the UI and get the commands..

Extra period here.

> Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:43
> +    if
(!WebProcess::shared().connection()->sendSync(Messages::WebPageProxy::GenerateE
ditorCommands(),
> +						       
Messages::WebPageProxy::GenerateEditorCommands::Reply(pendingEditorCommands),
> +							m_page->pageID(),
CoreIPC::Connection::NoTimeout))
> +	   return;

I don't think this return or if statement are necessary.

> Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp:61
> +	       success = false;
> +	       break;

If you're going to break, you can probably just return false here and dispense
with the success variable altogether.


More information about the webkit-reviews mailing list