767676 - Implement Security UI Telemetry

Status: RESOLVED FIXED

(Firefox :: Security, defect)

Description

[Devdatta Akhawe [:devd]]

More details on why here (currently a little rough)

wiki.mozilla.org/Security/Features/UI_Telemetry

Comment 1

[Devdatta Akhawe [:devd]]

Attachment: Base patch that setups the IDL

first time actually using queues, this 'base patch' might be stupid.

Comment 2

[Devdatta Akhawe [:devd]]

Attachment: Measure UI and Clickthrough rates for Addon install flow

Comment 3

[Devdatta Akhawe [:devd]]

Attachment: Measure UI and Clickthrough rates for modal dialogs thrown by nsSecurityWarnings.cpp

Comment 4

[Devdatta Akhawe [:devd]]

Attachment: Telemetry for Malware/Phishing Warnings

Comment 5

[Devdatta Akhawe [:devd]]

Attachment: Telemetry for SSL Warnings UI

Comment 6

[Devdatta Akhawe [:devd]]

Attachment: Telemetry for SSL Warnings UI

Comment 7

[Johnathan Nightingale [:johnath]]

I'm excited to see this happening! Are these works in progress, or should we be finding reviewers?

Comment 8

[Devdatta Akhawe [:devd]]

@johnath: I am waiting on reviews.

Comment 9

[Johnathan Nightingale [:johnath]]

(In reply to Devdatta Akhawe from comment #8)
> @johnath: I am waiting on reviews.

Typically, then, you should set the review:? flag and name specific reviewers for each patch. If you need help finding reviewers, I suspect dherman can help out.

Comment 10

[Devdatta Akhawe [:devd]]

uggh.. I messed up :(

I confused this bug with 707275 (SSL telemetry) where I am waiting for review.

For this bug, I want to get initial feedback from Sid Stamm. I am meeting with Sid today, and we will be discussing this. I will then request a review. Sorry for the confusing comment earlier.

Thanks for your interest. If you have thoughts on the feature and what things we should measure, I would love to hear them.

Comment 11

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 636036 [details] [diff] [review]
Base patch that setups the IDL

Review of attachment 636036 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/TelemetryHistograms.h
@@ +452,5 @@
>  
> +/**
> + * Security UI Telemetry
> + */
> +HISTOGRAM_ENUMERATED_VALUES(SECURITY_UI, 1000, "Security UI Telemetry")

1000?  Are there really 1000 things we're gonna measure?

Comment 12

[Devdatta Akhawe [:devd]]

Nope. I will change the final value once I am done with all of them.

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 636037 [details] [diff] [review]
Measure UI and Clickthrough rates for Addon install flow

Review of attachment 636037 [details] [diff] [review]:
-----------------------------------------------------------------

Your patches need more lines of context.  I'm only getting three which is not enough for reviewing.  Need 8.  https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in

The approach looks reasonable.  Fix my nits, then we'll flag it for review (probably mossop on this patch).

::: browser/base/content/browser-addons.js
@@ +3,4 @@
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +

Unnecessary whitespace change.  Remove this please.

::: security/manager/ssl/public/Makefile.in
@@ +67,4 @@
>      nsIKeyModule.idl \
>      nsIProtectedAuthThread.idl \
>      nsIDataSignatureVerifier.idl \
> +    nsISecurityUITelemetry.idl \

Please fix this typo in the patch that adds this line.

::: security/manager/ssl/public/nsISecurityUITelemetry.idl
@@ +14,5 @@
> + * Addon installation warnings
> + */
> +
> +//Firefox prevented this site from asking you to install addon
> +const PRUint32 WARNING_ADDON_ASKING_PREVENTED = 1;

This name confuses me... perhaps use something like WARNING_PREVENTED_ADDON_INSTALL

@@ +16,5 @@
> +
> +//Firefox prevented this site from asking you to install addon
> +const PRUint32 WARNING_ADDON_ASKING_PREVENTED = 1;
> +//User clicks through and allows site to ask to install addons
> +const PRUint32 WARNING_ADDON_ASKING_PREVENTED_CLICK_THROUGH = 2;

And then here... something like WARNING_CLICKEDTHRU_PREVENTED_ADDON_INSTALL.  Or if you describe them well in the IDL, you could use shorter names like:
ADDON_INSTALL_1, ADDON_INSTALL_2, etc.

@@ +22,5 @@
> +//Are you sure you want to install this addon? Only install addons you trust
> +const PRUint32 WARNING_CONFIRM_ADDON_INSTALL = 3;
> +
> +//User clicked she is sure after waiting 3secs
> +const PRUint32 WARNING_CONFIRM_ADDON_INSTALL_CLICK_THROUGH  = 4;

Nit: Use blank lines here consistently.

::: toolkit/mozapps/extensions/amWebInstallListener.js
@@ +173,4 @@
>      args.wrappedJSObject = args;
>  
>      try {
> +      let secHistogram = Cc["@mozilla.org/base/telemetry;1"].getService(Ci.nsITelemetry).getHistogramById("SECURITY_UI");

Nit: this line is way too long, please wrap it.

Comment 14

[Devdatta Akhawe [:devd]]

Sorry about the context, I was pretty sure I had fixed my .hgrc. 

There are two popups in the addon install flow. The first is "Firefox prevented this site from asking you to install", once you allow the site to ask you, the site actually tries to install and Firefox prevents that and shows up a dialog saying "Are you sure you want to install?" This is why I chose "ADDON_ASKING_PREVENTED" as the name here: PREVENTED_ADDON_INSTALL doesn't seem right.


browser.js already has some pretty  long JS lines; so I figured that line wrapping was only recommended for C/C++. What's the recommended way to wrap JS lines? I tried breaking on property accesses (the dots), but it looked pretty ugly to me.

Comment 15

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 636038 [details] [diff] [review]
Measure UI and Clickthrough rates for modal dialogs thrown by nsSecurityWarnings.cpp

Review of attachment 636038 [details] [diff] [review]:
-----------------------------------------------------------------

Please upload new patches with more lines of context and re-flag for feedback.  While you're at it, please look for any unnecessary newlines you've added -- there is at least one in this patch.

Comment 16

[Devdatta Akhawe [:devd]]

Attachment: UI Telemetry for SSL Warnings

Comment 17

[Devdatta Akhawe [:devd]]

Attachment: Measure prevalence and clickthrough of malware/phishing warnings

Comment 18

[Devdatta Akhawe [:devd]]

Attachment: Measure UI and Clickthrough rates for modal dialogs thrown by nsSecurityWarnings.cpp

Comment 19

[Devdatta Akhawe [:devd]]

Attachment: Measure UI and Clickthrough rates for Addon install flow

Comment 20

[Devdatta Akhawe [:devd]]

@geekboy: I think I have fixed all the nits. Should I go ahead and flag mossop for review?

Comment 21

[Sid Stamm [:geekboy or :sstamm]]

Yes... and please update the names on the "imported patches" (attachment 639860 [details] [diff] [review] and attachment 639861 [details] [diff] [review]) so it's clear what they do.

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 639859 [details] [diff] [review]
Measure prevalence and clickthrough of malware/phishing warnings

global code style nits:

>+        if(isMalware){

if (isMalware) {

>+          secHistogram.add(Ci.nsISecurityUITelemetry.WARNING_MALWARE_PAGE_GET_ME_OUT_OF_HERE);
>+        }else{

} else {

>+          //We log even if malware info URL couldn't be found: the measurement is for how

// We [...]

Comment 23

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 639858 [details] [diff] [review]
UI Telemetry for SSL Warnings

I had only looked at the one add-on UI patch and perhaps I wasn't clear that we should get mossop to review that one.  I still want to look at the rest (and will help identify reviewers for those).

Comment 24

[Devdatta Akhawe [:devd]]

@dao: thanks. I will fix those along with other nits I will have to fix after feedback.

Comment 25

[Devdatta Akhawe [:devd]]

I just talked to Taras, and he told me that changing the number of buckets in a Histogram would require changing the name of the histogram. In that case, it seems sensible to me to have a large bucket count for enumerated values such as these.

Comment 26

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 639860 [details] [diff] [review]
Measure UI and Clickthrough rates for modal dialogs thrown by nsSecurityWarnings.cpp

Review of attachment 639860 [details] [diff] [review]:
-----------------------------------------------------------------

General approach looks fine.  We should get mayhemer to look at this one.

::: security/manager/boot/src/nsSecurityWarningDialogs.cpp
@@ +129,4 @@
>    : mPrompt(aPrompt), mPrefName(aPrefName),
>      mDialogMessageName(aDialogMessageName),
>      mShowAgainName(aShowAgainName), mPrefBranch(aPrefBranch),
> +    mStringBundle(aStringBundle) {mTelemetryId = aTelemetryId;}

Use the same pattern as the rest of the default initializers here: mTelemetryId(aTelemetryId).

@@ +155,5 @@
>    // Stop if alert is not requested
>    if (!prefValue) return NS_OK;
>  
> +  MOZ_ASSERT(NS_IsMainThread());
> +  mozilla::Telemetry::Accumulate(mozilla::Telemetry::SECURITY_UI, mTelemetryId);

If you're looking to make this threadsafe, instead of MOZ_ASSERT, you should actually check if it's the main thread with an if statement, and only call Accumulate if it is.

@@ +218,2 @@
>    NS_ENSURE_TRUE(alert, NS_ERROR_OUT_OF_MEMORY);
> +  return aAsync ? NS_DispatchToCurrentThread(alert):alert->Run();

Please don't change the spacing here.

@@ +331,5 @@
>    if (NS_FAILED(rv)) return rv;
>  
>    *_result = (buttonPressed != 1);
> +  if(*_result){
> +  mozilla::Telemetry::Accumulate(mozilla::Telemetry::SECURITY_UI,aTelemetryId+1);

What is the +1 here?  And please fix if block and indent formatting.

Comment 27

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 639858 [details] [diff] [review]
UI Telemetry for SSL Warnings

Review of attachment 639858 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Fix nits, then flag gavin for review on this patch.

::: security/manager/pki/resources/content/exceptionDialog.js
@@ +11,5 @@
>  var gChecking;
>  var gBroken;
>  var gNeedReset;
>  
> +let secHistogram = Components.classes["@mozilla.org/base/telemetry;1"].getService(Components.interfaces.nsITelemetry).getHistogramById("SECURITY_UI");

Declare and use this like the other globals above it.  Call it gSomething, and initialize it in initExceptionDialog.

Also, this line is really long... please wrap it.

@@ +215,5 @@
>        var uts = "addExceptionUnverifiedShort";
>        var utl = "addExceptionUnverifiedLong";
>        var use1 = false;
>        if (gSSLStatus.isDomainMismatch) {
> +        telemetryId += Components.interfaces.nsISecurityUITelemetry.WARNING_BAD_CERT_ADD_EXCEPTION_FLAG_DOMAIN;

Ah, this += is a bitwise OR, then?  Please actually use a bitwise OR to avoid mistakes where the same flag is included twice.

For shorter lines, assign Components.interfaces.nsISecurityUITelemetry to some local var (say nsISecTel) and use that instead of x.y.z.foo.

Comment 28

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 639859 [details] [diff] [review]
Measure prevalence and clickthrough of malware/phishing warnings

Review of attachment 639859 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good too... please wrap long lines and flag gavin (or whomever he recommends) for review of this patch.

Comment 29

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 636036 [details] [diff] [review]
Base patch that setups the IDL

Brian, can you either r/sr this new IDL (constants for telemetry) or tell us who can r/sr the patch?

Comment 30

[Devdatta Akhawe [:devd]]

Attachment: Measure UI and clickthrough rates for modal dialogs thrown by nsSecurityWarningDialogs.cpp

Comment 31

[Devdatta Akhawe [:devd]]

Attachment: UI Telemetry for SSL Warnings

Comment 32

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 640405 [details] [diff] [review]
UI Telemetry for SSL Warnings

forwarding to felipe!

Comment 33

[Devdatta Akhawe [:devd]]

Attachment: Measure prevalence and clickthrough of malware/phishing warnings

Comment 34

[Honza Bambas (:mayhemer)]

Comment on attachment 636036 [details] [diff] [review]
Base patch that setups the IDL

Place nsISecurityUITelemetry.idl in security/manager/boot/public.

Comment 35

[Honza Bambas (:mayhemer)]

Comment on attachment 640397 [details] [diff] [review]
Measure UI and clickthrough rates for modal dialogs thrown by nsSecurityWarningDialogs.cpp

Review of attachment 640397 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/boot/src/nsSecurityWarningDialogs.cpp
@@ +332,5 @@
>  
>    *_result = (buttonPressed != 1);
> +  if (*_result) {
> +  //For confirmation dialogs, the clickthrough constant is 1 more than the constant
> +  //for the dialog.

Space after //.

You should reflect this also in the idl file and where ConfirmDialog is called.

@@ +333,5 @@
>    *_result = (buttonPressed != 1);
> +  if (*_result) {
> +  //For confirmation dialogs, the clickthrough constant is 1 more than the constant
> +  //for the dialog.
> +  mozilla::Telemetry::Accumulate(mozilla::Telemetry::SECURITY_UI,aTelemetryId+1);

space after , and spaces around +.

Comment 36

[Devdatta Akhawe [:devd]]

Attachment: Measure UI and clickthrough rates for modal dialogs thrown by nsSecurityWarningDialogs.cpp

Comment 37

[Devdatta Akhawe [:devd]]

Thanks Honza. I have fixed the patch; it seems I can't actually carry over the review+ though. Can you please set the flag for the new attachment?

Also, re moving the idl file: I am not certain whether that is also the correct place for it (maybe it is better to keep it in components/toolkit/telemetry). Since updating the location means updating all the patches, I am going to keep it in the current place for now and move it once I have further input on the correct place.

Comment 38

[Honza Bambas (:mayhemer)]

Comment on attachment 640776 [details] [diff] [review]
Measure UI and clickthrough rates for modal dialogs thrown by nsSecurityWarningDialogs.cpp

r=honzab

Comment 39

[Dave Townsend [:mossop]]

Comment on attachment 639861 [details] [diff] [review]
Measure UI and Clickthrough rates for Addon install flow

r+ for the add-ons bits. You should probably double-check the nsISecurityUITelemetry.idl changes with someone appropriate.

Comment 40

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Devdatta Akhawe from comment #25)
> I just talked to Taras, and he told me that changing the number of buckets
> in a Histogram would require changing the name of the histogram. In that
> case, it seems sensible to me to have a large bucket count for enumerated
> values such as these.

It is also important to limit the number of buckets. I suggest cutting the number from 1000 to 100 or less.

Comment 41

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 640405 [details] [diff] [review]
UI Telemetry for SSL Warnings

Review of attachment 640405 [details] [diff] [review]:
-----------------------------------------------------------------

How does the enumerated histogram work? Does it value gets its own bucket, which don't necessarily have any statistical correlation? That sounds ok, just trying to understand if that's what it means.

The OR'ing of values you're doing in exceptionDialog.js seems weird to me. You should just add a data point entry for each of the cases, otherwise the data for a single case will be under-represented for when it's merged (OR'ed) with another value. (sure in theory the data will still be there but it will make analysis harder)
Also you're already at the powers of 2 limit because adding another one will make it fall into the next group (e.g. 30 + 4 + 8 will be mixed with 40 + 2).
If there's any value in the merged data you can always report it in an extra bucket.


And the variable name 'telemetryId' isn't really great because all this data is going to the same histogram (with id="SECURITY_UI"), so if you still need to use it there or elsewhere I'd suggest "telemetryBucket" or "bucketId" for example.

Comment 42

[Devdatta Akhawe [:devd]]

@bsmith: sure. Do you also have input on where to put the idl ?

Comment 43

[Devdatta Akhawe [:devd]]

Attachment: UI Telemetry for SSL Warnings

Comment 44

[Devdatta Akhawe [:devd]]

thanks Felipe. I have uploaded an updated patch.

Comment 45

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 636036 [details] [diff] [review]
Base patch that setups the IDL

Review of attachment 636036 [details] [diff] [review]:
-----------------------------------------------------------------

I think all or most of the PSM UI stuff is in boot/, so put this interface in security/manager/boot/ instead of security/manager/ssl/.

Please file bugs for removing the prompts, especially the prompts that originate from within PSM.

::: toolkit/components/telemetry/TelemetryHistograms.h
@@ +452,5 @@
>  
> +/**
> + * Security UI Telemetry
> + */
> +HISTOGRAM_ENUMERATED_VALUES(SECURITY_UI, 1000, "Security UI Telemetry")

Reduce this to 100 or even less unless you think we already have close to 100 different warnings that we are going to measure here.

Comment 46

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 640413 [details] [diff] [review]
Measure prevalence and clickthrough of malware/phishing warnings

Review of attachment 640413 [details] [diff] [review]:
-----------------------------------------------------------------

feedback+=felipe

Just nits for a review+, but I wanna take a quick look at the final patch

::: browser/base/content/browser.js
@@ +2531,5 @@
>        // The event came from a button on a malware/phishing block page
>        // First check whether it's malware or phishing, so that we can
>        // use the right strings/links
>        var isMalware = /e=malwareBlocked/.test(ownerDoc.documentURI);
>  

You can reduce the telemetry footprint being added in this file by doing:

let bucketName = isMalware ? "WARNING_MALWARE_PAGE_" : "WARNING_PHISHING_PAGE_";

and then on each about:* case you just need to add one line:

secHistogram.add(Ci.nsISecurityUITelemetry[bucketName + "GET_ME_OUT_OF_HERE"]);

::: docshell/base/nsDocShell.cpp
@@ +4047,5 @@
>              Preferences::GetCString("urlclassifier.alternate_error_page");
>          if (alternateErrorPage)
>              errorPage.Assign(alternateErrorPage);
>  
> +        PRUint32 telemetryid;

Same comment as the previous patch, change this name to bucketId;

Comment 47

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 641167 [details] [diff] [review]
UI Telemetry for SSL Warnings

Review of attachment 641167 [details] [diff] [review]:
-----------------------------------------------------------------

r=felipe with the change to updateCertStatus and removal of IgnoreCase (if that is not needed)

::: docshell/base/nsDocShell.cpp
@@ +4039,5 @@
>                          "security.alternate_certificate_error_page");
>                  if (alternateErrorPage)
>                      errorPage.Assign(alternateErrorPage);
> +
> +                if (errorPage.EqualsIgnoreCase("certerror")) 

do you need IgnoreCase? I believe Equals should be enough

::: security/manager/pki/resources/content/exceptionDialog.js
@@ +15,1 @@
>  

Suggestion: you could define nsISecTel as a const here only once and use it elsewhere, including in the viewCertButtonClick.

@@ +218,5 @@
>        var uts = "addExceptionUnverifiedShort";
>        var utl = "addExceptionUnverifiedLong";
>        var use1 = false;
>        if (gSSLStatus.isDomainMismatch) {
> +        bucketId |= nsISecTel.WARNING_BAD_CERT_ADD_EXCEPTION_FLAG_DOMAIN;

forgot to change the bitwise OR to addition in this section

Comment 48

[Devdatta Akhawe [:devd]]

Attachment: UI Telemetry for SSL Warnings

Comment 49

[Devdatta Akhawe [:devd]]

Attachment: Measure prevalence and clickthrough of malware/phishing warnings

Comment 50

[Devdatta Akhawe [:devd]]

Attachment: Measure UI and clickthrough rates for modal dialogs thrown by nsSecurityWarningDialogs.cpp

Comment 51

[Devdatta Akhawe [:devd]]

Attachment: Measure UI and Clickthrough rates for Addon install flow

Comment 52

[Devdatta Akhawe [:devd]]

Attachment: Base patch that setups the IDL

Comment 53

[Devdatta Akhawe [:devd]]

Thanks, bsmith! I have moved the location of the IDL, and modified the histogram. We are already nearing 50; I propose keeping it at 100. 

Moving the location changed all the patches: I don't have the privileges to carry over the review flags. Can someone with the requisite privileges carry the review +s ?

Comment 54

[Devdatta Akhawe [:devd]]

Thanks felipe. I have modified all the patches. As discussed on IRC, I am keeping EqualsIgnoreCase since aboutRedirector.cpp lower cases everything before redirecting. Thus, about:CertError could also work; so I am being conservative.

Comment 55

[Devdatta Akhawe [:devd]]

Attachment: Measure UI and Clickthrough rates for Addon install flow

Comment 56

[Devdatta Akhawe [:devd]]

Attachment: Measure UI and clickthrough rates for modal dialogs thrown by nsSecurityWarningDialogs.cpp

Comment 57

[Mozilla RelEng Bot]

Try run for 4cca78c3a102 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4cca78c3a102
Results (out of 219 total builds):
    success: 198
    warnings: 12
    failure: 9
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4cca78c3a102

Comment 58

[Mozilla RelEng Bot]

Try run for 4cca78c3a102 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4cca78c3a102
Results (out of 225 total builds):
    success: 203
    warnings: 13
    failure: 9
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4cca78c3a102

Comment 59

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 642279 [details] [diff] [review]
Measure UI and clickthrough rates for modal dialogs thrown by nsSecurityWarningDialogs.cpp

Review of attachment 642279 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/boot/public/nsISecurityUITelemetry.idl
@@ +30,5 @@
> +
> +const PRUint32 WARNING_ENTERING_SECURE_SITE = 5;
> +const PRUint32 WARNING_ENTERING_WEAK_SITE = 6;
> +const PRUint32 WARNING_LEAVING_SECURE_SITE = 7;
> +const PRUint32 WARNING_MIXED_CONTENT  = 8;

Insert newline here.

@@ +32,5 @@
> +const PRUint32 WARNING_ENTERING_WEAK_SITE = 6;
> +const PRUint32 WARNING_LEAVING_SECURE_SITE = 7;
> +const PRUint32 WARNING_MIXED_CONTENT  = 8;
> +// For confirmation dialogs, the clickthrough constant needs to be 1
> +// more than the dialog constant

... so that WARNING_CONFIRM_<X> + 1 == WARNING_CONFIRM_<X>_CLICK_THROUGH.

::: security/manager/boot/src/nsSecurityWarningDialogs.cpp
@@ +139,5 @@
>    nsString                  mDialogMessageName;
>    nsString                  mShowAgainName;
>    nsCOMPtr<nsIPrefBranch>   mPrefBranch;
>    nsCOMPtr<nsIStringBundle> mStringBundle;
> +  PRUint32                  mTelemetryId;

Should be const.

Do not name these variables "telemetryId" because mozilla::Telemetry::ID is something different (the ID of the probe). "telemetryValue" would be better as it's the value (not the ID) used for the telemetry.

@@ +155,5 @@
>  
>    // Stop if alert is not requested
>    if (!prefValue) return NS_OK;
>  
> +  mozilla::Telemetry::Accumulate(mozilla::Telemetry::SECURITY_UI, mTelemetryId);

"

@@ +200,5 @@
>                                        const char* aPrefName,
>                                        const PRUnichar* aDialogMessageName,
>                                        const PRUnichar* aShowAgainName,
> +                                      bool aAsync,
> +                                      PRUint32 aTelemetryId)

"

@@ +212,5 @@
>                                                    aDialogMessageName,
>                                                    aShowAgainName,
>                                                    mPrefBranch,
> +                                                  mStringBundle,
> +                                                  aTelemetryId);

"

@@ +225,5 @@
>  nsSecurityWarningDialogs::ConfirmPostToInsecure(nsIInterfaceRequestor *ctx, bool* _result)
>  {
>    nsresult rv;
>  
> +  // The Telemetry clickthrough constant is 1 more than the constant for the dialog.

Do not need this comment.

@@ +254,5 @@
>  nsresult
>  nsSecurityWarningDialogs::ConfirmDialog(nsIInterfaceRequestor *ctx, const char *prefName,
>                              const PRUnichar *messageName, 
>                              const PRUnichar *showAgainName, 
> +                            PRUint32 aTelemetryId,

"

Comment 60

[Devdatta Akhawe [:devd]]

Attachment: Measure UI and clickthrough rates for modal dialogs thrown by nsSecurityWarningDialogs.cpp

Comment 61

[Devdatta Akhawe [:devd]]

Comment on attachment 642277 [details] [diff] [review]
Measure UI and Clickthrough rates for Addon install flow

carrying over r+ from dtownsend

Comment 62

[Devdatta Akhawe [:devd]]

Comment on attachment 647404 [details] [diff] [review]
Measure UI and clickthrough rates for modal dialogs thrown by nsSecurityWarningDialogs.cpp

carrying over r+ from honzab and bsmith

Comment 63

[Devdatta Akhawe [:devd]]

Attachment: Folded Patch

Merged into a single patch.

Comment 64

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 648590 [details] [diff] [review]
Folded Patch

Bringing forward r+

Comment 65

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c2090a46d34e

Comment 66

[Ed Morley [:emorley]]

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

Comment 67

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 648590 [details] [diff] [review]
Folded Patch

Just to be clear, the landing was for this rollup patch of all the above patches.

Comment 68

[Devdatta Akhawe [:devd]]

I would like to propose a probe for the geolocation  prompt. I didn't implement this originally because I wasn't aware of the WiFi+IP based location capability. See Bug 787738