Skip to content

Commit

Permalink
Migrate URLLoaderWrapper to base::OnceCallback
Browse files Browse the repository at this point in the history 
Switches chrome_pdf::URLLoaderWrapper's API from pp::CompletionCallback
to base::OnceCallback. pp::CompletionCallback is still used internally
when calling Pepper APIs., but clients of URLLoaderWrapper no longer
need to deal with pp::CompletionCallback.

Using base::OnceCallback also simplifies resource management,
eliminating the need to manage references to pp::CompletionCallback.

With respect to thread safety, most Pepper APIs invoke asynchronous
callbacks on the original caller's thread, so there should be no new
thread safety issues. The base:: APIs also do sequence checking in
DCHECK mode, which should catch any unanticipated issues.

Bug: 1099022, 1101169
Change-Id: Ia355588b25b54a776ac972a7b5b2104b1a3b18c8
Cq-Do-Not-Cancel-Tryjobs: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2278169
Commit-Queue: K. Moon <kmoon@chromium.org>
Reviewed-by: Henrique Nakashima <hnakashima@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#785816}
  • Loading branch information
@kmoon-work
kmoon-work authored and Commit Bot committed Jul 7, 2020
1 parent 75b18e0 commit 544e095
Show file tree
Hide file tree
Showing 6 changed files with 74 additions and 72 deletions.
13 changes: 7 additions & 6 deletions pdf/document_loader_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
#include <algorithm>
#include <utility>

#include "base/bind.h"
#include "base/callback.h"
#include "base/check_op.h"
#include "base/notreached.h"
#include "base/numerics/safe_math.h"
Expand Down Expand Up @@ -63,8 +65,7 @@ void DocumentLoaderImpl::Chunk::Clear() {
chunk_data.reset();
}

DocumentLoaderImpl::DocumentLoaderImpl(Client* client)
: client_(client), loader_factory_(this) {}
DocumentLoaderImpl::DocumentLoaderImpl(Client* client) : client_(client) {}

DocumentLoaderImpl::~DocumentLoaderImpl() = default;

Expand Down Expand Up @@ -257,9 +258,9 @@ void DocumentLoaderImpl::ContinueDownload() {

loader_ = client_->CreateURLLoader();

loader_->OpenRange(
url_, url_, start, length,
loader_factory_.NewCallback(&DocumentLoaderImpl::DidOpenPartial));
loader_->OpenRange(url_, url_, start, length,
base::BindOnce(&DocumentLoaderImpl::DidOpenPartial,
weak_factory_.GetWeakPtr()));
}

void DocumentLoaderImpl::DidOpenPartial(int32_t result) {
Expand Down Expand Up @@ -298,7 +299,7 @@ void DocumentLoaderImpl::DidOpenPartial(int32_t result) {
void DocumentLoaderImpl::ReadMore() {
loader_->ReadResponseBody(
buffer_, sizeof(buffer_),
loader_factory_.NewCallback(&DocumentLoaderImpl::DidRead));
base::BindOnce(&DocumentLoaderImpl::DidRead, weak_factory_.GetWeakPtr()));
}

void DocumentLoaderImpl::DidRead(int32_t result) {
Expand Down
11 changes: 6 additions & 5 deletions pdf/document_loader_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,14 @@
#ifndef PDF_DOCUMENT_LOADER_IMPL_H_
#define PDF_DOCUMENT_LOADER_IMPL_H_

#include <stdint.h>

#include <memory>
#include <string>

#include "base/macros.h"
#include "base/memory/weak_ptr.h"
#include "pdf/chunk_stream.h"
#include "pdf/document_loader.h"
#include "ppapi/utility/completion_callback_factory.h"

namespace chrome_pdf {

Expand All @@ -21,6 +22,8 @@ class DocumentLoaderImpl : public DocumentLoader {
static constexpr uint32_t kDefaultRequestSize = 65536;

explicit DocumentLoaderImpl(Client* client);
DocumentLoaderImpl(const DocumentLoaderImpl&) = delete;
DocumentLoaderImpl& operator=(const DocumentLoaderImpl&) = delete;
~DocumentLoaderImpl() override;

// DocumentLoader:
Expand Down Expand Up @@ -75,8 +78,6 @@ class DocumentLoaderImpl : public DocumentLoader {
std::string url_;
std::unique_ptr<URLLoaderWrapper> loader_;

pp::CompletionCallbackFactory<DocumentLoaderImpl> loader_factory_;

DataStream chunk_stream_;
bool partial_loading_enabled_ = true;
bool is_partial_loader_active_ = false;
Expand All @@ -92,7 +93,7 @@ class DocumentLoaderImpl : public DocumentLoader {

uint32_t bytes_received_ = 0;

DISALLOW_COPY_AND_ASSIGN(DocumentLoaderImpl);
base::WeakPtrFactory<DocumentLoaderImpl> weak_factory_{this};
};

} // namespace chrome_pdf
Expand Down
32 changes: 18 additions & 14 deletions pdf/document_loader_impl_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,12 @@
#include <algorithm>
#include <memory>
#include <string>
#include <utility>
#include <vector>

#include "base/callback.h"
#include "base/check.h"
#include "pdf/ppapi_migration/callback.h"
#include "pdf/url_loader_wrapper.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
Expand Down Expand Up @@ -86,38 +89,38 @@ class TestURLLoader : public URLLoaderWrapper {
open_byte_range_ = open_byte_range;
}

bool IsWaitRead() const { return !did_read_callback_.IsOptional(); }
bool IsWaitOpen() const { return !did_open_callback_.IsOptional(); }
bool IsWaitRead() const { return !did_read_callback_.is_null(); }
bool IsWaitOpen() const { return !did_open_callback_.is_null(); }
char* buffer() const { return buffer_; }
int buffer_size() const { return buffer_size_; }

void SetReadCallback(const pp::CompletionCallback& read_callback,
void SetReadCallback(ResultCallback read_callback,
char* buffer,
int buffer_size) {
did_read_callback_ = read_callback;
did_read_callback_ = std::move(read_callback);
buffer_ = buffer;
buffer_size_ = buffer_size;
}

void SetOpenCallback(const pp::CompletionCallback& open_callback,
void SetOpenCallback(ResultCallback open_callback,
gfx::Range req_byte_range) {
did_open_callback_ = open_callback;
did_open_callback_ = std::move(open_callback);
set_open_byte_range(req_byte_range);
}

void CallOpenCallback(int result) {
DCHECK(IsWaitOpen());
did_open_callback_.RunAndClear(result);
std::move(did_open_callback_).Run(result);
}

void CallReadCallback(int result) {
DCHECK(IsWaitRead());
did_read_callback_.RunAndClear(result);
std::move(did_read_callback_).Run(result);
}

private:
pp::CompletionCallback did_open_callback_;
pp::CompletionCallback did_read_callback_;
ResultCallback did_open_callback_;
ResultCallback did_read_callback_;
char* buffer_ = nullptr;
int buffer_size_ = 0;

Expand Down Expand Up @@ -171,14 +174,15 @@ class TestURLLoader : public URLLoaderWrapper {
const std::string& referrer_url,
uint32_t position,
uint32_t size,
const pp::CompletionCallback& cc) override {
data_->SetOpenCallback(cc, gfx::Range(position, position + size));
ResultCallback callback) override {
data_->SetOpenCallback(std::move(callback),
gfx::Range(position, position + size));
}

void ReadResponseBody(char* buffer,
int buffer_size,
const pp::CompletionCallback& cc) override {
data_->SetReadCallback(cc, buffer, buffer_size);
ResultCallback callback) override {
data_->SetReadCallback(std::move(callback), buffer, buffer_size);
}

bool GetDownloadProgress(int64_t* bytes_received,
Expand Down
10 changes: 6 additions & 4 deletions pdf/url_loader_wrapper.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,11 @@
#ifndef PDF_URL_LOADER_WRAPPER_H_
#define PDF_URL_LOADER_WRAPPER_H_

#include <stdint.h>

#include <string>

#include "base/macros.h"
#include "ppapi/cpp/completion_callback.h"
#include "pdf/ppapi_migration/callback.h"

namespace chrome_pdf {

Expand Down Expand Up @@ -49,14 +50,15 @@ class URLLoaderWrapper {
const std::string& referrer_url,
uint32_t position,
uint32_t size,
const pp::CompletionCallback& cc) = 0;
ResultCallback callback) = 0;

// Read the response body. The size of the buffer must be large enough to
// hold the specified number of bytes to read.
// This function might perform a partial read.
virtual void ReadResponseBody(char* buffer,
int buffer_size,
const pp::CompletionCallback& cc) = 0;
ResultCallback callback) = 0;

// Returns the current download progress.
// Progress only refers to the response body and does not include the headers.
// If false, progress is unknown, bytes_received/total_bytes_to_be_received
Expand Down
56 changes: 25 additions & 31 deletions pdf/url_loader_wrapper_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,16 @@
#include "pdf/url_loader_wrapper_impl.h"

#include <memory>
#include <utility>

#include "base/bind.h"
#include "base/callback.h"
#include "base/check_op.h"
#include "base/strings/string_util.h"
#include "base/strings/stringprintf.h"
#include "net/http/http_util.h"
#include "ppapi/c/pp_errors.h"
#include "ppapi/cpp/completion_callback.h"
#include "ppapi/cpp/logging.h"
#include "ppapi/cpp/url_request_info.h"
#include "ppapi/cpp/url_response_info.h"
Expand Down Expand Up @@ -99,21 +102,12 @@ bool IsDoubleEndLineAtEnd(const char* buffer, int size) {

URLLoaderWrapperImpl::URLLoaderWrapperImpl(pp::Instance* plugin_instance,
const pp::URLLoader& url_loader)
: plugin_instance_(plugin_instance),
url_loader_(url_loader),
callback_factory_(this) {
: plugin_instance_(plugin_instance), url_loader_(url_loader) {
SetHeadersFromLoader();
}

URLLoaderWrapperImpl::~URLLoaderWrapperImpl() {
Close();
// We should call callbacks to prevent memory leaks.
// The callbacks don't do anything, because the objects that created the
// callbacks have been destroyed.
if (!did_open_callback_.IsOptional())
did_open_callback_.RunAndClear(-1);
if (!did_read_callback_.IsOptional())
did_read_callback_.RunAndClear(-1);
}

int URLLoaderWrapperImpl::GetContentLength() const {
Expand Down Expand Up @@ -165,35 +159,35 @@ void URLLoaderWrapperImpl::OpenRange(const std::string& url,
const std::string& referrer_url,
uint32_t position,
uint32_t size,
const pp::CompletionCallback& cc) {
did_open_callback_ = cc;
pp::CompletionCallback callback =
callback_factory_.NewCallback(&URLLoaderWrapperImpl::DidOpen);
ResultCallback callback) {
pp::CompletionCallback cc = PPCompletionCallbackFromResultCallback(
base::BindOnce(&URLLoaderWrapperImpl::DidOpen, weak_factory_.GetWeakPtr(),
std::move(callback)));
int rv = url_loader_.Open(
MakeRangeRequest(plugin_instance_, url, referrer_url, position, size),
callback);
cc);
if (rv != PP_OK_COMPLETIONPENDING)
callback.Run(rv);
cc.Run(rv);
}

void URLLoaderWrapperImpl::ReadResponseBody(char* buffer,
int buffer_size,
const pp::CompletionCallback& cc) {
did_read_callback_ = cc;
ResultCallback callback) {
buffer_ = buffer;
buffer_size_ = buffer_size;
read_starter_.Start(
FROM_HERE, kReadDelayMs,
base::BindOnce(&URLLoaderWrapperImpl::ReadResponseBodyImpl,
base::Unretained(this)));
base::Unretained(this), std::move(callback)));
}

void URLLoaderWrapperImpl::ReadResponseBodyImpl() {
pp::CompletionCallback callback =
callback_factory_.NewCallback(&URLLoaderWrapperImpl::DidRead);
int rv = url_loader_.ReadResponseBody(buffer_, buffer_size_, callback);
void URLLoaderWrapperImpl::ReadResponseBodyImpl(ResultCallback callback) {
pp::CompletionCallback cc = PPCompletionCallbackFromResultCallback(
base::BindOnce(&URLLoaderWrapperImpl::DidRead, weak_factory_.GetWeakPtr(),
std::move(callback)));
int rv = url_loader_.ReadResponseBody(buffer_, buffer_size_, cc);
if (rv != PP_OK_COMPLETIONPENDING) {
callback.Run(rv);
cc.Run(rv);
}
}

Expand Down Expand Up @@ -255,12 +249,12 @@ void URLLoaderWrapperImpl::ParseHeaders() {
}
}

void URLLoaderWrapperImpl::DidOpen(int32_t result) {
void URLLoaderWrapperImpl::DidOpen(ResultCallback callback, int32_t result) {
SetHeadersFromLoader();
did_open_callback_.RunAndClear(result);
std::move(callback).Run(result);
}

void URLLoaderWrapperImpl::DidRead(int32_t result) {
void URLLoaderWrapperImpl::DidRead(ResultCallback callback, int32_t result) {
if (multi_part_processed_) {
// Reset this flag so we look inside the buffer in calls of DidRead for this
// response only once. Note that this code DOES NOT handle multi part
Expand All @@ -269,12 +263,12 @@ void URLLoaderWrapperImpl::DidRead(int32_t result) {
is_multipart_ = false;
}
if (result <= 0 || !is_multipart_) {
did_read_callback_.RunAndClear(result);
std::move(callback).Run(result);
return;
}
if (result <= 2) {
// TODO(art-snake): Accumulate data for parse headers.
did_read_callback_.RunAndClear(result);
std::move(callback).Run(result);
return;
}

Expand All @@ -297,12 +291,12 @@ void URLLoaderWrapperImpl::DidRead(int32_t result) {
result = length;
if (result == 0) {
// Continue receiving.
return ReadResponseBodyImpl();
return ReadResponseBodyImpl(std::move(callback));
}
DCHECK_GT(result, 0);
memmove(buffer_, start, result);

did_read_callback_.RunAndClear(result);
std::move(callback).Run(result);
}

void URLLoaderWrapperImpl::SetHeadersFromLoader() {
Expand Down

0 comments on commit 544e095

Please sign in to comment.