[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 #85059|review?                     |review-
               Flag|                            |




--- Comment #11 from Martin Robinson <mrobinson at webkit.org>  2011-03-13 16:41:40 PST ---
(From update of attachment 85059)
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::GenerateEditorCommands(),
> +                                                     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.

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