New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

protocol: provide blob uuid with uploadData #6941

Merged
merged 7 commits into from Sep 16, 2016

Conversation

Projects
None yet
2 participants
@deepak1556
Member

deepak1556 commented Aug 23, 2016

Also adds a generic way to access blob data in the main process from either public blob url or uuid.

Fixes #6894

Show outdated Hide outdated docs/api/session.md
@@ -504,6 +512,7 @@ The `uploadData` is an array of `data` objects:
* `data` Object
* `bytes` Buffer - Content being sent.
* `file` String - Path of file being uploaded.
* `blobUUID` String - UUID of blob data.

This comment has been minimized.

@zcbenz

zcbenz Aug 24, 2016

Contributor

It can probably be a blob object that has a method to get length and read the data.

EDIT: Due to how the webRequest API is implemented by sending objects through threads, we think it does not worth the efforts.

@zcbenz

zcbenz Aug 24, 2016

Contributor

It can probably be a blob object that has a method to get length and read the data.

EDIT: Due to how the webRequest API is implemented by sending objects through threads, we think it does not worth the efforts.

Show outdated Hide outdated atom/browser/api/atom_api_session.h
enum class CacheAction {
CLEAR,
STATS,
};
enum class BlobIdType {
PUBLIC_URL,

This comment has been minimized.

@zcbenz

zcbenz Aug 29, 2016

Contributor

Only supporting UUID should be enough, I don't think users would use the API to read from a blob URL.

@zcbenz

zcbenz Aug 29, 2016

Contributor

Only supporting UUID should be enough, I don't think users would use the API to read from a blob URL.

Show outdated Hide outdated atom/browser/api/atom_api_session.cc
BlobDataCallback callback;
BlobIdType type = BlobIdType::UUID;
if (!args->GetNext(&identifier)) {
args->ThrowError("Must pass uuid or public url");

This comment has been minimized.

@zcbenz

zcbenz Aug 29, 2016

Contributor

Errors should be passed in callback since the API requires passing a callback.

@zcbenz

zcbenz Aug 29, 2016

Contributor

Errors should be passed in callback since the API requires passing a callback.

Show outdated Hide outdated atom/browser/api/atom_api_session.cc
void DidReadBlobData(const scoped_refptr<net::IOBuffer>& blob_data,
const Session::BlobDataCallback& completion_callback,
int bytes_read) {
std::unique_ptr<base::BinaryValue> result(

This comment has been minimized.

@zcbenz

zcbenz Aug 29, 2016

Contributor

Converting BinaryValue to Buffer will trigger a copy of the data, which is unnecessary since the data is already copied here. A more efficient way is to use the copied data to construct Buffer directly.

An example can be found at chromium_src/chrome/browser/printing/print_preview_message_handler.cc.

@zcbenz

zcbenz Aug 29, 2016

Contributor

Converting BinaryValue to Buffer will trigger a copy of the data, which is unnecessary since the data is already copied here. A more efficient way is to use the copied data to construct Buffer directly.

An example can be found at chromium_src/chrome/browser/printing/print_preview_message_handler.cc.

Show outdated Hide outdated atom/browser/api/atom_api_session.cc
auto blob_reader = blob_data_handle->CreateReader(
file_system_context,
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE).get());
std::shared_ptr<storage::BlobReader>

This comment has been minimized.

@zcbenz

zcbenz Aug 29, 2016

Contributor

Moving the pointer from unique_ptr to shared_ptr usually means a bad design, you can add a helper class to put all blob reading methods there.

@zcbenz

zcbenz Aug 29, 2016

Contributor

Moving the pointer from unique_ptr to shared_ptr usually means a bad design, you can add a helper class to put all blob reading methods there.

@deepak1556

This comment has been minimized.

Show comment
Hide comment
@deepak1556

deepak1556 Sep 1, 2016

Member

Have made the changes, thanks!

Member

deepak1556 commented Sep 1, 2016

Have made the changes, thanks!

@zcbenz

This comment has been minimized.

Show comment
Hide comment
@zcbenz

zcbenz Sep 16, 2016

Contributor

👍

Contributor

zcbenz commented Sep 16, 2016

👍

@zcbenz zcbenz merged commit 9714348 into electron:master Sep 16, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment