Make DataUseTabModel and DataUseMatcher more independent

DataUseTabModel and DataUseMatcher make more independent

Removed DataUseTabModel and ExternalDataUseObserverBridge dependency in
DataUseMatcher. Removed ExternalDataUseObserverBridge dependency in
DataUseTabModel. This will make it easy for testing.

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

[Raj, 2016-07-18 16:22:17]

Description was changed from

==========
DataUseTabModel and DataUseMatcher make more independent

Removed DataUseTabModel and ExternalDataUseObserverBridge dependency in
DataUseMatcher. Removed ExternalDataUseObserverBridge dependency in
DataUseTabModel. This will make it easy for testing.

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

to

==========
DataUseTabModel and DataUseMatcher make more independent

Removed DataUseTabModel and ExternalDataUseObserverBridge dependency in
DataUseMatcher. Removed ExternalDataUseObserverBridge dependency in
DataUseTabModel. This will make it easy for testing.

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

[Raj, 2016-07-18 16:22:17]

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

[Raj, 2016-07-18 16:23:28]

WDYT of the approach of passing around base::Callback instead of direct
dependency.

Will fix unittests once hearing your review.

[tbansal1, 2016-07-18 17:04:38]

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

https://codereview.chromium.org/2158913002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_matcher.cc:14: #include
"base/memory/weak_ptr.h"
include not needed anymore.

https://codereview.chromium.org/2158913002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_matcher.cc:92:
on_matching_rules_fetched_callback_.Run(!matching_rules_.empty());
Would this callback run on UI thread? Is it expected to run on IO?

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

https://codereview.chromium.org/2158913002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_matcher.h:38: DataUseMatcher(
add comments about the callback.

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

https://codereview.chromium.org/2158913002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer.cc:140:
base::Unretained(external_data_use_observer_bridge_)),
Add comment about why it is okay to use base::Unretained here. Essentially show
that the bridge is guaranteed to be non-null anytime this callback is run.

Same below.

https://codereview.chromium.org/2158913002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer.cc:169:
base::Bind(&DataUseTabModel::InitOnUIThread,
Is it possible that this initialization calls the bridge callback when bridge is
not initialized?

[Raj, 2016-07-19 00:18:44]

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

[Raj, 2016-07-19 00:19:03]

ptal

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

https://codereview.chromium.org/2158913002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_matcher.cc:14: #include
"base/memory/weak_ptr.h"
On 2016/07/18 17:04:38, tbansal1 wrote:
> include not needed anymore.

Done.

https://codereview.chromium.org/2158913002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_matcher.cc:92:
on_matching_rules_fetched_callback_.Run(!matching_rules_.empty());
On 2016/07/18 17:04:38, tbansal1 wrote:
> Would this callback run on UI thread? Is it expected to run on IO?

The callback runs on UI thread. The bridge expects it on UI thread, and does
posttask to IO.

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

https://codereview.chromium.org/2158913002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/data_use_matcher.h:38: DataUseMatcher(
On 2016/07/18 17:04:38, tbansal1 wrote:
> add comments about the callback.

Done.

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

https://codereview.chromium.org/2158913002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer.cc:140:
base::Unretained(external_data_use_observer_bridge_)),
On 2016/07/18 17:04:38, tbansal1 wrote:
> Add comment about why it is okay to use base::Unretained here. Essentially
show
> that the bridge is guaranteed to be non-null anytime this callback is run.
> 
> Same below.

Done.

https://codereview.chromium.org/2158913002/diff/1/chrome/browser/android/data...
chrome/browser/android/data_usage/external_data_use_observer.cc:169:
base::Bind(&DataUseTabModel::InitOnUIThread,
On 2016/07/18 17:04:38, tbansal1 wrote:
> Is it possible that this initialization calls the bridge callback when bridge
is
> not initialized?

Removed this. InitOnUIThread() is not needed.

[commit-bot: I haz the power, 2016-07-19 00:19:55]

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-19 01:37:35]

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

[commit-bot: I haz the power, 2016-07-19 01:37:36]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-07-19 17:59:32]

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

https://codereview.chromium.org/2158913002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_matcher.cc:36:
DCHECK(on_tracking_label_removed_callback);
DCHECK the corresponding local variables. So.
DCHECK(on_tracking_label_removed_callback_);

https://codereview.chromium.org/2158913002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_matcher.cc:89: for (const
std::string& label : removed_matching_rule_labels) {
braces not needed.

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

https://codereview.chromium.org/2158913002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_matcher_unittest.cc:88:
base::Unretained(external_data_use_observer_bridge_.get())),
include base/bind_helpers for base::Unretained. Here, and everywhere else in
this CL.

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

https://codereview.chromium.org/2158913002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_tab_model.cc:151:
DCHECK(fetch_matching_rules_callback);
DCHECK the local variables.

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

https://codereview.chromium.org/2158913002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_tab_model.h:113: const base::Closure&
fetch_matching_rules_callback,
include callback_forward.h for base::Closure.

https://codereview.chromium.org/2158913002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_tab_model.h:113: const base::Closure&
fetch_matching_rules_callback,
May be rename to |force_fetch_matching_rules_callback|
Right now, the names of the two callbacks are pretty similar, and it is slightly
confusing.

[Raj, 2016-07-19 21:27:07]

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

[Raj, 2016-07-19 21:27:57]

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

[commit-bot: I haz the power, 2016-07-19 21:29:00]

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-19 21:29:08]

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

https://codereview.chromium.org/2158913002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_matcher.cc:36:
DCHECK(on_tracking_label_removed_callback);
On 2016/07/19 17:59:31, tbansal1 wrote:
> DCHECK the corresponding local variables. So.
> DCHECK(on_tracking_label_removed_callback_);

Done.

https://codereview.chromium.org/2158913002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_matcher.cc:89: for (const
std::string& label : removed_matching_rule_labels) {
On 2016/07/19 17:59:32, tbansal1 wrote:
> braces not needed.

Done.

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

https://codereview.chromium.org/2158913002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_matcher_unittest.cc:88:
base::Unretained(external_data_use_observer_bridge_.get())),
On 2016/07/19 17:59:32, tbansal1 wrote:
> include base/bind_helpers for base::Unretained. Here, and everywhere else in
> this CL.

Done.

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

https://codereview.chromium.org/2158913002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_tab_model.cc:151:
DCHECK(fetch_matching_rules_callback);
On 2016/07/19 17:59:32, tbansal1 wrote:
> DCHECK the local variables.

Done.

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

https://codereview.chromium.org/2158913002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_tab_model.h:113: const base::Closure&
fetch_matching_rules_callback,
On 2016/07/19 17:59:32, tbansal1 wrote:
> include callback_forward.h for base::Closure.

Done.

https://codereview.chromium.org/2158913002/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/data_use_tab_model.h:113: const base::Closure&
fetch_matching_rules_callback,
On 2016/07/19 17:59:32, tbansal1 wrote:
> May be rename to |force_fetch_matching_rules_callback|
> Right now, the names of the two callbacks are pretty similar, and it is
slightly
> confusing.

Done.

[commit-bot: I haz the power, 2016-07-19 21:41:17]

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

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

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tbansal1, 2016-07-19 22:51:42]

lgtm

[Raj, 2016-07-20 00:12:05]

The CQ bit was checked by rajendrant@chromium.org

[commit-bot: I haz the power, 2016-07-20 00:13:40]

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-20 01:39:14]

Description was changed from

==========
DataUseTabModel and DataUseMatcher make more independent

Removed DataUseTabModel and ExternalDataUseObserverBridge dependency in
DataUseMatcher. Removed ExternalDataUseObserverBridge dependency in
DataUseTabModel. This will make it easy for testing.

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

to

==========
DataUseTabModel and DataUseMatcher make more independent

Removed DataUseTabModel and ExternalDataUseObserverBridge dependency in
DataUseMatcher. Removed ExternalDataUseObserverBridge dependency in
DataUseTabModel. This will make it easy for testing.

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

[commit-bot: I haz the power, 2016-07-20 01:39:15]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-20 01:39:37]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-20 01:42:29]

Description was changed from

==========
DataUseTabModel and DataUseMatcher make more independent

Removed DataUseTabModel and ExternalDataUseObserverBridge dependency in
DataUseMatcher. Removed ExternalDataUseObserverBridge dependency in
DataUseTabModel. This will make it easy for testing.

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

to

==========
DataUseTabModel and DataUseMatcher make more independent

Removed DataUseTabModel and ExternalDataUseObserverBridge dependency in
DataUseMatcher. Removed ExternalDataUseObserverBridge dependency in
DataUseTabModel. This will make it easy for testing.

BUG=570884

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

[commit-bot: I haz the power, 2016-07-20 01:42:30]

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