Update reading list entry on download

Update reading list entry on download

Update reading list entry with title and downloaded path; also handle
errors. Left to possibly do is handling retry vs. permanent errors.

BUG=616747
Committed: https://crrev.com/65e74f6fd94834eac7bcb233f663383aa4e0bb5e
Cr-Commit-Position: refs/heads/master@{#420014}

[lody, 2016-09-09 09:28:00]

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

[noyau (Ping after 24h), 2016-09-09 09:39:58]

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

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/dom_dist...
ios/chrome/browser/dom_distiller/distiller_viewer.h:32: typedef
base::Callback<void(const GURL&,
Add url, to be consistent.

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

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader.cc:84: base::Unretained(this),
url, title, success);
I'm always worried when I see a base::Unretained. Is there something making sure
that the task is cancelled if this is deallocated?

[sdefresne, 2016-09-09 09:58:58]

drive by

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

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/dom_dist...
ios/chrome/browser/dom_distiller/distiller_viewer.cc:46:
images.push_back({GURL(image.url()), image.data()});
I think this should be:

images.push_back(ImageInfo(GURL(image.url()), image.data()));

or:

images.emplace_back(GURL(image.url()), image.data());

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/dom_dist...
ios/chrome/browser/dom_distiller/distiller_viewer.cc:58: callback_.Run(url_, "",
{}, "");
Please prefer std::string() to "".

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/reading_list_download_service.cc (right):

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_download_service.cc:105: const
GURL& distilledURL,
style: s/distilledURL/distilled_url/

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

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_download_service.h:68: const GURL&
distilledURL,
style: s/distilledURL/distilled_url/

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

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader.cc:74: auto postDelete =
base::Bind(
style: variable should name should use name_with_underscore in c++ (note that
there are many violation of that rule in the file, maybe they can also be fixed)

nit: pass the title per const-reference

auto delete_post = base::Bind(
  [](URLDownloader* downloader, const GURL& url,
     const std::string& title, SuccessState success) {

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader.cc:79: GURL("file://" +
_this->OfflineURLPagePath(url).value()), title);
nit: maybe use url::kFileScheme instead of "file"

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader.cc:84: base::Unretained(this),
url, title, success);
On 2016/09/09 09:39:58, noyau wrote:
> I'm always worried when I see a base::Unretained. Is there something making
sure
> that the task is cancelled if this is deallocated?

+1

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader.cc:91: [](base::FilePath
offlineDirectoryPath) {
style: variable should name should use name_with_underscore in c++
nit: pass the path per const-reference

   [](const base::FilePath& offline_directory_path) {

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader.cc:216: base::FilePath
localImagePath;
style: s/localImagePath/local_image_path/

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader.cc:218: return "";
Please prefer to use std::string() instead of "".

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

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader.h:81: std::string title,
Prefer to use "const std::string&" (unless you have a valid reason to copy the
value when calling this function).

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/url_downloader_unittest.cc (right):

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader_unittest.cc:76:
DownloadCompletionHandler(url, "", false);
s/""/std::string()/

[lody, 2016-09-09 12:17:54]

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

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/dom_dist...
ios/chrome/browser/dom_distiller/distiller_viewer.cc:58: callback_.Run(url_, "",
{}, "");
On 2016/09/09 09:58:57, sdefresne wrote:
> Please prefer std::string() to "".

Done.

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

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/dom_dist...
ios/chrome/browser/dom_distiller/distiller_viewer.h:32: typedef
base::Callback<void(const GURL&,
On 2016/09/09 09:39:58, noyau wrote:
> Add url, to be consistent.

Done.

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/reading_list_download_service.cc (right):

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_download_service.cc:105: const
GURL& distilledURL,
On 2016/09/09 09:58:57, sdefresne wrote:
> style: s/distilledURL/distilled_url/

Done.

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

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_download_service.h:68: const GURL&
distilledURL,
On 2016/09/09 09:58:57, sdefresne wrote:
> style: s/distilledURL/distilled_url/

Done.

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

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader.cc:74: auto postDelete =
base::Bind(
On 2016/09/09 09:58:57, sdefresne wrote:
> style: variable should name should use name_with_underscore in c++ (note that
> there are many violation of that rule in the file, maybe they can also be
fixed)
> 
> nit: pass the title per const-reference
> 
> auto delete_post = base::Bind(
>   [](URLDownloader* downloader, const GURL& url,
>      const std::string& title, SuccessState success) {

Done.

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader.cc:79: GURL("file://" +
_this->OfflineURLPagePath(url).value()), title);
On 2016/09/09 09:58:57, sdefresne wrote:
> nit: maybe use url::kFileScheme instead of "file"

Done.

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader.cc:84: base::Unretained(this),
url, title, success);
On 2016/09/09 09:58:57, sdefresne wrote:
> On 2016/09/09 09:39:58, noyau wrote:
> > I'm always worried when I see a base::Unretained. Is there something making
> sure
> > that the task is cancelled if this is deallocated?
> 
> +1

That's what the task_tracker_ is for. Already had this issue for all the other
async calls in this file.

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader.cc:91: [](base::FilePath
offlineDirectoryPath) {
On 2016/09/09 09:58:57, sdefresne wrote:
> style: variable should name should use name_with_underscore in c++
> nit: pass the path per const-reference
> 
>    [](const base::FilePath& offline_directory_path) {

Done.

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader.cc:216: base::FilePath
localImagePath;
On 2016/09/09 09:58:58, sdefresne wrote:
> style: s/localImagePath/local_image_path/

Done.

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader.cc:218: return "";
On 2016/09/09 09:58:58, sdefresne wrote:
> Please prefer to use std::string() instead of "".

Done.

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

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader.h:81: std::string title,
On 2016/09/09 09:58:58, sdefresne wrote:
> Prefer to use "const std::string&" (unless you have a valid reason to copy the
> value when calling this function).

Done.

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/url_downloader_unittest.cc (right):

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader_unittest.cc:76:
DownloadCompletionHandler(url, "", false);
On 2016/09/09 09:58:58, sdefresne wrote:
> s/""/std::string()/

Done.

[noyau (Ping after 24h), 2016-09-09 12:45:22]

lgtm

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

https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/url_downloader.cc:84: base::Unretained(this),
url, title, success);
On 2016/09/09 12:17:53, lody wrote:
> On 2016/09/09 09:58:57, sdefresne wrote:
> > On 2016/09/09 09:39:58, noyau wrote:
> > > I'm always worried when I see a base::Unretained. Is there something
making
> > sure
> > > that the task is cancelled if this is deallocated?
> > 
> > +1
> 
> That's what the task_tracker_ is for. Already had this issue for all the other
> async calls in this file.

Ah, yes, I remember now. Sorry, should have checked.

https://codereview.chromium.org/2320403002/diff/40001/ios/chrome/browser/dom_...
File ios/chrome/browser/dom_distiller/distiller_viewer.cc (right):

https://codereview.chromium.org/2320403002/diff/40001/ios/chrome/browser/dom_...
ios/chrome/browser/dom_distiller/distiller_viewer.cc:46:
images.push_back(ImageInfo{GURL(image.url()), image.data()});
imageInfo{ ? Should this be ImageInfo( instead? I didn't know this even compile,
must some newfangled C++ thing.

[lody, 2016-09-09 12:47:26]

On 2016/09/09 12:45:22, noyau wrote:
> lgtm
> 
>
https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
> File ios/chrome/browser/reading_list/url_downloader.cc (right):
> 
>
https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
> ios/chrome/browser/reading_list/url_downloader.cc:84: base::Unretained(this),
> url, title, success);
> On 2016/09/09 12:17:53, lody wrote:
> > On 2016/09/09 09:58:57, sdefresne wrote:
> > > On 2016/09/09 09:39:58, noyau wrote:
> > > > I'm always worried when I see a base::Unretained. Is there something
> making
> > > sure
> > > > that the task is cancelled if this is deallocated?
> > > 
> > > +1
> > 
> > That's what the task_tracker_ is for. Already had this issue for all the
other
> > async calls in this file.
> 
> Ah, yes, I remember now. Sorry, should have checked.
> 
>
https://codereview.chromium.org/2320403002/diff/40001/ios/chrome/browser/dom_...
> File ios/chrome/browser/dom_distiller/distiller_viewer.cc (right):
> 
>
https://codereview.chromium.org/2320403002/diff/40001/ios/chrome/browser/dom_...
> ios/chrome/browser/dom_distiller/distiller_viewer.cc:46:
> images.push_back(ImageInfo{GURL(image.url()), image.data()});
> imageInfo{ ? Should this be ImageInfo( instead? I didn't know this even
compile,
> must some newfangled C++ thing.

ImageInfo() doesn't compile. We checked with Sylvain. Some sort of not named
constructor thing I think?
Thanks for the lgtm (I can't submit before the other CL is also lgtm though ;) )

[sdefresne, 2016-09-09 13:01:10]

On 2016/09/09 12:45:22, noyau wrote:
> lgtm
> 
>
https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
> File ios/chrome/browser/reading_list/url_downloader.cc (right):
> 
>
https://codereview.chromium.org/2320403002/diff/1/ios/chrome/browser/reading_...
> ios/chrome/browser/reading_list/url_downloader.cc:84: base::Unretained(this),
> url, title, success);
> On 2016/09/09 12:17:53, lody wrote:
> > On 2016/09/09 09:58:57, sdefresne wrote:
> > > On 2016/09/09 09:39:58, noyau wrote:
> > > > I'm always worried when I see a base::Unretained. Is there something
> making
> > > sure
> > > > that the task is cancelled if this is deallocated?
> > > 
> > > +1
> > 
> > That's what the task_tracker_ is for. Already had this issue for all the
other
> > async calls in this file.
> 
> Ah, yes, I remember now. Sorry, should have checked.
> 
>
https://codereview.chromium.org/2320403002/diff/40001/ios/chrome/browser/dom_...
> File ios/chrome/browser/dom_distiller/distiller_viewer.cc (right):
> 
>
https://codereview.chromium.org/2320403002/diff/40001/ios/chrome/browser/dom_...
> ios/chrome/browser/dom_distiller/distiller_viewer.cc:46:
> images.push_back(ImageInfo{GURL(image.url()), image.data()});
> imageInfo{ ? Should this be ImageInfo( instead? I didn't know this even
compile,
> must some newfangled C++ thing.

This is uniform initialisation syntax. Added in C99 IIRC, and C++11 (see
do/don't do for when to use
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-...).
Here ImageInfo is a struct with no custom constructor, so ImageInfo() does not
work, the alternative option is to use a local variable:

    ImageInfo image_info = {GURL(image.url()), image.data() };
    images.push_back(image_info);

C++11 allow to omit the local variable. The old syntax
(images.push_back({GURL(image.url()), image.data())})) was doing the same thing
but less readable (as you need to check the images local variable definition to
know which type is constructed).

[gambard, 2016-09-15 12:35:56]

The CQ bit was checked by gambard@chromium.org

[commit-bot: I haz the power, 2016-09-15 12:36:07]

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

[commit-bot: I haz the power, 2016-09-15 12:36:08]

This CL has an open dependency (Issue 2327783002 Patch 1). Please resolve the
dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[gambard, 2016-09-15 12:36:23]

The CQ bit was unchecked by gambard@chromium.org

[gambard, 2016-09-15 13:14:11]

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

https://codereview.chromium.org/2320403002/diff/40001/ios/chrome/browser/dom_...
ios/chrome/browser/dom_distiller/distiller_viewer.h:35: const std::string&
title)>
This breaks reader_mode_controller.mm

[lody, 2016-09-21 08:45:32]

Patchset #4 (id:60001) has been deleted

[lody, 2016-09-21 08:58:20]

The CQ bit was checked by lod@chromium.org

[commit-bot: I haz the power, 2016-09-21 08:58:43]

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, 2016-09-21 09:13:09]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-21 09:15:09]

Description was changed from

==========
Update reading list entry on download

Update reading list entry with title and downloaded path; also handle
errors. Left to possibly do is handling retry vs. permanent errors.

BUG=616747
==========

to

==========
Update reading list entry on download

Update reading list entry with title and downloaded path; also handle
errors. Left to possibly do is handling retry vs. permanent errors.

BUG=616747

Committed: https://crrev.com/65e74f6fd94834eac7bcb233f663383aa4e0bb5e
Cr-Commit-Position: refs/heads/master@{#420014}
==========

[commit-bot: I haz the power, 2016-09-21 09:15:10]

Patchset 3 (id:??) landed as
https://crrev.com/65e74f6fd94834eac7bcb233f663383aa4e0bb5e
Cr-Commit-Position: refs/heads/master@{#420014}