Add integration of the new heuristics

components/dom_distiller/content/distillable_page_utils_browsertest.cc

Index: components/dom_distiller/content/distillable_page_utils_browsertest.cc
diff --git a/components/dom_distiller/content/distillable_page_utils_browsertest.cc b/components/dom_distiller/content/distillable_page_utils_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..98017031220c7a6f37847613b592aeaa9dd5e5a7
--- /dev/null
+++ b/components/dom_distiller/content/distillable_page_utils_browsertest.cc
@@ -0,0 +1,168 @@
+// Copyright 2015 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/bind.h"
+#include "base/path_service.h"
+#include "base/run_loop.h"
+#include "components/dom_distiller/content/distillable_page_utils.h"
+#include "components/dom_distiller/core/distillable_page_detector.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "content/public/test/content_browser_test.h"
+#include "content/shell/browser/shell.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
+#include "ui/base/resource/resource_bundle.h"
+
+namespace dom_distiller {
+namespace {
+
+const char* kArticlePath = "/og_article.html";
+const char* kNonArticlePath = "/non_og_article.html";
+
+// This needs to match the number of derived features from
+// dom_distiller::CalculateDerivedFeatures.

[nyquist, 2015/04/01 18:12:20]

Nit: Would it be helpful to add a link to this constant from
dom_distiller::CalculateDerivedFeatures?

[cjhopman, 2015/04/01 20:59:29]

I just moved this into that header, it makes sense there and then it's actually
by the thing it corresponds to.

+int kNumDetectorFeatures = 29;
+
+class DomDistillerDistillablePageUtilsTest : public content::ContentBrowserTest,
+                                             content::WebContentsObserver {
+ public:
+  void SetUpOnMainThread() override {
+    AddComponentsResources();
+    SetUpTestServer();
+    ContentBrowserTest::SetUpOnMainThread();
+  }
+
+  void LoadURL(const std::string& url) {
+    content::WebContents* current_web_contents = shell()->web_contents();
+    Observe(current_web_contents);
+    base::RunLoop url_loaded_runner;
+    main_frame_loaded_callback_ = url_loaded_runner.QuitClosure();
+    DUMP(embedded_test_server()->GetURL(url));
+    current_web_contents->GetController().LoadURL(
+        embedded_test_server()->GetURL(url),
+        content::Referrer(),
+        ui::PAGE_TRANSITION_TYPED,
+        std::string());
+    url_loaded_runner.Run();
+    main_frame_loaded_callback_ = base::Closure();
+    Observe(nullptr);
+  }
+
+ private:
+  void AddComponentsResources() {
+    base::FilePath pak_file;
+    base::FilePath pak_dir;
+    PathService::Get(base::DIR_MODULE, &pak_dir);
+    pak_file =
+        pak_dir.Append(FILE_PATH_LITERAL("components_tests_resources.pak"));
+    ui::ResourceBundle::GetSharedInstance().AddDataPackFromPath(
+        pak_file, ui::SCALE_FACTOR_NONE);
+  }
+
+  void SetUpTestServer() {
+    base::FilePath path;
+    PathService::Get(base::DIR_SOURCE_ROOT, &path);
+    path = path.AppendASCII("components/test/data/dom_distiller");
+    embedded_test_server()->ServeFilesFromDirectory(path);
+    ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
+  }
+
+  void DocumentLoadedInFrame(
+      content::RenderFrameHost* render_frame_host) override {
+    if (!render_frame_host->GetParent())
+      main_frame_loaded_callback_.Run();
+  }
+
+  base::Closure main_frame_loaded_callback_;
+};
+
+class ResultHolder {
+ public:
+  ResultHolder(base::Closure callback) : callback_(callback) {}
+
+  void OnResult(bool result) {
+    result_ = result;
+    callback_.Run();
+  }
+
+  bool GetResult() {
+    return result_;
+  }
+
+  base::Callback<void(bool)> GetCallback() {
+    return base::Bind(&ResultHolder::OnResult, base::Unretained(this));
+  }
+
+ private:
+  base::Closure callback_;
+  bool result_;
+};
+
+}  // namespace
+
+IN_PROC_BROWSER_TEST_F(DomDistillerDistillablePageUtilsTest, TestIsOGArticle) {
+  LoadURL(kArticlePath);
+  base::RunLoop run_loop_;
+  ResultHolder holder(run_loop_.QuitClosure());
+  IsOpenGraphArticle(shell()->web_contents(), holder.GetCallback());
+  run_loop_.Run();
+  ASSERT_TRUE(holder.GetResult());
+}
+
+IN_PROC_BROWSER_TEST_F(DomDistillerDistillablePageUtilsTest,
+                       TestIsNotOGArticle) {
+  LoadURL(kNonArticlePath);
+  base::RunLoop run_loop_;
+  ResultHolder holder(run_loop_.QuitClosure());
+  IsOpenGraphArticle(shell()->web_contents(), holder.GetCallback());
+  run_loop_.Run();
+  ASSERT_FALSE(holder.GetResult());
+}
+
+IN_PROC_BROWSER_TEST_F(DomDistillerDistillablePageUtilsTest,
+                       TestIsDistillablePage) {
+  scoped_ptr<AdaBoostProto> proto(new AdaBoostProto);
+  proto->set_num_features(kNumDetectorFeatures);
+  proto->set_num_stumps(1);
+  // The first value of the first feature is either 0 or 1. This model should

[nyquist, 2015/04/01 18:12:19]

Yes, it's true that the first value is false/true (0/1 here), but the first time
I read through this I got a little bit confused. For example, why was this an
important fact, etc., probably because I'm not as used to this as you.

Anyway; I now understand it and this test makes total sense.

However, is there something we could do to make it easier for new readers? Maybe
change the wording a bit?

Not verbatim, but maybe something along the lines of:
"Since -1 as the split value will always be below both 0 and 1, the weight of 1
will always be added to the score.
The threshold for the detector will be half the total weight ( 1 / 2 = 0.5), and
that will always be below the total score of 1. Therefore this detector will
always yield a positive result."

Now when I read it it feels very verbose though, but maybe something along those
lines will help new readers.

If you end up changing anything here, please update the negative-test below as
well.

[cjhopman, 2015/04/01 20:59:29]

This might be better.

[nyquist, 2015/04/01 22:30:24]

Yes. Thanks!

+  // accept any set of derived features.
+  StumpProto* stump = proto->add_stump();
+  stump->set_feature_number(0);
+  stump->set_weight(1);
+  stump->set_split(-1);
+  scoped_ptr<DistillablePageDetector> detector(
+      new DistillablePageDetector(proto.Pass()));
+  LoadURL(kArticlePath);
+  base::RunLoop run_loop_;
+  ResultHolder holder(run_loop_.QuitClosure());
+  IsDistillablePageForDetector(shell()->web_contents(), detector.get(),
+                               holder.GetCallback());
+  run_loop_.Run();
+  ASSERT_TRUE(holder.GetResult());
+}
+
+IN_PROC_BROWSER_TEST_F(DomDistillerDistillablePageUtilsTest,
+                       TestIsNotDistillablePage) {
+  scoped_ptr<AdaBoostProto> proto(new AdaBoostProto);
+  proto->set_num_features(kNumDetectorFeatures);
+  proto->set_num_stumps(1);
+  // The first value of the first feature is either 0 or 1. This model should
+  // reject (because of the negative weight) any set of derived features.
+  StumpProto* stump = proto->add_stump();
+  stump->set_feature_number(0);
+  stump->set_weight(-1);
+  stump->set_split(-1);
+  scoped_ptr<DistillablePageDetector> detector(
+      new DistillablePageDetector(proto.Pass()));
+  LoadURL(kArticlePath);
+  base::RunLoop run_loop_;
+  ResultHolder holder(run_loop_.QuitClosure());
+  IsDistillablePageForDetector(shell()->web_contents(), detector.get(),
+                               holder.GetCallback());
+  run_loop_.Run();
+  ASSERT_FALSE(holder.GetResult());
+}
+
+}  // namespace dom_distiller