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.
Created attachment 238655 [details] Patch Proposed patch
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,
(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 ..).
(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?
(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, ...)
After talk with Carlos we deciced that we will indeed expose this API in WebProcess instead in UIProcess. Patch follows.
Created attachment 242949 [details] Patch
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.
(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.
Created attachment 245140 [details] WIP patch
Created attachment 245143 [details] WIP patch v2 Move the editor client into WebKitWebEditor
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
(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).
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?
(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).
Created attachment 253314 [details] Patch Patch with suggestions from comment #12, test is now a WebProcess test
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.
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.
(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.
Created attachment 256838 [details] Patch
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.
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.
Created attachment 257081 [details] Patch
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.
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.
Created attachment 257093 [details] Patch
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.
Comment on attachment 257093 [details] Patch Clearing flags on attachment: 257093 Committed r187024: <http://trac.webkit.org/changeset/187024>
All reviewed patches have been landed. Closing bug.