mac: Update layout of the autofill element to get contact permissions.

mac: Update layout of the autofill element to get contact permissions.

The icon should be left-aligned, to the left of the name. I refactored the
layout code of the autofill popup view to better support both image alignment,
as well as RTL alignment.

This CL depends on https://codereview.chromium.org/286243002/, which has not
yet landed.

See the associated bug for visual confirmation of the correctness of the layout
logic.

BUG=139154
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274185

[erikchen, 2014-05-19 23:44:58]

Please review.

[erikchen, 2014-05-21 22:01:38]

ping?

[Ilya Sherman, 2014-05-22 15:40:06]

I think Rachel or Avi might be better reviewers for this than I am.

[Avi (use Gerrit), 2014-05-22 19:58:26]

This looks like reasonable drawing code to me. I'm not familiar enough with
autofill to visualize it all, but LGTM as long as it seems OK to an autofill
peep too.

[groby-ooo-7-16, 2014-05-22 21:22:16]

A couple of stylistic nits, but otherwise LGTM.

(Assuming you visually inspected the output for both LTR/RTL and things look OK.
Maybe attach screenshots on bug?)

https://codereview.chromium.org/296603002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right):

https://codereview.chromium.org/296603002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:41: 
Since this covers three functions, might want to make that clear in comment :)

https://codereview.chromium.org/296603002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:42: // If
rightAlign == YES
nit:|rightAlign|, period

https://codereview.chromium.org/296603002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:107: [self
drawSeparatorWithBounds:rowBounds];
Add a return here, then you can skip needsDraw altogether. I.e

if(SEPARATOR) {
  drawSeparator;
  return;
}

imageFirst = (identifier == ACCESS_CONTACTS);
...

https://codereview.chromium.org/296603002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:165: CGFloat leftX
= bounds.origin.x + AutofillPopupView::kEndPadding;
NSMinX(bounds) + padding

https://codereview.chromium.org/296603002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:167:
bounds.origin.x + bounds.size.width - AutofillPopupView::kEndPadding;
NSMaxX(bounds) + padding;

https://codereview.chromium.org/296603002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:171: // logic is
simply inverted for RTL == YES.
I _think_ all that can be expressed a bit more concisely, but there's nothing
wrong with the current path.

if (isRTL) 
  std::swap(rightX, leftX);

// Draw left side.
CGFloat x = leftX; 
if (imageFirst)
  x = [self drawIconAtIndex:index atX:x rightAlign:isRTL bounds:bounds];
[self drawName:name atX:x index:index rightAlign:isRTL bounds:bounds];

// Draw right side.
x = rightX;
if (!imageFirst)
  x = [self drawIconAtIndex:index atX:x rightAlign:!isRTL bounds:bounds];
[self drawSubtext:subtext atX:x rightAlign:!isRTL bounds:bounds];

[erikchen, 2014-05-22 22:24:35]

I've attached visual confirmations of the correctness of the logic to the
associated bug. All other comments addressed.

https://codereview.chromium.org/296603002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right):

https://codereview.chromium.org/296603002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:41: 
On 2014/05/22 21:22:17, groby wrote:
> Since this covers three functions, might want to make that clear in comment :)

Done.

https://codereview.chromium.org/296603002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:42: // If
rightAlign == YES
On 2014/05/22 21:22:17, groby wrote:
> nit:|rightAlign|, period

Done.

https://codereview.chromium.org/296603002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:107: [self
drawSeparatorWithBounds:rowBounds];
On 2014/05/22 21:22:17, groby wrote:
> Add a return here, then you can skip needsDraw altogether. I.e
> 
> if(SEPARATOR) {
>   drawSeparator;
>   return;
> }
> 
> imageFirst = (identifier == ACCESS_CONTACTS);
> ...

Done. I also simplified the following else if/else logic.

https://codereview.chromium.org/296603002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:165: CGFloat leftX
= bounds.origin.x + AutofillPopupView::kEndPadding;
On 2014/05/22 21:22:17, groby wrote:
> NSMinX(bounds) + padding

Done.

https://codereview.chromium.org/296603002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:167:
bounds.origin.x + bounds.size.width - AutofillPopupView::kEndPadding;
On 2014/05/22 21:22:17, groby wrote:
> NSMaxX(bounds) + padding;

Done.

https://codereview.chromium.org/296603002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:171: // logic is
simply inverted for RTL == YES.
On 2014/05/22 21:22:17, groby wrote:
> I _think_ all that can be expressed a bit more concisely, but there's nothing
> wrong with the current path.
> 
> if (isRTL) 
>   std::swap(rightX, leftX);
> 
> // Draw left side.
> CGFloat x = leftX; 
> if (imageFirst)
>   x = [self drawIconAtIndex:index atX:x rightAlign:isRTL bounds:bounds];
> [self drawName:name atX:x index:index rightAlign:isRTL bounds:bounds];
> 
> // Draw right side.
> x = rightX;
> if (!imageFirst)
>   x = [self drawIconAtIndex:index atX:x rightAlign:!isRTL bounds:bounds];
> [self drawSubtext:subtext atX:x rightAlign:!isRTL bounds:bounds];

I simplified the logic following your example, but avoided the std::swap between
rightX and leftX, as that muddles the meaning of the variables and seems prone
to future errors.

[groby-ooo-7-16, 2014-05-22 22:27:13]

SLGTM

[Ilya Sherman, 2014-05-23 15:28:03]

(With Rachel and Avi's approval, there is no need to wait for my review.)

[erikchen, 2014-06-02 05:44:03]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-06-02 05:44:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikchen@chromium.org/296603002/60001

[commit-bot: I haz the power, 2014-06-02 07:40:47]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[commit-bot: I haz the power, 2014-06-02 08:05:12]

Change committed as 274185

[ma234596, 2014-06-04 06:11:25]

On 2014/06/02 08:05:12, I haz the power (commit-bot) wrote:
> Change committed as 274185