[Webkit-unassigned] [Bug 137116] [GTK] Add selection-changed signal to the WebKit2 API

[bugzilla-daemon at webkit.org]

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

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

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

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

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

Sorry for the delay reviewing this, I still have a few more comments.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:25
> +#include "WebKitWebPage.h"

This is already included form WebKitWebPagePrivate.h, but also from WebKitWebEditor.h

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:27
> +#include "WebPage.h"

And this should be included from WebKitWebPagePrivate.h

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:37
> + * @Title: WebKitWebEditor
> + *

Add See also: #WebKitWebPage

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:38
> + * The #WebKitWebEditor provides access to various editing capabilities of

Do not use # in this particular case, as it creates a link to this section.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:41
> + */

Since: 2.10

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:49
> +    GRefPtr<WebKitWebPage> webPage;

Why do you need to keep a reference of WebKitWebPage?. The WebKitWebPage is the one creating and destroying the editor, so the web page will be alive while the editor is alive.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:63
> +     * This signal is emitted when the selection inside a #WebKitWebPage has been
> +     * changed.

I think we need to explain here what selection actually means, since it's not obvious that a cursor move triggers a selection change

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:115
> +/**
> + * webkit_web_editor_get_page:
> + * @editor: a #WebKitWebEditor
> + *
> + * Gets the #WebKitWebPage that is associated with #WebKitWebEditor.
> + *
> + * Returns: (transfer none): the #WebKitWebPage that can be used to access
> + * the #WebKitDOMDocument currently loaded into it.

If this is mainly useful for accessing the DOM, maybe it makes more sense to have webkit_web_editor_get_dom_document?

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:402
> +    g_return_val_if_fail(WEBKIT_IS_WEB_PAGE(webPage), nullptr);

Do not use g_return macros in private API

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:473
> +    page->priv->webEditor = nullptr;
> +

This is not needed, the struct is already filled with 0 on allocation, and the GRefPtr initialized by the constructor.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:595
> + * Gets the #WebPageEditor of a #WebKitWebPage.

#WebPageEditor -> #WebKitWebEditor

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:598
> + * Returns: (transfer none): the #WebKitWebEditor that can be used to access
> + * editting properties of a @web_page

WebKitWebEditor is more about editor notifications than properties, no? Since this links to the editor section, I wouldn't mention anything, just that it returns the editor of the page

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:40
> +        webkit_dom_document_exec_command(document, "SelectAll", false, "");
> +        webkit_dom_document_exec_command(document, "Unselect", false, "");

I think we should test more cases, for example when caret cursor moved or text selection is extended.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:51
> +            return retVal;

I don't think this is enough, if the signal was emitted when text was selected, but not when it was unselected, this would be true. We need to check the value after every selection change, and reset the variable before next test.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150706/5a9ad77a/attachment-0001.html>