HTTP cache: Add tests to improve coverage of AddTruncatedFlag.

HTTP cache: Add tests to improve coverage of AddTruncatedFlag.

There's also a small change in the logic that sets done_reading_, which
is used to determine if the truncated flag should be set or not, and should
not be used for sparse entries.

BUG=

Committed: https://crrev.com/1c7570e7aafdebddd7c97f7ad9381826d7928c0d
Cr-Commit-Position: refs/heads/master@{#349528}

[rvargas (doing something else), 2015-09-16 01:16:24]

rvargas@chromium.org changed reviewers:
+ asanka@chromium.org

[rvargas (doing something else), 2015-09-16 01:16:24]

PTAL

[asanka, 2015-09-17 14:43:34]

lgtm % nits

https://codereview.chromium.org/1346793002/diff/1/net/http/http_cache_transac...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/1346793002/diff/1/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:1878: } else if (!done_reading_ && entry_ &&
(!partial_ || truncated_)) {
I'll note that this change doesn't affect the newly added tests. Perhaps with
this change, we can make done_reading_ be a stronger guarantee so that once the
SM moves past this point, there's no longer a need to check for partial_ or
sparse_ to decide whether to mark the entry as truncated.

https://codereview.chromium.org/1346793002/diff/1/net/http/http_cache_unittes...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/1346793002/diff/1/net/http/http_cache_unittes...
net/http/http_cache_unittest.cc:5642: void VerifyTruncatedFlag(MockHttpCache*
cache,
Nit: I'd move this to the top since it's not specific to the test or cluster
immediately following.

(Though I admit I might occasionally forget follow this guidance myself)

[rvargas (doing something else), 2015-09-17 20:18:38]

Thanks.

https://codereview.chromium.org/1346793002/diff/1/net/http/http_cache_transac...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/1346793002/diff/1/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:1878: } else if (!done_reading_ && entry_ &&
(!partial_ || truncated_)) {
On 2015/09/17 14:43:34, asanka wrote:
> I'll note that this change doesn't affect the newly added tests. Perhaps with
> this change, we can make done_reading_ be a stronger guarantee so that once
the
> SM moves past this point, there's no longer a need to check for partial_ or
> sparse_ to decide whether to mark the entry as truncated.

These tests are actually a regression test for
https://codereview.chromium.org/1253173002/ (I believe you based your CL between
the regression and the fix).

One of the tests I'm adding doesn't fail without this line... the other fails
all the time.

There's certainly room for improvement in the meaning of the flags, and when
they are valid :). What I'm saying with this line is that (so far), byte range
request imply specific logic to figure out when we are done, not what we do here
for done_reading_ (which is quite permissive, due to buggy servers).

https://codereview.chromium.org/1346793002/diff/1/net/http/http_cache_unittes...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/1346793002/diff/1/net/http/http_cache_unittes...
net/http/http_cache_unittest.cc:5642: void VerifyTruncatedFlag(MockHttpCache*
cache,
On 2015/09/17 14:43:34, asanka wrote:
> Nit: I'd move this to the top since it's not specific to the test or cluster
> immediately following.
> 
> (Though I admit I might occasionally forget follow this guidance myself)

I only placed it here 'cause I saw you doing that :P

done.

[asanka, 2015-09-17 22:26:00]

lgtm

https://codereview.chromium.org/1346793002/diff/1/net/http/http_cache_unittes...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/1346793002/diff/1/net/http/http_cache_unittes...
net/http/http_cache_unittest.cc:5642: void VerifyTruncatedFlag(MockHttpCache*
cache,
On 2015/09/17 at 20:18:37, rvargas (slow to respond) wrote:
> On 2015/09/17 14:43:34, asanka wrote:
> > Nit: I'd move this to the top since it's not specific to the test or cluster
> > immediately following.
> > 
> > (Though I admit I might occasionally forget follow this guidance myself)
> 
> I only placed it here 'cause I saw you doing that :P
> 
> done.

Hehe. I caught one of these strays in my CL and moved it. My general rule still
stands though (that some accessory function or class that's specific to a test
gets placed adjacent to the test instead of at the top of the file).

[rvargas (doing something else), 2015-09-17 22:26:07]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2015-09-17 22:26:53]

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

[commit-bot: I haz the power, 2015-09-17 23:05:55]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-09-17 23:06:26]

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