Bug 137116 – [GTK] Add selection-changed signal to the WebKit2 API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Tomas Popela]

The selection-changed signal is missing in WK2 (compared to WK1) API. It would be nice to have it there as it is needed for the successful port of Evolution composer from WK1 to WK2.

[Tomas Popela]

Created attachment 238655 [details]
Patch

Proposed patch

[Carlos Garcia Campos]

Comment on attachment 238655 [details]
Patch

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

Thanks for the patch, but new API should include a unit test.

> Source/WebKit2/ChangeLog:9
> +        Add the selection-changed signal into the WebKit2 GTK+ API. This
> +        signal will be emitted everytime the selection in page is changed.

What's the use case? what does evo do when the selection changes?

> Source/WebKit2/ChangeLog:14
> +        * WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:
> +        (didChangeSelection):
> +        (webkit_web_page_class_init):
> +        (webkitWebPageCreate):

Why do you need this in the web extensions API?

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:289
> +     */

Since: 2.8

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:294
> +        0, 0, 0,

0, nullptr, nullptr,

[Tomas Popela]

(In reply to comment #2)
> > Source/WebKit2/ChangeLog:9
> > +        Add the selection-changed signal into the WebKit2 GTK+ API. This
> > +        signal will be emitted everytime the selection in page is changed.
> 
> What's the use case? what does evo do when the selection changes?
Basically the whole UI (all buttons regarding the formatting (ie. bold font, block format)) is operating on callback on this signal. So when user is moving with the caret through the content of the composer the UI is updated accordingly (it also applies situations where some content is selected).
> 
> > Source/WebKit2/ChangeLog:14
> > +        * WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:
> > +        (didChangeSelection):
> > +        (webkit_web_page_class_init):
> > +        (webkitWebPageCreate):
> 
> Why do you need this in the web extensions API?

Hm it was mostly selfish decision (at least regarding to the Evolution) as we are doing various operations above DOM to get the actual state of content format (ie. is this paragraph indented, size of font, format of the block ..).

[Carlos Garcia Campos]

(In reply to comment #3)
> (In reply to comment #2)
> > > Source/WebKit2/ChangeLog:9
> > > +        Add the selection-changed signal into the WebKit2 GTK+ API. This
> > > +        signal will be emitted everytime the selection in page is changed.
> > 
> > What's the use case? what does evo do when the selection changes?
> Basically the whole UI (all buttons regarding the formatting (ie. bold font, block format)) is operating on callback on this signal. So when user is moving with the caret through the content of the composer the UI is updated accordingly (it also applies situations where some content is selected).

So, this sounds like you need that in the UI process then.

> > > Source/WebKit2/ChangeLog:14
> > > +        * WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:
> > > +        (didChangeSelection):
> > > +        (webkit_web_page_class_init):
> > > +        (webkitWebPageCreate):
> > 
> > Why do you need this in the web extensions API?
> 
> Hm it was mostly selfish decision (at least regarding to the Evolution) as we are doing various operations above DOM to get the actual state of content format (ie. is this paragraph indented, size of font, format of the block ..).

Would it be possible to do the same by exposing the EditorState in the UI process API?

[Tomas Popela]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > > Source/WebKit2/ChangeLog:9
> > > > +        Add the selection-changed signal into the WebKit2 GTK+ API. This
> > > > +        signal will be emitted everytime the selection in page is changed.
> > > 
> > > What's the use case? what does evo do when the selection changes?
> > Basically the whole UI (all buttons regarding the formatting (ie. bold font, block format)) is operating on callback on this signal. So when user is moving with the caret through the content of the composer the UI is updated accordingly (it also applies situations where some content is selected).
> 
> So, this sounds like you need that in the UI process then.
OK, let's move it there. I will update the patch with previous recommendations.
> 
> > > > Source/WebKit2/ChangeLog:14
> > > > +        * WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:
> > > > +        (didChangeSelection):
> > > > +        (webkit_web_page_class_init):
> > > > +        (webkitWebPageCreate):
> > > 
> > > Why do you need this in the web extensions API?
> > 
> > Hm it was mostly selfish decision (at least regarding to the Evolution) as we are doing various operations above DOM to get the actual state of content format (ie. is this paragraph indented, size of font, format of the block ..).
> 
> Would it be possible to do the same by exposing the EditorState in the UI process API?

For some things yes (font properties, ...) but for some not as some of them depend on other things (DOM structure, class names, ...)

[Tomas Popela]

After talk with Carlos we deciced that we will indeed expose this API in WebProcess instead in UIProcess. Patch follows.

[Tomas Popela]

Created attachment 242949 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 242949 [details]
Patch

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

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:420
> +    WKBundlePageEditorClientV0 editorClient = {
> +        {
> +            0, // version
> +            page, // clientInfo
> +        },
> +        0, // shouldBeginEditing
> +        0, // shouldEndEditing
> +        0, // shouldInsertNode
> +        0, // shouldInsertText
> +        0, // shouldDeleteRange
> +        0, // shouldChangeSelectedRange
> +        0, // shouldApplyStyle
> +        0, // didBeginEditing
> +        0, // didEndEditing
> +        0, // didChange
> +        didChangeSelection
> +    };
> +    WKBundlePageSetEditorClient(toAPI(webPage), &editorClient.base);

I wonder if it would be better to expose this as a WebKitWebEditor, and add the signals there, just in case we end up needing more of those calbacks exposed. The user would do webkit_web_page_get_editor, and connect to the signals there.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebExtensions.cpp:100
> +    GUniquePtr<char> extensionBusName(g_strdup_printf("org.webkit.gtk.WebExtensionTest%u", Test::s_webExtensionID));
> +    GRefPtr<GDBusProxy> proxy = adoptGRef(bus->createProxy(extensionBusName.get(),
> +        "/org/webkit/gtk/WebExtensionTest", "org.webkit.gtk.WebExtensionTest", test->m_mainLoop));
> +    GDBusConnection* connection = g_dbus_proxy_get_connection(proxy.get());
> +    guint id = g_dbus_connection_signal_subscribe(connection,
> +        0,
> +        "org.webkit.gtk.WebExtensionTest",
> +        "SelectionChanged",
> +        "/org/webkit/gtk/WebExtensionTest",
> +        0,
> +        G_DBUS_SIGNAL_FLAGS_NONE,
> +        reinterpret_cast<GDBusSignalCallback>(selectionChangedCallback),
> +        test,
> +        0);
> +    g_assert(id);

We can avoid all this now by using user script messages.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebExtensions.cpp:107
> +    test->loadHtml("<html><body>All work and no play make Jack a dull boy.</body></html>", 0);
> +    test->waitUntilLoadFinished();
> +
> +    webkit_web_view_execute_editing_command(test->m_webView, WEBKIT_EDITING_COMMAND_SELECT_ALL);
> +
> +    g_main_loop_run(test->m_mainLoop);

We could also tests that the signal is emitted again after clearing the selection, for example.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp:147
> +    // FIXME: Too much code just to send a message, we need convenient custom API for this.
> +    WebKitDOMDocument* document = webkit_web_page_get_dom_document(webPage);
> +    WebKitDOMDOMWindow* window = webkit_dom_document_get_default_view(document);
> +    if (WebKitDOMWebKitNamespace* webkit = webkit_dom_dom_window_get_webkit_namespace(window)) {
> +        WebKitDOMUserMessageHandlersNamespace* messageHandlers = webkit_dom_webkit_namespace_get_message_handlers(webkit);
> +        if (WebKitDOMUserMessageHandler* handler = webkit_dom_user_message_handlers_namespace_get_handler(messageHandlers, "dom"))
> +            webkit_dom_user_message_handler_post_message(handler, "SelectionChanged");
> +    }
> +
> +    webkit_dom_dom_window_webkit_message_handlers_post_message(window, "dom-convenience", "SelectionChanged");

So, you are indeed using user script messages, but not connecting to the signal. I guess you copy-pasted. So, we now have the convenient api, so you could simply call webkit_dom_dom_window_webkit_message_handlers_post_message() with "editor" as message handler name. In the UI process part of the test, instead of doing all the dbus proxy, you can simply connect to "script-message-received::editor" of WebKitUserContentManager. With that we don't need the the dbus signal at all, and everything is simpler.

[Martin Robinson]

(In reply to comment #8)
> I wonder if it would be better to expose this as a WebKitWebEditor, and add
> the signals there, just in case we end up needing more of those calbacks
> exposed. The user would do webkit_web_page_get_editor, and connect to the
> signals there.

I had roughly the same internal dialog.

[Tomas Popela]

Created attachment 245140 [details]
WIP patch

[Tomas Popela]

Created attachment 245143 [details]
WIP patch v2

Move the editor client into WebKitWebEditor

[Carlos Garcia Campos]

Comment on attachment 245143 [details]
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

[Tomas Popela]

(In reply to comment #12)
> > 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.

The WebPage will be removed as I will do the private method that you suggested. I mean to keep it for the future as I think that the stuff that we will be done in WebEditor will need to access the WebPage. For the WebKitWebPage I left it there to simplify the access to the DOM (as shown in the tests).
 
> > 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?
>

For every selection change in a WebView.
 
> > 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.

I already tried it and:

In file included from ../../Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.h:29:0,
                 from ../../Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:21:
DerivedSources/ForwardingHeaders/webkit2gtk-webextension/webkit2/WebKitWebPage.h:74:40: error: ‘WebKitWebEditor’ does not name a type
 WEBKIT_API WebKitWebEditor *
 
> > 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

I moved it here because the infrastructure for handlers is there. But doing it in separate module makes really sense (in TestEditor).

[Gustavo Noronha (kov)]

Comment on attachment 245143 [details]
WIP patch v2

API looks sensible to me. From what you said before, Tomas, you use this signal to watch cursor movement even, so I guess when you say it will be emitted for every selection change in webview you mean it will happen when: something is selected, something is deselected, cursor changes position?

[Tomas Popela]

(In reply to comment #14)
> Comment on attachment 245143 [details]
> WIP patch v2
> 
> API looks sensible to me. From what you said before, Tomas, you use this
> signal to watch cursor movement even, so I guess when you say it will be
> emitted for every selection change in webview you mean it will happen when:
> something is selected, something is deselected, cursor changes position?

Yes, that's exactly it. It is emitted when the selection in view is changed (cursor (caret) is just the special case of selection - the collapsed one).

[Tomas Popela]

Created attachment 253314 [details]
Patch

Patch with suggestions from comment #12, test is now a WebProcess test

[WebKit Commit Bot]

Attachment 253314 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.h"
ERROR: Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/WebProcess/InjectedBundle/API/gtk/webkit-web-extension.h"
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 253314 [details]
Patch

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

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

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:25
> +#include "WebKitWebPage.h"

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

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:27
> +#include "WebPage.h"

And this should be included from WebKitWebPagePrivate.h

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:37
> + * @Title: WebKitWebEditor
> + *

Add See also: #WebKitWebPage

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:38
> + * The #WebKitWebEditor provides access to various editing capabilities of

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

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

Since: 2.10

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:49
> +    GRefPtr<WebKitWebPage> webPage;

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.

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

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

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:115
> +/**
> + * webkit_web_editor_get_page:
> + * @editor: a #WebKitWebEditor
> + *
> + * Gets the #WebKitWebPage that is associated with #WebKitWebEditor.
> + *
> + * Returns: (transfer none): the #WebKitWebPage that can be used to access
> + * the #WebKitDOMDocument currently loaded into it.

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

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

Do not use g_return macros in private API

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:473
> +    page->priv->webEditor = nullptr;
> +

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

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

#WebPageEditor -> #WebKitWebEditor

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:598
> + * Returns: (transfer none): the #WebKitWebEditor that can be used to access
> + * editting properties of a @web_page

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

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:40
> +        webkit_dom_document_exec_command(document, "SelectAll", false, "");
> +        webkit_dom_document_exec_command(document, "Unselect", false, "");

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

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:51
> +            return retVal;

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.

[Tomas Popela]

(In reply to comment #18)
> > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:115
> > +/**
> > + * webkit_web_editor_get_page:
> > + * @editor: a #WebKitWebEditor
> > + *
> > + * Gets the #WebKitWebPage that is associated with #WebKitWebEditor.
> > + *
> > + * Returns: (transfer none): the #WebKitWebPage that can be used to access
> > + * the #WebKitDOMDocument currently loaded into it.
> 
> If this is mainly useful for accessing the DOM, maybe it makes more sense to
> have webkit_web_editor_get_dom_document?
> 

As you wrote in #c12:

> the page associated to the editor, that can be used for anything else apart from accessing the DOM

I would not do a special API just for accessing the DOM as for me it looks like some unnecessary and unwanted duplication. You are also closing the doors for accessing information that are in the WebKitWebPage (we really don't know what we will add there in the future) and I don't think we want to have getters for every interesting information in WebKitWebEditor's POV.

[Tomas Popela]

Created attachment 256838 [details]
Patch

[WebKit Commit Bot]

Attachment 256838 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.h"
ERROR: Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:25:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/WebProcess/InjectedBundle/API/gtk/webkit-web-extension.h"
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 256838 [details]
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.

[Tomas Popela]

Created attachment 257081 [details]
Patch

[WebKit Commit Bot]

Attachment 257081 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.h"
ERROR: Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:25:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/WebProcess/InjectedBundle/API/gtk/webkit-web-extension.h"
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 257081 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:51
> +        GRefPtr<WebKitDOMDOMWindow> domWindow = adoptGRef(webkit_dom_document_get_default_view(document));

g_assert(WEBKIT_DOM_IS_DOM_WINDOW(domWindow.get()));

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:52
> +        GRefPtr<WebKitDOMDOMSelection> domSelection = adoptGRef(webkit_dom_dom_window_get_selection(domWindow.get()));

And same here for the selection.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:62
> +        GRefPtr<WebKitDOMDOMWindow> domWindow = adoptGRef(webkit_dom_document_get_default_view(document));
> +        GRefPtr<WebKitDOMDOMSelection> domSelection = adoptGRef(webkit_dom_dom_window_get_selection(domWindow.get()));

Ditto.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/EditorTest.cpp:72
> +        GRefPtr<WebKitDOMDOMWindow> domWindow = adoptGRef(webkit_dom_document_get_default_view(document));
> +        GRefPtr<WebKitDOMDOMSelection> domSelection = adoptGRef(webkit_dom_dom_window_get_selection(domWindow.get()));

Ditto.

[Tomas Popela]

Created attachment 257093 [details]
Patch

[WebKit Commit Bot]

Attachment 257093 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.h"
ERROR: Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:25:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/WebProcess/InjectedBundle/API/gtk/webkit-web-extension.h"
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 257093 [details]
Patch

Clearing flags on attachment: 257093

Committed r187024: <http://trac.webkit.org/changeset/187024>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.