Finch experiments on SSL, malware, and phishing interstitials

Finch experiments on SSL, malware, and phishing interstitials

Adds new versions of the interstitials according to a controlled experiment. Also adds some new UMA histogram fields for SSL and Safe Browsing.

This folio shows what each of the conditions look like: https://folio.googleplex.com/finch_interstitial/

See bug for detailed description of the experiment.

BUG=224070
TBR=agl@chromium.org, jhawkins@chromium.org, jwd@chromium.org, mattm@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198656

[felt, 2013-05-01 06:58:02]

Hi Jesse,

Could you please take a look at the use of FieldTrial in ssl_blocking_page.cc?

I'll be uploading additional changes to safe_browsing_blocking_page.cc tomorrow,
but it's essentially a repetition of what I did in ssl_blocking_page.

Of note: the images in three of the conditions (the images that look like
sketches) will be replaced with updated versions as soon as I have the final
versions in hand.

Thanks,
Adrienne

[felt, 2013-05-01 06:59:28]

Re-sending because I accidentally entered your @google instead of @chromium.

On 2013/05/01 06:58:02, felt wrote:
> Hi Jesse,
> 
> Could you please take a look at the use of FieldTrial in ssl_blocking_page.cc?
> 
> I'll be uploading additional changes to safe_browsing_blocking_page.cc
tomorrow,
> but it's essentially a repetition of what I did in ssl_blocking_page.
> 
> Of note: the images in three of the conditions (the images that look like
> sketches) will be replaced with updated versions as soon as I have the final
> versions in hand.
> 
> Thanks,
> Adrienne

[felt, 2013-05-01 17:06:45]

Hi abarth & agl: could you take a look at the modifications I made to the SSL
interstitial for a Finch trial?

I need OWNERS from abarth, but last time I touched the SSL interstitial, agl did
the primary review.

Thanks,
Adrienne

[felt, 2013-05-01 17:07:40]

Hi Matt,

Can you please take a look at the modifications I made to the SB interstitials
for a Finch experiment?

Thanks,
Adrienne

[agl, 2013-05-01 18:54:12]

It's not obvious to me where all the different variations of the page are
getting used, but I'll assume that it works somehow :)

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:117: static const
std::string kCond1MalwareControl = "Cond1MalwareControl";
static const char kCond1Foo[] = "...";

avoid static initialisers!

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:504:
MALWARE_ADVANCED,
likewise, I'm not sure that these enum elements have the same style as the
others.

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:928: trial_ = "";
std::strings do not need to be explicitly cleared.

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:929: if
(unsafe_resources_[0].threat_type == SB_THREAT_TYPE_URL_MALWARE)
I feel that once a conditional grows past an if and a single line body then
braces are needed.

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.h (right):

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.h:114: ADVANCED
a comma at the end of the line is typical.

This doesn't appear to fit with the style of the other elements of the enum.
Should it be "SHOW_ADVANCED"?

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.h:266: // For the
FieldTrial.
// trail_ contains the Finch trial name.

(Or whatever is actually accurate.)

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/ssl/ssl_bl...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:156: static const std::string
kCondition15Control = "Condition15SSLControl";
static const char kFoo[] = "...";

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:186: trial_ = "";
doesn't need to be initially cleared.

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:269: if (trial_ == kCondition16Firefox)
braces needed I think.

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/ssl/ssl_bl...
File chrome/browser/ssl/ssl_blocking_page.h (right):

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.h:76: // For the FieldTrial.
ditto.

[felt, 2013-05-01 19:39:45]

Most of the logic that controls the conditions is actually in the JavaScript,
not in the C++. For most conditions, all the C++ code does is tell the JS page
what the condition is, and the JS does the rest. The only exceptions are the
"Firefox" and "fancy Firefox" SLL conditions which needed their own HTML because
they were too different.

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:117: static const
std::string kCond1MalwareControl = "Cond1MalwareControl";
On 2013/05/01 18:54:12, agl wrote:
> static const char kCond1Foo[] = "...";
> 
> avoid static initialisers!

Done.

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:504:
MALWARE_ADVANCED,
On 2013/05/01 18:54:12, agl wrote:
> likewise, I'm not sure that these enum elements have the same style as the
> others.

Done.

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:928: trial_ = "";
On 2013/05/01 18:54:12, agl wrote:
> std::strings do not need to be explicitly cleared.

Done.

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:929: if
(unsafe_resources_[0].threat_type == SB_THREAT_TYPE_URL_MALWARE)
On 2013/05/01 18:54:12, agl wrote:
> I feel that once a conditional grows past an if and a single line body then
> braces are needed.

Done.

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.h (right):

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.h:114: ADVANCED
On 2013/05/01 18:54:12, agl wrote:
> a comma at the end of the line is typical.
> 
> This doesn't appear to fit with the style of the other elements of the enum.
> Should it be "SHOW_ADVANCED"?

Done.

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.h:266: // For the
FieldTrial.
On 2013/05/01 18:54:12, agl wrote:
> // trail_ contains the Finch trial name.
> 
> (Or whatever is actually accurate.)

Done.

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/ssl/ssl_bl...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:156: static const std::string
kCondition15Control = "Condition15SSLControl";
On 2013/05/01 18:54:12, agl wrote:
> static const char kFoo[] = "...";

Done.

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:186: trial_ = "";
On 2013/05/01 18:54:12, agl wrote:
> doesn't need to be initially cleared.

Done.

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:269: if (trial_ == kCondition16Firefox)
On 2013/05/01 18:54:12, agl wrote:
> braces needed I think.

Done.

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:269: if (trial_ == kCondition16Firefox)
On 2013/05/01 18:54:12, agl wrote:
> braces needed I think.

Done.

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/ssl/ssl_bl...
File chrome/browser/ssl/ssl_blocking_page.h (right):

https://codereview.chromium.org/14752005/diff/16002/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.h:76: // For the FieldTrial.
On 2013/05/01 18:54:12, agl wrote:
> ditto.

Done.

[agl, 2013-05-01 19:46:20]

LGTM for *.h, *.cc.

https://codereview.chromium.org/14752005/diff/28001/chrome/browser/ssl/ssl_bl...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/28001/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:216: int resource_id =
IDR_SSL_ROAD_BLOCK_HTML;
it's not clear why this line moved.

[felt, 2013-05-01 19:54:44]

jhawkins - could you please take a look at the HTML and JS files (in
chrome/browser/resources/)?

On 2013/05/01 19:46:20, agl wrote:
> LGTM for *.h, *.cc.
> 
>
https://codereview.chromium.org/14752005/diff/28001/chrome/browser/ssl/ssl_bl...
> File chrome/browser/ssl/ssl_blocking_page.cc (right):
> 
>
https://codereview.chromium.org/14752005/diff/28001/chrome/browser/ssl/ssl_bl...
> chrome/browser/ssl/ssl_blocking_page.cc:216: int resource_id =
> IDR_SSL_ROAD_BLOCK_HTML;
> it's not clear why this line moved.

[James Hawkins, 2013-05-01 20:01:54]

These added files need to be in a new subdirectory of resources/

[mattm, 2013-05-01 20:17:17]

https://codereview.chromium.org/14752005/diff/28001/chrome/browser/resources/...
File chrome/browser/resources/safe_browsing/malware_block_v2.js (right):

https://codereview.chromium.org/14752005/diff/28001/chrome/browser/resources/...
chrome/browser/resources/safe_browsing/malware_block_v2.js:42: var condition =
$('trial-type').innerHTML;
Don't need to stick this in a hidden div to get it, the data should be in
templataData.trialType

https://codereview.chromium.org/14752005/diff/28001/chrome/browser/resources/...
chrome/browser/resources/safe_browsing/malware_block_v2.js:55: 'proceed. Go
back!';
strings need to go in chrome/app/generated_resources.grd, then get passed
through templateData

[felt, 2013-05-01 20:36:29]

https://codereview.chromium.org/14752005/diff/28001/chrome/browser/resources/...
File chrome/browser/resources/safe_browsing/malware_block_v2.js (right):

https://codereview.chromium.org/14752005/diff/28001/chrome/browser/resources/...
chrome/browser/resources/safe_browsing/malware_block_v2.js:55: 'proceed. Go
back!';
Is that worth doing even though this is just an experiment? It's not being
internationalized - the experiment is set up to only go to en-US and en-GB.

On 2013/05/01 20:17:17, mattm wrote:
> strings need to go in chrome/app/generated_resources.grd, then get passed
> through templateData

https://codereview.chromium.org/14752005/diff/28001/chrome/browser/ssl/ssl_bl...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/28001/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:216: int resource_id =
IDR_SSL_ROAD_BLOCK_HTML;
I just thought it was clearer to move it up here, rather than being in the
middle of all the SetString statements/logic.

On 2013/05/01 19:46:20, agl wrote:
> it's not clear why this line moved.

[mattm, 2013-05-01 20:49:11]

https://codereview.chromium.org/14752005/diff/28001/chrome/browser/resources/...
File chrome/browser/resources/safe_browsing/malware_block_v2.js (right):

https://codereview.chromium.org/14752005/diff/28001/chrome/browser/resources/...
chrome/browser/resources/safe_browsing/malware_block_v2.js:55: 'proceed. Go
back!';
On 2013/05/01 20:36:29, felt wrote:
> Is that worth doing even though this is just an experiment? It's not being
> internationalized - the experiment is set up to only go to en-US and en-GB.
> 
> On 2013/05/01 20:17:17, mattm wrote:
> > strings need to go in chrome/app/generated_resources.grd, then get passed
> > through templateData
> 

Ah, missed that part. I think the style is still to put strings in grit, even
non-translated ones (setting translateable="false" on them).  James, do you have
any additional feelings on this?

Wherever they end up, there should be a comment about why they aren't
translated.

[James Hawkins, 2013-05-01 20:49:55]

https://codereview.chromium.org/14752005/diff/28001/chrome/browser/resources/...
File chrome/browser/resources/safe_browsing/malware_block_v2.js (right):

https://codereview.chromium.org/14752005/diff/28001/chrome/browser/resources/...
chrome/browser/resources/safe_browsing/malware_block_v2.js:55: 'proceed. Go
back!';
On 2013/05/01 20:49:12, mattm wrote:
> On 2013/05/01 20:36:29, felt wrote:
> > Is that worth doing even though this is just an experiment? It's not being
> > internationalized - the experiment is set up to only go to en-US and en-GB.
> > 
> > On 2013/05/01 20:17:17, mattm wrote:
> > > strings need to go in chrome/app/generated_resources.grd, then get passed
> > > through templateData
> > 
> 
> Ah, missed that part. I think the style is still to put strings in grit, even
> non-translated ones (setting translateable="false" on them).  James, do you
have
> any additional feelings on this?
> 
> Wherever they end up, there should be a comment about why they aren't
> translated.

Yes, strings in grit.

[felt, 2013-05-01 22:46:44]

I moved everything relating to the ssl warning into a ssl/ folder.

https://codereview.chromium.org/14752005/diff/28001/chrome/browser/resources/...
File chrome/browser/resources/safe_browsing/malware_block_v2.js (right):

https://codereview.chromium.org/14752005/diff/28001/chrome/browser/resources/...
chrome/browser/resources/safe_browsing/malware_block_v2.js:42: var condition =
$('trial-type').innerHTML;
On 2013/05/01 20:17:17, mattm wrote:
> Don't need to stick this in a hidden div to get it, the data should be in
> templataData.trialType

Done.

https://codereview.chromium.org/14752005/diff/28001/chrome/browser/resources/...
chrome/browser/resources/safe_browsing/malware_block_v2.js:55: 'proceed. Go
back!';
On 2013/05/01 20:49:55, James Hawkins wrote:
> On 2013/05/01 20:49:12, mattm wrote:
> > On 2013/05/01 20:36:29, felt wrote:
> > > Is that worth doing even though this is just an experiment? It's not being
> > > internationalized - the experiment is set up to only go to en-US and
en-GB.
> > > 
> > > On 2013/05/01 20:17:17, mattm wrote:
> > > > strings need to go in chrome/app/generated_resources.grd, then get
passed
> > > > through templateData
> > > 
> > 
> > Ah, missed that part. I think the style is still to put strings in grit,
even
> > non-translated ones (setting translateable="false" on them).  James, do you
> have
> > any additional feelings on this?
> > 
> > Wherever they end up, there should be a comment about why they aren't
> > translated.
> 
> Yes, strings in grit.

Done.

[James Hawkins, 2013-05-01 22:53:19]

Please remove ssl_ from all of the files in the ssl/ directory, now that the ssl
directory serves the same namespacing purpose.

[felt, 2013-05-01 23:12:32]

On 2013/05/01 22:53:19, James Hawkins wrote:
> Please remove ssl_ from all of the files in the ssl/ directory, now that the
ssl
> directory serves the same namespacing purpose.

Done.

[James Hawkins, 2013-05-01 23:21:45]

Note that I'm not going over the experimental .html files very closely; once we
pick a winner we need to review them properly.

https://codereview.chromium.org/14752005/diff/44001/chrome/browser/resources/...
File chrome/browser/resources/safe_browsing/malware_block_v2.js (right):

https://codereview.chromium.org/14752005/diff/44001/chrome/browser/resources/...
chrome/browser/resources/safe_browsing/malware_block_v2.js:41: function
setupFinch() {
What is Finch?  That's a rhetorical question.  The name 'Finch' should not be
used; consider naming it something like "setupInterstitialExperiment" or
something.

https://codereview.chromium.org/14752005/diff/44001/chrome/browser/resources/...
File chrome/browser/resources/ssl/roadblock.html (right):

https://codereview.chromium.org/14752005/diff/44001/chrome/browser/resources/...
chrome/browser/resources/ssl/roadblock.html:187: function setupFinch() {
setup<Some other name>

https://codereview.chromium.org/14752005/diff/44001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.h (right):

https://codereview.chromium.org/14752005/diff/44001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.h:267: std::string
trial_;
Maybe trialCondition? or something

https://codereview.chromium.org/14752005/diff/44001/chrome/browser/ssl/ssl_bl...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/44001/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:156: static const char
kCondition15Control[] = "Condition15SSLControl";
static is unnecessary (and should not be used) in an unnamed namespace.

https://codereview.chromium.org/14752005/diff/44001/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:216: int resource_id =
IDR_SSL_ROAD_BLOCK_HTML;
static const int

https://codereview.chromium.org/14752005/diff/44001/chrome/browser/ssl/ssl_bl...
File chrome/browser/ssl/ssl_blocking_page.h (right):

https://codereview.chromium.org/14752005/diff/44001/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.h:77: std::string trial_;
trialCondition_?

[felt, 2013-05-02 16:07:12]

James: once you give approval, I'm going to move all the image renaming changes
into another CL so that I can directly commit the images changes (trybots can't
run this CL with the image changes in it). I'll TBR you on that second CL.

https://codereview.chromium.org/14752005/diff/44001/chrome/browser/resources/...
File chrome/browser/resources/safe_browsing/malware_block_v2.js (right):

https://codereview.chromium.org/14752005/diff/44001/chrome/browser/resources/...
chrome/browser/resources/safe_browsing/malware_block_v2.js:41: function
setupFinch() {
On 2013/05/01 23:21:45, James Hawkins wrote:
> What is Finch?  That's a rhetorical question.  The name 'Finch' should not be
> used; consider naming it something like "setupInterstitialExperiment" or
> something.

Done.

https://codereview.chromium.org/14752005/diff/44001/chrome/browser/resources/...
File chrome/browser/resources/ssl/roadblock.html (right):

https://codereview.chromium.org/14752005/diff/44001/chrome/browser/resources/...
chrome/browser/resources/ssl/roadblock.html:187: function setupFinch() {
On 2013/05/01 23:21:45, James Hawkins wrote:
> setup<Some other name>

Done.

https://codereview.chromium.org/14752005/diff/44001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.h (right):

https://codereview.chromium.org/14752005/diff/44001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.h:267: std::string
trial_;
On 2013/05/01 23:21:45, James Hawkins wrote:
> Maybe trialCondition? or something

Done.

https://codereview.chromium.org/14752005/diff/44001/chrome/browser/ssl/ssl_bl...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/44001/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:216: int resource_id =
IDR_SSL_ROAD_BLOCK_HTML;
On 2013/05/01 23:21:45, James Hawkins wrote:
> static const int

I assign to this int later.

https://codereview.chromium.org/14752005/diff/44001/chrome/browser/ssl/ssl_bl...
File chrome/browser/ssl/ssl_blocking_page.h (right):

https://codereview.chromium.org/14752005/diff/44001/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.h:77: std::string trial_;
On 2013/05/01 23:21:45, James Hawkins wrote:
> trialCondition_?

Done.

[felt, 2013-05-02 16:13:57]

Carlos- can you please give OWNERS approval for
chrome/app/generated_resources.grd? I had to add some string constants.

On 2013/05/02 16:07:12, felt wrote:
> James: once you give approval, I'm going to move all the image renaming
changes
> into another CL so that I can directly commit the images changes (trybots
can't
> run this CL with the image changes in it). I'll TBR you on that second CL.
> 
>
https://codereview.chromium.org/14752005/diff/44001/chrome/browser/resources/...
> File chrome/browser/resources/safe_browsing/malware_block_v2.js (right):
> 
>
https://codereview.chromium.org/14752005/diff/44001/chrome/browser/resources/...
> chrome/browser/resources/safe_browsing/malware_block_v2.js:41: function
> setupFinch() {
> On 2013/05/01 23:21:45, James Hawkins wrote:
> > What is Finch?  That's a rhetorical question.  The name 'Finch' should not
be
> > used; consider naming it something like "setupInterstitialExperiment" or
> > something.
> 
> Done.
> 
>
https://codereview.chromium.org/14752005/diff/44001/chrome/browser/resources/...
> File chrome/browser/resources/ssl/roadblock.html (right):
> 
>
https://codereview.chromium.org/14752005/diff/44001/chrome/browser/resources/...
> chrome/browser/resources/ssl/roadblock.html:187: function setupFinch() {
> On 2013/05/01 23:21:45, James Hawkins wrote:
> > setup<Some other name>
> 
> Done.
> 
>
https://codereview.chromium.org/14752005/diff/44001/chrome/browser/safe_brows...
> File chrome/browser/safe_browsing/safe_browsing_blocking_page.h (right):
> 
>
https://codereview.chromium.org/14752005/diff/44001/chrome/browser/safe_brows...
> chrome/browser/safe_browsing/safe_browsing_blocking_page.h:267: std::string
> trial_;
> On 2013/05/01 23:21:45, James Hawkins wrote:
> > Maybe trialCondition? or something
> 
> Done.
> 
>
https://codereview.chromium.org/14752005/diff/44001/chrome/browser/ssl/ssl_bl...
> File chrome/browser/ssl/ssl_blocking_page.cc (right):
> 
>
https://codereview.chromium.org/14752005/diff/44001/chrome/browser/ssl/ssl_bl...
> chrome/browser/ssl/ssl_blocking_page.cc:216: int resource_id =
> IDR_SSL_ROAD_BLOCK_HTML;
> On 2013/05/01 23:21:45, James Hawkins wrote:
> > static const int
> 
> I assign to this int later.
> 
>
https://codereview.chromium.org/14752005/diff/44001/chrome/browser/ssl/ssl_bl...
> File chrome/browser/ssl/ssl_blocking_page.h (right):
> 
>
https://codereview.chromium.org/14752005/diff/44001/chrome/browser/ssl/ssl_bl...
> chrome/browser/ssl/ssl_blocking_page.h:77: std::string trial_;
> On 2013/05/01 23:21:45, James Hawkins wrote:
> > trialCondition_?
> 
> Done.

[cpu_(ooo_6.6-7.5), 2013-05-02 19:55:58]

check the owners file, I think it has * for grd files.

[jwd, 2013-05-02 19:58:35]

Except for the question in my comment, trial code LGTM.

https://codereview.chromium.org/14752005/diff/54001/chrome/browser/ssl/ssl_bl...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/54001/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:186: trialCondition_ =
base::FieldTrialList::FindFullName("InterstitialSSL");
I want to make sure you're aware that one the group is assigned, the client will
be tagging UMA uploads with that same group until chrome is restarted.

Is there any logging on pages that can't be clicked through that could pollute
data if UMA reports being in this trial?

[mattm, 2013-05-02 20:00:10]

I think the best way to do the file rename is to make another CL which only
copies the binary files to the new location, with no code changes.  Then this CL
can update to use the new location and delete them from the old location.

https://codereview.chromium.org/14752005/diff/54001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/54001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:117: static const
char kCond1MalwareControl[] = "Cond1MalwareControl";
probably should change these to match the same style as the above string
constants for consistency.

https://codereview.chromium.org/14752005/diff/54001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:999:
l10n_util::GetStringUTF16(IDS_SAFE_BROWSING_MALWARE_BACK_BUTTON));
should be IDS_SAFE_BROWSING_MALWARE_FEAR_HEADLINE?

[felt, 2013-05-02 22:15:20]

The files are now added in a separate CL: 14752005. I need someone to approve it
since I'm not a full committer. (I'll add the delete statements back to this CL
once the other one lands.)

Made all the other changes as requested.

https://codereview.chromium.org/14752005/diff/54001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/54001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:117: static const
char kCond1MalwareControl[] = "Cond1MalwareControl";
Made the first letter lowercase.

On 2013/05/02 20:00:11, mattm wrote:
> probably should change these to match the same style as the above string
> constants for consistency.

https://codereview.chromium.org/14752005/diff/54001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:999:
l10n_util::GetStringUTF16(IDS_SAFE_BROWSING_MALWARE_BACK_BUTTON));
On 2013/05/02 20:00:11, mattm wrote:
> should be IDS_SAFE_BROWSING_MALWARE_FEAR_HEADLINE?

Fixed! (And I'll make sure to double check all 21 test cases before committing,
to ensure none of the small changes I've made during the review have broken
anything.)

https://codereview.chromium.org/14752005/diff/54001/chrome/browser/ssl/ssl_bl...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/54001/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:186: trialCondition_ =
base::FieldTrialList::FindFullName("InterstitialSSL");
On 2013/05/02 19:58:35, Jesse Doherty wrote:
> I want to make sure you're aware that one the group is assigned, the client
will
> be tagging UMA uploads with that same group until chrome is restarted.
> 
> Is there any logging on pages that can't be clicked through that could pollute
> data if UMA reports being in this trial?

I designed the histogram to differentiate between the overridable and
non-overridable warnings, so it's not a problem -- we can tell which is which.
The if-statement was just there to avoid "wasting" sample size, but I guess it
would be confusing to people if they visited both, so I've removed it.

[felt, 2013-05-03 18:29:11]

jhawkins, mattm - Do you have any additional changes you'd like me to make?
(Sorry to bug you, I'm anxious because of the branch point on Monday.)

On 2013/05/02 22:15:20, felt wrote:
> The files are now added in a separate CL: 14752005. I need someone to approve
it
> since I'm not a full committer. (I'll add the delete statements back to this
CL
> once the other one lands.)
> 
> Made all the other changes as requested.
> 
>
https://codereview.chromium.org/14752005/diff/54001/chrome/browser/safe_brows...
> File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):
> 
>
https://codereview.chromium.org/14752005/diff/54001/chrome/browser/safe_brows...
> chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:117: static const
> char kCond1MalwareControl[] = "Cond1MalwareControl";
> Made the first letter lowercase.
> 
> On 2013/05/02 20:00:11, mattm wrote:
> > probably should change these to match the same style as the above string
> > constants for consistency.
> 
>
https://codereview.chromium.org/14752005/diff/54001/chrome/browser/safe_brows...
> chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:999:
> l10n_util::GetStringUTF16(IDS_SAFE_BROWSING_MALWARE_BACK_BUTTON));
> On 2013/05/02 20:00:11, mattm wrote:
> > should be IDS_SAFE_BROWSING_MALWARE_FEAR_HEADLINE?
> 
> Fixed! (And I'll make sure to double check all 21 test cases before
committing,
> to ensure none of the small changes I've made during the review have broken
> anything.)
> 
>
https://codereview.chromium.org/14752005/diff/54001/chrome/browser/ssl/ssl_bl...
> File chrome/browser/ssl/ssl_blocking_page.cc (right):
> 
>
https://codereview.chromium.org/14752005/diff/54001/chrome/browser/ssl/ssl_bl...
> chrome/browser/ssl/ssl_blocking_page.cc:186: trialCondition_ =
> base::FieldTrialList::FindFullName("InterstitialSSL");
> On 2013/05/02 19:58:35, Jesse Doherty wrote:
> > I want to make sure you're aware that one the group is assigned, the client
> will
> > be tagging UMA uploads with that same group until chrome is restarted.
> > 
> > Is there any logging on pages that can't be clicked through that could
pollute
> > data if UMA reports being in this trial?
> 
> I designed the histogram to differentiate between the overridable and
> non-overridable warnings, so it's not a problem -- we can tell which is which.
> The if-statement was just there to avoid "wasting" sample size, but I guess it
> would be confusing to people if they visited both, so I've removed it.

[James Hawkins, 2013-05-03 18:31:54]

https://codereview.chromium.org/14752005/diff/72001/chrome/browser/resources/...
File chrome/browser/resources/safe_browsing/malware_block_v2.js (right):

https://codereview.chromium.org/14752005/diff/72001/chrome/browser/resources/...
chrome/browser/resources/safe_browsing/malware_block_v2.js:41: function
setupInterstitialExperiment() {
nit: Document method.

https://codereview.chromium.org/14752005/diff/72001/chrome/browser/resources/...
chrome/browser/resources/safe_browsing/malware_block_v2.js:47:
$('logo').style.display = 'none';
Why display none instead of hidden?

https://codereview.chromium.org/14752005/diff/72001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/72001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:117: static const
char kCond1MalwareControl[] = "cond1MalwareControl";
Get rid of all these statics by wrapping this whole var block in an unnamed
namespace.

https://codereview.chromium.org/14752005/diff/72001/chrome/browser/ssl/ssl_bl...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/72001/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:156: static const char
kCondition15Control[] = "Condition15SSLControl";
Remove 'static' in unnamed namespace.

https://codereview.chromium.org/14752005/diff/72001/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:180: 
nit: Remove blank line.

https://codereview.chromium.org/14752005/diff/72001/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:181: trialCondition_ =
base::FieldTrialList::FindFullName("InterstitialSSL517");
Shouldn't this string be in a const var?

https://codereview.chromium.org/14752005/diff/72001/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:195:
RecordSSLBlockingPageTimeStats(false, cert_error_,
nit: Start of parameter rows must align on the same column.

[felt, 2013-05-03 18:43:14]

Thanks. Changes uploaded.

On 2013/05/03 18:31:54, James Hawkins wrote:
>
https://codereview.chromium.org/14752005/diff/72001/chrome/browser/resources/...
> File chrome/browser/resources/safe_browsing/malware_block_v2.js (right):
> 
>
https://codereview.chromium.org/14752005/diff/72001/chrome/browser/resources/...
> chrome/browser/resources/safe_browsing/malware_block_v2.js:41: function
> setupInterstitialExperiment() {
> nit: Document method.
> 
>
https://codereview.chromium.org/14752005/diff/72001/chrome/browser/resources/...
> chrome/browser/resources/safe_browsing/malware_block_v2.js:47:
> $('logo').style.display = 'none';
> Why display none instead of hidden?
> 
>
https://codereview.chromium.org/14752005/diff/72001/chrome/browser/safe_brows...
> File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):
> 
>
https://codereview.chromium.org/14752005/diff/72001/chrome/browser/safe_brows...
> chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:117: static const
> char kCond1MalwareControl[] = "cond1MalwareControl";
> Get rid of all these statics by wrapping this whole var block in an unnamed
> namespace.
> 
>
https://codereview.chromium.org/14752005/diff/72001/chrome/browser/ssl/ssl_bl...
> File chrome/browser/ssl/ssl_blocking_page.cc (right):
> 
>
https://codereview.chromium.org/14752005/diff/72001/chrome/browser/ssl/ssl_bl...
> chrome/browser/ssl/ssl_blocking_page.cc:156: static const char
> kCondition15Control[] = "Condition15SSLControl";
> Remove 'static' in unnamed namespace.
> 
>
https://codereview.chromium.org/14752005/diff/72001/chrome/browser/ssl/ssl_bl...
> chrome/browser/ssl/ssl_blocking_page.cc:180: 
> nit: Remove blank line.
> 
>
https://codereview.chromium.org/14752005/diff/72001/chrome/browser/ssl/ssl_bl...
> chrome/browser/ssl/ssl_blocking_page.cc:181: trialCondition_ =
> base::FieldTrialList::FindFullName("InterstitialSSL517");
> Shouldn't this string be in a const var?
> 
>
https://codereview.chromium.org/14752005/diff/72001/chrome/browser/ssl/ssl_bl...
> chrome/browser/ssl/ssl_blocking_page.cc:195:
> RecordSSLBlockingPageTimeStats(false, cert_error_,
> nit: Start of parameter rows must align on the same column.

[felt, 2013-05-03 18:43:25]

https://codereview.chromium.org/14752005/diff/72001/chrome/browser/resources/...
File chrome/browser/resources/safe_browsing/malware_block_v2.js (right):

https://codereview.chromium.org/14752005/diff/72001/chrome/browser/resources/...
chrome/browser/resources/safe_browsing/malware_block_v2.js:41: function
setupInterstitialExperiment() {
On 2013/05/03 18:31:55, James Hawkins wrote:
> nit: Document method.

Done.

https://codereview.chromium.org/14752005/diff/72001/chrome/browser/resources/...
chrome/browser/resources/safe_browsing/malware_block_v2.js:47:
$('logo').style.display = 'none';
On 2013/05/03 18:31:55, James Hawkins wrote:
> Why display none instead of hidden?

Hidden would leave extra empty whitespace, right? Display none looks nicer.

https://codereview.chromium.org/14752005/diff/72001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/72001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:117: static const
char kCond1MalwareControl[] = "cond1MalwareControl";
On 2013/05/03 18:31:55, James Hawkins wrote:
> Get rid of all these statics by wrapping this whole var block in an unnamed
> namespace.

Done.

https://codereview.chromium.org/14752005/diff/72001/chrome/browser/ssl/ssl_bl...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/72001/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:156: static const char
kCondition15Control[] = "Condition15SSLControl";
On 2013/05/03 18:31:55, James Hawkins wrote:
> Remove 'static' in unnamed namespace.

Done.

https://codereview.chromium.org/14752005/diff/72001/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:180: 
On 2013/05/03 18:31:55, James Hawkins wrote:
> nit: Remove blank line.

Done.

https://codereview.chromium.org/14752005/diff/72001/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:181: trialCondition_ =
base::FieldTrialList::FindFullName("InterstitialSSL517");
On 2013/05/03 18:31:55, James Hawkins wrote:
> Shouldn't this string be in a const var?

Done. Also did the same for safe browsing file.

https://codereview.chromium.org/14752005/diff/72001/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:195:
RecordSSLBlockingPageTimeStats(false, cert_error_,
On 2013/05/03 18:31:55, James Hawkins wrote:
> nit: Start of parameter rows must align on the same column.

Done.

[mattm, 2013-05-03 23:25:45]

https://codereview.chromium.org/14752005/diff/54001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/54001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:117: static const
char kCond1MalwareControl[] = "Cond1MalwareControl";
ah, actually I was referring to the char[] vs char*

On 2013/05/02 22:15:20, felt wrote:
> Made the first letter lowercase.
> 
> On 2013/05/02 20:00:11, mattm wrote:
> > probably should change these to match the same style as the above string
> > constants for consistency.
>

https://codereview.chromium.org/14752005/diff/84001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/84001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:405:
RecordUserAction(SHOW_ADVANCED);
Is this ever triggered?  RecordUserReactionTime is called at the start of this
method, so it seems like has_expanded_see_more_section_ would never be false by
the time we get here.  (You can check chrome://histograms to see what have been
recorded in the current session)

[felt, 2013-05-03 23:44:49]

https://codereview.chromium.org/14752005/diff/54001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/54001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:117: static const
char kCond1MalwareControl[] = "Cond1MalwareControl";
On 2013/05/03 23:25:45, mattm wrote:
> ah, actually I was referring to the char[] vs char*
> 
> On 2013/05/02 22:15:20, felt wrote:
> > Made the first letter lowercase.
> > 
> > On 2013/05/02 20:00:11, mattm wrote:
> > > probably should change these to match the same style as the above string
> > > constants for consistency.
> > 
> 

Done.

https://codereview.chromium.org/14752005/diff/84001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/84001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:405:
RecordUserAction(SHOW_ADVANCED);
You're right. I moved the call.

On 2013/05/03 23:25:45, mattm wrote:
> Is this ever triggered?  RecordUserReactionTime is called at the start of this
> method, so it seems like has_expanded_see_more_section_ would never be false
by
> the time we get here.  (You can check chrome://histograms to see what have
been
> recorded in the current session)

[mattm, 2013-05-03 23:56:02]

https://codereview.chromium.org/14752005/diff/88001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/88001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:661: 
need it here too?

[felt, 2013-05-03 23:59:49]

https://codereview.chromium.org/14752005/diff/88001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/88001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:661: 
Yup, sorry.

On 2013/05/03 23:56:02, mattm wrote:
> need it here too?

[mattm, 2013-05-04 00:05:04]

safebrowsing lgtm

[abarth-chromium, 2013-05-06 18:02:36]

Looks fine to me.  I should probably be removed from this OWNERS file, however.

[James Hawkins, 2013-05-06 18:13:57]

https://codereview.chromium.org/14752005/diff/90001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/90001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:119: static
base::LazyInstance<SafeBrowsingBlockingPage::UnsafeResourceMap>
nit: All of these non-class statics should be added to the unnamed namespace
(minus the static).

https://codereview.chromium.org/14752005/diff/90001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:142: } // namespace
nit: Two spaces before start of comment.

https://codereview.chromium.org/14752005/diff/90001/chrome/browser/ssl/ssl_bl...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/90001/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:68: CMD_SHOW_UNDERSTAND,   // Used by
the Finch trial.
nit: Only two spaces before comment.

https://codereview.chromium.org/14752005/diff/90001/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:156: static const char kStudyName[] =
"InterstitialSSL517";
nit: Remove static in unnamed namespace.

[felt, 2013-05-06 19:47:52]

jhawkins - I fixed the spacing and moved the other variables into the new
namespace.

https://codereview.chromium.org/14752005/diff/90001/chrome/browser/safe_brows...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/90001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:119: static
base::LazyInstance<SafeBrowsingBlockingPage::UnsafeResourceMap>
On 2013/05/06 18:13:57, James Hawkins wrote:
> nit: All of these non-class statics should be added to the unnamed namespace
> (minus the static).

Done.

https://codereview.chromium.org/14752005/diff/90001/chrome/browser/safe_brows...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:142: } // namespace
On 2013/05/06 18:13:57, James Hawkins wrote:
> nit: Two spaces before start of comment.

Done.

https://codereview.chromium.org/14752005/diff/90001/chrome/browser/ssl/ssl_bl...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/90001/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:68: CMD_SHOW_UNDERSTAND,   // Used by
the Finch trial.
On 2013/05/06 18:13:57, James Hawkins wrote:
> nit: Only two spaces before comment.

Done.

https://codereview.chromium.org/14752005/diff/90001/chrome/browser/ssl/ssl_bl...
chrome/browser/ssl/ssl_blocking_page.cc:156: static const char kStudyName[] =
"InterstitialSSL517";
On 2013/05/06 18:13:57, James Hawkins wrote:
> nit: Remove static in unnamed namespace.

Done.

[James Hawkins, 2013-05-06 19:51:55]

lgtm

[commit-bot: I haz the power, 2013-05-06 21:59:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/felt@chromium.org/14752005/105001

[felt, 2013-05-07 06:45:37]

Committed patchset #18 manually as r198656 (presubmit successful).

[satorux1, 2013-05-07 07:23:17]

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.h (right):

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.h:77: std::string trialCondition_;
drive-by: trial_condition_;

[Dan Beam, 2013-05-07 08:07:15]

I have no idea how all this made it through review... I stopped after the first
copy of your resources

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
File chrome/browser/resources/safe_browsing/malware_block_v2.js (right):

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/safe_browsing/malware_block_v2.js:48: return;  // This
is the most common case.
make this 1 line, don't use } else { after a return in a conditional, i.e.

  if (something_bad)
    return;

  if (something_else)
    ...

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/safe_browsing/malware_block_v2.js:51:
$('logo').style.display = 'none';
why are you using .display = 'none'; here vs. hidden?

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
File chrome/browser/resources/ssl/fancy_firefox.html (right):

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:9: <style>
move to external file

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:11: background-color: #444444;
#444

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:12: color: #555555;
#555

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:17: background-color: #ffffff;
white

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:22: padding: 50px 0px 30px 0px;
50px 0 30px

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:26: color: #ffffff;
white

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:44: margin: 0 80px 0 30px;
RTL

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:47: margin: 1em 0 0 95px;
RTL

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:49: .open {
why is there a) a class named `open`, b) it actually hiding something?

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:53: color: #ffffff;
white

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:54: background-color: #4285f4;
use rgb()

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:58: font-weight: bold;
alpha sort rules

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:59: -webkit-border-radius: 3px;
drop -webkit prefix, not needed, also put -webkit rules on top

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:62: margin: 1em 0 0 95px;
RTL

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:71: margin: 0 0 0 95px;
RTL

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:75: float:left;
nit: missing space, RTL

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:83: }
\n between all of these

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:85: color: #000000;
s/#000000/black/

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:90: <script>
move to external file

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:92: var CMD_DONT_PROCEED = 0;
nit:

/** @const */ var CMD = {
  DONT_PROCEED: 0,
  ...
};

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:104: return
document.getElementById(o);
why are you duplicating this from util.js?

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:117:
$('more-info-twisty-open').style.display = 'none';
hidden

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:133:
$('understand-twisty-open').style.display = 'none';
use hidden

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:147: if (gainFocus == false) {
if (!gainFocus) instead

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:169: toggleMoreInfo();
$('more-info-title').addEventListener('click', toggleMoreInfo); (just put the
name of the function instead of wrapping it with an anonymous function)

everywhere

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:192:
document.addEventListener('contextmenu', function(e) {
I think there is already some shared functionality to disable context menus in
src/ui/webui/resources/

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:214: You have asked Chrome to
connect securely to <b><span
indent +2 \s on creation of text nodes

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:227: <p>If you usually connect
to this site without problems, this error could
wrap consistently, this doesn't match L214-221

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/fancy_firefox.html:244: <p
i18n-values=".innerHTML:moreInfo5"></p>
do all these need to be innerHTML? instead of i18n-content (which uses
innerText)

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
File chrome/browser/resources/ssl/firefox.html (right):

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/firefox.html:11: background-color: #dddad5;
rgb()

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/firefox.html:12: color: #000000;
black

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/firefox.html:17: background-color: #ffffff;
white

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/resources...
chrome/browser/resources/ssl/firefox.html:22: padding: 50px 0px 30px 0px;
0px -> 0 everywhere

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:59: const char*
const kSbDiagnosticUrl =
why aren't you using

  const char kSbDiagnosticUrl[] =

instead?

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:934: if
(unsafe_resources_[0].threat_type == SB_THREAT_TYPE_URL_MALWARE) {
indent off, can you be sure that unsafe_resources_ is not empty?  can you add a
DCHECK()?

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:1001: // Field trial
end with .

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:1026:
strings->SetString("headLine",
nit: arguably:

  std::string headline;
  if () {
    headline = ...;
  }
  strings->SetString("headLine", headline);

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.h (right):

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_blocking_page.h:267: std::string
trialCondition_;
trial_condition_ here as well

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:212: int resource_id =
IDR_SSL_ROAD_BLOCK_HTML;
why did you move this?

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:255: font_family =
l10n_util::GetStringUTF8(IDS_WEB_FONT_FAMILY_XP);
no curlies

https://codereview.chromium.org/14752005/diff/105001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:257: #endif
you should probably be defining |resource_id| right here as it's *way* above
where it's used at the moment

[felt, 2013-05-07 08:12:26]

dbeam: The new HTML/JS files are solely there for a Finch experiment. The files
will be deleted at the end of the experiment. Is it worth reverting to fix style
changes for code that's going to be deleted? (Also, most of the code is copied
from the existing warning HTML/JS.)

On 2013/05/07 08:07:15, Dan Beam wrote:
> I have no idea how all this made it through review... I stopped after the
first
> copy of your resources

[James Hawkins, 2013-05-07 16:50:59]

On 2013/05/07 08:12:26, felt wrote:
> dbeam: The new HTML/JS files are solely there for a Finch experiment. The
files
> will be deleted at the end of the experiment. Is it worth reverting to fix
style
> changes for code that's going to be deleted? (Also, most of the code is copied
> from the existing warning HTML/JS.)
> 
> On 2013/05/07 08:07:15, Dan Beam wrote:
> > I have no idea how all this made it through review... I stopped after the
> first
> > copy of your resources

dbeam: Quoted from my response above:

"Note that I'm not going over the experimental .html files very closely; once we
pick a winner we need to review them properly."

[Dan Beam, 2013-05-07 22:04:44]

jhawkins@: felt@ and I settled this offline by filing a bug to track and setting
up a calendar date to delete the extra code so this doesn't slip through the
cracks. if I had a nickel for every time somebody said "I'll add tests soon",
"it's not staying like this", etc. the bar tab would be on me everytime ;)

[Dan Beam, 2013-06-03 18:49:14]

felt@: have you gathered enough data now?

[felt, 2013-06-03 18:50:38]

On 2013/06/03 18:49:14, Dan Beam wrote:
> felt@: have you gathered enough data now?

dbeam: It needs to go to stable before it can be disabled. I was hoping it would
make it in to M28 but it didn't make the cutoff, so it'll have to go to M29.