Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2073713002:
Add field trials and platform to permission report (Closed)
|
Created:
4 years, 6 months ago by stefanocs Modified:
4 years, 5 months ago CC:
chromium-reviews, grt+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@permission-reporter-implementation Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd field trials and platform to permission report
BUG=613883
Committed: https://crrev.com/9542b193976dacb41d703f00884c399777f474d3
Cr-Commit-Position: refs/heads/master@{#402628}
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 4
Patch Set 3 : Rebase #Patch Set 4 : Add tests #
Total comments: 14
Patch Set 5 : Move send report with field trials test #Patch Set 6 : Add error message #
Total comments: 10
Patch Set 7 : Resolve nits #
Total comments: 4
Patch Set 8 : Resolve nits #
Total comments: 6
Patch Set 9 : Move typedef ActiveGroupIdSet #
Messages
Total messages: 28 (8 generated)
Description was changed from ========== Add field trials and platform to permission report BUG=613883 ========== to ========== Add field trials and platform to permission report BUG=613883 ==========
stefanocs@google.com changed reviewers: + kcarattini@chromium.org, raymes@chromium.org
This patch will be rebased once 2037693004 is merged, so you don't have to review permission_report.proto.
We'll need to add tests for this to the unittest. https://codereview.chromium.org/2073713002/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2073713002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.cc:114: report.set_platform_type(PermissionReport::PLATFORM_TYPE_UNSPECIFIED); Perhaps add a #error here instead of setting UNSPECIFIED? I don't think it should be compiled on other platforms but could be wrong (in which case it would be good to know what platforms it gets compiled on). https://codereview.chromium.org/2073713002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.cc:118: variations::GetFieldTrialActiveGroupIds(&active_groups_ids); I think Kendra mentioned we may only want to add field trials that we're interested in. But adding them all doesn't seem too bad either (and seems safer) as long as it doesn't make the reports too big. Kendra, wdyt?
https://codereview.chromium.org/2073713002/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2073713002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.cc:118: variations::GetFieldTrialActiveGroupIds(&active_groups_ids); On 2016/06/20 01:32:11, raymes wrote: > I think Kendra mentioned we may only want to add field trials that we're > interested in. But adding them all doesn't seem too bad either (and seems safer) > as long as it doesn't make the reports too big. > > Kendra, wdyt? Hmm, I don't want to bloat the reports with unnecessary information, but I don't know a way to only add ones we care about (without waiting for the Chrome release process to change the code). It seems it would be quicker just to add them all?
Tests added https://codereview.chromium.org/2073713002/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2073713002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.cc:114: report.set_platform_type(PermissionReport::PLATFORM_TYPE_UNSPECIFIED); On 2016/06/20 01:32:11, raymes wrote: > Perhaps add a #error here instead of setting UNSPECIFIED? I don't think it > should be compiled on other platforms but could be wrong (in which case it would > be good to know what platforms it gets compiled on). Done.
Thanks! https://codereview.chromium.org/2073713002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2073713002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.cc:114: #error I think you can add a message after #error e.g. #error Unsupported platform. https://codereview.chromium.org/2073713002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2073713002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter_unittest.cc:94: base::FieldTrialList field_trial_list(nullptr); nit: is this needed? https://codereview.chromium.org/2073713002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter_unittest.cc:103: trial_one); I guess this isn't needed for the on-by-default feature? we could leave it out https://codereview.chromium.org/2073713002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter_unittest.cc:113: base::FeatureList::IsEnabled(kFeatureOffByDefault); These just check whether the feature is enabled, right? Did you meant to use EXPECT_TRUE here? https://codereview.chromium.org/2073713002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter_unittest.cc:116: EXPECT_TRUE(base::FieldTrialList::IsTrialActive(trial_two->trial_name())); Since there is a lot of setup/checking to test the field trials, perhaps we could put it into its own test? (SendReportWithFieldTrials) https://codereview.chromium.org/2073713002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter_unittest.cc:149: expected_group_ids.erase(expected_group); Could you just call erase on group_id?
https://codereview.chromium.org/2073713002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2073713002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter.cc:114: #error On 2016/06/22 01:23:44, raymes wrote: > I think you can add a message after #error e.g. > #error Unsupported platform. Done. https://codereview.chromium.org/2073713002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2073713002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter_unittest.cc:94: base::FieldTrialList field_trial_list(nullptr); On 2016/06/22 01:23:44, raymes wrote: > nit: is this needed? Yes, the constructor will initialize a singleton instance of FieldTrialList. This instance will be used when calling FieldTrialList::CreateFieldTrial. https://cs.chromium.org/chromium/src/base/metrics/field_trial.cc?rcl=0&l=311 https://codereview.chromium.org/2073713002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter_unittest.cc:103: trial_one); On 2016/06/22 01:23:44, raymes wrote: > I guess this isn't needed for the on-by-default feature? we could leave it out The test doesn't seem to work with this removed. Why do you isn't needed? https://codereview.chromium.org/2073713002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter_unittest.cc:113: base::FeatureList::IsEnabled(kFeatureOffByDefault); On 2016/06/22 01:23:44, raymes wrote: > These just check whether the feature is enabled, right? Did you meant to use > EXPECT_TRUE here? While this seems to be a non-modifying function, calling this function seems to be necessary to activate the field trials. https://cs.chromium.org/chromium/src/base/feature_list.cc?rcl=1466536938&l=211 https://codereview.chromium.org/2073713002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter_unittest.cc:116: EXPECT_TRUE(base::FieldTrialList::IsTrialActive(trial_two->trial_name())); On 2016/06/22 01:23:44, raymes wrote: > Since there is a lot of setup/checking to test the field trials, perhaps we > could put it into its own test? (SendReportWithFieldTrials) Done. https://codereview.chromium.org/2073713002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter_unittest.cc:149: expected_group_ids.erase(expected_group); On 2016/06/22 01:23:44, raymes wrote: > Could you just call erase on group_id? Done.
lgtm https://codereview.chromium.org/2073713002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2073713002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter_unittest.cc:103: trial_one); On 2016/06/22 02:07:06, stefanocs wrote: > On 2016/06/22 01:23:44, raymes wrote: > > I guess this isn't needed for the on-by-default feature? we could leave it out > > The test doesn't seem to work with this removed. Why do you isn't needed? Oh I assumed the default would apply in that case. It's ok to leave it if necessary https://codereview.chromium.org/2073713002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/permission_reporter_unittest.cc:113: base::FeatureList::IsEnabled(kFeatureOffByDefault); On 2016/06/22 02:07:06, stefanocs wrote: > On 2016/06/22 01:23:44, raymes wrote: > > These just check whether the feature is enabled, right? Did you meant to use > > EXPECT_TRUE here? > > While this seems to be a non-modifying function, calling this function seems to > be necessary to activate the field trials. > > https://cs.chromium.org/chromium/src/base/feature_list.cc?rcl=1466536938&l=211 I see. Perhaps make a note in the comment https://codereview.chromium.org/2073713002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2073713002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:93: // Send permission report. nit: remove https://codereview.chromium.org/2073713002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:144: // Send permission report. nit: remove https://codereview.chromium.org/2073713002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:162: EXPECT_FALSE(expected_group_ids.find(group_id) == expected_group_ids.end()); I don't think this is needed - you can check the return value of the erase call below if you want to ensure it exists.
https://codereview.chromium.org/2073713002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2073713002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:106: report.set_action(PermissionActionForReport(action)); nit: add newline below https://codereview.chromium.org/2073713002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:115: #endif nit: add newline below
https://codereview.chromium.org/2073713002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2073713002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:106: report.set_action(PermissionActionForReport(action)); On 2016/06/22 02:17:59, raymes wrote: > nit: add newline below Done. https://codereview.chromium.org/2073713002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:115: #endif On 2016/06/22 02:17:59, raymes wrote: > nit: add newline below Done. https://codereview.chromium.org/2073713002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2073713002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:93: // Send permission report. On 2016/06/22 02:16:27, raymes wrote: > nit: remove Done. https://codereview.chromium.org/2073713002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:144: // Send permission report. On 2016/06/22 02:16:27, raymes wrote: > nit: remove Done. https://codereview.chromium.org/2073713002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:162: EXPECT_FALSE(expected_group_ids.find(group_id) == expected_group_ids.end()); On 2016/06/22 02:16:27, raymes wrote: > I don't think this is needed - you can check the return value of the erase call > below if you want to ensure it exists. Done.
Just a couple of nits. https://codereview.chromium.org/2073713002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2073713002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:118: // Collect field trials data. trials -> trial https://codereview.chromium.org/2073713002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:119: std::vector<variations::ActiveGroupId> active_groups_ids; active_groups_ids -> active_group_ids
https://codereview.chromium.org/2073713002/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2073713002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:118: // Collect field trials data. On 2016/06/24 03:42:35, kcarattini wrote: > trials -> trial Done. https://codereview.chromium.org/2073713002/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:119: std::vector<variations::ActiveGroupId> active_groups_ids; On 2016/06/24 03:42:35, kcarattini wrote: > active_groups_ids -> active_group_ids Done.
lgtm
stefanocs@google.com changed reviewers: + nparker@chromium.org
Thanks! nparker@chromium.org: Hi Nathan, please review this cl. Thanks
lgtm https://codereview.chromium.org/2073713002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2073713002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:120: variations::GetFieldTrialActiveGroupIds(&active_group_ids); I'm no expert in fetching field trials -- you might want to have someone from chrome-metrics-team@ review this (unless you're confident this is right). https://codereview.chromium.org/2073713002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2073713002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:137: base::FeatureList::IsEnabled(kFeatureOnByDefault); If a field trial doesn't get activated until some code asks about it's state, it's possible that we may have reports that are missing a field trial that infact the user is part of, but just hasn't exercised yet. This is just something to keep in mind when analyzing the logs.
stefanocs@google.com changed reviewers: + asvitkine@chromium.org
Thanks Nathan. +asvitkine@chromium.org: Please review code for fetching field trials and the test. https://codereview.chromium.org/2073713002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter.cc (right): https://codereview.chromium.org/2073713002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter.cc:120: variations::GetFieldTrialActiveGroupIds(&active_group_ids); On 2016/06/27 18:17:20, Nathan Parker wrote: > I'm no expert in fetching field trials -- you might want to have someone from > chrome-metrics-team@ review this (unless you're confident this is right). Acknowledged. https://codereview.chromium.org/2073713002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2073713002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:137: base::FeatureList::IsEnabled(kFeatureOnByDefault); On 2016/06/27 18:17:20, Nathan Parker wrote: > If a field trial doesn't get activated until some code asks about it's state, > it's possible that we may have reports that are missing a field trial that > infact the user is part of, but just hasn't exercised yet. This is just > something to keep in mind when analyzing the logs. Acknowledged.
lgtm https://codereview.chromium.org/2073713002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2073713002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:23: ActiveGroupIdSet; Nit: Seems strange to typedef this if you only use the type once. At the very least, you can maybe typedef it within the function?
Thanks. https://codereview.chromium.org/2073713002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/permission_reporter_unittest.cc (right): https://codereview.chromium.org/2073713002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/permission_reporter_unittest.cc:23: ActiveGroupIdSet; On 2016/06/28 15:15:27, Alexei Svitkine (very slow) wrote: > Nit: Seems strange to typedef this if you only use the type once. At the very > least, you can maybe typedef it within the function? Done.
The CQ bit was checked by stefanocs@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org, kcarattini@chromium.org, asvitkine@chromium.org, nparker@chromium.org Link to the patchset: https://codereview.chromium.org/2073713002/#ps160001 (title: "Move typedef ActiveGroupIdSet")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add field trials and platform to permission report BUG=613883 ========== to ========== Add field trials and platform to permission report BUG=613883 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Add field trials and platform to permission report BUG=613883 ========== to ========== Add field trials and platform to permission report BUG=613883 Committed: https://crrev.com/9542b193976dacb41d703f00884c399777f474d3 Cr-Commit-Position: refs/heads/master@{#402628} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/9542b193976dacb41d703f00884c399777f474d3 Cr-Commit-Position: refs/heads/master@{#402628} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
