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..ce06acf46584001a845ca9e20bec57c02813f6c9 100644
--- a/content/browser/loader/resource_dispatcher_host_browsertest.cc
+++ b/content/browser/loader/resource_dispatcher_host_browsertest.cc
@@ -2,7 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/memory/ref_counted.h"
+#include "base/run_loop.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
@@ -18,6 +21,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"
@@ -28,6 +32,8 @@
 #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"
+#include "net/url_request/url_request.h"
+#include "url/gurl.h"
 
 using base::ASCIIToUTF16;
 
@@ -526,4 +532,207 @@ IN_PROC_BROWSER_TEST_F(ResourceDispatcherHostBrowserTest,
       delegate.page_transition() & ui::PAGE_TRANSITION_CLIENT_REDIRECT);
 }
 
+namespace {
+
+// Checks whether the given urls are requested, set the Lo-Fi state, and have
+// the proper value for IsUsingLoFi.
+class LoFiModeResourceDispatcherHostDelegate
+    : public ResourceDispatcherHostDelegate {
+ public:
+  LoFiModeResourceDispatcherHostDelegate(const GURL& main_frame_url,
+                                         const GURL& subresource_url,
+                                         const GURL& iframe_url,
+                                         const GURL& ignore_url)
+      : main_frame_url_(main_frame_url),
+        subresource_url_(subresource_url),
+        iframe_url_(iframe_url),
+        ignore_url_(ignore_url),
+        main_frame_url_seen_(false),
+        subresource_url_seen_(false),
+        iframe_url_seen_(false),
+        use_lofi_(false),
+        should_enable_lofi_mode_called_(false) {}

[mmenke, 2015/10/13 15:42:49]

child classes should have explicit override destructors.

[megjablon, 2015/10/14 18:09:46]

Done.

+
+  // ResourceDispatcherHostDelegate implementation:
+  void RequestBeginning(net::URLRequest* request,
+                        ResourceContext* resource_context,
+                        AppCacheService* appcache_service,
+                        ResourceType resource_type,
+                        ScopedVector<ResourceThrottle>* throttles) override {

[mmenke, 2015/10/13 15:42:49]

Should have DCHECK_CURRENTLY_ON(BrowserThread::IO); in all these methods, except
the constructor.

[megjablon, 2015/10/14 18:09:46]

Done.

+    const ResourceRequestInfo* info = ResourceRequestInfo::ForRequest(request);
+    if (request->url() == ignore_url_)
+      return;

[mmenke, 2015/10/13 15:42:49]

Should probably just ignore everything that doesn't match one of the three
expected URLs, and remove ignore_url

[megjablon, 2015/10/14 18:09:46]

Done.

+    if (request->url() == main_frame_url_) {
+      EXPECT_FALSE(main_frame_url_seen_);
+      main_frame_url_seen_ = true;
+    } else if (request->url() == subresource_url_) {
+      EXPECT_TRUE(main_frame_url_seen_);
+      EXPECT_FALSE(subresource_url_seen_);
+      subresource_url_seen_ = true;
+    } else if (request->url() == iframe_url_) {
+      EXPECT_TRUE(main_frame_url_seen_);
+      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(main_frame_url_, request->url());
+    return use_lofi_;
+  }
+
+  void Reset(bool use_lofi) {
+    main_frame_url_seen_ = false;
+    subresource_url_seen_ = false;
+    iframe_url_seen_ = false;
+    use_lofi_ = use_lofi;
+    should_enable_lofi_mode_called_ = false;
+  }
+
+  void CheckResourcesRequested(bool should_enable_lofi_mode_called) {
+    EXPECT_EQ(should_enable_lofi_mode_called, should_enable_lofi_mode_called_);
+    EXPECT_TRUE(main_frame_url_seen_);
+    EXPECT_TRUE(subresource_url_seen_);
+    EXPECT_TRUE(iframe_url_seen_);
+  }
+
+ private:
+  const GURL main_frame_url_;
+  const GURL subresource_url_;
+  const GURL iframe_url_;
+  const GURL ignore_url_;
+
+  bool main_frame_url_seen_;
+  bool subresource_url_seen_;
+  bool iframe_url_seen_;
+  bool use_lofi_;
+  bool should_enable_lofi_mode_called_;
+
+  DISALLOW_COPY_AND_ASSIGN(LoFiModeResourceDispatcherHostDelegate);
+};
+
+}  // namespace
+
+class LoFiResourceDispatcherHostBrowserTest : public ContentBrowserTest {

[mmenke, 2015/10/13 15:42:49]

Should have a destructor with override

[megjablon, 2015/10/14 18:09:46]

Done.

+ protected:
+  void SetUpOnMainThread() override {
+    ContentBrowserTest::SetUpOnMainThread();
+
+    ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
+
+    delegate_.reset(new LoFiModeResourceDispatcherHostDelegate(
+        embedded_test_server()->GetURL("/page_with_iframe.html"),
+        embedded_test_server()->GetURL("/image.jpg"),
+        embedded_test_server()->GetURL("/title1.html"),
+        GURL("about:blank")));
+
+    content::BrowserThread::PostTask(
+           content::BrowserThread::IO,
+           FROM_HERE,
+           base::Bind(&LoFiModeResourceDispatcherHostDelegate::SetDelegate,
+                      base::Unretained(delegate_.get())));
+  }
+
+  void Reset(bool use_lofi) {
+    base::RunLoop run_loop;
+    content::BrowserThread::PostTaskAndReply(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(&LoFiModeResourceDispatcherHostDelegate::Reset,
+                   base::Unretained(delegate_.get()), use_lofi),
+        base::Bind(&base::RunLoop::Quit, base::Unretained(&run_loop)));
+    run_loop.Run();

[mmenke, 2015/10/13 15:42:49]

The PostTask SetUpOnMainThread() doesn't need to wait for the reply, and neither
does this.  Should be consistent about whether you wait for the reply when you
don't need it.

I suggest either not waiting for it when you don't have to, or just make a
method that always waits, and use that everywhere.

e.g.

void PostTaskToIOThreadAndWait(const base::Callback& callback) {
  // Do the RunLoop + PostTaskAndReply here.
}

[megjablon, 2015/10/14 18:09:46]

Done.

+  }
+
+  void CheckResourcesRequested(
+      bool should_enable_lofi_mode_called) {
+    base::RunLoop run_loop;
+    content::BrowserThread::PostTaskAndReply(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(
+            &LoFiModeResourceDispatcherHostDelegate::CheckResourcesRequested,
+            base::Unretained(delegate_.get()), should_enable_lofi_mode_called),
+        base::Bind(&base::RunLoop::Quit, base::Unretained(&run_loop)));
+    run_loop.Run();
+  }
+
+ private:
+  scoped_ptr<LoFiModeResourceDispatcherHostDelegate> delegate_;
+};
+
+IN_PROC_BROWSER_TEST_F(LoFiResourceDispatcherHostBrowserTest,
+                       ShouldEnableLoFiModeOn) {
+  // Navigate with ShouldEnableLoFiMode true.

[mmenke, 2015/10/13 15:42:49]

nit:  Maybe "Navigate with ShouldEnableLoFiMode returning true." (For all of
these).  Or rephrase in some other way that makes it clearer
ShouldEnableLoFiModeOn is a method, not a value.

[megjablon, 2015/10/14 18:09:46]

Done.

+  Reset(true);
+  NavigateToURLBlockUntilNavigationsComplete(
+      shell(), embedded_test_server()->GetURL("/page_with_iframe.html"), 1);
+  CheckResourcesRequested(true);
+}
+
+IN_PROC_BROWSER_TEST_F(LoFiResourceDispatcherHostBrowserTest,
+                       ShouldEnableLoFiModeOff) {
+  // Navigate with ShouldEnableLoFiMode false.
+  NavigateToURLBlockUntilNavigationsComplete(
+      shell(), embedded_test_server()->GetURL("/page_with_iframe.html"), 1);
+  CheckResourcesRequested(true);
+}
+
+IN_PROC_BROWSER_TEST_F(LoFiResourceDispatcherHostBrowserTest,
+                       ShouldEnableLoFiModeReload) {
+  // Navigate with ShouldEnableLoFiMode false.
+  NavigateToURLBlockUntilNavigationsComplete(
+      shell(), embedded_test_server()->GetURL("/page_with_iframe.html"), 1);
+  CheckResourcesRequested(true);
+
+  // Reload. ShouldEnableLoFiMode should be called.
+  Reset(true);
+  ReloadBlockUntilNavigationsComplete(shell(), 1);
+  CheckResourcesRequested(true);
+}
+
+IN_PROC_BROWSER_TEST_F(LoFiResourceDispatcherHostBrowserTest,
+                       ShouldEnableLoFiModeNavigateBackThenForward) {
+  // Navigate with ShouldEnableLoFiMode true.
+  Reset(true);
+  NavigateToURLBlockUntilNavigationsComplete(
+      shell(), embedded_test_server()->GetURL("/page_with_iframe.html"), 1);
+  CheckResourcesRequested(true);
+
+  // Go to a different page.
+  NavigateToURLBlockUntilNavigationsComplete(shell(), GURL("about:blank"), 1);
+
+  // Go back.
+  Reset(false);
+  WaitForLoadStop(shell()->web_contents());
+  TestNavigationObserver tab_observer(shell()->web_contents(), 1);
+  shell()->GoBackOrForward(-1);
+  tab_observer.Wait();
+  CheckResourcesRequested(true);
+}
+
+IN_PROC_BROWSER_TEST_F(LoFiResourceDispatcherHostBrowserTest,
+                       ShouldEnableLoFiModeReloadDisableLoFi) {
+  // Navigate with ShouldEnableLoFiMode true.
+  Reset(true);

[mmenke, 2015/10/13 15:42:49]

May want to go from true to false here, just to do the opposite of the above
test.

[megjablon, 2015/10/14 18:09:46]

Done.

+  NavigateToURLBlockUntilNavigationsComplete(
+      shell(), embedded_test_server()->GetURL("/page_with_iframe.html"), 1);
+  CheckResourcesRequested(true);
+
+  // Reload with Lo-Fi disabled.
+  Reset(false);
+  WaitForLoadStop(shell()->web_contents());
+  TestNavigationObserver tab_observer(shell()->web_contents(), 1);
+  shell()->web_contents()->GetController().ReloadDisableLoFi(true);
+  tab_observer.Wait();
+  CheckResourcesRequested(false);
+}
+
 }  // namespace content