[NTP::Downloads] Scale up type based icons.

[NTP::Downloads] Scale up type based icons.

1) Add missing 36dp png resources 
(5 icons, i.e. 25 png's, overall 6790 bytes);
2) Request 36dp resources instead of 24dp and downscale them to 32dp.

Reason: UI people request.

BUG=693552
Review-Url: https://codereview.chromium.org/2721043002
Cr-Commit-Position: refs/heads/master@{#453999}
Committed: https://chromium.googlesource.com/chromium/src/+/fac08957f5011b15190fb2c18fe6c15fe8ed5381

[vitaliii, 2017-02-28 15:21:24]

Patchset #1 (id:1) has been deleted

[vitaliii, 2017-02-28 15:22:26]

vitaliii@chromium.org changed reviewers:
+ dgn@chromium.org

[vitaliii, 2017-02-28 15:22:27]

Hi dgn@,

Could you have a look at ntp/snippets please?

[vitaliii, 2017-02-28 15:22:46]

vitaliii@chromium.org changed reviewers:
+ dfalcantara@chromium.org
- dgn@chromium.org

[vitaliii, 2017-02-28 15:22:46]

vitaliii@chromium.org changed reviewers:
+ dfalcantara@chromium.org
- dgn@chromium.org

[vitaliii, 2017-02-28 15:22:47]

Hi dfalcantara@

[vitaliii, 2017-02-28 15:22:47]

Hi dfalcantara@

[vitaliii, 2017-02-28 15:23:28]

vitaliii@chromium.org changed reviewers:
+ dgn@chromium.org

[vitaliii, 2017-02-28 15:23:28]

Hi dfalcantara@,

Could you have a look at java/res and browser/download please?

[dgn, 2017-02-28 17:39:13]

Did you try something like
https://medium.com/@emmaguy/dynamically-changing-svg-colours-on-android-b026a...

Also, do you have numbers for the APK size difference?

[dgn, 2017-02-28 18:30:17]

also tried to get tinting svg inside chromium to work without success. Might try
again from a toy project at some point, but this is getting comments. If the
size increase is acceptable I guess we won't have much choice.

Dan: do you know if downscaling the 36dp to 24 for usage in the downloads UI is
better than duplicating the images?

Other possibility that might be worth trying: render vector to bitmap of the
appropriate size (something like http://stackoverflow.com/a/35633411/1725655),
then tint it (We have been using tinted bitmaps before so that should work), and
keep that around to avoid doing the computations repeatedly?

https://codereview.chromium.org/2721043002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java
(right):

https://codereview.chromium.org/2721043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:683:
assert sizeDp == 24 || sizeDp == 36;
nit: use 2 @IntDef constants to make clear through the API which values are
supported?

https://codereview.chromium.org/2721043002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
(right):

https://codereview.chromium.org/2721043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:306:
int viewWidth = mThumbnailView.getResources().getDimensionPixelSize(
nit: just name it viewSize and skip creating viewHeight below? the resource
itself is named that way. Also, AndroidStudio would show warnings about the
extra variable creation.

[gone, 2017-02-28 18:46:25]

1) Have you run tools/resources/optimize-png-files.sh on the assets?

2) I'd prefer having a single asset scaled down, but UX will probably push back
on that.  Having multiple versions of the same assets is a waste of space for
potentially negligible detail gain.

3) While there _is_ SVG support, it's a pain to get it working.  You need to
convert the files into using a special tool, then you lose the ability to tint
the icons (according to some email thread I'm reading).

https://codereview.chromium.org/2721043002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java
(right):

https://codereview.chromium.org/2721043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:652:
* e.g. thisisaverylongfilename.txt => thisisave....txt.
I'd remove this period; it makes the example ambiguous

https://codereview.chromium.org/2721043002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
(right):

https://codereview.chromium.org/2721043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:314:
viewHeight / 2f - drawableHeight * scale / 2f);
Why's this matrix necessary?  Can't you just use ScaleType.center_inside?

[vitaliii, 2017-03-01 09:05:25]

Patchset #3 (id:60001) has been deleted

[vitaliii, 2017-03-01 09:08:30]

Hi dgn@ and dfalcantara@,

I addressed your comments.
Currently I am waiting for rachelis@ to decide whether we want to downscale 36dp
to 32dp.

>1) Have you run tools/resources/optimize-png-files.sh on the assets?

No, thank you for the remainder.

I did 3 runs.

Optimized 19/25 files in 00:10:16s
Result: 6950 => 6793 bytes (157 bytes: 2%)

Optimized 3/25 files in 00:09:41s
Result: 1609 => 1606 bytes (3 bytes: 0%)

Optimized 0/25 files in 00:07:36s

>2) I'd prefer having a single asset scaled down, but UX will probably
>push back on that.  Having multiple versions of the same assets is a
>waste of space for potentially negligible detail gain.

Whatever the decision, I would like to address it in a different CL.

>3) While there _is_ SVG support, it's a pain to get it working.  You
>need to convert the files into using a special tool, then you lose the
>ability to tint the icons (according to some email thread I'm reading).

I tried to tint SVG too, but did not manage.

https://codereview.chromium.org/2721043002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java
(right):

https://codereview.chromium.org/2721043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:652:
* e.g. thisisaverylongfilename.txt => thisisave....txt.
On 2017/02/28 18:46:25, dfalcantara (load balance plz) wrote:
> I'd remove this period; it makes the example ambiguous

Done.

"thisisaverylongfilename.txt" => "thisisave....txt".

https://codereview.chromium.org/2721043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:683:
assert sizeDp == 24 || sizeDp == 36;
On 2017/02/28 18:30:16, dgn wrote:
> nit: use 2 @IntDef constants to make clear through the API which values are
> supported?

Done.

https://codereview.chromium.org/2721043002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
(right):

https://codereview.chromium.org/2721043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:306:
int viewWidth = mThumbnailView.getResources().getDimensionPixelSize(
On 2017/02/28 18:30:16, dgn wrote:
> nit: just name it viewSize and skip creating viewHeight below? the resource
> itself is named that way. Also, AndroidStudio would show warnings about the
> extra variable creation.

Done.

https://codereview.chromium.org/2721043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:314:
viewHeight / 2f - drawableHeight * scale / 2f);
On 2017/02/28 18:46:25, dfalcantara (load balance plz) wrote:
> Why's this matrix necessary?  Can't you just use ScaleType.center_inside?

Done. No matrix anymore.

It was used for scaling, translation was a consequence.

However, setPadding happened to work with setImageDrawable (it did not work with
setImageResource). Thus, now I use setPadding instead of a matrix.

I need to downscale to 32dp, because this was a UI request. 

ScaleType.center_inside by itself leads to 36dp icons.

[vitaliii, 2017-03-01 09:08:47]

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

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

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

[vitaliii, 2017-03-01 09:46:05]

Hi folks,

I've just talked to rachelis@ and she is fine with the current approach (see the
bug for details).

Therefore, please have a look, I would like to land this.

As for downscaling 36dp to 24dp in Downloads Home (and removing 24dp png's),
rachelis@ is fine as long as dfalcantara@ is fine. However, I would prefer not
to address this in this CL.

[commit-bot: I haz the power, 2017-03-01 09:46:31]

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

[commit-bot: I haz the power, 2017-03-01 09:46:32]

Dry run: This issue passed the CQ dry run.

[dgn, 2017-03-01 10:31:21]

On 2017/03/01 09:46:05, vitaliii wrote:
> Hi folks,
> 
> I've just talked to rachelis@ and she is fine with the current approach (see
the
> bug for details).
> 
> Therefore, please have a look, I would like to land this.
> 
> As for downscaling 36dp to 24dp in Downloads Home (and removing 24dp png's),
> rachelis@ is fine as long as dfalcantara@ is fine. However, I would prefer not
> to address this in this CL.

Why not? that would remove the need to add 36/24 dp switches, reverting a bunch
of the changes introduced in this CL. And if one of the CL lands, both would
have to be in the 58 branch anyway.

[dgn, 2017-03-01 10:57:01]

lgtm if Dan is happy. 

Also please put the resource size diff in the commit message.

https://codereview.chromium.org/2721043002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java
(right):

https://codereview.chromium.org/2721043002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:93:
public @interface IconSize {}
nit: also add @Retention(RetentionPolicy.SOURCE)

https://codereview.chromium.org/2721043002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:689:
* @param iconSize Size of the returned icon.
nit: if the parameter is described as "size of the icon" then the value for the
enums should be 24/36. If you want to keep them as 0/1, then "size selector for
the icon" or something similar would be more appropriate.

[vitaliii, 2017-03-01 11:04:31]

Description was changed from

==========
[NTP::Downloads] Scale up type based icons.

1) Add missing 36dp png resources;
2) Request 36dp resources instead of 24dp and downscale them to 32dp.

Reason: UI people request.

BUG=693552
==========

to

==========
[NTP::Downloads] Scale up type based icons.

1) Add missing 36dp png resources 
(5 icons, i.e. 25 png's, overall 6790 bytes);
2) Request 36dp resources instead of 24dp and downscale them to 32dp.

Reason: UI people request.

BUG=693552
==========

[vitaliii, 2017-03-01 11:23:55]

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

[commit-bot: I haz the power, 2017-03-01 11:24:13]

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

[vitaliii, 2017-03-01 11:24:44]

Hi dgn@, 

I've addressed your comments. No need to look.

https://codereview.chromium.org/2721043002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java
(right):

https://codereview.chromium.org/2721043002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:93:
public @interface IconSize {}
On 2017/03/01 10:57:01, dgn wrote:
> nit: also add @Retention(RetentionPolicy.SOURCE)

Done.

https://codereview.chromium.org/2721043002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:689:
* @param iconSize Size of the returned icon.
On 2017/03/01 10:57:01, dgn wrote:
> nit: if the parameter is described as "size of the icon" then the value for
the
> enums should be 24/36. If you want to keep them as 0/1, then "size selector
for
> the icon" or something similar would be more appropriate.  

Done.

24/36.

[vitaliii, 2017-03-01 11:24:45]

Hi dgn@, 

I've addressed your comments. No need to look.

https://codereview.chromium.org/2721043002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java
(right):

https://codereview.chromium.org/2721043002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:93:
public @interface IconSize {}
On 2017/03/01 10:57:01, dgn wrote:
> nit: also add @Retention(RetentionPolicy.SOURCE)

Done.

https://codereview.chromium.org/2721043002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:689:
* @param iconSize Size of the returned icon.
On 2017/03/01 10:57:01, dgn wrote:
> nit: if the parameter is described as "size of the icon" then the value for
the
> enums should be 24/36. If you want to keep them as 0/1, then "size selector
for
> the icon" or something similar would be more appropriate.  

Done.

24/36.

[tschumann, 2017-03-01 11:30:28]

On 2017/03/01 10:31:21, dgn wrote:
> On 2017/03/01 09:46:05, vitaliii wrote:
> > Hi folks,
> > 
> > I've just talked to rachelis@ and she is fine with the current approach (see
> the
> > bug for details).
> > 
> > Therefore, please have a look, I would like to land this.
> > 
> > As for downscaling 36dp to 24dp in Downloads Home (and removing 24dp png's),
> > rachelis@ is fine as long as dfalcantara@ is fine. However, I would prefer
not
> > to address this in this CL.
> 
> Why not? that would remove the need to add 36/24 dp switches, reverting a
bunch
> of the changes introduced in this CL. And if one of the CL lands, both would
> have to be in the 58 branch anyway.

+1 for keeping it out of this CL. These are decoupled features and given the
turn-around times and code-review dependencies, I wouldn't want to couple them
unnecessarily.

[commit-bot: I haz the power, 2017-03-01 11:56:07]

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

[commit-bot: I haz the power, 2017-03-01 11:56:08]

Dry run: This issue passed the CQ dry run.

[vitaliii, 2017-03-01 14:08:21]

I noticed that sometimes the placeholder was set in a padded view, so now I
reset the padding before setting the placeholder.

[gone, 2017-03-01 18:21:13]

lgtm for this as is, but I'm pushing back on the bug now that I've seen the
screenshots.

https://codereview.chromium.org/2721043002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
(right):

https://codereview.chromium.org/2721043002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:298:
final int actualIconSizeDp = 36;
Add a comment here about why this calculation is needed.

// The provided asset is 36dp but the spec requires 32dp.  Add padding to force
the asset to be scaled down.

[vitaliii, 2017-03-01 19:01:31]

Hi dfalcantara@, 

I addressed your comment. No need to look.
Just in case I will lend the CL today (you convinced me).

https://codereview.chromium.org/2721043002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
(right):

https://codereview.chromium.org/2721043002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:298:
final int actualIconSizeDp = 36;
On 2017/03/01 18:21:13, dfalcantara (load balance plz) wrote:
> Add a comment here about why this calculation is needed.
> 
> // The provided asset is 36dp but the spec requires 32dp.  Add padding to
force
> the asset to be scaled down.

Done.

[vitaliii, 2017-03-01 19:01:53]

The CQ bit was checked by vitaliii@chromium.org

[vitaliii, 2017-03-01 19:01:54]

The patchset sent to the CQ was uploaded after l-g-t-m from dgn@chromium.org,
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2721043002/#ps140001
(title: "dfalcantara@ comment.")

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

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-01 19:42:54]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488394913706990,
"parent_rev": "9afc576bd3e06d83d6abc870000293b4be9d6132", "commit_rev":
"fac08957f5011b15190fb2c18fe6c15fe8ed5381"}

[commit-bot: I haz the power, 2017-03-01 19:44:17]

Description was changed from

==========
[NTP::Downloads] Scale up type based icons.

1) Add missing 36dp png resources 
(5 icons, i.e. 25 png's, overall 6790 bytes);
2) Request 36dp resources instead of 24dp and downscale them to 32dp.

Reason: UI people request.

BUG=693552
==========

to

==========
[NTP::Downloads] Scale up type based icons.

1) Add missing 36dp png resources 
(5 icons, i.e. 25 png's, overall 6790 bytes);
2) Request 36dp resources instead of 24dp and downscale them to 32dp.

Reason: UI people request.

BUG=693552

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

[commit-bot: I haz the power, 2017-03-01 19:44:19]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/fac08957f5011b15190fb2c18fe6...

[Theresa, 2017-05-24 21:57:10]

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

[Theresa, 2017-05-24 21:57:10]

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

[Theresa, 2017-05-24 21:57:11]

https://codereview.chromium.org/2721043002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java
(right):

https://codereview.chromium.org/2721043002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:714:
: R.drawable.ic_drive_text_white_36dp;
Is this supposed to be ic_drive_file_white_36dp?

[Theresa, 2017-05-24 21:57:11]

https://codereview.chromium.org/2721043002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java
(right):

https://codereview.chromium.org/2721043002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:714:
: R.drawable.ic_drive_text_white_36dp;
Is this supposed to be ic_drive_file_white_36dp?

[vitaliii, 2017-05-26 09:41:12]

https://codereview.chromium.org/2721043002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java
(right):

https://codereview.chromium.org/2721043002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:714:
: R.drawable.ic_drive_text_white_36dp;
On 2017/05/24 21:57:11, Theresa (OOO) wrote:
> Is this supposed to be ic_drive_file_white_36dp?

Yes (copy-paste mistake). Good catch! 

Fix: https://chromium-review.googlesource.com/c/517062/