From f7c4165dde9cc1f3c84fbc49c8496dea3a898491 Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Wed, 21 Aug 2024 06:39:02 -0400 Subject: [PATCH] LibWeb: Make the drag data store reference counted Ownership of the drag data store is a bit weird. In a normal drag-and- drop operation, the DragAndDropEventHandler owns the store. When events are fired for the operation, the DataTransfer object assigned to those events are "associated" with the store. We currently represent that with an Optional. However, it's also possible to create DataTransfer objects from scripts. Those objects create their own drag data store. This puts DataTransfer in a weird situation where it may own a store or just reference one. Rather than coming up with something like Variant or using MaybeOwned here, we can get by with just making the store reference counted. --- Userland/Libraries/LibWeb/HTML/DataTransfer.cpp | 12 ++++++------ Userland/Libraries/LibWeb/HTML/DataTransfer.h | 4 ++-- Userland/Libraries/LibWeb/HTML/DragDataStore.cpp | 5 +++++ Userland/Libraries/LibWeb/HTML/DragDataStore.h | 8 ++++++-- .../LibWeb/Page/DragAndDropEventHandler.cpp | 6 +++--- .../Libraries/LibWeb/Page/DragAndDropEventHandler.h | 4 ++-- 6 files changed, 24 insertions(+), 15 deletions(-) diff --git a/Userland/Libraries/LibWeb/HTML/DataTransfer.cpp b/Userland/Libraries/LibWeb/HTML/DataTransfer.cpp index 8cd25b62284..a569403d543 100644 --- a/Userland/Libraries/LibWeb/HTML/DataTransfer.cpp +++ b/Userland/Libraries/LibWeb/HTML/DataTransfer.cpp @@ -76,7 +76,7 @@ void DataTransfer::set_effect_allowed(FlyString effect_allowed) // On setting, if drag data store's mode is the read/write mode and the new value is one of "none", "copy", "copyLink", // "copyMove", "link", "linkMove", "move", "all", or "uninitialized", then the attribute's current value must be set // to the new value. Otherwise, it must be left unchanged. - if (m_associated_drag_data_store.has_value() && m_associated_drag_data_store->mode() == DragDataStore::Mode::ReadWrite) + if (m_associated_drag_data_store && m_associated_drag_data_store->mode() == DragDataStore::Mode::ReadWrite) set_effect_allowed_internal(move(effect_allowed)); } @@ -109,7 +109,7 @@ ReadonlySpan DataTransfer::types() const String DataTransfer::get_data(String const& format_argument) const { // 1. If the DataTransfer object is no longer associated with a drag data store, then return the empty string. - if (!m_associated_drag_data_store.has_value()) + if (!m_associated_drag_data_store) return {}; // 2. If the drag data store's mode is the protected mode, then return the empty string. @@ -165,7 +165,7 @@ JS::NonnullGCPtr DataTransfer::files() const // 2. If the DataTransfer object is no longer associated with a drag data store, the FileList is empty. Return // the empty list L. - if (!m_associated_drag_data_store.has_value()) + if (!m_associated_drag_data_store) return files; // 3. If the drag data store's mode is the protected mode, return the empty list L. @@ -195,9 +195,9 @@ JS::NonnullGCPtr DataTransfer::files() const return files; } -void DataTransfer::associate_with_drag_data_store(DragDataStore& drag_data_store) +void DataTransfer::associate_with_drag_data_store(NonnullRefPtr drag_data_store) { - m_associated_drag_data_store = drag_data_store; + m_associated_drag_data_store = move(drag_data_store); update_data_transfer_types_list(); } @@ -214,7 +214,7 @@ void DataTransfer::update_data_transfer_types_list() Vector types; // 2. If the DataTransfer object is still associated with a drag data store, then: - if (m_associated_drag_data_store.has_value()) { + if (m_associated_drag_data_store) { bool contains_file = false; // 1. For each item in the DataTransfer object's drag data store item list whose kind is text, add an entry to L diff --git a/Userland/Libraries/LibWeb/HTML/DataTransfer.h b/Userland/Libraries/LibWeb/HTML/DataTransfer.h index 0bdeacb2cb7..c6d327ab6fd 100644 --- a/Userland/Libraries/LibWeb/HTML/DataTransfer.h +++ b/Userland/Libraries/LibWeb/HTML/DataTransfer.h @@ -55,7 +55,7 @@ public: String get_data(String const& format) const; JS::NonnullGCPtr files() const; - void associate_with_drag_data_store(DragDataStore& drag_data_store); + void associate_with_drag_data_store(NonnullRefPtr drag_data_store); void disassociate_with_drag_data_store(); private: @@ -79,7 +79,7 @@ private: Vector m_types; // https://html.spec.whatwg.org/multipage/dnd.html#the-datatransfer-interface:drag-data-store-3 - Optional m_associated_drag_data_store; + RefPtr m_associated_drag_data_store; }; } diff --git a/Userland/Libraries/LibWeb/HTML/DragDataStore.cpp b/Userland/Libraries/LibWeb/HTML/DragDataStore.cpp index 9c1572c9e70..8cdfd4a175a 100644 --- a/Userland/Libraries/LibWeb/HTML/DragDataStore.cpp +++ b/Userland/Libraries/LibWeb/HTML/DragDataStore.cpp @@ -9,6 +9,11 @@ namespace Web::HTML { +NonnullRefPtr DragDataStore::create() +{ + return adopt_ref(*new DragDataStore()); +} + DragDataStore::DragDataStore() : m_allowed_effects_state(DataTransferEffect::uninitialized) { diff --git a/Userland/Libraries/LibWeb/HTML/DragDataStore.h b/Userland/Libraries/LibWeb/HTML/DragDataStore.h index 7df8e7a75c2..ddc6bb8a65f 100644 --- a/Userland/Libraries/LibWeb/HTML/DragDataStore.h +++ b/Userland/Libraries/LibWeb/HTML/DragDataStore.h @@ -9,6 +9,8 @@ #include #include #include +#include +#include #include #include #include @@ -33,7 +35,7 @@ struct DragDataStoreItem { }; // https://html.spec.whatwg.org/multipage/dnd.html#drag-data-store -class DragDataStore { +class DragDataStore : public RefCounted { public: enum class Mode { ReadWrite, @@ -41,7 +43,7 @@ public: Protected, }; - DragDataStore(); + static NonnullRefPtr create(); ~DragDataStore(); void add_item(DragDataStoreItem item) { m_item_list.append(move(item)); } @@ -55,6 +57,8 @@ public: void set_allowed_effects_state(FlyString allowed_effects_state) { m_allowed_effects_state = move(allowed_effects_state); } private: + DragDataStore(); + // https://html.spec.whatwg.org/multipage/dnd.html#drag-data-store-item-list Vector m_item_list; diff --git a/Userland/Libraries/LibWeb/Page/DragAndDropEventHandler.cpp b/Userland/Libraries/LibWeb/Page/DragAndDropEventHandler.cpp index 717aaa5466c..8f2fa3db242 100644 --- a/Userland/Libraries/LibWeb/Page/DragAndDropEventHandler.cpp +++ b/Userland/Libraries/LibWeb/Page/DragAndDropEventHandler.cpp @@ -51,7 +51,7 @@ bool DragAndDropEventHandler::handle_drag_start( // 2. Create a drag data store. All the DND events fired subsequently by the steps in this section must use this drag // data store. - m_drag_data_store.emplace(); + m_drag_data_store = HTML::DragDataStore::create(); // 3. Establish which DOM node is the source node, as follows: // @@ -185,7 +185,7 @@ bool DragAndDropEventHandler::handle_drag_move( unsigned buttons, unsigned modifiers) { - if (!m_drag_data_store.has_value()) + if (!has_ongoing_drag_and_drop_operation()) return false; auto fire_a_drag_and_drop_event = [&](JS::GCPtr target, FlyString const& name, JS::GCPtr related_target = nullptr) { @@ -361,7 +361,7 @@ bool DragAndDropEventHandler::handle_drag_end( unsigned buttons, unsigned modifiers) { - if (!m_drag_data_store.has_value()) + if (!has_ongoing_drag_and_drop_operation()) return false; auto fire_a_drag_and_drop_event = [&](JS::GCPtr target, FlyString const& name, JS::GCPtr related_target = nullptr) { diff --git a/Userland/Libraries/LibWeb/Page/DragAndDropEventHandler.h b/Userland/Libraries/LibWeb/Page/DragAndDropEventHandler.h index 995b1de9184..b96b3b98f9d 100644 --- a/Userland/Libraries/LibWeb/Page/DragAndDropEventHandler.h +++ b/Userland/Libraries/LibWeb/Page/DragAndDropEventHandler.h @@ -18,7 +18,7 @@ class DragAndDropEventHandler { public: void visit_edges(JS::Cell::Visitor& visitor) const; - bool has_ongoing_drag_and_drop_operation() const { return m_drag_data_store.has_value(); } + bool has_ongoing_drag_and_drop_operation() const { return !m_drag_data_store.is_null(); } bool handle_drag_start(JS::Realm&, CSSPixelPoint screen_position, CSSPixelPoint page_offset, CSSPixelPoint client_offset, CSSPixelPoint offset, unsigned button, unsigned buttons, unsigned modifiers, Vector files); bool handle_drag_move(JS::Realm&, JS::NonnullGCPtr, JS::NonnullGCPtr, CSSPixelPoint screen_position, CSSPixelPoint page_offset, CSSPixelPoint client_offset, CSSPixelPoint offset, unsigned button, unsigned buttons, unsigned modifiers); @@ -49,7 +49,7 @@ private: void reset(); - Optional m_drag_data_store; + RefPtr m_drag_data_store; // https://html.spec.whatwg.org/multipage/dnd.html#source-node JS::GCPtr m_source_node;