Moving common code into OmniboxView from OmniboxView*

Moving common code into OmniboxView from OmniboxView*



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150426

[dominich, 2012-07-23 23:00:07]

I realized there's quite a bit of code that is shared between the cross-platform
OmniboxView implementations, so this is an attempt to start to consolidate that
code.

~203 lines of code removed.

No functional changes intended.

[Matt Perry, 2012-07-23 23:56:42]

I'm not the best person to review this. If you included me as an extensions
OWNER, then the extensions change LGTM.

[tfarina, 2012-07-24 01:47:25]

Peter is probably the most indicated for Omnibox/Autocomplete changes.
Copied him.

On Monday, July 23, 2012, wrote:

> I'm not the best person to review this. If you included me as an extensions
> OWNER, then the extensions change LGTM.
>
>
http://codereview.chromium.**org/10810062/<http://codereview.chromium.org/108...
>


-- 
Thiago

[sky, 2012-07-24 04:36:18]

Agreed, Peter should review this.

  -Scott

On Mon, Jul 23, 2012 at 6:47 PM, Thiago Farina <tfarina@chromium.org> wrote:
> Peter is probably the most indicated for Omnibox/Autocomplete changes.
> Copied him.
>
>
> On Monday, July 23, 2012, wrote:
>>
>> I'm not the best person to review this. If you included me as an
>> extensions
>> OWNER, then the extensions change LGTM.
>>
>> http://codereview.chromium.org/10810062/
>
>
>
> --
> Thiago

[dominich, 2012-07-24 05:15:17]

sgtm. I just went with git cl upload's suggestions.
On Jul 23, 2012 9:36 PM, "Scott Violet" <sky@chromium.org> wrote:

> Agreed, Peter should review this.
>
>   -Scott
>
> On Mon, Jul 23, 2012 at 6:47 PM, Thiago Farina <tfarina@chromium.org>
> wrote:
> > Peter is probably the most indicated for Omnibox/Autocomplete changes.
> > Copied him.
> >
> >
> > On Monday, July 23, 2012, wrote:
> >>
> >> I'm not the best person to review this. If you included me as an
> >> extensions
> >> OWNER, then the extensions change LGTM.
> >>
> >> http://codereview.chromium.org/10810062/
> >
> >
> >
> > --
> > Thiago
>

[Peter Kasting, 2012-07-26 03:59:17]

http://codereview.chromium.org/10810062/diff/12001/chrome/browser/ui/cocoa/om...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

http://codereview.chromium.org/10810062/diff/12001/chrome/browser/ui/cocoa/om...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:906:
OmniboxView::ClosePopup();
Nit: Why do we need to provide this override at all?

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/cocoa/om...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/cocoa/om...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:977: return
static_cast<int>(GetTextLength());
Nit: For all these derived classes, I kind of feel like we should just have one
base-class GetTextLength() virtual, the derived classes should implement it
directly, and then call it directly -- instead of having both GetTextLength()
and GetOmniboxTextLength() in most.

If we use "int" as the return type for the base class function (as it is right
now), then most places in most derived classes don't need explicit casts -- I
think in this file, for example, maybe just the comparison in IsCaretAtEnd()
does?  (Not sure)

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/gtk/omni...
File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.h (right):

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/gtk/omni...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.h:226: // OmniboxView overrides
Nit: This seems like a bit of a random place in the ordering to have an override
appear.

I also normally suggest not putting a descriptive comment on the function in the
derived class.

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_unittest.cc (right):

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_unittest.cc:23: TestingOmniboxView() :
OmniboxView(NULL, NULL, NULL, NULL) {}
Nit: Technically, we're not supposed to provide inline definitions for
non-unix-hacker()-style function even in .cc files... but maybe these sorts of
"do nothing" overrides should be an exception.

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_unittest.cc:25: virtual OmniboxEditModel*
GetModel() OVERRIDE { return NULL; }
Nit: Given that this class already passes the superclass a NULL Profile* (just
above), I think GetModel() would already always return NULL; thus we don't need
to override it here, which means it doesn't need to be virtual, which means it
can go back to being "model()".

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_view.cc (right):

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_view.cc:93: // floor.
I think this can perhaps happen if for example you type a lone question mark and
hit enter.  See if that reaches here and if so you can probably replace Scott's
comment with a better one.

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_view.cc:94: if
(!match.destination_url.is_valid()) {
Nit: No {}

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_view.cc:119: // TODO(deanm): something about
selection / focus change here.
I don't know what this comment means, and I'm inclined to just nuke it at this
point.  Maybe erg might have a guess.

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_view.h:223: virtual ~OmniboxView();
Nit: Destructor should be at the top of the relevant section, just below the
constructor.  This destructor can perhaps be protected?

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_view.h:240: scoped_ptr<OmniboxEditModel>
model_;
Nit: The Google style guide forbids protected data members and requests that
subclasses use accessors to get at base class members.

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/views/om...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:671:
OmniboxView::SetUserText(L"?");
Why is this a direct superclass call?  That changes the functionality.

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:983:
OmniboxView::SetUserText(text);
Another case where I'm not sure why this is a superclass call.

[dominich, 2012-07-26 22:33:23]

http://codereview.chromium.org/10810062/diff/12001/chrome/browser/ui/cocoa/om...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

http://codereview.chromium.org/10810062/diff/12001/chrome/browser/ui/cocoa/om...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:906:
OmniboxView::ClosePopup();
On 2012/07/26 03:59:17, Peter Kasting wrote:
> Nit: Why do we need to provide this override at all?

It's an override of AutocompleteTextFieldObserver.

Multiple inheritance has made me very sad working on this CL.

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/cocoa/om...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/cocoa/om...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:977: return
static_cast<int>(GetTextLength());
On 2012/07/26 03:59:17, Peter Kasting wrote:
> Nit: For all these derived classes, I kind of feel like we should just have
one
> base-class GetTextLength() virtual, the derived classes should implement it
> directly, and then call it directly -- instead of having both GetTextLength()
> and GetOmniboxTextLength() in most.
> 
> If we use "int" as the return type for the base class function (as it is right
> now), then most places in most derived classes don't need explicit casts -- I
> think in this file, for example, maybe just the comparison in IsCaretAtEnd()
> does?  (Not sure)

I originally built this however there are some problems:

Many of the users require unsigned (this is true for OSX NSUInteger and win
size_t). Changing the call-sites would have been static_cast spaghetti. I'm not
convinced this is better, but I couldn't come up with another plan.

Also, there's a subtlety in the win version where GetTextLength is actually
defined in the ATL rich text control that OmniboxViewWin inherits from. Hence
the need for the indirection and the rename to make it explicitly the Omnibox
version. Prior to this, the derived version was overriding both base classes on
win. Ugh.

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/gtk/omni...
File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.h (right):

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/gtk/omni...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.h:226: // OmniboxView overrides
On 2012/07/26 03:59:17, Peter Kasting wrote:
> Nit: This seems like a bit of a random place in the ordering to have an
override
> appear.
> 
> I also normally suggest not putting a descriptive comment on the function in
the
> derived class.

Done.

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_unittest.cc (right):

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_unittest.cc:23: TestingOmniboxView() :
OmniboxView(NULL, NULL, NULL, NULL) {}
On 2012/07/26 03:59:17, Peter Kasting wrote:
> Nit: Technically, we're not supposed to provide inline definitions for
> non-unix-hacker()-style function even in .cc files... but maybe these sorts of
> "do nothing" overrides should be an exception.

I think for testing it's often ignored. Especially for these do-nothing
overrides.

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_unittest.cc:25: virtual OmniboxEditModel*
GetModel() OVERRIDE { return NULL; }
On 2012/07/26 03:59:17, Peter Kasting wrote:
> Nit: Given that this class already passes the superclass a NULL Profile* (just
> above), I think GetModel() would already always return NULL; thus we don't
need
> to override it here, which means it doesn't need to be virtual, which means it
> can go back to being "model()".

Done.

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_view.cc (right):

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_view.cc:94: if
(!match.destination_url.is_valid()) {
On 2012/07/26 03:59:17, Peter Kasting wrote:
> Nit: No {}

Done.

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_view.h:223: virtual ~OmniboxView();
On 2012/07/26 03:59:17, Peter Kasting wrote:
> Nit: Destructor should be at the top of the relevant section, just below the
> constructor.  This destructor can perhaps be protected?

It can't be protected. It's destroyed through scoped_ptr on at least CrOS.

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_view.h:240: scoped_ptr<OmniboxEditModel>
model_;
On 2012/07/26 03:59:17, Peter Kasting wrote:
> Nit: The Google style guide forbids protected data members and requests that
> subclasses use accessors to get at base class members.

Done.

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/views/om...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:671:
OmniboxView::SetUserText(L"?");
On 2012/07/26 03:59:17, Peter Kasting wrote:
> Why is this a direct superclass call?  That changes the functionality.

There is no version in the derived class that takes a single parameter. Instead,
we call the base class version which then dispatches to the virtual call with
multiple parameters.

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:983:
OmniboxView::SetUserText(text);
On 2012/07/26 03:59:17, Peter Kasting wrote:
> Another case where I'm not sure why this is a superclass call.

see above. There shouldn't be a functional change here.

[Peter Kasting, 2012-07-26 23:03:24]

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/cocoa/om...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/cocoa/om...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:977: return
static_cast<int>(GetTextLength());
On 2012/07/26 22:33:24, dominich wrote:
> Many of the users require unsigned (this is true for OSX NSUInteger and win
> size_t). Changing the call-sites would have been static_cast spaghetti. I'm
not
> convinced this is better, but I couldn't come up with another plan.

My hope was that for most of those cases, if we were passing an unsigned value
to a function taking a signed argument, we didn't need an explicit cast.  But
I'm not sure.  I thought about suggesting that the base class version return
size_t -- which seems like the more correct type -- but it looked like much of
the existing usage of the size_t-returning function on Win was actually to
assign/compare to int :/.

It may still be slightly better on Mac to have the implementationn of this be
the only such function, and then cast its return as needed, but I'm not sure.

> Also, there's a subtlety in the win version where GetTextLength is actually
> defined in the ATL rich text control that OmniboxViewWin inherits from. Hence
> the need for the indirection and the rename to make it explicitly the Omnibox
> version. Prior to this, the derived version was overriding both base classes
on
> win. Ugh.

Ugh is right.  That means that if we named this GetTextLength, the Windows code
would need to explicitly qualify which base class it wanted to use.  That seems
unfortunate enough to make me think the route you've gone is better, perhaps
with a comment in the base class about avoiding this naming conflict.

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_view.cc (right):

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_view.cc:119: // TODO(deanm): something about
selection / focus change here.
On 2012/07/26 03:59:17, Peter Kasting wrote:
> I don't know what this comment means, and I'm inclined to just nuke it at this
> point.  Maybe erg might have a guess.

Note this is still outstanding.

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_view.h:223: virtual ~OmniboxView();
On 2012/07/26 22:33:24, dominich wrote:
> On 2012/07/26 03:59:17, Peter Kasting wrote:
> > Nit: Destructor should be at the top of the relevant section, just below the
> > constructor.  This destructor can perhaps be protected?
> 
> It can't be protected. It's destroyed through scoped_ptr on at least CrOS.

Ah, I see.  Yeah, has to be public.

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/views/om...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:252: model_.reset();
Sort of worries me that we had this code in the first place when the comment
doesn't make it clear what it's for -- and that we're no longer doing half of it
if it mattered.

Can you spelunk this a bit and figure out if we can just nuke the whole block?

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/views/om...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:671:
OmniboxView::SetUserText(L"?");
On 2012/07/26 22:33:24, dominich wrote:
> On 2012/07/26 03:59:17, Peter Kasting wrote:
> > Why is this a direct superclass call?  That changes the functionality.
> 
> There is no version in the derived class that takes a single parameter.
Instead,
> we call the base class version which then dispatches to the virtual call with
> multiple parameters.

Oh, and scope resolution requires that we qualify this for it to compile?  Ugh.

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/extensions/...
File chrome/browser/extensions/api/omnibox/omnibox_apitest.cc (right):

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/extensions/...
chrome/browser/extensions/api/omnibox/omnibox_apitest.cc:56:
popup_model()->autocomplete_controller();
Nit: This change doesn't really improve things, I'd just revert

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/cocoa/om...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/cocoa/om...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:32: // Focus-handling
between |field_| and |model()| is a bit subtle.
Nit: Now that it's model() and not model_, the || can be removed; in comments
you may actually want to say "the model" instead of "model()", although use your
best judgment.  (10 places)

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/cocoa/om...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:676: // Figure out what
changed and notify the model().
Nit: This should probably not have parens.

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/cocoa/om...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:988: selection.location ==
GetTextLength();
Nit: This change unnecessary

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/gtk/omni...
File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right):

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/gtk/omni...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:666: // If only selection was
changed, we don't need to call |controller()|'s
Nit: More spots with "|foo()|" that can be "foo()" or "the foo" (5 places)

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/gtk/omni...
File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.h (right):

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/gtk/omni...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.h:220: // OmniboxView overrides.
Nit: Still seems kind of randomly located.  Maybe above or below the
CHROMEGTK_CALLBACK list?  I don't know if we have any consistency elsewhere.

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_view.cc (right):

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_view.cc:84: CHECK(false);
Nit: Don't handle checkfailures -- if this really shouldn't happen, do
"DCHECK(match.destination_url.is_valid());" and then continue processing (and
omit Scott's TODO).  Note use of DCHECK in place of CHECK -- I don't think we
need CHECK as this isn't a security hole to continue.

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right):

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:529:
UTF16ToUTF8(omnibox_view->model()->keyword()));
Nit: This change unnecessary

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/views/om...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:656: // If only selection
was changed, we don't need to call |model()|'s
Nit: Another case of |model()|

[dominich, 2012-07-27 20:33:54]

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/extensions/...
File chrome/browser/extensions/api/omnibox/omnibox_apitest.cc (right):

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/extensions/...
chrome/browser/extensions/api/omnibox/omnibox_apitest.cc:56:
popup_model()->autocomplete_controller();
On 2012/07/26 23:03:24, Peter Kasting wrote:
> Nit: This change doesn't really improve things, I'd just revert
hangover from model->GetModel->model rename.

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/cocoa/om...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/cocoa/om...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:32: // Focus-handling
between |field_| and |model()| is a bit subtle.
On 2012/07/26 23:03:24, Peter Kasting wrote:
> Nit: Now that it's model() and not model_, the || can be removed; in comments
> you may actually want to say "the model" instead of "model()", although use
your
> best judgment.  (10 places)

Done.

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/cocoa/om...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:676: // Figure out what
changed and notify the model().
On 2012/07/26 23:03:24, Peter Kasting wrote:
> Nit: This should probably not have parens.

Done.

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/cocoa/om...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:988: selection.location ==
GetTextLength();
On 2012/07/26 23:03:24, Peter Kasting wrote:
> Nit: This change unnecessary

Done.

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/gtk/omni...
File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right):

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/gtk/omni...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:666: // If only selection was
changed, we don't need to call |controller()|'s
On 2012/07/26 23:03:24, Peter Kasting wrote:
> Nit: More spots with "|foo()|" that can be "foo()" or "the foo" (5 places)

Done.

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/gtk/omni...
File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.h (right):

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/gtk/omni...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.h:220: // OmniboxView overrides.
On 2012/07/26 23:03:24, Peter Kasting wrote:
> Nit: Still seems kind of randomly located.  Maybe above or below the
> CHROMEGTK_CALLBACK list?  I don't know if we have any consistency elsewhere.

my thinking was:

below callbacks.
below static method.
below non-static methods relating to static method.

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_view.cc (right):

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_view.cc:84: CHECK(false);
On 2012/07/26 23:03:24, Peter Kasting wrote:
> Nit: Don't handle checkfailures -- if this really shouldn't happen, do
> "DCHECK(match.destination_url.is_valid());" and then continue processing (and
> omit Scott's TODO).  Note use of DCHECK in place of CHECK -- I don't think we
> need CHECK as this isn't a security hole to continue.

Confirmed that we don't get a hit here when typing '?' on its own (at least on
linux).

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right):

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:529:
UTF16ToUTF8(omnibox_view->model()->keyword()));
On 2012/07/26 23:03:24, Peter Kasting wrote:
> Nit: This change unnecessary

Done.

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/views/om...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:656: // If only selection
was changed, we don't need to call |model()|'s
On 2012/07/26 23:03:24, Peter Kasting wrote:
> Nit: Another case of |model()|

Done.

[Peter Kasting, 2012-07-28 00:23:06]

There are still a few comments on older patch sets.  Take a glance at my last
email, I think all the older comments come at the top.  I think there are only
like two things.

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/gtk/omni...
File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.h (right):

http://codereview.chromium.org/10810062/diff/25001/chrome/browser/ui/gtk/omni...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.h:220: // OmniboxView overrides.
On 2012/07/27 20:33:54, dominich wrote:
> On 2012/07/26 23:03:24, Peter Kasting wrote:
> > Nit: Still seems kind of randomly located.  Maybe above or below the
> > CHROMEGTK_CALLBACK list?  I don't know if we have any consistency elsewhere.
> 
> my thinking was:
> 
> below callbacks.
> below static method.
> below non-static methods relating to static method.

Yeah, I'd probably put it above the static method, but it's not a big deal at
all.  Whatever works.

[dominich, 2012-07-30 16:42:59]

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_view.cc (right):

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_view.cc:119: // TODO(deanm): something about
selection / focus change here.
On 2012/07/26 23:03:24, Peter Kasting wrote:
> On 2012/07/26 03:59:17, Peter Kasting wrote:
> > I don't know what this comment means, and I'm inclined to just nuke it at
this
> > point.  Maybe erg might have a guess.
> 
> Note this is still outstanding.

I'm tempted to just nuke it. We don't have any open bugs about selection and
focus change issues. It looks like the comment was added for gtk and then
brought over to mac by shess. Win and views have no similar comment.

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/views/om...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:252: model_.reset();
On 2012/07/26 23:03:24, Peter Kasting wrote:
> Sort of worries me that we had this code in the first place when the comment
> doesn't make it clear what it's for -- and that we're no longer doing half of
it
> if it mattered.

What are we no longer doing? The model_.reset() call has been moved into the
base destructor.

The comment is clear to me: We pass 'this' to the model when we initialize it so
when we're going away we want to make sure we destroy the model_. Then anyone
accessing it can't get hold of a stale 'this'.

Unless they've cached it, of course.

> 
> Can you spelunk this a bit and figure out if we can just nuke the whole block?

[Peter Kasting, 2012-07-30 17:06:03]

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/views/om...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:252: model_.reset();
On 2012/07/30 16:42:59, dominich wrote:
> On 2012/07/26 23:03:24, Peter Kasting wrote:
> > Sort of worries me that we had this code in the first place when the comment
> > doesn't make it clear what it's for -- and that we're no longer doing half
of
> it
> > if it mattered.
> 
> What are we no longer doing? The model_.reset() call has been moved into the
> base destructor.

What I mean is that the bit about "reset these before we tear down any internal
state" is no longer fully true -- with the moved call to model_.reset(), we now
tear down all subclass members before we reset the model in the base class
destructor.  Don't know if that matters or not.

If it doesn't matter, then we can remove the explicit calls and let things be
cleaned up implicitly.  But if it does then this is problematic.

[dominich, 2012-07-30 17:21:41]

On 2012/07/30 17:06:03, Peter Kasting wrote:
>
http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/views/om...
> File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):
> 
>
http://codereview.chromium.org/10810062/diff/21001/chrome/browser/ui/views/om...
> chrome/browser/ui/views/omnibox/omnibox_view_views.cc:252: model_.reset();
> On 2012/07/30 16:42:59, dominich wrote:
> > On 2012/07/26 23:03:24, Peter Kasting wrote:
> > > Sort of worries me that we had this code in the first place when the
comment
> > > doesn't make it clear what it's for -- and that we're no longer doing half
> of
> > it
> > > if it mattered.
> > 
> > What are we no longer doing? The model_.reset() call has been moved into the
> > base destructor.
> 
> What I mean is that the bit about "reset these before we tear down any
internal
> state" is no longer fully true -- with the moved call to model_.reset(), we
now
> tear down all subclass members before we reset the model in the base class
> destructor.  Don't know if that matters or not.
> 
> If it doesn't matter, then we can remove the explicit calls and let things be
> cleaned up implicitly.  But if it does then this is problematic.

I see what you mean. I suppose the concern was that the controller destructor
might try to do something with the model which might try to dereference the view
which is in the middle of being destroyed. What I can't tell is if this code was
added to defend against a found bug or just someone being extra cautious.

I'm tempted to remove these calls. If the controller is doing something in the
destructor that uses the model then that's just wrong. Looking through the
Controllers I don't see anything dangerous.

However, I'm not sure how to catch any issues that might come up. I assume tests
would start crashing with a callstack somewhere in the omnibox shutdown code.

[Peter Kasting, 2012-07-30 17:22:56]

I'm also tempted to remove it, I was hoping that if you spelunked in the svn
history you might find the commit that added these.

[dominich, 2012-07-30 17:25:11]

On 2012/07/30 17:22:56, Peter Kasting wrote:
> I'm also tempted to remove it, I was hoping that if you spelunked in the svn
> history you might find the commit that added these.

I'm digging now. It looks like the same technique is used for the popup view. It
destroys the model before the object is destroyed as the model could call back
into the view.

I don't think it /does/, but it /can/.

[dominich, 2012-07-30 17:34:50]

On 2012/07/30 17:25:11, dominich wrote:
> On 2012/07/30 17:22:56, Peter Kasting wrote:
> > I'm also tempted to remove it, I was hoping that if you spelunked in the svn
> > history you might find the commit that added these.
> 
> I'm digging now. It looks like the same technique is used for the popup view.
It
> destroys the model before the object is destroyed as the model could call back
> into the view.
> 
> I don't think it /does/, but it /can/.

http://git.chromium.org/gitweb/?p=chromium.git;a=blobdiff;f=chrome/browser/au...

Looks like it was put in pretty much the day after the gtk version was first
written. I don't think there's any need to keep it having dug into the code that
runs as we destroy these objects.

[Peter Kasting, 2012-07-30 17:38:10]

SGTM

[dominich, 2012-08-06 16:06:24]

PTAL - I don't intend to submit this before the next branch point, but I'd like
to get all my ducks in a row so I'm ready to drop it in immediately afterwards.

[Peter Kasting, 2012-08-07 02:47:05]

LGTM

http://codereview.chromium.org/10810062/diff/18029/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_view.cc (right):

http://codereview.chromium.org/10810062/diff/18029/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_view.cc:79: // Invalid URLs such as
chrome://history can end up here.
Oh?  I thought this didn't happen?  Did you find a repro case?

http://codereview.chromium.org/10810062/diff/18029/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_view.cc:82: // |model_| can be NULL in tests.
Nit: Instead of putting these comments in every function, put one comment on the
class members' declaration.

http://codereview.chromium.org/10810062/diff/18029/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_view.cc:95: return IsEditingOrEmpty() ?
Nit: Don't nest ?:s

http://codereview.chromium.org/10810062/diff/18029/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_view.cc:111: // TODO(deanm): something about
selection / focus change here.
Nit: I thought you were going to kill this comment?

http://codereview.chromium.org/10810062/diff/18029/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

http://codereview.chromium.org/10810062/diff/18029/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_view.h:144: virtual void
CloseAutocompletePopup();
Nit: If we're renaming this, it should be CloseOmniboxPopup().

[dominich, 2012-08-07 17:48:20]

Fixed nits.

I'll make sure try bots are happy and then commit.

http://codereview.chromium.org/10810062/diff/18029/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_view.cc (right):

http://codereview.chromium.org/10810062/diff/18029/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_view.cc:79: // Invalid URLs such as
chrome://history can end up here.
On 2012/08/07 02:47:05, Peter Kasting wrote:
> Oh?  I thought this didn't happen?  Did you find a repro case?

Yes - typing chrome://history :)

http://codereview.chromium.org/10810062/diff/18029/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_view.cc:82: // |model_| can be NULL in tests.
On 2012/08/07 02:47:05, Peter Kasting wrote:
> Nit: Instead of putting these comments in every function, put one comment on
the
> class members' declaration.

Done.

http://codereview.chromium.org/10810062/diff/18029/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_view.cc:95: return IsEditingOrEmpty() ?
On 2012/08/07 02:47:05, Peter Kasting wrote:
> Nit: Don't nest ?:s

Done.

http://codereview.chromium.org/10810062/diff/18029/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_view.cc:111: // TODO(deanm): something about
selection / focus change here.
On 2012/08/07 02:47:05, Peter Kasting wrote:
> Nit: I thought you were going to kill this comment?

Done.

http://codereview.chromium.org/10810062/diff/18029/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

http://codereview.chromium.org/10810062/diff/18029/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_view.h:144: virtual void
CloseAutocompletePopup();
On 2012/08/07 02:47:05, Peter Kasting wrote:
> Nit: If we're renaming this, it should be CloseOmniboxPopup().

Done.

[commit-bot: I haz the power, 2012-08-07 19:07:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominich@chromium.org/10810062/21023

[commit-bot: I haz the power, 2012-08-07 19:07:08]

Presubmit check for 10810062-21023 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/browser/automation

Presubmit checks took 2.0s to calculate.

[dominich, 2012-08-07 19:09:24]

ben@ Could you take a look for OWNERS approval for chrome/ and
chrome/browser/automation please?

[Ben Goodger (Google), 2012-08-07 19:40:20]

lgtm

[commit-bot: I haz the power, 2012-08-07 20:32:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominich@chromium.org/10810062/21023

[commit-bot: I haz the power, 2012-08-07 22:01:57]

Change committed as 150426