Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2546653003:
Factor out upload progress handling from AsyncResourceHandler (Closed)
DescriptionFactor out upload progress handling from AsyncResourceHandler
This CL extracts the upload progress handling logic in AsyncResourceHandler
into UploadProgressTracker, so that we can reuse it in MojoAsyncResourceHandler.
BUG=603396
TBR=jam@chromium.org
Committed: https://crrev.com/a691461098c542dc098995edc9779a9b1a6aca07
Cr-Commit-Position: refs/heads/master@{#438814}
Patch Set 1 #Patch Set 2 : . #
Total comments: 10
Patch Set 3 : . #Patch Set 4 : +Start, +OnUploadCompleted, -ForceReportUploadProgress #
Total comments: 2
Patch Set 5 : -#include. +final upload progress in OnResponseCompleted #
Total comments: 10
Patch Set 6 : . #
Total comments: 10
Patch Set 7 : rebase #Patch Set 8 : . #
Dependent Patchsets: Messages
Total messages: 61 (41 generated)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Factor out upload progress handling from AsyncResourceHandler BUG= ========== to ========== Factor out upload progress handling from AsyncResourceHandler This CL extracts the upload progress handling logic in AsyncResourceHandler into UploadProgressTracker, so that we can reuse it in MojoAsyncResourceHandler. TODO: comments & tests BUG= ==========
tzik@chromium.org changed reviewers: + yhirano@chromium.org
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL
Description was changed from ========== Factor out upload progress handling from AsyncResourceHandler This CL extracts the upload progress handling logic in AsyncResourceHandler into UploadProgressTracker, so that we can reuse it in MojoAsyncResourceHandler. TODO: comments & tests BUG= ========== to ========== Factor out upload progress handling from AsyncResourceHandler This CL extracts the upload progress handling logic in AsyncResourceHandler into UploadProgressTracker, so that we can reuse it in MojoAsyncResourceHandler. BUG= ==========
https://codereview.chromium.org/2546653003/diff/20001/content/browser/loader/... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/2546653003/diff/20001/content/browser/loader/... content/browser/loader/async_resource_handler.cc:383: bool AsyncResourceHandler::OnReadCompleted(int bytes_read, bool* defer) { Is it good to nullify upload_progress_tracker_ in this function? https://codereview.chromium.org/2546653003/diff/20001/content/browser/loader/... File content/browser/loader/upload_progress_tracker.cc (right): https://codereview.chromium.org/2546653003/diff/20001/content/browser/loader/... content/browser/loader/upload_progress_tracker.cc:31: waiting_for_upload_progress_ack_ = true; false? https://codereview.chromium.org/2546653003/diff/20001/content/browser/loader/... File content/browser/loader/upload_progress_tracker.h (right): https://codereview.chromium.org/2546653003/diff/20001/content/browser/loader/... content/browser/loader/upload_progress_tracker.h:25: class UploadProgressTracker final { Plase write some comments. https://codereview.chromium.org/2546653003/diff/20001/content/browser/loader/... content/browser/loader/upload_progress_tracker.h:44: bool waiting_for_upload_progress_ack_ = false; This is not used.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2546653003/diff/20001/content/browser/loader/... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/2546653003/diff/20001/content/browser/loader/... content/browser/loader/async_resource_handler.cc:383: bool AsyncResourceHandler::OnReadCompleted(int bytes_read, bool* defer) { On 2016/12/02 09:17:52, yhirano wrote: > Is it good to nullify upload_progress_tracker_ in this function? OnResponseStarted should clear it before OnReadComplete. Let me add a DCHECK here for that. https://codereview.chromium.org/2546653003/diff/20001/content/browser/loader/... File content/browser/loader/upload_progress_tracker.cc (right): https://codereview.chromium.org/2546653003/diff/20001/content/browser/loader/... content/browser/loader/upload_progress_tracker.cc:31: waiting_for_upload_progress_ack_ = true; On 2016/12/02 09:17:52, yhirano wrote: > false? Done. https://codereview.chromium.org/2546653003/diff/20001/content/browser/loader/... File content/browser/loader/upload_progress_tracker.h (right): https://codereview.chromium.org/2546653003/diff/20001/content/browser/loader/... content/browser/loader/upload_progress_tracker.h:25: class UploadProgressTracker final { On 2016/12/02 09:17:52, yhirano wrote: > Plase write some comments. Done. https://codereview.chromium.org/2546653003/diff/20001/content/browser/loader/... content/browser/loader/upload_progress_tracker.h:44: bool waiting_for_upload_progress_ack_ = false; On 2016/12/02 09:17:52, yhirano wrote: > This is not used. Oops. Fixed. Now, it's used.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2546653003/diff/20001/content/browser/loader/... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/2546653003/diff/20001/content/browser/loader/... content/browser/loader/async_resource_handler.cc:383: bool AsyncResourceHandler::OnReadCompleted(int bytes_read, bool* defer) { On 2016/12/05 08:00:06, tzik wrote: > On 2016/12/02 09:17:52, yhirano wrote: > > Is it good to nullify upload_progress_tracker_ in this function? > > OnResponseStarted should clear it before OnReadComplete. Let me add a DCHECK > here for that. Oops, sorry, I wanted to comment at OnResponseCompleted, not OnReadCompleted. You don't have to put a DCHECK here. OnResponseCompleted can be called without OnResponseStarted call, and nullify upload_progress_tracker_ there may be good, I think. https://codereview.chromium.org/2546653003/diff/60001/content/browser/loader/... File content/browser/loader/async_resource_handler.h (right): https://codereview.chromium.org/2546653003/diff/60001/content/browser/loader/... content/browser/loader/async_resource_handler.h:14: #include "base/timer/timer.h" not needed
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2546653003/diff/20001/content/browser/loader/... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/2546653003/diff/20001/content/browser/loader/... content/browser/loader/async_resource_handler.cc:383: bool AsyncResourceHandler::OnReadCompleted(int bytes_read, bool* defer) { On 2016/12/07 13:50:03, yhirano wrote: > On 2016/12/05 08:00:06, tzik wrote: > > On 2016/12/02 09:17:52, yhirano wrote: > > > Is it good to nullify upload_progress_tracker_ in this function? > > > > OnResponseStarted should clear it before OnReadComplete. Let me add a DCHECK > > here for that. > > Oops, sorry, I wanted to comment at OnResponseCompleted, not OnReadCompleted. > You don't have to put a DCHECK here. OnResponseCompleted can be called without > OnResponseStarted call, and nullify upload_progress_tracker_ there may be good, > I think. That makes sense. Done. https://codereview.chromium.org/2546653003/diff/60001/content/browser/loader/... File content/browser/loader/async_resource_handler.h (right): https://codereview.chromium.org/2546653003/diff/60001/content/browser/loader/... content/browser/loader/async_resource_handler.h:14: #include "base/timer/timer.h" On 2016/12/07 13:50:03, yhirano wrote: > not needed Done.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
tzik@chromium.org changed reviewers: + mmenke@chromium.org
tzik@chromium.org changed reviewers: + jam@chromium.org
Description was changed from ========== Factor out upload progress handling from AsyncResourceHandler This CL extracts the upload progress handling logic in AsyncResourceHandler into UploadProgressTracker, so that we can reuse it in MojoAsyncResourceHandler. BUG= ========== to ========== Factor out upload progress handling from AsyncResourceHandler This CL extracts the upload progress handling logic in AsyncResourceHandler into UploadProgressTracker, so that we can reuse it in MojoAsyncResourceHandler. BUG=603396 ==========
Adding mmenke and jam for OWNER review. PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2546653003/diff/80001/content/browser/loader/... File content/browser/loader/DEPS (right): https://codereview.chromium.org/2546653003/diff/80001/content/browser/loader/... content/browser/loader/DEPS:23: "+content/browser/loader/upload_progress_tracker.h", Need to add deps entry for upload_progress_tracker.(cc|h). Perhaps we should be separating out the files that are legal to depend on into another (sub)directory (Or those that aren't). Then we don't need a per-file whitelist of stuff in content/browser/loader/. Not suggesting it for this CL. https://codereview.chromium.org/2546653003/diff/80001/content/browser/loader/... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/2546653003/diff/80001/content/browser/loader/... content/browser/loader/async_resource_handler.cc:451: // empty response body. This comment is false. The only cases we don't call OnResponseStarted are errors and cancellation. https://codereview.chromium.org/2546653003/diff/80001/content/browser/loader/... File content/browser/loader/upload_progress_tracker.cc (right): https://codereview.chromium.org/2546653003/diff/80001/content/browser/loader/... content/browser/loader/upload_progress_tracker.cc:22: DCHECK(report_progress_); include base/logging.h https://codereview.chromium.org/2546653003/diff/80001/content/browser/loader/... content/browser/loader/upload_progress_tracker.cc:33: &UploadProgressTracker::ReportUploadProgressIfNeeded); Just do this in the constructor? https://codereview.chromium.org/2546653003/diff/80001/content/browser/loader/... content/browser/loader/upload_progress_tracker.cc:34: UploadProgressTracker::ReportUploadProgressIfNeeded(); Why do we call this on start? We haven't uploaded any data yet, if the request hasn't started.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2546653003/diff/80001/content/browser/loader/... File content/browser/loader/DEPS (right): https://codereview.chromium.org/2546653003/diff/80001/content/browser/loader/... content/browser/loader/DEPS:23: "+content/browser/loader/upload_progress_tracker.h", On 2016/12/08 15:37:08, mmenke wrote: > Need to add deps entry for upload_progress_tracker.(cc|h). Perhaps we should be > separating out the files that are legal to depend on into another (sub)directory > (Or those that aren't). Then we don't need a per-file whitelist of stuff in > content/browser/loader/. Not suggesting it for this CL. Done. https://codereview.chromium.org/2546653003/diff/80001/content/browser/loader/... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/2546653003/diff/80001/content/browser/loader/... content/browser/loader/async_resource_handler.cc:451: // empty response body. On 2016/12/08 15:37:08, mmenke wrote: > This comment is false. The only cases we don't call OnResponseStarted are > errors and cancellation. Done. https://codereview.chromium.org/2546653003/diff/80001/content/browser/loader/... File content/browser/loader/upload_progress_tracker.cc (right): https://codereview.chromium.org/2546653003/diff/80001/content/browser/loader/... content/browser/loader/upload_progress_tracker.cc:22: DCHECK(report_progress_); On 2016/12/08 15:37:08, mmenke wrote: > include base/logging.h Done. https://codereview.chromium.org/2546653003/diff/80001/content/browser/loader/... content/browser/loader/upload_progress_tracker.cc:33: &UploadProgressTracker::ReportUploadProgressIfNeeded); On 2016/12/08 15:37:08, mmenke wrote: > Just do this in the constructor? Done. https://codereview.chromium.org/2546653003/diff/80001/content/browser/loader/... content/browser/loader/upload_progress_tracker.cc:34: UploadProgressTracker::ReportUploadProgressIfNeeded(); On 2016/12/08 15:37:08, mmenke wrote: > Why do we call this on start? We haven't uploaded any data yet, if the request > hasn't started. It's just to preserve previous behavior. Seems like this is always nop, let me remove this now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Sorry for the unreasonable delay. Took Monday off, but I should have gotten to this one last week. Looking now.
https://codereview.chromium.org/2546653003/diff/100001/content/browser/loader... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/2546653003/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler.cc:75: int request_id, Hrm...Any reason to prefer this over just making this a method of AsyncResourceHandler, and grabbing filter/request ID as needed? They shouldn't change, but I don't like to duplicate information, particularly when one of the locations is hidden in a callback. https://codereview.chromium.org/2546653003/diff/100001/content/browser/loader... File content/browser/loader/async_resource_handler.h (right): https://codereview.chromium.org/2546653003/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler.h:8: #include <stdint.h> no longer needed, unless you follow my suggestion of making the callback an AsyncResourceHandler method. https://codereview.chromium.org/2546653003/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler.h:90: std::unique_ptr<UploadProgressTracker> upload_progress_tracker_; include <memory> https://codereview.chromium.org/2546653003/diff/100001/content/browser/loader... File content/browser/loader/upload_progress_tracker.cc (right): https://codereview.chromium.org/2546653003/diff/100001/content/browser/loader... content/browser/loader/upload_progress_tracker.cc:19: const tracked_objects::Location& location, Do we really get anything from taking this as an argument? https://codereview.chromium.org/2546653003/diff/100001/content/browser/loader... File content/browser/loader/upload_progress_tracker.h (right): https://codereview.chromium.org/2546653003/diff/100001/content/browser/loader... content/browser/loader/upload_progress_tracker.h:30: base::RepeatingCallback<void(int64_t, int64_t)>; include <stdint.h>
Oh, and LGTM
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2546653003/diff/100001/content/browser/loader... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/2546653003/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler.cc:75: int request_id, On 2016/12/13 18:29:46, mmenke wrote: > Hrm...Any reason to prefer this over just making this a method of > AsyncResourceHandler, and grabbing filter/request ID as needed? They shouldn't > change, but I don't like to duplicate information, particularly when one of the > locations is hidden in a callback. Converted it to a method. That's not strong preference, but I prefer functions over methods, since we can see better what object the function depends on. https://codereview.chromium.org/2546653003/diff/100001/content/browser/loader... File content/browser/loader/async_resource_handler.h (right): https://codereview.chromium.org/2546653003/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler.h:8: #include <stdint.h> On 2016/12/13 18:29:46, mmenke wrote: > no longer needed, unless you follow my suggestion of making the callback an > AsyncResourceHandler method. I did follow it, so now this is needed. https://codereview.chromium.org/2546653003/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler.h:90: std::unique_ptr<UploadProgressTracker> upload_progress_tracker_; On 2016/12/13 18:29:46, mmenke wrote: > include <memory> Done. https://codereview.chromium.org/2546653003/diff/100001/content/browser/loader... File content/browser/loader/upload_progress_tracker.cc (right): https://codereview.chromium.org/2546653003/diff/100001/content/browser/loader... content/browser/loader/upload_progress_tracker.cc:19: const tracked_objects::Location& location, On 2016/12/13 18:29:46, mmenke wrote: > Do we really get anything from taking this as an argument? Taking it from a param will leave slightly better info into the crash report. Since we'll use this class from both AsyncResourceHandler and MojoAsyncResourceHandler for a while, we will be able to distinguish which ResourceHandler is used by taking |location|. https://codereview.chromium.org/2546653003/diff/100001/content/browser/loader... File content/browser/loader/upload_progress_tracker.h (right): https://codereview.chromium.org/2546653003/diff/100001/content/browser/loader... content/browser/loader/upload_progress_tracker.h:30: base::RepeatingCallback<void(int64_t, int64_t)>; On 2016/12/13 18:29:46, mmenke wrote: > include <stdint.h> Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Factor out upload progress handling from AsyncResourceHandler This CL extracts the upload progress handling logic in AsyncResourceHandler into UploadProgressTracker, so that we can reuse it in MojoAsyncResourceHandler. BUG=603396 ========== to ========== Factor out upload progress handling from AsyncResourceHandler This CL extracts the upload progress handling logic in AsyncResourceHandler into UploadProgressTracker, so that we can reuse it in MojoAsyncResourceHandler. BUG=603396 TBD=jam@chromium.org ==========
TBRing jam for simple file additions to //content/browser/BUILD.gn.
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2546653003/#ps140001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Factor out upload progress handling from AsyncResourceHandler This CL extracts the upload progress handling logic in AsyncResourceHandler into UploadProgressTracker, so that we can reuse it in MojoAsyncResourceHandler. BUG=603396 TBD=jam@chromium.org ========== to ========== Factor out upload progress handling from AsyncResourceHandler This CL extracts the upload progress handling logic in AsyncResourceHandler into UploadProgressTracker, so that we can reuse it in MojoAsyncResourceHandler. BUG=603396 TBR=jam@chromium.org ==========
The CQ bit was checked by tzik@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481802996510170,
"parent_rev": "0ed6110896a2075c0fa045696b99aaec79a26b5d", "commit_rev":
"0b1976d889797d0854d4b89d0c5395ed0941dc8b"}
Message was sent while issue was closed.
Description was changed from ========== Factor out upload progress handling from AsyncResourceHandler This CL extracts the upload progress handling logic in AsyncResourceHandler into UploadProgressTracker, so that we can reuse it in MojoAsyncResourceHandler. BUG=603396 TBR=jam@chromium.org ========== to ========== Factor out upload progress handling from AsyncResourceHandler This CL extracts the upload progress handling logic in AsyncResourceHandler into UploadProgressTracker, so that we can reuse it in MojoAsyncResourceHandler. BUG=603396 TBR=jam@chromium.org Review-Url: https://codereview.chromium.org/2546653003 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Factor out upload progress handling from AsyncResourceHandler This CL extracts the upload progress handling logic in AsyncResourceHandler into UploadProgressTracker, so that we can reuse it in MojoAsyncResourceHandler. BUG=603396 TBR=jam@chromium.org Review-Url: https://codereview.chromium.org/2546653003 ========== to ========== Factor out upload progress handling from AsyncResourceHandler This CL extracts the upload progress handling logic in AsyncResourceHandler into UploadProgressTracker, so that we can reuse it in MojoAsyncResourceHandler. BUG=603396 TBR=jam@chromium.org Committed: https://crrev.com/a691461098c542dc098995edc9779a9b1a6aca07 Cr-Commit-Position: refs/heads/master@{#438814} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/a691461098c542dc098995edc9779a9b1a6aca07 Cr-Commit-Position: refs/heads/master@{#438814} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
