Remove usage of DomDistillerService in ReadingListDownloadService.

Remove usage of DomDistillerService in ReadingListDownloadService.

DomDistillerService is a service destined to display a distilled version to the user.
It's settings are optimized to distill pages that are already fully loaded and to
display the page to the user as fast as possible (through a cache feature)..

ReadingList has a different objective.
Download the page in background and store the result for a later use.
ReadingList does not want cache, sync or storage of distilled articles.

This led to have special cases in the usage of DomDistillerService in ReadingList
code. But the fundamental issue is that ReadingListDownloadService should not
use DomDistillerService and directly use the distiller.
This will avoid side effects if some settings changes for ReaderMode.

Remove dependency to the DomDistillerService and use directly the distiller.

BUG=686640
Review-Url: https://codereview.chromium.org/2666643002
Cr-Commit-Position: refs/heads/master@{#447986}
Committed: https://chromium.googlesource.com/chromium/src/+/38219b8d2fbcb7cb75723e3602bda3ce314f0ec2

[Olivier, 2017-01-30 20:44:03]

The CQ bit was checked by olivierrobin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-30 20:44:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-30 20:55:44]

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

[commit-bot: I haz the power, 2017-01-30 20:55:46]

Dry run: This issue passed the CQ dry run.

[Olivier, 2017-01-30 21:27:38]

The CQ bit was checked by olivierrobin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-30 21:28:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-30 21:47:54]

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

[commit-bot: I haz the power, 2017-01-30 21:47:55]

Dry run: This issue passed the CQ dry run.

[Olivier, 2017-01-31 09:23:02]

Description was changed from

==========
Remove usage of DomDistillerService in ReadingListDownloadService.

BUG=
==========

to

==========
Remove usage of DomDistillerService in ReadingListDownloadService.

DomDistillerService is a service that provides cache to the distillation system.
ReadingListDownloadService does not want to use cache.
Remove dependency to the DomDistillerService and use directly the distiller.

BUG=686640
==========

[Olivier, 2017-01-31 09:23:17]

olivierrobin@chromium.org changed reviewers:
+ jif@chromium.org

[jif, 2017-01-31 10:07:53]

lgtm with small nits

https://codereview.chromium.org/2666643002/diff/60001/ios/chrome/browser/dom_...
File ios/chrome/browser/dom_distiller/distiller_viewer.h (right):

https://codereview.chromium.org/2666643002/diff/60001/ios/chrome/browser/dom_...
ios/chrome/browser/dom_distiller/distiller_viewer.h:57:
DistillerViewer(dom_distiller::DomDistillerService* distillerService,
I think this constructor is not used.

https://codereview.chromium.org/2666643002/diff/60001/ios/chrome/browser/dom_...
ios/chrome/browser/dom_distiller/distiller_viewer.h:74: void OnArticleReady(
Can you add a "// DistillerViewerInterface implementation" comment?

https://codereview.chromium.org/2666643002/diff/60001/ios/chrome/browser/dom_...
ios/chrome/browser/dom_distiller/distiller_viewer.h:78: void
OnDistillerFinished(
OnDistillerFinished and OnArticleDistillationUpdated should be private.

https://codereview.chromium.org/2666643002/diff/60001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_download_service_factory.cc
(right):

https://codereview.chromium.org/2666643002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_download_service_factory.cc:64:
std::unique_ptr<dom_distiller::DistillerFactory> distiller_factory =
For info, I think we can start writing:
auto distiller_factory = base::MakeUnique<...

Makes code easier to read, especially with the very limited syntax highlighting
of the codereview tool.

[Olivier, 2017-01-31 10:25:24]

Thanks

https://codereview.chromium.org/2666643002/diff/60001/ios/chrome/browser/dom_...
File ios/chrome/browser/dom_distiller/distiller_viewer.h (right):

https://codereview.chromium.org/2666643002/diff/60001/ios/chrome/browser/dom_...
ios/chrome/browser/dom_distiller/distiller_viewer.h:57:
DistillerViewer(dom_distiller::DomDistillerService* distillerService,
On 2017/01/31 10:07:53, jif wrote:
> I think this constructor is not used.

It is used in ReaderModeController below (even if it is never called in fact...)

https://codereview.chromium.org/2666643002/diff/60001/ios/chrome/browser/dom_...
ios/chrome/browser/dom_distiller/distiller_viewer.h:74: void OnArticleReady(
On 2017/01/31 10:07:53, jif wrote:
> Can you add a "// DistillerViewerInterface implementation" comment?

Done.

https://codereview.chromium.org/2666643002/diff/60001/ios/chrome/browser/dom_...
ios/chrome/browser/dom_distiller/distiller_viewer.h:78: void
OnDistillerFinished(
On 2017/01/31 10:07:53, jif wrote:
> OnDistillerFinished and OnArticleDistillationUpdated should be private.

Done.

https://codereview.chromium.org/2666643002/diff/60001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_download_service_factory.cc
(right):

https://codereview.chromium.org/2666643002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_download_service_factory.cc:64:
std::unique_ptr<dom_distiller::DistillerFactory> distiller_factory =
On 2017/01/31 10:07:53, jif wrote:
> For info, I think we can start writing:
> auto distiller_factory = base::MakeUnique<...
> 
> Makes code easier to read, especially with the very limited syntax
highlighting
> of the codereview tool.

Done.

[Olivier, 2017-01-31 10:25:25]

Thanks

[Olivier, 2017-01-31 10:29:31]

Description was changed from

==========
Remove usage of DomDistillerService in ReadingListDownloadService.

DomDistillerService is a service that provides cache to the distillation system.
ReadingListDownloadService does not want to use cache.
Remove dependency to the DomDistillerService and use directly the distiller.

BUG=686640
==========

to

==========
Remove usage of DomDistillerService in ReadingListDownloadService.

DomDistillerService is a service destined to display a distilled version to the
user.
It's settings are optimized to distill pages that are already fully loaded and
to
display the page to the user as fast as possible (through a cache feature)..

ReadingList has a different objective.
Download the page in background and store the result for a later use.
ReadingList does not want cache, sync or storage of distilled articles.

This led to have special cases in the usage of DomDistillerService in
ReadingList
code. But the fundamental issue is that ReadingListDownloadService should not
use DomDistillerService and directly use the distiller.
This will avoid side effects if some settings changes for ReaderMode.

Remove dependency to the DomDistillerService and use directly the distiller.

BUG=686640
==========

[Olivier, 2017-01-31 10:29:58]

olivierrobin@chromium.org changed reviewers:
+ noyau@chromium.org

[Olivier, 2017-01-31 10:29:58]

+noyau as DomDistiller owner.

[noyau (Ping after 24h), 2017-02-03 12:56:11]

lgtm

https://codereview.chromium.org/2666643002/diff/80001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_download_service_factory.cc
(right):

https://codereview.chromium.org/2666643002/diff/80001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_download_service_factory.cc:62:
dom_distiller::proto::DomDistillerOptions options;
Any options we should set here?

[Olivier, 2017-02-03 13:01:32]

https://codereview.chromium.org/2666643002/diff/80001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_download_service_factory.cc
(right):

https://codereview.chromium.org/2666643002/diff/80001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_download_service_factory.cc:62:
dom_distiller::proto::DomDistillerOptions options;
On 2017/02/03 12:56:11, noyau wrote:
> Any options we should set here?

Options are limited: debug level, text/html and pagination.
I would like to add pagination=none, but it is not yet implemented.
crbug.com/676265

[Olivier, 2017-02-03 13:17:58]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2017-02-03 13:17:59]

The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org
Link to the patchset: https://codereview.chromium.org/2666643002/#ps80001
(title: "feedback")

[commit-bot: I haz the power, 2017-02-03 13:18:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 13:38:18]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486127878634850,
"parent_rev": "e78becc4bf2ae384a486fe08dd827d56e4c45628", "commit_rev":
"38219b8d2fbcb7cb75723e3602bda3ce314f0ec2"}

[commit-bot: I haz the power, 2017-02-03 13:38:55]

Description was changed from

==========
Remove usage of DomDistillerService in ReadingListDownloadService.

DomDistillerService is a service destined to display a distilled version to the
user.
It's settings are optimized to distill pages that are already fully loaded and
to
display the page to the user as fast as possible (through a cache feature)..

ReadingList has a different objective.
Download the page in background and store the result for a later use.
ReadingList does not want cache, sync or storage of distilled articles.

This led to have special cases in the usage of DomDistillerService in
ReadingList
code. But the fundamental issue is that ReadingListDownloadService should not
use DomDistillerService and directly use the distiller.
This will avoid side effects if some settings changes for ReaderMode.

Remove dependency to the DomDistillerService and use directly the distiller.

BUG=686640
==========

to

==========
Remove usage of DomDistillerService in ReadingListDownloadService.

DomDistillerService is a service destined to display a distilled version to the
user.
It's settings are optimized to distill pages that are already fully loaded and
to
display the page to the user as fast as possible (through a cache feature)..

ReadingList has a different objective.
Download the page in background and store the result for a later use.
ReadingList does not want cache, sync or storage of distilled articles.

This led to have special cases in the usage of DomDistillerService in
ReadingList
code. But the fundamental issue is that ReadingListDownloadService should not
use DomDistillerService and directly use the distiller.
This will avoid side effects if some settings changes for ReaderMode.

Remove dependency to the DomDistillerService and use directly the distiller.

BUG=686640

Review-Url: https://codereview.chromium.org/2666643002
Cr-Commit-Position: refs/heads/master@{#447986}
Committed:
https://chromium.googlesource.com/chromium/src/+/38219b8d2fbcb7cb75723e3602bd...
==========

[commit-bot: I haz the power, 2017-02-03 13:38:56]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/38219b8d2fbcb7cb75723e3602bd...