Refactor ManifestIconSelector and update it for Manifest.icon.purpose

Refactor ManifestIconSelector and update it for Manifest.icon.purpose

This CL refactors ManifestIconSelector:
1. Removes the extra copy of icons introduced in FilterIconsByType;
2. Combines the iteration for finding the ideal icon and the iteration
for finding the icon closest to ideal icon into one iteration;
3. Changes out-dated name "preferred icon" to current "ideal icon";
4. Removes non-static class members and functions.

This CL also updates ManifestIconSelector to allow users to pass
desired icon purpose as a parameter to FindBestMatchingIcon.

BUG=649771
Review-Url: https://codereview.chromium.org/2662103002
Cr-Commit-Position: refs/heads/master@{#447595}
Committed: https://chromium.googlesource.com/chromium/src/+/19e2302e5d56e1aaa20a1ac7ee3a863a013ed60c

[F, 2017-01-30 22:53:42]

Description was changed from

==========
Refactor ManifestIconSelector

BUG=
==========

to

==========
Refactor ManifestIconSelector and update it for Manifest.icon.purpose

This CL refactors ManifestIconSelector:
1. Removes the extra copy of icons introduced in FilterIconsByType;
2. Combines the iteration for finding the ideal icon and the iteration
for finding the icon closest to ideal icon into one iteration;
3. Changes out-dated name "preferred icon" to current "ideal icon";
4. Removes non-static class members and functions.

This CL also updates ManifestIconSelector to allow users to pass
desired icon purpose as a parameter to FindBestMatchingIcon.

BUG=649771
==========

[F, 2017-01-30 23:00:29]

zpeng@chromium.org changed reviewers:
+ dominickn@chromium.org

[F, 2017-01-30 23:00:30]

Hi Dominick, PTAL. Thanks!

I ended up with only one static function. I'm not sure whether this is the right
approach. However, I'm reluctant to create single-line static helper functions.
WDUT?

[dominickn, 2017-01-31 01:52:18]

Thanks for doing this so quickly!

https://codereview.chromium.org/2662103002/diff/1/chrome/browser/manifest/man...
File chrome/browser/manifest/manifest_icon_selector.cc (right):

https://codereview.chromium.org/2662103002/diff/1/chrome/browser/manifest/man...
chrome/browser/manifest/manifest_icon_selector.cc:24: for (size_t i = 0; i <
icons.size() && exact_size_match_index == -1; ++i) {
The only time exact_size_match_index is set is immediately prior to a break, so
you can remove that check from the conditional to simplify this loop.

Additionally, I think that means you don't need to have separate indices for
exact_size_match and closest_size_match - they can just be best_size_match, with
an exact match exiting early like you currently have.

https://codereview.chromium.org/2662103002/diff/1/chrome/browser/manifest/man...
chrome/browser/manifest/manifest_icon_selector.cc:39: // Check for size "any".
Nit:

// Check for size "any". Return this icon if no better one is found.

[F, 2017-01-31 16:50:29]

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

[commit-bot: I haz the power, 2017-01-31 16:50:43]

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

[F, 2017-01-31 16:52:59]

Thanks Dominick, PTAL!

Hi Dan, PTAL. Thanks!

https://codereview.chromium.org/2662103002/diff/1/chrome/browser/manifest/man...
File chrome/browser/manifest/manifest_icon_selector.cc (right):

https://codereview.chromium.org/2662103002/diff/1/chrome/browser/manifest/man...
chrome/browser/manifest/manifest_icon_selector.cc:24: for (size_t i = 0; i <
icons.size() && exact_size_match_index == -1; ++i) {
On 2017/01/31 01:52:17, dominickn wrote:
> The only time exact_size_match_index is set is immediately prior to a break,
so
> you can remove that check from the conditional to simplify this loop.
> 
> Additionally, I think that means you don't need to have separate indices for
> exact_size_match and closest_size_match - they can just be best_size_match,
with
> an exact match exiting early like you currently have.

Done. Thanks!

https://codereview.chromium.org/2662103002/diff/1/chrome/browser/manifest/man...
chrome/browser/manifest/manifest_icon_selector.cc:39: // Check for size "any".
On 2017/01/31 01:52:17, dominickn wrote:
> Nit:
> 
> // Check for size "any". Return this icon if no better one is found.

Done.

[F, 2017-01-31 16:55:43]

zpeng@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[F, 2017-01-31 16:55:43]

Hi Dan, PTAL. Thanks!

(Oops, didn't actually add reviewer last email. Sorry for extra email.)

[commit-bot: I haz the power, 2017-01-31 17:44:39]

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

[commit-bot: I haz the power, 2017-01-31 17:44:40]

Dry run: This issue passed the CQ dry run.

[dominickn, 2017-01-31 18:13:03]

lgtm with a nit

https://codereview.chromium.org/2662103002/diff/20001/chrome/browser/installa...
File chrome/browser/installable/installable_manager.cc (right):

https://codereview.chromium.org/2662103002/diff/20001/chrome/browser/installa...
chrome/browser/installable/installable_manager.cc:7: #include <algorithm>
This is unused now?

[pkotwicz, 2017-01-31 18:24:52]

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

[pkotwicz, 2017-01-31 18:24:53]

LGTM

https://codereview.chromium.org/2662103002/diff/20001/chrome/browser/manifest...
File chrome/browser/manifest/manifest_icon_selector.cc (left):

https://codereview.chromium.org/2662103002/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.cc:133:
DCHECK(minimum_icon_size_in_px <= ideal_icon_size_in_px);
Shouldn't we keep this DCHECK?

https://codereview.chromium.org/2662103002/diff/20001/chrome/browser/manifest...
File chrome/browser/manifest/manifest_icon_selector.cc (right):

https://codereview.chromium.org/2662103002/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.cc:30:
mime_util::IsSupportedImageMimeType(base::UTF16ToUTF8(icon.type)))) {
Nit: Expand this out

icon.type.empty() &&
!mime_util::IsSupportedImageMimeType(base::UTF16ToUTF8(icon.type))

https://codereview.chromium.org/2662103002/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.cc:35: if
(!base::ContainsValue(icon.purpose, purpose))
Cool! I didn't know about base::ContainsValue()

https://codereview.chromium.org/2662103002/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.cc:39: for (const auto& size :
icon.sizes) {
Nit: "const auto& size" -> "const gfx::Size& size". I only use auto when it
saves a lot of typing

https://codereview.chromium.org/2662103002/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.cc:40: // Check for size "any".
Return this icon if no better one is found.
Remove "Return this icon if no better one is found." it is obvious from the code

https://codereview.chromium.org/2662103002/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.cc:54: // Check for ideal size.
Return this icon immediately.
Remove "Return this icon immediately." It is obvious from the code

[F, 2017-01-31 18:42:13]

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

[F, 2017-01-31 18:42:50]

Thanks Dominick & Peter, PTAL!

https://codereview.chromium.org/2662103002/diff/20001/chrome/browser/installa...
File chrome/browser/installable/installable_manager.cc (right):

https://codereview.chromium.org/2662103002/diff/20001/chrome/browser/installa...
chrome/browser/installable/installable_manager.cc:7: #include <algorithm>
On 2017/01/31 18:13:03, dominickn wrote:
> This is unused now?

Done.

https://codereview.chromium.org/2662103002/diff/20001/chrome/browser/manifest...
File chrome/browser/manifest/manifest_icon_selector.cc (left):

https://codereview.chromium.org/2662103002/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.cc:133:
DCHECK(minimum_icon_size_in_px <= ideal_icon_size_in_px);
On 2017/01/31 18:24:53, pkotwicz wrote:
> Shouldn't we keep this DCHECK?

Done. Thanks!

https://codereview.chromium.org/2662103002/diff/20001/chrome/browser/manifest...
File chrome/browser/manifest/manifest_icon_selector.cc (right):

https://codereview.chromium.org/2662103002/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.cc:30:
mime_util::IsSupportedImageMimeType(base::UTF16ToUTF8(icon.type)))) {
On 2017/01/31 18:24:53, pkotwicz wrote:
> Nit: Expand this out
> 
> icon.type.empty() &&
> !mime_util::IsSupportedImageMimeType(base::UTF16ToUTF8(icon.type))

Done.

https://codereview.chromium.org/2662103002/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.cc:35: if
(!base::ContainsValue(icon.purpose, purpose))
On 2017/01/31 18:24:53, pkotwicz wrote:
> Cool! I didn't know about base::ContainsValue()

:)

https://codereview.chromium.org/2662103002/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.cc:39: for (const auto& size :
icon.sizes) {
On 2017/01/31 18:24:53, pkotwicz wrote:
> Nit: "const auto& size" -> "const gfx::Size& size". I only use auto when it
> saves a lot of typing

Done.

https://codereview.chromium.org/2662103002/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.cc:40: // Check for size "any".
Return this icon if no better one is found.
On 2017/01/31 18:24:53, pkotwicz wrote:
> Remove "Return this icon if no better one is found." it is obvious from the
code

Resolved offline.

https://codereview.chromium.org/2662103002/diff/20001/chrome/browser/manifest...
chrome/browser/manifest/manifest_icon_selector.cc:54: // Check for ideal size.
Return this icon immediately.
On 2017/01/31 18:24:53, pkotwicz wrote:
> Remove "Return this icon immediately." It is obvious from the code

Resolved offline.

[commit-bot: I haz the power, 2017-01-31 18:43:08]

Dry run: 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-31 19:36:31]

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

[commit-bot: I haz the power, 2017-01-31 19:36:33]

Dry run: This issue passed the CQ dry run.

[gone, 2017-02-01 18:31:30]

lgtm

[F, 2017-02-01 19:00:45]

The CQ bit was checked by zpeng@chromium.org

[F, 2017-02-01 19:00:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, pkotwicz@chromium.org
Link to the patchset: https://codereview.chromium.org/2662103002/#ps40001
(title: "Addressing comments")

[commit-bot: I haz the power, 2017-02-01 19:01:26]

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-02-01 20:22:27]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485975645303320,
"parent_rev": "7b4b3bfebbb99287415c516f8792b9e9c9bc3436", "commit_rev":
"19e2302e5d56e1aaa20a1ac7ee3a863a013ed60c"}

[commit-bot: I haz the power, 2017-02-01 20:23:05]

Description was changed from

==========
Refactor ManifestIconSelector and update it for Manifest.icon.purpose

This CL refactors ManifestIconSelector:
1. Removes the extra copy of icons introduced in FilterIconsByType;
2. Combines the iteration for finding the ideal icon and the iteration
for finding the icon closest to ideal icon into one iteration;
3. Changes out-dated name "preferred icon" to current "ideal icon";
4. Removes non-static class members and functions.

This CL also updates ManifestIconSelector to allow users to pass
desired icon purpose as a parameter to FindBestMatchingIcon.

BUG=649771
==========

to

==========
Refactor ManifestIconSelector and update it for Manifest.icon.purpose

This CL refactors ManifestIconSelector:
1. Removes the extra copy of icons introduced in FilterIconsByType;
2. Combines the iteration for finding the ideal icon and the iteration
for finding the icon closest to ideal icon into one iteration;
3. Changes out-dated name "preferred icon" to current "ideal icon";
4. Removes non-static class members and functions.

This CL also updates ManifestIconSelector to allow users to pass
desired icon purpose as a parameter to FindBestMatchingIcon.

BUG=649771

Review-Url: https://codereview.chromium.org/2662103002
Cr-Commit-Position: refs/heads/master@{#447595}
Committed:
https://chromium.googlesource.com/chromium/src/+/19e2302e5d56e1aaa20a1ac7ee3a...
==========

[commit-bot: I haz the power, 2017-02-01 20:23:06]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/19e2302e5d56e1aaa20a1ac7ee3a...