<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:cgarcia@igalia.com" title="Carlos Garcia Campos <cgarcia@igalia.com>"> <span class="fn">Carlos Garcia Campos</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Add selection-changed signal to the WebKit2 API"
href="https://bugs.webkit.org/show_bug.cgi?id=137116">bug 137116</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #256838 Flags</td>
<td>review?, commit-queue?
</td>
<td>review-, commit-queue-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Add selection-changed signal to the WebKit2 API"
href="https://bugs.webkit.org/show_bug.cgi?id=137116#c22">Comment # 22</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Add selection-changed signal to the WebKit2 API"
href="https://bugs.webkit.org/show_bug.cgi?id=137116">bug 137116</a>
from <span class="vcard"><a class="email" href="mailto:cgarcia@igalia.com" title="Carlos Garcia Campos <cgarcia@igalia.com>"> <span class="fn">Carlos Garcia Campos</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=256838&action=diff" name="attach_256838" title="Patch">attachment 256838</a> <a href="attachment.cgi?id=256838&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=256838&action=review">https://bugs.webkit.org/attachment.cgi?id=256838&action=review</a>
Patch looks good to me I only have a few comments about the test.
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:34
> + static void selectionChangedCallback(WebKitWebEditor* editor, bool* retVal)</span >
Remove the editor parameter as it's unsused. Or you can use connect_swapped and leave bool* retval only.
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:36
> + *retVal = true;</span >
This should be called returnValue, or something less generic like selectionChanged
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:42
> + WebKitDOMDocument* document = webkit_web_page_get_dom_document(page);
> +</span >
g_assert(WEBKIT_IS_DOM_DOCUMENT(document));
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:52
> + document = webkit_web_page_get_dom_document(page);</span >
WebKitDOMDocument* document = webkit_web_page_get_dom_document(page);
g_assert(WEBKIT_IS_DOM_DOCUMENT(document));
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:54
> + domWindow = webkit_dom_document_get_default_view(document);
> + domSelection = webkit_dom_dom_window_get_selection(domWindow);</span >
Same here, but in this case use GRefPtr<> foo = adoptGRef();
<span class="quote">> 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);</span >
Ditto.
<span class="quote">> 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);</span >
Ditto.
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:97
> + WebKitDOMDocument* document = webkit_web_page_get_dom_document(page);
> +</span >
g_assert(WEBKIT_IS_DOM_DOCUMENT(document));
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:104
> + bool retVal = false;</span >
Same here, either use returnValue or something less generic.
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:106
> + g_signal_connect(webkit_web_page_get_editor(page), "selection-changed", G_CALLBACK(selectionChangedCallback), &retVal);</span >
Get the editor into a local variable and add g_assert(WEBKIT_IS_WEB_EDITOR(editor)); and also assertObjectIsDeletedWhenTestFinishes(G_OBJECT(editor));
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:110
> + if (!retVal)
> + return false;</span >
Use asserts here, because this way we know which subtest failed.
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:115
> + if (!retVal)
> + return false;</span >
Ditto.
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:120
> + if (!retVal)
> + return false;</span >
Ditto.
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:125
> + if (!retVal)
> + return false;</span >
Ditto.
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:130
> + if (!retVal)
> + return false;</span >
Ditto.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>