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

Issue 1205633002: Set alternative service even if it is broken. (Closed)

Created:
5 years, 6 months ago by Bence
Modified:
5 years, 6 months ago
Reviewers:
Ryan Hamilton
CC:
chromium-reviews, cbentzel+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Set alternative service even if it is broken. Modify HttpServerPropertiesImpl::SetAlternativeService() not to ignore a broken alternative service. This is consistent with the current behavior of keeping broken alternative services in the mapping, and of refusing to use the alternative service of a canonical host if the origin has an alternative service, even if it is broken. Both of these show that broken alternative services have a significance and thus should not be ignored as input of SetAlternativeService. BUG=392575 Committed: https://crrev.com/eb2e6601f1851335bbef50127805d5c9ed4f92db Cr-Commit-Position: refs/heads/master@{#336145}

Patch Set 1 #

Patch Set 2 : Add test, remove Patch Set 1. See failing test results. #

Patch Set 3 : Re-add Patch Set 1. See tests passing in trybot output. #

Total comments: 4

Patch Set 4 : Rebase. #

Patch Set 5 : Extend test. #

Total comments: 4

Patch Set 6 : Re: #7. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+23 lines, -9 lines) Patch
M net/http/http_server_properties_impl.cc View 1 2 3 1 chunk +0 lines, -9 lines 0 comments Download
M net/http/http_server_properties_impl_unittest.cc View 1 2 3 4 5 1 chunk +23 lines, -0 lines 0 comments Download

Messages

Total messages: 21 (7 generated)
Bence
Ryan: PTAL. Thanks.
5 years, 6 months ago (2015-06-23 19:03:42 UTC) #2
Ryan Hamilton
Can you add a regression test?
5 years, 6 months ago (2015-06-24 02:19:15 UTC) #3
Bence
PTAL. Thanks. On 2015/06/24 02:19:15, Ryan Hamilton wrote: > Can you add a regression test? ...
5 years, 6 months ago (2015-06-24 13:00:02 UTC) #4
Ryan Hamilton
https://codereview.chromium.org/1205633002/diff/40001/net/http/http_server_properties_impl_unittest.cc File net/http/http_server_properties_impl_unittest.cc (right): https://codereview.chromium.org/1205633002/diff/40001/net/http/http_server_properties_impl_unittest.cc#newcode379 net/http/http_server_properties_impl_unittest.cc:379: ASSERT_TRUE(HasAlternativeService(test_host_port_pair)); What will GetAlternateService return? Does this test fail ...
5 years, 6 months ago (2015-06-24 18:55:21 UTC) #5
Bence
PTAL. Thanks. https://codereview.chromium.org/1205633002/diff/40001/net/http/http_server_properties_impl_unittest.cc File net/http/http_server_properties_impl_unittest.cc (right): https://codereview.chromium.org/1205633002/diff/40001/net/http/http_server_properties_impl_unittest.cc#newcode379 net/http/http_server_properties_impl_unittest.cc:379: ASSERT_TRUE(HasAlternativeService(test_host_port_pair)); On 2015/06/24 18:55:21, Ryan Hamilton wrote: ...
5 years, 6 months ago (2015-06-24 20:30:47 UTC) #6
Ryan Hamilton
lgtm https://codereview.chromium.org/1205633002/diff/80001/net/http/http_server_properties_impl_unittest.cc File net/http/http_server_properties_impl_unittest.cc (right): https://codereview.chromium.org/1205633002/diff/80001/net/http/http_server_properties_impl_unittest.cc#newcode386 net/http/http_server_properties_impl_unittest.cc:386: ASSERT_TRUE(impl_.GetAlternativeService(test_host_port_pair) == nit: Does ASSERT_EQ() work? https://codereview.chromium.org/1205633002/diff/80001/net/http/http_server_properties_impl_unittest.cc#newcode388 net/http/http_server_properties_impl_unittest.cc:388: ...
5 years, 6 months ago (2015-06-24 21:08:32 UTC) #7
Bence
Thanks. https://codereview.chromium.org/1205633002/diff/80001/net/http/http_server_properties_impl_unittest.cc File net/http/http_server_properties_impl_unittest.cc (right): https://codereview.chromium.org/1205633002/diff/80001/net/http/http_server_properties_impl_unittest.cc#newcode386 net/http/http_server_properties_impl_unittest.cc:386: ASSERT_TRUE(impl_.GetAlternativeService(test_host_port_pair) == On 2015/06/24 21:08:32, Ryan Hamilton wrote: ...
5 years, 6 months ago (2015-06-25 11:24:35 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1205633002/100001
5 years, 6 months ago (2015-06-25 11:25:13 UTC) #11
commit-bot: I haz the power
Exceeded global retry quota
5 years, 6 months ago (2015-06-25 12:14:55 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1205633002/100001
5 years, 6 months ago (2015-06-25 12:50:44 UTC) #15
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/71913)
5 years, 6 months ago (2015-06-25 13:35:28 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1205633002/100001
5 years, 6 months ago (2015-06-25 13:59:36 UTC) #19
commit-bot: I haz the power
Committed patchset #6 (id:100001)
5 years, 6 months ago (2015-06-25 14:28:33 UTC) #20
commit-bot: I haz the power
5 years, 6 months ago (2015-06-25 14:29:26 UTC) #21
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/eb2e6601f1851335bbef50127805d5c9ed4f92db
Cr-Commit-Position: refs/heads/master@{#336145}

Powered by Google App Engine
This is Rietveld 408576698