Last Comment Bug 787738 - Telemetry for Geolocation Prompt UI
: Telemetry for Geolocation Prompt UI
Status: RESOLVED FIXED
[qa-]
:
Product: Firefox
Classification: Client Software
Component: Security (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: Firefox 18
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-01 16:42 PDT by Devdatta Akhawe [:devd]
Modified: 2012-10-18 10:57 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Telemetry for Geolocation Prompt UI (4.32 KB, patch)
2012-09-01 18:59 PDT, Devdatta Akhawe [:devd]
felipc: review+
Details | Diff | Splinter Review
Telemetry for Geolocation Prompt UI (4.28 KB, patch)
2012-09-07 17:53 PDT, Devdatta Akhawe [:devd]
no flags Details | Diff | Splinter Review
Telemetry for Geolocation Prompt UI (4.28 KB, patch)
2012-09-07 18:48 PDT, Devdatta Akhawe [:devd]
felipc: review+
Details | Diff | Splinter Review
Telemetry for Geolocation Prompt UI (4.88 KB, patch)
2012-09-08 09:27 PDT, Devdatta Akhawe [:devd]
dev.akhawe+mozilla: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Devdatta Akhawe [:devd] 2012-09-01 16:42:01 PDT
We would like to measure the efficacy of the geo-location prompt via the Security UI Telemetry (Bug 767676)
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 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

Note You need to log in before you can comment on or make changes to this bug.