Store distilled URL during distillation in Reading List on iOS

Store distilled URL during distillation in Reading List on iOS

The URL of the distilled page is not necessarily the URL of the Reading
List entry (incase of redirection).
If the distillation is done on a redirected page, store it along the
distilled_path.

I also filed crbug.com/682705 to have the "redirected_url" directly in the proto file
and avoid having to take it via callback.

BUG=682666
Review-Url: https://codereview.chromium.org/2646993002
Cr-Commit-Position: refs/heads/master@{#445071}
Committed: https://chromium.googlesource.com/chromium/src/+/69e7e3b844f1cedf53701f07bf0d3219fc2d3176

[Olivier, 2017-01-20 09:04:42]

olivierrobin@chromium.org changed reviewers:
+ gambard@chromium.org, jif@chromium.org, noyau@chromium.org

[Olivier, 2017-01-20 09:04:42]

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

https://codereview.chromium.org/2646993002/diff/1/components/dom_distiller/io...
components/dom_distiller/ios/distiller_page_ios.h:9: #include <objc/objc.h>
This file use the "id" type, so it needs this header.

[jif, 2017-01-20 10:23:19]

quick first pass

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

https://codereview.chromium.org/2646993002/diff/1/components/dom_distiller/io...
components/dom_distiller/ios/distiller_page_ios.h:9: #include <objc/objc.h>
On 2017/01/20 09:04:42, Olivier Robin wrote:
> This file use the "id" type, so it needs this header.

#import?

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

https://codereview.chromium.org/2646993002/diff/1/ios/chrome/browser/dom_dist...
ios/chrome/browser/dom_distiller/distiller_viewer.h:56: // If |page| is null,
the default factory of |distillerService| will be used.
I think that forcing the caller of the constructor to construct the page is a
better API.

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

https://codereview.chromium.org/2646993002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.h:35: void
SetRedirectionCallback(RedirectionCallback redirection_callback);
pass the RedirectionCallback by reference

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

https://codereview.chromium.org/2646993002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:130: if
(redirected_url_ != original_url_ && redirection_callback_) {
testing |redirection_callback_| does not result in a compilation error??

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

https://codereview.chromium.org/2646993002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader.h:135: // A callback called if
the URL passed to the distilled leaded to a
needs verification:
s/leaded/led/

[Olivier, 2017-01-20 10:47:07]

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

https://codereview.chromium.org/2646993002/diff/1/components/dom_distiller/io...
components/dom_distiller/ios/distiller_page_ios.h:9: #include <objc/objc.h>
On 2017/01/20 10:23:19, jif wrote:
> On 2017/01/20 09:04:42, Olivier Robin wrote:
> > This file use the "id" type, so it needs this header.
> 
> #import?

objc/objc.h is a C++ header.

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

https://codereview.chromium.org/2646993002/diff/1/ios/chrome/browser/dom_dist...
ios/chrome/browser/dom_distiller/distiller_viewer.h:56: // If |page| is null,
the default factory of |distillerService| will be used.
On 2017/01/20 10:23:19, jif wrote:
> I think that forcing the caller of the constructor to construct the page is a
> better API.

The pattern with a default factory is used on other platform.
I agree with you, but I think we should keep the factory for consistency.

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

https://codereview.chromium.org/2646993002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.h:35: void
SetRedirectionCallback(RedirectionCallback redirection_callback);
On 2017/01/20 10:23:19, jif wrote:
> pass the RedirectionCallback by reference

https://cs.chromium.org/chromium/src/docs/callback.md?q=callback.md&sq=packag...
Apparently, we must not use const reference.

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

https://codereview.chromium.org/2646993002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_distiller_page.mm:130: if
(redirected_url_ != original_url_ && redirection_callback_) {
On 2017/01/20 10:23:19, jif wrote:
> testing |redirection_callback_| does not result in a compilation error??

It compiles and works.
But the callback provides an is_null method that i may use.

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

https://codereview.chromium.org/2646993002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader.h:135: // A callback called if
the URL passed to the distilled leaded to a
On 2017/01/20 10:23:19, jif wrote:
> needs verification:
> s/leaded/led/

Done.

[gambard, 2017-01-20 14:15:14]

lgtm with some suggestions.

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

https://codereview.chromium.org/2646993002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_distiller_page.h:34: typedef
base::Callback<void(const GURL&, const GURL&)> RedirectionCallback;
Do we want to put the typedef here or at the beginning of the classes?

https://codereview.chromium.org/2646993002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/url_downloader.cc (right):

https://codereview.chromium.org/2646993002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/url_downloader.cc:177: distilled_url_ =
redirected_url;
You don't use the page_url? Maybe to check if this is still the current page?

https://codereview.chromium.org/2646993002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/url_downloader_unittest.mm (right):

https://codereview.chromium.org/2646993002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/url_downloader_unittest.mm:125:
TEST_F(URLDownloaderTest, SingleDownload) {
Maybe you can add to this test the redirection url?

[Olivier, 2017-01-20 15:31:16]

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

https://codereview.chromium.org/2646993002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_distiller_page.h:34: typedef
base::Callback<void(const GURL&, const GURL&)> RedirectionCallback;
On 2017/01/20 14:15:14, gambard wrote:
> Do we want to put the typedef here or at the beginning of the classes?

I like to put it where is is used, but it may be better at the begining of the
class (like in other classes).
Removed the setter and put in constructor

https://codereview.chromium.org/2646993002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/url_downloader.cc (right):

https://codereview.chromium.org/2646993002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/url_downloader.cc:177: distilled_url_ =
redirected_url;
On 2017/01/20 14:15:14, gambard wrote:
> You don't use the page_url? Maybe to check if this is still the current page?

Done.

https://codereview.chromium.org/2646993002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/url_downloader_unittest.mm (right):

https://codereview.chromium.org/2646993002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/url_downloader_unittest.mm:125:
TEST_F(URLDownloaderTest, SingleDownload) {
On 2017/01/20 14:15:14, gambard wrote:
> Maybe you can add to this test the redirection url?

Not easy because all the calls here are synchronous and the call to the
distillation callback has to be done  during the task.
I will study how to do it.

[noyau (Ping after 24h), 2017-01-20 15:52:17]

lgtm

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

https://codereview.chromium.org/2646993002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/url_downloader.h:60: const std::string&)>;
Please update comment for the new argument

[Olivier, 2017-01-20 16:17:34]

Thanks

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

https://codereview.chromium.org/2646993002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/url_downloader.h:60: const std::string&)>;
On 2017/01/20 15:52:17, noyau wrote:
> Please update comment for the new argument

Done.

[Olivier, 2017-01-20 16:17:39]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2017-01-20 16:17:39]

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/2646993002/#ps60001
(title: "feedback")

[commit-bot: I haz the power, 2017-01-20 16:17:56]

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-20 16:29:55]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484929059042200,
"parent_rev": "6432d4b76477d8587270c0bc75689ae0f13aa989", "commit_rev":
"69e7e3b844f1cedf53701f07bf0d3219fc2d3176"}

[commit-bot: I haz the power, 2017-01-20 16:30:24]

Description was changed from

==========
Store distilled URL during distillation in Reading List on iOS

The URL of the distilled page is not necessarily the URL of the Reading
List entry (incase of redirection).
If the distillation is done on a redirected page, store it along the
distilled_path.

I also filed crbug.com/682705 to have the "redirected_url" directly in the proto
file
and avoid having to take it via callback.

BUG=682666
==========

to

==========
Store distilled URL during distillation in Reading List on iOS

The URL of the distilled page is not necessarily the URL of the Reading
List entry (incase of redirection).
If the distillation is done on a redirected page, store it along the
distilled_path.

I also filed crbug.com/682705 to have the "redirected_url" directly in the proto
file
and avoid having to take it via callback.

BUG=682666

Review-Url: https://codereview.chromium.org/2646993002
Cr-Commit-Position: refs/heads/master@{#445071}
Committed:
https://chromium.googlesource.com/chromium/src/+/69e7e3b844f1cedf53701f07bf0d...
==========

[commit-bot: I haz the power, 2017-01-20 16:30:25]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/69e7e3b844f1cedf53701f07bf0d...