Skip to content

Commit d8fa226

Browse files
trflynn89awesomekling
authored andcommitted
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).
1 parent 93db790 commit d8fa226

File tree

13 files changed

+121
-47
lines changed

13 files changed

+121
-47
lines changed

Ladybird/AppKit/UI/LadybirdWebView.mm

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -791,14 +791,16 @@ - (void)searchSelectedText:(id)sender
791791

792792
- (void)takeVisibleScreenshot:(id)sender
793793
{
794-
auto result = m_web_view_bridge->take_screenshot(WebView::ViewImplementation::ScreenshotType::Visible);
795-
(void)result; // FIXME: Display an error if this failed.
794+
m_web_view_bridge->take_screenshot(WebView::ViewImplementation::ScreenshotType::Visible)->when_rejected([](auto const& error) {
795+
(void)error; // FIXME: Display the error.
796+
});
796797
}
797798

798799
- (void)takeFullScreenshot:(id)sender
799800
{
800-
auto result = m_web_view_bridge->take_screenshot(WebView::ViewImplementation::ScreenshotType::Full);
801-
(void)result; // FIXME: Display an error if this failed.
801+
m_web_view_bridge->take_screenshot(WebView::ViewImplementation::ScreenshotType::Full)->when_rejected([](auto const& error) {
802+
(void)error; // FIXME: Display the error.
803+
});
802804
}
803805

804806
- (void)openLink:(id)sender

Ladybird/Qt/Tab.cpp

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -310,19 +310,19 @@ Tab::Tab(BrowserWindow* window, WebContentOptions const& web_content_options, St
310310
auto* take_visible_screenshot_action = new QAction("Take &Visible Screenshot", this);
311311
take_visible_screenshot_action->setIcon(load_icon_from_uri("resource://icons/16x16/filetype-image.png"sv));
312312
QObject::connect(take_visible_screenshot_action, &QAction::triggered, this, [this]() {
313-
if (auto result = view().take_screenshot(WebView::ViewImplementation::ScreenshotType::Visible); result.is_error()) {
314-
auto error = MUST(String::formatted("{}", result.error()));
315-
QMessageBox::warning(this, "Ladybird", qstring_from_ak_string(error));
316-
}
313+
view().take_screenshot(WebView::ViewImplementation::ScreenshotType::Visible)->when_rejected([this](auto const& error) {
314+
auto error_message = MUST(String::formatted("{}", error));
315+
QMessageBox::warning(this, "Ladybird", qstring_from_ak_string(error_message));
316+
});
317317
});
318318

319319
auto* take_full_screenshot_action = new QAction("Take &Full Screenshot", this);
320320
take_full_screenshot_action->setIcon(load_icon_from_uri("resource://icons/16x16/filetype-image.png"sv));
321321
QObject::connect(take_full_screenshot_action, &QAction::triggered, this, [this]() {
322-
if (auto result = view().take_screenshot(WebView::ViewImplementation::ScreenshotType::Full); result.is_error()) {
323-
auto error = MUST(String::formatted("{}", result.error()));
324-
QMessageBox::warning(this, "Ladybird", qstring_from_ak_string(error));
325-
}
322+
view().take_screenshot(WebView::ViewImplementation::ScreenshotType::Full)->when_rejected([this](auto const& error) {
323+
auto error_message = MUST(String::formatted("{}", error));
324+
QMessageBox::warning(this, "Ladybird", qstring_from_ak_string(error_message));
325+
});
326326
});
327327

328328
m_page_context_menu = new QMenu("Context menu", this);

Userland/Applications/Browser/Tab.cpp

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -657,20 +657,20 @@ Tab::Tab(BrowserWindow& window)
657657

658658
auto take_visible_screenshot_action = GUI::Action::create(
659659
"Take &Visible Screenshot"sv, g_icon_bag.filetype_image, [this](auto&) {
660-
if (auto result = view().take_screenshot(WebView::ViewImplementation::ScreenshotType::Visible); result.is_error()) {
661-
auto error = String::formatted("{}", result.error()).release_value_but_fixme_should_propagate_errors();
662-
GUI::MessageBox::show_error(&this->window(), error);
663-
}
660+
view().take_screenshot(WebView::ViewImplementation::ScreenshotType::Visible)->when_rejected([this](auto const& error) {
661+
auto error_message = MUST(String::formatted("{}", error));
662+
GUI::MessageBox::show_error(&this->window(), error_message);
663+
});
664664
},
665665
this);
666666
take_visible_screenshot_action->set_status_tip("Save a screenshot of the visible portion of the current tab to the Downloads directory"_string);
667667

668668
auto take_full_screenshot_action = GUI::Action::create(
669669
"Take &Full Screenshot"sv, g_icon_bag.filetype_image, [this](auto&) {
670-
if (auto result = view().take_screenshot(WebView::ViewImplementation::ScreenshotType::Full); result.is_error()) {
671-
auto error = String::formatted("{}", result.error()).release_value_but_fixme_should_propagate_errors();
672-
GUI::MessageBox::show_error(&this->window(), error);
673-
}
670+
view().take_screenshot(WebView::ViewImplementation::ScreenshotType::Full)->when_rejected([this](auto const& error) {
671+
auto error_message = MUST(String::formatted("{}", error));
672+
GUI::MessageBox::show_error(&this->window(), error_message);
673+
});
674674
},
675675
this);
676676
take_full_screenshot_action->set_status_tip("Save a screenshot of the entirety of the current tab to the Downloads directory"_string);

Userland/Libraries/LibWebView/InspectorClient.cpp

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -254,10 +254,13 @@ void InspectorClient::context_menu_screenshot_dom_node()
254254
{
255255
VERIFY(m_context_menu_data.has_value());
256256

257-
if (auto result = m_content_web_view.take_dom_node_screenshot(m_context_menu_data->dom_node_id); result.is_error())
258-
append_console_warning(MUST(String::formatted("Warning: {}", result.error())));
259-
else
260-
append_console_message(MUST(String::formatted("Screenshot saved to: {}", result.value())));
257+
m_content_web_view.take_dom_node_screenshot(m_context_menu_data->dom_node_id)
258+
->when_resolved([this](auto const& path) {
259+
append_console_message(MUST(String::formatted("Screenshot saved to: {}", path)));
260+
})
261+
.when_rejected([this](auto const& error) {
262+
append_console_warning(MUST(String::formatted("Warning: {}", error)));
263+
});
261264

262265
m_context_menu_data.clear();
263266
}

Userland/Libraries/LibWebView/ViewImplementation.cpp

Lines changed: 46 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -370,27 +370,65 @@ static ErrorOr<LexicalPath> save_screenshot(Gfx::ShareableBitmap const& bitmap)
370370
return path;
371371
}
372372

373-
ErrorOr<LexicalPath> ViewImplementation::take_screenshot(ScreenshotType type)
373+
NonnullRefPtr<Core::Promise<LexicalPath>> ViewImplementation::take_screenshot(ScreenshotType type)
374374
{
375+
auto promise = Core::Promise<LexicalPath>::construct();
376+
377+
if (m_pending_screenshot) {
378+
// For simplicitly, only allow taking one screenshot at a time for now. Revisit if we need
379+
// to allow spamming screenshot requests for some reason.
380+
promise->reject(Error::from_string_literal("A screenshot request is already in progress"));
381+
return promise;
382+
}
383+
375384
Gfx::ShareableBitmap bitmap;
376385

377386
switch (type) {
378387
case ScreenshotType::Visible:
379-
if (auto* visible_bitmap = m_client_state.has_usable_bitmap ? m_client_state.front_bitmap.bitmap.ptr() : m_backup_bitmap.ptr())
380-
bitmap = visible_bitmap->to_shareable_bitmap();
388+
if (auto* visible_bitmap = m_client_state.has_usable_bitmap ? m_client_state.front_bitmap.bitmap.ptr() : m_backup_bitmap.ptr()) {
389+
if (auto result = save_screenshot(visible_bitmap->to_shareable_bitmap()); result.is_error())
390+
promise->reject(result.release_error());
391+
else
392+
promise->resolve(result.release_value());
393+
}
381394
break;
395+
382396
case ScreenshotType::Full:
383-
bitmap = client().take_document_screenshot();
397+
m_pending_screenshot = promise;
398+
client().async_take_document_screenshot();
384399
break;
385400
}
386401

387-
return save_screenshot(bitmap);
402+
return promise;
403+
}
404+
405+
NonnullRefPtr<Core::Promise<LexicalPath>> ViewImplementation::take_dom_node_screenshot(i32 node_id)
406+
{
407+
auto promise = Core::Promise<LexicalPath>::construct();
408+
409+
if (m_pending_screenshot) {
410+
// For simplicitly, only allow taking one screenshot at a time for now. Revisit if we need
411+
// to allow spamming screenshot requests for some reason.
412+
promise->reject(Error::from_string_literal("A screenshot request is already in progress"));
413+
return promise;
414+
}
415+
416+
m_pending_screenshot = promise;
417+
client().async_take_dom_node_screenshot(node_id);
418+
419+
return promise;
388420
}
389421

390-
ErrorOr<LexicalPath> ViewImplementation::take_dom_node_screenshot(i32 node_id)
422+
void ViewImplementation::did_receive_screenshot(Badge<WebContentClient>, Gfx::ShareableBitmap const& screenshot)
391423
{
392-
auto bitmap = client().take_dom_node_screenshot(node_id);
393-
return save_screenshot(bitmap);
424+
VERIFY(m_pending_screenshot);
425+
426+
if (auto result = save_screenshot(screenshot); result.is_error())
427+
m_pending_screenshot->reject(result.release_error());
428+
else
429+
m_pending_screenshot->resolve(result.release_value());
430+
431+
m_pending_screenshot = nullptr;
394432
}
395433

396434
ErrorOr<LexicalPath> ViewImplementation::dump_gc_graph()

Userland/Libraries/LibWebView/ViewImplementation.h

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
#include <AK/Function.h>
1212
#include <AK/LexicalPath.h>
1313
#include <AK/String.h>
14+
#include <LibCore/Promise.h>
1415
#include <LibGfx/Forward.h>
1516
#include <LibGfx/StandardCursor.h>
1617
#include <LibWeb/Forward.h>
@@ -95,8 +96,9 @@ class ViewImplementation {
9596
Visible,
9697
Full,
9798
};
98-
ErrorOr<LexicalPath> take_screenshot(ScreenshotType);
99-
ErrorOr<LexicalPath> take_dom_node_screenshot(i32);
99+
NonnullRefPtr<Core::Promise<LexicalPath>> take_screenshot(ScreenshotType);
100+
NonnullRefPtr<Core::Promise<LexicalPath>> take_dom_node_screenshot(i32);
101+
virtual void did_receive_screenshot(Badge<WebContentClient>, Gfx::ShareableBitmap const&);
100102

101103
ErrorOr<LexicalPath> dump_gc_graph();
102104

@@ -229,6 +231,8 @@ class ViewImplementation {
229231

230232
size_t m_crash_count = 0;
231233
RefPtr<Core::Timer> m_repeated_crash_timer;
234+
235+
RefPtr<Core::Promise<LexicalPath>> m_pending_screenshot;
232236
};
233237

234238
}

Userland/Libraries/LibWebView/WebContentClient.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,11 @@ void WebContentClient::did_get_dom_node_html(String const& html)
231231
m_view.on_received_dom_node_html(html);
232232
}
233233

234+
void WebContentClient::did_take_screenshot(Gfx::ShareableBitmap const& screenshot)
235+
{
236+
m_view.did_receive_screenshot({}, screenshot);
237+
}
238+
234239
void WebContentClient::did_output_js_console_message(i32 message_index)
235240
{
236241
if (m_view.on_received_console_message)

Userland/Libraries/LibWebView/WebContentClient.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ class WebContentClient final
5858
virtual void did_get_hovered_node_id(i32 node_id) override;
5959
virtual void did_finish_editing_dom_node(Optional<i32> const& node_id) override;
6060
virtual void did_get_dom_node_html(String const& html) override;
61+
virtual void did_take_screenshot(Gfx::ShareableBitmap const& screenshot) override;
6162
virtual void did_output_js_console_message(i32 message_index) override;
6263
virtual void did_get_js_console_messages(i32 start_index, Vector<ByteString> const& message_types, Vector<ByteString> const& messages) override;
6364
virtual void did_change_favicon(Gfx::ShareableBitmap const&) override;

Userland/Services/WebContent/ConnectionFromClient.cpp

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -854,33 +854,37 @@ void ConnectionFromClient::js_console_request_messages(i32 start_index)
854854
m_top_level_document_console_client->send_messages(start_index);
855855
}
856856

857-
Messages::WebContentServer::TakeDocumentScreenshotResponse ConnectionFromClient::take_document_screenshot()
857+
void ConnectionFromClient::take_document_screenshot()
858858
{
859859
auto* document = page().page().top_level_browsing_context().active_document();
860-
if (!document || !document->document_element())
861-
return Gfx::ShareableBitmap {};
860+
if (!document || !document->document_element()) {
861+
async_did_take_screenshot({});
862+
return;
863+
}
862864

863865
auto const& content_size = page().content_size();
864866
Web::DevicePixelRect rect { { 0, 0 }, content_size };
865867

866868
auto bitmap = Gfx::Bitmap::create(Gfx::BitmapFormat::BGRA8888, rect.size().to_type<int>()).release_value_but_fixme_should_propagate_errors();
867869
page().paint(rect, *bitmap);
868870

869-
return bitmap->to_shareable_bitmap();
871+
async_did_take_screenshot(bitmap->to_shareable_bitmap());
870872
}
871873

872-
Messages::WebContentServer::TakeDomNodeScreenshotResponse ConnectionFromClient::take_dom_node_screenshot(i32 node_id)
874+
void ConnectionFromClient::take_dom_node_screenshot(i32 node_id)
873875
{
874876
auto* dom_node = Web::DOM::Node::from_unique_id(node_id);
875-
if (!dom_node || !dom_node->paintable_box())
876-
return Gfx::ShareableBitmap {};
877+
if (!dom_node || !dom_node->paintable_box()) {
878+
async_did_take_screenshot({});
879+
return;
880+
}
877881

878882
auto rect = page().page().enclosing_device_rect(dom_node->paintable_box()->absolute_border_box_rect());
879883

880884
auto bitmap = Gfx::Bitmap::create(Gfx::BitmapFormat::BGRA8888, rect.size().to_type<int>()).release_value_but_fixme_should_propagate_errors();
881885
page().paint(rect, *bitmap, { .paint_overlay = Web::PaintOptions::PaintOverlay::No });
882886

883-
return bitmap->to_shareable_bitmap();
887+
async_did_take_screenshot(bitmap->to_shareable_bitmap());
884888
}
885889

886890
Messages::WebContentServer::DumpGcGraphResponse ConnectionFromClient::dump_gc_graph()

Userland/Services/WebContent/ConnectionFromClient.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -121,8 +121,8 @@ class ConnectionFromClient final
121121

122122
virtual void enable_inspector_prototype() override;
123123

124-
virtual Messages::WebContentServer::TakeDocumentScreenshotResponse take_document_screenshot() override;
125-
virtual Messages::WebContentServer::TakeDomNodeScreenshotResponse take_dom_node_screenshot(i32 node_id) override;
124+
virtual void take_document_screenshot() override;
125+
virtual void take_dom_node_screenshot(i32 node_id) override;
126126

127127
virtual Messages::WebContentServer::DumpGcGraphResponse dump_gc_graph() override;
128128

0 commit comments

Comments
 (0)