Use new DomDistillerJs Proto API

components/dom_distiller/core/distiller_page.cc

Index: components/dom_distiller/core/distiller_page.cc
diff --git a/components/dom_distiller/core/distiller_page.cc b/components/dom_distiller/core/distiller_page.cc
index a935c7a36decda737eb210153c52d7e487a484c5..6431371549907aa47c6e3391f6fe38df1226ba73 100644
--- a/components/dom_distiller/core/distiller_page.cc
+++ b/components/dom_distiller/core/distiller_page.cc
@@ -5,14 +5,45 @@
 #include "components/dom_distiller/core/distiller_page.h"
 
 #include "base/bind.h"
+#include "base/json/json_writer.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
+#include "base/strings/string_util.h"
+#include "base/strings/utf_string_conversions.h"
+#include "dom_distiller_js/dom_distiller.pb.h"
+#include "dom_distiller_js/dom_distiller_json_converter.h"
 #include "grit/component_resources.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "url/gurl.h"
 
 namespace dom_distiller {
 
+namespace {
+
+const char* kOptionsPlaceholder = "$$OPTIONS";
+
+std::string GetDistillerScriptWithOptions(
+    dom_distiller::proto::DomDistillerOptions options) {

[Yaron, 2014/05/09 01:00:55]

const&

[cjhopman, 2014/05/09 01:55:06]

Done.

+  std::string script = ResourceBundle::GetSharedInstance()
+                           .GetRawDataResource(IDR_DISTILLER_JS)
+                           .as_string();
+  scoped_ptr<base::Value> options_value(
+      dom_distiller::proto::json::DomDistillerOptions::WriteToValue(options));
+  std::string options_json;
+  if (!base::JSONWriter::Write(options_value.get(), &options_json)) {
+    NOTREACHED();
+  }
+  size_t options_offset = script.find(kOptionsPlaceholder);

[Yaron, 2014/05/09 01:00:55]

Instead of rolling your own, why not use base::ReplaceStringPlaceholders?

[cjhopman, 2014/05/09 01:55:06]

ReplaceStringPlaceholders is rather dangerous, it will just blindly replace all
instances of $1 and $$ with the options and $ respectively. We can't really
ensure that those strings don't appear in the generated javascript (and when I
tried using ReplaceStringPlaceholders they did appear and it led to really
confusing javascript errors).

+  DCHECK_NE(std::string::npos, options_offset);
+  DCHECK_EQ(std::string::npos,
+            script.find(kOptionsPlaceholder, options_offset + 1));
+  script =
+      script.replace(options_offset, strlen(kOptionsPlaceholder), options_json);
+  return script;
+}
+
+}
+
 DistilledPageInfo::DistilledPageInfo() {}
 
 DistilledPageInfo::~DistilledPageInfo() {}
@@ -30,59 +61,37 @@ void DistillerPage::DistillPage(const GURL& gurl,
   // the callback to OnDistillationDone happens.
   ready_ = false;
   distiller_page_callback_ = callback;
-  std::string script = ResourceBundle::GetSharedInstance()
-                           .GetRawDataResource(IDR_DISTILLER_JS)
-                           .as_string();
-  DistillPageImpl(gurl, script);
+  dom_distiller::proto::DomDistillerOptions options;
+  DistillPageImpl(gurl, GetDistillerScriptWithOptions(options));
 }
 
 void DistillerPage::OnDistillationDone(const GURL& page_url,
                                        const base::Value* value) {
   DCHECK(!ready_);
   ready_ = true;
+
   scoped_ptr<DistilledPageInfo> page_info(new DistilledPageInfo());
-  std::string result;
-  const base::ListValue* result_list = NULL;
-  bool found_content = false;
-  if (!value->GetAsList(&result_list)) {
-    base::MessageLoop::current()->PostTask(
-        FROM_HERE,
-        base::Bind(distiller_page_callback_, base::Passed(&page_info), false));
-  } else {
-    int i = 0;
-    for (base::ListValue::const_iterator iter = result_list->begin();
-         iter != result_list->end();
-         ++iter, ++i) {
-      std::string item;
-      (*iter)->GetAsString(&item);
-      // The JavaScript returns an array where the first element is the title,
-      // the second element is the article content HTML, and the remaining
-      // elements are image URLs referenced in the HTML.
-      switch (i) {
-        case 0:
-          page_info->title = item;
-          break;
-        case 1:
-          page_info->html = item;
-          found_content = true;
-          break;
-        case 2:
-          page_info->next_page_url = item;
-          break;
-        case 3:
-          page_info->prev_page_url = item;
-          break;
-        default:
-          if (GURL(item).is_valid()) {
-            page_info->image_urls.push_back(item);
-          }
+  bool found_content = !value->IsType(base::Value::TYPE_NULL);
+  if (found_content) {
+    dom_distiller::proto::DomDistillerResult distiller_result =
+        dom_distiller::proto::json::DomDistillerResult::ReadFromValue(value);
+
+    page_info->title = distiller_result.title();
+    page_info->html = distiller_result.distilled_content().html();
+    page_info->next_page_url = distiller_result.pagination_info().next_page();
+    page_info->prev_page_url = distiller_result.pagination_info().prev_page();
+    for (int i = 0; i < distiller_result.image_urls_size(); ++i) {
+      const std::string image_url = distiller_result.image_urls(i);
+      if (GURL(image_url).is_valid()) {
+        page_info->image_urls.push_back(image_url);
       }
     }
-    base::MessageLoop::current()->PostTask(
-        FROM_HERE,
-        base::Bind(
-            distiller_page_callback_, base::Passed(&page_info), found_content));
   }
+
+  base::MessageLoop::current()->PostTask(
+      FROM_HERE,
+      base::Bind(
+          distiller_page_callback_, base::Passed(&page_info), found_content));
 }
 
 }  // namespace dom_distiller