Simplify the the logic for setting the final response status for a

Simplify the the logic for setting the final response status for a
QUIC HTTP stream, by centralizing the logic into a single method.

Review-Url: https://codereview.chromium.org/2777333002
Cr-Commit-Position: refs/heads/master@{#460590}
Committed: https://chromium.googlesource.com/chromium/src/+/4cbd5e215ed47201e8914cb93269051eaf41fc96

[Ryan Hamilton, 2017-03-27 21:57:15]

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

[Buck, 2017-03-28 21:19:57]

https://codereview.chromium.org/2777333002/diff/1/net/quic/chromium/quic_http...
File net/quic/chromium/quic_http_stream.cc (right):

https://codereview.chromium.org/2777333002/diff/1/net/quic/chromium/quic_http...
net/quic/chromium/quic_http_stream.cc:481: DoCallback(GetResponseStatus());
Why not use a cached result from the call above?  If there is a reason, perhaps
explain in a comment.

https://codereview.chromium.org/2777333002/diff/1/net/quic/chromium/quic_http...
net/quic/chromium/quic_http_stream.cc:858: // For bening errors, return
ERR_CONNECTION_CLOSED to permit
s/bening/benign/

https://codereview.chromium.org/2777333002/diff/1/net/quic/chromium/quic_http...
net/quic/chromium/quic_http_stream.cc:860: // headers have not been received,
and this is not the firs stream).
s/firs/first/

https://codereview.chromium.org/2777333002/diff/1/net/quic/chromium/quic_http...
File net/quic/chromium/quic_http_stream.h (right):

https://codereview.chromium.org/2777333002/diff/1/net/quic/chromium/quic_http...
net/quic/chromium/quic_http_stream.h:139: int GetResponseStatus();
There seem to be instances where the return value of GetResponseStatus is
deliberately ignored.    So the intent there is to make sure that
SetResponseStatus(ComputeResponseStatus()) is invoked.

I wonder if it would be cleaner to wrap the latter in a new method something
like :'void UpdateResponseStatus()'?

[Ryan Hamilton, 2017-03-28 21:54:04]

Thanks!

https://codereview.chromium.org/2777333002/diff/1/net/quic/chromium/quic_http...
File net/quic/chromium/quic_http_stream.cc (right):

https://codereview.chromium.org/2777333002/diff/1/net/quic/chromium/quic_http...
net/quic/chromium/quic_http_stream.cc:481: DoCallback(GetResponseStatus());
On 2017/03/28 21:19:57, Buck wrote:
> Why not use a cached result from the call above?  If there is a reason,
perhaps
> explain in a comment.

Done. Good idea!

(Well, and then it changed as a result of a different suggestion of yours.  but
still done :>)

https://codereview.chromium.org/2777333002/diff/1/net/quic/chromium/quic_http...
net/quic/chromium/quic_http_stream.cc:858: // For bening errors, return
ERR_CONNECTION_CLOSED to permit
On 2017/03/28 21:19:57, Buck wrote:
> s/bening/benign/

Done.

https://codereview.chromium.org/2777333002/diff/1/net/quic/chromium/quic_http...
net/quic/chromium/quic_http_stream.cc:860: // headers have not been received,
and this is not the firs stream).
On 2017/03/28 21:19:57, Buck wrote:
> s/firs/first/

Done.

https://codereview.chromium.org/2777333002/diff/1/net/quic/chromium/quic_http...
File net/quic/chromium/quic_http_stream.h (right):

https://codereview.chromium.org/2777333002/diff/1/net/quic/chromium/quic_http...
net/quic/chromium/quic_http_stream.h:139: int GetResponseStatus();
On 2017/03/28 21:19:57, Buck wrote:
> There seem to be instances where the return value of GetResponseStatus is
> deliberately ignored.    So the intent there is to make sure that
> SetResponseStatus(ComputeResponseStatus()) is invoked.
> 
> I wonder if it would be cleaner to wrap the latter in a new method something
> like :'void UpdateResponseStatus()'?
> 

Good idea. I thought about that but couldn't decide if I liked it. But since you
do, I'm sold! :)

[Buck, 2017-03-28 21:56:34]

lgtm

[Ryan Hamilton, 2017-03-28 22:28:33]

The CQ bit was checked by rch@chromium.org

[Ryan Hamilton, 2017-03-28 22:28:33]

The patchset sent to the CQ was uploaded after l-g-t-m from ckrasic@chromium.org
Link to the patchset: https://codereview.chromium.org/2777333002/#ps40001
(title: "Fix")

[commit-bot: I haz the power, 2017-03-28 22:29:15]

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

[Ryan Hamilton, 2017-03-28 22:34:34]

The CQ bit was unchecked by rch@chromium.org

[Ryan Hamilton, 2017-03-29 21:17:14]

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

[commit-bot: I haz the power, 2017-03-29 21:18:17]

Dry run: 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, 2017-03-29 23:46:35]

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

[commit-bot: I haz the power, 2017-03-29 23:46:36]

Dry run: This issue passed the CQ dry run.

[Ryan Hamilton, 2017-03-29 23:55:07]

The CQ bit was checked by rch@chromium.org

[Ryan Hamilton, 2017-03-29 23:55:07]

The patchset sent to the CQ was uploaded after l-g-t-m from ckrasic@chromium.org
Link to the patchset: https://codereview.chromium.org/2777333002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-29 23:56:04]

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, 2017-03-30 00:13:34]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490831707281590,
"parent_rev": "0d7958ef3d03590b0063fe6d284763132e718b3f", "commit_rev":
"4cbd5e215ed47201e8914cb93269051eaf41fc96"}

[commit-bot: I haz the power, 2017-03-30 00:14:37]

Description was changed from

==========
Simplify the the logic for setting the final response status for a
QUIC HTTP stream, by centralizing the logic into a single method.
==========

to

==========
Simplify the the logic for setting the final response status for a
QUIC HTTP stream, by centralizing the logic into a single method.

Review-Url: https://codereview.chromium.org/2777333002
Cr-Commit-Position: refs/heads/master@{#460590}
Committed:
https://chromium.googlesource.com/chromium/src/+/4cbd5e215ed47201e8914cb93269...
==========

[commit-bot: I haz the power, 2017-03-30 00:14:38]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/4cbd5e215ed47201e8914cb93269...