Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
664503006:
Implementation of the full clock interstitial. (Closed)
DescriptionImplementation of the full clock interstitial.
This also adds support for launching the Unity date and time panel (needed for Ubuntu 14.04).
The "Update date and time" button doesn't do anything right now on iOS. We will refine the iOS behaviour in a separate CL.
BUG=420813
Committed: https://crrev.com/9292093c4528d728d92808181dff9fb97a0036c0
Cr-Commit-Position: refs/heads/master@{#301205}
Patch Set 1 : #
Total comments: 14
Patch Set 2 : Reuse IDS_SSL_NONOVERRIDABLE_RELOAD_BUTTON for SSL clock error (+ small style changes). #
Total comments: 4
Patch Set 3 : More style changes based on comments. #Patch Set 4 : Prevent "danger" override on clock interstitial. #
Total comments: 5
Patch Set 5 : Improve comments for danger_overridable_. #Patch Set 6 : Changed comment wrapping for Chris. :-) #
Messages
Total messages: 34 (11 generated)
Patchset #1 (id:1) has been deleted
lgarron@chromium.org changed reviewers: + palmer@chromium.org
This CL finishes up palmer@'s work on the clock interstitial, and turns it into a real thing. See https://code.google.com/p/chromium/issues/detail?id=414843 for screenshots. Miscellaneous details: - This also adds support for launching the Unity date and time panel (needed for Ubuntu 14.04). - The "Update date and time" button doesn't do anything right now on iOS. We will refine the iOS behaviour in a separate CL. - On that note, the "Advanced" explanations for Android and iOS have been removed. - The current date format doesn't exactly match the mock ("Tuesday, October 23, 2018 at 8:42:35 AM" instead of "October 23, 2018. 08:42AM"). I did this because base::TimeFormatFriendlyDateAndTime was the closest method I could find, and I didn't want to reconstitute an alternate date+time string from scratch.
Patchset #1 (id:20001) has been deleted
Apparently, you can now launch the date and time applet in iOS 8: http://stackoverflow.com/questions/23824054/how-to-open-settings-programmatic...
Yeah, I saw, and I was going to look into it. The post only indicates that you can open the *Settings* app, though. Getting to Date and Time still requires a click, a scroll, and a click. iOS 8 adoption is also going a bit slow: https://mixpanel.com/trends/#report/ios_8/from_date:-29,report_unit:day,to_da...
lgarron@chromium.org changed reviewers: + bauerb@chromium.org
bauerb: Would you please the review the files in chrome/browser/resources/security_warnings/ ?
felt@chromium.org changed reviewers: + felt@chromium.org
https://codereview.chromium.org/664503006/diff/40001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/664503006/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:9636: + Reload can you combine this with IDS_SSL_NONOVERRIDABLE_RELOAD_BUTTON? https://codereview.chromium.org/664503006/diff/40001/chrome/browser/resources... File chrome/browser/resources/security_warnings/interstitial_v2.js (right): https://codereview.chromium.org/664503006/diff/40001/chrome/browser/resources... chrome/browser/resources/security_warnings/interstitial_v2.js:72: var bodyClass = badClock ? 'bad-clock' : 'ssl'; you could simplify here: $('body').classList.dd(badClock ? 'bad-clock' : 'ssl'); https://codereview.chromium.org/664503006/diff/40001/chrome/browser/resources... chrome/browser/resources/security_warnings/interstitial_v2.js:73: $('body').classList.add(bodyClass); I'm surprised you don't need both ssl and bad-clock in the class list. Is that because only the icon selection relies on ssl being in the body class list? https://codereview.chromium.org/664503006/diff/40001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/664503006/diff/40001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:583: case CMD_PROCEED: { I don't want people to be able to type danger and "proceed". Can you make sure that this just returns if it's showing the clock interstitial? (Or make sure to disable that JS in the interstitial. Either way works.)
https://codereview.chromium.org/664503006/diff/40001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/664503006/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:9636: + Reload On 2014/10/23 01:54:10, felt wrote: > can you combine this with IDS_SSL_NONOVERRIDABLE_RELOAD_BUTTON? Sure. (Any potential concerns the SSL clock interstitial uses this for a reload "link" on the left side of the page, instead of the primary button? Should I possibly change the identifier "IDS_SSL_NONOVERRIDABLE_RELOAD_BUTTON" not to say "BUTTON"?) https://codereview.chromium.org/664503006/diff/40001/chrome/browser/resources... File chrome/browser/resources/security_warnings/interstitial_v2.js (right): https://codereview.chromium.org/664503006/diff/40001/chrome/browser/resources... chrome/browser/resources/security_warnings/interstitial_v2.js:72: var bodyClass = badClock ? 'bad-clock' : 'ssl'; On 2014/10/23 01:54:10, felt wrote: > you could simplify here: $('body').classList.dd(badClock ? 'bad-clock' : 'ssl'); Personally, I prefer not to use ternary operators as function arguments. I can't find anything about this in the style guide, but if you'd really like, I can change it. https://codereview.chromium.org/664503006/diff/40001/chrome/browser/resources... chrome/browser/resources/security_warnings/interstitial_v2.js:73: $('body').classList.add(bodyClass); On 2014/10/23 01:54:10, felt wrote: > I'm surprised you don't need both ssl and bad-clock in the class list. Is that > because only the icon selection relies on ssl being in the body class list? Yep, exactly. (I was surprised, too!) If we expect to add more styling to all SSL interstitials (including bad clock), I wouldn't mind giving them all the `.ssl` class in addition to more specific selectors like `.bad-clock`. https://codereview.chromium.org/664503006/diff/40001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/664503006/diff/40001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:583: case CMD_PROCEED: { On 2014/10/23 01:54:11, felt wrote: > I don't want people to be able to type danger and "proceed". Can you make sure > that this just returns if it's showing the clock interstitial? (Or make sure to > disable that JS in the interstitial. Either way works.) The interstitial JS can't call "proceed". There are a few steps in the way, so it's unlikely to happen by accident: - The `proceed-link` is added by a string for overridable SSL interstitial, and is placed inside the paragraph that shows up when you click "Advanced". The SSL clock interstitial creates a "Reload" link instead of "Advanced". - In addition, the click listener for `proceed-link` that sends SSL_CMD_PROCEED is also only add if the interstitial is "overridable" (but I explicitly set the "override" variable to false for SSL clock interstitials.) I wouldn't mind creating an instance variable to keep track of which interstitial we're in (we've talked about an enum for the different types of interstitials, anyhow), but it would only guard against a little bit of developer ignorance/refactoring accidents.
https://codereview.chromium.org/664503006/diff/40001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/664503006/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:9636: + Reload On 2014/10/23 03:02:04, lgarron wrote: > On 2014/10/23 01:54:10, felt wrote: > > can you combine this with IDS_SSL_NONOVERRIDABLE_RELOAD_BUTTON? > > Sure. > > (Any potential concerns the SSL clock interstitial uses this for a reload "link" > on the left side of the page, instead of the primary button? Should I possibly > change the identifier "IDS_SSL_NONOVERRIDABLE_RELOAD_BUTTON" not to say > "BUTTON"?) Yes, it would make sense to rename it: IDS_SSL_RELOAD, or some such. https://codereview.chromium.org/664503006/diff/40001/chrome/browser/resources... File chrome/browser/resources/security_warnings/interstitial_v2.js (right): https://codereview.chromium.org/664503006/diff/40001/chrome/browser/resources... chrome/browser/resources/security_warnings/interstitial_v2.js:72: var bodyClass = badClock ? 'bad-clock' : 'ssl'; On 2014/10/23 03:02:04, lgarron wrote: > On 2014/10/23 01:54:10, felt wrote: > > you could simplify here: $('body').classList.dd(badClock ? 'bad-clock' : > 'ssl'); > > Personally, I prefer not to use ternary operators as function arguments. > > I can't find anything about this in the style guide, but if you'd really like, I > can change it. It seems odd to define a variable that's only used once. https://codereview.chromium.org/664503006/diff/40001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/664503006/diff/40001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:583: case CMD_PROCEED: { On 2014/10/23 03:02:04, lgarron wrote: > On 2014/10/23 01:54:11, felt wrote: > > I don't want people to be able to type danger and "proceed". Can you make sure > > that this just returns if it's showing the clock interstitial? (Or make sure > to > > disable that JS in the interstitial. Either way works.) > > The interstitial JS can't call "proceed". There are a few steps in the way, so > it's unlikely to happen by accident: > > - The `proceed-link` is added by a string for overridable SSL interstitial, and > is placed inside the paragraph that shows up when you click "Advanced". The SSL > clock interstitial creates a "Reload" link instead of "Advanced". > - In addition, the click listener for `proceed-link` that sends SSL_CMD_PROCEED > is also only add if the interstitial is "overridable" (but I explicitly set the > "override" variable to false for SSL clock interstitials.) > > I wouldn't mind creating an instance variable to keep track of which > interstitial we're in (we've talked about an enum for the different types of > interstitials, anyhow), but it would only guard against a little bit of > developer ignorance/refactoring accidents. handleKeypress is registered, which means that you can type "danger" and it will send the CMD_PROCEED, right?
bauerb@ or palmer@, do you know if Lucas will be able to land the images on his own via another CL with NOTRY=true? Or do images all have to be direct committed, regardless of whether the CQ is involved? (Lucas: trybots don't play nice with binary files like png's. Typically you have to land the images via a separate CL. It's a bit silly.) https://codereview.chromium.org/664503006/diff/60001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/664503006/diff/60001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:441: load_time_data.SetString("tabTitle", nit: to be consistent, can you put "tabTitle" on the next line?
LGTM I'm not sure if the CQ can deal with binary files at all, as it also gets its data out of Rietveld like the trybots do. It would be so much easier if we had Gerrit :) https://codereview.chromium.org/664503006/diff/60001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/664503006/diff/60001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:461: load_time_data.SetString("explanationParagraph", ""); Using the empty std::string() constructor instead of a literal saves some instructions.
https://codereview.chromium.org/664503006/diff/40001/chrome/browser/resources... File chrome/browser/resources/security_warnings/interstitial_v2.js (right): https://codereview.chromium.org/664503006/diff/40001/chrome/browser/resources... chrome/browser/resources/security_warnings/interstitial_v2.js:72: var bodyClass = badClock ? 'bad-clock' : 'ssl'; On 2014/10/23 05:26:22, felt wrote: > On 2014/10/23 03:02:04, lgarron wrote: > > On 2014/10/23 01:54:10, felt wrote: > > > you could simplify here: $('body').classList.dd(badClock ? 'bad-clock' : > > 'ssl'); > > > > Personally, I prefer not to use ternary operators as function arguments. > > > > I can't find anything about this in the style guide, but if you'd really like, > I > > can change it. > > It seems odd to define a variable that's only used once. Hmm, I don't really find it odd. In any case, I've already changed it in the second patch. :-) https://codereview.chromium.org/664503006/diff/40001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/664503006/diff/40001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:583: case CMD_PROCEED: { On 2014/10/23 05:26:22, felt wrote: > On 2014/10/23 03:02:04, lgarron wrote: > > On 2014/10/23 01:54:11, felt wrote: > > > I don't want people to be able to type danger and "proceed". Can you make > sure > > > that this just returns if it's showing the clock interstitial? (Or make sure > > to > > > disable that JS in the interstitial. Either way works.) > > > > The interstitial JS can't call "proceed". There are a few steps in the way, so > > it's unlikely to happen by accident: > > > > - The `proceed-link` is added by a string for overridable SSL interstitial, > and > > is placed inside the paragraph that shows up when you click "Advanced". The > SSL > > clock interstitial creates a "Reload" link instead of "Advanced". > > - In addition, the click listener for `proceed-link` that sends > SSL_CMD_PROCEED > > is also only add if the interstitial is "overridable" (but I explicitly set > the > > "override" variable to false for SSL clock interstitials.) > > > > I wouldn't mind creating an instance variable to keep track of which > > interstitial we're in (we've talked about an enum for the different types of > > interstitials, anyhow), but it would only guard against a little bit of > > developer ignorance/refactoring accidents. > > handleKeypress is registered, which means that you can type "danger" and it will > send the CMD_PROCEED, right? I didn't know about typing "danger". I can look into how to prevent it here. Any reason you should be able to type "danger" for regular nonoverridable SSL errors, but not for this? Is it because the cert might still be incorrect after the clock is fixed, and we don't want those users to make a dangerous override decision based on only seeing the more innocent-sounding error? https://codereview.chromium.org/664503006/diff/60001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/664503006/diff/60001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:441: load_time_data.SetString("tabTitle", On 2014/10/23 05:37:20, felt wrote: > nit: to be consistent, can you put "tabTitle" on the next line? Sure, good catch. https://codereview.chromium.org/664503006/diff/60001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:461: load_time_data.SetString("explanationParagraph", ""); On 2014/10/23 08:42:13, Bernhard Bauer wrote: > Using the empty std::string() constructor instead of a literal saves some > instructions. Good to know. Will do.
Patchset #3 (id:80001) has been deleted
https://codereview.chromium.org/664503006/diff/40001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/664503006/diff/40001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:583: case CMD_PROCEED: { On 2014/10/23 19:37:44, lgarron wrote: > On 2014/10/23 05:26:22, felt wrote: > > On 2014/10/23 03:02:04, lgarron wrote: > > > On 2014/10/23 01:54:11, felt wrote: > > > > I don't want people to be able to type danger and "proceed". Can you make > > sure > > > > that this just returns if it's showing the clock interstitial? (Or make > sure > > > to > > > > disable that JS in the interstitial. Either way works.) > > > > > > The interstitial JS can't call "proceed". There are a few steps in the way, > so > > > it's unlikely to happen by accident: > > > > > > - The `proceed-link` is added by a string for overridable SSL interstitial, > > and > > > is placed inside the paragraph that shows up when you click "Advanced". The > > SSL > > > clock interstitial creates a "Reload" link instead of "Advanced". > > > - In addition, the click listener for `proceed-link` that sends > > SSL_CMD_PROCEED > > > is also only add if the interstitial is "overridable" (but I explicitly set > > the > > > "override" variable to false for SSL clock interstitials.) > > > > > > I wouldn't mind creating an instance variable to keep track of which > > > interstitial we're in (we've talked about an enum for the different types of > > > interstitials, anyhow), but it would only guard against a little bit of > > > developer ignorance/refactoring accidents. > > > > handleKeypress is registered, which means that you can type "danger" and it > will > > send the CMD_PROCEED, right? > > I didn't know about typing "danger". I can look into how to prevent it here. > > Any reason you should be able to type "danger" for regular nonoverridable SSL > errors, but not for this? > Is it because the cert might still be incorrect after the clock is fixed, and we > don't want those users to make a dangerous override decision based on only > seeing the more innocent-sounding error? It's because the normal error warns the user. This doesn't really say much about security, just that your clock is wrong. So since we haven't warned about the risks I don't want it to be overridable.
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
Added a bool to prevent overriding the SSL clock interstitial by typing "danger".
nits https://codereview.chromium.org/664503006/diff/160001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/664503006/diff/160001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:377: danger_overridable_ = true; you should do this in the init block at the top: danger_overridable_(true) https://codereview.chromium.org/664503006/diff/160001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.h (right): https://codereview.chromium.org/664503006/diff/160001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.h:107: const bool overridable_; ^ Can you add a note specifying that this controls whether there is *UI* that makes the error overridable? https://codereview.chromium.org/664503006/diff/160001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.h:109: // the interstitial. This bool can be set to false to prevent the behaviour. ^ Can you explain that this applies regardless of whether the warning is marked "overridable_"?
LGTM mod nits https://codereview.chromium.org/664503006/diff/160001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/664503006/diff/160001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:467: // but we're not using them. So we send blank strings for now. Nit: Wrap/format comment paragraphs, throughout (not just this one).
lgtm
On 2014/10/24 01:47:28, felt wrote: > lgtm P.S. I'll land the images for you either this evening or first thing in the morning.
On 2014/10/24 01:47:45, felt wrote: > On 2014/10/24 01:47:28, felt wrote: > > lgtm > > P.S. I'll land the images for you either this evening or first thing in the > morning. Landed, you should be able to rebase.
https://codereview.chromium.org/664503006/diff/160001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/664503006/diff/160001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:467: // but we're not using them. So we send blank strings for now. On 2014/10/24 00:40:48, Chromium Palmer wrote: > Nit: Wrap/format comment paragraphs, throughout (not just this one). I'd be happy to format my comments properly, but I don't really understand what you mean by this. :-( (I try to wrap comments by picking sensible locations for line breaks. cpplint and the style guide don't seem to give me more guidance about exactly how to do it.)
> I'd be happy to format my comments properly, but I don't really understand
what
> you mean by this. :-(
>
> (I try to wrap comments by picking sensible locations for line breaks. cpplint
> and the style guide don't seem to give me more guidance about exactly how to
do
> it.)
:set textwidth=80 then {gq} in vim; every text editor has some facility for it.
The CQ bit was checked by lgarron@chromium.org
The CQ bit was unchecked by lgarron@chromium.org
The CQ bit was checked by lgarron@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/664503006/200001
Message was sent while issue was closed.
Committed patchset #6 (id:200001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/9292093c4528d728d92808181dff9fb97a0036c0 Cr-Commit-Position: refs/heads/master@{#301205} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
