Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2001963003:
Add protocol buffer for reporting permission actions to Safe Browsing servers (Closed)
|
Created:
4 years, 7 months ago by stefanocs Modified:
4 years, 6 months ago CC:
chromium-reviews, grt+watch_chromium.org, extensions-reviews_chromium.org, chromium-apps-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org, awoz Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd protocol buffer for reporting permission actions to Safe Browsing servers
BUG=613883
Committed: https://crrev.com/fc47295f0af8d16a52d9a5458c36985cc5d4385a
Cr-Commit-Position: refs/heads/master@{#396675}
Patch Set 1 #
Total comments: 22
Patch Set 2 : #Patch Set 3 : Update TODO comment #
Total comments: 4
Patch Set 4 : Rename proto #
Total comments: 10
Patch Set 5 : Add unspecified action #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : Update platform enum #
Dependent Patchsets: Messages
Total messages: 43 (16 generated)
Description was changed from ========== Add protocol buffer for reporting permission actions to Safe Browing servers BUG=613883 ========== to ========== Add protocol buffer for reporting permission actions to Safe Browing servers BUG=613883 ==========
stefanocs@google.com changed reviewers: + kcarattini@chromium.org, raymes@chromium.org
kcarattini@chromium.org changed reviewers: + nparker@chromium.org
https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... File chrome/common/safe_browsing/permission_extension.proto (right): https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... chrome/common/safe_browsing/permission_extension.proto:73: // https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... Is this already in sync with the existing enum? You can just use it directly instead of defining a new one here.
kcarattini@chromium.org changed reviewers: - raymes@chromium.org
https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... File chrome/common/safe_browsing/permission_extension.proto (right): https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... chrome/common/safe_browsing/permission_extension.proto:73: // https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... On 2016/05/23 05:55:52, kcarattini wrote: > Is this already in sync with the existing enum? You can just use it directly > instead of defining a new one here. It seems it is not. How do I use it directly here?
kcarattini@chromium.org changed reviewers: + raymes@chromium.org
https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... File chrome/common/safe_browsing/permission_extension.proto (right): https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... chrome/common/safe_browsing/permission_extension.proto:73: // https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... On 2016/05/23 06:47:06, stefanocs wrote: > On 2016/05/23 05:55:52, kcarattini wrote: > > Is this already in sync with the existing enum? You can just use it directly > > instead of defining a new one here. > > It seems it is not. How do I use it directly here? Actually, Raymes, I'm curious about the enum in permission_type.h. Why does it start from 1? Is the 0 value ever used?
Thanks Stefano. Kendra might be able to help answer the questions. https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... File chrome/common/safe_browsing/permission_extension.proto (right): https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... chrome/common/safe_browsing/permission_extension.proto:23: // The platform nit: "." at the end of a comment https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... chrome/common/safe_browsing/permission_extension.proto:24: optional PlatformType platform_type = 3; Should this be optional? https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... chrome/common/safe_browsing/permission_extension.proto:30: optional Action action = 5; This says "required" in the comment but is marked as optional? https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... chrome/common/safe_browsing/permission_extension.proto:33: optional SourceUI source_ui = 6; Should this be optional? https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... chrome/common/safe_browsing/permission_extension.proto:40: PLATFORM_TYPE_UNSPECIFIED = 0; I know nothing about protocol buffers :P Is _UNSPECIFIED commonly used as a pattern for some reason? Would we ever expect the platform not to be specified? Would we want an _UNSPECIFIED for Action too? https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... chrome/common/safe_browsing/permission_extension.proto:66: CONTENT_SETTINGS = 3; Could we remove this and just use SITE_SETTINGS? Conceptually they are the same surface (content settings will be going away soon). We will be able to distinguish from the platform type anyway. Kendra - wdyt? https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... chrome/common/safe_browsing/permission_extension.proto:70: // The various types of permissions. nit: fill 80 chars https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... chrome/common/safe_browsing/permission_extension.proto:73: // https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... I'm not sure - I believe 0 is never used.
Description was changed from ========== Add protocol buffer for reporting permission actions to Safe Browing servers BUG=613883 ========== to ========== Add protocol buffer for reporting permission actions to Safe Browsing servers BUG=613883 ==========
https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... File chrome/common/safe_browsing/permission_extension.proto (right): https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... chrome/common/safe_browsing/permission_extension.proto:24: optional PlatformType platform_type = 3; On 2016/05/24 03:01:45, raymes wrote: > Should this be optional? I tried to keep these optional in-line with proto3 guidelines. Making something required can cause crashes, so it's better to define them optional and handle failures gracefully. https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... chrome/common/safe_browsing/permission_extension.proto:30: optional Action action = 5; On 2016/05/24 03:01:45, raymes wrote: > This says "required" in the comment but is marked as optional? See above. https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... chrome/common/safe_browsing/permission_extension.proto:33: optional SourceUI source_ui = 6; On 2016/05/24 03:01:45, raymes wrote: > Should this be optional? See above. https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... chrome/common/safe_browsing/permission_extension.proto:40: PLATFORM_TYPE_UNSPECIFIED = 0; On 2016/05/24 03:01:45, raymes wrote: > I know nothing about protocol buffers :P Is _UNSPECIFIED commonly used as a > pattern for some reason? Would we ever expect the platform not to be specified? > Would we want an _UNSPECIFIED for Action too? It was suggested to have an unspecified value for anything that had the possibility of being unset to avoid confusion with an actual 0 value. This matches the report logs proto design. https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... chrome/common/safe_browsing/permission_extension.proto:66: CONTENT_SETTINGS = 3; On 2016/05/24 03:01:45, raymes wrote: > Could we remove this and just use SITE_SETTINGS? Conceptually they are the same > surface (content settings will be going away soon). We will be able to > distinguish from the platform type anyway. Kendra - wdyt? I'm fine with that. We'll need to change the logs proto to match. Stefano, can you put a TODO in the comments to remember to change the logs proto? https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... chrome/common/safe_browsing/permission_extension.proto:73: // https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... On 2016/05/24 03:01:45, raymes wrote: > I'm not sure - I believe 0 is never used. I spoke with Raymes offline, and we decided to keep the two enums separate. Just change the comment to say "This should stay in sync with the corresponding Safe Browsing logs enum" or something to that effect.
https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... File chrome/common/safe_browsing/permission_extension.proto (right): https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... chrome/common/safe_browsing/permission_extension.proto:23: // The platform On 2016/05/24 03:01:45, raymes wrote: > nit: "." at the end of a comment Done. https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... chrome/common/safe_browsing/permission_extension.proto:66: CONTENT_SETTINGS = 3; On 2016/05/24 03:01:45, raymes wrote: > Could we remove this and just use SITE_SETTINGS? Conceptually they are the same > surface (content settings will be going away soon). We will be able to > distinguish from the platform type anyway. Kendra - wdyt? Done. https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... chrome/common/safe_browsing/permission_extension.proto:66: CONTENT_SETTINGS = 3; On 2016/05/24 06:01:01, kcarattini wrote: > On 2016/05/24 03:01:45, raymes wrote: > > Could we remove this and just use SITE_SETTINGS? Conceptually they are the > same > > surface (content settings will be going away soon). We will be able to > > distinguish from the platform type anyway. Kendra - wdyt? > > I'm fine with that. We'll need to change the logs proto to match. Stefano, can > you put a TODO in the comments to remember to change the logs proto? Done. https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... chrome/common/safe_browsing/permission_extension.proto:73: // https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... On 2016/05/24 06:01:01, kcarattini wrote: > On 2016/05/24 03:01:45, raymes wrote: > > I'm not sure - I believe 0 is never used. > > I spoke with Raymes offline, and we decided to keep the two enums separate. Just > change the comment to say "This should stay in sync with the corresponding Safe > Browsing logs enum" or something to that effect. Done.
How about changing the file name of permission_extension.proto to permission_report.proto, since it isn't actually an extension as far as Chrome is concerned? https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... File chrome/common/safe_browsing/permission_extension.proto (right): https://codereview.chromium.org/2001963003/diff/1/chrome/common/safe_browsing... chrome/common/safe_browsing/permission_extension.proto:16: message PermissionReportExtension { This isn't actually an extension in this context. What about just "PermissionsReport"? https://codereview.chromium.org/2001963003/diff/40001/chrome/common/safe_brow... File chrome/common/safe_browsing/permission_extension.proto (right): https://codereview.chromium.org/2001963003/diff/40001/chrome/common/safe_brow... chrome/common/safe_browsing/permission_extension.proto:75: // disabling of all APIs for a given full hash. The comment isn't relevant in this context, please remove. https://codereview.chromium.org/2001963003/diff/40001/chrome/common/safe_brow... chrome/common/safe_browsing/permission_extension.proto:77: remove extra line
cwj4au@gmail.com changed reviewers: + Cwj4au@gmail.com
cwj4au@gmail.com changed reviewers: + cwj4au@gmail.com
lgtm
stefanocs@google.com changed reviewers: - Cwj4au@gmail.com, cwj4au@gmail.com
Sure, I have renamed the file name to permission_report.proto. https://codereview.chromium.org/2001963003/diff/40001/chrome/common/safe_brow... File chrome/common/safe_browsing/permission_extension.proto (right): https://codereview.chromium.org/2001963003/diff/40001/chrome/common/safe_brow... chrome/common/safe_browsing/permission_extension.proto:75: // disabling of all APIs for a given full hash. On 2016/05/25 00:21:29, kcarattini wrote: > The comment isn't relevant in this context, please remove. Done. https://codereview.chromium.org/2001963003/diff/40001/chrome/common/safe_brow... chrome/common/safe_browsing/permission_extension.proto:77: On 2016/05/25 00:21:29, kcarattini wrote: > remove extra line Done.
lgtm
lgtm
lgtm https://codereview.chromium.org/2001963003/diff/60001/chrome/common/safe_brow... File chrome/common/safe_browsing/permission_report.proto (right): https://codereview.chromium.org/2001963003/diff/60001/chrome/common/safe_brow... chrome/common/safe_browsing/permission_report.proto:36: repeated string field_trials = 7; Once you figure out what form the field trial strings will take, add a comment here to describe it (can be later). https://codereview.chromium.org/2001963003/diff/60001/chrome/common/safe_brow... chrome/common/safe_browsing/permission_report.proto:54: GRANTED = 0; I have a theory, not sure if it's a good one, that 0 shouldn't be used for anything other an "unspecified" or "unknown," so that if it got left unset and you read the default value (0), you wouldn't be fooled into thinking it had a concrete value. So maybe start this at 1. https://codereview.chromium.org/2001963003/diff/60001/chrome/common/safe_brow... chrome/common/safe_browsing/permission_report.proto:72: // corresponding Safe Browsing logs enum. How about a comment up top to make it easy for people to find the corresponding google3 proto?
https://codereview.chromium.org/2001963003/diff/60001/chrome/common/safe_brow... File chrome/common/safe_browsing/permission_report.proto (right): https://codereview.chromium.org/2001963003/diff/60001/chrome/common/safe_brow... chrome/common/safe_browsing/permission_report.proto:36: repeated string field_trials = 7; On 2016/05/25 21:13:02, Nathan Parker wrote: > Once you figure out what form the field trial strings will take, add a comment > here to describe it (can be later). Stefano, please add a TOTO in here to update the field_trials field to store hashes instead of string. https://codereview.chromium.org/2001963003/diff/60001/chrome/common/safe_brow... chrome/common/safe_browsing/permission_report.proto:54: GRANTED = 0; On 2016/05/25 21:13:02, Nathan Parker wrote: > I have a theory, not sure if it's a good one, that 0 shouldn't be used for > anything other an "unspecified" or "unknown," so that if it got left unset and > you read the default value (0), you wouldn't be fooled into thinking it had a > concrete value. So maybe start this at 1. I don't disagree with you in theory, but in this case I was trying to keep things in sync with the enum in permission_uma_util.h: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/per... Not only does that start at 0, but its final value is NUM, which wouldn't actually be the number of permissions anymore if the values started at 1. Raymes, what's your opinion on whether we should bother trying to keep these enums in sync? After all, we decided not to just combine them into one for the very reason that they might diverge. I would still find it a bit confusing to have them "off-by-one" though. https://codereview.chromium.org/2001963003/diff/60001/chrome/common/safe_brow... chrome/common/safe_browsing/permission_report.proto:72: // corresponding Safe Browsing logs enum. On 2016/05/25 21:13:02, Nathan Parker wrote: > How about a comment up top to make it easy for people to find the corresponding > google3 proto? Do we usually put pointers into google3 code in chromium? I would have thought not since it isn't accessible outside code. Regardless a comment at the top to stay in sync with the Safe Browsing logs proto file is a good idea.
https://codereview.chromium.org/2001963003/diff/60001/chrome/common/safe_brow... File chrome/common/safe_browsing/permission_report.proto (right): https://codereview.chromium.org/2001963003/diff/60001/chrome/common/safe_brow... chrome/common/safe_browsing/permission_report.proto:54: GRANTED = 0; On 2016/05/26 01:53:26, kcarattini wrote: > On 2016/05/25 21:13:02, Nathan Parker wrote: > > I have a theory, not sure if it's a good one, that 0 shouldn't be used for > > anything other an "unspecified" or "unknown," so that if it got left unset and > > you read the default value (0), you wouldn't be fooled into thinking it had a > > concrete value. So maybe start this at 1. > > I don't disagree with you in theory, but in this case I was trying to keep > things in sync with the enum in permission_uma_util.h: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/per... > > Not only does that start at 0, but its final value is NUM, which wouldn't > actually be the number of permissions anymore if the values started at 1. > > Raymes, what's your opinion on whether we should bother trying to keep these > enums in sync? After all, we decided not to just combine them into one for the > very reason that they might diverge. I would still find it a bit confusing to > have them "off-by-one" though. Just had a lengthy discussion about PB enums! I agree with Nathan, let's add an unspecified value as 0 here and a TODO to update this in the SB proto as well. We should probably also add a comment that this is intentionally different from the enum in permission_uma_util.h
https://codereview.chromium.org/2001963003/diff/60001/chrome/common/safe_brow... File chrome/common/safe_browsing/permission_report.proto (right): https://codereview.chromium.org/2001963003/diff/60001/chrome/common/safe_brow... chrome/common/safe_browsing/permission_report.proto:36: repeated string field_trials = 7; On 2016/05/26 01:53:26, kcarattini wrote: > On 2016/05/25 21:13:02, Nathan Parker wrote: > > Once you figure out what form the field trial strings will take, add a comment > > here to describe it (can be later). > > Stefano, please add a TOTO in here to update the field_trials field to store > hashes instead of string. Done. https://codereview.chromium.org/2001963003/diff/60001/chrome/common/safe_brow... chrome/common/safe_browsing/permission_report.proto:54: GRANTED = 0; On 2016/05/26 03:02:48, kcarattini wrote: > On 2016/05/26 01:53:26, kcarattini wrote: > > On 2016/05/25 21:13:02, Nathan Parker wrote: > > > I have a theory, not sure if it's a good one, that 0 shouldn't be used for > > > anything other an "unspecified" or "unknown," so that if it got left unset > and > > > you read the default value (0), you wouldn't be fooled into thinking it had > a > > > concrete value. So maybe start this at 1. > > > > I don't disagree with you in theory, but in this case I was trying to keep > > things in sync with the enum in permission_uma_util.h: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/per... > > > > Not only does that start at 0, but its final value is NUM, which wouldn't > > actually be the number of permissions anymore if the values started at 1. > > > > Raymes, what's your opinion on whether we should bother trying to keep these > > enums in sync? After all, we decided not to just combine them into one for the > > very reason that they might diverge. I would still find it a bit confusing to > > have them "off-by-one" though. > > Just had a lengthy discussion about PB enums! I agree with Nathan, let's add an > unspecified value as 0 here and a TODO to update this in the SB proto as well. > We should probably also add a comment that this is intentionally different from > the enum in permission_uma_util.h Done.
https://codereview.chromium.org/2001963003/diff/60001/chrome/common/safe_brow... File chrome/common/safe_browsing/permission_report.proto (right): https://codereview.chromium.org/2001963003/diff/60001/chrome/common/safe_brow... chrome/common/safe_browsing/permission_report.proto:72: // corresponding Safe Browsing logs enum. On 2016/05/25 21:13:02, Nathan Parker wrote: > How about a comment up top to make it easy for people to find the corresponding > google3 proto? Done.
lgtm
The CQ bit was checked by stefanocs@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from Cwj4au@gmail.com, nparker@chromium.org, raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2001963003/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2001963003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2001963003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by stefanocs@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org, kcarattini@chromium.org, raymes@chromium.org, Cwj4au@gmail.com Link to the patchset: https://codereview.chromium.org/2001963003/#ps140001 (title: "Update platform enum")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2001963003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2001963003/140001
Message was sent while issue was closed.
Description was changed from ========== Add protocol buffer for reporting permission actions to Safe Browsing servers BUG=613883 ========== to ========== Add protocol buffer for reporting permission actions to Safe Browsing servers BUG=613883 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add protocol buffer for reporting permission actions to Safe Browsing servers BUG=613883 ========== to ========== Add protocol buffer for reporting permission actions to Safe Browsing servers BUG=613883 Committed: https://crrev.com/fc47295f0af8d16a52d9a5458c36985cc5d4385a Cr-Commit-Position: refs/heads/master@{#396675} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/fc47295f0af8d16a52d9a5458c36985cc5d4385a Cr-Commit-Position: refs/heads/master@{#396675} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
