1) Which UX people are you working with?

2) Are they aware that the only assets available on go/icons website are 36dp?  

3) Are they aware that you'll be downscaling assets down to match what they're asking for?  I don't want you to land this and then have to land something again later because they're blurry.

4) Have they been shown screenshots of your UI with the assets changed?

> 1) Which UX people are you working with?

rachelis@

> 2) Are they aware that the only assets available on go/icons website are
> 36dp?

I do not know, I've asked.

> 3) Are they aware that you'll be downscaling assets down to match what
> they're asking for?  I don't want you to land this and then have to land
> something again later because they're blurry.

Likely not.
I guess just using 36dp would resolve all of these.

> 4) Have they been shown screenshots of your UI with the assets changed?

See attached.

Deleted: scr_matrix_32dp_downscaled.png 263 KB

	scr_matrix_32dp_downscaled.png 263 KB View Download

Deleted: scr_padding_32dp_downscaled.png 376 KB

	scr_padding_32dp_downscaled.png 376 KB View Download

Deleted: scr_scale_inside_36dp_original.png 378 KB

	scr_scale_inside_36dp_original.png 378 KB View Download

For comparison a screenshot of downloads UI.

Deleted: scr_downloads_ui.png 171 KB

	scr_downloads_ui.png 171 KB View Download

Thanks Dan, we appreciate the support!

//2) Are they aware that the only assets available on go/icons website are 36dp?  
I wasn't aware - thanks for the heads-up!

//3) Are they aware that you'll be downscaling assets down to match what they're asking for?  I don't want you to land this and then have to land something again later because they're blurry.

I had a look at the screenshots and they look good to me. Dan, any concerns? Any other considerations we should be aware of?

Slight correction on go/icons; there's a 24, 36, and 48, but no 32.

No concerns; just wanted to make sure everything looked good before finishing the review.

I guess I _am_ confused about why 36dp isn't a valid number to use here.  There's easily more than enough space on that block to accommodate an additional 4dp, and the code required to munge the drawable to be that size looks really out of place and unnecessary.  How much would it change the aesthetic?

I am fine to wait for rachelis@ to reply before landing the CL (in case she agrees to go with 36dp).

I'd say your CL is fine to land as is; removing the calculation should be an easy merge back.  Either way, branch point is tomorrow, so I'll leave it up to you.

Poking again.  Can this be 36dp instead of 32dp?

I'd prefer 32, but I'd like to understand the impact. Can you clarify?

There are 2 arguments for 36dp:
1) https://icons.googleplex.com/#icon=ic_image&search=image have 36dp icons and downscaling to 32dp may cause them to look blurry.
2) Since original icons are 36dp, it is very easy in code to show them as 36 dp. Showing them as 32dp requires additional 10 lines of code (for downscaling).

I *guess* dfalcantara@ feels about 36dp->32dp same way as I would feel about 36dp->35dp (not much visual difference, but requires some effort).

What vitaliii@ said.  There is a difference between scaling from 36->32 instead of from 36->35, though: the math is cleaner for the first case than the second case and results in less fuzziness when trying to figure out which pixels to toss out.

That said, will this matter once you switch to vectors?  You're moving faster on that that I'd expected, and those should be DP agnostic.

> will this matter once you switch to vectors?
No, we will have to scale anyway (unless something breaks under old APIs).

Still the ratio in Downloads Home is 24dp/48dp, while on the NTP it is 32dp/72dp.

> You're moving faster on that that I'd expected
Sorry, I just wanted to address an .apk size regression I introduced (issue 697861).

Vectors _should_ avoid that issue because they're scaled differently; vectors define the curves and paths via formulas that can be adjusted, while regular bitmaps have to deal with interpolation of pre-defined pixels.  You'll probably be able to get by with just one set of vectors for both the Home and NTP cases.

Does this resolve the question? (Switching to vectors)?

I am going to revert vectors CL and use png's instead.

Reason: time to draw the resources increases 10x (measurements are in https://bugs.chromium.org/p/chromium/issues/detail?id=697888#c7), while the size reduction is small (as twellington@ wrote in https://bugs.chromium.org/p/chromium/issues/detail?id=697888#c22). I agree that we should not convert frequently used resources (e.g. on NTP) to xml to save a couple of bytes.

Now that this is being reverted, the question is reopened.  Can the image in app be 36dp given that it is a 36dp asset, or can UX provide a 32dp asset?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ec2749f33bd385686d8d6c522d20317ff00fecdb

commit ec2749f33bd385686d8d6c522d20317ff00fecdb
Author: vitaliii <vitaliii@chromium.org>
Date: Mon Apr 03 06:56:12 2017

Revert "[NTP::Downloads] Replace 36dp ... resources with vector ... ."

This is an exact revert of commit
267b85777ea8bb3859472dc88582e946131a552c
(https://codereview.chromium.org/2731303002).

Reason: The apk size win is not comparable to performance loss on such
frequently used resources.

BUG= 693552 ,697861

Review-Url: https://codereview.chromium.org/2794513002
Cr-Commit-Position: refs/heads/master@{#461378}

[add] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/res/drawable-hdpi/ic_drive_file_white_36dp.png
[add] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/res/drawable-hdpi/ic_drive_site_white_36dp.png
[add] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/res/drawable-hdpi/ic_drive_text_white_36dp.png
[add] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/res/drawable-hdpi/ic_image_white_36dp.png
[add] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/res/drawable-hdpi/ic_music_note_white_36dp.png
[add] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/res/drawable-mdpi/ic_drive_file_white_36dp.png
[add] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/res/drawable-mdpi/ic_drive_site_white_36dp.png
[add] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/res/drawable-mdpi/ic_drive_text_white_36dp.png
[add] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/res/drawable-mdpi/ic_image_white_36dp.png
[add] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/res/drawable-mdpi/ic_music_note_white_36dp.png
[add] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/res/drawable-xhdpi/ic_drive_file_white_36dp.png
[add] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/res/drawable-xhdpi/ic_drive_site_white_36dp.png
[add] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/res/drawable-xhdpi/ic_drive_text_white_36dp.png
[add] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/res/drawable-xhdpi/ic_image_white_36dp.png
[add] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/res/drawable-xhdpi/ic_music_note_white_36dp.png
[add] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/res/drawable-xxhdpi/ic_drive_file_white_36dp.png
[add] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/res/drawable-xxhdpi/ic_drive_site_white_36dp.png
[add] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/res/drawable-xxhdpi/ic_drive_text_white_36dp.png
[add] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/res/drawable-xxhdpi/ic_image_white_36dp.png
[add] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/res/drawable-xxhdpi/ic_music_note_white_36dp.png
[add] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/res/drawable-xxxhdpi/ic_drive_file_white_36dp.png
[add] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/res/drawable-xxxhdpi/ic_drive_site_white_36dp.png
[add] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/res/drawable-xxxhdpi/ic_drive_text_white_36dp.png
[add] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/res/drawable-xxxhdpi/ic_image_white_36dp.png
[add] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/res/drawable-xxxhdpi/ic_music_note_white_36dp.png
[delete] https://crrev.com/f1f078d5f56503e5cf8b5490875e1e1ec9c252ba/chrome/android/java/res/drawable/ic_drive_file_white.xml
[delete] https://crrev.com/f1f078d5f56503e5cf8b5490875e1e1ec9c252ba/chrome/android/java/res/drawable/ic_drive_site_white.xml
[delete] https://crrev.com/f1f078d5f56503e5cf8b5490875e1e1ec9c252ba/chrome/android/java/res/drawable/ic_drive_text_white.xml
[delete] https://crrev.com/f1f078d5f56503e5cf8b5490875e1e1ec9c252ba/chrome/android/java/res/drawable/ic_image_white.xml
[delete] https://crrev.com/f1f078d5f56503e5cf8b5490875e1e1ec9c252ba/chrome/android/java/res/drawable/ic_music_note_white.xml
[delete] https://crrev.com/f1f078d5f56503e5cf8b5490875e1e1ec9c252ba/chrome/android/java/res/drawable/ic_play_arrow_white.xml
[modify] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/res/values/dimens.xml
[modify] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java
[modify] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemView.java
[modify] https://crrev.com/ec2749f33bd385686d8d6c522d20317ff00fecdb/chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/51e2977044591f818e84c297e5dec72c639f465f

commit 51e2977044591f818e84c297e5dec72c639f465f
Author: vitaliii <vitaliii@chromium.org>
Date: Mon Apr 03 11:36:55 2017

[NTP::Downloads] Store file type icon padding as a constant.

Add file type icon padding into dimens.xml, read it once and store as a
member of SnippetArticleViewHolder.

BUG= 693552 

Review-Url: https://codereview.chromium.org/2789133002
Cr-Commit-Position: refs/heads/master@{#461408}

[modify] https://crrev.com/51e2977044591f818e84c297e5dec72c639f465f/chrome/android/java/res/values/dimens.xml
[modify] https://crrev.com/51e2977044591f818e84c297e5dec72c639f465f/chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java

Oh, sorry, I missed that 32 vs 36 dp discussion happens in this bug. Reopened.

Summary:
- we have a 36 dp resource, it's bad to downsize it because it may look blurry (dan, can you confirm this? It looks fine in our screenshots).
- downloads uses 24dp / 48dp square
- we use 32 / 72 - let's go ahead and use 36dp and take the resource from go/icons

Doesn't have anything to do with these assets in particular.  It's that I've gotten lots of grief from UX in the past about using incorrectly sized assets because thin lines would become blurry borders.  UX has always given us the right-sized assets so they could control how the downsampling worked from their end instead of trusting Android to do the right thing.

CL https://codereview.chromium.org/2797933002

See screenshots attached.

Deleted: icon_size_32dp.png 230 KB

	icon_size_32dp.png 230 KB View Download

Deleted: icon_size_36_dp_aligned.png 235 KB

	icon_size_36_dp_aligned.png 235 KB View Download

Deleted: icon_size_36_dp.png 262 KB

	icon_size_36_dp.png 262 KB View Download

Looks good!

Something funny seems to be going on in that second screenshot - something unrelated?

I think the second screenshot was edited so the items are in the same exact place as in the first screenshot. For easier comparison :-)

rachelis@, yes, mvanouwerkerk@ is right. The screenshot has been aligned manually to be compared with the first screenshot. Sorry for not making this obvious.

No worries, thanks Vitalii! I get it now - that's useful. :D

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3fccf108a962616588beaad5adfe2e9cece5ba04

commit 3fccf108a962616588beaad5adfe2e9cece5ba04
Author: vitaliii <vitaliii@chromium.org>
Date: Wed Apr 05 10:28:19 2017

[NTP::Downloads] Show file type icons as 36dp.

UI people agreed to move from 32dp to 36dp.

Reasons:
1) We have 36dp resources;
2) The ratio is consistent with Downloads UI.

BUG= 693552 

Review-Url: https://codereview.chromium.org/2797933002
Cr-Commit-Position: refs/heads/master@{#462027}

[modify] https://crrev.com/3fccf108a962616588beaad5adfe2e9cece5ba04/chrome/android/java/res/values/dimens.xml
[modify] https://crrev.com/3fccf108a962616588beaad5adfe2e9cece5ba04/chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java