Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
319193002:
Update the malware interstitial to have the new layout (Closed)
|
Created:
6 years, 6 months ago by felt Modified:
6 years, 6 months ago CC:
chromium-reviews, arv+watch_chromium.org, edwardjung Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUpdate the malware interstitial to have the new layout
This puts the existing malware & phishing text into the new security interstitial layout. Since it's not polished enough yet, this CL keeps it behind a flag.
Down the line, I'm going to want to make a new c/b/resources/security_interstitial/ folder and move all of the related SSL & SB files into there. But it doesn't make sense to do that yet, until we deprecate the older versions.
BUG=381260
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276414
Patch Set 1 #Patch Set 2 : Old wording in place in new layout #Patch Set 3 : Got it working for phishing too #Patch Set 4 : Pre-review style fixes #
Total comments: 28
Patch Set 5 : Fixed comments for bauerb #
Total comments: 10
Patch Set 6 : Round 2 of edits #
Total comments: 6
Patch Set 7 : Renamed command #
Total comments: 4
Patch Set 8 : Renamed variables, updated malware CSS label names #
Total comments: 10
Patch Set 9 : Changes for mattm #
Total comments: 2
Patch Set 10 : Removed field trial #Patch Set 11 : Rebased #Patch Set 12 : Unused variable #Patch Set 13 : Rebased again #
Messages
Total messages: 29 (0 generated)
Hi Bernhard, can you please review the c/b/resources/* changes? Thank you. Adrienne
https://codereview.chromium.org/319193002/diff/60001/chrome/browser/resources... File chrome/browser/resources/safe_browsing/safe_browsing_v3.js (right): https://codereview.chromium.org/319193002/diff/60001/chrome/browser/resources... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:6: var SB_CMD_SHOW_DIAGNOSTIC = 'showDiagnostic'; Sort these alphabetically? It might also make sense to rename them to match the value. https://codereview.chromium.org/319193002/diff/60001/chrome/browser/resources... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:31: loadTimeData.getString('detailsText')); Could you put this into the template and hide it if necessary? https://codereview.chromium.org/319193002/diff/60001/chrome/browser/resources... File chrome/browser/resources/ssl/interstitial_v2.html (right): https://codereview.chromium.org/319193002/diff/60001/chrome/browser/resources... chrome/browser/resources/ssl/interstitial_v2.html:12: <script src="../safe_browsing/safe_browsing_v3.js"></script> The versioning is getting a bit confusing here. Why do have v3 JS in the v2 HTML? https://codereview.chromium.org/319193002/diff/60001/chrome/browser/resources... File chrome/browser/resources/ssl/interstitial_v2.js (right): https://codereview.chromium.org/319193002/diff/60001/chrome/browser/resources... chrome/browser/resources/ssl/interstitial_v2.js:23: sendCommand(ssl ? CMD_DONT_PROCEED : SB_CMD_GO_BACK); I think you could reorder this as if (!ssl) sendCommand(SB_CMD_GO_BACK); else if (overridable) sendCommand(CMD_DONT_PROCEED); else sendCommand(CMD_RELOAD); https://codereview.chromium.org/319193002/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right): https://codereview.chromium.org/319193002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:156: return true; You could split these individual conditions up and return true for each of them. https://codereview.chromium.org/319193002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:1217: : SafeBrowsingBlockingPage(ui_manager, web_contents, unsafe_resources) { Indent two more spaces. https://codereview.chromium.org/319193002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:1223: NOTREACHED(); When is this branch reached? If you want to express that something has not been implemented yet, you can use NOTIMPLEMENTED(). https://codereview.chromium.org/319193002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:1230: load_time_data.SetString("textDirection", rtl ? "rtl" : "ltr"); Doesn't SetFontAndTextDirection already set this? https://codereview.chromium.org/319193002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:1256: threat_type == SB_THREAT_TYPE_CLIENT_SIDE_PHISHING_URL); How many threat types are there? Could you replace this with a switch statement? https://codereview.chromium.org/319193002/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_blocking_page.h (right): https://codereview.chromium.org/319193002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_blocking_page.h:283: class SafeBrowsingBlockingPageV3 : public SafeBrowsingBlockingPage { If this class isn't meant to be directly instantiated, you could move it to an anonymous namespace in the .cc file.
https://codereview.chromium.org/319193002/diff/60001/chrome/browser/resources... File chrome/browser/resources/safe_browsing/safe_browsing_v3.js (right): https://codereview.chromium.org/319193002/diff/60001/chrome/browser/resources... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:6: var SB_CMD_SHOW_DIAGNOSTIC = 'showDiagnostic'; On 2014/06/09 10:04:02, Bernhard Bauer wrote: > Sort these alphabetically? It might also make sense to rename them to match the > value. I sorted them alphabetically. I don't want to rename them though -- I want them to match safe_browsing_blocking_page.cc, and I would prefer not to refactor the old code right now in the same CL. https://codereview.chromium.org/319193002/diff/60001/chrome/browser/resources... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:31: loadTimeData.getString('detailsText')); On 2014/06/09 10:04:03, Bernhard Bauer wrote: > Could you put this into the template and hide it if necessary? If I put it into the template, I get JS console errors in the SSL interstitial when this value isn't available. Is there a way around that? https://codereview.chromium.org/319193002/diff/60001/chrome/browser/resources... File chrome/browser/resources/ssl/interstitial_v2.html (right): https://codereview.chromium.org/319193002/diff/60001/chrome/browser/resources... chrome/browser/resources/ssl/interstitial_v2.html:12: <script src="../safe_browsing/safe_browsing_v3.js"></script> On 2014/06/09 10:04:03, Bernhard Bauer wrote: > The versioning is getting a bit confusing here. Why do have v3 JS in the v2 > HTML? Yeah, I have some regrets. I started off with V2 because it is the second version of the SSL warning. However... there was already a malware warning redesign, so there is already a malware warning V2. So this is V2 for SSL and V3 for malware. :/ https://codereview.chromium.org/319193002/diff/60001/chrome/browser/resources... File chrome/browser/resources/ssl/interstitial_v2.js (right): https://codereview.chromium.org/319193002/diff/60001/chrome/browser/resources... chrome/browser/resources/ssl/interstitial_v2.js:23: sendCommand(ssl ? CMD_DONT_PROCEED : SB_CMD_GO_BACK); On 2014/06/09 10:04:03, Bernhard Bauer wrote: > I think you could reorder this as > > if (!ssl) > sendCommand(SB_CMD_GO_BACK); > else if (overridable) > sendCommand(CMD_DONT_PROCEED); > else > sendCommand(CMD_RELOAD); Done. https://codereview.chromium.org/319193002/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right): https://codereview.chromium.org/319193002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:156: return true; On 2014/06/09 10:04:03, Bernhard Bauer wrote: > You could split these individual conditions up and return true for each of them. Done. https://codereview.chromium.org/319193002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:1217: : SafeBrowsingBlockingPage(ui_manager, web_contents, unsafe_resources) { On 2014/06/09 10:04:03, Bernhard Bauer wrote: > Indent two more spaces. Done. https://codereview.chromium.org/319193002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:1223: NOTREACHED(); On 2014/06/09 10:04:03, Bernhard Bauer wrote: > When is this branch reached? If you want to express that something has not been > implemented yet, you can use NOTIMPLEMENTED(). That can happen if a page is marked as both malware and phishing. However, SafeBrowsingBlockingPageFactoryImpl should not dispatch that case to the V3 warning so this shouldn't be reachable. Switching to NOTIMPLEMENTED(). https://codereview.chromium.org/319193002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:1230: load_time_data.SetString("textDirection", rtl ? "rtl" : "ltr"); On 2014/06/09 10:04:03, Bernhard Bauer wrote: > Doesn't SetFontAndTextDirection already set this? Done. https://codereview.chromium.org/319193002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:1256: threat_type == SB_THREAT_TYPE_CLIENT_SIDE_PHISHING_URL); On 2014/06/09 10:04:03, Bernhard Bauer wrote: > How many threat types are there? Could you replace this with a switch statement? Done. https://codereview.chromium.org/319193002/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_blocking_page.h (right): https://codereview.chromium.org/319193002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_blocking_page.h:283: class SafeBrowsingBlockingPageV3 : public SafeBrowsingBlockingPage { On 2014/06/09 10:04:03, Bernhard Bauer wrote: > If this class isn't meant to be directly instantiated, you could move it to an > anonymous namespace in the .cc file. I copied the pattern the pattern that's already in place in this file (if you look above, V1 and V2 are handled in precisely the same way). Do you mind if I leave as-is for consistency? Or should I refactor all three?
palmer, can you please do an OWNERS review for a one-line change in c/b/ssl/ssl_blocking_page.cc
https://codereview.chromium.org/319193002/diff/60001/chrome/browser/resources... File chrome/browser/resources/safe_browsing/safe_browsing_v3.js (right): https://codereview.chromium.org/319193002/diff/60001/chrome/browser/resources... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:31: loadTimeData.getString('detailsText')); On 2014/06/09 14:24:09, felt wrote: > On 2014/06/09 10:04:03, Bernhard Bauer wrote: > > Could you put this into the template and hide it if necessary? > > If I put it into the template, I get JS console errors in the SSL interstitial > when this value isn't available. Is there a way around that? Hm, set the value to an empty string otherwise? https://codereview.chromium.org/319193002/diff/60001/chrome/browser/resources... File chrome/browser/resources/ssl/interstitial_v2.html (right): https://codereview.chromium.org/319193002/diff/60001/chrome/browser/resources... chrome/browser/resources/ssl/interstitial_v2.html:12: <script src="../safe_browsing/safe_browsing_v3.js"></script> On 2014/06/09 14:24:09, felt wrote: > On 2014/06/09 10:04:03, Bernhard Bauer wrote: > > The versioning is getting a bit confusing here. Why do have v3 JS in the v2 > > HTML? > > Yeah, I have some regrets. I started off with V2 because it is the second > version of the SSL warning. However... there was already a malware warning > redesign, so there is already a malware warning V2. So this is V2 for SSL and V3 > for malware. :/ Hrm, ok. Can you at least triple-pinky-promise me that you'll clean this up eventually to get rid of all the version numbers here? https://codereview.chromium.org/319193002/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right): https://codereview.chromium.org/319193002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:1223: NOTREACHED(); On 2014/06/09 14:24:09, felt wrote: > On 2014/06/09 10:04:03, Bernhard Bauer wrote: > > When is this branch reached? If you want to express that something has not > been > > implemented yet, you can use NOTIMPLEMENTED(). > > That can happen if a page is marked as both malware and phishing. However, > SafeBrowsingBlockingPageFactoryImpl should not dispatch that case to the V3 > warning so this shouldn't be reachable. Switching to NOTIMPLEMENTED(). OK... the usual way would be to DCHECK this instead, but if we want to handle that case eventually, that's fine. https://codereview.chromium.org/319193002/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_blocking_page.h (right): https://codereview.chromium.org/319193002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_blocking_page.h:283: class SafeBrowsingBlockingPageV3 : public SafeBrowsingBlockingPage { On 2014/06/09 14:24:09, felt wrote: > On 2014/06/09 10:04:03, Bernhard Bauer wrote: > > If this class isn't meant to be directly instantiated, you could move it to an > > anonymous namespace in the .cc file. > > I copied the pattern the pattern that's already in place in this file (if you > look above, V1 and V2 are handled in precisely the same way). Do you mind if I > leave as-is for consistency? Or should I refactor all three? If you don't mind shaving some yak, it would be a nice cleanup. If you do mind, it would be nice to leave at least a TODO here, so you (or some other helpful contributor) can do it in a followup. https://codereview.chromium.org/319193002/diff/80001/chrome/browser/resources... File chrome/browser/resources/safe_browsing/safe_browsing_v3.js (right): https://codereview.chromium.org/319193002/diff/80001/chrome/browser/resources... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:7: var SB_CMD_DISPLAY_CHECK = 'displaycheckbox'; Hm, this is called kDisplayCheckBox in safe_browsing_blocking_page.cc. https://codereview.chromium.org/319193002/diff/80001/chrome/browser/resources... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:11: var SB_CMD_GO_BACK = 'takeMeBack'; This one is called kTakeMeBackCommand. https://codereview.chromium.org/319193002/diff/80001/chrome/browser/resources... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:12: var SB_CMD_LEARN_MORE = 'learnMore2'; This one is called kLearnMoreCommandV2. https://codereview.chromium.org/319193002/diff/80001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right): https://codereview.chromium.org/319193002/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:155: } else if (base::FieldTrialList::FindFullName("MalwareInterstitialVersion") Nit: else isn't necessary if you return in the if branch.
https://codereview.chromium.org/319193002/diff/60001/chrome/browser/resources... File chrome/browser/resources/safe_browsing/safe_browsing_v3.js (right): https://codereview.chromium.org/319193002/diff/60001/chrome/browser/resources... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:31: loadTimeData.getString('detailsText')); On 2014/06/09 14:45:41, Bernhard Bauer wrote: > On 2014/06/09 14:24:09, felt wrote: > > On 2014/06/09 10:04:03, Bernhard Bauer wrote: > > > Could you put this into the template and hide it if necessary? > > > > If I put it into the template, I get JS console errors in the SSL interstitial > > when this value isn't available. Is there a way around that? > > Hm, set the value to an empty string otherwise? Sorry, I just realized that I was thinking of something else. That doesn't fix this. The reason I'm setting this up dynamically is so that I can linkify the text. I don't want to add the HTML to linkify to the grd file string definition -- it would break the V2 version, which uses the same string but doesn't have the same link attributes. However, once the designers have settled on *new* text here, I will create a new grd text entry and that will let me get rid of this dynamic link setup. I added a TODO. https://codereview.chromium.org/319193002/diff/60001/chrome/browser/resources... File chrome/browser/resources/ssl/interstitial_v2.html (right): https://codereview.chromium.org/319193002/diff/60001/chrome/browser/resources... chrome/browser/resources/ssl/interstitial_v2.html:12: <script src="../safe_browsing/safe_browsing_v3.js"></script> On 2014/06/09 14:45:41, Bernhard Bauer wrote: > On 2014/06/09 14:24:09, felt wrote: > > On 2014/06/09 10:04:03, Bernhard Bauer wrote: > > > The versioning is getting a bit confusing here. Why do have v3 JS in the v2 > > > HTML? > > > > Yeah, I have some regrets. I started off with V2 because it is the second > > version of the SSL warning. However... there was already a malware warning > > redesign, so there is already a malware warning V2. So this is V2 for SSL and > V3 > > for malware. :/ > > Hrm, ok. Can you at least triple-pinky-promise me that you'll clean this up > eventually to get rid of all the version numbers here? Triple-pinky-promised. I want to move all of the new code into a new /security_interstitial/ folder once the old versions are deprecated, which will let me get rid of all the old code and version numbers entirely. https://codereview.chromium.org/319193002/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_blocking_page.h (right): https://codereview.chromium.org/319193002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_blocking_page.h:283: class SafeBrowsingBlockingPageV3 : public SafeBrowsingBlockingPage { On 2014/06/09 14:45:41, Bernhard Bauer wrote: > On 2014/06/09 14:24:09, felt wrote: > > On 2014/06/09 10:04:03, Bernhard Bauer wrote: > > > If this class isn't meant to be directly instantiated, you could move it to > an > > > anonymous namespace in the .cc file. > > > > I copied the pattern the pattern that's already in place in this file (if you > > look above, V1 and V2 are handled in precisely the same way). Do you mind if I > > leave as-is for consistency? Or should I refactor all three? > > If you don't mind shaving some yak, it would be a nice cleanup. If you do mind, > it would be nice to leave at least a TODO here, so you (or some other helpful > contributor) can do it in a followup. I moved the V3 one into an anonymous namespace. I'll send another CL that does the same for V1 and V2, to avoid confusion about what this CL touches. https://codereview.chromium.org/319193002/diff/80001/chrome/browser/resources... File chrome/browser/resources/safe_browsing/safe_browsing_v3.js (right): https://codereview.chromium.org/319193002/diff/80001/chrome/browser/resources... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:7: var SB_CMD_DISPLAY_CHECK = 'displaycheckbox'; On 2014/06/09 14:45:41, Bernhard Bauer wrote: > Hm, this is called kDisplayCheckBox in safe_browsing_blocking_page.cc. These need to be JS style though right? I was trying to make JS style ones that would match up. https://codereview.chromium.org/319193002/diff/80001/chrome/browser/resources... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:11: var SB_CMD_GO_BACK = 'takeMeBack'; On 2014/06/09 14:45:41, Bernhard Bauer wrote: > This one is called kTakeMeBackCommand. Done. https://codereview.chromium.org/319193002/diff/80001/chrome/browser/resources... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:12: var SB_CMD_LEARN_MORE = 'learnMore2'; On 2014/06/09 14:45:42, Bernhard Bauer wrote: > This one is called kLearnMoreCommandV2. Done. https://codereview.chromium.org/319193002/diff/80001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right): https://codereview.chromium.org/319193002/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:155: } else if (base::FieldTrialList::FindFullName("MalwareInterstitialVersion") On 2014/06/09 14:45:42, Bernhard Bauer wrote: > Nit: else isn't necessary if you return in the if branch. Done.
https://codereview.chromium.org/319193002/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_blocking_page.h (right): https://codereview.chromium.org/319193002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_blocking_page.h:283: class SafeBrowsingBlockingPageV3 : public SafeBrowsingBlockingPage { On 2014/06/09 15:24:02, felt wrote: > On 2014/06/09 14:45:41, Bernhard Bauer wrote: > > On 2014/06/09 14:24:09, felt wrote: > > > On 2014/06/09 10:04:03, Bernhard Bauer wrote: > > > > If this class isn't meant to be directly instantiated, you could move it > to > > an > > > > anonymous namespace in the .cc file. > > > > > > I copied the pattern the pattern that's already in place in this file (if > you > > > look above, V1 and V2 are handled in precisely the same way). Do you mind if > I > > > leave as-is for consistency? Or should I refactor all three? > > > > If you don't mind shaving some yak, it would be a nice cleanup. If you do > mind, > > it would be nice to leave at least a TODO here, so you (or some other helpful > > contributor) can do it in a followup. > > I moved the V3 one into an anonymous namespace. I'll send another CL that does > the same for V1 and V2, to avoid confusion about what this CL touches. Awesome, thanks! https://codereview.chromium.org/319193002/diff/80001/chrome/browser/resources... File chrome/browser/resources/safe_browsing/safe_browsing_v3.js (right): https://codereview.chromium.org/319193002/diff/80001/chrome/browser/resources... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:7: var SB_CMD_DISPLAY_CHECK = 'displaycheckbox'; On 2014/06/09 15:24:02, felt wrote: > On 2014/06/09 14:45:41, Bernhard Bauer wrote: > > Hm, this is called kDisplayCheckBox in safe_browsing_blocking_page.cc. > > These need to be JS style though right? I was trying to make JS style ones that > would match up. Yes, but with _BOX at the end please. https://codereview.chromium.org/319193002/diff/100001/chrome/browser/resource... File chrome/browser/resources/safe_browsing/safe_browsing_v3.js (right): https://codereview.chromium.org/319193002/diff/100001/chrome/browser/resource... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:10: var SB_CMD_EXPANDED_MORE = 'expandedSeeMore'; And this one is kExpandedSeeMore (without Command... <sigh>). Basically, what I'd like is to make all these match the values.
https://codereview.chromium.org/319193002/diff/80001/chrome/browser/resources... File chrome/browser/resources/safe_browsing/safe_browsing_v3.js (right): https://codereview.chromium.org/319193002/diff/80001/chrome/browser/resources... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:7: var SB_CMD_DISPLAY_CHECK = 'displaycheckbox'; On 2014/06/09 15:43:00, Bernhard Bauer wrote: > On 2014/06/09 15:24:02, felt wrote: > > On 2014/06/09 14:45:41, Bernhard Bauer wrote: > > > Hm, this is called kDisplayCheckBox in safe_browsing_blocking_page.cc. > > > > These need to be JS style though right? I was trying to make JS style ones > that > > would match up. > > Yes, but with _BOX at the end please. Done. Apparently my brain cannot see the difference between CHECK_BOX and BOX_CHECK. https://codereview.chromium.org/319193002/diff/100001/chrome/browser/resource... File chrome/browser/resources/safe_browsing/safe_browsing_v3.js (right): https://codereview.chromium.org/319193002/diff/100001/chrome/browser/resource... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:10: var SB_CMD_EXPANDED_MORE = 'expandedSeeMore'; On 2014/06/09 15:43:00, Bernhard Bauer wrote: > And this one is kExpandedSeeMore (without Command... <sigh>). > > Basically, what I'd like is to make all these match the values. Done. I don't want to add Command to the end of them... it would be quite long. But I like prefixing them all as SB_CMD_ so it's clear what the variable is about.
https://codereview.chromium.org/319193002/diff/100001/chrome/browser/resource... File chrome/browser/resources/safe_browsing/safe_browsing_v3.js (right): https://codereview.chromium.org/319193002/diff/100001/chrome/browser/resource... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:10: var SB_CMD_EXPANDED_MORE = 'expandedSeeMore'; On 2014/06/09 16:55:27, felt wrote: > On 2014/06/09 15:43:00, Bernhard Bauer wrote: > > And this one is kExpandedSeeMore (without Command... <sigh>). > > > > Basically, what I'd like is to make all these match the values. > > Done. > > I don't want to add Command to the end of them... it would be quite long. But I > like prefixing them all as SB_CMD_ so it's clear what the variable is about. Yes, I like that (and I was just lamenting about the lack of consistency on the C++ side). Oh, I just realized that not all of them are actually commands! In that case, it may be more confusing than helpful to prefix them with CMD_... Could we do the following: * Sort the strings in safe_browsing_blocking_page.cc first by type (command vs. key in loadTimeData), then alphabetically * Apply the same sort order here * Remove the CMD_ prefix here from things that aren't commands We could also leave out the non-commands here if we don't actually need them. And sorry for not catching this earlier :-( https://codereview.chromium.org/319193002/diff/120001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (left): https://codereview.chromium.org/319193002/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:107: const char kExpandedSeeMore[] = "expandedSeeMore"; Note that this one actually *is* a command.
lgtm
https://codereview.chromium.org/319193002/diff/100001/chrome/browser/resource... File chrome/browser/resources/safe_browsing/safe_browsing_v3.js (right): https://codereview.chromium.org/319193002/diff/100001/chrome/browser/resource... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:10: var SB_CMD_EXPANDED_MORE = 'expandedSeeMore'; On 2014/06/09 17:17:10, Bernhard Bauer wrote: > On 2014/06/09 16:55:27, felt wrote: > > On 2014/06/09 15:43:00, Bernhard Bauer wrote: > > > And this one is kExpandedSeeMore (without Command... <sigh>). > > > > > > Basically, what I'd like is to make all these match the values. > > > > Done. > > > > I don't want to add Command to the end of them... it would be quite long. But > I > > like prefixing them all as SB_CMD_ so it's clear what the variable is about. > > Yes, I like that (and I was just lamenting about the lack of consistency on the > C++ side). > > Oh, I just realized that not all of them are actually commands! In that case, it > may be more confusing than helpful to prefix them with CMD_... > > Could we do the following: > * Sort the strings in safe_browsing_blocking_page.cc first by type (command vs. > key in loadTimeData), then alphabetically > * Apply the same sort order here > * Remove the CMD_ prefix here from things that aren't commands > > We could also leave out the non-commands here if we don't actually need them. > > And sorry for not catching this earlier :-( I'm not sure what distinction you're drawing between command vs non-command. They all tell the c++ to do something -- open another window, navigate the page, or record an UMA statistic. What did you have in mind for the distinction?
I had a suggestion on the CSS classes organisation. https://codereview.chromium.org/319193002/diff/120001/chrome/browser/resource... File chrome/browser/resources/safe_browsing/safe_browsing_v3.js (right): https://codereview.chromium.org/319193002/diff/120001/chrome/browser/resource... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:57: links[i].classList.add('a-red'); It'll be cleaner to just add a single class to the body, then target in the CSS, rather than adding classes to individual elements. So your CSS would be something like: .malware { … } .malware #icon { … } .malware #primary-button { … } then you might also have a body class for SSL, or whatever type of page you want. .ssl .icon { … }
https://codereview.chromium.org/319193002/diff/100001/chrome/browser/resource... File chrome/browser/resources/safe_browsing/safe_browsing_v3.js (right): https://codereview.chromium.org/319193002/diff/100001/chrome/browser/resource... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:10: var SB_CMD_EXPANDED_MORE = 'expandedSeeMore'; On 2014/06/09 18:01:36, felt wrote: > On 2014/06/09 17:17:10, Bernhard Bauer wrote: > > On 2014/06/09 16:55:27, felt wrote: > > > On 2014/06/09 15:43:00, Bernhard Bauer wrote: > > > > And this one is kExpandedSeeMore (without Command... <sigh>). > > > > > > > > Basically, what I'd like is to make all these match the values. > > > > > > Done. > > > > > > I don't want to add Command to the end of them... it would be quite long. > But > > I > > > like prefixing them all as SB_CMD_ so it's clear what the variable is about. > > > > Yes, I like that (and I was just lamenting about the lack of consistency on > the > > C++ side). > > > > Oh, I just realized that not all of them are actually commands! In that case, > it > > may be more confusing than helpful to prefix them with CMD_... > > > > Could we do the following: > > * Sort the strings in safe_browsing_blocking_page.cc first by type (command > vs. > > key in loadTimeData), then alphabetically > > * Apply the same sort order here > > * Remove the CMD_ prefix here from things that aren't commands > > > > We could also leave out the non-commands here if we don't actually need them. > > > > And sorry for not catching this earlier :-( > > I'm not sure what distinction you're drawing between command vs non-command. > They all tell the c++ to do something -- open another window, navigate the page, > or record an UMA statistic. What did you have in mind for the distinction? No, kBoxChecked and kDisplayCheckBox are used as keys into loadTimeData, not as commands.
https://codereview.chromium.org/319193002/diff/100001/chrome/browser/resource... File chrome/browser/resources/safe_browsing/safe_browsing_v3.js (right): https://codereview.chromium.org/319193002/diff/100001/chrome/browser/resource... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:10: var SB_CMD_EXPANDED_MORE = 'expandedSeeMore'; On 2014/06/09 21:48:27, Bernhard Bauer wrote: > On 2014/06/09 18:01:36, felt wrote: > > On 2014/06/09 17:17:10, Bernhard Bauer wrote: > > > On 2014/06/09 16:55:27, felt wrote: > > > > On 2014/06/09 15:43:00, Bernhard Bauer wrote: > > > > > And this one is kExpandedSeeMore (without Command... <sigh>). > > > > > > > > > > Basically, what I'd like is to make all these match the values. > > > > > > > > Done. > > > > > > > > I don't want to add Command to the end of them... it would be quite long. > > But > > > I > > > > like prefixing them all as SB_CMD_ so it's clear what the variable is > about. > > > > > > Yes, I like that (and I was just lamenting about the lack of consistency on > > the > > > C++ side). > > > > > > Oh, I just realized that not all of them are actually commands! In that > case, > > it > > > may be more confusing than helpful to prefix them with CMD_... > > > > > > Could we do the following: > > > * Sort the strings in safe_browsing_blocking_page.cc first by type (command > > vs. > > > key in loadTimeData), then alphabetically > > > * Apply the same sort order here > > > * Remove the CMD_ prefix here from things that aren't commands > > > > > > We could also leave out the non-commands here if we don't actually need > them. > > > > > > And sorry for not catching this earlier :-( > > > > I'm not sure what distinction you're drawing between command vs non-command. > > They all tell the c++ to do something -- open another window, navigate the > page, > > or record an UMA statistic. What did you have in mind for the distinction? > > No, kBoxChecked and kDisplayCheckBox are used as keys into loadTimeData, not as > commands. Separated them out. https://codereview.chromium.org/319193002/diff/120001/chrome/browser/resource... File chrome/browser/resources/safe_browsing/safe_browsing_v3.js (right): https://codereview.chromium.org/319193002/diff/120001/chrome/browser/resource... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:57: links[i].classList.add('a-red'); On 2014/06/09 18:37:29, edwardjung wrote: > It'll be cleaner to just add a single class to the body, then target in the CSS, > rather than adding classes to individual elements. > > So your CSS would be something like: > > .malware { > … > } > > .malware #icon { > … > } > > .malware #primary-button { > … > } > > then you might also have a body class for SSL, or whatever type of page you > want. > > .ssl .icon { > … > } > Good idea, done. https://codereview.chromium.org/319193002/diff/120001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (left): https://codereview.chromium.org/319193002/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:107: const char kExpandedSeeMore[] = "expandedSeeMore"; On 2014/06/09 17:17:10, Bernhard Bauer wrote: > Note that this one actually *is* a command. Renamed to kExpandedSeeMoreCommand.
LGTM, thanks!
matt, can i get an OWNERS review for chrome/browser/safe_browsing/safe_browsing_blocking_page.cc?
lgtm
https://codereview.chromium.org/319193002/diff/140001/chrome/browser/resource... File chrome/browser/resources/safe_browsing/safe_browsing_v3.js (right): https://codereview.chromium.org/319193002/diff/140001/chrome/browser/resource... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:20: function applyMalwareStyle() { It's confusing that "malware" is used to refer to both types of safebrowsing warnings in various places. https://codereview.chromium.org/319193002/diff/140001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right): https://codereview.chromium.org/319193002/diff/140001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:158: if (base::FieldTrialList::FindFullName("MalwareInterstitialVersion") Doesn't the trial have to be initialized somewhere? https://codereview.chromium.org/319193002/diff/140001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:159: == "V3") { should be a constant? https://codereview.chromium.org/319193002/diff/140001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:1205: } Is it intentional that the new page doesn't show the malware details reporting check box? https://codereview.chromium.org/319193002/diff/140001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:1352: l10n_util::GetStringUTF16(IDS_SAFE_BROWSING_MALWARE_V2_DESCRIPTION2)); is MALWARE here intentional?
https://codereview.chromium.org/319193002/diff/140001/chrome/browser/resource... File chrome/browser/resources/safe_browsing/safe_browsing_v3.js (right): https://codereview.chromium.org/319193002/diff/140001/chrome/browser/resource... chrome/browser/resources/safe_browsing/safe_browsing_v3.js:20: function applyMalwareStyle() { On 2014/06/10 23:37:08, mattm wrote: > It's confusing that "malware" is used to refer to both types of safebrowsing > warnings in various places. I've renamed this (and all the CSS classes) to safe-browsing instead of malware. https://codereview.chromium.org/319193002/diff/140001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right): https://codereview.chromium.org/319193002/diff/140001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:158: if (base::FieldTrialList::FindFullName("MalwareInterstitialVersion") On 2014/06/10 23:37:09, mattm wrote: > Doesn't the trial have to be initialized somewhere? No, this works, I have been testing with it. The initialization happens in a server-side config file that's in google3. https://codereview.chromium.org/319193002/diff/140001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:159: == "V3") { On 2014/06/10 23:37:09, mattm wrote: > should be a constant? Sure. https://codereview.chromium.org/319193002/diff/140001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:1205: } On 2014/06/10 23:37:09, mattm wrote: > Is it intentional that the new page doesn't show the malware details reporting > check box? Yes, the designers were still deciding where to put the checkbox when I wrote this CL. I'll send out another one with the checkbox. https://codereview.chromium.org/319193002/diff/140001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:1352: l10n_util::GetStringUTF16(IDS_SAFE_BROWSING_MALWARE_V2_DESCRIPTION2)); On 2014/06/10 23:37:08, mattm wrote: > is MALWARE here intentional? No, good catch.
What about unittests for the V3 impl?
On 2014/06/11 01:27:31, mattm wrote: > What about unittests for the V3 impl? I haven't written any yet, but this is still behind a flag. I know that tests will be required before taking it out from behind the flag. I'm thinking about how to test it in this CL: https://codereview.chromium.org/326673002/ since Bernhard didn't want the classes to be shared (since that creates the risk of someone accidentally instantiating them), but then I need to figure out if there is a clean way to unit test them. I'll send you a CL with tests tomorrowish.
https://codereview.chromium.org/319193002/diff/160001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right): https://codereview.chromium.org/319193002/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:163: return true; It might be better not to honor the field trial until the feature is more baked, since otherwise someone who is running an old build (or even a current build if the branch happens at a bad time) could get opted-in to an incomplete version of the change.
https://codereview.chromium.org/319193002/diff/160001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right): https://codereview.chromium.org/319193002/diff/160001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:163: return true; On 2014/06/11 02:11:45, mattm wrote: > It might be better not to honor the field trial until the feature is more baked, > since otherwise someone who is running an old build (or even a current build if > the branch happens at a bad time) could get opted-in to an incomplete version of > the change. Good idea, removing the field trial.
lgtm
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/319193002/190008
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/319193002/230001
Message was sent while issue was closed.
Change committed as 276414 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
