Add SourceUI field to permission report

Add SourceUI field to permission report

PAGE_ACTION also added as a new possible SourceUI value to both proto file enum and code enum.

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

[stefanocs, 2016-06-17 05:57:00]

Description was changed from

==========
Add SourceUI field to permission report

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

to

==========
Add SourceUI field to permission report

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

[stefanocs, 2016-06-17 05:57:01]

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

[stefanocs, 2016-06-20 01:28:53]

Description was changed from

==========
Add SourceUI field to permission report

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

to

==========
Add SourceUI field to permission report

PAGE_ACTION also added as a new possible SourceUI value to both proto file enum
and code enum.

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

[kcarattini, 2016-07-06 06:54:09]

https://codereview.chromium.org/2075523002/diff/60001/chrome/browser/media/me...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/2075523002/diff/60001/chrome/browser/media/me...
chrome/browser/media/media_stream_devices_controller.cc:74: using
PermissionActionCallback = base::Callback<
Make sure you coordinate with Liu who is refactoring in this area of the code.

https://codereview.chromium.org/2075523002/diff/60001/chrome/browser/permissi...
File chrome/browser/permissions/permission_uma_util.cc (right):

https://codereview.chromium.org/2075523002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_uma_util.cc:231: SourceUI source_ui,
Add a TODO to add reporting for revocations from the Page Action bubbles.

https://codereview.chromium.org/2075523002/diff/60001/chrome/browser/permissi...
File chrome/browser/permissions/permission_uma_util.h (right):

https://codereview.chromium.org/2075523002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_uma_util.h:38: enum SourceUI {
Add a comment to keep this in sync with the Report proto field.

[stefanocs, 2016-07-07 01:26:03]

https://codereview.chromium.org/2075523002/diff/60001/chrome/browser/media/me...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/2075523002/diff/60001/chrome/browser/media/me...
chrome/browser/media/media_stream_devices_controller.cc:74: using
PermissionActionCallback = base::Callback<
On 2016/07/06 06:54:08, kcarattini wrote:
> Make sure you coordinate with Liu who is refactoring in this area of the code.

Done. She is okay with this.

https://codereview.chromium.org/2075523002/diff/60001/chrome/browser/permissi...
File chrome/browser/permissions/permission_uma_util.cc (right):

https://codereview.chromium.org/2075523002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_uma_util.cc:231: SourceUI source_ui,
On 2016/07/06 06:54:08, kcarattini wrote:
> Add a TODO to add reporting for revocations from the Page Action bubbles.

Done. Revocations from the Page Action bubbles had been recorded but it is
considered as SITE_SETTINGS in this CL
(chrome/browser/permissions/permission_util.cc). So I added a comment below.

https://codereview.chromium.org/2075523002/diff/60001/chrome/browser/permissi...
File chrome/browser/permissions/permission_uma_util.h (right):

https://codereview.chromium.org/2075523002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_uma_util.h:38: enum SourceUI {
On 2016/07/06 06:54:08, kcarattini wrote:
> Add a comment to keep this in sync with the Report proto field.

Done.

[kcarattini, 2016-07-07 01:57:51]

https://codereview.chromium.org/2075523002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_util.cc (right):

https://codereview.chromium.org/2075523002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_util.cc:104: // source UI instead of
SITE_SETTINGS source UI.
Is this complex enough to warrant a separate cl? Let's chat offline.

[stefanocs, 2016-07-07 03:50:55]

https://codereview.chromium.org/2075523002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_util.cc (right):

https://codereview.chromium.org/2075523002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_util.cc:104: // source UI instead of
SITE_SETTINGS source UI.
On 2016/07/07 01:57:51, kcarattini wrote:
> Is this complex enough to warrant a separate cl? Let's chat offline.

After chatting offline, this will be fixed in the 2075053002.

[raymes, 2016-07-12 00:54:36]

https://codereview.chromium.org/2075523002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base.cc (right):

https://codereview.chromium.org/2075523002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:220: PROMPT, profile_);
As discussed, I think in most cases we don't need to pass the SourceUI here
because we can infer it from the operation (e.g. PermissionGranted can only be
called for prompts).

[kcarattini, 2016-07-12 01:01:42]

https://codereview.chromium.org/2075523002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base.cc (right):

https://codereview.chromium.org/2075523002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:220: PROMPT, profile_);
On 2016/07/12 00:54:36, raymes wrote:
> As discussed, I think in most cases we don't need to pass the SourceUI here
> because we can infer it from the operation (e.g. PermissionGranted can only be
> called for prompts).

I assume that we don't call it for anything but the prompt now, but will that
change? E.g. OIB or Site Settings?

[raymes, 2016-07-12 01:43:31]

My impression was we would just record revocations for other surfaces for
the time being? We could always add the parameters in again later when/if
they are needed.

On Tue, 12 Jul 2016 at 11:01 <kcarattini@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/2075523002/diff/100001/chrome/browser/permiss...
> File chrome/browser/permissions/permission_context_base.cc (right):
>
>
>
https://codereview.chromium.org/2075523002/diff/100001/chrome/browser/permiss...
> chrome/browser/permissions/permission_context_base.cc:220: PROMPT,
> profile_);
> On 2016/07/12 00:54:36, raymes wrote:
> > As discussed, I think in most cases we don't need to pass the SourceUI
> here
> > because we can infer it from the operation (e.g. PermissionGranted can
> only be
> > called for prompts).
>
> I assume that we don't call it for anything but the prompt now, but will
> that change? E.g. OIB or Site Settings?
>
> https://codereview.chromium.org/2075523002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kcarattini, 2016-07-12 01:44:22]

sgtm

On 12 July 2016 at 11:43, Raymes Khoury <raymes@chromium.org> wrote:

> My impression was we would just record revocations for other surfaces for
> the time being? We could always add the parameters in again later when/if
> they are needed.
>
>
> On Tue, 12 Jul 2016 at 11:01 <kcarattini@chromium.org> wrote:
>
>>
>>
>>
https://codereview.chromium.org/2075523002/diff/100001/chrome/browser/permiss...
>> File chrome/browser/permissions/permission_context_base.cc (right):
>>
>>
>>
https://codereview.chromium.org/2075523002/diff/100001/chrome/browser/permiss...
>> chrome/browser/permissions/permission_context_base.cc:220: PROMPT,
>> profile_);
>> On 2016/07/12 00:54:36, raymes wrote:
>> > As discussed, I think in most cases we don't need to pass the SourceUI
>> here
>> > because we can infer it from the operation (e.g. PermissionGranted can
>> only be
>> > called for prompts).
>>
>> I assume that we don't call it for anything but the prompt now, but will
>> that change? E.g. OIB or Site Settings?
>>
>> https://codereview.chromium.org/2075523002/
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[stefanocs, 2016-07-12 04:33:33]

https://codereview.chromium.org/2075523002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base.cc (right):

https://codereview.chromium.org/2075523002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:220: PROMPT, profile_);
On 2016/07/12 00:54:36, raymes wrote:
> As discussed, I think in most cases we don't need to pass the SourceUI here
> because we can infer it from the operation (e.g. PermissionGranted can only be
> called for prompts).

Done.

[raymes, 2016-07-12 07:18:15]

https://codereview.chromium.org/2075523002/diff/120001/chrome/browser/permiss...
File chrome/browser/permissions/permission_uma_util.h (right):

https://codereview.chromium.org/2075523002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_uma_util.h:40: enum SourceUI {
nit: I think we should call this PermissionSourceUI. 

Also, consider using an enum class here. It will make the enum values less
ambiguous when they are used (e.g. they will become PermissionSourceUI::OIB
instead of just OIB).

https://codereview.chromium.org/2075523002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_uma_util.h:116: SourceUI source_ui,
nit: I think this should go before the origin (grouped with the
PermissionAction)

https://codereview.chromium.org/2075523002/diff/120001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right):

https://codereview.chromium.org/2075523002/diff/120001/chrome/browser/safe_br...
chrome/browser/safe_browsing/permission_reporter_unittest.cc:101:
EXPECT_EQ(kDummyPermissionReportSourceUI, permission_report.source_ui());
nit: maybe just inline the 3 constants on the lhs of these EXPECTs above (since
they're only used here)? I'll leave it up to you though.

[stefanocs, 2016-07-13 01:23:52]

https://codereview.chromium.org/2075523002/diff/120001/chrome/browser/permiss...
File chrome/browser/permissions/permission_uma_util.h (right):

https://codereview.chromium.org/2075523002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_uma_util.h:40: enum SourceUI {
On 2016/07/12 07:18:14, raymes wrote:
> nit: I think we should call this PermissionSourceUI. 
> 
> Also, consider using an enum class here. It will make the enum values less
> ambiguous when they are used (e.g. they will become PermissionSourceUI::OIB
> instead of just OIB).

Done.

https://codereview.chromium.org/2075523002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_uma_util.h:116: SourceUI source_ui,
On 2016/07/12 07:18:14, raymes wrote:
> nit: I think this should go before the origin (grouped with the
> PermissionAction)

Done.

https://codereview.chromium.org/2075523002/diff/120001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right):

https://codereview.chromium.org/2075523002/diff/120001/chrome/browser/safe_br...
chrome/browser/safe_browsing/permission_reporter_unittest.cc:101:
EXPECT_EQ(kDummyPermissionReportSourceUI, permission_report.source_ui());
On 2016/07/12 07:18:14, raymes wrote:
> nit: maybe just inline the 3 constants on the lhs of these EXPECTs above
(since
> they're only used here)? I'll leave it up to you though.

Done.

[raymes, 2016-07-13 01:35:05]

lgtm

[kcarattini, 2016-07-13 01:42:31]

lgtm

[stefanocs, 2016-07-13 05:54:01]

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

[stefanocs, 2016-07-13 05:54:02]

Nathan, please review changes in chrome/browser/safe_browsing and
chrome/common/safe_browsing.

Thanks

[Nathan Parker, 2016-07-14 21:43:42]

lgtm

[stefanocs, 2016-07-19 10:17:06]

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

[commit-bot: I haz the power, 2016-07-19 10:17:43]

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 10:19:50]

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

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

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[stefanocs, 2016-07-19 10:58:05]

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

[commit-bot: I haz the power, 2016-07-19 10:58:23]

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 11:06:58]

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

[commit-bot: I haz the power, 2016-07-19 11:06:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[stefanocs, 2016-07-19 11:20:42]

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

[commit-bot: I haz the power, 2016-07-19 11:20:57]

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 13:12:43]

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

[commit-bot: I haz the power, 2016-07-19 13:12:43]

Dry run: This issue passed the CQ dry run.

[stefanocs, 2016-07-21 00:59:45]

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

[commit-bot: I haz the power, 2016-07-21 01:00: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-21 01:20:10]

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

[commit-bot: I haz the power, 2016-07-21 01:20:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[stefanocs, 2016-07-21 04:15:59]

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

[commit-bot: I haz the power, 2016-07-21 04:16:11]

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-21 04:59:08]

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

[commit-bot: I haz the power, 2016-07-21 04:59:09]

Dry run: This issue passed the CQ dry run.

[stefanocs, 2016-07-21 05:03:33]

The CQ bit was checked by stefanocs@google.com

[stefanocs, 2016-07-21 05:03:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
kcarattini@chromium.org, raymes@chromium.org, nparker@chromium.org
Link to the patchset: https://codereview.chromium.org/2075523002/#ps260001
(title: "merge")

[commit-bot: I haz the power, 2016-07-21 05:03:43]

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-21 05:07:16]

Description was changed from

==========
Add SourceUI field to permission report

PAGE_ACTION also added as a new possible SourceUI value to both proto file enum
and code enum.

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

to

==========
Add SourceUI field to permission report

PAGE_ACTION also added as a new possible SourceUI value to both proto file enum
and code enum.

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

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

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-07-21 05:09:24]

Description was changed from

==========
Add SourceUI field to permission report

PAGE_ACTION also added as a new possible SourceUI value to both proto file enum
and code enum.

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

to

==========
Add SourceUI field to permission report

PAGE_ACTION also added as a new possible SourceUI value to both proto file enum
and code enum.

BUG=613883

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

[commit-bot: I haz the power, 2016-07-21 05:09:26]

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