|
|
Created:
3 years, 8 months ago by Shanfeng Modified:
3 years, 8 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, agrieve+watch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, Jared Saul Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAlways initialize this year and month
Currently, after click newCardLink for server cards,
the confirm button is always greyed. This Cl fixes this.
The bug is introduced by:
https://codereview.chromium.org/1964323002/patch/1/10001
When CardUnmaskPrompt is created. mShouldRequestExpirationDate
is false. So CalendarTask() is never executed:
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java?l=207
After onNewCardLinkClicked(), mShouldRequestExpirationDate
is true. That's why getExpirationDateErrorType is
always returning ERROR_TYPE_NOT_ENOUGH_INFO:
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java?l=615
We need to execute CalendarTask() from somewhere.
BUG=706143
Review-Url: https://codereview.chromium.org/2784663002
Cr-Commit-Position: refs/heads/master@{#461241}
Committed: https://chromium.googlesource.com/chromium/src/+/4d4693afa913c9272ba70ae0dc8b2eb387f83bce
Patch Set 1 #
Total comments: 4
Patch Set 2 : Put calendar task in Update #Messages
Total messages: 23 (12 generated)
szhangcs@google.com changed reviewers: + mathp@chromium.org, sebsg@chromium.org
Description was changed from ========== Always initialize this year and month BUG=706143 ========== to ========== Always initialize this year and month BUG=706143 ==========
The CQ bit was checked by szhangcs@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I left a comment inline. Also can you be a bit more descriptive in your CL description? Something like Currently, the newCardLink for server cards does not show the expiration date field. This Cl fixes... I know we talked offline but people reading this later or sheriffs don't :P https://codereview.chromium.org/2784663002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (left): https://codereview.chromium.org/2784663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:201: if (mShouldRequestExpirationDate) new CalendarTask().execute(); I would rather you do this logic in the onNewCardLinkClicked function. Also there a few things I'm confused about. For example, in onNewCardLinkClicked there is an "assert mShouldRequestExpirationDate". This would always trigger in you case no? because if it's not, the CalendarTask would have executed in the constructor?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Always initialize this year and month BUG=706143 ========== to ========== Currently, after click newCardLink for server cards, the confirm button is always greyed. This Cl fixes this. the bug is introduced by https://codereview.chromium.org/1964323002/patch/1/10001 When CardUnmaskPrompt is created. mShouldRequestExpirationDate is false. So CalendarTask() is never executed: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... After onNewCardLinkClicked(), mShouldRequestExpirationDate is true. That's why getExpirationDateErrorType is always returning ERROR_TYPE_NOT_ENOUGH_INFO: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... We need to execute CalendarTask() from somewhere. BUG=706143 ==========
Thanks for your review. https://codereview.chromium.org/2784663002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (left): https://codereview.chromium.org/2784663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:201: if (mShouldRequestExpirationDate) new CalendarTask().execute(); On 2017/03/29 18:58:21, sebsg wrote: > I would rather you do this logic in the onNewCardLinkClicked function. > > Also there a few things I'm confused about. For example, in onNewCardLinkClicked > there is an "assert mShouldRequestExpirationDate". This would always trigger in > you case no? because if it's not, the CalendarTask would have executed in the > constructor? Actually prefer to keep it here after more thoughts: 1) The original code before https://codereview.chromium.org/1964323002/patch/1/10001 initialize mThisYear and mThisMonth here. 2) We should initialize a private parameter at the constructor. Because it may be used anywhere in this class. 3) This task is light weight. 4) It's an async task. So will not block the constructor. As for "assert mShouldRequestExpirationDate", I believe mShouldRequestExpirationDate is false when we initialize the prompt. But it is set to true later: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... (For more info, the controller will call update, and set mShouldRequestExpirationDate to be true: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...)
https://codereview.chromium.org/2784663002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (left): https://codereview.chromium.org/2784663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:201: if (mShouldRequestExpirationDate) new CalendarTask().execute(); On 2017/03/29 22:16:58, Shanfeng wrote: > On 2017/03/29 18:58:21, sebsg wrote: > > I would rather you do this logic in the onNewCardLinkClicked function. > > > > Also there a few things I'm confused about. For example, in > onNewCardLinkClicked > > there is an "assert mShouldRequestExpirationDate". This would always trigger > in > > you case no? because if it's not, the CalendarTask would have executed in the > > constructor? > > Actually prefer to keep it here after more thoughts: > 1) The original code before > https://codereview.chromium.org/1964323002/patch/1/10001 initialize mThisYear > and mThisMonth here. > 2) We should initialize a private parameter at the constructor. Because it may > be used anywhere in this class. > 3) This task is light weight. > 4) It's an async task. So will not block the constructor. > > As for "assert mShouldRequestExpirationDate", I believe > mShouldRequestExpirationDate is false when we initialize the prompt. But it is > set to true later: > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > (For more info, the controller will call update, and set > mShouldRequestExpirationDate to be true: > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...) Right, when you send the onNewCardLinkClicked to the native code, it will change it's value of shoudlRequestExpiration and call update on the Java version Then my proposition would be that in update() when assigning the value of mShouldRequestExpirationDate, you check if it was false before and now set to true, and then load the calendar. I prefer this solution because loading the calendar in the constructor is a waste the majority of the time, since the usual operation is to just enter the CVC.
szhangcs@google.com changed reviewers: + rouslan@chromium.org - mathp@chromium.org
Thanks https://codereview.chromium.org/2784663002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (left): https://codereview.chromium.org/2784663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:201: if (mShouldRequestExpirationDate) new CalendarTask().execute(); On 2017/03/30 14:02:35, sebsg wrote: > On 2017/03/29 22:16:58, Shanfeng wrote: > > On 2017/03/29 18:58:21, sebsg wrote: > > > I would rather you do this logic in the onNewCardLinkClicked function. > > > > > > Also there a few things I'm confused about. For example, in > > onNewCardLinkClicked > > > there is an "assert mShouldRequestExpirationDate". This would always trigger > > in > > > you case no? because if it's not, the CalendarTask would have executed in > the > > > constructor? > > > > Actually prefer to keep it here after more thoughts: > > 1) The original code before > > https://codereview.chromium.org/1964323002/patch/1/10001 initialize mThisYear > > and mThisMonth here. > > 2) We should initialize a private parameter at the constructor. Because it may > > be used anywhere in this class. > > 3) This task is light weight. > > 4) It's an async task. So will not block the constructor. > > > > As for "assert mShouldRequestExpirationDate", I believe > > mShouldRequestExpirationDate is false when we initialize the prompt. But it is > > set to true later: > > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > (For more info, the controller will call update, and set > > mShouldRequestExpirationDate to be true: > > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...) > > Right, when you send the onNewCardLinkClicked to the native code, it will change > it's value of shoudlRequestExpiration and call update on the Java version > > Then my proposition would be that in update() when assigning the value of > mShouldRequestExpirationDate, you check if it was false before and now set to > true, and then load the calendar. > > I prefer this solution because loading the calendar in the constructor is a > waste the majority of the time, since the usual operation is to just enter the > CVC. Done.
Description was changed from ========== Currently, after click newCardLink for server cards, the confirm button is always greyed. This Cl fixes this. the bug is introduced by https://codereview.chromium.org/1964323002/patch/1/10001 When CardUnmaskPrompt is created. mShouldRequestExpirationDate is false. So CalendarTask() is never executed: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... After onNewCardLinkClicked(), mShouldRequestExpirationDate is true. That's why getExpirationDateErrorType is always returning ERROR_TYPE_NOT_ENOUGH_INFO: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... We need to execute CalendarTask() from somewhere. BUG=706143 ========== to ========== Always initialize this year and month Currently, after click newCardLink for server cards, the confirm button is always greyed. This Cl fixes this. The bug is introduced by: https://codereview.chromium.org/1964323002/patch/1/10001 When CardUnmaskPrompt is created. mShouldRequestExpirationDate is false. So CalendarTask() is never executed: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... After onNewCardLinkClicked(), mShouldRequestExpirationDate is true. That's why getExpirationDateErrorType is always returning ERROR_TYPE_NOT_ENOUGH_INFO: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... We need to execute CalendarTask() from somewhere. BUG=706143 ==========
I've cleaned up your CL description, FYI.
On 2017/03/31 14:44:09, ಠ_ಠ wrote: > I've cleaned up your CL description, FYI. Thanks.
LGTM. I saw you have a bug to add tests and since they are not easy to implement and this CL fixes a bug, I don't want to block you on them. Instead I manually tested your patch. But please add the tests soon :)
On 2017/03/31 19:32:04, sebsg wrote: > LGTM. > > I saw you have a bug to add tests and since they are not easy to implement and > this CL fixes a bug, I don't want to block you on them. Instead I manually > tested your patch. But please add the tests soon :) RS LGTM
The CQ bit was checked by szhangcs@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490994470431510, "parent_rev": "7ee0fa238b36d1f1a12a240f2caf025559727ec4", "commit_rev": "4d4693afa913c9272ba70ae0dc8b2eb387f83bce"}
Message was sent while issue was closed.
Description was changed from ========== Always initialize this year and month Currently, after click newCardLink for server cards, the confirm button is always greyed. This Cl fixes this. The bug is introduced by: https://codereview.chromium.org/1964323002/patch/1/10001 When CardUnmaskPrompt is created. mShouldRequestExpirationDate is false. So CalendarTask() is never executed: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... After onNewCardLinkClicked(), mShouldRequestExpirationDate is true. That's why getExpirationDateErrorType is always returning ERROR_TYPE_NOT_ENOUGH_INFO: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... We need to execute CalendarTask() from somewhere. BUG=706143 ========== to ========== Always initialize this year and month Currently, after click newCardLink for server cards, the confirm button is always greyed. This Cl fixes this. The bug is introduced by: https://codereview.chromium.org/1964323002/patch/1/10001 When CardUnmaskPrompt is created. mShouldRequestExpirationDate is false. So CalendarTask() is never executed: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... After onNewCardLinkClicked(), mShouldRequestExpirationDate is true. That's why getExpirationDateErrorType is always returning ERROR_TYPE_NOT_ENOUGH_INFO: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... We need to execute CalendarTask() from somewhere. BUG=706143 Review-Url: https://codereview.chromium.org/2784663002 Cr-Commit-Position: refs/heads/master@{#461241} Committed: https://chromium.googlesource.com/chromium/src/+/4d4693afa913c9272ba70ae0dc8b... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/4d4693afa913c9272ba70ae0dc8b... |