Add support for distilling arbitrary URLs in DOM Distiller Viewer.

components/dom_distiller/webui/dom_distiller_handler.cc

Index: components/dom_distiller/webui/dom_distiller_handler.cc
diff --git a/components/dom_distiller/webui/dom_distiller_handler.cc b/components/dom_distiller/webui/dom_distiller_handler.cc
index ede4f40615b78c919609e262ce382bb0c0809667..17bdda731b16201d21299432c9ca16e3dcd6ca1e 100644
--- a/components/dom_distiller/webui/dom_distiller_handler.cc
+++ b/components/dom_distiller/webui/dom_distiller_handler.cc
@@ -10,8 +10,10 @@
 #include "base/values.h"
 #include "components/dom_distiller/core/dom_distiller_service.h"
 #include "components/dom_distiller/core/proto/distilled_page.pb.h"
+#include "components/dom_distiller/core/url_utils.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_ui.h"
+#include "net/base/escape.h"
 #include "url/gurl.h"
 
 namespace dom_distiller {
@@ -35,6 +37,9 @@ void DomDistillerHandler::RegisterMessages() {
       "selectArticle",
       base::Bind(&DomDistillerHandler::HandleSelectArticle,
                  base::Unretained(this)));
+  web_ui()->RegisterMessageCallback(
+      "viewUrl",
+      base::Bind(&DomDistillerHandler::HandleViewUrl, base::Unretained(this)));
 }
 
 void DomDistillerHandler::HandleAddArticle(const base::ListValue* args) {
@@ -51,13 +56,34 @@ void DomDistillerHandler::HandleAddArticle(const base::ListValue* args) {
   }
 }
 
+void DomDistillerHandler::HandleViewUrl(const base::ListValue* args) {
+  std::string url;
+  args->GetString(0, &url);

[nasko, 2014/02/27 22:40:44]

nit: You aren't checking the success of GetString.

[nyquist, 2014/02/28 16:46:51]

Done.

+  const GURL gurl(url);
+  if (url_utils::IsDistillableUrl(gurl)) {

[nasko, 2014/02/27 22:40:44]

nit: Just a personal preference, but it reads better if you invert the boolean
check and all the code moves out of the code block. It makes the if statement
very simple and concise.

[nyquist, 2014/02/28 16:46:51]

I couldn't find a pretty way to write the method, but by extracting parts of it
to a method in an anonymous namespace at least the high level logic should be
easy to read. I hope you like this better.

On the personal note, after I grew up I've usually followed the "positive branch
first", if there's both if-else. If the else-block can make it become an early
return, that's a different story. In this case, I ended up checking three
conditionals in the end (GetString, IsUrlDistillable and is_valid), so I did
what I felt was easiest for new readers.

+    GURL view_url =
+        url_utils::GetDistillerViewUrlFromUrl(article_scheme_, gurl);
+    DCHECK(view_url.is_valid());

[nasko, 2014/02/27 22:40:44]

DCHECKs should almost always be followed by an if statement to handle the
failure condition. Why try to load if you know it is not a valid URL?

[nyquist, 2014/02/28 16:46:51]

Removed DCHECK, and instead handle an error. The style guide says we should not
handle DCHECK fails:
http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-
"you should not handle DCHECK() failures, even if failure would result in a
crash"

But since this is not really a crash, but more security/validation, and in
addition what to do in a failure is very well defined, I decided to handle the
error instead.

+    web_ui()->GetWebContents()->GetController().LoadURL(
+        view_url,
+        content::Referrer(),
+        content::PAGE_TRANSITION_GENERATED,
+        std::string());
+  } else {
+    web_ui()->CallJavascriptFunction("domDistiller.onViewUrlFailed");
+  }
+}
+
 void DomDistillerHandler::HandleSelectArticle(const base::ListValue* args) {
   std::string entry_id;
   args->GetString(0, &entry_id);
-  GURL url(article_scheme_ + std::string("://") + entry_id);
+  GURL url =
+      url_utils::GetDistillerViewUrlFromEntryId(article_scheme_, entry_id);
   DCHECK(url.is_valid());
-  web_ui()->GetWebContents()->GetController().LoadURL(url,
-      content::Referrer(), content::PAGE_TRANSITION_GENERATED,
+  web_ui()->GetWebContents()->GetController().LoadURL(
+      url,
+      content::Referrer(),
+      content::PAGE_TRANSITION_GENERATED,
       std::string());
 }
 
@@ -74,9 +100,10 @@ void DomDistillerHandler::HandleRequestEntries(const base::ListValue* args) {
     std::string title = (!article.has_title() || article.title().empty())
                             ? article.entry_id()
                             : article.title();
-    entry->SetString("title", title);
+    entry->SetString("title", net::EscapeForHTML(title));
     entries.Append(entry.release());
   }
+  // TODO(nyquist): Write a test that ensures we sanitize the data we send.
   web_ui()->CallJavascriptFunction("domDistiller.onReceivedEntries", entries);
 }