Remove DownloadRequest registry from FaviconHandler

Remove DownloadRequest registry from FaviconHandler

No behavioral changes: the adoption of a CancelableCallback is a more
natural way to represent a single ongoing request, which needs to be
cancelled in certain cases.

This is possible after https://codereview.chromium.org/2691933004
which reduced the number of received download completions, now
restricted to the ones that the FaviconHandler instance itself
triggered.

BUG=699980
Review-Url: https://codereview.chromium.org/2738063004
Cr-Commit-Position: refs/heads/master@{#458360}
Committed: https://chromium.googlesource.com/chromium/src/+/893e6a902a3ebe3bcee042e6e2bec1413e977a5f

[mastiz, 2017-03-09 13:41:29]

mastiz@chromium.org changed reviewers:
+ pkotwicz@chromium.org

[pkotwicz, 2017-03-10 03:42:29]

Just nits! Can you modify the CL description to indicate why the
|download_requests_| map is no longer needed.
FaviconHandler::OnDidDownloadFavicon() used to get called for downloads that it
did not request.

The reason is that FaviconHandler used to handle IPCs directly. I think that
this is the CL which changed that

https://codereview.chromium.org/2738063004/diff/20001/components/favicon/core...
File components/favicon/core/favicon_handler.cc (right):

https://codereview.chromium.org/2738063004/diff/20001/components/favicon/core...
components/favicon/core/favicon_handler.cc:428: // Mark ongoing download as
finalized.
Nit: How about: "Mark download as finished."

https://codereview.chromium.org/2738063004/diff/20001/components/favicon/core...
components/favicon/core/favicon_handler.cc:438:
DCHECK(DoUrlAndIconMatch(*current_candidate(), image_url, icon_type));
Nit: Remove the DCHECKs(). I generally don't think that DCHECKs are very useful.

[pkotwicz, 2017-03-10 03:42:45]

LGTM with above nits

[mastiz, 2017-03-10 11:17:06]

Description was changed from

==========
Remove DownloadRequest registry from FaviconHandler

No behavioral changes: the adoption of a CancelableCallback is a more
natural way to represent a single ongoing request, which needs to be
cancelled in certain cases.

BUG=699980
==========

to

==========
Remove DownloadRequest registry from FaviconHandler

No behavioral changes: the adoption of a CancelableCallback is a more
natural way to represent a single ongoing request, which needs to be
cancelled in certain cases.

This is possible after https://codereview.chromium.org/2691933004
which reduced the number of received download completions, now
restricted to the ones that the FaviconHandler instance itself
triggered.

BUG=699980
==========

[mastiz, 2017-03-10 11:34:50]

Update CL description as suggested, thanks for the review!

(unfortunately, the patch cannot land right now because current tests cause two
ongoing downloads, which I believe is not possible in production)

https://codereview.chromium.org/2738063004/diff/20001/components/favicon/core...
File components/favicon/core/favicon_handler.cc (right):

https://codereview.chromium.org/2738063004/diff/20001/components/favicon/core...
components/favicon/core/favicon_handler.cc:428: // Mark ongoing download as
finalized.
On 2017/03/10 03:42:29, pkotwicz wrote:
> Nit: How about: "Mark download as finished."

Done.

https://codereview.chromium.org/2738063004/diff/20001/components/favicon/core...
components/favicon/core/favicon_handler.cc:438:
DCHECK(DoUrlAndIconMatch(*current_candidate(), image_url, icon_type));
On 2017/03/10 03:42:29, pkotwicz wrote:
> Nit: Remove the DCHECKs(). I generally don't think that DCHECKs are very
useful.

Done.

[pkotwicz, 2017-03-10 15:59:48]

I applied Patch Set #3 locally and all of the FaviconHandlerTests pass including
FaviconHandlerTest.UpdateDuringDownloading

I am fine with waiting for your other CL land though

[mastiz, 2017-03-13 08:59:16]

Patchset #4 (id:60001) has been deleted

[mastiz, 2017-03-13 09:00:20]

On 2017/03/10 15:59:48, pkotwicz wrote:
> I applied Patch Set #3 locally and all of the FaviconHandlerTests pass
including
> FaviconHandlerTest.UpdateDuringDownloading
> 
> I am fine with waiting for your other CL land though

Thanks! Perhaps I tried the tests before removing the DCHECK, going ahead with
the CQ.

[mastiz, 2017-03-13 09:00:24]

The CQ bit was checked by mastiz@chromium.org

[mastiz, 2017-03-13 09:00:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkotwicz@chromium.org
Link to the patchset: https://codereview.chromium.org/2738063004/#ps40001
(title: "Rebased on head.")

[commit-bot: I haz the power, 2017-03-13 09:00:33]

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-03-13 09:19:21]

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

[commit-bot: I haz the power, 2017-03-13 09:19:22]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mastiz, 2017-03-13 12:50:58]

On 2017/03/13 09:00:20, mastiz wrote:
> On 2017/03/10 15:59:48, pkotwicz wrote:
> > I applied Patch Set #3 locally and all of the FaviconHandlerTests pass
> including
> > FaviconHandlerTest.UpdateDuringDownloading
> > 
> > I am fine with waiting for your other CL land though
> 
> Thanks! Perhaps I tried the tests before removing the DCHECK, going ahead with
> the CQ.

Nope, tests are indeed failing due to multiple parallel downloads. Rebased again
on top of https://codereview.chromium.org/2703603002/.

[mastiz, 2017-03-21 09:05:55]

The CQ bit was checked by mastiz@chromium.org

[mastiz, 2017-03-21 09:05:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkotwicz@chromium.org
Link to the patchset: https://codereview.chromium.org/2738063004/#ps100001
(title: "Rebased.")

[commit-bot: I haz the power, 2017-03-21 09:06:15]

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-03-21 09:48:39]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490087155775950,
"parent_rev": "5d0381faf29ba02bec52363bb62c0237cfd4a314", "commit_rev":
"893e6a902a3ebe3bcee042e6e2bec1413e977a5f"}

[commit-bot: I haz the power, 2017-03-21 09:49:24]

Description was changed from

==========
Remove DownloadRequest registry from FaviconHandler

No behavioral changes: the adoption of a CancelableCallback is a more
natural way to represent a single ongoing request, which needs to be
cancelled in certain cases.

This is possible after https://codereview.chromium.org/2691933004
which reduced the number of received download completions, now
restricted to the ones that the FaviconHandler instance itself
triggered.

BUG=699980
==========

to

==========
Remove DownloadRequest registry from FaviconHandler

No behavioral changes: the adoption of a CancelableCallback is a more
natural way to represent a single ongoing request, which needs to be
cancelled in certain cases.

This is possible after https://codereview.chromium.org/2691933004
which reduced the number of received download completions, now
restricted to the ones that the FaviconHandler instance itself
triggered.

BUG=699980

Review-Url: https://codereview.chromium.org/2738063004
Cr-Commit-Position: refs/heads/master@{#458360}
Committed:
https://chromium.googlesource.com/chromium/src/+/893e6a902a3ebe3bcee042e6e2be...
==========

[commit-bot: I haz the power, 2017-03-21 09:49:25]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/893e6a902a3ebe3bcee042e6e2be...