In chromium, context menu is displayed when right mouse button is released....

In chromium, a context menu is displayed when right mouse button is 'released'.
(e.g. context menu for a tab, etc)

However, unlike other cases, a context menu for an app (the context menu that is displayed on the title bar) is created when right mouse button is 'pressed'.

So, for a consistency, modify it so that the context menu for an app is displayed not on mouse-press but on mouse-release like other cases.

* Above is also described on the 'Comment 11 by sy3620, Jan 08, 2009' in Issue 5695. This is not about fixing the bug, but suggesting about changing the way the context menu for an app is displayed.

http://crbug.com/5695

[Finnur, 2009-01-15 17:40:15]

I don't think you need two people to review this code and since I am less
familiar with it than Ben, I think I'll leave the decision up to him.

[Ben Goodger (Google), 2009-01-15 19:36:59]

http://codereview.chromium.org/18095/diff/1/2
File chrome/views/window.cc (right):

http://codereview.chromium.org/18095/diff/1/2#newcode426
Line 426: RunSystemMenu(cursor_point);
why can't you just do this in OnNCRButtonUp?

[DeArto20, 2009-01-15 21:32:57]

Basically, when the user releases the right mouse button on the title bar,
WM_NCRBUTTONUP is not generated immediately. There is a short delay between
WM_NCRBUTTONDOWN and WM_NCRBUTTONUP messages. (In some cases, it takes more than
a second.)

But if we call SetCapture() in the OnNCRButtonDown(), there is not any delay
between them (Down and Up messages). But in this case, WM_NCRBUTTONUP is not
generated. Instead, WM_RBUTTONUP is generated. (Until we call ReleaseCapture()).

Actually, when the context menu on title bar is displayed in other programs
(i.e. IE, etc), it seems that it is processed not on OnNCRButtonUp but on
OnRButtonUp(). (Simply checked with spy++)

On 2009/01/15 19:36:59, Ben Goodger wrote:
> http://codereview.chromium.org/18095/diff/1/2
> File chrome/views/window.cc (right):
> 
> http://codereview.chromium.org/18095/diff/1/2#newcode426
> Line 426: RunSystemMenu(cursor_point);
> why can't you just do this in OnNCRButtonUp?

[Peter Kasting, 2009-01-15 21:36:07]

On 2009/01/15 21:32:57, DeArto20 wrote:
> Basically, when the user releases the right mouse button on the title bar,
> WM_NCRBUTTONUP is not generated immediately. There is a short delay between
> WM_NCRBUTTONDOWN and WM_NCRBUTTONUP messages. (In some cases, it takes more
than
> a second.)
> 
> But if we call SetCapture() in the OnNCRButtonDown(), there is not any delay
> between them (Down and Up messages).

Using SetCapture() this way is a hack.  You shouldn't do this.  You should only
SetCapture if you want mouse capture behavior, which you don't (if I click down
and drag my mouse away and click up the release should not be sent to the
original element, as opposed to, say, scrollbars).

[DeArto20, 2009-01-16 05:25:03]

Are there any problems in using SetCapture() and ReleaseCapture() in this way?

Plz check following two cases.

[1] Wine - Open Source Project (http://www.winehq.org)
The 'WINE' project uses the same way for displaying context menu on title bar.

http://wine.sourcearchive.com/documentation/1.1.5-1/defwnd_8c-source.html
(see the lines from 333 to 386)

In the code, SetCapture() is called in OnNCRButtonDown() and ReleaseCapture() is
called in OnRButtonUp() for displaying context menu. The only difference between
this code and above patch is that the patch doesn't use WM_CONTEXTMENU message
and directly call RunSystemMenu() to display the context menu.

[2] MS Windows Program - Default Behavior (DefWindowProc)
And it seems that the 'default behavior' of windows program (which has a title
bar) is very similar to this (simply checked with spy++).
When the right mouse button is pressed on the title bar, WM_NCRBUTTONDOWN is
generated and the window captures the mouse (we can know this since the mouse
input is directed to the window at this time). And then, if the button is
released, WM_RBUTTONUP is generated instead of WM_NCRBUTTONUP and the mouse
capture is released (we can know this by the generation of WM_CAPTURECHANGED at
this time).


On 2009/01/15 21:36:07, pkasting wrote:
> On 2009/01/15 21:32:57, DeArto20 wrote:
> > Basically, when the user releases the right mouse button on the title bar,
> > WM_NCRBUTTONUP is not generated immediately. There is a short delay between
> > WM_NCRBUTTONDOWN and WM_NCRBUTTONUP messages. (In some cases, it takes more
> than
> > a second.)
> > 
> > But if we call SetCapture() in the OnNCRButtonDown(), there is not any delay
> > between them (Down and Up messages).
> 
> Using SetCapture() this way is a hack.  You shouldn't do this.  You should
only
> SetCapture if you want mouse capture behavior, which you don't (if I click
down
> and drag my mouse away and click up the release should not be sent to the
> original element, as opposed to, say, scrollbars).

[Peter Kasting, 2009-01-16 17:54:34]

On 2009/01/16 05:25:03, DeArto20 wrote:
> [1] Wine - Open Source Project (http://www.winehq.org)
> The 'WINE' project uses the same way for displaying context menu on title bar.
> 
> http://wine.sourcearchive.com/documentation/1.1.5-1/defwnd_8c-source.html
> (see the lines from 333 to 386)
> 
> [2] MS Windows Program - Default Behavior (DefWindowProc)
> And it seems that the 'default behavior' of windows program (which has a title
> bar) is very similar to this (simply checked with spy++).
> When the right mouse button is pressed on the title bar, WM_NCRBUTTONDOWN is
> generated and the window captures the mouse (we can know this since the mouse
> input is directed to the window at this time). And then, if the button is
> released, WM_RBUTTONUP is generated instead of WM_NCRBUTTONUP and the mouse
> capture is released (we can know this by the generation of WM_CAPTURECHANGED
at
> this time).

That seems incredibly weird, but if that's what Windows does natively, then
please add appropriate comments to this effect, e.g.:

// Using SetCapture() here matches Windows native behavior for right-clicks on
the titlebar.  It's not obvious why Windows does this.

I still wonder whether we should copy this.  Does it really buy us much, other
than confusing code?

[Peter Kasting, 2009-01-16 17:57:54]

http://codereview.chromium.org/18095/diff/1/2
File chrome/views/window.cc (right):

http://codereview.chromium.org/18095/diff/1/2#newcode400
Line 400: is_right_mouse_pressed_on_caption_ = true;
Nit: Don't add a blank line after this line.

http://codereview.chromium.org/18095/diff/1/2#newcode409
Line 409: if (is_right_mouse_pressed_on_caption_ == true) {
Nit: Don't add useless "== true" here.  Don't use {}.

http://codereview.chromium.org/18095/diff/1/2#newcode417
Line 417: POINT cursor_point = {point.x, point.y};
Nit: Don't add blank lines after every single line here.

http://codereview.chromium.org/18095/diff/1/2#newcode419
Line 419: if (is_right_mouse_pressed_on_caption_ == true) {
Nit: Don't add useless "== true" here.

http://codereview.chromium.org/18095/diff/1/2#newcode429
Line 429: WidgetWin::OnRButtonUp(ht_component, point);
Should this be in an } else { clause?

http://codereview.chromium.org/18095/diff/1/3
File chrome/views/window.h (right):

http://codereview.chromium.org/18095/diff/1/3#newcode260
Line 260: // Whether the right mouse button is pressed on caption.
This comment doesn't say anything more than the variable name.  Instead write
about what we're trying to accomplish.

[DeArto20, 2009-01-18 06:20:39]

http://codereview.chromium.org/18095/diff/1/2
File chrome/views/window.cc (right):

http://codereview.chromium.org/18095/diff/1/2#newcode400
Line 400: is_right_mouse_pressed_on_caption_ = true;
On 2009/01/16 17:57:55, pkasting wrote:
> Nit: Don't add a blank line after this line.

Done.

http://codereview.chromium.org/18095/diff/1/2#newcode409
Line 409: if (is_right_mouse_pressed_on_caption_ == true) {
On 2009/01/16 17:57:55, pkasting wrote:
> Nit: Don't add useless "== true" here.  Don't use {}.

Done.

http://codereview.chromium.org/18095/diff/1/2#newcode417
Line 417: POINT cursor_point = {point.x, point.y};
On 2009/01/16 17:57:55, pkasting wrote:
> Nit: Don't add blank lines after every single line here.

Done.

http://codereview.chromium.org/18095/diff/1/2#newcode419
Line 419: if (is_right_mouse_pressed_on_caption_ == true) {
On 2009/01/16 17:57:55, pkasting wrote:
> Nit: Don't add useless "== true" here.

Done.

http://codereview.chromium.org/18095/diff/1/2#newcode426
Line 426: RunSystemMenu(cursor_point);
On 2009/01/15 19:37:00, Ben Goodger wrote:
> why can't you just do this in OnNCRButtonUp?

When SetCapture is used, WM_RBUTTONUP is generated instead of WM_NCRBUTTONUP.

http://codereview.chromium.org/18095/diff/1/2#newcode429
Line 429: WidgetWin::OnRButtonUp(ht_component, point);
On 2009/01/16 17:57:55, pkasting wrote:
> Should this be in an } else { clause?

Done.

http://codereview.chromium.org/18095/diff/1/3
File chrome/views/window.h (right):

http://codereview.chromium.org/18095/diff/1/3#newcode260
Line 260: // Whether the right mouse button is pressed on caption.
On 2009/01/16 17:57:55, pkasting wrote:
> This comment doesn't say anything more than the variable name.  Instead write
> about what we're trying to accomplish.

Done.

[DeArto20, 2009-01-19 13:09:19]

Besides the Windows native behavior, 'SetCapture() & ReleaseCapture()' method is
already used in other cases in chrome browser.


1. Displaying context menu on a 'Tab' or on a 'Bookmark'

On right mouse press:
- SetCapture() is called in WidgetWin::ProcessMousePressed()

On right mouse release:
- ReleaseCapture() is called in WidgetWin::ProcessMouseReleased()


2. Displaying context menu on a 'Web page'

On right mouse press:
- SetCapture() is called in
RenderWidgetHostViewWin::ForwardMouseEventToRenderer()

On right mouse release:
- ReleaseCapture() is called in
RenderWidgetHostViewWin::ForwardMouseEventToRenderer()


So, I think it is natural that we use the 'SetCapture() & ReleaseCapture()'
method on displyaing context menu.

[rtanabe999, 2009-02-12 08:31:54]

handle WM_CONTEXTMENU ?

[DeArto20, 2009-02-13 02:05:28]

The context menu on the title bar is displayed not on processing WM_CONTEXTMENU
but on processing WM_RBUTTONUP.

On 2009/02/12 08:31:54, rtanabe999 wrote:
> handle WM_CONTEXTMENU ?

[DeArto20, 2009-02-15 17:46:29]

Code Modifications:
I have modified all the points that pkasting has indicated.

The Way It Works:
'SetCapture-ReleaseCapture' Method is already being used in other cases in the
chromium project as I posted in the prior message on this issue.

Plz finish this issue. (Additional review or commit)

[DeArto20, 2009-03-22 11:49:45]

Do you need more time to finish this issue? It's about one month after I
modified the code that has been indicated by reviewer.

Plz finish this issue asap.

On 2009/02/15 17:46:29, DeArto20 wrote:
> Code Modifications:
> I have modified all the points that pkasting has indicated.
> 
> The Way It Works:
> 'SetCapture-ReleaseCapture' Method is already being used in other cases in the
> chromium project as I posted in the prior message on this issue.
> 
> Plz finish this issue. (Additional review or commit)

[Peter Kasting, 2009-03-23 01:04:06]

http://codereview.chromium.org/18095/diff/12/205
File chrome/views/window.cc (right):

http://codereview.chromium.org/18095/diff/12/205#newcode410
Line 410: if (is_right_mouse_pressed_on_caption_) {
Nit: no {}

http://codereview.chromium.org/18095/diff/12/205#newcode417
Line 417: void Window::OnRButtonUp(UINT ht_component, const CPoint& point) {
Is this backwards?  Wouldn't we want to run the menu in the NC hit handler, and
just reset in this one?  Or am I totally confused?

http://codereview.chromium.org/18095/diff/12/205#newcode422
Line 422: ClientToScreen(GetHWND(), &cursor_point);
Nit: Use MapWindowPoints() instead of ClientToScreen() or ScreenToClient()

http://codereview.chromium.org/18095/diff/12/205#newcode425
Line 425: WidgetWin::OnRButtonUp(ht_component, point);
Tiny nit: Maybe "return;" inside the conditional and then an unconditional call
to this is better?  I dunno.

http://codereview.chromium.org/18095/diff/12/206
File chrome/views/window.h (right):

http://codereview.chromium.org/18095/diff/12/206#newcode260
Line 260: // Used on displaying context menu on caption. Context menu is
displayed only
Nit: "on" -> "when"

You might want a better comment here about what we're doing.  For example, "Set
to true when the user presses the right mouse button on the caption area.  We
need this so we can correctly show the context menu on mouse-up."

[DeArto20, 2009-03-23 15:20:16]

http://codereview.chromium.org/18095/diff/12/205
File chrome/views/window.cc (right):

http://codereview.chromium.org/18095/diff/12/205#newcode410
Line 410: if (is_right_mouse_pressed_on_caption_) {
On 2009/03/23 01:04:06, pkasting wrote:
> Nit: no {}

Done.

http://codereview.chromium.org/18095/diff/12/205#newcode417
Line 417: void Window::OnRButtonUp(UINT ht_component, const CPoint& point) {
When the right mouse button is pressed on the title bar, WM_NCRBUTTONDOWN is
generated and the window captures the mouse (we can know this since the mouse
input is directed to the window at this time). And then, if the button is
released, WM_RBUTTONUP is generated instead of WM_NCRBUTTONUP and the mouse
capture is released (we can verify this by the generation of WM_CAPTURECHANGED
at this time).

So, we should handle this NOT on OnNCRButtonUp() BUT on OnRButtonUp().

Above is the default windows behavior. You can check this on other windows
programs (Windows Explorer, Internet Explorer, etc) simply using spy++.

On 2009/03/23 01:04:06, pkasting wrote:
> Is this backwards?  Wouldn't we want to run the menu in the NC hit handler,
and
> just reset in this one?  Or am I totally confused?

http://codereview.chromium.org/18095/diff/12/205#newcode422
Line 422: ClientToScreen(GetHWND(), &cursor_point);
On 2009/03/23 01:04:06, pkasting wrote:
> Nit: Use MapWindowPoints() instead of ClientToScreen() or ScreenToClient()

Done.

http://codereview.chromium.org/18095/diff/12/205#newcode425
Line 425: WidgetWin::OnRButtonUp(ht_component, point);
On 2009/03/23 01:04:06, pkasting wrote:
> Tiny nit: Maybe "return;" inside the conditional and then an unconditional
call
> to this is better?  I dunno.

Done.

http://codereview.chromium.org/18095/diff/12/206
File chrome/views/window.h (right):

http://codereview.chromium.org/18095/diff/12/206#newcode260
Line 260: // Used on displaying context menu on caption. Context menu is
displayed only
On 2009/03/23 01:04:06, pkasting wrote:
> Nit: "on" -> "when"
> 
> You might want a better comment here about what we're doing.  For example,
"Set
> to true when the user presses the right mouse button on the caption area.  We
> need this so we can correctly show the context menu on mouse-up."

Done.

[Peter Kasting, 2009-03-23 16:29:30]

http://codereview.chromium.org/18095/diff/12/205
File chrome/views/window.cc (right):

http://codereview.chromium.org/18095/diff/12/205#newcode417
Line 417: void Window::OnRButtonUp(UINT ht_component, const CPoint& point) {
On 2009/03/23 15:20:16, DeArto20 wrote:
> if the button is
> released, WM_RBUTTONUP is generated instead of WM_NCRBUTTONUP

You should add comments somewhere (perhaps here) to this effect.

http://codereview.chromium.org/18095/diff/12/205#newcode425
Line 425: WidgetWin::OnRButtonUp(ht_component, point);
On 2009/03/23 15:20:16, DeArto20 wrote:
> On 2009/03/23 01:04:06, pkasting wrote:
> > Tiny nit: Maybe "return;" inside the conditional and then an unconditional
> call
> > to this is better?  I dunno.
> 
> Done.

You didn't add "return;".  We don't want to run the default OnRButtonUp() after
returning from the system menu.

[DeArto20, 2009-03-23 22:45:47]

http://codereview.chromium.org/18095/diff/12/205
File chrome/views/window.cc (right):

http://codereview.chromium.org/18095/diff/12/205#newcode417
Line 417: void Window::OnRButtonUp(UINT ht_component, const CPoint& point) {
On 2009/03/23 16:29:30, pkasting wrote:
> On 2009/03/23 15:20:16, DeArto20 wrote:
> > if the button is
> > released, WM_RBUTTONUP is generated instead of WM_NCRBUTTONUP
> 
> You should add comments somewhere (perhaps here) to this effect.

Done.

http://codereview.chromium.org/18095/diff/12/205#newcode425
Line 425: WidgetWin::OnRButtonUp(ht_component, point);
On 2009/03/23 16:29:30, pkasting wrote:
> On 2009/03/23 15:20:16, DeArto20 wrote:
> > On 2009/03/23 01:04:06, pkasting wrote:
> > > Tiny nit: Maybe "return;" inside the conditional and then an unconditional
> > call
> > > to this is better?  I dunno.
> > 
> > Done.
> 
> You didn't add "return;".  We don't want to run the default OnRButtonUp()
after
> returning from the system menu.

Done.

[Peter Kasting, 2009-03-23 23:10:39]

LGTM with nits

http://codereview.chromium.org/18095/diff/4401/3402
File chrome/views/window.cc (right):

http://codereview.chromium.org/18095/diff/4401/3402#newcode434
Line 434: // When the right mouse button is pressed on the title bar,
WM_NCRBUTTONDOWN
Perhaps this comment would be clearer as:

"We handle running the system menu on mouseup here because calling SetCapture()
on mousedown makes the mouseup generate WM_RBUTTONUP instead of WM_NCRBUTTONUP."

NOTE: This assumes it's SetCapture() that causes this... if the cause is
different, modify accordingly.

http://codereview.chromium.org/18095/diff/4401/3402#newcode445
Line 445: } else
Nit: No "else"

[DeArto20, 2009-03-24 12:29:27]

http://codereview.chromium.org/18095/diff/4401/3402
File chrome/views/window.cc (right):

http://codereview.chromium.org/18095/diff/4401/3402#newcode434
Line 434: // When the right mouse button is pressed on the title bar,
WM_NCRBUTTONDOWN
On 2009/03/23 23:10:39, pkasting wrote:
> Perhaps this comment would be clearer as:
> 
> "We handle running the system menu on mouseup here because calling
SetCapture()
> on mousedown makes the mouseup generate WM_RBUTTONUP instead of
WM_NCRBUTTONUP."
> 
> NOTE: This assumes it's SetCapture() that causes this... if the cause is
> different, modify accordingly.

Done.

http://codereview.chromium.org/18095/diff/4401/3402#newcode445
Line 445: } else
On 2009/03/23 23:10:39, pkasting wrote:
> Nit: No "else"

Done.

[DeArto20, 2009-03-26 16:55:29]

I have modified all that you indicated.

So, If there are no more problems, please commit this patch to the server for
me. (I don't have the access right)

[Peter Kasting, 2009-03-26 17:24:05]

On 2009/03/26 16:55:29, DeArto20 wrote:
> I have modified all that you indicated.
> 
> So, If there are no more problems, please commit this patch to the server for
> me. (I don't have the access right)

Sorry for the delay, I was out for a couple of days.

Your patch is bitrotted because you seemingly haven't updated your checkout for
a couple of weeks.  The files in question have moved and the code has also
changed some so that your patches no longer apply.  Can you update to the latest
source and upload a new snapshot that applies against the current tree?  I
should be able to commit after that.

[DeArto20, 2009-03-28 03:19:48]

I have updated to the latest source and made new patch.
Check the new patch and commit it for me. Thanks.

On 2009/03/26 17:24:05, pkasting wrote:
> On 2009/03/26 16:55:29, DeArto20 wrote:
> > I have modified all that you indicated.
> > 
> > So, If there are no more problems, please commit this patch to the server
for
> > me. (I don't have the access right)
> 
> Sorry for the delay, I was out for a couple of days.
> 
> Your patch is bitrotted because you seemingly haven't updated your checkout
for
> a couple of weeks.  The files in question have moved and the code has also
> changed some so that your patches no longer apply.  Can you update to the
latest
> source and upload a new snapshot that applies against the current tree?  I
> should be able to commit after that.

[Peter Kasting, 2009-03-31 17:55:37]

On 2009/03/28 03:19:48, DeArto20 wrote:
> I have updated to the latest source and made new patch.
> Check the new patch and commit it for me. Thanks.

I have successfully applied and built with your patch.  While testing I found
that the patch does not match Windows native behavior in one particular way: if
the mouse button is released while the mouse position is off the caption area,
the menu still appears, whereas native apps do nothing here.  It seems like
maybe you need to HitTest() in your button-up handler?  If you could fix this,
that'd be great.

Once again, sorry for all the delays here.  My availability has been sporadic
lately...

[Peter Kasting, 2009-05-21 21:36:08]

ping

[DeArto20, 2009-05-23 10:24:48]

On 2009/05/21 21:36:08, pkasting wrote:
> ping
I updated my chrome browser to the new version (v2.0.172.18) and found that this
patch has not been applied yet. Is there any problem?

[DeArto20, 2009-06-01 18:04:38]

On 2009/05/23 10:24:48, DeArto20 wrote:
> On 2009/05/21 21:36:08, pkasting wrote:
> > ping
> I updated my chrome browser to the new version (v2.0.172.18) and found that
this
> patch has not been applied yet. Is there any problem?


Can you tell me why this patch has not been applied yet?

[sky, 2009-06-01 18:07:27]

As far as I can tell Peter had concerns about your patch a couple months back:

> I have successfully applied and built with your patch.  While testing I found
> that the patch does not match Windows native behavior in one particular way:
if
> the mouse button is released while the mouse position is off the caption area,
> the menu still appears, whereas native apps do nothing here.  It seems like
> maybe you need to HitTest() in your button-up handler?  If you could fix this,
> that'd be great.

Have you addressed these concerns?

   -Scott

[DeArto20, 2009-06-03 05:35:21]

On 2009/06/01 18:07:27, sky wrote:
> As far as I can tell Peter had concerns about your patch a couple months back:
> 
> > I have successfully applied and built with your patch.  While testing I
found
> > that the patch does not match Windows native behavior in one particular way:
> if
> > the mouse button is released while the mouse position is off the caption
area,
> > the menu still appears, whereas native apps do nothing here.  It seems like
> > maybe you need to HitTest() in your button-up handler?  If you could fix
this,
> > that'd be great.
> 
> Have you addressed these concerns?
> 
>    -Scott

I thought that it is the another issue that should be treated on the later
patch. Sorry for my misunderstanding.

I'll fix the above issue soon. Thanks.

[DeArto20, 2009-06-06 21:00:12]

Modified the following problem.
: When the mouse button is released while the mouse position is off the caption
area, the menu still appears.

[Peter Kasting, 2009-06-06 22:07:10]

http://codereview.chromium.org/18095/diff/9405/10402
File views/window/window_win.cc (right):

http://codereview.chromium.org/18095/diff/9405/10402#newcode976
Line 976: ::SendMessage(GetNativeView(), WM_NCHITTEST, 0, MAKELPARAM(temp.x,
temp.y));
Nit: This line is longer than 80 characters.

Also, why SendMessage(WM_NCHITTEST)?  Why not just call the appropriate
HitTest() views method directly?

[DeArto20, 2009-06-07 08:03:30]

http://codereview.chromium.org/18095/diff/9405/10402
File views/window/window_win.cc (right):

http://codereview.chromium.org/18095/diff/9405/10402#newcode976
Line 976: ::SendMessage(GetNativeView(), WM_NCHITTEST, 0, MAKELPARAM(temp.x,
temp.y));
I modified the code. Now it is not over 80 characters and exactly same to the
Line 686 in this file.
- WidgetWin::OnMouseLeave uses the SendMessage(WM_NCHITTEST) method.

I wanted to use the same way that other function in this file (widget_win.cc)
does.

On 2009/06/06 22:07:10, pkasting wrote:
> Nit: This line is longer than 80 characters.
> 
> Also, why SendMessage(WM_NCHITTEST)?  Why not just call the appropriate
> HitTest() views method directly?

[DeArto20, 2009-06-07 08:05:05]

http://codereview.chromium.org/18095/diff/9405/10402
File views/window/window_win.cc (right):

http://codereview.chromium.org/18095/diff/9405/10402#newcode976
Line 976: ::SendMessage(GetNativeView(), WM_NCHITTEST, 0, MAKELPARAM(temp.x,
temp.y));
On 2009/06/07 08:03:31, DeArto20 wrote:
> I modified the code. Now it is not over 80 characters and exactly same to the
> Line 686 in this file.
> - WidgetWin::OnMouseLeave uses the SendMessage(WM_NCHITTEST) method.
> 
> I wanted to use the same way that other function in this file (widget_win.cc)
> does.
> 
> On 2009/06/06 22:07:10, pkasting wrote:
> > Nit: This line is longer than 80 characters.
> > 
> > Also, why SendMessage(WM_NCHITTEST)?  Why not just call the appropriate
> > HitTest() views method directly?
> 
> 

Done.

[Peter Kasting, 2009-06-07 23:24:09]

http://codereview.chromium.org/18095/diff/9405/10402
File views/window/window_win.cc (right):

http://codereview.chromium.org/18095/diff/9405/10402#newcode976
Line 976: ::SendMessage(GetNativeView(), WM_NCHITTEST, 0, MAKELPARAM(temp.x,
temp.y));
On 2009/06/07 08:03:31, DeArto20 wrote:
> - WidgetWin::OnMouseLeave uses the SendMessage(WM_NCHITTEST) method.
> 
> I wanted to use the same way that other function in this file (widget_win.cc)
> does.

I don't see why it should use that method either.  Can you fix both please?

[DeArto20, 2009-06-08 16:19:21]

What we wanna know here is whether the hit was occurred on the caption bar or
not.

SendMessage(WM_NCHITTEST) returns the exact hit location in the window  (caption
bar, client area, etc).

But using HitTest() function, the only information we can get is whether the hit
was occurred in the window or not. It doesn't tell us the exact location of the
hit in the window. (We can't know if the hit was occurred on the caption bar or
client area or else.)

So, in my thought, we should use SendMessage(WM_NCHITTEST) here to get the exact
hit location in the window.

On 2009/06/07 23:24:09, pkasting wrote:
> http://codereview.chromium.org/18095/diff/9405/10402
> File views/window/window_win.cc (right):
> 
> http://codereview.chromium.org/18095/diff/9405/10402#newcode976
> Line 976: ::SendMessage(GetNativeView(), WM_NCHITTEST, 0, MAKELPARAM(temp.x,
> temp.y));
> On 2009/06/07 08:03:31, DeArto20 wrote:
> > - WidgetWin::OnMouseLeave uses the SendMessage(WM_NCHITTEST) method.
> > 
> > I wanted to use the same way that other function in this file
(widget_win.cc)
> > does.
> 
> I don't see why it should use that method either.  Can you fix both please?

[Peter Kasting, 2009-06-08 17:32:45]

On 2009/06/08 16:19:21, DeArto20 wrote:
> What we wanna know here is whether the hit was occurred on the caption bar or
> not.
> 
> SendMessage(WM_NCHITTEST) returns the exact hit location in the window 
(caption
> bar, client area, etc).
> 
> But using HitTest() function, the only information we can get is whether the
hit
> was occurred in the window or not.

OK.  I was apparently thinking of NonClientHitTest(), which does give us this,
but I agree that there's no reason to obtain the frame and call that when you
can just do it this way.  So LGTM.

[DeArto20, 2009-06-09 14:33:58]

Please commit this patch for me (I don't have the access right to the server).
Thanks!

On 2009/06/08 17:32:45, pkasting wrote:
> On 2009/06/08 16:19:21, DeArto20 wrote:
> > What we wanna know here is whether the hit was occurred on the caption bar
or
> > not.
> > 
> > SendMessage(WM_NCHITTEST) returns the exact hit location in the window 
> (caption
> > bar, client area, etc).
> > 
> > But using HitTest() function, the only information we can get is whether the
> hit
> > was occurred in the window or not.
> 
> OK.  I was apparently thinking of NonClientHitTest(), which does give us this,
> but I agree that there's no reason to obtain the frame and call that when you
> can just do it this way.  So LGTM.

[Peter Kasting, 2009-06-09 21:27:14]

On 2009/06/09 14:33:58, DeArto20 wrote:
> Please commit this patch for me (I don't have the access right to the server).
> Thanks!

Doh.  I patched it in and retested and there's still a problem.

You have a screen-vs.-window coordinate transform problem.  With your new
hittesting code, clicks do nothing at all when the window is restored and moved
away from the screen edges.  You'll need to figure out whether your coordinates
here are screen or window coordinates, and which one WM_NCHITTEST expects, and
then convert.  If code elsewhere in this file uses WM_NCHITTEST it may already
be doing this; if not look for uses of views methods with names including
"screen".

[DeArto20, 2009-06-10 15:46:18]

I modified the indicated (screen-vs.-window coordinate transform) problem.

[Peter Kasting, 2009-06-11 17:21:54]

Modified and landed in r18170.