Rename best icon to best primary icon in Web app related code.

Rename best icon to best primary icon in Web app related code.

In order to differentiate primary icon from newly introduced badge
icon, this CL renames all occurrences of best icon mentions in Web app
code or WebAPK code.

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

[F, 2017-02-02 22:06:59]

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

[commit-bot: I haz the power, 2017-02-02 22:07:19]

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

[F, 2017-02-02 22:24:49]

Description was changed from

==========
Rename best icon to best primary icon in Web app related code.

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

to

==========
Rename best icon to best primary icon in Web app related code.

In order to differentiate primary icon to newly introduced badge icon,
this CL renames all best icon occurrences of best icon mentions in Web
app code or WebAPK code.

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

[F, 2017-02-02 22:25:08]

Description was changed from

==========
Rename best icon to best primary icon in Web app related code.

In order to differentiate primary icon to newly introduced badge icon,
this CL renames all best icon occurrences of best icon mentions in Web
app code or WebAPK code.

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

to

==========
Rename best icon to best primary icon in Web app related code.

In order to differentiate primary icon from newly introduced badge icon,
this CL renames all best icon occurrences of best icon mentions in Web
app code or WebAPK code.

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

[F, 2017-02-02 22:25:23]

Description was changed from

==========
Rename best icon to best primary icon in Web app related code.

In order to differentiate primary icon from newly introduced badge icon,
this CL renames all best icon occurrences of best icon mentions in Web
app code or WebAPK code.

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

to

==========
Rename best icon to best primary icon in Web app related code.

In order to differentiate primary icon from newly introduced badge
icon, this CL renames all best icon occurrences of best icon mentions
in Web app code or WebAPK code.

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

[F, 2017-02-02 22:25:47]

Description was changed from

==========
Rename best icon to best primary icon in Web app related code.

In order to differentiate primary icon from newly introduced badge
icon, this CL renames all best icon occurrences of best icon mentions
in Web app code or WebAPK code.

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

to

==========
Rename best icon to best primary icon in Web app related code.

In order to differentiate primary icon from newly introduced badge
icon, this CL renames all occurrences of best icon mentions in Web app
code or WebAPK code.

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

[F, 2017-02-02 22:26:29]

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

[F, 2017-02-02 22:26:30]

Hi Dominick, PTAL. Thanks!

[commit-bot: I haz the power, 2017-02-02 23:33:51]

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

[commit-bot: I haz the power, 2017-02-02 23:33:52]

Dry run: This issue passed the CQ dry run.

[dominickn, 2017-02-02 23:34:13]

https://codereview.chromium.org/2671853002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateDataFetcher.java
(right):

https://codereview.chromium.org/2671853002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateDataFetcher.java:2:
// Use of this source code is governed by a BSD-style license that can be
I don't actually think you need to rename any of the WebAPK update related
files. None of them use the badge icon at all. The main reason to rename to
bestPrimaryIcon is when you have a bestIcon and a bestBadgeIcon member, and
that's not the case here.

If in the future WebAPK updates start taking the badge icon into account, then
we can rename in place. For now, I think it's best to revert the changes to all
of the Java files in this CL. :)

https://codereview.chromium.org/2671853002/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapk/webapk_installer_unittest.cc (right):

https://codereview.chromium.org/2671853002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapk/webapk_installer_unittest.cc:433: //
|best_primary_icon_url|
Nit: comment formatting

https://codereview.chromium.org/2671853002/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right):

https://codereview.chromium.org/2671853002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:165:
IPC_MESSAGE_HANDLER(ChromeViewHostMsg_DidGetWebApplicationInfo,
The standard Chromium style is to have IPC_MESSAGE_HANDLER be indented two
spaces. Please restore (did you run clang-format on this CL? Did it do this?)

[F, 2017-02-03 00:46:45]

Patchset #2 (id:20001) has been deleted

[F, 2017-02-03 00:47:15]

Thanks Dominick, PTAL!

https://codereview.chromium.org/2671853002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateDataFetcher.java
(right):

https://codereview.chromium.org/2671853002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateDataFetcher.java:2:
// Use of this source code is governed by a BSD-style license that can be
On 2017/02/02 23:34:13, dominickn wrote:
> I don't actually think you need to rename any of the WebAPK update related
> files. None of them use the badge icon at all. The main reason to rename to
> bestPrimaryIcon is when you have a bestIcon and a bestBadgeIcon member, and
> that's not the case here.
> 
> If in the future WebAPK updates start taking the badge icon into account, then
> we can rename in place. For now, I think it's best to revert the changes to
all
> of the Java files in this CL. :)

There are two reasons that I included update files in this CL:
1. Currently I'm investigating modifying WebAPK update to take badge icon into
account, so I think these changes are not too early.
2. The WebApkUpdateDataFetcher uses the InstallableManager to fetch data. As a
result, there is a "primary icon" to "icon" change in parameter names. And then
WebApkUpdateDataFetcher calls into the Java code. So I think if I change the C++
code of WebAPK update to resolve the inconsistency, I might as well resolve the
inconsistency in the Java code.

On the other side, I don't mind splitting this CL into two parts. WDUT?

https://codereview.chromium.org/2671853002/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapk/webapk_installer_unittest.cc (right):

https://codereview.chromium.org/2671853002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapk/webapk_installer_unittest.cc:433: //
|best_primary_icon_url|
On 2017/02/02 23:34:13, dominickn wrote:
> Nit: comment formatting

Done. Thanks!

https://codereview.chromium.org/2671853002/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right):

https://codereview.chromium.org/2671853002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:165:
IPC_MESSAGE_HANDLER(ChromeViewHostMsg_DidGetWebApplicationInfo,
On 2017/02/02 23:34:13, dominickn wrote:
> The standard Chromium style is to have IPC_MESSAGE_HANDLER be indented two
> spaces. Please restore (did you run clang-format on this CL? Did it do this?)

Running clang-format restored the old formatting. I have no idea what
happened...

[dominickn, 2017-02-03 01:28:00]

On 2017/02/03 00:47:15, F wrote:
> Thanks Dominick, PTAL!
> 
>
https://codereview.chromium.org/2671853002/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateDataFetcher.java
> (right):
> 
>
https://codereview.chromium.org/2671853002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateDataFetcher.java:2:
> // Use of this source code is governed by a BSD-style license that can be
> On 2017/02/02 23:34:13, dominickn wrote:
> > I don't actually think you need to rename any of the WebAPK update related
> > files. None of them use the badge icon at all. The main reason to rename to
> > bestPrimaryIcon is when you have a bestIcon and a bestBadgeIcon member, and
> > that's not the case here.
> > 
> > If in the future WebAPK updates start taking the badge icon into account,
then
> > we can rename in place. For now, I think it's best to revert the changes to
> all
> > of the Java files in this CL. :)
> 
> There are two reasons that I included update files in this CL:
> 1. Currently I'm investigating modifying WebAPK update to take badge icon into
> account, so I think these changes are not too early.
> 2. The WebApkUpdateDataFetcher uses the InstallableManager to fetch data. As a
> result, there is a "primary icon" to "icon" change in parameter names. And
then
> WebApkUpdateDataFetcher calls into the Java code. So I think if I change the
C++
> code of WebAPK update to resolve the inconsistency, I might as well resolve
the
> inconsistency in the Java code.
> 
> On the other side, I don't mind splitting this CL into two parts. WDUT?

I think the primary icon to icon name change is fine at the C++/Java boundary.
The main question is how likely is it that WebAPK updates use incorporate the
badge icon? I'm not overly convinced they should - what's the reasoning for it?

If it's a sure thing that we need badge icons for WebAPK updates then it makes
sense to do the full renaming.

[pkotwicz, 2017-02-03 04:45:08]

When we send an update request we need to send all of the data including the
badge icon. The easy solution is to fetch the badge icon in
WebApkUpdateDataFetcher.java
I am unsure whether we should use the hash of the badge icon bitmap in order to
determine whether to do an update or not

[dominickn, 2017-02-03 18:31:42]

On 2017/02/03 04:45:08, pkotwicz wrote:
> When we send an update request we need to send all of the data including the
> badge icon. The easy solution is to fetch the badge icon in
> WebApkUpdateDataFetcher.java
> I am unsure whether we should use the hash of the badge icon bitmap in order
to
> determine whether to do an update or not

That is my question here - if the badge icon isn't going to be used to determine
whether or not to update, then we can probably avoid adding the "primary"
qualifier in all of the update code because it should be self-evident (it's only
ever checking one icon).

However, if we are going to use the badge icon for update checking, then it does
make sense to do the (extra 150 lines in this CL) renaming.

[pkotwicz, 2017-02-03 18:42:14]

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

[pkotwicz, 2017-02-03 18:42:15]

https://codereview.chromium.org/2671853002/diff/40001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java
(right):

https://codereview.chromium.org/2671853002/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:114:
* ManifestUpgradeDetector subclass which: - Uses mock
ManifestUpgradeDetectorFetcher. - Uses
Please keep the bullet points. There's the same issue with the comments for the
other tests

https://codereview.chromium.org/2671853002/diff/40001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java
(right):

https://codereview.chromium.org/2671853002/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:450:
/**
Leave the bullet points one per line with the AND on a seperate line

[F, 2017-02-06 12:26:35]

Patchset #3 (id:60001) has been deleted

[F, 2017-02-06 12:33:12]

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

[commit-bot: I haz the power, 2017-02-06 12:33:28]

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

[F, 2017-02-06 12:40:13]

Thanks Dominick & Peter, PTAL!

I'm removing the Java changes as per discussion we might not use badge icon hash
for checking updates. So I'm going to leave the Java part as it is now :)

https://codereview.chromium.org/2671853002/diff/40001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java
(right):

https://codereview.chromium.org/2671853002/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java:114:
* ManifestUpgradeDetector subclass which: - Uses mock
ManifestUpgradeDetectorFetcher. - Uses
On 2017/02/03 18:42:15, pkotwicz wrote:
> Please keep the bullet points. There's the same issue with the comments for
the
> other tests

Acknowledged. Will pay attention not to let the auto-formatter mess it up next
time :) Also sorry for not noticing that this test was unused before changing
it...

https://codereview.chromium.org/2671853002/diff/40001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java
(right):

https://codereview.chromium.org/2671853002/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:450:
/**
On 2017/02/03 18:42:15, pkotwicz wrote:
> Leave the bullet points one per line with the AND on a seperate line

Acknowledged. Will avoid auto-formatter next time :)

[commit-bot: I haz the power, 2017-02-06 13:11:52]

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

[commit-bot: I haz the power, 2017-02-06 13:11:53]

Dry run: This issue passed the CQ dry run.

[pkotwicz, 2017-02-06 23:50:05]

LGTM

[dominickn, 2017-02-06 23:56:10]

lgtm, thanks

[F, 2017-02-07 11:39:04]

The CQ bit was checked by zpeng@chromium.org

[commit-bot: I haz the power, 2017-02-07 11:39:11]

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-07 11:42:48]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486467544069410,
"parent_rev": "30d6e25b2f1c69090883e97110877815f1ae3c72", "commit_rev":
"a7856eef7d2f632f777a82a8af8eec76315c58b8"}

[commit-bot: I haz the power, 2017-02-07 11:43:25]

Description was changed from

==========
Rename best icon to best primary icon in Web app related code.

In order to differentiate primary icon from newly introduced badge
icon, this CL renames all occurrences of best icon mentions in Web app
code or WebAPK code.

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

to

==========
Rename best icon to best primary icon in Web app related code.

In order to differentiate primary icon from newly introduced badge
icon, this CL renames all occurrences of best icon mentions in Web app
code or WebAPK code.

BUG=649771

Review-Url: https://codereview.chromium.org/2671853002
Cr-Commit-Position: refs/heads/master@{#448601}
Committed:
https://chromium.googlesource.com/chromium/src/+/a7856eef7d2f632f777a82a8af8e...
==========

[commit-bot: I haz the power, 2017-02-07 11:43:26]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/a7856eef7d2f632f777a82a8af8e...