[WebPayments] Extracting common code from Shipping + Contact Info views.

Extracting common code from Shipping + Contact Info views.

BUG=679449
Review-Url: https://codereview.chromium.org/2735803003
Cr-Commit-Position: refs/heads/master@{#456393}
Committed: https://chromium.googlesource.com/chromium/src/+/2118ab72a4ce9ff79eccdbd769451c4ccc042fb1

[tmartino, 2017-03-06 21:23:21]

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

[commit-bot: I haz the power, 2017-03-06 21:24:05]

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

[tmartino, 2017-03-06 21:27:29]

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

[commit-bot: I haz the power, 2017-03-06 23:52:59]

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

[commit-bot: I haz the power, 2017-03-06 23:52:59]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[anthonyvd, 2017-03-07 15:13:15]

Awesome refactor, I'm loving this!

Can you please tag the CL title with [Web Payments]?

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

https://codereview.chromium.org/2735803003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/profile_list_view_controller.cc:56:
columns->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1,
Should the first argument be views::GridLayout::LEADING?

https://codereview.chromium.org/2735803003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/profile_list_view_controller.cc:78: void
ButtonPressed(views::Button* sender, const ui::Event& event) override {}
Can we move this to the base item class and have semantically meaningful virtual
functions in the children instead? I'm thinking of something like:

PaymentRequestListItem::Item::ButtonPressed() {
  if (CanSelect()) {
    SelectItem(this);
  } else {
    DoSomethingElse();
  }
}

Where CanSelect() and DoSomethingElse() (open the editor probably) are pure
virtuals implemented by the concrete item classes.

This would consolidate even more code (and even some in the PaymentMethod
equivalent of this since the ButtonPressed handlers are basically going to be
the same everywhere.

https://codereview.chromium.org/2735803003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/profile_list_view_controller.cc:111:
PaymentRequest* request_;
nit: This could be on the base with a protected accessor.

https://codereview.chromium.org/2735803003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/profile_list_view_controller.cc:172: auto*
selected_profile = request()->selected_shipping_profile();
I *think* the style guide prohibits the use of auto when it expands to a raw
pointer type. Can we spell the type out here?

https://codereview.chromium.org/2735803003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/profile_list_view_controller.h (right):

https://codereview.chromium.org/2735803003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/profile_list_view_controller.h:54: virtual
std::vector<autofill::AutofillProfile*> GetProfiles() = 0;
nit: include <vector>

https://codereview.chromium.org/2735803003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/profile_list_view_controller.h:60:
std::unique_ptr<views::Button> CreateRow(autofill::AutofillProfile* profile);
nit: include <memory>

[tmartino, 2017-03-10 15:53:48]

PTAL. Note added changes to views_util.

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

https://codereview.chromium.org/2735803003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/profile_list_view_controller.cc:56:
columns->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1,
On 2017/03/07 at 15:13:14, anthonyvd wrote:
> Should the first argument be views::GridLayout::LEADING?

Done

https://codereview.chromium.org/2735803003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/profile_list_view_controller.cc:78: void
ButtonPressed(views::Button* sender, const ui::Event& event) override {}
On 2017/03/07 at 15:13:14, anthonyvd wrote:
> Can we move this to the base item class and have semantically meaningful
virtual functions in the children instead? I'm thinking of something like:
> 
> PaymentRequestListItem::Item::ButtonPressed() {
>   if (CanSelect()) {
>     SelectItem(this);
>   } else {
>     DoSomethingElse();
>   }
> }
> 
> Where CanSelect() and DoSomethingElse() (open the editor probably) are pure
virtuals implemented by the concrete item classes.
> 
> This would consolidate even more code (and even some in the PaymentMethod
equivalent of this since the ButtonPressed handlers are basically going to be
the same everywhere.

Agreed, moved. Not adding virtual methods until we implement handlers because I
have no clue what they should be yet.

https://codereview.chromium.org/2735803003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/profile_list_view_controller.cc:111:
PaymentRequest* request_;
On 2017/03/07 at 15:13:14, anthonyvd wrote:
> nit: This could be on the base with a protected accessor.

Done

https://codereview.chromium.org/2735803003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/profile_list_view_controller.cc:172: auto*
selected_profile = request()->selected_shipping_profile();
On 2017/03/07 at 15:13:14, anthonyvd wrote:
> I *think* the style guide prohibits the use of auto when it expands to a raw
pointer type. Can we spell the type out here?

Nope: https://google.github.io/styleguide/cppguide.html#auto

But I'm happy to expand it since you have concerns. I'm leaving it in the
for-loop, though.

https://codereview.chromium.org/2735803003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/profile_list_view_controller.h (right):

https://codereview.chromium.org/2735803003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/profile_list_view_controller.h:54: virtual
std::vector<autofill::AutofillProfile*> GetProfiles() = 0;
On 2017/03/07 at 15:13:14, anthonyvd wrote:
> nit: include <vector>

Done

https://codereview.chromium.org/2735803003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/profile_list_view_controller.h:60:
std::unique_ptr<views::Button> CreateRow(autofill::AutofillProfile* profile);
On 2017/03/07 at 15:13:14, anthonyvd wrote:
> nit: include <memory>

Done

[tmartino, 2017-03-10 15:57:08]

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

[commit-bot: I haz the power, 2017-03-10 15:57:27]

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

[anthonyvd, 2017-03-10 16:09:54]

lgtm

[commit-bot: I haz the power, 2017-03-10 17:16:05]

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

[commit-bot: I haz the power, 2017-03-10 17:16:06]

Dry run: This issue passed the CQ dry run.

[tmartino, 2017-03-10 17:33:01]

The CQ bit was checked by tmartino@chromium.org

[commit-bot: I haz the power, 2017-03-10 17:33:34]

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-03-10 17:40:32]

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

[commit-bot: I haz the power, 2017-03-10 17:40:33]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tmartino, 2017-03-10 17:42:28]

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

[tmartino, 2017-03-10 17:42:29]

Whoops, OWNERS coverage changed with my last patch.

+rouslan for OWNERS on payment_request.h

[please use gerrit instead, 2017-03-10 20:23:12]

lgtm % nits

https://codereview.chromium.org/2735803003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_item_list.h (right):

https://codereview.chromium.org/2735803003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_item_list.h:71: void
ButtonPressed(views::Button* sender, const ui::Event& event) override {
Shouldn't this be protected, so that deriving classes can override it, if
needed?

https://codereview.chromium.org/2735803003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_item_list.h:83:
~PaymentRequestItemList();
Needs to be virtual. Deriving classes should override.

https://codereview.chromium.org/2735803003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_views_util.cc (right):

https://codereview.chromium.org/2735803003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:60:
base::string16 s1,
const-ref these strings to avoid copies.

https://codereview.chromium.org/2735803003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right):

https://codereview.chromium.org/2735803003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:385: // |
Contact info       Barack Obama              |
Let's keep the order of the contact elements consistent throughout to avoid
accidentally using name as email, phone as name, etc.

On lines 375-378, the order is name-email-phone. In this comment, the order is
name-phone-email. Please pick one style (preferably the one in the mocks, so
building UI is easier) and stick with it.

That is, please update all the files in this patch to use the same exact
ordering. Thank you!

https://codereview.chromium.org/2735803003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/profile_list_view_controller.cc (right):

https://codereview.chromium.org/2735803003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/profile_list_view_controller.cc:79: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/2735803003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/profile_list_view_controller.h (right):

https://codereview.chromium.org/2735803003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/profile_list_view_controller.h:18: 
namespace views {
class Button;
}

https://codereview.chromium.org/2735803003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/profile_list_view_controller.h:62:
PaymentRequest* request_;
Add a comment: "// Not owned. Never null. Outlives this object."

[tmartino, 2017-03-10 22:23:40]

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

[commit-bot: I haz the power, 2017-03-10 22:24:00]

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-03-10 23:45:21]

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

[commit-bot: I haz the power, 2017-03-10 23:45:22]

Dry run: This issue passed the CQ dry run.

[tmartino, 2017-03-10 23:46:17]

The CQ bit was checked by tmartino@chromium.org

[tmartino, 2017-03-10 23:46:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
rouslan@chromium.org, anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/2735803003/#ps60001
(title: "rouslan nits")

[tmartino, 2017-03-10 23:46:23]

https://codereview.chromium.org/2735803003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_item_list.h (right):

https://codereview.chromium.org/2735803003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_item_list.h:71: void
ButtonPressed(views::Button* sender, const ui::Event& event) override {
On 2017/03/10 at 20:23:12, ಠ_ಠ wrote:
> Shouldn't this be protected, so that deriving classes can override it, if
needed?

Per anthonyvd's comment, we're going to first try exposing semantically
meaningful chunks of this as pure virtuals, e.g., CanSelect or SelectItem. If it
turns out we need more flexibility we can always make it protected later.

https://codereview.chromium.org/2735803003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_item_list.h:83:
~PaymentRequestItemList();
On 2017/03/10 at 20:23:12, ಠ_ಠ wrote:
> Needs to be virtual. Deriving classes should override.

Done

https://codereview.chromium.org/2735803003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_views_util.cc (right):

https://codereview.chromium.org/2735803003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:60:
base::string16 s1,
On 2017/03/10 at 20:23:12, ಠ_ಠ wrote:
> const-ref these strings to avoid copies.

Done

https://codereview.chromium.org/2735803003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right):

https://codereview.chromium.org/2735803003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:385: // |
Contact info       Barack Obama              |
On 2017/03/10 at 20:23:12, ಠ_ಠ wrote:
> Let's keep the order of the contact elements consistent throughout to avoid
accidentally using name as email, phone as name, etc.
> 
> On lines 375-378, the order is name-email-phone. In this comment, the order is
name-phone-email. Please pick one style (preferably the one in the mocks, so
building UI is easier) and stick with it.
> 
> That is, please update all the files in this patch to use the same exact
ordering. Thank you!

Done

https://codereview.chromium.org/2735803003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/profile_list_view_controller.cc (right):

https://codereview.chromium.org/2735803003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/profile_list_view_controller.cc:79: };
On 2017/03/10 at 20:23:12, ಠ_ಠ wrote:
> DISALLOW_COPY_AND_ASSIGN

Done

https://codereview.chromium.org/2735803003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/profile_list_view_controller.h (right):

https://codereview.chromium.org/2735803003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/profile_list_view_controller.h:18: 
On 2017/03/10 at 20:23:12, ಠ_ಠ wrote:
> namespace views {
> class Button;
> }

Done. Also added View

https://codereview.chromium.org/2735803003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/profile_list_view_controller.h:62:
PaymentRequest* request_;
On 2017/03/10 at 20:23:12, ಠ_ಠ wrote:
> Add a comment: "// Not owned. Never null. Outlives this object."

Done

[commit-bot: I haz the power, 2017-03-10 23:46:37]

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-03-10 23:52:23]

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

[commit-bot: I haz the power, 2017-03-10 23:52:24]

Failed to apply patch for components/payments/content/payment_request.h:
While running git apply --index -p1;
  error: patch failed: components/payments/content/payment_request.h:143
  error: components/payments/content/payment_request.h: patch does not apply

Patch:       components/payments/content/payment_request.h
Index: components/payments/content/payment_request.h
diff --git a/components/payments/content/payment_request.h
b/components/payments/content/payment_request.h
index
8ae228f5dab23bb4a1683243ebb4334ac3cac281..e51eec457d175731315ae58f2d0f387a7327cbe2
100644
--- a/components/payments/content/payment_request.h
+++ b/components/payments/content/payment_request.h
@@ -143,6 +143,7 @@ class PaymentRequest : public mojom::PaymentRequest,
   autofill::PersonalDataManager* personal_data_manager() {
     return delegate_->GetPersonalDataManager();
   }
+  const std::string& locale() { return delegate_->GetApplicationLocale(); }
 
   payments::mojom::PaymentDetails* details() { return details_.get(); }
   const std::vector<std::string>& supported_card_networks() {

[tmartino, 2017-03-13 14:38:21]

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

[commit-bot: I haz the power, 2017-03-13 14:38:33]

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-03-13 15:58:35]

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

[commit-bot: I haz the power, 2017-03-13 15:58:36]

Dry run: This issue passed the CQ dry run.

[tmartino, 2017-03-13 16:02:42]

The CQ bit was checked by tmartino@chromium.org

[tmartino, 2017-03-13 16:02:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
rouslan@chromium.org, anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/2735803003/#ps80001
(title: "monday rebase")

[commit-bot: I haz the power, 2017-03-13 16:03:26]

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-03-13 16:08:50]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489420962229930,
"parent_rev": "ba2dd95d1e41ffcc9c9f0f10909619fd7a173ef5", "commit_rev":
"2118ab72a4ce9ff79eccdbd769451c4ccc042fb1"}

[commit-bot: I haz the power, 2017-03-13 16:09:54]

Description was changed from

==========
Extracting common code from Shipping + Contact Info views.

BUG=679449
==========

to

==========
Extracting common code from Shipping + Contact Info views.

BUG=679449

Review-Url: https://codereview.chromium.org/2735803003
Cr-Commit-Position: refs/heads/master@{#456393}
Committed:
https://chromium.googlesource.com/chromium/src/+/2118ab72a4ce9ff79eccdbd76945...
==========

[commit-bot: I haz the power, 2017-03-13 16:09:59]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/2118ab72a4ce9ff79eccdbd76945...