[Extensions] Add histogram for insecure uninstall URLs [chromium/src : main]
Anton Bershanskyi (Gerrit)
Anton Bershanskyi added 1 comment![]( "Open in Gerrit")
Hi Devlin and Oliver,
I what do you think about issue 510816360 (and this CL)? I propose a slight API behavior change, so I realize that you might need a while to think about and of course do not expect a quick reply.
Thanks!
Related details
- Devlin Cronin
- Oliver Dunk
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Oliver Dunk (Gerrit)
Oliver Dunk added 3 comments![]( "Open in Gerrit")
Thanks Anton!
insecure. This histogram might let us solve "mixed content"I'm never opposed to moving in the direction of HTTPS, but I'm 50/50 on the effort / value tradeoff here. Happy to defer to @rdevlin...@chromium.org on that.
// Record histogram one during session start to count every extension installIt seems like this would not capture the uninstall URL until the extension has been unloaded and loaded again, since onInstalled presumably runs after OnExtensionLoaded. Is that the case? If so, can we handle that somehow?
Note that we are also considering changing the persistence in https://github.com/w3c/webextensions/issues/981. I don't think that's relevant but I wanted to mention in case you can think of a way it might be.
Related details
- Anton Bershanskyi
- Devlin Cronin
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. |
Anton Bershanskyi (Gerrit)
Anton Bershanskyi added 2 comments![]( "Open in Gerrit")
insecure. This histogram might let us solve "mixed content"I'm never opposed to moving in the direction of HTTPS, but I'm 50/50 on the effort / value tradeoff here. Happy to defer to @rdevlin...@chromium.org on that.
Here I proposed a staged migration which requires a rather large amount of effort. If we could disregard backward compatibility, we could implement the change in literally 3-line change in `RuntimeSetUninstallURLFunction()::Run()`:
```diff
- if (!params->url.empty() && !GURL(params->url).SchemeIsHTTPOrHTTPS()) {
- + if (!params->url.empty() && !(url.SchemeIs(url::kHttpsScheme)
- + || net::IsLocalhost(url))) {
- ```
Alternatively, we could start by raising awareness of this issue by logging a benign warning right before `UpdateExtensionPref()`, also a 3-line change.
// Record histogram one during session start to count every extension installIt seems like this would not capture the uninstall URL until the extension has been unloaded and loaded again, since onInstalled presumably runs after OnExtensionLoaded. Is that the case? If so, can we handle that somehow?
Note that we are also considering changing the persistence in https://github.com/w3c/webextensions/issues/981. I don't think that's relevant but I wanted to mention in case you can think of a way it might be.
It seems like this would not capture the uninstall URL until the extension has been unloaded and loaded again, since onInstalled presumably runs after OnExtensionLoaded.
Yes, this is the case. No method of counting is perfect, this method avoids counting each call to `setUninstallURL()` and effectively getting data skewed by a single extension resetting uninstall URL on every background context invocation. We could implement a histogram in `OnExtensionUninstalled()` and count the number of actual page loads instead. If you have a better approach, I can implement that instead.
Related details
- Devlin Cronin
- Oliver Dunk
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. |
Oliver Dunk (Gerrit)
Oliver Dunk added 1 comment![]( "Open in Gerrit")
Removing myself from the attention set for now, as I'm curious what Devlin's thoughts are. Feel free to add me back once you're ready for another review :)
Related details
- Devlin Cronin
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. |
Devlin Cronin (Gerrit)
Devlin Cronin added 3 comments![]( "Open in Gerrit")
Thanks, Anton!
insecure. This histogram might let us solve "mixed content"Anton BershanskyiI'm never opposed to moving in the direction of HTTPS, but I'm 50/50 on the effort / value tradeoff here. Happy to defer to @rdevlin...@chromium.org on that.
Here I proposed a staged migration which requires a rather large amount of effort. If we could disregard backward compatibility, we could implement the change in literally 3-line change in `RuntimeSetUninstallURLFunction()::Run()`:
```diff
- if (!params->url.empty() && !GURL(params->url).SchemeIsHTTPOrHTTPS()) {
- + if (!params->url.empty() && !(url.SchemeIs(url::kHttpsScheme)
- + || net::IsLocalhost(url))) {
- ```
Alternatively, we could start by raising awareness of this issue by logging a benign warning right before `UpdateExtensionPref()`, also a 3-line change.
I'm okay with us exploring this; it doesn't seem like it'll be too much effort.
I *do* think we should do the impact evaluation (i.e., these histograms) first; I don't feel comfortable just making a behavior change.
// Record histogram one during session start to count every extension installAnton BershanskyiIt seems like this would not capture the uninstall URL until the extension has been unloaded and loaded again, since onInstalled presumably runs after OnExtensionLoaded. Is that the case? If so, can we handle that somehow?
Note that we are also considering changing the persistence in https://github.com/w3c/webextensions/issues/981. I don't think that's relevant but I wanted to mention in case you can think of a way it might be.
It seems like this would not capture the uninstall URL until the extension has been unloaded and loaded again, since onInstalled presumably runs after OnExtensionLoaded.
Yes, this is the case. No method of counting is perfect, this method avoids counting each call to `setUninstallURL()` and effectively getting data skewed by a single extension resetting uninstall URL on every background context invocation. We could implement a histogram in `OnExtensionUninstalled()` and count the number of actual page loads instead. If you have a better approach, I can implement that instead.
I'm okay with us recording this in OnExtensionLoaded() -- that happens for each extension on every startup, which is reasonable.
However, I do think we should tweak the histogram. Just recording a raw number here isn't very helpful, especially for the secure ones. For instance, if we see that this number is "1000", we don't know what to do with that data -- it doesn't indicate what kind of breakage we might see if we changed the behavior.
Instead, I think we need a ratio here. At the very least, we should record secure vs insecure, and I'd lean towards going a step further and having three buckets: https, localhost, and http. Then, we would know what percentage of users would be affected if we were to make a change.
Related details
- Anton Bershanskyi
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. |
Anton Bershanskyi (Gerrit)
Anton Bershanskyi added 2 comments![]( "Open in Gerrit")
insecure. This histogram might let us solve "mixed content"Anton BershanskyiI'm never opposed to moving in the direction of HTTPS, but I'm 50/50 on the effort / value tradeoff here. Happy to defer to @rdevlin...@chromium.org on that.
Devlin CroninHere I proposed a staged migration which requires a rather large amount of effort. If we could disregard backward compatibility, we could implement the change in literally 3-line change in `RuntimeSetUninstallURLFunction()::Run()`:
```diff
- if (!params->url.empty() && !GURL(params->url).SchemeIsHTTPOrHTTPS()) {
- + if (!params->url.empty() && !(url.SchemeIs(url::kHttpsScheme)
- + || net::IsLocalhost(url))) {
- ```
Alternatively, we could start by raising awareness of this issue by logging a benign warning right before `UpdateExtensionPref()`, also a 3-line change.
I'm okay with us exploring this; it doesn't seem like it'll be too much effort.
I *do* think we should do the impact evaluation (i.e., these histograms) first; I don't feel comfortable just making a behavior change.
I don't feel comfortable just making a behavior change.
We are on the same page. This is why I propose incremental change in steps: histogram, then awareness campaign via console warning and PSA, then new behavior with a kill-switch, then unconditional new behavior. The resulting timeline comes out very stretched (easily can last a full year), but al least we are making incremental progress.
// Record histogram one during session start to count every extension installAnton BershanskyiIt seems like this would not capture the uninstall URL until the extension has been unloaded and loaded again, since onInstalled presumably runs after OnExtensionLoaded. Is that the case? If so, can we handle that somehow?
Note that we are also considering changing the persistence in https://github.com/w3c/webextensions/issues/981. I don't think that's relevant but I wanted to mention in case you can think of a way it might be.
Devlin CroninIt seems like this would not capture the uninstall URL until the extension has been unloaded and loaded again, since onInstalled presumably runs after OnExtensionLoaded.
Yes, this is the case. No method of counting is perfect, this method avoids counting each call to `setUninstallURL()` and effectively getting data skewed by a single extension resetting uninstall URL on every background context invocation. We could implement a histogram in `OnExtensionUninstalled()` and count the number of actual page loads instead. If you have a better approach, I can implement that instead.
I'm okay with us recording this in OnExtensionLoaded() -- that happens for each extension on every startup, which is reasonable.
However, I do think we should tweak the histogram. Just recording a raw number here isn't very helpful, especially for the secure ones. For instance, if we see that this number is "1000", we don't know what to do with that data -- it doesn't indicate what kind of breakage we might see if we changed the behavior.
Instead, I think we need a ratio here. At the very least, we should record secure vs insecure, and I'd lean towards going a step further and having three buckets: https, localhost, and http. Then, we would know what percentage of users would be affected if we were to make a change.
At the very least, we should record secure vs insecure
I believe we are currently recording "secure" vs "insecure", where "secure" is "HTTPS and local HTTP" and "insecure" is "remote HTTP". The current CL revision ("Patchset 1") has `base::UmaHistogramBoolean()` with the value being `uninstall_url.SchemeIs(url::kHttpsScheme) || net::IsLocalhost(uninstall_url)`.
I'd lean towards going a step further and having three buckets: https, localhost, and http.
I'm OK with changing to an "enum" histogram. Where exactly would you prefer to count HTTPS localhost, as HTTPS or as localhost? I expect very few measurements in this category, but we should't dismiss them. I can make 4 buckets.
Related details
- Devlin Cronin
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. |
Devlin Cronin (Gerrit)
Devlin Cronin added 2 comments![]( "Open in Gerrit")
insecure. This histogram might let us solve "mixed content"Anton BershanskyiI'm never opposed to moving in the direction of HTTPS, but I'm 50/50 on the effort / value tradeoff here. Happy to defer to @rdevlin...@chromium.org on that.
Devlin CroninHere I proposed a staged migration which requires a rather large amount of effort. If we could disregard backward compatibility, we could implement the change in literally 3-line change in `RuntimeSetUninstallURLFunction()::Run()`:
```diff
- if (!params->url.empty() && !GURL(params->url).SchemeIsHTTPOrHTTPS()) {
- + if (!params->url.empty() && !(url.SchemeIs(url::kHttpsScheme)
- + || net::IsLocalhost(url))) {
- ```
Alternatively, we could start by raising awareness of this issue by logging a benign warning right before `UpdateExtensionPref()`, also a 3-line change.
Anton BershanskyiI'm okay with us exploring this; it doesn't seem like it'll be too much effort.
I *do* think we should do the impact evaluation (i.e., these histograms) first; I don't feel comfortable just making a behavior change.
I don't feel comfortable just making a behavior change.
We are on the same page. This is why I propose incremental change in steps: histogram, then awareness campaign via console warning and PSA, then new behavior with a kill-switch, then unconditional new behavior. The resulting timeline comes out very stretched (easily can last a full year), but al least we are making incremental progress.
Hopefully we won't need quite that many steps, but yep, starting with metrics is first.
// Record histogram one during session start to count every extension installAnton BershanskyiIt seems like this would not capture the uninstall URL until the extension has been unloaded and loaded again, since onInstalled presumably runs after OnExtensionLoaded. Is that the case? If so, can we handle that somehow?
Note that we are also considering changing the persistence in https://github.com/w3c/webextensions/issues/981. I don't think that's relevant but I wanted to mention in case you can think of a way it might be.
Devlin CroninIt seems like this would not capture the uninstall URL until the extension has been unloaded and loaded again, since onInstalled presumably runs after OnExtensionLoaded.
Yes, this is the case. No method of counting is perfect, this method avoids counting each call to `setUninstallURL()` and effectively getting data skewed by a single extension resetting uninstall URL on every background context invocation. We could implement a histogram in `OnExtensionUninstalled()` and count the number of actual page loads instead. If you have a better approach, I can implement that instead.
Anton BershanskyiI'm okay with us recording this in OnExtensionLoaded() -- that happens for each extension on every startup, which is reasonable.
However, I do think we should tweak the histogram. Just recording a raw number here isn't very helpful, especially for the secure ones. For instance, if we see that this number is "1000", we don't know what to do with that data -- it doesn't indicate what kind of breakage we might see if we changed the behavior.
Instead, I think we need a ratio here. At the very least, we should record secure vs insecure, and I'd lean towards going a step further and having three buckets: https, localhost, and http. Then, we would know what percentage of users would be affected if we were to make a change.
At the very least, we should record secure vs insecure
I believe we are currently recording "secure" vs "insecure", where "secure" is "HTTPS and local HTTP" and "insecure" is "remote HTTP". The current CL revision ("Patchset 1") has `base::UmaHistogramBoolean()` with the value being `uninstall_url.SchemeIs(url::kHttpsScheme) || net::IsLocalhost(uninstall_url)`.
I'd lean towards going a step further and having three buckets: https, localhost, and http.
I'm OK with changing to an "enum" histogram. Where exactly would you prefer to count HTTPS localhost, as HTTPS or as localhost? I expect very few measurements in this category, but we should't dismiss them. I can make 4 buckets.
I believe we are currently recording "secure" vs "insecure", where "secure" is "HTTPS and local HTTP" and "insecure" is "remote HTTP".
You're completely correct; I don't know why I read that as just emitting a count. My mistake.
I'm OK with changing to an "enum" histogram. Where exactly would you prefer to count HTTPS localhost, as HTTPS or as localhost?
I think I'd count those as https. We'll likely allow any https site, so we can put those in the "safe" bucket. I think http://localhost is the more interesting one (since I could see us optentially allowing or disallowing those, based on usage)
Related details
- Anton Bershanskyi
- Oliver Dunk
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. |
Anton Bershanskyi (Gerrit)
Anton Bershanskyi added 1 comment![]( "Open in Gerrit")
// Record histogram one during session start to count every extension installAnton BershanskyiIt seems like this would not capture the uninstall URL until the extension has been unloaded and loaded again, since onInstalled presumably runs after OnExtensionLoaded. Is that the case? If so, can we handle that somehow?
Note that we are also considering changing the persistence in https://github.com/w3c/webextensions/issues/981. I don't think that's relevant but I wanted to mention in case you can think of a way it might be.
Devlin CroninIt seems like this would not capture the uninstall URL until the extension has been unloaded and loaded again, since onInstalled presumably runs after OnExtensionLoaded.
Yes, this is the case. No method of counting is perfect, this method avoids counting each call to `setUninstallURL()` and effectively getting data skewed by a single extension resetting uninstall URL on every background context invocation. We could implement a histogram in `OnExtensionUninstalled()` and count the number of actual page loads instead. If you have a better approach, I can implement that instead.
Anton BershanskyiI'm okay with us recording this in OnExtensionLoaded() -- that happens for each extension on every startup, which is reasonable.
However, I do think we should tweak the histogram. Just recording a raw number here isn't very helpful, especially for the secure ones. For instance, if we see that this number is "1000", we don't know what to do with that data -- it doesn't indicate what kind of breakage we might see if we changed the behavior.
Instead, I think we need a ratio here. At the very least, we should record secure vs insecure, and I'd lean towards going a step further and having three buckets: https, localhost, and http. Then, we would know what percentage of users would be affected if we were to make a change.
Devlin CroninAt the very least, we should record secure vs insecure
I believe we are currently recording "secure" vs "insecure", where "secure" is "HTTPS and local HTTP" and "insecure" is "remote HTTP". The current CL revision ("Patchset 1") has `base::UmaHistogramBoolean()` with the value being `uninstall_url.SchemeIs(url::kHttpsScheme) || net::IsLocalhost(uninstall_url)`.
I'd lean towards going a step further and having three buckets: https, localhost, and http.
I'm OK with changing to an "enum" histogram. Where exactly would you prefer to count HTTPS localhost, as HTTPS or as localhost? I expect very few measurements in this category, but we should't dismiss them. I can make 4 buckets.
I believe we are currently recording "secure" vs "insecure", where "secure" is "HTTPS and local HTTP" and "insecure" is "remote HTTP".
You're completely correct; I don't know why I read that as just emitting a count. My mistake.
I'm OK with changing to an "enum" histogram. Where exactly would you prefer to count HTTPS localhost, as HTTPS or as localhost?
I think I'd count those as https. We'll likely allow any https site, so we can put those in the "safe" bucket. I think http://localhost is the more interesting one (since I could see us optentially allowing or disallowing those, based on usage)
So rewrote the patch with three buckets: "HTTPS", "Local HTTP" and "Remote HTTPS".
Related details
- Devlin Cronin
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. |
Anton Bershanskyi (Gerrit)
Anton Bershanskyi added 1 comment![]( "Open in Gerrit")
Hi Devlin, Oliver, and Justin,
Since this CL modifies histogram enums, I'm adding Justin as reviewer and moving Oliver to CC. Please feel free to revert.
Justin, here is the summary of past discussion:
1. Oliver questioned whether we should record the histogram during extension load. Devlin said it was fine as is. If needed, I can move histogram call wherever (e.g., record on actual extension uninstall, or record every call to `runtime.setUninstallUrl()`).
2. Devlin suggested recording three buckets so I updated CL accordingly to use "HTTPS", "Local HTTP" and "Remote HTTPS".
I'm currently leaving only Devlin in attention set.
Related details
- Devlin Cronin
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. |
Devlin Cronin (Gerrit)
Devlin Cronin added 5 comments![]( "Open in Gerrit")
Thanks, Anton! Largely LG, but a few comments
Add histogram tracking extension uninstall URLs considereingtypo: considering
// TODO(crbug.com/510816360): Remove this enum.nit: maybe add a qualifier to these, like "Remove this enum around M155, once we've gathered enough data to analyze usage."
Ditto for others.
Otherwise, this reads a bit too much like someone should just go in and clean it up today : )
and non-local hostname. This histogram may allow us to deprecqate supportnit: typo: deprecate
IP. Recorded once per extension at "user profile" (profiles where
people can install extensions, specifically profiles that can have
non-component extensions installed) initialization if and only if extension
has an uninstall URL set.this isn't quite true -- the handling for this restriction is in InstalledLoader, which has checks for the type of profile before it emits metrics. This, on the other hand, emits this once per loaded extension if and only if the extension has an uninstall URL set, and is reported independent of profile type.
That's actually okay, in this case -- we're not counting the number of extensions per profile, so there's not a risk of having a bunch of zeros for the signin profile, and component extensions wouldn't set the uninstall URL.
I'd tweak this to instead be something like:
"Recorded on extension load if an extension has a valid uninstall URL set."
Related details
- Anton Bershanskyi
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. |
Anton Bershanskyi (Gerrit)
Anton Bershanskyi added 6 comments![]( "Open in Gerrit")
Hi Devlin, thanks for review, I incorporated all feedback.
Add histogram tracking extension uninstall URLs considereingAnton Bershanskyitypo: considering
Done
nit: maybe add a qualifier to these, like "Remove this enum around M155, once we've gathered enough data to analyze usage."
Ditto for others.
Otherwise, this reads a bit too much like someone should just go in and clean it up today : )
Done
// Record histogram one during session start to count every extension installAnton BershanskyiIt seems like this would not capture the uninstall URL until the extension has been unloaded and loaded again, since onInstalled presumably runs after OnExtensionLoaded. Is that the case? If so, can we handle that somehow?
Note that we are also considering changing the persistence in https://github.com/w3c/webextensions/issues/981. I don't think that's relevant but I wanted to mention in case you can think of a way it might be.
Devlin CroninIt seems like this would not capture the uninstall URL until the extension has been unloaded and loaded again, since onInstalled presumably runs after OnExtensionLoaded.
Yes, this is the case. No method of counting is perfect, this method avoids counting each call to `setUninstallURL()` and effectively getting data skewed by a single extension resetting uninstall URL on every background context invocation. We could implement a histogram in `OnExtensionUninstalled()` and count the number of actual page loads instead. If you have a better approach, I can implement that instead.
Anton BershanskyiI'm okay with us recording this in OnExtensionLoaded() -- that happens for each extension on every startup, which is reasonable.
However, I do think we should tweak the histogram. Just recording a raw number here isn't very helpful, especially for the secure ones. For instance, if we see that this number is "1000", we don't know what to do with that data -- it doesn't indicate what kind of breakage we might see if we changed the behavior.
Instead, I think we need a ratio here. At the very least, we should record secure vs insecure, and I'd lean towards going a step further and having three buckets: https, localhost, and http. Then, we would know what percentage of users would be affected if we were to make a change.
Devlin CroninAt the very least, we should record secure vs insecure
I believe we are currently recording "secure" vs "insecure", where "secure" is "HTTPS and local HTTP" and "insecure" is "remote HTTP". The current CL revision ("Patchset 1") has `base::UmaHistogramBoolean()` with the value being `uninstall_url.SchemeIs(url::kHttpsScheme) || net::IsLocalhost(uninstall_url)`.
I'd lean towards going a step further and having three buckets: https, localhost, and http.
I'm OK with changing to an "enum" histogram. Where exactly would you prefer to count HTTPS localhost, as HTTPS or as localhost? I expect very few measurements in this category, but we should't dismiss them. I can make 4 buckets.
Anton BershanskyiI believe we are currently recording "secure" vs "insecure", where "secure" is "HTTPS and local HTTP" and "insecure" is "remote HTTP".
You're completely correct; I don't know why I read that as just emitting a count. My mistake.
I'm OK with changing to an "enum" histogram. Where exactly would you prefer to count HTTPS localhost, as HTTPS or as localhost?
I think I'd count those as https. We'll likely allow any https site, so we can put those in the "safe" bucket. I think http://localhost is the more interesting one (since I could see us optentially allowing or disallowing those, based on usage)
Anton BershanskyiSo rewrote the patch with three buckets: "HTTPS", "Local HTTP" and "Remote HTTPS".
Done
and non-local hostname. This histogram may allow us to deprecqate supportAnton Bershanskyinit: typo: deprecate
Done
IP. Recorded once per extension at "user profile" (profiles where
people can install extensions, specifically profiles that can have
non-component extensions installed) initialization if and only if extension
has an uninstall URL set.this isn't quite true -- the handling for this restriction is in InstalledLoader, which has checks for the type of profile before it emits metrics. This, on the other hand, emits this once per loaded extension if and only if the extension has an uninstall URL set, and is reported independent of profile type.
That's actually okay, in this case -- we're not counting the number of extensions per profile, so there's not a risk of having a bunch of zeros for the signin profile, and component extensions wouldn't set the uninstall URL.
I'd tweak this to instead be something like:
"Recorded on extension load if an extension has a valid uninstall URL set."
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 2 comments![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
2 comments
LGTM; thank you, Anton!
// TODO(crbug.com/510816360): Remove two following lines around M155, once we've
// gathered enough data in "Extensions.RuntimeUninstallURL.Host" histogram.nit: let's actually remove this TODO. It's a bit fragile -- other uses could crop up within this same file, includes could be reordered, etc. And removing unused includes should be done whenever removing code (though, of course, we do miss it sometimes), so the TODO below on removing the histogram emission should implicitly cover this
Related details
- Anton Bershanskyi
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. |
Anton Bershanskyi (Gerrit)
Anton Bershanskyi added 1 comment![]( "Open in Gerrit")
// TODO(crbug.com/510816360): Remove two following lines around M155, once we've
// gathered enough data in "Extensions.RuntimeUninstallURL.Host" histogram.nit: let's actually remove this TODO. It's a bit fragile -- other uses could crop up within this same file, includes could be reordered, etc. And removing unused includes should be done whenever removing code (though, of course, we do miss it sometimes), so the TODO below on removing the histogram emission should implicitly cover this
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
- Anton Bershanskyi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |