Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(362)

Issue 581803002: Component updater must fallback on using HTTP on Windows XPSP2 and below (Closed)

Created:
6 years, 3 months ago by Sorin Jianu
Modified:
6 years, 3 months ago
CC:
chromium-reviews, laforge, Ryan Sleevi, agl
Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr
Project:
chromium
Visibility:
Public.

Description

Component updater must fallback on using HTTP on Windows XPSP2 and below BUG=411009 Committed: https://crrev.com/c512b7a845d12db2eb9d573f14aab6577c7509f7 Cr-Commit-Position: refs/heads/master@{#295633}

Patch Set 1 #

Total comments: 6

Patch Set 2 : #

Total comments: 1

Patch Set 3 : Refactor common version check code. #

Patch Set 4 : #

Total comments: 4

Patch Set 5 : #

Total comments: 7

Patch Set 6 : Made Windows-specific name; changed the logic polarity. #

Total comments: 2

Patch Set 7 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+63 lines, -10 lines) Patch
M base/win/win_util.h View 1 2 3 4 5 6 1 chunk +7 lines, -0 lines 0 comments Download
M base/win/win_util.cc View 1 2 1 chunk +19 lines, -0 lines 0 comments Download
M chrome/browser/component_updater/chrome_component_updater_configurator.cc View 1 2 7 chunks +28 lines, -2 lines 0 comments Download
M chrome/browser/ssl/ssl_blocking_page.cc View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ssl/ssl_error_classification.h View 1 2 3 4 5 1 chunk +3 lines, -1 line 0 comments Download
M chrome/browser/ssl/ssl_error_classification.cc View 1 2 3 4 5 2 chunks +5 lines, -6 lines 0 comments Download

Messages

Total messages: 39 (10 generated)
waffles
lgtm but see comment https://codereview.chromium.org/581803002/diff/1/chrome/browser/component_updater/chrome_component_updater_configurator.cc File chrome/browser/component_updater/chrome_component_updater_configurator.cc (right): https://codereview.chromium.org/581803002/diff/1/chrome/browser/component_updater/chrome_component_updater_configurator.cc#newcode84 chrome/browser/component_updater/chrome_component_updater_configurator.cc:84: if (os_info->version() != base::win::VERSION_XP) Do ...
6 years, 3 months ago (2014-09-17 21:26:45 UTC) #2
cpu_(ooo_6.6-7.5)
https://codereview.chromium.org/581803002/diff/1/chrome/browser/component_updater/chrome_component_updater_configurator.cc File chrome/browser/component_updater/chrome_component_updater_configurator.cc (right): https://codereview.chromium.org/581803002/diff/1/chrome/browser/component_updater/chrome_component_updater_configurator.cc#newcode83 chrome/browser/component_updater/chrome_component_updater_configurator.cc:83: const base::win::OSInfo* os_info = base::win::OSInfo::GetInstance(); no need to cache ...
6 years, 3 months ago (2014-09-17 21:28:19 UTC) #3
Sorin Jianu
https://codereview.chromium.org/581803002/diff/1/chrome/browser/component_updater/chrome_component_updater_configurator.cc File chrome/browser/component_updater/chrome_component_updater_configurator.cc (right): https://codereview.chromium.org/581803002/diff/1/chrome/browser/component_updater/chrome_component_updater_configurator.cc#newcode84 chrome/browser/component_updater/chrome_component_updater_configurator.cc:84: if (os_info->version() != base::win::VERSION_XP) On 2014/09/17 21:26:45, waffles wrote: ...
6 years, 3 months ago (2014-09-17 21:29:06 UTC) #4
cpu_(ooo_6.6-7.5)
lets not do anything for the server OS. They need to upgrade to get sha2 ...
6 years, 3 months ago (2014-09-17 21:29:40 UTC) #5
Sorin Jianu
Thank you, no changes made, PTAL. I'll make code changes if needed. I just want ...
6 years, 3 months ago (2014-09-17 21:45:36 UTC) #6
Sorin Jianu
Also, do we need to handle XP 64bit, as suggested by waffles@
6 years, 3 months ago (2014-09-17 21:46:20 UTC) #7
Sorin Jianu
Uploaded to reflect the last feedback from cpu@.
6 years, 3 months ago (2014-09-17 22:19:22 UTC) #8
Will Harris
https://codereview.chromium.org/581803002/diff/20001/chrome/browser/component_updater/chrome_component_updater_configurator.cc File chrome/browser/component_updater/chrome_component_updater_configurator.cc (right): https://codereview.chromium.org/581803002/diff/20001/chrome/browser/component_updater/chrome_component_updater_configurator.cc#newcode81 chrome/browser/component_updater/chrome_component_updater_configurator.cc:81: bool CanUseAltUrlSource() { This code is also in SSLErrorClassification::IsWindowsVersionSP3OrLower ...
6 years, 3 months ago (2014-09-17 23:29:17 UTC) #10
Sorin Jianu
Will, I attempted to refactor the common code, it is somehow different that what we've ...
6 years, 3 months ago (2014-09-18 02:00:44 UTC) #11
Will Harris
lgtm w/nit feel free to raise a bug to make MaybeHasSHA256Support() less "maybe" and more ...
6 years, 3 months ago (2014-09-18 17:35:22 UTC) #12
waffles
https://codereview.chromium.org/581803002/diff/60001/chrome/browser/ssl/ssl_blocking_page.cc File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/581803002/diff/60001/chrome/browser/ssl/ssl_blocking_page.cc#newcode474 chrome/browser/ssl/ssl_blocking_page.cc:474: MaybeWindowsHasSHA256Support()) { Both SSLErrorClassification::IsWindowsVersionSP3OrLower and SSLErrorClassification::MaybeWindowsHasSHA256Support return false if ...
6 years, 3 months ago (2014-09-18 18:00:07 UTC) #13
Sorin Jianu
Thank you! agl@, I need an owner approval for /chrome/browser/ssl/ changes. cpu@, I need an ...
6 years, 3 months ago (2014-09-18 18:22:23 UTC) #15
waffles
lgtm
6 years, 3 months ago (2014-09-18 18:24:48 UTC) #16
Ryan Sleevi
c/b/ssl lgtm
6 years, 3 months ago (2014-09-18 18:28:06 UTC) #19
cpu_(ooo_6.6-7.5)
lgtm for base\win and CUS
6 years, 3 months ago (2014-09-18 18:28:07 UTC) #21
Will Harris
lgtm
6 years, 3 months ago (2014-09-18 18:28:48 UTC) #23
felt
https://codereview.chromium.org/581803002/diff/80001/chrome/browser/ssl/ssl_blocking_page.cc File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/581803002/diff/80001/chrome/browser/ssl/ssl_blocking_page.cc#newcode475 chrome/browser/ssl/ssl_blocking_page.cc:475: load_time_data.SetString( This check displays the string IDS_SSL_NONOVERRIDABLE_MORE_INVALID_SP3, which says: ...
6 years, 3 months ago (2014-09-18 18:30:36 UTC) #24
Sorin Jianu
Thank you. I did not make any code changes in response to the last batch ...
6 years, 3 months ago (2014-09-18 18:41:07 UTC) #25
felt
https://codereview.chromium.org/581803002/diff/80001/chrome/browser/ssl/ssl_blocking_page.cc File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/581803002/diff/80001/chrome/browser/ssl/ssl_blocking_page.cc#newcode475 chrome/browser/ssl/ssl_blocking_page.cc:475: load_time_data.SetString( On 2014/09/18 18:41:07, Sorin Jianu wrote: > On ...
6 years, 3 months ago (2014-09-18 18:44:28 UTC) #26
felt
https://codereview.chromium.org/581803002/diff/80001/chrome/browser/ssl/ssl_blocking_page.cc File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/581803002/diff/80001/chrome/browser/ssl/ssl_blocking_page.cc#newcode475 chrome/browser/ssl/ssl_blocking_page.cc:475: load_time_data.SetString( On 2014/09/18 18:44:27, felt wrote: > On 2014/09/18 ...
6 years, 3 months ago (2014-09-18 18:52:23 UTC) #27
Sorin Jianu
ok, so I've tried many things until I settled for this version of the code, ...
6 years, 3 months ago (2014-09-18 21:24:22 UTC) #28
felt
Thank you, I appreciate it! https://codereview.chromium.org/581803002/diff/100001/base/win/win_util.h File base/win/win_util.h (right): https://codereview.chromium.org/581803002/diff/100001/base/win/win_util.h#newcode137 base/win/win_util.h:137: // Returns true is ...
6 years, 3 months ago (2014-09-18 21:42:45 UTC) #29
felt
lgtm
6 years, 3 months ago (2014-09-18 21:42:52 UTC) #30
Sorin Jianu
Thank you all! https://codereview.chromium.org/581803002/diff/100001/base/win/win_util.h File base/win/win_util.h (right): https://codereview.chromium.org/581803002/diff/100001/base/win/win_util.h#newcode137 base/win/win_util.h:137: // Returns true is the current ...
6 years, 3 months ago (2014-09-18 21:55:19 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/581803002/120001
6 years, 3 months ago (2014-09-18 21:57:46 UTC) #33
commit-bot: I haz the power
Exceeded time limit waiting for builds to trigger.
6 years, 3 months ago (2014-09-19 00:01:01 UTC) #35
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/581803002/120001
6 years, 3 months ago (2014-09-19 00:05:13 UTC) #37
commit-bot: I haz the power
Committed patchset #7 (id:120001) as 3366aad2feba9868f2a40529edbae88ddcf2e58b
6 years, 3 months ago (2014-09-19 01:12:24 UTC) #38
commit-bot: I haz the power
6 years, 3 months ago (2014-09-19 01:12:57 UTC) #39
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/c512b7a845d12db2eb9d573f14aab6577c7509f7
Cr-Commit-Position: refs/heads/master@{#295633}

Powered by Google App Engine
This is Rietveld 408576698