Adds Lo-Fi fallback support for new Data Reduction Proxy protocol.

content/renderer/previews_state_helper.cc

Index: content/renderer/previews_state_helper.cc
diff --git a/content/renderer/previews_state_helper.cc b/content/renderer/previews_state_helper.cc
new file mode 100644
index 0000000000000000000000000000000000000000..fcbafd496f7d25a1b6f479352a49bdd3f0979bfa
--- /dev/null
+++ b/content/renderer/previews_state_helper.cc
@@ -0,0 +1,80 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/renderer/previews_state_helper.h"
+
+#include "base/strings/string_split.h"
+
+namespace content {
+
+// Chrome Proxy Previews header and directives.
+const char kChromeProxyHeader[] = "chrome-proxy";
+const char kChromeProxyContentTransformHeader[] =
+    "chrome-proxy-content-transform";
+const char kChromeProxyPagePoliciesDirective[] = "page-policies";
+const char kChromeProxyEmptyImageDirective[] = "empty-image";
+const char kChromeProxyLitePageDirective[] = "lite-page";
+
+PreviewsState UpdatePreviewsStateFromMainFrameResponse(
+    PreviewsState original_state,
+    const blink::WebURLResponse& web_url_response) {
+  // First check if no previews were enabled for this request.
+  if (original_state == PREVIEWS_UNSPECIFIED)
+    return original_state;
+
+  // Next handle legacy path where Lite Page not enabled but Lo-Fi is enabled.
+  if (!(original_state & SERVER_LITE_PAGE_ON) &&
+      (original_state & SERVER_LOFI_ON)) {

[sclittle, 2017/06/01 00:59:29]

I don't think this is quite right. If Server LoFi is disabled (e.g. if the user
is in the Server LoFi field trial's control group), but Client LoFi is enabled,
then Client LoFi should be used.

Maybe change to:
if (!(original_state & SERVER_LITE_PAGE_ON) && (original_state & (SERVER_LOFI_ON
| CLIENT_LOFI_ON))) { ... }

[dougarnett, 2017/06/01 20:53:11]

Adding an early return above if neither server preview is specified which will
shortcut this case.

+    return original_state;
+  }
+
+  PreviewsState updated_state = original_state;
+
+  // Clear the Lite Page bit if Lite Page transformation did not occur.

[sclittle, 2017/06/01 00:59:29]

Looking at the code here, it looks like if the DRP returns a light page AND an
"empty-image" page policy, then Server LoFi is allowed to be used if its bit is
set.

In this situation, Client LoFi should also be allowed to be used if its bit is
set.

However, currently there's an explicit check in
RenderFrameImpl::ShouldUseClientLoFiForRequest() that doesn't allow Client LoFi
and Server Lite Pages to be used on the same page.

Could you remove that check from ShouldUseClientLoFiForRequest()? We'll also
need tests around this.

[dougarnett, 2017/06/01 20:53:11]

Let's discuss. It depends on when this method is called. If called for
subresource, then I think this check is good (we will clear the LITE_PAGE_ON bit
for main frame in order to proceed with Lo-Fi)

[dougarnett, 2017/06/01 22:05:10]

Done.

+  if (web_url_response
+          .HttpHeaderField(
+              blink::WebString::FromUTF8(kChromeProxyContentTransformHeader))
+          .Utf8() != kChromeProxyLitePageDirective) {
+    updated_state &= ~(SERVER_LITE_PAGE_ON);
+  }
+
+  // Determine whether to keep or clear Lo-Fi bits. We need to receive the
+  // empty-image policy directive and have SERVER_LOFI_ON in order to retain
+  // Lo-Fi bits.
+  if (!(updated_state & SERVER_LOFI_ON)) {
+    // Server Lo-Fi not enabled so ensure Client Lo-Fi off for this request.

[sclittle, 2017/06/01 00:59:29]

Client LoFi shouldn't be dependent on Server LoFi - if only Server Lite Pages
and Client LoFi are enabled, then Server Lite Pages should be able to fall back
to Client LoFi.

[dougarnett, 2017/06/01 20:53:11]

No, that was not my understanding. I thought that we do not want https-only
fallback for proxied pages. We should discuss further if this doesn't sound
right to you.

+    updated_state &= ~(CLIENT_LOFI_ON);
+  } else {
+    // Keep/clear Lo-Fi bits based on having empty-image policy directive.
+    bool has_empty_page_directive = false;
+    std::string chrome_proxy_value =
+        web_url_response
+            .HttpHeaderField(blink::WebString::FromUTF8(kChromeProxyHeader))
+            .Utf8();
+    size_t page_policies_pos =

[sclittle, 2017/06/01 00:59:29]

Could you separate this string searching into a helper function?

[dougarnett, 2017/06/01 20:53:11]

Done.

+        chrome_proxy_value.find(kChromeProxyPagePoliciesDirective);

[sclittle, 2017/06/01 00:59:29]

Could you just use base::SplitStringPiece here to go through the directives?
That way, it'll work case-insensitively and it'll be resilient to whitespace
around the '=' sign and stuff.

e.g.:

for (const auto& directive : base::SplitStringPiece(
         header, ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY)) {
  if (!base::StartsWith(directive, kChromeProxyPagePoliciesDirective,
                        base::CompareCase::INSENSITIVE_ASCII)) {
    continue;
  }

  // ... Search for the policy in |directive| ...
}

[dougarnett, 2017/06/01 20:53:11]

Done.

+    if (page_policies_pos != std::string::npos) {
+      size_t end_pos =
+          chrome_proxy_value.find_first_of(" ,", page_policies_pos);
+      std::string page_policies_value = chrome_proxy_value.substr(

[sclittle, 2017/06/01 00:59:29]

nit: Don't make an unnecessary copy of the std::string here; you can just use
base::StringPiece(chrome_proxy_value).substr(...)

[dougarnett, 2017/06/01 20:53:11]

Done.

+          page_policies_pos + strlen(kChromeProxyPagePoliciesDirective) + 1,

[sclittle, 2017/06/01 00:59:29]

nit: you can use arraysize or sizeof instead of strlen since it's a char array,
plus arraysize can be executed at compile time.

Note that arraysize includes the trailing null character, so (strlen() ==
arraysize() - 1).

[dougarnett, 2017/06/01 20:53:11]

Done.

+          end_pos);

[sclittle, 2017/06/01 00:59:29]

string::substr takes in a total number of characters to extract, not an end
index. Please fix.

[dougarnett, 2017/06/01 20:53:11]

Acknowledged.

+      for (const auto& policy :
+           base::SplitString(page_policies_value, "|", base::TRIM_WHITESPACE,

[sclittle, 2017/06/01 00:59:29]

nit: use base::SplitStringPiece, not base::SplitString, since there's no need to
allocate and copy all the strings here.

[dougarnett, 2017/06/01 20:53:11]

Done.

+                             base::SPLIT_WANT_NONEMPTY)) {
+        if (policy == kChromeProxyEmptyImageDirective) {

[sclittle, 2017/06/01 00:59:29]

Use case-insensitive comparison, e.g. base::LowerCaseEqualsASCII.

[dougarnett, 2017/06/01 20:53:11]

Done.

+          has_empty_page_directive = true;
+          break;
+        }
+      }
+    }
+    if (!has_empty_page_directive) {
+      updated_state &= ~(SERVER_LOFI_ON | CLIENT_LOFI_ON);
+    }
+  }
+
+  return updated_state;
+}
+
+}  // namespace content