[Payment Request] Keep selection for edited item when canceling editors

[Payment Request] Keep selection for edited item when canceling editors

Previously, the various editors would return |null| to the caller
(PaymentRequestImpl) in the case of a cancel. We are now returning the
original item that was passed in for edit.

BUG=672996
TEST=PaymentRequest*Test*

Committed: https://crrev.com/7f8a29ea00d9ab273a33a42c77d4afe13a8707ac
Cr-Commit-Position: refs/heads/master@{#437723}

[please use gerrit instead, 2016-12-09 17:08:22]

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

[please use gerrit instead, 2016-12-09 17:08:23]

https://codereview.chromium.org/2561373002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestContactDetailsTest.java
(right):

https://codereview.chromium.org/2561373002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestContactDetailsTest.java:153:
mReadyToEdit.waitForCallback(callCount);
You're clicking on the close_button before the editor UI has shown up. You can't
do it "quickly", as your comment indicates, unfortunately. Doing things
"quickly" is only possible for UI elements that are on the screen at the same
time. The pencil icon is not on the same UI screen as the close_button. Split
this operation into two steps:

int callCount = mReadyToEdit.getCallCount();
ThreadUtils.runOnUiThreadBlocking(new Runnable() {
    @Override
        mUI.getContactDetailsSectionForTest()
                .findViewById(R.id.payments_open_editor_pencil_button)
                .performClick();
    }
});
mReadyToEdit.waitForCallback(callCount);

callCount = mReadyForInput.getCallCount();
ThreadUtils.runOnUiThreadBlocking(new Runnable() {
    @Override
        mUI.getDialogForTest().findViewById(R.id.close_button).performClick();
    }
});
mReadyForInput.waitForCallback(callCount);

[Mathieu, 2016-12-09 20:33:37]

Description was changed from

==========
[Payment Request] Keep selection for edited item when canceling editors

BUG=None
==========

to

==========
[Payment Request] Keep selection for edited item when canceling editors

BUG=None
TEST=PaymentRequest*Test*
==========

[Mathieu, 2016-12-09 20:33:37]

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

[Mathieu, 2016-12-09 20:33:51]

Patchset #2 (id:20001) has been deleted

[Mathieu, 2016-12-09 20:33:58]

Patchset #1 (id:1) has been deleted

[Mathieu, 2016-12-09 20:35:13]

Hi Rouslan, PTAL Payment Request stuff
Hi Ian, PTAL the change in ids.xml

Thanks!

[Mathieu, 2016-12-09 20:42:50]

Description was changed from

==========
[Payment Request] Keep selection for edited item when canceling editors

BUG=None
TEST=PaymentRequest*Test*
==========

to

==========
[Payment Request] Keep selection for edited item when canceling editors

Previously, the various editors would return |null| to the caller
(PaymentRequestImpl) in the case of a cancel. We are now returning the original
item that was passed in for edit.

BUG=None
TEST=PaymentRequest*Test*
==========

[Mathieu, 2016-12-09 20:42:59]

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

[commit-bot: I haz the power, 2016-12-09 20:44:23]

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

[please use gerrit instead, 2016-12-09 20:58:42]

1) Please file a bug.

2) Please attach a video of the fix to the bug.

3) Can you look into removing the NO_SELECTION clause here? 

if (completeCard == null) {
   
mPaymentMethodsSection.setSelectedItemIndex(SectionInformation.NO_SELECTION);
} else if (toEdit == null) {
    mPaymentMethodsSection.addAndSelectItem(completeCard);
}

After this patch, completeCard will be null only if the user clicks "+ ADD CARD"
at the bottom of the "Payment" section and then "Cancel" in the editor.
Currently this sequence of user operations will deselect the current element in
UI. It would be best to extend your fix to "+ ADD CARD" as well and not alter
the selection here.

This can be done in a follow up patch, because your patch is net positive.

Places where NO_SELECTION thing happens:

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

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

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

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

https://codereview.chromium.org/2561373002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/ContactEditor.java:139:
// If the user clicks [Cancel], send |toEdit| address back to the caller, which
was the
s/address/contact

[Mathieu, 2016-12-09 21:09:11]

Description was changed from

==========
[Payment Request] Keep selection for edited item when canceling editors

Previously, the various editors would return |null| to the caller
(PaymentRequestImpl) in the case of a cancel. We are now returning the original
item that was passed in for edit.

BUG=None
TEST=PaymentRequest*Test*
==========

to

==========
[Payment Request] Keep selection for edited item when canceling editors

Previously, the various editors would return |null| to the caller
(PaymentRequestImpl) in the case of a cancel. We are now returning the original
item that was passed in for edit.

BUG=672996
TEST=PaymentRequest*Test*
==========

[commit-bot: I haz the power, 2016-12-09 21:25:21]

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

[commit-bot: I haz the power, 2016-12-09 21:25:21]

Dry run: This issue passed the CQ dry run.

[Mathieu, 2016-12-09 21:38:31]

Done PTAL.

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

https://codereview.chromium.org/2561373002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/ContactEditor.java:139:
// If the user clicks [Cancel], send |toEdit| address back to the caller, which
was the
On 2016/12/09 20:58:42, rouslan wrote:
> s/address/contact

Done.

[please use gerrit instead, 2016-12-09 21:39:09]

Description was changed from

==========
[Payment Request] Keep selection for edited item when canceling editors

Previously, the various editors would return |null| to the caller
(PaymentRequestImpl) in the case of a cancel. We are now returning the original
item that was passed in for edit.

BUG=672996
TEST=PaymentRequest*Test*
==========

to

==========
[Payment Request] Keep selection for edited item when canceling editors

Previously, the various editors would return |null| to the caller
(PaymentRequestImpl) in the case of a cancel. We are now returning the
original item that was passed in for edit.

BUG=672996
TEST=PaymentRequest*Test*
==========

[please use gerrit instead, 2016-12-09 21:45:59]

lgtm

[Ian Wen, 2016-12-09 22:17:54]

https://codereview.chromium.org/2561373002/diff/60001/chrome/android/java/res...
File chrome/android/java/res/values/ids.xml (right):

https://codereview.chromium.org/2561373002/diff/60001/chrome/android/java/res...
chrome/android/java/res/values/ids.xml:70: <item type="id"
name="payments_open_editor_pencil_button" />
Instead of giving it an ID at runtime, why not declare it in
payment_option_edit_icon.xml? This way you don't need to set it in Java.

[Ian Wen, 2016-12-09 22:31:24]

lgtm after the previous comment is addressed.

[Mathieu, 2016-12-09 22:57:01]

Patchset #3 (id:80001) has been deleted

[Mathieu, 2016-12-09 22:57:10]

Thanks!

https://codereview.chromium.org/2561373002/diff/60001/chrome/android/java/res...
File chrome/android/java/res/values/ids.xml (right):

https://codereview.chromium.org/2561373002/diff/60001/chrome/android/java/res...
chrome/android/java/res/values/ids.xml:70: <item type="id"
name="payments_open_editor_pencil_button" />
On 2016/12/09 22:17:53, Ian Wen wrote:
> Instead of giving it an ID at runtime, why not declare it in
> payment_option_edit_icon.xml? This way you don't need to set it in Java.

Good idea, done

[Mathieu, 2016-12-09 23:00:55]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2016-12-09 23:00:55]

The patchset sent to the CQ was uploaded after l-g-t-m from ianwen@chromium.org,
rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/2561373002/#ps100001
(title: "move id to xml")

[commit-bot: I haz the power, 2016-12-09 23:01:33]

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-12-10 01:05:27]

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

[commit-bot: I haz the power, 2016-12-10 01:05:28]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[Mathieu, 2016-12-10 01:43:43]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2016-12-10 01:44:41]

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-12-10 02:06:24]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481334223823510,
"parent_rev": "48ba79ddd54a34835c2151d578beb58b9c053275", "commit_rev":
"fc8a21408f06870c7b79715ffdba7beb237fcb45"}

[commit-bot: I haz the power, 2016-12-10 02:06:50]

Description was changed from

==========
[Payment Request] Keep selection for edited item when canceling editors

Previously, the various editors would return |null| to the caller
(PaymentRequestImpl) in the case of a cancel. We are now returning the
original item that was passed in for edit.

BUG=672996
TEST=PaymentRequest*Test*
==========

to

==========
[Payment Request] Keep selection for edited item when canceling editors

Previously, the various editors would return |null| to the caller
(PaymentRequestImpl) in the case of a cancel. We are now returning the
original item that was passed in for edit.

BUG=672996
TEST=PaymentRequest*Test*

Review-Url: https://codereview.chromium.org/2561373002
==========

[commit-bot: I haz the power, 2016-12-10 02:06:51]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2016-12-12 15:05:20]

Description was changed from

==========
[Payment Request] Keep selection for edited item when canceling editors

Previously, the various editors would return |null| to the caller
(PaymentRequestImpl) in the case of a cancel. We are now returning the
original item that was passed in for edit.

BUG=672996
TEST=PaymentRequest*Test*

Review-Url: https://codereview.chromium.org/2561373002
==========

to

==========
[Payment Request] Keep selection for edited item when canceling editors

Previously, the various editors would return |null| to the caller
(PaymentRequestImpl) in the case of a cancel. We are now returning the
original item that was passed in for edit.

BUG=672996
TEST=PaymentRequest*Test*

Committed: https://crrev.com/7f8a29ea00d9ab273a33a42c77d4afe13a8707ac
Cr-Commit-Position: refs/heads/master@{#437723}
==========

[commit-bot: I haz the power, 2016-12-12 15:05:22]

Patchset 3 (id:??) landed as
https://crrev.com/7f8a29ea00d9ab273a33a42c77d4afe13a8707ac
Cr-Commit-Position: refs/heads/master@{#437723}