Reset the Lo-Fi main frame state when there is a new main frame request

Reset the Lo-Fi main frame state when there is a new main frame request and
remove the Lo-Fi field trial.

BUG=607693
Committed: https://crrev.com/060808575a58a2a318f3d3231ef065d0571ca2e8
Cr-Commit-Position: refs/heads/master@{#392501}

[megjablon, 2016-05-04 01:36:18]

Description was changed from

==========
Reset the Lo-Fi main frame state when there is a new main frame request

BUG=607693
==========

to

==========
Reset the Lo-Fi main frame state when there is a new main frame request and 
remove the Lo-Fi field trial.

BUG=607693
==========

[megjablon, 2016-05-04 01:36:34]

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

[megjablon, 2016-05-04 01:36:35]

PTAL, thanks!

[tbansal1, 2016-05-04 01:46:23]

https://chromiumcodereview.appspot.com/1933653004/diff/1/components/data_redu...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(left):

https://chromiumcodereview.appspot.com/1933653004/diff/1/components/data_redu...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:173:
// TODO(megjablon): Need to switch to per page.
Why is this TODO gone? Seems like the Lo-Fi state is still stored in
DRP_settings and retrieved using WasLoFiModeActiveOnMainFrame() by Java?

[megjablon, 2016-05-04 03:02:44]

https://chromiumcodereview.appspot.com/1933653004/diff/1/components/data_redu...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(left):

https://chromiumcodereview.appspot.com/1933653004/diff/1/components/data_redu...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:173:
// TODO(megjablon): Need to switch to per page.
On 2016/05/04 01:46:23, tbansal1 wrote:
> Why is this TODO gone? Seems like the Lo-Fi state is still stored in
> DRP_settings and retrieved using WasLoFiModeActiveOnMainFrame() by Java?

This isn't going to be fixable. This TODO was added for the snackbar triggering
which has since been fixed by triggering on response. It is still being used by
context menus which are used much less than the snackbar, and the way a context
menu is generated has no tie to a page and we won't be putting in the effort to
fix this.

[tbansal1, 2016-05-04 05:16:57]

https://chromiumcodereview.appspot.com/1933653004/diff/1/components/data_redu...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(left):

https://chromiumcodereview.appspot.com/1933653004/diff/1/components/data_redu...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:173:
// TODO(megjablon): Need to switch to per page.
On 2016/05/04 03:02:44, megjablon wrote:
> On 2016/05/04 01:46:23, tbansal1 wrote:
> > Why is this TODO gone? Seems like the Lo-Fi state is still stored in
> > DRP_settings and retrieved using WasLoFiModeActiveOnMainFrame() by Java?
> 
> This isn't going to be fixable. This TODO was added for the snackbar
triggering
> which has since been fixed by triggering on response. It is still being used
by
> context menus which are used much less than the snackbar, and the way a
context
> menu is generated has no tie to a page and we won't be putting in the effort
to
> fix this.

Acknowledged.

https://chromiumcodereview.appspot.com/1933653004/diff/1/components/data_redu...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://chromiumcodereview.appspot.com/1933653004/diff/1/components/data_redu...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:160:
if ((request->load_flags() & net::LOAD_MAIN_FRAME)) {
braces not needed.

https://chromiumcodereview.appspot.com/1933653004/diff/1/components/data_redu...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:160:
if ((request->load_flags() & net::LOAD_MAIN_FRAME)) {
Is request guaranteed to be non-null here? I think so but please check.

https://chromiumcodereview.appspot.com/1933653004/diff/1/components/data_redu...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:171:
if (proxy_info.is_empty())
Is it useful to add a check here that proxy_info.proxy_server is DRP, and return
early if it is not?
Currently, it is checked separately two times below.

https://chromiumcodereview.appspot.com/1933653004/diff/1/components/data_redu...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h
(right):

https://chromiumcodereview.appspot.com/1933653004/diff/1/components/data_redu...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:16:
#include "net/base/completion_callback.h"
forward declaration should be enough since you are not calling any functions on
CompletionCallback object.

[megjablon, 2016-05-06 22:13:00]

https://chromiumcodereview.appspot.com/1933653004/diff/1/components/data_redu...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://chromiumcodereview.appspot.com/1933653004/diff/1/components/data_redu...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:160:
if ((request->load_flags() & net::LOAD_MAIN_FRAME)) {
On 2016/05/04 05:16:57, tbansal1 wrote:
> braces not needed.

Done.

https://chromiumcodereview.appspot.com/1933653004/diff/1/components/data_redu...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:160:
if ((request->load_flags() & net::LOAD_MAIN_FRAME)) {
On 2016/05/04 05:16:57, tbansal1 wrote:
> Is request guaranteed to be non-null here? I think so but please check.

Yes, see
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/network_d...

Added DCHECK

https://chromiumcodereview.appspot.com/1933653004/diff/1/components/data_redu...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:171:
if (proxy_info.is_empty())
On 2016/05/04 05:16:56, tbansal1 wrote:
> Is it useful to add a check here that proxy_info.proxy_server is DRP, and
return
> early if it is not?
> Currently, it is checked separately two times below.

Done.

https://chromiumcodereview.appspot.com/1933653004/diff/1/components/data_redu...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h
(right):

https://chromiumcodereview.appspot.com/1933653004/diff/1/components/data_redu...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:16:
#include "net/base/completion_callback.h"
On 2016/05/04 05:16:57, tbansal1 wrote:
> forward declaration should be enough since you are not calling any functions
on
> CompletionCallback object.

CompletionCallback is a typedef so we can't forward declare it.

[tbansal1, 2016-05-06 23:36:52]

few nits. Thanks for cleaning this up.

https://codereview.chromium.org/1933653004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h
(right):

https://codereview.chromium.org/1933653004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:16:
#include "net/base/completion_callback.h"
On 2016/05/06 22:13:00, megjablon wrote:
> On 2016/05/04 05:16:57, tbansal1 wrote:
> > forward declaration should be enough since you are not calling any functions
> on
> > CompletionCallback object.
> 
> CompletionCallback is a typedef so we can't forward declare it.

Acknowledged.

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

https://codereview.chromium.org/1933653004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:160:
DCHECK(request);
This DCHECK is not necessary. If |request| is null, the program will crash on
next line regardless.

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

https://codereview.chromium.org/1933653004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:163:
static void VerifyLoFiHeader(bool expected_data_reduction_proxy_used,
s/verifyLoFiHeader/VerifyHeaders/

https://codereview.chromium.org/1933653004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:167:
headers.HasHeader(kChromeProxyHeader));
#include
components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h

https://codereview.chromium.org/1933653004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:383:
tests[i].is_data_reduction_proxy, headers);
nit: For the second argument, can you not just pass true as before because in
verifyLoFiHeader, you are checking again that DRP is enabled?

https://codereview.chromium.org/1933653004/diff/20001/components/data_reducti...
File components/data_reduction_proxy/core/common/lofi_decider.h (right):

https://codereview.chromium.org/1933653004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/lofi_decider.h:31: // the |headers|.
Only adds this header if the provided |proxy_server| is a
nit: Update the comments.

[megjablon, 2016-05-07 00:29:08]

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

https://codereview.chromium.org/1933653004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:160:
DCHECK(request);
On 2016/05/06 23:36:51, tbansal1 wrote:
> This DCHECK is not necessary. If |request| is null, the program will crash on
> next line regardless.

Done.

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

https://codereview.chromium.org/1933653004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:163:
static void VerifyLoFiHeader(bool expected_data_reduction_proxy_used,
On 2016/05/06 23:36:51, tbansal1 wrote:
> s/verifyLoFiHeader/VerifyHeaders/

Done.

https://codereview.chromium.org/1933653004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:167:
headers.HasHeader(kChromeProxyHeader));
On 2016/05/06 23:36:51, tbansal1 wrote:
> #include
> components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h

Done.

https://codereview.chromium.org/1933653004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:383:
tests[i].is_data_reduction_proxy, headers);
On 2016/05/06 23:36:51, tbansal1 wrote:
> nit: For the second argument, can you not just pass true as before because in
> verifyLoFiHeader, you are checking again that DRP is enabled?

Whoops yep, missed switching this one.

https://codereview.chromium.org/1933653004/diff/20001/components/data_reducti...
File components/data_reduction_proxy/core/common/lofi_decider.h (right):

https://codereview.chromium.org/1933653004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/lofi_decider.h:31: // the |headers|.
Only adds this header if the provided |proxy_server| is a
On 2016/05/06 23:36:51, tbansal1 wrote:
> nit: Update the comments.

Done.

[tbansal1, 2016-05-07 00:31:20]

lgtm

[megjablon, 2016-05-09 17:30:14]

Patchset #4 (id:60001) has been deleted

[megjablon, 2016-05-09 21:33:04]

The CQ bit was checked by megjablon@chromium.org

[megjablon, 2016-05-09 21:33:05]

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/1933653004/#ps80001
(title: "test fix")

[commit-bot: I haz the power, 2016-05-09 21:33:46]

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

[commit-bot: I haz the power, 2016-05-09 23:25:28]

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

[commit-bot: I haz the power, 2016-05-09 23:25:31]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[megjablon, 2016-05-10 00:30:20]

The CQ bit was checked by megjablon@chromium.org

[megjablon, 2016-05-10 00:30:20]

The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1933653004/#ps100001 (title: "check
data_reduction_proxy_io_data_")

[commit-bot: I haz the power, 2016-05-10 00:30:41]

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

[commit-bot: I haz the power, 2016-05-10 00:56:15]

Description was changed from

==========
Reset the Lo-Fi main frame state when there is a new main frame request and 
remove the Lo-Fi field trial.

BUG=607693
==========

to

==========
Reset the Lo-Fi main frame state when there is a new main frame request and 
remove the Lo-Fi field trial.

BUG=607693
==========

[commit-bot: I haz the power, 2016-05-10 00:56:16]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-05-10 00:57:31]

Description was changed from

==========
Reset the Lo-Fi main frame state when there is a new main frame request and 
remove the Lo-Fi field trial.

BUG=607693
==========

to

==========
Reset the Lo-Fi main frame state when there is a new main frame request and
remove the Lo-Fi field trial.

BUG=607693

Committed: https://crrev.com/060808575a58a2a318f3d3231ef065d0571ca2e8
Cr-Commit-Position: refs/heads/master@{#392501}
==========

[commit-bot: I haz the power, 2016-05-10 00:57:32]

Patchset 5 (id:??) landed as
https://crrev.com/060808575a58a2a318f3d3231ef065d0571ca2e8
Cr-Commit-Position: refs/heads/master@{#392501}