Support moving pending requests from one DataUseRecorder to another

Support moving pending requests from one DataUseRecorder to another

To support such a move, data use of requests should be tracked separately.

BUG=733417
Review-Url: https://codereview.chromium.org/2947973002
Cr-Commit-Position: refs/heads/master@{#482530}
Committed: https://chromium.googlesource.com/chromium/src/+/fb7689d8d23c71776e80f8a62c28b7f65bea6230

[rajendrant, 2017-06-20 21:33:59]

rajendrant@google.com changed reviewers:
+ rajendrant@google.com, tbansal@chromium.org

[rajendrant, 2017-06-20 21:34:00]

ptal

https://codereview.chromium.org/2947973002/diff/1/components/data_use_measure...
File components/data_use_measurement/core/data_use_recorder.h (right):

https://codereview.chromium.org/2947973002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_recorder.h:31: 
base::TimeTicks start_time will be added in a later CL. That will indicate when
the request started.

[tbansal1, 2017-06-23 22:11:22]

https://codereview.chromium.org/2947973002/diff/1/components/data_use_measure...
File components/data_use_measurement/core/data_use_recorder.cc (right):

https://codereview.chromium.org/2947973002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_recorder.cc:37:
other->UpdateNetworkByteCounts(request, request_it->second.bytes_received,
Do we need to insert |request| in |other-> pending_url_requests_|?

https://codereview.chromium.org/2947973002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_recorder.cc:39:
data_use_.IncrementTotalBytes(request_it->second.bytes_received,
Why is IncrementTotalBytes() called here?

https://codereview.chromium.org/2947973002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_recorder.cc:65: DCHECK(request_it
!= pending_url_requests_.end());
dcheck not needed.

https://codereview.chromium.org/2947973002/diff/1/components/data_use_measure...
File components/data_use_measurement/core/data_use_recorder.h (right):

https://codereview.chromium.org/2947973002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_recorder.h:9: #include <map>
nit: line break before #include <map>

https://codereview.chromium.org/2947973002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_recorder.h:33: int64_t
bytes_received;
For consistency, it would be better to have bytes_received  before bytes_sent. I
believe that's the order being followed everywhere else in this file.

[Raj, 2017-06-26 21:40:21]

rajendrant@chromium.org changed reviewers:
- rajendrant@google.com

[Raj, 2017-06-26 21:40:22]

https://codereview.chromium.org/2947973002/diff/1/components/data_use_measure...
File components/data_use_measurement/core/data_use_recorder.cc (right):

https://codereview.chromium.org/2947973002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_recorder.cc:37:
other->UpdateNetworkByteCounts(request, request_it->second.bytes_received,
On 2017/06/23 at 22:11:21, tbansal1 wrote:
> Do we need to insert |request| in |other-> pending_url_requests_|?

Yes. Fixed.
Will add unittest when MovePendingURLRequest() gets used in the next CL.

https://codereview.chromium.org/2947973002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_recorder.cc:39:
data_use_.IncrementTotalBytes(request_it->second.bytes_received,
On 2017/06/23 at 22:11:21, tbansal1 wrote:
> Why is IncrementTotalBytes() called here?

It should be called with -ve bytes, for decrementing.

https://codereview.chromium.org/2947973002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_recorder.cc:65: DCHECK(request_it
!= pending_url_requests_.end());
On 2017/06/23 at 22:11:21, tbansal1 wrote:
> dcheck not needed.

Done

https://codereview.chromium.org/2947973002/diff/1/components/data_use_measure...
File components/data_use_measurement/core/data_use_recorder.h (right):

https://codereview.chromium.org/2947973002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_recorder.h:9: #include <map>
On 2017/06/23 at 22:11:21, tbansal1 wrote:
> nit: line break before #include <map>

Done

https://codereview.chromium.org/2947973002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_recorder.h:33: int64_t
bytes_received;
On 2017/06/23 at 22:11:22, tbansal1 wrote:
> For consistency, it would be better to have bytes_received  before bytes_sent.
I believe that's the order being followed everywhere else in this file.

Done

[tbansal1, 2017-06-26 21:44:28]

lgtm % comments.

https://codereview.chromium.org/2947973002/diff/1/components/data_use_measure...
File components/data_use_measurement/core/data_use_recorder.h (right):

https://codereview.chromium.org/2947973002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_recorder.h:31: 
On 2017/06/20 21:34:00, rajendrant wrote:
> base::TimeTicks start_time will be added in a later CL. That will indicate
when
> the request started.

Acknowledged.

https://codereview.chromium.org/2947973002/diff/20001/components/data_use_mea...
File components/data_use_measurement/core/data_use.cc (right):

https://codereview.chromium.org/2947973002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use.cc:18: total_bytes_sent_ +=
bytes_sent;
DCHECK_LE(0, total_bytes_received_);
DCHECK_LE(0, total_bytes_sent_);

https://codereview.chromium.org/2947973002/diff/20001/components/data_use_mea...
File components/data_use_measurement/core/data_use_recorder.cc (right):

https://codereview.chromium.org/2947973002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_recorder.cc:43:
data_use_.IncrementTotalBytes(-request_it->second.bytes_received,
nit: Add a comment on why this is negative.

[Raj, 2017-06-26 22:42:09]

The CQ bit was checked by rajendrant@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-26 22:42:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-26 23:18:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-26 23:18:31]

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_...)

[Raj, 2017-06-27 00:42:30]

Patchset #3 (id:40001) has been deleted

[Raj, 2017-06-27 00:42:53]

The CQ bit was checked by rajendrant@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-27 00:43:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 01:46:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-27 01:46:59]

Dry run: This issue passed the CQ dry run.

[Raj, 2017-06-27 02:08:50]

The CQ bit was checked by rajendrant@chromium.org

[Raj, 2017-06-27 02:08:51]

The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/2947973002/#ps50001
(title: "nits")

[commit-bot: I haz the power, 2017-06-27 02:09:19]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 02:09:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-27 02:09:30]

CQ cannot get SignCLA result. Please try later.

[Raj, 2017-06-27 02:17:47]

The CQ bit was checked by rajendrant@chromium.org

[commit-bot: I haz the power, 2017-06-27 02:18:01]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 02:24:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-27 02:24:49]

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...)

[Raj, 2017-06-27 02:29:54]

The CQ bit was checked by rajendrant@chromium.org

[Raj, 2017-06-27 02:29:54]

The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/2947973002/#ps70001
(title: "rebase")

[Raj, 2017-06-27 02:30:04]

https://codereview.chromium.org/2947973002/diff/20001/components/data_use_mea...
File components/data_use_measurement/core/data_use.cc (right):

https://codereview.chromium.org/2947973002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use.cc:18: total_bytes_sent_ +=
bytes_sent;
On 2017/06/26 at 21:44:28, tbansal1 wrote:
> DCHECK_LE(0, total_bytes_received_);
> DCHECK_LE(0, total_bytes_sent_);

Done

https://codereview.chromium.org/2947973002/diff/20001/components/data_use_mea...
File components/data_use_measurement/core/data_use_recorder.cc (right):

https://codereview.chromium.org/2947973002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_recorder.cc:43:
data_use_.IncrementTotalBytes(-request_it->second.bytes_received,
On 2017/06/26 at 21:44:28, tbansal1 wrote:
> nit: Add a comment on why this is negative.

Done

[commit-bot: I haz the power, 2017-06-27 02:30:11]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 03:37:16]

CQ is committing da patch.

Bot data: {"patchset_id": 70001, "attempt_start_ts": 1498530594079200,
"parent_rev": "fa77f79916a5bac4e8e62745b28d1a500295fb71", "commit_rev":
"fb7689d8d23c71776e80f8a62c28b7f65bea6230"}

[commit-bot: I haz the power, 2017-06-27 03:37:29]

Description was changed from

==========
Support moving pending requests from one DataUseRecorder to another

To support such a move, data use of requests should be tracked separately.

BUG=733417
==========

to

==========
Support moving pending requests from one DataUseRecorder to another

To support such a move, data use of requests should be tracked separately.

BUG=733417

Review-Url: https://codereview.chromium.org/2947973002
Cr-Commit-Position: refs/heads/master@{#482530}
Committed:
https://chromium.googlesource.com/chromium/src/+/fb7689d8d23c71776e80f8a62c28...
==========

[commit-bot: I haz the power, 2017-06-27 03:37:30]

Committed patchset #4 (id:70001) as
https://chromium.googlesource.com/chromium/src/+/fb7689d8d23c71776e80f8a62c28...