Add AlternateProtocolInfo to HttpStreamFactoryImpl::Job.

Add AlternateProtocolInfo to HttpStreamFactoryImpl::Job.

Add AlternateProtocolInfo member to HttpStreamFactoryImpl::Job.  Add
AlternateProtocolInfo argument to constructor, remove it from MarkAsAlternate.
This will be necessary in MaybeMarkAlternateProtocolBroken to pass it on to
SetBrokenAlternateProtocol().  See https://crrev.com/665083009 for context.

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

[Bence, 2015-01-30 22:59:21]

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

[Bence, 2015-01-30 22:59:21]

Ryan, PTAL.  Thank you.

[Ryan Hamilton, 2015-01-30 23:12:51]

https://codereview.chromium.org/871753006/diff/1/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.h (right):

https://codereview.chromium.org/871753006/diff/1/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.h:45: AlternateProtocolInfo
alternate_protocol);
I'm not sure that I'm a fan of moving this to the constructor. Since only
alternate jobs (those which have had MarkAsAlternate called) should have
alternatives it seems odd to pass in here, instead of when marking it alternate.

Or, I guess, you could add both of these arguments to the constructor, but that
doesn't feel super awesome?

[Bence, 2015-01-31 00:32:01]

Ryan, I understand your concern, but in the context of the bigger change, I
think this is the best approach.  What do you recommend?

https://codereview.chromium.org/871753006/diff/1/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.h (right):

https://codereview.chromium.org/871753006/diff/1/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.h:45: AlternateProtocolInfo
alternate_protocol);
On 2015/01/30 23:12:51, Ryan Hamilton wrote:
> I'm not sure that I'm a fan of moving this to the constructor. Since only
> alternate jobs (those which have had MarkAsAlternate called) should have
> alternatives it seems odd to pass in here, instead of when marking it
alternate.
> 
> Or, I guess, you could add both of these arguments to the constructor, but
that
> doesn't feel super awesome?

Well, if you look at
https://codereview.chromium.org/665083009/diff/400001/net/http/http_stream_fa...,
you can see that both the alternate job and the non-alternate one must keep a
copy of alternate_protocol, because they will need to pass it to
SetBrokenAlternateProtocol().  This is because each server will be allowed to
have multiple AlternateProtocols, and SetBrokenAlternateProtocol() will need to
know which one to mark as broken.

[Ryan Hamilton, 2015-01-31 01:17:48]

https://codereview.chromium.org/871753006/diff/1/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.h (right):

https://codereview.chromium.org/871753006/diff/1/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.h:45: AlternateProtocolInfo
alternate_protocol);
On 2015/01/31 00:32:01, Bence wrote:
> On 2015/01/30 23:12:51, Ryan Hamilton wrote:
> > I'm not sure that I'm a fan of moving this to the constructor. Since only
> > alternate jobs (those which have had MarkAsAlternate called) should have
> > alternatives it seems odd to pass in here, instead of when marking it
> alternate.
> > 
> > Or, I guess, you could add both of these arguments to the constructor, but
> that
> > doesn't feel super awesome?
> 
> Well, if you look at
>
https://codereview.chromium.org/665083009/diff/400001/net/http/http_stream_fa...,
> you can see that both the alternate job and the non-alternate one must keep a
> copy of alternate_protocol, because they will need to pass it to
> SetBrokenAlternateProtocol().  This is because each server will be allowed to
> have multiple AlternateProtocols, and SetBrokenAlternateProtocol() will need
to
> know which one to mark as broken.

I'm not sure that's the best way to accomplish this. It seems like that change
makes alternate_protocol_ mean two different things. it's either the protocol
being used by this jobs (if it's an alternate job) or else is that alternate
protocol being used but the other job (if this is a main job). Instead, it might
make more sense to change MarkOtherJobComplete() so that it takes the
AlternateProtocol (if any) from the other job and then save it in a member like
other_job_alternate_protocol_.

[Bence, 2015-02-02 15:08:17]

Ryan, PTAL.  Thanks for the suggestion.

https://codereview.chromium.org/871753006/diff/1/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.h (right):

https://codereview.chromium.org/871753006/diff/1/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.h:45: AlternateProtocolInfo
alternate_protocol);
On 2015/01/31 01:17:48, Ryan Hamilton wrote:
> On 2015/01/31 00:32:01, Bence wrote:
> > On 2015/01/30 23:12:51, Ryan Hamilton wrote:
> > > I'm not sure that I'm a fan of moving this to the constructor. Since only
> > > alternate jobs (those which have had MarkAsAlternate called) should have
> > > alternatives it seems odd to pass in here, instead of when marking it
> > alternate.
> > > 
> > > Or, I guess, you could add both of these arguments to the constructor, but
> > that
> > > doesn't feel super awesome?
> > 
> > Well, if you look at
> >
>
https://codereview.chromium.org/665083009/diff/400001/net/http/http_stream_fa...,
> > you can see that both the alternate job and the non-alternate one must keep
a
> > copy of alternate_protocol, because they will need to pass it to
> > SetBrokenAlternateProtocol().  This is because each server will be allowed
to
> > have multiple AlternateProtocols, and SetBrokenAlternateProtocol() will need
> to
> > know which one to mark as broken.
> 
> I'm not sure that's the best way to accomplish this. It seems like that change
> makes alternate_protocol_ mean two different things. it's either the protocol
> being used by this jobs (if it's an alternate job) or else is that alternate
> protocol being used but the other job (if this is a main job). Instead, it
might
> make more sense to change MarkOtherJobComplete() so that it takes the
> AlternateProtocol (if any) from the other job and then save it in a member
like
> other_job_alternate_protocol_.

Excellent idea!  Done.

[Ryan Hamilton, 2015-02-02 18:18:51]

lgtm

[Bence, 2015-02-02 20:10:35]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2015-02-02 20:12:31]

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

[commit-bot: I haz the power, 2015-02-02 20:17:37]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-02 20:18:16]

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