[webkit-reviews] review denied: [Bug 19456] [GTK] Editing capabilities : [Attachment 45143] Implement webkit_web_view_execute_command, command-executed, catch internal commands

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has denied Christian Dywan
<christian at twotoasts.de>'s request for review:
Bug 19456: [GTK] Editing capabilities
https://bugs.webkit.org/show_bug.cgi?id=19456

Attachment 45143: Implement webkit_web_view_execute_command, command-executed,
catch internal commands
https://bugs.webkit.org/attachment.cgi?id=45143&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
> +#if PLATFORM(GTK)
> +	if (!m_frame->editor()->client()->shouldExecuteCommand(m_command->value
(m_frame.get(), triggeringEvent)))
> +	   return false;
> +#endif

Indentation seems off. I think we should not make this gtk-specific. Just add a
call to shouldExecuteCommand, and make the default implementation for all ports
except ours be return true;

> +
> +#if PLATFORM(GTK)
> +    virtual bool shouldExecuteCommand(const String&) = 0;
> +#endif

Same here. Just add the function, and make sure to add it to all the ports.

> +    /**
> +	* WebKitWebView::command-executed:

Perhaps we should name this editor-command, and get away with the 'executed'
part? The command has not really been executed yet - it will be.

> +	* Since: 1.1.14

1.1.19, I guess =)

> +    webkit_web_view_signals[RESOURCE_REQUEST_STARTING] =
g_signal_new("command-executed",

wrong index

> +/**
> + * webkit_web_view_execute_command:

I don't like 'execute_command'. It is too generic, and it's not really easy to
figure out what we are talking about. I would prefer
webkit_web_view_execute_editor_command, or editing_command.

> + * @web_view: a #WebKitWebView
> + * @command: a command string
> + *
> + * Executes the specified command, if possible.

What makes it not possible? We should document that the command will be
executed in the focused frame.