Add MarkAlternativeServiceRecentlyBroken.

Add MarkAlternativeServiceRecentlyBroken.

Add HttpServiceProperties::MarkAlternativeServiceRecentlyBroken() and its
unittests.  Call it in QuicStreamFactory::ProcessGoingAwaySession.

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

Committed: https://crrev.com/d1e0aa2e708ac4be98b119d4b936723285a1cc7d
Cr-Commit-Position: refs/heads/master@{#320476}

[Bence, 2015-03-11 19:34:06]

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

[Bence, 2015-03-11 19:34:06]

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

[Bence, 2015-03-11 19:34:06]

Ryan, PTAL.  Thanks.

[Bence, 2015-03-11 19:34:07]

Ryan, PTAL.  Thanks.

[Ryan Hamilton, 2015-03-11 20:28:03]

Cool. Just a few comments.

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

https://codereview.chromium.org/989273005/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:348: if
(recently_broken_alternative_services_[alternative_service] == 0)
If it were me, I'd replace this if:

if (!ContainsKey(recently_broken_alternative_services_, alternative_service))

https://codereview.chromium.org/989273005/diff/1/net/quic/quic_stream_factory.cc
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/989273005/diff/1/net/quic/quic_stream_factory...
net/quic/quic_stream_factory.cc:1346: alternative_service);
How 'bout just:

  http_server_properties_->MarkAlternativeServiceRecentlyBroken(
      AlternativeService(alternate.protocol, server.host(), alternate.port));

Actually, might as well just use QUIC instead of alternate.protocol. 

In fact, even better would be:

  http_server_properties_->MarkAlternativeServiceRecentlyBroken(
      AlternativeService(QUIC, server.host(), server.port));

The server's port should match the alternate but still...

I think that would let you simplify the code on 1325 to be:

  if (http_server_properties_->GetAlternateProtocol(server).protocol ==
      UNINITIALIZED_ALTERNATE_PROTOCOL) {
  }

(Or possibly introduce a HasAlternateProtocol()) method.

Oh, and while you're here, that code is actually wrong because it's for an
origin not a server so it can probably be removed altogether.

[Bence, 2015-03-11 21:17:40]

Patchset #2 (id:20001) has been deleted

[Ryan Hamilton, 2015-03-11 23:02:34]

lgtm

https://codereview.chromium.org/989273005/diff/1/net/quic/quic_stream_factory.cc
File net/quic/quic_stream_factory.cc (left):

https://codereview.chromium.org/989273005/diff/1/net/quic/quic_stream_factory...
net/quic/quic_stream_factory.cc:1345:
http_server_properties_->ClearAlternateProtocol(server);
I didn't mention this explicitly, but you can remove this method now.

[Ryan Hamilton, 2015-03-11 23:03:04]

Sigh, did not mean to click l-g-t-m NOT LGTM for now. *Facepalm*

[Bence, 2015-03-12 00:40:55]

Patchset #2 (id:40001) has been deleted

[Bence, 2015-03-12 01:12:54]

Ryan, PTAL.  Thanks.

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

https://codereview.chromium.org/989273005/diff/1/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:348: if
(recently_broken_alternative_services_[alternative_service] == 0)
On 2015/03/11 20:28:02, Ryan Hamilton wrote:
> If it were me, I'd replace this if:
> 
> if (!ContainsKey(recently_broken_alternative_services_, alternative_service))

Done.

https://codereview.chromium.org/989273005/diff/1/net/quic/quic_stream_factory.cc
File net/quic/quic_stream_factory.cc (left):

https://codereview.chromium.org/989273005/diff/1/net/quic/quic_stream_factory...
net/quic/quic_stream_factory.cc:1345:
http_server_properties_->ClearAlternateProtocol(server);
On 2015/03/11 23:02:34, Ryan Hamilton wrote:
> I didn't mention this explicitly, but you can remove this method now.

You mean remove from HttpServerProperties?  We'll need it to reset alternative
services upon receiving a new AltSvc header or frame.  Besides, it's currently
called at
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_serv....

https://codereview.chromium.org/989273005/diff/1/net/quic/quic_stream_factory.cc
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/989273005/diff/1/net/quic/quic_stream_factory...
net/quic/quic_stream_factory.cc:1327: if (alternate.protocol ==
UNINITIALIZED_ALTERNATE_PROTOCOL)
I'm removing these couple of lines, because this used to be a workaround for
using SetBrokenAlternateProtocol in line 1344, but it's not needed any more.

https://codereview.chromium.org/989273005/diff/1/net/quic/quic_stream_factory...
net/quic/quic_stream_factory.cc:1346: alternative_service);
On 2015/03/11 20:28:02, Ryan Hamilton wrote:
> How 'bout just:
> 
>   http_server_properties_->MarkAlternativeServiceRecentlyBroken(
>       AlternativeService(alternate.protocol, server.host(), alternate.port));
> 
> Actually, might as well just use QUIC instead of alternate.protocol. 
> 
> In fact, even better would be:
> 
>   http_server_properties_->MarkAlternativeServiceRecentlyBroken(
>       AlternativeService(QUIC, server.host(), server.port));
> 
> The server's port should match the alternate but still...
> 
> I think that would let you simplify the code on 1325 to be:
> 
>   if (http_server_properties_->GetAlternateProtocol(server).protocol ==
>       UNINITIALIZED_ALTERNATE_PROTOCOL) {
>   }
> 
> (Or possibly introduce a HasAlternateProtocol()) method.
> 
> Oh, and while you're here, that code is actually wrong because it's for an
> origin not a server so it can probably be removed altogether.

Done.

[Ryan Hamilton, 2015-03-12 03:31:41]

Looks great! LGMT

https://codereview.chromium.org/989273005/diff/1/net/quic/quic_stream_factory.cc
File net/quic/quic_stream_factory.cc (left):

https://codereview.chromium.org/989273005/diff/1/net/quic/quic_stream_factory...
net/quic/quic_stream_factory.cc:1345:
http_server_properties_->ClearAlternateProtocol(server);
On 2015/03/12 01:12:53, Bence wrote:
> On 2015/03/11 23:02:34, Ryan Hamilton wrote:
> > I didn't mention this explicitly, but you can remove this method now.
> 
> You mean remove from HttpServerProperties?  We'll need it to reset alternative
> services upon receiving a new AltSvc header or frame.  

Hm. That's possible. I'm not sure that I love requiring the caller to first
clear then call set N-times, but perhaps that'll be best.

> Besides, it's currently called at
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_serv....

Sure, but if that's the only caller it could simply be inlined. But we can worry
about it later, as you say.

[Ryan Hamilton, 2015-03-12 03:34:54]

err LGTM

[Bence, 2015-03-12 16:53:29]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2015-03-12 16:54:03]

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

[commit-bot: I haz the power, 2015-03-12 17:55:05]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2015-03-12 17:55:56]

Patchset 2 (id:??) landed as
https://crrev.com/35470e76130167053b49c81da373e056ce60b184
Cr-Commit-Position: refs/heads/master@{#320315}

[Bence, 2015-03-12 18:30:32]

A revert of this CL (patchset #2 id:60001) has been created in
https://codereview.chromium.org/1002043002/ by bnc@chromium.org.

The reason for reverting is: Reverting so that
https://codereview.chromium.org/997953003/ can be reverted.

BUG=466706.

[Bence, 2015-03-13 09:04:33]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2015-03-13 09:04:45]

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

[Bence, 2015-03-13 09:05:04]

On 2015/03/12 18:30:32, Bence wrote:
> A revert of this CL (patchset #2 id:60001) has been created in
> https://codereview.chromium.org/1002043002/ by mailto:bnc@chromium.org.
> 
> The reason for reverting is: Reverting so that
> https://codereview.chromium.org/997953003/ can be reverted.
> 
> BUG=466706.

Landing again after https://codereview.chromium.org/997953003/ is fixed and
landed and https://crbug.com/466706 fixed.

[commit-bot: I haz the power, 2015-03-13 10:14:38]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2015-03-13 10:15:26]

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