Fix data overwriting issue on ReadFromDownloadData.

Fix data overwriting issue on ReadFromDownloadData.

There is an issue which overwrites the download_data_ if the pending
data is not yet consumed. This CL fixes it.

BUG=223603
TEST=Ran unit_tests and tested manually.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190561

[hidehiko, 2013-03-26 04:07:34]

This CL is based on 13078005, but conceptually independent.

Thank you for your review in advance,
- hidehiko

[kinaba, 2013-03-26 04:33:14]

lgtm

[hashimoto, 2013-03-26 04:33:58]

https://codereview.chromium.org/13065007/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/drive_url_request_job.cc (right):

https://codereview.chromium.org/13065007/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/drive_url_request_job.cc:533: for (;
nit: We don't generally use iterators to iterate over random accessible
containers. How about using size_t instead?

https://codereview.chromium.org/13065007/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/drive_url_request_job.cc:536: const std::string&
chunk = **iter;
nit: How about adding DCHECK(!chunk.empty())?

https://codereview.chromium.org/13065007/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/drive_url_request_job.cc:550: const size_t offset
= read_buf_remaining_.data() - read_buf_->data();
Hmm, read_buf_->data() + offset == read_buf_remaining_.data(), but it can't be
replaced because of const... :(

https://codereview.chromium.org/13065007/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/drive_url_request_job.cc:551:
std::memmove(read_buf_->data() + offset, chunk.data(), chunk.size());
Shouldn't we move |read_buf_remaining_| forward |chunk.size()| bytes?

[hidehiko, 2013-03-26 04:55:16]

Thank you for your review. PTAL?
- hidehiko

https://codereview.chromium.org/13065007/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/drive_url_request_job.cc (right):

https://codereview.chromium.org/13065007/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/drive_url_request_job.cc:533: for (;
On 2013/03/26 04:33:58, hashimoto wrote:
> nit: We don't generally use iterators to iterate over random accessible
> containers. How about using size_t instead?

Done.

https://codereview.chromium.org/13065007/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/drive_url_request_job.cc:536: const std::string&
chunk = **iter;
On 2013/03/26 04:33:58, hashimoto wrote:
> nit: How about adding DCHECK(!chunk.empty())?

Done.

https://codereview.chromium.org/13065007/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/drive_url_request_job.cc:550: const size_t offset
= read_buf_remaining_.data() - read_buf_->data();
On 2013/03/26 04:33:58, hashimoto wrote:
> Hmm, read_buf_->data() + offset == read_buf_remaining_.data(), but it can't be
> replaced because of const... :(

Yes it cannot... Probably I can clean it up, but IMHO we should clean the code
in the class level, so please let me keep this as is for now.

https://codereview.chromium.org/13065007/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/drive_url_request_job.cc:551:
std::memmove(read_buf_->data() + offset, chunk.data(), chunk.size());
On 2013/03/26 04:33:58, hashimoto wrote:
> Shouldn't we move |read_buf_remaining_| forward |chunk.size()| bytes?

It is done in RecordBytesRead...

[hashimoto, 2013-03-26 04:57:14]

lgtm

[commit-bot: I haz the power, 2013-03-26 05:04:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/13065007/9001

[hidehiko, 2013-03-26 06:09:48]

Committed patchset #3 manually as r190561 (presubmit successful).