Add checkbox for reporting invalid TLS/SSL cert chains

chrome/browser/resources/security_warnings/safe_browsing.js

Index: chrome/browser/resources/security_warnings/safe_browsing.js
diff --git a/chrome/browser/resources/security_warnings/safe_browsing.js b/chrome/browser/resources/security_warnings/safe_browsing.js
index 3673da5ceefcc616a57118fa3d285c562b05d3a0..8283d6bec35e91189fcf5413ddcf2ba3624ad838 100644
--- a/chrome/browser/resources/security_warnings/safe_browsing.js
+++ b/chrome/browser/resources/security_warnings/safe_browsing.js
@@ -12,13 +12,24 @@ var SB_CMD_SHOW_DIAGNOSTIC = 'showDiagnostic';
 var SB_CMD_SHOW_PRIVACY = 'showPrivacy';
 var SB_CMD_TAKE_ME_BACK = 'takeMeBack';
 
-// Other constants defined in safe_browsing_blocking_page.cc.
+// Must match SSLBlockingPageCommands in ssl_blocking_page.h.

[felt, 2015/02/19 00:32:32]

Hmm, seems unfortunate to have SSLBlockingPageCommands in safe_browsing.js.
Maybe since this code is now shared, setupCheckbox should be moved into the
shared .js file?

[estark, 2015/02/19 02:32:08]

Hm, when you say "setupCheckbox should be moved" you mean "all the code in this
file", right? (I don't see another way to do it.) It does seem sort of nice to
have the separate file just for cleanliness, so what do you think of renaming
this file to extended_reporting.js, and maybe also renaming setupCheckbox to
setupExtendedReportingCheckbox?

[felt, 2015/02/24 01:57:52]

Yes, renaming it to extended_reporting.js could work.

The only remaining issue is w/r/t the constants. As you notice all the other
constants for ssl are defined in ssl.js. I believe that is OK, you can put them
there and they will still be usable in this scope. But double check in case I'm
mis-remembering.

[estark, 2015/02/24 18:47:08]

Done.

+var SSL_CMD_DO_REPORT = 6;
+var SSL_CMD_DONT_REPORT = 7;
+
+// Other constants defined in security_interstitial_page.h.
 var SB_BOX_CHECKED = 'boxchecked';
 var SB_DISPLAY_CHECK_BOX = 'displaycheckbox';
 
-// This sets up the Extended Safe Browsing Reporting opt-in.
-function setupCheckbox() {
-  if (loadTimeData.getString('type') != 'SAFEBROWSING' ||
+// This sets up the Extended Safe Browsing Reporting opt-in, either for
+// reporting malware or invalid certificate chains, depending on which
+// |type| is passed in. |type| can be 'SAFEBROWSING' or
+// 'SSL' (to set up the invalid cert collection checkbox).
+function setupCheckbox(type) {
+  if (type != 'SAFEBROWSING' && type != 'SSL') {

[felt, 2015/02/19 00:32:32]

what other type might this be? what are you checking for here?

[estark, 2015/02/19 02:32:08]

Oops, this check was blatantly wrong: replaced it with an early return for
irrelevant types.

+    throw new Error('setupCheckbox called for invalid interstitial.');
+  }
+
+  if (loadTimeData.getString('type') != type ||

[felt, 2015/02/19 00:32:32]

if you're going to check loadTimeData.getString('type') again here, why are you
also passing the value in as a parameter? seems like you should do one or the
other?

[estark, 2015/02/19 02:32:08]

Done.

       !loadTimeData.getBoolean(SB_DISPLAY_CHECK_BOX)) {
     return;
   }
@@ -29,7 +40,13 @@ function setupCheckbox() {
   $('body').classList.add('safe-browsing-has-checkbox');
 
   $('opt-in-checkbox').addEventListener('click', function() {
-    sendCommand(
-        $('opt-in-checkbox').checked ? SB_CMD_DO_REPORT : SB_CMD_DONT_REPORT);
+    var command;
+    if ($('opt-in-checkbox').checked) {
+      command = type == 'SAFEBROWSING' ? SB_CMD_DO_REPORT : SSL_CMD_DO_REPORT;
+    } else {
+      command = type == 'SAFEBROWSING' ?
+        SB_CMD_DONT_REPORT : SSL_CMD_DONT_REPORT;
+    }
+    sendCommand(command);
   });
 }