Store page no for distilled pages undergoing 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 <string>
 
 #include "base/callback.h"
@@ skipping 51 matching lines @@
   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;
 
  private:
-  void OnFetchImageDone(DistilledPageProto* distilled_page_proto,
+  // In case of multiple pages, the Distiller maintains state of multiple pages
+  // as relative page numbers. E.g. if distillation starts at page 2 for a 3
+  // page article. The relative page numbers assigned to pages will be [-1,0,1].
+
+  // Class representing the state of a page under distillation.
+  struct DistilledPageData {
+    DistilledPageData();
+    virtual ~DistilledPageData();
+    // Relative page number of the page.
+    int page_no;
+    std::string title;
+    ScopedVector<DistillerURLFetcher> image_fetchers_;
+    scoped_ptr<DistilledPageProto> proto;
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(DistilledPageData);
+  };
+
+  void OnFetchImageDone(DistilledPageData* distilled_page_data,
                         DistillerURLFetcher* url_fetcher,
                         const std::string& id,
                         const std::string& response);
 
-  void OnPageDistillationFinished(const GURL& page_url,
+  void OnPageDistillationFinished(int page_no,
                                   scoped_ptr<DistilledPageInfo> distilled_page,
                                   bool distillation_successful);
 
-  virtual void FetchImage(DistilledPageProto* distilled_page_proto,
+  virtual void FetchImage(DistilledPageData* distilled_page_data,
                           const std::string& image_id,
                           const std::string& item);
 
-  // Distills the page and adds the new page to |article_proto|.
-  void DistillPage(const GURL& url);
+  // Distills the next page.
+  void DistillNextPage();
+
+  // Adds the |url| to |pages_to_be_distilled| if |page_no| is a valid relative
+  // page number and |url| is valid. Ignores duplicate pages and urls.
+  void AddToDistillationQueue(int page_no, const GURL& url);
+
+  // Check if |page_no| is a valid relative page number, i.e. page with
+  // |page_no| is either under distillation or has already completed
+  // distillation.
+  bool IsValidPageNo(int page_no) const;
 
   // Runs |distillation_cb_| if all distillation callbacks and image fetches are
   // complete.
   void RunDistillerCallbackIfDone();
 
+  // Checks if page |distilled_page_data| has finished distillation, including
+  // all image fetches.
+  void CheckIfPageDone(const DistilledPageData* distilled_page_data);
+
   const DistillerURLFetcherFactory& distiller_url_fetcher_factory_;
   scoped_ptr<PageDistiller> page_distiller_;
   DistillerCallback distillation_cb_;
 
-  ScopedVector<DistillerURLFetcher> image_fetchers_;
-  scoped_ptr<DistilledArticleProto> article_proto_;
-  bool distillation_in_progress_;
+  // Set of pages which have finished distillation. Note: some pages may be
+  // waiting for image fetches to be complete.
+  // |distilled_pages_index_| maintains the mapping from page number to the
+  // index in |distilled_pages_|.
+  ScopedVector<DistilledPageData> distilled_pages_;
+
+  // Maps page number to the index in |distilled_pages_|.
+  std::map<int, size_t> distilled_pages_index_;

[cjhopman, 2014/02/12 20:39:09]

Couldn't this just point right to the DPD in distilled_pages? i.e. std::map<int,
DistilledPageData*>?

[shashi, 2014/02/13 01:03:11]

It could but then I have to manually manage the lifetime of distilled pages,
instead of using ScopedVector, I can switch to  STLDeleteSecondElements?

On 2014/02/12 20:39:09, cjhopman wrote:

[cjhopman, 2014/02/13 02:48:48]

I mean it would point into distilled_pages. I.e. the ScopedVector still owns
them, and this maps relative page number directly to a DPD*. Both of these
approaches basically require that we don't remove something from
distilled_pages_ without also removing them from the index map. Maybe it is
actually simpler to just use a map to pointers and just manually handle the
memory; at least then it is obvious that you need to be careful.

Another option is to use a std::map<int, linked_ptr<DPD> >. Or an std::map<int,
scoped_refptr<RefCountedData<DPD> > >. That last one might be good because we
will probably want to use RefCountedData for incremental updates.

[shashi, 2014/02/13 20:09:51]

When I will add incremental updates in my later patch I can switch it to
linked_ptr/RefCounted Data as appropriate. Currently the distiller just copies
these pages to outgoing article_proto and then deletes these pages once
distillation finishes. 
I think advantage of having them in a ScopedVector, is there is not need to
delete these pages when distillation finishes.

I do not have strong opinions about storing pointers in the map vs indices. One
advantage that I can think of the index based approach is: if there is an
invalid index in the map, and the code tries to reference it, the code will
crash in an *obvious* way at the point of reference, since the ScopedVector will
fail with index out of bounds, DCHECK. 
In case of pointers, the code will need to insert some DCHECKs to ensure safety.

+
+  // The list of pages that are still waiting for distillation to finish.
+  std::map<int, GURL> pages_to_be_distilled_;
+
+  // The page number of pages that are either waiting for distillation or image

[cjhopman, 2014/02/12 20:39:09]

This includes pages waiting in pages_to_be_distilled?

[shashi, 2014/02/13 01:03:11]

Yes, it does, any unfinished pages.

+  // fetches. If a page is |in_progress_pages_| that means it is still waiting
+  // for an action (distillation or image fetch) to finish.
+  base::hash_set<int> in_progress_pages_;

[cjhopman, 2014/02/12 20:39:09]

It's unclear how a page distillation works through these steps. Could we rename
these to something like:
distilled_pages_
in_progress_pages_
waiting_pages_

other options
distilled/finished
in_progress/working/distilling/started
waiting/pending/queued

Maybe waiting/started/finished is best?

[shashi, 2014/02/13 01:03:11]

Done, hopefully more clear now.
On 2014/02/12 20:39:09, cjhopman wrote:

[cjhopman, 2014/02/13 02:48:48]

This isn't really more clear, particularly because pending and waiting mean
almost the same thing.

My intention was that a particular page_no should only be in one of three
states: pending, started, or finished. These three containers should correspond
to those three states. Then the flow of a page is more obvious.

On 2014/02/13 01:03:11, shashi wrote:

[shashi, 2014/02/13 20:09:51]

in_progress_pages_ = all pages that are not finished, this might include pages
waiting in the queue + pages currently not finished. A lot of places inside the
distiller we query about any unfinished pages, so it feels cleaner to just have
in_progress pages. Although I do not feel strongly about this and can change it
and replace the query with a function: bool AnyUnfinishedPages().

On 2014/02/13 02:48:48, cjhopman wrote:

[cjhopman, 2014/02/13 20:43:19]

I would much prefer that. It was difficult reading this code to figure out what
these all meant and what container each thing was expected to be in at each
point. I really think a page should only be in one of these containers at a
time.

[shashi, 2014/02/13 21:57:25]

Done.

+
   // Set to keep track of which urls are already seen by the distiller.

[cjhopman, 2014/02/12 20:39:09]

This comment should say what this is used for. Also, this should probably be
seen_urls_ or something and we should be adding things to it when we first see
them (i.e. when they are added to pages_to_be_distilled).

[shashi, 2014/02/13 01:03:11]

Done.

   base::hash_set<std::string> processed_urls_;
 
+  scoped_ptr<DistilledArticleProto> article_proto_;

[cjhopman, 2014/02/12 20:39:09]

This is only actually created and used in RunDistillerCallbackIfDone, so why not
just make it local in there?

[shashi, 2014/02/13 01:03:11]

Because I was concerned about the change in lifetime of article_proto_
Note the api documentation for ViewRequestDelegate:
https://code.google.com/p/chromium/codesearch#chromium/src/components/dom_dis...

On 2014/02/12 20:39:09, cjhopman wrote:

[cjhopman, 2014/02/13 02:48:48]

Ownership of it is passed to the DistillerCallback.

On 2014/02/13 01:03:11, shashi wrote:

[shashi, 2014/02/13 20:09:51]

Duh! Done,  that file is task_tracker :).
On 2014/02/13 02:48:48, cjhopman wrote:

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