Consistently use LoFi for an entire page

content/browser/loader/resource_dispatcher_host_browsertest.cc

Index: content/browser/loader/resource_dispatcher_host_browsertest.cc
diff --git a/content/browser/loader/resource_dispatcher_host_browsertest.cc b/content/browser/loader/resource_dispatcher_host_browsertest.cc
index b612d302df5e85ae96088109474302b5df7708d8..c101e8820624461300b828d631cf88e345a1e8ac 100644
--- a/content/browser/loader/resource_dispatcher_host_browsertest.cc
+++ b/content/browser/loader/resource_dispatcher_host_browsertest.cc
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/bind_helpers.h"
 #include "base/memory/ref_counted.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
@@ -18,6 +19,7 @@
 #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"
@@ -526,4 +528,251 @@ IN_PROC_BROWSER_TEST_F(ResourceDispatcherHostBrowserTest,
       delegate.page_transition() & ui::PAGE_TRANSITION_CLIENT_REDIRECT);
 }
 
+class LoFiModeResourceDispatcherHostDelegate

[mmenke, 2015/10/12 20:19:28]

Should document this class.

[mmenke, 2015/10/12 20:19:29]

This needs to be in an anonymous namespace.

[megjablon, 2015/10/12 22:06:06]

Done.

[megjablon, 2015/10/12 22:06:06]

Done.

+    : public ResourceDispatcherHostDelegate {
+ public:
+  LoFiModeResourceDispatcherHostDelegate(const GURL& mainframe_url,
+                                         const GURL& subresource_url,
+                                         const GURL& iframe_url)

[mmenke, 2015/10/12 20:19:29]

Should include gurl.h (Yea, should have already been included in this file)

[megjablon, 2015/10/12 22:06:06]

Done.

+      : mainframe_url_(mainframe_url),
+        subresource_url_(subresource_url),
+        iframe_url_(iframe_url),
+        use_lofi_(false),
+        should_enable_lofi_mode_called_(false) {}
+
+  // ResourceDispatcherHostDelegate implementation:
+  void RequestBeginning(net::URLRequest* request,

[mmenke, 2015/10/12 20:19:29]

Should include net/url_request/url_request.h

[megjablon, 2015/10/12 22:06:06]

Done.

+                        ResourceContext* resource_context,
+                        AppCacheService* appcache_service,
+                        ResourceType resource_type,
+                        ScopedVector<ResourceThrottle>* throttles) override {
+    const ResourceRequestInfo* info = ResourceRequestInfo::ForRequest(request);
+    if (request->url() == mainframe_url_) {
+      EXPECT_FALSE(mainframe_url_seen_);
+      mainframe_url_seen_ = true;

[mmenke, 2015/10/12 20:19:28]

nit:  main_frame_...

[megjablon, 2015/10/12 22:06:06]

Done.

+    } else if (request->url() == subresource_url_) {

[mmenke, 2015/10/12 20:19:29]

EXPECT_TRUE(mainframe_url_seen);?

[megjablon, 2015/10/12 22:06:06]

Done.

+      EXPECT_FALSE(subresource_url_seen_);
+      subresource_url_seen_ = true;
+    } else if (request->url() == iframe_url_) {

[mmenke, 2015/10/12 20:19:28]

EXPECT_TRUE(mainframe_url_seen);?

[megjablon, 2015/10/12 22:06:05]

Done.

+      EXPECT_FALSE(iframe_url_seen_);
+      iframe_url_seen_ = true;
+    }
+    EXPECT_EQ(use_lofi_, info->IsUsingLoFi());
+  }
+
+  void SetDelegate() {
+    ResourceDispatcherHost::Get()->SetDelegate(this);
+  }
+
+  bool ShouldEnableLoFiMode(
+      net::URLRequest* request,
+      content::ResourceContext* resource_context) override {
+    EXPECT_FALSE(should_enable_lofi_mode_called_);
+    should_enable_lofi_mode_called_ = true;
+    EXPECT_EQ(mainframe_url_, request->url());
+    return use_lofi_;
+  }
+
+  void Reset(bool use_lofi) {
+    mainframe_url_seen_ = false;
+    subresource_url_seen_ = false;
+    iframe_url_seen_ = false;
+    use_lofi_ = use_lofi;
+    should_enable_lofi_mode_called_ = false;
+  }
+
+  void SetUrls(const GURL& mainframe_url,
+               const GURL& subresource_url,
+               const GURL& iframe_url) {
+    mainframe_url_ = mainframe_url;
+    subresource_url_ = subresource_url;
+    iframe_url_ = iframe_url;
+  }
+
+  void CheckResourcesRequested() {

[mmenke, 2015/10/12 20:19:29]

const

[megjablon, 2015/10/12 22:06:05]

This doesn't work with binding, do you know why?

+    EXPECT_TRUE(mainframe_url_seen_);
+    if (!subresource_url_.is_empty())
+      EXPECT_TRUE(subresource_url_seen_);
+    if (!iframe_url_.is_empty())

[mmenke, 2015/10/12 20:19:29]

Don't think we need to handle empty urls (See comment further down about not
testing after the second nav in the only test that does that)

[megjablon, 2015/10/12 22:06:05]

Done.

+      EXPECT_TRUE(iframe_url_seen_);
+  }
+
+  bool should_enable_lofi_mode_called() {

[mmenke, 2015/10/12 20:19:29]

const

[megjablon, 2015/10/12 22:06:06]

Done.

+    return should_enable_lofi_mode_called_;
+  }
+
+ private:
+  GURL mainframe_url_;
+  GURL subresource_url_;
+  GURL iframe_url_;

[mmenke, 2015/10/12 20:19:28]

Suggest a blank line between the URLs and the bools.

[megjablon, 2015/10/12 22:06:05]

Done.

+  bool mainframe_url_seen_;
+  bool subresource_url_seen_;
+  bool iframe_url_seen_;
+  bool use_lofi_;
+  bool should_enable_lofi_mode_called_;

[mmenke, 2015/10/12 20:19:28]

You need to initialize all of the bools in the constructor.

[mmenke, 2015/10/12 20:19:29]

DISALLOW_COPY_AND_ASSIGN

[megjablon, 2015/10/12 22:06:06]

Done.

[megjablon, 2015/10/12 22:06:06]

Done.

+};
+
+IN_PROC_BROWSER_TEST_F(ResourceDispatcherHostBrowserTest,
+                       ShouldEnableLoFiModeOn) {
+  ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
+
+  LoFiModeResourceDispatcherHostDelegate delegate(

[mmenke, 2015/10/12 20:19:29]

Should making a new test fixture, and installing the delegate in
SetUpOnMainThread.  That ensures it outlives the RDH, which it currently doesn't
do.  Makes this code much more robust against regressions.  Can also set up the
embedded test server in the text fixture, in SetUp (Make sure to call the base
classes methods in both cases)

[megjablon, 2015/10/12 22:06:06]

Done.

+      embedded_test_server()->GetURL("/page_with_iframe.html"),
+      embedded_test_server()->GetURL("/image.jpg"),
+      embedded_test_server()->GetURL("/title1.html"));
+
+  content::BrowserThread::PostTask(
+         content::BrowserThread::IO,
+         FROM_HERE,
+         base::Bind(&LoFiModeResourceDispatcherHostDelegate::SetDelegate,
+                    base::Unretained(&delegate)));

[mmenke, 2015/10/12 20:19:29]

include bind.h

[megjablon, 2015/10/12 22:06:05]

Done.

+
+  // 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());
+  delegate.CheckResourcesRequested();
+}
+
+IN_PROC_BROWSER_TEST_F(ResourceDispatcherHostBrowserTest,
+                       ShouldEnableLoFiModeOff) {
+  ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
+
+  LoFiModeResourceDispatcherHostDelegate delegate(
+      embedded_test_server()->GetURL("/page_with_iframe.html"),
+      embedded_test_server()->GetURL("/image.jpg"),
+      embedded_test_server()->GetURL("/title1.html"));
+
+  content::BrowserThread::PostTask(
+         content::BrowserThread::IO,
+         FROM_HERE,
+         base::Bind(&LoFiModeResourceDispatcherHostDelegate::SetDelegate,
+                    base::Unretained(&delegate)));
+
+  // Navigate with ShouldEnableLoFiMode true.
+  delegate.Reset(false);

[mmenke, 2015/10/12 20:19:29]

not needed.

[megjablon, 2015/10/12 22:06:06]

Done.

+  NavigateToURLBlockUntilNavigationsComplete(
+      shell(), embedded_test_server()->GetURL("/page_with_iframe.html"), 1);
+  EXPECT_TRUE(delegate.should_enable_lofi_mode_called());
+  delegate.CheckResourcesRequested();

[mmenke, 2015/10/12 20:19:28]

Not really safe to call this on the UI thread - should either use
PostTaskAndReply with a RunLoop, or a lock to protect access to everything
(bools and the GURLs) on both threads.

[megjablon, 2015/10/12 22:06:06]

Done.

+}
+
+IN_PROC_BROWSER_TEST_F(ResourceDispatcherHostBrowserTest,
+                       ShouldEnableLoFiModeReload) {
+  ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
+
+  LoFiModeResourceDispatcherHostDelegate delegate(
+      embedded_test_server()->GetURL("/page_with_iframe.html"),
+      embedded_test_server()->GetURL("/image.jpg"),
+      embedded_test_server()->GetURL("/title1.html"));
+
+  content::BrowserThread::PostTask(
+         content::BrowserThread::IO,
+         FROM_HERE,
+         base::Bind(&LoFiModeResourceDispatcherHostDelegate::SetDelegate,
+                    base::Unretained(&delegate)));
+
+  // Navigate with ShouldEnableLoFiMode false.
+  delegate.Reset(false);

[mmenke, 2015/10/12 20:19:29]

Not needed.

[megjablon, 2015/10/12 22:06:05]

Done.

+  NavigateToURLBlockUntilNavigationsComplete(
+      shell(), embedded_test_server()->GetURL("/page_with_iframe.html"), 1);
+  EXPECT_TRUE(delegate.should_enable_lofi_mode_called());
+  delegate.CheckResourcesRequested();
+
+  // Reload. ShouldEnableLoFiMode should be called.
+  delegate.Reset(true);

[mmenke, 2015/10/12 20:19:29]

Again, should be called on the other thread, or use locks.  Not going to
annotate all cases of this.

[megjablon, 2015/10/12 22:06:05]

Done.

+  ReloadBlockUntilNavigationsComplete(shell(), 1);
+  EXPECT_TRUE(delegate.should_enable_lofi_mode_called());
+  delegate.CheckResourcesRequested();
+}
+
+IN_PROC_BROWSER_TEST_F(ResourceDispatcherHostBrowserTest,
+                       ShouldEnableLoFiModeNavigateBackThenForward) {
+  ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
+
+  LoFiModeResourceDispatcherHostDelegate delegate(
+      embedded_test_server()->GetURL("/page_with_iframe.html"),
+      embedded_test_server()->GetURL("/image.jpg"),
+      embedded_test_server()->GetURL("/title1.html"));
+
+  content::BrowserThread::PostTask(
+         content::BrowserThread::IO,
+         FROM_HERE,
+         base::Bind(&LoFiModeResourceDispatcherHostDelegate::SetDelegate,
+                    base::Unretained(&delegate)));
+
+  // 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());
+  delegate.CheckResourcesRequested();
+
+  // Go to a different page.
+  delegate.Reset(false);
+  delegate.SetUrls(embedded_test_server()->GetURL("/title1.html"), GURL(),
+                   GURL());

[mmenke, 2015/10/12 20:19:29]

Maybe just navigate to about:blank, so we don't have to care about this?  Can
also remove the SetUrls method, and make the GURLs in the delegate const.

[megjablon, 2015/10/12 22:06:06]

Done.

+  NavigateToURLBlockUntilNavigationsComplete(
+      shell(), embedded_test_server()->GetURL("/title1.html"), 1);
+  EXPECT_TRUE(delegate.should_enable_lofi_mode_called());
+  delegate.CheckResourcesRequested();
+
+  // Go back.
+  delegate.Reset(true);
+  delegate.SetUrls(
+      embedded_test_server()->GetURL("/page_with_iframe.html"),
+      embedded_test_server()->GetURL("/image.jpg"),
+      embedded_test_server()->GetURL("/title1.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());
+  delegate.CheckResourcesRequested();
+
+  // Go forward.
+  delegate.Reset(false);
+  delegate.SetUrls(embedded_test_server()->GetURL("/title1.html"), GURL(),
+                   GURL());
+  WaitForLoadStop(shell()->web_contents());
+  TestNavigationObserver tab_observer2(shell()->web_contents(), 1);
+  shell()->GoBackOrForward(1);
+  tab_observer2.Wait();
+  EXPECT_TRUE(delegate.should_enable_lofi_mode_called());
+  delegate.CheckResourcesRequested();

[mmenke, 2015/10/12 20:19:29]

I don't think this last navigation is needed.  If back works, I'm willing to
take it on faith that forward does.

[megjablon, 2015/10/12 22:06:06]

Done.

+}
+
+IN_PROC_BROWSER_TEST_F(ResourceDispatcherHostBrowserTest,
+                       ShouldEnableLoFiModeReloadDisableLoFi) {
+  ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
+
+  LoFiModeResourceDispatcherHostDelegate delegate(
+      embedded_test_server()->GetURL("/page_with_iframe.html"),
+      embedded_test_server()->GetURL("/image.jpg"),
+      embedded_test_server()->GetURL("/title1.html"));
+
+  content::BrowserThread::PostTask(
+         content::BrowserThread::IO,
+         FROM_HERE,
+         base::Bind(&LoFiModeResourceDispatcherHostDelegate::SetDelegate,
+                    base::Unretained(&delegate)));
+
+  // 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());
+  delegate.CheckResourcesRequested();
+
+  // Reload with Lo-Fi disabled.
+  delegate.Reset(false);
+  WaitForLoadStop(shell()->web_contents());
+  TestNavigationObserver tab_observer(shell()->web_contents(), 1);
+  shell()->web_contents()->GetController().ReloadDisableLoFi(true);
+  tab_observer.Wait();
+  EXPECT_FALSE(delegate.should_enable_lofi_mode_called());
+  delegate.CheckResourcesRequested();
+}
+
 }  // namespace content