Create distiller files for Reading List.

Create distiller files for Reading List.

DomDistillerService is used for ReadingList and ReaderMode.
The requirements for both feature are different.
As DomDistillerService main purpose (cross-platform) is Reader Mode,
this CL creates files for the Reading List distillation.
These files ensure that:
- WebState used for distillation survives at least 10 seconds after the
  end of distillation to allow loading favicon
- There is a 2 seconds delay between end of page load and start of
  distillation.
- Reading List files are in Reading List components.

These 2 requirements are irrelevant for Reader Mode as the page is
already loaded and the favicon is not used.
This CL also restore the behavior of Reader Mode before cl/2529283002.

BUG=648923
Committed: https://crrev.com/31daab21ab182a3272daec45c77626c1628bd4d3
Cr-Commit-Position: refs/heads/master@{#441060}

[Olivier, 2016-12-27 08:52:18]

Description was changed from

==========
Create distiller files for Reading List.

DomDistillerService is used for ReadingList and ReaderMode.
The requirement for both feature are different.
As DomDistillerService main purpose (cross-platform) is Reader Mode,
this CL creates files for the Reading List distillation.
These files ensure that:
- WebState used for distillation survives at least 10 seconds after the
  end of distillation to allow loading favicon
- There is a 2 seconds delay between end of page load and start of
  distillation.

These 2 requirements are irrelevant for Reader Mode as the page is
already loaded and the favicon is not used.
This CL also restore the behavior of Reader Mode before cl/2529283002.

BUG=648923
==========

to

==========
Create distiller files for Reading List.

DomDistillerService is used for ReadingList and ReaderMode.
The requirement for both feature are different.
As DomDistillerService main purpose (cross-platform) is Reader Mode,
this CL creates files for the Reading List distillation.
These files ensure that:
- WebState used for distillation survives at least 10 seconds after the
  end of distillation to allow loading favicon
- There is a 2 seconds delay between end of page load and start of
  distillation.
- Reading List files are in Reading List components.

These 2 requirements are irrelevant for Reader Mode as the page is
already loaded and the favicon is not used.
This CL also restore the behavior of Reader Mode before cl/2529283002.

BUG=648923
==========

[Olivier, 2016-12-27 08:56:07]

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

[Olivier, 2016-12-27 08:56:50]

Description was changed from

==========
Create distiller files for Reading List.

DomDistillerService is used for ReadingList and ReaderMode.
The requirement for both feature are different.
As DomDistillerService main purpose (cross-platform) is Reader Mode,
this CL creates files for the Reading List distillation.
These files ensure that:
- WebState used for distillation survives at least 10 seconds after the
  end of distillation to allow loading favicon
- There is a 2 seconds delay between end of page load and start of
  distillation.
- Reading List files are in Reading List components.

These 2 requirements are irrelevant for Reader Mode as the page is
already loaded and the favicon is not used.
This CL also restore the behavior of Reader Mode before cl/2529283002.

BUG=648923
==========

to

==========
Create distiller files for Reading List.

DomDistillerService is used for ReadingList and ReaderMode.
The requirements for both feature are different.
As DomDistillerService main purpose (cross-platform) is Reader Mode,
this CL creates files for the Reading List distillation.
These files ensure that:
- WebState used for distillation survives at least 10 seconds after the
  end of distillation to allow loading favicon
- There is a 2 seconds delay between end of page load and start of
  distillation.
- Reading List files are in Reading List components.

These 2 requirements are irrelevant for Reader Mode as the page is
already loaded and the favicon is not used.
This CL also restore the behavior of Reader Mode before cl/2529283002.

BUG=648923
==========

[gambard, 2016-12-27 09:35:59]

https://codereview.chromium.org/2604773002/diff/1/components/dom_distiller/io...
File components/dom_distiller/ios/distiller_page_ios.h (right):

https://codereview.chromium.org/2604773002/diff/1/components/dom_distiller/io...
components/dom_distiller/ios/distiller_page_ios.h:45: void DistillPage();
Comment

https://codereview.chromium.org/2604773002/diff/1/components/dom_distiller/io...
components/dom_distiller/ios/distiller_page_ios.h:45: void DistillPage();
This function name is already used to start the distillation. It might create
some confusion.
Maybe you can change it to something like InjectScript?

https://codereview.chromium.org/2604773002/diff/1/components/dom_distiller/io...
File components/dom_distiller/ios/distiller_page_ios.mm (left):

https://codereview.chromium.org/2604773002/diff/1/components/dom_distiller/io...
components/dom_distiller/ios/distiller_page_ios.mm:168:
favicon_driver->FetchFavicon(url_);
I don't see this code in the new version.

https://codereview.chromium.org/2604773002/diff/1/components/dom_distiller/io...
File components/dom_distiller/ios/distiller_page_ios.mm (right):

https://codereview.chromium.org/2604773002/diff/1/components/dom_distiller/io...
components/dom_distiller/ios/distiller_page_ios.mm:153:
std::unique_ptr<web::WebState> DistillerPageIOS::ReplaceWebState(
I am not fond of this logic of switching web states around.
Instead, can't you use a lazy getter?
The subclass would only need to destroy the WebState and override the getter.
WDYT?

https://codereview.chromium.org/2604773002/diff/1/components/dom_distiller/io...
components/dom_distiller/ios/distiller_page_ios.mm:198:
base::TimeDelta::FromSeconds(2));
Use a constant for the "2".

https://codereview.chromium.org/2604773002/diff/1/components/reading_list/ios...
File components/reading_list/ios/favicon_web_state_dispatcher.h (right):

https://codereview.chromium.org/2604773002/diff/1/components/reading_list/ios...
components/reading_list/ios/favicon_web_state_dispatcher.h:16: // their creation
and their return. After a WebState is returned, the dispatcher
Update the comment: the webstate is not kept alive between the two calls
anymore.

https://codereview.chromium.org/2604773002/diff/1/components/reading_list/ios...
components/reading_list/ios/favicon_web_state_dispatcher.h:32: }  // namespace
dom_distiller
Update namespace

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/dom_dist...
File ios/chrome/browser/dom_distiller/distiller_viewer.h (right):

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/dom_dist...
ios/chrome/browser/dom_distiller/distiller_viewer.h:58: const
DistillerPageFactory* factory);
Update comment

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/favicon_web_state_dispatcher_impl.h
(right):

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/favicon_web_state_dispatcher_impl.h:42: }  //
namespace dom_distiller
Update namespace

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/favicon_web_state_dispatcher_impl.mm
(right):

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/favicon_web_state_dispatcher_impl.mm:78: }  //
namespace dom_distiller
Update namespace

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
File
ios/chrome/browser/reading_list/favicon_web_state_dispatcher_impl_unittest.mm
(right):

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/favicon_web_state_dispatcher_impl_unittest.mm:16:
namespace dom_distiller {
Update namespace

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/favicon_web_state_dispatcher_impl_unittest.mm:54:
web::WebState* web_state = dispatcher.RequestWebState();
The test will fail as it is using raw pointers.

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/favicon_web_state_dispatcher_impl_unittest.mm:76:
}  // namespace dom_distiller
Update namespace

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/reading_list_distiller_page.h (right):

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.h:1: // Copyright
2014 The Chromium Authors. All rights reserved.
Update year?

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.h:41: }  //
namespace dom_distiller
Update namespace

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/reading_list_distiller_page.mm (right):

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:1: // Copyright
2014 The Chromium Authors. All rights reserved.
Update year?

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:52: FROM_HERE,
base::Bind(&ReadingListDistillerPage::DelayedOnLoadURLDone,
Is this really necessary? ReadingListDistillerPage::DelayedOnLoadURLDone is just
forwarding call to DistillerPageIOS::OnLoadURLDone

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:61: }
// namespace reading_list

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/reading_list_distiller_page_factory.mm
(right):

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page_factory.mm:34: }  //
namespace dom_distiller
Update namespace

[Olivier, 2016-12-27 10:08:28]

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

[Olivier, 2016-12-27 10:08:37]

Thanks

https://codereview.chromium.org/2604773002/diff/1/components/dom_distiller/io...
File components/dom_distiller/ios/distiller_page_ios.h (right):

https://codereview.chromium.org/2604773002/diff/1/components/dom_distiller/io...
components/dom_distiller/ios/distiller_page_ios.h:45: void DistillPage();
On 2016/12/27 09:35:58, gambard wrote:
> This function name is already used to start the distillation. It might create
> some confusion.
> Maybe you can change it to something like InjectScript?

This method is replaced by the inheritance. Removed.

https://codereview.chromium.org/2604773002/diff/1/components/dom_distiller/io...
File components/dom_distiller/ios/distiller_page_ios.mm (left):

https://codereview.chromium.org/2604773002/diff/1/components/dom_distiller/io...
components/dom_distiller/ios/distiller_page_ios.mm:168:
favicon_driver->FetchFavicon(url_);
On 2016/12/27 09:35:58, gambard wrote:
> I don't see this code in the new version.

Done.

https://codereview.chromium.org/2604773002/diff/1/components/dom_distiller/io...
File components/dom_distiller/ios/distiller_page_ios.mm (right):

https://codereview.chromium.org/2604773002/diff/1/components/dom_distiller/io...
components/dom_distiller/ios/distiller_page_ios.mm:153:
std::unique_ptr<web::WebState> DistillerPageIOS::ReplaceWebState(
On 2016/12/27 09:35:58, gambard wrote:
> I am not fond of this logic of switching web states around.
> Instead, can't you use a lazy getter?
> The subclass would only need to destroy the WebState and override the getter.
> WDYT?

We need to update the observer. I am not sure how to do it with a simple getter.

https://codereview.chromium.org/2604773002/diff/1/components/dom_distiller/io...
components/dom_distiller/ios/distiller_page_ios.mm:198:
base::TimeDelta::FromSeconds(2));
On 2016/12/27 09:35:58, gambard wrote:
> Use a constant for the "2".

Code removed.

https://codereview.chromium.org/2604773002/diff/1/components/reading_list/ios...
File components/reading_list/ios/favicon_web_state_dispatcher.h (right):

https://codereview.chromium.org/2604773002/diff/1/components/reading_list/ios...
components/reading_list/ios/favicon_web_state_dispatcher.h:16: // their creation
and their return. After a WebState is returned, the dispatcher
On 2016/12/27 09:35:58, gambard wrote:
> Update the comment: the webstate is not kept alive between the two calls
> anymore.

Done.

https://codereview.chromium.org/2604773002/diff/1/components/reading_list/ios...
components/reading_list/ios/favicon_web_state_dispatcher.h:32: }  // namespace
dom_distiller
On 2016/12/27 09:35:58, gambard wrote:
> Update namespace

Done.

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/dom_dist...
File ios/chrome/browser/dom_distiller/distiller_viewer.h (right):

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/dom_dist...
ios/chrome/browser/dom_distiller/distiller_viewer.h:58: const
DistillerPageFactory* factory);
On 2016/12/27 09:35:58, gambard wrote:
> Update comment

there is no comment :s

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/favicon_web_state_dispatcher_impl.h
(right):

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/favicon_web_state_dispatcher_impl.h:42: }  //
namespace dom_distiller
On 2016/12/27 09:35:58, gambard wrote:
> Update namespace

Done.

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/favicon_web_state_dispatcher_impl.mm
(right):

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/favicon_web_state_dispatcher_impl.mm:78: }  //
namespace dom_distiller
On 2016/12/27 09:35:58, gambard wrote:
> Update namespace

Done.

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
File
ios/chrome/browser/reading_list/favicon_web_state_dispatcher_impl_unittest.mm
(right):

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/favicon_web_state_dispatcher_impl_unittest.mm:16:
namespace dom_distiller {
On 2016/12/27 09:35:58, gambard wrote:
> Update namespace

Done.

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/favicon_web_state_dispatcher_impl_unittest.mm:54:
web::WebState* web_state = dispatcher.RequestWebState();
On 2016/12/27 09:35:58, gambard wrote:
> The test will fail as it is using raw pointers.

Done.

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/favicon_web_state_dispatcher_impl_unittest.mm:76:
}  // namespace dom_distiller
On 2016/12/27 09:35:58, gambard wrote:
> Update namespace

Done.

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/reading_list_distiller_page.h (right):

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.h:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2016/12/27 09:35:59, gambard wrote:
> Update year?

Done.

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.h:41: }  //
namespace dom_distiller
On 2016/12/27 09:35:58, gambard wrote:
> Update namespace

Done.

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/reading_list_distiller_page.mm (right):

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2016/12/27 09:35:59, gambard wrote:
> Update year?

Done.

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:52: FROM_HERE,
base::Bind(&ReadingListDistillerPage::DelayedOnLoadURLDone,
On 2016/12/27 09:35:59, gambard wrote:
> Is this really necessary? ReadingListDistillerPage::DelayedOnLoadURLDone is
just
> forwarding call to DistillerPageIOS::OnLoadURLDone

You cannot bind directly DistillerPageIOS::OnLoadURLDone as it is protected and
base::bind creates an helper class.

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:61: }
On 2016/12/27 09:35:59, gambard wrote:
> // namespace reading_list

Done.

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/reading_list_distiller_page_factory.mm
(right):

https://codereview.chromium.org/2604773002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page_factory.mm:34: }  //
namespace dom_distiller
On 2016/12/27 09:35:59, gambard wrote:
> Update namespace

Done.

[commit-bot: I haz the power, 2016-12-27 10:08:40]

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

[gambard, 2016-12-27 10:53:30]

https://codereview.chromium.org/2604773002/diff/20001/components/dom_distille...
File components/dom_distiller/ios/distiller_page_ios.mm (left):

https://codereview.chromium.org/2604773002/diff/20001/components/dom_distille...
components/dom_distiller/ios/distiller_page_ios.mm:204: web_state_ = nullptr;
reset WebStateObserver.

[commit-bot: I haz the power, 2016-12-27 11:23:22]

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

[commit-bot: I haz the power, 2016-12-27 11:23:23]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Olivier, 2016-12-27 12:35:39]

Switched to Attach/Detach.
PTAL.

[gambard, 2016-12-27 13:56:57]

lgtm with comments

https://codereview.chromium.org/2604773002/diff/40001/components/dom_distille...
File components/dom_distiller/ios/distiller_page_factory_ios.mm (right):

https://codereview.chromium.org/2604773002/diff/40001/components/dom_distille...
components/dom_distiller/ios/distiller_page_factory_ios.mm:10: #include
"ios/web/public/web_state/web_state.h"
Not needed.

https://codereview.chromium.org/2604773002/diff/40001/components/dom_distille...
File components/dom_distiller/ios/distiller_page_ios.h (right):

https://codereview.chromium.org/2604773002/diff/40001/components/dom_distille...
components/dom_distiller/ios/distiller_page_ios.h:13: #include
"ios/web/public/web_state/web_state_observer.h"
Is this import needed?

https://codereview.chromium.org/2604773002/diff/40001/components/dom_distille...
components/dom_distiller/ios/distiller_page_ios.h:35: virtual void
AttachWebState(std::unique_ptr<web::WebState> web_state);
Comment

https://codereview.chromium.org/2604773002/diff/40001/components/dom_distille...
File components/dom_distiller/ios/distiller_page_ios.mm (left):

https://codereview.chromium.org/2604773002/diff/40001/components/dom_distille...
components/dom_distiller/ios/distiller_page_ios.mm:189: // Inject the script.
Why removing the comment?

https://codereview.chromium.org/2604773002/diff/40001/components/dom_distille...
File components/dom_distiller/ios/distiller_page_ios.mm (right):

https://codereview.chromium.org/2604773002/diff/40001/components/dom_distille...
components/dom_distiller/ios/distiller_page_ios.mm:18: #include
"base/threading/thread_task_runner_handle.h"
Not needed.

https://codereview.chromium.org/2604773002/diff/40001/components/dom_distille...
components/dom_distiller/ios/distiller_page_ios.mm:177:
AttachWebState(std::move(web_state_unique));
Nit: you can probably create it directly inside the function call.

https://codereview.chromium.org/2604773002/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_distiller_page.h (right):

https://codereview.chromium.org/2604773002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_distiller_page.h:13: #include
"components/reading_list/ios/favicon_web_state_dispatcher.h"
Can you use forward declaration?

https://codereview.chromium.org/2604773002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_distiller_page.h:14: #include
"ios/web/public/web_state/web_state_observer.h"
Not needed.

https://codereview.chromium.org/2604773002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_distiller_page.h:25: class
ReadingListDistillerPage : public dom_distiller::DistillerPageIOS {
Update comment?

https://codereview.chromium.org/2604773002/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_distiller_page.mm (right):

https://codereview.chromium.org/2604773002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:61:
base::TimeDelta::FromSeconds(2));
Nit: Use constant instead of "2"

https://codereview.chromium.org/2604773002/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_distiller_page_factory.h
(right):

https://codereview.chromium.org/2604773002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_distiller_page_factory.h:22: class
ReadingListDistillerPageFactory
Update comment

[Olivier, 2016-12-27 14:52:13]

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

[Olivier, 2016-12-27 14:52:15]

Thanks +noyau for owner.

https://codereview.chromium.org/2604773002/diff/40001/components/dom_distille...
File components/dom_distiller/ios/distiller_page_factory_ios.mm (right):

https://codereview.chromium.org/2604773002/diff/40001/components/dom_distille...
components/dom_distiller/ios/distiller_page_factory_ios.mm:10: #include
"ios/web/public/web_state/web_state.h"
On 2016/12/27 13:56:57, gambard wrote:
> Not needed.

Done.

https://codereview.chromium.org/2604773002/diff/40001/components/dom_distille...
File components/dom_distiller/ios/distiller_page_ios.h (right):

https://codereview.chromium.org/2604773002/diff/40001/components/dom_distille...
components/dom_distiller/ios/distiller_page_ios.h:13: #include
"ios/web/public/web_state/web_state_observer.h"
On 2016/12/27 13:56:57, gambard wrote:
> Is this import needed?

Yes. (PageComplectionStatus enum).

https://codereview.chromium.org/2604773002/diff/40001/components/dom_distille...
components/dom_distiller/ios/distiller_page_ios.h:35: virtual void
AttachWebState(std::unique_ptr<web::WebState> web_state);
On 2016/12/27 13:56:57, gambard wrote:
> Comment

Done.

https://codereview.chromium.org/2604773002/diff/40001/components/dom_distille...
File components/dom_distiller/ios/distiller_page_ios.mm (left):

https://codereview.chromium.org/2604773002/diff/40001/components/dom_distille...
components/dom_distiller/ios/distiller_page_ios.mm:189: // Inject the script.
On 2016/12/27 13:56:57, gambard wrote:
> Why removing the comment?

Done.

https://codereview.chromium.org/2604773002/diff/40001/components/dom_distille...
File components/dom_distiller/ios/distiller_page_ios.mm (right):

https://codereview.chromium.org/2604773002/diff/40001/components/dom_distille...
components/dom_distiller/ios/distiller_page_ios.mm:18: #include
"base/threading/thread_task_runner_handle.h"
On 2016/12/27 13:56:57, gambard wrote:
> Not needed.

Done.

https://codereview.chromium.org/2604773002/diff/40001/components/dom_distille...
components/dom_distiller/ios/distiller_page_ios.mm:177:
AttachWebState(std::move(web_state_unique));
On 2016/12/27 13:56:57, gambard wrote:
> Nit: you can probably create it directly inside the function call.

What do you mean?

https://codereview.chromium.org/2604773002/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_distiller_page.h (right):

https://codereview.chromium.org/2604773002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_distiller_page.h:13: #include
"components/reading_list/ios/favicon_web_state_dispatcher.h"
On 2016/12/27 13:56:57, gambard wrote:
> Can you use forward declaration?

Done.

https://codereview.chromium.org/2604773002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_distiller_page.h:14: #include
"ios/web/public/web_state/web_state_observer.h"
On 2016/12/27 13:56:57, gambard wrote:
> Not needed.

Done.

https://codereview.chromium.org/2604773002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_distiller_page.h:25: class
ReadingListDistillerPage : public dom_distiller::DistillerPageIOS {
On 2016/12/27 13:56:57, gambard wrote:
> Update comment?

Done.

https://codereview.chromium.org/2604773002/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_distiller_page.mm (right):

https://codereview.chromium.org/2604773002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:61:
base::TimeDelta::FromSeconds(2));
On 2016/12/27 13:56:57, gambard wrote:
> Nit: Use constant instead of "2"

Done.

https://codereview.chromium.org/2604773002/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_distiller_page_factory.h
(right):

https://codereview.chromium.org/2604773002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_distiller_page_factory.h:22: class
ReadingListDistillerPageFactory
On 2016/12/27 13:56:57, gambard wrote:
> Update comment

Done.

[noyau (Ping after 24h), 2017-01-02 09:52:49]

lgtm

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

https://codereview.chromium.org/2604773002/diff/60001/ios/chrome/browser/dom_...
ios/chrome/browser/dom_distiller/distiller_viewer.h:58: const
DistillerPageFactory* factory);
This does some magic when nil, can you add a comment to cover it? Or better
always require a factory and DCHECK on nullptr...

https://codereview.chromium.org/2604773002/diff/60001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_distiller_page.mm (right):

https://codereview.chromium.org/2604773002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:15: const int64_t
kDistillationDelayInSeconds = 2;
Can this get a signal from ios/web/ instead of relying on 2 seconds? This is
fine for now but I'd rather have some notification from web that the page is
stable if possible. It could be used for other things (screenshots for example).
Can you create a bug to carry this discussion?

[Olivier, 2017-01-02 11:40:04]

Done.
Thanks

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

https://codereview.chromium.org/2604773002/diff/60001/ios/chrome/browser/dom_...
ios/chrome/browser/dom_distiller/distiller_viewer.h:58: const
DistillerPageFactory* factory);
On 2017/01/02 09:52:49, noyau (OOO until Jan 2) wrote:
> This does some magic when nil, can you add a comment to cover it? Or better
> always require a factory and DCHECK on nullptr...

Done.
I prefer to let the possibility to pass nullptr as the other platforms allow it.

https://codereview.chromium.org/2604773002/diff/60001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_distiller_page.mm (right):

https://codereview.chromium.org/2604773002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:15: const int64_t
kDistillationDelayInSeconds = 2;
On 2017/01/02 09:52:49, noyau (OOO until Jan 2) wrote:
> Can this get a signal from ios/web/ instead of relying on 2 seconds? This is
> fine for now but I'd rather have some notification from web that the page is
> stable if possible. It could be used for other things (screenshots for
example).
> Can you create a bug to carry this discussion?

I can create a bug, but I think that this is hard to do as some pages (the one
that
needs this timeout) are completely built by Javascript after the loading is
done.
ios/web handles the loading, not the rendering, so it may not be aware that
further
rendering is needed.

[Olivier, 2017-01-02 11:40:11]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2017-01-02 11:40:11]

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

[commit-bot: I haz the power, 2017-01-02 11:40:21]

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-02 12:32:39]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483357211421630,
"parent_rev": "49c9211b9c2523e8d24e1e2e16fbee82c829453b", "commit_rev":
"94542489c49840436064fa0e791fb2bd1c9b405e"}

[commit-bot: I haz the power, 2017-01-02 12:33:06]

Description was changed from

==========
Create distiller files for Reading List.

DomDistillerService is used for ReadingList and ReaderMode.
The requirements for both feature are different.
As DomDistillerService main purpose (cross-platform) is Reader Mode,
this CL creates files for the Reading List distillation.
These files ensure that:
- WebState used for distillation survives at least 10 seconds after the
  end of distillation to allow loading favicon
- There is a 2 seconds delay between end of page load and start of
  distillation.
- Reading List files are in Reading List components.

These 2 requirements are irrelevant for Reader Mode as the page is
already loaded and the favicon is not used.
This CL also restore the behavior of Reader Mode before cl/2529283002.

BUG=648923
==========

to

==========
Create distiller files for Reading List.

DomDistillerService is used for ReadingList and ReaderMode.
The requirements for both feature are different.
As DomDistillerService main purpose (cross-platform) is Reader Mode,
this CL creates files for the Reading List distillation.
These files ensure that:
- WebState used for distillation survives at least 10 seconds after the
  end of distillation to allow loading favicon
- There is a 2 seconds delay between end of page load and start of
  distillation.
- Reading List files are in Reading List components.

These 2 requirements are irrelevant for Reader Mode as the page is
already loaded and the favicon is not used.
This CL also restore the behavior of Reader Mode before cl/2529283002.

BUG=648923

Review-Url: https://codereview.chromium.org/2604773002
==========

[commit-bot: I haz the power, 2017-01-02 12:33:07]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2017-01-02 15:55:21]

Description was changed from

==========
Create distiller files for Reading List.

DomDistillerService is used for ReadingList and ReaderMode.
The requirements for both feature are different.
As DomDistillerService main purpose (cross-platform) is Reader Mode,
this CL creates files for the Reading List distillation.
These files ensure that:
- WebState used for distillation survives at least 10 seconds after the
  end of distillation to allow loading favicon
- There is a 2 seconds delay between end of page load and start of
  distillation.
- Reading List files are in Reading List components.

These 2 requirements are irrelevant for Reader Mode as the page is
already loaded and the favicon is not used.
This CL also restore the behavior of Reader Mode before cl/2529283002.

BUG=648923

Review-Url: https://codereview.chromium.org/2604773002
==========

to

==========
Create distiller files for Reading List.

DomDistillerService is used for ReadingList and ReaderMode.
The requirements for both feature are different.
As DomDistillerService main purpose (cross-platform) is Reader Mode,
this CL creates files for the Reading List distillation.
These files ensure that:
- WebState used for distillation survives at least 10 seconds after the
  end of distillation to allow loading favicon
- There is a 2 seconds delay between end of page load and start of
  distillation.
- Reading List files are in Reading List components.

These 2 requirements are irrelevant for Reader Mode as the page is
already loaded and the favicon is not used.
This CL also restore the behavior of Reader Mode before cl/2529283002.

BUG=648923

Committed: https://crrev.com/31daab21ab182a3272daec45c77626c1628bd4d3
Cr-Commit-Position: refs/heads/master@{#441060}
==========

[commit-bot: I haz the power, 2017-01-02 15:55:22]

Patchset 5 (id:??) landed as
https://crrev.com/31daab21ab182a3272daec45c77626c1628bd4d3
Cr-Commit-Position: refs/heads/master@{#441060}