[Mac] Omnibox keyword, keyword hint, and search hint support.

[Mac] Omnibox keyword, keyword hint, and search hint support.

http://crbug.com/10944
http://crbug.com/10943
http://crbug.com/12285

TEST=With empty field, should see "Type to search" in light text on
the RHS of the omnibox.  Type "google" and should see "Press [Tab] to
search Google" in light text on the RHS.  Hit TAB, should see a
rounded [Search Google:] item on the left and the cursor is now right
of that.  Popup should show "Keyword:" in some entries.

[Scott Hess - ex-Googler, 2009-07-23 00:46:33]

Sorry to dump this on you, but ... I've been trying to get this ready to go,
which obviously involves pumping in a few unit tests, but I'm a little nervous
about whether I'll be able to get that done, this submitted for review, then
checked in before Friday afternoon.

Since I'm going out of town on Saturday, I'd rather get that done earlier than
later (it's the bulk of my remaining deliverables).  I think I'll have the unit
tests ready tomorrow.  So I figure I should throw this up and see if you're
willing to look at it for overall direction or not.  Or possibly delegate it to
some other party.

Thanks,
scott

[John Grabowski, 2009-07-23 05:21:37]

Scott, my general impression is that I have little idea what is going on here! 
As an example, the omnibox has both an NSTextField and an NSTextView; from the
diff and context and can't easily tell which is used for what.  I'd guess the
text field is for entry and the text view is the pop-up?

If you have time I'd appreciate it if you could do a higher level overview of
the classes you are using and the function they represent so I can better get my
head around things.

http://codereview.chromium.org/159250/diff/1/5
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/159250/diff/1/5#newcode36
Line 36: - (void)setKeywordString:(NSString*)aString {
If convenient, keep them.  However, it seems common in Cocoa to call
  [[NSControlThing cell] doSomething].
Your wrappers give the hint that the cell is somehow private.
Note [NSControl cell] returns an (id), not an NSCell, to turn off type checking.

[Scott Hess - ex-Googler, 2009-07-23 17:08:29]

John,

I've added more comments.  Bring popcorn.  Haven't gotten around to making code
changes.

-scott


On 2009/07/23 05:21:37, John Grabowski wrote:
> Scott, my general impression is that I have little idea what is going on here!

> As an example, the omnibox has both an NSTextField and an NSTextView; from the
> diff and context and can't easily tell which is used for what.  I'd guess the
> text field is for entry and the text view is the pop-up?
> 
> If you have time I'd appreciate it if you could do a higher level overview of
> the classes you are using and the function they represent so I can better get
my
> head around things.
> 
> http://codereview.chromium.org/159250/diff/1/5
> File chrome/browser/cocoa/autocomplete_text_field.mm (right):
> 
> http://codereview.chromium.org/159250/diff/1/5#newcode36
> Line 36: - (void)setKeywordString:(NSString*)aString {
> If convenient, keep them.  However, it seems common in Cocoa to call
>   [[NSControlThing cell] doSomething].
> Your wrappers give the hint that the cell is somehow private.
> Note [NSControl cell] returns an (id), not an NSCell, to turn off type
checking.

[John Grabowski, 2009-07-23 18:08:08]

Will do.
jrg


On Thu, Jul 23, 2009 at 10:08 AM, <shess@chromium.org> wrote:

> John,
>
> I've added more comments.  Bring popcorn.  Haven't gotten around to
> making code changes.
>
> -scott
>
>
>
> On 2009/07/23 05:21:37, John Grabowski wrote:
>
>> Scott, my general impression is that I have little idea what is going
>>
> on here!
>
>> As an example, the omnibox has both an NSTextField and an NSTextView;
>>
> from the
>
>> diff and context and can't easily tell which is used for what.  I'd
>>
> guess the
>
>> text field is for entry and the text view is the pop-up?
>>
>
>  If you have time I'd appreciate it if you could do a higher level
>>
> overview of
>
>> the classes you are using and the function they represent so I can
>>
> better get my
>
>> head around things.
>>
>
>  http://codereview.chromium.org/159250/diff/1/5
>> File chrome/browser/cocoa/autocomplete_text_field.mm (right):
>>
>
>  http://codereview.chromium.org/159250/diff/1/5#newcode36
>> Line 36: - (void)setKeywordString:(NSString*)aString {
>> If convenient, keep them.  However, it seems common in Cocoa to call
>>   [[NSControlThing cell] doSomething].
>> Your wrappers give the hint that the cell is somehow private.
>> Note [NSControl cell] returns an (id), not an NSCell, to turn off type
>>
> checking.
>
>
>
> http://codereview.chromium.org/159250
>

[Scott Hess - ex-Googler, 2009-07-23 22:46:51]

John, I've put up some unit tests, but I haven't had time to fix a couple bugs
they exposed.  Yay tests!  Boo time!  I marked them with TODO(shess):, they must
be fixed before checkin.  I've been called to home, though, and I won't probably
get back to this until tomorrow morning.

Thanks, -scott


On 2009/07/23 18:08:08, John Grabowski wrote:
> Will do.
> jrg
> 
> 
> On Thu, Jul 23, 2009 at 10:08 AM, <shess@chromium.org> wrote:
> 
> > John,
> >
> > I've added more comments.  Bring popcorn.  Haven't gotten around to
> > making code changes.
> >
> > -scott
> >
> >
> >
> > On 2009/07/23 05:21:37, John Grabowski wrote:
> >
> >> Scott, my general impression is that I have little idea what is going
> >>
> > on here!
> >
> >> As an example, the omnibox has both an NSTextField and an NSTextView;
> >>
> > from the
> >
> >> diff and context and can't easily tell which is used for what.  I'd
> >>
> > guess the
> >
> >> text field is for entry and the text view is the pop-up?
> >>
> >
> >  If you have time I'd appreciate it if you could do a higher level
> >>
> > overview of
> >
> >> the classes you are using and the function they represent so I can
> >>
> > better get my
> >
> >> head around things.
> >>
> >
> >  http://codereview.chromium.org/159250/diff/1/5
> >> File chrome/browser/cocoa/autocomplete_text_field.mm (right):
> >>
> >
> >  http://codereview.chromium.org/159250/diff/1/5#newcode36
> >> Line 36: - (void)setKeywordString:(NSString*)aString {
> >> If convenient, keep them.  However, it seems common in Cocoa to call
> >>   [[NSControlThing cell] doSomething].
> >> Your wrappers give the hint that the cell is somehow private.
> >> Note [NSControl cell] returns an (id), not an NSCell, to turn off type
> >>
> > checking.
> >
> >
> >
> > http://codereview.chromium.org/159250
> >
>

[John Grabowski, 2009-08-04 23:10:19]

2nd pass with real comments.
At a high level, I like.

http://codereview.chromium.org/159250/diff/22/2043
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/159250/diff/22/2043#newcode291
Line 291: controller_->OnChanged();
From the header I see this:
// TODO(pkasting): http://b/1343512  The names and contents of the classes in
// this file are temporary.  I am in hack-and-slash mode right now.

It seems this "temporary" problem continues.

AutocompleteEditController is defined in autocomplete_edit.h, not
autocomplete_edit_controller.h.  

AutocompleteEditViewMac is a Mac version of a class named SomethingView,
although it's not an actual "views" view.  Unlike other Mac SomethingView
classes, this isn't an NSView.

LocationBarViewMac, which is more controller than view, derives from
AutocompleteEditController.

Seems like:
autocomplete_edit.h should be renamed autocomplete_edit_controller.h
AutocompleteEditView should be renamed AutocompleteEditViewController
LocationBarViewMac should be called AutocompleteEditControllerBridge (to match
convention of C++ bridge class in the objc world) or
AutocompleteEditControllerMac.

Confusing names are tough to follow.

http://codereview.chromium.org/159250/diff/22/2043#newcode440
Line 440: controller_->OnChanged();
The bookmark system is nicely done; all BookmarkNode pointers you can ever get
access to are const, and all modifications to them must go through the
BookmarkModel.  This allows the model to ping observers when changes happen.  It
would be nice if such a model was applied here so you don't have the option of
accidentally forgetting to call OnChanged().  For example, if  
SetWindowTextAndCaretPos() called OnChanged(), you can't forget!
I think you can just bottleneck OnChanged() to the end of SetText()?

http://codereview.chromium.org/159250/diff/22/2044
File chrome/browser/cocoa/autocomplete_text_field.h (right):

http://codereview.chromium.org/159250/diff/22/2044#newcode13
Line 13: // AutocompleteEditViewMac, and provides a custom look.  It works
Reading this I had to check my notes to know AutocompleteEditViewMac is
functionally a controller, not a view (in MVC sense).  Perhaps an additional
comment is warranted.

http://codereview.chromium.org/159250/diff/22/2046
File chrome/browser/cocoa/autocomplete_text_field_cell.h (right):

http://codereview.chromium.org/159250/diff/22/2046#newcode56
Line 56: scoped_nsobject<NSButtonCell> infoCell_;
I've seen views own multiple cells (NSTableView is a classic example), but I've
never seen NSCells own other NSCells like you're doing here.  Is this a common
pattern?  Might it be better for the NSView than owns this cell to instead own
multiple cells?

http://codereview.chromium.org/159250/diff/22/2046#newcode85
Line 85: @interface AutocompleteTextFieldCell ()
() --> (TestingAPI) or
() --> (Private)

http://codereview.chromium.org/159250/diff/22/2047
File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/159250/diff/22/2047#newcode15
Line 15: const NSInteger kKeywordXOffset = 3;
Put all these consts in an anonymous namespace, or 'static' them.

http://codereview.chromium.org/159250/diff/22/2047#newcode33
Line 33: if (aString != keywordString_.get() &&
If you use isEqualToString, the pointer comparison is redundant.
Ditto for other spots in this file.

http://codereview.chromium.org/159250/diff/22/2047#newcode103
Line 103: - (void)initInfoCell {
This method is WAY too big.  Please split up.  That'll make it easier to unit
test.  It's a little hard for me to eyeball this and tell if things look
correct-ish.

http://codereview.chromium.org/159250/diff/22/2047#newcode124
Line 124: if ([keywordHintPrefixString_ length] > 0) {
Length checks redundant; it's just fine to append a @"".
Perhaps even use stringWithFormat:@"%@%@$%@"

http://codereview.chromium.org/159250/diff/22/2047#newcode221
Line 221: - (void)splitFrame:(NSRect)cellFrame
Glad to see a unit test for this!  Might be good to add doc that textFrame can
also be cellFrame; other code requires that to work.

http://codereview.chromium.org/159250/diff/22/2048
File chrome/browser/cocoa/autocomplete_text_field_cell_unittest.mm (right):

http://codereview.chromium.org/159250/diff/22/2048#newcode244
Line 244: EXPECT_FALSE(NSIsEmptyRect(textFrame));
Re: this whole file; jrg <3 shess

http://codereview.chromium.org/159250/diff/22/2050
File chrome/browser/cocoa/autocomplete_text_field_unittest.mm (right):

http://codereview.chromium.org/159250/diff/22/2050#newcode219
Line 219: TEST_F(AutocompleteTextFieldTest, ResetFieldEditorBlocksEndEditing) {
More <3 from me 2 u for this

http://codereview.chromium.org/159250/diff/22/2052
File chrome/browser/cocoa/location_bar_view_mac.mm (right):

http://codereview.chromium.org/159250/diff/22/2052#newcode145
Line 145: void LocationBarViewMac::OnChanged() {
Seems knarly.  Split up as well?  Unit test components of it?

[Scott Hess - ex-Googler, 2009-08-07 20:30:13]

Sorry for the long intermission.

I'm not ready for another review pass, I'll let you know when it is.  But I did
want to put up some comment responses for review in the meanwhile.  Thanks.

http://codereview.chromium.org/159250/diff/22/2043
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/159250/diff/22/2043#newcode291
Line 291: controller_->OnChanged();
On 2009/08/04 23:10:19, John Grabowski wrote:
> From the header I see this:
> // TODO(pkasting): http://b/1343512  The names and contents of the classes in
> // this file are temporary.  I am in hack-and-slash mode right now.
> 
> It seems this "temporary" problem continues.
> 
> AutocompleteEditController is defined in autocomplete_edit.h, not
> autocomplete_edit_controller.h.  
> 
> AutocompleteEditViewMac is a Mac version of a class named SomethingView,
> although it's not an actual "views" view.  Unlike other Mac SomethingView
> classes, this isn't an NSView.
> 
> LocationBarViewMac, which is more controller than view, derives from
> AutocompleteEditController.
> 
> Seems like:
> autocomplete_edit.h should be renamed autocomplete_edit_controller.h
> AutocompleteEditView should be renamed AutocompleteEditViewController
> LocationBarViewMac should be called AutocompleteEditControllerBridge (to match
> convention of C++ bridge class in the objc world) or
> AutocompleteEditControllerMac.
> 
> Confusing names are tough to follow.

I've been thinking a nice long-term giving-back-to-Chrome project for me might
be to refactor this stuff into something more ... palatable.  It would be a good
experience for learning to work with the Linux and Windows code a bit more,
which is definitely a weakness for me right now.

http://codereview.chromium.org/159250/diff/22/2043#newcode440
Line 440: controller_->OnChanged();
On 2009/08/04 23:10:19, John Grabowski wrote:
> The bookmark system is nicely done; all BookmarkNode pointers you can ever get
> access to are const, and all modifications to them must go through the
> BookmarkModel.  This allows the model to ping observers when changes happen. 
It
> would be nice if such a model was applied here so you don't have the option of
> accidentally forgetting to call OnChanged().  For example, if  
> SetWindowTextAndCaretPos() called OnChanged(), you can't forget!
> I think you can just bottleneck OnChanged() to the end of SetText()?

This is a good point, and I was looking at it, but then I realized that
SetText() comes before other adjustments such as changes to the selection. 
Since SetText() and associated methods manipulate the field from outside, and
OnChanged() manipulates the field from inside, I'm a little nervous about
intermingling things.  It should, of course, work, but I need to let the idea
stew for a bit to see if there's anything outlandish it might provoke.

Really, as you mention there should be cleaner cleavage between these classes.

http://codereview.chromium.org/159250/diff/22/2046
File chrome/browser/cocoa/autocomplete_text_field_cell.h (right):

http://codereview.chromium.org/159250/diff/22/2046#newcode56
Line 56: scoped_nsobject<NSButtonCell> infoCell_;
On 2009/08/04 23:10:19, John Grabowski wrote:
> I've seen views own multiple cells (NSTableView is a classic example), but
I've
> never seen NSCells own other NSCells like you're doing here.  Is this a common
> pattern?  Might it be better for the NSView than owns this cell to instead own
> multiple cells?

There were a couple reasons I went this way.  The decorative cells impact the
positioning of the "real" cell, which is not really something that the
control/cell abstraction is setup for, so I'd probably have to leak
implementation details between them to do it that way.  Another issue is that
the cell draws everything within the control's rect, including the border and
focus ring, but these additional decorations need to be within those elements. 
I suspect that would end up looking pretty weird in the end.

I think the strongest alternative would be to use a system of distinct views,
which is somewhat like how Windows does things.  Why I didn't go in that
direction was because I was looking at how to manage to border and focus ring,
and it seemed odd because the text field wouldn't be able to directly handle the
difference between the border being around everything and the editing area being
smaller.  I think it would have needed to have a border view, views for each
decoration, plus a borderless text-field for editing and selection.  And it
would have somehow needed to continue to function just like an augmented text
field...

Overall, I agree that it's sort of funky.  My goal with the infoCell_ was to
keep it entirely an implementation detail.  Since infoCell_ doesn't get to
interact with events, it would be pretty straight-forward to convert things to
use drawing primitives directly.

http://codereview.chromium.org/159250/diff/22/2046#newcode85
Line 85: @interface AutocompleteTextFieldCell ()
On 2009/08/04 23:10:19, John Grabowski wrote:
> () --> (TestingAPI) or
> () --> (Private)

OK.  I had done this because () is documented as an interface which must be
implemented in the main @implementation, otherwise the compiler will warn.  A
named interface will not draw such a warning.

http://developer.apple.com/documentation/Cocoa/Conceptual/ObjectiveC/Articles...
Under "Extensions", towards the end.

http://codereview.chromium.org/159250/diff/22/2047
File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/159250/diff/22/2047#newcode15
Line 15: const NSInteger kKeywordXOffset = 3;
On 2009/08/04 23:10:19, John Grabowski wrote:
> Put all these consts in an anonymous namespace, or 'static' them.
> 

Done.  Good point!

http://codereview.chromium.org/159250/diff/22/2047#newcode33
Line 33: if (aString != keywordString_.get() &&
On 2009/08/04 23:10:19, John Grabowski wrote:
> If you use isEqualToString, the pointer comparison is redundant.
> Ditto for other spots in this file.

Except in the case where aString and keywordString_ are both nil.  In that case
[nil isEqualToString:nil] is false, and the cell _doesn't_ need laidout again.

I could just document and DCHECK to enforce aString non-nil?

http://codereview.chromium.org/159250/diff/22/2047#newcode103
Line 103: - (void)initInfoCell {
On 2009/08/04 23:10:19, John Grabowski wrote:
> This method is WAY too big.  Please split up.  That'll make it easier to unit
> test.  It's a little hard for me to eyeball this and tell if things look
> correct-ish.

OK, I'll work on that.

http://codereview.chromium.org/159250/diff/22/2047#newcode124
Line 124: if ([keywordHintPrefixString_ length] > 0) {
On 2009/08/04 23:10:19, John Grabowski wrote:
> Length checks redundant; it's just fine to append a @"".
> Perhaps even use stringWithFormat:@"%@%@$%@"

The appended string must not be nil.  Another argument for requiring non-nil for
those strings?  I'll change things along those lines.

http://codereview.chromium.org/159250/diff/22/2047#newcode221
Line 221: - (void)splitFrame:(NSRect)cellFrame
On 2009/08/04 23:10:19, John Grabowski wrote:
> Glad to see a unit test for this!  Might be good to add doc that textFrame can
> also be cellFrame; other code requires that to work.

Since cellFrame is pass-by-value and textFrame's initial value is immediately
overwritten, that shouldn't matter, should it?

http://codereview.chromium.org/159250/diff/22/2052
File chrome/browser/cocoa/location_bar_view_mac.mm (right):

http://codereview.chromium.org/159250/diff/22/2052#newcode145
Line 145: void LocationBarViewMac::OnChanged() {
On 2009/08/04 23:10:19, John Grabowski wrote:
> Seems knarly.  Split up as well?  Unit test components of it?

I'll see what can be done.

[John Grabowski, 2009-08-12 17:38:56]

http://codereview.chromium.org/159250/diff/22/2043
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/159250/diff/22/2043#newcode291
Line 291: controller_->OnChanged();
On 2009/08/07 20:30:13, shess wrote:
> On 2009/08/04 23:10:19, John Grabowski wrote:
> > From the header I see this:
> > // TODO(pkasting): http://b/1343512  The names and contents of the classes
in
> > // this file are temporary.  I am in hack-and-slash mode right now.
> > 
> > It seems this "temporary" problem continues.
> > 
> > AutocompleteEditController is defined in autocomplete_edit.h, not
> > autocomplete_edit_controller.h.  
> > 
> > AutocompleteEditViewMac is a Mac version of a class named SomethingView,
> > although it's not an actual "views" view.  Unlike other Mac SomethingView
> > classes, this isn't an NSView.
> > 
> > LocationBarViewMac, which is more controller than view, derives from
> > AutocompleteEditController.
> > 
> > Seems like:
> > autocomplete_edit.h should be renamed autocomplete_edit_controller.h
> > AutocompleteEditView should be renamed AutocompleteEditViewController
> > LocationBarViewMac should be called AutocompleteEditControllerBridge (to
match
> > convention of C++ bridge class in the objc world) or
> > AutocompleteEditControllerMac.
> > 
> > Confusing names are tough to follow.
> 
> I've been thinking a nice long-term giving-back-to-Chrome project for me might
> be to refactor this stuff into something more ... palatable.  It would be a
good
> experience for learning to work with the Linux and Windows code a bit more,
> which is definitely a weakness for me right now.

Call it Gardening, make it an OKR, and get a pkasting peer bonus.

http://codereview.chromium.org/159250/diff/22/2043#newcode440
Line 440: controller_->OnChanged();
On 2009/08/07 20:30:13, shess wrote:
> On 2009/08/04 23:10:19, John Grabowski wrote:
> > The bookmark system is nicely done; all BookmarkNode pointers you can ever
get
> > access to are const, and all modifications to them must go through the
> > BookmarkModel.  This allows the model to ping observers when changes happen.

> It
> > would be nice if such a model was applied here so you don't have the option
of
> > accidentally forgetting to call OnChanged().  For example, if  
> > SetWindowTextAndCaretPos() called OnChanged(), you can't forget!
> > I think you can just bottleneck OnChanged() to the end of SetText()?
> 
> This is a good point, and I was looking at it, but then I realized that
> SetText() comes before other adjustments such as changes to the selection. 
> Since SetText() and associated methods manipulate the field from outside, and
> OnChanged() manipulates the field from inside, I'm a little nervous about
> intermingling things.  It should, of course, work, but I need to let the idea
> stew for a bit to see if there's anything outlandish it might provoke.
> 
> Really, as you mention there should be cleaner cleavage between these classes.

Stew on!

http://codereview.chromium.org/159250/diff/22/2046
File chrome/browser/cocoa/autocomplete_text_field_cell.h (right):

http://codereview.chromium.org/159250/diff/22/2046#newcode56
Line 56: scoped_nsobject<NSButtonCell> infoCell_;
On 2009/08/07 20:30:13, shess wrote:
> On 2009/08/04 23:10:19, John Grabowski wrote:
> > I've seen views own multiple cells (NSTableView is a classic example), but
> I've
> > never seen NSCells own other NSCells like you're doing here.  Is this a
common
> > pattern?  Might it be better for the NSView than owns this cell to instead
own
> > multiple cells?
> 
> There were a couple reasons I went this way.  The decorative cells impact the
> positioning of the "real" cell, which is not really something that the
> control/cell abstraction is setup for, so I'd probably have to leak
> implementation details between them to do it that way.  Another issue is that
> the cell draws everything within the control's rect, including the border and
> focus ring, but these additional decorations need to be within those elements.

> I suspect that would end up looking pretty weird in the end.
> 
> I think the strongest alternative would be to use a system of distinct views,
> which is somewhat like how Windows does things.  Why I didn't go in that
> direction was because I was looking at how to manage to border and focus ring,
> and it seemed odd because the text field wouldn't be able to directly handle
the
> difference between the border being around everything and the editing area
being
> smaller.  I think it would have needed to have a border view, views for each
> decoration, plus a borderless text-field for editing and selection.  And it
> would have somehow needed to continue to function just like an augmented text
> field...
> 
> Overall, I agree that it's sort of funky.  My goal with the infoCell_ was to
> keep it entirely an implementation detail.  Since infoCell_ doesn't get to
> interact with events, it would be pretty straight-forward to convert things to
> use drawing primitives directly.
> 

As you point out, there are lots of options.  Many of the things you hint at
(e.g. use views and subviews) seem fine.  But, at a quick glance, it doesn't
seem like a good idea for cells to own child cells.  (One of the reasons an
NSCell exists is to be a cheap NSView which doesn't have to deal with ownership
of children et al).

In any case, please reconsider your abstraction choices.

http://codereview.chromium.org/159250/diff/22/2046#newcode85
Line 85: @interface AutocompleteTextFieldCell ()
On 2009/08/07 20:30:13, shess wrote:
> On 2009/08/04 23:10:19, John Grabowski wrote:
> > () --> (TestingAPI) or
> > () --> (Private)
> 
> OK.  I had done this because () is documented as an interface which must be
> implemented in the main @implementation, otherwise the compiler will warn.  A
> named interface will not draw such a warning.
> 
>
http://developer.apple.com/documentation/Cocoa/Conceptual/ObjectiveC/Articles...
> Under "Extensions", towards the end.

Seems like a good reason then, even if inconsistent with the rest of the code. 
(But if you didn't do it, you'd still fail unit tests at runtime, so it would be
caught).

http://codereview.chromium.org/159250/diff/22/2047
File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/159250/diff/22/2047#newcode33
Line 33: if (aString != keywordString_.get() &&
On 2009/08/07 20:30:13, shess wrote:
> On 2009/08/04 23:10:19, John Grabowski wrote:
> > If you use isEqualToString, the pointer comparison is redundant.
> > Ditto for other spots in this file.
> 
> Except in the case where aString and keywordString_ are both nil.  In that
case
> [nil isEqualToString:nil] is false, and the cell _doesn't_ need laidout again.
> 
> I could just document and DCHECK to enforce aString non-nil?

Hmm... I didn't think of that, even though you have a comment to that effect
(sort of).  Perhaps you could reword the comment.  Or perhaps I could read them
before commenting next time.

http://codereview.chromium.org/159250/diff/22/2047#newcode221
Line 221: - (void)splitFrame:(NSRect)cellFrame
On 2009/08/07 20:30:13, shess wrote:
> On 2009/08/04 23:10:19, John Grabowski wrote:
> > Glad to see a unit test for this!  Might be good to add doc that textFrame
can
> > also be cellFrame; other code requires that to work.
> 
> Since cellFrame is pass-by-value and textFrame's initial value is immediately
> overwritten, that shouldn't matter, should it?

You are right; not sure what I meant.  Maybe I misread.

[Scott Hess - ex-Googler, 2009-08-14 20:04:03]

Thanks for your patience, John.  I hope we're getting close :-).

http://codereview.chromium.org/159250/diff/22/2043
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/159250/diff/22/2043#newcode440
Line 440: controller_->OnChanged();
On 2009/08/12 17:38:56, John Grabowski wrote:
> On 2009/08/07 20:30:13, shess wrote:
> > On 2009/08/04 23:10:19, John Grabowski wrote:
> > > The bookmark system is nicely done; all BookmarkNode pointers you can ever
> get
> > > access to are const, and all modifications to them must go through the
> > > BookmarkModel.  This allows the model to ping observers when changes
happen.
> 
> > It
> > > would be nice if such a model was applied here so you don't have the
option
> of
> > > accidentally forgetting to call OnChanged().  For example, if  
> > > SetWindowTextAndCaretPos() called OnChanged(), you can't forget!
> > > I think you can just bottleneck OnChanged() to the end of SetText()?
> > 
> > This is a good point, and I was looking at it, but then I realized that
> > SetText() comes before other adjustments such as changes to the selection. 
> > Since SetText() and associated methods manipulate the field from outside,
and
> > OnChanged() manipulates the field from inside, I'm a little nervous about
> > intermingling things.  It should, of course, work, but I need to let the
idea
> > stew for a bit to see if there's anything outlandish it might provoke.
> > 
> > Really, as you mention there should be cleaner cleavage between these
classes.
> 
> Stew on!

I've stewed, and experimented, and I believe that making this change (OnChanged
moved to SetText) has potential to break various semi-related code, like the
tab-restore logic.  I have good reason to believe that it is broken as-is, but
right now it's broken such that everything works, just not the way you want it. 
My worry about injecting it every time is that I'm missing a use-case which will
break things in the browser-crashes way.

I've put in a TODO around this, I think there are two possibilities.  Move
OnChanged() into SetText() and make sure everything expects it and works right
is one.  The other is to pull the implementation of OnChanged() itself into
SetText(), since OnChanged() just talks to |model_| and updates |field_|.

http://codereview.chromium.org/159250/diff/22/2044
File chrome/browser/cocoa/autocomplete_text_field.h (right):

http://codereview.chromium.org/159250/diff/22/2044#newcode13
Line 13: // AutocompleteEditViewMac, and provides a custom look.  It works
On 2009/08/04 23:10:19, John Grabowski wrote:
> Reading this I had to check my notes to know AutocompleteEditViewMac is
> functionally a controller, not a view (in MVC sense).  Perhaps an additional
> comment is warranted.

Done.

http://codereview.chromium.org/159250/diff/22/2046
File chrome/browser/cocoa/autocomplete_text_field_cell.h (right):

http://codereview.chromium.org/159250/diff/22/2046#newcode56
Line 56: scoped_nsobject<NSButtonCell> infoCell_;
On 2009/08/12 17:38:56, John Grabowski wrote:
> On 2009/08/07 20:30:13, shess wrote:
> > On 2009/08/04 23:10:19, John Grabowski wrote:
> > > I've seen views own multiple cells (NSTableView is a classic example), but
> > I've
> > > never seen NSCells own other NSCells like you're doing here.  Is this a
> common
> > > pattern?  Might it be better for the NSView than owns this cell to instead
> own
> > > multiple cells?
> > 
> > There were a couple reasons I went this way.  The decorative cells impact
the
> > positioning of the "real" cell, which is not really something that the
> > control/cell abstraction is setup for, so I'd probably have to leak
> > implementation details between them to do it that way.  Another issue is
that
> > the cell draws everything within the control's rect, including the border
and
> > focus ring, but these additional decorations need to be within those
elements.
> 
> > I suspect that would end up looking pretty weird in the end.
> > 
> > I think the strongest alternative would be to use a system of distinct
views,
> > which is somewhat like how Windows does things.  Why I didn't go in that
> > direction was because I was looking at how to manage to border and focus
ring,
> > and it seemed odd because the text field wouldn't be able to directly handle
> the
> > difference between the border being around everything and the editing area
> being
> > smaller.  I think it would have needed to have a border view, views for each
> > decoration, plus a borderless text-field for editing and selection.  And it
> > would have somehow needed to continue to function just like an augmented
text
> > field...
> > 
> > Overall, I agree that it's sort of funky.  My goal with the infoCell_ was to
> > keep it entirely an implementation detail.  Since infoCell_ doesn't get to
> > interact with events, it would be pretty straight-forward to convert things
to
> > use drawing primitives directly.
> > 
> 
> As you point out, there are lots of options.  Many of the things you hint at
> (e.g. use views and subviews) seem fine.  But, at a quick glance, it doesn't
> seem like a good idea for cells to own child cells.  (One of the reasons an
> NSCell exists is to be a cheap NSView which doesn't have to deal with
ownership
> of children et al).
> 
> In any case, please reconsider your abstraction choices.

Done.  I still don't think the use of sub-cells was a problem, since I was just
using them for drawing and not for anything interesting, but ... using drawing
primitives directly let me also knock off getting the keyword-string token
closer to visually correct, and that was the main area where the cell was a gain
in the first place.

http://codereview.chromium.org/159250/diff/22/2046#newcode85
Line 85: @interface AutocompleteTextFieldCell ()
On 2009/08/12 17:38:56, John Grabowski wrote:
> On 2009/08/07 20:30:13, shess wrote:
> > On 2009/08/04 23:10:19, John Grabowski wrote:
> > > () --> (TestingAPI) or
> > > () --> (Private)
> > 
> > OK.  I had done this because () is documented as an interface which must be
> > implemented in the main @implementation, otherwise the compiler will warn. 
A
> > named interface will not draw such a warning.
> > 
> >
>
http://developer.apple.com/documentation/Cocoa/Conceptual/ObjectiveC/Articles...
> > Under "Extensions", towards the end.
> 
> Seems like a good reason then, even if inconsistent with the rest of the code.

> (But if you didn't do it, you'd still fail unit tests at runtime, so it would
be
> caught).

Done.  Maybe that article is more useful as a style-guide suggestion than a
cowboy implementation detail.

http://codereview.chromium.org/159250/diff/22/2047
File chrome/browser/cocoa/autocomplete_text_field_cell.mm (right):

http://codereview.chromium.org/159250/diff/22/2047#newcode33
Line 33: if (aString != keywordString_.get() &&
On 2009/08/12 17:38:56, John Grabowski wrote:
> On 2009/08/07 20:30:13, shess wrote:
> > On 2009/08/04 23:10:19, John Grabowski wrote:
> > > If you use isEqualToString, the pointer comparison is redundant.
> > > Ditto for other spots in this file.
> > 
> > Except in the case where aString and keywordString_ are both nil.  In that
> case
> > [nil isEqualToString:nil] is false, and the cell _doesn't_ need laidout
again.
> > 
> > I could just document and DCHECK to enforce aString non-nil?
> 
> Hmm... I didn't think of that, even though you have a comment to that effect
> (sort of).  Perhaps you could reword the comment.  Or perhaps I could read
them
> before commenting next time.

I've reworked the code to shift from of the other if/elseif/elseif code into
these methods.  This removed the entire problem.

http://codereview.chromium.org/159250/diff/22/2052
File chrome/browser/cocoa/location_bar_view_mac.mm (right):

http://codereview.chromium.org/159250/diff/22/2052#newcode145
Line 145: void LocationBarViewMac::OnChanged() {
On 2009/08/07 20:30:13, shess wrote:
> On 2009/08/04 23:10:19, John Grabowski wrote:
> > Seems knarly.  Split up as well?  Unit test components of it?
> 
> I'll see what can be done.

I've made it more straight-forward, but no unit tests, yet, because I'm not
certain where to take it.  I can see pulling out the code which localizes and
sets the cell values, and then checking if the right things happened.  [Well, I
assume so, I'd have to go double-check how localization happens in unit test
environment.]  It would be more relevant, though, to verify that the different
decisions about which thing to select were tested.

Hmm.  The core problem, here, is that this class doesn't have an existing unit
test environment (the peer implementations didn't either, at least when I
originally was implementing).  I've pulled the guts out for testing purposes,
let me know if it flies.

[John Grabowski, 2009-08-18 20:04:30]

LGTM

[rohitrao (ping after 24h), 2009-08-19 13:00:48]

I will take this over and land it as-is, then go back for another pass.  I'm
hitting Martha's Vineyard for the day, so I will land this tonight or tomorrow
morning.

[Scott Hess - ex-Googler, 2009-08-23 14:07:04]

Oooh, shiny!  I sure hope it didn't expose interesting edge cases in the wild...

On Wed, Aug 19, 2009 at 6:00 AM, <rohitrao@chromium.org> wrote:
> I will take this over and land it as-is, then go back for another pass.
> I'm hitting Martha's Vineyard for the day, so I will land this tonight
> or tomorrow morning.
>
> http://codereview.chromium.org/159250
>