Comment 1 Devdatta Akhawe [:devd] 2012-09-01 18:59:57 PDT

Created attachment 657621 [details] [diff] [review]
Telemetry for Geolocation Prompt UI

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?

Comment 2 Devdatta Akhawe [:devd] 2012-09-05 13:20:55 PDT

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 ?

Comment 3 Ian Melven :imelven 2012-09-05 15:02:05 PDT

(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.

Comment 4 Ian Melven :imelven 2012-09-05 16:28:04 PDT

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 5 :Felipe Gomes (needinfo me!) 2012-09-07 16:03:51 PDT

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

Comment 6 Devdatta Akhawe [:devd] 2012-09-07 17:42:25 PDT

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.

Comment 7 Devdatta Akhawe [:devd] 2012-09-07 17:53:17 PDT

Created attachment 659415 [details] [diff] [review]
Telemetry for Geolocation Prompt UI

Comment 8 Devdatta Akhawe [:devd] 2012-09-07 18:48:45 PDT

Created attachment 659432 [details] [diff] [review]
Telemetry for Geolocation Prompt UI

Comment 9 :Felipe Gomes (needinfo me!) 2012-09-07 23:11:39 PDT

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

Comment 10 Devdatta Akhawe [:devd] 2012-09-08 09:27:21 PDT

Created attachment 659493 [details] [diff] [review]
Telemetry for Geolocation Prompt UI

Comment 11 Devdatta Akhawe [:devd] 2012-09-08 13:06:53 PDT

Comment on attachment 659493 [details] [diff] [review]
Telemetry for Geolocation Prompt UI

New uuid, carrying over the r+ from felipe

Comment 12 Ian Melven :imelven 2012-09-10 14:16:26 PDT

Dev pushed this to try : https://tbpl.mozilla.org/?tree=Try&rev=bc2f144e83b3 - looks good.

Comment 13 Ian Melven :imelven 2012-09-10 14:20:56 PDT

Pushed to inbound : https://hg.mozilla.org/integration/mozilla-inbound/rev/ec2b20c68ba2

Comment 14 Ryan VanderMeulen [:RyanVM] 2012-09-10 18:43:29 PDT

https://hg.mozilla.org/mozilla-central/rev/ec2b20c68ba2

Comment 15 Ian Melven :imelven 2012-09-12 13:54:49 PDT

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.

Comment 16 Lukas Blakk [:lsblakk] use ?needinfo 2012-09-12 15:53:51 PDT

Since the UUID change has Telemetry in the name we are assuming there shouldn't be much fallout from taking this.

Comment 17 Ian Melven :imelven 2012-09-13 13:42:58 PDT

https://hg.mozilla.org/releases/mozilla-aurora/rev/d648d93f9d76

Comment 18 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-17 16:35:21 PDT

Is there something QA can do to verify this fix?

Comment 19 Devdatta Akhawe [:devd] 2012-10-17 18:31:19 PDT

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.

Comment 20 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-18 10:57:01 PDT

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