Separate data use reporting logic in ExternalDataUseObserver

Separate data use reporting logic in ExternalDataUseObserver

ExternalDataUseReporter class is created with the data use reporting
logic. ExternalDataUseReporter is created in IO thread and moved to UI
thread. It is owned by ExternalDataUseObserver.

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

[Raj, 2016-07-13 02:45:20]

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

[Raj, 2016-07-13 02:45:21]

ptal

Fixing tests.

[tbansal1, 2016-07-13 16:10:58]

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

https://codereview.chromium.org/2145863002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer.cc:131: void
ExternalDataUseObserver::OnReportDataUseDone(bool success) {
Can this be moved to reporter? That will save two thread hops. Preferably in the
next CL.

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

https://codereview.chromium.org/2145863002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_reporter.cc:131:
DCHECK(thread_checker_.CalledOnValidThread());
Remove the thread checker because the dtor is called on a different thread than
other functions.

https://codereview.chromium.org/2145863002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_reporter.cc:181: void
ExternalDataUseReporter::DataUseTrackingInfoRetrieved(
Is this function called somewhere?

https://codereview.chromium.org/2145863002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_reporter.cc:252: if
(!immediate && total_bytes_buffered_ < data_use_report_min_bytes_)
This seems like a bug. When Chromium is moved to background, EDUO or EDUR should
essentially submit all the buffered reports without waiting to hit 
data_use_report_min_bytes limit. It is not clear if that's happening in the
current code. May be fix in a different CL?

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

https://codereview.chromium.org/2145863002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_reporter.h:57:
ExternalDataUseReporter(
In the comments, say something lifetime. e.g., The caller should guarantee that
|data_use_tab_model| and |external_data_...| to be non-null during the lifetime
of |this|.

https://codereview.chromium.org/2145863002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_reporter.h:173: //
ExternalDataUseObserver.
Better not to talk about ExternalDataUseObserver in this class.

https://codereview.chromium.org/2145863002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_reporter.h:176: // Pointer
to the DataUseTabModel owned by ExternalDataUseObserver.
Comments should not mention EDUO.

[tbansal1, 2016-07-13 16:11:57]

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

https://codereview.chromium.org/2145863002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_reporter.cc:131:
DCHECK(thread_checker_.CalledOnValidThread());
On 2016/07/13 16:10:58, tbansal1 wrote:
> Remove the thread checker because the dtor is called on a different thread
than
> other functions.

Actually, the thread checker is needed. dtor is called on UI thread (same as
other functions).

[Raj, 2016-07-13 17:06:52]

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

https://codereview.chromium.org/2145863002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer.cc:131: void
ExternalDataUseObserver::OnReportDataUseDone(bool success) {
On 2016/07/13 16:10:58, tbansal1 wrote:
> Can this be moved to reporter? That will save two thread hops. Preferably in
the
> next CL.

Yes. I have a TODO in the header file.

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

https://codereview.chromium.org/2145863002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_reporter.cc:131:
DCHECK(thread_checker_.CalledOnValidThread());
On 2016/07/13 16:11:57, tbansal1 wrote:
> On 2016/07/13 16:10:58, tbansal1 wrote:
> > Remove the thread checker because the dtor is called on a different thread
> than
> > other functions.
> 
> Actually, the thread checker is needed. dtor is called on UI thread (same as
> other functions).

Acknowledged.

https://codereview.chromium.org/2145863002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_reporter.cc:181: void
ExternalDataUseReporter::DataUseTrackingInfoRetrieved(
On 2016/07/13 16:10:58, tbansal1 wrote:
> Is this function called somewhere?

Oops. Not called.

https://codereview.chromium.org/2145863002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_reporter.cc:252: if
(!immediate && total_bytes_buffered_ < data_use_report_min_bytes_)
On 2016/07/13 16:10:58, tbansal1 wrote:
> This seems like a bug. When Chromium is moved to background, EDUO or EDUR
should
> essentially submit all the buffered reports without waiting to hit 
> data_use_report_min_bytes limit. It is not clear if that's happening in the
> current code. May be fix in a different CL? 

When Chromium is backgrounded SubmitBufferedDataUseReport(true) is called, and
it submits one report immediately. Only one report could be submitted because
the bridge needs to report the result of submission. The external API supports
only one pending report. I have added a TODO at Line:174.
So that the external API could support sending multiple reports. It is ok for
our current requirements since there is only one matching rule.

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

https://codereview.chromium.org/2145863002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_reporter.h:57:
ExternalDataUseReporter(
On 2016/07/13 16:10:58, tbansal1 wrote:
> In the comments, say something lifetime. e.g., The caller should guarantee
that
> |data_use_tab_model| and |external_data_...| to be non-null during the
lifetime
> of |this|.

Done.

https://codereview.chromium.org/2145863002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_reporter.h:173: //
ExternalDataUseObserver.
On 2016/07/13 16:10:58, tbansal1 wrote:
> Better not to talk about ExternalDataUseObserver in this class.

Done.

https://codereview.chromium.org/2145863002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_reporter.h:176: // Pointer
to the DataUseTabModel owned by ExternalDataUseObserver.
On 2016/07/13 16:10:58, tbansal1 wrote:
> Comments should not mention EDUO.

Done.

[tbansal1, 2016-07-15 16:26:31]

https://codereview.chromium.org/2145863002/diff/20001/chrome/browser/android/...
File chrome/browser/android/data_usage/external_data_use_observer.h (right):

https://codereview.chromium.org/2145863002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_observer.h:78: friend class
ExternalDataUseReporterTest;
reporter tests should not be poking in EDUO.

https://codereview.chromium.org/2145863002/diff/20001/chrome/browser/android/...
File chrome/browser/android/data_usage/external_data_use_reporter.cc (right):

https://codereview.chromium.org/2145863002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_reporter.cc:14: #include
"chrome/browser/android/data_usage/external_data_use_observer.h"
why is the include to e_d_u_o needed? If it is only needed for getting the field
trial, then  consider passing it as an argument to ctor of e_d_u_reporter.

https://codereview.chromium.org/2145863002/diff/20001/chrome/browser/android/...
File chrome/browser/android/data_usage/external_data_use_reporter.h (right):

https://codereview.chromium.org/2145863002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_reporter.h:18: #include
"chrome/browser/android/data_usage/data_use_tab_model.h"
You should only need to forward declare this, and move the include to the .cc.

https://codereview.chromium.org/2145863002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_reporter.h:74:
FRIEND_TEST_ALL_PREFIXES(ExternalDataUseObserverTest, Variations);
Can the relevant part of the unittest be moved to e_d_u_reporter.unittest so
this class does not have to friend tests of other classes. Ideally, this class
should not know anything about e_d_u_o.

https://codereview.chromium.org/2145863002/diff/20001/chrome/browser/android/...
File chrome/browser/android/data_usage/external_data_use_reporter_unittest.cc
(right):

https://codereview.chromium.org/2145863002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_reporter_unittest.cc:22:
#include "chrome/browser/android/data_usage/external_data_use_observer.h"
This include should not be necessary.

https://codereview.chromium.org/2145863002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_reporter_unittest.cc:295:
external_data_use_observer()->GetDataUseTabModel()->OnNavigationEvent(
Is it possible to call DataUseTabModel directly from the reporter? This would
remove any dependency to EDUO.

https://codereview.chromium.org/2145863002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_reporter_unittest.cc:398:
base::RunLoop().RunUntilIdle();
Why is RunLoop needed? IIUC, EDUR and all its dependencies are on the same
thread.

https://codereview.chromium.org/2145863002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_reporter_unittest.cc:399:
external_data_use_observer()->OnReportDataUseDone(true);
s//reporter->OnReportDataUseDone(...);/

[Raj, 2016-07-15 18:54:35]

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

[Raj, 2016-07-15 18:54:42]

ptal

https://codereview.chromium.org/2145863002/diff/20001/chrome/browser/android/...
File chrome/browser/android/data_usage/external_data_use_observer.h (right):

https://codereview.chromium.org/2145863002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_observer.h:78: friend class
ExternalDataUseReporterTest;
On 2016/07/15 16:26:30, tbansal1 wrote:
> reporter tests should not be poking in EDUO.

Done. Created GetExternalDataUseReporterForTests() to go around it.

https://codereview.chromium.org/2145863002/diff/20001/chrome/browser/android/...
File chrome/browser/android/data_usage/external_data_use_reporter.cc (right):

https://codereview.chromium.org/2145863002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_reporter.cc:14: #include
"chrome/browser/android/data_usage/external_data_use_observer.h"
On 2016/07/15 16:26:30, tbansal1 wrote:
> why is the include to e_d_u_o needed? If it is only needed for getting the
field
> trial, then  consider passing it as an argument to ctor of e_d_u_reporter.

Done.

https://codereview.chromium.org/2145863002/diff/20001/chrome/browser/android/...
File chrome/browser/android/data_usage/external_data_use_reporter.h (right):

https://codereview.chromium.org/2145863002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_reporter.h:18: #include
"chrome/browser/android/data_usage/data_use_tab_model.h"
On 2016/07/15 16:26:30, tbansal1 wrote:
> You should only need to forward declare this, and move the include to the .cc.

Done.

https://codereview.chromium.org/2145863002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_reporter.h:74:
FRIEND_TEST_ALL_PREFIXES(ExternalDataUseObserverTest, Variations);
On 2016/07/15 16:26:30, tbansal1 wrote:
> Can the relevant part of the unittest be moved to e_d_u_reporter.unittest so
> this class does not have to friend tests of other classes. Ideally, this class
> should not know anything about e_d_u_o.

Done.

https://codereview.chromium.org/2145863002/diff/20001/chrome/browser/android/...
File chrome/browser/android/data_usage/external_data_use_reporter_unittest.cc
(right):

https://codereview.chromium.org/2145863002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_reporter_unittest.cc:22:
#include "chrome/browser/android/data_usage/external_data_use_observer.h"
On 2016/07/15 16:26:30, tbansal1 wrote:
> This include should not be necessary.

I have removed most of the calls to EDUO. But EDUO is still needed because that
creates the other TabModel, Bridge objects, and helps to fetch rules and stuff.

Maybe once we create a ExternalDataUseUIManager class all UI objects will move
under it. Still there would be some dependency on the new class.

https://codereview.chromium.org/2145863002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_reporter_unittest.cc:295:
external_data_use_observer()->GetDataUseTabModel()->OnNavigationEvent(
On 2016/07/15 16:26:30, tbansal1 wrote:
> Is it possible to call DataUseTabModel directly from the reporter? This would
> remove any dependency to EDUO.

Done.

https://codereview.chromium.org/2145863002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_reporter_unittest.cc:398:
base::RunLoop().RunUntilIdle();
On 2016/07/15 16:26:30, tbansal1 wrote:
> Why is RunLoop needed? IIUC, EDUR and all its dependencies are on the same
> thread.

Done.

https://codereview.chromium.org/2145863002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_reporter_unittest.cc:399:
external_data_use_observer()->OnReportDataUseDone(true);
On 2016/07/15 16:26:30, tbansal1 wrote:
> s//reporter->OnReportDataUseDone(...);/

Done.

[commit-bot: I haz the power, 2016-07-15 18:55:13]

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, 2016-07-15 19:35:03]

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

[commit-bot: I haz the power, 2016-07-15 19:35:04]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-07-18 16:42:13]

lgtm % nits below.

https://codereview.chromium.org/2145863002/diff/60001/chrome/browser/android/...
File chrome/browser/android/data_usage/external_data_use_observer.h (right):

https://codereview.chromium.org/2145863002/diff/60001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_observer.h:74:
ExternalDataUseReporter* GetExternalDataUseReporterForTests() const {
s/ForTests/ForTesting/
https://cs.chromium.org/chromium/src/styleguide/c%2B%2B/c%2B%2B.md?rcl=0&l=41

https://codereview.chromium.org/2145863002/diff/60001/chrome/browser/android/...
File chrome/browser/android/data_usage/external_data_use_observer_unittest.cc
(right):

https://codereview.chromium.org/2145863002/diff/60001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_observer_unittest.cc:21:
#include "chrome/browser/android/data_usage/external_data_use_reporter.h"
Why is this include needed?

https://codereview.chromium.org/2145863002/diff/60001/chrome/browser/android/...
File chrome/browser/android/data_usage/external_data_use_reporter.cc (right):

https://codereview.chromium.org/2145863002/diff/60001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_reporter.cc:122:
app_state_listener_.reset(new base::android::ApplicationStatusListener(
thread checker

[Raj, 2016-07-18 16:51:51]

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

[commit-bot: I haz the power, 2016-07-18 16:52:28]

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, 2016-07-18 18:07:09]

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

[commit-bot: I haz the power, 2016-07-18 18:07:10]

Dry run: This issue passed the CQ dry run.

[Raj, 2016-07-18 18:12:32]

The CQ bit was checked by rajendrant@chromium.org

[Raj, 2016-07-18 18:12:32]

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/2145863002/#ps80001
(title: "Addressed nits")

[Raj, 2016-07-18 18:12:36]

https://codereview.chromium.org/2145863002/diff/60001/chrome/browser/android/...
File chrome/browser/android/data_usage/external_data_use_observer.h (right):

https://codereview.chromium.org/2145863002/diff/60001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_observer.h:74:
ExternalDataUseReporter* GetExternalDataUseReporterForTests() const {
On 2016/07/18 16:42:12, tbansal1 wrote:
> s/ForTests/ForTesting/
> https://cs.chromium.org/chromium/src/styleguide/c%2B%2B/c%2B%2B.md?rcl=0&l=41

Done.

https://codereview.chromium.org/2145863002/diff/60001/chrome/browser/android/...
File chrome/browser/android/data_usage/external_data_use_observer_unittest.cc
(right):

https://codereview.chromium.org/2145863002/diff/60001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_observer_unittest.cc:21:
#include "chrome/browser/android/data_usage/external_data_use_reporter.h"
On 2016/07/18 16:42:12, tbansal1 wrote:
> Why is this include needed?

Done.

https://codereview.chromium.org/2145863002/diff/60001/chrome/browser/android/...
File chrome/browser/android/data_usage/external_data_use_reporter.cc (right):

https://codereview.chromium.org/2145863002/diff/60001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_reporter.cc:122:
app_state_listener_.reset(new base::android::ApplicationStatusListener(
On 2016/07/18 16:42:12, tbansal1 wrote:
> thread checker

Done.

[commit-bot: I haz the power, 2016-07-18 18:12:55]

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-18 18:18:37]

Committed patchset #5 (id:80001)

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

Description was changed from

==========
Separate data use reporting logic in ExternalDataUseObserver

ExternalDataUseReporter class is created with the data use reporting
logic. ExternalDataUseReporter is created in IO thread and moved to UI
thread. It is owned by ExternalDataUseObserver.

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

to

==========
Separate data use reporting logic in ExternalDataUseObserver

ExternalDataUseReporter class is created with the data use reporting
logic. ExternalDataUseReporter is created in IO thread and moved to UI
thread. It is owned by ExternalDataUseObserver.

BUG=570884

Committed: https://crrev.com/7ef8efba12b3ce0c6101175451130ab5c48fce1c
Cr-Commit-Position: refs/heads/master@{#406041}
==========

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

Patchset 5 (id:??) landed as
https://crrev.com/7ef8efba12b3ce0c6101175451130ab5c48fce1c
Cr-Commit-Position: refs/heads/master@{#406041}