net: Avoid recursion in URLRequestJob::ReadFilteredData.

net: Avoid recursion in URLRequestJob::ReadFilteredData.

There's no need to use recursion when getting the data from a filter.
This version uses a loop instead.

The issue with recursion is that it opens the door to stack exhaustion
if there is no way to limit its depth. In this case, that would be a
hard thing to do given that it basically depends on interactions outside
of our control.

BUG=366149
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268030

[rvargas (doing something else), 2014-05-02 19:00:45]

PTAL

[wtc, 2014-05-02 22:59:13]

Patch set 1 LGTM.

The CL's description should explain why recursion is bad.

(I reviewed this code and at that time I thought the use of recursion was clever
:-) )

https://codereview.chromium.org/267793002/diff/1/net/url_request/url_request_...
File net/url_request/url_request_job.cc (left):

https://codereview.chromium.org/267793002/diff/1/net/url_request/url_request_...
net/url_request/url_request_job.cc:648: DVLOG(2) << __FUNCTION__ << "() "

Just wanted to confirm that you realize you deleted this DVLOG(2) statement.

https://codereview.chromium.org/267793002/diff/1/net/url_request/url_request_...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/267793002/diff/1/net/url_request/url_request_...
net/url_request/url_request_job.cc:660: } while(read_again);

1. Nit: add a space between "while" and "(".

2. Since you always follow "read_again = true" with "continue", this loop can be
rewritten without the use of read_again:

  for (;;) {
    ...
    if (...) {
      ...
      continue;
    }
    ...
    break;
  }

https://codereview.chromium.org/267793002/diff/1/net/url_request/url_request_...
File net/url_request/url_request_job_unittest.cc (right):

https://codereview.chromium.org/267793002/diff/1/net/url_request/url_request_...
net/url_request/url_request_job_unittest.cc:95: TEST(URLRequestJob,
SyncTransactionNotifiedWhenDone) {

The new SyncSlowTransaction test seems most similar to this test. Would it be
better to list SyncSlowTransaction right after this test?

[rvargas (doing something else), 2014-05-03 00:19:33]

Thanks!

https://codereview.chromium.org/267793002/diff/1/net/url_request/url_request_...
File net/url_request/url_request_job.cc (left):

https://codereview.chromium.org/267793002/diff/1/net/url_request/url_request_...
net/url_request/url_request_job.cc:648: DVLOG(2) << __FUNCTION__ << "() "
On 2014/05/02 22:59:13, wtc wrote:
> 
> Just wanted to confirm that you realize you deleted this DVLOG(2) statement.

Yes, I'm not a fan of leaving debug info forever in the code... I think if
someone needs to track this it's trivial to add it to a local build (which is
needed anyway given that we don't have pre-built debug builds).

https://codereview.chromium.org/267793002/diff/1/net/url_request/url_request_...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/267793002/diff/1/net/url_request/url_request_...
net/url_request/url_request_job.cc:660: } while(read_again);
On 2014/05/02 22:59:13, wtc wrote:
> 
> 1. Nit: add a space between "while" and "(".
> 
> 2. Since you always follow "read_again = true" with "continue", this loop can
be
> rewritten without the use of read_again:
> 
>   for (;;) {
>     ...
>     if (...) {
>       ...
>       continue;
>     }
>     ...
>     break;
>   }

I thought about that before... they are pretty much the same to me so I rewrote
it as a for.

https://codereview.chromium.org/267793002/diff/1/net/url_request/url_request_...
File net/url_request/url_request_job_unittest.cc (right):

https://codereview.chromium.org/267793002/diff/1/net/url_request/url_request_...
net/url_request/url_request_job_unittest.cc:95: TEST(URLRequestJob,
SyncTransactionNotifiedWhenDone) {
On 2014/05/02 22:59:13, wtc wrote:
> 
> The new SyncSlowTransaction test seems most similar to this test. Would it be
> better to list SyncSlowTransaction right after this test?

Done.

[rvargas (doing something else), 2014-05-03 01:52:16]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-05-03 01:53:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rvargas@chromium.org/267793002/10001

[commit-bot: I haz the power, 2014-05-03 03:30:08]

Change committed as 268030