feat: add getUploadProgress API to the net API #13783
Conversation
| } | ||
|
|
||
| atom_request_->GetUploadProgress(progress); | ||
| return progress; |
There was a problem hiding this comment.
(minor) This control flow is a little awkward; e.g. the use of an else block after a return. Lines 135 and 138 go together.
More readable flow:
mate::Dictionary progress = ...
if (atom_request_) {
progress.Set("active", true);
atom_request_->GetUploadProgress(progress);
} else {
progress.Set("active", false);
}
return progress;
atom/browser/net/atom_url_request.h
Outdated
| @@ -43,6 +43,7 @@ class AtomURLRequest : public base::RefCountedThreadSafe<AtomURLRequest>, | |||
| void PassLoginInformation(const base::string16& username, | |||
| const base::string16& password) const; | |||
| void SetLoadFlags(int flags) const; | |||
| void GetUploadProgress(mate::Dictionary& progress); | |||
There was a problem hiding this comment.
URLRequest::GetUploadProgress() is const, so I think the compiler should be happy with making this new method const
docs/api/client-request.md
Outdated
|
|
||
| * `active` Boolean - Whether the request is currently active, if this is false | ||
| no other properties will be set | ||
| * `started` Boolean - Whether the upload has started, if this is false both |
There was a problem hiding this comment.
(minor) In the two lines above, ", if" should be ". If"
atom/browser/net/atom_url_request.cc
Outdated
| auto upload_progress = request_->GetUploadProgress(); | ||
| progress.Set("current", upload_progress.position()); | ||
| progress.Set("total", upload_progress.size()); | ||
| } |
There was a problem hiding this comment.
since mate::Dictionary::Set() is a template method, it looks like here we're sometimes setting current/total to int and sometimes to uint64_t. At first read I think that will make the former invoke Converter::ToV8(isolate,int32_t) and the latter Converter::ToV8(uint64_t). Does that matter?
If that matters, we could consistently use net::UploadProgress' types:
net::UploadProgress upload_progress;
if (request_) {
progress.set("started", true);
upload_progress = request_->GetUploadProgress();
} else {
progress.set("started", false);
}
progress.set("current", upload_progress.position());
progress.set("total", upload_progress.size());There was a problem hiding this comment.
Not sure it matters, but the consistent code looks nicer anyway 👍
|
@ckerr feedback addressed 👍 |
MarshallOfSound
force-pushed
the
net/upload-progress
branch
from
2d5e8d2
to
fe383c9
Compare
|
Why not add |
|
@YurySolovyov Because there is no progress event in chromium and I'd rather not make that API / performance decision for people. |
|
There is UploadProgressTracker, not sure how suitable it is though |
|
@YurySolovyov That helper is basically a setInterval watching the value but less customizable. I'm happy to implement that as an event in the future. But the objective of this PR is to be as safe as possible to make it easier to send back to a minor release. |
|
@MarshallOfSound |
|
@YurySolovyov I'm perfectly happy to watch by default in |
|
I still agree/think that progress watching should be opt-in |
|
/trop run backport-to 2-1-x |
|
The backport process for this PR has been manually initiated, sending your 1's and 0's to "2-1-x" here we go! :D |
|
We have automatically backported this PR to "2-1-x", please check out #13985 |
|
/trop run backport-to 3-0-x |
|
The backport process for this PR has been manually initiated, |
|
We have automatically backported this PR to "3-0-x", please check out #14446 |
As the title says, added a new method to the
netAPI to get the upload progress of requests. This is required because we can't use streaming to get upload progress (unlike nodes http module) because Chromium handles the backpressure / buffering.