[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

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

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3533
> + * webkit_web_view_get_editable:

hmm, after reading the api again, I wonder if it would be better to use webkit_web_view_is_editable()

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3542
> + * Returns whether the user is allowed to edit the document.
> + *
> + * Returns %TRUE if @web_view allows the user to edit the HTML document, %FALSE if
> + * it doesn't. You can change @web_view's document through #WebKitWebInspector or
> + * #WebKitWebExtension regardless this setting. By default a #WebKitWebView is not editable.
> + *
> + * Returns: a #gboolean indicating the editable state

There are 3 returns here :-) I would reorganiza this a bit. The last one is redundant so I would just remove it. I don't understand the part "You can change @web_view's document through #WebKitWebInspector or #WebKitWebExtension regardless this setting". I guess you mean that you can change the html so make the document editable. I think it's confusing, I wouldn't mention the inspector nor web extensions here. We could explain how this affect what the document contains, for example, what happens if the body tag contains contentEditable=True? does this method return true or false? (We could probably add a test case for that indeed).

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3563
> + * If @editable is %TRUE, @web_view allows the user to edit the document. If
> + * @editable is %FALSE, an element in @web_view's document can only be edited if the
> + * CONTENTEDITABLE attribute has been set on the element or one of its parent
> + * elements. You can change @web_view's document through #WebKitWebInspector or

This looks better. We should add something similar to the getter, explaining that the document might still be editable if the conentEditable attr is set, even if the web view is not editable.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:3564
> + * #WebKitWebExtension regardless this setting. By default a #WebKitWebView is not editable.

I wouldn't mention inspector and extensions here either.

> Tools/TestWebKitAPI/gtk/WebKit2Gtk/WebViewTest.cpp:265
> +    webkit_web_view_set_editable(m_webView, editable);

So, we are only testing the setter, but not the getter. We should check that by default is_editable returns FALSE as the doc says, and that after calling set_editable with TRUE, is_editable also return TRUE. It would be nice to check also that the web view can override the document, so that if body has contentEditable=false, making the view editable allows to edit the web view. So maybe it's better to add specific test cases for this, instead of reusing the selection ones.

-- 
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/20150116/6776190d/attachment-0002.html>