Don't access .spec on a !is_valid PUSH_PROMISE URL.

Don't access .spec on a !is_valid PUSH_PROMISE URL.

This avoids a DCHECK, and also logs the failing URL, rather than ""
(the fallback behavior in the method's non-debug build).

BUG=688004
Review-Url: https://codereview.chromium.org/2673363003
Cr-Commit-Position: refs/heads/master@{#448708}
Committed: https://chromium.googlesource.com/chromium/src/+/ab1d1c1eb4095b86642845b826e6c43a1204be2a

[Maks Orlovich, 2017-02-06 19:00:56]

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

[commit-bot: I haz the power, 2017-02-06 19:01:29]

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

[Maks Orlovich, 2017-02-06 19:22:30]

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

[Maks Orlovich, 2017-02-06 19:22:31]

(I am not sure if behavior change wrt to logging is beneficial, but it certainly
seems to be the intent of the code...)

https://codereview.chromium.org/2673363003/diff/20001/net/spdy/spdy_network_t...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/2673363003/diff/20001/net/spdy/spdy_network_t...
net/spdy/spdy_network_transaction_unittest.cc:2893: NormalSpdyTransactionHelper
helper(CreateGetRequest(), DEFAULT_PRIORITY,
Lots of boilerplate overlap here with methods below and also RunServerPushTest
--- should I try to refactor it out? 
(Might be slightly tricky due to object lifetime..)

[commit-bot: I haz the power, 2017-02-06 20:04:11]

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

[commit-bot: I haz the power, 2017-02-06 20:04:12]

Dry run: This issue passed the CQ dry run.

[Bence, 2017-02-06 20:48:48]

LGTM.

https://codereview.chromium.org/2673363003/diff/20001/net/spdy/spdy_network_t...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/2673363003/diff/20001/net/spdy/spdy_network_t...
net/spdy/spdy_network_transaction_unittest.cc:2893: NormalSpdyTransactionHelper
helper(CreateGetRequest(), DEFAULT_PRIORITY,
On 2017/02/06 19:22:31, morlovich1 wrote:
> Lots of boilerplate overlap here with methods below and also RunServerPushTest
> --- should I try to refactor it out? 
> (Might be slightly tricky due to object lifetime..)

Whatever you think makes sense.  There's a lot of boilerplate in these tests
already anyway.  If there's an easy way to simplify tests that makes them easier
to read, then by all means, go for it!

https://codereview.chromium.org/2673363003/diff/20001/net/spdy/spdy_network_t...
net/spdy/spdy_network_transaction_unittest.cc:2904: rv =
callback.WaitForResult();
Did you not want to test the value of |rv| after this assignment?

[Maks Orlovich, 2017-02-07 15:34:30]

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

[commit-bot: I haz the power, 2017-02-07 15:34:53]

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

[Maks Orlovich, 2017-02-07 15:36:29]

PTAL --- did a simple refactor, which also included the missing |rv| test.

[commit-bot: I haz the power, 2017-02-07 16:32:53]

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

[commit-bot: I haz the power, 2017-02-07 16:32:53]

Dry run: This issue passed the CQ dry run.

[Bence, 2017-02-07 19:49:40]

LGTM, nice refactor, thank you!

[Maks Orlovich, 2017-02-07 19:53:34]

The CQ bit was checked by morlovich@chromium.org

[commit-bot: I haz the power, 2017-02-07 19:54:23]

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-02-07 19:59:34]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486497214873880,
"parent_rev": "12976b2107b692819f81041ba8c15a033dae1940", "commit_rev":
"ab1d1c1eb4095b86642845b826e6c43a1204be2a"}

[commit-bot: I haz the power, 2017-02-07 20:01:01]

Description was changed from

==========
Don't access .spec on a !is_valid PUSH_PROMISE URL.

This avoids a DCHECK, and also logs the failing URL, rather than ""
(the fallback behavior in the method's non-debug build).

BUG=688004
==========

to

==========
Don't access .spec on a !is_valid PUSH_PROMISE URL.

This avoids a DCHECK, and also logs the failing URL, rather than ""
(the fallback behavior in the method's non-debug build).

BUG=688004

Review-Url: https://codereview.chromium.org/2673363003
Cr-Commit-Position: refs/heads/master@{#448708}
Committed:
https://chromium.googlesource.com/chromium/src/+/ab1d1c1eb4095b86642845b826e6...
==========

[commit-bot: I haz the power, 2017-02-07 20:01:02]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/ab1d1c1eb4095b86642845b826e6...