Start requiring DistillerPage for calls to DomDistillerService.

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 221af72dae3fcbd3fbb2fe800a5c1e30f30cdf15..bb9d340177e2271626445edff95dbf313dbb6ad1 100644
--- a/components/dom_distiller/core/dom_distiller_service.h
+++ b/components/dom_distiller/core/dom_distiller_service.h
@@ -13,6 +13,7 @@
 #include "base/memory/scoped_vector.h"
 #include "base/memory/weak_ptr.h"
 #include "components/dom_distiller/core/article_entry.h"
+#include "components/dom_distiller/core/distiller_page.h"
 
 class GURL;
 
@@ -25,6 +26,7 @@ namespace dom_distiller {
 class DistilledArticleProto;
 class DistilledContentStore;
 class DistillerFactory;
+class DistillerPageFactory;
 class DomDistillerObserver;
 class DomDistillerStoreInterface;
 class TaskTracker;
@@ -44,8 +46,10 @@ class DomDistillerServiceInterface {
   // Distill the article at |url| and add the resulting entry to the DOM
   // distiller list. |article_cb| is always invoked, and the bool argument to it
   // represents whether the article is available offline.

[cjhopman, 2014/04/24 23:03:05]

One important point here, the provided distiller_page is only used if there
isn't already a distillation task in progress for this url.

This means two things:

1. The content extracted should be the same regardless of the DistillerPage impl
(because this might not use the one that I ask it to). I don't know if this is
called out in the DistillerPage documentation.

2. Creating a DistillerPage should be cheap (since it might just be thrown out).
Does the current once create it's web_contents when created or when needed?

I don't know if you want to point those out in the documentation here or not.

[nyquist, 2014/04/25 15:41:13]

I have now tried to clarify this in the documentation of DistillerPage.
Added this requirement to the DistillerPageFactory documentation. FWIW, the
current implementation has a cheap constructor for this exact reason.
It split it up a little bit to be closer to where it might feel natural.

I would be happy to revise the documentation and where it is though.

+  // Use CreateDefaultDistillerPage() to create a default |distiller_page|.
   virtual const std::string AddToList(
       const GURL& url,
+      scoped_ptr<DistillerPage> distiller_page,
       const ArticleAvailableCallback& article_cb) = 0;
 
   // Gets the full list of entries.
@@ -59,12 +63,21 @@ class DomDistillerServiceInterface {
   // ViewRequestDelegate. The request will be cancelled when the handle is
   // destroyed (or when this service is destroyed), which also ensures that
   // the |delegate| is not called after that.
-  virtual scoped_ptr<ViewerHandle> ViewEntry(ViewRequestDelegate* delegate,
-                                             const std::string& entry_id) = 0;
+  // Use CreateDefaultDistillerPage() to create a default |distiller_page|.
+  virtual scoped_ptr<ViewerHandle> ViewEntry(
+      ViewRequestDelegate* delegate,
+      scoped_ptr<DistillerPage> distiller_page,
+      const std::string& entry_id) = 0;
 
   // Request to view an article by url.
-  virtual scoped_ptr<ViewerHandle> ViewUrl(ViewRequestDelegate* delegate,
-                                           const GURL& url) = 0;
+  // Use CreateDefaultDistillerPage() to create a default |distiller_page|.
+  virtual scoped_ptr<ViewerHandle> ViewUrl(
+      ViewRequestDelegate* delegate,
+      scoped_ptr<DistillerPage> distiller_page,
+      const GURL& url) = 0;
+
+  // Creates a default DistillerPage.
+  virtual scoped_ptr<DistillerPage> CreateDefaultDistillerPage() = 0;
 
   virtual void AddObserver(DomDistillerObserver* observer) = 0;
   virtual void RemoveObserver(DomDistillerObserver* observer) = 0;
@@ -80,22 +93,28 @@ class DomDistillerServiceInterface {
 class DomDistillerService : public DomDistillerServiceInterface {
  public:
   DomDistillerService(scoped_ptr<DomDistillerStoreInterface> store,
-                      scoped_ptr<DistillerFactory> distiller_factory);
+                      scoped_ptr<DistillerFactory> distiller_factory,
+                      scoped_ptr<DistillerPageFactory> distiller_page_factory);
   virtual ~DomDistillerService();
 
   // DomDistillerServiceInterface implementation.
   virtual syncer::SyncableService* GetSyncableService() const OVERRIDE;
   virtual const std::string AddToList(
       const GURL& url,
+      scoped_ptr<DistillerPage> distiller_page,
       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 scoped_ptr<ArticleEntry> RemoveEntry(
+      const std::string& entry_id) OVERRIDE;
+  virtual scoped_ptr<ViewerHandle> ViewEntry(
+      ViewRequestDelegate* delegate,
+      scoped_ptr<DistillerPage> distiller_page,
+      const std::string& entry_id) OVERRIDE;
+  virtual scoped_ptr<ViewerHandle> ViewUrl(
+      ViewRequestDelegate* delegate,
+      scoped_ptr<DistillerPage> distiller_page,
+      const GURL& url) OVERRIDE;
+  virtual scoped_ptr<DistillerPage> CreateDefaultDistillerPage() OVERRIDE;
   virtual void AddObserver(DomDistillerObserver* observer) OVERRIDE;
   virtual void RemoveObserver(DomDistillerObserver* observer) OVERRIDE;
 
@@ -118,6 +137,7 @@ class DomDistillerService : public DomDistillerServiceInterface {
   scoped_ptr<DomDistillerStoreInterface> store_;
   scoped_ptr<DistilledContentStore> content_store_;
   scoped_ptr<DistillerFactory> distiller_factory_;
+  scoped_ptr<DistillerPageFactory> distiller_page_factory_;
 
   typedef ScopedVector<TaskTracker> TaskList;
   TaskList tasks_;