Start requiring DistillerPage for calls to DomDistillerService.

Start requiring DistillerPage for calls to DomDistillerService.

The DomDistillerService now requires callers to pass in DistillerPage.
This enables callers to more closely control how the page is distilled.

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

[nyquist, 2014-04-23 23:57:31]

cjhopman: PTAL

This depends on another CL, so all tryjobs will fail. I'll retry when that CL
has been submitted.

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

https://codereview.chromium.org/254483003/diff/1/components/dom_distiller/cor...
File components/dom_distiller/core/dom_distiller_service.h (right):

https://codereview.chromium.org/254483003/diff/1/components/dom_distiller/cor...
components/dom_distiller/core/dom_distiller_service.h:48: // represents whether
the article is available offline.
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:12]

cjhopman: PTAL

https://codereview.chromium.org/254483003/diff/1/components/dom_distiller/cor...
File components/dom_distiller/core/dom_distiller_service.h (right):

https://codereview.chromium.org/254483003/diff/1/components/dom_distiller/cor...
components/dom_distiller/core/dom_distiller_service.h:48: // represents whether
the article is available offline.
On 2014/04/24 23:03:05, cjhopman wrote:
> 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.

I have now tried to clarify this in the documentation of DistillerPage.

> 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?

Added this requirement to the DistillerPageFactory documentation. FWIW, the
current implementation has a cheap constructor for this exact reason.

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

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.

[cjhopman, 2014-04-25 17:23:00]

lgtm

https://codereview.chromium.org/254483003/diff/20001/components/dom_distiller...
File components/dom_distiller/core/distiller_page.h (right):

https://codereview.chromium.org/254483003/diff/20001/components/dom_distiller...
components/dom_distiller/core/distiller_page.h:46: // fails. May be called when
the distiller is idle. Callers can assume, that
s/, that/ that,/

https://codereview.chromium.org/254483003/diff/20001/components/dom_distiller...
components/dom_distiller/core/distiller_page.h:47: // for a given |url|, and
DistillerPage implementation will extract the same
s/and/any

[nyquist, 2014-04-25 21:37:23]

addressed all comments.
Adding asargent@chromium.org for TBR of consumption of DomDistillerService API
from extension API.

https://codereview.chromium.org/254483003/diff/20001/components/dom_distiller...
File components/dom_distiller/core/distiller_page.h (right):

https://codereview.chromium.org/254483003/diff/20001/components/dom_distiller...
components/dom_distiller/core/distiller_page.h:46: // fails. May be called when
the distiller is idle. Callers can assume, that
On 2014/04/25 17:23:01, cjhopman wrote:
> s/, that/ that,/

Done.

https://codereview.chromium.org/254483003/diff/20001/components/dom_distiller...
components/dom_distiller/core/distiller_page.h:47: // for a given |url|, and
DistillerPage implementation will extract the same
On 2014/04/25 17:23:01, cjhopman wrote:
> s/and/any

Done.

[nyquist, 2014-04-25 23:02:43]

The CQ bit was checked by nyquist@chromium.org

[commit-bot: I haz the power, 2014-04-25 23:04:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nyquist@chromium.org/254483003/30001

[asargent_no_longer_on_chrome, 2014-04-25 23:24:57]

lgtm

https://codereview.chromium.org/254483003/diff/30001/chrome/browser/extension...
File
chrome/browser/extensions/api/reading_list_private/reading_list_private_api.cc
(right):

https://codereview.chromium.org/254483003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/reading_list_private/reading_list_private_api.cc:41:
service->CreateDefaultDistillerPage().Pass(),
style nit: indentation looks like it might be wrong? 

This might fail the auto-clang-format presubmit check - if it does you just need
to run 'git cl format' (which will run across all the files in your CL - if that
results in a ton of changes you don't want, just revert the changes it makes
outside the chrome/browser/extensions/ directory)

[commit-bot: I haz the power, 2014-04-25 23:57:45]

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

[commit-bot: I haz the power, 2014-04-25 23:57:45]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[nyquist, 2014-04-27 00:30:30]

https://codereview.chromium.org/254483003/diff/30001/chrome/browser/extension...
File
chrome/browser/extensions/api/reading_list_private/reading_list_private_api.cc
(right):

https://codereview.chromium.org/254483003/diff/30001/chrome/browser/extension...
chrome/browser/extensions/api/reading_list_private/reading_list_private_api.cc:41:
service->CreateDefaultDistillerPage().Pass(),
On 2014/04/25 23:24:57, Antony Sargent wrote:
> style nit: indentation looks like it might be wrong? 
> 
> This might fail the auto-clang-format presubmit check - if it does you just
need
> to run 'git cl format' (which will run across all the files in your CL - if
that
> results in a ton of changes you don't want, just revert the changes it makes
> outside the chrome/browser/extensions/ directory)

Done.

[nyquist, 2014-04-27 00:30:38]

The CQ bit was checked by nyquist@chromium.org

[nyquist, 2014-04-27 00:30:46]

The CQ bit was unchecked by nyquist@chromium.org

[nyquist, 2014-04-27 00:31:04]

The CQ bit was checked by nyquist@chromium.org

[commit-bot: I haz the power, 2014-04-27 00:31:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nyquist@chromium.org/254483003/50001

[commit-bot: I haz the power, 2014-04-27 01:10:37]

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

[commit-bot: I haz the power, 2014-04-27 01:10:37]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[nyquist, 2014-04-27 22:32:49]

The CQ bit was checked by nyquist@chromium.org

[commit-bot: I haz the power, 2014-04-27 22:33:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nyquist@chromium.org/254483003/50001

[commit-bot: I haz the power, 2014-04-28 08:14:03]

Change committed as 266486