Skip to content

Commit

Permalink
Pepper: Remove Will* from PPB_FileIOTrusted.
Browse files Browse the repository at this point in the history 
Nobody uses these methods today. Additionally, WillWrite/WillSetLength are no
good for plugin-side writes. They require an extra IPC round-trip that has to
be made for every write call. I don't see any advantage of this over a plugin
just calling Write()/SetLength() directly and getting a NOQUOTA error back.

I'm removing these methods to clean up code a bit and make the FileIO move from
the renderer to the browser smaller.

R=bbudge@chromium.org, dmichael@chromium.org, kinuko@chromium.org
TBR=jschuh
BUG=246396

Review URL: https://codereview.chromium.org/24615002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@225472 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
teravest@chromium.org committed Sep 26, 2013
1 parent 8e7415e commit ca1d0ca
Show file tree
Hide file tree
Showing 17 changed files with 68 additions and 447 deletions.
8 changes: 0 additions & 8 deletions chrome/test/ppapi/ppapi_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -781,7 +781,6 @@ IN_PROC_BROWSER_TEST_F(PPAPITest, FileIO) {
LIST_TEST(FileIO_ReadWriteSetLength)
LIST_TEST(FileIO_ReadToArrayWriteSetLength)
LIST_TEST(FileIO_TouchQuery)
LIST_TEST(FileIO_WillWriteWillSetLength)
LIST_TEST(FileIO_RequestOSFileHandle)
LIST_TEST(FileIO_RequestOSFileHandleWithOpenExclusive)
LIST_TEST(FileIO_Mmap)
Expand All @@ -797,7 +796,6 @@ IN_PROC_BROWSER_TEST_F(OutOfProcessPPAPITest, FileIO) {
LIST_TEST(FileIO_ReadWriteSetLength)
LIST_TEST(FileIO_ReadToArrayWriteSetLength)
LIST_TEST(FileIO_TouchQuery)
LIST_TEST(FileIO_WillWriteWillSetLength)
LIST_TEST(FileIO_RequestOSFileHandle)
LIST_TEST(FileIO_RequestOSFileHandleWithOpenExclusive)
LIST_TEST(FileIO_Mmap)
Expand All @@ -813,8 +811,6 @@ IN_PROC_BROWSER_TEST_F(PPAPINaClNewlibTest, FileIO) {
LIST_TEST(FileIO_ReadWriteSetLength)
LIST_TEST(FileIO_ReadToArrayWriteSetLength)
LIST_TEST(FileIO_TouchQuery)
// The following test requires PPB_FileIO_Trusted, not available in NaCl.
LIST_TEST(DISABLED_FileIO_WillWriteWillSetLength)
LIST_TEST(FileIO_RequestOSFileHandle)
LIST_TEST(FileIO_RequestOSFileHandleWithOpenExclusive)
LIST_TEST(FileIO_Mmap)
Expand All @@ -830,8 +826,6 @@ IN_PROC_BROWSER_TEST_F(PPAPINaClGLibcTest, MAYBE_GLIBC(FileIO)) {
LIST_TEST(FileIO_ReadWriteSetLength)
LIST_TEST(FileIO_ReadToArrayWriteSetLength)
LIST_TEST(FileIO_TouchQuery)
// The following test requires PPB_FileIO_Trusted, not available in NaCl.
LIST_TEST(DISABLED_FileIO_WillWriteWillSetLength)
LIST_TEST(FileIO_RequestOSFileHandle)
LIST_TEST(FileIO_RequestOSFileHandleWithOpenExclusive)
LIST_TEST(FileIO_Mmap)
Expand All @@ -847,8 +841,6 @@ IN_PROC_BROWSER_TEST_F(PPAPINaClPNaClTest, FileIO) {
LIST_TEST(FileIO_ReadWriteSetLength)
LIST_TEST(FileIO_ReadToArrayWriteSetLength)
LIST_TEST(FileIO_TouchQuery)
// The following test requires PPB_FileIO_Trusted, not available in NaCl.
LIST_TEST(DISABLED_FileIO_WillWriteWillSetLength)
LIST_TEST(FileIO_RequestOSFileHandle)
LIST_TEST(FileIO_RequestOSFileHandleWithOpenExclusive)
LIST_TEST(FileIO_Mmap)
Expand Down
49 changes: 0 additions & 49 deletions content/renderer/pepper/pepper_file_io_host.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,10 +166,6 @@ int32_t PepperFileIOHost::OnResourceMessageReceived(
OnHostMsgFlush)
PPAPI_DISPATCH_HOST_RESOURCE_CALL_0(PpapiHostMsg_FileIO_Close,
OnHostMsgClose)
PPAPI_DISPATCH_HOST_RESOURCE_CALL(PpapiHostMsg_FileIO_WillWrite,
OnHostMsgWillWrite)
PPAPI_DISPATCH_HOST_RESOURCE_CALL(PpapiHostMsg_FileIO_WillSetLength,
OnHostMsgWillSetLength)
PPAPI_DISPATCH_HOST_RESOURCE_CALL_0(PpapiHostMsg_FileIO_GetOSFileDescriptor,
OnHostMsgGetOSFileDescriptor)
PPAPI_DISPATCH_HOST_RESOURCE_CALL_0(PpapiHostMsg_FileIO_RequestOSFileHandle,
Expand Down Expand Up @@ -395,51 +391,6 @@ int32_t PepperFileIOHost::OnHostMsgClose(
return PP_OK;
}

int32_t PepperFileIOHost::OnHostMsgWillWrite(
ppapi::host::HostMessageContext* context,
int64_t offset,
int32_t bytes_to_write) {
int32_t rv = state_manager_.CheckOperationState(
FileIOStateManager::OPERATION_EXCLUSIVE, true);
if (rv != PP_OK)
return rv;

if (!quota_file_io_)
return PP_OK;

if (!quota_file_io_->WillWrite(
offset, bytes_to_write,
base::Bind(&PepperFileIOHost::ExecutePlatformWriteCallback,
weak_factory_.GetWeakPtr(),
context->MakeReplyMessageContext())))
return PP_ERROR_FAILED;

state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_EXCLUSIVE);
return PP_OK_COMPLETIONPENDING;
}

int32_t PepperFileIOHost::OnHostMsgWillSetLength(
ppapi::host::HostMessageContext* context,
int64_t length) {
int32_t rv = state_manager_.CheckOperationState(
FileIOStateManager::OPERATION_EXCLUSIVE, true);
if (rv != PP_OK)
return rv;

if (!quota_file_io_)
return PP_OK;

if (!quota_file_io_->WillSetLength(
length,
base::Bind(&PepperFileIOHost::ExecutePlatformGeneralCallback,
weak_factory_.GetWeakPtr(),
context->MakeReplyMessageContext())))
return PP_ERROR_FAILED;

state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_EXCLUSIVE);
return PP_OK_COMPLETIONPENDING;
}

int32_t PepperFileIOHost::OnHostMsgRequestOSFileHandle(
ppapi::host::HostMessageContext* context) {
if (!is_running_in_process_ &&
Expand Down
5 changes: 0 additions & 5 deletions content/renderer/pepper/pepper_file_io_host.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,6 @@ class PepperFileIOHost : public ppapi::host::ResourceHost,
// Trusted API.
int32_t OnHostMsgGetOSFileDescriptor(
ppapi::host::HostMessageContext* context);
int32_t OnHostMsgWillWrite(ppapi::host::HostMessageContext* context,
int64_t offset,
int32_t bytes_to_write);
int32_t OnHostMsgWillSetLength(ppapi::host::HostMessageContext* context,
int64_t length);

// Callback handlers. These mostly convert the PlatformFileError to the
// PP_Error code and send back the reply. Note that the argument
Expand Down
48 changes: 9 additions & 39 deletions content/renderer/pepper/quota_file_io.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,38 +51,33 @@ class QuotaFileIO::PendingOperationBase {
virtual void DidFail(PlatformFileError error) = 0;

protected:
PendingOperationBase(QuotaFileIO* quota_io, bool is_will_operation)
: quota_io_(quota_io), is_will_operation_(is_will_operation) {
explicit PendingOperationBase(QuotaFileIO* quota_io) : quota_io_(quota_io) {
DCHECK(quota_io_);
quota_io_->WillUpdate();
}

QuotaFileIO* quota_io_;
const bool is_will_operation_;
};

class QuotaFileIO::WriteOperation : public PendingOperationBase {
public:
WriteOperation(QuotaFileIO* quota_io,
bool is_will_operation,
int64_t offset,
const char* buffer,
int32_t bytes_to_write,
const WriteCallback& callback)
: PendingOperationBase(quota_io, is_will_operation),
: PendingOperationBase(quota_io),
offset_(offset),
bytes_to_write_(bytes_to_write),
callback_(callback),
finished_(false),
status_(base::PLATFORM_FILE_OK),
bytes_written_(0),
weak_factory_(this) {
if (!is_will_operation) {
// TODO(kinuko): Check the API convention if we really need to keep a copy
// of the buffer during the async write operations.
buffer_.reset(new char[bytes_to_write]);
memcpy(buffer_.get(), buffer, bytes_to_write);
}
// TODO(kinuko): Check the API convention if we really need to keep a copy
// of the buffer during the async write operations.
buffer_.reset(new char[bytes_to_write]);
memcpy(buffer_.get(), buffer, bytes_to_write);
}
virtual ~WriteOperation() {}
virtual void Run() OVERRIDE {
Expand All @@ -91,11 +86,6 @@ class QuotaFileIO::WriteOperation : public PendingOperationBase {
DidFail(base::PLATFORM_FILE_ERROR_NO_SPACE);
return;
}
if (is_will_operation_) {
// Assuming the write will succeed.
DidFinish(base::PLATFORM_FILE_OK, bytes_to_write_);
return;
}
DCHECK(buffer_.get());

if (!base::PostTaskAndReplyWithResult(
Expand Down Expand Up @@ -161,10 +151,9 @@ class QuotaFileIO::WriteOperation : public PendingOperationBase {
class QuotaFileIO::SetLengthOperation : public PendingOperationBase {
public:
SetLengthOperation(QuotaFileIO* quota_io,
bool is_will_operation,
int64_t length,
const StatusCallback& callback)
: PendingOperationBase(quota_io, is_will_operation),
: PendingOperationBase(quota_io),
length_(length),
callback_(callback),
weak_factory_(this) {}
Expand All @@ -177,10 +166,6 @@ class QuotaFileIO::SetLengthOperation : public PendingOperationBase {
DidFail(base::PLATFORM_FILE_ERROR_NO_SPACE);
return;
}
if (is_will_operation_) {
DidFinish(base::PLATFORM_FILE_OK);
return;
}

if (!base::FileUtilProxy::Truncate(
quota_io_->delegate()->GetFileThreadMessageLoopProxy().get(),
Expand Down Expand Up @@ -247,28 +232,13 @@ bool QuotaFileIO::Write(
return false;

WriteOperation* op = new WriteOperation(
this, false, offset, buffer, bytes_to_write, callback);
this, offset, buffer, bytes_to_write, callback);
return RegisterOperationForQuotaChecks(op);
}

bool QuotaFileIO::SetLength(int64_t length, const StatusCallback& callback) {
DCHECK(pending_operations_.empty());
SetLengthOperation* op = new SetLengthOperation(
this, false, length, callback);
return RegisterOperationForQuotaChecks(op);
}

bool QuotaFileIO::WillWrite(
int64_t offset, int32_t bytes_to_write, const WriteCallback& callback) {
WriteOperation* op = new WriteOperation(
this, true, offset, NULL, bytes_to_write, callback);
return RegisterOperationForQuotaChecks(op);
}

bool QuotaFileIO::WillSetLength(int64_t length,
const StatusCallback& callback) {
DCHECK(pending_operations_.empty());
SetLengthOperation* op = new SetLengthOperation(this, true, length, callback);
SetLengthOperation* op = new SetLengthOperation(this, length, callback);
return RegisterOperationForQuotaChecks(op);
}

Expand Down
10 changes: 2 additions & 8 deletions content/renderer/pepper/quota_file_io.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,21 +62,15 @@ class QuotaFileIO {
// Otherwise it returns false and |callback| will not be dispatched.
// |callback| will not be dispatched either when this instance is
// destroyed before the operation completes.
// SetLength/WillSetLength cannot be called while there're any in-flight
// operations. For Write/WillWrite it is guaranteed that |callback| are
// SetLength cannot be called while there're any in-flight
// operations. For Write it is guaranteed that |callback| are
// always dispatched in the same order as Write being called.
CONTENT_EXPORT bool Write(int64_t offset,
const char* buffer,
int32_t bytes_to_write,
const WriteCallback& callback);
CONTENT_EXPORT bool WillWrite(int64_t offset,
int32_t bytes_to_write,
const WriteCallback& callback);

CONTENT_EXPORT bool SetLength(int64_t length,
const StatusCallback& callback);
CONTENT_EXPORT bool WillSetLength(int64_t length,
const StatusCallback& callback);

Delegate* delegate() const { return delegate_.get(); }

Expand Down
Loading

0 comments on commit ca1d0ca

Please sign in to comment.