From 3572a047d13c37ba199a65419c34e4e9754c3507 Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Sat, 30 Dec 2023 10:08:33 -0500 Subject: [PATCH] LibWebView+WebContent: Make the DOM node editing IPCs async All DOM node mutation IPCs now invoke an async completion IPC after the DOM is mutated. This allows consolidating where the Inspector updates its view and the selected DOM node. This also allows improving the response to removing a DOM node. We would previously just select the tag after removing a DOM node because the Inspector client had no idea what node preceded the removed node. Now the WebContent process can just indicate what that node is. So now after removing a DOM node, we inspect either its previous sibling (if it had one) or its parent. --- .../Libraries/LibWebView/InspectorClient.cpp | 39 +++------- .../LibWebView/ViewImplementation.cpp | 16 ++--- .../Libraries/LibWebView/ViewImplementation.h | 9 +-- .../Libraries/LibWebView/WebContentClient.cpp | 6 ++ .../Libraries/LibWebView/WebContentClient.h | 1 + .../WebContent/ConnectionFromClient.cpp | 72 +++++++++++++------ .../WebContent/ConnectionFromClient.h | 8 +-- .../Services/WebContent/WebContentClient.ipc | 1 + .../Services/WebContent/WebContentServer.ipc | 8 +-- 9 files changed, 91 insertions(+), 69 deletions(-) diff --git a/Userland/Libraries/LibWebView/InspectorClient.cpp b/Userland/Libraries/LibWebView/InspectorClient.cpp index 0d2d95095a..1c77856666 100644 --- a/Userland/Libraries/LibWebView/InspectorClient.cpp +++ b/Userland/Libraries/LibWebView/InspectorClient.cpp @@ -93,6 +93,13 @@ InspectorClient::InspectorClient(ViewImplementation& content_web_view, ViewImple select_node(node_id); }; + m_content_web_view.on_finshed_editing_dom_node = [this](auto const& node_id) { + m_pending_selection = node_id; + m_dom_tree_loaded = false; + + inspect(); + }; + m_content_web_view.on_received_console_message = [this](auto message_index) { handle_console_message(message_index); }; @@ -136,28 +143,18 @@ InspectorClient::InspectorClient(ViewImplementation& content_web_view, ViewImple m_inspector_web_view.on_inspector_set_dom_node_text = [this](auto node_id, auto const& text) { m_content_web_view.set_dom_node_text(node_id, text); - - m_pending_selection = node_id; - inspect(); }; m_inspector_web_view.on_inspector_set_dom_node_tag = [this](auto node_id, auto const& tag) { - m_pending_selection = m_content_web_view.set_dom_node_tag(node_id, tag); - inspect(); + m_content_web_view.set_dom_node_tag(node_id, tag); }; m_inspector_web_view.on_inspector_added_dom_node_attributes = [this](auto node_id, auto const& attributes) { m_content_web_view.add_dom_node_attributes(node_id, attributes); - - m_pending_selection = node_id; - inspect(); }; m_inspector_web_view.on_inspector_replaced_dom_node_attribute = [this](auto node_id, auto const& name, auto const& replacement_attributes) { m_content_web_view.replace_dom_node_attribute(node_id, name, replacement_attributes); - - m_pending_selection = node_id; - inspect(); }; m_inspector_web_view.on_inspector_executed_console_script = [this](auto const& script) { @@ -268,9 +265,7 @@ void InspectorClient::context_menu_create_child_element() { VERIFY(m_context_menu_data.has_value()); - m_pending_selection = m_content_web_view.create_child_element(m_context_menu_data->dom_node_id); - inspect(); - + m_content_web_view.create_child_element(m_context_menu_data->dom_node_id); m_context_menu_data.clear(); } @@ -278,9 +273,7 @@ void InspectorClient::context_menu_create_child_text_node() { VERIFY(m_context_menu_data.has_value()); - m_pending_selection = m_content_web_view.create_child_text_node(m_context_menu_data->dom_node_id); - inspect(); - + m_content_web_view.create_child_text_node(m_context_menu_data->dom_node_id); m_context_menu_data.clear(); } @@ -288,9 +281,7 @@ void InspectorClient::context_menu_clone_dom_node() { VERIFY(m_context_menu_data.has_value()); - m_pending_selection = m_content_web_view.clone_dom_node(m_context_menu_data->dom_node_id); - inspect(); - + m_content_web_view.clone_dom_node(m_context_menu_data->dom_node_id); m_context_menu_data.clear(); } @@ -299,10 +290,6 @@ void InspectorClient::context_menu_remove_dom_node() VERIFY(m_context_menu_data.has_value()); m_content_web_view.remove_dom_node(m_context_menu_data->dom_node_id); - - m_pending_selection = m_body_node_id; - inspect(); - m_context_menu_data.clear(); } @@ -322,10 +309,6 @@ void InspectorClient::context_menu_remove_dom_node_attribute() VERIFY(m_context_menu_data->attribute.has_value()); m_content_web_view.replace_dom_node_attribute(m_context_menu_data->dom_node_id, m_context_menu_data->attribute->name, {}); - - m_pending_selection = m_context_menu_data->dom_node_id; - inspect(); - m_context_menu_data.clear(); } diff --git a/Userland/Libraries/LibWebView/ViewImplementation.cpp b/Userland/Libraries/LibWebView/ViewImplementation.cpp index 8039f14e1d..6cff070d57 100644 --- a/Userland/Libraries/LibWebView/ViewImplementation.cpp +++ b/Userland/Libraries/LibWebView/ViewImplementation.cpp @@ -156,9 +156,9 @@ void ViewImplementation::set_dom_node_text(i32 node_id, String text) client().async_set_dom_node_text(node_id, move(text)); } -Optional ViewImplementation::set_dom_node_tag(i32 node_id, String name) +void ViewImplementation::set_dom_node_tag(i32 node_id, String name) { - return client().set_dom_node_tag(node_id, move(name)); + client().async_set_dom_node_tag(node_id, move(name)); } void ViewImplementation::add_dom_node_attributes(i32 node_id, Vector attributes) @@ -171,19 +171,19 @@ void ViewImplementation::replace_dom_node_attribute(i32 node_id, String name, Ve client().async_replace_dom_node_attribute(node_id, move(name), move(replacement_attributes)); } -Optional ViewImplementation::create_child_element(i32 node_id) +void ViewImplementation::create_child_element(i32 node_id) { - return client().create_child_element(node_id); + client().async_create_child_element(node_id); } -Optional ViewImplementation::create_child_text_node(i32 node_id) +void ViewImplementation::create_child_text_node(i32 node_id) { - return client().create_child_text_node(node_id); + client().async_create_child_text_node(node_id); } -Optional ViewImplementation::clone_dom_node(i32 node_id) +void ViewImplementation::clone_dom_node(i32 node_id) { - return client().clone_dom_node(node_id); + client().async_clone_dom_node(node_id); } void ViewImplementation::remove_dom_node(i32 node_id) diff --git a/Userland/Libraries/LibWebView/ViewImplementation.h b/Userland/Libraries/LibWebView/ViewImplementation.h index 754f222eb7..1925575e57 100644 --- a/Userland/Libraries/LibWebView/ViewImplementation.h +++ b/Userland/Libraries/LibWebView/ViewImplementation.h @@ -65,12 +65,12 @@ public: void get_hovered_node_id(); void set_dom_node_text(i32 node_id, String text); - Optional set_dom_node_tag(i32 node_id, String name); + void set_dom_node_tag(i32 node_id, String name); void add_dom_node_attributes(i32 node_id, Vector attributes); void replace_dom_node_attribute(i32 node_id, String name, Vector replacement_attributes); - Optional create_child_element(i32 node_id); - Optional create_child_text_node(i32 node_id); - Optional clone_dom_node(i32 node_id); + void create_child_element(i32 node_id); + void create_child_text_node(i32 node_id); + void clone_dom_node(i32 node_id); void remove_dom_node(i32 node_id); Optional get_dom_node_html(i32 node_id); @@ -144,6 +144,7 @@ public: Function)> on_received_dom_node_properties; Function on_received_accessibility_tree; Function on_received_hovered_node_id; + Function const& node_id)> on_finshed_editing_dom_node; Function on_received_console_message; Function const& message_types, Vector const& messages)> on_received_console_messages; Function(AK::URL const& url)> on_get_all_cookies; diff --git a/Userland/Libraries/LibWebView/WebContentClient.cpp b/Userland/Libraries/LibWebView/WebContentClient.cpp index fe6ac53dd2..b590dc40a1 100644 --- a/Userland/Libraries/LibWebView/WebContentClient.cpp +++ b/Userland/Libraries/LibWebView/WebContentClient.cpp @@ -219,6 +219,12 @@ void WebContentClient::did_get_hovered_node_id(i32 node_id) m_view.on_received_hovered_node_id(node_id); } +void WebContentClient::did_finish_editing_dom_node(Optional const& node_id) +{ + if (m_view.on_finshed_editing_dom_node) + m_view.on_finshed_editing_dom_node(node_id); +} + void WebContentClient::did_output_js_console_message(i32 message_index) { if (m_view.on_received_console_message) diff --git a/Userland/Libraries/LibWebView/WebContentClient.h b/Userland/Libraries/LibWebView/WebContentClient.h index e2f3e8f4b9..60bf0f1be8 100644 --- a/Userland/Libraries/LibWebView/WebContentClient.h +++ b/Userland/Libraries/LibWebView/WebContentClient.h @@ -56,6 +56,7 @@ private: virtual void did_inspect_dom_node(bool has_style, ByteString const& computed_style, ByteString const& resolved_style, ByteString const& custom_properties, ByteString const& node_box_sizing, ByteString const& aria_properties_state) override; virtual void did_inspect_accessibility_tree(ByteString const&) override; virtual void did_get_hovered_node_id(i32 node_id) override; + virtual void did_finish_editing_dom_node(Optional const& node_id) override; virtual void did_output_js_console_message(i32 message_index) override; virtual void did_get_js_console_messages(i32 start_index, Vector const& message_types, Vector const& messages) override; virtual void did_change_favicon(Gfx::ShareableBitmap const&) override; diff --git a/Userland/Services/WebContent/ConnectionFromClient.cpp b/Userland/Services/WebContent/ConnectionFromClient.cpp index ab66c3cf77..3e00c4aff2 100644 --- a/Userland/Services/WebContent/ConnectionFromClient.cpp +++ b/Userland/Services/WebContent/ConnectionFromClient.cpp @@ -636,18 +636,24 @@ void ConnectionFromClient::get_hovered_node_id() void ConnectionFromClient::set_dom_node_text(i32 node_id, String const& text) { auto* dom_node = Web::DOM::Node::from_unique_id(node_id); - if (!dom_node || (!dom_node->is_text() && !dom_node->is_comment())) + if (!dom_node || (!dom_node->is_text() && !dom_node->is_comment())) { + async_did_finish_editing_dom_node({}); return; + } auto& character_data = static_cast(*dom_node); character_data.set_data(text); + + async_did_finish_editing_dom_node(character_data.unique_id()); } -Messages::WebContentServer::SetDomNodeTagResponse ConnectionFromClient::set_dom_node_tag(i32 node_id, String const& name) +void ConnectionFromClient::set_dom_node_tag(i32 node_id, String const& name) { auto* dom_node = Web::DOM::Node::from_unique_id(node_id); - if (!dom_node || !dom_node->is_element() || !dom_node->parent()) - return OptionalNone {}; + if (!dom_node || !dom_node->is_element() || !dom_node->parent()) { + async_did_finish_editing_dom_node({}); + return; + } auto& element = static_cast(*dom_node); auto new_element = Web::DOM::create_element(element.document(), name, element.namespace_uri(), element.prefix(), element.is_value()).release_value_but_fixme_should_propagate_errors(); @@ -662,26 +668,32 @@ Messages::WebContentServer::SetDomNodeTagResponse ConnectionFromClient::set_dom_ } element.parent()->replace_child(*new_element, element).release_value_but_fixme_should_propagate_errors(); - return new_element->unique_id(); + async_did_finish_editing_dom_node(new_element->unique_id()); } void ConnectionFromClient::add_dom_node_attributes(i32 node_id, Vector const& attributes) { auto* dom_node = Web::DOM::Node::from_unique_id(node_id); - if (!dom_node || !dom_node->is_element()) + if (!dom_node || !dom_node->is_element()) { + async_did_finish_editing_dom_node({}); return; + } auto& element = static_cast(*dom_node); for (auto const& attribute : attributes) element.set_attribute(attribute.name, attribute.value).release_value_but_fixme_should_propagate_errors(); + + async_did_finish_editing_dom_node(element.unique_id()); } void ConnectionFromClient::replace_dom_node_attribute(i32 node_id, String const& name, Vector const& replacement_attributes) { auto* dom_node = Web::DOM::Node::from_unique_id(node_id); - if (!dom_node || !dom_node->is_element()) + if (!dom_node || !dom_node->is_element()) { + async_did_finish_editing_dom_node({}); return; + } auto& element = static_cast(*dom_node); bool should_remove_attribute = true; @@ -695,53 +707,69 @@ void ConnectionFromClient::replace_dom_node_attribute(i32 node_id, String const& if (should_remove_attribute) element.remove_attribute(name); + + async_did_finish_editing_dom_node(element.unique_id()); } -Messages::WebContentServer::CreateChildElementResponse ConnectionFromClient::create_child_element(i32 node_id) +void ConnectionFromClient::create_child_element(i32 node_id) { auto* dom_node = Web::DOM::Node::from_unique_id(node_id); - if (!dom_node) - return OptionalNone {}; + if (!dom_node) { + async_did_finish_editing_dom_node({}); + return; + } auto element = Web::DOM::create_element(dom_node->document(), Web::HTML::TagNames::div, Web::Namespace::HTML).release_value_but_fixme_should_propagate_errors(); dom_node->append_child(element).release_value_but_fixme_should_propagate_errors(); - return element->unique_id(); + async_did_finish_editing_dom_node(element->unique_id()); } -Messages::WebContentServer::CreateChildTextNodeResponse ConnectionFromClient::create_child_text_node(i32 node_id) +void ConnectionFromClient::create_child_text_node(i32 node_id) { auto* dom_node = Web::DOM::Node::from_unique_id(node_id); - if (!dom_node) - return OptionalNone {}; + if (!dom_node) { + async_did_finish_editing_dom_node({}); + return; + } auto text_node = dom_node->heap().allocate(dom_node->realm(), dom_node->document(), "text"_string); dom_node->append_child(text_node).release_value_but_fixme_should_propagate_errors(); - return text_node->unique_id(); + async_did_finish_editing_dom_node(text_node->unique_id()); } -Messages::WebContentServer::CloneDomNodeResponse ConnectionFromClient::clone_dom_node(i32 node_id) +void ConnectionFromClient::clone_dom_node(i32 node_id) { auto* dom_node = Web::DOM::Node::from_unique_id(node_id); - if (!dom_node || !dom_node->parent_node()) - return OptionalNone {}; + if (!dom_node || !dom_node->parent_node()) { + async_did_finish_editing_dom_node({}); + return; + } auto dom_node_clone = dom_node->clone_node(nullptr, true); dom_node->parent_node()->insert_before(dom_node_clone, dom_node->next_sibling()); - return dom_node_clone->unique_id(); + async_did_finish_editing_dom_node(dom_node_clone->unique_id()); } void ConnectionFromClient::remove_dom_node(i32 node_id) { auto* active_document = page().page().top_level_browsing_context().active_document(); - if (!active_document) + if (!active_document) { + async_did_finish_editing_dom_node({}); return; + } auto* dom_node = Web::DOM::Node::from_unique_id(node_id); - if (!dom_node) + if (!dom_node) { + async_did_finish_editing_dom_node({}); return; + } + + auto* previous_dom_node = dom_node->previous_sibling(); + if (!previous_dom_node) + previous_dom_node = dom_node->parent(); dom_node->remove(); @@ -749,6 +777,8 @@ void ConnectionFromClient::remove_dom_node(i32 node_id) // remain in the layout tree. This has to be fixed, this just causes everything to be recomputed // which really hurts performance. active_document->force_layout(); + + async_did_finish_editing_dom_node(previous_dom_node->unique_id()); } Messages::WebContentServer::GetDomNodeHtmlResponse ConnectionFromClient::get_dom_node_html(i32 node_id) diff --git a/Userland/Services/WebContent/ConnectionFromClient.h b/Userland/Services/WebContent/ConnectionFromClient.h index f946da0fe4..a16d375cd0 100644 --- a/Userland/Services/WebContent/ConnectionFromClient.h +++ b/Userland/Services/WebContent/ConnectionFromClient.h @@ -77,12 +77,12 @@ private: virtual void get_hovered_node_id() override; virtual void set_dom_node_text(i32 node_id, String const& text) override; - virtual Messages::WebContentServer::SetDomNodeTagResponse set_dom_node_tag(i32 node_id, String const& name) override; + virtual void set_dom_node_tag(i32 node_id, String const& name) override; virtual void add_dom_node_attributes(i32 node_id, Vector const& attributes) override; virtual void replace_dom_node_attribute(i32 node_id, String const& name, Vector const& replacement_attributes) override; - virtual Messages::WebContentServer::CreateChildElementResponse create_child_element(i32 node_id) override; - virtual Messages::WebContentServer::CreateChildTextNodeResponse create_child_text_node(i32 node_id) override; - virtual Messages::WebContentServer::CloneDomNodeResponse clone_dom_node(i32 node_id) override; + virtual void create_child_element(i32 node_id) override; + virtual void create_child_text_node(i32 node_id) override; + virtual void clone_dom_node(i32 node_id) override; virtual void remove_dom_node(i32 node_id) override; virtual Messages::WebContentServer::GetDomNodeHtmlResponse get_dom_node_html(i32 node_id) override; diff --git a/Userland/Services/WebContent/WebContentClient.ipc b/Userland/Services/WebContent/WebContentClient.ipc index 5f2d29066f..a78c8bf338 100644 --- a/Userland/Services/WebContent/WebContentClient.ipc +++ b/Userland/Services/WebContent/WebContentClient.ipc @@ -45,6 +45,7 @@ endpoint WebContentClient did_inspect_dom_node(bool has_style, ByteString computed_style, ByteString resolved_style, ByteString custom_properties, ByteString node_box_sizing, ByteString aria_properties_state) =| did_inspect_accessibility_tree(ByteString accessibility_tree) =| did_get_hovered_node_id(i32 node_id) =| + did_finish_editing_dom_node(Optional node_id) =| did_change_favicon(Gfx::ShareableBitmap favicon) =| did_request_all_cookies(URL url) => (Vector cookies) diff --git a/Userland/Services/WebContent/WebContentServer.ipc b/Userland/Services/WebContent/WebContentServer.ipc index 751b00915c..045f4448de 100644 --- a/Userland/Services/WebContent/WebContentServer.ipc +++ b/Userland/Services/WebContent/WebContentServer.ipc @@ -45,12 +45,12 @@ endpoint WebContentServer js_console_request_messages(i32 start_index) =| set_dom_node_text(i32 node_id, String text) =| - set_dom_node_tag(i32 node_id, String name) => (Optional node_id) + set_dom_node_tag(i32 node_id, String name) =| add_dom_node_attributes(i32 node_id, Vector attributes) =| replace_dom_node_attribute(i32 node_id, String name, Vector replacement_attributes) =| - create_child_element(i32 node_id) => (Optional node_id) - create_child_text_node(i32 node_id) => (Optional node_id) - clone_dom_node(i32 node_id) => (Optional node_id) + create_child_element(i32 node_id) =| + create_child_text_node(i32 node_id) =| + clone_dom_node(i32 node_id) =| remove_dom_node(i32 node_id) =| get_dom_node_html(i32 node_id) => (Optional html)