Dragging within the popup window of the omnibar didn't result in the desired ...

Dragging within the popup window of the omnibar didn't result in the desired 
behavior. Now, dragging moves the selection around and releasing the mouse
within the popup window activates the link. Middle mouse button still opens
a new tab. In addition, moving the mouse out of the popup now clears the
hovered selection.

BUG=13703
TEST=Type goog in the omnibar, drag the mouse around the popup window

[PhilBeaudoin, 2010-01-28 07:14:23]

As discussed by email.

[Peter Kasting, 2010-01-28 18:42:42]

Added myself as a reviewer since I'd like to look at this.  Won't get to it
until later today probably.

[PhilBeaudoin, 2010-01-28 18:43:52]

On 2010/01/28 18:42:42, Peter Kasting wrote:
> Added myself as a reviewer since I'd like to look at this.  Won't get to it
> until later today probably.

Great! Thanks I look forward to your comments.

[Peter Kasting, 2010-01-28 19:41:40]

Looks like a good start.

http://codereview.chromium.org/555145/diff/1/2
File AUTHORS (right):

http://codereview.chromium.org/555145/diff/1/2#newcode63
AUTHORS:63: Philippe Beaudoin <philippe.beaudoin@gmail.com>
Did you already sign the Committer License Agreement?

http://codereview.chromium.org/555145/diff/1/3
File chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc
(right):

http://codereview.chromium.org/555145/diff/1/3#newcode761
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc:761: void
AutocompletePopupContentsView::OnMouseExited(
Is it possible to get this while left-dragging, or does mouse capture prevent
it?  If it is possible, then perhaps we should set the selected line too, so
that if we go off the window edge the selection will disappear?

http://codereview.chromium.org/555145/diff/1/3#newcode801
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc:801: const
gfx::Point& /*point*/) {
Nit: We don't normally comment out unused arg names.

http://codereview.chromium.org/555145/diff/1/3#newcode803
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc:803: //
active view for any point inside of it
Nit: Period at end

http://codereview.chromium.org/555145/diff/1/3#newcode891
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc:891: //
Hovering selected line so that nothing is blurred
The need for this is not totally clear to me.  It seems like it's only needed
when index == kNoMatch, so that the hover marking disappears.  If that's true
then maybe we can just remove the conditional and pass this straight through to
the model, since I believe the model turns off hover appropriately when provided
kNoMatch?  And if _that's_ true, then we can get rid of this function entirely,
and just replace calls to it with calls straight to the model.

http://codereview.chromium.org/555145/diff/1/3#newcode897
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc:897: if
(HasMatchAt(index))
As above, do we need to check whether there's a match, or can we pass straight
through to the model (and thus eliminate this function)?

http://codereview.chromium.org/555145/diff/1/3#newcode905
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc:905: for
(int i = GetChildViewCount() - 1; i >= 0; --i) {
Nit: Any reason to do this as a decrementing loop?  It's a little unusual and
slightly more verbose.

http://codereview.chromium.org/555145/diff/1/4
File chrome/browser/views/autocomplete/autocomplete_popup_contents_view.h
(right):

http://codereview.chromium.org/555145/diff/1/4#newcode75
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.h:75: void
OnMouseEntered(const views::MouseEvent& event);
These are all View overrides, right?  Then it seems like they should be marked
virtual for clarity.

http://codereview.chromium.org/555145/diff/1/4#newcode82
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.h:82:
private:
Nit: blank line above this

http://codereview.chromium.org/555145/diff/1/4#newcode116
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.h:116: // in
window coordinate
Nit: "coordinate" -> "coordinates."  Also "GetIndexForPoint()" might be a tiny
bit better name (maybe).  Also either one or two words here would fit on the
previous line

http://codereview.chromium.org/555145/diff/1/4#newcode120
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.h:120: void
SetHoveredAtPoint(const gfx::Point& point);
Nit: It seems like we shouldn't need this or the helper below as it's just the
union of two function calls.  I'd prefer to keep the API surface low when these
don't buy us much.

[PhilBeaudoin, 2010-01-28 20:42:03]

I'll submit a new CL as soon as I have your answers to the two points not marked
"done" (unused parameters and decrementing loop).

http://codereview.chromium.org/555145/diff/1/2
File AUTHORS (right):

http://codereview.chromium.org/555145/diff/1/2#newcode63
AUTHORS:63: Philippe Beaudoin <philippe.beaudoin@gmail.com>
On 2010/01/28 19:41:40, Peter Kasting wrote:
> Did you already sign the Committer License Agreement?
Yes.

http://codereview.chromium.org/555145/diff/1/3
File chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc
(right):

http://codereview.chromium.org/555145/diff/1/3#newcode761
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc:761: void
AutocompletePopupContentsView::OnMouseExited(
On 2010/01/28 19:41:40, Peter Kasting wrote:
> Is it possible to get this while left-dragging, or does mouse capture prevent
> it?  If it is possible, then perhaps we should set the selected line too, so
> that if we go off the window edge the selection will disappear?

The model doesn't support "no selection". There always needs to be a selected
line that will be triggered when the user presses enter.

http://codereview.chromium.org/555145/diff/1/3#newcode801
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc:801: const
gfx::Point& /*point*/) {
On 2010/01/28 19:41:40, Peter Kasting wrote:
> Nit: We don't normally comment out unused arg names.

Got this from the Google style doc, I believe. Do you leave them out? Doesn't
leaving them in generate a warning?

http://codereview.chromium.org/555145/diff/1/3#newcode803
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc:803: //
active view for any point inside of it
On 2010/01/28 19:41:40, Peter Kasting wrote:
> Nit: Period at end

Done.

http://codereview.chromium.org/555145/diff/1/3#newcode891
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc:891: //
Hovering selected line so that nothing is blurred
On 2010/01/28 19:41:40, Peter Kasting wrote:
> The need for this is not totally clear to me.  It seems like it's only needed
> when index == kNoMatch, so that the hover marking disappears.  If that's true
> then maybe we can just remove the conditional and pass this straight through
to
> the model, since I believe the model turns off hover appropriately when
provided
> kNoMatch?  And if _that's_ true, then we can get rid of this function
entirely,
> and just replace calls to it with calls straight to the model.

Just looked at it and you are right. This code is cut&paste of the previous one,
so I assumed the HasMatchAt() was there to keep kNoMatch from propagating. The
model has uses a DCHECK() for array bounds. I think we can assume this is enough
and that the UI code wont generate out-of-bound indices. I tested it without the
HasMatchAt() and it seems to work.

Done.

http://codereview.chromium.org/555145/diff/1/3#newcode897
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc:897: if
(HasMatchAt(index))
On 2010/01/28 19:41:40, Peter Kasting wrote:
> As above, do we need to check whether there's a match, or can we pass straight
> through to the model (and thus eliminate this function)?

Same as above, although here the model uses a CHECK(). I removed it and it seems
to work.

Done.

http://codereview.chromium.org/555145/diff/1/3#newcode905
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc:905: for
(int i = GetChildViewCount() - 1; i >= 0; --i) {
On 2010/01/28 19:41:40, Peter Kasting wrote:
> Nit: Any reason to do this as a decrementing loop?  It's a little unusual and
> slightly more verbose.

Inspired from View::GetViewForPoint. It might be slightly more efficient since
GetChildViewCount() is called just once. I could place it in a local var, but it
would be even more verbose.

http://codereview.chromium.org/555145/diff/1/4
File chrome/browser/views/autocomplete/autocomplete_popup_contents_view.h
(right):

http://codereview.chromium.org/555145/diff/1/4#newcode75
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.h:75: void
OnMouseEntered(const views::MouseEvent& event);
On 2010/01/28 19:41:40, Peter Kasting wrote:
> These are all View overrides, right?  Then it seems like they should be marked
> virtual for clarity.

Done.

http://codereview.chromium.org/555145/diff/1/4#newcode82
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.h:82:
private:
On 2010/01/28 19:41:40, Peter Kasting wrote:
> Nit: blank line above this

Done.

http://codereview.chromium.org/555145/diff/1/4#newcode116
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.h:116: // in
window coordinate
On 2010/01/28 19:41:40, Peter Kasting wrote:
> Nit: "coordinate" -> "coordinates."  Also "GetIndexForPoint()" might be a tiny
> bit better name (maybe).  Also either one or two words here would fit on the
> previous line

Done.

http://codereview.chromium.org/555145/diff/1/4#newcode120
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.h:120: void
SetHoveredAtPoint(const gfx::Point& point);
On 2010/01/28 19:41:40, Peter Kasting wrote:
> Nit: It seems like we shouldn't need this or the helper below as it's just the
> union of two function calls.  I'd prefer to keep the API surface low when
these
> don't buy us much.

Ok. I first thought of using some caching to speed-up the index look-up when the
hovered or selected line doesn't change, but I ended up not doing it. I guess
I'll get rid of these methods then.

Done.

[Peter Kasting, 2010-01-28 21:03:35]

http://codereview.chromium.org/555145/diff/1/3
File chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc
(right):

http://codereview.chromium.org/555145/diff/1/3#newcode761
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc:761: void
AutocompletePopupContentsView::OnMouseExited(
On 2010/01/28 20:42:03, PhilBeaudoin wrote:
> On 2010/01/28 19:41:40, Peter Kasting wrote:
> > Is it possible to get this while left-dragging, or does mouse capture
prevent
> > it?  If it is possible, then perhaps we should set the selected line too, so
> > that if we go off the window edge the selection will disappear?
> 
> The model doesn't support "no selection". There always needs to be a selected
> line that will be triggered when the user presses enter.

You'd think I'd remember that since I designed it.  Sorry for the stupid
question :)

http://codereview.chromium.org/555145/diff/1/3#newcode801
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc:801: const
gfx::Point& /*point*/) {
On 2010/01/28 20:42:03, PhilBeaudoin wrote:
> On 2010/01/28 19:41:40, Peter Kasting wrote:
> > Nit: We don't normally comment out unused arg names.
> 
> Got this from the Google style doc, I believe. Do you leave them out? Doesn't
> leaving them in generate a warning?

You're right, in checking the style guide I see it recommends this.  Strange
that I've never seen us actually do it.  In any case, what you're doing is
right, don't change it.

http://codereview.chromium.org/555145/diff/1/3#newcode891
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc:891: //
Hovering selected line so that nothing is blurred
On 2010/01/28 20:42:03, PhilBeaudoin wrote:
> The
> model has uses a DCHECK() for array bounds. I think we can assume this is
enough
> and that the UI code wont generate out-of-bound indices.

That should be true as long as GetViewForPoint() does the right thing.

http://codereview.chromium.org/555145/diff/1/3#newcode897
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc:897: if
(HasMatchAt(index))
On 2010/01/28 20:42:03, PhilBeaudoin wrote:
> here the model uses a CHECK(). I removed it and it seems
> to work.

You removed what, the CHECK() or your conditional?  Hopefully the latter; we
want to keep the former in place.

http://codereview.chromium.org/555145/diff/1/3#newcode905
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc:905: for
(int i = GetChildViewCount() - 1; i >= 0; --i) {
On 2010/01/28 20:42:03, PhilBeaudoin wrote:
> On 2010/01/28 19:41:40, Peter Kasting wrote:
> > Nit: Any reason to do this as a decrementing loop?  It's a little unusual
and
> > slightly more verbose.
> 
> Inspired from View::GetViewForPoint. It might be slightly more efficient since
> GetChildViewCount() is called just once. I could place it in a local var, but
it
> would be even more verbose.

GetChildViewCount() is a trivial accessor (it's basically std::vector::size()),
so don't worry about calling it repeatedly.  It's sad that our Views code is
inconsistent with the rest of our codebase in that it doesn't mark cheap
functions with unix_hacker_style() names.

In fact, a decrementing loop is _less_ efficient here because once we reach the
max number of children we never remove them, we just hide them.  So the last
children are likely to be invisible and thus not the mouse target.

[PhilBeaudoin, 2010-01-28 22:20:49]

http://codereview.chromium.org/555145/diff/1/3
File chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc
(right):

http://codereview.chromium.org/555145/diff/1/3#newcode897
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc:897: if
(HasMatchAt(index))
On 2010/01/28 21:03:35, Peter Kasting wrote:
> On 2010/01/28 20:42:03, PhilBeaudoin wrote:
> > here the model uses a CHECK(). I removed it and it seems
> > to work.
> 
> You removed what, the CHECK() or your conditional?  Hopefully the latter; we
> want to keep the former in place.

Kept the CHECK, removed the conditional (Got rid of the method, in fact.)

http://codereview.chromium.org/555145/diff/1/3#newcode905
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc:905: for
(int i = GetChildViewCount() - 1; i >= 0; --i) {
On 2010/01/28 21:03:35, Peter Kasting wrote:
> On 2010/01/28 20:42:03, PhilBeaudoin wrote:
> > On 2010/01/28 19:41:40, Peter Kasting wrote:
> > > Nit: Any reason to do this as a decrementing loop?  It's a little unusual
> and
> > > slightly more verbose.
> > 
> > Inspired from View::GetViewForPoint. It might be slightly more efficient
since
> > GetChildViewCount() is called just once. I could place it in a local var,
but
> it
> > would be even more verbose.
> 
> GetChildViewCount() is a trivial accessor (it's basically
std::vector::size()),
> so don't worry about calling it repeatedly.  It's sad that our Views code is
> inconsistent with the rest of our codebase in that it doesn't mark cheap
> functions with unix_hacker_style() names.
> 
> In fact, a decrementing loop is _less_ efficient here because once we reach
the
> max number of children we never remove them, we just hide them.  So the last
> children are likely to be invisible and thus not the mouse target.

I agree with your argument.

Done.

Done.

[Peter Kasting, 2010-01-28 22:31:32]

LGTM with two nits.  Thanks for the nice work on this, it's been bugging me for
a while and I'm glad the solution turned out to involve such a small amount of
code.  Please do continue contributing :)

http://codereview.chromium.org/555145/diff/4002/6003
File chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc
(right):

http://codereview.chromium.org/555145/diff/4002/6003#newcode893
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc:893: int
nbMatch = model_->result().size();
Nit: We use unix_hacker style for variable names, and try to make the names
fairly readable.  I suggest calling this num_children.

http://codereview.chromium.org/555145/diff/4002/6004
File chrome/browser/views/autocomplete/autocomplete_popup_contents_view.h
(right):

http://codereview.chromium.org/555145/diff/4002/6004#newcode111
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.h:111: //
coordinates
Nit: Add period.

[PhilBeaudoin, 2010-01-28 22:36:29]

Thanks for your comments! I'll continue contributing for sure. I love the
experience.

Maybe we should wait for Ben's OK? After that, I'll need somebody to commit this
for me.

http://codereview.chromium.org/555145/diff/4002/6003
File chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc
(right):

http://codereview.chromium.org/555145/diff/4002/6003#newcode893
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc:893: int
nbMatch = model_->result().size();
On 2010/01/28 22:31:32, Peter Kasting wrote:
> Nit: We use unix_hacker style for variable names, and try to make the names
> fairly readable.  I suggest calling this num_children.

Done.

http://codereview.chromium.org/555145/diff/4002/6004
File chrome/browser/views/autocomplete/autocomplete_popup_contents_view.h
(right):

http://codereview.chromium.org/555145/diff/4002/6004#newcode111
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.h:111: //
coordinates
On 2010/01/28 22:31:32, Peter Kasting wrote:
> Nit: Add period.

Done.

[Peter Kasting, 2010-01-28 22:40:41]

On 2010/01/28 22:36:29, PhilBeaudoin wrote:
> Maybe we should wait for Ben's OK? After that, I'll need somebody to commit
this
> for me.

I think this is simple enough that I'm willing to go ahead and locally test +
commit it.  Ben and I have both worked on this code enough to be confident
reviewing it.

[Ben Goodger (Google), 2010-01-28 22:41:53]

Yes I took a peek last night at a high level and am happy with the
direction. Peter is a very thorough reviewer so if he is OK with this
I am.

-Ben

On Thu, Jan 28, 2010 at 2:40 PM,  <pkasting@chromium.org> wrote:
> On 2010/01/28 22:36:29, PhilBeaudoin wrote:
>>
>> Maybe we should wait for Ben's OK? After that, I'll need somebody to
>> commit
>
> this
>>
>> for me.
>
> I think this is simple enough that I'm willing to go ahead and locally test
> +
> commit it.  Ben and I have both worked on this code enough to be confident
> reviewing it.
>
> http://codereview.chromium.org/555145
>

[PhilBeaudoin, 2010-01-28 22:42:22]

On 2010/01/28 22:40:41, Peter Kasting wrote:
> On 2010/01/28 22:36:29, PhilBeaudoin wrote:
> > Maybe we should wait for Ben's OK? After that, I'll need somebody to commit
> this
> > for me.
> 
> I think this is simple enough that I'm willing to go ahead and locally test +
> commit it.  Ben and I have both worked on this code enough to be confident
> reviewing it.

Go ahead then. Thanks. I'll be around if any test fail. I'm trying to wrap my
head around the testing process and the waterfall.

[Peter Kasting, 2010-01-29 01:27:30]

Landed in r37477.

While testing this I found a small bug that I filed as issue 33460 .  (It wasn't
trivial enough for me to fix inline.)

[PhilBeaudoin, 2010-01-29 03:28:47]

On 2010/01/29 01:27:30, Peter Kasting wrote:
> Landed in r37477.
> 
> While testing this I found a small bug that I filed as issue 33460 .  (It
wasn't
> trivial enough for me to fix inline.)

I'll try to take care of this.