Add support for distilling arbitrary URLs in DOM Distiller Viewer.

components/dom_distiller/content/dom_distiller_viewer_source.cc

Index: components/dom_distiller/content/dom_distiller_viewer_source.cc
diff --git a/components/dom_distiller/content/dom_distiller_viewer_source.cc b/components/dom_distiller/content/dom_distiller_viewer_source.cc
index 8e913d59d470405ac881913a6923cda2e4924a9d..38be0fe57e93e39bd4da0bd06eb8f2a8a6084714 100644
--- a/components/dom_distiller/content/dom_distiller_viewer_source.cc
+++ b/components/dom_distiller/content/dom_distiller_viewer_source.cc
@@ -16,11 +16,13 @@
 #include "components/dom_distiller/core/proto/distilled_article.pb.h"
 #include "components/dom_distiller/core/proto/distilled_page.pb.h"
 #include "components/dom_distiller/core/task_tracker.h"
+#include "components/dom_distiller/core/url_constants.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "grit/component_strings.h"
 #include "grit/dom_distiller_resources.h"
 #include "net/base/escape.h"
+#include "net/base/url_util.h"
 #include "net/url_request/url_request.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
@@ -29,16 +31,17 @@
 namespace {
 
 const char kCssPath[] = "readability.css";
+const char kRequestIdKey[] = "request_id";
 
 std::string ReplaceHtmlTemplateValues(std::string title, std::string content) {
   base::StringPiece html_template =
       ResourceBundle::GetSharedInstance().GetRawDataResource(
           IDR_DOM_DISTILLER_VIEWER_HTML);
   std::vector<std::string> substitutions;
-  substitutions.push_back(title);     // $1
-  substitutions.push_back(kCssPath);  // $2
-  substitutions.push_back(title);     // $3
-  substitutions.push_back(content);   // $4
+  substitutions.push_back(title);                        // $1
+  substitutions.push_back(std::string("/") + kCssPath);  // $2

[cjhopman, 2014/02/05 23:03:06]

why not put this slash in the html file?

[nyquist, 2014/02/25 20:30:24]

Done.

+  substitutions.push_back(title);                        // $3
+  substitutions.push_back(content);                      // $4
   return ReplaceStringPlaceholders(html_template, substitutions, NULL);
 }
 
@@ -48,7 +51,8 @@ namespace dom_distiller {
 
 // Handles receiving data asynchronously for a specific entry, and passing
 // it along to the data callback for the data source.
-class RequestViewerHandle : public ViewRequestDelegate {
+class DomDistillerViewerSource::RequestViewerHandle
+    : public ViewRequestDelegate {
  public:
   explicit RequestViewerHandle(
       const content::URLDataSource::GotDataCallback& callback);
@@ -69,13 +73,13 @@ class RequestViewerHandle : public ViewRequestDelegate {
   content::URLDataSource::GotDataCallback callback_;
 };
 
-RequestViewerHandle::RequestViewerHandle(
+DomDistillerViewerSource::RequestViewerHandle::RequestViewerHandle(
     const content::URLDataSource::GotDataCallback& callback)
     : callback_(callback) {}
 
-RequestViewerHandle::~RequestViewerHandle() {}
+DomDistillerViewerSource::RequestViewerHandle::~RequestViewerHandle() {}
 
-void RequestViewerHandle::OnArticleReady(
+void DomDistillerViewerSource::RequestViewerHandle::OnArticleReady(
     const DistilledArticleProto* article_proto) {
   DCHECK(article_proto);
   std::string title;
@@ -101,7 +105,7 @@ void RequestViewerHandle::OnArticleReady(
   base::MessageLoop::current()->DeleteSoon(FROM_HERE, this);
 }
 
-void RequestViewerHandle::TakeViewerHandle(
+void DomDistillerViewerSource::RequestViewerHandle::TakeViewerHandle(
     scoped_ptr<ViewerHandle> viewer_handle) {
   viewer_handle_ = viewer_handle.Pass();
 }
@@ -140,9 +144,9 @@ void DomDistillerViewerSource::StartDataRequest(
 
   RequestViewerHandle* request_viewer_handle =
       new RequestViewerHandle(callback);
-  std::string entry_id = StringToUpperASCII(path);
   scoped_ptr<ViewerHandle> viewer_handle =
-      dom_distiller_service_->ViewEntry(request_viewer_handle, entry_id);
+      CreateViewRequest(path, request_viewer_handle);
+
   if (viewer_handle) {
     // The service returned a |ViewerHandle| and guarantees it will call
     // the |RequestViewerHandle|, so passing ownership to it, to ensure the
@@ -178,16 +182,45 @@ bool DomDistillerViewerSource::ShouldServiceRequest(
 void DomDistillerViewerSource::WillServiceRequest(
     const net::URLRequest* request,
     std::string* path) const {
-  if (*path != kCssPath) {
-    // Since the full request is not available to StartDataRequest, replace the
-    // path to contain the data needed.
-    *path = request->url().host();
-  }
+  if (*path == kCssPath)
+    return;
+  GURL dummy_url(scheme_ + "://dummy/?" + request->url().query());

[shashi, 2014/02/05 22:28:08]

nit: Instead of using dummy, maybe use dom-distiller-viewer-internal or some
other scheme, so that when I see this in the log, I can make sense of the url.

Also add a comment, that host is not passed when starting data request so we are
modifying the path to include the host component. This does feel a bit hacky
though.

[nyquist, 2014/02/25 20:30:24]

Done.

+  // Use the host component as the request ID.
+  GURL url = net::AppendOrReplaceQueryParameter(

[shashi, 2014/02/05 22:28:08]

Maybe a unit_test to make sure this works with urls that already have
request_id. also testing when request_id is actually in the distilled url: e.g. 
?url="http://example.com/request_id=abc"

[nyquist, 2014/02/25 20:30:24]

Removed request_id now.

+      dummy_url, kRequestIdKey, request->url().host());

[cjhopman, 2014/02/05 23:03:06]

It looks like the request id is never used.

[nyquist, 2014/02/25 20:30:24]

Done.

+  *path = url.query();
 };
 
 std::string DomDistillerViewerSource::GetContentSecurityPolicyObjectSrc()
     const {
-  return "object-src 'none'; style-src 'self'";
+  return "object-src 'none'; style-src 'self';";
+}
+
+scoped_ptr<ViewerHandle> DomDistillerViewerSource::CreateViewRequest(
+    const std::string& path,
+    RequestViewerHandle* request_viewer_handle) {
+  GURL dummy_url(scheme_ + "://dummy/?" + path);

[cjhopman, 2014/02/05 23:03:06]

I don't really like how we are passing data around here. I'd really like to see
us figure out the long-term plan for this view stuff.

[cjhopman, 2014/02/05 23:03:06]

Could you comment on what you need this dummy_url for? Or extract it to a nicely
named function?

Maybe just add a function like "GetValueForKeyInURLPathQuery" either here or in
url_utils that constructs the dummy_url itself.

[nyquist, 2014/02/25 20:30:24]

Done.

+  std::string request_id;
+  net::GetValueForKeyInQuery(dummy_url, kRequestIdKey, &request_id);
+  std::string entry_id;
+  net::GetValueForKeyInQuery(dummy_url, kEntryIdKey, &entry_id);
+  bool has_entry_id = !entry_id.empty();
+  entry_id = StringToUpperASCII(entry_id);
+
+  std::string requested_url_str;
+  net::GetValueForKeyInQuery(dummy_url, kUrlKey, &requested_url_str);
+  GURL requested_url(requested_url_str);
+  bool has_url = !requested_url_str.empty() && requested_url.is_valid();
+
+  if (has_entry_id) {
+    return dom_distiller_service_->ViewEntry(request_viewer_handle, entry_id)
+        .Pass();
+  } else if (has_url) {
+    return dom_distiller_service_->ViewUrl(request_viewer_handle, requested_url)
+        .Pass();
+  } else {
+    return scoped_ptr<ViewerHandle>();
+  }
 }
 
 }  // namespace dom_distiller