Disambiguate OnMouseCaptureLost from OnMouseReleased, etc.

Disambiguate OnMouseCaptureLost from OnMouseReleased.
Nix RenderWidgetHostViewViews' OnMouseCaptureLost forwarding.
Consolidate code, remove unnecessary overrides, etc.
Fix up test, |event| args, OVERRIDEs, and (c) dates.

BUG=72040
TEST=Views mouse button release, cancelling mouse down/drag.

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

[msw, 2011-03-17 23:28:51]

Still a work in progress, but ready enough for review.
Need guidance / testing / investigation at comments/TODOs.

http://codereview.chromium.org/6685069/diff/2001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_host_view_views.cc (right):

http://codereview.chromium.org/6685069/diff/2001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_views.cc:675: // TODO(msw):
Replicate behavior of OnMouseReleased with synthesized event?
Need guidance / deeper investigation.

http://codereview.chromium.org/6685069/diff/2001/chrome/browser/ui/views/tabs...
File chrome/browser/ui/views/tabs/side_tab_strip.cc (right):

http://codereview.chromium.org/6685069/diff/2001/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/side_tab_strip.cc:36: // TODO(msw): Remove?
Overrides BaseTab::OnMouseCaptureLost() with no-op.
Need guidance / testing / investigation.

http://codereview.chromium.org/6685069/diff/2001/views/controls/button/button...
File views/controls/button/button_dropdown.h (right):

http://codereview.chromium.org/6685069/diff/2001/views/controls/button/button...
views/controls/button/button_dropdown.h:32: // TODO(msw): Remove? Overrides
CustomButton::OnMouseCaptureLost() with no-op.
Need guidance / testing / investigation.

http://codereview.chromium.org/6685069/diff/2001/views/controls/button/custom...
File views/controls/button/custom_button.cc (right):

http://codereview.chromium.org/6685069/diff/2001/views/controls/button/custom...
views/controls/button/custom_button.cc:133: // TODO(msw): Is this still true?
Perhaps the InDrag() check is unnecessary.
Need guidance / testing; or leave as-is.

http://codereview.chromium.org/6685069/diff/2001/views/controls/single_split_...
File views/controls/single_split_view.cc (right):

http://codereview.chromium.org/6685069/diff/2001/views/controls/single_split_...
views/controls/single_split_view.cc:186: // TODO(msw): Was this intentionally a
no-op on non-cancelled mouse releases?
Need guidance / testing / revision history research.

http://codereview.chromium.org/6685069/diff/2001/views/controls/single_split_...
File views/controls/single_split_view_unittest.cc (right):

http://codereview.chromium.org/6685069/diff/2001/views/controls/single_split_...
views/controls/single_split_view_unittest.cc:169: // TODO(msw): Wasn't
SingleSplitView::OnMouseReleased(foo, *false*) a no-op?
Need guidance / testing / revision history research.

[sadrul, 2011-03-19 09:15:30]

It looks like you are moving a lot of overrides from private to public. Why so?
If you are going to change the visibility of the overrides, I would suggest you
move them the other way around (i.e. move the public ones into private/protected
when possible). For details:
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...
(Ben's original mail, and Peter's reply)

The OnMouseReleased and OnMouseCaptureLost handlers are duplicated in some
places. That needs to be avoided (see comment in
render_widget_host_view_views.cc)

http://codereview.chromium.org/6685069/diff/162/chrome/browser/chromeos/input...
File chrome/browser/chromeos/input_method/candidate_window.cc (right):

http://codereview.chromium.org/6685069/diff/162/chrome/browser/chromeos/input...
chrome/browser/chromeos/input_method/candidate_window.cc:530: // Overridden from
View:
views::View

Why are these moved from private to public? I believe private overrides are
preferred unless public/protected is necessary. (here and other places)

http://codereview.chromium.org/6685069/diff/162/chrome/browser/renderer_host/...
File chrome/browser/renderer_host/render_widget_host_view_views.cc (right):

http://codereview.chromium.org/6685069/diff/162/chrome/browser/renderer_host/...
chrome/browser/renderer_host/render_widget_host_view_views.cc:665: const
views::MouseEvent& event) {
Merge lines 664-665 into one.

http://codereview.chromium.org/6685069/diff/162/chrome/browser/renderer_host/...
chrome/browser/renderer_host/render_widget_host_view_views.cc:675: // TODO(msw):
Replicate behavior of OnMouseReleased with synthesized event?
Instead of duplicating the code in OnMouseReleased and OnMouseCaptureLost (here
and other places, for example PanelScrollerHeader), you could do either

(1) the default implementation (View::OnMouseCaptureLost) generates a synthetic
mouse-released event and invokes View::OnMouseReleased).

(2) View::OnMouseCaptureLost returns a bool, indicating whether a synthetic
MouseReleased event should be sent or not. RootView already creates such a
synthetic released event, so it can easily do (line root_view.cc:285):

if (!mouse_pressed_handler->OnMouseCaptureLost())
    mouse_pressed_handler->ProcessMouseReleased(...);

and the default (View::OnMouseCaptureLost) would return false.

http://codereview.chromium.org/6685069/diff/162/chrome/browser/ui/views/conte...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

http://codereview.chromium.org/6685069/diff/162/chrome/browser/ui/views/conte...
chrome/browser/ui/views/content_setting_bubble_contents.cc:56: virtual void
OnMouseReleased(const views::MouseEvent& event) OVERRIDE;
Why are these moved from private to public?

http://codereview.chromium.org/6685069/diff/162/chrome/browser/ui/views/downl...
File chrome/browser/ui/views/download_item_view.cc (right):

http://codereview.chromium.org/6685069/diff/162/chrome/browser/ui/views/downl...
chrome/browser/ui/views/download_item_view.cc:565: 
Code duplication like this needs to be avoided. See note on
render_widget_host_view_views.cc about this.

http://codereview.chromium.org/6685069/diff/162/chrome/browser/ui/views/locat...
File chrome/browser/ui/views/location_bar/content_setting_image_view.h (right):

http://codereview.chromium.org/6685069/diff/162/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/content_setting_image_view.h:43: virtual
void OnMouseReleased(const views::MouseEvent& event) OVERRIDE;
Why private->public?

http://codereview.chromium.org/6685069/diff/162/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/content_setting_image_view.h:55: virtual
void OnPaintBackground(gfx::Canvas* canvas) OVERRIDE;
Why private->protected?

http://codereview.chromium.org/6685069/diff/162/chrome/browser/ui/views/locat...
File chrome/browser/ui/views/location_bar/star_view.h (right):

http://codereview.chromium.org/6685069/diff/162/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/star_view.h:42: private:
Why are all the private functions made public?

http://codereview.chromium.org/6685069/diff/162/views/widget/root_view.cc
File views/widget/root_view.cc (right):

http://codereview.chromium.org/6685069/diff/162/views/widget/root_view.cc#new...
views/widget/root_view.cc:277: UpdateCursor(MouseEvent(release_event, this));
The purpose of MouseEvent(event, RootView*) is to convert the location of
'event' to the RootView's coordinate system from the widget's coordinate system.
last_mouse_event_x_ and _y_ are already in the RootView's coordinate, so you can
just send release_event to UpdateCursor here.

http://codereview.chromium.org/6685069/diff/162/views/widget/widget_gtk.h
File views/widget/widget_gtk.h (right):

http://codereview.chromium.org/6685069/diff/162/views/widget/widget_gtk.h#new...
views/widget/widget_gtk.h:1: // Copyright (c) 2010 The Chromium Authors. All
rights reserved.
?

[Ben Goodger (Google), 2011-03-19 15:32:55]

http://codereview.chromium.org/6685069/diff/162/chrome/browser/chromeos/frame...
File chrome/browser/chromeos/frame/panel_controller.cc (right):

http://codereview.chromium.org/6685069/diff/162/chrome/browser/chromeos/frame...
chrome/browser/chromeos/frame/panel_controller.cc:397: DCHECK(panel_controller_)
<< "OnMouseCaptureLost after Close";
These kind of DCHECKs are unnecessary. You're going to crash on the next line
anyway if panel_controller_ is NULL.

http://codereview.chromium.org/6685069/diff/162/chrome/browser/ui/views/conte...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

http://codereview.chromium.org/6685069/diff/162/chrome/browser/ui/views/conte...
chrome/browser/ui/views/content_setting_bubble_contents.cc:103:
HitTest(event.location()))
braces around this block

http://codereview.chromium.org/6685069/diff/162/views/controls/menu/menu_host...
File views/controls/menu/menu_host_root_view.cc (right):

http://codereview.chromium.org/6685069/diff/162/views/controls/menu/menu_host...
views/controls/menu/menu_host_root_view.cc:59: RootView::OnMouseCaptureLost();
Again with the duplication

[msw, 2011-03-26 00:09:50]

Sorry for the delay; debugging tests via print statements sux.
I reverted function re-orderings, reduced duplication, fix a test, deprecated
RenderWidgetHostViewViews' OnMouseCaptureLost WebMouseEvent forwarding, cleaned
up a bunch of code, etc. PTAL.

http://codereview.chromium.org/6685069/diff/162/chrome/browser/chromeos/frame...
File chrome/browser/chromeos/frame/panel_controller.cc (right):

http://codereview.chromium.org/6685069/diff/162/chrome/browser/chromeos/frame...
chrome/browser/chromeos/frame/panel_controller.cc:397: DCHECK(panel_controller_)
<< "OnMouseCaptureLost after Close";
On 2011/03/19 15:32:55, Ben Goodger wrote:
> These kind of DCHECKs are unnecessary. You're going to crash on the next line
> anyway if panel_controller_ is NULL.

Done. Fixed this file and double-checked my other changes.

http://codereview.chromium.org/6685069/diff/162/chrome/browser/chromeos/input...
File chrome/browser/chromeos/input_method/candidate_window.cc (right):

http://codereview.chromium.org/6685069/diff/162/chrome/browser/chromeos/input...
chrome/browser/chromeos/input_method/candidate_window.cc:530: // Overridden from
View:
On 2011/03/19 09:15:30, sadrul wrote:
> views::View
> 
> Why are these moved from private to public? I believe private overrides are
> preferred unless public/protected is necessary. (here and other places)

Done.

http://codereview.chromium.org/6685069/diff/162/chrome/browser/renderer_host/...
File chrome/browser/renderer_host/render_widget_host_view_views.cc (right):

http://codereview.chromium.org/6685069/diff/162/chrome/browser/renderer_host/...
chrome/browser/renderer_host/render_widget_host_view_views.cc:665: const
views::MouseEvent& event) {
On 2011/03/19 09:15:30, sadrul wrote:
> Merge lines 664-665 into one.

That would be 81 characters :(

http://codereview.chromium.org/6685069/diff/162/chrome/browser/renderer_host/...
chrome/browser/renderer_host/render_widget_host_view_views.cc:675: // TODO(msw):
Replicate behavior of OnMouseReleased with synthesized event?
On 2011/03/19 09:15:30, sadrul wrote:
> Instead of duplicating the code in OnMouseReleased and OnMouseCaptureLost
(here
> and other places, for example PanelScrollerHeader), you could do either
> 
> (1) the default implementation (View::OnMouseCaptureLost) generates a
synthetic
> mouse-released event and invokes View::OnMouseReleased).
> 
> (2) View::OnMouseCaptureLost returns a bool, indicating whether a synthetic
> MouseReleased event should be sent or not. RootView already creates such a
> synthetic released event, so it can easily do (line root_view.cc:285):
> 
> if (!mouse_pressed_handler->OnMouseCaptureLost())
>     mouse_pressed_handler->ProcessMouseReleased(...);
> 
> and the default (View::OnMouseCaptureLost) would return false.

I removed this override altogether to parallel RenderWidgetHostViewWin, which
doesn't generate a MouseUp WebMouseEvent on mouse capture loss (event is
un-handled). As such, we can avoid complicating the framework to handle this
case.

http://codereview.chromium.org/6685069/diff/162/chrome/browser/ui/views/conte...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

http://codereview.chromium.org/6685069/diff/162/chrome/browser/ui/views/conte...
chrome/browser/ui/views/content_setting_bubble_contents.cc:56: virtual void
OnMouseReleased(const views::MouseEvent& event) OVERRIDE;
On 2011/03/19 09:15:30, sadrul wrote:
> Why are these moved from private to public?

Done.

http://codereview.chromium.org/6685069/diff/162/chrome/browser/ui/views/conte...
chrome/browser/ui/views/content_setting_bubble_contents.cc:103:
HitTest(event.location()))
On 2011/03/19 15:32:55, Ben Goodger wrote:
> braces around this block

Done.

http://codereview.chromium.org/6685069/diff/162/chrome/browser/ui/views/downl...
File chrome/browser/ui/views/download_item_view.cc (right):

http://codereview.chromium.org/6685069/diff/162/chrome/browser/ui/views/downl...
chrome/browser/ui/views/download_item_view.cc:565: 
On 2011/03/19 09:15:30, sadrul wrote:
> Code duplication like this needs to be avoided. See note on
> render_widget_host_view_views.cc about this.

Done. In this instance, the |dragging_| block wasn't needed in OnMouseReleased.

http://codereview.chromium.org/6685069/diff/162/chrome/browser/ui/views/locat...
File chrome/browser/ui/views/location_bar/content_setting_image_view.h (right):

http://codereview.chromium.org/6685069/diff/162/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/content_setting_image_view.h:43: virtual
void OnMouseReleased(const views::MouseEvent& event) OVERRIDE;
On 2011/03/19 09:15:30, sadrul wrote:
> Why private->public?

Done.

http://codereview.chromium.org/6685069/diff/162/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/content_setting_image_view.h:55: virtual
void OnPaintBackground(gfx::Canvas* canvas) OVERRIDE;
On 2011/03/19 09:15:30, sadrul wrote:
> Why private->protected?

Done.

http://codereview.chromium.org/6685069/diff/162/chrome/browser/ui/views/locat...
File chrome/browser/ui/views/location_bar/star_view.h (right):

http://codereview.chromium.org/6685069/diff/162/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/star_view.h:42: private:
On 2011/03/19 09:15:30, sadrul wrote:
> Why are all the private functions made public?

Done.

http://codereview.chromium.org/6685069/diff/162/views/controls/menu/menu_host...
File views/controls/menu/menu_host_root_view.cc (right):

http://codereview.chromium.org/6685069/diff/162/views/controls/menu/menu_host...
views/controls/menu/menu_host_root_view.cc:59: RootView::OnMouseCaptureLost();
On 2011/03/19 15:32:55, Ben Goodger wrote:
> Again with the duplication

It turns out that this canceled code path was causing inadvertent menu closures
on mouse drag starts (now that WidgetWin directly calls this OnMouseCaptureLost,
rather than bailing at RootView::ProcessMouseDragCanceled's
mouse_pressed_handler_ check). Removing this path doesn't seem to affect menu
behavior adversely. I also removed the unused suspend_events flag and the
unnecessary OnMouseExited override.

http://codereview.chromium.org/6685069/diff/162/views/widget/root_view.cc
File views/widget/root_view.cc (right):

http://codereview.chromium.org/6685069/diff/162/views/widget/root_view.cc#new...
views/widget/root_view.cc:277: UpdateCursor(MouseEvent(release_event, this));
On 2011/03/19 09:15:30, sadrul wrote:
> The purpose of MouseEvent(event, RootView*) is to convert the location of
> 'event' to the RootView's coordinate system from the widget's coordinate
system.
> last_mouse_event_x_ and _y_ are already in the RootView's coordinate, so you
can
> just send release_event to UpdateCursor here.

Done.

http://codereview.chromium.org/6685069/diff/162/views/widget/widget_gtk.h
File views/widget/widget_gtk.h (right):

http://codereview.chromium.org/6685069/diff/162/views/widget/widget_gtk.h#new...
views/widget/widget_gtk.h:1: // Copyright (c) 2010 The Chromium Authors. All
rights reserved.
On 2011/03/19 09:15:30, sadrul wrote:
> ?

Done.

[Ben Goodger (Google), 2011-03-28 23:47:37]

LGTM from my perspective.

[sadrul, 2011-03-29 00:18:22]

LGTM 2

[sky, 2016-03-02 18:34:57]

sky@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2016-03-02 18:34:58]

Ok, LGTM