Introduce AlternateProtocolInfo.is_broken.

Introduce AlternateProtocolInfo.is_broken.

Introduce bool AlternateProtocolInfo.is_broken and remove
ALTERNATE_PROTOCOL_BROKEN from enum AlternateProtocol.  This allows
AlternateProtocolInfo to keep track of which protocol is broken.  Note that the
port does not identify the protocol: most notably, SPDY and QUIC can use the
same port, but one uses TCP, the other UDP.

This change is essential if we want to keep track of multiple
AlternateProtocolInfo structs for each server, in preparation for alternate
service support.

Note that this change stops writing out broken protocols to prefs in
HttpServerPropertiesManager::UpdatePrefsOnPrefThread().  This is okay, because
HttpServerPropertiesManager::UpdateCacheFromPrefsOnPrefThread() used to refuse
to load broken ones already, see IsAlternateProtocolValid(protocol) in line 436.

BUG=392575
Committed: https://crrev.com/cbd55d5f181ec117dda702fcc0e8bf5f87b78379
Cr-Commit-Position: refs/heads/master@{#303075}

[Bence, 2014-11-05 16:10:39]

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

[Bence, 2014-11-05 16:10:40]

Ryan: PTAL.

[Ryan Hamilton, 2014-11-05 17:05:20]

Sweet! Thanks for doing this. I think it's much clearer now.

https://codereview.chromium.org/701163002/diff/20001/net/http/http_server_pro...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/701163002/diff/20001/net/http/http_server_pro...
net/http/http_server_properties.h:88: bool is_broken = false)
Alas, using arguments with default values is prohibited by the style guide :<

https://codereview.chromium.org/701163002/diff/20001/net/http/http_server_pro...
File net/http/http_server_properties_impl.cc (left):

https://codereview.chromium.org/701163002/diff/20001/net/http/http_server_pro...
net/http/http_server_properties_impl.cc:257: }
NICE! I've always hated this DFATAL.

https://codereview.chromium.org/701163002/diff/20001/net/http/http_server_pro...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/701163002/diff/20001/net/http/http_server_pro...
net/http/http_server_properties_impl.cc:306: AlternateProtocolInfo
alternate(server.port(),
consider adding a DLOG(DFATAL) here since I don't think we ever want to set
AlternateProtocolBroken if we don't have an existing alternate protocol.

(If it turns out that we *do* invoke this code path, we should do more thinking
about it because in your new world order, marking UNINITIALIZED as broken won't
actually do anything useful.

https://codereview.chromium.org/701163002/diff/20001/net/http/http_server_pro...
File net/http/http_server_properties_impl_unittest.cc (right):

https://codereview.chromium.org/701163002/diff/20001/net/http/http_server_pro...
net/http/http_server_properties_impl_unittest.cc:377: << "Second attempt should
be ignored.";
nit: can this fit on one line?

https://codereview.chromium.org/701163002/diff/20001/net/http/http_stream_fac...
File net/http/http_stream_factory.cc (right):

https://codereview.chromium.org/701163002/diff/20001/net/http/http_stream_fac...
net/http/http_stream_factory.cc:71:
AlternateProtocolFromString(port_protocol_vector[1]);
Can this fit on one line?

Can you confirm what will happen if an old version of chrome writes out BROKEN,
and then a new version of Chrome tries to read it in? I *suspect* that
AlternateProtocolFromString() will not return a valid alternate protocol in this
case, and all will be well, but please make sure this is correct.

https://codereview.chromium.org/701163002/diff/20001/net/http/http_stream_fac...
net/http/http_stream_factory.cc:96: }
net/ style prohibits {}s on one-line ifs. Whee!

[Bence, 2014-11-06 00:26:00]

Ryan: PTAL.

https://codereview.chromium.org/701163002/diff/20001/net/http/http_server_pro...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/701163002/diff/20001/net/http/http_server_pro...
net/http/http_server_properties.h:88: bool is_broken = false)
On 2014/11/05 17:05:19, Ryan Hamilton wrote:
> Alas, using arguments with default values is prohibited by the style guide :<

Done.

https://codereview.chromium.org/701163002/diff/20001/net/http/http_server_pro...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/701163002/diff/20001/net/http/http_server_pro...
net/http/http_server_properties_impl.cc:306: AlternateProtocolInfo
alternate(server.port(),
On 2014/11/05 17:05:20, Ryan Hamilton wrote:
> consider adding a DLOG(DFATAL) here since I don't think we ever want to set
> AlternateProtocolBroken if we don't have an existing alternate protocol.
> 
> (If it turns out that we *do* invoke this code path, we should do more
thinking
> about it because in your new world order, marking UNINITIALIZED as broken
won't
> actually do anything useful. 

Done.

https://codereview.chromium.org/701163002/diff/20001/net/http/http_server_pro...
File net/http/http_server_properties_impl_unittest.cc (right):

https://codereview.chromium.org/701163002/diff/20001/net/http/http_server_pro...
net/http/http_server_properties_impl_unittest.cc:377: << "Second attempt should
be ignored.";
On 2014/11/05 17:05:20, Ryan Hamilton wrote:
> nit: can this fit on one line?

Sorry, git cl format is broken on my machine.

Done.

https://codereview.chromium.org/701163002/diff/20001/net/http/http_stream_fac...
File net/http/http_stream_factory.cc (right):

https://codereview.chromium.org/701163002/diff/20001/net/http/http_stream_fac...
net/http/http_stream_factory.cc:71:
AlternateProtocolFromString(port_protocol_vector[1]);
On 2014/11/05 17:05:20, Ryan Hamilton wrote:
> Can this fit on one line?

Sorry, git cl format is broken on my machine.

> Can you confirm what will happen if an old version of chrome writes out
BROKEN,
> and then a new version of Chrome tries to read it in? I *suspect* that
> AlternateProtocolFromString() will not return a valid alternate protocol in
this
> case, and all will be well, but please make sure this is correct.

Confirmed.  Protocol string will be "Broken", for which
AlternateProtocolFromString will return UNINITIALIZED_ALTERNATE_PROTOCOL, for
which IsAlternateProtocolValid will return false.

https://codereview.chromium.org/701163002/diff/20001/net/http/http_stream_fac...
net/http/http_stream_factory.cc:96: }
On 2014/11/05 17:05:20, Ryan Hamilton wrote:
> net/ style prohibits {}s on one-line ifs. Whee!

Lovely.

Done.

[Ryan Hamilton, 2014-11-06 01:34:27]

LGTM, modulo one small formatting nit.

Thanks for doing this!

https://codereview.chromium.org/701163002/diff/40001/net/http/http_server_pro...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/701163002/diff/40001/net/http/http_server_pro...
net/http/http_server_properties_impl.cc:307: return;
One more nit. Can you write this as:

if (it == ... end()) {
  LOG(DFATAL)
  return;
}
it->second.is_broken = true;

(I'm a big fan of early return to improve readability)

[Bence, 2014-11-06 19:00:49]

The CQ bit was checked by bnc@chromium.org

[Bence, 2014-11-06 19:00:55]

Thanks.

https://codereview.chromium.org/701163002/diff/40001/net/http/http_server_pro...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/701163002/diff/40001/net/http/http_server_pro...
net/http/http_server_properties_impl.cc:307: return;
On 2014/11/06 01:34:27, Ryan Hamilton wrote:
> One more nit. Can you write this as:
> 
> if (it == ... end()) {
>   LOG(DFATAL)
>   return;
> }
> it->second.is_broken = true;
> 
> (I'm a big fan of early return to improve readability)

Excellent, thanks for catching this.

Done.

[commit-bot: I haz the power, 2014-11-06 19:02:32]

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

[commit-bot: I haz the power, 2014-11-06 19:50:49]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-06 19:51:31]

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