Batch data use objects in ExternalDataUseObserver

Batch data use objects in ExternalDataUseObserver

ExternalDataUseObserver should batch data use objects before posting to
the UI thread. This will reduce the overhead due to thread hopping.

BUG=570884
Committed: https://crrev.com/dcd82add07039d1b2b63aa225759b4183c04b5ba
Cr-Commit-Position: refs/heads/master@{#406870}

[Raj, 2016-07-21 03:42:51]

rajendrant@chromium.org changed reviewers:
+ tbansal@chromium.org

[Raj, 2016-07-21 03:42:52]

ptal

[Raj, 2016-07-21 03:43:31]

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

[commit-bot: I haz the power, 2016-07-21 03:43:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2016-07-21 04:40:30]

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
File chrome/browser/android/data_usage/external_data_use_observer.cc (right):

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer.cc:139:
DCHECK(data_use_list);
This DCHECK is not needed because if it is null, the binary would crash anyways
on the next line.

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer.cc:143: void
ExternalDataUseObserver::OnDataUseBatchComplete() {
Add thread checker.

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer.cc:158:
base::Passed(&data_use_list)));
Add DCHECK(!data_use_list_);

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
File chrome/browser/android/data_usage/external_data_use_observer.h (right):

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer.h:97: // Called
when a batch of data use objects are added to |data_use_list|.
s/list/list_/

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer.h:117:
std::unique_ptr<std::deque<const data_usage::DataUse>> data_use_list;
s/list/list_/

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer.h:117:
std::unique_ptr<std::deque<const data_usage::DataUse>> data_use_list;
Add a comment that when this variable is null, it means ..... When it is
non-null, it means...

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
File chrome/browser/android/data_usage/external_data_use_reporter.cc (right):

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_reporter.cc:150:
previous_report_time_ = now_time;
Move this outside the for loop so that it is not modified multiple times.

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
File chrome/browser/android/data_usage/external_data_use_reporter.h (right):

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_reporter.h:75:
std::unique_ptr<std::deque<const data_usage::DataUse>> data_use_list);
you can also mark the deque as const which will prevent addition of new objects
to it.
const std::deque

[commit-bot: I haz the power, 2016-07-21 04:56:17]

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

[commit-bot: I haz the power, 2016-07-21 04:56:18]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-07-21 04:57:12]

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
File chrome/browser/android/data_usage/external_data_use_reporter.cc (right):

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_reporter.cc:141: for (auto
data_use : *data_use_list) {
ALso this should be const auto &
Otherwise, a copy of data_use will be made while iterating(which is
unnecessary).

[Raj, 2016-07-21 05:56:37]

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

[commit-bot: I haz the power, 2016-07-21 05:56:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Raj, 2016-07-21 05:58:22]

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
File chrome/browser/android/data_usage/external_data_use_observer.cc (right):

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer.cc:139:
DCHECK(data_use_list);
On 2016/07/21 04:40:30, tbansal1 wrote:
> This DCHECK is not needed because if it is null, the binary would crash
anyways
> on the next line.

Done.

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer.cc:143: void
ExternalDataUseObserver::OnDataUseBatchComplete() {
On 2016/07/21 04:40:30, tbansal1 wrote:
> Add thread checker.

Done.

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer.cc:158:
base::Passed(&data_use_list)));
On 2016/07/21 04:40:30, tbansal1 wrote:
> Add DCHECK(!data_use_list_);

Done.

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
File chrome/browser/android/data_usage/external_data_use_observer.h (right):

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer.h:97: // Called
when a batch of data use objects are added to |data_use_list|.
On 2016/07/21 04:40:30, tbansal1 wrote:
> s/list/list_/

Done.

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer.h:117:
std::unique_ptr<std::deque<const data_usage::DataUse>> data_use_list;
On 2016/07/21 04:40:30, tbansal1 wrote:
> s/list/list_/

Done.

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer.h:117:
std::unique_ptr<std::deque<const data_usage::DataUse>> data_use_list;
On 2016/07/21 04:40:30, tbansal1 wrote:
> Add a comment that when this variable is null, it means ..... When it is
> non-null, it means...

Done.

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
File chrome/browser/android/data_usage/external_data_use_reporter.cc (right):

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_reporter.cc:141: for (auto
data_use : *data_use_list) {
On 2016/07/21 04:57:11, tbansal1 wrote:
> ALso this should be const auto &
> Otherwise, a copy of data_use will be made while iterating(which is
> unnecessary).

Done.

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_reporter.cc:150:
previous_report_time_ = now_time;
On 2016/07/21 04:40:30, tbansal1 wrote:
> Move this outside the for loop so that it is not modified multiple times.

Done.

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
File chrome/browser/android/data_usage/external_data_use_reporter.h (right):

https://codereview.chromium.org/2169773002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_reporter.h:75:
std::unique_ptr<std::deque<const data_usage::DataUse>> data_use_list);
On 2016/07/21 04:40:30, tbansal1 wrote:
> you can also mark the deque as const which will prevent addition of new
objects
> to it.
> const std::deque

Done.

[commit-bot: I haz the power, 2016-07-21 06:34:49]

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

[commit-bot: I haz the power, 2016-07-21 06:34:50]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-07-21 15:40:20]

lgtm

[Raj, 2016-07-21 16:19:04]

The CQ bit was checked by rajendrant@chromium.org

[commit-bot: I haz the power, 2016-07-21 16:19:27]

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, 2016-07-21 16:23:25]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-21 16:25:19]

Description was changed from

==========
Batch data use objects in ExternalDataUseObserver

ExternalDataUseObserver should batch data use objects before posting to
the UI thread. This will reduce the overhead due to thread hopping.

BUG=570884
==========

to

==========
Batch data use objects in ExternalDataUseObserver

ExternalDataUseObserver should batch data use objects before posting to
the UI thread. This will reduce the overhead due to thread hopping.

BUG=570884

Committed: https://crrev.com/dcd82add07039d1b2b63aa225759b4183c04b5ba
Cr-Commit-Position: refs/heads/master@{#406870}
==========

[commit-bot: I haz the power, 2016-07-21 16:25:20]

Patchset 2 (id:??) landed as
https://crrev.com/dcd82add07039d1b2b63aa225759b4183c04b5ba
Cr-Commit-Position: refs/heads/master@{#406870}