<html>
    <head>
      <base href="https://bugs.webkit.org/" />
    </head>
    <body><span class="vcard"><a class="email" href="mailto:cgarcia&#64;igalia.com" title="Carlos Garcia Campos &lt;cgarcia&#64;igalia.com&gt;"> <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&#64;igalia.com" title="Carlos Garcia Campos &lt;cgarcia&#64;igalia.com&gt;"> <span class="fn">Carlos Garcia Campos</span></a>
</span></b>
        <pre>Comment on <span class=""><a href="attachment.cgi?id=256838&amp;action=diff" name="attach_256838" title="Patch">attachment 256838</a> <a href="attachment.cgi?id=256838&amp;action=edit" title="Patch">[details]</a></span>
Patch

View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=256838&amp;action=review">https://bugs.webkit.org/attachment.cgi?id=256838&amp;action=review</a>

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

<span class="quote">&gt; Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:34
&gt; +    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">&gt; Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:36
&gt; +        *retVal = true;</span >

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

<span class="quote">&gt; Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:42
&gt; +        WebKitDOMDocument* document = webkit_web_page_get_dom_document(page);
&gt; +</span >

g_assert(WEBKIT_IS_DOM_DOCUMENT(document));

<span class="quote">&gt; Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:52
&gt; +        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">&gt; Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:54
&gt; +        domWindow = webkit_dom_document_get_default_view(document);
&gt; +        domSelection = webkit_dom_dom_window_get_selection(domWindow);</span >

Same here, but in this case use GRefPtr&lt;&gt; foo = adoptGRef();

<span class="quote">&gt; Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:70
&gt; +        WebKitDOMDocument* document;
&gt; +        WebKitDOMDOMSelection* domSelection;
&gt; +        WebKitDOMDOMWindow* domWindow;
&gt; +
&gt; +        document = webkit_web_page_get_dom_document(page);
&gt; +        domWindow = webkit_dom_document_get_default_view(document);
&gt; +        domSelection = webkit_dom_dom_window_get_selection(domWindow);</span >

Ditto.

<span class="quote">&gt; Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:86
&gt; +        WebKitDOMDocument* document;
&gt; +        WebKitDOMDOMSelection* domSelection;
&gt; +        WebKitDOMDOMWindow* domWindow;
&gt; +
&gt; +        document = webkit_web_page_get_dom_document(page);
&gt; +        domWindow = webkit_dom_document_get_default_view(document);
&gt; +        domSelection = webkit_dom_dom_window_get_selection(domWindow);</span >

Ditto.

<span class="quote">&gt; Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:97
&gt; +        WebKitDOMDocument* document = webkit_web_page_get_dom_document(page);
&gt; +</span >

g_assert(WEBKIT_IS_DOM_DOCUMENT(document));

<span class="quote">&gt; Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:104
&gt; +            bool retVal = false;</span >

Same here, either use returnValue or something less generic.

<span class="quote">&gt; Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:106
&gt; +            g_signal_connect(webkit_web_page_get_editor(page), &quot;selection-changed&quot;, G_CALLBACK(selectionChangedCallback), &amp;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">&gt; Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:110
&gt; +            if (!retVal)
&gt; +                return false;</span >

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

<span class="quote">&gt; Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:115
&gt; +            if (!retVal)
&gt; +                return false;</span >

Ditto.

<span class="quote">&gt; Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:120
&gt; +            if (!retVal)
&gt; +                return false;</span >

Ditto.

<span class="quote">&gt; Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:125
&gt; +            if (!retVal)
&gt; +                return false;</span >

Ditto.

<span class="quote">&gt; Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:130
&gt; +            if (!retVal)
&gt; +                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>