[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