Reload Lo-Fi images inline instead of reloading the whole page

content/renderer/render_frame_impl.cc

 // Copyright 2013 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/render_frame_impl.h"
 
 #include <map>
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 1298 matching lines @@
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(RenderFrameImpl, msg)
     IPC_MESSAGE_HANDLER(FrameMsg_Navigate, OnNavigate)
     IPC_MESSAGE_HANDLER(FrameMsg_BeforeUnload, OnBeforeUnload)
     IPC_MESSAGE_HANDLER(FrameMsg_SwapOut, OnSwapOut)
     IPC_MESSAGE_HANDLER(FrameMsg_Delete, OnDeleteFrame)
     IPC_MESSAGE_HANDLER(FrameMsg_Stop, OnStop)
     IPC_MESSAGE_HANDLER(FrameMsg_ContextMenuClosed, OnContextMenuClosed)
     IPC_MESSAGE_HANDLER(FrameMsg_CustomContextMenuAction,
                         OnCustomContextMenuAction)
+    IPC_MESSAGE_HANDLER(FrameMsg_ReloadLoFiImages, OnReloadLoFiImages)
     IPC_MESSAGE_HANDLER(InputMsg_Undo, OnUndo)
     IPC_MESSAGE_HANDLER(InputMsg_Redo, OnRedo)
     IPC_MESSAGE_HANDLER(InputMsg_Cut, OnCut)
     IPC_MESSAGE_HANDLER(InputMsg_Copy, OnCopy)
     IPC_MESSAGE_HANDLER(InputMsg_Paste, OnPaste)
     IPC_MESSAGE_HANDLER(InputMsg_PasteAndMatchStyle, OnPasteAndMatchStyle)
     IPC_MESSAGE_HANDLER(InputMsg_Delete, OnDelete)
     IPC_MESSAGE_HANDLER(InputMsg_SelectAll, OnSelectAll)
     IPC_MESSAGE_HANDLER(InputMsg_SelectRange, OnSelectRange)
     IPC_MESSAGE_HANDLER(InputMsg_AdjustSelectionByCharacterOffset,
@@ skipping 310 matching lines @@
     ContextMenuClient* client =
         pending_context_menus_.Lookup(custom_context.request_id);
     if (client)
       client->OnMenuAction(custom_context.request_id, action);
   } else {
     // Internal request, forward to WebKit.
     render_view_->webview()->performCustomContextMenuAction(action);
   }
 }
 
+void RenderFrameImpl::OnReloadLoFiImages() {
+  is_using_lofi_ = false;

[Charlie Reis, 2016/01/29 23:01:12]

This line is surprising to me, since this CL isn't changing any other LoFi state
(e.g., in the browser process).  Is there no other state to update?  Or will
this put us into an inconsistent state?

[megjablon, 2016/01/29 23:22:23]

There is no other state to update. This is the only LoFi state we store, we do
not keep the state in the browser process. I can update the method name to
OnReloadAndDisableLoFiImages if that is more clear.

[Charlie Reis, 2016/01/29 23:52:00]

As in web_contents.h, a comment (perhaps in frame_messages.h) that this also
disables LoFi should be sufficient.

+  GetWebFrame()->reloadLoFiImages();
+}
+
 void RenderFrameImpl::OnUndo() {
   frame_->executeCommand(WebString::fromUTF8("Undo"), GetFocusedElement());
 }
 
 void RenderFrameImpl::OnRedo() {
   frame_->executeCommand(WebString::fromUTF8("Redo"), GetFocusedElement());
 }
 
 void RenderFrameImpl::OnCut() {
   base::AutoReset<bool> handling_select_range(&handling_select_range_, true);
@@ skipping 1419 matching lines @@
   TRACE_EVENT2("navigation", "RenderFrameImpl::didCommitProvisionalLoad",
                "id", routing_id_,
                "url", GetLoadingUrl().possibly_invalid_spec());
   DCHECK(!frame_ || frame_ == frame);
   DocumentState* document_state =
       DocumentState::FromDataSource(frame->dataSource());
   NavigationStateImpl* navigation_state =
       static_cast<NavigationStateImpl*>(document_state->navigation_state());
   WebURLResponseExtraDataImpl* extra_data = GetExtraDataFromResponse(
       frame->dataSource()->response());
-  is_using_lofi_ = extra_data && extra_data->is_using_lofi();
+  if (is_main_frame_ && !navigation_state->WasWithinSamePage()) {

[Charlie Reis, 2016/01/29 23:01:12]

This looks unrelated to the rest of the CL, and it's subtle.  I would suggest
landing it separately (with a test) in case there are any regressions, to help
with bisecting.

By "it's subtle," I mean that we're changing from assuming the extra_data is
correct about the mode to enforcing the mode can only change for cross-document
main frame navigations, such that children inherit the mode and can't change. 
(Well, until this new reload images feature, anyway.)

Was there a bug that prompted this?

[megjablon, 2016/01/29 23:22:23]

The initial intention was this behavior. Children should only inherit the mode
and not change unless explicitly told to (such as by this reload images
feature). This wasn't visible before because reloads simply reloaded the whole
page bypassing the cache and only the main frame got the lo_fi extra data flag,
but now we need to make sure all the frames for a page have the same state and
don't switch unless a new navigation outside of the page happens.

[Charlie Reis, 2016/01/29 23:52:00]

Sorry, I'm not following.

If I understand correctly, the old behavior was that main frames would set
is_using_lofi_ based on the extra_data at commit time.  Subframes would inherit
is_using_lofi_ from the main frame.

Technically, the subframes would also update their is_using_lofi_ on each
commit, but that apparently never changed the value from what was inherited by
the parent, since it was always stamped on the extra_data in willSendRequest. 
Or is this where the bug was?

I'm taking a guess-- did in-page navigations lack the extra_data bit and make a
frame (either subframe or main frame) forget that it was in LoFi mode?  That
would explain the need for this change.

Sorry for the extra questions here, but I want to understand why this change is
needed.  An example would help.

[megjablon, 2016/01/30 08:30:09]

The first paragraph you're describing is the only behavior I wanted. The
"technically" portion is the behavior that was unintended; I didn't want
subframes to update their value on each commit. There's no reason a subframe
should update its own LoFi state, each subframe state should match the main
frame state and only change when the main frame state does. This didn't have an
effect until now since a page would only change state with a full reload. 

I'll have to double check on this (I'm OOO till Wednesday), but what I believe
is happening is that extra_data doesn't get updated with the new LoFi state when
we turn LoFi off for a frame. Then the frame ends up turning LoFi back on.
Rather than the frame forgetting it was in LoFi mode without this extra line, I
was having the opposite issue of a frame turning LoFi back on after I had turned
it off during in-page navigations.

[Charlie Reis, 2016/02/01 22:12:00]

Ok, thanks.  I'd recommend adding a test for that subframe behavior if you can,
to prevent it from regressing.

(Actually, a comment here might help as well.  Something like, "Only update LoFi
state for new main frame documents.  Subframes inherit from the main frame and
should not change at commit time.")

[megjablon, 2016/02/04 02:37:26]

Done.

+    is_using_lofi_ = extra_data && extra_data->is_using_lofi();
+  }
 
   if (proxy_routing_id_ != MSG_ROUTING_NONE) {
     RenderFrameProxy* proxy =
         RenderFrameProxy::FromRoutingID(proxy_routing_id_);
 
     // The proxy might have been detached while the provisional LocalFrame was
     // being navigated.  In that case, don't swap the frame back in the tree
     // and return early (to avoid sending confusing IPCs to the browser
     // process).  See https://crbug.com/526304 and https://crbug.com/568676.
     // TODO(nasko, alexmos): Eventually, the browser process will send an IPC
@@ skipping 2942 matching lines @@
                                     int match_count,
                                     int ordinal,
                                     const WebRect& selection_rect,
                                     bool final_status_update) {
   Send(new FrameHostMsg_Find_Reply(routing_id_, request_id, match_count,
                                    selection_rect, ordinal,
                                    final_status_update));
 }
 
 }  // namespace content