From d8fa226a8f24dc4c668c2dbfa04b1ee761232477 Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Sun, 31 Dec 2023 09:51:02 -0500 Subject: [PATCH] Ladybird+LibWebView+WebContent: Make the screenshot IPCs async These IPCs are different than other IPCs in that we can't just set up a callback function to be invoked when WebContent sends us the screenshot data. There are multiple places that would set that callback, and they would step on each other's toes. Instead, the screenshot APIs on ViewImplementation now return a Promise which callers can interact with to receive the screenshot (or an error). --- Ladybird/AppKit/UI/LadybirdWebView.mm | 10 ++-- Ladybird/Qt/Tab.cpp | 16 +++--- Userland/Applications/Browser/Tab.cpp | 16 +++--- .../Libraries/LibWebView/InspectorClient.cpp | 11 ++-- .../LibWebView/ViewImplementation.cpp | 54 ++++++++++++++++--- .../Libraries/LibWebView/ViewImplementation.h | 8 ++- .../Libraries/LibWebView/WebContentClient.cpp | 5 ++ .../Libraries/LibWebView/WebContentClient.h | 1 + .../WebContent/ConnectionFromClient.cpp | 20 ++++--- .../WebContent/ConnectionFromClient.h | 4 +- .../Services/WebContent/WebContentClient.ipc | 2 + .../Services/WebContent/WebContentServer.ipc | 4 +- Userland/Utilities/headless-browser.cpp | 17 +++++- 13 files changed, 121 insertions(+), 47 deletions(-) diff --git a/Ladybird/AppKit/UI/LadybirdWebView.mm b/Ladybird/AppKit/UI/LadybirdWebView.mm index 3ada6c05fb..ca5073c50d 100644 --- a/Ladybird/AppKit/UI/LadybirdWebView.mm +++ b/Ladybird/AppKit/UI/LadybirdWebView.mm @@ -791,14 +791,16 @@ static void copy_data_to_clipboard(StringView data, NSPasteboardType pasteboard_ - (void)takeVisibleScreenshot:(id)sender { - auto result = m_web_view_bridge->take_screenshot(WebView::ViewImplementation::ScreenshotType::Visible); - (void)result; // FIXME: Display an error if this failed. + m_web_view_bridge->take_screenshot(WebView::ViewImplementation::ScreenshotType::Visible)->when_rejected([](auto const& error) { + (void)error; // FIXME: Display the error. + }); } - (void)takeFullScreenshot:(id)sender { - auto result = m_web_view_bridge->take_screenshot(WebView::ViewImplementation::ScreenshotType::Full); - (void)result; // FIXME: Display an error if this failed. + m_web_view_bridge->take_screenshot(WebView::ViewImplementation::ScreenshotType::Full)->when_rejected([](auto const& error) { + (void)error; // FIXME: Display the error. + }); } - (void)openLink:(id)sender diff --git a/Ladybird/Qt/Tab.cpp b/Ladybird/Qt/Tab.cpp index bc3f7c8535..470275e823 100644 --- a/Ladybird/Qt/Tab.cpp +++ b/Ladybird/Qt/Tab.cpp @@ -310,19 +310,19 @@ Tab::Tab(BrowserWindow* window, WebContentOptions const& web_content_options, St auto* take_visible_screenshot_action = new QAction("Take &Visible Screenshot", this); take_visible_screenshot_action->setIcon(load_icon_from_uri("resource://icons/16x16/filetype-image.png"sv)); QObject::connect(take_visible_screenshot_action, &QAction::triggered, this, [this]() { - if (auto result = view().take_screenshot(WebView::ViewImplementation::ScreenshotType::Visible); result.is_error()) { - auto error = MUST(String::formatted("{}", result.error())); - QMessageBox::warning(this, "Ladybird", qstring_from_ak_string(error)); - } + view().take_screenshot(WebView::ViewImplementation::ScreenshotType::Visible)->when_rejected([this](auto const& error) { + auto error_message = MUST(String::formatted("{}", error)); + QMessageBox::warning(this, "Ladybird", qstring_from_ak_string(error_message)); + }); }); auto* take_full_screenshot_action = new QAction("Take &Full Screenshot", this); take_full_screenshot_action->setIcon(load_icon_from_uri("resource://icons/16x16/filetype-image.png"sv)); QObject::connect(take_full_screenshot_action, &QAction::triggered, this, [this]() { - if (auto result = view().take_screenshot(WebView::ViewImplementation::ScreenshotType::Full); result.is_error()) { - auto error = MUST(String::formatted("{}", result.error())); - QMessageBox::warning(this, "Ladybird", qstring_from_ak_string(error)); - } + view().take_screenshot(WebView::ViewImplementation::ScreenshotType::Full)->when_rejected([this](auto const& error) { + auto error_message = MUST(String::formatted("{}", error)); + QMessageBox::warning(this, "Ladybird", qstring_from_ak_string(error_message)); + }); }); m_page_context_menu = new QMenu("Context menu", this); diff --git a/Userland/Applications/Browser/Tab.cpp b/Userland/Applications/Browser/Tab.cpp index ca886b43aa..35305404ab 100644 --- a/Userland/Applications/Browser/Tab.cpp +++ b/Userland/Applications/Browser/Tab.cpp @@ -657,20 +657,20 @@ Tab::Tab(BrowserWindow& window) auto take_visible_screenshot_action = GUI::Action::create( "Take &Visible Screenshot"sv, g_icon_bag.filetype_image, [this](auto&) { - if (auto result = view().take_screenshot(WebView::ViewImplementation::ScreenshotType::Visible); result.is_error()) { - auto error = String::formatted("{}", result.error()).release_value_but_fixme_should_propagate_errors(); - GUI::MessageBox::show_error(&this->window(), error); - } + view().take_screenshot(WebView::ViewImplementation::ScreenshotType::Visible)->when_rejected([this](auto const& error) { + auto error_message = MUST(String::formatted("{}", error)); + GUI::MessageBox::show_error(&this->window(), error_message); + }); }, this); take_visible_screenshot_action->set_status_tip("Save a screenshot of the visible portion of the current tab to the Downloads directory"_string); auto take_full_screenshot_action = GUI::Action::create( "Take &Full Screenshot"sv, g_icon_bag.filetype_image, [this](auto&) { - if (auto result = view().take_screenshot(WebView::ViewImplementation::ScreenshotType::Full); result.is_error()) { - auto error = String::formatted("{}", result.error()).release_value_but_fixme_should_propagate_errors(); - GUI::MessageBox::show_error(&this->window(), error); - } + view().take_screenshot(WebView::ViewImplementation::ScreenshotType::Full)->when_rejected([this](auto const& error) { + auto error_message = MUST(String::formatted("{}", error)); + GUI::MessageBox::show_error(&this->window(), error_message); + }); }, this); take_full_screenshot_action->set_status_tip("Save a screenshot of the entirety of the current tab to the Downloads directory"_string); diff --git a/Userland/Libraries/LibWebView/InspectorClient.cpp b/Userland/Libraries/LibWebView/InspectorClient.cpp index 30cfa1d3d3..d6a9999281 100644 --- a/Userland/Libraries/LibWebView/InspectorClient.cpp +++ b/Userland/Libraries/LibWebView/InspectorClient.cpp @@ -254,10 +254,13 @@ void InspectorClient::context_menu_screenshot_dom_node() { VERIFY(m_context_menu_data.has_value()); - if (auto result = m_content_web_view.take_dom_node_screenshot(m_context_menu_data->dom_node_id); result.is_error()) - append_console_warning(MUST(String::formatted("Warning: {}", result.error()))); - else - append_console_message(MUST(String::formatted("Screenshot saved to: {}", result.value()))); + m_content_web_view.take_dom_node_screenshot(m_context_menu_data->dom_node_id) + ->when_resolved([this](auto const& path) { + append_console_message(MUST(String::formatted("Screenshot saved to: {}", path))); + }) + .when_rejected([this](auto const& error) { + append_console_warning(MUST(String::formatted("Warning: {}", error))); + }); m_context_menu_data.clear(); } diff --git a/Userland/Libraries/LibWebView/ViewImplementation.cpp b/Userland/Libraries/LibWebView/ViewImplementation.cpp index f0cbbf12e2..82a0360cd4 100644 --- a/Userland/Libraries/LibWebView/ViewImplementation.cpp +++ b/Userland/Libraries/LibWebView/ViewImplementation.cpp @@ -370,27 +370,65 @@ static ErrorOr save_screenshot(Gfx::ShareableBitmap const& bitmap) return path; } -ErrorOr ViewImplementation::take_screenshot(ScreenshotType type) +NonnullRefPtr> ViewImplementation::take_screenshot(ScreenshotType type) { + auto promise = Core::Promise::construct(); + + if (m_pending_screenshot) { + // For simplicitly, only allow taking one screenshot at a time for now. Revisit if we need + // to allow spamming screenshot requests for some reason. + promise->reject(Error::from_string_literal("A screenshot request is already in progress")); + return promise; + } + Gfx::ShareableBitmap bitmap; switch (type) { case ScreenshotType::Visible: - if (auto* visible_bitmap = m_client_state.has_usable_bitmap ? m_client_state.front_bitmap.bitmap.ptr() : m_backup_bitmap.ptr()) - bitmap = visible_bitmap->to_shareable_bitmap(); + if (auto* visible_bitmap = m_client_state.has_usable_bitmap ? m_client_state.front_bitmap.bitmap.ptr() : m_backup_bitmap.ptr()) { + if (auto result = save_screenshot(visible_bitmap->to_shareable_bitmap()); result.is_error()) + promise->reject(result.release_error()); + else + promise->resolve(result.release_value()); + } break; + case ScreenshotType::Full: - bitmap = client().take_document_screenshot(); + m_pending_screenshot = promise; + client().async_take_document_screenshot(); break; } - return save_screenshot(bitmap); + return promise; } -ErrorOr ViewImplementation::take_dom_node_screenshot(i32 node_id) +NonnullRefPtr> ViewImplementation::take_dom_node_screenshot(i32 node_id) { - auto bitmap = client().take_dom_node_screenshot(node_id); - return save_screenshot(bitmap); + auto promise = Core::Promise::construct(); + + if (m_pending_screenshot) { + // For simplicitly, only allow taking one screenshot at a time for now. Revisit if we need + // to allow spamming screenshot requests for some reason. + promise->reject(Error::from_string_literal("A screenshot request is already in progress")); + return promise; + } + + m_pending_screenshot = promise; + client().async_take_dom_node_screenshot(node_id); + + return promise; +} + +void ViewImplementation::did_receive_screenshot(Badge, Gfx::ShareableBitmap const& screenshot) +{ + VERIFY(m_pending_screenshot); + + if (auto result = save_screenshot(screenshot); result.is_error()) + m_pending_screenshot->reject(result.release_error()); + else + m_pending_screenshot->resolve(result.release_value()); + + m_pending_screenshot = nullptr; } ErrorOr ViewImplementation::dump_gc_graph() diff --git a/Userland/Libraries/LibWebView/ViewImplementation.h b/Userland/Libraries/LibWebView/ViewImplementation.h index 1e33c1c4da..527cbe89f5 100644 --- a/Userland/Libraries/LibWebView/ViewImplementation.h +++ b/Userland/Libraries/LibWebView/ViewImplementation.h @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -95,8 +96,9 @@ public: Visible, Full, }; - ErrorOr take_screenshot(ScreenshotType); - ErrorOr take_dom_node_screenshot(i32); + NonnullRefPtr> take_screenshot(ScreenshotType); + NonnullRefPtr> take_dom_node_screenshot(i32); + virtual void did_receive_screenshot(Badge, Gfx::ShareableBitmap const&); ErrorOr dump_gc_graph(); @@ -229,6 +231,8 @@ protected: size_t m_crash_count = 0; RefPtr m_repeated_crash_timer; + + RefPtr> m_pending_screenshot; }; } diff --git a/Userland/Libraries/LibWebView/WebContentClient.cpp b/Userland/Libraries/LibWebView/WebContentClient.cpp index f939b2da8f..0cee3a28be 100644 --- a/Userland/Libraries/LibWebView/WebContentClient.cpp +++ b/Userland/Libraries/LibWebView/WebContentClient.cpp @@ -231,6 +231,11 @@ void WebContentClient::did_get_dom_node_html(String const& html) m_view.on_received_dom_node_html(html); } +void WebContentClient::did_take_screenshot(Gfx::ShareableBitmap const& screenshot) +{ + m_view.did_receive_screenshot({}, screenshot); +} + 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 44bba319b8..8edef728da 100644 --- a/Userland/Libraries/LibWebView/WebContentClient.h +++ b/Userland/Libraries/LibWebView/WebContentClient.h @@ -58,6 +58,7 @@ private: 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_get_dom_node_html(String const& html) override; + virtual void did_take_screenshot(Gfx::ShareableBitmap const& screenshot) 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 a197224fbc..deb3b2e533 100644 --- a/Userland/Services/WebContent/ConnectionFromClient.cpp +++ b/Userland/Services/WebContent/ConnectionFromClient.cpp @@ -854,11 +854,13 @@ void ConnectionFromClient::js_console_request_messages(i32 start_index) m_top_level_document_console_client->send_messages(start_index); } -Messages::WebContentServer::TakeDocumentScreenshotResponse ConnectionFromClient::take_document_screenshot() +void ConnectionFromClient::take_document_screenshot() { auto* document = page().page().top_level_browsing_context().active_document(); - if (!document || !document->document_element()) - return Gfx::ShareableBitmap {}; + if (!document || !document->document_element()) { + async_did_take_screenshot({}); + return; + } auto const& content_size = page().content_size(); Web::DevicePixelRect rect { { 0, 0 }, content_size }; @@ -866,21 +868,23 @@ Messages::WebContentServer::TakeDocumentScreenshotResponse ConnectionFromClient: auto bitmap = Gfx::Bitmap::create(Gfx::BitmapFormat::BGRA8888, rect.size().to_type()).release_value_but_fixme_should_propagate_errors(); page().paint(rect, *bitmap); - return bitmap->to_shareable_bitmap(); + async_did_take_screenshot(bitmap->to_shareable_bitmap()); } -Messages::WebContentServer::TakeDomNodeScreenshotResponse ConnectionFromClient::take_dom_node_screenshot(i32 node_id) +void ConnectionFromClient::take_dom_node_screenshot(i32 node_id) { auto* dom_node = Web::DOM::Node::from_unique_id(node_id); - if (!dom_node || !dom_node->paintable_box()) - return Gfx::ShareableBitmap {}; + if (!dom_node || !dom_node->paintable_box()) { + async_did_take_screenshot({}); + return; + } auto rect = page().page().enclosing_device_rect(dom_node->paintable_box()->absolute_border_box_rect()); auto bitmap = Gfx::Bitmap::create(Gfx::BitmapFormat::BGRA8888, rect.size().to_type()).release_value_but_fixme_should_propagate_errors(); page().paint(rect, *bitmap, { .paint_overlay = Web::PaintOptions::PaintOverlay::No }); - return bitmap->to_shareable_bitmap(); + async_did_take_screenshot(bitmap->to_shareable_bitmap()); } Messages::WebContentServer::DumpGcGraphResponse ConnectionFromClient::dump_gc_graph() diff --git a/Userland/Services/WebContent/ConnectionFromClient.h b/Userland/Services/WebContent/ConnectionFromClient.h index 2df6d520ef..9ac49d8035 100644 --- a/Userland/Services/WebContent/ConnectionFromClient.h +++ b/Userland/Services/WebContent/ConnectionFromClient.h @@ -121,8 +121,8 @@ private: virtual void enable_inspector_prototype() override; - virtual Messages::WebContentServer::TakeDocumentScreenshotResponse take_document_screenshot() override; - virtual Messages::WebContentServer::TakeDomNodeScreenshotResponse take_dom_node_screenshot(i32 node_id) override; + virtual void take_document_screenshot() override; + virtual void take_dom_node_screenshot(i32 node_id) override; virtual Messages::WebContentServer::DumpGcGraphResponse dump_gc_graph() override; diff --git a/Userland/Services/WebContent/WebContentClient.ipc b/Userland/Services/WebContent/WebContentClient.ipc index bec98f90b0..eb7cdf9b05 100644 --- a/Userland/Services/WebContent/WebContentClient.ipc +++ b/Userland/Services/WebContent/WebContentClient.ipc @@ -48,6 +48,8 @@ endpoint WebContentClient did_finish_editing_dom_node(Optional node_id) =| did_get_dom_node_html(String html) =| + did_take_screenshot(Gfx::ShareableBitmap screenshot) =| + did_change_favicon(Gfx::ShareableBitmap favicon) =| did_request_all_cookies(URL url) => (Vector cookies) did_request_named_cookie(URL url, ByteString name) => (Optional cookie) diff --git a/Userland/Services/WebContent/WebContentServer.ipc b/Userland/Services/WebContent/WebContentServer.ipc index 1290a9dc96..ab111559b7 100644 --- a/Userland/Services/WebContent/WebContentServer.ipc +++ b/Userland/Services/WebContent/WebContentServer.ipc @@ -54,8 +54,8 @@ endpoint WebContentServer remove_dom_node(i32 node_id) =| get_dom_node_html(i32 node_id) =| - take_document_screenshot() => (Gfx::ShareableBitmap data) - take_dom_node_screenshot(i32 node_id) => (Gfx::ShareableBitmap data) + take_document_screenshot() =| + take_dom_node_screenshot(i32 node_id) =| dump_gc_graph() => (String json) diff --git a/Userland/Utilities/headless-browser.cpp b/Userland/Utilities/headless-browser.cpp index 42a364b949..2092acc50d 100644 --- a/Userland/Utilities/headless-browser.cpp +++ b/Userland/Utilities/headless-browser.cpp @@ -96,7 +96,21 @@ public: RefPtr take_screenshot() { - return client().take_document_screenshot().bitmap(); + VERIFY(!m_pending_screenshot); + + m_pending_screenshot = Core::Promise>::construct(); + client().async_take_document_screenshot(); + + auto screenshot = MUST(m_pending_screenshot->await()); + m_pending_screenshot = nullptr; + + return screenshot; + } + + virtual void did_receive_screenshot(Badge, Gfx::ShareableBitmap const& screenshot) override + { + VERIFY(m_pending_screenshot); + m_pending_screenshot->resolve(screenshot.bitmap()); } ErrorOr dump_layout_tree() @@ -145,6 +159,7 @@ private: private: Gfx::IntRect m_viewport_rect; + RefPtr>> m_pending_screenshot; }; static ErrorOr> load_page_for_screenshot_and_exit(Core::EventLoop& event_loop, HeadlessWebContentView& view, int screenshot_timeout)