[Webkit-unassigned] [Bug 139443] [GTK] Add API to set the web view editable into WebKit2 GTK+ API

[bugzilla-daemon at webkit.org]

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

Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #242912|review?, commit-queue?      |review-, commit-queue-
              Flags|                            |

--- Comment #3 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 242912
  --> https://bugs.webkit.org/attachment.cgi?id=242912
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=242912&action=review

You should also add the new symbols to the documentation

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:885
> +     * information see #webkit_web_view_set_editable.

#webkit_web_view_set_editable -> webkit_web_view_set_editable()

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:895
> +    g_object_class_install_property(gObjectClass,
> +                                    PROP_EDITABLE,
> +                                    g_param_spec_boolean("editable",
> +                                                         _("Editable"),
> +                                                         _("Whether the content can be modified by the user."),
> +                                                         FALSE,
> +                                                         WEBKIT_PARAM_READWRITE));

This should be indented following wk coding style.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3367
> + * it doesn't. You can change @web_view's document programmatically regardless of
> + * this setting.

You can change @web_view's document programmatically regardless of this setting. <- I don't understand this sentence

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3369
> + * Return value: a #gboolean indicating the editable state

Use Returns instead of Return value:

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3391
> + * elements. You can change @web_view's document programmatically regardless of
> + * this setting. By default a #WebKitWebView is not editable.

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3394
> + * document are editable. This function provides a low-level way to make the

Why low-level?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3404
> +    if (editable == webkit_web_view_get_editable(webView))

You could use etPage(webView)->isEditable() directly here instead of the public method to avoid doing the g_return_if_fail again

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3410
> +    if (editable)
> +        getPage(webView)->applyEditingStyleToBodyElement();

Isn't setEditable() enough? shouldn't setEditable do this automatically?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1344
> +#if PLATFORM (GTK)
> +void WebPageProxy::setEditable(bool editable)

Why is this specific to GTK?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1362
> +    m_process->send(Messages::WebPage::ApplyEditingStyleToBodyElement(), m_pageID);

Can we get rid of this message and call this in the web process when setEditable is called with true? What happens if setEditable is called with false afterwards?

> Source/WebKit2/UIProcess/WebPageProxy.h:398
> +    void setEditable(bool);
> +    bool isEditable() const { return m_isEditable; };

I think should be either setIsEditable() and isEditable() or setEditable() and editable() I think

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebViewEditor.cpp:121
> +    test->setEditable(TRUE);

TRUE -> true

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebViewEditor.cpp:174
> +    test->setEditable(TRUE);

Ditto.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20141209/356d6abd/attachment-0002.html>