Make PaymentOption store a Drawable instead of id

Make PaymentOption store a Drawable instead of id

Make PaymentOption store a Drawable instead of id for the icon.
This will allow to use icons not contained in the default resources.

BUG=587995
Committed: https://crrev.com/84b683c8c0e9495462022b214b6f08922e4fd6fb
Cr-Commit-Position: refs/heads/master@{#426596}

[rwlbuis, 2016-10-19 20:50:00]

Description was changed from

==========
Drawable WIP

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

to

==========
Drawable refactor WIP

BUG=587995
==========

[rwlbuis, 2016-10-19 22:20:27]

Patchset #2 (id:20001) has been deleted

[rwlbuis, 2016-10-19 23:55:02]

Description was changed from

==========
Drawable refactor WIP

BUG=587995
==========

to

==========
Make PaymentOption store a Drawable instead of id

BUG=587995
==========

[rwlbuis, 2016-10-19 23:58:03]

Description was changed from

==========
Make PaymentOption store a Drawable instead of id

BUG=587995
==========

to

==========
Make PaymentOption store a Drawable instead of id

Make PaymentOption store a Drawable instead of id for the icon.
This will allow to use icons not contained in the default resources.

BUG=587995
==========

[rwlbuis, 2016-10-20 00:00:48]

rob.buis@samsung.com changed reviewers:
+ rouslan@chromium.org

[rwlbuis, 2016-10-20 00:00:49]

PTAL.

Note that I had to add a card.getIssuerIconDrawableId() < 1 check in
AutofillPaymentInstrument ctor, before I had that, when there were no autofill
credit cards added and I tried to add one in the payment request main dialog,
there would be a runtime assert since resource id 0 was used.

[please use gerrit instead, 2016-10-20 15:22:18]

lgtm % comments below. Please address these comments before submitting.

https://codereview.chromium.org/2436883002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java
(right):

https://codereview.chromium.org/2436883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:49:
card.getIssuerIconDrawableId() < 1
"== 0" should be sufficient instead of "< 1".
https://developer.android.com/reference/android/content/res/Resources.html#ge...
says that only "0" is an invalid value.

https://codereview.chromium.org/2436883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:158:
assert card.getBillingAddressId().equals(billingAddress.getGUID());
Please add this line:

assert card.getIssuerIconDraweableId() != 0;

https://codereview.chromium.org/2436883002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentOption.java
(right):

https://codereview.chromium.org/2436883002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentOption.java:109:
*/
This comment is short enough to collapse into a single line. This helps to read
the code easier.

/** @return The drawable icon for this payment option. */

[rwlbuis, 2016-10-20 17:19:16]

Patchset #3 (id:60001) has been deleted

[rwlbuis, 2016-10-20 17:20:52]

The CQ bit was checked by rob.buis@samsung.com

[rwlbuis, 2016-10-20 17:20:53]

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/2436883002/#ps80001
(title: "Patch for landing")

[commit-bot: I haz the power, 2016-10-20 17:21:23]

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, 2016-10-20 17:51:45]

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

[commit-bot: I haz the power, 2016-10-20 17:51:46]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[rwlbuis, 2016-10-20 18:21:47]

rob.buis@samsung.com changed reviewers:
+ miguelg@chromium.org

[rwlbuis, 2016-10-20 18:21:48]

@miguelg need blessing for the PaymentRequestTestBase.java change.

[rwlbuis, 2016-10-20 19:32:21]

rob.buis@samsung.com changed reviewers:
+ tedchoc@chromium.org

[rwlbuis, 2016-10-20 19:32:22]

@tedchoc you may be closer to my timezone than miguelg :) Could you take a look
at the (trivial) PaymentRequestTestBase.java change?

[please use gerrit instead, 2016-10-20 19:34:05]

On 2016/10/20 19:32:22, rwlbuis wrote:
> @tedchoc you may be closer to my timezone than miguelg :) Could you take a
look
> at the (trivial) PaymentRequestTestBase.java change?

FYI, I own chrome/android/javatests/src/org/chromium/chrome/browser/payments/
now as well, so you don't need Ted's approval anymore.

[please use gerrit instead, 2016-10-20 19:34:19]

The CQ bit was checked by rouslan@chromium.org

[commit-bot: I haz the power, 2016-10-20 19:34:57]

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

[rwlbuis, 2016-10-20 19:47:13]

On 2016/10/20 19:34:05, rouslan wrote:
> FYI, I own chrome/android/javatests/src/org/chromium/chrome/browser/payments/
> now as well, so you don't need Ted's approval anymore.

That makes sense, thanks!

[commit-bot: I haz the power, 2016-10-20 19:52:56]

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

[commit-bot: I haz the power, 2016-10-20 19:52:57]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[please use gerrit instead, 2016-10-20 19:54:19]

Error from presubmit:

* Presubmit ERRORS **
 
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:36:
Wrong order for org.json.JSONObject import. Use :JavaImportOrganize (ECLIM) or
Ctrl+Shift+O (Eclipse) to sort imports. An importorder file for configuring
Eclipse can be found at //tools/android/eclipse/android.importorder.

[rwlbuis, 2016-10-20 20:22:57]

The CQ bit was checked by rob.buis@samsung.com

[rwlbuis, 2016-10-20 20:22:58]

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/2436883002/#ps100001
(title: "Fix presubmit warning")

[commit-bot: I haz the power, 2016-10-20 20:23:42]

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, 2016-10-20 21:08:18]

Description was changed from

==========
Make PaymentOption store a Drawable instead of id

Make PaymentOption store a Drawable instead of id for the icon.
This will allow to use icons not contained in the default resources.

BUG=587995
==========

to

==========
Make PaymentOption store a Drawable instead of id

Make PaymentOption store a Drawable instead of id for the icon.
This will allow to use icons not contained in the default resources.

BUG=587995
==========

[commit-bot: I haz the power, 2016-10-20 21:08:19]

Committed patchset #4 (id:100001)

[Dmitry Skiba, 2016-10-20 22:18:42]

A revert of this CL (patchset #4 id:100001) has been created in
https://codereview.chromium.org/2437223002/ by dskiba@google.com.

The reason for reverting is: This broke internal class (which relied on 'icon'
being int). Please coordinate with rouslan@ to push a fix right after
relanding..

[commit-bot: I haz the power, 2016-10-21 13:22:28]

Description was changed from

==========
Make PaymentOption store a Drawable instead of id

Make PaymentOption store a Drawable instead of id for the icon.
This will allow to use icons not contained in the default resources.

BUG=587995
==========

to

==========
Make PaymentOption store a Drawable instead of id

Make PaymentOption store a Drawable instead of id for the icon.
This will allow to use icons not contained in the default resources.

BUG=587995

Committed: https://crrev.com/84b683c8c0e9495462022b214b6f08922e4fd6fb
Cr-Commit-Position: refs/heads/master@{#426596}
==========

[commit-bot: I haz the power, 2016-10-21 13:22:29]

Patchset 4 (id:??) landed as
https://crrev.com/84b683c8c0e9495462022b214b6f08922e4fd6fb
Cr-Commit-Position: refs/heads/master@{#426596}