Add "Checking" spinner for server-side shipping address validation.

Add "Checking" spinner for server-side shipping address validation.

PaymentRequest should  let the user know that their section is being
verified. This is used, for example, when the website is verifying
availability of shipping options to the shipping address that the user
has selected. If an address is not supported, an error message should be
shown. The unsupported address should be disabled.

BUG=619318
Committed: https://crrev.com/fbb096bf49b5a758a14d487370cf1ea8c0ea9519
Cr-Commit-Position: refs/heads/master@{#399612}

[please use gerrit instead, 2016-06-12 22:40:23]

Description was changed from

==========
PaymentRequestImpl side of "Checking" spinner implementation.

This is the first part of the "Checking" spinner implementation in
PaymentRequest, which changes PaymentRequestImpl to let PaymentRequestUI
know that the user selection needs to be checked before it can be
accepted.

BUG=619318
==========

to

==========
Add "Checking" spinner for server-side shipping address validation.

BUG=619318
==========

[please use gerrit instead, 2016-06-12 22:41:39]

Description was changed from

==========
Add "Checking" spinner for server-side shipping address validation.

BUG=619318
==========

to

==========
Add "Checking" spinner for server-side shipping address validation.

PaymentRequest should  let the user know that their section is being verified.
This is used, for example, when the website is verifying availability of
shipping options to the shipping address that the user has selected. If an
address is not supported, an error message should be shown. The unsupported
address should be disabled.

BUG=619318
==========

[please use gerrit instead, 2016-06-12 22:44:32]

Description was changed from

==========
Add "Checking" spinner for server-side shipping address validation.

PaymentRequest should  let the user know that their section is being verified.
This is used, for example, when the website is verifying availability of
shipping options to the shipping address that the user has selected. If an
address is not supported, an error message should be shown. The unsupported
address should be disabled.

BUG=619318
==========

to

==========
Add "Checking" spinner for server-side shipping address validation.

PaymentRequest should  let the user know that their section is being
verified. This is used, for example, when the website is verifying
availability of shipping options to the shipping address that the user
has selected. If an address is not supported, an error message should be
shown. The unsupported address should be disabled.

BUG=619318
==========

[please use gerrit instead, 2016-06-12 22:45:04]

Patchset #1 (id:1) has been deleted

[please use gerrit instead, 2016-06-12 23:04:06]

Patchset #1 (id:20001) has been deleted

[please use gerrit instead, 2016-06-13 19:03:46]

Patchset #1 (id:40001) has been deleted

[please use gerrit instead, 2016-06-13 19:03:56]

Patchset #1 (id:60001) has been deleted

[please use gerrit instead, 2016-06-13 19:04:00]

Patchset #1 (id:80001) has been deleted

[please use gerrit instead, 2016-06-13 19:42:38]

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

[please use gerrit instead, 2016-06-13 19:42:39]

dfalcantara@, ptal. This is close to the mocks, but not exactly. (We don't have
redlines yet.) Follow up CLs can polish the exact look, but the functionality is
all here.

[gone, 2016-06-13 20:34:04]

https://codereview.chromium.org/2059073002/diff/100001/chrome/android/java/re...
File chrome/android/java/res/values/colors.xml (right):

https://codereview.chromium.org/2059073002/diff/100001/chrome/android/java/re...
chrome/android/java/res/values/colors.xml:20: <color
name="disabled_text_color">#999</color>
Where'd the #999 come from?  Seems like this is something that should be defined
elsewhere, already, or that we should at least use a close and already-existing
gray like "explanation_text_color".

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

https://codereview.chromium.org/2059073002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentOption.java:21:
private boolean mValid = true;
mIsValid

https://codereview.chromium.org/2059073002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java
(right):

https://codereview.chromium.org/2059073002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:174:
boolean expanded =
isExpanded

https://codereview.chromium.org/2059073002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:557:
private View createButton(GridLayout parent, int rowIndex, boolean isSelected,
Everything after and including "GridLayout parent" on next line like the
OptionRow constructor

https://codereview.chromium.org/2059073002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:724:
mOptionsContainer = new RelativeLayout(getContext());
No point in using an expensive RelativeLayout if they're just sitting on top of
each other.  Use FrameLayout or ViewSwitcher instead and assign the children a
center layout_gravity.

https://codereview.chromium.org/2059073002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java
(right):

https://codereview.chromium.org/2059073002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:509:
option, callback);
Feels like you should only pass a real callback in if you're actually expecting
something to happen and null otherwise.

https://codereview.chromium.org/2059073002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:783:
public boolean isAdditionalTextWarning(OptionSection section) {
Maybe isAdditionalTextDisplayingWarning?

https://codereview.chromium.org/2059073002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:786:
== SectionInformation.INVALID_SELECTION;
I think this requires a double indentation on the second equality because it's
continuing
the condition on the previous line.

Could avoid the issue entirely by making a temporary boolean for it.

[gone, 2016-06-13 20:34:50]

Alternatively, I'm fine with landing the functionality and then iterating on the
layout from my side.  I think we'd both move faster with this CL in because it'd
have the piping in.

[please use gerrit instead, 2016-06-13 21:59:08]

dfallcanara@, ptal patch 2.

On 2016/06/13 20:34:50, dfalcantara wrote:
> Alternatively, I'm fine with landing the functionality and then iterating on
the
> layout from my side.  I think we'd both move faster with this CL in because
it'd
> have the piping in.

Agreed.

https://codereview.chromium.org/2059073002/diff/100001/chrome/android/java/re...
File chrome/android/java/res/values/colors.xml (right):

https://codereview.chromium.org/2059073002/diff/100001/chrome/android/java/re...
chrome/android/java/res/values/colors.xml:20: <color
name="disabled_text_color">#999</color>
On 2016/06/13 20:34:03, dfalcantara wrote:
> Where'd the #999 come from?

I picked it because it looks good. No UX guidance on color yet.

>  Seems like this is something that should be defined
> elsewhere, already, or that we should at least use a close and
already-existing
> gray like "explanation_text_color".

OK, using #909090 from explanation_text_color.

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

https://codereview.chromium.org/2059073002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentOption.java:21:
private boolean mValid = true;
On 2016/06/13 20:34:03, dfalcantara wrote:
> mIsValid

Done.

https://codereview.chromium.org/2059073002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java
(right):

https://codereview.chromium.org/2059073002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:174:
boolean expanded =
On 2016/06/13 20:34:03, dfalcantara wrote:
> isExpanded

Done.

https://codereview.chromium.org/2059073002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:557:
private View createButton(GridLayout parent, int rowIndex, boolean isSelected,
On 2016/06/13 20:34:03, dfalcantara wrote:
> Everything after and including "GridLayout parent" on next line like the
> OptionRow constructor

Done.

https://codereview.chromium.org/2059073002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:724:
mOptionsContainer = new RelativeLayout(getContext());
On 2016/06/13 20:34:03, dfalcantara wrote:
> No point in using an expensive RelativeLayout if they're just sitting on top
of
> each other.  Use FrameLayout or ViewSwitcher instead and assign the children a
> center layout_gravity.

Talked to UX. They want us to collapse the view, so it no longer makes sense to
hide mOptionsContainer via INVISIBLE and use CENTER_IN_PARENT for
mCheckingProgress. I can put the spinner and the message directly in
mainSectionLayout.

https://codereview.chromium.org/2059073002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java
(right):

https://codereview.chromium.org/2059073002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:509:
option, callback);
On 2016/06/13 20:34:03, dfalcantara wrote:
> Feels like you should only pass a real callback in if you're actually
expecting
> something to happen and null otherwise.

Done.

https://codereview.chromium.org/2059073002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:783:
public boolean isAdditionalTextWarning(OptionSection section) {
On 2016/06/13 20:34:03, dfalcantara wrote:
> Maybe isAdditionalTextDisplayingWarning?

Done.

https://codereview.chromium.org/2059073002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:786:
== SectionInformation.INVALID_SELECTION;
On 2016/06/13 20:34:03, dfalcantara wrote:
> I think this requires a double indentation on the second equality because it's
> continuing
> the condition on the previous line.
> 
> Could avoid the issue entirely by making a temporary boolean for it.

Done.

[please use gerrit instead, 2016-06-13 22:01:22]

Screenshot in http://crbug.com/619318#c3

[gone, 2016-06-13 22:44:54]

https://codereview.chromium.org/2059073002/diff/120001/chrome/android/java/re...
File chrome/android/java/res/values/colors.xml (right):

https://codereview.chromium.org/2059073002/diff/120001/chrome/android/java/re...
chrome/android/java/res/values/colors.xml:20: <color
name="disabled_text_color">#909090</color>
Should just explanation_text_color where use currently use disabled_text_color. 
No need to redefine the same hex code twice.

https://codereview.chromium.org/2059073002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java
(right):

https://codereview.chromium.org/2059073002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:499:
} else if (section == mShippingOptionSection) {
Do you have to explicitly handle setting mIsClientCheckingSelection to false in
the other cases?  What happens if the user clicks another section while this is
currently happening?  Should we disable inputs (but not the close button) while
this is happening?

[please use gerrit instead, 2016-06-13 23:07:09]

dfalcantara@, ptal patch 3.

https://codereview.chromium.org/2059073002/diff/120001/chrome/android/java/re...
File chrome/android/java/res/values/colors.xml (right):

https://codereview.chromium.org/2059073002/diff/120001/chrome/android/java/re...
chrome/android/java/res/values/colors.xml:20: <color
name="disabled_text_color">#909090</color>
On 2016/06/13 22:44:52, dfalcantara wrote:
> Should just explanation_text_color where use currently use
disabled_text_color. 
> No need to redefine the same hex code twice.

Done.

https://codereview.chromium.org/2059073002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java
(right):

https://codereview.chromium.org/2059073002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:499:
} else if (section == mShippingOptionSection) {
On 2016/06/13 22:44:52, dfalcantara wrote:
> Do you have to explicitly handle setting mIsClientCheckingSelection to false
in
> the other cases? What happens if the user clicks another section while this is
> currently happening?  Should we disable inputs (but not the close button)
while
> this is happening?

No need to alter mIsClientCheckingSelection elsewhere. It can be set to true
only by mClient.onSectionOptionChanged(), which will fire the callback to reset
mIsClientCheckingSelection to false on line 483.

I am blocking user input in isAcceptingUserInput() if mIsClientCheckingSelection
is false. (See line 622.) It would be nice to dim the other sections to indicate
that interaction with them is not necessary, but that's a question for UX.

[gone, 2016-06-13 23:12:13]

lgtm

https://codereview.chromium.org/2059073002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java
(right):

https://codereview.chromium.org/2059073002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:770:
* This particular animation is not tracked because it is not meant to be
cancellable.
Technically correct for the US:
http://www.collinsdictionary.com/dictionary/english/cancelable

[please use gerrit instead, 2016-06-13 23:18:37]

The CQ bit was checked by rouslan@chromium.org

[commit-bot: I haz the power, 2016-06-13 23:19:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2059073002/140001

[please use gerrit instead, 2016-06-13 23:19:38]

Sending to Cq.

Sorry about Britishizing your comments. That must be a bug in Vim's spellcheck
dictionary. Feel free to Americanize my comments in the future :-)

[commit-bot: I haz the power, 2016-06-14 00:20:39]

Committed patchset #3 (id:140001)

[commit-bot: I haz the power, 2016-06-14 00:20:40]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-14 00:23:11]

Description was changed from

==========
Add "Checking" spinner for server-side shipping address validation.

PaymentRequest should  let the user know that their section is being
verified. This is used, for example, when the website is verifying
availability of shipping options to the shipping address that the user
has selected. If an address is not supported, an error message should be
shown. The unsupported address should be disabled.

BUG=619318
==========

to

==========
Add "Checking" spinner for server-side shipping address validation.

PaymentRequest should  let the user know that their section is being
verified. This is used, for example, when the website is verifying
availability of shipping options to the shipping address that the user
has selected. If an address is not supported, an error message should be
shown. The unsupported address should be disabled.

BUG=619318

Committed: https://crrev.com/fbb096bf49b5a758a14d487370cf1ea8c0ea9519
Cr-Commit-Position: refs/heads/master@{#399612}
==========

[commit-bot: I haz the power, 2016-06-14 00:23:12]

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