Add "q=preview" directive on main frame requests on poor networks

Add "q=preview" directive on main frame requests on poor networks

Similar to Lo-Fi, when a bad connection is detected, Chrome should add the
"q=preview" directive in the ChromeProxy header. However, this should only
be added for main frame requests.

BUG=558632
Committed: https://crrev.com/0a3af14a0cd39a7bc93c2b9c4993217f702e138a
Cr-Commit-Position: refs/heads/master@{#361459}

[megjablon, 2015-11-20 22:10:59]

Patchset #1 (id:1) has been deleted

[megjablon, 2015-11-20 23:24:37]

Patchset #1 (id:20001) has been deleted

[megjablon, 2015-11-20 23:25:03]

Description was changed from

==========
Add q=preview directive on main frame requests on poor networks

Similar to Lo-Fi, when a bad connection is detected, Chrome should add the
q=preview directive in the ChromeProxy header. However, this should only
be added for main frame requests.

BUG=558632
==========

to

==========
Add "q=preview" directive on main frame requests on poor networks

Similar to Lo-Fi, when a bad connection is detected, Chrome should add the
"q=preview" directive in the ChromeProxy header. However, this should only
be added for main frame requests.

BUG=558632
==========

[megjablon, 2015-11-20 23:25:31]

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

[tbansal1, 2015-11-23 22:07:05]

https://codereview.chromium.org/1455333004/diff/40001/components/data_reducti...
File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc
(right):

https://codereview.chromium.org/1455333004/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:30:
const char kPreview[] = "Preview";
May be change the name to Enabled_Preview

https://codereview.chromium.org/1455333004/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:84: }
Line break between functions.

https://codereview.chromium.org/1455333004/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:86:
return FieldTrialList::FindFullName(GetLoFiFieldTrialName()) == kEnabled ||
Change this to check if the field trial begins with "Enabled"

https://codereview.chromium.org/1455333004/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:135:
bool ShouldUseLoFiPreviews() {
s/ShouldUseLoFiPreviews/IsLoFiPreviewsEnabledViaFlags?

https://codereview.chromium.org/1455333004/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_switches.cc
(right):

https://codereview.chromium.org/1455333004/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_switches.cc:58:
"enable-data-reduction-proxy-lo-fi-previews";
s/previews/preview since that's what you are using as field trial group name OR
change name of field trial group.

[megjablon, 2015-11-24 01:23:48]

Patchset #2 (id:60001) has been deleted

[megjablon, 2015-11-24 01:30:41]

megjablon@chromium.org changed reviewers:
+ thestig@chromium.org
- sclittle@chromium.org

[megjablon, 2015-11-24 01:30:41]

thestig: chrome/*

https://chromiumcodereview.appspot.com/1455333004/diff/40001/components/data_...
File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc
(right):

https://chromiumcodereview.appspot.com/1455333004/diff/40001/components/data_...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:30:
const char kPreview[] = "Preview";
On 2015/11/23 22:07:05, tbansal1 wrote:
> May be change the name to Enabled_Preview

Done.

https://chromiumcodereview.appspot.com/1455333004/diff/40001/components/data_...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:84: }
On 2015/11/23 22:07:05, tbansal1 wrote:
> Line break between functions.

Done.

https://chromiumcodereview.appspot.com/1455333004/diff/40001/components/data_...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:86:
return FieldTrialList::FindFullName(GetLoFiFieldTrialName()) == kEnabled ||
On 2015/11/23 22:07:05, tbansal1 wrote:
> Change this to check if the field trial begins with "Enabled"

Done.

https://chromiumcodereview.appspot.com/1455333004/diff/40001/components/data_...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:135:
bool ShouldUseLoFiPreviews() {
On 2015/11/23 22:07:05, tbansal1 wrote:
> s/ShouldUseLoFiPreviews/IsLoFiPreviewsEnabledViaFlags?

Done.

https://chromiumcodereview.appspot.com/1455333004/diff/40001/components/data_...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_switches.cc
(right):

https://chromiumcodereview.appspot.com/1455333004/diff/40001/components/data_...
components/data_reduction_proxy/core/common/data_reduction_proxy_switches.cc:58:
"enable-data-reduction-proxy-lo-fi-previews";
On 2015/11/23 22:07:05, tbansal1 wrote:
> s/previews/preview since that's what you are using as field trial group name
OR
> change name of field trial group.

Done.

[tbansal1, 2015-11-24 01:45:19]

lgtm

[Lei Zhang, 2015-11-24 02:08:35]

https://chromiumcodereview.appspot.com/1455333004/diff/80001/chrome/browser/a...
File chrome/browser/about_flags.cc (right):

https://chromiumcodereview.appspot.com/1455333004/diff/80001/chrome/browser/a...
chrome/browser/about_flags.cc:1786:
{"enable-data-reduction-proxy-lo-fi-preview",
I think the bots are complaining about the missing entry for this in
histograms.xml.

https://chromiumcodereview.appspot.com/1455333004/diff/80001/chrome/browser/a...
chrome/browser/about_flags.cc:1790:
SINGLE_VALUE_TYPE(data_reduction_proxy::switches::
Be consistent with the formatting in the next entry below?

https://chromiumcodereview.appspot.com/1455333004/diff/80001/components/data_...
File components/data_reduction_proxy/content/browser/content_lofi_decider.cc
(right):

https://chromiumcodereview.appspot.com/1455333004/diff/80001/components/data_...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:68:
(request.load_flags() & net::LOAD_MAIN_FRAME) != 0) {
nit: you don't need the != 0, right?

https://chromiumcodereview.appspot.com/1455333004/diff/80001/components/data_...
File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc
(right):

https://chromiumcodereview.appspot.com/1455333004/diff/80001/components/data_...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:87:
return FieldTrialList::FindFullName(GetLoFiFieldTrialName())
Is the following more readable?

std::string name = FieldTrialList::FindFullName(GetLoFiFieldTrialName());
return base::StartsWith(name, kEnabled, base::CompareCase::SENSITIVE);

https://chromiumcodereview.appspot.com/1455333004/diff/80001/components/data_...
File components/data_reduction_proxy/core/common/data_reduction_proxy_switches.h
(right):

https://chromiumcodereview.appspot.com/1455333004/diff/80001/components/data_...
components/data_reduction_proxy/core/common/data_reduction_proxy_switches.h:28:
extern const char kEnableDataReductionProxyLoFiPreview[];
Alphabetical order, please. Bonus points for sorting the whole file.

[megjablon, 2015-11-24 18:41:16]

Patchset #3 (id:100001) has been deleted

[megjablon, 2015-11-24 18:41:58]

megjablon@chromium.org changed reviewers:
+ asvitkine@google.com

[megjablon, 2015-11-24 18:41:58]

asvitkine: histograms.xml

https://chromiumcodereview.appspot.com/1455333004/diff/80001/chrome/browser/a...
File chrome/browser/about_flags.cc (right):

https://chromiumcodereview.appspot.com/1455333004/diff/80001/chrome/browser/a...
chrome/browser/about_flags.cc:1786:
{"enable-data-reduction-proxy-lo-fi-preview",
On 2015/11/24 02:08:35, Lei Zhang wrote:
> I think the bots are complaining about the missing entry for this in
> histograms.xml.

Done.

https://chromiumcodereview.appspot.com/1455333004/diff/80001/chrome/browser/a...
chrome/browser/about_flags.cc:1790:
SINGLE_VALUE_TYPE(data_reduction_proxy::switches::
On 2015/11/24 02:08:35, Lei Zhang wrote:
> Be consistent with the formatting in the next entry below?

Done.

https://chromiumcodereview.appspot.com/1455333004/diff/80001/components/data_...
File components/data_reduction_proxy/content/browser/content_lofi_decider.cc
(right):

https://chromiumcodereview.appspot.com/1455333004/diff/80001/components/data_...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:68:
(request.load_flags() & net::LOAD_MAIN_FRAME) != 0) {
On 2015/11/24 02:08:35, Lei Zhang wrote:
> nit: you don't need the != 0, right?

Done.

https://chromiumcodereview.appspot.com/1455333004/diff/80001/components/data_...
File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc
(right):

https://chromiumcodereview.appspot.com/1455333004/diff/80001/components/data_...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:87:
return FieldTrialList::FindFullName(GetLoFiFieldTrialName())
On 2015/11/24 02:08:35, Lei Zhang wrote:
> Is the following more readable?
> 
> std::string name = FieldTrialList::FindFullName(GetLoFiFieldTrialName());
> return base::StartsWith(name, kEnabled, base::CompareCase::SENSITIVE);

Yes, I like this much better. Thanks!

https://chromiumcodereview.appspot.com/1455333004/diff/80001/components/data_...
File components/data_reduction_proxy/core/common/data_reduction_proxy_switches.h
(right):

https://chromiumcodereview.appspot.com/1455333004/diff/80001/components/data_...
components/data_reduction_proxy/core/common/data_reduction_proxy_switches.h:28:
extern const char kEnableDataReductionProxyLoFiPreview[];
On 2015/11/24 02:08:35, Lei Zhang wrote:
> Alphabetical order, please. Bonus points for sorting the whole file.

Done.

[Lei Zhang, 2015-11-24 18:47:08]

lgtm

[Alexei Svitkine (slow), 2015-11-24 18:57:06]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2015-11-24 18:57:07]

https://chromiumcodereview.appspot.com/1455333004/diff/90012/chrome/browser/a...
File chrome/browser/about_flags.cc (right):

https://chromiumcodereview.appspot.com/1455333004/diff/90012/chrome/browser/a...
chrome/browser/about_flags.cc:1791:
data_reduction_proxy::switches::kEnableDataReductionProxyLoFiPreview)},
Given this is a new flag, I suggest using the base/feature_list.h API for this
instead.

See announcement here:

https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/Esavy4-JTHY

[megjablon, 2015-11-24 19:23:18]

megjablon@chromium.org changed reviewers:
- asvitkine@google.com

[megjablon, 2015-11-24 19:38:12]

https://chromiumcodereview.appspot.com/1455333004/diff/90012/chrome/browser/a...
File chrome/browser/about_flags.cc (right):

https://chromiumcodereview.appspot.com/1455333004/diff/90012/chrome/browser/a...
chrome/browser/about_flags.cc:1791:
data_reduction_proxy::switches::kEnableDataReductionProxyLoFiPreview)},
On 2015/11/24 18:57:06, Alexei Svitkine (slow) wrote:
> Given this is a new flag, I suggest using the base/feature_list.h API for this
> instead.
> 
> See announcement here:
> 
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/Esavy4-JTHY

For organization purposes, I'd like to the keep the switch the way it is now.
This is layered on top of the lo-fi switch, and it is confusing to enable the
two switches in different ways. Also, this feature does not need a disable
switch.

[Alexei Svitkine (slow), 2015-11-24 19:45:52]

fair enough, lgtm

[megjablon, 2015-11-24 20:16:05]

The CQ bit was checked by megjablon@chromium.org

[megjablon, 2015-11-24 20:16:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
tbansal@chromium.org, thestig@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1455333004/#ps130001 (title: "limit flag
channels")

[commit-bot: I haz the power, 2015-11-24 20:18:55]

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

[commit-bot: I haz the power, 2015-11-24 21:54:59]

Committed patchset #4 (id:130001)

[commit-bot: I haz the power, 2015-11-24 21:55:59]

Patchset 4 (id:??) landed as
https://crrev.com/0a3af14a0cd39a7bc93c2b9c4993217f702e138a
Cr-Commit-Position: refs/heads/master@{#361459}