JobController3: Move MarkAlternativeServiceBroken to job controller

JobController3: Move MarkAlternativeServiceBroken to job controller

BUG=

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

[Zhongyi Shi, 2016-09-13 01:58:06]

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

[Zhongyi Shi, 2016-09-13 01:58:07]

Ryan:

The unittests in quic_network_transaction_unittest.cc already covers this
change:
https://cs.chromium.org/search/?q=ExpectBrokenAlternateProtocolMapping&sq=pac...

ptal!

[Ryan Hamilton, 2016-09-13 04:17:04]

https://codereview.chromium.org/2332193003/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/2332193003/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job.cc:616:
delegate_->OnAlternativeProxyServerBroken(this);
Instead of requiring the job to explicitly notify the delegate of broken-ness,
it seems like it would be cleaner to have the controller figure this out via
OnStreamFailed (called from OnStreamFailedCallback()). Would that work?

https://codereview.chromium.org/2332193003/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job.h (right):

https://codereview.chromium.org/2332193003/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job.h:51: // Invoked when |job| has
alternative service identified borken.
s/borken/broken/ (and below)

[tbansal1, 2016-09-13 05:06:44]

tbansal@chromium.org changed reviewers:
+ tbansal@chromium.org

[tbansal1, 2016-09-13 05:06:45]

https://codereview.chromium.org/2332193003/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/2332193003/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job.cc:209: job_status_(STATUS_RUNNING),
It seems that job_status_, other_job_status_ and
other_job_alternative_proxy_server_ can all be removed?

https://codereview.chromium.org/2332193003/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job.h (right):

https://codereview.chromium.org/2332193003/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job.h:246: const AlternativeService
alternative_service() {
Can this method be a const method? Same below.

https://codereview.chromium.org/2332193003/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2332193003/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:782:
proxy_delegate->OnAlternativeProxyBroken(
nit: missing parenthesis.

https://codereview.chromium.org/2332193003/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller.h (right):

https://codereview.chromium.org/2332193003/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.h:270: bool
alternative_service_is_broken_;
Is it possible to remove the bool variable since 
alternative_service_is_broken_ should be true iff broken_alternative_service_ is
a valid alternative service.

Same comment for the bool alternative_proxy_server_is_broken_ below.

[Zhongyi Shi, 2016-09-13 22:52:37]

Move code to let the controller cache alt service broken OnStreamFailed.

https://codereview.chromium.org/2332193003/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/2332193003/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job.cc:209: job_status_(STATUS_RUNNING),
On 2016/09/13 05:06:45, tbansal1 wrote:
> It seems that job_status_, other_job_status_ and
> other_job_alternative_proxy_server_ can all be removed?

Done.

https://codereview.chromium.org/2332193003/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job.h (right):

https://codereview.chromium.org/2332193003/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job.h:51: // Invoked when |job| has
alternative service identified borken.
On 2016/09/13 04:17:04, Ryan Hamilton wrote:
> s/borken/broken/ (and below)

Done.

https://codereview.chromium.org/2332193003/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job.h:246: const AlternativeService
alternative_service() {
On 2016/09/13 05:06:45, tbansal1 wrote:
> Can this method be a const method? Same below.

Done.

[Ryan Hamilton, 2016-09-13 23:23:16]

https://codereview.chromium.org/2332193003/diff/40001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job.h (right):

https://codereview.chromium.org/2332193003/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job.h:423: bool alternative_broken_;
Is this different than the job failing?

https://codereview.chromium.org/2332193003/diff/40001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2332193003/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:774: return;
As discussed offline, I think we need to make sure that we mark QUIC broken when
TCP succeed and QUIC fails, regardless of the order they happen in. Perhaps we
need some Controller unit tests to verify this? (And that we don't mark broken
if both fail)

[Zhongyi Shi, 2016-09-14 20:54:02]

Feel free to bug me if you need help to review the code. PTAL!

[Ryan Hamilton, 2016-09-14 21:45:42]

https://codereview.chromium.org/2332193003/diff/80001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2332193003/diff/80001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:249: if (request_) {
I'm having a hard time following this. I think that this is trying to handle the
case where the main job has already succeeded. Is that right?

https://codereview.chromium.org/2332193003/diff/80001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:778: Job* job) {
This |job| argument is unused, right? If so, it's nuke it.

https://codereview.chromium.org/2332193003/diff/80001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller.h (right):

https://codereview.chromium.org/2332193003/diff/80001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.h:209: void
ReportAlternativeServiceBroken(Job* job);
nit: How about "ReportBrokenAlternativeService", which reads ever so slightly
more like a sentence.

https://codereview.chromium.org/2332193003/diff/80001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.h:267: bool
alternative_service_is_broken_;
Instead of "broken", let's use "failed" here. Broken is only the state we reach
when alt-svc fails and main succeeds. (and in the members below)

That would also suggest renaming MarkAlternativeServiceBroken to
MarkAlternativeServiceJobFailed. (Or perhaps OnAlternativeServiceJobFailed).

[Zhongyi Shi, 2016-09-14 23:04:51]

Patchset #6 (id:100001) has been deleted

[Zhongyi Shi, 2016-09-14 23:07:07]

Addressing Ryan's nits.

https://codereview.chromium.org/2332193003/diff/80001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2332193003/diff/80001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:249: if (request_) {
On 2016/09/14 21:45:42, Ryan Hamilton wrote:
> I'm having a hard time following this. I think that this is trying to handle
the
> case where the main job has already succeeded. Is that right? 

Yup. I have moved the logic up to OnAlternativeJobFailed.

https://codereview.chromium.org/2332193003/diff/80001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:778: Job* job) {
On 2016/09/14 21:45:42, Ryan Hamilton wrote:
> This |job| argument is unused, right? If so, it's nuke it.

Done.

https://codereview.chromium.org/2332193003/diff/80001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller.h (right):

https://codereview.chromium.org/2332193003/diff/80001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.h:209: void
ReportAlternativeServiceBroken(Job* job);
On 2016/09/14 21:45:42, Ryan Hamilton wrote:
> nit: How about "ReportBrokenAlternativeService", which reads ever so slightly
> more like a sentence.

Done.

https://codereview.chromium.org/2332193003/diff/80001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.h:267: bool
alternative_service_is_broken_;
On 2016/09/14 21:45:42, Ryan Hamilton wrote:
> Instead of "broken", let's use "failed" here. Broken is only the state we
reach
> when alt-svc fails and main succeeds. (and in the members below)
> 
> That would also suggest renaming MarkAlternativeServiceBroken to
> MarkAlternativeServiceJobFailed. (Or perhaps OnAlternativeServiceJobFailed).

Done.

[Ryan Hamilton, 2016-09-14 23:13:04]

lgtm modulo a couple of nits

https://codereview.chromium.org/2332193003/diff/120001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2332193003/diff/120001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller.cc:775: // |main_job_| must
succeed.
 // If |request_| is gone then it must have been successfully served by the main
job.
 // If the the request is bound to a different job, then it is being
successfully
 // served by the main job.

https://codereview.chromium.org/2332193003/diff/120001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job_controller.h (right):

https://codereview.chromium.org/2332193003/diff/120001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller.h:267: bool
alternative_job_is_failed_;
nit: remove "_is"

https://codereview.chromium.org/2332193003/diff/120001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller.h:272: ProxyServer
broken_alternative_proxy_server_;
nit: can you use failed instead of broken here too? (both the variables and the
comments)

[Zhongyi Shi, 2016-09-14 23:29:18]

The CQ bit was checked by zhongyi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 23:29:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Zhongyi Shi, 2016-09-14 23:30:33]

https://codereview.chromium.org/2332193003/diff/120001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job_controller.h (right):

https://codereview.chromium.org/2332193003/diff/120001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller.h:267: bool
alternative_job_is_failed_;
On 2016/09/14 23:13:04, Ryan Hamilton wrote:
> nit: remove "_is"

Done.

https://codereview.chromium.org/2332193003/diff/120001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job_controller.h:272: ProxyServer
broken_alternative_proxy_server_;
On 2016/09/14 23:13:04, Ryan Hamilton wrote:
> nit: can you use failed instead of broken here too? (both the variables and
the
> comments)

Done.

[commit-bot: I haz the power, 2016-09-15 01:50:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-15 01:50:29]

Dry run: This issue passed the CQ dry run.

[Zhongyi Shi, 2016-09-15 02:57:51]

The CQ bit was checked by zhongyi@chromium.org

[Zhongyi Shi, 2016-09-15 02:57:51]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org
Link to the patchset: https://codereview.chromium.org/2332193003/#ps140001
(title: "nits")

[commit-bot: I haz the power, 2016-09-15 02:58:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 03:03:14]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-09-15 03:05:32]

Description was changed from

==========
JobController3: Move MarkAlternativeServiceBroken to job controller

BUG=
==========

to

==========
JobController3: Move MarkAlternativeServiceBroken to job controller

BUG=

Committed: https://crrev.com/a692903a8ea6b8bfa576a29d64b413aeae641432
Cr-Commit-Position: refs/heads/master@{#418768}
==========

[commit-bot: I haz the power, 2016-09-15 03:05:33]

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