Add cws ukm for enable and disable [chromium/src : main]
Robert Kaplow (Gerrit)
Robert Kaplow added 1 comment![]( "Open in Gerrit")
similar to other bug - can you link the approved privacy doc
Related details
- Devlin Cronin
- Howard Chang
- Justin Lulejian
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Howard Chang (Gerrit)
Howard Chang added 1 comment![]( "Open in Gerrit")
similar to other bug - can you link the approved privacy doc
- Devlin Cronin
- Justin Lulejian
- Robert Kaplow
Robert Kaplow (Gerrit)
Robert Kaplow voted Code-Review+1![]( "Open in Gerrit")
- Howard Chang
- Justin Lulejian
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Lulejian (Gerrit)
Justin Lulejian voted and added 4 comments![]( "Open in Gerrit")
Votes added by Justin Lulejian
| Code-Review | +1 |
4 comments
lgtm enums.xml, but adding back Emilia for the ExtensionRegistrar comment.
Add cws ukm for enable and disable```suggestion
Add cws ukm for extension enable and disable
```
if (enable_rlz) {
deps += [ "//rlz:rlz_lib" ]
}is this necessary for this CL?
RecordUkmForExtension(extension->url(), ExtensionUsageAction::kEnabled);@emil...@chromium.org to double-check me here.
Unfortunately, despite it being the most convenient, I don't think ExtensionRegistrar is the best place for recording this UMA. Non-UI actions like the sync service can enable/disable extension through this method/class so this UKM would record for those too. I believe the intent is just for user UI actions?
If so I think the better location would be in the webUI code when we [set the extension to enabled or disabled as a result of a UI action](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/resources/extensions/service.ts;l=221;drc=304cd9cdced1bdf31585d77e0c0b34dc90597462):
This covers the known enable/disable UI surfaces (management chrome://extensions page and the detail page (chrome://extensions?id=...)).
Unfortunately I don't see an existing [no UKM `chrome.metricsPrivate` call yet](https://source.chromium.org/chromium/chromium/src/+/main:extensions/browser/api/metrics_private/metrics_private_api.cc) so that would need to be created first in order to use it in this commit.
Related details
- Emilia Paz
- Howard Chang
- Robert Kaplow
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Emilia Paz (Gerrit)
Emilia Paz added 3 comments![]( "Open in Gerrit")
Thanks Howard!
enum class ExtensionUsageAction {
kPinned,
kUnpinned,
kContextMenuInit,
kActionClicked,
kEnabled,
kDisabled,
};From review in previous CL, it would be better to use the same enum for the UKM
(changes here and in extension_context_menu_model.h, extension_registrar.h).
RecordUkmForExtension(extension->url(), ExtensionUsageAction::kEnabled);@emil...@chromium.org to double-check me here.
Unfortunately, despite it being the most convenient, I don't think ExtensionRegistrar is the best place for recording this UMA. Non-UI actions like the sync service can enable/disable extension through this method/class so this UKM would record for those too. I believe the intent is just for user UI actions?
If so I think the better location would be in the webUI code when we [set the extension to enabled or disabled as a result of a UI action](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/resources/extensions/service.ts;l=221;drc=304cd9cdced1bdf31585d77e0c0b34dc90597462):
This covers the known enable/disable UI surfaces (management chrome://extensions page and the detail page (chrome://extensions?id=...)).
Unfortunately I don't see an existing [no UKM `chrome.metricsPrivate` call yet](https://source.chromium.org/chromium/chromium/src/+/main:extensions/browser/api/metrics_private/metrics_private_api.cc) so that would need to be created first in order to use it in this commit.
If the goal is to track whether a user enabled or disables an extension, I agree with Justin that here is not the correct place. Manually enabling/disabling an extension can be done from:
- user interaction on chrome://extensions or chrome://extensions/?id=<extension_id>
- extension callschrome.management.setEnabled, which requires user interaction
`ExtensionRegistrar::EnableExtension` is used by many places to enable the extension as a "side effect". For example if an extension is re installed and it was previously disabled ([code](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/extensions/webstore_standalone_installer.cc;l=214-218;drc=45a1118875a1b3a2a1ae9fee639cf1b546a43547)), or supervised account updates ([code](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/supervised_user/supervised_user_extensions_manager.cc;l=433-436;drc=45a1118875a1b3a2a1ae9fee639cf1b546a43547;bpv=1;bpt=1))
If we want tor record every time extension is enabled/disabled for any reason, then yes this is correct. It would be noisy and hard to understand though
Related details
- Howard Chang
- Robert Kaplow
Robert Kaplow (Gerrit)
Robert Kaplow voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Howard Chang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |