[Payments] Selecting incomplete items will open editors

[Payments] Selecting incomplete items will open editors

BUG=709571
TEST=browser_tests
TBR=mahmadi

Review-Url: https://codereview.chromium.org/2805263003
Cr-Commit-Position: refs/heads/master@{#463406}
Committed: https://chromium.googlesource.com/chromium/src/+/4baea33bdc3f22c101176a43e694cbc1cbb56650

[Mathieu, 2017-04-07 19:25:44]

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

[Mathieu, 2017-04-07 19:25:44]

Hello! PTAL?

[anthonyvd, 2017-04-07 20:08:09]

Hello, looks good! Few comments/questions.

https://codereview.chromium.org/2805263003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_method_view_controller.cc (right):

https://codereview.chromium.org/2805263003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_method_view_controller.cc:103:
AutofillPaymentInstrument* instrument_;
I don't think we want this, even if it's a temporary thing. Is there a way we
can make this instrument type agnostic right away?

What about adding a function on PaymentInstrument (something like
instrument_type()) that returns an enum value corresponding to the concrete
instrument's type? This way we can switch here to create derived
PaymentMethodListItem types.

https://codereview.chromium.org/2805263003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc
(right):

https://codereview.chromium.org/2805263003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:102:
// TODO(crbug.com/709451): Use GetInfo() here.
Why not use it already? I'm not sure what in that bug is preventing us from
doing it if we know it's the correct thing.

https://codereview.chromium.org/2805263003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:136:
profile.set_origin(autofill::kSettingsOrigin);
Do we want a different origin for PaymentRequest?

https://codereview.chromium.org/2805263003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:144:
profile_to_edit_->SetRawInfo(field.second.type,
If GetInfo is preferred over GetRawInfo above, shouldn't this be SetInfo in some
cases?

https://codereview.chromium.org/2805263003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:151:
profile_to_edit_->set_origin(autofill::kSettingsOrigin);
Same question re: origin.

https://codereview.chromium.org/2805263003/diff/20001/components/payments/con...
File components/payments/content/payment_request_state.cc (left):

https://codereview.chromium.org/2805263003/diff/20001/components/payments/con...
components/payments/content/payment_request_state.cc:44: if
(instrument.get()->IsValid() &&
Is this just that the PR spec doesn't require the instrument to be valid for
CanMakePayment to return true?

[Mathieu, 2017-04-09 00:27:15]

Patchset #3 (id:40001) has been deleted

[Mathieu, 2017-04-09 00:35:18]

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

[commit-bot: I haz the power, 2017-04-09 00:35:30]

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

[Mathieu, 2017-04-09 00:35:30]

Patchset #3 (id:60001) has been deleted

[Mathieu, 2017-04-09 00:35:36]

Patchset #3 (id:80001) has been deleted

[Mathieu, 2017-04-09 00:35:45]

PTAL

https://codereview.chromium.org/2805263003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_method_view_controller.cc (right):

https://codereview.chromium.org/2805263003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_method_view_controller.cc:103:
AutofillPaymentInstrument* instrument_;
On 2017/04/07 20:08:09, anthonyvd wrote:
> I don't think we want this, even if it's a temporary thing. Is there a way we
> can make this instrument type agnostic right away?
> 
> What about adding a function on PaymentInstrument (something like
> instrument_type()) that returns an enum value corresponding to the concrete
> instrument's type? This way we can switch here to create derived
> PaymentMethodListItem types.

You're right! Added a switch on the type() so that this will fail to compile if
we add a new instrument type.

https://codereview.chromium.org/2805263003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc
(right):

https://codereview.chromium.org/2805263003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:102:
// TODO(crbug.com/709451): Use GetInfo() here.
On 2017/04/07 20:08:09, anthonyvd wrote:
> Why not use it already? I'm not sure what in that bug is preventing us from
> doing it if we know it's the correct thing.

You're right, I can get the app_locale

https://codereview.chromium.org/2805263003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:136:
profile.set_origin(autofill::kSettingsOrigin);
On 2017/04/07 20:08:09, anthonyvd wrote:
> Do we want a different origin for PaymentRequest?

I don't think so. "settings" means the profile has been verified by the user in
some way (it then becomes stronger than an inferred profile when merging with
others). Since this is an editor and the person has clicked Save, I think
keeping this origin is correct and equivalent to editing in chrome://settings.

https://codereview.chromium.org/2805263003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:144:
profile_to_edit_->SetRawInfo(field.second.type,
On 2017/04/07 20:08:09, anthonyvd wrote:
> If GetInfo is preferred over GetRawInfo above, shouldn't this be SetInfo in
some
> cases? 

Yeah I think we should play it safe and use GetInfo/SetInfo always.

https://codereview.chromium.org/2805263003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc:151:
profile_to_edit_->set_origin(autofill::kSettingsOrigin);
On 2017/04/07 20:08:09, anthonyvd wrote:
> Same question re: origin.

Acknowledged.

https://codereview.chromium.org/2805263003/diff/20001/components/payments/con...
File components/payments/content/payment_request_state.cc (left):

https://codereview.chromium.org/2805263003/diff/20001/components/payments/con...
components/payments/content/payment_request_state.cc:44: if
(instrument.get()->IsValid() &&
On 2017/04/07 20:08:09, anthonyvd wrote:
> Is this just that the PR spec doesn't require the instrument to be valid for
> CanMakePayment to return true?

You're right this was unclear. I've rearranged so that the PaymentInstrument API
is clearer, and implemented it to be the same as in Java.

[commit-bot: I haz the power, 2017-04-09 01:15:42]

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

[commit-bot: I haz the power, 2017-04-09 01:15:42]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[anthonyvd, 2017-04-10 15:59:55]

Great, lgtm! Thanks for the changes, and your explanation for the settings
origin makes sense.

https://codereview.chromium.org/2805263003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc
(right):

https://codereview.chromium.org/2805263003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:175:
const std::string& locale = state()->GetApplicationLocale();
minor unrelated nit: why is GetApplicationLocale on state? It seems like it fits
more on spec, no (since it doesn't change)?

[Mathieu, 2017-04-10 16:05:17]

https://codereview.chromium.org/2805263003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc
(right):

https://codereview.chromium.org/2805263003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:175:
const std::string& locale = state()->GetApplicationLocale();
On 2017/04/10 15:59:55, anthonyvd wrote:
> minor unrelated nit: why is GetApplicationLocale on state? It seems like it
fits
> more on spec, no (since it doesn't change)?

Yeah, there's no good place for this (and I don't want to create another class
alongside state and spec for helpers). Agreed it's probably better in spec. I'll
change it in a followup?

[Mathieu, 2017-04-10 16:10:37]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2017-04-10 16:11:00]

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-04-10 17:12:42]

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

[commit-bot: I haz the power, 2017-04-10 17:12:43]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Mathieu, 2017-04-10 17:47:22]

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

[Mathieu, 2017-04-10 17:47:23]

TBR mahmadi@chromium.org for ios

[Mathieu, 2017-04-10 17:47:36]

Description was changed from

==========
[Payments] Selecting incomplete items will open editors

BUG=709571
TEST=browser_tests
==========

to

==========
[Payments] Selecting incomplete items will open editors

BUG=709571
TEST=browser_tests
TBR=mahmadi
==========

[Mathieu, 2017-04-10 17:47:42]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2017-04-10 17:47:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/2805263003/#ps120001
(title: "fix ios test")

[commit-bot: I haz the power, 2017-04-10 17:48:15]

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-04-10 18:49:32]

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

[commit-bot: I haz the power, 2017-04-10 18:49:33]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Mathieu, 2017-04-10 20:19:35]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2017-04-10 20:19:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/2805263003/#ps140001
(title: "fix ios test for realz")

[commit-bot: I haz the power, 2017-04-10 20:20:00]

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-04-10 21:42:35]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1491855575362880,
"parent_rev": "5b67e8ee116312b0fe17172105c8575cbbf45e66", "commit_rev":
"4baea33bdc3f22c101176a43e694cbc1cbb56650"}

[commit-bot: I haz the power, 2017-04-10 21:43:45]

Description was changed from

==========
[Payments] Selecting incomplete items will open editors

BUG=709571
TEST=browser_tests
TBR=mahmadi
==========

to

==========
[Payments] Selecting incomplete items will open editors

BUG=709571
TEST=browser_tests
TBR=mahmadi

Review-Url: https://codereview.chromium.org/2805263003
Cr-Commit-Position: refs/heads/master@{#463406}
Committed:
https://chromium.googlesource.com/chromium/src/+/4baea33bdc3f22c101176a43e694...
==========

[commit-bot: I haz the power, 2017-04-10 21:43:46]

Committed patchset #5 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/4baea33bdc3f22c101176a43e694...