[Refactor] Simplify HistoryBackend::UpdateFaviconMappingsAndFetchImpl() signature

[Refactor] Simplify HistoryBackend::UpdateFaviconMappingsAndFetchImpl() signature

This CL makes HistoryBackend::UpdateFaviconMappingsAndFetchImpl() take one icon
type and one icon URL. All of its callers only pass in a single type and a
single icon URL

This CL makes HistoryService::GetFavicon() take a one icon type and one icon
URL as well.

BUG=712466
Review-Url: https://codereview.chromium.org/2828173002
Cr-Commit-Position: refs/heads/master@{#468874}
Committed: https://chromium.googlesource.com/chromium/src/+/2cdb043cb1911124da7f20aa869aa8423c7a6714

[pkotwicz, 2017-04-20 03:15:51]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2017-04-20 03:16:45]

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

[pkotwicz, 2017-04-20 03:16:46]

mastiz@ can you please take a look?

[mastiz, 2017-04-20 09:11:35]

Really nice, LGTM!

Please briefly mention the signature change for GetFavicon[s] too in the CL
description.

https://codereview.chromium.org/2828173002/diff/20001/components/favicon/core...
File components/favicon/core/favicon_service.h (right):

https://codereview.chromium.org/2828173002/diff/20001/components/favicon/core...
components/favicon/core/favicon_service.h:123: // edge sizes most closely match
|desired_sizes| from the favicons which were
s/desired_sizes/desired_size_in_dip/

https://codereview.chromium.org/2828173002/diff/20001/components/favicon/core...
components/favicon/core/favicon_service.h:124: // just mapped to |page_url| are
returned. If |desired_sizes| has a '0' entry,
Ditto.

[pkotwicz, 2017-04-29 23:21:23]

Description was changed from

==========
[Refactor] Simplify HistoryBackend::UpdateFaviconMappingsAndFetchImpl()
signature

This CL makes HistoryBackend::UpdateFaviconMappingsAndFetchImpl() take one icon
type and one icon URL. All of its callers only pass in a single type and a
single icon URL

BUG=712466
==========

to

==========
[Refactor] Simplify HistoryBackend::UpdateFaviconMappingsAndFetchImpl()
signature

This CL makes HistoryBackend::UpdateFaviconMappingsAndFetchImpl() take one icon
type and one icon URL. All of its callers only pass in a single type and a
single icon URL

This CL makes HistoryService::GetFavicon() take a one icon type and one icon
URL as well.

BUG=712466
==========

[pkotwicz, 2017-04-29 23:26:54]

Scott, can you please take a look? The purpose of this CL is to enable
https://codereview.chromium.org/2823093002/

I chose to do the refactoring in the way that I did it because it is the fastest
way to landing https://codereview.chromium.org/2823093002/

[pkotwicz, 2017-04-29 23:27:39]

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

[pkotwicz, 2017-04-29 23:27:46]

Scott, can you please take a look? The purpose of this CL is to enable
https://codereview.chromium.org/2823093002/

I chose to do the refactoring in the way that I did it because it is the fastest
way to landing https://codereview.chromium.org/2823093002/

[sky, 2017-05-01 21:34:30]

LGTM

https://codereview.chromium.org/2828173002/diff/40001/components/history/core...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/2828173002/diff/40001/components/history/core...
components/history/core/browser/history_backend.cc:1895: *page_url, icon_type,
favicon_ids);
Do we ever use more than one icon in this function?

[pkotwicz, 2017-05-02 21:49:14]

https://codereview.chromium.org/2828173002/diff/40001/components/history/core...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/2828173002/diff/40001/components/history/core...
components/history/core/browser/history_backend.cc:1895: *page_url, icon_type,
favicon_ids);
We don't ever pass more than one icon to the function

[sky, 2017-05-02 21:59:50]

Feel free to land this and cleanup in a separate patch.

  -Scott

On Tue, May 2, 2017 at 2:49 PM, <pkotwicz@chromium.org> wrote:

>
> https://codereview.chromium.org/2828173002/diff/40001/
> components/history/core/browser/history_backend.cc
> File components/history/core/browser/history_backend.cc (right):
>
> https://codereview.chromium.org/2828173002/diff/40001/
> components/history/core/browser/history_backend.cc#newcode1895
> components/history/core/browser/history_backend.cc:1895: *page_url,
> icon_type, favicon_ids);
> We don't ever pass more than one icon to the function
>
> https://codereview.chromium.org/2828173002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[pkotwicz, 2017-05-03 01:21:03]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2017-05-03 01:21:03]

The patchset sent to the CQ was uploaded after l-g-t-m from mastiz@chromium.org
Link to the patchset: https://codereview.chromium.org/2828173002/#ps40001
(title: "Merge branch 'master' into icon_type0")

[commit-bot: I haz the power, 2017-05-03 01:21:47]

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-05-03 02:24:43]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493774463244180,
"parent_rev": "90fd23c70a8d62bf83332ea62449484cdedee5af", "commit_rev":
"2cdb043cb1911124da7f20aa869aa8423c7a6714"}

[commit-bot: I haz the power, 2017-05-03 02:24:56]

Description was changed from

==========
[Refactor] Simplify HistoryBackend::UpdateFaviconMappingsAndFetchImpl()
signature

This CL makes HistoryBackend::UpdateFaviconMappingsAndFetchImpl() take one icon
type and one icon URL. All of its callers only pass in a single type and a
single icon URL

This CL makes HistoryService::GetFavicon() take a one icon type and one icon
URL as well.

BUG=712466
==========

to

==========
[Refactor] Simplify HistoryBackend::UpdateFaviconMappingsAndFetchImpl()
signature

This CL makes HistoryBackend::UpdateFaviconMappingsAndFetchImpl() take one icon
type and one icon URL. All of its callers only pass in a single type and a
single icon URL

This CL makes HistoryService::GetFavicon() take a one icon type and one icon
URL as well.

BUG=712466

Review-Url: https://codereview.chromium.org/2828173002
Cr-Commit-Position: refs/heads/master@{#468874}
Committed:
https://chromium.googlesource.com/chromium/src/+/2cdb043cb1911124da7f20aa869a...
==========

[commit-bot: I haz the power, 2017-05-03 02:24:57]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/2cdb043cb1911124da7f20aa869a...