Lazy RemoveCanonicalHost().

Lazy RemoveCanonicalHost().

Call RemoveCanonicalHost() when the alternative service is found to be broken
upon lookup, instead of when the alternative service is first marked broken.
This will allow SetBrokenAlternateProtocol to take an alternative service
instead of an origin.

BUG=392576
Committed: https://crrev.com/f71da630b63cdc705515027527b720ed130fcfd9
Cr-Commit-Position: refs/heads/master@{#321466}

[Bence, 2015-03-18 15:31:12]

bnc@chromium.org changed reviewers:
+ rch@chromium.org

[Bence, 2015-03-18 15:31:13]

Ryan, PTAL.  Thanks.

[Ryan Hamilton, 2015-03-18 22:44:36]

https://codereview.chromium.org/1014303002/diff/1/net/http/http_server_proper...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/1014303002/diff/1/net/http/http_server_proper...
net/http/http_server_properties_impl.cc:489: return it;
Do you need to (possibly) do the removal here too?

[Bence, 2015-03-19 12:44:02]

https://codereview.chromium.org/1014303002/diff/1/net/http/http_server_proper...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/1014303002/diff/1/net/http/http_server_proper...
net/http/http_server_properties_impl.cc:489: return it;
On 2015/03/18 22:44:36, Ryan Hamilton wrote:
> Do you need to (possibly) do the removal here too?

It's not necessary.  Consider foo being the canonical host for bar.  If foo has
an alternative service, and we detect it to be broken, and later we query the
alternative service of foo, we might or might not RemoveCanonicalHost(foo), but
we get back the broken alternative service in either case.  When we query the
alternative service of bar, the codepath below gets executed, foo is removed
from the canonical host list, and no alternative service is returned,
irrespectively from whether foo was removed from the canonical host list earlier
or not.

Since most hosts are not canonical hosts, I think it's better not to add a call
to RemoveCanonicalHost here.

[Ryan Hamilton, 2015-03-19 20:05:18]

lgtm if we have the test coverage. If we don't please add it :)

https://codereview.chromium.org/1014303002/diff/1/net/http/http_server_proper...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/1014303002/diff/1/net/http/http_server_proper...
net/http/http_server_properties_impl.cc:489: return it;
Ah, I see. Ok, do we have a test which covers this case? foo is canonical for
bar, foo is broken. GetAlternateProtocol for bar does not return the broken
entry?

[Bence, 2015-03-19 21:30:46]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2015-03-19 21:36:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1014303002/1

[Bence, 2015-03-19 21:43:09]

Thanks.

https://codereview.chromium.org/1014303002/diff/1/net/http/http_server_proper...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/1014303002/diff/1/net/http/http_server_proper...
net/http/http_server_properties_impl.cc:489: return it;
On 2015/03/19 20:05:18, Ryan Hamilton wrote:
> Ah, I see. Ok, do we have a test which covers this case? foo is canonical for
> bar, foo is broken. GetAlternateProtocol for bar does not return the broken
> entry?

Yes, AlternateProtocolServerPropertiesTest.CanonicalBroken does.

[commit-bot: I haz the power, 2015-03-19 23:09:26]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-03-19 23:10:07]

Patchset 1 (id:??) landed as
https://crrev.com/f71da630b63cdc705515027527b720ed130fcfd9
Cr-Commit-Position: refs/heads/master@{#321466}