Rename "icon" to "primary icon" in InstallableManager

Rename "icon" to "primary icon" in InstallableManager.

This CL renames "icon" to "primary icon" in InstallableParams &
InstallableData in
InstallableManager to differentiate icons with purpose "any" from icons
with purpose "badge".

Followup CLs will update InstallableManager to download icons with
purpose "badge" if "badge" icons are requested.

BUG=649771
Review-Url: https://codereview.chromium.org/2625423002
Cr-Commit-Position: refs/heads/master@{#443896}
Committed: https://chromium.googlesource.com/chromium/src/+/919511bd14650ee9b2dde04116861f5c52db30c5

[F, 2017-01-12 21:10:33]

Description was changed from

==========
Rename icon to default icon in InstallableParams

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

to

==========
Rename "icon" to "default icon" in InstallableParams in InstallableManager.

This CL renames "icon" to "default icon" in InstallableParams in
InstallableManager to prepare for the addition of "badge"-purposed icons.

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

[F, 2017-01-12 21:24:09]

Description was changed from

==========
Rename "icon" to "default icon" in InstallableParams in InstallableManager.

This CL renames "icon" to "default icon" in InstallableParams in
InstallableManager to prepare for the addition of "badge"-purposed icons.

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

to

==========
Rename "icon" to "default icon" in InstallableParams in InstallableManager.

This CL renames "icon" to "default icon" in InstallableParams in
InstallableManager to differentiate icons with purpose "any" from icons with
purpose "badge".

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

[F, 2017-01-12 21:45:12]

Description was changed from

==========
Rename "icon" to "default icon" in InstallableParams in InstallableManager.

This CL renames "icon" to "default icon" in InstallableParams in
InstallableManager to differentiate icons with purpose "any" from icons with
purpose "badge".

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

to

==========
Rename "icon" to "default icon" in InstallableParams in InstallableManager.

This CL renames "icon" to "default icon" in InstallableParams in
InstallableManager to differentiate icons with purpose "any" from icons
with purpose "badge".

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

[F, 2017-01-12 21:47:32]

Description was changed from

==========
Rename "icon" to "default icon" in InstallableParams in InstallableManager.

This CL renames "icon" to "default icon" in InstallableParams in
InstallableManager to differentiate icons with purpose "any" from icons
with purpose "badge".

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

to

==========
Rename "icon" to "default icon" in InstallableParams in InstallableManager.

This CL renames "icon" to "default icon" in InstallableParams in
InstallableManager to differentiate icons with purpose "any" from icons
with purpose "badge".

Followup CLs will update InstallableManager to download icons with
purpose "badge" if "badge" icons are requested.

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

[F, 2017-01-12 21:48:16]

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

[commit-bot: I haz the power, 2017-01-12 21:48:55]

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-12 22:45:31]

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

[commit-bot: I haz the power, 2017-01-12 22:45:32]

Dry run: This issue passed the CQ dry run.

[F, 2017-01-12 23:07:32]

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

[F, 2017-01-12 23:07:32]

Hi Dominick & Peter, PTAL. Thanks!

[dominickn, 2017-01-12 23:22:14]

Hi Felix,

I'm not sure "default" is the best name here. Perhaps "primary" is a better
name, since the icon fetched here is the primary icon used by banners and A2HS?
Default makes it sound like we're getting the icon that the developer specified
as default in the manifest, which isn't the case.

[F, 2017-01-13 15:45:31]

Description was changed from

==========
Rename "icon" to "default icon" in InstallableParams in InstallableManager.

This CL renames "icon" to "default icon" in InstallableParams in
InstallableManager to differentiate icons with purpose "any" from icons
with purpose "badge".

Followup CLs will update InstallableManager to download icons with
purpose "badge" if "badge" icons are requested.

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

to

==========
Rename "icon" to "primary icon" in InstallableParams in InstallableManager.

This CL renames "icon" to "primary icon" in InstallableParams in
InstallableManager to differentiate icons with purpose "any" from icons
with purpose "badge".

Followup CLs will update InstallableManager to download icons with
purpose "badge" if "badge" icons are requested.

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

[F, 2017-01-13 15:46:57]

On 2017/01/12 23:22:14, dominickn wrote:
> Hi Felix,
> 
> I'm not sure "default" is the best name here. Perhaps "primary" is a better
> name, since the icon fetched here is the primary icon used by banners and
A2HS?
> Default makes it sound like we're getting the icon that the developer
specified
> as default in the manifest, which isn't the case.

Hi Dominick, thanks for the suggestion. I agree that "primary" is a better name.
PTAL!

[F, 2017-01-13 20:01:58]

Patchset #4 (id:60001) has been deleted

[F, 2017-01-13 20:03:44]

Description was changed from

==========
Rename "icon" to "primary icon" in InstallableParams in InstallableManager.

This CL renames "icon" to "primary icon" in InstallableParams in
InstallableManager to differentiate icons with purpose "any" from icons
with purpose "badge".

Followup CLs will update InstallableManager to download icons with
purpose "badge" if "badge" icons are requested.

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

to

==========
Rename "icon" to "primary icon" in InstallableManager.

This CL renames "icon" to "primary icon" in InstallableParams &
InstallableData in
InstallableManager to differentiate icons with purpose "any" from icons
with purpose "badge".

Followup CLs will update InstallableManager to download icons with
purpose "badge" if "badge" icons are requested.

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

[F, 2017-01-13 20:18:08]

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

[commit-bot: I haz the power, 2017-01-13 20:19:04]

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-13 22:01:06]

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

[commit-bot: I haz the power, 2017-01-13 22:01:06]

Dry run: This issue passed the CQ dry run.

[dominickn, 2017-01-16 02:10:06]

lgtm % nit

https://codereview.chromium.org/2625423002/diff/40001/chrome/browser/installa...
File chrome/browser/installable/installable_manager.h (right):

https://codereview.chromium.org/2625423002/diff/40001/chrome/browser/installa...
chrome/browser/installable/installable_manager.h:41: // Check whether there is a
primary icon in the manifest conforming to the
Nit: we're not checking for a primary icon in the manifest. We're checking for
any icon in the manifest that conforms to the primary size, and we'll use that
as the primary. So this comment should read:

Check whether there is a fetchable, non-empty icon in the manifest conforming to
the primary icon size parameters.

[F, 2017-01-16 14:37:52]

Thanks Dominick! I'm going to submit it soon

https://codereview.chromium.org/2625423002/diff/40001/chrome/browser/installa...
File chrome/browser/installable/installable_manager.h (right):

https://codereview.chromium.org/2625423002/diff/40001/chrome/browser/installa...
chrome/browser/installable/installable_manager.h:41: // Check whether there is a
primary icon in the manifest conforming to the
On 2017/01/16 02:10:06, dominickn wrote:
> Nit: we're not checking for a primary icon in the manifest. We're checking for
> any icon in the manifest that conforms to the primary size, and we'll use that
> as the primary. So this comment should read:
> 
> Check whether there is a fetchable, non-empty icon in the manifest conforming
to
> the primary icon size parameters.

Done. Thanks!

[F, 2017-01-16 14:38:04]

The CQ bit was checked by zpeng@chromium.org

[F, 2017-01-16 14:38:04]

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

[commit-bot: I haz the power, 2017-01-16 14:38:20]

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-16 15:17:09]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484577484435000,
"parent_rev": "d8018eedb0e782756bc86db7a629f272589309a6", "commit_rev":
"919511bd14650ee9b2dde04116861f5c52db30c5"}

[commit-bot: I haz the power, 2017-01-16 15:17:41]

Description was changed from

==========
Rename "icon" to "primary icon" in InstallableManager.

This CL renames "icon" to "primary icon" in InstallableParams &
InstallableData in
InstallableManager to differentiate icons with purpose "any" from icons
with purpose "badge".

Followup CLs will update InstallableManager to download icons with
purpose "badge" if "badge" icons are requested.

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

to

==========
Rename "icon" to "primary icon" in InstallableManager.

This CL renames "icon" to "primary icon" in InstallableParams &
InstallableData in
InstallableManager to differentiate icons with purpose "any" from icons
with purpose "badge".

Followup CLs will update InstallableManager to download icons with
purpose "badge" if "badge" icons are requested.

BUG=649771

Review-Url: https://codereview.chromium.org/2625423002
Cr-Commit-Position: refs/heads/master@{#443896}
Committed:
https://chromium.googlesource.com/chromium/src/+/919511bd14650ee9b2dde0411686...
==========

[commit-bot: I haz the power, 2017-01-16 15:17:43]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/919511bd14650ee9b2dde0411686...