Focus first invalid field of payment request editor

Focus first invalid field of payment request editor

BUG=718582
Review-Url: https://codereview.chromium.org/2881643002
Cr-Commit-Position: refs/heads/master@{#473223}
Committed: https://chromium.googlesource.com/chromium/src/+/7bd80624d68ec2ff9cca61c0bdf0754c6830f6ef

[MAD, 2017-05-11 20:24:17]

mad@chromium.org changed reviewers:
+ anthonyvd@chromium.org

[MAD, 2017-05-11 20:24:18]

I couldn't find a clean way of displaying the error on top of focusing invalid
field. If it's OK with you, we could file it as a separate bug and fix it in
another CL.

Thanks!

BYE
MAD

[MAD, 2017-05-11 21:08:45]

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

[commit-bot: I haz the power, 2017-05-11 21:09:43]

Dry run: 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, 2017-05-11 21:16:01]

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

[commit-bot: I haz the power, 2017-05-11 21:16:02]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[MAD, 2017-05-12 00:51:49]

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

[commit-bot: I haz the power, 2017-05-12 00:52:37]

Dry run: 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, 2017-05-12 01:09:40]

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

[commit-bot: I haz the power, 2017-05-12 01:09:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[MAD, 2017-05-12 02:01:25]

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

[commit-bot: I haz the power, 2017-05-12 02:02:37]

Dry run: 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, 2017-05-12 02:43:28]

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

[commit-bot: I haz the power, 2017-05-12 02:43:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[anthonyvd, 2017-05-12 14:19:45]

Thanks, it'll be awesome to get this working! Few comments.

https://codereview.chromium.org/2881643002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc
(right):

https://codereview.chromium.org/2881643002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:171:
return std::unique_ptr<views::View>();
nit: just nullptr was fine and it's what we use elsewhere.

https://codereview.chromium.org/2881643002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/editor_view_controller.cc (right):

https://codereview.chromium.org/2881643002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.cc:284: views::View*
potential_first_field_view = nullptr;
I'm confused: we seem to be creating the fields both here and at line 266 above.
Is this the result of a mixed up rebase?

https://codereview.chromium.org/2881643002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/editor_view_controller.h (right):

https://codereview.chromium.org/2881643002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.h:166: // field value
that could receive the initial focus if not invalid value is
I'm not sure what this means? Does it return any valid field if there's no
invalid one?

https://codereview.chromium.org/2881643002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_sheet_controller.cc
(right):

https://codereview.chromium.org/2881643002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.cc:187: if
(GetSheetId(&sheet_id)) {
Some views probably don't want this to happen here. Can we extract it into
another function? I'm thinking about something like:

void UpdateFocus(views::View* v) {
  DialogViewID sheet_id;
  if (GetSheetId(&sheet_id)) {
    SheetView* sheet_view = static_cast<SheetView*>(
        dialog()->GetViewByID(static_cast<int>(sheet_id)));
    // This will be null on first call since it's not been set until CreateView
    // returns, and the first call to UpdateContentView comes from CreateView.
    if (sheet_view) {
      sheet_view->SetFirstFocusableView(v);
      dialog()->RequestFocus();
    }
  }
}

and then have editors call it with GetFirstFocusedView() as the argument after
they call UpdateContentView.

This will also make it reusable when we implement post update focus restoration.
WDYT?

https://codereview.chromium.org/2881643002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/validation_delegate.h (right):

https://codereview.chromium.org/2881643002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/validation_delegate.h:21: virtual bool
ValidateTextfield(views::Textfield* textfield,
I think we should separate the validation logic from the error display here.
This would allow us to call the validation function alone as needed and remove
the need for this |display_error|.

Maybe have Validate{Textfield|Combobox} return whether the field is valid or not
and take a string16* (that defaults to nullptr) as an argument to fill with the
error string. Then, add DisplayError that takes the error string as an argument
and updates the UI with it.

I think this would also make testing the validation functions easier. WDYT?

[MAD, 2017-05-17 14:00:13]

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

[commit-bot: I haz the power, 2017-05-17 14:00:31]

Dry run: 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, 2017-05-17 14:14:30]

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

[commit-bot: I haz the power, 2017-05-17 14:14:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[MAD, 2017-05-18 12:06:02]

Patchset #9 (id:160001) has been deleted

[MAD, 2017-05-18 16:01:31]

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

[MAD, 2017-05-18 16:01:49]

OK, this should be ready to take another look.

Sorry about the confusion of the multiple patches and rebase mixed up, this CL
got me to find other issues that I needed to fix in separate CLs, and I got a
bit branch tracking merge confused at one point... :-(

Thanks!

BYE
MAD

https://codereview.chromium.org/2881643002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc
(right):

https://codereview.chromium.org/2881643002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:171:
return std::unique_ptr<views::View>();
On 2017/05/12 14:19:44, anthonyvd wrote:
> nit: just nullptr was fine and it's what we use elsewhere.

Done.

https://codereview.chromium.org/2881643002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/editor_view_controller.cc (right):

https://codereview.chromium.org/2881643002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.cc:284: views::View*
potential_first_field_view = nullptr;
On 2017/05/12 14:19:44, anthonyvd wrote:
> I'm confused: we seem to be creating the fields both here and at line 266
above.
> Is this the result of a mixed up rebase?

Oups, yes, merge fluke... Sorry about that... :-(

https://codereview.chromium.org/2881643002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/editor_view_controller.h (right):

https://codereview.chromium.org/2881643002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.h:166: // field value
that could receive the initial focus if not invalid value is
On 2017/05/12 14:19:44, anthonyvd wrote:
> I'm not sure what this means? Does it return any valid field if there's no
> invalid one?

Rephrased. Better?

https://codereview.chromium.org/2881643002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_sheet_controller.cc
(right):

https://codereview.chromium.org/2881643002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.cc:187: if
(GetSheetId(&sheet_id)) {
On 2017/05/12 14:19:44, anthonyvd wrote:
> Some views probably don't want this to happen here. Can we extract it into
> another function? I'm thinking about something like:
> 
> void UpdateFocus(views::View* v) {
>   DialogViewID sheet_id;
>   if (GetSheetId(&sheet_id)) {
>     SheetView* sheet_view = static_cast<SheetView*>(
>         dialog()->GetViewByID(static_cast<int>(sheet_id)));
>     // This will be null on first call since it's not been set until
CreateView
>     // returns, and the first call to UpdateContentView comes from CreateView.
>     if (sheet_view) {
>       sheet_view->SetFirstFocusableView(v);
>       dialog()->RequestFocus();
>     }
>   }
> }
> 
> and then have editors call it with GetFirstFocusedView() as the argument after
> they call UpdateContentView.
> 
> This will also make it reusable when we implement post update focus
restoration.
> WDYT?

Done.

https://codereview.chromium.org/2881643002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/validation_delegate.h (right):

https://codereview.chromium.org/2881643002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/validation_delegate.h:21: virtual bool
ValidateTextfield(views::Textfield* textfield,
On 2017/05/12 14:19:44, anthonyvd wrote:
> I think we should separate the validation logic from the error display here.
> This would allow us to call the validation function alone as needed and remove
> the need for this |display_error|.
> 
> Maybe have Validate{Textfield|Combobox} return whether the field is valid or
not
> and take a string16* (that defaults to nullptr) as an argument to fill with
the
> error string. Then, add DisplayError that takes the error string as an
argument
> and updates the UI with it.
> 
> I think this would also make testing the validation functions easier. WDYT?

As discussed... OK?

[commit-bot: I haz the power, 2017-05-18 16:02:57]

Dry run: 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, 2017-05-18 16:17:06]

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

[commit-bot: I haz the power, 2017-05-18 16:17:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[MAD, 2017-05-18 17:22:40]

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

[commit-bot: I haz the power, 2017-05-18 17:23:44]

Dry run: CQ is trying da patch.

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

[anthonyvd, 2017-05-18 18:12:25]

This is starting to look great :) Couple more comments.

https://codereview.chromium.org/2881643002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/editor_view_controller.h (right):

https://codereview.chromium.org/2881643002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.h:166: // field value
that could receive the initial focus if not invalid value is
On 2017/05/18 at 16:01:45, MAD wrote:
> On 2017/05/12 14:19:44, anthonyvd wrote:
> > I'm not sure what this means? Does it return any valid field if there's no
> > invalid one?
> 
> Rephrased. Better?

Yep, thanks!

https://codereview.chromium.org/2881643002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/validation_delegate.h (right):

https://codereview.chromium.org/2881643002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/validation_delegate.h:21: virtual bool
ValidateTextfield(views::Textfield* textfield,
On 2017/05/18 at 16:01:46, MAD wrote:
> On 2017/05/12 14:19:44, anthonyvd wrote:
> > I think we should separate the validation logic from the error display here.
> > This would allow us to call the validation function alone as needed and
remove
> > the need for this |display_error|.
> > 
> > Maybe have Validate{Textfield|Combobox} return whether the field is valid or
not
> > and take a string16* (that defaults to nullptr) as an argument to fill with
the
> > error string. Then, add DisplayError that takes the error string as an
argument
> > and updates the UI with it.
> > 
> > I think this would also make testing the validation functions easier. WDYT?
> 
> As discussed... OK?

Yeah, I like this way better :) Thanks!

https://codereview.chromium.org/2881643002/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/editor_view_controller.cc (right):

https://codereview.chromium.org/2881643002/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.cc:265: if
(!potential_first_field_view)
I find this really hard to follow because initial_focus_field_view_ and friends
are update in multiple places in the code. Can we instead change
CreateInputField to take a views::View** (the created field) and a bool*
(whether the created field is valid or not) as arguments? This way, we can
remove the focusing logic from CreateInputField entirely and change this loop
here to 

initial_focus_field_view_ = nullptr;
views::View* first_field = nullptr;
for (const auto& field : GetFieldDefinitions()) {
  views::View* field_view;
  bool is_valid;
  CreateInputField(editor_layout.get(), field, &field_view, &is_valid);
  if (!first_field)
    first_field = field_view;
  if (!initial_focus_field_view_ && !is_valid)
    initial_focus_field_view_ = field_view;
}

if (!initial_focus_field_view_)
  initial_focus_field_view_ = first_field;

Which is *slightly* more lines but makes it clearer what's happening and
centralizes the focus choosing logic here.

WDYT?

https://codereview.chromium.org/2881643002/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/editor_view_controller.h (right):

https://codereview.chromium.org/2881643002/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.h:166: // are no field
identified as invalid once all the fields are created.
nit: field -> fields

[commit-bot: I haz the power, 2017-05-18 19:17:01]

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

[commit-bot: I haz the power, 2017-05-18 19:17:02]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[MAD, 2017-05-18 20:31:19]

I kept the focusable view as a return value if you don't mind, and I added the
bool for invalid as your suggested.

OK?

I needed to fix a few other things related to validity that were failing tests
for some reason, maybe a mix between this and another CL that got committed
recently.

Thanks!

BYE
MAD

https://codereview.chromium.org/2881643002/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/editor_view_controller.cc (right):

https://codereview.chromium.org/2881643002/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.cc:265: if
(!potential_first_field_view)
On 2017/05/18 18:12:25, anthonyvd wrote:
> I find this really hard to follow because initial_focus_field_view_ and
friends
> are update in multiple places in the code. Can we instead change
> CreateInputField to take a views::View** (the created field) and a bool*
> (whether the created field is valid or not) as arguments? This way, we can
> remove the focusing logic from CreateInputField entirely and change this loop
> here to 
> 
> initial_focus_field_view_ = nullptr;
> views::View* first_field = nullptr;
> for (const auto& field : GetFieldDefinitions()) {
>   views::View* field_view;
>   bool is_valid;
>   CreateInputField(editor_layout.get(), field, &field_view, &is_valid);
>   if (!first_field)
>     first_field = field_view;
>   if (!initial_focus_field_view_ && !is_valid)
>     initial_focus_field_view_ = field_view;
> }
> 
> if (!initial_focus_field_view_)
>   initial_focus_field_view_ = first_field;
> 
> Which is *slightly* more lines but makes it clearer what's happening and
> centralizes the focus choosing logic here.
> 
> WDYT?

Yeah, I agree that it wasn't very elegant, I shouldn't have tried to keep the
concept of the CreateInputField updating the data member, and returning
something for the caller.

Thanks for the suggestion.

https://codereview.chromium.org/2881643002/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/editor_view_controller.h (right):

https://codereview.chromium.org/2881643002/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.h:166: // are no field
identified as invalid once all the fields are created.
On 2017/05/18 18:12:25, anthonyvd wrote:
> nit: field -> fields

Done.

[MAD, 2017-05-18 20:31:31]

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

[commit-bot: I haz the power, 2017-05-18 20:33:30]

Dry run: CQ is trying da patch.

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

[anthonyvd, 2017-05-18 21:15:38]

Yeah, returning the field makes sense.

https://codereview.chromium.org/2881643002/diff/260001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/editor_view_controller.cc (right):

https://codereview.chromium.org/2881643002/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.cc:262: views::View*
potential_first_field_view = nullptr;
nit: first_field?

https://codereview.chromium.org/2881643002/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.cc:270: else if
(!potential_first_field_view)
If you pull this else if statement out of the parent if (the first field is
always going into this when it matters anyways), you'll be able to collapse both
conditions above into if (!initial_focus_field_view_ && !valid).

Basically either we need to remember the very first field, or it doesn't matter
because one of them is invalid.

https://codereview.chromium.org/2881643002/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.cc:320: views::View*
focusable_field = nullptr;
Since this isn't necessarily related to focus anymore, maybe it can be renamed
to just field?

[commit-bot: I haz the power, 2017-05-18 22:48:08]

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

[commit-bot: I haz the power, 2017-05-18 22:48:09]

Dry run: This issue passed the CQ dry run.

[MAD, 2017-05-19 13:42:57]

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

[commit-bot: I haz the power, 2017-05-19 13:43:11]

Dry run: CQ is trying da patch.

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

[MAD, 2017-05-19 13:43:51]

Renamed variable as requested, and explained the reasons for the other comments.
Let me know what you think...

Thanks!

BYE
MAD

https://codereview.chromium.org/2881643002/diff/260001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/editor_view_controller.cc (right):

https://codereview.chromium.org/2881643002/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.cc:262: views::View*
potential_first_field_view = nullptr;
On 2017/05/18 21:15:37, anthonyvd wrote:
> nit: first_field?

Done.

https://codereview.chromium.org/2881643002/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.cc:270: else if
(!potential_first_field_view)
On 2017/05/18 21:15:37, anthonyvd wrote:
> If you pull this else if statement out of the parent if (the first field is
> always going into this when it matters anyways), you'll be able to collapse
both
> conditions above into if (!initial_focus_field_view_ && !valid).
> 
> Basically either we need to remember the very first field, or it doesn't
matter
> because one of them is invalid.

I'm not sure I understand. The idea here is that, if initial_focus_field_view_
is set, there's nothing else to be done, no need to check for a potential first
field, since we already found it.

https://codereview.chromium.org/2881643002/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.cc:320: views::View*
focusable_field = nullptr;
On 2017/05/18 21:15:37, anthonyvd wrote:
> Since this isn't necessarily related to focus anymore, maybe it can be renamed
> to just field?

Well, it still kind of is, especially in the custom field case, where there
might be more than one field, but we want to return the one that should receive
focus. I could add documentation here if you want, but I thought it was clear
enough in the header file documentation of this method.

[commit-bot: I haz the power, 2017-05-19 14:43:54]

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

[commit-bot: I haz the power, 2017-05-19 14:43:56]

Dry run: This issue passed the CQ dry run.

[anthonyvd, 2017-05-19 15:11:42]

https://codereview.chromium.org/2881643002/diff/260001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/editor_view_controller.cc (right):

https://codereview.chromium.org/2881643002/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.cc:270: else if
(!potential_first_field_view)
On 2017/05/19 at 13:43:51, MAD wrote:
> On 2017/05/18 21:15:37, anthonyvd wrote:
> > If you pull this else if statement out of the parent if (the first field is
> > always going into this when it matters anyways), you'll be able to collapse
both
> > conditions above into if (!initial_focus_field_view_ && !valid).
> > 
> > Basically either we need to remember the very first field, or it doesn't
matter
> > because one of them is invalid.
> 
> I'm not sure I understand. The idea here is that, if initial_focus_field_view_
is set, there's nothing else to be done, no need to check for a potential first
field, since we already found it.

Sorry I'm being unclear. The algorithm is "If at least one field is invalid,
focus the first invalid field. Otherwise, focus the first field in the editor"
so this code can become

views::View* first_field = nullptr;
for (const auto& field : GetFieldDefinitions()) {
    bool valid = false;
    views::View* focusable_field =
        CreateInputField(editor_layout.get(), field, &valid);
    if (!first_field)
        first_field = focusable_field;
    if (!initial_focus_field_view_ && !valid)
        initial_focus_field_view_ = focusable_field;
}

When this loop exists, |first_field| is always set to the first field in the
editor and |initial_focus_field_| is set to the first invalid one if there's
one. The if at line 275 takes care of the rest.

https://codereview.chromium.org/2881643002/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.cc:320: views::View*
focusable_field = nullptr;
On 2017/05/19 at 13:43:51, MAD wrote:
> On 2017/05/18 21:15:37, anthonyvd wrote:
> > Since this isn't necessarily related to focus anymore, maybe it can be
renamed
> > to just field?
> 
> Well, it still kind of is, especially in the custom field case, where there
might be more than one field, but we want to return the one that should receive
focus. I could add documentation here if you want, but I thought it was clear
enough in the header file documentation of this method.

Ah I didn't think about composite fields. Yeah that makes sense!

[MAD, 2017-05-19 15:35:21]

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

[MAD, 2017-05-19 15:35:36]

OK you win... :-)

All good?

Thanks!

BYE
MAD

https://codereview.chromium.org/2881643002/diff/260001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/editor_view_controller.cc (right):

https://codereview.chromium.org/2881643002/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.cc:270: else if
(!potential_first_field_view)
On 2017/05/19 15:11:42, anthonyvd wrote:
> On 2017/05/19 at 13:43:51, MAD wrote:
> > On 2017/05/18 21:15:37, anthonyvd wrote:
> > > If you pull this else if statement out of the parent if (the first field
is
> > > always going into this when it matters anyways), you'll be able to
collapse
> both
> > > conditions above into if (!initial_focus_field_view_ && !valid).
> > > 
> > > Basically either we need to remember the very first field, or it doesn't
> matter
> > > because one of them is invalid.
> > 
> > I'm not sure I understand. The idea here is that, if
initial_focus_field_view_
> is set, there's nothing else to be done, no need to check for a potential
first
> field, since we already found it.
> 
> Sorry I'm being unclear. The algorithm is "If at least one field is invalid,
> focus the first invalid field. Otherwise, focus the first field in the editor"
> so this code can become
> 
> views::View* first_field = nullptr;
> for (const auto& field : GetFieldDefinitions()) {
>     bool valid = false;
>     views::View* focusable_field =
>         CreateInputField(editor_layout.get(), field, &valid);
>     if (!first_field)
>         first_field = focusable_field;
>     if (!initial_focus_field_view_ && !valid)
>         initial_focus_field_view_ = focusable_field;
> }
> 
> When this loop exists, |first_field| is always set to the first field in the
> editor and |initial_focus_field_| is set to the first invalid one if there's
> one. The if at line 275 takes care of the rest.

Done.

[commit-bot: I haz the power, 2017-05-19 15:36:11]

Dry run: CQ is trying da patch.

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

[anthonyvd, 2017-05-19 15:41:13]

lgtm

[MAD, 2017-05-19 15:50:49]

On 2017/05/19 15:41:13, anthonyvd wrote:
> lgtm

Merci... :-)

[commit-bot: I haz the power, 2017-05-19 17:09:49]

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

[commit-bot: I haz the power, 2017-05-19 17:09:50]

Dry run: This issue passed the CQ dry run.

[MAD, 2017-05-19 17:10:17]

The CQ bit was checked by mad@chromium.org

[commit-bot: I haz the power, 2017-05-19 17:10:53]

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, 2017-05-19 17:16:48]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1495213817725340,
"parent_rev": "6d2438074669d3251deee6d17a9e6692ffcff297", "commit_rev":
"7bd80624d68ec2ff9cca61c0bdf0754c6830f6ef"}

[commit-bot: I haz the power, 2017-05-19 17:17:00]

Description was changed from

==========
Focus first invalid field of payment request editor

BUG=718582
==========

to

==========
Focus first invalid field of payment request editor

BUG=718582

Review-Url: https://codereview.chromium.org/2881643002
Cr-Commit-Position: refs/heads/master@{#473223}
Committed:
https://chromium.googlesource.com/chromium/src/+/7bd80624d68ec2ff9cca61c0bdf0...
==========

[commit-bot: I haz the power, 2017-05-19 17:17:05]

Committed patchset #16 (id:320001) as
https://chromium.googlesource.com/chromium/src/+/7bd80624d68ec2ff9cca61c0bdf0...