Add a DistilledContentStore (and an in-memory impl)

Add a DistilledContentStore (and an in-memory impl)

This is the interface to whatever is used for storing and fetching
distilled content. In the future, this should be provided by sync
attachments. For now, we need something.

The interface is a very simple pair of functions, SaveContent and
LoadContent, both asynchronous.

There is a simple in-memory implementation that just stores saved
content in a map.

The DomDistillerService creates the store and uses a save callback on
the TaskTracker to save content to it. The TaskTracker receives a
pointer to the store and uses it when StartBlobFetcher is called.

BUG=288015
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264624

[cjhopman, 2014-03-12 22:52:32]

nyquist: *

[shashi, 2014-03-13 00:32:44]

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
File components/dom_distiller.gypi (right):

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
components/dom_distiller.gypi:63:
'dom_distiller/core/distilled_content_store.h',
sort.

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
File components/dom_distiller/core/distilled_content_store.cc (right):

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
components/dom_distiller/core/distilled_content_store.cc:1: // Copyright 2013
The Chromium Authors. All rights reserved.
s/2013/2014

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
components/dom_distiller/core/distilled_content_store.cc:30: if
(callback.is_null())
This null check should be before the lookup.

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
File components/dom_distiller/core/distilled_content_store.h (right):

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
components/dom_distiller/core/distilled_content_store.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
components/dom_distiller/core/distilled_content_store.h:5: #ifndef
COMPONENTS_DOM_DISTILLER_CORE_DOM_DISTILLER_CONTENT_CACHE_H_
s/CACHE/STORE/

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
components/dom_distiller/core/distilled_content_store.h:48: typedef
std::map<std::string, DistilledArticleProto> ContentMap;
why not base::hash_map?

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
File components/dom_distiller/core/dom_distiller_service.cc (right):

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_service.cc:146:
base::Bind(&DomDistillerService::AddDistilledContentToStore,
Feels like these 3 lines can be now refactored to a function.

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
File components/dom_distiller/core/task_tracker.cc (right):

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
components/dom_distiller/core/task_tracker.cc:184: content_ready_ = true;
redundant now.

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
File components/dom_distiller/core/task_tracker_unittest.cc (right):

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
components/dom_distiller/core/task_tracker_unittest.cc:273: const
DistilledArticleProto* final_distilled_article;
Why is final_distilled_article needed?

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
components/dom_distiller/core/task_tracker_unittest.cc:281: 
Should there be a  testing::Mock::VerifyAndClearExpectations(&viewer_delegate);
here?

[cjhopman, 2014-03-18 16:06:10]

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
File components/dom_distiller.gypi (right):

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
components/dom_distiller.gypi:63:
'dom_distiller/core/distilled_content_store.h',
On 2014/03/13 00:32:45, shashi wrote:
> sort.

Done.

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
File components/dom_distiller/core/distilled_content_store.cc (right):

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
components/dom_distiller/core/distilled_content_store.cc:1: // Copyright 2013
The Chromium Authors. All rights reserved.
On 2014/03/13 00:32:45, shashi wrote:
> s/2013/2014

Done.

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
components/dom_distiller/core/distilled_content_store.cc:30: if
(callback.is_null())
On 2014/03/13 00:32:45, shashi wrote:
> This null check should be before the lookup.

Done.

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
File components/dom_distiller/core/distilled_content_store.h (right):

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
components/dom_distiller/core/distilled_content_store.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2014/03/13 00:32:45, shashi wrote:
> 2014

Done.

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
components/dom_distiller/core/distilled_content_store.h:5: #ifndef
COMPONENTS_DOM_DISTILLER_CORE_DOM_DISTILLER_CONTENT_CACHE_H_
On 2014/03/13 00:32:45, shashi wrote:
> s/CACHE/STORE/

Done.

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
components/dom_distiller/core/distilled_content_store.h:48: typedef
std::map<std::string, DistilledArticleProto> ContentMap;
On 2014/03/13 00:32:45, shashi wrote:
> why not base::hash_map?

Done.

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
File components/dom_distiller/core/task_tracker.cc (right):

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
components/dom_distiller/core/task_tracker.cc:184: content_ready_ = true;
On 2014/03/13 00:32:45, shashi wrote:
> redundant now.

Done.

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
File components/dom_distiller/core/task_tracker_unittest.cc (right):

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
components/dom_distiller/core/task_tracker_unittest.cc:273: const
DistilledArticleProto* final_distilled_article;
On 2014/03/13 00:32:45, shashi wrote:
> Why is final_distilled_article needed?

Done.

https://codereview.chromium.org/189833002/diff/60001/components/dom_distiller...
components/dom_distiller/core/task_tracker_unittest.cc:281: 
On 2014/03/13 00:32:45, shashi wrote:
> Should there be a 
testing::Mock::VerifyAndClearExpectations(&viewer_delegate);
> here?
How about this?

[shashi, 2014-03-18 21:14:57]

https://chromiumcodereview.appspot.com/189833002/diff/80001/components/dom_di...
File components/dom_distiller/core/distilled_content_store.cc (right):

https://chromiumcodereview.appspot.com/189833002/diff/80001/components/dom_di...
components/dom_distiller/core/distilled_content_store.cc:31:
ContentMap::iterator it = cache_.find(entry.entry_id());
nit: const_iterator?

https://chromiumcodereview.appspot.com/189833002/diff/80001/components/dom_di...
File components/dom_distiller/core/distilled_content_store.h (right):

https://chromiumcodereview.appspot.com/189833002/diff/80001/components/dom_di...
components/dom_distiller/core/distilled_content_store.h:26: virtual void
LoadContent(const ArticleEntry& entry,
nit: Can this method be const? i.e. LoadContent just looks up ArticleEntry
|entry| does not modify the content store.

https://chromiumcodereview.appspot.com/189833002/diff/80001/components/dom_di...
components/dom_distiller/core/distilled_content_store.h:29: virtual
~DistilledContentStore() {}
DISALLOW_COPY_AND_ASSIGN(DistilledContentStore);

https://chromiumcodereview.appspot.com/189833002/diff/80001/components/dom_di...
File components/dom_distiller/core/task_tracker.cc (right):

https://chromiumcodereview.appspot.com/189833002/diff/80001/components/dom_di...
components/dom_distiller/core/task_tracker.cc:184: if (distillation_successful)
{
Can OnDistillerFinished determine if the distillation was successful:
(distilled_article->pages_size() > 0)  and pass that to
OnDistilledArticleReady(scoped_ptr<> distilled_article, bool success)

This was both onBlobFetched and OnDistilledFinished call OnDistilledArticleReady
with additional success parameter and only OnDistillerFinished adds the content
to the store.

[nyquist, 2014-04-10 19:00:12]

https://codereview.chromium.org/189833002/diff/80001/components/dom_distiller...
File components/dom_distiller/core/distilled_content_store.h (right):

https://codereview.chromium.org/189833002/diff/80001/components/dom_distiller...
components/dom_distiller/core/distilled_content_store.h:51: };
DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/189833002/diff/80001/components/dom_distiller...
File components/dom_distiller/core/task_tracker.cc (right):

https://codereview.chromium.org/189833002/diff/80001/components/dom_distiller...
components/dom_distiller/core/task_tracker.cc:172: bool distillation_successful
= false;
how about bool distillation_successful = distilled_article_->pages_size() > 0;

And then maybe even merge this and the block below?
Or have an additional if for the success if you do something along the lines of
what shashi mentioned inside the block.

https://codereview.chromium.org/189833002/diff/80001/components/dom_distiller...
components/dom_distiller/core/task_tracker.cc:184: if (distillation_successful)
{
On 2014/03/18 21:14:57, shashi wrote:
> Can OnDistillerFinished determine if the distillation was successful:
> (distilled_article->pages_size() > 0)  and pass that to
> OnDistilledArticleReady(scoped_ptr<> distilled_article, bool success)
> 
> This was both onBlobFetched and OnDistilledFinished call
OnDistilledArticleReady
> with additional success parameter and only OnDistillerFinished adds the
content
> to the store.

Or how about just adding a param |store_if_valid| to this method and only pass
true in OnDistillerFinished?

[cjhopman, 2014-04-11 22:37:21]

This is now a bit more disciplined about handling the multiple sources of
distilled articles.

When a source finishes it:

Calls DistilledArticleReady
  This sets content_ready_ if the distilled article is good
  and also alerts viewers + callbacks
Calls ContentSourceFinished
  Which cancels all pending sources if the content_ready_ is true
  Otherwise if all sources have finished, it alerts viewers+callbacks of failure

https://codereview.chromium.org/189833002/diff/80001/components/dom_distiller...
File components/dom_distiller/core/distilled_content_store.cc (right):

https://codereview.chromium.org/189833002/diff/80001/components/dom_distiller...
components/dom_distiller/core/distilled_content_store.cc:31:
ContentMap::iterator it = cache_.find(entry.entry_id());
On 2014/03/18 21:14:57, shashi wrote:
> nit: const_iterator?

Done.

https://codereview.chromium.org/189833002/diff/80001/components/dom_distiller...
File components/dom_distiller/core/distilled_content_store.h (right):

https://codereview.chromium.org/189833002/diff/80001/components/dom_distiller...
components/dom_distiller/core/distilled_content_store.h:26: virtual void
LoadContent(const ArticleEntry& entry,
On 2014/03/18 21:14:57, shashi wrote:
> nit: Can this method be const? i.e. LoadContent just looks up ArticleEntry
> |entry| does not modify the content store.

Done.

https://codereview.chromium.org/189833002/diff/80001/components/dom_distiller...
components/dom_distiller/core/distilled_content_store.h:29: virtual
~DistilledContentStore() {}
On 2014/03/18 21:14:57, shashi wrote:
> DISALLOW_COPY_AND_ASSIGN(DistilledContentStore);

Done.

https://codereview.chromium.org/189833002/diff/80001/components/dom_distiller...
components/dom_distiller/core/distilled_content_store.h:51: };
On 2014/04/10 19:00:13, nyquist wrote:
> DISALLOW_COPY_AND_ASSIGN?
Now disallowed in the base class.

https://codereview.chromium.org/189833002/diff/80001/components/dom_distiller...
File components/dom_distiller/core/task_tracker.cc (right):

https://codereview.chromium.org/189833002/diff/80001/components/dom_distiller...
components/dom_distiller/core/task_tracker.cc:172: bool distillation_successful
= false;
On 2014/04/10 19:00:13, nyquist wrote:
> how about bool distillation_successful = distilled_article_->pages_size() > 0;
> 
> And then maybe even merge this and the block below?
> Or have an additional if for the success if you do something along the lines
of
> what shashi mentioned inside the block.

Done.

https://codereview.chromium.org/189833002/diff/80001/components/dom_distiller...
components/dom_distiller/core/task_tracker.cc:184: if (distillation_successful)
{
On 2014/04/10 19:00:13, nyquist wrote:
> On 2014/03/18 21:14:57, shashi wrote:
> > Can OnDistillerFinished determine if the distillation was successful:
> > (distilled_article->pages_size() > 0)  and pass that to
> > OnDistilledArticleReady(scoped_ptr<> distilled_article, bool success)
> > 
> > This was both onBlobFetched and OnDistilledFinished call
> OnDistilledArticleReady
> > with additional success parameter and only OnDistillerFinished adds the
> content
> > to the store.
> 
> Or how about just adding a param |store_if_valid| to this method and only pass
> true in OnDistillerFinished?

So this should be more correct now.

[nyquist, 2014-04-14 22:42:21]

lgtm

https://codereview.chromium.org/189833002/diff/130009/components/dom_distille...
File components/dom_distiller/core/distilled_content_store.h (right):

https://codereview.chromium.org/189833002/diff/130009/components/dom_distille...
components/dom_distiller/core/distilled_content_store.h:35: class
InMemoryContentStore : public DistilledContentStore {
This is very simple and nice for now. But this store can have unbounded growth
with no support for cleaning up. Could you add a quick comment explaining that?
I guess it is kind of given with the name *Store instead of *Cache, but still,
it would be nice to clarify.

https://codereview.chromium.org/189833002/diff/130009/components/dom_distille...
File components/dom_distiller/core/task_tracker_unittest.cc (right):

https://codereview.chromium.org/189833002/diff/130009/components/dom_distille...
components/dom_distiller/core/task_tracker_unittest.cc:264: 
Nit: some extra newlines here.

[cjhopman, 2014-04-15 01:04:25]

The CQ bit was checked by cjhopman@chromium.org

[commit-bot: I haz the power, 2014-04-15 01:05:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cjhopman@chromium.org/189833002/180001

[commit-bot: I haz the power, 2014-04-15 01:53:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-15 01:53:08]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[cjhopman, 2014-04-17 17:59:59]

The CQ bit was checked by cjhopman@chromium.org

[commit-bot: I haz the power, 2014-04-17 18:01:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cjhopman@chromium.org/189833002/180001

[commit-bot: I haz the power, 2014-04-17 20:29:44]

Change committed as 264624