Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10855260:
Safe Browsing malware interstitial redesign field trial. (Closed)
|
Created:
8 years, 4 months ago by mattm Modified:
8 years, 4 months ago Reviewers:
Dan Beam, csilv, Nico, arv (Not doing code reviews), Alexei Svitkine (slow), kewang, Ilya Sherman, noé CC:
chromium-reviews, Ilya Sherman, MAD, arv (Not doing code reviews), pam+watch_chromium.org, jar (doing other things) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSafe Browsing malware interstitial redesign field trial.
BUG=143999
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152941
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 12
Patch Set 3 : review changes #Patch Set 4 : changes for review comment #8 #
Total comments: 28
Patch Set 5 : webui style fixes #
Total comments: 82
Patch Set 6 : changes for review comments #16 and #18 #Patch Set 7 : rebase #Patch Set 8 : fix compile warning #
Total comments: 1
Messages
Total messages: 26 (0 generated)
arv: chrome/browser/resources/ noelutz,kewang: chrome/browser/safe_browsing/ thakis: chrome/browser,chrome/tools
http://codereview.chromium.org/10855260/diff/14/chrome/tools/extract_actions.py File chrome/tools/extract_actions.py (right): http://codereview.chromium.org/10855260/diff/14/chrome/tools/extract_actions.... chrome/tools/extract_actions.py:137: actions.add('SBInterstitial%s%s%s' % (interstitial, action, group)) Note that the variations dashboard is not going to show this data, so you'll only see this in the "counts" dashboard. If you're interested in comparing these access points using the new dashboard, you'll need to make sure there are histograms tracking the same data...
anywhere to see some screenshots of the new interstitial page? Thanks! On Tue, Aug 21, 2012 at 11:31 AM, <mattm@chromium.org> wrote: > Reviewers: arv, noelutz1, kewang, Nico, > > Message: > arv: chrome/browser/resources/ > noelutz,kewang: chrome/browser/safe_browsing/ > thakis: chrome/browser,chrome/tools > > > Description: > Safe Browsing malware interstitial redesign field trial. > > BUG=none > > > Please review this at http://codereview.chromium.org/10855260/ > > SVN Base: svn://svn.chromium.org/chrome/trunk/src > > Affected files: > M chrome/app/generated_resources.grd > M chrome/browser/browser_resources.grd > M chrome/browser/chrome_browser_field_trials.h > M chrome/browser/chrome_browser_field_trials.cc > A chrome/browser/resources/safe_browsing_malware_block_v2.html > M chrome/browser/safe_browsing/safe_browsing_blocking_page.h > M chrome/browser/safe_browsing/safe_browsing_blocking_page.cc > M chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc > M chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc > M chrome/tools/chromeactions.txt > M chrome/tools/extract_actions.py > > -- Ke
> anywhere to see some screenshots of the new interstitial page? Thanks! www/~mattm/no_crawl/chrome/sb_interstitial http://codereview.chromium.org/10855260/diff/14/chrome/tools/extract_actions.py File chrome/tools/extract_actions.py (right): http://codereview.chromium.org/10855260/diff/14/chrome/tools/extract_actions.... chrome/tools/extract_actions.py:137: actions.add('SBInterstitial%s%s%s' % (interstitial, action, group)) On 2012/08/21 03:46:53, Ilya Sherman wrote: > Note that the variations dashboard is not going to show this data, so you'll > only see this in the "counts" dashboard. If you're interested in comparing > these access points using the new dashboard, you'll need to make sure there are > histograms tracking the same data... Yeah, I added histograms for these counts too.
looks great. Just some nits: 1. No unittests for V2 pages? 2. Please test phishing warning, walware+phishing warning locally too. http://codereview.chromium.org/10855260/diff/14/chrome/app/generated_resource... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/10855260/diff/14/chrome/app/generated_resource... chrome/app/generated_resources.grd:10735: + If you <ph name="BEGIN_LINK2"><a href="" onclick="sendCommand('proceed'); return false;" onmousedown="return false;"></ph>proceed against Google's recommendation<ph name="END_LINK2"></a></ph>, you'll most likely become a victim of malware even if you have anti-virus software. Please check again tomorrow. 'check again tomorrow'. I assume the UI is reviewed and approved. :) http://codereview.chromium.org/10855260/diff/14/chrome/browser/safe_browsing/... File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right): http://codereview.chromium.org/10855260/diff/14/chrome/browser/safe_browsing/... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:79: "https://www.google.com/goodtoknow/online-safety/phishing/"; when will this be shown? http://codereview.chromium.org/10855260/diff/14/chrome/browser/safe_browsing/... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:133: // Only do the trial if the interstitial is a malware one, the other types Basically we do this trial if there is only one unsafe resource and it's malware. If we have multiple malware ones, we still in V1. Maybe can be more specific in comment here. http://codereview.chromium.org/10855260/diff/14/chrome/browser/safe_browsing/... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:273: threat_type == SafeBrowsingService::CLIENT_SIDE_PHISHING_URL) { Wil this part ever be reached? I think the v2 only works for malware warning?
asvitkine, do the changes in chrome/browser/chrome_browser_field_trials.cc look good to you?
> 1. No unittests for V2 pages? done. > 2. Please test phishing warning, walware+phishing warning locally too. I've tested that the v1 phishing warning still comes up in either case. I haven't figured out how to get a multiple-resource warning to appear (with or without my change..) http://codereview.chromium.org/10855260/diff/14/chrome/app/generated_resource... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/10855260/diff/14/chrome/app/generated_resource... chrome/app/generated_resources.grd:10735: + If you <ph name="BEGIN_LINK2"><a href="" onclick="sendCommand('proceed'); return false;" onmousedown="return false;"></ph>proceed against Google's recommendation<ph name="END_LINK2"></a></ph>, you'll most likely become a victim of malware even if you have anti-virus software. Please check again tomorrow. On 2012/08/21 10:23:05, kewang wrote: > 'check again tomorrow'. I assume the UI is reviewed and approved. :) Yeah, Yi has gotten this reviewed by the UI leads. http://codereview.chromium.org/10855260/diff/14/chrome/browser/safe_browsing/... File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right): http://codereview.chromium.org/10855260/diff/14/chrome/browser/safe_browsing/... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:79: "https://www.google.com/goodtoknow/online-safety/phishing/"; On 2012/08/21 10:23:05, kewang wrote: > when will this be shown? Ah, added that prematurely. Removed for now. http://codereview.chromium.org/10855260/diff/14/chrome/browser/safe_browsing/... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:133: // Only do the trial if the interstitial is a malware one, the other types On 2012/08/21 10:23:05, kewang wrote: > Basically we do this trial if there is only one unsafe resource and it's > malware. If we have multiple malware ones, we still in V1. Maybe can be more > specific in comment here. Done. http://codereview.chromium.org/10855260/diff/14/chrome/browser/safe_browsing/... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:273: threat_type == SafeBrowsingService::CLIENT_SIDE_PHISHING_URL) { On 2012/08/21 10:23:05, kewang wrote: > Wil this part ever be reached? I think the v2 only works for malware warning? removed.
http://codereview.chromium.org/10855260/diff/14/chrome/browser/chrome_browser... File chrome/browser/chrome_browser_field_trials.h (right): http://codereview.chromium.org/10855260/diff/14/chrome/browser/chrome_browser... chrome/browser/chrome_browser_field_trials.h:66: void SafeBrowsingInterstitialFieldTrial(); Nit: Can you rename this to SetUpSafeBrowsingInterstitialFieldTrial() and make the comment a proper sentence? I know that's not the convention currently used by the other ones, but I'd like to rename them in the future too so that they follow our style conventions.
chrome/browser/chrome_browser_field_trials.[cc|h] LGTM with the above nit.
http://codereview.chromium.org/10855260/diff/14/chrome/browser/chrome_browser... File chrome/browser/chrome_browser_field_trials.h (right): http://codereview.chromium.org/10855260/diff/14/chrome/browser/chrome_browser... chrome/browser/chrome_browser_field_trials.h:66: void SafeBrowsingInterstitialFieldTrial(); On 2012/08/21 18:37:38, Alexei Svitkine wrote: > Nit: Can you rename this to SetUpSafeBrowsingInterstitialFieldTrial() and make > the comment a proper sentence? I know that's not the convention currently used > by the other ones, but I'd like to rename them in the future too so that they > follow our style conventions. Done.
The files I'm supposed to look at lgtm
As an aside, shouldn't you have a crbug associated with this change, given it's non-trivial?
filling in for arv@ on yihan@google's request http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... File chrome/browser/resources/safe_browsing_malware_block_v2.html (right): http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:1: <html id="template_root" jsvalues="dir:textdirection"> where is the <!DOCTYPE>? http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:4: <style> this should be a separate CSS file http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:15: width:700px; none of this CSS adheres to the style guide, move to .css file and the presubmit will help you through this. http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:27: text-align:right; use text-align: start; in the non-RTL version and it'll work for both. http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:29: .right { you should use start/end and styles that work with RTL preferably http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:139: <script> this should be a separate .js file http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:155: if (document.getElementById('see_less_text').style.display == 'none') { this should use $() from src/chrome/browser/resources/shared/util.js and should be using .hidden instead of .style.display http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:168: <body oncontextmenu="return false;"> no inline handlers, make these in the JS instead. http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:173: <div><img src="../../app/theme/%DISTRIBUTION%/product_logo_name_108.png" class="logo"></div> all lines should be less than 80 columns http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:183: onclick="sendCommand('learnMore2'); return false;" 4 \s indent http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:195: <div style="clear: both;"></div> all styles should be in the CSS http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:206: <hr class="hr"> hr.hr makes little to no sense to me http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:208: <a class="right" href="" onclick="seeMore(); return false;" use preventDefault() instead of return false; and make href="#" and call preventDefaultForPoundLinkClicks() in src/chrome/browser/resources/shared/util.js
http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... File chrome/browser/resources/safe_browsing_malware_block_v2.html (right): http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:1: <html id="template_root" jsvalues="dir:textdirection"> This file does not comply with current webui style guidelines: http://dev.chromium.org/developers/web-development-style-guide There are too many style issues to mention, so please take a look at the guidelines so we can clean this up. I would like to ask that we split this into three files (.html, .css, .js) and put into a new subdirectory titled 'safe_browsing'.
thanks, updated http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... File chrome/browser/resources/safe_browsing_malware_block_v2.html (right): http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:1: <html id="template_root" jsvalues="dir:textdirection"> On 2012/08/21 21:54:15, Dan Beam wrote: > where is the <!DOCTYPE>? Done. http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:1: <html id="template_root" jsvalues="dir:textdirection"> On 2012/08/21 21:59:40, csilv wrote: > This file does not comply with current webui style guidelines: > http://dev.chromium.org/developers/web-development-style-guide > > There are too many style issues to mention, so please take a look at the > guidelines so we can clean this up. I would like to ask that we split this into > three files (.html, .css, .js) and put into a new subdirectory titled > 'safe_browsing'. Done. http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:4: <style> On 2012/08/21 21:54:15, Dan Beam wrote: > this should be a separate CSS file Done. http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:15: width:700px; On 2012/08/21 21:54:15, Dan Beam wrote: > none of this CSS adheres to the style guide, move to .css file and the presubmit > will help you through this. Done. http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:27: text-align:right; On 2012/08/21 21:54:15, Dan Beam wrote: > use text-align: start; in the non-RTL version and it'll work for both. Done. http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:29: .right { On 2012/08/21 21:54:15, Dan Beam wrote: > you should use start/end and styles that work with RTL preferably Done. http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:139: <script> On 2012/08/21 21:54:15, Dan Beam wrote: > this should be a separate .js file Done. http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:155: if (document.getElementById('see_less_text').style.display == 'none') { On 2012/08/21 21:54:15, Dan Beam wrote: > this should use $() from src/chrome/browser/resources/shared/util.js and should > be using .hidden instead of .style.display Done. http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:168: <body oncontextmenu="return false;"> On 2012/08/21 21:54:15, Dan Beam wrote: > no inline handlers, make these in the JS instead. Done. http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:173: <div><img src="../../app/theme/%DISTRIBUTION%/product_logo_name_108.png" class="logo"></div> On 2012/08/21 21:54:15, Dan Beam wrote: > all lines should be less than 80 columns Done. http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:183: onclick="sendCommand('learnMore2'); return false;" On 2012/08/21 21:54:15, Dan Beam wrote: > 4 \s indent Done. http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:195: <div style="clear: both;"></div> On 2012/08/21 21:54:15, Dan Beam wrote: > all styles should be in the CSS Done. http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:206: <hr class="hr"> On 2012/08/21 21:54:15, Dan Beam wrote: > hr.hr makes little to no sense to me Done. http://codereview.chromium.org/10855260/diff/12017/chrome/browser/resources/s... chrome/browser/resources/safe_browsing_malware_block_v2.html:208: <a class="right" href="" onclick="seeMore(); return false;" On 2012/08/21 21:54:15, Dan Beam wrote: > use preventDefault() instead of return false; and make href="#" and call > preventDefaultForPoundLinkClicks() in > src/chrome/browser/resources/shared/util.js Done.
http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... File chrome/browser/resources/safe_browsing/malware_block_v2.css (right): http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:7: background-image: url('images/stripe.png'); combine into a background rule http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:8: font-family: Arial,Helvetica,sans-serif; spaces in front of commas, you might want to figure out which of these you really what per platform and make this work with the user customized font as set per settings. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:33: html[dir=rtl] .float { nit: [dir='rtl'] (single quotes) http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:50: margin: 0 90px 0 20px; nothing that sets (margin/padding)-(left/right) will work in RTL. you can see what I mean by running: document.documentElement.dir = 'rtl'; in the console and/or running chrome in Hebrew or Arabic, with: LANG=he ./out/Debug/chrome # on Linux one way to solve this is by using -webkit-(padding/margin)-*, let me give you an example: /* 20px of left margin, 90px of right margin in LTR. * In RTL, 20px of right margin and 90px of left margin. */ -webkit-margin-start: 20px; -webkit-margin-end: 90px; margin-bottom: 0; margin-top: 0; http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:51: margin-bottom: 6pt; combine with above margin statement http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:84: 0 1px 2px rgba(0,0,0,0.2); nit: rgba(0, 0, 0, 0.2) http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:88: border-bottom: 1px solid rgb(53, 116, 203); border-bottom-color: (and below as well there are a couple of occurrences) http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:93: font-family: arial, helvetica, sans-serif; font: bold 14px/1 arial, helvetica, sans-serif; http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:123: margin: 0 20px 0 20px; shorten to 0 20px; http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:130: a:link { combine, i.e. a:link, a:visited { text-decoration: none; } a:active, a:hover { text-decoration: underline; } http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... File chrome/browser/resources/safe_browsing/malware_block_v2.html (right): http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:11: <table width="100%" cellspacing="0" cellpadding="0"> you really need a table for this? also, indent <table>, IMO http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:14: <div><img \n before <img> http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... File chrome/browser/resources/safe_browsing/malware_block_v2.js (right): http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.js:4: might want to add a doc comment (i.e. /** This is what I'm doing. */) to each of these functions http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.js:12: if (checkBox.checked) { nit: no curlies for 1-liners at every level (conditional expressions, block bodies, etc.) http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.js:46: } }; (after all these function expressions) http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.js:49: e.preventDefault(); none of these need prevenDefault() if they're simply clicks on <a href="#">, that's what preventDefaultOnPoundLinkClicks() does.
LGTM for my parts.
http://codereview.chromium.org/10855260/diff/18001/chrome/app/generated_resou... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/10855260/diff/18001/chrome/app/generated_resou... chrome/app/generated_resources.grd:10736: + <ph name="BEGIN_SPAN"><span jsdisplay="!proceedDisabled"></ph>If you <ph name="BEGIN_LINK2"><a id="proceed" href="#"></ph>proceed against Google's recommendation<ph name="END_LINK2"></a></ph>, you'll most likely become a victim of malware even if you have anti-virus software.<ph name="END_SPAN"></span></ph> Please don't put logic in here http://codereview.chromium.org/10855260/diff/18001/chrome/browser/chrome_brow... File chrome/browser/chrome_browser_field_trials.cc (right): http://codereview.chromium.org/10855260/diff/18001/chrome/browser/chrome_brow... chrome/browser/chrome_browser_field_trials.cc:590: base::FieldTrialList::FactoryGetFieldTrial("SBInterstitial", 100, What are these constant? Same Q below. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... File chrome/browser/resources/safe_browsing/malware_block_v2.css (right): http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:8: font-family: Arial,Helvetica,sans-serif; On 2012/08/22 02:34:03, Dan Beam wrote: > spaces in front of commas, you might want to figure out which of these you > really what per platform and make this work with the user customized font as set > per settings. Dan meant space after comma ;-) http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:21: font-size: 10pt; Use px sizes http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:30: float: right; Don't use float. Floats are hard to reason about http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:79: margin: 0; This should not be needed http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:83: -webkit-box-shadow: inset 0 1px 0 0 rgb(129, 169, 243), Can you import a shared button css file? http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:131: text-decoration: none; Why? I don't think we do this in any other part of the UI http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... File chrome/browser/resources/safe_browsing/malware_block_v2.html (right): http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:2: <html id="template_root" i18n-values="dir:textdirection"> template-root http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:11: <table width="100%" cellspacing="0" cellpadding="0"> Useless table also, no presentational attributes please http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:14: <div><img Is this div needed? http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:19: <div class="float"><img src="images/malware_icon_v2.jpg" Useless div? http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:21: <div class="main" i18n-values=".innerHTML:description1"></div> Do you really need innerHTML? http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:29: <form class="submission"> Why form? http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:39: <a id="show-diagnostic-link" href="#" i18n-content="details" Don't use # links Use a button if the link doesn't take the user anywhere http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:40: jsdisplay="displayShowDiagnostic"></a> I would remove the use of jsdisplay and then remove jstemplate too. It is really overkill for this page. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:46: <div id="footer"> <footer> http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:52: <label for="check-report"> useless attribute http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:53: <input id="check-report" name="checked" type="checkbox" Is the name really used? http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:54: jsvalues=".checked:boxchecked"> i18n-values http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:55: <span i18n-values=".innerHTML:confirm_text"></span> innerHTML? http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... File chrome/browser/resources/safe_browsing/malware_block_v2.js (right): http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.js:33: * Context menu handler to disable context menu during interstitial. Why? http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.js:43: window.onload = function() { DOMContentLoaded is faster http://ie.microsoft.com/testdrive/HTML5/DOMContentLoaded/Default.html http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.js:49: e.preventDefault(); On 2012/08/22 02:34:03, Dan Beam wrote: > none of these need prevenDefault() if they're simply clicks on <a href="#">, > that's what preventDefaultOnPoundLinkClicks() does. Another rule is to never have a preventDefault or stopPropagation without a comment explaining why. People abuse these two a lot. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/safe_browsi... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): http://codereview.chromium.org/10855260/diff/18001/chrome/browser/safe_browsi... chrome/browser/safe_browsing/safe_browsing_service.cc:1119: DVLOG(1) << __FUNCTION__ << " " << resource.url.spec(); remove these http://codereview.chromium.org/10855260/diff/18001/chrome/tools/chromeactions... File chrome/tools/chromeactions.txt (right): http://codereview.chromium.org/10855260/diff/18001/chrome/tools/chromeactions... chrome/tools/chromeactions.txt:18: 0x26ca6c633d183f66 AboutFlags_enable-pnacl unrelated
Looking at the screenshots... here are two alternatives to floats: 1. Use padding-end/margin-end and a positioned background-image 2. Use a flexbox
http://codereview.chromium.org/10855260/diff/18001/chrome/app/generated_resou... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/10855260/diff/18001/chrome/app/generated_resou... chrome/app/generated_resources.grd:10736: + <ph name="BEGIN_SPAN"><span jsdisplay="!proceedDisabled"></ph>If you <ph name="BEGIN_LINK2"><a id="proceed" href="#"></ph>proceed against Google's recommendation<ph name="END_LINK2"></a></ph>, you'll most likely become a victim of malware even if you have anti-virus software.<ph name="END_SPAN"></span></ph> On 2012/08/22 14:33:45, arv wrote: > Please don't put logic in here Done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/chrome_brow... File chrome/browser/chrome_browser_field_trials.cc (right): http://codereview.chromium.org/10855260/diff/18001/chrome/browser/chrome_brow... chrome/browser/chrome_browser_field_trials.cc:590: base::FieldTrialList::FactoryGetFieldTrial("SBInterstitial", 100, On 2012/08/22 14:33:45, arv wrote: > What are these constant? > > Same Q below. Done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... File chrome/browser/resources/safe_browsing/malware_block_v2.css (right): http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:7: background-image: url('images/stripe.png'); On 2012/08/22 02:34:03, Dan Beam wrote: > combine into a background rule done http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:8: font-family: Arial,Helvetica,sans-serif; On 2012/08/22 14:33:45, arv wrote: > On 2012/08/22 02:34:03, Dan Beam wrote: > > spaces in front of commas, you might want to figure out which of these you > > really what per platform and make this work with the user customized font as > set > > per settings. > > Dan meant space after comma ;-) Done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:21: font-size: 10pt; On 2012/08/22 14:33:45, arv wrote: > Use px sizes Done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:30: float: right; On 2012/08/22 14:33:45, arv wrote: > Don't use float. Floats are hard to reason about Do you mind if I leave the float for now? I don't see anything prohibiting floats in the style guide and I'd rather not block the change on reimplementing this again. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:33: html[dir=rtl] .float { On 2012/08/22 02:34:03, Dan Beam wrote: > nit: [dir='rtl'] (single quotes) Done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:50: margin: 0 90px 0 20px; On 2012/08/22 02:34:03, Dan Beam wrote: > nothing that sets (margin/padding)-(left/right) will work in RTL. you can see > what I mean by running: > > document.documentElement.dir = 'rtl'; > > in the console and/or running chrome in Hebrew or Arabic, with: > > LANG=he ./out/Debug/chrome # on Linux > > one way to solve this is by using -webkit-(padding/margin)-*, let me give you an > example: > > /* 20px of left margin, 90px of right margin in LTR. > * In RTL, 20px of right margin and 90px of left margin. */ > -webkit-margin-start: 20px; > -webkit-margin-end: 90px; > margin-bottom: 0; > margin-top: 0; Thanks, done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:51: margin-bottom: 6pt; On 2012/08/22 02:34:03, Dan Beam wrote: > combine with above margin statement Done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:79: margin: 0; On 2012/08/22 14:33:45, arv wrote: > This should not be needed Done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:83: -webkit-box-shadow: inset 0 1px 0 0 rgb(129, 169, 243), On 2012/08/22 14:33:45, arv wrote: > Can you import a shared button css file? When I implement the new versions of the other sb interstitial pages I'll pull out the common bits. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:84: 0 1px 2px rgba(0,0,0,0.2); On 2012/08/22 02:34:03, Dan Beam wrote: > nit: rgba(0, 0, 0, 0.2) Done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:88: border-bottom: 1px solid rgb(53, 116, 203); On 2012/08/22 02:34:03, Dan Beam wrote: > border-bottom-color: (and below as well there are a couple of occurrences) Done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:93: font-family: arial, helvetica, sans-serif; On 2012/08/22 02:34:03, Dan Beam wrote: > font: bold 14px/1 arial, helvetica, sans-serif; Done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:123: margin: 0 20px 0 20px; On 2012/08/22 02:34:03, Dan Beam wrote: > shorten to 0 20px; Done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:130: a:link { On 2012/08/22 02:34:03, Dan Beam wrote: > combine, i.e. > > a:link, > a:visited { > text-decoration: none; > } > > a:active, > a:hover { > text-decoration: underline; > } Done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.css:131: text-decoration: none; On 2012/08/22 14:33:45, arv wrote: > Why? I don't think we do this in any other part of the UI In this case it's to de-emphasize the links so the user can focus on reading the warning instead of immediately trying to click through. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... File chrome/browser/resources/safe_browsing/malware_block_v2.html (right): http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:2: <html id="template_root" i18n-values="dir:textdirection"> On 2012/08/22 14:33:45, arv wrote: > template-root Done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:11: <table width="100%" cellspacing="0" cellpadding="0"> On 2012/08/22 02:34:03, Dan Beam wrote: > you really need a table for this? Do you mind if I leave the table for now? This is all just based off the previous version of this page, which you can see was not style compliant. I can try to get rid of this table but I'd rather not block the change on it. > also, indent <table>, IMO done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:14: <div><img On 2012/08/22 02:34:03, Dan Beam wrote: > \n before <img> Done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:14: <div><img On 2012/08/22 14:33:45, arv wrote: > Is this div needed? Done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:19: <div class="float"><img src="images/malware_icon_v2.jpg" On 2012/08/22 14:33:45, arv wrote: > Useless div? Done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:21: <div class="main" i18n-values=".innerHTML:description1"></div> On 2012/08/22 14:33:45, arv wrote: > Do you really need innerHTML? Done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:29: <form class="submission"> On 2012/08/22 14:33:45, arv wrote: > Why form? Done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:39: <a id="show-diagnostic-link" href="#" i18n-content="details" On 2012/08/22 14:33:45, arv wrote: > Don't use # links > > Use a button if the link doesn't take the user anywhere This link does actually take you somewhere, it's just handled by sending a command to the C++ handler due to how interstitial pages work. (only the "more" / "less" link doesn't go somewhere. But that is the UI that was decided on, and it's been through UI review. You can ask Yi if you have additional questions.) http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:40: jsdisplay="displayShowDiagnostic"></a> On 2012/08/22 14:33:45, arv wrote: > I would remove the use of jsdisplay and then remove jstemplate too. It is really > overkill for this page. Doesn't jstemplate also do all the i18n strings and stuff too? Stuffing that all in manually doesn't seem easier. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:46: <div id="footer"> On 2012/08/22 14:33:45, arv wrote: > <footer> Done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:52: <label for="check-report"> On 2012/08/22 14:33:45, arv wrote: > useless attribute Done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:53: <input id="check-report" name="checked" type="checkbox" On 2012/08/22 14:33:45, arv wrote: > Is the name really used? removed http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:54: jsvalues=".checked:boxchecked"> On 2012/08/22 14:33:45, arv wrote: > i18n-values I'm not sure the exact difference, but that doesn't seem right. This is setting whether it's checked by default, it's not internationalization related. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.html:55: <span i18n-values=".innerHTML:confirm_text"></span> On 2012/08/22 14:33:45, arv wrote: > innerHTML? the string contains an embedded link http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... File chrome/browser/resources/safe_browsing/malware_block_v2.js (right): http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.js:4: On 2012/08/22 02:34:03, Dan Beam wrote: > might want to add a doc comment (i.e. /** This is what I'm doing. */) to each of > these functions Done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.js:12: if (checkBox.checked) { On 2012/08/22 02:34:03, Dan Beam wrote: > nit: no curlies for 1-liners at every level (conditional expressions, block > bodies, etc.) Done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.js:33: * Context menu handler to disable context menu during interstitial. On 2012/08/22 14:33:45, arv wrote: > Why? I think it's because some browser functions don't work correctly when on an interstitial page. But on further testing I see the InterstitialPage implementation itself already does this in c++, so I removed this handler. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.js:43: window.onload = function() { On 2012/08/22 14:33:45, arv wrote: > DOMContentLoaded is faster > > http://ie.microsoft.com/testdrive/HTML5/DOMContentLoaded/Default.html Done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.js:46: } On 2012/08/22 02:34:03, Dan Beam wrote: > }; (after all these function expressions) Done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.js:49: e.preventDefault(); On 2012/08/22 02:34:03, Dan Beam wrote: > none of these need prevenDefault() if they're simply clicks on <a href="#">, > that's what preventDefaultOnPoundLinkClicks() does. Removed them all except for the 'see-more-link' one. preventDefaultOnPoundLinkClicks doesn't catch it since the click target is actually the <span> inside the link. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.js:49: e.preventDefault(); On 2012/08/22 14:33:45, arv wrote: > On 2012/08/22 02:34:03, Dan Beam wrote: > > none of these need prevenDefault() if they're simply clicks on <a href="#">, > > that's what preventDefaultOnPoundLinkClicks() does. > > Another rule is to never have a preventDefault or stopPropagation without a > comment explaining why. People abuse these two a lot. Done. http://codereview.chromium.org/10855260/diff/18001/chrome/browser/safe_browsi... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): http://codereview.chromium.org/10855260/diff/18001/chrome/browser/safe_browsi... chrome/browser/safe_browsing/safe_browsing_service.cc:1119: DVLOG(1) << __FUNCTION__ << " " << resource.url.spec(); On 2012/08/22 14:33:45, arv wrote: > remove these Done. http://codereview.chromium.org/10855260/diff/18001/chrome/tools/chromeactions... File chrome/tools/chromeactions.txt (right): http://codereview.chromium.org/10855260/diff/18001/chrome/tools/chromeactions... chrome/tools/chromeactions.txt:18: 0x26ca6c633d183f66 AboutFlags_enable-pnacl On 2012/08/22 14:33:45, arv wrote: > unrelated It happens if someone else forgets to run chrome/tools/extract_actions.py.
chrome/browser/resources LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mattm@chromium.org/10855260/28002
Try job failure for 10855260-28002 (retry) on linux_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mattm@chromium.org/10855260/24007
Change committed as 152941
http://codereview.chromium.org/10855260/diff/24007/chrome/browser/resources/s... File chrome/browser/resources/safe_browsing/malware_block_v2.js (right): http://codereview.chromium.org/10855260/diff/24007/chrome/browser/resources/s... chrome/browser/resources/safe_browsing/malware_block_v2.js:62: // contains <span>s, which confuse preventDefaultOnPoundLinkClicks. preventDefaultOnPoundLinkClicks can be fixed to handle this |
