Add a policy to disable proceeding through the Safe Browsing interstitials.

Add a policy to disable proceeding through the Safe Browsing interstitials.

BUG=119184
TEST=browser_tests:SafeBrowsingBlockingPageTest.*, Safe Browsing warning pages don't have a "proceed" link when opening malware sites (e.g. ianfette.org) and the policy is set


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=145523

[Joao da Silva, 2012-06-29 17:20:27]

Please have a look, thanks!

@mnissler: policy/ stuff
@mirandac: profiles/ stuff
@jhawkins: resources/ stuff
@shess: safe_browsing/ stuff

[James Hawkins, 2012-06-29 17:31:51]

http://codereview.chromium.org/10694037/diff/1/chrome/browser/resources/safe_...
File chrome/browser/resources/safe_browsing_malware_block.html (right):

http://codereview.chromium.org/10694037/diff/1/chrome/browser/resources/safe_...
chrome/browser/resources/safe_browsing_malware_block.html:163: <div class="main"
i18n-values=".innerHTML:description3" jsdisplay="!proceed_disabled"></div>
80 cols.

http://codereview.chromium.org/10694037/diff/1/chrome/browser/resources/safe_...
File chrome/browser/resources/safe_browsing_multiple_threat_block.html (right):

http://codereview.chromium.org/10694037/diff/1/chrome/browser/resources/safe_...
chrome/browser/resources/safe_browsing_multiple_threat_block.html:134: <input
name="checky" id="checky" type="checkbox"
onclick="agreed(this.form)">&nbsp;<label for="checky"
i18n-content="confirm_text" jsdisplay="!proceed_disabled"></label>
80 cols.

http://codereview.chromium.org/10694037/diff/1/chrome/browser/resources/safe_...
chrome/browser/resources/safe_browsing_multiple_threat_block.html:135: <input
type="button" name="continue_button" i18n-values="value:continue_button"
disabled="true" jsdisplay="!proceed_disabled" onclick="proceed();"><br>
80 cols.

http://codereview.chromium.org/10694037/diff/1/chrome/browser/resources/safe_...
chrome/browser/resources/safe_browsing_multiple_threat_block.html:135: <input
type="button" name="continue_button" i18n-values="value:continue_button"
disabled="true" jsdisplay="!proceed_disabled" onclick="proceed();"><br>
Use <button> instead of type=button

http://codereview.chromium.org/10694037/diff/1/chrome/browser/resources/safe_...
File chrome/browser/resources/safe_browsing_phishing_block.html (right):

http://codereview.chromium.org/10694037/diff/1/chrome/browser/resources/safe_...
chrome/browser/resources/safe_browsing_phishing_block.html:131: <div
class="main" i18n-values=".innerHTML:description3"
jsdisplay="!proceed_disabled"></div>
80 cols.

http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:234:
strings->SetBoolean("proceed_disabled",
s/proceed_disabled/proceedDisabled/

http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:470: // We are
deleted after this.
nit: Don't use pronouns in comments (we).  Pronouns are ambiguous.

http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.h (right):

http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
chrome/browser/safe_browsing/safe_browsing_blocking_page.h:143: // Helpers that
returns the boolean value of the given |pref| from the
s/Helpers that returns/Returns/

http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right):

http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:383:
ASCIIToUTF16(""),
string16().

http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:433:
ASSERT_TRUE(GetProceedLinkIsHidden(&hidden));
s/ASSERT/EXPECT/

http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:593:
IN_PROC_BROWSER_TEST_F(SafeBrowsingBlockingPageTest, ProceedDisabled) {
nit: Document what this test is testing.

[Miranda Callahan, 2012-06-29 17:37:47]

profiles/* LGTM.

On 2012/06/29 17:31:51, James Hawkins wrote:
>
http://codereview.chromium.org/10694037/diff/1/chrome/browser/resources/safe_...
> File chrome/browser/resources/safe_browsing_malware_block.html (right):
> 
>
http://codereview.chromium.org/10694037/diff/1/chrome/browser/resources/safe_...
> chrome/browser/resources/safe_browsing_malware_block.html:163: <div
class="main"
> i18n-values=".innerHTML:description3" jsdisplay="!proceed_disabled"></div>
> 80 cols.
> 
>
http://codereview.chromium.org/10694037/diff/1/chrome/browser/resources/safe_...
> File chrome/browser/resources/safe_browsing_multiple_threat_block.html
(right):
> 
>
http://codereview.chromium.org/10694037/diff/1/chrome/browser/resources/safe_...
> chrome/browser/resources/safe_browsing_multiple_threat_block.html:134: <input
> name="checky" id="checky" type="checkbox"
> onclick="agreed(this.form)">&nbsp;<label for="checky"
> i18n-content="confirm_text" jsdisplay="!proceed_disabled"></label>
> 80 cols.
> 
>
http://codereview.chromium.org/10694037/diff/1/chrome/browser/resources/safe_...
> chrome/browser/resources/safe_browsing_multiple_threat_block.html:135: <input
> type="button" name="continue_button" i18n-values="value:continue_button"
> disabled="true" jsdisplay="!proceed_disabled" onclick="proceed();"><br>
> 80 cols.
> 
>
http://codereview.chromium.org/10694037/diff/1/chrome/browser/resources/safe_...
> chrome/browser/resources/safe_browsing_multiple_threat_block.html:135: <input
> type="button" name="continue_button" i18n-values="value:continue_button"
> disabled="true" jsdisplay="!proceed_disabled" onclick="proceed();"><br>
> Use <button> instead of type=button
> 
>
http://codereview.chromium.org/10694037/diff/1/chrome/browser/resources/safe_...
> File chrome/browser/resources/safe_browsing_phishing_block.html (right):
> 
>
http://codereview.chromium.org/10694037/diff/1/chrome/browser/resources/safe_...
> chrome/browser/resources/safe_browsing_phishing_block.html:131: <div
> class="main" i18n-values=".innerHTML:description3"
> jsdisplay="!proceed_disabled"></div>
> 80 cols.
> 
>
http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
> File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):
> 
>
http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
> chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:234:
> strings->SetBoolean("proceed_disabled",
> s/proceed_disabled/proceedDisabled/
> 
>
http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
> chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:470: // We are
> deleted after this.
> nit: Don't use pronouns in comments (we).  Pronouns are ambiguous.
> 
>
http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
> File chrome/browser/safe_browsing/safe_browsing_blocking_page.h (right):
> 
>
http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
> chrome/browser/safe_browsing/safe_browsing_blocking_page.h:143: // Helpers
that
> returns the boolean value of the given |pref| from the
> s/Helpers that returns/Returns/
> 
>
http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
> File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right):
> 
>
http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
> chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:383:
> ASCIIToUTF16(""),
> string16().
> 
>
http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
> chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:433:
> ASSERT_TRUE(GetProceedLinkIsHidden(&hidden));
> s/ASSERT/EXPECT/
> 
>
http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
> chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:593:
> IN_PROC_BROWSER_TEST_F(SafeBrowsingBlockingPageTest, ProceedDisabled) {
> nit: Document what this test is testing.

[Mattias Nissler (ping if slow), 2012-07-02 09:51:56]

LGTM w/ nits for chrome/browser/policy. It seems like writing a PyAuto for this
would be relatively easy, care to add one?

http://codereview.chromium.org/10694037/diff/1/chrome/app/policy/policy_templ...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/10694037/diff/1/chrome/app/policy/policy_templ...
chrome/app/policy/policy_templates.json:115: #   For your editing convenience:
highest ID currently used: 150
I think we're past that already. Just saw 151 fly by. Are you in sync?

http://codereview.chromium.org/10694037/diff/1/chrome/app/policy/policy_templ...
chrome/app/policy/policy_templates.json:2751: The Safe Browsing service shows a
warning page when users navigate to sites that are flagged as potentially
mallicious. Enabling this setting prevents users from proceeding anyway from the
warning page to the flagged site.
malicious

[Joao da Silva, 2012-07-02 10:08:40]

Thanks for the reviews!

@James: PTAL. Please have a look at the comment about the longer-than-80-cols
line.

@Scott: ping

http://codereview.chromium.org/10694037/diff/1/chrome/app/policy/policy_templ...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/10694037/diff/1/chrome/app/policy/policy_templ...
chrome/app/policy/policy_templates.json:115: #   For your editing convenience:
highest ID currently used: 150
On 2012/07/02 09:51:56, Mattias Nissler wrote:
> I think we're past that already. Just saw 151 fly by. Are you in sync?

Just rebased, this is still good. Notice that the cloud policy protobufs have
the ids off by 2.

http://codereview.chromium.org/10694037/diff/1/chrome/app/policy/policy_templ...
chrome/app/policy/policy_templates.json:2751: The Safe Browsing service shows a
warning page when users navigate to sites that are flagged as potentially
mallicious. Enabling this setting prevents users from proceeding anyway from the
warning page to the flagged site.
On 2012/07/02 09:51:56, Mattias Nissler wrote:
> malicious

Done.

http://codereview.chromium.org/10694037/diff/1/chrome/browser/resources/safe_...
File chrome/browser/resources/safe_browsing_malware_block.html (right):

http://codereview.chromium.org/10694037/diff/1/chrome/browser/resources/safe_...
chrome/browser/resources/safe_browsing_malware_block.html:163: <div class="main"
i18n-values=".innerHTML:description3" jsdisplay="!proceed_disabled"></div>
On 2012/06/29 17:31:51, James Hawkins wrote:
> 80 cols.

Done.

http://codereview.chromium.org/10694037/diff/1/chrome/browser/resources/safe_...
File chrome/browser/resources/safe_browsing_multiple_threat_block.html (right):

http://codereview.chromium.org/10694037/diff/1/chrome/browser/resources/safe_...
chrome/browser/resources/safe_browsing_multiple_threat_block.html:134: <input
name="checky" id="checky" type="checkbox"
onclick="agreed(this.form)">&nbsp;<label for="checky"
i18n-content="confirm_text" jsdisplay="!proceed_disabled"></label>
On 2012/06/29 17:31:51, James Hawkins wrote:
> 80 cols.

Done.

http://codereview.chromium.org/10694037/diff/1/chrome/browser/resources/safe_...
chrome/browser/resources/safe_browsing_multiple_threat_block.html:135: <input
type="button" name="continue_button" i18n-values="value:continue_button"
disabled="true" jsdisplay="!proceed_disabled" onclick="proceed();"><br>
On 2012/06/29 17:31:51, James Hawkins wrote:
> 80 cols.

Done.

http://codereview.chromium.org/10694037/diff/1/chrome/browser/resources/safe_...
chrome/browser/resources/safe_browsing_multiple_threat_block.html:135: <input
type="button" name="continue_button" i18n-values="value:continue_button"
disabled="true" jsdisplay="!proceed_disabled" onclick="proceed();"><br>
On 2012/06/29 17:31:51, James Hawkins wrote:
> Use <button> instead of type=button

Done.

http://codereview.chromium.org/10694037/diff/1/chrome/browser/resources/safe_...
File chrome/browser/resources/safe_browsing_phishing_block.html (right):

http://codereview.chromium.org/10694037/diff/1/chrome/browser/resources/safe_...
chrome/browser/resources/safe_browsing_phishing_block.html:131: <div
class="main" i18n-values=".innerHTML:description3"
jsdisplay="!proceed_disabled"></div>
On 2012/06/29 17:31:51, James Hawkins wrote:
> 80 cols.

Done.

http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:234:
strings->SetBoolean("proceed_disabled",
On 2012/06/29 17:31:51, James Hawkins wrote:
> s/proceed_disabled/proceedDisabled/

Done.

http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:470: // We are
deleted after this.
On 2012/06/29 17:31:51, James Hawkins wrote:
> nit: Don't use pronouns in comments (we).  Pronouns are ambiguous.

Done.

http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.h (right):

http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
chrome/browser/safe_browsing/safe_browsing_blocking_page.h:143: // Helpers that
returns the boolean value of the given |pref| from the
On 2012/06/29 17:31:51, James Hawkins wrote:
> s/Helpers that returns/Returns/

Done.

http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right):

http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:383:
ASCIIToUTF16(""),
On 2012/06/29 17:31:51, James Hawkins wrote:
> string16().

Done.

http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:433:
ASSERT_TRUE(GetProceedLinkIsHidden(&hidden));
On 2012/06/29 17:31:51, James Hawkins wrote:
> s/ASSERT/EXPECT/

Done.

http://codereview.chromium.org/10694037/diff/1/chrome/browser/safe_browsing/s...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:593:
IN_PROC_BROWSER_TEST_F(SafeBrowsingBlockingPageTest, ProceedDisabled) {
On 2012/06/29 17:31:51, James Hawkins wrote:
> nit: Document what this test is testing.

Done.

http://codereview.chromium.org/10694037/diff/9002/chrome/browser/resources/sa...
File chrome/browser/resources/safe_browsing_multiple_threat_block.html (right):

http://codereview.chromium.org/10694037/diff/9002/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing_multiple_threat_block.html:129:
jsvalues="chromiumID:$counter.value;chromiumIsMalware:type=='malware'"
Any hint on how to break this one into 80 cols? Should the indentation be
shifted back instead?

[Scott Hess - ex-Googler, 2012-07-02 14:13:10]

Swapping in Matt.  I'm on jury duty right now, so my reviewing time is pretty
limited.  Ping me back in if it turns out to be uniquely targetting specific
shess stuff.

[James Hawkins, 2012-07-02 15:56:06]

http://codereview.chromium.org/10694037/diff/1018/chrome/browser/resources/sa...
File chrome/browser/resources/safe_browsing_malware_block.html (right):

http://codereview.chromium.org/10694037/diff/1018/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing_malware_block.html:146: <div
class="background"><img src="ssl_roadblock_background.png" width="100%"
<img> needs to be on a new line.

http://codereview.chromium.org/10694037/diff/1018/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing_malware_block.html:147: height="100%"
alt="background" onmousedown="return false;"/></div>
nit: </div> must be on a new line since the <div> line is wrapped.

Here and pretty much all of the lines changed in this file in this CL.

http://codereview.chromium.org/10694037/diff/1018/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing_malware_block.html:147: height="100%"
alt="background" onmousedown="return false;"/></div>
Why do you need to specify the onmousedown here?

http://codereview.chromium.org/10694037/diff/1018/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing_malware_block.html:156: <div class="main"
i18n-values=".innerHTML:description5" id="detailinfo"
nit: ID must be the first attribute specified.

http://codereview.chromium.org/10694037/diff/1018/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing_malware_block.html:157:
style="display:block"></div>
<div> is already display: block.

http://codereview.chromium.org/10694037/diff/1018/chrome/browser/resources/sa...
File chrome/browser/resources/safe_browsing_multiple_threat_block.html (right):

http://codereview.chromium.org/10694037/diff/1018/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing_multiple_threat_block.html:102: function
showOrReport() {
nit: Document method.

http://codereview.chromium.org/10694037/diff/1018/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing_multiple_threat_block.html:107: return
false;
Why are you returning any value?

http://codereview.chromium.org/10694037/diff/1018/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing_multiple_threat_block.html:113: <div
class="background"><img src="ssl_roadblock_background.png" width="100%"
Newlines for <img> and </div>.

[mattm, 2012-07-02 21:47:00]

http://codereview.chromium.org/10694037/diff/1018/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

http://codereview.chromium.org/10694037/diff/1018/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:604:
RecordUserAction(DONT_PROCEED);
Probably want to split this into two user metrics, so that the proceed:don't
proceed choice ratio isn't polluted by results where the user didn't have a
choice.

(Also need to update AddComputedActions in chrome/tools/extract_actions.py)

[Joao da Silva, 2012-07-03 13:39:52]

@Matt: PTAL

@James: PTYAL

http://codereview.chromium.org/10694037/diff/1018/chrome/browser/resources/sa...
File chrome/browser/resources/safe_browsing_malware_block.html (right):

http://codereview.chromium.org/10694037/diff/1018/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing_malware_block.html:146: <div
class="background"><img src="ssl_roadblock_background.png" width="100%"
On 2012/07/02 15:56:06, James Hawkins wrote:
> <img> needs to be on a new line.

Done.

http://codereview.chromium.org/10694037/diff/1018/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing_malware_block.html:147: height="100%"
alt="background" onmousedown="return false;"/></div>
On 2012/07/02 15:56:06, James Hawkins wrote:
> nit: </div> must be on a new line since the <div> line is wrapped.
> 
> Here and pretty much all of the lines changed in this file in this CL.

Done.

http://codereview.chromium.org/10694037/diff/1018/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing_malware_block.html:147: height="100%"
alt="background" onmousedown="return false;"/></div>
On 2012/07/02 15:56:06, James Hawkins wrote:
> Why do you need to specify the onmousedown here?

I honestly don't know why this is here. Please notice that this isn't my code
:-) I'm happy to clean it up, but I don't know why much of it is the way it is.
Someone probably fixed some bug when this was introduced.

http://codereview.chromium.org/10694037/diff/1018/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing_malware_block.html:156: <div class="main"
i18n-values=".innerHTML:description5" id="detailinfo"
On 2012/07/02 15:56:06, James Hawkins wrote:
> nit: ID must be the first attribute specified.

Done.

http://codereview.chromium.org/10694037/diff/1018/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing_malware_block.html:157:
style="display:block"></div>
On 2012/07/02 15:56:06, James Hawkins wrote:
> <div> is already display: block.

Done.

http://codereview.chromium.org/10694037/diff/1018/chrome/browser/resources/sa...
File chrome/browser/resources/safe_browsing_multiple_threat_block.html (right):

http://codereview.chromium.org/10694037/diff/1018/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing_multiple_threat_block.html:102: function
showOrReport() {
On 2012/07/02 15:56:06, James Hawkins wrote:
> nit: Document method.

Done.

http://codereview.chromium.org/10694037/diff/1018/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing_multiple_threat_block.html:107: return
false;
On 2012/07/02 15:56:06, James Hawkins wrote:
> Why are you returning any value?

Again, this isn't my code. Returning false from an event handler prevents the
default behavior for that event, so this is used to prevent the link from
navigating to the current page, which would reload it.

http://codereview.chromium.org/10694037/diff/1018/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing_multiple_threat_block.html:113: <div
class="background"><img src="ssl_roadblock_background.png" width="100%"
On 2012/07/02 15:56:06, James Hawkins wrote:
> Newlines for <img> and </div>.

Done.

http://codereview.chromium.org/10694037/diff/1018/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

http://codereview.chromium.org/10694037/diff/1018/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:604:
RecordUserAction(DONT_PROCEED);
On 2012/07/02 21:47:00, mattm wrote:
> Probably want to split this into two user metrics, so that the proceed:don't
> proceed choice ratio isn't polluted by results where the user didn't have a
> choice.
> 
> (Also need to update AddComputedActions in chrome/tools/extract_actions.py)

Done. Please have a look at the changes in chrome/tools; I guess that's what you
meant, but extract_actions.py added a couple other entries that might have been
missing.

[James Hawkins, 2012-07-03 16:56:47]

LGTM with nit and question.  I'll clean up this page later (it's on my OKRs).

http://codereview.chromium.org/10694037/diff/8002/chrome/browser/resources/sa...
File chrome/browser/resources/safe_browsing_malware_block.html (right):

http://codereview.chromium.org/10694037/diff/8002/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing_malware_block.html:148: alt="background"
onmousedown="return false;"/>
nit: Sorry I didn't spot this before.  Don't close single tags, i.e. remove the
last forward slash.

Here and other <img> elements.

http://codereview.chromium.org/10694037/diff/8002/chrome/browser/resources/sa...
File chrome/browser/resources/safe_browsing_multiple_threat_block.html (right):

http://codereview.chromium.org/10694037/diff/8002/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing_multiple_threat_block.html:2: <html
id="template_root">
What is this change about?

[mattm, 2012-07-03 19:41:34]

lgtm

http://codereview.chromium.org/10694037/diff/1018/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

http://codereview.chromium.org/10694037/diff/1018/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:604:
RecordUserAction(DONT_PROCEED);
On 2012/07/03 13:39:52, Joao da Silva wrote:
> On 2012/07/02 21:47:00, mattm wrote:
> > Probably want to split this into two user metrics, so that the proceed:don't
> > proceed choice ratio isn't polluted by results where the user didn't have a
> > choice.
> > 
> > (Also need to update AddComputedActions in chrome/tools/extract_actions.py)
> 
> Done. Please have a look at the changes in chrome/tools; I guess that's what
you
> meant, but extract_actions.py added a couple other entries that might have
been
> missing.

Yep, I think that just means someone added some metrics without running the
script.

[mattm, 2012-07-03 19:42:03]

(btw, when you are uploading changes after rebasing/merging in new changes from
the trunk, it is helpful if you upload it in two parts, one containing just the
rebase, and the other containing the new changes to the CL.)

[Joao da Silva, 2012-07-05 09:49:52]

Thanks all for reviewing!

http://codereview.chromium.org/10694037/diff/8002/chrome/browser/resources/sa...
File chrome/browser/resources/safe_browsing_malware_block.html (right):

http://codereview.chromium.org/10694037/diff/8002/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing_malware_block.html:148: alt="background"
onmousedown="return false;"/>
On 2012/07/03 16:56:47, James Hawkins wrote:
> nit: Sorry I didn't spot this before.  Don't close single tags, i.e. remove
the
> last forward slash.
> 
> Here and other <img> elements.

Done.

http://codereview.chromium.org/10694037/diff/8002/chrome/browser/resources/sa...
File chrome/browser/resources/safe_browsing_multiple_threat_block.html (right):

http://codereview.chromium.org/10694037/diff/8002/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing_multiple_threat_block.html:2: <html
id="template_root">
On 2012/07/03 16:56:47, James Hawkins wrote:
> What is this change about?

safe_browsing_blocking_page.cc applies the string dictionary build there to the
"template_root" element:

jstemplate_builder::GetTemplatesHtml(html, &strings, "template_root")

This used to be a <div> somewhere within the <table> in the <body>, but I've
added the "jsdisplay" attribute in another <div> that wasn't a subelement of the
"template_root". So the "template_root" had to be changed to a common parent
element, and all the other files use the toplevel <html> element, therefore this
change.

[commit-bot: I haz the power, 2012-07-05 09:50:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/10694037/9007

[commit-bot: I haz the power, 2012-07-05 11:16:41]

Change committed as 145523

[cpu_(ooo_6.6-7.5), 2012-07-06 23:59:22]

I think this change broke a mac test (ProceedDisabled)

http://build.chromium.org/p/chromium/builders/Mac%2010.6%20Tests%20%28dbg%29%...

http://build.chromium.org/p/chromium/builders/Mac%2010.6%20Tests%20%28dbg%29%...