Add a flag for reporting permission actions to Safe Browsing servers

Add a flag for reporting permission actions to Safe Browsing servers

BUG=613883
Committed: https://crrev.com/540173f9b77d793d320d25468d5a775965344894
Cr-Commit-Position: refs/heads/master@{#395777}

[stefanocs, 2016-05-23 02:30:21]

Description was changed from

==========
Add a flag for reporting permission actions to Safe Browing servers

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

to

==========
Add a flag for reporting permission actions to Safe Browing servers

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

[stefanocs, 2016-05-23 02:30:42]

stefanocs@google.com changed reviewers:
+ kcarattini@chromium.org, raymes@chromium.org

[kcarattini, 2016-05-23 04:00:54]

https://codereview.chromium.org/2006523002/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2006523002/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6507: +        Enables permission actions
reporting to Safe Browsing servers for Safe Browsing users.
"for Safe Browsing users" isn't quite correct, because we'll have to limit the
reporting to a subset of those users. What about "for opted in users"?

https://codereview.chromium.org/2006523002/diff/40001/chrome/common/chrome_sw...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/2006523002/diff/40001/chrome/common/chrome_sw...
chrome/common/chrome_switches.cc:304: // Safe Browsing users.
Again, change to "opted in users"

https://codereview.chromium.org/2006523002/diff/40001/chrome/common/chrome_sw...
chrome/common/chrome_switches.cc:485: // Safe Browsing users.
Same as above.

[stefanocs, 2016-05-23 04:16:36]

https://codereview.chromium.org/2006523002/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2006523002/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6507: +        Enables permission actions
reporting to Safe Browsing servers for Safe Browsing users.
On 2016/05/23 04:00:53, kcarattini wrote:
> "for Safe Browsing users" isn't quite correct, because we'll have to limit the
> reporting to a subset of those users. What about "for opted in users"?

Done.

https://codereview.chromium.org/2006523002/diff/40001/chrome/common/chrome_sw...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/2006523002/diff/40001/chrome/common/chrome_sw...
chrome/common/chrome_switches.cc:304: // Safe Browsing users.
On 2016/05/23 04:00:53, kcarattini wrote:
> Again, change to "opted in users"

Done.

https://codereview.chromium.org/2006523002/diff/40001/chrome/common/chrome_sw...
chrome/common/chrome_switches.cc:485: // Safe Browsing users.
On 2016/05/23 04:00:53, kcarattini wrote:
> Same as above.

Done.

[kcarattini, 2016-05-23 05:52:43]

https://codereview.chromium.org/2006523002/diff/60001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2006523002/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6504: +        Permissions Reporting
Change to "Permission Action Reporting"

https://codereview.chromium.org/2006523002/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6507: +        Enables permission actions
reporting to Safe Browsing servers for opted in users.
actions -> action

Here and throughout the cl.

[stefanocs, 2016-05-23 06:42:49]

https://codereview.chromium.org/2006523002/diff/60001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2006523002/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6504: +        Permissions Reporting
On 2016/05/23 05:52:43, kcarattini wrote:
> Change to "Permission Action Reporting"

Done.

https://codereview.chromium.org/2006523002/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6507: +        Enables permission actions
reporting to Safe Browsing servers for opted in users.
On 2016/05/23 05:52:43, kcarattini wrote:
> actions -> action
> 
> Here and throughout the cl.

Done.

[stefanocs, 2016-05-23 07:06:03]

Description was changed from

==========
Add a flag for reporting permission actions to Safe Browing servers

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

to

==========
Add a flag for reporting permission actions to Safe Browsing servers

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

[kcarattini, 2016-05-24 00:59:48]

lgtm. You'll need to add an OWNER of histograms.xml as a reviewer.

[raymes, 2016-05-24 01:47:43]

A few minor nits

https://codereview.chromium.org/2006523002/diff/20007/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2006523002/diff/20007/chrome/app/generated_re...
chrome/app/generated_resources.grd:6508: +      </message>
nit: we might as well sort this closer to (right before)
IDS_FLAGS_PERMISSIONS_BLACKLIST_NAME

https://codereview.chromium.org/2006523002/diff/20007/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2006523002/diff/20007/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1277:
switches::kDisablePermissionActionReporting)},
Same here: We can put this right before permissions-blacklist

https://codereview.chromium.org/2006523002/diff/20007/chrome/common/chrome_sw...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/2006523002/diff/20007/chrome/common/chrome_sw...
chrome/common/chrome_switches.cc:304: // opted in users.
nit: fill 80 chars on each line

https://codereview.chromium.org/2006523002/diff/20007/chrome/common/chrome_sw...
chrome/common/chrome_switches.cc:486: // opted in users.
nit: same here

https://codereview.chromium.org/2006523002/diff/20007/chrome/common/chrome_sw...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/2006523002/diff/20007/chrome/common/chrome_sw...
chrome/common/chrome_switches.h:92: extern const char
kDisablePermissionActionReporting[];
nit: these look mostly sorted so we should keep that if possible. I think this
should be moved 1 line up.

https://codereview.chromium.org/2006523002/diff/20007/chrome/common/chrome_sw...
chrome/common/chrome_switches.h:149: extern const char
kEnablePermissionActionReporting[];
Same here

https://codereview.chromium.org/2006523002/diff/20007/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2006523002/diff/20007/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:77299: +  <int value="-1478876902"
label="disable-permission-action-reporting"/>
Can we sort these closer to permissions-blacklist too or is the order based on
the hash?

[stefanocs, 2016-05-24 03:00:09]

https://codereview.chromium.org/2006523002/diff/20007/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2006523002/diff/20007/chrome/app/generated_re...
chrome/app/generated_resources.grd:6508: +      </message>
On 2016/05/24 01:47:42, raymes wrote:
> nit: we might as well sort this closer to (right before)
> IDS_FLAGS_PERMISSIONS_BLACKLIST_NAME

Done.

https://codereview.chromium.org/2006523002/diff/20007/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2006523002/diff/20007/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1277:
switches::kDisablePermissionActionReporting)},
On 2016/05/24 01:47:42, raymes wrote:
> Same here: We can put this right before permissions-blacklist

Done.

https://codereview.chromium.org/2006523002/diff/20007/chrome/common/chrome_sw...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/2006523002/diff/20007/chrome/common/chrome_sw...
chrome/common/chrome_switches.cc:304: // opted in users.
On 2016/05/24 01:47:43, raymes wrote:
> nit: fill 80 chars on each line

Done.

https://codereview.chromium.org/2006523002/diff/20007/chrome/common/chrome_sw...
chrome/common/chrome_switches.cc:486: // opted in users.
On 2016/05/24 01:47:43, raymes wrote:
> nit: same here

Done.

https://codereview.chromium.org/2006523002/diff/20007/chrome/common/chrome_sw...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/2006523002/diff/20007/chrome/common/chrome_sw...
chrome/common/chrome_switches.h:92: extern const char
kDisablePermissionActionReporting[];
On 2016/05/24 01:47:43, raymes wrote:
> nit: these look mostly sorted so we should keep that if possible. I think this
> should be moved 1 line up.

Done.

https://codereview.chromium.org/2006523002/diff/20007/chrome/common/chrome_sw...
chrome/common/chrome_switches.h:149: extern const char
kEnablePermissionActionReporting[];
On 2016/05/24 01:47:43, raymes wrote:
> Same here

Done.

https://codereview.chromium.org/2006523002/diff/20007/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2006523002/diff/20007/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:77299: +  <int value="-1478876902"
label="disable-permission-action-reporting"/>
On 2016/05/24 01:47:43, raymes wrote:
> Can we sort these closer to permissions-blacklist too or is the order based on
> the hash?

I think it is ordered by the hash value.

[raymes, 2016-05-24 03:47:13]

lgtm

[stefanocs, 2016-05-24 04:02:51]

stefanocs@google.com changed reviewers:
+ holte@chromium.org

[stefanocs, 2016-05-24 04:02:52]

Thanks.

+holte@chromium.org
Please review changes in histograms.xml

[Steven Holte, 2016-05-24 17:26:51]

lgtm

[stefanocs, 2016-05-25 00:16:16]

The CQ bit was checked by stefanocs@google.com

[stefanocs, 2016-05-25 00:16:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
kcarattini@chromium.org
Link to the patchset: https://codereview.chromium.org/2006523002/#ps100001
(title: "Reorder flag entry")

[commit-bot: I haz the power, 2016-05-25 00:16:45]

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

[commit-bot: I haz the power, 2016-05-25 02:03:26]

Description was changed from

==========
Add a flag for reporting permission actions to Safe Browsing servers

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

to

==========
Add a flag for reporting permission actions to Safe Browsing servers

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

[commit-bot: I haz the power, 2016-05-25 02:03:27]

Committed patchset #7 (id:100001)

[commit-bot: I haz the power, 2016-05-25 02:05:04]

Description was changed from

==========
Add a flag for reporting permission actions to Safe Browsing servers

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

to

==========
Add a flag for reporting permission actions to Safe Browsing servers

BUG=613883

Committed: https://crrev.com/540173f9b77d793d320d25468d5a775965344894
Cr-Commit-Position: refs/heads/master@{#395777}
==========

[commit-bot: I haz the power, 2016-05-25 02:05:05]

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