ExternalDataUseObserver matches package names

ExternalDataUseObserver matches package names

ExternalDataUseObserver receives package names and labels
from the matching rules, and provides an API to get the
matching label for a given package name.

BUG=557832
Committed: https://crrev.com/36fbcc9a3108e3a76e4304fe44a79d5a86134bd1
Cr-Commit-Position: refs/heads/master@{#360490}

[tbansal1, 2015-11-18 17:33:40]

Patchset #1 (id:1) has been deleted

[tbansal1, 2015-11-18 17:34:35]

Description was changed from

==========
ExternalDataUseObserver matches package names

BUG=
==========

to

==========
ExternalDataUseObserver matches package names

ExternalDataUseObserver receives package names and labels
from the matching rules, and provides an API to get the
matching label for a given package name.

BUG=
==========

[tbansal1, 2015-11-18 17:40:04]

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

[tbansal1, 2015-11-18 17:40:04]

ptal. thanks!

[tbansal1, 2015-11-18 17:41:21]

Description was changed from

==========
ExternalDataUseObserver matches package names

ExternalDataUseObserver receives package names and labels
from the matching rules, and provides an API to get the
matching label for a given package name.

BUG=
==========

to

==========
ExternalDataUseObserver matches package names

ExternalDataUseObserver receives package names and labels
from the matching rules, and provides an API to get the
matching label for a given package name.

BUG=557832
==========

[Raj, 2015-11-18 18:36:51]

MatchesAppPackageName() is not being used.
Will that be used in later CLs, or more code will be added to this CL.

Thanks

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

https://codereview.chromium.org/1447363006/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_observer.h:95: // names.
|label| must not be null. If a match is found, the |label| is set
nits:
change |label| to |output_label| for more clarity.

https://codereview.chromium.org/1447363006/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_observer.h:192: const
std::string& app_package_name() const;
nits:
If its only used in tests, consider making the test class as friend to
MatchingRule.
Not sure if this is intended to be used by new code.

[tbansal1, 2015-11-18 18:50:43]

On 2015/11/18 18:36:51, Raj wrote:
> MatchesAppPackageName() is not being used.
> Will that be used in later CLs, or more code will be added to this CL.
this should be used by DataUseTabModel to match the package name before matching
the URL. As discussed offline,
that will be done in a separate CL.
> 
> Thanks
> 
>
https://codereview.chromium.org/1447363006/diff/20001/chrome/browser/android/...
> File chrome/browser/android/data_usage/external_data_use_observer.h (right):
> 
>
https://codereview.chromium.org/1447363006/diff/20001/chrome/browser/android/...
> chrome/browser/android/data_usage/external_data_use_observer.h:95: // names.
> |label| must not be null. If a match is found, the |label| is set
> nits:
> change |label| to |output_label| for more clarity.
> 
>
https://codereview.chromium.org/1447363006/diff/20001/chrome/browser/android/...
> chrome/browser/android/data_usage/external_data_use_observer.h:192: const
> std::string& app_package_name() const;
> nits:
> If its only used in tests, consider making the test class as friend to
> MatchingRule.
> Not sure if this is intended to be used by new code.

[tbansal1, 2015-11-18 18:50:50]

ptal. thanks.

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

https://codereview.chromium.org/1447363006/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_observer.h:95: // names.
|label| must not be null. If a match is found, the |label| is set
On 2015/11/18 18:36:51, Raj wrote:
> nits:
> change |label| to |output_label| for more clarity.

I think it is clear that since it is a pointer and from the comments, that it
will be modified.

https://codereview.chromium.org/1447363006/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_observer.h:192: const
std::string& app_package_name() const;
On 2015/11/18 18:36:51, Raj wrote:
> nits:
> If its only used in tests, consider making the test class as friend to
> MatchingRule.
> Not sure if this is intended to be used by new code.

this is used in external_data_use_observer.cc

[Raj, 2015-11-18 18:51:41]

lgtm

[sclittle, 2015-11-18 22:17:08]

lgtm with nit

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

https://codereview.chromium.org/1447363006/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_observer.cc:409: for (size_t
i = 0; i < matching_rules_.size(); ++i) {
nit: use range-based for loop, e.g. something like

for (const auto* rule : matching_rules_) {
  if (app_package_name == rule->app_package_name()) {
...

[tbansal1, 2015-11-18 23:46:43]

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

https://codereview.chromium.org/1447363006/diff/20001/chrome/browser/android/...
chrome/browser/android/data_usage/external_data_use_observer.cc:409: for (size_t
i = 0; i < matching_rules_.size(); ++i) {
On 2015/11/18 22:17:07, sclittle wrote:
> nit: use range-based for loop, e.g. something like
> 
> for (const auto* rule : matching_rules_) {
>   if (app_package_name == rule->app_package_name()) {
> ...

Done.

[tbansal1, 2015-11-18 23:46:47]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2015-11-18 23:46:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
rajendrant@chromium.org, sclittle@chromium.org
Link to the patchset: https://codereview.chromium.org/1447363006/#ps40001
(title: "Rebased, addressed sclittle comments")

[commit-bot: I haz the power, 2015-11-18 23:48:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1447363006/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1447363006/40001

[tbansal1, 2015-11-19 00:54:48]

The CQ bit was unchecked by tbansal@chromium.org

[tbansal1, 2015-11-19 00:57:50]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2015-11-19 00:57:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
rajendrant@chromium.org, sclittle@chromium.org
Link to the patchset: https://codereview.chromium.org/1447363006/#ps60001
(title: "Removed invalid DCHECK")

[commit-bot: I haz the power, 2015-11-19 00:59:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1447363006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1447363006/60001

[commit-bot: I haz the power, 2015-11-19 02:20:54]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-11-19 02:22:47]

Patchset 3 (id:??) landed as
https://crrev.com/36fbcc9a3108e3a76e4304fe44a79d5a86134bd1
Cr-Commit-Position: refs/heads/master@{#360490}