Initial implemention of Mac Omnibox.

Initial implemention of Mac Omnibox.

AutocompletePopupViewMac implements AutocompletePopupView in terms of a bare NSWindow containing an NSTableView.  AutocompleteTableTarget implements an Obj-C class to bridge from appkit callbacks back to the popup view (and from there to the model which contains the data it needs).

AutocompleteEditViewMac implements AutocompleteEditView in terms of an NSTextField, which is passed down from a nib owner.  It works with the popup view to make sure the popup is positioned correctly.  AutocompleteFieldDelegate is an internal Obj-C class to bridge from appkit callbacks back to the edit view (and then the edit model).

LocationBarViewMac implements LocationBar for interacting with the rest of the browser, and AutocompleteEditController for managing the edit and popup views.  It is mostly placeholder code stolen from the gtk implementation.

---

I've tried to implement an amount of code which worked and was useful, but which didn't drag on and on into the future.  So no tab to search or hints or anything, sometimes ugly, selection may be funky, etc.

[Scott Hess - ex-Googler, 2009-03-21 00:14:55]

Oops, I uploaded, entered the comments, and forgot to send it out.

[Dean McNamee, 2009-03-23 11:22:45]

I don't know this obj-c business at all, so my review was pretty useless.

http://codereview.chromium.org/50074/diff/1016/1021
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/50074/diff/1016/1021#newcode46
Line 46: // TODO(shess): Why no attempt to clean up or something?
What do you mean, what is there to clean up?

http://codereview.chromium.org/50074/diff/1016/1021#newcode54
Line 54: RevertAll();  // Revert the box to its unedited state
period at the end

http://codereview.chromium.org/50074/diff/1016/1021#newcode71
Line 71: // even know if that makes sense for Mac.
I can try to answer that question.  Basically when you have a selection, you
have an "insertion point" and the "selection bounds".  The only difference I see
it making is that on Windows you can change the selection with shift left/right
arrow.  This is changing the "section bounds" point, so reversing it will have
it at the beginning vs the end.  I am not sure if something similar applies to
the Mac (or even GTK), or if there are other places where it's important to have
the insertion point be the beginning/end.  Does your widget's selection code not
have a similar concept?

http://codereview.chromium.org/50074/diff/1016/1021#newcode121
Line 121: // TODO(shess): GTK has this as a member var, figure out why.
It is because it is used in EmphasizeURLComponents.  This matches what Windows
does.

http://codereview.chromium.org/50074/diff/1016/1022
File chrome/browser/autocomplete/autocomplete_popup_view_mac.h (right):

http://codereview.chromium.org/50074/diff/1016/1022#newcode48
Line 48: // nothing to do here.
I think this is mostly an anti-flicker optimization on Windows.

http://codereview.chromium.org/50074/diff/1016/1023
File chrome/browser/autocomplete/autocomplete_popup_view_mac.mm (right):

http://codereview.chromium.org/50074/diff/1016/1023#newcode200
Line 200: if (columnIndex==1) {
are you writing in sqlite style? :)  column_indexs, spaces around == , etc?

http://codereview.chromium.org/50074/diff/1016/1025
File chrome/browser/cocoa/location_bar_view_mac.mm (right):

http://codereview.chromium.org/50074/diff/1016/1025#newcode30
Line 30: // it before starting the code review.
I am trying to get most of our UI code to have Init() methods like this, it's
bad to do a bunch of work in the constructor.  Another reason why we have some
Init() methods is that you get the profile after the constructor is called via
SetProfile(), so it's like Constructor(), SetProfile(), Init() and Init() needs
profile_.

[Mark Mentovai, 2009-03-23 14:54:59]

This is kind of a light first pass, but I can get away with that when you've
picked four reviewers, right?

Overall, I think this is really good.

http://codereview.chromium.org/50074/diff/1016/1018
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/50074/diff/1016/1018#newcode7
Line 7: #import "base/message_loop.h"
#include (because it's not an Obj-C header).  Same for all of the rest of these.
 app_controller_mac.h is an Obj-C header, so that one should remain #import.

http://codereview.chromium.org/50074/diff/1016/1018#newcode87
Line 87: // How to get the profile created on line 314 of browser_main? Ugh.
TODO:FIXME
(Existing code being moved, I know.)

A line number reference (without a fixed revision) is bad, because the line
number is very likely to change, and nobody's going to update this comment.  You
should provide a better description of what we want here.  Give the file name
with the extension so that it's clear you mean browser_main.cc and not
browser_main_mac.mm or some other file.  And TODOs should have the TODO at the
beginning of the comment.

http://codereview.chromium.org/50074/diff/1016/1020
File chrome/browser/autocomplete/autocomplete_edit_view_mac.h (right):

http://codereview.chromium.org/50074/diff/1016/1020#newcode39
Line 39: virtual const AutocompleteEditModel* model() const { return
model_.get(); }
What's with one being const and one not?  What a weird interface to have to
implement.  (Yeah, I know you're just working with the extant interface here.)

FYI (you may already know), the normal way to handle this is to write a single
const method that returns non-const (unless it needs to return const because the
object it accesses is const, which it is not in this case).  That would be:

  virtual AutocompleteEditModel* model() const { return model_.get(); }

I checked the Windows and GTK code and it would be fine with this pattern as
well, so I think we should change everything to be this way.  You don't need to
do that in this changelist if you don't want, if you find it easier to do it in
a followup or to "svn blame" the guilty party and ask them to do it, that's
fine.

http://codereview.chromium.org/50074/diff/1016/1022
File chrome/browser/autocomplete/autocomplete_popup_view_mac.h (right):

http://codereview.chromium.org/50074/diff/1016/1022#newcode68
Line 68: void SetField(NSTextField *field) {
The style for parameters in this file (and for Chrome generally) is to use
NSTextField* field and not NSTextField *field.

http://codereview.chromium.org/50074/diff/1016/1022#newcode76
Line 76: // TODO(shess): Can table_target_ be a scoped ref?
Yes, and so can a few other things in this change.

I added scoped_cftyperef.h to base a long time ago.  We should also provide
scoped_nsobject.h and use it here.  That would keep the "strong" ownership claim
in code (so you wouldn't need to write "we own") and would reduce the tendency
toward leaks like the one in my next comment.

See (lightly-tested) http://codereview.chromium.org/52014 .

http://codereview.chromium.org/50074/diff/1016/1022#newcode78
Line 78: NSWindow* popup_;  // we own
Looks like you leak popup_.

http://codereview.chromium.org/50074/diff/1016/1023
File chrome/browser/autocomplete/autocomplete_popup_view_mac.mm (right):

http://codereview.chromium.org/50074/diff/1016/1023#newcode40
Line 40: : model_(new AutocompletePopupModel(this, edit_model, profile)),
Format's a little off, needs two more spaces of indentation here.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Initia...

http://codereview.chromium.org/50074/diff/1016/1023#newcode56
Line 56: static NSTableColumn *CreateTableColumn(int tag, id field_class) {
|id| is a sucky type here, you don't really mean |id|.  I think you want
|Class|.

http://codereview.chromium.org/50074/diff/1016/1023#newcode58
Line 58: NSTableColumn *col
I'd put the = on this line.

The next line (the continuation line) should have a 4-space, not a 2-space,
indent.

http://codereview.chromium.org/50074/diff/1016/1023#newcode79
Line 79: NSTableView *table = [[NSTableView alloc] initWithFrame:NSZeroRect];
Leak.

(popup_ will retain it on the next line, but you never release your ownership
claim.)

http://codereview.chromium.org/50074/diff/1016/1023#newcode166
Line 166: void AutocompletePopupViewMac::
Breaking after the :: is really weird.  How about:

void AutocompletePopupViewMac::AcceptInput(
    WindowOpenDisposition disposition, bool for_drop) {

http://codereview.chromium.org/50074/diff/1016/1023#newcode193
Line 193: row:(int)ri {
What's with the indentation here?

http://codereview.chromium.org/50074/diff/1016/1023#newcode200
Line 200: if (columnIndex==1) {
deanm wrote:
> are you writing in sqlite style? :)  column_indexs, spaces around == , etc?

Obj-C style for variable names is correct within Objective-C @interface and
@implementation sections.

But you should put spaces around your ==, <, etc.

http://codereview.chromium.org/50074/diff/1016/1023#newcode214
Line 214: NSMutableParagraphStyle *style
Prefer to see the = on the first line, not the second.  Happens again in a few
lines.

http://codereview.chromium.org/50074/diff/1016/1025
File chrome/browser/cocoa/location_bar_view_mac.mm (right):

http://codereview.chromium.org/50074/diff/1016/1025#newcode20
Line 20: : command_updater_(command_updater),
more space

http://codereview.chromium.org/50074/diff/1016/1025#newcode72
Line 72: /* http://crbug.com/8832 */
//

http://codereview.chromium.org/50074/diff/1016/1025#newcode81
Line 81: OnAutocompleteAccept(const GURL& url,
Bad spot for the line break again.

http://codereview.chromium.org/50074/diff/1016/1027
File chrome/browser/cocoa/tab_contents_controller.mm (right):

http://codereview.chromium.org/50074/diff/1016/1027#newcode82
Line 82: // perhaps _could_ move locationBarView_'s construction down here.
In autocomplete_popup_view_mac.h, you left a note that referred to
initialization order in tab_contents_mac.mm, which is good: if someone finds
that other TODO and then refactors this, they'll know they can get rid of
SetField.  But if someone finds this TODO and refactors, they won't know to get
rid of SetField.  TODO comments should be more explicit about what can be
cleaned up, and when, so that when they're split across multiple files (as in
this case), everything gets cleaned up together when the TODO is fixed.

[pink (ping after 24hrs), 2009-03-23 17:07:08]

a good start, though i agree with mark that there's a lot of spacing that needs
cleaning up (but that's minor). I'd like to see another round with everything
cleaned up before it gets a LGTM. I can't wait to see this in action!!!!!

http://codereview.chromium.org/50074/diff/1016/1020
File chrome/browser/autocomplete/autocomplete_edit_view_mac.h (right):

http://codereview.chromium.org/50074/diff/1016/1020#newcode114
Line 114: NSTextField* field_;  // owned by controller
do you mean controller_?

http://codereview.chromium.org/50074/diff/1016/1021
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/50074/diff/1016/1021#newcode19
Line 19: - initWithEditView:(AutocompleteEditViewMac *)view;
inconsistent spacing on (foo*) and (foo *). Stick with one through the whole
file.

http://codereview.chromium.org/50074/diff/1016/1021#newcode33
Line 33: edit_helper_([[AutocompleteFieldDelegate alloc] initWithEditView:this])
{
add some DCHECKs on the parameters just in case?

http://codereview.chromium.org/50074/diff/1016/1021#newcode71
Line 71: // even know if that makes sense for Mac.
On 2009/03/23 11:22:45, Dean McNamee wrote:
> I can try to answer that question.  Basically when you have a selection, you
> have an "insertion point" and the "selection bounds".  The only difference I
see
> it making is that on Windows you can change the selection with shift
left/right
> arrow.  This is changing the "section bounds" point, so reversing it will have
> it at the beginning vs the end.  I am not sure if something similar applies to
> the Mac (or even GTK), or if there are other places where it's important to
have
> the insertion point be the beginning/end.  Does your widget's selection code
not
> have a similar concept?

I think there is that concept under the hood, but what user gesture would
trigger such an action?

http://codereview.chromium.org/50074/diff/1016/1021#newcode107
Line 107: = [[[NSMutableAttributedString alloc] initWithString:ss] autorelease];
indent 4 spaces on continuation

http://codereview.chromium.org/50074/diff/1016/1021#newcode151
Line 151: [[field_ currentEditor] setSelectedRange:selected_range];
DCHECK that it became the first responder?

http://codereview.chromium.org/50074/diff/1016/1021#newcode240
Line 240: if (cmd==@selector(moveUp:)) {
spaces between variables and operators.

http://codereview.chromium.org/50074/diff/1016/1023
File chrome/browser/autocomplete/autocomplete_popup_view_mac.mm (right):

http://codereview.chromium.org/50074/diff/1016/1023#newcode56
Line 56: static NSTableColumn *CreateTableColumn(int tag, id field_class) {
this method has "create" in the name, yet autoreleases. That mixes the CF
allocation naming rules. Change the name, or lose the autorelease, depending on
what's most appropriate for the caller.

http://codereview.chromium.org/50074/diff/1016/1023#newcode70
Line 70: popup_ = [[NSWindow alloc] initWithContentRect:NSZeroRect
this should probably be a child window of the main window as well.

http://codereview.chromium.org/50074/diff/1016/1023#newcode183
Line 183: return [NSImage imageNamed:NSImageNameQuickLookTemplate];
don't we already have a star image? how does this differ from what we use for
the toolbar already?

http://codereview.chromium.org/50074/diff/1016/1025
File chrome/browser/cocoa/location_bar_view_mac.mm (right):

http://codereview.chromium.org/50074/diff/1016/1025#newcode20
Line 20: : command_updater_(command_updater),
On 2009/03/23 14:54:59, Mark Mentovai wrote:
> more space

he means these should be indented 4 spaces since it's a continuation.

http://codereview.chromium.org/50074/diff/1016/1025#newcode23
Line 23: transition_(PageTransition::TYPED) {
DCHECKs on parameters?

http://codereview.chromium.org/50074/diff/1016/1027
File chrome/browser/cocoa/tab_contents_controller.mm (right):

http://codereview.chromium.org/50074/diff/1016/1027#newcode89
Line 89: return dynamic_cast<LocationBar*>(locationBarView_);
we probably shouldn't be using dynamic_cast.

[Scott Hess - ex-Googler, 2009-03-23 21:49:59]

Checkpointing what I have WRT Dean's comments before a meeting...

http://codereview.chromium.org/50074/diff/1016/1021
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/50074/diff/1016/1021#newcode46
Line 46: // TODO(shess): Why no attempt to clean up or something?
On 2009/03/23 11:22:45, Dean McNamee wrote:
> What do you mean, what is there to clean up?

I wasn't sure when it would be correct for this code to receive an invalid url. 
Seems like either the caller should have taken care of it, or this code should.

http://codereview.chromium.org/50074/diff/1016/1021#newcode54
Line 54: RevertAll();  // Revert the box to its unedited state
On 2009/03/23 11:22:45, Dean McNamee wrote:
> period at the end

Copied from gtk :-).  I'll get it for both while I'm in there.

http://codereview.chromium.org/50074/diff/1016/1021#newcode71
Line 71: // even know if that makes sense for Mac.
On 2009/03/23 11:22:45, Dean McNamee wrote:
> I can try to answer that question.  Basically when you have a selection, you
> have an "insertion point" and the "selection bounds".  The only difference I
see
> it making is that on Windows you can change the selection with shift
left/right
> arrow.  This is changing the "section bounds" point, so reversing it will have
> it at the beginning vs the end.  I am not sure if something similar applies to
> the Mac (or even GTK), or if there are other places where it's important to
have
> the insertion point be the beginning/end.  Does your widget's selection code
not
> have a similar concept?

That sounds like my understanding of it.  Does this rise to a level justifying
something more than a TODO?

http://codereview.chromium.org/50074/diff/1016/1021#newcode121
Line 121: // TODO(shess): GTK has this as a member var, figure out why.
On 2009/03/23 11:22:45, Dean McNamee wrote:
> It is because it is used in EmphasizeURLComponents.  This matches what Windows
> does.

OK.  I don't have the emphasize stuff wired up fully at this time, so its use
may become clearer.

http://codereview.chromium.org/50074/diff/1016/1022
File chrome/browser/autocomplete/autocomplete_popup_view_mac.h (right):

http://codereview.chromium.org/50074/diff/1016/1022#newcode48
Line 48: // nothing to do here.
On 2009/03/23 11:22:45, Dean McNamee wrote:
> I think this is mostly an anti-flicker optimization on Windows.

That's what I figured.  Our code currently delegates this kind of thing to a
native widget, so we may be getting some flicker.  I'm still not 100% certain we
won't have to replace that widget with something a little more targeted to
handle things like hovering, so this function might get some implementation
later on.

http://codereview.chromium.org/50074/diff/1016/1023
File chrome/browser/autocomplete/autocomplete_popup_view_mac.mm (right):

http://codereview.chromium.org/50074/diff/1016/1023#newcode200
Line 200: if (columnIndex==1) {
On 2009/03/23 11:22:45, Dean McNamee wrote:
> are you writing in sqlite style? :)  column_indexs, spaces around == , etc?

Probably!  And, of course, now that you point it out it becomes blatantly
obvious.  And DCHECK_LT.  OK.

http://codereview.chromium.org/50074/diff/1016/1025
File chrome/browser/cocoa/location_bar_view_mac.mm (right):

http://codereview.chromium.org/50074/diff/1016/1025#newcode30
Line 30: // it before starting the code review.
On 2009/03/23 11:22:45, Dean McNamee wrote:
> I am trying to get most of our UI code to have Init() methods like this, it's
> bad to do a bunch of work in the constructor.  Another reason why we have some
> Init() methods is that you get the profile after the constructor is called via
> SetProfile(), so it's like Constructor(), SetProfile(), Init() and Init()
needs
> profile_.

Makes sense.  Updated the TODO to this end.

[Scott Hess - ex-Googler, 2009-03-24 05:53:18]

On 2009/03/23 14:54:59, Mark Mentovai wrote:
> This is kind of a light first pass, but I can get away with that when you've
> picked four reviewers, right?

Probably the other way around, you need to protect your code base... thanks for
the good review, lots of little things I sort of was grokking, but not quite
strongly enough.

Unfortunately, now things aren't working very well, and I don't know why.  Could
be the history-url-provider stuff being more enabled (and not working well),
could be Factor X.  Running it in the debugger just makes things worse :-).

http://codereview.chromium.org/50074/diff/1016/1018
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/50074/diff/1016/1018#newcode7
Line 7: #import "base/message_loop.h"
On 2009/03/23 14:54:59, Mark Mentovai wrote:
> #include (because it's not an Obj-C header).  Same for all of the rest of
these.
>  app_controller_mac.h is an Obj-C header, so that one should remain #import.

OK.  Adding full path for app_controller_mac.h while I'm in there.

http://codereview.chromium.org/50074/diff/1016/1018#newcode87
Line 87: // How to get the profile created on line 314 of browser_main? Ugh.
TODO:FIXME
On 2009/03/23 14:54:59, Mark Mentovai wrote:
> (Existing code being moved, I know.)
> 
> A line number reference (without a fixed revision) is bad, because the line
> number is very likely to change, and nobody's going to update this comment. 
You
> should provide a better description of what we want here.  Give the file name
> with the extension so that it's clear you mean browser_main.cc and not
> browser_main_mac.mm or some other file.  And TODOs should have the TODO at the
> beginning of the comment.

Oddly enough, the only point I could find that looked right had indeed moved a
bit, and it's not really clear that it does what this thinks it does.  I've
tried to improve the comment, though.

http://codereview.chromium.org/50074/diff/1016/1020
File chrome/browser/autocomplete/autocomplete_edit_view_mac.h (right):

http://codereview.chromium.org/50074/diff/1016/1020#newcode39
Line 39: virtual const AutocompleteEditModel* model() const { return
model_.get(); }
On 2009/03/23 14:54:59, Mark Mentovai wrote:
> What's with one being const and one not?  What a weird interface to have to
> implement.  (Yeah, I know you're just working with the extant interface here.)
> 
> FYI (you may already know), the normal way to handle this is to write a single
> const method that returns non-const (unless it needs to return const because
the
> object it accesses is const, which it is not in this case).  That would be:
> 
>   virtual AutocompleteEditModel* model() const { return model_.get(); }
> 
> I checked the Windows and GTK code and it would be fine with this pattern as
> well, so I think we should change everything to be this way.  You don't need
to
> do that in this changelist if you don't want, if you find it easier to do it
in
> a followup or to "svn blame" the guilty party and ask them to do it, that's
> fine.

I think I get you - the return value is non-const, but returning the value
doesn't change the receiver so the method is const.  Added a TODO.

http://codereview.chromium.org/50074/diff/1016/1022
File chrome/browser/autocomplete/autocomplete_popup_view_mac.h (right):

http://codereview.chromium.org/50074/diff/1016/1022#newcode68
Line 68: void SetField(NSTextField *field) {
On 2009/03/23 14:54:59, Mark Mentovai wrote:
> The style for parameters in this file (and for Chrome generally) is to use
> NSTextField* field and not NSTextField *field.

Done.  Think I got them all.

http://codereview.chromium.org/50074/diff/1016/1022#newcode76
Line 76: // TODO(shess): Can table_target_ be a scoped ref?
On 2009/03/23 14:54:59, Mark Mentovai wrote:
> Yes, and so can a few other things in this change.
> 
> I added scoped_cftyperef.h to base a long time ago.  We should also provide
> scoped_nsobject.h and use it here.  That would keep the "strong" ownership
claim
> in code (so you wouldn't need to write "we own") and would reduce the tendency
> toward leaks like the one in my next comment.
> 
> See (lightly-tested) http://codereview.chromium.org/52014 .

Done.

CF is still voodoo to me, don't have a great feel for the intersection with
NSObject stuff.

Interesting thing I noticed.  When I converted table_target_, it just worked
because it's initialized in the constructor.  Converting popup_ did not, because
it was initialized by assignment using =.  The error was something like
"Confused by earlier errors", without any earlier errors.  reset() was of course
the solution, but maybe not the most obvious suggestion given the error.

http://codereview.chromium.org/50074/diff/1016/1022#newcode78
Line 78: NSWindow* popup_;  // we own
On 2009/03/23 14:54:59, Mark Mentovai wrote:
> Looks like you leak popup_.

Done.

I'm a little scared of this one, though, I'm going to need to step back and make
sure that objects get destructed in the right order (or that references are
broken so that destruction order doesn't matter).  So I've made the change but
added a TODO to address that will need re-review.

http://codereview.chromium.org/50074/diff/1016/1023
File chrome/browser/autocomplete/autocomplete_popup_view_mac.mm (right):

http://codereview.chromium.org/50074/diff/1016/1023#newcode40
Line 40: : model_(new AutocompletePopupModel(this, edit_model, profile)),
On 2009/03/23 14:54:59, Mark Mentovai wrote:
> Format's a little off, needs two more spaces of indentation here.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Initia...

Done.

http://codereview.chromium.org/50074/diff/1016/1023#newcode56
Line 56: static NSTableColumn *CreateTableColumn(int tag, id field_class) {
On 2009/03/23 14:54:59, Mark Mentovai wrote:
> |id| is a sucky type here, you don't really mean |id|.  I think you want
> |Class|.

You're probably right, but I'm going to need to go refresh my memory in this
area.  Based on pink's comment, I might restructure it in any case.

http://codereview.chromium.org/50074/diff/1016/1023#newcode58
Line 58: NSTableColumn *col
On 2009/03/23 14:54:59, Mark Mentovai wrote:
> I'd put the = on this line.
> 
> The next line (the continuation line) should have a 4-space, not a 2-space,
> indent.

Done.

http://codereview.chromium.org/50074/diff/1016/1023#newcode79
Line 79: NSTableView *table = [[NSTableView alloc] initWithFrame:NSZeroRect];
On 2009/03/23 14:54:59, Mark Mentovai wrote:
> Leak.
> 
> (popup_ will retain it on the next line, but you never release your ownership
> claim.)

Done.

That is so cool - a bona fide holdover from maybe NS3.3 or so.  I'll figure out
this new-fangled -retain/-release stuff eventually :-).

http://codereview.chromium.org/50074/diff/1016/1023#newcode166
Line 166: void AutocompletePopupViewMac::
On 2009/03/23 14:54:59, Mark Mentovai wrote:
> Breaking after the :: is really weird.  How about:
> 
> void AutocompletePopupViewMac::AcceptInput(
>     WindowOpenDisposition disposition, bool for_drop) {

Done.  And the other locations.

http://codereview.chromium.org/50074/diff/1016/1023#newcode193
Line 193: row:(int)ri {
On 2009/03/23 14:54:59, Mark Mentovai wrote:
> What's with the indentation here?

The second parameter will not fit under 80 columns on the first line.  My
impression was that the goal is to line up the :, but that can't happen for the
second line without adding spaces on the first line.  So I gsearched the client
corpus and picked an example, I think it was something from QSB.

I could alternately do aTableView->v and aTableColumn->c or something like that,
but it's not satisfactory, either.

http://codereview.chromium.org/50074/diff/1016/1023#newcode200
Line 200: if (columnIndex==1) {
On 2009/03/23 14:54:59, Mark Mentovai wrote:
> deanm wrote:
> > are you writing in sqlite style? :)  column_indexs, spaces around == , etc?
> 
> Obj-C style for variable names is correct within Objective-C @interface and
> @implementation sections.
> 
> But you should put spaces around your ==, <, etc.

Done.  Think I got all of these in the CL.

http://codereview.chromium.org/50074/diff/1016/1023#newcode214
Line 214: NSMutableParagraphStyle *style
On 2009/03/23 14:54:59, Mark Mentovai wrote:
> Prefer to see the = on the first line, not the second.  Happens again in a few
> lines.

Done.  Think I got all of these in the CL.

http://codereview.chromium.org/50074/diff/1016/1025
File chrome/browser/cocoa/location_bar_view_mac.mm (right):

http://codereview.chromium.org/50074/diff/1016/1025#newcode20
Line 20: : command_updater_(command_updater),
On 2009/03/23 14:54:59, Mark Mentovai wrote:
> more space

Done.

http://codereview.chromium.org/50074/diff/1016/1025#newcode72
Line 72: /* http://crbug.com/8832 */
On 2009/03/23 14:54:59, Mark Mentovai wrote:
> //

Done.

http://codereview.chromium.org/50074/diff/1016/1025#newcode81
Line 81: OnAutocompleteAccept(const GURL& url,
On 2009/03/23 14:54:59, Mark Mentovai wrote:
> Bad spot for the line break again.

Done.

http://codereview.chromium.org/50074/diff/1016/1027
File chrome/browser/cocoa/tab_contents_controller.mm (right):

http://codereview.chromium.org/50074/diff/1016/1027#newcode82
Line 82: // perhaps _could_ move locationBarView_'s construction down here.
On 2009/03/23 14:54:59, Mark Mentovai wrote:
> In autocomplete_popup_view_mac.h, you left a note that referred to
> initialization order in tab_contents_mac.mm, which is good: if someone finds
> that other TODO and then refactors this, they'll know they can get rid of
> SetField.  But if someone finds this TODO and refactors, they won't know to
get
> rid of SetField.  TODO comments should be more explicit about what can be
> cleaned up, and when, so that when they're split across multiple files (as in
> this case), everything gets cleaned up together when the TODO is fixed.

Done, I think (I reworded the TODO).  It occurs to me that locationBar_ itself
may need to be refactored away from this class, added another TODO at the only
other use.

[Mark Mentovai, 2009-03-24 13:56:42]

For an initial implementation of a huge chunk, I think this is ready for
checkin.  I'd say LGTM but your most recent message said it's not working very
well.  Ideally we wouldn't check anything in that makes things worse for anyone,
so we should probably figure out what it takes to get over that hump before
checking this in.

http://codereview.chromium.org/50074/diff/2017/2022
File chrome/browser/autocomplete/autocomplete_edit_view_mac.h (right):

http://codereview.chromium.org/50074/diff/2017/2022#newcode35
Line 35: ~AutocompleteEditViewMac();
When the class has virtual methods, the destructor should be virtual too.  I'm
surprised that you didn't get a warning from this.

http://codereview.chromium.org/50074/diff/2017/2022#newcode84
Line 84: virtual bool OnAfterPossibleChange() { NOTIMPLEMENTED(); return 0; }
0 isn't a bool, false is.

http://codereview.chromium.org/50074/diff/2017/2022#newcode117
Line 117: AutocompleteEditHelper* edit_helper_;
Expected scoped_nsobject<AutocompleteEditHelper> now.

http://codereview.chromium.org/50074/diff/2017/2023
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/50074/diff/2017/2023#newcode150
Line 150: // no field editor?  Well, at least it won't crash, think on
think of

http://codereview.chromium.org/50074/diff/2017/2023#newcode201
Line 201: void AutocompleteEditViewMac::OnAfterPossibleChange(const
std::wstring& new_text,
For really nutty cases like this, you can write:

void AutocompleteEditViewMac::OnAfterPossibleChange(
    const std::wstring& new_text,
    bool selection_differs,
    ...

http://codereview.chromium.org/50074/diff/2017/2024
File chrome/browser/autocomplete/autocomplete_popup_view_mac.h (right):

http://codereview.chromium.org/50074/diff/2017/2024#newcode33
Line 33: ~AutocompletePopupViewMac();
virtual again.

http://codereview.chromium.org/50074/diff/2017/2024#newcode68
Line 68: const std::wstring &ResultContentsAt(size_t i);
The & is like the *, if you're putting one on the left side of the space you
should do the same with the other.  Again in a few lines.

http://codereview.chromium.org/50074/diff/2017/2024#newcode91
Line 91: void CreatePopupIfNeeded();
The method should come before the data in the private: section (or public: or
protected:).

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

http://codereview.chromium.org/50074/diff/2017/2025
File chrome/browser/autocomplete/autocomplete_popup_view_mac.mm (right):

http://codereview.chromium.org/50074/diff/2017/2025#newcode55
Line 55: static NSTableColumn* CreateTableColumn(int tag, id field_class) {
I think I mentioned this yesterday.  |id| is a bad type to use when you have
something more specific handy.  Have you tried |Class| instead?

http://codereview.chromium.org/50074/diff/2017/2025#newcode69
Line 69: popup_.reset([[NSWindow alloc] initWithContentRect:NSZeroRect
This is correct.  You can't assign directly into our scoped pointers.  It's
intentional, and avoids certain types of memory bugs.

Sorry the compiler error was so bad.  Compilers sometimes do that.  In Obj-C
mode, gcc sometimes doesn't even try.  It'll often just get "confused by earlier
errors" and "bail out" as you saw.

http://codereview.chromium.org/50074/diff/2017/2025#newcode118
Line 118: [[cols objectAtIndex:0] setWidth:floor((r.size.width-kStarWidth)/2)];
Space around - and / operators, same in 2 lines.

http://codereview.chromium.org/50074/diff/2017/2025#newcode125
Line 125: r.origin.y -= r.size.height;
This and the next line can be

r.origin.y -= r.size.height + 2;

http://codereview.chromium.org/50074/diff/2017/2025#newcode193
Line 193: row:(int)ri {
Is this the one we talked about before?  Lining up the colons is cool, but I
don't see these lining up or any other reason for "row" to be floating here
where it is.  (Maybe some tabs slipped in there?)

http://codereview.chromium.org/50074/diff/2017/2025#newcode226
Line 226: // Defering in the interests of getting a minimal implementation in.
Deferring (extra r)

http://codereview.chromium.org/50074/diff/2017/2026
File chrome/browser/cocoa/location_bar_view_mac.h (right):

http://codereview.chromium.org/50074/diff/2017/2026#newcode24
Line 24: ~LocationBarViewMac();
virtual, again?

Or if the destructor body is just going to be empty, leave it out?  (The same
may apply to your other destructors).

[Scott Hess - ex-Googler, 2009-03-28 00:56:57]

On 2009/03/24 13:56:42, Mark Mentovai wrote:
> For an initial implementation of a huge chunk, I think this is ready for
> checkin.  I'd say LGTM but your most recent message said it's not working very
> well.  Ideally we wouldn't check anything in that makes things worse for
anyone,
> so we should probably figure out what it takes to get over that hump before
> checking this in.

Not quite back in, yet, so not a full response, but this part is easy.  I'm
93.7% certain that I'm seeing something related to browse history which is only
happening because this code tickles it.  On Monday I'm going to try to verify
this, and if it's the case I'll probably wire out the history part of
autocomplete so that this can go in.  I think after that there are a lot of much
more isolated features which can drop in a pipeline (history fully working,
mouse hover, tweaking fonts, various other UI issues, deleting items from the
pulldown, tab-search, etc).

[Scott Hess - ex-Googler, 2009-03-30 21:29:26]

This is essentially a checkpoint before digging in to look for remaining logic
problems.

http://codereview.chromium.org/50074/diff/1016/1020
File chrome/browser/autocomplete/autocomplete_edit_view_mac.h (right):

http://codereview.chromium.org/50074/diff/1016/1020#newcode114
Line 114: NSTextField* field_;  // owned by controller
On 2009/03/23 17:07:08, pink wrote:
> do you mean controller_?

Hmm, that code got refactored from somewhere else during development.  It's
coming down from the tab controller, but I believe the ownership is implicit as
part of the window.  Later I expect it will move to the location bar as that
implementation is fleshed out.

Should it be scoped_nsobject<NSTextField>?  That doesn't quite feel consistent
with the C++ object references, though, so I'm thinking not.

http://codereview.chromium.org/50074/diff/1016/1020#newcode115
Line 115: AutocompleteEditHelper* edit_helper_;
Oops, adding scoped_nsobject<> here, and making sure I disconnect field_
delegate at destruct to prevent any funky callbacks.

http://codereview.chromium.org/50074/diff/1016/1021
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/50074/diff/1016/1021#newcode19
Line 19: - initWithEditView:(AutocompleteEditViewMac *)view;
On 2009/03/23 17:07:08, pink wrote:
> inconsistent spacing on (foo*) and (foo *). Stick with one through the whole
> file.

Done.

http://codereview.chromium.org/50074/diff/1016/1021#newcode33
Line 33: edit_helper_([[AutocompleteFieldDelegate alloc] initWithEditView:this])
{
On 2009/03/23 17:07:08, pink wrote:
> add some DCHECKs on the parameters just in case?

You mean like for controller, toolbar_model, profile, command_updater?  If so,
done.

Doesn't seem to be the style for the other code, though.

http://codereview.chromium.org/50074/diff/1016/1021#newcode107
Line 107: = [[[NSMutableAttributedString alloc] initWithString:ss] autorelease];
On 2009/03/23 17:07:08, pink wrote:
> indent 4 spaces on continuation

Done.  I believe I have all of these.

http://codereview.chromium.org/50074/diff/1016/1021#newcode151
Line 151: [[field_ currentEditor] setSelectedRange:selected_range];
On 2009/03/23 17:07:08, pink wrote:
> DCHECK that it became the first responder?

Done.

http://codereview.chromium.org/50074/diff/1016/1021#newcode240
Line 240: if (cmd==@selector(moveUp:)) {
On 2009/03/23 17:07:08, pink wrote:
> spaces between variables and operators.

Done, think I got them all.  SQLite unfortunately used the exact opposite style
guidelines :-).

http://codereview.chromium.org/50074/diff/1016/1023
File chrome/browser/autocomplete/autocomplete_popup_view_mac.mm (right):

http://codereview.chromium.org/50074/diff/1016/1023#newcode56
Line 56: static NSTableColumn *CreateTableColumn(int tag, id field_class) {
On 2009/03/24 05:53:18, shess wrote:
> On 2009/03/23 14:54:59, Mark Mentovai wrote:
> > |id| is a sucky type here, you don't really mean |id|.  I think you want
> > |Class|.
> 
> You're probably right, but I'm going to need to go refresh my memory in this
> area.  Based on pink's comment, I might restructure it in any case.

I thought I had an idea for how to rearrange this code to not need the helper,
but I was incorrect.  Renamed Create -> Make, id -> Class.

http://codereview.chromium.org/50074/diff/1016/1023#newcode70
Line 70: popup_ = [[NSWindow alloc] initWithContentRect:NSZeroRect
On 2009/03/23 17:07:08, pink wrote:
> this should probably be a child window of the main window as well.

I have only a cargo-cult understanding of the parent-child window relationship. 
The current code installs popup_ as the window's child at show, and removes the
child relationship at hide.  Adding popup_ as a child seems to force it to be
visible, and [popup_ orderOut:nil] seems to hide the parent window?  Makes no
sense.

http://codereview.chromium.org/50074/diff/1016/1023#newcode183
Line 183: return [NSImage imageNamed:NSImageNameQuickLookTemplate];
On 2009/03/23 17:07:08, pink wrote:
> don't we already have a star image? how does this differ from what we use for
> the toolbar already?

Oops.  Done.

http://codereview.chromium.org/50074/diff/1016/1025
File chrome/browser/cocoa/location_bar_view_mac.mm (right):

http://codereview.chromium.org/50074/diff/1016/1025#newcode23
Line 23: transition_(PageTransition::TYPED) {
On 2009/03/23 17:07:08, pink wrote:
> DCHECKs on parameters?

Done.

http://codereview.chromium.org/50074/diff/1016/1027
File chrome/browser/cocoa/tab_contents_controller.mm (right):

http://codereview.chromium.org/50074/diff/1016/1027#newcode89
Line 89: return dynamic_cast<LocationBar*>(locationBarView_);
On 2009/03/23 17:07:08, pink wrote:
> we probably shouldn't be using dynamic_cast.

For some reason I thought that was the way to move from derived to base class. 
But, I only find one use of it for this
(browser/task_manager_resource_providers.cc), so dropping my use.

http://codereview.chromium.org/50074/diff/2017/2022
File chrome/browser/autocomplete/autocomplete_edit_view_mac.h (right):

http://codereview.chromium.org/50074/diff/2017/2022#newcode35
Line 35: ~AutocompleteEditViewMac();
On 2009/03/24 13:56:42, Mark Mentovai wrote:
> When the class has virtual methods, the destructor should be virtual too.  I'm
> surprised that you didn't get a warning from this.

Done.  Nothing for you on the warning.

http://codereview.chromium.org/50074/diff/2017/2022#newcode84
Line 84: virtual bool OnAfterPossibleChange() { NOTIMPLEMENTED(); return 0; }
On 2009/03/24 13:56:42, Mark Mentovai wrote:
> 0 isn't a bool, false is.

Done.

http://codereview.chromium.org/50074/diff/2017/2022#newcode117
Line 117: AutocompleteEditHelper* edit_helper_;
On 2009/03/24 13:56:42, Mark Mentovai wrote:
> Expected scoped_nsobject<AutocompleteEditHelper> now.

Hadn't noticed that until I did Pink's review, done now.

http://codereview.chromium.org/50074/diff/2017/2023
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/50074/diff/2017/2023#newcode150
Line 150: // no field editor?  Well, at least it won't crash, think on
On 2009/03/24 13:56:42, Mark Mentovai wrote:
> think of

Done.

http://codereview.chromium.org/50074/diff/2017/2023#newcode201
Line 201: void AutocompleteEditViewMac::OnAfterPossibleChange(const
std::wstring& new_text,
On 2009/03/24 13:56:42, Mark Mentovai wrote:
> For really nutty cases like this, you can write:
> 
> void AutocompleteEditViewMac::OnAfterPossibleChange(
>     const std::wstring& new_text,
>     bool selection_differs,
>     ...

Done, as this snuck past 80 cols at some point.

http://codereview.chromium.org/50074/diff/2017/2024
File chrome/browser/autocomplete/autocomplete_popup_view_mac.h (right):

http://codereview.chromium.org/50074/diff/2017/2024#newcode33
Line 33: ~AutocompletePopupViewMac();
On 2009/03/24 13:56:42, Mark Mentovai wrote:
> virtual again.

Done.

http://codereview.chromium.org/50074/diff/2017/2024#newcode68
Line 68: const std::wstring &ResultContentsAt(size_t i);
On 2009/03/24 13:56:42, Mark Mentovai wrote:
> The & is like the *, if you're putting one on the left side of the space you
> should do the same with the other.  Again in a few lines.

Done.

http://codereview.chromium.org/50074/diff/2017/2024#newcode91
Line 91: void CreatePopupIfNeeded();
On 2009/03/24 13:56:42, Mark Mentovai wrote:
> The method should come before the data in the private: section (or public: or
> protected:).
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

Done.

http://codereview.chromium.org/50074/diff/2017/2025
File chrome/browser/autocomplete/autocomplete_popup_view_mac.mm (right):

http://codereview.chromium.org/50074/diff/2017/2025#newcode118
Line 118: [[cols objectAtIndex:0] setWidth:floor((r.size.width-kStarWidth)/2)];
On 2009/03/24 13:56:42, Mark Mentovai wrote:
> Space around - and / operators, same in 2 lines.

Done.

http://codereview.chromium.org/50074/diff/2017/2025#newcode125
Line 125: r.origin.y -= r.size.height;
On 2009/03/24 13:56:42, Mark Mentovai wrote:
> This and the next line can be
> 
> r.origin.y -= r.size.height + 2;

Oops, used to be just the height, then something I saw elsewhere hinted that an
additional couple pixels might be in order.  Hadn't consolidated after verifying
this...

http://codereview.chromium.org/50074/diff/2017/2025#newcode193
Line 193: row:(int)ri {
On 2009/03/24 13:56:42, Mark Mentovai wrote:
> Is this the one we talked about before?  Lining up the colons is cool, but I
> don't see these lining up or any other reason for "row" to be floating here
> where it is.  (Maybe some tabs slipped in there?)

No tabs that I can see.  colon in row: lines up with colon in tableView:.

http://codereview.chromium.org/50074/diff/2017/2025#newcode226
Line 226: // Defering in the interests of getting a minimal implementation in.
On 2009/03/24 13:56:42, Mark Mentovai wrote:
> Deferring (extra r)

Done.

http://codereview.chromium.org/50074/diff/2017/2026
File chrome/browser/cocoa/location_bar_view_mac.h (right):

http://codereview.chromium.org/50074/diff/2017/2026#newcode24
Line 24: ~LocationBarViewMac();
On 2009/03/24 13:56:42, Mark Mentovai wrote:
> virtual, again?
> 
> Or if the destructor body is just going to be empty, leave it out?  (The same
> may apply to your other destructors).

virtual done.  Needs the empty destructor so that clients don't have to include
AutocompleteEditViewMac definition (scoped_ptr and all).  Other classes all have
acquired new code in their destructors.

[pink (ping after 24hrs), 2009-03-30 21:55:51]

I just scanned over your comments, I didn't look at the updated patch
thoroughly. LGTM given your comments.

http://codereview.chromium.org/50074/diff/1016/1020
File chrome/browser/autocomplete/autocomplete_edit_view_mac.h (right):

http://codereview.chromium.org/50074/diff/1016/1020#newcode114
Line 114: NSTextField* field_;  // owned by controller
On 2009/03/30 21:29:26, shess wrote:
> On 2009/03/23 17:07:08, pink wrote:
> > do you mean controller_?
> 
> Hmm, that code got refactored from somewhere else during development.  It's
> coming down from the tab controller, but I believe the ownership is implicit
as
> part of the window.  Later I expect it will move to the location bar as that
> implementation is fleshed out.
> 
> Should it be scoped_nsobject<NSTextField>?  That doesn't quite feel consistent
> with the C++ object references, though, so I'm thinking not.

If this class doesn't retain it, it shouldn't be a scoped_nsobject. You should
probably also mention that it's a weak reference just to be clear.

http://codereview.chromium.org/50074/diff/1016/1023
File chrome/browser/autocomplete/autocomplete_popup_view_mac.mm (right):

http://codereview.chromium.org/50074/diff/1016/1023#newcode70
Line 70: popup_ = [[NSWindow alloc] initWithContentRect:NSZeroRect
On 2009/03/30 21:29:26, shess wrote:
> On 2009/03/23 17:07:08, pink wrote:
> > this should probably be a child window of the main window as well.
> 
> I have only a cargo-cult understanding of the parent-child window
relationship. 
> The current code installs popup_ as the window's child at show, and removes
the
> child relationship at hide.  Adding popup_ as a child seems to force it to be
> visible, and [popup_ orderOut:nil] seems to hide the parent window?  Makes no
> sense.

Ah ok, I missed it being a child window at show. You're one step ahead of me.
Nothing to see here...

http://codereview.chromium.org/50074/diff/1016/1027
File chrome/browser/cocoa/tab_contents_controller.mm (right):

http://codereview.chromium.org/50074/diff/1016/1027#newcode89
Line 89: return dynamic_cast<LocationBar*>(locationBarView_);
On 2009/03/30 21:29:26, shess wrote:
> On 2009/03/23 17:07:08, pink wrote:
> > we probably shouldn't be using dynamic_cast.
> 
> For some reason I thought that was the way to move from derived to base class.

> But, I only find one use of it for this
> (browser/task_manager_resource_providers.cc), so dropping my use.

Derived -> base is always ok and can be done with static_cast, it's the reverse
(casting to more specific a more-specific type) that needs dynamic_cast to be
certain. That said, we still shouldn't be using dynamic_cast in google C++.

[Scott Hess - ex-Googler, 2009-04-03 23:01:01]

I've hacked things to drop the popup when switching tabs.  It is not pretty at
all.  Basically, I've added stuff so that TabStripController can say "Drop the
popup", that is all.

I am very confident that there is a better way, but I am not confident that I
know what it is, yet.  I also don't want to cause reviewer-fatigue or let this
review run on too long.  So I'm thinking check things in this way on Monday, so
that I can respond to howls of pain ASAP.

I also fixed an out-of-order include, and pulled some NOTIMPLEMENTED() functions
back into their header file (because I have no immediate intention to implement
them at this time, having them in the .mm file felt like an implied promise).

[Scott Hess - ex-Googler, 2009-04-06 22:12:18]

On Mon, Apr 6, 2009 at 3:06 PM, Scott Hess <shess@chromium.org> wrote:
> I've found my last problem, a case where destructor ordering needs to
> be very specific because of interconnected objects.  Dean's GTK code
> already handled this, either I missed it the first time, or was just
> plain stupid (the associated comment in the gtk code seems pretty
> clear).  I'm shipping this.

And by "shipping this", I mean that the tree is closed, and I'm scheduled to be
away from computers this evening, and hopefully I can sneak it in after the kids
are in bed so that you can "enjoy" in the EDT AM.

[Scott Hess - ex-Googler, 2009-04-09 23:55:14]

After wasting a bunch of time trying to mind-meld with ui_tests, and finding it
to be too deep, I just sat down and started bisecting the changes to see which
were different in ui_tests.  Even though the different behaviour on my machine
doesn't match the difference on the try servers..  Turned out there was a
non-change which had happened in a merge which landed at some point during
development, so I was linking the _mac and _generic variant of
autocomplete_provider_list.

That and now I've adapted to your new code.

Currently floating past the try servers.

[With Dog as my witness, I will land this change tomorrow.]

[Scott Hess - ex-Googler, 2009-04-10 17:41:15]

On 2009/04/09 23:55:14, shess wrote:
> After wasting a bunch of time trying to mind-meld with ui_tests, and finding
it
> to be too deep, I just sat down and started bisecting the changes to see which
> were different in ui_tests.  Even though the different behaviour on my machine
> doesn't match the difference on the try servers..  Turned out there was a
> non-change which had happened in a merge which landed at some point during
> development, so I was linking the _mac and _generic variant of
> autocomplete_provider_list.
> 
> That and now I've adapted to your new code.
> 
> Currently floating past the try servers.
> 
> [With Dog as my witness, I will land this change tomorrow.]

Update: Passed try servers with LocationBarViewMacTest disabled.  I'm turning
the test into a TODO and a tracking bug, because making it work in the new world
will be somewhat daunting, and I'm quite worried about spending another week
pending on this CL.