Consistently use LoFi for an entire page

content/browser/loader/resource_dispatcher_host_browsertest.cc

 // Copyright (c) 2012 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 "base/memory/ref_counted.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "content/browser/download/download_manager_impl.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/resource_dispatcher_host.h"
 #include "content/public/browser/resource_dispatcher_host_delegate.h"
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/url_constants.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test.h"
 #include "content/public/test/content_browser_test_utils.h"
+#include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/test_utils.h"
 #include "content/shell/browser/shell.h"
 #include "content/shell/browser/shell_content_browser_client.h"
 #include "content/shell/browser/shell_network_delegate.h"
 #include "net/base/net_errors.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "net/test/embedded_test_server/http_request.h"
 #include "net/test/embedded_test_server/http_response.h"
 #include "net/test/url_request/url_request_failed_job.h"
 #include "net/test/url_request/url_request_mock_http_job.h"
@@ skipping 488 matching lines @@
 
   NavigateToURLBlockUntilNavigationsComplete(
       shell(),
       embedded_test_server()->GetURL("/client_redirect.html"),
       2);
 
   EXPECT_TRUE(
       delegate.page_transition() & ui::PAGE_TRANSITION_CLIENT_REDIRECT);
 }
 
+class ShouldEnableLoFiModeResourceDispatcherHostDelegate
+    : public ResourceDispatcherHostDelegate {

[mmenke, 2015/10/08 19:06:09]

This class seems very sensitive to receiving unexpected requests, or not
receiving requests (Favicon request after navigation, for instance, could cause
confusion).

Suggest two watch URLs - one for the main frame, one for a subresource, and
asserting we see them on destruction, and ignore all others, and make sure we
see them once and only once (First check can be done on destruction, other can
be done in RequestBeginning).  And same for ShouldEnableLoFiMode with the main
frame one.

Can also check the order, out of paranoia (If we see the subframe request first,
even if it has the right flag, it may be a request from the previous navigation
or something).

[megjablon, 2015/10/08 22:20:40]

Done.

+ public:
+  ShouldEnableLoFiModeResourceDispatcherHostDelegate(GURL watch_url)

[mmenke, 2015/10/08 19:06:09]

nit:  const GURL&

[megjablon, 2015/10/08 22:20:40]

Done.

+      : watch_url_(watch_url),
+        use_lofi_(false),
+        should_enable_lofi_mode_called_(false) {}
+
+  // ResourceDispatcherHostDelegate implementation:
+  void RequestBeginning(net::URLRequest* request,
+                        ResourceContext* resource_context,
+                        AppCacheService* appcache_service,
+                        ResourceType resource_type,
+                        ScopedVector<ResourceThrottle>* throttles) override {
+    const ResourceRequestInfo* info = ResourceRequestInfo::ForRequest(request);
+    EXPECT_EQ(use_lofi_, info->IsUsingLoFi());
+  }
+
+  bool ShouldEnableLoFiMode(
+      net::URLRequest* request,
+      content::ResourceContext* resource_context) override {
+    if (should_enable_lofi_mode_called_)
+      EXPECT_TRUE(false);
+    should_enable_lofi_mode_called_ = true;
+    EXPECT_EQ(watch_url_, request->url());
+    return use_lofi_;
+  }
+
+  void Reset(bool use_lofi) {
+    use_lofi_ = use_lofi;
+    should_enable_lofi_mode_called_ = false;
+  }
+
+  void SetWatchUrl(GURL watch_url) {

[mmenke, 2015/10/08 19:06:09]

const GURL&

[megjablon, 2015/10/08 22:20:40]

Done.

+    watch_url_ = watch_url;
+  }
+
+  bool should_enable_lofi_mode_called() {
+    return should_enable_lofi_mode_called_;
+  }
+
+ private:
+  GURL watch_url_;
+  bool use_lofi_;
+  bool should_enable_lofi_mode_called_;
+};
+
+IN_PROC_BROWSER_TEST_F(ResourceDispatcherHostBrowserTest,
+                       ShouldEnableLoFiMode) {
+  ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
+
+  ShouldEnableLoFiModeResourceDispatcherHostDelegate delegate(
+      embedded_test_server()->GetURL("/page_with_iframe.html"));
+  ResourceDispatcherHost::Get()->SetDelegate(&delegate);

[mmenke, 2015/10/08 19:06:09]

Teardown also does seem threadsafe - we should restore the original delegate
before deleting the delegate we injected.

[mmenke, 2015/10/08 19:06:09]

Hrm...This doesn't seem threadsafe - the RDH lives on the IO thread, we're on
the IO thread.  We should post a task to do this.

[megjablon, 2015/10/08 22:20:40]

I'm not sure I follow. Why do we need to post a task to the IO thread if we're
on the IO thread? All the other browser tests simply set a delegate in this
manner and then some of them set it to a nullptr at the end of the test, which
I've added.

[mmenke, 2015/10/08 22:25:15]

Sorry, that was a typo - we're on the UI thread, which is the problem.

[megjablon, 2015/10/08 23:39:07]

Is this feasible? I'm just getting test failed with no useful output when I do
it this way. Also, why do none of the other test ResourceDispatcherHostDelegates
do this?

[mmenke, 2015/10/09 15:31:56]

Tests that do that are just crossing their fingers and hoping the RDH isn't
doing anything when there are no navigations pending, which seems rather
problematic.  Tests that do that also tend to delete the Delegate long before
the RDH, which also seems really bad.

If you upload the code that's giving you the problem, I'll take a look at it.

+
+  // Navigate with ShouldEnableLoFiMode true.
+  delegate.Reset(true);
+  NavigateToURLBlockUntilNavigationsComplete(
+      shell(), embedded_test_server()->GetURL("/page_with_iframe.html"), 1);
+  EXPECT_TRUE(delegate.should_enable_lofi_mode_called());

[mmenke, 2015/10/08 19:06:09]

Each of these should be a separate test (the reload one and disabled one will
need some extra navigations, of course).  We should create the delegate, set it
up, set it on the RDH (On the IOThread, by posting a task), and then start the
tests.

For the requests that need extra navigations, you can inject a second the
preliminary navigation.

[megjablon, 2015/10/08 22:20:40]

Done.

+
+  // Navigate with ShouldEnableLoFiMode false.
+  delegate.Reset(false);
+  NavigateToURLBlockUntilNavigationsComplete(
+      shell(), embedded_test_server()->GetURL("/page_with_iframe.html"), 1);
+  EXPECT_TRUE(delegate.should_enable_lofi_mode_called());
+
+  // Reload. ShouldEnableLoFiMode should be called.
+  delegate.Reset(true);
+  ReloadBlockUntilNavigationsComplete(shell(), 1);
+  EXPECT_TRUE(delegate.should_enable_lofi_mode_called());
+
+  // Go to a different page.
+  delegate.Reset(false);
+  delegate.SetWatchUrl(embedded_test_server()->GetURL("/title1.html"));
+  NavigateToURLBlockUntilNavigationsComplete(
+      shell(), embedded_test_server()->GetURL("/title1.html"), 1);
+  EXPECT_TRUE(delegate.should_enable_lofi_mode_called());
+
+  // Go back.
+  delegate.Reset(true);
+  delegate.SetWatchUrl(
+      embedded_test_server()->GetURL("/page_with_iframe.html"));
+  WaitForLoadStop(shell()->web_contents());
+  TestNavigationObserver tab_observer(shell()->web_contents(), 1);
+  shell()->GoBackOrForward(-1);
+  tab_observer.Wait();
+  EXPECT_TRUE(delegate.should_enable_lofi_mode_called());
+
+
+  // Reload with Lo-Fi disabled.
+  delegate.Reset(false);
+  WaitForLoadStop(shell()->web_contents());
+  TestNavigationObserver tab_observer2(shell()->web_contents(), 1);
+  shell()->web_contents()->GetController().ReloadDisableLoFi(true);
+  tab_observer2.Wait();
+  EXPECT_FALSE(delegate.should_enable_lofi_mode_called());
+}
+
 }  // namespace content