[rAc OSX] Factor out AutofillHeader class to contain dialog header elements.

[rAc OSX] Factor out AutofillHeader class to contain dialog header elements.

Currently contains the title and the account chooser.  In the next CL, I'm
planning to move notifications into the header as well.

BUG=315977
TEST=none
R=groby@chromium.org

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

[groby-ooo-7-16, 2013-11-20 02:14:51]

Ahhhh. Nooooo! Please let me land the WindowController first! (
https://codereview.chromium.org/73293002/)


On Tue, Nov 19, 2013 at 6:02 PM, <isherman@chromium.org> wrote:

> Reviewers: groby,
>
> Description:
> [rAc OSX] Factor out AutofillHeader class to contain dialog header
> elements.
>
> Currently contains the title and the account chooser.  In the next CL, I'm
> planning to move notifications into the header as well.
>
> BUG=315977
> TEST=none
> R=groby@chromium.org
>
> Please review this at https://codereview.chromium.org/77283002/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+170, -65 lines):
>   M chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.h
>   M chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm
>   M chrome/browser/ui/cocoa/autofill/autofill_dialog_constants.h
>   A chrome/browser/ui/cocoa/autofill/autofill_header.h
>   A chrome/browser/ui/cocoa/autofill/autofill_header.mm
>   M chrome/chrome_browser_ui.gypi
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ilya Sherman, 2013-11-20 06:46:44]

On 2013/11/20 02:14:51, groby wrote:
> Ahhhh. Nooooo! Please let me land the WindowController first! (
> https://codereview.chromium.org/73293002/)

Go for it!  I'm happy to let you land your change and then rebase on top of it
:)

[groby-ooo-7-16, 2013-11-20 23:51:44]

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm (right):

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:37: const CGFloat
kDecorationHeight = autofill::kAccountChooserHeight +
If this is part of the header, why do we need this in size computations?

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:417: 
Header size probably needs to be taken into account here.

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_dialog_constants.h (right):

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_dialog_constants.h:35: // Height of
the account chooser.
I'd prefer a not-shared constant...

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_header.h (right):

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_header.h:19: @interface AutofillHeader
: NSView<AutofillLayout> {
Please make this an NSViewController - unless things do custom drawing, they
should rarely be NSViews.

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_header.mm (right):

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_header.mm:50: return
chrome_style::kTitleTopPadding +
Why? Why not just have preferredSize return the right value? (With a width of 0)

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_header.mm:65: [title_
setStringValue:base::SysUTF16ToNSString(delegate_->DialogTitle())];
Yes. No. Maybe. :)

No, because it resizes a dialog field - which would otherwise happen before the
rest of the dialog resizes.
Yes, because the resize doesn't affect another field.

So I'd say maybe, with a strong side of yes.

https://codereview.chromium.org/77283002/diff/1/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/77283002/diff/1/chrome/chrome_browser_ui.gypi...
chrome/chrome_browser_ui.gypi:522:
'browser/ui/cocoa/autofill/autofill_header.mm',
Alphabetize?

[Ilya Sherman, 2013-11-22 05:01:12]

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm (right):

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:37: const CGFloat
kDecorationHeight = autofill::kAccountChooserHeight +
On 2013/11/20 23:51:44, groby wrote:
> If this is part of the header, why do we need this in size computations?

Done.

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:417: 
On 2013/11/20 23:51:44, groby wrote:
> Header size probably needs to be taken into account here.

Done.

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_dialog_constants.h (right):

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_dialog_constants.h:35: // Height of
the account chooser.
On 2013/11/20 23:51:44, groby wrote:
> I'd prefer a not-shared constant...

Done.

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_header.h (right):

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_header.h:19: @interface AutofillHeader
: NSView<AutofillLayout> {
On 2013/11/20 23:51:44, groby wrote:
> Please make this an NSViewController - unless things do custom drawing, they
> should rarely be NSViews.

I made this an NSView because I wanted a flipped view.  So, two questions:

(1) How do you recommend having a flipped view if I switch this over to be an
NSViewController?
(2) What's the advantage of inheriting from NSViewController?  From what I've
seen, that just adds an extra object into the mix, as our NSViewControllers all
have one primary view that we attach a bunch of subviews to, and clients of the
controller frequently access that primary view directly as [controller view]. 
So, it doesn't seem like we instantiate fewer views, nor do we provide a smaller
API surface.  What's the benefit?

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_header.mm (right):

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_header.mm:50: return
chrome_style::kTitleTopPadding +
On 2013/11/20 23:51:44, groby wrote:
> Why? Why not just have preferredSize return the right value? (With a width of
0)

Well, I could do that right now, but I kept -heightForWidth: around because
it'll be needed as soon as I tuck notifications into the header, which I was
planning to do next.  I figured this way we could have less code churn, but I'm
happy to move this into -preferredSize: for now if it really bugs you :)

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_header.mm:65: [title_
setStringValue:base::SysUTF16ToNSString(delegate_->DialogTitle())];
On 2013/11/20 23:51:44, groby wrote:
> Yes. No. Maybe. :)
> 
> No, because it resizes a dialog field - which would otherwise happen before
the
> rest of the dialog resizes.
> Yes, because the resize doesn't affect another field.
> 
> So I'd say maybe, with a strong side of yes.

I'm doin' it! :)

https://codereview.chromium.org/77283002/diff/1/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/77283002/diff/1/chrome/chrome_browser_ui.gypi...
chrome/chrome_browser_ui.gypi:522:
'browser/ui/cocoa/autofill/autofill_header.mm',
On 2013/11/20 23:51:44, groby wrote:
> Alphabetize?

Maybe my powers of alphabetization are failing me, but I'm pretty sure these are
already positioned correctly.  I did alphabetize some lines below, though :)

[groby-ooo-7-16, 2013-11-22 14:51:37]

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_header.h (right):

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_header.h:19: @interface AutofillHeader
: NSView<AutofillLayout> {
On 2013/11/22 05:01:13, Ilya Sherman wrote:
> On 2013/11/20 23:51:44, groby wrote:
> > Please make this an NSViewController - unless things do custom drawing, they
> > should rarely be NSViews.
> 
> I made this an NSView because I wanted a flipped view.  So, two questions:
> 
> (1) How do you recommend having a flipped view if I switch this over to be an
> NSViewController?
> (2) What's the advantage of inheriting from NSViewController?  From what I've
> seen, that just adds an extra object into the mix, as our NSViewControllers
all
> have one primary view that we attach a bunch of subviews to, and clients of
the
> controller frequently access that primary view directly as [controller view]. 
> So, it doesn't seem like we instantiate fewer views, nor do we provide a
smaller
> API surface.  What's the benefit?

Don't make it flipped unless you have to :) (The easiest way to do it is a
top-level FlippedView, and then children beneath that)

As to the larger NSViewController question, it's a stylistic choice, separating
logic and presentation. In general, we try to only use views for things that
need drawing things. I don't feel that strongly about it (since NSViewController
is, uh, anemic at best :), but in previous reviews of my code, I've received
strong pushback on not using a controller.

Maybe it's worth raising this with the other Cocoa peeps on Chrome?

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_header.mm (right):

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_header.mm:50: return
chrome_style::kTitleTopPadding +
On 2013/11/22 05:01:13, Ilya Sherman wrote:
> On 2013/11/20 23:51:44, groby wrote:
> > Why? Why not just have preferredSize return the right value? (With a width
of
> 0)
> 
> Well, I could do that right now, but I kept -heightForWidth: around because
> it'll be needed as soon as I tuck notifications into the header, which I was
> planning to do next.  I figured this way we could have less code churn, but
I'm
> happy to move this into -preferredSize: for now if it really bugs you :)

If we need it anyways, that'd be kind of silly :) Wondering if we should extend
the AutofillLayout protocol to include heightForWidth... (Don't do it here, but
I'd love to hear your thoughts)

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_header.mm:65: [title_
setStringValue:base::SysUTF16ToNSString(delegate_->DialogTitle())];
On 2013/11/22 05:01:13, Ilya Sherman wrote:
> On 2013/11/20 23:51:44, groby wrote:
> > Yes. No. Maybe. :)
> > 
> > No, because it resizes a dialog field - which would otherwise happen before
> the
> > rest of the dialog resizes.
> > Yes, because the resize doesn't affect another field.
> > 
> > So I'd say maybe, with a strong side of yes.
> 
> I'm doin' it! :)

You might want to  pay close attention to the redraws - I vaguely recall I did
this to reduce flicker, but I'm not 100% certain.

https://codereview.chromium.org/77283002/diff/1/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/77283002/diff/1/chrome/chrome_browser_ui.gypi...
chrome/chrome_browser_ui.gypi:522:
'browser/ui/cocoa/autofill/autofill_header.mm',
On 2013/11/22 05:01:13, Ilya Sherman wrote:
> On 2013/11/20 23:51:44, groby wrote:
> > Alphabetize?
> 
> Maybe my powers of alphabetization are failing me, but I'm pretty sure these
are
> already positioned correctly.  I did alphabetize some lines below, though :)

Maybe I'm crazy, but it was in a different location when I wrote this :)

(It was also in a different font than the rest, so I suspect one of my 35 "Make
Rietveld actually useful" plugins was acting up...)

https://codereview.chromium.org/77283002/diff/180001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/autofill/autofill_dialog_window_controller.mm
(right):

https://codereview.chromium.org/77283002/diff/180001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/autofill_dialog_window_controller.mm:198: //
Always make room for the header.
This comment is confusing if combined with the bottom padding

https://codereview.chromium.org/77283002/diff/180001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/autofill_dialog_window_controller.mm:200:
[header_ frame].size.height + chrome_style::kClientBottomPadding;
You might want to use heightForWidth here. (Since preferredSize is "what size
would you _like_ to be")

https://codereview.chromium.org/77283002/diff/180001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/autofill/autofill_header.mm (right):

https://codereview.chromium.org/77283002/diff/180001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/autofill_header.mm:46: }
Does this really need to be flipped?

[Ilya Sherman, 2013-11-23 00:23:53]

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_header.h (right):

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_header.h:19: @interface AutofillHeader
: NSView<AutofillLayout> {
On 2013/11/22 14:51:37, groby wrote:
> On 2013/11/22 05:01:13, Ilya Sherman wrote:
> > On 2013/11/20 23:51:44, groby wrote:
> > > Please make this an NSViewController - unless things do custom drawing,
they
> > > should rarely be NSViews.
> > 
> > I made this an NSView because I wanted a flipped view.  So, two questions:
> > 
> > (1) How do you recommend having a flipped view if I switch this over to be
an
> > NSViewController?
> > (2) What's the advantage of inheriting from NSViewController?  From what
I've
> > seen, that just adds an extra object into the mix, as our NSViewControllers
> all
> > have one primary view that we attach a bunch of subviews to, and clients of
> the
> > controller frequently access that primary view directly as [controller
view]. 
> > So, it doesn't seem like we instantiate fewer views, nor do we provide a
> smaller
> > API surface.  What's the benefit?
> 
> Don't make it flipped unless you have to :) (The easiest way to do it is a
> top-level FlippedView, and then children beneath that)

Well, fair enough :)

> As to the larger NSViewController question, it's a stylistic choice,
separating
> logic and presentation. In general, we try to only use views for things that
> need drawing things. I don't feel that strongly about it (since
NSViewController
> is, uh, anemic at best :), but in previous reviews of my code, I've received
> strong pushback on not using a controller.
> 
> Maybe it's worth raising this with the other Cocoa peeps on Chrome?

Made it an NSViewController, because I do like encapsulating the contents of
-update: within this class, and I guess I'm fine with the tiny bit of overhead
from separating out the view and the controller as per MVC.

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
File chrome/browser/ui/cocoa/autofill/autofill_header.mm (right):

https://codereview.chromium.org/77283002/diff/1/chrome/browser/ui/cocoa/autof...
chrome/browser/ui/cocoa/autofill/autofill_header.mm:50: return
chrome_style::kTitleTopPadding +
On 2013/11/22 14:51:37, groby wrote:
> On 2013/11/22 05:01:13, Ilya Sherman wrote:
> > On 2013/11/20 23:51:44, groby wrote:
> > > Why? Why not just have preferredSize return the right value? (With a width
> of
> > 0)
> > 
> > Well, I could do that right now, but I kept -heightForWidth: around because
> > it'll be needed as soon as I tuck notifications into the header, which I was
> > planning to do next.  I figured this way we could have less code churn, but
> I'm
> > happy to move this into -preferredSize: for now if it really bugs you :)
> 
> If we need it anyways, that'd be kind of silly :) Wondering if we should
extend
> the AutofillLayout protocol to include heightForWidth... (Don't do it here,
but
> I'd love to hear your thoughts)

Yeah, I was thinking about that too -- maybe heightForWidth ought to be the
default, and preferredSize the exception?

https://codereview.chromium.org/77283002/diff/180001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/autofill/autofill_dialog_window_controller.mm
(right):

https://codereview.chromium.org/77283002/diff/180001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/autofill_dialog_window_controller.mm:198: //
Always make room for the header.
On 2013/11/22 14:51:38, groby wrote:
> This comment is confusing if combined with the bottom padding

Done.

https://codereview.chromium.org/77283002/diff/180001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/autofill_dialog_window_controller.mm:200:
[header_ frame].size.height + chrome_style::kClientBottomPadding;
On 2013/11/22 14:51:38, groby wrote:
> You might want to use heightForWidth here. (Since preferredSize is "what size
> would you _like_ to be")

Done.

https://codereview.chromium.org/77283002/diff/180001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/autofill/autofill_header.mm (right):

https://codereview.chromium.org/77283002/diff/180001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/autofill_header.mm:46: }
On 2013/11/22 14:51:38, groby wrote:
> Does this really need to be flipped?

Done.

[Ilya Sherman, 2013-11-23 00:27:12]

https://codereview.chromium.org/77283002/diff/300001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/autofill/autofill_header.mm (right):

https://codereview.chromium.org/77283002/diff/300001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/autofill_header.mm:73:
autofill::kDetailVerticalPadding)];
I got rid of the layout_manager stuff because this way aligns the baselines of
the text with one another, which is what the mocks actually ask for.  (Well,
it's technically maybe a pixel off, but it's close enough IMO.)

[groby-ooo-7-16, 2013-11-23 02:07:19]

lgtm

https://codereview.chromium.org/77283002/diff/300001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/autofill/autofill_header.mm (right):

https://codereview.chromium.org/77283002/diff/300001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/autofill/autofill_header.mm:73:
autofill::kDetailVerticalPadding)];
On 2013/11/23 00:27:13, Ilya Sherman wrote:
> I got rid of the layout_manager stuff because this way aligns the baselines of
> the text with one another, which is what the mocks actually ask for.  (Well,
> it's technically maybe a pixel off, but it's close enough IMO.)

I'm not sure how that would align the baselines - but if it works out that way,
let's keep it.

[commit-bot: I haz the power, 2013-11-23 02:37:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/77283002/430001

[commit-bot: I haz the power, 2013-11-23 02:57:56]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-11-23 22:35:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/77283002/430001

[commit-bot: I haz the power, 2013-11-23 23:54:33]

Retried try job too often on mac_rel for step(s) app_list_unittests,
base_unittests, browser_tests, cacheinvalidation_unittests, cc_unittests,
check_deps, chromedriver2_unittests, components_unittests, content_browsertests,
content_unittests, crypto_unittests, google_apis_unittests, gpu_unittests,
interactive_ui_tests, ipc_tests, jingle_unittests, media_unittests,
message_center_unittests, nacl_integration, net_unittests, ppapi_unittests,
printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests,
sync_unit_tests, telemetry_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-11-24 01:18:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/77283002/180003

[commit-bot: I haz the power, 2013-11-24 19:09:15]

Change committed as 236997