Add support for distilling arbitrary URLs in DOM Distiller Viewer.

components/dom_distiller/webui/dom_distiller_handler.cc

 // Copyright 2013 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 "components/dom_distiller/webui/dom_distiller_handler.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #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 {
 
 DomDistillerHandler::DomDistillerHandler(DomDistillerService* service,
                                          const std::string& scheme)
     : service_(service), article_scheme_(scheme), weak_ptr_factory_(this) {}
 
 DomDistillerHandler::~DomDistillerHandler() {}
 
 void DomDistillerHandler::RegisterMessages() {
   web_ui()->RegisterMessageCallback(
       "requestEntries",
       base::Bind(&DomDistillerHandler::HandleRequestEntries,
                  base::Unretained(this)));
   web_ui()->RegisterMessageCallback(
       "addArticle",
       base::Bind(&DomDistillerHandler::HandleAddArticle,
                  base::Unretained(this)));
   web_ui()->RegisterMessageCallback(
       "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) {
   std::string url;
   args->GetString(0, &url);
   GURL gurl(url);
   if (gurl.is_valid()) {
     service_->AddToList(
         gurl,
         base::Bind(base::Bind(&DomDistillerHandler::OnArticleAdded,
                               base::Unretained(this))));
   } else {
     web_ui()->CallJavascriptFunction("domDistiller.onArticleAddFailed");
   }
 }
 
+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());
 }
 
 void DomDistillerHandler::HandleRequestEntries(const base::ListValue* args) {
   base::ListValue entries;
   const std::vector<ArticleEntry>& entries_specifics = service_->GetEntries();
   for (std::vector<ArticleEntry>::const_iterator it = entries_specifics.begin();
        it != entries_specifics.end();
        ++it) {
     const ArticleEntry& article = *it;
     DCHECK(IsEntryValid(article));
     scoped_ptr<base::DictionaryValue> entry(new base::DictionaryValue());
     entry->SetString("entry_id", article.entry_id());
     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);
 }
 
 void DomDistillerHandler::OnArticleAdded(bool article_available) {
   // TODO(nyquist): Update this function.
   if (article_available) {
     HandleRequestEntries(NULL);
   } else {
     web_ui()->CallJavascriptFunction("domDistiller.onArticleAddFailed");
   }
 }
 
 }  // namespace dom_distiller