Add webkit_web_editor_is_editing_command_enabled() so that context menu item availability can be decided in the web process and then sent to the UI process via the WebKitContextMenu user data. This is superior to sending the WebKitContextMenu from the web process to the UI process without this information, and then subsequently performing several IPC queries from the UI process to the web process to check the availability of editing commands. I want it to fix a context menu flickering problem in Epiphany. Since we have to show the WebKitContextMenu in a WebKitWebView::context-menu handler, but cannot check the availability of editing commands there without using sync IPC, we currently assume all editing commands are available, add them to the menu, start querying the availability with webkit_web_view_can_execute_editing_command(), show the menu, and later remove them from the menu once that finishes, causing the flicker.
Note: I decided not to name it webkit_web_editor_can_execute_editing_command() because there's still no API to do that from WebKitWebEditor.
Created attachment 301330 [details] Patch
Comment on attachment 301330 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301330&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitEditingCommands.h:122 > +/* If exposing new editing commands, also update WebKitWebEditor.h. */ No, we could simply make this header common to both APIs. Like WebKitURIRequest.h. > Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-4.0-sections.txt:1356 > +webkit_web_editor_is_editing_command_enabled I know enabled is used internally, but it made me think as if it can be disabled or something, like a setting. I really like the can_execute and it would be consistent with the UI process API. > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:123 > + g_return_val_if_fail(WEBKIT_IS_WEB_EDITOR(editor), nullptr); Oops, good catch. > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:135 > + * Determines whether the editing command can currently be executed in > + * the #WebKitWebView corresponding to @editor using > + * webkit_web_view_execute_editing_command(). the #WebKitWebView corresponding to @editor. This doesn't belong to WebKitWebEditor object, but to WebKitWebPage. webkit_web_page_can_execute_editing_command(). > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:145 > + return webkitWebPageGetPage(editor->priv->webPage)->isEditingCommandEnabled(String::fromUTF8(command)); It's indeed Page API. This web editor is actually a client. This will actually check if the command can be executed in the editor of the focused or main frame. If there's a plugin focused, the plugin will decide. > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.h:33 > +/* Duplicated in WebKitEditingCommands.h. */ No, please.
(In reply to comment #1) > Note: I decided not to name it > webkit_web_editor_can_execute_editing_command() because there's still no API > to do that from WebKitWebEditor. But we could add it eventually. I prefer to be consistent with the UI process API. Also, user can execute editing commands with javascript, or using the DOM bindings (webkit_dom_document_exec_command), so we don't need the execute method to have a can execute.
(In reply to comment #3) > No, we could simply make this header common to both APIs. Like > WebKitURIRequest.h. OK. > > Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-4.0-sections.txt:1356 > > +webkit_web_editor_is_editing_command_enabled > > I know enabled is used internally, but it made me think as if it can be > disabled or something, like a setting. I really like the can_execute and it > would be consistent with the UI process API. OK. > > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:123 > > + g_return_val_if_fail(WEBKIT_IS_WEB_EDITOR(editor), nullptr); > > Oops, good catch. Will do this in a separate patch now that this doesn't touch WebKitWebEditor.cpp anymore. > > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:135 > > + * Determines whether the editing command can currently be executed in > > + * the #WebKitWebView corresponding to @editor using > > + * webkit_web_view_execute_editing_command(). > > the #WebKitWebView corresponding to @editor. This doesn't belong to > WebKitWebEditor object, but to WebKitWebPage. > webkit_web_page_can_execute_editing_command(). OK.
(In reply to comment #5) > (In reply to comment #3) > > No, we could simply make this header common to both APIs. Like > > WebKitURIRequest.h. > > OK. > > > > Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-4.0-sections.txt:1356 > > > +webkit_web_editor_is_editing_command_enabled > > > > I know enabled is used internally, but it made me think as if it can be > > disabled or something, like a setting. I really like the can_execute and it > > would be consistent with the UI process API. > > OK. > > > > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:123 > > > + g_return_val_if_fail(WEBKIT_IS_WEB_EDITOR(editor), nullptr); > > > > Oops, good catch. > > Will do this in a separate patch now that this doesn't touch > WebKitWebEditor.cpp anymore. You can land it unreviewed. > > > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebEditor.cpp:135 > > > + * Determines whether the editing command can currently be executed in > > > + * the #WebKitWebView corresponding to @editor using > > > + * webkit_web_view_execute_editing_command(). > > > > the #WebKitWebView corresponding to @editor. This doesn't belong to > > WebKitWebEditor object, but to WebKitWebPage. > > webkit_web_page_can_execute_editing_command(). > > OK.
Created attachment 301357 [details] Patch
Comment on attachment 301357 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301357&action=review hmm, I've just realized that what we want is already sent from web process to the ui process when showing the context, it was added long time ago in r176999, but we never exposed it in our API. So, I think it would be better to expose allowsCopy in our hit test result, and maybe other useful information not exposed. > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:745 > + * Determines whether the editing command can currently be executed in > + * the #WebKitWebView corresponding to @web_page using > + * webkit_web_view_execute_editing_command(). I wouldn't mention WebKitWebView here, people are already confused enough between the two APIs. Determines whether the editing command can currently be executed in @web_page.
(In reply to comment #8) > hmm, I've just realized that what we want is already sent from web process > to the ui process when showing the context, it was added long time ago in > r176999, but we never exposed it in our API. So, I think it would be better > to expose allowsCopy in our hit test result, and maybe other useful > information not exposed. So looking at this, I at first thought it might indeed be better to expose more of this instead. But now I'm not sure. I don't quite understand how to do it. What we need in Epiphany is whether cut, copy, paste, undo, and redo are allowed. WebHitTestResult only exposes Copy. I think we could get the answer for cut and paste using the existing webkit_hit_test_result_context_is_editable() and webkit_hit_test_result_context_is_selection() functions: they should be available if both are true, and not otherwise. Assuming there's no plugin container. But this feels more fragile than using the Editor. And I don't see any way to get undo/redo information via hit tests. What do you think? > > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:745 > > + * Determines whether the editing command can currently be executed in > > + * the #WebKitWebView corresponding to @web_page using > > + * webkit_web_view_execute_editing_command(). > > I wouldn't mention WebKitWebView here, people are already confused enough > between the two APIs. > > Determines whether the editing command can currently be executed in > @web_page. OK.
(In reply to comment #9) > (In reply to comment #8) > > hmm, I've just realized that what we want is already sent from web process > > to the ui process when showing the context, it was added long time ago in > > r176999, but we never exposed it in our API. So, I think it would be better > > to expose allowsCopy in our hit test result, and maybe other useful > > information not exposed. > > So looking at this, I at first thought it might indeed be better to expose > more of this instead. But now I'm not sure. I don't quite understand how to > do it. What we need in Epiphany is whether cut, copy, paste, undo, and redo > are allowed. WebHitTestResult only exposes Copy. I think we could get the > answer for cut and paste using the existing > webkit_hit_test_result_context_is_editable() and > webkit_hit_test_result_context_is_selection() functions: they should be > available if both are true, and not otherwise. Assuming there's no plugin > container. But this feels more fragile than using the Editor. And I don't > see any way to get undo/redo information via hit tests. > > What do you think? > The problem of using the editor like your patch does is that it checks if the web page can execute the giving command. That's actually the editor associated to the main or focused frame. If the right click always focuses, then that's not a problem, though. The advantage of the hit test result is that it always refers to the most specific node where the right click happened. What I would do here, is extend hit test result to provide more editor capabilities (or state) and return a bitmask with CanCopy/CanCut/CanPaste/CanUndo/CanRedo and replace allowsCopy with that.
Comment on attachment 301357 [details] Patch OK then.
(CCing Beth in case there's some reason that only copy was ever exposed in the past.)
Here's a first stab. Not tested yet; I'll wait for API comments first. Not really happy with how it turned out....
Created attachment 302007 [details] Patch
cgarcia: mcatanzaro: btw, I think we can leave the new context menu api for the next cycle, and you can probably workaround the problem in ephy using dom bdings and context menu user data mcatanzaro: cgarcia: I don't have more time to think about it now. :( Context menu user data is great. Do the DOM bindings provide canCut/Copy/Paste/Undo/Redo somehow...? cgarcia: yes cgarcia: in dom_document I think mcatanzaro: cgarcia: webkit_dom_document_query_command_enabled() cgarcia: right mcatanzaro: cgarcia: TBH if I knew that existed, I would not have bothered with writing a WebKit patch. :P
mcatanzaro: cgarcia: Looks like we have to set settings.javaScriptCanAccessClipboard() and also settings.clipboardAccessPolicy() mcatanzaro: So DOM API looks like a no-go
(In reply to Michael Catanzaro from comment #13) > Here's a first stab. Not tested yet; I'll wait for API comments first. Not > really happy with how it turned out.... Not sure why I said I was unhappy with this. It's more code than my original patch, but it seems nicer to use the hit test result for this. I'll rebase it and add a test.
Created attachment 319064 [details] Patch
Comment on attachment 319064 [details] Patch Mac EWS are red with this patch
Created attachment 319080 [details] Patch
Comment on attachment 319080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319080&action=review > Source/WebCore/rendering/HitTestResult.cpp:599 > -bool HitTestResult::allowsCopy() const > +auto HitTestResult::allowedEditCommands() const -> EditCommands You could use OptionSet instead of simple enum + static_cast > Source/WebCore/rendering/HitTestResult.h:135 > + enum EditCommands { This should be an enum class AllowedEditCommands > Source/WebKit/Shared/API/glib/WebKitHitTestResult.cpp:510 > + * webkit_hit_test_result_get_available_commands: available commands sounds too generic, we are referring to editing commands. I'm not sure about exposing an enum for this API here. I would rather expose methods to query them. It's more boilerplate, but the API is less confusing. > Source/WebKit/Shared/WebHitTestResultData.h:63 > + unsigned allowedEditCommands; This should also be an OptionSet > Source/WebKit/UIProcess/API/gtk/WebKitHitTestResult.h:67 > + * WebKitHitTestResultCommands: So the name of the enum sounds like it's going to list commands, but it lists availability to run them > Source/WebKit/UIProcess/API/gtk/WebKitHitTestResult.h:75 > + * Enum values with flags representing commands that can be performed at > + * hit test locations. This is only about editing commands > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:567 > + // Copied from TestWebViewEditor. There's no way to know when the selection > + // has been copied to the clipboard, so use a timeout source to query the > + // clipboard. Note this function could quit early if some other text is > + // already present in the clipboard, so this function can't be relied on to > + // actually copy the desired text before returning, but our test only cares > + // that *something* is copied to the clipboard. > + webkit_web_view_execute_editing_command(test->m_webView, WEBKIT_EDITING_COMMAND_COPY); We could move this to WebViewTest and then do test->waitUntilTextIsAvailableInClipboard(). > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:584 > + webkit_web_view_run_javascript(test->m_webView, "document.getElementById('editable').select();", nullptr, [](GObject* sourceObject, GAsyncResult* result, gpointer userData) { Use test->runJavaScriptAndWaitUntilFinished() > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:593 > + auto mainLoop = static_cast<GMainLoop*>(userData); > + auto webView = WEBKIT_WEB_VIEW(sourceObject); > + GUniqueOutPtr<GError> error; > + WebKitJavascriptResult* jsResult = webkit_web_view_run_javascript_finish(webView, result, &error.outPtr()); > + g_assert_no_error(error.get()); > + webkit_javascript_result_unref(jsResult); > + g_main_loop_quit(mainLoop); > + }, test->m_mainLoop); > + g_main_loop_run(test->m_mainLoop); And you don't need to do all this. > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:605 > + webkit_web_view_run_javascript(test->m_webView, "document.getElementById('editable').value;", nullptr, [](GObject* sourceObject, GAsyncResult* result, gpointer userData) { test->runJavaScriptAndWaitUntilFinished() > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:621 > + JSGlobalContextRef context = webkit_javascript_result_get_global_context(jsResult); > + JSValueRef value = webkit_javascript_result_get_value(jsResult); > + g_assert(JSValueIsString(context, value)); > + > + JSStringRef jsStringValue = JSValueToStringCopy(context, value, nullptr); > + gsize stringLength = JSStringGetMaximumUTF8CStringSize(jsStringValue); > + GUniquePtr<char> stringValue(static_cast<char*>(g_malloc(stringLength))); > + JSStringGetUTF8CString(jsStringValue, stringValue.get(), stringLength); > + JSStringRelease(jsStringValue); > + webkit_javascript_result_unref(jsResult); test->javascriptResultToCString
I've made all the changes you suggested. Much better, thanks. Especially the test.
Created attachment 321510 [details] Patch
Could an owner review this patch, please?
Comment on attachment 321510 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=321510&action=review > Source/WebKit/Shared/API/glib/WebKitHitTestResult.cpp:252 > "link-title", !hitTestResult.linkTitle.isEmpty() ? hitTestResult.linkTitle.utf8().data() : nullptr, > "link-label", !hitTestResult.linkLabel.isEmpty() ? hitTestResult.linkLabel.utf8().data() : nullptr, > nullptr)); > + result->priv->availableEditCommands = hitTestResult.availableEditCommands; > + return result; Carlos, is this the proper idiomatic way to initialize a priv member that's not a GObject property? This would definitely be wrong if it was a public webkit_hit_test_result_new() method since those are not introspectable and bindings would be broken. But this is a private function, so there's no correctness issue, and I can't think of any better way to do it, so I guess it's fine.
You should be able to check this in EditorState. I don't think we should be tying this to HitTestResult.
Comment on attachment 321510 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=321510&action=review r- because exposing editor states in HitTestResult is a clear layering violation. > Source/WebKit/ChangeLog:13 > + I want to fix a context menu flickering problem in Epiphany. Since we have to show the > + WebKitContextMenu in a WebKitWebView::context-menu handler, but cannot check the > + availability of editing commands there without using sync IPC, we currently assume all > + editing commands are available, add them to the menu, start querying the availability with > + webkit_web_view_can_execute_editing_command(), show the menu, and later remove them from the > + menu once that finishes, causing the flicker. You should be able to do that by checking EditorState. If not, we should be sending EditorState update as we tell UI process to show the context menu.
(In reply to Ryosuke Niwa from comment #27) > Comment on attachment 321510 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=321510&action=review > > r- because exposing editor states in HitTestResult is a clear layering > violation. I guess there's two possible layering violations: * Do you not want WebCore types used in the WebKit C++ API? If we still care about this API, then I can add a matching WebKit-level enum class with functions to translate between the two. * Do you not want anything editor-related exposed in HitTestResult? If so, the copy availability state is already there, and I presume that Safari probably depends on that. And I think it's actually needed, see below. > You should be able to do that by checking EditorState. > If not, we should be sending EditorState update as we tell UI process to > show the context menu. My original patch, attachment #301330 [details], made that possible to do, but I got r- from Carlos because: (In reply to Carlos Garcia Campos from comment #10) > The problem of using the editor like your patch does is that it checks if > the web page can execute the giving command. That's actually the editor > associated to the main or focused frame. If the right click always focuses, > then that's not a problem, though. The advantage of the hit test result is > that it always refers to the most specific node where the right click > happened. What I would do here, is extend hit test result to provide more > editor capabilities (or state) and return a bitmask with > CanCopy/CanCut/CanPaste/CanUndo/CanRedo and replace allowsCopy with that. So that's why I took this more complicated approach. Do you have an alternative suggestion?
(In reply to Michael Catanzaro from comment #28) > (In reply to Ryosuke Niwa from comment #27) > > Comment on attachment 321510 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=321510&action=review > > > > r- because exposing editor states in HitTestResult is a clear layering > > violation. > > I guess there's two possible layering violations: > > * Do you not want WebCore types used in the WebKit C++ API? If we still > care about this API, then I can add a matching WebKit-level enum class with > functions to translate between the two. > > * Do you not want anything editor-related exposed in HitTestResult? If so, > the copy availability state is already there, and I presume that Safari > probably depends on that. And I think it's actually needed, see below. Well, that code is just wrong. As far as I can, that's a dead code at this point. We should just remove it. > > You should be able to do that by checking EditorState. > > If not, we should be sending EditorState update as we tell UI process to > > show the context menu. > > My original patch, attachment #301330 [details], made that possible to do, > but I got r- from Carlos because: > > (In reply to Carlos Garcia Campos from comment #10) > > The problem of using the editor like your patch does is that it checks if > > the web page can execute the giving command. That's actually the editor > > associated to the main or focused frame. If the right click always focuses, > > then that's not a problem, though. Well, you're still talking to Editor in which the hit test result occurred so you're still at mercy of editing code. EditorState is always reflecting the state of the currently focused frame's Editor object if that's the concern. Also, Editor doesn't make decisions based on whether an element is focused or not. It only looks at the selection, and we always move the selection to the element upon which the user clicked or right clicked. So, using Editor is the right thing to do here. > > The advantage of the hit test result is > > that it always refers to the most specific node where the right click > > happened. What I would do here, is extend hit test result to provide more > > editor capabilities (or state) and return a bitmask with > > CanCopy/CanCut/CanPaste/CanUndo/CanRedo and replace allowsCopy with that. > > So that's why I took this more complicated approach. Do you have an > alternative suggestion? Here's the problem, HitTestResult sits in the layer below editing and other user interaction models. For example, there are CSS features like user-select: none which prohibits certain user actions from being operated on, and editing code is the one that respects these constraints so you should simply talk to EditorState which encapsulates and takes care of all those issues.
OK, I'll investigate EditorState then.
Created attachment 321875 [details] Patch
Hey Ryosuke, could you approve or reject this new approach, please? This adds canCut/Copy/Paste/Delete/Undo/Redo methods to EditorState. They could go into the GTK platform portion of the EditorState, since they're only used on GTK, but they should work fine on other platforms too and I imagine they'll be useful. I also removed canCopy from HitTestResult like you suggested. If you're OK with this, then I'll cook up some tests and then ask Carlos to do the final review.
I'll also need to remember to update the bug title and the changelog entries, since the new patch does not do what the title says it does.
Comment on attachment 321875 [details] Patch Oh yeah, this is much better but you'd have to hide it under IncludePostLayoutDataHint section. Otherwise, some of those function calls can trigger a sync layout and massively regress Speedometer benchmark.
(In reply to Ryosuke Niwa from comment #34) > Oh yeah, this is much better but you'd have to hide it under > IncludePostLayoutDataHint section. Otherwise, some of those function calls > can trigger a sync layout and massively regress Speedometer benchmark. OK, but now this introduces problems when the post layout data isn't available. I'm still trying to understand the code that schedules the editor state updates, but this is causing problems because flushPendingEditorStateUpdate is never called when running TestWebViewEditor. I see that it's only ever called by AcceleratedDrawingArea, CoordinatedLayerTreeHost, DrawingAreaImpl, or (Mac-only) TiledCoreAnimationDrawingArea, which is all graphics code I don't understand. But I wonder if it's because there's not an real window or any actual drawing when running API tests. I see flushPendingEditorStateUpdate is called all the time when running in browser mode. Continuing to investigate. (Note: there's no context menu at all in this test.)
(In reply to Michael Catanzaro from comment #35) > (In reply to Ryosuke Niwa from comment #34) > > Oh yeah, this is much better but you'd have to hide it under > > IncludePostLayoutDataHint section. Otherwise, some of those function calls > > can trigger a sync layout and massively regress Speedometer benchmark. > > OK, but now this introduces problems when the post layout data isn't > available. I'm still trying to understand the code that schedules the editor > state updates, but this is causing problems because > flushPendingEditorStateUpdate is never called when running > TestWebViewEditor. I see that it's only ever called by > AcceleratedDrawingArea, CoordinatedLayerTreeHost, DrawingAreaImpl, or > (Mac-only) TiledCoreAnimationDrawingArea, which is all graphics code I don't > understand. But I wonder if it's because there's not an real window or any > actual drawing when running API tests. I see flushPendingEditorStateUpdate > is called all the time when running in browser mode. Continuing to > investigate. > > (Note: there's no context menu at all in this test.) The idea behind flushing pending EditorState updates is that we can defer computation of the EditorState until the next display flush. On iOS, we ferry this information across to the UI process via RemoteLayerTreeTransaction, and on Mac, we dispatch a pending EditorState update when flushing layers, after layout has happened. So in many places, instead of performing layout to compute editor state, we instead schedule an editor state update by setting a flag. To ensure consistency in API tests on Cocoa platforms w.r.t. editor states, we have testing hooks to wait until the next presentation update, forcing a drawing area flush it necessary, so by the time the UI process learns that the presentation update is finished, we will have computed in the web process and received in the UI process any pending editor state update that has been scheduled.
Thanks for the tips, Wenson. (In reply to Wenson Hsieh from comment #36) > To ensure consistency in API tests on Cocoa platforms w.r.t. editor states, > we have testing hooks to wait until the next presentation update, forcing a > drawing area flush it necessary, so by the time the UI process learns that > the presentation update is finished, we will have computed in the web > process and received in the UI process any pending editor state update that > has been scheduled. I'll start looking for these hooks since I'll probably need something similar for GTK/WPE... if you could point me in the right direction, that'd be appreciated.
I found _doAfterNextPresentationUpdate in WKWebView.mm. Hm....
I think that for GTK/WPE we can't really wait for presentation updates, because our platform API doesn't allow for having private internal functions on the API objects. So we can't add a presentation hook accessible to API tests without exposing it in our public API. Now, we might eventually need to do that anyway (it's probably required for bug #164180), but that's not this bug.... Anyway, we probably don't need to, because the tests already wait for the load finished event. It works if I add a call to sendEditorStateUpdate() at the top of WebPage::didFinishLoad. Pretty sure that's both harmless and very much not correct. Will investigate more tomorrow.
g_signal_connect_swapped(webView, "draw", G_CALLBACK(webViewDrawCallback), mainLoop); gtk_widget_queue_draw(webView); g_main_loop_run(mainLoop); static gboolean webViewDrawCallback(GMainLoop* mainLoop) { g_main_loop_quit(mainLoop); return FALSE; }
Ah, and yes, I think you need the web view to be in a toplevel to ensure it receives draw events.
Yes, that works in combination with packing the web view into a temporary window. Yay!
Created attachment 322148 [details] Patch
Phew... this patch is starting to feel like an odyssey. Ryosuke, do you want to review the changes I made to post layout state? Carlos, I think it's ready for API review.
Comment on attachment 322148 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322148&action=review > Source/WebKit/UIProcess/API/glib/WebKitEditorState.cpp:36 > + * #WebKitEditorState represents the state of a #WebKitWebView editor. This is on purpose, we don't use # for the class in its own section, because it makes a link to this point. > Source/WebKit/UIProcess/API/glib/WebKitEditorState.cpp:41 > + * The #WebKitEditorState may not return accurate results if its > + * associated #WebKitWebView is not visible. This just adds more confusion, we should either explain in detail the thing, or not say anything. You had problems in unit tests, in real cases it's not a problem because you always use the editor state on a web view that is mapped on a toplevel window. > Source/WebKit/UIProcess/API/glib/WebKitEditorState.cpp:58 > + unsigned isCutAvailable : 1; > + unsigned isCopyAvailable : 1; > + unsigned isPasteAvailable : 1; > + unsigned isUndoAvailable : 1; > + unsigned isRedoAvailable : 1; Maybe we can use flags internally, even if they are exposed as separate functions to the API. > Source/WebKit/UIProcess/API/glib/WebKitEditorState.cpp:163 > + * webkit_editor_state_get_is_cut_available: I don't like get_is, I prefer just is > Source/WebKit/UIProcess/API/glib/WebKitEditorState.cpp:172 > +gboolean webkit_editor_state_get_is_cut_available(WebKitEditorState* editorState) So, this is the sync version of webkit_web_view_can_execute_editing_command? We can really deprecate the existing API, because it supports more commands, but for a few of them this duplicated. If we are going to keep both, we should at least explain when to use one or the other. Maybe we should consider cut/copy/paste/undo/redo special cases in webkit_web_view_can_execute_editing_command and use the editor state internally to avoid the IPC and return immediately. > Source/WebKit/UIProcess/API/glib/WebKitEditorState.cpp:189 > +gboolean webkit_editor_state_get_is_copy_available(WebKitEditorState* editorState) And same comments for all other commands. > Tools/TestWebKitAPI/Tests/WebKitGtk/TestWebViewEditor.cpp:48 > + void waitUntilDrawAfterLoadFinished() > + { > + waitUntilLoadFinished(); > + triggerDrawAndWaitUntilFinished(); > + } This is not saving much, I think tests can call both methods directly. > Tools/TestWebKitAPI/Tests/WebKitGtk/TestWebViewEditor.cpp:67 > + if (!strcmp(command, WEBKIT_EDITING_COMMAND_CUT)) > + g_assert(m_canExecuteEditingCommand == webkit_editor_state_get_is_cut_available(editorState())); > + else if (!strcmp(command, WEBKIT_EDITING_COMMAND_COPY)) > + g_assert(m_canExecuteEditingCommand == webkit_editor_state_get_is_copy_available(editorState())); > + else if (!strcmp(command, WEBKIT_EDITING_COMMAND_PASTE)) > + g_assert(m_canExecuteEditingCommand == webkit_editor_state_get_is_paste_available(editorState())); > + // FIXME: Figure out how to add tests for undo and redo. It will probably require using user > + // scripts to message the UI process when the content has been altered. > + else if (!strcmp(command, WEBKIT_EDITING_COMMAND_UNDO)) > + g_assert(m_canExecuteEditingCommand == webkit_editor_state_get_is_undo_available(editorState())); > + else if (!strcmp(command, WEBKIT_EDITING_COMMAND_REDO)) > + g_assert(m_canExecuteEditingCommand == webkit_editor_state_get_is_redo_available(editorState())); This is the proof we are duplicating the functionality in the API. > Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.h:70 > + void waitUntilTextIsAvailableInClipboard(); Does this really belong here? It's only used by editor tests, no? > Tools/TestWebKitAPI/glib/WebKitGLib/gtk/WebViewTestGtk.cpp:97 > + g_main_loop_quit(mainLoop); > + return FALSE; This approach works for editor tests that don't really care about the repaint itself, but you added this to WebViewTest to make it generic. triggerDrawAndWaitUntilFinished() sounds like when done a draw has happened, but we are quitting the loop before it happens, if the test is using this, for example to take a screenshot of the web view, the contents will not be updated. So, I would either leave this in the editor tests as it is, or if you leave it here as generic, schedule an idle here to quit the main loop on the next iterations, after the draw has been done. > Tools/TestWebKitAPI/glib/WebKitGLib/gtk/WebViewTestGtk.cpp:100 > +void WebViewTest::triggerDrawAndWaitUntilFinished() repaintAndWaitUntilFinished? > Tools/TestWebKitAPI/glib/WebKitGLib/gtk/WebViewTestGtk.cpp:106 > + bool shouldDestroyTemporaryWindow = false; > + if (!m_parentWindow) { > + showInWindow(GTK_WINDOW_TOPLEVEL); > + shouldDestroyTemporaryWindow = true; > + } Tests that will use this, should do this explicitly using showInWindowAndWaitUntilMapped at the beginning. > Tools/TestWebKitAPI/glib/WebKitGLib/gtk/WebViewTestGtk.cpp:129 > + g_timeout_add(50, [](gpointer userData) -> gboolean { > + auto* mainLoop = static_cast<GMainLoop*>(userData); > + if (gtk_clipboard_wait_is_text_available(gtk_clipboard_get(GDK_SELECTION_CLIPBOARD))) { > + g_main_loop_quit(mainLoop); > + return G_SOURCE_REMOVE; > + } > + return G_SOURCE_CONTINUE; > + }, m_mainLoop); > + g_main_loop_run(m_mainLoop); This can enter an infinite loop, the triesCount was there for a reason.
(In reply to Carlos Garcia Campos from comment #45) > This is on purpose, we don't use # for the class in its own section, because > it makes a link to this point. OK. We should fix WebKitWebView then, since that's the class I checked first for an example. > > Source/WebKit/UIProcess/API/glib/WebKitEditorState.cpp:41 > > + * The #WebKitEditorState may not return accurate results if its > > + * associated #WebKitWebView is not visible. > > This just adds more confusion, we should either explain in detail the thing, > or not say anything. You had problems in unit tests, in real cases it's not > a problem because you always use the editor state on a web view that is > mapped on a toplevel window. It would be too much information to explain in detail, so I'll omit it. > > Source/WebKit/UIProcess/API/glib/WebKitEditorState.cpp:58 > > + unsigned isCutAvailable : 1; > > + unsigned isCopyAvailable : 1; > > + unsigned isPasteAvailable : 1; > > + unsigned isUndoAvailable : 1; > > + unsigned isRedoAvailable : 1; > > Maybe we can use flags internally, even if they are exposed as separate > functions to the API. We could, but I prefer using bools in EditorState to parallel Editor, and there's not much value in converting to flags in WebKitEditorState where it's easier to just use the bitfields. > > Source/WebKit/UIProcess/API/glib/WebKitEditorState.cpp:163 > > + * webkit_editor_state_get_is_cut_available: > > I don't like get_is, I prefer just is OK. I had asked you about this earlier. :P I settled on _get_is because that seems to be GLib style. But I don't really care; I'll change it to _is. > > Source/WebKit/UIProcess/API/glib/WebKitEditorState.cpp:172 > > +gboolean webkit_editor_state_get_is_cut_available(WebKitEditorState* editorState) > > So, this is the sync version of webkit_web_view_can_execute_editing_command? > We can really deprecate the existing API, because it supports more commands, > but for a few of them this duplicated. If we are going to keep both, we > should at least explain when to use one or the other. Maybe we should > consider cut/copy/paste/undo/redo special cases in > webkit_web_view_can_execute_editing_command and use the editor state > internally to avoid the IPC and return immediately. It seems reasonable to use WebKitEditorState to implement webkit_web_view_can_execute_editing_command for those special cases. I'll do that. I don't think we need to explain when to use one or the other, because they should both always return the same thing... it's fine to have two different ways of doing things. The advantage of can_execute_editing_command is obvious (it can handle dozens of commands) and the advantage of WebKitEditorState is also obvious (it returns immediately). > > > Tools/TestWebKitAPI/Tests/WebKitGtk/TestWebViewEditor.cpp:48 > > + void waitUntilDrawAfterLoadFinished() > > + { > > + waitUntilLoadFinished(); > > + triggerDrawAndWaitUntilFinished(); > > + } > > This is not saving much, I think tests can call both methods directly. As you prefer. > > Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.h:70 > > + void waitUntilTextIsAvailableInClipboard(); > > Does this really belong here? It's only used by editor tests, no? Hm, it can go where you prefer. I figured it could be useful for other tests (as it was for TestContextMenu) in the future, but for now it's only used by TestWebViewEditor, indeed. You suggested moving it to WebViewTest in comment #21, but that was when I was using it in TestContextMenu as well. > > Tools/TestWebKitAPI/glib/WebKitGLib/gtk/WebViewTestGtk.cpp:97 > > + g_main_loop_quit(mainLoop); > > + return FALSE; > > This approach works for editor tests that don't really care about the > repaint itself, but you added this to WebViewTest to make it generic. > triggerDrawAndWaitUntilFinished() sounds like when done a draw has happened, > but we are quitting the loop before it happens, if the test is using this, > for example to take a screenshot of the web view, the contents will not be > updated. So, I would either leave this in the editor tests as it is, or if > you leave it here as generic, schedule an idle here to quit the main loop on > the next iterations, after the draw has been done. Good point. I'd prefer to use an idle. > > Tools/TestWebKitAPI/glib/WebKitGLib/gtk/WebViewTestGtk.cpp:100 > > +void WebViewTest::triggerDrawAndWaitUntilFinished() > > repaintAndWaitUntilFinished? I like keeping the word "trigger" since otherwise it's not really clear from the name of the function what there is to wait for. Also, it's not necessarily a repaint as it could be the first time the web view is painted! > > Tools/TestWebKitAPI/glib/WebKitGLib/gtk/WebViewTestGtk.cpp:106 > > + bool shouldDestroyTemporaryWindow = false; > > + if (!m_parentWindow) { > > + showInWindow(GTK_WINDOW_TOPLEVEL); > > + shouldDestroyTemporaryWindow = true; > > + } > > Tests that will use this, should do this explicitly using > showInWindowAndWaitUntilMapped at the beginning. Probably. I'll investigate this. > > Tools/TestWebKitAPI/glib/WebKitGLib/gtk/WebViewTestGtk.cpp:129 > > + g_timeout_add(50, [](gpointer userData) -> gboolean { > > + auto* mainLoop = static_cast<GMainLoop*>(userData); > > + if (gtk_clipboard_wait_is_text_available(gtk_clipboard_get(GDK_SELECTION_CLIPBOARD))) { > > + g_main_loop_quit(mainLoop); > > + return G_SOURCE_REMOVE; > > + } > > + return G_SOURCE_CONTINUE; > > + }, m_mainLoop); > > + g_main_loop_run(m_mainLoop); > > This can enter an infinite loop, the triesCount was there for a reason. But that's fine. If it infinite loops, then the test is broken, and it will time out. I was more worried that 150ms might not be enough on a resource-constrained device, or even on a fast computer if it's overloaded with other tasks. That's unlikely, of course, but it could lead to spurious failures.
(In reply to Michael Catanzaro from comment #46) > > > Tools/TestWebKitAPI/glib/WebKitGLib/gtk/WebViewTestGtk.cpp:106 > > > + bool shouldDestroyTemporaryWindow = false; > > > + if (!m_parentWindow) { > > > + showInWindow(GTK_WINDOW_TOPLEVEL); > > > + shouldDestroyTemporaryWindow = true; > > > + } > > > > Tests that will use this, should do this explicitly using > > showInWindowAndWaitUntilMapped at the beginning. > > Probably. I'll investigate this. Unfortunately it doesn't work in this case. I suppose that probably means the API is broken and shouldn't be landed. It will need to wait until next week, because it will require more serious debugging than I want to do now.
I'm uploading my latest version (broken) just to have a backup here.
Created attachment 322374 [details] Patch
Attachment 322374 [details] did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/wpe/WebKitEditorState.h" WARNING: File exempt from style guide. Skipping: "Tools/TestWebKitAPI/Tests/WebKitGtk/TestWebViewEditor.cpp" ERROR: Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3072: One line control clauses should not use braces. [whitespace/braces] [4] WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitEditorState.h" Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
> This approach works for editor tests that don't really care about the > repaint itself, but you added this to WebViewTest to make it generic. > triggerDrawAndWaitUntilFinished() sounds like when done a draw has happened, > but we are quitting the loop before it happens, if the test is using this, > for example to take a screenshot of the web view, the contents will not be > updated. So, I would either leave this in the editor tests as it is, or if > you leave it here as generic, schedule an idle here to quit the main loop on > the next iterations, after the draw has been done. I wound up moving it back to EditorTest, since I've spent too much time trying to make your suggestion work. > > Tools/TestWebKitAPI/glib/WebKitGLib/gtk/WebViewTestGtk.cpp:106 > > + bool shouldDestroyTemporaryWindow = false; > > + if (!m_parentWindow) { > > + showInWindow(GTK_WINDOW_TOPLEVEL); > > + shouldDestroyTemporaryWindow = true; > > + } > > Tests that will use this, should do this explicitly using > showInWindowAndWaitUntilMapped at the beginning. I agree, but I've spent way too long trying to make it work, and this is such a minor point to block this serious bug. I'll upload a new patch with a FIXME here; will you accept that?
Created attachment 325033 [details] Patch
Committed r224179: <https://trac.webkit.org/changeset/224179>
(In reply to Michael Catanzaro from comment #51) > I agree, but I've spent way too long trying to make it work, and this is > such a minor point to block this serious bug. I'll upload a new patch with a > FIXME here; will you accept that? (He said yes. :)
This changed introduced additional sync IPC from the web process to the UI process inside WebPage::editorState(), which is showing up in profiles: 75 WebCore::LayerFlushScheduler::layerFlushCallback() (in WebCore) + 28 [0x10cceddbc] 74 WebKit::TiledCoreAnimationDrawingArea::flushLayers() (in WebKit) + 54 [0x10b0a75da] ! 72 WebKit::WebPage::sendEditorStateUpdate() (in WebKit) + 79 [0x10b2f965f] ! : 47 WebKit::WebPage::editorState(WebKit::WebPage::IncludePostLayoutDataHint) const (in WebKit) + 375 [0x10b2f36c1] ! : | 47 WebKit::WebEditorClient::canUndo() const (in WebKit) + 103 [0x10b0f1dd7] ! : | 44 bool IPC::Connection::sendSync<Messages::WebPageProxy::CanUndoRedo>(Messages::WebPageProxy::CanUndoRedo&&, Messages::WebPageProxy::CanUndoRedo::Reply&&, unsigned long long, WTF::Seconds, WTF::OptionSet<IPC::SendSyncOption>) (in WebKit) + 170 [0x10b2bcaf8] ! : | + 44 IPC::Connection::sendSyncMessage(unsigned long long, std::__1::unique_ptr<IPC::Encoder, std::__1::default_delete<IPC::Encoder> >, WTF::Seconds, WTF::OptionSet<IPC::SendSyncOption>) (in WebKit) + 292 [0x10b13b4c8] ! : | + 44 IPC::Connection::waitForSyncReply(unsigned long long, WTF::Seconds, WTF::OptionSet<IPC::SendSyncOption>) (in WebKit) + 162 [0x10b13bc2a] ! : | + 44 WTF::BinarySemaphore::wait(WTF::TimeWithDynamicClockType) (in JavaScriptCore) + 73 [0x10f25e139] ! : | + 44 WTF::ThreadCondition::timedWait(WTF::Mutex&, double) (in JavaScriptCore) + 63 [0x10e6fd6ff] ! : | + 44 _pthread_cond_wait (in libsystem_pthread.dylib) + 732 [0x7fff7b282662] This is a really bad time to do sync IPC, because we're in layerFlushCallback() which is blocking painting. Also, this stuff: postLayoutData.canCut = editor.canCut(); postLayoutData.canCopy = editor.canCopy(); postLayoutData.canPaste = editor.canPaste(); postLayoutData.canUndo = editor.canUndo(); postLayoutData.canRedo = editor.canRedo(); is stupid to do in the web process because what's the point of doing sync IPC to the UI process to get data that you just turn around and send back to the UI process? I'd like to roll back this change.
> postLayoutData.canCut = editor.canCut(); > postLayoutData.canCopy = editor.canCopy(); > postLayoutData.canPaste = editor.canPaste(); > postLayoutData.canUndo = editor.canUndo(); > postLayoutData.canRedo = editor.canRedo(); This also triggers IPCs from the web process to the UI process and back 5 times...I wonder if we can batch these together into a single struct? (or alternately, just a single unsigned value, and have these as flags).
(In reply to Simon Fraser (smfr) from comment #55) > I'd like to roll back this change. Investigating. Will roll it out if I don't see an easy fix. (In reply to Simon Fraser (smfr) from comment #55) > is stupid to do in the web process because what's the point of doing sync > IPC to the UI process to get data that you just turn around and send back to > the UI process? Yes, that's definitely not how this is supposed to work.
So, what went wrong here is I did not sufficiently investigate the implementations of Editor::canUndo and Editor::canRedo. I did not realize those operations require sync IPC to the UI process. So these definitely need to be removed from the editor state flush; they should be computed in the UI process instead. canCut/canCopy/canPaste are all decided by the Editor in the web process without resorting to IPC to the WebEditorClient. So these need to be in the editor state flush for GTK/WPE, where this is needed to implement public API. (It's not being used by other ports and could be #ifdefed out, but as long as these three are not impacting performance, it seems better to avoid unneeded #ifdefs.) (In reply to Wenson Hsieh from comment #56) > > postLayoutData.canCut = editor.canCut(); > > postLayoutData.canCopy = editor.canCopy(); > > postLayoutData.canPaste = editor.canPaste(); > > postLayoutData.canUndo = editor.canUndo(); > > postLayoutData.canRedo = editor.canRedo(); > > This also triggers IPCs from the web process to the UI process and back 5 > times...I wonder if we can batch these together into a single struct? (or > alternately, just a single unsigned value, and have these as flags). I think it's only twice, for canUndo and canRedo. Correct? Still twice too many. ;)
Simon, Wenson, I'm working on a patch to try to eliminate the canUndo/canRedo, while leaving the other three calls. I assume canCut/canCopy/canPaste are not going to show up in profiles (if so, then this needs a rethink). Is that OK?
I think so. Reported bug #179797.
(In reply to Michael Catanzaro from comment #58) > So, what went wrong here is I did not sufficiently investigate the > implementations of Editor::canUndo and Editor::canRedo. I did not realize > those operations require sync IPC to the UI process. So these definitely > need to be removed from the editor state flush; they should be computed in > the UI process instead. > > canCut/canCopy/canPaste are all decided by the Editor in the web process > without resorting to IPC to the WebEditorClient. So these need to be in the > editor state flush for GTK/WPE, where this is needed to implement public > API. (It's not being used by other ports and could be #ifdefed out, but as > long as these three are not impacting performance, it seems better to avoid > unneeded #ifdefs.) > > (In reply to Wenson Hsieh from comment #56) > > > postLayoutData.canCut = editor.canCut(); > > > postLayoutData.canCopy = editor.canCopy(); > > > postLayoutData.canPaste = editor.canPaste(); > > > postLayoutData.canUndo = editor.canUndo(); > > > postLayoutData.canRedo = editor.canRedo(); > > > > This also triggers IPCs from the web process to the UI process and back 5 > > times...I wonder if we can batch these together into a single struct? (or > > alternately, just a single unsigned value, and have these as flags). > > I think it's only twice, for canUndo and canRedo. Correct? Still twice too > many. ;) Ah, that's correct — the calls for cut, copy and paste don't IPC out to the UI process.
(In reply to Michael Catanzaro from comment #60) > I think so. Reported bug #179797. This patch was never rolled out because I fixed the issue in bug #179797 instead. Closing again.