Migrate VoidCallbacks to use simple success/failure callbacks
This CL is a continuation of the effort to simplify the callbacks
implementation in t_p/b/renderer/modules/filesystem/file_system_callbacks.h
At this time, VoidCallbacks wrapper class is migrated to use
simple (BindOnce) callbacks, one for success, another for failure.
Note that VoidCallbacks did not share the same callback completion
implementation as some other methods that use the templates in
async_callback_helper.h. For this reason, two non-template methods
were added to async_callback_helper.h, to be used exclusively by
VoidCallbacks (no hard enforcement, only the documentation in the
file header).
This is the last CL before we are able to clean up sync_callback_helper.h
a lot, as well as file_system_callbacks.h.
R=mek@chromium.org
CC=blink-reviews-vendor@chromium.org
BUG=933878
Change-Id: I16c1d2e1defc9afeff0a616fe0b2fcc00a2d5c65
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1529482
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Cr-Commit-Position: refs/heads/master@{#642772}diff --git a/third_party/blink/renderer/modules/filesystem/async_callback_helper.h b/third_party/blink/renderer/modules/filesystem/async_callback_helper.h
index 5114722..1d97d9bc 100644
--- a/third_party/blink/renderer/modules/filesystem/async_callback_helper.h
+++ b/third_party/blink/renderer/modules/filesystem/async_callback_helper.h
@@ -69,6 +69,26 @@
},
WrapPersistentIfNeeded(resolver));
}
+
+ // The two methods below are not templetized, to be used exclusively for
+ // VoidCallbacks.
+ static base::OnceCallback<void()> VoidSuccessCallback(
+ V8VoidCallback* success_callback) {
+ auto success_callback_wrapper = WTF::Bind(
+ [](V8PersistentCallbackInterface<V8VoidCallback>* persistent_callback) {
+ persistent_callback->InvokeAndReportException(nullptr);
+ },
+ WrapPersistentIfNeeded(
+ ToV8PersistentCallbackInterface(success_callback)));
+ return success_callback_wrapper;
+ }
+
+ static base::OnceCallback<void()> VoidSuccessPromise(
+ ScriptPromiseResolver* resolver) {
+ return WTF::Bind(
+ [](ScriptPromiseResolver* resolver) { resolver->Resolve(); },
+ WrapPersistentIfNeeded(resolver));
+ }
};
} // namespace blink
diff --git a/third_party/blink/renderer/modules/filesystem/directory_entry.cc b/third_party/blink/renderer/modules/filesystem/directory_entry.cc
index ef10104..e14b35c 100644
--- a/third_party/blink/renderer/modules/filesystem/directory_entry.cc
+++ b/third_party/blink/renderer/modules/filesystem/directory_entry.cc
@@ -76,9 +76,13 @@
void DirectoryEntry::removeRecursively(V8VoidCallback* success_callback,
V8ErrorCallback* error_callback) const {
- file_system_->RemoveRecursively(
- this, VoidCallbacks::OnDidSucceedV8Impl::Create(success_callback),
- ScriptErrorCallback::Wrap(error_callback));
+ auto success_callback_wrapper =
+ AsyncCallbackHelper::VoidSuccessCallback(success_callback);
+ auto error_callback_wrapper =
+ AsyncCallbackHelper::ErrorCallback(error_callback);
+
+ file_system_->RemoveRecursively(this, std::move(success_callback_wrapper),
+ std::move(error_callback_wrapper));
}
void DirectoryEntry::Trace(blink::Visitor* visitor) {
diff --git a/third_party/blink/renderer/modules/filesystem/directory_entry_sync.cc b/third_party/blink/renderer/modules/filesystem/directory_entry_sync.cc
index b707ba1..89d669c6 100644
--- a/third_party/blink/renderer/modules/filesystem/directory_entry_sync.cc
+++ b/third_party/blink/renderer/modules/filesystem/directory_entry_sync.cc
@@ -87,8 +87,12 @@
void DirectoryEntrySync::removeRecursively(ExceptionState& exception_state) {
auto* sync_helper = MakeGarbageCollected<VoidCallbacksSyncHelper>();
- file_system_->RemoveRecursively(this, nullptr,
- sync_helper->GetErrorCallback(),
+
+ auto error_callback_wrapper = WTF::Bind(&VoidCallbacksSyncHelper::OnError,
+ WrapPersistentIfNeeded(sync_helper));
+
+ file_system_->RemoveRecursively(this, VoidCallbacks::SuccessCallback(),
+ std::move(error_callback_wrapper),
DOMFileSystemBase::kSynchronous);
sync_helper->GetResultOrThrow(exception_state);
}
diff --git a/third_party/blink/renderer/modules/filesystem/dom_file_system_base.cc b/third_party/blink/renderer/modules/filesystem/dom_file_system_base.cc
index c99739c4..dd08da5e 100644
--- a/third_party/blink/renderer/modules/filesystem/dom_file_system_base.cc
+++ b/third_party/blink/renderer/modules/filesystem/dom_file_system_base.cc
@@ -314,20 +314,20 @@
dispatcher.Copy(src, dest, std::move(callbacks));
}
-void DOMFileSystemBase::Remove(
- const EntryBase* entry,
- VoidCallbacks::OnDidSucceedCallback* success_callback,
- ErrorCallbackBase* error_callback,
- SynchronousType synchronous_type) {
+void DOMFileSystemBase::Remove(const EntryBase* entry,
+ VoidCallbacks::SuccessCallback success_callback,
+ ErrorCallback error_callback,
+ SynchronousType synchronous_type) {
DCHECK(entry);
// We don't allow calling remove() on the root directory.
if (entry->fullPath() == String(DOMFilePath::kRoot)) {
- ReportError(error_callback, base::File::FILE_ERROR_INVALID_OPERATION);
+ ReportError(std::move(error_callback),
+ base::File::FILE_ERROR_INVALID_OPERATION);
return;
}
auto callbacks = std::make_unique<VoidCallbacks>(
- success_callback, error_callback, context_, this);
+ std::move(success_callback), std::move(error_callback), context_, this);
const KURL& url = CreateFileSystemURL(entry);
FileSystemDispatcher& dispatcher = FileSystemDispatcher::From(context_);
if (synchronous_type == kSynchronous)
@@ -338,19 +338,20 @@
void DOMFileSystemBase::RemoveRecursively(
const EntryBase* entry,
- VoidCallbacks::OnDidSucceedCallback* success_callback,
- ErrorCallbackBase* error_callback,
+ VoidCallbacks::SuccessCallback success_callback,
+ ErrorCallback error_callback,
SynchronousType synchronous_type) {
DCHECK(entry);
DCHECK(entry->isDirectory());
// We don't allow calling remove() on the root directory.
if (entry->fullPath() == String(DOMFilePath::kRoot)) {
- ReportError(error_callback, base::File::FILE_ERROR_INVALID_OPERATION);
+ ReportError(std::move(error_callback),
+ base::File::FILE_ERROR_INVALID_OPERATION);
return;
}
auto callbacks = std::make_unique<VoidCallbacks>(
- success_callback, error_callback, context_, this);
+ std::move(success_callback), std::move(error_callback), context_, this);
const KURL& url = CreateFileSystemURL(entry);
FileSystemDispatcher& dispatcher = FileSystemDispatcher::From(context_);
if (synchronous_type == kSynchronous)
diff --git a/third_party/blink/renderer/modules/filesystem/dom_file_system_base.h b/third_party/blink/renderer/modules/filesystem/dom_file_system_base.h
index 4631ce6..d5c7e33 100644
--- a/third_party/blink/renderer/modules/filesystem/dom_file_system_base.h
+++ b/third_party/blink/renderer/modules/filesystem/dom_file_system_base.h
@@ -126,12 +126,12 @@
EntryCallbacks::ErrorCallback,
SynchronousType = kAsynchronous);
void Remove(const EntryBase*,
- VoidCallbacks::OnDidSucceedCallback*,
- ErrorCallbackBase*,
+ VoidCallbacks::SuccessCallback,
+ VoidCallbacks::ErrorCallback,
SynchronousType = kAsynchronous);
void RemoveRecursively(const EntryBase*,
- VoidCallbacks::OnDidSucceedCallback*,
- ErrorCallbackBase*,
+ VoidCallbacks::SuccessCallback,
+ VoidCallbacks::ErrorCallback,
SynchronousType = kAsynchronous);
void GetParent(const EntryBase*,
EntryCallbacks::SuccessCallback,
diff --git a/third_party/blink/renderer/modules/filesystem/entry.cc b/third_party/blink/renderer/modules/filesystem/entry.cc
index 83f2cd8c..b2b0cd9 100644
--- a/third_party/blink/renderer/modules/filesystem/entry.cc
+++ b/third_party/blink/renderer/modules/filesystem/entry.cc
@@ -116,9 +116,14 @@
UseCounter::Count(ExecutionContext::From(script_state),
WebFeature::kEntry_Remove_Method_IsolatedFileSystem);
}
- file_system_->Remove(
- this, VoidCallbacks::OnDidSucceedV8Impl::Create(success_callback),
- ScriptErrorCallback::Wrap(error_callback));
+
+ auto success_callback_wrapper =
+ AsyncCallbackHelper::VoidSuccessCallback(success_callback);
+ auto error_callback_wrapper =
+ AsyncCallbackHelper::ErrorCallback(error_callback);
+
+ file_system_->Remove(this, std::move(success_callback_wrapper),
+ std::move(error_callback_wrapper));
}
void Entry::getParent(ScriptState* script_state,
diff --git a/third_party/blink/renderer/modules/filesystem/entry_sync.cc b/third_party/blink/renderer/modules/filesystem/entry_sync.cc
index f9036ab..77271db 100644
--- a/third_party/blink/renderer/modules/filesystem/entry_sync.cc
+++ b/third_party/blink/renderer/modules/filesystem/entry_sync.cc
@@ -100,7 +100,12 @@
void EntrySync::remove(ExceptionState& exception_state) const {
auto* sync_helper = MakeGarbageCollected<VoidCallbacksSyncHelper>();
- file_system_->Remove(this, nullptr, sync_helper->GetErrorCallback(),
+
+ auto error_callback_wrapper = WTF::Bind(&VoidCallbacksSyncHelper::OnError,
+ WrapPersistentIfNeeded(sync_helper));
+
+ file_system_->Remove(this, VoidCallbacks::SuccessCallback(),
+ std::move(error_callback_wrapper),
DOMFileSystemBase::kSynchronous);
sync_helper->GetResultOrThrow(exception_state);
}
diff --git a/third_party/blink/renderer/modules/filesystem/file_system_base_handle.cc b/third_party/blink/renderer/modules/filesystem/file_system_base_handle.cc
index 11c6785..8dc6755 100644
--- a/third_party/blink/renderer/modules/filesystem/file_system_base_handle.cc
+++ b/third_party/blink/renderer/modules/filesystem/file_system_base_handle.cc
@@ -62,10 +62,13 @@
ScriptPromise FileSystemBaseHandle::remove(ScriptState* script_state) {
auto* resolver = ScriptPromiseResolver::Create(script_state);
ScriptPromise result = resolver->Promise();
- filesystem()->Remove(
- this,
- MakeGarbageCollected<VoidCallbacks::OnDidSucceedPromiseImpl>(resolver),
- MakeGarbageCollected<PromiseErrorCallback>(resolver));
+
+ auto success_callback_wrapper =
+ AsyncCallbackHelper::VoidSuccessPromise(resolver);
+ auto error_callback_wrapper = AsyncCallbackHelper::ErrorPromise(resolver);
+
+ filesystem()->Remove(this, std::move(success_callback_wrapper),
+ std::move(error_callback_wrapper));
return result;
}
diff --git a/third_party/blink/renderer/modules/filesystem/file_system_callbacks.cc b/third_party/blink/renderer/modules/filesystem/file_system_callbacks.cc
index 41d490b95..04011819 100644
--- a/third_party/blink/renderer/modules/filesystem/file_system_callbacks.cc
+++ b/third_party/blink/renderer/modules/filesystem/file_system_callbacks.cc
@@ -368,48 +368,28 @@
// VoidCallbacks --------------------------------------------------------------
-void VoidCallbacks::OnDidSucceedV8Impl::Trace(blink::Visitor* visitor) {
- visitor->Trace(callback_);
- OnDidSucceedCallback::Trace(visitor);
-}
-
-void VoidCallbacks::OnDidSucceedV8Impl::OnSuccess(
- ExecutionContext* dummy_arg_for_sync_helper) {
- callback_->InvokeAndReportException(nullptr);
-}
-
-VoidCallbacks::OnDidSucceedPromiseImpl::OnDidSucceedPromiseImpl(
- ScriptPromiseResolver* resolver)
- : resolver_(resolver) {}
-
-void VoidCallbacks::OnDidSucceedPromiseImpl::Trace(Visitor* visitor) {
- OnDidSucceedCallback::Trace(visitor);
- visitor->Trace(resolver_);
-}
-
-void VoidCallbacks::OnDidSucceedPromiseImpl::OnSuccess(ExecutionContext*) {
- resolver_->Resolve();
-}
-
-VoidCallbacks::VoidCallbacks(OnDidSucceedCallback* success_callback,
- ErrorCallbackBase* error_callback,
+VoidCallbacks::VoidCallbacks(SuccessCallback success_callback,
+ ErrorCallback error_callback,
ExecutionContext* context,
DOMFileSystemBase* file_system)
- : FileSystemCallbacksBase(error_callback, file_system, context),
- success_callback_(success_callback) {}
+ : FileSystemCallbacksBase(/*error_callback =*/nullptr,
+ file_system,
+ context),
+ success_callback_(std::move(success_callback)),
+ error_callback_(std::move(error_callback)) {}
void VoidCallbacks::DidSucceed() {
if (!success_callback_)
return;
- success_callback_.Release()->OnSuccess(execution_context_.Get());
+ std::move(success_callback_).Run();
}
void VoidCallbacks::DidFail(base::File::Error error) {
if (!error_callback_)
return;
- error_callback_.Release()->Invoke(error);
+ std::move(error_callback_).Run(error);
}
} // namespace blink
diff --git a/third_party/blink/renderer/modules/filesystem/file_system_callbacks.h b/third_party/blink/renderer/modules/filesystem/file_system_callbacks.h
index c1449e6..ac94f67b 100644
--- a/third_party/blink/renderer/modules/filesystem/file_system_callbacks.h
+++ b/third_party/blink/renderer/modules/filesystem/file_system_callbacks.h
@@ -392,6 +392,10 @@
class VoidCallbacks final : public FileSystemCallbacksBase {
public:
+ // TODO(tonikitoo,mek): This class is not being effectively used anymore.
+ // Remove it when all classes have switched to simple success/error callbacks
+ // implementation, and there is no reference to it in sync_callback_helper.h
+ // anymore.
class OnDidSucceedCallback
: public GarbageCollectedFinalized<OnDidSucceedCallback> {
public:
@@ -403,35 +407,11 @@
OnDidSucceedCallback() = default;
};
- class OnDidSucceedV8Impl : public OnDidSucceedCallback {
- public:
- static OnDidSucceedV8Impl* Create(V8VoidCallback* callback) {
- return callback ? MakeGarbageCollected<OnDidSucceedV8Impl>(callback)
- : nullptr;
- }
+ using SuccessCallback = base::OnceCallback<void()>;
+ using ErrorCallback = base::OnceCallback<void(base::File::Error error)>;
- OnDidSucceedV8Impl(V8VoidCallback* callback)
- : callback_(ToV8PersistentCallbackInterface(callback)) {}
-
- void Trace(blink::Visitor*) override;
- void OnSuccess(ExecutionContext* dummy_arg_for_sync_helper) override;
-
- private:
- Member<V8PersistentCallbackInterface<V8VoidCallback>> callback_;
- };
-
- class OnDidSucceedPromiseImpl : public OnDidSucceedCallback {
- public:
- OnDidSucceedPromiseImpl(ScriptPromiseResolver*);
- void Trace(Visitor*) override;
- void OnSuccess(ExecutionContext*) override;
-
- private:
- Member<ScriptPromiseResolver> resolver_;
- };
-
- VoidCallbacks(OnDidSucceedCallback*,
- ErrorCallbackBase*,
+ VoidCallbacks(SuccessCallback,
+ ErrorCallback,
ExecutionContext*,
DOMFileSystemBase*);
@@ -442,7 +422,8 @@
void DidFail(base::File::Error error);
private:
- Persistent<OnDidSucceedCallback> success_callback_;
+ SuccessCallback success_callback_;
+ ErrorCallback error_callback_;
};
} // namespace blink
diff --git a/third_party/blink/renderer/modules/filesystem/file_system_directory_handle.cc b/third_party/blink/renderer/modules/filesystem/file_system_directory_handle.cc
index 62aabff..822dfa6 100644
--- a/third_party/blink/renderer/modules/filesystem/file_system_directory_handle.cc
+++ b/third_party/blink/renderer/modules/filesystem/file_system_directory_handle.cc
@@ -107,10 +107,13 @@
ScriptState* script_state) {
auto* resolver = ScriptPromiseResolver::Create(script_state);
ScriptPromise result = resolver->Promise();
- filesystem()->RemoveRecursively(
- this,
- MakeGarbageCollected<VoidCallbacks::OnDidSucceedPromiseImpl>(resolver),
- MakeGarbageCollected<PromiseErrorCallback>(resolver));
+
+ auto success_callback_wrapper =
+ AsyncCallbackHelper::VoidSuccessPromise(resolver);
+ auto error_callback_wrapper = AsyncCallbackHelper::ErrorPromise(resolver);
+
+ filesystem()->RemoveRecursively(this, std::move(success_callback_wrapper),
+ std::move(error_callback_wrapper));
return result;
}
