Closed Bug 787738 Opened 9 years ago Closed 9 years ago
Telemetry for Geolocation Prompt UI
We would like to measure the efficacy of the geo-location prompt via the Security UI Telemetry (Bug 767676)
Proposed Patch. Two cases are not covered because the popup notification API does not provide enough data to differentiate these cases. Does anyone have suggestions for how to fix this?
The lack of the two cases shouldn't impede this patch imho. We can still measure how many people didn't respond to the prompt. What we will miss is: out of those who didn't respond, how many clicked 'X' and closed vs. how many just clicked outside the prompt to make it go away. IMO, that doesn't matter for the security question. Anyone else have thoughts on this ?
(In reply to Devdatta Akhawe from comment #2) > The lack of the two cases shouldn't impede this patch imho. We can still > measure how many people didn't respond to the prompt. What we will miss is: > out of those who didn't respond, how many clicked 'X' and closed vs. how > many just clicked outside the prompt to make it go away. > > IMO, that doesn't matter for the security question. Anyone else have > thoughts on this ? Yeah, I don't see any reason we'd really care about these differentiating those cases for our purposes - they seem the same to me.
dveditz suggested that if this finishes up soon we should try to get it uplifted to ff17/Aurora so it can ship with the rest of the UI telemetry probes.
Comment on attachment 657621 [details] [diff] [review] Telemetry for Geolocation Prompt UI Review of attachment 657621 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/boot/public/nsISecurityUITelemetry.idl @@ +90,5 @@ > +const uint32_t WARNING_GEOLOCATION_REQUEST = 46; > +const uint32_t WARNING_GEOLOCATION_REQUEST_SHARE_LOCATION = 47; > +const uint32_t WARNING_GEOLOCATION_REQUEST_ALWAYS_SHARE = 48; > +const uint32_t WARNING_GEOLOCATION_REQUEST_NEVER_SHARE = 49; > +//TODO: It would be nice to measure the two cases below too. Don't add the TODO comment and unused values
Attachment #657621 - Flags: review?(felipc) → review+
Thanks! How about I remove the unused values, but leave a comment saying just "Space for 2 constants since it would be nice to know how many people click outside the button"? I basically want to mark off space in the IDL for the constants.
Attachment #657621 - Attachment is obsolete: true
Attachment #659415 - Attachment is obsolete: true
I forgot one detail: you need to rev the uuid of this file. No need for re-review, just remember to add that before pushing
Attachment #659432 - Attachment is obsolete: true
Comment on attachment 659493 [details] [diff] [review] Telemetry for Geolocation Prompt UI New uuid, carrying over the r+ from felipe
Attachment #659493 - Flags: review+
Dev pushed this to try : https://tbpl.mozilla.org/?tree=Try&rev=bc2f144e83b3 - looks good.
Pushed to inbound : https://hg.mozilla.org/integration/mozilla-inbound/rev/ec2b20c68ba2
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Comment on attachment 659493 [details] [diff] [review] Telemetry for Geolocation Prompt UI [Approval Request Comment] Bug caused by (feature/regressing bug #): n/a User impact if declined: n/a Testing completed (on m-c, etc.): has been on m-c since Monday Risk to taking this patch (and alternatives if risky): should be very low - adding a new telemetry probe String or UUID changes made by this patch: nsISecurityUITelemetry UUID change (interface was introduced in ff17, i believe) we would like to uplift this probe to Aurora/17 as the rest of the security UI telemetry probes landed in 17 and we can then start collecting a comprehensive set of security UI data in 17.
Attachment #659493 - Flags: approval-mozilla-aurora?
Attachment #659493 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Since the UUID change has Telemetry in the name we are assuming there shouldn't be much fallout from taking this.
Is there something QA can do to verify this fix?
I am afraid I am not the right person to answer that. One thing though: this was really an additional feature, not a fix. I don't see what QA can do beyond the basic testing I did.
I don't see much value in repeating what you've already tested, Devdatta. For now, flagging this [qa-]. Please change this whiteboard tag to the verifyme keyword if there is any requirement for manual verification from QA. Thanks
Whiteboard: [qa?] → [qa-]
You need to log in before you can comment on or make changes to this bug.