Theme Preferences for Distilled Pages

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 26c650266d3d6f5e0bde7c13e774c7cef8c9cf95..1723fd37b8e5ee55df7e0114913962fc461e203e 100644
--- a/components/dom_distiller/content/dom_distiller_viewer_source.cc
+++ b/components/dom_distiller/content/dom_distiller_viewer_source.cc
@@ -12,6 +12,8 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/utf_string_conversions.h"
+#include "components/dom_distiller/core/dom_distiller_service.h"
+#include "components/dom_distiller/core/reader_mode_preferences.h"
 #include "components/dom_distiller/core/task_tracker.h"
 #include "components/dom_distiller/core/url_constants.h"
 #include "components/dom_distiller/core/viewer.h"
@@ -31,13 +33,15 @@ namespace dom_distiller {
 // the current main frame's page in the Viewer instance.
 class DomDistillerViewerSource::RequestViewerHandle
     : public ViewRequestDelegate,
-      public content::WebContentsObserver {
+      public content::WebContentsObserver,
+      public ReaderModePrefs::Observer {
  public:
   explicit RequestViewerHandle(
       content::WebContents* web_contents,
       const std::string& expected_scheme,
       const std::string& expected_request_path,
-      const content::URLDataSource::GotDataCallback& callback);
+      const content::URLDataSource::GotDataCallback& callback,
+      ReaderModePrefs* reader_mode_prefs);
   virtual ~RequestViewerHandle();
 
   // ViewRequestDelegate implementation.

[nyquist, 2014/06/26 20:39:57]

// ViewRequestDelegate implementation:

[smaslo, 2014/06/27 18:33:16]

Done.

@@ -61,6 +65,11 @@ class DomDistillerViewerSource::RequestViewerHandle
       bool is_main_frame,
       content::RenderViewHost* render_view_host) OVERRIDE;
 
+  // ReaderModePrefs::Observer

[nyquist, 2014/06/26 20:39:58]

// ReaderModePrefs::Observer implementation:

[smaslo, 2014/06/27 18:33:17]

Done.

+  virtual void OnChangeTheme(ReaderModePrefs* reader_mode_prefs) OVERRIDE;

[nyquist, 2014/06/26 20:39:58]

Also, is it possible to make all of these interface implementations private?

[smaslo, 2014/06/27 18:33:16]

Done.

+
+  void SetBodyClassName();

[nyquist, 2014/06/26 20:39:57]

How about SendBodyClassName()? And could it take the theme as an argument? Also,
could this method be private?

[smaslo, 2014/06/27 18:33:17]

Done.

+
  private:
   // Sends JavaScript to the attached Viewer, buffering data if the viewer isn't
   // ready.
@@ -98,25 +107,31 @@ class DomDistillerViewerSource::RequestViewerHandle
   // Temporary store of pending JavaScript if the page isn't ready to receive
   // data from distillation.
   std::string buffer_;
+
+  ReaderModePrefs* reader_mode_prefs_;

[nyquist, 2014/06/26 20:39:57]

Add a comment. For example, what can we get from this member that we use?

[smaslo, 2014/06/27 18:33:17]

Done.

 };
 
 DomDistillerViewerSource::RequestViewerHandle::RequestViewerHandle(
     content::WebContents* web_contents,
     const std::string& expected_scheme,
     const std::string& expected_request_path,
-    const content::URLDataSource::GotDataCallback& callback)
+    const content::URLDataSource::GotDataCallback& callback,
+    ReaderModePrefs* reader_mode_prefs)
     : web_contents_(web_contents),
       expected_scheme_(expected_scheme),
       expected_request_path_(expected_request_path),
       callback_(callback),
       page_count_(0),
-      waiting_for_page_ready_(true) {
+      waiting_for_page_ready_(true),
+      reader_mode_prefs_(reader_mode_prefs) {
   content::WebContentsObserver::Observe(web_contents_);
+  reader_mode_prefs->AddObserver(this);

[nyquist, 2014/06/26 20:39:57]

Nit: Use reader_mode_prefs_

[smaslo, 2014/06/27 18:33:17]

Done.

 }
 
 DomDistillerViewerSource::RequestViewerHandle::~RequestViewerHandle() {
   // Balanced with constructor although can be a no-op if frame navigated away.
   content::WebContentsObserver::Observe(NULL);
+  reader_mode_prefs_->RemoveObserver(this);
 }
 
 void DomDistillerViewerSource::RequestViewerHandle::SendJavaScript(
@@ -206,6 +221,8 @@ void DomDistillerViewerSource::RequestViewerHandle::OnArticleReady(
               page_count_ == article_proto->pages_size()));
     }
   }
+  SendJavaScript("alert(0);");

[nyquist, 2014/06/26 20:39:58]

You probably don't want this.

[smaslo, 2014/06/27 18:33:17]

Done.

+  SetBodyClassName();
   // No need to hold on to the ViewerHandle now that distillation is complete.
   viewer_handle_.reset();
 }
@@ -278,7 +295,8 @@ void DomDistillerViewerSource::StartDataRequest(
   const std::string path_after_query_separator =
       path.size() > 0 ? path.substr(1) : "";
   RequestViewerHandle* request_viewer_handle = new RequestViewerHandle(
-      web_contents, scheme_, path_after_query_separator, callback);
+      web_contents, scheme_, path_after_query_separator, callback,
+      dom_distiller_service_->GetReaderModePrefs());
   scoped_ptr<ViewerHandle> viewer_handle = viewer::CreateViewRequest(
       dom_distiller_service_, path, request_viewer_handle);
 
@@ -325,4 +343,16 @@ std::string DomDistillerViewerSource::GetContentSecurityPolicyObjectSrc()
   return "object-src 'none'; style-src 'self';";
 }
 
+void DomDistillerViewerSource::RequestViewerHandle::OnChangeTheme(

[nyquist, 2014/06/26 20:39:57]

Could you move these methods up to the rest of the methods for
DomDistillerViewerSource::RequestViewerHandle?

[smaslo, 2014/06/27 18:33:17]

Done.

+    ReaderModePrefs* ReaderModePrefs) {
+  SetBodyClassName();
+}
+
+void DomDistillerViewerSource::RequestViewerHandle::SetBodyClassName() {
+  std::string s = "document.body.className='";

[nyquist, 2014/06/26 20:39:57]

Could you maybe use std::ostringstream?

std::ostringstream js;
js << "document.body.className='" << reader_mode_prefs_->GetBodyCssClass() <<
"';";
SendJavaScript(js.str());

If the middle line is too long, maybe just store the body CSS class as a
variable.

[smaslo, 2014/06/27 18:33:17]

Done.

+  s += reader_mode_prefs_->GetBodyCssClass();
+  s += "';";
+  SendJavaScript(s);
+}
+
 }  // namespace dom_distiller