Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
339503004:
Add the extended reporting checkbox to the malware interstitial v3 (Closed)
|
Created:
6 years, 6 months ago by felt Modified:
6 years, 6 months ago CC:
chromium-reviews, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAdd the extended reporting checkbox to the malware interstitial v3
The new layout was missing space for a checkbox. This adds the checkbox, and adjusts the surrounding margins.
BUG=381260
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277920
Patch Set 1 #
Total comments: 26
Patch Set 2 : Comments #Patch Set 3 : Enable relevant test #
Total comments: 12
Patch Set 4 : changes for dbeam #
Total comments: 4
Patch Set 5 : No more pointer #
Total comments: 4
Patch Set 6 : Tweak #Patch Set 7 : nits #Patch Set 8 : Typo fix #Patch Set 9 : opt-in, not optin #
Messages
Total messages: 19 (0 generated)
PTAL.
https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/saf... File chrome/browser/resources/safe_browsing/safe_browsing_v3.js (right): https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/saf... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:23: if (loadTimeData.getBoolean('phishing')) return; ^ are 'ssl' or 'phishing' defined in a .cc file somewhere like SB_*? https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/saf... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:24: if (!loadTimeData.getBoolean(SB_DISPLAY_CHECK_BOX)) return; nit: combine if -> returns https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/saf... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:26: $('optin-label').innerHTML = loadTimeData.getString('optinText'); can you use .innerText or .textContent instead of .innerHTML? https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/saf... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:34: sendCommand(SB_CMD_DONT_REPORT); ternary, imo https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/ssl... File chrome/browser/resources/ssl/interstitial_v2.css (right): https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/ssl... chrome/browser/resources/ssl/interstitial_v2.css:108: margin-left: 32px; RTL? https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/ssl... chrome/browser/resources/ssl/interstitial_v2.css:140: float: left; RTL? https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/ssl... chrome/browser/resources/ssl/interstitial_v2.css:161: border-right: 0; RTL? https://codereview.chromium.org/339503004/diff/1/chrome/browser/safe_browsing... File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right): https://codereview.chromium.org/339503004/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:1313: if (!CanShowMalwareDetailsOption()) return; nit: if () { ... } instead of if (!) return; so people are less likely to add unrelated code below here that accidentally doesn't get run
https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/saf... File chrome/browser/resources/safe_browsing/safe_browsing_v3.js (right): https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/saf... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:23: if (loadTimeData.getBoolean('phishing')) return; I wonder if it should have some single value like getString('pageType') == 'malware' instead of a bunch of random bools like this. https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/saf... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:28: $('malware-optin').classList.remove('hidden'); Should be possible to do a lot of this without JS, see the old html: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... Or was it done this way to avoid needing to initialize the loadTimeData for these things to empty values for the other page types? https://codereview.chromium.org/339503004/diff/1/chrome/browser/safe_browsing... File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right): https://codereview.chromium.org/339503004/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:1314: load_time_data->SetBoolean(kDisplayCheckBox, true); is it okay that this is undefined instead of false when !CanShowMalwareDetailsOption? https://codereview.chromium.org/339503004/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:1324: kBoxChecked, IsPrefEnabled(prefs::kSafeBrowsingReportingEnabled)); This should do the migration stuff like 336673002?
https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/saf... File chrome/browser/resources/safe_browsing/safe_browsing_v3.js (right): https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/saf... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:23: if (loadTimeData.getBoolean('phishing')) return; On 2014/06/16 19:05:50, Dan Beam wrote: > ^ are 'ssl' or 'phishing' defined in a .cc file somewhere like SB_*? No. What I want to do down the line is create a parent class SecurityInterstitial that contains common state, like these booleans and other shared setup. Then have the SafeBrowsingBlockingPage and SSLBlockingPage hang off of it to do the customized portions. That will make this neater. https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/saf... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:24: if (!loadTimeData.getBoolean(SB_DISPLAY_CHECK_BOX)) return; On 2014/06/16 19:05:50, Dan Beam wrote: > nit: combine if -> returns Done. https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/saf... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:26: $('optin-label').innerHTML = loadTimeData.getString('optinText'); On 2014/06/16 19:05:50, Dan Beam wrote: > can you use .innerText or .textContent instead of .innerHTML? No, the string has a link in it. https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/saf... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:28: $('malware-optin').classList.remove('hidden'); On 2014/06/16 21:02:21, mattm wrote: > Should be possible to do a lot of this without JS, see the old html: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > Or was it done this way to avoid needing to initialize the loadTimeData for > these things to empty values for the other page types? Yes, I didn't want to have to set them to empty values for SSL. In a future world where the SSL and Safe Browsing warnings share methods and constants, it will be possible to just stick these in because they will have default empty values. https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/saf... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:34: sendCommand(SB_CMD_DONT_REPORT); On 2014/06/16 19:05:50, Dan Beam wrote: > ternary, imo Done. https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/ssl... File chrome/browser/resources/ssl/interstitial_v2.css (right): https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/ssl... chrome/browser/resources/ssl/interstitial_v2.css:108: margin-left: 32px; On 2014/06/16 19:05:50, Dan Beam wrote: > RTL? Done. https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/ssl... chrome/browser/resources/ssl/interstitial_v2.css:140: float: left; On 2014/06/16 19:05:50, Dan Beam wrote: > RTL? Done. https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/ssl... chrome/browser/resources/ssl/interstitial_v2.css:161: border-right: 0; On 2014/06/16 19:05:50, Dan Beam wrote: > RTL? Is it normal for checkboxes to be checked backwards in RTL? Seems weird since it is an image. https://codereview.chromium.org/339503004/diff/1/chrome/browser/safe_browsing... File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right): https://codereview.chromium.org/339503004/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:1313: if (!CanShowMalwareDetailsOption()) return; On 2014/06/16 19:05:50, Dan Beam wrote: > nit: if () { ... } instead of if (!) return; so people are less likely to add > unrelated code below here that accidentally doesn't get run Done. https://codereview.chromium.org/339503004/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:1314: load_time_data->SetBoolean(kDisplayCheckBox, true); On 2014/06/16 21:02:21, mattm wrote: > is it okay that this is undefined instead of false when > !CanShowMalwareDetailsOption? Done. https://codereview.chromium.org/339503004/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:1324: kBoxChecked, IsPrefEnabled(prefs::kSafeBrowsingReportingEnabled)); On 2014/06/16 21:02:21, mattm wrote: > This should do the migration stuff like 336673002? Done now that I could rebase off 336673002.
https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/saf... File chrome/browser/resources/safe_browsing/safe_browsing_v3.js (right): https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/saf... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:24: if (!loadTimeData.getBoolean(SB_DISPLAY_CHECK_BOX)) return; On 2014/06/17 05:41:54, felt wrote: > On 2014/06/16 19:05:50, Dan Beam wrote: > > nit: combine if -> returns > > Done. not done, but it was a nit https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/ssl... File chrome/browser/resources/ssl/interstitial_v2.css (right): https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/ssl... chrome/browser/resources/ssl/interstitial_v2.css:161: border-right: 0; On 2014/06/17 05:41:55, felt wrote: > On 2014/06/16 19:05:50, Dan Beam wrote: > > RTL? > > Is it normal for checkboxes to be checked backwards in RTL? Seems weird since it > is an image. i guess not https://codereview.chromium.org/339503004/diff/40001/chrome/browser/resources... File chrome/browser/resources/ssl/interstitial_v2.css (right): https://codereview.chromium.org/339503004/diff/40001/chrome/browser/resources... chrome/browser/resources/ssl/interstitial_v2.css:108: margin-left: 32px; -webkit-margin-start: 32px; (no need for [dir='rtl'] this way) https://codereview.chromium.org/339503004/diff/40001/chrome/browser/resources... chrome/browser/resources/ssl/interstitial_v2.css:158: cursor: pointer; ^ why is this a hand? https://codereview.chromium.org/339503004/diff/40001/chrome/browser/resources... chrome/browser/resources/ssl/interstitial_v2.css:161: position: absolute; does this need a right: 0;? (which would take precedence if [dir='rtl']) https://codereview.chromium.org/339503004/diff/40001/chrome/browser/resources... chrome/browser/resources/ssl/interstitial_v2.css:169: border-right: 0; border-right-width: 0; /* @noflip */ border-top-width: 0; https://codereview.chromium.org/339503004/diff/40001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right): https://codereview.chromium.org/339503004/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:1338: "optinText", change to optinLink to indicate it has HTML in it (and preferably, optInLink so I can tell this isn't simply option misspelled)
https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/saf... File chrome/browser/resources/safe_browsing/safe_browsing_v3.js (right): https://codereview.chromium.org/339503004/diff/1/chrome/browser/resources/saf... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:24: if (!loadTimeData.getBoolean(SB_DISPLAY_CHECK_BOX)) return; On 2014/06/17 17:52:54, Dan Beam wrote: > On 2014/06/17 05:41:54, felt wrote: > > On 2014/06/16 19:05:50, Dan Beam wrote: > > > nit: combine if -> returns > > > > Done. > > not done, but it was a nit Done for realsies. https://codereview.chromium.org/339503004/diff/40001/chrome/browser/resources... File chrome/browser/resources/ssl/interstitial_v2.css (right): https://codereview.chromium.org/339503004/diff/40001/chrome/browser/resources... chrome/browser/resources/ssl/interstitial_v2.css:108: margin-left: 32px; On 2014/06/17 17:52:54, Dan Beam wrote: > -webkit-margin-start: 32px; (no need for [dir='rtl'] this way) oh! that's how it's done. I was trying to remember. thanks. https://codereview.chromium.org/339503004/diff/40001/chrome/browser/resources... chrome/browser/resources/ssl/interstitial_v2.css:158: cursor: pointer; On 2014/06/17 17:52:54, Dan Beam wrote: > ^ why is this a hand? because the checkbox is clickable. note that this label is being used to make a fancy styled checkbox, there is no text in it. https://codereview.chromium.org/339503004/diff/40001/chrome/browser/resources... chrome/browser/resources/ssl/interstitial_v2.css:161: position: absolute; On 2014/06/17 17:52:54, Dan Beam wrote: > does this need a right: 0;? (which would take precedence if [dir='rtl']) Done. https://codereview.chromium.org/339503004/diff/40001/chrome/browser/resources... chrome/browser/resources/ssl/interstitial_v2.css:169: border-right: 0; On 2014/06/17 17:52:54, Dan Beam wrote: > border-right-width: 0; /* @noflip */ > border-top-width: 0; Done. https://codereview.chromium.org/339503004/diff/40001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right): https://codereview.chromium.org/339503004/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:1338: "optinText", On 2014/06/17 17:52:54, Dan Beam wrote: > change to optinLink to indicate it has HTML in it (and preferably, optInLink so > I can tell this isn't simply option misspelled) Done.
https://codereview.chromium.org/339503004/diff/40001/chrome/browser/resources... File chrome/browser/resources/ssl/interstitial_v2.css (right): https://codereview.chromium.org/339503004/diff/40001/chrome/browser/resources... chrome/browser/resources/ssl/interstitial_v2.css:158: cursor: pointer; On 2014/06/17 18:03:28, felt wrote: > On 2014/06/17 17:52:54, Dan Beam wrote: > > ^ why is this a hand? > > because the checkbox is clickable. note that this label is being used to make a > fancy styled checkbox, there is no text in it. there's no hand on any label/checkbox/radio in settings. how is this different? https://codereview.chromium.org/339503004/diff/60001/chrome/browser/resources... File chrome/browser/resources/ssl/interstitial_v2.css (right): https://codereview.chromium.org/339503004/diff/60001/chrome/browser/resources... chrome/browser/resources/ssl/interstitial_v2.css:49: margin: 45px 0 50px 0; did this ever work in RTL? can you try the code?
https://codereview.chromium.org/339503004/diff/40001/chrome/browser/resources... File chrome/browser/resources/ssl/interstitial_v2.css (right): https://codereview.chromium.org/339503004/diff/40001/chrome/browser/resources... chrome/browser/resources/ssl/interstitial_v2.css:158: cursor: pointer; On 2014/06/17 18:07:23, Dan Beam wrote: > On 2014/06/17 18:03:28, felt wrote: > > On 2014/06/17 17:52:54, Dan Beam wrote: > > > ^ why is this a hand? > > > > because the checkbox is clickable. note that this label is being used to make > a > > fancy styled checkbox, there is no text in it. > > there's no hand on any label/checkbox/radio in settings. how is this different? ah, I didn't realize that. un-handing. https://codereview.chromium.org/339503004/diff/60001/chrome/browser/resources... File chrome/browser/resources/ssl/interstitial_v2.css (right): https://codereview.chromium.org/339503004/diff/60001/chrome/browser/resources... chrome/browser/resources/ssl/interstitial_v2.css:49: margin: 45px 0 50px 0; On 2014/06/17 18:07:23, Dan Beam wrote: > did this ever work in RTL? can you try the code? this specifies the top and bottom margins, with both left and right set to 0. it appears to work fine in arabic. maybe I am missing something -- does RTL change top and bottom margins too?
https://codereview.chromium.org/339503004/diff/60001/chrome/browser/resources... File chrome/browser/resources/ssl/interstitial_v2.css (right): https://codereview.chromium.org/339503004/diff/60001/chrome/browser/resources... chrome/browser/resources/ssl/interstitial_v2.css:49: margin: 45px 0 50px 0; On 2014/06/17 18:25:52, felt wrote: > On 2014/06/17 18:07:23, Dan Beam wrote: > > did this ever work in RTL? can you try the code? > > this specifies the top and bottom margins, with both left and right set to 0. it > appears to work fine in arabic. maybe I am missing something -- does RTL change > top and bottom margins too? oh, sorry, i just misread. FYI: this could be margin: 45px 0 50px;
https://codereview.chromium.org/339503004/diff/60001/chrome/browser/resources... File chrome/browser/resources/ssl/interstitial_v2.css (right): https://codereview.chromium.org/339503004/diff/60001/chrome/browser/resources... chrome/browser/resources/ssl/interstitial_v2.css:49: margin: 45px 0 50px 0; On 2014/06/17 18:28:31, Dan Beam wrote: > On 2014/06/17 18:25:52, felt wrote: > > On 2014/06/17 18:07:23, Dan Beam wrote: > > > did this ever work in RTL? can you try the code? > > > > this specifies the top and bottom margins, with both left and right set to 0. > it > > appears to work fine in arabic. maybe I am missing something -- does RTL > change > > top and bottom margins too? > > oh, sorry, i just misread. > > FYI: this could be > > margin: 45px 0 50px; Done.
lgtm https://codereview.chromium.org/339503004/diff/80001/chrome/browser/resources... File chrome/browser/resources/safe_browsing/safe_browsing_v3.js (right): https://codereview.chromium.org/339503004/diff/80001/chrome/browser/resources... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:23: !loadTimeData.getBoolean(SB_DISPLAY_CHECK_BOX)) nit: curlies https://codereview.chromium.org/339503004/diff/80001/chrome/browser/resources... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:26: $('optin-label').innerHTML = loadTimeData.getString('optInLink'); opt nit: opt-in-label, etc.
lgtm
nits https://codereview.chromium.org/339503004/diff/80001/chrome/browser/resources... File chrome/browser/resources/safe_browsing/safe_browsing_v3.js (right): https://codereview.chromium.org/339503004/diff/80001/chrome/browser/resources... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:23: !loadTimeData.getBoolean(SB_DISPLAY_CHECK_BOX)) On 2014/06/17 18:31:06, Dan Beam wrote: > nit: curlies Done. https://codereview.chromium.org/339503004/diff/80001/chrome/browser/resources... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:26: $('optin-label').innerHTML = loadTimeData.getString('optInLink'); On 2014/06/17 18:31:05, Dan Beam wrote: > opt nit: opt-in-label, etc. Done.
The CQ bit was checked by felt@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felt@chromium.org/339503004/140001
The CQ bit was unchecked by felt@chromium.org
The CQ bit was checked by felt@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felt@chromium.org/339503004/160001
Message was sent while issue was closed.
Change committed as 277920 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
