Reland add hooks to permission layer for permission action reporting

Reland add hooks to permission layer for permission action reporting

Readd r405051 with fix for the memory.fyi issue.

Changes compared to r405051:
- Use ScopedCommandLine instead of the global variable command line.
- Change few lines of code in chrome/browser/permissions/permission_uma_util_unittest.cc and chrome/browser/permissions/permission_uma_util.h according to comments in cl 2047253002.

BUG=613883
Committed: https://crrev.com/9274014049dc1cbfc0f8aaadcd86c321cb2fae72
Cr-Commit-Position: refs/heads/master@{#406166}

[stefanocs, 2016-07-14 12:54:24]

The CQ bit was checked by stefanocs@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-14 12:54:42]

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

[stefanocs, 2016-07-14 13:25:27]

Description was changed from

==========
Readd hooks to permission layer for permission action reporting

Readd cl 2047253002 with fix for the memory.fyi issue.

Changes compared to 2047253002:
- Use ScopedCommandLine instead of the global variable command line.
- Change few lines of code in
chrome/browser/permissions/permission_uma_util_unittest.cc and
chrome/browser/permissions/permission_uma_util.h according to comments in
2047253002.

BUG=613883
==========

to

==========
Readd hooks to permission layer for permission action reporting

Readd cl 2047253002 with resolve the memory.fyi issue.

Changes compared to 2047253002:
- Use ScopedCommandLine instead of the global variable command line.
- Change few lines of code in
chrome/browser/permissions/permission_uma_util_unittest.cc and
chrome/browser/permissions/permission_uma_util.h according to comments in
2047253002.

BUG=613883
==========

[stefanocs, 2016-07-14 13:28:39]

stefanocs@google.com changed reviewers:
+ johnme@chromium.org, kcarattini@chromium.org, nparker@chromium.org,
pavely@chromium.org, raymes@chromium.org, sergeyu@chromium.org,
thestig@chromium.org

[stefanocs, 2016-07-14 13:28:40]

Hi all,

Please re-review this cl:

thestig@chromium.org: Please review changes in
chrome/browser/permissions/permission_uma_util_unittest.cc

pavely@chromium.org: Please review changes in components/browser_sync/browser
and chrome/browser/permissions/permission_uma_util_unittest.cc (The sync
preference test)

nparker@chromium.org: Please review changes in chrome/browser/safe_browsing

sergeyu@chromium.org: Please review changes in
chrome/browser/media/media_stream_devices_controller.cc

johnme@chromium.org: Please review changes in
chrome/browser/push_messaging/push_messaging_permission_context.cc

kcarattini@chromium.org, raymes@chromium.org: Please review this cl

Thanks!

[johnme, 2016-07-14 14:14:44]

push_messaging/ lgtm

[commit-bot: I haz the power, 2016-07-14 14:49:55]

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

[commit-bot: I haz the power, 2016-07-14 14:49:56]

Dry run: This issue passed the CQ dry run.

[pavely, 2016-07-14 19:29:29]

https://codereview.chromium.org/2150903002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_uma_util_unittest.cc (right):

https://codereview.chromium.org/2150903002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_uma_util_unittest.cc:44:
GetProfileSyncService()->backend_.reset(
Your code in permission_uma_util.cc relies on interface of ProfileSyncService.
Could you use mock of ProfileSyncService instead of tweaking internal members of
the real one.
Here is example of how it can be done:
https://cs.chromium.org/chromium/src/chrome/browser/password_manager/chrome_p...

https://codereview.chromium.org/2150903002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_uma_util_unittest.cc:50:
GetProfileSyncService()->sync_prefs_.SetKeepEverythingSynced(false);
You can create SyncPrefs object on the stack. No need to get one from
ProfileSyncService.

https://codereview.chromium.org/2150903002/diff/1/components/browser_sync/bro...
File components/browser_sync/browser/profile_sync_service.h (right):

https://codereview.chromium.org/2150903002/diff/1/components/browser_sync/bro...
components/browser_sync/browser/profile_sync_service.h:672: friend class
PermissionUmaUtilTest;
In your code you are using public virtual functions of ProfileSyncService, you
shouldn't open it's private members to the test and rely on internal behavior.
Instead mock ProfileSyncService to make functions return values you want.

[Nathan Parker, 2016-07-14 20:54:29]

lgtm

for safe_browsing.

If this is a re-land, it'd be easier to see what changed if you, 1) upload the
original patch, 2) upload the changes.

[Lei Zhang, 2016-07-15 00:19:49]

CL description - s/Readd/Reland/

Since the previous CL landed, you can refer to it as r405051.

[Lei Zhang, 2016-07-15 00:26:07]

https://codereview.chromium.org/2150903002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_uma_util_unittest.cc (right):

https://codereview.chromium.org/2150903002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_uma_util_unittest.cc:39:
static_cast<FakeSigninManagerForTesting*>(
As mentioned on the previous CL, this only works if you build a fake in the
first place. Has one been built? If so, how? Maybe add a LOG() statement to the
FakeSigninManagerForTesting ctor for local testing and see if it actually gets
created?

[stefanocs, 2016-07-15 01:02:49]

Description was changed from

==========
Readd hooks to permission layer for permission action reporting

Readd cl 2047253002 with resolve the memory.fyi issue.

Changes compared to 2047253002:
- Use ScopedCommandLine instead of the global variable command line.
- Change few lines of code in
chrome/browser/permissions/permission_uma_util_unittest.cc and
chrome/browser/permissions/permission_uma_util.h according to comments in
2047253002.

BUG=613883
==========

to

==========
Reland add hooks to permission layer for permission action reporting

Readd cl 2047253002 with resolve the memory.fyi issue.

Changes compared to 2047253002:
- Use ScopedCommandLine instead of the global variable command line.
- Change few lines of code in
chrome/browser/permissions/permission_uma_util_unittest.cc and
chrome/browser/permissions/permission_uma_util.h according to comments in
2047253002.

BUG=613883
==========

[stefanocs, 2016-07-15 01:03:53]

Description was changed from

==========
Reland add hooks to permission layer for permission action reporting

Readd cl 2047253002 with resolve the memory.fyi issue.

Changes compared to 2047253002:
- Use ScopedCommandLine instead of the global variable command line.
- Change few lines of code in
chrome/browser/permissions/permission_uma_util_unittest.cc and
chrome/browser/permissions/permission_uma_util.h according to comments in
2047253002.

BUG=613883
==========

to

==========
Reland add hooks to permission layer for permission action reporting

Readd r405051 with fix for the memory.fyi issue.

Changes compared to r405051:
- Use ScopedCommandLine instead of the global variable command line.
- Change few lines of code in
chrome/browser/permissions/permission_uma_util_unittest.cc and
chrome/browser/permissions/permission_uma_util.h according to comments in cl
2047253002.

BUG=613883
==========

[stefanocs, 2016-07-15 01:05:02]

https://codereview.chromium.org/2150903002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_uma_util_unittest.cc (right):

https://codereview.chromium.org/2150903002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_uma_util_unittest.cc:39:
static_cast<FakeSigninManagerForTesting*>(
On 2016/07/15 00:26:07, Lei Zhang wrote:
> As mentioned on the previous CL, this only works if you build a fake in the
> first place. Has one been built? If so, how? Maybe add a LOG() statement to
the
> FakeSigninManagerForTesting ctor for local testing and see if it actually gets
> created?

Yes, you were right, I have added this to the profile builder in the new patch
set.

https://codereview.chromium.org/2150903002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_uma_util_unittest.cc:44:
GetProfileSyncService()->backend_.reset(
On 2016/07/14 19:29:28, pavely wrote:
> Your code in permission_uma_util.cc relies on interface of ProfileSyncService.
> Could you use mock of ProfileSyncService instead of tweaking internal members
of
> the real one.
> Here is example of how it can be done:
>
https://cs.chromium.org/chromium/src/chrome/browser/password_manager/chrome_p...

Acknowledged.

https://codereview.chromium.org/2150903002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_uma_util_unittest.cc:50:
GetProfileSyncService()->sync_prefs_.SetKeepEverythingSynced(false);
On 2016/07/14 19:29:28, pavely wrote:
> You can create SyncPrefs object on the stack. No need to get one from
> ProfileSyncService.

Done.

https://codereview.chromium.org/2150903002/diff/1/components/browser_sync/bro...
File components/browser_sync/browser/profile_sync_service.h (right):

https://codereview.chromium.org/2150903002/diff/1/components/browser_sync/bro...
components/browser_sync/browser/profile_sync_service.h:672: friend class
PermissionUmaUtilTest;
On 2016/07/14 19:29:28, pavely wrote:
> In your code you are using public virtual functions of ProfileSyncService, you
> shouldn't open it's private members to the test and rely on internal behavior.
> Instead mock ProfileSyncService to make functions return values you want.

I didn't use mock but I used sync pref to set the preferable data types instead,
is that okay?

[Lei Zhang, 2016-07-15 01:15:54]

https://codereview.chromium.org/2150903002/diff/20001/chrome/browser/permissi...
File chrome/browser/permissions/permission_uma_util_unittest.cc (right):

https://codereview.chromium.org/2150903002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_uma_util_unittest.cc:146: new
sync_driver::SyncPrefs(profile()->GetPrefs());
Who deletes |sync_pref| ? Does this get leaked? Use std::unique_ptr, or add a
comment to explain why it doesn't leak.

[stefanocs, 2016-07-15 01:27:35]

https://codereview.chromium.org/2150903002/diff/20001/chrome/browser/permissi...
File chrome/browser/permissions/permission_uma_util_unittest.cc (right):

https://codereview.chromium.org/2150903002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_uma_util_unittest.cc:146: new
sync_driver::SyncPrefs(profile()->GetPrefs());
On 2016/07/15 01:15:54, Lei Zhang wrote:
> Who deletes |sync_pref| ? Does this get leaked? Use std::unique_ptr, or add a
> comment to explain why it doesn't leak.

Done. Sorry I missed that.

[Lei Zhang, 2016-07-15 01:32:29]

https://codereview.chromium.org/2150903002/diff/20001/chrome/browser/permissi...
File chrome/browser/permissions/permission_uma_util_unittest.cc (right):

https://codereview.chromium.org/2150903002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_uma_util_unittest.cc:146: new
sync_driver::SyncPrefs(profile()->GetPrefs());
On 2016/07/15 01:27:35, stefanocs wrote:
> On 2016/07/15 01:15:54, Lei Zhang wrote:
> > Who deletes |sync_pref| ? Does this get leaked? Use std::unique_ptr, or add
a
> > comment to explain why it doesn't leak.
> 
> Done. Sorry I missed that.

Thanks, CL generally lgtm but please work out the sync specifics with pavely@.

[raymes, 2016-07-18 02:52:38]

lgtm

[kcarattini, 2016-07-18 03:32:26]

lgtm

[pavely, 2016-07-18 05:21:55]

lgtm with small nit

https://codereview.chromium.org/2150903002/diff/40001/chrome/browser/permissi...
File chrome/browser/permissions/permission_uma_util_unittest.cc (right):

https://codereview.chromium.org/2150903002/diff/40001/chrome/browser/permissi...
chrome/browser/permissions/permission_uma_util_unittest.cc:145:
std::unique_ptr<sync_driver::SyncPrefs> sync_pref(
nit: You don't need to allocate SyncPrefs object from heap. Just create one on
the stack:
SyncPrefs sync_prefs(profile()->GetPrefs());

[stefanocs, 2016-07-18 05:46:00]

Thanks.

https://codereview.chromium.org/2150903002/diff/40001/chrome/browser/permissi...
File chrome/browser/permissions/permission_uma_util_unittest.cc (right):

https://codereview.chromium.org/2150903002/diff/40001/chrome/browser/permissi...
chrome/browser/permissions/permission_uma_util_unittest.cc:145:
std::unique_ptr<sync_driver::SyncPrefs> sync_pref(
On 2016/07/18 05:21:55, pavely wrote:
> nit: You don't need to allocate SyncPrefs object from heap. Just create one on
> the stack:
> SyncPrefs sync_prefs(profile()->GetPrefs());

Done.

[stefanocs, 2016-07-18 06:09:54]

The CQ bit was checked by stefanocs@google.com to run a CQ dry run

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

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 06:40:46]

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

[commit-bot: I haz the power, 2016-07-18 06:40:47]

Dry run: This issue passed the CQ dry run.

[Sergey Ulanov, 2016-07-19 00:31:12]

lgtm

[stefanocs, 2016-07-19 00:45:43]

The CQ bit was checked by stefanocs@google.com

[stefanocs, 2016-07-19 00:45:44]

The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org,
nparker@chromium.org, pavely@chromium.org, thestig@chromium.org,
raymes@chromium.org, kcarattini@chromium.org
Link to the patchset: https://codereview.chromium.org/2150903002/#ps60001
(title: "fix nits")

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

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 00:54:09]

Description was changed from

==========
Reland add hooks to permission layer for permission action reporting

Readd r405051 with fix for the memory.fyi issue.

Changes compared to r405051:
- Use ScopedCommandLine instead of the global variable command line.
- Change few lines of code in
chrome/browser/permissions/permission_uma_util_unittest.cc and
chrome/browser/permissions/permission_uma_util.h according to comments in cl
2047253002.

BUG=613883
==========

to

==========
Reland add hooks to permission layer for permission action reporting

Readd r405051 with fix for the memory.fyi issue.

Changes compared to r405051:
- Use ScopedCommandLine instead of the global variable command line.
- Change few lines of code in
chrome/browser/permissions/permission_uma_util_unittest.cc and
chrome/browser/permissions/permission_uma_util.h according to comments in cl
2047253002.

BUG=613883
==========

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

Committed patchset #4 (id:60001)

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

Description was changed from

==========
Reland add hooks to permission layer for permission action reporting

Readd r405051 with fix for the memory.fyi issue.

Changes compared to r405051:
- Use ScopedCommandLine instead of the global variable command line.
- Change few lines of code in
chrome/browser/permissions/permission_uma_util_unittest.cc and
chrome/browser/permissions/permission_uma_util.h according to comments in cl
2047253002.

BUG=613883
==========

to

==========
Reland add hooks to permission layer for permission action reporting

Readd r405051 with fix for the memory.fyi issue.

Changes compared to r405051:
- Use ScopedCommandLine instead of the global variable command line.
- Change few lines of code in
chrome/browser/permissions/permission_uma_util_unittest.cc and
chrome/browser/permissions/permission_uma_util.h according to comments in cl
2047253002.

BUG=613883

Committed: https://crrev.com/9274014049dc1cbfc0f8aaadcd86c321cb2fae72
Cr-Commit-Position: refs/heads/master@{#406166}
==========

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

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