Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
952463002:
Add UMA to track permission changes from the content settings menu (Closed)
|
Created:
5 years, 10 months ago by Miguel Garcia Modified:
5 years, 10 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UMA to track permission changes from the content settings menu
Update the descriptions for the equivalent OriginSettings UMA.
TBR=jhawkins
BUG=433774
Committed: https://crrev.com/1fc158a1ecea9f282362d9958a150303b3a0dd94
Cr-Commit-Position: refs/heads/master@{#318223}
Patch Set 1 #
Total comments: 12
Patch Set 2 : #Patch Set 3 : #
Total comments: 15
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Messages
Total messages: 21 (5 generated)
miguelg@chromium.org changed reviewers: + finnur@chromium.org
Can you have an initial look before I get owners to approve it? Thanks!
https://codereview.chromium.org/952463002/diff/1/chrome/browser/android/prefe... File chrome/browser/android/preferences/website_preference_bridge.cc (right): https://codereview.chromium.org/952463002/diff/1/chrome/browser/android/prefe... chrome/browser/android/preferences/website_preference_bridge.cc:59: UMA_HISTOGRAM_ENUMERATION("WebsiteSettings.Menu.PermissionChanged.Ask", 'Ask' is, as I recall, not something we can set for individual origins (it is only available for global defaults), so I'm surprised to see CONTENT_SETTING_ASK handled here. In what cases do you need to handle 'Ask' (for origins specifically)? I would like to think of this in terms of Allowed/Block/Reset to default, which better reflects behind the scenes what happens (and is more future-proof, if we change the global default for a category to be something other than 'Ask'). Therefore, I'd remove line 58 and change the UMA histogram name to be WebsiteSettings.Menu.PermissionChanged.Reset, or something to that effect. https://codereview.chromium.org/952463002/diff/1/chrome/browser/android/prefe... chrome/browser/android/preferences/website_preference_bridge.cc:365: } What about adding... ContentSetting setting = CONTENT_SETTING_DEFAULT; ... at line 360. Then change this else to: } else { setting = value ? CONTENT_SETTING_ALLOW : CONTENT_SETTING_BLOCK; GetCookieSettings()->SetCookieSetting(primary_pattern, setting); } LogPermissionChange(CONTENT_SETTINGS_TYPE_NOTIFICATIONS, setting); https://codereview.chromium.org/952463002/diff/1/chrome/browser/ui/webui/opti... File chrome/browser/ui/webui/options/content_settings_handler.cc (right): https://codereview.chromium.org/952463002/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/content_settings_handler.cc:1422: ContentSettingsTypeHistogram histogram_value = nit: Line-break above (looks as-is a bit like part of the else clause). https://codereview.chromium.org/952463002/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/content_settings_handler.cc:1423: ContentSettingTypeToHistogramValue(type); In the other place, you DCHECK if you get an invalid value. Maybe follow suit here? https://codereview.chromium.org/952463002/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/content_settings_handler.cc:1430: "WebsiteSettings.ContentSettings.PermissionChanged.Ask", histogram_value, Same here: s/Ask/Reset/ https://codereview.chromium.org/952463002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/952463002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:43431: + using the content settings menu. Same here. The content settings menu doesn't provide an Ask option. Only Allow/Block/Clear&Reset.
Thanks for the quick review! https://codereview.chromium.org/952463002/diff/1/chrome/browser/android/prefe... File chrome/browser/android/preferences/website_preference_bridge.cc (right): https://codereview.chromium.org/952463002/diff/1/chrome/browser/android/prefe... chrome/browser/android/preferences/website_preference_bridge.cc:59: UMA_HISTOGRAM_ENUMERATION("WebsiteSettings.Menu.PermissionChanged.Ask", On 2015/02/23 14:15:27, Finnur wrote: > 'Ask' is, as I recall, not something we can set for individual origins (it is > only available for global defaults), so I'm surprised to see CONTENT_SETTING_ASK > handled here. In what cases do you need to handle 'Ask' (for origins > specifically)? > > I would like to think of this in terms of Allowed/Block/Reset to default, which > better reflects behind the scenes what happens (and is more future-proof, if we > change the global default for a category to be something other than 'Ask'). > > Therefore, I'd remove line 58 and change the UMA histogram name to be > WebsiteSettings.Menu.PermissionChanged.Reset, or something to that effect. Done. https://codereview.chromium.org/952463002/diff/1/chrome/browser/android/prefe... chrome/browser/android/preferences/website_preference_bridge.cc:365: } Good catch thanks. On 2015/02/23 14:15:27, Finnur wrote: > What about adding... > ContentSetting setting = CONTENT_SETTING_DEFAULT; > ... at line 360. Then change this else to: > > } else { > setting = value ? CONTENT_SETTING_ALLOW : CONTENT_SETTING_BLOCK; > GetCookieSettings()->SetCookieSetting(primary_pattern, setting); > } > > LogPermissionChange(CONTENT_SETTINGS_TYPE_NOTIFICATIONS, setting); https://codereview.chromium.org/952463002/diff/1/chrome/browser/ui/webui/opti... File chrome/browser/ui/webui/options/content_settings_handler.cc (right): https://codereview.chromium.org/952463002/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/content_settings_handler.cc:1422: ContentSettingsTypeHistogram histogram_value = On 2015/02/23 14:15:27, Finnur wrote: > nit: Line-break above (looks as-is a bit like part of the else clause). Done. https://codereview.chromium.org/952463002/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/content_settings_handler.cc:1423: ContentSettingTypeToHistogramValue(type); On 2015/02/23 14:15:27, Finnur wrote: > In the other place, you DCHECK if you get an invalid value. Maybe follow suit > here? Done. https://codereview.chromium.org/952463002/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/content_settings_handler.cc:1430: "WebsiteSettings.ContentSettings.PermissionChanged.Ask", histogram_value, On 2015/02/23 14:15:27, Finnur wrote: > Same here: s/Ask/Reset/ Done. https://codereview.chromium.org/952463002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/952463002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:43431: + using the content settings menu. On 2015/02/23 14:15:27, Finnur wrote: > Same here. The content settings menu doesn't provide an Ask option. Only > Allow/Block/Clear&Reset. Done.
LGTM, one nit. https://codereview.chromium.org/952463002/diff/40001/chrome/browser/android/p... File chrome/browser/android/preferences/website_preference_bridge.cc (right): https://codereview.chromium.org/952463002/diff/40001/chrome/browser/android/p... chrome/browser/android/preferences/website_preference_bridge.cc:366: value ? CONTENT_SETTING_ALLOW : CONTENT_SETTING_BLOCK); nit: Probably more readable to use |setting| here.
miguelg@chromium.org changed reviewers: + isherman@chromium.org, jhawkins@google.com
Adding owners now isherman: please review histograms.xml jhawkins: please review content_settings_handler.cc Thanks!
On 2015/02/23 14:57:41, Miguel Garcia wrote: > Adding owners now > > isherman: please review histograms.xml > jhawkins: please review content_settings_handler.cc > > Thanks! Friendly request to get the review in ASAP since we'd like to merge this in M42
https://codereview.chromium.org/952463002/diff/40001/chrome/browser/android/p... File chrome/browser/android/preferences/website_preference_bridge.cc (right): https://codereview.chromium.org/952463002/diff/40001/chrome/browser/android/p... chrome/browser/android/preferences/website_preference_bridge.cc:13: #include "base/metrics/histogram.h" nit: Please include histogram_macros instead. https://codereview.chromium.org/952463002/diff/40001/chrome/browser/android/p... chrome/browser/android/preferences/website_preference_bridge.cc:39: static void LogPermissionChange(ContentSettingsType type, nit: No need for "static" in an anonymous namespace. https://codereview.chromium.org/952463002/diff/40001/chrome/browser/android/p... chrome/browser/android/preferences/website_preference_bridge.cc:44: << "Invalid content setting type specified."; nit: Please omit this string. It adds weight to the binary, without adding developer benefit beyond what a comment can provide. https://codereview.chromium.org/952463002/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/content_settings_handler.cc (right): https://codereview.chromium.org/952463002/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/content_settings_handler.cc:14: #include "base/metrics/histogram.h" Ditto https://codereview.chromium.org/952463002/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/content_settings_handler.cc:1434: histogram_value, CONTENT_SETTINGS_HISTOGRAM_NUM_TYPES); Please call LogPermissionChange() to emit to these histograms, rather than duplicating (part of) the code here. https://codereview.chromium.org/952463002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/952463002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:43444: +</histogram> What sort of decisions are you planning to make based on these metrics? You're measuring how often users toggle states, but not measuring which states users come to rest in. Suppose that 100 people toggle back and forth between allowed and blocked each day, and all the remaining Chrome users toggle once, and then come to rest where they land. This metric will show you what the 100 people are doing, but not what the Chrome population looks like. Is that really what you want to measure?
Thanks a lot for the quick review Ilya https://chromiumcodereview.appspot.com/952463002/diff/40001/chrome/browser/an... File chrome/browser/android/preferences/website_preference_bridge.cc (right): https://chromiumcodereview.appspot.com/952463002/diff/40001/chrome/browser/an... chrome/browser/android/preferences/website_preference_bridge.cc:13: #include "base/metrics/histogram.h" On 2015/02/23 22:55:20, Ilya Sherman wrote: > nit: Please include histogram_macros instead. Done. https://chromiumcodereview.appspot.com/952463002/diff/40001/chrome/browser/an... chrome/browser/android/preferences/website_preference_bridge.cc:39: static void LogPermissionChange(ContentSettingsType type, On 2015/02/23 22:55:20, Ilya Sherman wrote: > nit: No need for "static" in an anonymous namespace. Done. https://chromiumcodereview.appspot.com/952463002/diff/40001/chrome/browser/an... chrome/browser/android/preferences/website_preference_bridge.cc:44: << "Invalid content setting type specified."; On 2015/02/23 22:55:20, Ilya Sherman wrote: > nit: Please omit this string. It adds weight to the binary, without adding > developer benefit beyond what a comment can provide. Done. https://chromiumcodereview.appspot.com/952463002/diff/40001/chrome/browser/an... chrome/browser/android/preferences/website_preference_bridge.cc:366: value ? CONTENT_SETTING_ALLOW : CONTENT_SETTING_BLOCK); On 2015/02/23 14:47:19, Finnur wrote: > nit: Probably more readable to use |setting| here. Done. https://chromiumcodereview.appspot.com/952463002/diff/40001/chrome/browser/ui... File chrome/browser/ui/webui/options/content_settings_handler.cc (right): https://chromiumcodereview.appspot.com/952463002/diff/40001/chrome/browser/ui... chrome/browser/ui/webui/options/content_settings_handler.cc:14: #include "base/metrics/histogram.h" On 2015/02/23 22:55:20, Ilya Sherman wrote: > Ditto Done. https://chromiumcodereview.appspot.com/952463002/diff/40001/chrome/browser/ui... chrome/browser/ui/webui/options/content_settings_handler.cc:1434: histogram_value, CONTENT_SETTINGS_HISTOGRAM_NUM_TYPES); On 2015/02/23 22:55:20, Ilya Sherman wrote: > Please call LogPermissionChange() to emit to these histograms, rather than > duplicating (part of) the code here. So the thing is that those two files are a world appart. website_preference_bridge is android only, it does not even get compiled on desktop chrome. similarly content_settings_handler (this file) is desktop specific. There is no single common ancestor to them, they both get events from the UX and update content settings independently. This is obviously far from ideal but refactoring all of it seems way outside the scope of this CL (especially considering that I am still hoping it will get cherry picked). I am open to suggestions if you think it can be done differently. https://chromiumcodereview.appspot.com/952463002/diff/40001/tools/metrics/his... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/952463002/diff/40001/tools/metrics/his... tools/metrics/histograms/histograms.xml:43444: +</histogram> The main motivation for this change is that we want to measure if there is any significant change in the way people opt out of notifications after we launch the push API. We want to do that by comparing how often the notification permission notification changes compared to say geolocation. And how often it changes over time as new web pages introduce the push API. Also, together with the already existing Origin Info metrics below we'd like to compare how often people toggle permissions from the omnibox padlock vs the content settings menu. On 2015/02/23 22:55:20, Ilya Sherman wrote: > What sort of decisions are you planning to make based on these metrics? You're > measuring how often users toggle states, but not measuring which states users > come to rest in. Suppose that 100 people toggle back and forth between allowed > and blocked each day, and all the remaining Chrome users toggle once, and then > come to rest where they land. This metric will show you what the 100 people are > doing, but not what the Chrome population looks like. Is that really what you > want to measure?
Thanks for the explanation. Histograms LGTM, though I'd still recommend avoiding the code duplication, as I've mentioned below. https://chromiumcodereview.appspot.com/952463002/diff/40001/chrome/browser/ui... File chrome/browser/ui/webui/options/content_settings_handler.cc (right): https://chromiumcodereview.appspot.com/952463002/diff/40001/chrome/browser/ui... chrome/browser/ui/webui/options/content_settings_handler.cc:1434: histogram_value, CONTENT_SETTINGS_HISTOGRAM_NUM_TYPES); On 2015/02/24 13:25:24, Miguel Garcia wrote: > On 2015/02/23 22:55:20, Ilya Sherman wrote: > > Please call LogPermissionChange() to emit to these histograms, rather than > > duplicating (part of) the code here. > > So the thing is that those two files are a world appart. > > website_preference_bridge is android only, it does not even get compiled on > desktop chrome. > > similarly content_settings_handler (this file) is desktop specific. > > > There is no single common ancestor to them, they both get events from the UX and > update content settings independently. This is obviously far from ideal but > refactoring all of it seems way outside the scope of this CL (especially > considering that I am still hoping it will get cherry picked). I am open to > suggestions if you think it can be done differently. I'm suggesting moving the helper method out into a content_settings_metrics file that you could access from both locations. Does that address your concerns?
I went ahead with your suggestion, adding bauerb to review the new files. On 2015/02/24 19:13:40, Ilya Sherman wrote: > Thanks for the explanation. Histograms LGTM, though I'd still recommend > avoiding the code duplication, as I've mentioned below. > > https://chromiumcodereview.appspot.com/952463002/diff/40001/chrome/browser/ui... > File chrome/browser/ui/webui/options/content_settings_handler.cc (right): > > https://chromiumcodereview.appspot.com/952463002/diff/40001/chrome/browser/ui... > chrome/browser/ui/webui/options/content_settings_handler.cc:1434: > histogram_value, CONTENT_SETTINGS_HISTOGRAM_NUM_TYPES); > On 2015/02/24 13:25:24, Miguel Garcia wrote: > > On 2015/02/23 22:55:20, Ilya Sherman wrote: > > > Please call LogPermissionChange() to emit to these histograms, rather than > > > duplicating (part of) the code here. > > > > So the thing is that those two files are a world appart. > > > > website_preference_bridge is android only, it does not even get compiled on > > desktop chrome. > > > > similarly content_settings_handler (this file) is desktop specific. > > > > > > There is no single common ancestor to them, they both get events from the UX > and > > update content settings independently. This is obviously far from ideal but > > refactoring all of it seems way outside the scope of this CL (especially > > considering that I am still hoping it will get cherry picked). I am open to > > suggestions if you think it can be done differently. > > I'm suggesting moving the helper method out into a content_settings_metrics file > that you could access from both locations. Does that address your concerns?
miguelg@chromium.org changed reviewers: + bauerb@chromium.org
+bauerb for content_settings OWNERS
content_settings/ LGTM
On 2015/02/26 11:23:48, Bernhard (back on Feb 26) wrote: > content_settings/ LGTM Thanks!
The CQ bit was checked by miguelg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/952463002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/952463002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/1fc158a1ecea9f282362d9958a150303b3a0dd94 Cr-Commit-Position: refs/heads/master@{#318223} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
