<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 #253314 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#c18">Comment # 18</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=253314&amp;action=diff" name="attach_253314" title="Patch">attachment 253314</a> <a href="attachment.cgi?id=253314&amp;action=edit" title="Patch">[details]</a></span>
Patch

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

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

<span class="quote">&gt; Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:25
&gt; +#include &quot;WebKitWebPage.h&quot;</span >

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

<span class="quote">&gt; Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:27
&gt; +#include &quot;WebPage.h&quot;</span >

And this should be included from WebKitWebPagePrivate.h

<span class="quote">&gt; Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:37
&gt; + * &#64;Title: WebKitWebEditor
&gt; + *</span >

Add See also: #WebKitWebPage

<span class="quote">&gt; Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:38
&gt; + * The #WebKitWebEditor provides access to various editing capabilities of</span >

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

<span class="quote">&gt; Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:41
&gt; + */</span >

Since: 2.10

<span class="quote">&gt; Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:49
&gt; +    GRefPtr&lt;WebKitWebPage&gt; webPage;</span >

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.

<span class="quote">&gt; Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:63
&gt; +     * This signal is emitted when the selection inside a #WebKitWebPage has been
&gt; +     * changed.</span >

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

<span class="quote">&gt; Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:115
&gt; +/**
&gt; + * webkit_web_editor_get_page:
&gt; + * &#64;editor: a #WebKitWebEditor
&gt; + *
&gt; + * Gets the #WebKitWebPage that is associated with #WebKitWebEditor.
&gt; + *
&gt; + * Returns: (transfer none): the #WebKitWebPage that can be used to access
&gt; + * the #WebKitDOMDocument currently loaded into it.</span >

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

<span class="quote">&gt; Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:402
&gt; +    g_return_val_if_fail(WEBKIT_IS_WEB_PAGE(webPage), nullptr);</span >

Do not use g_return macros in private API

<span class="quote">&gt; Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:473
&gt; +    page-&gt;priv-&gt;webEditor = nullptr;
&gt; +</span >

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

<span class="quote">&gt; Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:595
&gt; + * Gets the #WebPageEditor of a #WebKitWebPage.</span >

#WebPageEditor -&gt; #WebKitWebEditor

<span class="quote">&gt; Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:598
&gt; + * Returns: (transfer none): the #WebKitWebEditor that can be used to access
&gt; + * editting properties of a &#64;web_page</span >

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

<span class="quote">&gt; Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:40
&gt; +        webkit_dom_document_exec_command(document, &quot;SelectAll&quot;, false, &quot;&quot;);
&gt; +        webkit_dom_document_exec_command(document, &quot;Unselect&quot;, false, &quot;&quot;);</span >

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

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

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.</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>