New CPAT support in ContentLoFiDecider guarded by feature flag.

New CPAT support in ContentLoFiDecider guarded by feature flag.
Updates MaybeSetAcceptTransformHeader to set accept header based
only on PreviewsState (if new feature enabled) vs. checking various
other flags. Also new path will not add "ifheavy" qualifier.

This is step 2 of 4 in adding new CPAT protocol support.
Step 1 is in CL 2889993004.
Step 3 is in CL 2903453003.

BUG=701802
Review-Url: https://codereview.chromium.org/2911673002
Cr-Commit-Position: refs/heads/master@{#476678}
Committed: https://chromium.googlesource.com/chromium/src/+/f134cffafa9fca0640bacbb56d9de9af0a8f25aa

[dougarnett, 2017-05-26 15:55:36]

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

[commit-bot: I haz the power, 2017-05-26 15:55:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougarnett, 2017-05-26 16:00:07]

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

[commit-bot: I haz the power, 2017-05-26 16:00:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougarnett, 2017-05-26 16:33:21]

dougarnett@chromium.org changed reviewers:
+ bengr@chromium.org, megjablon@chromium.org

[dougarnett, 2017-05-26 16:55:25]

Description was changed from

==========
New CPAT support in ContentLoFiDecider guarded by feature flag.
Updates MaybeSetAcceptTransformHeader to set accept header based
only on PreviewsState (if new feature enabled) vs. checking various
other flags. Also new path will not add "ifheavy" qualifier.

This is step 2 of 4 in adding new CPAT protocol support.
Step 1 is in CL 2889993004.

BUG=701802
==========

to

==========
New CPAT support in ContentLoFiDecider guarded by feature flag.
Updates MaybeSetAcceptTransformHeader to set accept header based
only on PreviewsState (if new feature enabled) vs. checking various
other flags. Also new path will not add "ifheavy" qualifier.

This is step 2 of 4 in adding new CPAT protocol support.
Step 1 is in CL 2889993004.
Step 3 is in CL 2903453003.

BUG=701802
==========

[commit-bot: I haz the power, 2017-05-26 17:02:42]

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

[commit-bot: I haz the power, 2017-05-26 17:02:43]

Dry run: This issue passed the CQ dry run.

[megjablon, 2017-05-30 18:38:53]

https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti...
File components/data_reduction_proxy/content/browser/content_lofi_decider.cc
(right):

https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:73:
content::PreviewsState previews_state = request_info->GetPreviewsState();
I'd move this line above the comment.

https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:74: if
(previews_state & content::PREVIEWS_NO_TRANSFORM) {
If you want, you can remove the brackets here.

https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:78: //
For the proxy-decides-transform feature, we determine whether to set
Don't use we. "the header is set based only on the request's PreviewsState"

https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:85: bool
decide_by_previews_state = base::FeatureList::IsEnabled(
Maybe call this check_previews_flags and use the inverse since we still use the
previews state in the old world and this implies that we don't.

bool check_previews_flags = !base::FeatureList::IsEnabled(

Also import src/base/feature_list.h

https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:127:
resource_type == content::RESOURCE_TYPE_MAIN_FRAME) {
Do we want:

if (previews_state & content::SERVER_LITE_PAGE_ON) {
  if (resource_type == content::RESOURCE_TYPE_MAIN_FRAME)
    accept_transform_value = lite_page_directive();
  }
}

With what you have here we have to guarantee the Lo-Fi bit is never on if we get
a preview back. With the above option we have to guarantee that the lite page
bit stays on if we get a preview back. I think I'm in favor of the later, but
could be convinced otherwise and either way we should guarantee both in the
content code. If we keep what you have here then we could do Lo-Fi images on a
lite page by only changing the bits, which may be the policy we want at some
point.

https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc
(right):

https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc:311:
TEST_F(ContentLoFiDeciderTest, MaybeSetAcceptTransformHeaderProxyDecides) {
It looks like there's a lot of repeated code in this test. Is there any way we
can use a struct for it?

[dougarnett, 2017-05-30 21:45:18]

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

[commit-bot: I haz the power, 2017-05-30 21:46:01]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougarnett, 2017-05-30 22:03:35]

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

[commit-bot: I haz the power, 2017-05-30 22:04:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougarnett, 2017-05-30 22:15:48]

https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti...
File components/data_reduction_proxy/content/browser/content_lofi_decider.cc
(right):

https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:73:
content::PreviewsState previews_state = request_info->GetPreviewsState();
On 2017/05/30 18:38:53, megjablon wrote:
> I'd move this line above the comment.

Done.

https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:74: if
(previews_state & content::PREVIEWS_NO_TRANSFORM) {
On 2017/05/30 18:38:53, megjablon wrote:
> If you want, you can remove the brackets here.

Acknowledged.

https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:78: //
For the proxy-decides-transform feature, we determine whether to set
On 2017/05/30 18:38:53, megjablon wrote:
> Don't use we. "the header is set based only on the request's PreviewsState"

Done.

https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:85: bool
decide_by_previews_state = base::FeatureList::IsEnabled(
On 2017/05/30 18:38:53, megjablon wrote:
> Maybe call this check_previews_flags and use the inverse since we still use
the
> previews state in the old world and this implies that we don't.
> 
> bool check_previews_flags = !base::FeatureList::IsEnabled(
> 
> Also import src/base/feature_list.h

Done.

https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:127:
resource_type == content::RESOURCE_TYPE_MAIN_FRAME) {
On 2017/05/30 18:38:53, megjablon wrote:
> Do we want:
> 
> if (previews_state & content::SERVER_LITE_PAGE_ON) {
>   if (resource_type == content::RESOURCE_TYPE_MAIN_FRAME)
>     accept_transform_value = lite_page_directive();
>   }
> }
> 
> With what you have here we have to guarantee the Lo-Fi bit is never on if we
get
> a preview back. With the above option we have to guarantee that the lite page
> bit stays on if we get a preview back. I think I'm in favor of the later, but
> could be convinced otherwise and either way we should guarantee both in the
> content code. If we keep what you have here then we could do Lo-Fi images on a
> lite page by only changing the bits, which may be the policy we want at some
> point. 

Discussed in person. I favor the client not wiring together a constraint that
Lo-Fi can't happen if we got a lite page. Instead if we can separate and
simplify the logic to keep/clear Lo-Fi bit based on Server's page-policies, then
it would be possible for the server to do both without the client blocking that
combination (new HD preview + Lo-Fi maybe?).

For reference, the logic for updating the Previews bits from what is enabled by
the client to what is decided by the server is in CL 2910783002 (RenderFrameImpl
calling PreviewsStateHelper method). It is a bit different from the emailed
logic (to separately decide whether to clear/keep LO_FI bits (based on
page-policies) from clearing/keeping the LITE_PAGE bit).

https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc
(right):

https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc:311:
TEST_F(ContentLoFiDeciderTest, MaybeSetAcceptTransformHeaderProxyDecides) {
On 2017/05/30 18:38:53, megjablon wrote:
> It looks like there's a lot of repeated code in this test. Is there any way we
> can use a struct for it?

In my previous mondo CL, I had used the struct approach and Ben commented to not
take that approach (which I agree with). 
Perhaps this would be an interesting example to discuss trade-offs
with the struct approach.

[commit-bot: I haz the power, 2017-05-31 00:59:42]

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

[commit-bot: I haz the power, 2017-05-31 00:59:43]

Dry run: This issue passed the CQ dry run.

[bengr, 2017-05-31 18:48:28]

https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc
(right):

https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc:311:
TEST_F(ContentLoFiDeciderTest, MaybeSetAcceptTransformHeaderProxyDecides) {
On 2017/05/30 22:15:48, dougarnett wrote:
> On 2017/05/30 18:38:53, megjablon wrote:
> > It looks like there's a lot of repeated code in this test. Is there any way
we
> > can use a struct for it?
> 
> In my previous mondo CL, I had used the struct approach and Ben commented to
not
> take that approach (which I agree with). 
> Perhaps this would be an interesting example to discuss trade-offs
> with the struct approach.

I don't like the pattern of using an array of structs to handle various test
cases. Imo, each logical struct instance should be its own test. However I do
agree with Megan that there is a lot of code repetition. Can each block call the
same helper method? Something like VerifyStuff(...)? Also, unless these blocks
need to be in series, I'd break them into separate tests.

[dougarnett, 2017-05-31 22:57:25]

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

[commit-bot: I haz the power, 2017-05-31 22:58:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougarnett, 2017-05-31 23:05:21]

https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc
(right):

https://codereview.chromium.org/2911673002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc:311:
TEST_F(ContentLoFiDeciderTest, MaybeSetAcceptTransformHeaderProxyDecides) {
On 2017/05/31 18:48:28, bengr wrote:
> On 2017/05/30 22:15:48, dougarnett wrote:
> > On 2017/05/30 18:38:53, megjablon wrote:
> > > It looks like there's a lot of repeated code in this test. Is there any
way
> we
> > > can use a struct for it?
> > 
> > In my previous mondo CL, I had used the struct approach and Ben commented to
> not
> > take that approach (which I agree with). 
> > Perhaps this would be an interesting example to discuss trade-offs
> > with the struct approach.
> 
> I don't like the pattern of using an array of structs to handle various test
> cases. Imo, each logical struct instance should be its own test. However I do
> agree with Megan that there is a lot of code repetition. Can each block call
the
> same helper method? Something like VerifyStuff(...)? Also, unless these blocks
> need to be in series, I'd break them into separate tests.

Ok, broke up into 4 difference tests with simplified verify method.

[megjablon, 2017-06-01 00:06:21]

https://codereview.chromium.org/2911673002/diff/80001/components/data_reducti...
File components/data_reduction_proxy/content/browser/content_lofi_decider.cc
(right):

https://codereview.chromium.org/2911673002/diff/80001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:72:
content::PreviewsState previews_state = request_info->GetPreviewsState();
Replace request_info->GetPreviewsState() calls below in existing code with
previews_state.

https://codereview.chromium.org/2911673002/diff/80001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:103: //
Previews has been disabled.
nit: Previews have?

https://codereview.chromium.org/2911673002/diff/80001/components/data_reducti...
File
components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc
(right):

https://codereview.chromium.org/2911673002/diff/80001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc:196:

Can we move even more of the repetitive code:

static void VerifyAcceptTransformHeader(
    content::ResourceType resource_type,
    bool is_https,
    content::PreviewsState previews_state,
    bool expected_accept_lite_page,
    bool expected_accept_empty_image) {
  std::unique_ptr<net::URLRequest> request =
       CreateRequestByType(resource_type, is_https, previews_state);

  net::HttpRequestHeaders headers;
  lofi_decider->MaybeSetAcceptTransformHeader(*request.get(), false, &headers);

  std::string header_value;
  EXPECT_EQ(expected_accept_lite_page || expected_accept_empty_image,
            headers.GetHeader(chrome_proxy_accept_transform_header(),
&header_value));

  if (expected_accept_lite_page) {
    EXPECT_TRUE(header_value == lite_page_directive());
  } else if (expected_accept_empty_image) {
    EXPECT_TRUE(header_value == empty_image_directive());
  } else {
    EXPECT_FALSE(headers.HasHeader(chrome_proxy_accept_transform_header()));
  }
}

[commit-bot: I haz the power, 2017-06-01 00:21:09]

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

[commit-bot: I haz the power, 2017-06-01 00:21:10]

Dry run: This issue passed the CQ dry run.

[dougarnett, 2017-06-01 18:10:04]

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

[commit-bot: I haz the power, 2017-06-01 18:10:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougarnett, 2017-06-01 18:13:35]

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

[commit-bot: I haz the power, 2017-06-01 18:14:01]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougarnett, 2017-06-01 18:15:27]

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

[commit-bot: I haz the power, 2017-06-01 18:16:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougarnett, 2017-06-01 18:16:48]

https://codereview.chromium.org/2911673002/diff/80001/components/data_reducti...
File components/data_reduction_proxy/content/browser/content_lofi_decider.cc
(right):

https://codereview.chromium.org/2911673002/diff/80001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:72:
content::PreviewsState previews_state = request_info->GetPreviewsState();
On 2017/06/01 00:06:21, megjablon wrote:
> Replace request_info->GetPreviewsState() calls below in existing code with
> previews_state.

Done.

https://codereview.chromium.org/2911673002/diff/80001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider.cc:103: //
Previews has been disabled.
On 2017/06/01 00:06:21, megjablon wrote:
> nit: Previews have?

Done.

https://codereview.chromium.org/2911673002/diff/80001/components/data_reducti...
File
components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc
(right):

https://codereview.chromium.org/2911673002/diff/80001/components/data_reducti...
components/data_reduction_proxy/content/browser/content_lofi_decider_unittest.cc:196:

On 2017/06/01 00:06:21, megjablon wrote:
> Can we move even more of the repetitive code:
> 
> static void VerifyAcceptTransformHeader(
>     content::ResourceType resource_type,
>     bool is_https,
>     content::PreviewsState previews_state,
>     bool expected_accept_lite_page,
>     bool expected_accept_empty_image) {
>   std::unique_ptr<net::URLRequest> request =
>        CreateRequestByType(resource_type, is_https, previews_state);
> 
>   net::HttpRequestHeaders headers;
>   lofi_decider->MaybeSetAcceptTransformHeader(*request.get(), false,
&headers);
> 
>   std::string header_value;
>   EXPECT_EQ(expected_accept_lite_page || expected_accept_empty_image,
>             headers.GetHeader(chrome_proxy_accept_transform_header(),
> &header_value));
> 
>   if (expected_accept_lite_page) {
>     EXPECT_TRUE(header_value == lite_page_directive());
>   } else if (expected_accept_empty_image) {
>     EXPECT_TRUE(header_value == empty_image_directive());
>   } else {
>     EXPECT_FALSE(headers.HasHeader(chrome_proxy_accept_transform_header()));
>   }
> }

Pulling in the target api call helps quite a bit. I went with passing in the
request rather than all the parms for the request so that the tests themselves
are more readable.

[commit-bot: I haz the power, 2017-06-01 19:26:51]

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

[commit-bot: I haz the power, 2017-06-01 19:26:52]

Dry run: This issue passed the CQ dry run.

[megjablon, 2017-06-01 22:24:03]

lgtm

[dougarnett, 2017-06-02 15:54:38]

The CQ bit was checked by dougarnett@chromium.org

[commit-bot: I haz the power, 2017-06-02 15:55:04]

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-06-02 15:59:21]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496418878445090,
"parent_rev": "9bdbcce6c4e83bbbc7c52ef37f939b6c76602b20", "commit_rev":
"f134cffafa9fca0640bacbb56d9de9af0a8f25aa"}

[commit-bot: I haz the power, 2017-06-02 15:59:29]

Description was changed from

==========
New CPAT support in ContentLoFiDecider guarded by feature flag.
Updates MaybeSetAcceptTransformHeader to set accept header based
only on PreviewsState (if new feature enabled) vs. checking various
other flags. Also new path will not add "ifheavy" qualifier.

This is step 2 of 4 in adding new CPAT protocol support.
Step 1 is in CL 2889993004.
Step 3 is in CL 2903453003.

BUG=701802
==========

to

==========
New CPAT support in ContentLoFiDecider guarded by feature flag.
Updates MaybeSetAcceptTransformHeader to set accept header based
only on PreviewsState (if new feature enabled) vs. checking various
other flags. Also new path will not add "ifheavy" qualifier.

This is step 2 of 4 in adding new CPAT protocol support.
Step 1 is in CL 2889993004.
Step 3 is in CL 2903453003.

BUG=701802

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

[commit-bot: I haz the power, 2017-06-02 15:59:31]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/f134cffafa9fca0640bacbb56d9d...