[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

--- Comment #12 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 245143
  --> https://bugs.webkit.org/attachment.cgi?id=245143
WIP patch v2

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

The API looks good to me, we can add more signals to the Editor object in the future if needed without breaking the API. Gustavo? Martin?

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

We should add a doc section here explaining a bit what this class is.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:40
> +    GRefPtr<WebKitWebPage> webkitWebPage;
> +    WebPage* webPage;

Why do you need to keep a reference of WebKitWebPage and why do we need to keep a pointer to WebPage?. 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:50
> +     * WebKitWebPage::selection-changed:

WebKitWebPage -> WebKitWebEditor

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

Is this emitted for every selection change in a WebView? or only selections in editable areas?

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:67
> +static void didChangeSelection(WKBundlePageRef, WKStringRef /* notificationName */, const void *clientInfo)

const void *clientInfo -> const void* clientInfo

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:72
> +WebKitWebEditor* webkitWebEditorCreate(WebKitWebPage* webkitWebPage, WebPage* webPage)

Add a private method to WebKitWebPage to return its WebPage. That way here we would only receive the WebKitWebPage and we don't need to keep a pointer to the WebPage in the private struct.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:74
> +    WebKitWebEditor* editor = WEBKIT_WEB_EDITOR(g_object_new(WEBKIT_TYPE_WEB_EDITOR, NULL));

nullptr instead of NULL

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:92
> +        0, // shouldBeginEditing
> +        0, // shouldEndEditing
> +        0, // shouldInsertNode
> +        0, // shouldInsertText
> +        0, // shouldDeleteRange
> +        0, // shouldChangeSelectedRange
> +        0, // shouldApplyStyle
> +        0, // didBeginEditing
> +        0, // didEndEditing
> +        0, // didChange

Use nullptr instead of 0 in all these.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:107
> + * Returns: (transfer none): the #WebKitWebPage that can be used to access
> + * the #WebKitDOMDocument currently loaded into it.

I would not mention #WebKitDOMDocument, this is just the page associated to the editor, that can be used for anything else apart from accessing the DOM.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.h:61
> +WEBKIT_API GType
> +webkit_web_editor_get_type                           (void);
> +
> +WEBKIT_API WebKitWebPage *
> +webkit_web_editor_get_page                           (WebKitWebEditor *editor);

Remove all the extra spaces between function name and arguments

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

Use Gets instead of Returns to avoid the double Returns

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

Use nullptr instead of 0

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:585
> +    return webPage->priv->webEditor.get();

Maybe we could create the editor object on demand here. That way if it's not used by the application, the object will never be created.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.h:47
> +/* Forward declarations */
> +typedef struct _WebKitWebEditor      WebKitWebEditor;

You shouldn't need this, since you are including WebKitWebEditor.h.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:375
> +    test->loadHtml("<html><body>All work and no play make Jack a dull boy.</body></html>", 0);

nullptr instead of 0

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitUserContentManager.cpp:404
> +    UserScriptMessageTest::add("WebKitWebEditor", "selection-changed", testSelectionChangedSignal);

Why is this a user content manger test? This should be in TestEditor I guess, if we need to make UserScriptMessageTest available to other tests we can move it to the test lib

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp:244
> +    WebKitWebEditor *webEditor = webkit_web_page_get_editor (webPage);

estra space between function name and (

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp:245
> +    g_signal_connect(webEditor, "selection-changed", G_CALLBACK(selectionChangedCallback), nullptr);

we could use webkit_web_page_get_editor(webPage) here directly

-- 
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/20150123/4d208f31/attachment-0002.html>