[NTP::Downloads] Replace 36dp file type resources with vector drawables.

[NTP::Downloads] Replace 36dp file type resources with vector drawables.

Replace 36dp file type resources added in
https://codereview.chromium.org/2721043002/ with vector drawables. The
new icons are used in both Downloads Home and NTP.

There are other clients of the corresponding 24dp icons and
ic_play_arrow_white_36dp, which I am going to address in the following
CLs.

8889 bytes removed and 2441 bytes added.

BUG=697861
Review-Url: https://codereview.chromium.org/2731303002
Cr-Commit-Position: refs/heads/master@{#455446}
Committed: https://chromium.googlesource.com/chromium/src/+/267b85777ea8bb3859472dc88582e946131a552c

[vitaliii, 2017-03-06 15:48:05]

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

[commit-bot: I haz the power, 2017-03-06 15:48:29]

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-06 15:49:14]

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

[vitaliii, 2017-03-06 15:49:15]

Hi dgn@ & dfalcantara@,

Could you have a look at my CL?
dgn@ -> ntp/snippets
dfalcantara@ -> java/res & browser/download

[commit-bot: I haz the power, 2017-03-06 17:09:15]

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

[commit-bot: I haz the power, 2017-03-06 17:09:16]

Dry run: This issue passed the CQ dry run.

[gone, 2017-03-06 18:09:47]

Can I get some comparison screenshots here?

[vitaliii, 2017-03-06 18:19:20]

On 2017/03/06 18:09:47, dfalcantara (load balance plz) wrote:
> Can I get some comparison screenshots here?

Sure, will do tomorrow. However, my aim was to have no visible difference after
this CL (so screenshots are rather for verification, but not evaluation).

[gone, 2017-03-06 18:23:32]

On 2017/03/06 18:19:20, vitaliii wrote:
> On 2017/03/06 18:09:47, dfalcantara (load balance plz) wrote:
> > Can I get some comparison screenshots here?
> 
> Sure, will do tomorrow. However, my aim was to have no visible difference
after
> this CL (so screenshots are rather for verification, but not evaluation).

Yeah, that's why I was asking ;)  I'll review in the meantime.

[gone, 2017-03-06 18:40:46]

https://codereview.chromium.org/2731303002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemView.java
(right):

https://codereview.chromium.org/2731303002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemView.java:265:
mIconView.setPadding(padding, padding, padding, padding);
I'd prefer that you not propagate that code here, especially because this is a
constant value in both cases.  Define a second value in dimens.xml and set the
padding to that.

[vitaliii, 2017-03-07 09:38:47]

Hi dfalcantara@, 

I've addressed your comment.

For screenshots, please see the bug.

https://codereview.chromium.org/2731303002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemView.java
(right):

https://codereview.chromium.org/2731303002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemView.java:265:
mIconView.setPadding(padding, padding, padding, padding);
On 2017/03/06 18:40:46, dfalcantara (load balance plz) wrote:
> I'd prefer that you not propagate that code here, especially because this is a
> constant value in both cases.  Define a second value in dimens.xml and set the
> padding to that.

Done.

I've made a constant for padding into dimens.xml and removed icon size constant.
This is a bit indirect, so I've added a comment.

[vitaliii, 2017-03-07 09:38:53]

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

[commit-bot: I haz the power, 2017-03-07 09:39:23]

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-03-07 10:12:04]

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

[commit-bot: I haz the power, 2017-03-07 10:12:05]

Dry run: This issue passed the CQ dry run.

[dgn, 2017-03-07 13:17:51]

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

https://codereview.chromium.org/2731303002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:698:
return iconSize == ICON_SIZE_24_DP ? R.drawable.ic_drive_site_white_24dp
what about the 24dp assets? Are they still used somewhere? If not can they be
also deleted?

[vitaliii, 2017-03-07 16:34:50]

Hi dgn@,

I replied to your comment.

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

https://codereview.chromium.org/2731303002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:698:
return iconSize == ICON_SIZE_24_DP ? R.drawable.ic_drive_site_white_24dp
On 2017/03/07 13:17:51, dgn wrote:
> what about the 24dp assets? Are they still used somewhere? If not can they be
> also deleted?

Yes, there is one more user

https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

I was going to address this is a different CL, since I need to ensure that
everything works on their side.

Moreover, there is one user of ic_play_arrow_white_36dp

https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

[dgn, 2017-03-07 17:01:07]

snippets lgtm

[gone, 2017-03-07 17:08:28]

lgtm % comments

https://codereview.chromium.org/2731303002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/values/dimens.xml (right):

https://codereview.chromium.org/2731303002/diff/40001/chrome/android/java/res...
chrome/android/java/res/values/dimens.xml:405: <!-- file_type_icon_size =
downloads_item_icon_size - 2 * padding -->
There's no value called file_type_icon_size; just say that the generic file type
icon size = download_item_icon_size - 2 * padding;

https://codereview.chromium.org/2731303002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemView.java
(right):

https://codereview.chromium.org/2731303002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemView.java:260:
final int padding = mIconView.getResources().getDimensionPixelSize(
This value should be cached as a member variable.  It's reused a lot.

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

https://codereview.chromium.org/2731303002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:301:
final int padding = mThumbnailView.getResources().getDimensionPixelSize(
How often is this value reused?  Should this be cached, as well?

[vitaliii, 2017-03-08 11:51:38]

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

[commit-bot: I haz the power, 2017-03-08 11:51:49]

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-03-08 12:24:08]

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

[commit-bot: I haz the power, 2017-03-08 12:24:09]

Dry run: This issue passed the CQ dry run.

[vitaliii, 2017-03-08 12:36:50]

Hi dfalcantara@,

I've addressed your comments, no need to look.

https://codereview.chromium.org/2731303002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/values/dimens.xml (right):

https://codereview.chromium.org/2731303002/diff/40001/chrome/android/java/res...
chrome/android/java/res/values/dimens.xml:405: <!-- file_type_icon_size =
downloads_item_icon_size - 2 * padding -->
On 2017/03/07 17:08:28, dfalcantara (load balance plz) wrote:
> There's no value called file_type_icon_size; just say that the generic file
type
> icon size = download_item_icon_size - 2 * padding;

Done.

https://codereview.chromium.org/2731303002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemView.java
(right):

https://codereview.chromium.org/2731303002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadItemView.java:260:
final int padding = mIconView.getResources().getDimensionPixelSize(
On 2017/03/07 17:08:28, dfalcantara (load balance plz) wrote:
> This value should be cached as a member variable.  It's reused a lot.

Done.

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

https://codereview.chromium.org/2731303002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:301:
final int padding = mThumbnailView.getResources().getDimensionPixelSize(
On 2017/03/07 17:08:28, dfalcantara (load balance plz) wrote:
> How often is this value reused?  Should this be cached, as well?

Done.

https://codereview.chromium.org/2731303002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:306:
mThumbnailView.setImageDrawable(ApiCompatibilityUtils.getDrawable(
This did not work on Android 4.4.2 (device Micromax S301), the error was about
the resource not found.

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

The CQ bit was checked by vitaliii@chromium.org

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

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/2731303002/#ps80001
(title: "dfalcantara@ comments + fix.")

[commit-bot: I haz the power, 2017-03-08 14: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-03-08 14:06:05]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488981671065380,
"parent_rev": "c87c31da0d21e73e8307b15d84ddeae4216ec7f1", "commit_rev":
"267b85777ea8bb3859472dc88582e946131a552c"}

[commit-bot: I haz the power, 2017-03-08 14:06:51]

Description was changed from

==========
[NTP::Downloads] Replace 36dp file type resources with vector drawables.

Replace 36dp file type resources added in
https://codereview.chromium.org/2721043002/ with vector drawables. The
new icons are used in both Downloads Home and NTP.

There are other clients of the corresponding 24dp icons and
ic_play_arrow_white_36dp, which I am going to address in the following
CLs.

8889 bytes removed and 2441 bytes added.

BUG=697861
==========

to

==========
[NTP::Downloads] Replace 36dp file type resources with vector drawables.

Replace 36dp file type resources added in
https://codereview.chromium.org/2721043002/ with vector drawables. The
new icons are used in both Downloads Home and NTP.

There are other clients of the corresponding 24dp icons and
ic_play_arrow_white_36dp, which I am going to address in the following
CLs.

8889 bytes removed and 2441 bytes added.

BUG=697861

Review-Url: https://codereview.chromium.org/2731303002
Cr-Commit-Position: refs/heads/master@{#455446}
Committed:
https://chromium.googlesource.com/chromium/src/+/267b85777ea8bb3859472dc88582...
==========

[commit-bot: I haz the power, 2017-03-08 14:06:52]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/267b85777ea8bb3859472dc88582...