[Web Payments] Refactor the row display code.

[Web Payments] Refactor the row display code.

This CL extracts the common bits of row views into a separate class.
This will allow the hover effect as well as the border styling to be
used in multiple screens easily.

The rows also now properly display their hover effects.

BUG=685209
Review-Url: https://codereview.chromium.org/2656853002
Cr-Commit-Position: refs/heads/master@{#446821}
Committed: https://chromium.googlesource.com/chromium/src/+/8580766009627c5aeb0625172c3f3c576d7c3636

[anthonyvd, 2017-01-25 16:10:15]

Patchset #1 (id:1) has been deleted

[anthonyvd, 2017-01-25 16:13:53]

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

[anthonyvd, 2017-01-25 16:13:53]

Hi guys, can you PTAL at this CL? It refactors the rows display code of the
dialog to prepare it for reuse in the Payment Method selection sheet. 

Since the specifics of the refactor were driven by the common hover effects and
horizontal ruler of rows in different screens, this CL also implements those
small visual tweaks.

Thanks!

[please use gerrit instead, 2017-01-25 16:22:55]

lgtm % comments

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

https://codereview.chromium.org/2656853002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_row_view.cc:23: state() ==
views::Button::STATE_PRESSED) {
nit: I'd prefer hover and press to have different border colors, but don't take
my UX advise as gospel. We have UX people for that ;-)

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

https://codereview.chromium.org/2656853002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:57: 
Need to override the parent destructor.

https://codereview.chromium.org/2656853002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:58: protected:
This class has no children, so it's OK to make GetMinimumSize() and Paint()
either public or private. No need for protected methods.

[Mathieu, 2017-01-25 16:45:39]

lgtm

[anthonyvd, 2017-01-25 17:56:32]

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

[anthonyvd, 2017-01-25 17:56:33]

Thanks Math and Rouslan for the review!

sky@ can you PTAL as c/b/ui/views/* owner? In particular, can you advise on
whether there's a better way of having views::View::CanProcessEventsInSubtree()
return false without subclassing views::View like this CL is doing?

Thanks!

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

https://codereview.chromium.org/2656853002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_row_view.cc:23: state() ==
views::Button::STATE_PRESSED) {
On 2017/01/25 at 16:22:55, rouslan wrote:
> nit: I'd prefer hover and press to have different border colors, but don't
take my UX advise as gospel. We have UX people for that ;-)

Yeah the mocks aren't too specific about the difference between hover and
pressed state but I'd assume we'll want a slightly different highlight. Borders
disappear when the row is highlighted though (in this case both the highlight
and border are the same color so the effect is achieved).

I'll loop in UXers to take a look when this is in Canary, this way they'll be
able to try it out and advise!

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

https://codereview.chromium.org/2656853002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:57: 
On 2017/01/25 at 16:22:55, rouslan wrote:
> Need to override the parent destructor.

Good catch! Done here and in the other classes this CL adds.

https://codereview.chromium.org/2656853002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:58: protected:
On 2017/01/25 at 16:22:55, rouslan wrote:
> This class has no children, so it's OK to make GetMinimumSize() and Paint()
either public or private. No need for protected methods.

Done.

[sky, 2017-01-25 21:45:20]

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

https://codereview.chromium.org/2656853002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_row_view.cc:29: }
Don't you need a SchedulePaint here? View should really do that for you, but it
doesn't currently.

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

https://codereview.chromium.org/2656853002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:59: gfx::Size
GetMinimumSize() const override {
Prefix with where overrides come from.

https://codereview.chromium.org/2656853002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:64: int
line_height = size.height() - 1;
If you want a 1 pixel line then you'll need to potentially invert the scale when
the device scale is > 1.

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

https://codereview.chromium.org/2656853002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:72: bool
CanProcessEventsWithinSubtree() const override { return false; }
You are right that subclassing like is the only way to change the behavior. I'm
actually fine with adding a setter to View.

[anthonyvd, 2017-01-26 18:56:06]

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

https://codereview.chromium.org/2656853002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_row_view.cc:29: }
On 2017/01/25 at 21:45:20, sky wrote:
> Don't you need a SchedulePaint here? View should really do that for you, but
it doesn't currently.

I doesn't *look* like it, CustomButton calls it itself after calling
StateChanged() and that seems to be the only call to StateChanged ever.

See
https://cs.chromium.org/chromium/src/ui/views/controls/button/custom_button.c...

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

https://codereview.chromium.org/2656853002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:59: gfx::Size
GetMinimumSize() const override {
On 2017/01/25 at 21:45:20, sky wrote:
> Prefix with where overrides come from.

Done.

https://codereview.chromium.org/2656853002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:64: int
line_height = size.height() - 1;
On 2017/01/25 at 21:45:20, sky wrote:
> If you want a 1 pixel line then you'll need to potentially invert the scale
when the device scale is > 1.

Never encountered any device-scale dependent stuff and I do want a 1px line.
What's the scale I need to invert? Do Canvas::Draw* functions not take pixel
arguments?

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

https://codereview.chromium.org/2656853002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:72: bool
CanProcessEventsWithinSubtree() const override { return false; }
On 2017/01/25 at 21:45:20, sky wrote:
> You are right that subclassing like is the only way to change the behavior.
I'm actually fine with adding a setter to View.

Went ahead and added the setter. This'll be much simpler.

[sky, 2017-01-26 19:11:58]

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

https://codereview.chromium.org/2656853002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:64: int
line_height = size.height() - 1;
On 2017/01/26 18:56:06, anthonyvd wrote:
> On 2017/01/25 at 21:45:20, sky wrote:
> > If you want a 1 pixel line then you'll need to potentially invert the scale
> when the device scale is > 1.
> 
> Never encountered any device-scale dependent stuff and I do want a 1px line.
> What's the scale I need to invert? Do Canvas::Draw* functions not take pixel
> arguments?

Unless otherwise stated views and related libraries use dips. See
BrowserView::Paint1pxHorizontalLine for an example of drawing a pixel.

[anthonyvd, 2017-01-26 21:00:23]

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

https://codereview.chromium.org/2656853002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:64: int
line_height = size.height() - 1;
On 2017/01/26 at 19:11:58, sky wrote:
> On 2017/01/26 18:56:06, anthonyvd wrote:
> > On 2017/01/25 at 21:45:20, sky wrote:
> > > If you want a 1 pixel line then you'll need to potentially invert the
scale
> > when the device scale is > 1.
> > 
> > Never encountered any device-scale dependent stuff and I do want a 1px line.
> > What's the scale I need to invert? Do Canvas::Draw* functions not take pixel
> > arguments?
> 
> Unless otherwise stated views and related libraries use dips. See
BrowserView::Paint1pxHorizontalLine for an example of drawing a pixel.

Alright, talked with my team and our UXer, turns out 1dip is what we want. In
that case is this Draw call correct?

[sky, 2017-01-26 23:01:03]

Yes, LGTM

[anthonyvd, 2017-01-27 13:02:21]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2017-01-27 13:02:21]

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

[commit-bot: I haz the power, 2017-01-27 13:02:42]

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-01-27 14:54:12]

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

[commit-bot: I haz the power, 2017-01-27 14:54:13]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[anthonyvd, 2017-01-27 16:13:38]

The CQ bit was checked by anthonyvd@chromium.org

[commit-bot: I haz the power, 2017-01-27 16:14:03]

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-01-27 17:06:49]

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

[commit-bot: I haz the power, 2017-01-27 17:06:50]

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

[anthonyvd, 2017-01-27 21:33:01]

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

[commit-bot: I haz the power, 2017-01-27 21:33: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-01-27 23:02:14]

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

[commit-bot: I haz the power, 2017-01-27 23:02:15]

Dry run: This issue passed the CQ dry run.

[anthonyvd, 2017-01-27 23:10:02]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2017-01-27 23:10:03]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
rouslan@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2656853002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-27 23:10: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-01-27 23:16:39]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485558602886120,
"parent_rev": "ac485f81fdf1bb3c16921cd3e69cc89370f01c6e", "commit_rev":
"8580766009627c5aeb0625172c3f3c576d7c3636"}

[commit-bot: I haz the power, 2017-01-27 23:17:31]

Description was changed from

==========
[Web Payments] Refactor the row display code.

This CL extracts the common bits of row views into a separate class.
This will allow the hover effect as well as the border styling to be
used in multiple screens easily.

The rows also now properly display their hover effects.

BUG=685209
==========

to

==========
[Web Payments] Refactor the row display code.

This CL extracts the common bits of row views into a separate class.
This will allow the hover effect as well as the border styling to be
used in multiple screens easily.

The rows also now properly display their hover effects.

BUG=685209

Review-Url: https://codereview.chromium.org/2656853002
Cr-Commit-Position: refs/heads/master@{#446821}
Committed:
https://chromium.googlesource.com/chromium/src/+/8580766009627c5aeb0625172c3f...
==========

[commit-bot: I haz the power, 2017-01-27 23:17:33]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/8580766009627c5aeb0625172c3f...