Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
940543003:
added clock interstitial to chrome://interstitials (Closed)
Descriptionadded clock interstitial to chrome://interstitials
BUG=452218
TEST=Go to chrome://interstitials and click on "Clock is ahead" or "Clock is behind". The shown interstitial displays the clock error message instead of the common certificate warning.
Committed: https://crrev.com/920f3bdfc9fc08cf60da6708a050a9bc82a0d223
Cr-Commit-Position: refs/heads/master@{#318548}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Add the clock interstitial to chrome://interstitials. #
Total comments: 24
Patch Set 3 : added clock interstitial to chrome://interstitials #
Total comments: 6
Patch Set 4 : added clock interstitial to chrome://interstitials #
Total comments: 6
Patch Set 5 : chrome/browser/ssl/ssl_blocking_page.cc #
Total comments: 7
Patch Set 6 : added clock interstitial to chrome://interstitials #
Total comments: 6
Patch Set 7 : added clock interstitial to chrome://interstitials #
Total comments: 4
Patch Set 8 : added clock interstitial to chrome://interstitials #
Messages
Total messages: 53 (13 generated)
fahl@chromium.org changed reviewers: + felt@chromium.org
hey, can you review this for me?
felt@chromium.org changed reviewers: + lgarron@chromium.org
Hey, I have an idea. Lucas, you are learning to review code. Can you do a first pass review here? (I will then review it next.)
https://codereview.chromium.org/940543003/diff/1/chrome/browser/ssl/ssl_block... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/940543003/diff/1/chrome/browser/ssl/ssl_block... chrome/browser/ssl/ssl_blocking_page.cc:208: Nit: I try to avoid introducing extra whitespace, just to make the diffs as minimal and easy to read as possible. https://codereview.chromium.org/940543003/diff/1/chrome/browser/ssl/ssl_block... chrome/browser/ssl/ssl_blocking_page.cc:238: std::string testHost("yourclockiswrong.com"); If we have to hardcode a website, I think we should try to avoid something that can conflict with a real website. It seems the appropriate reserved TLD for this is .example: https://tools.ietf.org/html/rfc2606#section-2 Maybe something like badclock.example? I've spent a while trying to understand if this is needed; if you use the "clock_error=1" URL parameter, does CERT_DATE_INVALID propagate here?
https://codereview.chromium.org/940543003/diff/1/chrome/browser/ssl/ssl_block... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/940543003/diff/1/chrome/browser/ssl/ssl_block... chrome/browser/ssl/ssl_blocking_page.cc:238: std::string testHost("yourclockiswrong.com"); On 2015/02/19 22:57:06, lgarron wrote: > If we have to hardcode a website, I think we should try to avoid something that > can conflict with a real website. > > It seems the appropriate reserved TLD for this is .example: > https://tools.ietf.org/html/rfc2606#section-2 > Maybe something like badclock.example? > > I've spent a while trying to understand if this is needed; if you use the > "clock_error=1" URL parameter, does CERT_DATE_INVALID propagate here? We probably shouldn't be hardcoding anything for a test into production code. Is there a better way to do this?
On 2015/02/19 23:16:07, felt wrote: > https://codereview.chromium.org/940543003/diff/1/chrome/browser/ssl/ssl_block... > File chrome/browser/ssl/ssl_blocking_page.cc (right): > > https://codereview.chromium.org/940543003/diff/1/chrome/browser/ssl/ssl_block... > chrome/browser/ssl/ssl_blocking_page.cc:238: std::string > testHost("yourclockiswrong.com"); > On 2015/02/19 22:57:06, lgarron wrote: > > If we have to hardcode a website, I think we should try to avoid something > that > > can conflict with a real website. > > > > It seems the appropriate reserved TLD for this is .example: > > https://tools.ietf.org/html/rfc2606#section-2 > > Maybe something like badclock.example? > > > > I've spent a while trying to understand if this is needed; if you use the > > "clock_error=1" URL parameter, does CERT_DATE_INVALID propagate here? > > We probably shouldn't be hardcoding anything for a test into production code. Is > there a better way to do this? We were just discussing that. I'll try a new enum as a constructor's parameter for SSLBlockingPage. That would allow us to add further test cases in the future. Does that sound ok for you?
On 2015/02/19 23:26:09, fahl wrote: > On 2015/02/19 23:16:07, felt wrote: > > > https://codereview.chromium.org/940543003/diff/1/chrome/browser/ssl/ssl_block... > > File chrome/browser/ssl/ssl_blocking_page.cc (right): > > > > > https://codereview.chromium.org/940543003/diff/1/chrome/browser/ssl/ssl_block... > > chrome/browser/ssl/ssl_blocking_page.cc:238: std::string > > testHost("yourclockiswrong.com"); > > On 2015/02/19 22:57:06, lgarron wrote: > > > If we have to hardcode a website, I think we should try to avoid something > > that > > > can conflict with a real website. > > > > > > It seems the appropriate reserved TLD for this is .example: > > > https://tools.ietf.org/html/rfc2606#section-2 > > > Maybe something like badclock.example? > > > > > > I've spent a while trying to understand if this is needed; if you use the > > > "clock_error=1" URL parameter, does CERT_DATE_INVALID propagate here? > > > > We probably shouldn't be hardcoding anything for a test into production code. > Is > > there a better way to do this? > > We were just discussing that. I'll try a new enum as a constructor's parameter > for SSLBlockingPage. That would allow us to add further test cases in the > future. Does that sound ok for you? What would the enum be for?
On 2015/02/19 23:27:32, felt wrote: > On 2015/02/19 23:26:09, fahl wrote: > > On 2015/02/19 23:16:07, felt wrote: > > > > > > https://codereview.chromium.org/940543003/diff/1/chrome/browser/ssl/ssl_block... > > > File chrome/browser/ssl/ssl_blocking_page.cc (right): > > > > > > > > > https://codereview.chromium.org/940543003/diff/1/chrome/browser/ssl/ssl_block... > > > chrome/browser/ssl/ssl_blocking_page.cc:238: std::string > > > testHost("yourclockiswrong.com"); > > > On 2015/02/19 22:57:06, lgarron wrote: > > > > If we have to hardcode a website, I think we should try to avoid something > > > that > > > > can conflict with a real website. > > > > > > > > It seems the appropriate reserved TLD for this is .example: > > > > https://tools.ietf.org/html/rfc2606#section-2 > > > > Maybe something like badclock.example? > > > > > > > > I've spent a while trying to understand if this is needed; if you use the > > > > "clock_error=1" URL parameter, does CERT_DATE_INVALID propagate here? > > > > > > We probably shouldn't be hardcoding anything for a test into production > code. > > Is > > > there a better way to do this? > > > > We were just discussing that. I'll try a new enum as a constructor's parameter > > for SSLBlockingPage. That would allow us to add further test cases in the > > future. Does that sound ok for you? > > What would the enum be for? It would be for calling an overloaded constructor of SSLBlockingPage to signal interstitial test cases, e.g. CLOCK_PAST, CLOCK_AHEAD or whatever you come up with in the future.
On 2015/02/19 23:37:29, fahl wrote: > On 2015/02/19 23:27:32, felt wrote: > > On 2015/02/19 23:26:09, fahl wrote: > > > On 2015/02/19 23:16:07, felt wrote: > > > > > > > > > > https://codereview.chromium.org/940543003/diff/1/chrome/browser/ssl/ssl_block... > > > > File chrome/browser/ssl/ssl_blocking_page.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/940543003/diff/1/chrome/browser/ssl/ssl_block... > > > > chrome/browser/ssl/ssl_blocking_page.cc:238: std::string > > > > testHost("yourclockiswrong.com"); > > > > On 2015/02/19 22:57:06, lgarron wrote: > > > > > If we have to hardcode a website, I think we should try to avoid > something > > > > that > > > > > can conflict with a real website. > > > > > > > > > > It seems the appropriate reserved TLD for this is .example: > > > > > https://tools.ietf.org/html/rfc2606#section-2 > > > > > Maybe something like badclock.example? > > > > > > > > > > I've spent a while trying to understand if this is needed; if you use > the > > > > > "clock_error=1" URL parameter, does CERT_DATE_INVALID propagate here? > > > > > > > > We probably shouldn't be hardcoding anything for a test into production > > code. > > > Is > > > > there a better way to do this? > > > > > > We were just discussing that. I'll try a new enum as a constructor's > parameter > > > for SSLBlockingPage. That would allow us to add further test cases in the > > > future. Does that sound ok for you? > > > > What would the enum be for? > > It would be for calling an overloaded constructor of SSLBlockingPage to signal > interstitial test cases, e.g. CLOCK_PAST, CLOCK_AHEAD or whatever you come up > with in the future. Should be working now, added the current time as an instance variable to SSLBlockingPage objects.
looking good, just a few small changes https://codereview.chromium.org/940543003/diff/1/chrome/browser/ssl/ssl_block... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/940543003/diff/1/chrome/browser/ssl/ssl_block... chrome/browser/ssl/ssl_blocking_page.cc:208: On 2015/02/19 22:57:06, lgarron wrote: > Nit: I try to avoid introducing extra whitespace, just to make the diffs as > minimal and easy to read as possible. looks like you haven't addressed this? https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:298: // base::Time now = base::Time::NowFromSystemTime(); why are these lines here but commented out? https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:430: load_time_data->SetString("currentDate", nit: i liked putting all of these on the next line so that you can easily scan down and see all of the string names in parallel form https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.h (right): https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.h:123: // calculations nit: "...use this for all time-based calculations" https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.h:124: const base::Time base_time_; nit: "time_shown_" might be more descriptive? https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right): https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:138: } nit: why are you reformatting this? https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:221: nit: same question here, generally it's best to leave formatting of unrelated parts of code alone unless it's obviously wrong
Looks pretty good overall! Adding a bunch of nitpicks on top of felt@'s. ;-) https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:208: Nit: Remove all blank lines until the diff is semantically awesome. :-) https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:298: // base::Time now = base::Time::NowFromSystemTime(); On 2015/02/20 at 16:01:53, felt wrote: > why are these lines here but commented out? I think we've removed the need for these; I would just take out the comment and the two lines for this. https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:337: "primaryParagraph", I see you're using `git cl format`? :-D https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:337: "primaryParagraph", I see you're using `git cl format`? :-D https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:430: load_time_data->SetString("currentDate", On 2015/02/20 at 16:01:53, felt wrote: > nit: i liked putting all of these on the next line so that you can easily scan down and see all of the string names in parallel form I suggested Sascha use `git cl format`. In case he did, it might have caused this. https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.h (right): https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.h:124: const base::Time base_time_; On 2015/02/20 at 16:01:53, felt wrote: > nit: "time_shown_" might be more descriptive? time_shown_ sounds like it's about the UI (instead of the logic) to me. How about something like time_triggered_? https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right): https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:77: if (net::GetValueForKeyInQuery(web_contents->GetURL(), "clock_manipulation", I like the idea of being able to scale the number of years! What about naming it something more self-documenting like "year_offset"? https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:81: base::TimeDelta::FromDays(365 * atoi(clock_manipulation_param.c_str())); felt@: Do you know if there's a more semantic way to convert URL parameters to integers? https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:204: "href='ssl?clock_error=1&clock_manipulation=1&url=https://" We can remove the URL parameter now, right?
Could you check my comments? https://codereview.chromium.org/940543003/diff/1/chrome/browser/ssl/ssl_block... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/940543003/diff/1/chrome/browser/ssl/ssl_block... chrome/browser/ssl/ssl_blocking_page.cc:208: On 2015/02/20 16:01:53, felt wrote: > On 2015/02/19 22:57:06, lgarron wrote: > > Nit: I try to avoid introducing extra whitespace, just to make the diffs as > > minimal and easy to read as possible. > > looks like you haven't addressed this? Acknowledged. https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:298: // base::Time now = base::Time::NowFromSystemTime(); On 2015/02/20 16:01:53, felt wrote: > why are these lines here but commented out? Acknowledged. https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:298: // base::Time now = base::Time::NowFromSystemTime(); On 2015/02/20 22:12:46, lgarron wrote: > On 2015/02/20 at 16:01:53, felt wrote: > > why are these lines here but commented out? > > I think we've removed the need for these; I would just take out the comment and > the two lines for this. Acknowledged. https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:430: load_time_data->SetString("currentDate", On 2015/02/20 16:01:53, felt wrote: > nit: i liked putting all of these on the next line so that you can easily scan > down and see all of the string names in parallel form Acknowledged. https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.h (right): https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.h:123: // calculations On 2015/02/20 16:01:53, felt wrote: > nit: "...use this for all time-based calculations" Acknowledged. https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.h:124: const base::Time base_time_; On 2015/02/20 16:01:53, felt wrote: > nit: "time_shown_" might be more descriptive? hmm...although this time value is shown, its actual purpose is to be used as the base for interstitial calculations. what about "time_interstitial_"? https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.h:124: const base::Time base_time_; On 2015/02/20 22:12:46, lgarron wrote: > On 2015/02/20 at 16:01:53, felt wrote: > > nit: "time_shown_" might be more descriptive? > > time_shown_ sounds like it's about the UI (instead of the logic) to me. How > about something like time_triggered_? Did we agree on a variable name? https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right): https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:138: } On 2015/02/20 16:01:53, felt wrote: > nit: why are you reformatting this? Acknowledged. https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:204: "href='ssl?clock_error=1&clock_manipulation=1&url=https://" On 2015/02/20 22:12:46, lgarron wrote: > We can remove the URL parameter now, right? Acknowledged. https://codereview.chromium.org/940543003/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:221: On 2015/02/20 16:01:53, felt wrote: > nit: same question here, generally it's best to leave formatting of unrelated > parts of code alone unless it's obviously wrong Acknowledged.
LGTM - but in typical Chromium dev style, with a few remaining style nitpicks. ;-) https://codereview.chromium.org/940543003/diff/40001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/940543003/diff/40001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:236: time_triggered_(base_time) { I would make the constructor argument and the instance variable have the same name (apart from the underscore). https://codereview.chromium.org/940543003/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right): https://codereview.chromium.org/940543003/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:140: } // namespace Nit: still an uneeded extra space in the diff. https://codereview.chromium.org/940543003/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:215: Also an extra space.
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
By the way, make sure to update the TEST string on this CR (in the online interface), so that you don't end up with outdated testing info in the commit message.
On 2015/02/24 20:06:09, lgarron wrote: > By the way, make sure to update the TEST string on this CR (in the online > interface), so that you don't end up with outdated testing info in the commit > message. Sascha, once you address lgarron's last comments, post your response "Done, Done, Done, etc." and I'll review again.
On 2015/02/24 21:15:39, felt wrote: > On 2015/02/24 20:06:09, lgarron wrote: > > By the way, make sure to update the TEST string on this CR (in the online > > interface), so that you don't end up with outdated testing info in the commit > > message. > > Sascha, once you address lgarron's last comments, post your response "Done, > Done, Done, etc." and I'll review again. I marked lgarron's comments as done and uploaded the last cl.
On 2015/02/24 21:19:18, fahl wrote: > On 2015/02/24 21:15:39, felt wrote: > > On 2015/02/24 20:06:09, lgarron wrote: > > > By the way, make sure to update the TEST string on this CR (in the online > > > interface), so that you don't end up with outdated testing info in the > commit > > > message. > > > > Sascha, once you address lgarron's last comments, post your response "Done, > > Done, Done, etc." and I'll review again. > > I marked lgarron's comments as done and uploaded the last cl. Hmm, did you remember to mail your comments? I don't see a response after lgarron's last e-mail (with the l-g-t-m)
https://codereview.chromium.org/940543003/diff/40001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/940543003/diff/40001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_blocking_page.cc:236: time_triggered_(base_time) { On 2015/02/24 02:28:37, lgarron wrote: > I would make the constructor argument and the instance variable have the same > name (apart from the underscore). Done. https://codereview.chromium.org/940543003/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right): https://codereview.chromium.org/940543003/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:140: } // namespace On 2015/02/24 02:28:37, lgarron wrote: > Nit: still an uneeded extra space in the diff. Done. https://codereview.chromium.org/940543003/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:215: On 2015/02/24 02:28:37, lgarron wrote: > Also an extra space. Done.
lgtm
felt@chromium.org changed reviewers: + bauerb@chromium.org
bauerb@chromium.org: Please review changes in chrome/browser/ui/webui/interstitials/
https://codereview.chromium.org/940543003/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right): https://codereview.chromium.org/940543003/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:81: base::TimeDelta::FromDays(365 * atoi(clock_manipulation_param.c_str())); There is base::StringToInt (from base/strings/string_number_conversions.h). https://codereview.chromium.org/940543003/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:196: html = "<html><head><title>Interstitials</title></head>" Would it make sense to put this into chrome/browser/resources/?
https://codereview.chromium.org/940543003/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right): https://codereview.chromium.org/940543003/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:196: html = "<html><head><title>Interstitials</title></head>" On 2015/02/25 16:41:08, Bernhard (back on Feb 26) wrote: > Would it make sense to put this into chrome/browser/resources/? I think that probably makes sense. What's felt's and lgarron's opinion on this?
https://codereview.chromium.org/940543003/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right): https://codereview.chromium.org/940543003/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:196: html = "<html><head><title>Interstitials</title></head>" On 2015/02/26 03:00:26, fahl wrote: > On 2015/02/25 16:41:08, Bernhard (back on Feb 26) wrote: > > Would it make sense to put this into chrome/browser/resources/? > > I think that probably makes sense. What's felt's and lgarron's opinion on this? Sounds fine to me.
Patchset #5 (id:140001) has been deleted
Patchset #5 (id:160001) has been deleted
https://codereview.chromium.org/940543003/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right): https://codereview.chromium.org/940543003/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:81: base::TimeDelta::FromDays(365 * atoi(clock_manipulation_param.c_str())); On 2015/02/25 16:41:08, Bernhard (back on Feb 26) wrote: > There is base::StringToInt (from base/strings/string_number_conversions.h). Done. https://codereview.chromium.org/940543003/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:196: html = "<html><head><title>Interstitials</title></head>" On 2015/02/26 03:14:00, felt wrote: > On 2015/02/26 03:00:26, fahl wrote: > > On 2015/02/25 16:41:08, Bernhard (back on Feb 26) wrote: > > > Would it make sense to put this into chrome/browser/resources/? > > > > I think that probably makes sense. What's felt's and lgarron's opinion on > this? > > Sounds fine to me. Done.
https://codereview.chromium.org/940543003/diff/180001/chrome/browser/resource... File chrome/browser/resources/security_warnings/interstitial_ui.html (right): https://codereview.chromium.org/940543003/diff/180001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_ui.html:2: <head> nit: can you pls reformat this so it doesn't use 6-space tabs anymore?
LGTM w/ nits: https://codereview.chromium.org/940543003/diff/180001/chrome/browser/resource... File chrome/browser/resources/security_warnings/interstitial_ui.html (right): https://codereview.chromium.org/940543003/diff/180001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_ui.html:2: <head> On 2015/02/26 05:26:13, felt wrote: > nit: can you pls reformat this so it doesn't use 6-space tabs anymore? In addition, I would probably not indent the contents of the outer <html> tag at all. https://codereview.chromium.org/940543003/diff/180001/chrome/browser/ui/webui... File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right): https://codereview.chromium.org/940543003/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:85: time_triggered_ += base::TimeDelta::FromDays(365*2); Add spaces around the asterisk? https://codereview.chromium.org/940543003/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:205: .GetRawDataResource(IDR_SECURITY_INTERSTITIAL_UI_HTML) Nit: Alignment
https://codereview.chromium.org/940543003/diff/180001/chrome/browser/resource... File chrome/browser/resources/security_warnings/interstitial_ui.html (right): https://codereview.chromium.org/940543003/diff/180001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_ui.html:2: <head> On 2015/02/26 05:26:13, felt wrote: > nit: can you pls reformat this so it doesn't use 6-space tabs anymore? Done. https://codereview.chromium.org/940543003/diff/180001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_ui.html:2: <head> On 2015/02/26 05:26:13, felt wrote: > nit: can you pls reformat this so it doesn't use 6-space tabs anymore? Done. https://codereview.chromium.org/940543003/diff/180001/chrome/browser/ui/webui... File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right): https://codereview.chromium.org/940543003/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:85: time_triggered_ += base::TimeDelta::FromDays(365*2); On 2015/02/26 14:21:44, Bernhard (back on Feb 26) wrote: > Add spaces around the asterisk? Done.
On 2015/02/26 17:33:19, fahl wrote: > https://codereview.chromium.org/940543003/diff/180001/chrome/browser/resource... > File chrome/browser/resources/security_warnings/interstitial_ui.html (right): > > https://codereview.chromium.org/940543003/diff/180001/chrome/browser/resource... > chrome/browser/resources/security_warnings/interstitial_ui.html:2: <head> > On 2015/02/26 05:26:13, felt wrote: > > nit: can you pls reformat this so it doesn't use 6-space tabs anymore? > > Done. > > https://codereview.chromium.org/940543003/diff/180001/chrome/browser/resource... > chrome/browser/resources/security_warnings/interstitial_ui.html:2: <head> > On 2015/02/26 05:26:13, felt wrote: > > nit: can you pls reformat this so it doesn't use 6-space tabs anymore? > > Done. > > https://codereview.chromium.org/940543003/diff/180001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right): > > https://codereview.chromium.org/940543003/diff/180001/chrome/browser/ui/webui... > chrome/browser/ui/webui/interstitials/interstitial_ui.cc:85: time_triggered_ += > base::TimeDelta::FromDays(365*2); > On 2015/02/26 14:21:44, Bernhard (back on Feb 26) wrote: > > Add spaces around the asterisk? > > Done. Sascha, FYI you now need to add a reviewer for the one last file.
fahl@chromium.org changed reviewers: + rsleevi@chromium.org
@sleevi, could you please review my change to chrome/browser/browser_resources.grd?
https://codereview.chromium.org/940543003/diff/200001/chrome/browser/resource... File chrome/browser/resources/security_warnings/interstitial_ui.html (right): https://codereview.chromium.org/940543003/diff/200001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_ui.html:2: <head> Unindent this as well if you unindent the <body>. https://codereview.chromium.org/940543003/diff/200001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_ui.html:10: <a href='ssl?overridable=1&strict_enforcement=0'>example.com (Overridable)</a><br> I think the </a> hits exactly 80 columns, so just put the <br> on a new line? :)
https://codereview.chromium.org/940543003/diff/200001/chrome/browser/resource... File chrome/browser/resources/security_warnings/interstitial_ui.html (right): https://codereview.chromium.org/940543003/diff/200001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_ui.html:2: <head> On 2015/02/26 18:35:43, Bernhard (back on Feb 26) wrote: > Unindent this as well if you unindent the <body>. Should chrome/browser/resources/security_warnings/interstitial_v2.html be reformatted as well? ;) https://codereview.chromium.org/940543003/diff/200001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_ui.html:2: <head> On 2015/02/26 18:35:43, Bernhard (back on Feb 26) wrote: > Unindent this as well if you unindent the <body>. Done. https://codereview.chromium.org/940543003/diff/200001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_ui.html:10: <a href='ssl?overridable=1&strict_enforcement=0'>example.com (Overridable)</a><br> On 2015/02/26 18:35:43, Bernhard (back on Feb 26) wrote: > I think the </a> hits exactly 80 columns, so just put the <br> on a new line? :) Done.
https://codereview.chromium.org/940543003/diff/200001/chrome/browser/resource... File chrome/browser/resources/security_warnings/interstitial_ui.html (right): https://codereview.chromium.org/940543003/diff/200001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_ui.html:2: <head> On 2015/02/26 18:50:49, fahl wrote: > On 2015/02/26 18:35:43, Bernhard (back on Feb 26) wrote: > > Unindent this as well if you unindent the <body>. > > Should chrome/browser/resources/security_warnings/interstitial_v2.html be > reformatted as well? ;) Feel free to fix that also, but please do it in a separate CL!
I'm not an owner for that file but... I gave you some nits instead :) https://codereview.chromium.org/940543003/diff/220001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.h (right): https://codereview.chromium.org/940543003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.h:74: const base::Time& time_triggered); We tend to structure it so that callbacks are the last parameter of a method (since they're akin to output parameters in some ways). It's a pedantic nit, but could you switch the order of these last two parameters? https://codereview.chromium.org/940543003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.h:122: // The interstitial is supposed to use this for all time-based calculations As a comment/description, this doesn't really indicate what it contains, or how to use it properly. // The time at which the interstitial was triggered. The interstitial // calculates all times relative to this. (Or whtaever it does; I'm just making things up)
Patchset #8 (id:240001) has been deleted
On 2015/02/27 00:26:39, Ryan Sleevi wrote: > I'm not an owner for that file > > but... I gave you some nits instead :) > > https://codereview.chromium.org/940543003/diff/220001/chrome/browser/ssl/ssl_... > File chrome/browser/ssl/ssl_blocking_page.h (right): > > https://codereview.chromium.org/940543003/diff/220001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_blocking_page.h:74: const base::Time& time_triggered); > We tend to structure it so that callbacks are the last parameter of a method > (since they're akin to output parameters in some ways). It's a pedantic nit, but > could you switch the order of these last two parameters? > > https://codereview.chromium.org/940543003/diff/220001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_blocking_page.h:122: // The interstitial is supposed to > use this for all time-based calculations > As a comment/description, this doesn't really indicate what it contains, or how > to use it properly. > > // The time at which the interstitial was triggered. The interstitial > // calculates all times relative to this. > > (Or whtaever it does; I'm just making things up) +thestig for browser_resources.grd
fahl@chromium.org changed reviewers: + thestig@chromium.org - rsleevi@chromium.org
+thestig for browser_resources.grd https://codereview.chromium.org/940543003/diff/220001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.h (right): https://codereview.chromium.org/940543003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.h:74: const base::Time& time_triggered); On 2015/02/27 00:26:39, Ryan Sleevi wrote: > We tend to structure it so that callbacks are the last parameter of a method > (since they're akin to output parameters in some ways). It's a pedantic nit, but > could you switch the order of these last two parameters? Done. https://codereview.chromium.org/940543003/diff/220001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.h:122: // The interstitial is supposed to use this for all time-based calculations On 2015/02/27 00:26:39, Ryan Sleevi wrote: > As a comment/description, this doesn't really indicate what it contains, or how > to use it properly. > > // The time at which the interstitial was triggered. The interstitial > // calculates all times relative to this. > > (Or whtaever it does; I'm just making things up) Done.
lgtm
Now if you check the box next to "Commit" it will send it to the CQ and off you go!
The CQ bit was checked by fahl@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lgarron@chromium.org, felt@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/940543003/#ps260001 (title: "added clock interstitial to chrome://interstitials")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/940543003/260001
Message was sent while issue was closed.
Committed patchset #8 (id:260001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/920f3bdfc9fc08cf60da6708a050a9bc82a0d223 Cr-Commit-Position: refs/heads/master@{#318548}
Message was sent while issue was closed.
On 2015/02/27 23:47:01, I haz the power (commit-bot) wrote: > Patchset 8 (id:??) landed as > https://crrev.com/920f3bdfc9fc08cf60da6708a050a9bc82a0d223 > Cr-Commit-Position: refs/heads/master@{#318548} congrats!! first CL! :) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
