Implement multiple alternative services per origin.

Implement multiple alternative services per origin.

HttpServerProperties:
 * Introduce AlternativeServiceVector and AlternativeServiceInfoVector.
 * Change AlternativeServiceMap typedef.

HttpServerPropertiesImpl:
 * Add bool SetAlternativeServices.
 * Change SetAlternativeService return type to bool.
 * Replace GetAlternativeService with GetAlternativeServices.
 * Replace ClearAlternativeService with ClearAlternativeServices.

HttpServerPropertiesManager:
 * Implement saving and loading preferences to support multiple alternative
   services per origin.

HttpStreamFactory and QuicStreamFactory:
 * Use first alternative service for now; more work to be done here.

Also update misc clients and add and update unittests.

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

[Bence, 2015-06-26 20:34:35]

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

[Bence, 2015-06-26 20:34:36]

Ryan: PTAL.  Thanks.

[Ryan Hamilton, 2015-06-29 21:15:53]

https://codereview.chromium.org/1216703002/diff/1/net/http/http_server_proper...
File net/http/http_server_properties_manager.cc (left):

https://codereview.chromium.org/1216703002/diff/1/net/http/http_server_proper...
net/http/http_server_properties_manager.cc:212:
ScheduleUpdatePrefsOnNetworkThread(SET_ALTERNATIVE_SERVICE);
You seem to have removed the conditional here of not scheduling an update if
nothing changed. Was that intentional? Raman added this recently because
otherwise we were doing a noticeable amount of extra work when nothing had
changed. I think we perhaps need to move from Clear + Set/Add to a single
SetAlternateServices() so that this method can avoid the update. What do you
think?

https://codereview.chromium.org/1216703002/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/1216703002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl.cc:143: }
nit: instead of having an if with two bodies that create Job, it might be
simpler to just have a AlternativeService alternative_service;
local variable which is initialize to be empty and only assigned to if the
vector is non-empty.

[Bence, 2015-06-30 17:57:30]

Patchset #6 (id:90001) has been deleted

[Bence, 2015-06-30 18:07:33]

Patchset #6 (id:110001) has been deleted

[Bence, 2015-06-30 19:16:14]

Ryan: PTAL.  Thanks.

https://codereview.chromium.org/1216703002/diff/1/net/http/http_server_proper...
File net/http/http_server_properties_manager.cc (left):

https://codereview.chromium.org/1216703002/diff/1/net/http/http_server_proper...
net/http/http_server_properties_manager.cc:212:
ScheduleUpdatePrefsOnNetworkThread(SET_ALTERNATIVE_SERVICE);
On 2015/06/29 21:15:52, Ryan Hamilton wrote:
> You seem to have removed the conditional here of not scheduling an update if
> nothing changed. Was that intentional? Raman added this recently because
> otherwise we were doing a noticeable amount of extra work when nothing had
> changed. I think we perhaps need to move from Clear + Set/Add to a single
> SetAlternateServices() so that this method can avoid the update. What do you
> think?

I think SetAlternativeServices is a good idea.  It might make tests more
complicated, but is aligned with production use.  Done.

Note, however, that if the server keeps advertising the exact same alternative
services, this only cuts updates in half.  So I modified the return types to
bool.  This way alternative services are only updated if they really change.

https://codereview.chromium.org/1216703002/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/1216703002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl.cc:143: }
On 2015/06/29 21:15:53, Ryan Hamilton wrote:
> nit: instead of having an if with two bodies that create Job, it might be
> simpler to just have a AlternativeService alternative_service;
> local variable which is initialize to be empty and only assigned to if the
> vector is non-empty.
> 

Done.

[Bence, 2015-07-06 11:32:38]

PTAL.  Thanks.

[Ryan Hamilton, 2015-07-06 17:48:09]

lgtm

https://codereview.chromium.org/1216703002/diff/1/net/http/http_server_proper...
File net/http/http_server_properties_manager.cc (left):

https://codereview.chromium.org/1216703002/diff/1/net/http/http_server_proper...
net/http/http_server_properties_manager.cc:212:
ScheduleUpdatePrefsOnNetworkThread(SET_ALTERNATIVE_SERVICE);
On 2015/06/30 19:16:14, Bence wrote:
> On 2015/06/29 21:15:52, Ryan Hamilton wrote:
> > You seem to have removed the conditional here of not scheduling an update if
> > nothing changed. Was that intentional? Raman added this recently because
> > otherwise we were doing a noticeable amount of extra work when nothing had
> > changed. I think we perhaps need to move from Clear + Set/Add to a single
> > SetAlternateServices() so that this method can avoid the update. What do you
> > think?
> 
> I think SetAlternativeServices is a good idea.  It might make tests more
> complicated, but is aligned with production use.  Done.
> 
> Note, however, that if the server keeps advertising the exact same alternative
> services, this only cuts updates in half.  So I modified the return types to
> bool.  This way alternative services are only updated if they really change.

I'm not sure that I understand. If the output of GetAlternativeServices has not
change, we shouldn't need to persist, right? And if it has changed we should
persist, I would think. Can you expand on your "only cuts updates in half"
comment?

[Ryan Hamilton, 2015-07-06 17:48:26]

On 2015/07/06 17:48:09, Ryan Hamilton wrote:
> lgtm
> 
>
https://codereview.chromium.org/1216703002/diff/1/net/http/http_server_proper...
> File net/http/http_server_properties_manager.cc (left):
> 
>
https://codereview.chromium.org/1216703002/diff/1/net/http/http_server_proper...
> net/http/http_server_properties_manager.cc:212:
> ScheduleUpdatePrefsOnNetworkThread(SET_ALTERNATIVE_SERVICE);
> On 2015/06/30 19:16:14, Bence wrote:
> > On 2015/06/29 21:15:52, Ryan Hamilton wrote:
> > > You seem to have removed the conditional here of not scheduling an update
if
> > > nothing changed. Was that intentional? Raman added this recently because
> > > otherwise we were doing a noticeable amount of extra work when nothing had
> > > changed. I think we perhaps need to move from Clear + Set/Add to a single
> > > SetAlternateServices() so that this method can avoid the update. What do
you
> > > think?
> > 
> > I think SetAlternativeServices is a good idea.  It might make tests more
> > complicated, but is aligned with production use.  Done.
> > 
> > Note, however, that if the server keeps advertising the exact same
alternative
> > services, this only cuts updates in half.  So I modified the return types to
> > bool.  This way alternative services are only updated if they really change.
> 
> I'm not sure that I understand. If the output of GetAlternativeServices has
not
> change, we shouldn't need to persist, right? And if it has changed we should
> persist, I would think. Can you expand on your "only cuts updates in half"
> comment?

err, sorry, not LGTM. clicked the wrong button

[Bence, 2015-07-07 09:55:55]

Ryan: PTAL.  Thanks.

https://codereview.chromium.org/1216703002/diff/1/net/http/http_server_proper...
File net/http/http_server_properties_manager.cc (left):

https://codereview.chromium.org/1216703002/diff/1/net/http/http_server_proper...
net/http/http_server_properties_manager.cc:212:
ScheduleUpdatePrefsOnNetworkThread(SET_ALTERNATIVE_SERVICE);
On 2015/07/06 17:48:09, Ryan Hamilton wrote:
> On 2015/06/30 19:16:14, Bence wrote:
> > On 2015/06/29 21:15:52, Ryan Hamilton wrote:
> > > You seem to have removed the conditional here of not scheduling an update
if
> > > nothing changed. Was that intentional? Raman added this recently because
> > > otherwise we were doing a noticeable amount of extra work when nothing had
> > > changed. I think we perhaps need to move from Clear + Set/Add to a single
> > > SetAlternateServices() so that this method can avoid the update. What do
you
> > > think?
> > 
> > I think SetAlternativeServices is a good idea.  It might make tests more
> > complicated, but is aligned with production use.  Done.
> > 
> > Note, however, that if the server keeps advertising the exact same
alternative
> > services, this only cuts updates in half.  So I modified the return types to
> > bool.  This way alternative services are only updated if they really change.
> 
> I'm not sure that I understand. If the output of GetAlternativeServices has
not
> change, we shouldn't need to persist, right? And if it has changed we should
> persist, I would think.

There might be situations when the output of GetAlternativeServices does not
change but we need to persist.  For example, the server advertises an
alternative service with a probability below threshold, so it is hidden by
GetAlternativeServices, but might be important for the next browser session.  Or
later I might change GetAlternativeServices not to return broken alternative
services.  What matters instead is |alternative_service_map_|.  Therefore I
modified HttpServerPropertiesImpl::SetAlternativeServices to return whether the
|alternative_service_map_| entry for |origin| has actually changed, so that
HttpServerProperiesManager::SetAlternativeServices only triggers an update if
necessary.

> Can you expand on your "only cuts updates in half"
> comment?

Without detecting change in HttpServerPropertiesImpl::SetAlternativeServices,
replacing ClearAlternativeServices and AddAlternativeServices by
SetAlternativeServices (two methods by one) would mean one update triggered
instead of two every time the origin sends alternative service information.  I
didn't find this improvement satisfactory, so I modified
HttpServerPropertiesImpl::SetAlternativeServices to detect if persisting
settings is actually needed.

[Ryan Hamilton, 2015-07-07 17:25:06]

lgtm

[Bence, 2015-07-08 05:11:35]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2015-07-08 05:11:50]

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

[commit-bot: I haz the power, 2015-07-08 05:16:19]

Committed patchset #8 (id:170001)

[commit-bot: I haz the power, 2015-07-08 05:17:17]

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