Right-clicking on the omnibox with nothing selected now automatically selects...

Right-clicking on the omnibox with nothing selected now automatically selects all the text.

If there is any text already selected then the selection will not change.

BUG=6873, 30134
TEST=Middle-click the omnibox, then try to click it (or any other UI element), and make sure the click has an effect. Select a portion of the URL and right-click on the omnibox. Notice that the selection does not change. Clear the selection and right-click on the omnibox and notice that the URL is automatically selected.

[weinjared, 2010-05-26 05:55:31]

Right-clicking on the omnibox with nothing selected now automatically selects
all the text. 

If there is any text already selected then the selection will not change. 

BUG=30134
TEST=Select a portion of the URL and right-click on the omnibox. Notice that the
selection does not change. Clear the selection and right-click on the omnibox
and notice that the URL is automatically selected.

[Peter Kasting, 2010-05-26 18:10:03]

http://codereview.chromium.org/2241003/diff/1/2
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/2241003/diff/1/2#newcode1168
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:1168: if
(GetSelectedText().empty())
It seems like this will select all when the bar already has focus but has no
selection.  We don't want to do that because then people can't use the context
menu to paste to insert/append.  We should only select all when the click is
giving us focus.

You may need to add WM_RBUTTONXXX handlers, and parallel parts of the
left-button-select-all code, to accomplish this.  (It'd be somewhat simpler
because you don't need to worry about triple-click or click-and-drag canceling
the Select All.  Probably do need to worry about cases where the user presses
multiple buttons, though.)

[Peter Kasting, 2010-05-26 18:10:59]

I reduced the reviewer list to just me; only use multiple reviewers if you
really need multiple people to check something, and in that case tell them what
you want each to look at.

[weinjared, 2010-05-28 01:34:49]

On 2010/05/26 18:10:03, Peter Kasting wrote:
> Probably do need to worry about cases where the user presses
> multiple buttons, though.)

I'm not sure I follow here. What are some key combinations with right-click that
can cause a non-default behavior?

[Peter Kasting, 2010-05-28 01:39:08]

On 2010/05/28 01:34:49, weinjared wrote:
> On 2010/05/26 18:10:03, Peter Kasting wrote:
> > Probably do need to worry about cases where the user presses
> > multiple buttons, though.)
> 
> I'm not sure I follow here. What are some key combinations with right-click
that
> can cause a non-default behavior?

Sorry, maybe I was too vague.  Multiple _simultaneous_ buttons.  So e.g. LDOWN
RDOWN LUP RUP, LDOWN RDOWN RUP LUP, etc., especially if there is mouse movement
and/or typing at various points during those.

[weinjared, 2010-06-10 00:43:18]

I just wanted to let you know that I have not forgotten about this bug  and am
working on fixing the different cases for it.

[weinjared, 2010-06-13 00:45:16]

Sorry for the multiple submissions of the same patch.

I fell in to the same trap as I did on a different submission where I thought
the gcl tool didn't apply the patch because it asks for authentication which
fails, although the patch is still submitted.

[Peter Kasting, 2010-06-14 18:56:59]

http://codereview.chromium.org/2241003/diff/35001/36001
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/2241003/diff/35001/36001#newcode1485
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:1485: // later call
OnLButtonDblClk().  Make sure |gaining_focus_| gets reset both
Do we need to worry about this scenario for the R button as well, and add a
double-click handler?

http://codereview.chromium.org/2241003/diff/35001/36001#newcode1667
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:1667: void
AutocompleteEditViewWin::OnRButtonDown(UINT keys, const CPoint& point) {
Is it possible to combine the guts of these functions with the ones for the L
button, and simply tell the function whether this is an L or R click?

http://codereview.chromium.org/2241003/diff/35001/36002
File chrome/browser/autocomplete/autocomplete_edit_view_win.h (right):

http://codereview.chromium.org/2241003/diff/35001/36002#newcode173
chrome/browser/autocomplete/autocomplete_edit_view_win.h:173:
MSG_WM_MBUTTONDOWN(OnNonLButtonDown)
Nit: Probably should rename this OnMButtonXXX() now.

http://codereview.chromium.org/2241003/diff/35001/36002#newcode421
chrome/browser/autocomplete/autocomplete_edit_view_win.h:421: bool
tracking_click_;
Nit: For clarity, say "_left_" explicitly on these (and fix the comments to be
generic).

[weinjared, 2010-06-15 04:59:50]

Thanks for the comments. I've made the changes you requested (and I got to find
out about WM_RBUTTONDBLCLK). Good catch :)

I'm curious as to what you think about the MouseButton enum and the three
methods (GetMouseButtonState, SelectAllIfNecessary, and TrackMousePosition). I
looked around the codebase and saw other specific enumerated types and methods,
but I just wanted to make sure I was staying within the general style of
Chromium code.

[Peter Kasting, 2010-06-15 18:22:48]

http://codereview.chromium.org/2241003/diff/38001/39001
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/2241003/diff/38001/39001#newcode1457
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:1457: // call this for
M or R button down.
Nit: This comment no longer makes a ton of sense now that we have explicit
M-vs.-R functions.  I'd just remove it.

http://codereview.chromium.org/2241003/diff/38001/39001#newcode1460
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:1460:
OnPossibleDrag(point);
I think this call was a mistake before.  I think you should remove all the
drag-related calls from the M and R handlers.

http://codereview.chromium.org/2241003/diff/38001/39001#newcode1462
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:1462:
SetMsgHandled(false);
I bet if you remove SetMsgHandled() here and just below, you fix crbug.com/6873
.  You should check.  (You may need to add a middle-double-click handler and
ignore that message too.)

http://codereview.chromium.org/2241003/diff/38001/39001#newcode1481
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:1481: // rather than
in OnLButtonDown() since in many scenarios OnSetFocus() will be
Nit: OnXButtonDown()

http://codereview.chromium.org/2241003/diff/38001/39001#newcode1493
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:1493: // later call
OnLButtonDblClk().  Make sure |gaining_focus_| gets reset both
Nit: OnXButtonDblClk()

http://codereview.chromium.org/2241003/diff/38001/39001#newcode1658
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:1658:
gaining_focus_.reset();  // See NOTE in OnMouseActivate().
Also need SetMsgHandled(false);

http://codereview.chromium.org/2241003/diff/38001/39001#newcode1668
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:1668: // call this for
M or R button down.
Nit: As above, I'd remove this comment.

http://codereview.chromium.org/2241003/diff/38001/39001#newcode2422
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:2422: void
AutocompleteEditViewWin::UpdateDragDone(UINT keys) {
I bet if you remove the drag calls from the M and R handlers, this function can
disappear, and we can just clear |possible_drag_| in OnLButtonUp().

http://codereview.chromium.org/2241003/diff/38001/39001#newcode2463
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:2463: void
AutocompleteEditViewWin::GetMouseButtonState(MouseButton button,
Now that I see how little code is apparently common between the L and R
functions, I wonder if this factoring out of common code really buys much. 
Maybe we should just drop it.

At the very least, it seems like GetMouseButtonState() isn't necessary: if you
change the L/R member variables to be two-item arrays, and make the "left" and
"right" enum values 0 and 1, you can index the arrays directly via the passed in
enum values.

But it may just be clearer to duplicate the below code in the L and R functions
and be done with it.

http://codereview.chromium.org/2241003/diff/38001/39002
File chrome/browser/autocomplete/autocomplete_edit_view_win.h (right):

http://codereview.chromium.org/2241003/diff/38001/39002#newcode380
chrome/browser/autocomplete/autocomplete_edit_view_win.h:380: enum MouseButton {
Nit: enums go at the top of the relevant (public/private) section.

http://codereview.chromium.org/2241003/diff/38001/39002#newcode381
chrome/browser/autocomplete/autocomplete_edit_view_win.h:381: kLeft  = 1 << 0,
Nit: This implies that this is a bitmask.  Since it's not, just leave off the
explicit values.  (Or change to 0 and 1; see comments in .cc file.)

http://codereview.chromium.org/2241003/diff/38001/39002#newcode434
chrome/browser/autocomplete/autocomplete_edit_view_win.h:434: CPoint
left_click_mouse_down_point_;
Nit: |XXX_click_point_| would probably be just as good, and less verbose.

[weinjared, 2010-06-16 03:54:34]

I've made the changes requested. I also included the changes necessary to fix
bug 6873 since they were so minimal.

In a couple places there were unreferenced arguments to functions. I've
commented out the name of the variable but didn't see that done elsewhere in the
code nor a mention in the C++ style guide. Is this the right/wrong thing to do?

[Peter Kasting, 2010-06-16 18:39:56]

Commenting out unused args is preferred (see the details inside
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...
), it's just that MSVC doesn't warn if you don't do it, so in most Windows code
we haven't noticed and bothered.

LGTM!  If you fix the nits let me know and I'll land this for you.

http://codereview.chromium.org/2241003/diff/36003/43001
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/2241003/diff/36003/43001#newcode415
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:415:
tracking_click_[kLeft] = false;
Nit: Isn't it possible to init these in the initializer list above?

http://codereview.chromium.org/2241003/diff/36003/43001#newcode1462
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:1462: // Specifically
do not call SetMsgHandled here because the mouse is captured
Nit: Thanks for the comment!  You can probably be even clearer, and avoid
referencing a bug number, with something like:

By default, the edit responds to middle-clicks by capturing the mouse and
ignoring all subsequent events until it receives another click (of any of the
left, middle, or right buttons).  This bizarre behavior is not only useless but
can cause the UI to appear unresponsive if a user accidentally middle-clicks the
edit (instead of a tab above it), so we purposefully eat this message (instead
of calling SetMsgHandled(false)) to avoid triggering this.

You also should probably put this inside the OnMButtonDblClk() handler and then
add "See note in OnMButtonDblClk() above" comments in the other two functions.

http://codereview.chromium.org/2241003/diff/36003/43001#newcode1672
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:1672:
gaining_focus_.reset();
Nit: For consistency with OnLButtonDown(), I'd move this below the next two
statements.  You can probably also leave out the newlines between them.

http://codereview.chromium.org/2241003/diff/36003/43002
File chrome/browser/autocomplete/autocomplete_edit_view_win.h (right):

http://codereview.chromium.org/2241003/diff/36003/43002#newcode206
chrome/browser/autocomplete/autocomplete_edit_view_win.h:206: };
Nit: Blank line after this

http://codereview.chromium.org/2241003/diff/36003/43002#newcode424
chrome/browser/autocomplete/autocomplete_edit_view_win.h:424: // select all the
text in the edit.  During this process, tracking_XXX_click_
Nit: Update comments now that variable names have changed again.  You should say
what the array is about.

[weinjared, 2010-06-16 19:47:25]

On 2010/06/16 18:39:56, Peter Kasting wrote:
> Commenting out unused args is preferred (see the details inside
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...
> ), it's just that MSVC doesn't warn if you don't do it, so in most Windows
code
> we haven't noticed and bothered.

Turning on Level 4 warnings will call out unreferenced arguments. Specifically
it will produce a C4100 warning in MSVC.

[Peter Kasting, 2010-06-16 21:52:37]

On 2010/06/16 19:47:25, weinjared wrote:
> Turning on Level 4 warnings will call out unreferenced arguments. Specifically
> it will produce a C4100 warning in MSVC.

I filed crbug.com/46721 to turn this one on.

[weinjared, 2010-06-16 23:32:10]

http://codereview.chromium.org/2241003/diff/36003/43001
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/2241003/diff/36003/43001#newcode415
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:415:
tracking_click_[kLeft] = false;
On 2010/06/16 18:39:56, Peter Kasting wrote:
> Nit: Isn't it possible to init these in the initializer list above?

Yes, we can use |tracking_click_()| in the initializer list, but it will
generate warning C4351. This warning is innocuous because it is only present to
state that the behavior of this use has changed from VC8 to VC9. Microsoft's
solution is to disable this warning. Should we disable this warning or just use
assignment? 

Please see http://msdn.microsoft.com/en-us/library/1ywe7hcy(VS.80).aspx

[Peter Kasting, 2010-06-17 00:22:13]

http://codereview.chromium.org/2241003/diff/36003/43001
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/2241003/diff/36003/43001#newcode415
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:415:
tracking_click_[kLeft] = false;
On 2010/06/16 23:32:11, weinjared wrote:
> On 2010/06/16 18:39:56, Peter Kasting wrote:
> > Nit: Isn't it possible to init these in the initializer list above?
> 
> Yes, we can use |tracking_click_()| in the initializer list, but it will
> generate warning C4351. This warning is innocuous because it is only present
to
> state that the behavior of this use has changed from VC8 to VC9. Microsoft's
> solution is to disable this warning. Should we disable this warning or just
use
> assignment?

We should probably just globally disable the warning.  In src/build/common.gypi,
look for "msvs_disabled_warnings" and add your warning number, then do "gclient
runhooks" (with MSVC closed), load the solution back up and try compiling.

[weinjared, 2010-06-17 05:02:22]

I've made the changes requested.

I found three locations of "msvs_disabled_warnings" within common.gypi and
placed the warning number in each spot. I'm not familiar enough with the GYP
format to know if I was too aggressive in disabling the warning. Please let me
know if I should change it.

I updated the comment on autocomplete_edit_view_win.h:423 to explain the arrays.
Let me know if the explanation is not clear enough.

I wanted to say thanks for pushing me on the array initialization. I didn't
think it was possible to default-init an array but with a little research I
figured it out.

[weinjared, 2010-06-17 05:04:19]

http://codereview.chromium.org/2241003/diff/47001/19002
File build/common.gypi (right):

http://codereview.chromium.org/2241003/diff/47001/19002#newcode1242
build/common.gypi:1242: 'msvs_disabled_warnings': [4396, 4503, 4819, 4351],
In hindsight, I probably should have kept these in ascending order. I can
reorder these if you feel it is necessary.

[Peter Kasting, 2010-06-17 18:51:14]

http://codereview.chromium.org/2241003/diff/47001/19002
File build/common.gypi (right):

http://codereview.chromium.org/2241003/diff/47001/19002#newcode517
build/common.gypi:517: 'msvs_disabled_warnings': [4800, 4351],
I _think_ you don't need to modify this but I'm not sure.

http://codereview.chromium.org/2241003/diff/47001/19002#newcode724
build/common.gypi:724: 'msvs_disabled_warnings': [4275, 4351],
I don't think you need to modify this.

http://codereview.chromium.org/2241003/diff/47001/19002#newcode1242
build/common.gypi:1242: 'msvs_disabled_warnings': [4396, 4503, 4819, 4351],
On 2010/06/17 05:04:20, weinjared wrote:
> In hindsight, I probably should have kept these in ascending order. I can
> reorder these if you feel it is necessary.

Yeah, I'd keep them in order.

[Peter Kasting, 2010-06-17 18:52:10]

I added bradnelson as a reviewer.  Brad, can you look at the .gypi change here? 
The goal is to globally disable a warning for MSVC.  It's not clear to me which
places need modification.

[bradn, 2010-06-17 22:35:16]

http://codereview.chromium.org/2241003/diff/47001/19002
File build/common.gypi (right):

http://codereview.chromium.org/2241003/diff/47001/19002#newcode517
build/common.gypi:517: 'msvs_disabled_warnings': [4800, 4351],
Yeah, drop this. This would be if you wanted it in all non-chromium code.

http://codereview.chromium.org/2241003/diff/47001/19002#newcode724
build/common.gypi:724: 'msvs_disabled_warnings': [4275, 4351],
This too is very specific. Drop this one.

http://codereview.chromium.org/2241003/diff/47001/19002#newcode1242
build/common.gypi:1242: 'msvs_disabled_warnings': [4396, 4503, 4819, 4351],
Just here should be good (this puts it pretty much everywhere). And yeah
ascending order would be nice.

[weinjared, 2010-06-21 21:03:24]

I've made the respective changes.

[Peter Kasting, 2010-06-22 01:29:19]

LGTM.  I'll land this.

[weinjared, 2010-06-22 01:39:24]

On 2010/06/22 01:29:19, Peter Kasting wrote:
> LGTM.  I'll land this.

Thanks!

[Peter Kasting, 2010-06-22 02:02:32]

Landed in r50420.