Consistently use LoFi for an entire page

content/public/common/data_reduction_proxy_lofi_user_data.h

+// Copyright 2015 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.
+
+#ifndef CONTENT_PUBLIC_COMMON_DATA_REDUCTION_PROXY_LOFI_USER_DATA_H_
+#define CONTENT_PUBLIC_COMMON_DATA_REDUCTION_PROXY_LOFI_USER_DATA_H_
+
+#include "base/supports_user_data.h"
+
+namespace content {
+
+// The LoFi state which determines whether to add the LoFi header. Must stay in
+// sync with the enum in data_reduction_proxy_lofi_helper.h.
+enum LoFiState {
+  // Request a LoFi version of the resource.
+  LOFI_ON = 0,
+  // Request a normal (non-LoFi) version of the resource.
+  LOFI_OFF,
+  // Let the browser process decide whether or not to request the LoFi version.
+  LOFI_DEFAULT,

[mmenke, 2015/09/21 18:03:48]

This seems weird - seems to me like this should not be resolved to on/off on a
per request basis, but on a per-navigation basis.  Once a request is made, it
either is using lo-fi, or it isn't.

[megjablon, 2015/09/22 20:35:52]

LOFI_DEFAULT is only used by navigation requests to tell the Data Reduction
Proxy component to use the NQE to determine if Lo-Fi should be used. After that
the subresources will either get LOFI_ON or LOFI_OFF.

[mmenke, 2015/09/22 20:38:12]

That sounds reasonable - in that case, I think we should indeed just be storing
a bool with each request, rather than a LoFiState, to make it clearer it just
has one of two values.

[megjablon, 2015/09/22 20:53:40]

Maybe I didn't explain this right. We need to be able to tell the drp component
to query the NQE sometimes, but also have the ability to force LOFI_ON or
LOFI_OFF othertimes for navigation requests.

[mmenke, 2015/09/22 20:57:07]

Right, but DataReductionProxyLoFiUserData is associated with URLRequests, not
navigations, so while we want a LoFiState to be associated with a navigation, we
just need a bool associated with a URLRequest...Or am I missing something?

[megjablon, 2015/09/22 21:20:26]

The navigation creates a URLRequest and when that request hits
DRPNetworkDelegate::OnBeforeSendProxyHeadersInternal the NQE may make the
decision to use Lo-Fi or not on the URLRequest for the navigation. We need the
three states in OnBeforeSendProxyHeadersInternal because LOFI_DEFAULT tells us
to check the NQE for the request whereas LOFI_ON or LOFI_OFF just adds or
doesn't add the header respectively.

+};
+
+// Used to annotate URLRequests with the Data Reduction Proxy LoFiState.
+class DataReductionProxyLoFiUserData : public base::SupportsUserData::Data {
+ public:
+  DataReductionProxyLoFiUserData(LoFiState lofi_state);
+  ~DataReductionProxyLoFiUserData() override;
+
+  LoFiState lofi_state() const { return lofi_state_; }
+  void set_lofi_state(LoFiState lofi_state) { lofi_state_ = lofi_state; }

[mmenke, 2015/09/21 18:03:48]

Should this just be a value in ResourceRequestInfoImpl instead? Particularly if
the goal is to make this a web platform feature, rather than purely a DRP
feature.

[megjablon, 2015/09/22 20:35:52]

This sounds like a good and simpler idea to me. davidben@ and nasko@ any
objections?

[davidben, 2015/09/22 22:20:50]

Yes, please do that. But exposing a mutator here still makes the content/DRP
boundary a little awkward, ResourceRequestInfo or DRPLoFiUserData. I would
suggest going further and doing the following:

ResourceRequestInfo does not have set_lofi_state or even expose the LoFiState
enum. Instead, you have:

bool ResourceRequestInfo::is_lofi() const

Leave interpreting LOFI_DEFAULT and converting to bool to //content. //content
doesn't have the bandwidth estimator, so we add a method to
ResourceDispatcherHostDelegate or elsewhere like bool ShouldEnableLofiMode(/*
stuff */). This function tells you whether, at this point in time, LOFI_DEFAULT
means on or off.

//content calls into that very early to get rid of LOFI_DEFAULT and turn it into
a boolean. From there, the LofiState enum is gone.

As previously discussed, I'm not a huge fan of //content having this bit and not
knowing what it means, so I think it SHOULD be a header that //content also just
sets. But I'm okay with holding that for now since it's not yet standardized. In
the meantime, the semantics are:

- //content has this notion of whether requests and documents and navigations
are "lofi". It exposes this bit on ResourceRequestInfo.

- //content lets you read this bit, but not set it. Maintaining this bit is
//content's job.

- As part of maintaining that bit, //content asks its embedder whether, at any
point in time, bandwidth and such are low enough that "lofi" is on. This is used
to bootstrap the bit on top-level navigations, implemented with LOFI_DEFAULT.
//chrome will implement that hook by querying the DRP logic.

- //content doesn't do anything with this bit it maintains, but, it does let you
mutate the URLRequest before it goes out because it's always let you do that as
part of intercepting the net stack. When intercepting the net stack, you're free
to interpret that bit how you like and, notably, modify the DRP header.

In the future, that last step will be replaced by //content just setting the
Enable-Fancy-Standardized-Lofi-Mode header and then the DRP won't really be
involved in this but for a server-side polyfill. And maybe to implement the
ShouldEnableLofiMode hook, I dunno.

How does that sound?

[mmenke, 2015/09/22 22:36:28]

As discussed offline, this makes me, at least, much happier.

[megjablon, 2015/09/23 23:15:24]

Done! LTAL and let me know if you want to move anything around. I did my best
from this description, but you may have a better idea of where things should be
placed.

+
+  static const void* kUserDataKey;
+
+ private:
+  // Whether or not to request a LoFi version of the resource or let the
+  // browser decide.
+  LoFiState lofi_state_;
+};
+
+}  // namespace content
+
+#endif  // CONTENT_PUBLIC_COMMON_DATA_REDUCTION_PROXY_LOFI_USER_DATA_H_