[Payments] Use Autofill assets on Android Payment Request

[Payments] Use Autofill assets on Android Payment Request

Previously there were many PNGs for the cards in Payment Request. Now
this code uses the Autofill assets, which are vector drawables.

Additionally:
* MIR, JCB and Diners cards are now a drawable in Autofill
(previously PNG or generic icon on Android)
* Fixed a few other drawables.
* Removing the border from the PR icons because it's now built-in the asset
* Using setImageDrawable because it appears to be more efficient

BUG=722651
TEST=visual

Review-Url: https://codereview.chromium.org/2888803003
Cr-Commit-Position: refs/heads/master@{#473908}
Committed: https://chromium.googlesource.com/chromium/src/+/10cefa4851de9c468e6527d92205640e738f4890

[Mathieu, 2017-05-17 18:14:00]

Description was changed from

==========
[Payments] Use Autofill assets on Android Payment Request

Previous there were many PNGs for the cards in Payment Request. Now
this code uses the Autofill assets, which are vector drawables.

Additionally:
* MIR card is now a drawable in Autofill.
* Fixed a few other drawables.
* Using setImageDrawable because it appears to be more efficient

BUG=722651
TEST=visual
==========

to

==========
[Payments] Use Autofill assets on Android Payment Request

Previously there were many PNGs for the cards in Payment Request. Now
this code uses the Autofill assets, which are vector drawables.

Additionally:
* MIR card is now a drawable in Autofill (previously PNG on Android)
* Fixed a few other drawables.
* Using setImageDrawable because it appears to be more efficient

BUG=722651
TEST=visual
==========

[Mathieu, 2017-05-17 18:14:00]

mathp@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Mathieu, 2017-05-17 18:14:52]

Description was changed from

==========
[Payments] Use Autofill assets on Android Payment Request

Previously there were many PNGs for the cards in Payment Request. Now
this code uses the Autofill assets, which are vector drawables.

Additionally:
* MIR card is now a drawable in Autofill (previously PNG on Android)
* Fixed a few other drawables.
* Using setImageDrawable because it appears to be more efficient

BUG=722651
TEST=visual
==========

to

==========
[Payments] Use Autofill assets on Android Payment Request

Previously there were many PNGs for the cards in Payment Request. Now
this code uses the Autofill assets, which are vector drawables.

Additionally:
* MIR, JCB and Diners cards are now a drawable in Autofill
(previously PNG on Android)
* Fixed a few other drawables.
* Using setImageDrawable because it appears to be more efficient

BUG=722651
TEST=visual
==========

[Mathieu, 2017-05-17 18:19:17]

Description was changed from

==========
[Payments] Use Autofill assets on Android Payment Request

Previously there were many PNGs for the cards in Payment Request. Now
this code uses the Autofill assets, which are vector drawables.

Additionally:
* MIR, JCB and Diners cards are now a drawable in Autofill
(previously PNG on Android)
* Fixed a few other drawables.
* Using setImageDrawable because it appears to be more efficient

BUG=722651
TEST=visual
==========

to

==========
[Payments] Use Autofill assets on Android Payment Request

Previously there were many PNGs for the cards in Payment Request. Now
this code uses the Autofill assets, which are vector drawables.

Additionally:
* MIR, JCB and Diners cards are now a drawable in Autofill
(previously PNG on Android)
* Fixed a few other drawables.
* Removing the border from the PR icons because it's now built-in the asset
* Using setImageDrawable because it appears to be more efficient

BUG=722651
TEST=visual
==========

[Mathieu, 2017-05-17 18:19:18]

mathp@chromium.org changed reviewers:
+ rouslan@chromium.org

[Mathieu, 2017-05-17 18:20:19]

Rouslan, PTAL whole patch

Ted, I have a question about setImageDrawable. Should we use it, and am I doing
the right thing with the AppCompatResources.getDrawable call?

Thanks

[Mathieu, 2017-05-17 18:21:47]

Description was changed from

==========
[Payments] Use Autofill assets on Android Payment Request

Previously there were many PNGs for the cards in Payment Request. Now
this code uses the Autofill assets, which are vector drawables.

Additionally:
* MIR, JCB and Diners cards are now a drawable in Autofill
(previously PNG on Android)
* Fixed a few other drawables.
* Removing the border from the PR icons because it's now built-in the asset
* Using setImageDrawable because it appears to be more efficient

BUG=722651
TEST=visual
==========

to

==========
[Payments] Use Autofill assets on Android Payment Request

Previously there were many PNGs for the cards in Payment Request. Now
this code uses the Autofill assets, which are vector drawables.

Additionally:
* MIR, JCB and Diners cards are now a drawable in Autofill
(previously PNG or generic icon on Android)
* Fixed a few other drawables.
* Removing the border from the PR icons because it's now built-in the asset
* Using setImageDrawable because it appears to be more efficient

BUG=722651
TEST=visual
==========

[Mathieu, 2017-05-17 19:20:28]

Added screenshots on the bug

[Ted C, 2017-05-17 19:41:23]

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

[Ted C, 2017-05-17 19:41:24]

+twellington

AppCompatResources.getDrawable is the right thing to handle inflating vectors
(and other types).

One question I have is about the diner's club asset.  The images look to be
~18K, but the vector is 60K+.  That was with some quick checks, but if the
vector actually increases the size, we shouldn't make the switch to that.

[Mathieu, 2017-05-17 19:46:57]

On 2017/05/17 19:41:24, Ted C wrote:
> +twellington
> 
> AppCompatResources.getDrawable is the right thing to handle inflating vectors
> (and other types).
> 
> One question I have is about the diner's club asset.  The images look to be
> ~18K, but the vector is 60K+.  That was with some quick checks, but if the
> vector actually increases the size, we shouldn't make the switch to that.

Thanks, my mistake for assuming vectors were always lower-size. I will double
check others and update this CL.

[please use gerrit instead, 2017-05-17 19:56:34]

lgtm % comment

https://codereview.chromium.org/2888803003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorTextField.java
(right):

https://codereview.chromium.org/2888803003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorTextField.java:275:
mValueIcon.setImageDrawable(AppCompatResources.getDrawable(mContext,
mValueIconId));
You can use getContext(), so no need to store mContext.

[Mathieu, 2017-05-17 20:00:41]

Thanks, still need to investigate diners club and others as mentioned.

https://codereview.chromium.org/2888803003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorTextField.java
(right):

https://codereview.chromium.org/2888803003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorTextField.java:275:
mValueIcon.setImageDrawable(AppCompatResources.getDrawable(mContext,
mValueIconId));
On 2017/05/17 19:56:33, ಠ_ಠ wrote:
> You can use getContext(), so no need to store mContext.

Done.

[Mathieu, 2017-05-17 21:08:39]

I've updated diners and jcb to be PNG because their PNGs are smaller than the
XML file (thanks). 

However, since I had to create new PNG assets (and pngcrusht them), I'm finding
a size improvement in the new assets when compared to pr_diners and pr_jcb PNGs
from before. Therefore, I'll have to expand my analysis to the other 6 cards by
generating new PNGs and comparing with the XML size. 

All of this to say, this is not ready yet :)

[Mathieu, 2017-05-18 02:51:18]

Hi all, the result of my investigation into the remaining 6 card icons is at
https://pastebin.com/wBSe4Tdf

Compare the XML size with the sum of all the PNGs, and see that amex, unionpay
and mastercard are better off as PNGs. Hence I've added those assets to the CL
and remove their vector representations.

Overall going back to the whole CL, this is a width/height reduction from where
we started with the PR-specific assets (56x36dp) to the Autofill assets that we
reuse here (32x20dp). I can take final before/after screenshots tomorrow, but
since we were never displaying at 56dp in the PR UI, it shouldn't be a problem.

PTAL

[Mathieu, 2017-05-18 02:57:54]

On 2017/05/18 02:51:18, Mathieu wrote:
> Hi all, the result of my investigation into the remaining 6 card icons is at
> https://pastebin.com/wBSe4Tdf
> 
> Compare the XML size with the sum of all the PNGs, and see that amex, unionpay
> and mastercard are better off as PNGs. Hence I've added those assets to the CL
> and remove their vector representations.
> 
> Overall going back to the whole CL, this is a width/height reduction from
where
> we started with the PR-specific assets (56x36dp) to the Autofill assets that
we
> reuse here (32x20dp). I can take final before/after screenshots tomorrow, but
> since we were never displaying at 56dp in the PR UI, it shouldn't be a
problem.
> 
> PTAL

I have to say, we do lose flexibility when using fixed-size PNGs, but if we ever
get in a situation where a design requires icons say twice as big, we can always
migrate to vectors for all icons.

[Evan Stade, 2017-05-18 14:39:29]

On 2017/05/18 02:51:18, Mathieu wrote:
> Hi all, the result of my investigation into the remaining 6 card icons is at
> https://pastebin.com/wBSe4Tdf
> 
> Compare the XML size with the sum of all the PNGs, and see that amex, unionpay
> and mastercard are better off as PNGs. Hence I've added those assets to the CL
> and remove their vector representations.
> 

Is this a fair comparison? I'd assume the drawable XML is compressed at some
point in the build process, whereas PNGs are already compressed as small as
they'll go.

[Ted C, 2017-05-18 14:47:02]

tedchoc@chromium.org changed reviewers:
+ agrieve@chromium.org

[Ted C, 2017-05-18 14:47:03]

+agrieve

the xml should get compressed when it is packaged into the APK.  I think
we probably would want to look at the APK size deltas between the two
to get a fairer comparison (as I too would expect XML to compress
decently nicely due to the repeated tags)

Although I don't know if there is some intermediate output of the build process
that would give a clearer signal (adding agrieve for that)

[Mathieu, 2017-05-18 19:51:20]

On 2017/05/18 14:47:03, Ted C wrote:
> +agrieve
> 
> the xml should get compressed when it is packaged into the APK.  I think
> we probably would want to look at the APK size deltas between the two
> to get a fairer comparison (as I too would expect XML to compress
> decently nicely due to the repeated tags)
> 
> Although I don't know if there is some intermediate output of the build
process
> that would give a clearer signal (adding agrieve for that)

Thanks. 

Andrew and Ted, is there some guidance on how I should carry this analysis? for
each asset, calculate the binary size as PNG and as vector?
Should I use the tool that Andrew sent out to chromium-dev
(https://chromium.googlesource.com/chromium/src.git/+/master/tools/binary_size...

tools/binary_size/supersize archive chrome.size --apk-file
out/Release/apks/ChromePublic.apk -v

[agrieve, 2017-05-18 19:56:04]

On 2017/05/18 19:51:20, Mathieu wrote:
> On 2017/05/18 14:47:03, Ted C wrote:
> > +agrieve
> > 
> > the xml should get compressed when it is packaged into the APK.  I think
> > we probably would want to look at the APK size deltas between the two
> > to get a fairer comparison (as I too would expect XML to compress
> > decently nicely due to the repeated tags)
> > 
> > Although I don't know if there is some intermediate output of the build
> process
> > that would give a clearer signal (adding agrieve for that)
> 
> Thanks. 
> 
> Andrew and Ted, is there some guidance on how I should carry this analysis?
for
> each asset, calculate the binary size as PNG and as vector?
> Should I use the tool that Andrew sent out to chromium-dev
>
(https://chromium.googlesource.com/chromium/src.git/+/master/tools/binary_size...)
> 
> tools/binary_size/supersize archive chrome.size --apk-file
> out/Release/apks/ChromePublic.apk -v

supersize just tracks native code size atm, so not the right tool here.

To see which is actually smaller, the best way is to just build Chrome.apk with
them as pngs, then build again with them as vectors. The vector drawables will
be converted into binary xml, so the file size does get smaller when run through
aapt. Use "unzip -lv apks/ChromePublic.apk" to see how large the files are once
put into the apk.

[Mathieu, 2017-05-18 20:19:03]

On 2017/05/18 19:56:04, agrieve wrote:
> On 2017/05/18 19:51:20, Mathieu wrote:
> > On 2017/05/18 14:47:03, Ted C wrote:
> > > +agrieve
> > > 
> > > the xml should get compressed when it is packaged into the APK.  I think
> > > we probably would want to look at the APK size deltas between the two
> > > to get a fairer comparison (as I too would expect XML to compress
> > > decently nicely due to the repeated tags)
> > > 
> > > Although I don't know if there is some intermediate output of the build
> > process
> > > that would give a clearer signal (adding agrieve for that)
> > 
> > Thanks. 
> > 
> > Andrew and Ted, is there some guidance on how I should carry this analysis?
> for
> > each asset, calculate the binary size as PNG and as vector?
> > Should I use the tool that Andrew sent out to chromium-dev
> >
>
(https://chromium.googlesource.com/chromium/src.git/+/master/tools/binary_size...)
> > 
> > tools/binary_size/supersize archive chrome.size --apk-file
> > out/Release/apks/ChromePublic.apk -v
> 
> supersize just tracks native code size atm, so not the right tool here.
> 
> To see which is actually smaller, the best way is to just build Chrome.apk
with
> them as pngs, then build again with them as vectors. The vector drawables will
> be converted into binary xml, so the file size does get smaller when run
through
> aapt. Use "unzip -lv apks/ChromePublic.apk" to see how large the files are
once
> put into the apk.

Thanks! I'll have to do this for each asset as XML varies a lot by the design of
the card icon.

[Mathieu, 2017-05-19 00:47:34]

Patchset #5 (id:80001) has been deleted

[Mathieu, 2017-05-19 00:51:32]

On 2017/05/18 20:19:03, Mathieu wrote:
> On 2017/05/18 19:56:04, agrieve wrote:
> > On 2017/05/18 19:51:20, Mathieu wrote:
> > > On 2017/05/18 14:47:03, Ted C wrote:
> > > > +agrieve
> > > > 
> > > > the xml should get compressed when it is packaged into the APK.  I think
> > > > we probably would want to look at the APK size deltas between the two
> > > > to get a fairer comparison (as I too would expect XML to compress
> > > > decently nicely due to the repeated tags)
> > > > 
> > > > Although I don't know if there is some intermediate output of the build
> > > process
> > > > that would give a clearer signal (adding agrieve for that)
> > > 
> > > Thanks. 
> > > 
> > > Andrew and Ted, is there some guidance on how I should carry this
analysis?
> > for
> > > each asset, calculate the binary size as PNG and as vector?
> > > Should I use the tool that Andrew sent out to chromium-dev
> > >
> >
>
(https://chromium.googlesource.com/chromium/src.git/+/master/tools/binary_size...)
> > > 
> > > tools/binary_size/supersize archive chrome.size --apk-file
> > > out/Release/apks/ChromePublic.apk -v
> > 
> > supersize just tracks native code size atm, so not the right tool here.
> > 
> > To see which is actually smaller, the best way is to just build Chrome.apk
> with
> > them as pngs, then build again with them as vectors. The vector drawables
will
> > be converted into binary xml, so the file size does get smaller when run
> through
> > aapt. Use "unzip -lv apks/ChromePublic.apk" to see how large the files are
> once
> > put into the apk.
> 
> Thanks! I'll have to do this for each asset as XML varies a lot by the design
of
> the card icon.

Hi folks, the result of the analysis are here:

XML compressed sizes: https://pastebin.com/nSRmyUaL
crushed PNG sizes can be gathered from my previous link:
https://pastebin.com/wBSe4Tdf

Compression is pretty high, so XML wins in all cases except JCB and Diners,
which are 25K and 25K for the compressed XML and 10K and 6K for the PNGs. 

I've deleted the last patchset, which leaves everything as XML except JCB and
Diners.

PTAL

[Ted C, 2017-05-19 18:49:37]

lgtm...thanks for investigating the xml file.  at least my original diner's club
comment was a true red herring :-/

[Mathieu, 2017-05-19 19:52:46]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2017-05-19 19:52:47]

The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/2888803003/#ps60001
(title: "update diners and jcb")

[commit-bot: I haz the power, 2017-05-19 19:53:48]

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-05-19 21:25:02]

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

[commit-bot: I haz the power, 2017-05-19 21:25:04]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Mathieu, 2017-05-21 13:20:05]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2017-05-21 13:20:18]

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-05-21 15:20:31]

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

[commit-bot: I haz the power, 2017-05-21 15:20:32]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Mathieu, 2017-05-23 14:34:43]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2017-05-23 14:34:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/2888803003/#ps120001
(title: "AppCompatResources in more places")

[commit-bot: I haz the power, 2017-05-23 14:35: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-05-23 15:26:48]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495550083419420,
"parent_rev": "ec5db40750a7241b6615f21734e6cd9c34fe42e3", "commit_rev":
"10cefa4851de9c468e6527d92205640e738f4890"}

[commit-bot: I haz the power, 2017-05-23 15:27:03]

Description was changed from

==========
[Payments] Use Autofill assets on Android Payment Request

Previously there were many PNGs for the cards in Payment Request. Now
this code uses the Autofill assets, which are vector drawables.

Additionally:
* MIR, JCB and Diners cards are now a drawable in Autofill
(previously PNG or generic icon on Android)
* Fixed a few other drawables.
* Removing the border from the PR icons because it's now built-in the asset
* Using setImageDrawable because it appears to be more efficient

BUG=722651
TEST=visual
==========

to

==========
[Payments] Use Autofill assets on Android Payment Request

Previously there were many PNGs for the cards in Payment Request. Now
this code uses the Autofill assets, which are vector drawables.

Additionally:
* MIR, JCB and Diners cards are now a drawable in Autofill
(previously PNG or generic icon on Android)
* Fixed a few other drawables.
* Removing the border from the PR icons because it's now built-in the asset
* Using setImageDrawable because it appears to be more efficient

BUG=722651
TEST=visual

Review-Url: https://codereview.chromium.org/2888803003
Cr-Commit-Position: refs/heads/master@{#473908}
Committed:
https://chromium.googlesource.com/chromium/src/+/10cefa4851de9c468e6527d92205...
==========

[commit-bot: I haz the power, 2017-05-23 15:27:05]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/10cefa4851de9c468e6527d92205...

[estevenson, 2017-05-25 15:25:52]

On 2017/05/23 15:27:05, commit-bot: I haz the power wrote:
> Committed patchset #6 (id:120001) as
>
https://chromium.googlesource.com/chromium/src/+/10cefa4851de9c468e6527d92205...

Thanks for looking into this!

Shaved 200kb off of MonochromePublic.apk!

https://chromeperf.appspot.com/report?sid=99caa6c5389f77e13b562afa5703ad8fc1d...

[Mathieu, 2017-05-25 15:35:12]

On 2017/05/25 15:25:52, estevenson wrote:
> On 2017/05/23 15:27:05, commit-bot: I haz the power wrote:
> > Committed patchset #6 (id:120001) as
> >
>
https://chromium.googlesource.com/chromium/src/+/10cefa4851de9c468e6527d92205...
> 
> Thanks for looking into this!
> 
> Shaved 200kb off of MonochromePublic.apk!
> 
>
https://chromeperf.appspot.com/report?sid=99caa6c5389f77e13b562afa5703ad8fc1d...

I'm glad :D