Android Autofill card unmasking prompt - Add tooltip

Android Autofill card unmasking prompt - Add tooltip
for store locally checkbox

BUG=464640
Committed: https://crrev.com/a025878158db6fe6946bf49691a91ca896e2648c
Cr-Commit-Position: refs/heads/master@{#319792}

[Evan Stade, 2015-03-07 04:22:15]

estade@chromium.org changed reviewers:
+ newt@chromium.org

[Evan Stade, 2015-03-07 04:22:16]

added screenshot to
https://plus.google.com/photos/101067753755276187718/albums/6122898969014969153

[newt (away), 2015-03-09 18:15:53]

https://codereview.chromium.org/990543002/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java
(right):

https://codereview.chromium.org/990543002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:218:
assert (v == mStoreLocallyTooltipIcon);
nit: don't need () around assert statement

https://codereview.chromium.org/990543002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:226:
text.setWidth(mMainView.getWidth() - mStoreLocallyCheckbox.getPaddingStart()
All these *start() and *end() methods were added in API 17. You should use the
methods in ApiCompatibilityUtils.

I'm curious: did this cause Android lint "NewApi" warnings in your local build?
It should have.

[newt (away), 2015-03-09 18:20:23]

https://codereview.chromium.org/990543002/diff/1/chrome/android/java/res/layo...
File chrome/android/java/res/layout/autofill_card_unmask_prompt.xml (right):

https://codereview.chromium.org/990543002/diff/1/chrome/android/java/res/layo...
chrome/android/java/res/layout/autofill_card_unmask_prompt.xml:129:
android:layout_centerVertical="true"
you should specify layout_toStartOf here. Otherwise, if this text is really
long, it'll overlap the tooltip icon.

https://codereview.chromium.org/990543002/diff/1/chrome/android/java/res/layo...
chrome/android/java/res/layout/autofill_card_unmask_prompt.xml:143:
android:layout_marginEnd="12dp"
Do you really need marginEnd and paddingEnd? Same above.

[Evan Stade, 2015-03-09 18:54:13]

https://codereview.chromium.org/990543002/diff/1/chrome/android/java/res/layo...
File chrome/android/java/res/layout/autofill_card_unmask_prompt.xml (right):

https://codereview.chromium.org/990543002/diff/1/chrome/android/java/res/layo...
chrome/android/java/res/layout/autofill_card_unmask_prompt.xml:129:
android:layout_centerVertical="true"
On 2015/03/09 18:20:22, newt wrote:
> you should specify layout_toStartOf here. Otherwise, if this text is really
> long, it'll overlap the tooltip icon.

Done.

https://codereview.chromium.org/990543002/diff/1/chrome/android/java/res/layo...
chrome/android/java/res/layout/autofill_card_unmask_prompt.xml:143:
android:layout_marginEnd="12dp"
On 2015/03/09 18:20:22, newt wrote:
> Do you really need marginEnd and paddingEnd? Same above.

my understanding is that margin and padding affect the event hitbox differently,
so yes

https://codereview.chromium.org/990543002/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java
(right):

https://codereview.chromium.org/990543002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:218:
assert (v == mStoreLocallyTooltipIcon);
On 2015/03/09 18:15:53, newt wrote:
> nit: don't need () around assert statement

Done.

https://codereview.chromium.org/990543002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:226:
text.setWidth(mMainView.getWidth() - mStoreLocallyCheckbox.getPaddingStart()
On 2015/03/09 18:15:53, newt wrote:
> All these *start() and *end() methods were added in API 17. You should use the
> methods in ApiCompatibilityUtils.
> 
> I'm curious: did this cause Android lint "NewApi" warnings in your local
build?
> It should have.

Done. No lint errors seen (just a bunch of resource-related error messages).

[newt (away), 2015-03-09 20:53:56]

lgtm after one more getPaddingStart() is fixed

https://codereview.chromium.org/990543002/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java
(right):

https://codereview.chromium.org/990543002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:248:
mStoreLocallyCheckbox.getPaddingStart(), 0);
crash :/

[newt (away), 2015-03-09 21:19:24]

On 2015/03/09 20:53:56, newt wrote:
> lgtm after one more getPaddingStart() is fixed
> 
>
https://codereview.chromium.org/990543002/diff/20001/chrome/android/java/src/...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java
> (right):
> 
>
https://codereview.chromium.org/990543002/diff/20001/chrome/android/java/src/...
>
chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:248:
> mStoreLocallyCheckbox.getPaddingStart(), 0);
> crash :/

FYI, lint wasn't complaining about getPaddingStart() etc because the NewApi
check is currently disabled for CardUnmaskPrompt.java (and several other files)
because they had lint failures that I've been busy fixing. I'll be removing
those exceptions very soon, now that I've fixed all the lint warnings.

[Evan Stade, 2015-03-09 21:23:29]

thanks for being the interim linter

https://codereview.chromium.org/990543002/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java
(right):

https://codereview.chromium.org/990543002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:248:
mStoreLocallyCheckbox.getPaddingStart(), 0);
On 2015/03/09 20:53:56, newt wrote:
> crash :/

Done.

[Evan Stade, 2015-03-09 21:23:34]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-03-09 21:23:35]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org
Link to the patchset: https://codereview.chromium.org/990543002/#ps40001 (title:
"viewcompat 2")

[commit-bot: I haz the power, 2015-03-09 21:24:07]

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

[commit-bot: I haz the power, 2015-03-10 00:25:44]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-10 00:26:13]

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