Fixing redirect DataReductionProxyNewtorkDelegate behavior

Fixing redirect DataReductionProxyNewtorkDelegate behavior

The current behavior to clear out DataReductionProxyData from
URLRequest's UserData was based on OnBeforeSendHeaders being called for
the request after a redirect. However, when the resource is cached, the
DataReductionProxyData won't be cleared. This CL aims to fix this by
moving the code that clears the DRPData to OnBeforeRedirect, which is
called for every redirect response.

BUG=686367
Review-Url: https://codereview.chromium.org/2664983002
Cr-Commit-Position: refs/heads/master@{#447125}
Committed: https://chromium.googlesource.com/chromium/src/+/ee1900abec9b62a0657726f5f238ed7123df7d97

[RyanSturm, 2017-01-30 22:02:49]

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

[RyanSturm, 2017-01-30 22:04:03]

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

[RyanSturm, 2017-01-30 22:04:04]

tbansal: PTAL

[commit-bot: I haz the power, 2017-01-30 22:04:32]

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

[RyanSturm, 2017-01-30 22:07:58]

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

[commit-bot: I haz the power, 2017-01-30 22:08:45]

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

[tbansal1, 2017-01-30 22:23:19]

lgtm % nits.

https://codereview.chromium.org/2664983002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(left):

https://codereview.chromium.org/2664983002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:208:
// If this is after a redirect, reset |request|'s DataReductionProxyData.
Why remove from here? Either add this back, or add a DCHECK which ensures that
DRP data is already cleared when this method is executed.

https://codereview.chromium.org/2664983002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/2664983002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:793:
network_delegate()->NotifyBeforeRedirect(request.get(), GURL(kTestURL));
It might be useful to add a more end-to-end test that ensures that redirect() is
called before NotifyBeforeSendHeaders().

Can you file a bug and do it in a separate CL?

[commit-bot: I haz the power, 2017-01-30 22:47:27]

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

[commit-bot: I haz the power, 2017-01-30 22:47:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[RyanSturm, 2017-01-30 22:51:44]

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

[commit-bot: I haz the power, 2017-01-30 22:52:18]

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

[RyanSturm, 2017-01-30 22:59:03]

https://codereview.chromium.org/2664983002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(left):

https://codereview.chromium.org/2664983002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:208:
// If this is after a redirect, reset |request|'s DataReductionProxyData.
On 2017/01/30 22:23:19, tbansal1 wrote:
> Why remove from here? Either add this back, or add a DCHECK which ensures that
> DRP data is already cleared when this method is executed.

Done.

https://codereview.chromium.org/2664983002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/2664983002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:793:
network_delegate()->NotifyBeforeRedirect(request.get(), GURL(kTestURL));
On 2017/01/30 22:23:19, tbansal1 wrote:
> It might be useful to add a more end-to-end test that ensures that redirect()
is
> called before NotifyBeforeSendHeaders().
> 
> Can you file a bug and do it in a separate CL?

crbug.com/686916

[RyanSturm, 2017-01-30 23:01:31]

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

[commit-bot: I haz the power, 2017-01-30 23:02:46]

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-01-31 00:03:07]

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

[commit-bot: I haz the power, 2017-01-31 00:03:07]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2017-01-31 00:17:31]

The CQ bit was checked by ryansturm@chromium.org

[RyanSturm, 2017-01-31 00:17:32]

The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/2664983002/#ps60001
(title: "typo")

[commit-bot: I haz the power, 2017-01-31 00:18:05]

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-01-31 00:22:13]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485821851597380,
"parent_rev": "da69ab087bf5caa9e629401499691cb23cb532dc", "commit_rev":
"ee1900abec9b62a0657726f5f238ed7123df7d97"}

[commit-bot: I haz the power, 2017-01-31 00:22:46]

Description was changed from

==========
Fixing redirect DataReductionProxyNewtorkDelegate behavior

The current behavior to clear out DataReductionProxyData from
URLRequest's UserData was based on OnBeforeSendHeaders being called for
the request after a redirect. However, when the resource is cached, the
DataReductionProxyData won't be cleared. This CL aims to fix this by
moving the code that clears the DRPData to OnBeforeRedirect, which is
called for every redirect response.

BUG=686367
==========

to

==========
Fixing redirect DataReductionProxyNewtorkDelegate behavior

The current behavior to clear out DataReductionProxyData from
URLRequest's UserData was based on OnBeforeSendHeaders being called for
the request after a redirect. However, when the resource is cached, the
DataReductionProxyData won't be cleared. This CL aims to fix this by
moving the code that clears the DRPData to OnBeforeRedirect, which is
called for every redirect response.

BUG=686367

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

[commit-bot: I haz the power, 2017-01-31 00:22:48]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/ee1900abec9b62a0657726f5f238...