Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2612903008:
Add UploadProgressTracker unittest (Closed)
DescriptionAdd UploadProgressTracker unittest
BUG=603396
TBR=kinuko@chromium.org
Review-Url: https://codereview.chromium.org/2612903008
Cr-Commit-Position: refs/heads/master@{#443537}
Committed: https://chromium.googlesource.com/chromium/src/+/6d2c4d88c7d326cd5e711b2d9710e9c4ebc19cf0
Patch Set 1 #
Total comments: 6
Patch Set 2 : +Client::GetCurrentTime #Patch Set 3 : -gmock #
Total comments: 32
Patch Set 4 : . #Patch Set 5 : +#include #
Total comments: 6
Patch Set 6 : +expectation for unreported small progress. #
Messages
Total messages: 43 (28 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 ========== Add UploadProgressTracker unittest BUG= ========== to ========== Add UploadProgressTracker unittest BUG=603396 ==========
tzik@chromium.org changed reviewers: + mmenke@chromium.org, yhirano@chromium.org
PTAL
On 2017/01/06 09:58:28, tzik wrote: > PTAL Is it common for a Client class to have GetUploadProgress()? I feel it a bit strange...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2612903008/diff/1/content/browser/loader/uplo... File content/browser/loader/upload_progress_tracker.cc (right): https://codereview.chromium.org/2612903008/diff/1/content/browser/loader/uplo... content/browser/loader/upload_progress_tracker.cc:28: progress_timer_.SetTaskRunner(std::move(task_runner)); Why are we not just using the current thread? You can add a message loop to the test by just instantiating a base::MessageLoop in the test fixture. https://codereview.chromium.org/2612903008/diff/1/content/browser/loader/uplo... File content/browser/loader/upload_progress_tracker.h (right): https://codereview.chromium.org/2612903008/diff/1/content/browser/loader/uplo... content/browser/loader/upload_progress_tracker.h:57: void SetLastUploadTicksForTesting(const base::TimeTicks& ticks) { Think this delves too much into the underlying code. It mucks up production code more, but I'd suggest: virtual GetCurrentTime() { return base::TimeTicks::Now(); } And have a test subclass override it. That's more independent of implementation. Others may disagree, though. https://codereview.chromium.org/2612903008/diff/1/content/browser/loader/uplo... File content/browser/loader/upload_progress_tracker_unittest.cc (right): https://codereview.chromium.org/2612903008/diff/1/content/browser/loader/uplo... content/browser/loader/upload_progress_tracker_unittest.cc:24: MOCK_METHOD2(ReportUploadProgress, void(int64_t, int64_t)); I'd recommend against using gmock - it generally makes code significantly more obscure, and adds a higher barried to working on Chrome. There are some cases where it's just much simpler to use it, but doesn't seem needed here.
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/2612903008/diff/1/content/browser/loader/uplo... File content/browser/loader/upload_progress_tracker.cc (right): https://codereview.chromium.org/2612903008/diff/1/content/browser/loader/uplo... content/browser/loader/upload_progress_tracker.cc:28: progress_timer_.SetTaskRunner(std::move(task_runner)); On 2017/01/06 15:54:53, mmenke wrote: > Why are we not just using the current thread? You can add a message loop to the > test by just instantiating a base::MessageLoop in the test fixture. That is for using TestSimpleTaskRunner::RunPendingTasks, which runs all pending tasks including delayed tasks. Since base::Timer posts the task as a delayed task and it posts the task repeatedly, I think other APIs to flush the message loop doesn't work here. https://codereview.chromium.org/2612903008/diff/1/content/browser/loader/uplo... File content/browser/loader/upload_progress_tracker.h (right): https://codereview.chromium.org/2612903008/diff/1/content/browser/loader/uplo... content/browser/loader/upload_progress_tracker.h:57: void SetLastUploadTicksForTesting(const base::TimeTicks& ticks) { On 2017/01/06 15:54:53, mmenke wrote: > Think this delves too much into the underlying code. It mucks up production > code more, but I'd suggest: > > virtual GetCurrentTime() { return base::TimeTicks::Now(); } > > And have a test subclass override it. That's more independent of > implementation. Others may disagree, though. Done. https://codereview.chromium.org/2612903008/diff/1/content/browser/loader/uplo... File content/browser/loader/upload_progress_tracker_unittest.cc (right): https://codereview.chromium.org/2612903008/diff/1/content/browser/loader/uplo... content/browser/loader/upload_progress_tracker_unittest.cc:24: MOCK_METHOD2(ReportUploadProgress, void(int64_t, int64_t)); On 2017/01/06 15:54:53, mmenke wrote: > I'd recommend against using gmock - it generally makes code significantly more > obscure, and adds a higher barried to working on Chrome. There are some cases > where it's just much simpler to use it, but doesn't seem needed here. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/10 06:54:42, tzik wrote: > https://codereview.chromium.org/2612903008/diff/1/content/browser/loader/uplo... > File content/browser/loader/upload_progress_tracker.cc (right): > > https://codereview.chromium.org/2612903008/diff/1/content/browser/loader/uplo... > content/browser/loader/upload_progress_tracker.cc:28: > progress_timer_.SetTaskRunner(std::move(task_runner)); > On 2017/01/06 15:54:53, mmenke wrote: > > Why are we not just using the current thread? You can add a message loop to > the > > test by just instantiating a base::MessageLoop in the test fixture. > > That is for using TestSimpleTaskRunner::RunPendingTasks, which runs all pending > tasks including delayed tasks. > Since base::Timer posts the task as a delayed task and it posts the task > repeatedly, I think other APIs to flush the message loop doesn't work here. > > https://codereview.chromium.org/2612903008/diff/1/content/browser/loader/uplo... > File content/browser/loader/upload_progress_tracker.h (right): > > https://codereview.chromium.org/2612903008/diff/1/content/browser/loader/uplo... > content/browser/loader/upload_progress_tracker.h:57: void > SetLastUploadTicksForTesting(const base::TimeTicks& ticks) { > On 2017/01/06 15:54:53, mmenke wrote: > > Think this delves too much into the underlying code. It mucks up production > > code more, but I'd suggest: > > > > virtual GetCurrentTime() { return base::TimeTicks::Now(); } > > > > And have a test subclass override it. That's more independent of > > implementation. Others may disagree, though. > > Done. > > https://codereview.chromium.org/2612903008/diff/1/content/browser/loader/uplo... > File content/browser/loader/upload_progress_tracker_unittest.cc (right): > > https://codereview.chromium.org/2612903008/diff/1/content/browser/loader/uplo... > content/browser/loader/upload_progress_tracker_unittest.cc:24: > MOCK_METHOD2(ReportUploadProgress, void(int64_t, int64_t)); > On 2017/01/06 15:54:53, mmenke wrote: > > I'd recommend against using gmock - it generally makes code significantly more > > obscure, and adds a higher barried to working on Chrome. There are some cases > > where it's just much simpler to use it, but doesn't seem needed here. > > Done. Apologies, don't think I'll get to this today. First thing on my slate for tomorrow.
Looks good, think these are my final comments. https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... File content/browser/loader/upload_progress_tracker.h (right): https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker.h:36: virtual net::UploadProgress GetUploadProgress() = 0; Since we're returning this (And not just by ref or by value), should be be including the header instead of forward declaring it? https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker.h:38: int64_t total_size) = 0; Should document these. Also, why doesn't ReporterUploadProgress just take a net::UploadProgress? https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker.h:58: virtual base::TimeTicks GetCurrentTime(); nit: Can be private (Subclasses can override private functions, they just can't call them (Unless they also override them, though not relevant here, anyways)) https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker.h:58: virtual base::TimeTicks GetCurrentTime(); Maybe add a comment that this can be overridden for testing? https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker.h:58: virtual base::TimeTicks GetCurrentTime(); const? https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... File content/browser/loader/upload_progress_tracker_unittest.cc (right): https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker_unittest.cc:27: total_size_ = total_size; Both of these two lines seems wrong. Seems like we should either do: EXPECT_EQ(current_position_, current_position); EXPECT_EQ(total_size_, total_size); And/or we should store them in separate values, that are not the same as the ones returned by GetUploadProgress. https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker_unittest.cc:61: base::TimeTicks GetCurrentTime() override { return current_time_; } Can be private https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker_unittest.cc:63: base::TimeTicks current_time_; Should be private. https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker_unittest.cc:64: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker_unittest.cc:72: upload_progress_tracker_(FROM_HERE, &tracker_client_, task_runner_) {} include base/location.h for FROM_HERE https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker_unittest.cc:174: TEST_F(UploadProgressTrackerTest, TimePassed) { Can this just be a copy of NoProgress, with a change from 50->51 (Plus time update). Just suggesting making tests more similar when possible so it's easier to compare them. https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker_unittest.cc:183: EXPECT_EQ(1000, tracker_client_.total_size()); Suggest using 500 / 1000 instead of 50/100 on all of these tests, just to make them easier to compare. https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker_unittest.cc:187: upload_progress_tracker_.set_current_time(base::TimeTicks::Now()); Relying on returning a time of 0 by default seems a bit weird. Suggest defaulting time to base::TimeTicks::Now() in the UploadProgressTracker constructor, and incrementing it by whatever is necessary here, to make it trigger. https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker_unittest.cc:195: EXPECT_EQ(1000, tracker_client_.total_size()); Suggest another copy of this test, without the time delta, which should not send an update. https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker_unittest.cc:195: EXPECT_EQ(1000, tracker_client_.total_size()); Also suggest a copy of this test where progress goes backwards (500->499) (Also with the time delta). Progress can be rewound in two cases: Redirects (Entire body was uploaded, but we don't guarantee a progress update at 100% in that case, and even if we did, we don't destroy the UploadProgressTracker). Also, sometimes we retry the request after we've uploaded it. The retry is mostly invisible to consumers, but can see progress go backwards, and tracker shouldn't report that. https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker_unittest.cc:216: } It's not exciting, but maybe a test with an upload of unknown size (-1).
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/2612903008/diff/40001/content/browser/loader/... File content/browser/loader/upload_progress_tracker.h (right): https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker.h:36: virtual net::UploadProgress GetUploadProgress() = 0; On 2017/01/11 19:31:13, mmenke wrote: > Since we're returning this (And not just by ref or by value), should be be > including the header instead of forward declaring it? Done. https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker.h:38: int64_t total_size) = 0; On 2017/01/11 19:31:13, mmenke wrote: > Should document these. Also, why doesn't ReporterUploadProgress just take a > net::UploadProgress? Done. Updated the reporting function to take a net::UploadProgress. https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker.h:58: virtual base::TimeTicks GetCurrentTime(); On 2017/01/11 19:31:13, mmenke wrote: > nit: Can be private (Subclasses can override private functions, they just can't > call them (Unless they also override them, though not relevant here, anyways)) Done. https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker.h:58: virtual base::TimeTicks GetCurrentTime(); On 2017/01/11 19:31:13, mmenke wrote: > const? Done. https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker.h:58: virtual base::TimeTicks GetCurrentTime(); On 2017/01/11 19:31:13, mmenke wrote: > Maybe add a comment that this can be overridden for testing? Done. https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... File content/browser/loader/upload_progress_tracker_unittest.cc (right): https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker_unittest.cc:27: total_size_ = total_size; On 2017/01/11 19:31:13, mmenke wrote: > Both of these two lines seems wrong. > > Seems like we should either do: > EXPECT_EQ(current_position_, current_position); > EXPECT_EQ(total_size_, total_size); > > And/or we should store them in separate values, that are not the same as the > ones returned by GetUploadProgress. Oh... Right, it's wrong. https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker_unittest.cc:61: base::TimeTicks GetCurrentTime() override { return current_time_; } On 2017/01/11 19:31:14, mmenke wrote: > Can be private Done. https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker_unittest.cc:63: base::TimeTicks current_time_; On 2017/01/11 19:31:14, mmenke wrote: > Should be private. Done. https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker_unittest.cc:64: }; On 2017/01/11 19:31:14, mmenke wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker_unittest.cc:72: upload_progress_tracker_(FROM_HERE, &tracker_client_, task_runner_) {} On 2017/01/11 19:31:14, mmenke wrote: > include base/location.h for FROM_HERE Done. https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker_unittest.cc:174: TEST_F(UploadProgressTrackerTest, TimePassed) { On 2017/01/11 19:31:13, mmenke wrote: > Can this just be a copy of NoProgress, with a change from 50->51 (Plus time > update). Just suggesting making tests more similar when possible so it's easier > to compare them. Done. https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker_unittest.cc:183: EXPECT_EQ(1000, tracker_client_.total_size()); On 2017/01/11 19:31:14, mmenke wrote: > Suggest using 500 / 1000 instead of 50/100 on all of these tests, just to make > them easier to compare. Done. https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker_unittest.cc:187: upload_progress_tracker_.set_current_time(base::TimeTicks::Now()); On 2017/01/11 19:31:14, mmenke wrote: > Relying on returning a time of 0 by default seems a bit weird. Suggest > defaulting time to base::TimeTicks::Now() in the UploadProgressTracker > constructor, and incrementing it by whatever is necessary here, to make it > trigger. Done. https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker_unittest.cc:195: EXPECT_EQ(1000, tracker_client_.total_size()); On 2017/01/11 19:31:13, mmenke wrote: > Also suggest a copy of this test where progress goes backwards (500->499) (Also > with the time delta). Progress can be rewound in two cases: Redirects (Entire > body was uploaded, but we don't guarantee a progress update at 100% in that > case, and even if we did, we don't destroy the UploadProgressTracker). Also, > sometimes we retry the request after we've uploaded it. The retry is mostly > invisible to consumers, but can see progress go backwards, and tracker shouldn't > report that. Done. https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker_unittest.cc:216: } On 2017/01/11 19:31:14, mmenke wrote: > It's not exciting, but maybe a test with an upload of unknown size (-1). IIUC, request bodies always have fixed sizes that are known when the request was issued. The request body is from RequestInit#body, and its BodyInit is a union of types that have known sizes, unlike ResponseBodyInit. https://fetch.spec.whatwg.org/#request-class https://fetch.spec.whatwg.org/#bodyinit
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.
LGTM! https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... File content/browser/loader/upload_progress_tracker_unittest.cc (right): https://codereview.chromium.org/2612903008/diff/40001/content/browser/loader/... content/browser/loader/upload_progress_tracker_unittest.cc:216: } On 2017/01/12 13:00:57, tzik wrote: > On 2017/01/11 19:31:14, mmenke wrote: > > It's not exciting, but maybe a test with an upload of unknown size (-1). > > IIUC, request bodies always have fixed sizes that are known when the request was > issued. > The request body is from RequestInit#body, and its BodyInit is a union of types > that have known sizes, unlike ResponseBodyInit. > https://fetch.spec.whatwg.org/#request-class > https://fetch.spec.whatwg.org/#bodyinit That's not true when talking about Chrome in general (Since we do browser-initiated chunked uploads to Google services), but for requests that currently go through this code, you're right. https://codereview.chromium.org/2612903008/diff/80001/content/browser/loader/... File content/browser/loader/upload_progress_tracker.cc (right): https://codereview.chromium.org/2612903008/diff/80001/content/browser/loader/... content/browser/loader/upload_progress_tracker.cc:56: DCHECK(progress.size() >= 0); nit: DCHECK_GE https://codereview.chromium.org/2612903008/diff/80001/content/browser/loader/... File content/browser/loader/upload_progress_tracker_unittest.cc (right): https://codereview.chromium.org/2612903008/diff/80001/content/browser/loader/... content/browser/loader/upload_progress_tracker_unittest.cc:65: upload_progress_tracker_.set_current_time(base::TimeTicks::Now()); Just move this into UploadProgressTracker's constructor? Note that each test we run re-creates the test fixture, so both the constructor and SetUp are called once for each test.
lgtm https://codereview.chromium.org/2612903008/diff/80001/content/browser/loader/... File content/browser/loader/upload_progress_tracker_unittest.cc (right): https://codereview.chromium.org/2612903008/diff/80001/content/browser/loader/... content/browser/loader/upload_progress_tracker_unittest.cc:187: upload_progress_tracker_.set_upload_progress(net::UploadProgress(501, 1000)); Can you call task_runner_->RunPendingTasks() and check report_count_ here to make sure the progress is not big enough for immediate report?
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/2612903008/diff/80001/content/browser/loader/... File content/browser/loader/upload_progress_tracker.cc (right): https://codereview.chromium.org/2612903008/diff/80001/content/browser/loader/... content/browser/loader/upload_progress_tracker.cc:56: DCHECK(progress.size() >= 0); On 2017/01/12 16:06:54, mmenke wrote: > nit: DCHECK_GE Let me remove this DCHECK, since the condition is always true and the compiler warns that. The type of progress.size() is uint64_t. IIUC, the size of UploadProgress is set to 0 in case of the chunked upload, and UploadProgress event is unavailable, however it's not matter since UploadProgress event is only for XHR, and XHR doesn't use chunked upload. https://codereview.chromium.org/2612903008/diff/80001/content/browser/loader/... File content/browser/loader/upload_progress_tracker_unittest.cc (right): https://codereview.chromium.org/2612903008/diff/80001/content/browser/loader/... content/browser/loader/upload_progress_tracker_unittest.cc:65: upload_progress_tracker_.set_current_time(base::TimeTicks::Now()); On 2017/01/12 16:06:54, mmenke wrote: > Just move this into UploadProgressTracker's constructor? Note that each test we > run re-creates the test fixture, so both the constructor and SetUp are called > once for each test. Done. https://codereview.chromium.org/2612903008/diff/80001/content/browser/loader/... content/browser/loader/upload_progress_tracker_unittest.cc:187: upload_progress_tracker_.set_upload_progress(net::UploadProgress(501, 1000)); On 2017/01/13 03:30:37, yhirano wrote: > Can you call task_runner_->RunPendingTasks() and check report_count_ here to > make sure the progress is not big enough for immediate report? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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/2612903008/#ps100001 (title: "+expectation for unreported small progress.")
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 ========== Add UploadProgressTracker unittest BUG=603396 ========== to ========== Add UploadProgressTracker unittest BUG=603396 TBR=kinuko@chromium.org ==========
tzik@chromium.org changed reviewers: + kinuko@chromium.org
TBRing to kinuko-san for content_export.h addition to DEPS. PTAL later.
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": 100001, "attempt_start_ts": 1484307240745430,
"parent_rev": "93605d248126928596c785734491ea7b8ea1b7cf", "commit_rev":
"6d2c4d88c7d326cd5e711b2d9710e9c4ebc19cf0"}
Message was sent while issue was closed.
Description was changed from ========== Add UploadProgressTracker unittest BUG=603396 TBR=kinuko@chromium.org ========== to ========== Add UploadProgressTracker unittest BUG=603396 TBR=kinuko@chromium.org Review-Url: https://codereview.chromium.org/2612903008 Cr-Commit-Position: refs/heads/master@{#443537} Committed: https://chromium.googlesource.com/chromium/src/+/6d2c4d88c7d326cd5e711b2d9710... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/6d2c4d88c7d326cd5e711b2d9710... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
