Add support for multipage distillation.

components/dom_distiller/core/distiller.h

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef COMPONENTS_DOM_DISTILLER_CORE_DISTILLER_H_
 #define COMPONENTS_DOM_DISTILLER_CORE_DISTILLER_H_
 
 #include <map>
+#include <set>
 #include <string>
 
 #include "base/callback.h"
-#include "base/gtest_prod_util.h"
-#include "base/memory/ref_counted.h"
-#include "base/values.h"
-#include "components/dom_distiller/core/distiller_page.h"
+#include "base/memory/scoped_ptr.h"
 #include "components/dom_distiller/core/distiller_url_fetcher.h"
-#include "components/dom_distiller/core/proto/distilled_page.pb.h"
+#include "components/dom_distiller/core/page_distiller.h"
+#include "components/dom_distiller/core/proto/distilled_article.pb.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "url/gurl.h"
 
 namespace dom_distiller {
 
 class DistillerImpl;
 
 class Distiller {
  public:
-  typedef base::Callback<void(
-      scoped_ptr<DistilledPageProto>)> DistillerCallback;
+  typedef base::Callback<void(scoped_ptr<DistilledArticleProto>)>
+      DistillerCallback;
   virtual ~Distiller() {}
 
   // Distills a page, and asynchrounously returns the article HTML to the
   // supplied callback.
   virtual void DistillPage(const GURL& url,
                            const DistillerCallback& callback) = 0;
 };
 
 class DistillerFactory {
  public:
@@ skipping 10 matching lines @@
   virtual ~DistillerFactoryImpl();
   virtual scoped_ptr<Distiller> CreateDistiller() OVERRIDE;
 
  private:
   scoped_ptr<DistillerPageFactory> distiller_page_factory_;
   scoped_ptr<DistillerURLFetcherFactory> distiller_url_fetcher_factory_;
 };
 
 // Distills a article from a page and associated pages.
 class DistillerImpl : public Distiller,
-                      public DistillerPage::Delegate {
+                      public PageDistiller::PageDistillerCallback {

[cjhopman, 2014/02/03 21:47:22]

This should not subclass a Callback.

[shashi, 2014/02/03 23:19:29]

Done.

  public:
   DistillerImpl(
       const DistillerPageFactory& distiller_page_factory,
       const DistillerURLFetcherFactory& distiller_url_fetcher_factory);
   virtual ~DistillerImpl();
 
   // Creates an execution context. This must be called once before any calls are
   // made to distill the page.
   virtual void Init();
 
   virtual void DistillPage(const GURL& url,
                            const DistillerCallback& callback) OVERRIDE;
 
-  // PageDistillerContext::Delegate
-  virtual void OnLoadURLDone() OVERRIDE;
-  virtual void OnExecuteJavaScriptDone(const base::Value* value) OVERRIDE;
+  void OnFetchImageDone(DistilledPageProto* distilled_page_proto,
+                        const std::string& id,
+                        const std::string& response);
 
-  void OnFetchImageDone(const std::string& id, const std::string& response);
+  // PageDistiller::PageDistillerCallback implementation.

[cjhopman, 2014/02/03 21:47:22]

This comment isn't really right. PageDistillerCallback isn't really an
interface-y thing so there is no "implementation" of it. This is the function
passed as the PageDistillerCallback (I don't think it needs any comment, the
name is enough, unless you want to comment on what it does/how it works).

[shashi, 2014/02/03 23:19:29]

Done.

+  void OnPageDistillationFinished(const GURL& page_url,
+                                  const DistilledPageInfo& distilled_page,
+                                  bool distillation_successful);
 
  private:
-  virtual void LoadURL(const GURL& url);
-  virtual void FetchImage(const std::string& image_id, const std::string& item);
+  virtual void FetchImage(DistilledPageProto* distilled_page_proto,
+                          const std::string& image_id,
+                          const std::string& item);
 
-  // Injects JavaScript to distill a loaded page down to its important content,
-  // e.g., extracting a news article from its surrounding boilerplate.
-  void GetDistilledContent();
+  // Adds url as a new page to the |article_proto_| and triggers distillation
+  // for the newly added page.
+  void DistillNextPage(const GURL& url);

[cjhopman, 2014/02/03 21:47:22]

You should be thinking about how to handle the case where the url that we are
given to distill is not the first page of the article.

[shashi, 2014/02/03 23:19:29]

Good point, currently there is only a next page heuristic, I think what we are
talking about a previous page heuristic. I will have to first add heuristic for
the previous link from the current page. Currently readability does not detect
previous pages. I think depending on heurisitic and the behavior we desire, we
can use page number: negative page numbers and positive page numbers:
map[page_num] = DistilledPageInfo_instance. Once everything is done, we can
create the article and attach pages in sequence.

 
-  const DistillerPageFactory& distiller_page_factory_;
+  // Distills the page.
+  void DistillPage(const GURL& url);
+
+  // Checks if all distillation callbacks are finished and runs the
+  // |distillation_cb_| if callbacks are finished.
+  void CheckIfAllCallbacksAreFinished();
+
   const DistillerURLFetcherFactory& distiller_url_fetcher_factory_;
-  scoped_ptr<DistillerPage> distiller_page_;
+  scoped_ptr<PageDistiller> page_distiller_;
   DistillerCallback distillation_cb_;
 
-  std::map<std::string, DistillerURLFetcher* > image_fetchers_;
-
-  scoped_ptr<DistilledPageProto> proto_;
+  std::map<std::string, DistillerURLFetcher*> image_fetchers_;
+  scoped_ptr<DistilledArticleProto> article_proto_;
+  bool distillation_in_progress_;
+  // Set to keep track of which urls are already seen by the distiller.
+  std::set<std::string> processed_urls_;

[cjhopman, 2014/02/03 21:47:22]

I'd say that this should probably be a hash_set (base/containers/hash_tables.h).
image_fetchers should probably be a hash_map, too.

(then again, with a max size of 32, it doesn't really matter and a simple list
and linear search would probably actually be faster).

[shashi, 2014/02/03 23:19:29]

Done changed to use vector and std::find.

[cjhopman, 2014/02/03 23:56:53]

Sorry, my comment was unclear. I really don't think this should use vector+find.
Please use set or, preferably, hash_set.

[shashi, 2014/02/04 01:39:37]

Ah, changed back to use hash_set.
On 2014/02/03 23:56:53, cjhopman wrote:

 
   DISALLOW_COPY_AND_ASSIGN(DistillerImpl);
 };
 
 }  // namespace dom_distiller
 
 #endif  // COMPONENTS_DOM_DISTILLER_CORE_DISTILLER_H_