Consistently use LoFi for an entire page

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index b42860a347326f73bf1a8c8600251accf5273585..1e98587b940538f7134b27eb57eac68f62459da1 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -743,6 +743,7 @@ RenderFrameImpl::RenderFrameImpl(const CreateParams& params)
       manifest_manager_(NULL),
       accessibility_mode_(AccessibilityModeOff),
       renderer_accessibility_(NULL),
+      lofi_state_(LOFI_DEFAULT),
       weak_factory_(this) {
   std::pair<RoutingIDFrameMap::iterator, bool> result =
       g_routing_id_frame_map.Get().insert(std::make_pair(routing_id_, this));
@@ -812,9 +813,10 @@ void RenderFrameImpl::SetWebFrame(blink::WebLocalFrame* web_frame) {
   frame_ = web_frame;
 }
 
-void RenderFrameImpl::Initialize() {
+void RenderFrameImpl::Initialize(LoFiState lofi_state) {
   is_subframe_ = !!frame_->parent();
   is_local_root_ = !frame_->parent() || frame_->parent()->isWebRemoteFrame();
+  lofi_state_ = lofi_state;
 
 #if defined(ENABLE_PLUGINS)
   new PepperBrowserConnection(this);
@@ -2012,6 +2014,10 @@ void RenderFrameImpl::AddMessageToConsole(ConsoleMessageLevel level,
     devtools_agent_->AddMessageToConsole(level, message);
 }
 
+bool RenderFrameImpl::IsLoFiOn() {
+  return lofi_state_ == LOFI_ON;
+}
+
 // blink::WebFrameClient implementation ----------------------------------------
 
 blink::WebPlugin* RenderFrameImpl::createPlugin(
@@ -2231,7 +2237,7 @@ blink::WebFrame* RenderFrameImpl::createChildFrame(
 
   // Add the frame to the frame tree and initialize it.
   parent->appendChild(web_frame);
-  child_render_frame->Initialize();
+  child_render_frame->Initialize(lofi_state_);
 
   return web_frame;
 }
@@ -2703,6 +2709,9 @@ void RenderFrameImpl::didCommitProvisionalLoad(
       DocumentState::FromDataSource(frame->dataSource());
   NavigationStateImpl* navigation_state =
       static_cast<NavigationStateImpl*>(document_state->navigation_state());
+  WebURLResponseExtraDataImpl* extra_data = GetExtraDataFromResponse(
+      frame->dataSource()->response());
+  lofi_state_ = extra_data && extra_data->is_lofi() ? LOFI_ON : LOFI_OFF;
 
   if (proxy_routing_id_ != MSG_ROUTING_NONE) {
     RenderFrameProxy* proxy =
@@ -3357,6 +3366,12 @@ void RenderFrameImpl::willSendRequest(
       navigation_state->start_params().transferred_request_request_id);
   extra_data->set_service_worker_provider_id(provider_id);
   extra_data->set_stream_override(stream_override.Pass());
+  // TODO(megjablon): Set the navigation params for single image loads to
+  // LOFI_OFF and remove the dependency on ReloadBypassingCache.
+  if (request.cachePolicy() == WebURLRequest::ReloadBypassingCache)
+    extra_data->set_lofi_state(LOFI_OFF);
+  else
+    extra_data->set_lofi_state(lofi_state_);
   request.setExtraData(extra_data);
 
   // TODO(creis): Update prefetching to work with out-of-process iframes.
@@ -4483,6 +4498,8 @@ void RenderFrameImpl::NavigateInternal(
 
   GetContentClient()->SetActiveURL(common_params.url);
 
+  lofi_state_ = request_params.lofi_state;

[davidben, 2015/09/21 17:58:15]

This still hasn't been resolved. lofi_state_ must only be updated on commit, not
when you start a navigation. See previous discussions on whether this field was
a LofiState or a bool.

[davidben, 2015/09/21 18:09:01]

To clarify, this isn't a "layering; we'll fix this later" problem. It's a
correctness problem. Until the navigation has committed, it's not correct to
change any state relating to the current navigation. The state needs to flow at
commit from the ResourceResponseInfo sent in ResourceMsg_ReceivedResponse to
didCommitProvisionalLoad and be set at that point and only at that point. That
bit then gets stapled to subresources at willSendRequest. Frame requests will
hit a slightly different codepath.

The key thing to always keep in mind is that navigations (frame requests) are
completely different from subresources. It is expected that you will need to
treat them different for this sort of thing. (Treating them the same is usually
a red flag that something is wrong.)

(In fact, I'm not sure this CL works at all for renderer-initiated navigations.
Not every navigation hits this function.)

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

For renderer initiated navigations, we don't need to set Lo-Fi state because it
will always be LOFI_DEFAULT, and we have the NQE decide. This is used by browser
initiated navigations to force Lo-Fi on or off and will be used by the "Load
images" snackbar (to be hooked up in a follow-up cl). Do you have any
suggestions for a path to force the navigation to take the value of
RequestNavigationParams that doesn't break this correctness? Maybe the problem
is we're combining the navigation signal to force Lo-Fi on or off with the
response value at commit that sets the state. These values would be the same for
the navigations with forced Lo-Fi state, and that's why it was being set early
here.

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

I think we were talking about different things. Though I see that
didCommitProvisionalLoad *also* sets lofi_state_. Sorry, I missed that somehow.
The part I was thinking of works fine for renderer-initiated navigations. All
you actually want to do is ditch this line, switch it to a boolean, and then
plumb the value for navigations differently.
Why not attach it to the extraData in CreateURLRequestForNavigation? You'll just
need to make tweak the willSendRequest code to only inherit on subresource
requests, but not navigations, since those are pretty different. That would
translate to PlzNavigate well since you get a browser-side
CreateURLRequestForNavigation-looking function, but not willSendRequest in the
renderer because the renderer does not exist.
Yes! That's it in a nutshell. Thanks for phrasing it much better than I did. :-)
They're not quite the same because a navigation may not commit, and the previous
document may still issue subresource requests before this commit point.

[megjablon, 2015/09/23 01:39:04]

This sounds good. I'm working on implementing this. This may be a simple
question, but in willSendRequest how do I check if the request is a subresource?

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

I took a stab at this. PTAL!

+
   // If this frame isn't in the same process as the main frame, it may naively
   // assume that this is the first navigation in the iframe, but this may not
   // actually be the case. Inform the frame's state machine if this frame has