Add support for distilling arbitrary URLs in DOM Distiller Viewer.

components/dom_distiller/core/dom_distiller_service.h

Index: components/dom_distiller/core/dom_distiller_service.h
diff --git a/components/dom_distiller/core/dom_distiller_service.h b/components/dom_distiller/core/dom_distiller_service.h
index 630b184e5eb5e2fda8e5054fbfb5b649c4ad335b..ebc2599708f94e747388ef8470a1441dd798a636 100644
--- a/components/dom_distiller/core/dom_distiller_service.h
+++ b/components/dom_distiller/core/dom_distiller_service.h
@@ -30,42 +30,66 @@ class TaskTracker;
 class ViewerHandle;
 class ViewRequestDelegate;
 
-// Provide a view of the article list and ways of interacting with it.
-class DomDistillerService {
+// Service for interacting with the Dom Distiller.
+// Calls to, construction and destruction of this service must happen on the

[cjhopman, 2014/02/25 22:25:14]

This comma reads like a typo; i.e. that you meant "calls to construction and
destruction". How about "Construction, destruction, and usage of this ..."

[nyquist, 2014/02/27 00:07:37]

Done.

+// same thread. All callbacks will also be called on the caller's thread.

[cjhopman, 2014/02/25 22:25:14]

I'd say: Callbacks will be called on that same thread.

That is, refer back to the thread in the previous sentence rather than
introducing a new "thread"

[nyquist, 2014/02/27 00:07:37]

Done.

+class DomDistillerServiceInterface {
  public:
   typedef base::Callback<void(bool)> ArticleAvailableCallback;
+  virtual ~DomDistillerServiceInterface() {}
 
-  DomDistillerService(scoped_ptr<DomDistillerStoreInterface> store,
-                      scoped_ptr<DistillerFactory> distiller_factory);
-  ~DomDistillerService();
-
-  syncer::SyncableService* GetSyncableService() const;
+  virtual syncer::SyncableService* GetSyncableService() const = 0;
 
   // Distill the article at |url| and add the resulting entry to the DOM
   // distiller list. |article_cb| is invoked with true if article is
   // available offline.

[cjhopman, 2014/02/25 22:25:14]

Clarify the callback part of this: is it always called? is it called with false
if the article isn't available offline?

[nyquist, 2014/02/27 00:07:37]

Done.

-  const std::string AddToList(const GURL& url,
-                              const ArticleAvailableCallback& article_cb);
+  virtual const std::string AddToList(
+      const GURL& url,
+      const ArticleAvailableCallback& article_cb) = 0;
 
   // Gets the full list of entries.
-  std::vector<ArticleEntry> GetEntries() const;
+  virtual std::vector<ArticleEntry> GetEntries() const = 0;
 
   // Removes the specified entry from the dom distiller store.
-  scoped_ptr<ArticleEntry> RemoveEntry(const std::string& entry_id);
+  virtual scoped_ptr<ArticleEntry> RemoveEntry(const std::string& entry_id) = 0;
 
   // Request to view an article by entry id. Returns a null pointer if no entry
   // with |entry_id| exists. The ViewerHandle should be destroyed before the
   // ViewRequestDelegate. The request will be cancelled when the handle is
   // destroyed (or when this service is destroyed).

[cjhopman, 2014/02/25 22:25:14]

We should be clear about how we will handle the ViewRequestDelegate (either here
or in its declaration).

For example, is it possible to get a callback into the delegate after I've
deleted the handle? 

[nyquist, 2014/02/27 00:07:37]

Done.

-  scoped_ptr<ViewerHandle> ViewEntry(ViewRequestDelegate* delegate,
-                                     const std::string& entry_id);
+  virtual scoped_ptr<ViewerHandle> ViewEntry(ViewRequestDelegate* delegate,
+                                             const std::string& entry_id) = 0;
 
   // Request to view an article by url.
-  scoped_ptr<ViewerHandle> ViewUrl(ViewRequestDelegate* delegate,
-                                   const GURL& url);
+  virtual scoped_ptr<ViewerHandle> ViewUrl(ViewRequestDelegate* delegate,
+                                           const GURL& url) = 0;
+
+  virtual void AddObserver(DomDistillerObserver* observer) = 0;
+  virtual void RemoveObserver(DomDistillerObserver* observer) = 0;

[shashi, 2014/02/25 21:48:08]

Should the interface also have DISALLOW_COPY_AND_ASSIGN?

[nyquist, 2014/02/27 00:07:37]

Done.

+};
 
-  void AddObserver(DomDistillerObserver* observer);
-  void RemoveObserver(DomDistillerObserver* observer);
+// Provide a view of the article list and ways of interacting with it.
+class DomDistillerService : public DomDistillerServiceInterface {
+ public:
+  DomDistillerService(scoped_ptr<DomDistillerStoreInterface> store,
+                      scoped_ptr<DistillerFactory> distiller_factory);
+  virtual ~DomDistillerService();
+
+  // DomDistillerServiceInterface implementation.
+  virtual syncer::SyncableService* GetSyncableService() const OVERRIDE;
+  virtual const std::string AddToList(
+      const GURL& url,
+      const ArticleAvailableCallback& article_cb) OVERRIDE;
+  virtual std::vector<ArticleEntry> GetEntries() const OVERRIDE;
+  virtual scoped_ptr<ArticleEntry> RemoveEntry(const std::string& entry_id)
+      OVERRIDE;
+  virtual scoped_ptr<ViewerHandle> ViewEntry(ViewRequestDelegate* delegate,
+                                             const std::string& entry_id)
+      OVERRIDE;
+  virtual scoped_ptr<ViewerHandle> ViewUrl(ViewRequestDelegate* delegate,
+                                           const GURL& url) OVERRIDE;
+  virtual void AddObserver(DomDistillerObserver* observer) OVERRIDE;
+  virtual void RemoveObserver(DomDistillerObserver* observer) OVERRIDE;
 
  private:
   void CancelTask(TaskTracker* task);