Hook up new distillability signal to DistillablePageUtils

chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc

Index: chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc
diff --git a/chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc b/chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..abf2175c0fa6084a41855fda766905df4c5c93f8
--- /dev/null
+++ b/chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc
@@ -0,0 +1,155 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.

[Lei Zhang, 2015/11/11 23:12:46]

2015 is almost over

[wychen, 2015/11/12 05:39:28]

Good catch!

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <string.h>
+
+#include "base/command_line.h"
+#include "base/thread_task_runner_handle.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "components/dom_distiller/content/browser/distillable_page_utils.h"
+#include "components/dom_distiller/core/dom_distiller_switches.h"
+#include "content/public/test/browser_test_utils.h"
+#include "content/public/test/test_utils.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+namespace dom_distiller {
+
+using ::testing::_;
+using namespace switches::reader_mode_heuristics;
+
+namespace {
+
+const char* kSimpleArticlePath = "/dom_distiller/simple_article.html";
+const char* kArticlePath = "/dom_distiller/og_article.html";
+const char* kNonArticlePath = "/dom_distiller/non_og_article.html";
+
+class Holder {
+ public:
+  virtual ~Holder() {}
+  virtual void OnResult(bool, bool) = 0;
+};
+
+class MockDelegate : public Holder {
+ public:
+  MOCK_METHOD2(OnResult, void(bool, bool));
+
+  base::Callback<void(bool, bool)> GetDelegate() {
+    return base::Bind(&MockDelegate::OnResult, base::Unretained(this));
+  }
+};
+
+}  // namespace
+
+template<const char Option[]>
+class DistillablePageUtilsBrowserTestOption : public InProcessBrowserTest {
+ public:
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendSwitch(switches::kEnableDomDistiller);
+    command_line->AppendSwitchASCII(switches::kReaderModeHeuristics,
+        Option);
+  }
+
+  void SetUpOnMainThread() override {
+    InProcessBrowserTest::SetUpOnMainThread();
+    ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
+    web_contents_ =
+        browser()->tab_strip_model()->GetActiveWebContents();
+    setDelegate(web_contents_, holder_.GetDelegate());
+  }
+
+  void NavigateAndWait(const char* url) {
+    GURL article_url(url);
+    if (base::StartsWith(url, "/", base::CompareCase::SENSITIVE)) {
+      article_url = embedded_test_server()->GetURL(url);
+    }
+
+    // This blocks until the navigation has completely finished.
+    ui_test_utils::NavigateToURL(browser(), article_url);
+    content::WaitForLoadStop(web_contents_);
+
+    // Wait a bit for the message.
+    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(

[Lei Zhang, 2015/11/11 23:12:46]

How well will the timing work if the bot is being slow?

[wychen, 2015/11/12 05:39:28]

I tried waiting for only 1ms, and the error rate is 2/100 locally. Waiting for
100ms should be pretty safe.

[Lei Zhang, 2015/11/12 07:08:19]

Is there some other signal you can listen for? Memory tools incur slowdowns and
what may seem like a long wait on normal bots can actually time out on some
memory bots.

[wychen, 2015/11/12 22:42:51]

Unfortunately we couldn't guarantee the delegate would be called before a known
signal. The only thing we know is that the delegate would be called slightly
after the first paint after onload(). If 100X safety margin turns out to be not
enough for memory bots, we can increase the wait even longer later on.

One possible alternative is to create a run_loop and terminate it in the
MockDelegate. However, we can no longer use EXPECT_CALL, and the test code would
be much longer.

+        FROM_HERE, base::MessageLoop::QuitWhenIdleClosure(),
+        base::TimeDelta::FromMilliseconds(100));
+    content::RunMessageLoop();
+  }
+
+  MockDelegate holder_;
+  content::WebContents* web_contents_;
+};
+
+
+typedef DistillablePageUtilsBrowserTestOption<kAlwaysTrue>
+    DistillablePageUtilsBrowserTestAlways;
+
+IN_PROC_BROWSER_TEST_F(DistillablePageUtilsBrowserTestAlways,
+                       TestDelegate) {
+  // Run twice to make sure the delegate object is still alive.
+  for (unsigned i = 0; i < 2; i++) {

[Lei Zhang, 2015/11/11 23:12:46]

just use int, ++i

[wychen, 2015/11/12 05:39:28]

Done.

+    testing::InSequence dummy;
+    EXPECT_CALL(holder_, OnResult(true, true)).Times(1);
+    NavigateAndWait(kSimpleArticlePath);
+  }
+  // Test pages that we don't care about its distillability.
+  {
+    testing::InSequence dummy;
+    EXPECT_CALL(holder_, OnResult(_, _)).Times(0);
+    NavigateAndWait("about:blank");
+  }
+}
+
+
+typedef DistillablePageUtilsBrowserTestOption<kNone>
+    DistillablePageUtilsBrowserTestNone;
+
+IN_PROC_BROWSER_TEST_F(DistillablePageUtilsBrowserTestNone,
+                       TestDelegate) {
+  EXPECT_CALL(holder_, OnResult(_, _)).Times(0);
+  NavigateAndWait(kSimpleArticlePath);
+}
+
+
+typedef DistillablePageUtilsBrowserTestOption<kOGArticle>
+    DistillablePageUtilsBrowserTestOG;
+
+IN_PROC_BROWSER_TEST_F(DistillablePageUtilsBrowserTestOG,
+                       TestDelegate) {
+  {
+    testing::InSequence dummy;
+    EXPECT_CALL(holder_, OnResult(true, true)).Times(1);
+    NavigateAndWait(kArticlePath);
+  }
+  {
+    testing::InSequence dummy;
+    EXPECT_CALL(holder_, OnResult(false, true)).Times(1);
+    NavigateAndWait(kNonArticlePath);
+  }
+}
+
+
+typedef DistillablePageUtilsBrowserTestOption<kAdaBoost>

[Lei Zhang, 2015/11/11 23:12:46]

using foo = bar, instead of typedef bar foo

[wychen, 2015/11/12 05:39:28]

Thanks for this C++11 trick!

+    DistillablePageUtilsBrowserTestAdaboost;
+
+IN_PROC_BROWSER_TEST_F(DistillablePageUtilsBrowserTestAdaboost,
+                       TestDelegate) {
+  {
+    testing::InSequence dummy;
+    EXPECT_CALL(holder_, OnResult(true, false)).Times(1);
+    EXPECT_CALL(holder_, OnResult(true, true)).Times(1);
+    NavigateAndWait(kSimpleArticlePath);
+  }
+  {
+    testing::InSequence dummy;
+    EXPECT_CALL(holder_, OnResult(false, false)).Times(1);
+    EXPECT_CALL(holder_, OnResult(false, true)).Times(1);
+    NavigateAndWait(kNonArticlePath);
+  }
+}
+
+}  // namespace dom_distiller