[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 #256838|review?, commit-queue?      |review-, commit-queue-
              Flags|                            |

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

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

Patch looks good to me I only have a few comments about the test.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:34
> +    static void selectionChangedCallback(WebKitWebEditor* editor, bool* retVal)

Remove the editor parameter as it's unsused. Or you can use connect_swapped and leave bool* retval only.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:36
> +        *retVal = true;

This should be called returnValue, or something less generic like selectionChanged

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:42
> +        WebKitDOMDocument* document = webkit_web_page_get_dom_document(page);
> +

g_assert(WEBKIT_IS_DOM_DOCUMENT(document));

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:52
> +        document = webkit_web_page_get_dom_document(page);

WebKitDOMDocument* document = webkit_web_page_get_dom_document(page);
g_assert(WEBKIT_IS_DOM_DOCUMENT(document));

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:54
> +        domWindow = webkit_dom_document_get_default_view(document);
> +        domSelection = webkit_dom_dom_window_get_selection(domWindow);

Same here, but in this case use GRefPtr<> foo = adoptGRef();

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:70
> +        WebKitDOMDocument* document;
> +        WebKitDOMDOMSelection* domSelection;
> +        WebKitDOMDOMWindow* domWindow;
> +
> +        document = webkit_web_page_get_dom_document(page);
> +        domWindow = webkit_dom_document_get_default_view(document);
> +        domSelection = webkit_dom_dom_window_get_selection(domWindow);

Ditto.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:86
> +        WebKitDOMDocument* document;
> +        WebKitDOMDOMSelection* domSelection;
> +        WebKitDOMDOMWindow* domWindow;
> +
> +        document = webkit_web_page_get_dom_document(page);
> +        domWindow = webkit_dom_document_get_default_view(document);
> +        domSelection = webkit_dom_dom_window_get_selection(domWindow);

Ditto.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:97
> +        WebKitDOMDocument* document = webkit_web_page_get_dom_document(page);
> +

g_assert(WEBKIT_IS_DOM_DOCUMENT(document));

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:104
> +            bool retVal = false;

Same here, either use returnValue or something less generic.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:106
> +            g_signal_connect(webkit_web_page_get_editor(page), "selection-changed", G_CALLBACK(selectionChangedCallback), &retVal);

Get the editor into a local variable and add g_assert(WEBKIT_IS_WEB_EDITOR(editor)); and also assertObjectIsDeletedWhenTestFinishes(G_OBJECT(editor));

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:110
> +            if (!retVal)
> +                return false;

Use asserts here, because this way we know which subtest failed.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:115
> +            if (!retVal)
> +                return false;

Ditto.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:120
> +            if (!retVal)
> +                return false;

Ditto.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:125
> +            if (!retVal)
> +                return false;

Ditto.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:130
> +            if (!retVal)
> +                return false;

Ditto.

-- 
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/20150720/c60aa1f9/attachment-0001.html>