Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
336273002:
Suggest upgrading to SP3 or later for invalid certificate errors. (Closed)
|
Created:
6 years, 6 months ago by radhikabhar Modified:
6 years, 5 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionSuggest upgrading to SP3 or later for invalid certificate errors.
BUG=349655
TEST=Find a windows xp sp2 machine, install chrome, go to twitter.com, verify the message looks right
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285481
Patch Set 1 : Suggest upgrading to SP3 #Patch Set 2 : Changes #Patch Set 3 : Changes #
Total comments: 22
Patch Set 4 : Get the version using OSInfo #
Total comments: 2
Patch Set 5 : Addressed comments #
Total comments: 5
Patch Set 6 : Modified to include process #Patch Set 7 : Rebase-update #Patch Set 8 : Moved the method to ssl_error_classiffication #Patch Set 9 : Added for Server Pack 2003 #Patch Set 10 : Fixed complilation errors #
Total comments: 2
Patch Set 11 : Addressed comments #Patch Set 12 : Changed to chromium_strings.grd #
Total comments: 2
Patch Set 13 : Addressed comments #
Total comments: 2
Patch Set 14 : Changed to impostor #Patch Set 15 : Reverted back #
Messages
Total messages: 41 (0 generated)
Uploaded the CL
https://codereview.chromium.org/336273002/diff/320001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/336273002/diff/320001/chrome/app/generated_re... chrome/app/generated_resources.grd:9362: + Your computer is running an old version of Microsoft Windows that cannot handle this website's security certificate. Because of this problem, Chrome can't tell whether the certificate came <ph name="SITE">$1<ex>google.com</ex></ph> or from someone on your network pretending to be <ph name="SITE">$1<ex>google.com</ex></ph>. Please update your computer to Windows XP SP3 or higher. did you mean "came from"? https://codereview.chromium.org/336273002/diff/320001/chrome/browser/resource... File chrome/browser/resources/ssl/interstitial_v2.js (right): https://codereview.chromium.org/336273002/diff/320001/chrome/browser/resource... chrome/browser/resources/ssl/interstitial_v2.js:41: } else if ($('help-link')) { this is included in the other CL, right? so once that one lands and you rebase, this change will leave this CL https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (left): https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:545: any particular reason why you're deleting this line? https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:563: if (type == SSLErrorInfo::CERT_INVALID) { it seems like this would be simpler as: if (type == SSLErrorInfo::CERT_INVALID && WindowsVersionSP3Lower()) { ... } else { ... } instead of doing two separate comparisons https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:568: IDS_SSL_NONOVERRIDABLE_MORE_INVALID_SP3,url) : nit: you need a space here IDS_SSL_NONOVERRIDABLE_MORE_INVALID_SP3, url https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:775: bool on_windows_xp = base::win::GetVersion() < base::win::VERSION_VISTA; is it correct that this includes VERSION_SERVER_2003? https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:782: return false; I don't think you need "return false" twice https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.h (right): https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.h:69: static bool WindowsVersionSP3Lower(); This only needs to be invoked outside of the class by the unit test, right? It would be better to make it a private method, and then friend the unit test using FRIEND_TEST_ALL_PREFIXES. Here's an example that does this: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:778: GetVersionEx(reinterpret_cast<OSVERSIONINFO*>(&version_info)); I don't think you should be directly invoking ::GetVersionEx. Instead, it would be better for you to use the OSInfo class: https://code.google.com/p/chromium/codesearch#chromium/src/base/win/windows_v... https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.h (right): https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.h:17: #include "ssl_error_info.h" do you need this in the .h file? https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page_unittest.cc (right): https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page_unittest.cc:15: TEST(SSLBlockingPageTest, SSLBlockingPageWindowsVersionTest) { This test won't get coverage because we don't have bots that will cover all the cases. Instead, it would make more sense to use OSInfo from https://code.google.com/p/chromium/codesearch#chromium/src/base/win/windows_v... and then mock out the version() and service_pack() calls using EXPECT_CALL. See https://code.google.com/p/googlemock/wiki/ForDummies. That way you can make it act like various different versions and see if the code works as expected.
https://codereview.chromium.org/336273002/diff/320001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/336273002/diff/320001/chrome/app/generated_re... chrome/app/generated_resources.grd:9362: + Your computer is running an old version of Microsoft Windows that cannot handle this website's security certificate. Because of this problem, Chrome can't tell whether the certificate came <ph name="SITE">$1<ex>google.com</ex></ph> or from someone on your network pretending to be <ph name="SITE">$1<ex>google.com</ex></ph>. Please update your computer to Windows XP SP3 or higher. On 2014/06/30 18:16:07, felt wrote: > did you mean "came from"? Done. https://codereview.chromium.org/336273002/diff/320001/chrome/browser/resource... File chrome/browser/resources/ssl/interstitial_v2.js (right): https://codereview.chromium.org/336273002/diff/320001/chrome/browser/resource... chrome/browser/resources/ssl/interstitial_v2.js:41: } else if ($('help-link')) { On 2014/06/30 18:16:07, felt wrote: > this is included in the other CL, right? so once that one lands and you rebase, > this change will leave this CL Yes that's right. https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:563: if (type == SSLErrorInfo::CERT_INVALID) { On 2014/06/30 18:16:07, felt wrote: > it seems like this would be simpler as: > > if (type == SSLErrorInfo::CERT_INVALID && WindowsVersionSP3Lower()) { > ... > } else { > ... > } > > instead of doing two separate comparisons Done. https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:568: IDS_SSL_NONOVERRIDABLE_MORE_INVALID_SP3,url) : On 2014/06/30 18:16:07, felt wrote: > nit: you need a space here > > IDS_SSL_NONOVERRIDABLE_MORE_INVALID_SP3, url Done. https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:775: bool on_windows_xp = base::win::GetVersion() < base::win::VERSION_VISTA; On 2014/06/30 18:16:07, felt wrote: > is it correct that this includes VERSION_SERVER_2003? Changed it so that it directly compares with the version https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:778: GetVersionEx(reinterpret_cast<OSVERSIONINFO*>(&version_info)); On 2014/06/30 18:36:15, felt wrote: > I don't think you should be directly invoking ::GetVersionEx. Instead, it would > be better for you to use the OSInfo class: > https://code.google.com/p/chromium/codesearch#chromium/src/base/win/windows_v... Done. https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:782: return false; On 2014/06/30 18:16:07, felt wrote: > I don't think you need "return false" twice Done. https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page_unittest.cc (right): https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page_unittest.cc:15: TEST(SSLBlockingPageTest, SSLBlockingPageWindowsVersionTest) { On 2014/06/30 18:36:15, felt wrote: > This test won't get coverage because we don't have bots that will cover all the > cases. > > Instead, it would make more sense to use OSInfo from > https://code.google.com/p/chromium/codesearch#chromium/src/base/win/windows_v... > and then mock out the version() and service_pack() calls using EXPECT_CALL. See > https://code.google.com/p/googlemock/wiki/ForDummies. That way you can make it > act like various different versions and see if the code works as expected Should I change it to get the OS info from OSInfo. I did not do that because I would be using the same logic so I am not sure how much that will make sense.
A few suggestions. https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:775: bool on_windows_xp = base::win::GetVersion() < base::win::VERSION_VISTA; On 2014/07/01 23:56:49, radhikabhar wrote: > On 2014/06/30 18:16:07, felt wrote: > > is it correct that this includes VERSION_SERVER_2003? > > Changed it so that it directly compares with the version I can see it going either way. I don't know anything about VERSION_SERVER_2003, but there is something in the enum that makes it sound like maybe it *should* apply to VERSION_SERVER_2003 too. Can you ask wfh? I suspect he would know. https://codereview.chromium.org/336273002/diff/360001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/336273002/diff/360001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:773: if (os_info->version() == base::win::VERSION_XP) { nit: could you combine this into a single if-statement? something like if (os_info->version() == base::win::VERSION_XP && os_info->service_pack().major < 3)
https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (left): https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:545: On 2014/06/30 18:16:07, felt wrote: > any particular reason why you're deleting this line? Done. https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:775: bool on_windows_xp = base::win::GetVersion() < base::win::VERSION_VISTA; On 2014/07/02 18:11:57, felt wrote: > On 2014/07/01 23:56:49, radhikabhar wrote: > > On 2014/06/30 18:16:07, felt wrote: > > > is it correct that this includes VERSION_SERVER_2003? > > > > Changed it so that it directly compares with the version > > I can see it going either way. I don't know anything about VERSION_SERVER_2003, > but there is something in the enum that makes it sound like maybe it *should* > apply to VERSION_SERVER_2003 too. Can you ask wfh? I suspect he would know. Windows Server 2003 does not have support for SHA2 but it can be downloaded. This link gives the details http://blogs.technet.com/b/pki/archive/2010/09/30/sha2-and-windows.aspx. I have asked @wfh also and waiting for his reply. https://codereview.chromium.org/336273002/diff/360001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/336273002/diff/360001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:773: if (os_info->version() == base::win::VERSION_XP) { On 2014/07/02 18:11:58, felt wrote: > nit: could you combine this into a single if-statement? something like > > if (os_info->version() == base::win::VERSION_XP && os_info->service_pack().major > < 3) Done.
On 2014/07/02 18:51:09, radhikabhar wrote: > https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... > File chrome/browser/ssl/ssl_blocking_page.cc (left): > > https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_blocking_page.cc:545: > On 2014/06/30 18:16:07, felt wrote: > > any particular reason why you're deleting this line? > > Done. > > https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... > File chrome/browser/ssl/ssl_blocking_page.cc (right): > > https://codereview.chromium.org/336273002/diff/320001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_blocking_page.cc:775: bool on_windows_xp = > base::win::GetVersion() < base::win::VERSION_VISTA; > On 2014/07/02 18:11:57, felt wrote: > > On 2014/07/01 23:56:49, radhikabhar wrote: > > > On 2014/06/30 18:16:07, felt wrote: > > > > is it correct that this includes VERSION_SERVER_2003? > > > > > > Changed it so that it directly compares with the version > > > > I can see it going either way. I don't know anything about > VERSION_SERVER_2003, > > but there is something in the enum that makes it sound like maybe it *should* > > apply to VERSION_SERVER_2003 too. Can you ask wfh? I suspect he would know. > > > Windows Server 2003 does not have support for SHA2 but it can be downloaded. > This link gives the details > http://blogs.technet.com/b/pki/archive/2010/09/30/sha2-and-windows.aspx. > I have asked @wfh also and waiting for his reply. > > https://codereview.chromium.org/336273002/diff/360001/chrome/browser/ssl/ssl_... > File chrome/browser/ssl/ssl_blocking_page.cc (right): > > https://codereview.chromium.org/336273002/diff/360001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_blocking_page.cc:773: if (os_info->version() == > base::win::VERSION_XP) { > On 2014/07/02 18:11:58, felt wrote: > > nit: could you combine this into a single if-statement? something like > > > > if (os_info->version() == base::win::VERSION_XP && > os_info->service_pack().major > > < 3) > > Done. @palmer : Could you please review this CL. Do you have any ideas about how to go about with the unit_tests as currently they are not giving enough coverage and using mocks is non-trivial as the functions that I will be mocking are non-virtual.
LGTM. As for testing: Since we don't (?) have any test bots running < SP 3, it might be that the only way to test this is manually. https://codereview.chromium.org/336273002/diff/380001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/336273002/diff/380001/chrome/app/generated_re... chrome/app/generated_resources.grd:9362: + Your computer is running an old version of Microsoft Windows that cannot handle this website's security certificate. Because of this problem, Chrome can't tell whether the certificate came from <ph name="SITE">$1<ex>google.com</ex></ph> or from someone on your network pretending to be <ph name="SITE">$1<ex>google.com</ex></ph>. Please update your computer to Windows XP SP3 or higher. I hypothesize that "cannot process" will translate better than "cannot handle". https://codereview.chromium.org/336273002/diff/380001/chrome/browser/resource... File chrome/browser/resources/ssl/interstitial_v2.js (right): https://codereview.chromium.org/336273002/diff/380001/chrome/browser/resource... chrome/browser/resources/ssl/interstitial_v2.js:41: } else if ($('help-link')) { This looks like a fix for an unrelated bug? https://codereview.chromium.org/336273002/diff/380001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.h (right): https://codereview.chromium.org/336273002/diff/380001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.h:125: static bool WindowsVersionSP3Lower(); I wonder if this function should go elsewhere, like somewhere in chrome/common, and then we could also use it on application launch, and use an infobar (or other) to warn users that they are using a prehistoric platform. But, that would be a separate bug to be fixed in a later CL. Just an idea; maybe not a good one. For now this is fine.
https://codereview.chromium.org/336273002/diff/380001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/336273002/diff/380001/chrome/app/generated_re... chrome/app/generated_resources.grd:9362: + Your computer is running an old version of Microsoft Windows that cannot handle this website's security certificate. Because of this problem, Chrome can't tell whether the certificate came from <ph name="SITE">$1<ex>google.com</ex></ph> or from someone on your network pretending to be <ph name="SITE">$1<ex>google.com</ex></ph>. Please update your computer to Windows XP SP3 or higher. On 2014/07/07 18:10:03, Chromium Palmer wrote: > I hypothesize that "cannot process" will translate better than "cannot handle". Done. https://codereview.chromium.org/336273002/diff/380001/chrome/browser/resource... File chrome/browser/resources/ssl/interstitial_v2.js (right): https://codereview.chromium.org/336273002/diff/380001/chrome/browser/resource... chrome/browser/resources/ssl/interstitial_v2.js:41: } else if ($('help-link')) { On 2014/07/07 18:10:03, Chromium Palmer wrote: > This looks like a fix for an unrelated bug? Yes. I have just committed it and once I do a rebase update this will go away.
Hi radhika, can you do the rebase and update this patchset?
On 2014/07/14 17:07:39, felt wrote: > Hi radhika, can you do the rebase and update this patchset? Done.
On 2014/07/14 17:17:35, radhikabhar wrote: > On 2014/07/14 17:07:39, felt wrote: > > Hi radhika, can you do the rebase and update this patchset? > > Done. Now that 376663002 has landed, would it make sense to move WindowsVersionSP3Lower into chrome/browser/ssl/ssl_error_classification.h?
On 2014/07/14 17:21:25, felt wrote: > On 2014/07/14 17:17:35, radhikabhar wrote: > > On 2014/07/14 17:07:39, felt wrote: > > > Hi radhika, can you do the rebase and update this patchset? > > > > Done. > > Now that 376663002 has landed, would it make sense to move > WindowsVersionSP3Lower into chrome/browser/ssl/ssl_error_classification.h? radhika, have you forgotten about this CL? :)
On 2014/07/18 17:08:26, felt wrote: > On 2014/07/14 17:21:25, felt wrote: > > On 2014/07/14 17:17:35, radhikabhar wrote: > > > On 2014/07/14 17:07:39, felt wrote: > > > > Hi radhika, can you do the rebase and update this patchset? > > > > > > Done. > > > > Now that 376663002 has landed, would it make sense to move > > WindowsVersionSP3Lower into chrome/browser/ssl/ssl_error_classification.h? > > radhika, have you forgotten about this CL? :) No I wasn't sure about how to test it and I was also thinking of adding a check for the windows Service Pack 3 and then again sending it out for review. Or should I just land it?
On 2014/07/18 17:12:04, radhikabhar wrote: > On 2014/07/18 17:08:26, felt wrote: > > On 2014/07/14 17:21:25, felt wrote: > > > On 2014/07/14 17:17:35, radhikabhar wrote: > > > > On 2014/07/14 17:07:39, felt wrote: > > > > > Hi radhika, can you do the rebase and update this patchset? > > > > > > > > Done. > > > > > > Now that 376663002 has landed, would it make sense to move > > > WindowsVersionSP3Lower into chrome/browser/ssl/ssl_error_classification.h? > > > > radhika, have you forgotten about this CL? :) > > No I wasn't sure about how to test it and I was also thinking of adding a check > for the windows Service Pack 3 and then again sending it out for review. Or > should I just land it? * Ask Will for help with testing it; he should have enough VMs available to help. * Adding a check sounds like a good plan, but please do it sooner rather than later -- this CL has been out for 2 weeks now, I don't want it to be forgotten.
On 2014/07/18 17:13:56, felt wrote: > On 2014/07/18 17:12:04, radhikabhar wrote: > > On 2014/07/18 17:08:26, felt wrote: > > > On 2014/07/14 17:21:25, felt wrote: > > > > On 2014/07/14 17:17:35, radhikabhar wrote: > > > > > On 2014/07/14 17:07:39, felt wrote: > > > > > > Hi radhika, can you do the rebase and update this patchset? > > > > > > > > > > Done. > > > > > > > > Now that 376663002 has landed, would it make sense to move > > > > WindowsVersionSP3Lower into chrome/browser/ssl/ssl_error_classification.h? > > > > > > radhika, have you forgotten about this CL? :) > > > > No I wasn't sure about how to test it and I was also thinking of adding a > check > > for the windows Service Pack 3 and then again sending it out for review. Or > > should I just land it? > > * Ask Will for help with testing it; he should have enough VMs available to > help. > * Adding a check sounds like a good plan, but please do it sooner rather than > later -- this CL has been out for 2 weeks now, I don't want it to be forgotten. I've tested this on Windows XP SP2 and the message works. http://i.imgur.com/F4k0A1l.png I've added a few comments :)
https://codereview.chromium.org/336273002/diff/500001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/336273002/diff/500001/chrome/app/generated_re... chrome/app/generated_resources.grd:9394: + Your computer is running an old version of Microsoft Windows that cannot process this website's security certificate. Because of this problem, Chrome can't tell whether the certificate came from <ph name="SITE">$1<ex>google.com</ex></ph> or from someone on your network pretending to be <ph name="SITE">$1<ex>google.com</ex></ph>. Please update your computer to Windows XP SP3 or higher. should this be <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> instead of 'Chrome'? This happens on Win2k3 so telling those users to update to Windows XP SP3 might be confusing. Might only be a small number of users though. You could leave this vague and say 'update to a more recent version of Windows' since the key message here is that there's nothing Chrome can do about this error and it's the OS fault.
https://codereview.chromium.org/336273002/diff/500001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/336273002/diff/500001/chrome/app/generated_re... chrome/app/generated_resources.grd:9394: + Your computer is running an old version of Microsoft Windows that cannot process this website's security certificate. Because of this problem, Chrome can't tell whether the certificate came from <ph name="SITE">$1<ex>google.com</ex></ph> or from someone on your network pretending to be <ph name="SITE">$1<ex>google.com</ex></ph>. Please update your computer to Windows XP SP3 or higher. On 2014/07/22 20:02:38, Will Harris wrote: > should this be <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> instead of > 'Chrome'? > > This happens on Win2k3 so telling those users to update to Windows XP SP3 might > be confusing. Might only be a small number of users though. You could leave > this vague and say 'update to a more recent version of Windows' since the key > message here is that there's nothing Chrome can do about this error and it's the > OS fault. Done. However, when I upload this CL it gives me a presubmit warning that I shouldn't be using PRODUCT_NAME placeholders in string resources but in google_chrome_strings.grd or chromium_strings.grd. The reason it gives is - https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/PBs5JfR0Aoc.... Should I move this string there?
> Done. However, when I upload this CL it gives me a presubmit warning that I > shouldn't > be using PRODUCT_NAME placeholders in string resources but in > google_chrome_strings.grd or chromium_strings.grd. The reason it gives is - > https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/PBs5JfR0Aoc.... > Should I move this string there? Yes.
On 2014/07/22 23:52:48, Chromium Palmer wrote: > > Done. However, when I upload this CL it gives me a presubmit warning that I > > shouldn't > > be using PRODUCT_NAME placeholders in string resources but in > > google_chrome_strings.grd or chromium_strings.grd. The reason it gives is - > > > https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/PBs5JfR0Aoc.... > > Should I move this string there? > > Yes. You should move it, and also make sure to put "Chromium" in the version in chromium_strings.grd.
On 2014/07/22 23:53:37, felt wrote: > On 2014/07/22 23:52:48, Chromium Palmer wrote: > > > Done. However, when I upload this CL it gives me a presubmit warning that I > > > shouldn't > > > be using PRODUCT_NAME placeholders in string resources but in > > > google_chrome_strings.grd or chromium_strings.grd. The reason it gives is - > > > > > > https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/PBs5JfR0Aoc.... > > > Should I move this string there? > > > > Yes. > > You should move it, and also make sure to put "Chromium" in the version in > chromium_strings.grd. Done.
On 2014/07/22 20:02:38, Will Harris wrote: > https://codereview.chromium.org/336273002/diff/500001/chrome/app/generated_re... > File chrome/app/generated_resources.grd (right): > > https://codereview.chromium.org/336273002/diff/500001/chrome/app/generated_re... > chrome/app/generated_resources.grd:9394: + Your computer is running an > old version of Microsoft Windows that cannot process this website's security > certificate. Because of this problem, Chrome can't tell whether the certificate > came from <ph name="SITE">$1<ex>google.com</ex></ph> or from someone on your > network pretending to be <ph name="SITE">$1<ex>google.com</ex></ph>. Please > update your computer to Windows XP SP3 or higher. > should this be <ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> instead of > 'Chrome'? > > This happens on Win2k3 so telling those users to update to Windows XP SP3 might > be confusing. Might only be a small number of users though. You could leave > this vague and say 'update to a more recent version of Windows' since the key > message here is that there's nothing Chrome can do about this error and it's the > OS fault. Does (os_info->version() < base::win::VERSION_VISTA && service_pack.major < 3) really return true on Win2k3? (Apparently yes if you've tested this on Win2k3?)
radhika, for future reference -- please rebase as its own patch set (without any other changes), so that reviewers can look at each patch set with changes to see what you changed between patch sets https://codereview.chromium.org/336273002/diff/560001/chrome/app/chromium_str... File chrome/app/chromium_strings.grd (right): https://codereview.chromium.org/336273002/diff/560001/chrome/app/chromium_str... chrome/app/chromium_strings.grd:1271: <message name="IDS_SSL_NONOVERRIDABLE_MORE_INVALID_SP3" desc="Body text for the explanation shown if the user clicks on the details windows, the certificate is invalid and the user has an old version of Windows(before WINDOWS XP SP3) running."> nit: missing space "Windows(before"
On 2014/07/23 00:25:01, felt wrote: > > Does (os_info->version() < base::win::VERSION_VISTA && service_pack.major < 3) > really return true on Win2k3? (Apparently yes if you've tested this on Win2k3?) This code should be fine as base::win::Version defines VERSION_VISTA > VERSION_SERVER_2003 and there is no service pack 3 for Win2k3 - so this will always condition should always be true for Win2k3.
https://codereview.chromium.org/336273002/diff/560001/chrome/app/chromium_str... File chrome/app/chromium_strings.grd (right): https://codereview.chromium.org/336273002/diff/560001/chrome/app/chromium_str... chrome/app/chromium_strings.grd:1271: <message name="IDS_SSL_NONOVERRIDABLE_MORE_INVALID_SP3" desc="Body text for the explanation shown if the user clicks on the details windows, the certificate is invalid and the user has an old version of Windows(before WINDOWS XP SP3) running."> On 2014/07/23 00:25:53, felt wrote: > nit: missing space "Windows(before" Done.
On 2014/07/23 00:39:39, radhikabhar wrote: > https://codereview.chromium.org/336273002/diff/560001/chrome/app/chromium_str... > File chrome/app/chromium_strings.grd (right): > > https://codereview.chromium.org/336273002/diff/560001/chrome/app/chromium_str... > chrome/app/chromium_strings.grd:1271: <message > name="IDS_SSL_NONOVERRIDABLE_MORE_INVALID_SP3" desc="Body text for the > explanation shown if the user clicks on the details windows, the certificate is > invalid and the user has an old version of Windows(before WINDOWS XP SP3) > running."> > On 2014/07/23 00:25:53, felt wrote: > > nit: missing space "Windows(before" > > Done. lgtm
The CQ bit was checked by radhikabhar@chromium.org
The CQ bit was unchecked by radhikabhar@chromium.org
On 2014/07/23 01:05:48, radhikabhar wrote: > The CQ bit was unchecked by mailto:radhikabhar@chromium.org @felt, @palmer - Does the TEST parameter in the Description looks fine? Do we need it or should I delete it?
FWIW I did some more manual testing on Win2k3. Win2k3 SP1 - cert didn't validate, 'upgrade your windows' error was correctly displayed Win2k3 SP2 - cert didn't validate, 'upgrade your windows' error was correctly displayed Win2k3 SP2 with all the patches installed from Windows Update - cert worked!
On 2014/07/23 01:16:48, radhikabhar wrote: > On 2014/07/23 01:05:48, radhikabhar wrote: > > The CQ bit was unchecked by mailto:radhikabhar@chromium.org > > @felt, @palmer - Does the TEST parameter in the Description looks fine? Do we > need it or should I delete it? it looks helpful, please leave it
lgtm https://codereview.chromium.org/336273002/diff/580001/chrome/app/google_chrom... File chrome/app/google_chrome_strings.grd (right): https://codereview.chromium.org/336273002/diff/580001/chrome/app/google_chrom... chrome/app/google_chrome_strings.grd:1197: Your computer is running an old version of Microsoft Windows that cannot process this website's security certificate. Because of this problem, Google Chrome can't tell whether the certificate came from <ph name="SITE">$1<ex>google.com</ex></ph> or from someone on your network pretending to be <ph name="SITE">$1<ex>google.com</ex></ph>. Please update your computer to a more recent version of Windows. "...or from an impostor" is more concise, and concise is good. :) But until we have an agreed-upon vocabulary and style guide for these messages, it's a matter of taste.
https://codereview.chromium.org/336273002/diff/580001/chrome/app/google_chrom... File chrome/app/google_chrome_strings.grd (right): https://codereview.chromium.org/336273002/diff/580001/chrome/app/google_chrom... chrome/app/google_chrome_strings.grd:1197: Your computer is running an old version of Microsoft Windows that cannot process this website's security certificate. Because of this problem, Google Chrome can't tell whether the certificate came from <ph name="SITE">$1<ex>google.com</ex></ph> or from someone on your network pretending to be <ph name="SITE">$1<ex>google.com</ex></ph>. Please update your computer to a more recent version of Windows. On 2014/07/23 17:21:03, Chromium Palmer wrote: > "...or from an impostor" is more concise, and concise is good. :) > > But until we have an agreed-upon vocabulary and style guide for these messages, > it's a matter of taste. side tangent: jeff (APM intern) is running some GCS studies for us to help nail down a vocabulary
On 2014/07/23 17:24:01, felt wrote: > https://codereview.chromium.org/336273002/diff/580001/chrome/app/google_chrom... > File chrome/app/google_chrome_strings.grd (right): > > https://codereview.chromium.org/336273002/diff/580001/chrome/app/google_chrom... > chrome/app/google_chrome_strings.grd:1197: Your computer is running an old > version of Microsoft Windows that cannot process this website's security > certificate. Because of this problem, Google Chrome can't tell whether the > certificate came from <ph name="SITE">$1<ex>google.com</ex></ph> or from someone > on your network pretending to be <ph name="SITE">$1<ex>google.com</ex></ph>. > Please update your computer to a more recent version of Windows. > On 2014/07/23 17:21:03, Chromium Palmer wrote: > > "...or from an impostor" is more concise, and concise is good. :) > > > > But until we have an agreed-upon vocabulary and style guide for these > messages, > > it's a matter of taste. > > side tangent: jeff (APM intern) is running some GCS studies for us to help nail > down a vocabulary Changed it to impostor
On 2014/07/23 22:28:43, radhikabhar wrote: > On 2014/07/23 17:24:01, felt wrote: > > > https://codereview.chromium.org/336273002/diff/580001/chrome/app/google_chrom... > > File chrome/app/google_chrome_strings.grd (right): > > > > > https://codereview.chromium.org/336273002/diff/580001/chrome/app/google_chrom... > > chrome/app/google_chrome_strings.grd:1197: Your computer is running an old > > version of Microsoft Windows that cannot process this website's security > > certificate. Because of this problem, Google Chrome can't tell whether the > > certificate came from <ph name="SITE">$1<ex>google.com</ex></ph> or from > someone > > on your network pretending to be <ph name="SITE">$1<ex>google.com</ex></ph>. > > Please update your computer to a more recent version of Windows. > > On 2014/07/23 17:21:03, Chromium Palmer wrote: > > > "...or from an impostor" is more concise, and concise is good. :) > > > > > > But until we have an agreed-upon vocabulary and style guide for these > > messages, > > > it's a matter of taste. > > > > side tangent: jeff (APM intern) is running some GCS studies for us to help > nail > > down a vocabulary > > Changed it to impostor I disagree with Chris here, because the "impostor" language now makes it sound like it might be a phishing site (i.e., Chrome is trying to tell you that you've typed the wrong URL)
The CQ bit was checked by radhikabhar@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/radhikabhar@chromium.org/336273002/620001
The CQ bit was unchecked by radhikabhar@chromium.org
The CQ bit was checked by radhikabhar@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/radhikabhar@chromium.org/336273002/640001
Message was sent while issue was closed.
Change committed as 285481 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
