Refactor feedback to use native JS object

This change moves the triggering logic for feedback out of
DomDistillerViewerSource and into the native "distiller" JavaScript
object.

BUG=

Review URL: https://codereview.chromium.org/1265843005

Cr-Commit-Position: refs/heads/master@{#350574}

7 files changed, 81 insertions, 37 deletions

diff --git a/components/dom_distiller/content/browser/distiller_javascript_service_impl.cc b/components/dom_distiller/content/browser/distiller_javascript_service_impl.cc
index 16601d2..ef20275 100644
--- a/components/dom_distiller/content/browser/distiller_javascript_service_impl.cc
+++ b/components/dom_distiller/content/browser/distiller_javascript_service_impl.cc
@@ -3,23 +3,50 @@
 // found in the LICENSE file.
 
 #include "components/dom_distiller/content/browser/distiller_javascript_service_impl.h"
+#include "components/dom_distiller/content/browser/external_feedback_reporter.h"
+#include "components/dom_distiller/core/feedback_reporter.h"
 #include "third_party/mojo/src/mojo/public/cpp/bindings/string.h"
 
 namespace dom_distiller {
 
 DistillerJavaScriptServiceImpl::DistillerJavaScriptServiceImpl(
-      mojo::InterfaceRequest<DistillerJavaScriptService> request)
-    : binding_(this, request.Pass()) {}
+    content::RenderFrameHost* render_frame_host,
+    ExternalFeedbackReporter* external_feedback_reporter,
+    mojo::InterfaceRequest<DistillerJavaScriptService> request)
+    : binding_(this, request.Pass()),
+      render_frame_host_(render_frame_host),
+      external_feedback_reporter_(external_feedback_reporter) {}
 
 DistillerJavaScriptServiceImpl::~DistillerJavaScriptServiceImpl() {}
 
 void DistillerJavaScriptServiceImpl::HandleDistillerEchoCall(
     const mojo::String& message) {}
 
+void DistillerJavaScriptServiceImpl::HandleDistillerFeedbackCall(
+    bool good) {
+  FeedbackReporter::ReportQuality(good);
+  if (good) {
+    return;
+  }
+
+  // If feedback is bad try to start up external feedback.
+  if (!external_feedback_reporter_) {
+    return;
+  }
+  content::WebContents* contents =
+      content::WebContents::FromRenderFrameHost(render_frame_host_);
+  external_feedback_reporter_->ReportExternalFeedback(
+      contents, contents->GetURL(), false);
+  return;
+}
+
 void CreateDistillerJavaScriptService(
+    content::RenderFrameHost* render_frame_host,
+    ExternalFeedbackReporter* feedback_reporter,
     mojo::InterfaceRequest<DistillerJavaScriptService> request) {
   // This is strongly bound and owned by the pipe.
-  new DistillerJavaScriptServiceImpl(request.Pass());
+  new DistillerJavaScriptServiceImpl(render_frame_host, feedback_reporter,
+      request.Pass());
 }
 
 }  // namespace dom_distiller
diff --git a/components/dom_distiller/content/browser/distiller_javascript_service_impl.h b/components/dom_distiller/content/browser/distiller_javascript_service_impl.h
index d21cd4a..37634b4 100644
--- a/components/dom_distiller/content/browser/distiller_javascript_service_impl.h
+++ b/components/dom_distiller/content/browser/distiller_javascript_service_impl.h
@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_DOM_DISTILLER_CONTENT_BROWSER_DISTILLER_JAVASCRIPT_SERVICE_IMPL_H_
 #define COMPONENTS_DOM_DISTILLER_CONTENT_BROWSER_DISTILLER_JAVASCRIPT_SERVICE_IMPL_H_
 
+#include "components/dom_distiller/content/browser/external_feedback_reporter.h"
 #include "components/dom_distiller/content/common/distiller_javascript_service.mojom.h"
 #include "third_party/mojo/src/mojo/public/cpp/bindings/string.h"
 #include "third_party/mojo/src/mojo/public/cpp/bindings/strong_binding.h"
@@ -15,18 +16,30 @@ namespace dom_distiller {
 class DistillerJavaScriptServiceImpl : public DistillerJavaScriptService {
  public:
   DistillerJavaScriptServiceImpl(
+      content::RenderFrameHost* render_frame_host,
+      ExternalFeedbackReporter* external_feedback_reporter,
       mojo::InterfaceRequest<DistillerJavaScriptService> request);
   ~DistillerJavaScriptServiceImpl() override;
 
   // Mojo DistillerJavaScriptService implementation.
+
+  // Echo implementation, this call does not actually return as it would be
+  // blocking.
   void HandleDistillerEchoCall(const mojo::String& message) override;
 
+  // Send UMA feedback and start the external feedback reporter if one exists.
+  void HandleDistillerFeedbackCall(bool good) override;
+
  private:
   mojo::StrongBinding<DistillerJavaScriptService> binding_;
+  content::RenderFrameHost* render_frame_host_;
+  ExternalFeedbackReporter* external_feedback_reporter_;
 };
 
 // static
 void CreateDistillerJavaScriptService(
+    content::RenderFrameHost* render_frame_host,
+    ExternalFeedbackReporter* feedback_reporter,
     mojo::InterfaceRequest<DistillerJavaScriptService> request);
 
 }  // namespace dom_distiller
diff --git a/components/dom_distiller/content/browser/dom_distiller_viewer_source.cc b/components/dom_distiller/content/browser/dom_distiller_viewer_source.cc
index 03d9b2b..a71fc07 100644
--- a/components/dom_distiller/content/browser/dom_distiller_viewer_source.cc
+++ b/components/dom_distiller/content/browser/dom_distiller_viewer_source.cc
@@ -202,20 +202,6 @@ void DomDistillerViewerSource::StartDataRequest(
     content::RecordAction(base::UserMetricsAction("DomDistiller_ViewOriginal"));
     callback.Run(NULL);
     return;
-  } else if (kFeedbackBad == path) {
-    FeedbackReporter::ReportQuality(false);
-    callback.Run(NULL);
-    if (!external_feedback_reporter_)
-      return;
-    content::WebContents* contents =
-        content::WebContents::FromRenderFrameHost(render_frame_host);
-    external_feedback_reporter_->ReportExternalFeedback(
-        contents, contents->GetURL(), false);
-    return;
-  } else if (kFeedbackGood == path) {
-    FeedbackReporter::ReportQuality(true);
-    callback.Run(NULL);
-    return;
   }
   content::WebContents* web_contents =
       content::WebContents::FromRenderFrameHost(render_frame_host);
@@ -240,7 +226,9 @@ void DomDistillerViewerSource::StartDataRequest(
   // Add mojo service for JavaScript functionality. This is the receiving end
   // of this particular service.
   render_frame_host->GetServiceRegistry()->AddService(
-      base::Bind(&CreateDistillerJavaScriptService));
+      base::Bind(&CreateDistillerJavaScriptService,
+          render_frame_host,
+          external_feedback_reporter_.get()));
 
   // Tell the renderer that this is currently a distilled page.
   DistillerPageNotifierServicePtr page_notifier_service;
diff --git a/components/dom_distiller/content/common/distiller_javascript_service.mojom b/components/dom_distiller/content/common/distiller_javascript_service.mojom
index cf7b7a0..5316744 100644
--- a/components/dom_distiller/content/common/distiller_javascript_service.mojom
+++ b/components/dom_distiller/content/common/distiller_javascript_service.mojom
@@ -7,5 +7,10 @@ module dom_distiller;
 // This service is implemented by the browser process and is used by the
 // renderer when a distiller JavaScript function is called.
 interface DistillerJavaScriptService {
+
+  // Handle the "distiller.echo" function.
   HandleDistillerEchoCall(string message);
+
+  // Handle the "distiller.sendFeedback" function.
+  HandleDistillerFeedbackCall(bool good);
 };
diff --git a/components/dom_distiller/content/renderer/distiller_native_javascript.cc b/components/dom_distiller/content/renderer/distiller_native_javascript.cc
index 767dac6..aa6619b 100644
--- a/components/dom_distiller/content/renderer/distiller_native_javascript.cc
+++ b/components/dom_distiller/content/renderer/distiller_native_javascript.cc
@@ -45,14 +45,30 @@ void DistillerNativeJavaScript::AddJavaScriptObjectToFrame(
           isolate, base::Bind(&DistillerNativeJavaScript::DistillerEcho,
                               base::Unretained(this)))
           ->GetFunction());
+
+  distiller_obj->Set(
+      gin::StringToSymbol(isolate, "sendFeedback"),
+      gin::CreateFunctionTemplate(
+          isolate, base::Bind(&DistillerNativeJavaScript::DistillerSendFeedback,
+                              base::Unretained(this)))
+          ->GetFunction());
 }
 
-std::string DistillerNativeJavaScript::DistillerEcho(
-    const std::string& message) {
+void DistillerNativeJavaScript::EnsureServiceConnected() {
   if (!distiller_js_service_) {
     render_frame_->GetServiceRegistry()->ConnectToRemoteService(
         mojo::GetProxy(&distiller_js_service_));
   }
+}
+
+void DistillerNativeJavaScript::DistillerSendFeedback(bool good) {
+  EnsureServiceConnected();
+  distiller_js_service_->HandleDistillerFeedbackCall(good);
+}
+
+std::string DistillerNativeJavaScript::DistillerEcho(
+    const std::string& message) {
+  EnsureServiceConnected();
   // TODO(mdjones): It is possible and beneficial to have information
   // returned from the browser process with these calls. The problem
   // is waiting blocks this process.
diff --git a/components/dom_distiller/content/renderer/distiller_native_javascript.h b/components/dom_distiller/content/renderer/distiller_native_javascript.h
index 180ef9f..6914df2 100644
--- a/components/dom_distiller/content/renderer/distiller_native_javascript.h
+++ b/components/dom_distiller/content/renderer/distiller_native_javascript.h
@@ -25,10 +25,16 @@ class DistillerNativeJavaScript {
   void AddJavaScriptObjectToFrame(v8::Local<v8::Context> context);
 
  private:
+  // Make sure the mojo service is connected.
+  void EnsureServiceConnected();
+
   // Native code for "distiller.echo" in JavaScript. This simply returns the
   // provided string.
   std::string DistillerEcho(const std::string& message);
 
+  // Send feedback about distillation quality.
+  void DistillerSendFeedback(bool good);
+
   content::RenderFrame* render_frame_;
   DistillerJavaScriptServicePtr distiller_js_service_;
 };
diff --git a/components/dom_distiller/core/javascript/dom_distiller_viewer.js b/components/dom_distiller/core/javascript/dom_distiller_viewer.js
index 1c2fd08..c98539e 100644
--- a/components/dom_distiller/core/javascript/dom_distiller_viewer.js
+++ b/components/dom_distiller/core/javascript/dom_distiller_viewer.js
@@ -152,21 +152,6 @@ function showFeedbackForm(questionText, yesText, noText) {
   document.getElementById('feedbackContainer').classList.remove("hidden");
 }
 
-/**
- * Send feedback about this distilled article.
- * @param good True if the feedback was positive, false if negative.
- */
-function sendFeedback(good) {
-  var img = document.createElement('img');
-  if (good) {
-    img.src = '/feedbackgood';
-  } else {
-    img.src = '/feedbackbad';
-  }
-  img.style.display = "none";
-  document.body.appendChild(img);
-}
-
 // Add a listener to the "View Original" link to report opt-outs.
 document.getElementById('closeReaderView').addEventListener('click',
     function(e) {
@@ -177,12 +162,16 @@ document.getElementById('closeReaderView').addEventListener('click',
     }, true);
 
 document.getElementById('feedbackYes').addEventListener('click', function(e) {
-  sendFeedback(true);
+  if (distiller) {
+    distiller.sendFeedback(true);
+  }
   document.getElementById('feedbackContainer').className += " fadeOut";
 }, true);
 
 document.getElementById('feedbackNo').addEventListener('click', function(e) {
-  sendFeedback(false);
+  if (distiller) {
+    distiller.sendFeedback(false);
+  }
   document.getElementById('feedbackContainer').className += " fadeOut";
 }, true);