Context menu on views must show on mouse down for non-WIN.

Context menu on views must show on mouse down for non-WIN.
This is mostly a copy of https://chromiumcodereview.appspot.com/12395010 which was reverted due to a regression (https://code.google.com/p/chromium/issues/detail?id=189100). I have attempted to fix that regression here.

BUG=189100, 179381
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192525

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192688

[sky, 2013-03-20 21:35:53]

I think this means Widget::OnMouseEvent for ui::ET_MOUSE_PRESSED needs to deal
with 'this' being dleeted as part of OnMousePressed too.

https://codereview.chromium.org/12529012/diff/3001/ui/views/controls/menu/men...
File ui/views/controls/menu/menu_runner.cc (right):

https://codereview.chromium.org/12529012/diff/3001/ui/views/controls/menu/men...
ui/views/controls/menu/menu_runner.cc:348:
parent->GetRootView()->SetMouseHandler(NULL);
This means a view isn't going to get a mouse exited. Are you sure all views that
have context menus are set up to deal correctly with this?

https://codereview.chromium.org/12529012/diff/3001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/12529012/diff/3001/ui/views/view.cc#newcode2080
ui/views/view.cc:2080: // Assume that if there is a context menu controller we
won't be deleted
Hm. Don't you see the warning on 2075? I don't see how you can assume a context
menu controller implies 'this' hasn't been deleted by OnMousePressed.

[varunjain, 2013-03-20 22:05:52]

Hi Scott.. I am not sure I understand your Widget::OnMouseEvent comment. How
does the widget care if this view is deleted as a result of mouse press? Or are
you suggesting that the widget could be deleted? What case could that happen?

https://codereview.chromium.org/12529012/diff/3001/ui/views/controls/menu/men...
File ui/views/controls/menu/menu_runner.cc (right):

https://codereview.chromium.org/12529012/diff/3001/ui/views/controls/menu/men...
ui/views/controls/menu/menu_runner.cc:348:
parent->GetRootView()->SetMouseHandler(NULL);
On 2013/03/20 21:35:53, sky wrote:
> This means a view isn't going to get a mouse exited. Are you sure all views
that
> have context menus are set up to deal correctly with this?

SetMouseHandler(NULL) does not reset the mouse_move_handler in RootView. So the
view should still get exit. More generally speaking, the functionality of
RootView::OnMouseMove and RootView::OnMouseExit should not change after this.

https://codereview.chromium.org/12529012/diff/3001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/12529012/diff/3001/ui/views/view.cc#newcode2080
ui/views/view.cc:2080: // Assume that if there is a context menu controller we
won't be deleted
On 2013/03/20 21:35:53, sky wrote:
> Hm. Don't you see the warning on 2075? I don't see how you can assume a
context
> menu controller implies 'this' hasn't been deleted by OnMousePressed.

This seemed standard because the same assumption is made in ProcessMouseReleased
(line 2124 below).

[sky, 2013-03-20 23:42:15]

On Wed, Mar 20, 2013 at 3:05 PM, <varunjain@chromium.org> wrote:

> Hi Scott.. I am not sure I understand your Widget::OnMouseEvent comment.
> How
> does the widget care if this view is deleted as a result of mouse press?
> Or are
> you suggesting that the widget could be deleted?


Exactly.


> What case could that happen?


There is no way to prevent it. Say the window manager kills the window, or
some code path is triggered (maybe during shutdown) that deletes the window.


>
>
>
> https://codereview.chromium.**org/12529012/diff/3001/ui/**
>
views/controls/menu/menu_**runner.cc<https://codereview.chromium.org/12529012/diff/3001/ui/views/controls/menu/menu_runner.cc>
> File ui/views/controls/menu/menu_**runner.cc (right):
>
> https://codereview.chromium.**org/12529012/diff/3001/ui/**
>
views/controls/menu/menu_**runner.cc#newcode348<https://codereview.chromium.org/12529012/diff/3001/ui/views/controls/menu/menu_runner.cc#newcode348>
> ui/views/controls/menu/menu_**runner.cc:348:
> parent->GetRootView()->**SetMouseHandler(NULL);
> On 2013/03/20 21:35:53, sky wrote:
>
>> This means a view isn't going to get a mouse exited. Are you sure all
>>
> views that
>
>> have context menus are set up to deal correctly with this?
>>
>
> SetMouseHandler(NULL) does not reset the mouse_move_handler in RootView.
> So the view should still get exit. More generally speaking, the
> functionality of RootView::OnMouseMove and RootView::OnMouseExit should
> not change after this.


Does the capture triggered by the menu effect this at all?


>
>
>
https://codereview.chromium.**org/12529012/diff/3001/ui/**views/view.cc<https...
> File ui/views/view.cc (right):
>
> https://codereview.chromium.**org/12529012/diff/3001/ui/**
>
views/view.cc#newcode2080<https://codereview.chromium.org/12529012/diff/3001/ui/views/view.cc#newcode2080>
> ui/views/view.cc:2080: // Assume that if there is a context menu
> controller we won't be deleted
> On 2013/03/20 21:35:53, sky wrote:
>
>> Hm. Don't you see the warning on 2075? I don't see how you can assume
>>
> a context
>
>> menu controller implies 'this' hasn't been deleted by OnMousePressed.
>>
>
> This seemed standard because the same assumption is made in
> ProcessMouseReleased (line 2124 below).
>

Good point. Please move the comment below your code then.

>
>
https://codereview.chromium.**org/12529012/<https://codereview.chromium.org/1...
>

[varunjain, 2013-03-21 18:35:38]

On 2013/03/20 23:42:15, sky wrote:
> On Wed, Mar 20, 2013 at 3:05 PM, <mailto:varunjain@chromium.org> wrote:
> 
> > Hi Scott.. I am not sure I understand your Widget::OnMouseEvent comment.
> > How
> > does the widget care if this view is deleted as a result of mouse press?
> > Or are
> > you suggesting that the widget could be deleted?
> 
> 
> Exactly.
> 
> 
> > What case could that happen?
> 
> 
> There is no way to prevent it. Say the window manager kills the window, or
> some code path is triggered (maybe during shutdown) that deletes the window.
> 

Done.

> 
> >
> >
> >
> > https://codereview.chromium.**org/12529012/diff/3001/ui/**
> >
>
views/controls/menu/menu_**runner.cc<https://codereview.chromium.org/12529012/diff/3001/ui/views/controls/menu/menu_runner.cc>
> > File ui/views/controls/menu/menu_**runner.cc (right):
> >
> > https://codereview.chromium.**org/12529012/diff/3001/ui/**
> >
>
views/controls/menu/menu_**runner.cc#newcode348<https://codereview.chromium.org/12529012/diff/3001/ui/views/controls/menu/menu_runner.cc#newcode348>
> > ui/views/controls/menu/menu_**runner.cc:348:
> > parent->GetRootView()->**SetMouseHandler(NULL);
> > On 2013/03/20 21:35:53, sky wrote:
> >
> >> This means a view isn't going to get a mouse exited. Are you sure all
> >>
> > views that
> >
> >> have context menus are set up to deal correctly with this?
> >>
> >
> > SetMouseHandler(NULL) does not reset the mouse_move_handler in RootView.
> > So the view should still get exit. More generally speaking, the
> > functionality of RootView::OnMouseMove and RootView::OnMouseExit should
> > not change after this.
> 
> 
> Does the capture triggered by the menu effect this at all?

Looking at the code, it doesnt look like the capture will affect it. When the
mouse comes back to this widget after the capture is lifted, everything will
work. I have done some tests as well and it seems fine.

> 
> 
> >
> >
> >
>
https://codereview.chromium.**org/12529012/diff/3001/ui/**views/view.cc%3Chtt...>
> > File ui/views/view.cc (right):
> >
> > https://codereview.chromium.**org/12529012/diff/3001/ui/**
> >
>
views/view.cc#newcode2080<https://codereview.chromium.org/12529012/diff/3001/ui/views/view.cc#newcode2080>
> > ui/views/view.cc:2080: // Assume that if there is a context menu
> > controller we won't be deleted
> > On 2013/03/20 21:35:53, sky wrote:
> >
> >> Hm. Don't you see the warning on 2075? I don't see how you can assume
> >>
> > a context
> >
> >> menu controller implies 'this' hasn't been deleted by OnMousePressed.
> >>
> >
> > This seemed standard because the same assumption is made in
> > ProcessMouseReleased (line 2124 below).
> >
> 
> Good point. Please move the comment below your code then.

Done.

> 
> >
> >
>
https://codereview.chromium.**org/12529012/%3Chttps://codereview.chromium.org...>
> >

[sky, 2013-03-21 22:17:48]

https://codereview.chromium.org/12529012/diff/15001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/12529012/diff/15001/ui/views/widget/widget.cc...
ui/views/widget/widget.cc:64: class PostMousePressedProcessor : public
WidgetObserver {
How about declaring this as an inner class (you can keep all this here, just put
class PostMousePressedProcessor; in Widget). That way you don't need to expose
set_is_mouse_button_pressed.

https://codereview.chromium.org/12529012/diff/15001/ui/views/widget/widget.cc...
ui/views/widget/widget.cc:94: if (owner_) {
Consolidate this and 71-74.

[varunjain, 2013-03-21 22:47:13]

https://codereview.chromium.org/12529012/diff/15001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/12529012/diff/15001/ui/views/widget/widget.cc...
ui/views/widget/widget.cc:64: class PostMousePressedProcessor : public
WidgetObserver {
On 2013/03/21 22:17:48, sky wrote:
> How about declaring this as an inner class (you can keep all this here, just
put
> class PostMousePressedProcessor; in Widget). That way you don't need to expose
> set_is_mouse_button_pressed.

Done.

https://codereview.chromium.org/12529012/diff/15001/ui/views/widget/widget.cc...
ui/views/widget/widget.cc:94: if (owner_) {
On 2013/03/21 22:17:48, sky wrote:
> Consolidate this and 71-74.

Done.

[sky, 2013-03-22 04:01:56]

LGTM

[varunjain, 2013-03-27 23:23:25]

On 2013/03/22 04:01:56, sky wrote:
> LGTM

Hi Scott,
Made some more changes that requires more review PTAL:
1. changes in browser_action_view.cc to address crbug.com/181322 :
The BrowserActionButton was showing context menu on mouse down to begin with. In
this change I add this functionality to View. This resulted in two calls to
ShowContextMenu. I removed the one in BrowserActionButton.
2. changes in menu_controller: increase the offset to 2 pixels to address the
selection-on-mouse-up concerns being discussed in the bug. This solution was
suggested by Kuscher on that bug.

[sky, 2013-03-28 16:21:10]

I'm still of the opinion we should delay. I added comments to the bug.

[varunjain, 2013-04-04 20:08:00]

On 2013/03/28 16:21:10, sky wrote:
> I'm still of the opinion we should delay. I added comments to the bug.

Hi Scott, I have added the delay. PTAL.

[sky, 2013-04-04 20:36:04]

https://codereview.chromium.org/12529012/diff/42001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/browser_action_view.cc (left):

https://codereview.chromium.org/12529012/diff/42001/chrome/browser/ui/views/b...
chrome/browser/ui/views/browser_action_view.cc:317: // See comments in
MenuButton::Activate() as to why this is needed.
Why are you removing this?

https://codereview.chromium.org/12529012/diff/42001/ui/views/controls/menu/me...
File ui/views/controls/menu/menu_controller.h (right):

https://codereview.chromium.org/12529012/diff/42001/ui/views/controls/menu/me...
ui/views/controls/menu/menu_controller.h:571: base::TimeDelta menu_start_time_;
Why is this a timedeal and not timeticks?

https://codereview.chromium.org/12529012/diff/42001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/12529012/diff/42001/ui/views/view.cc#newcode2078
ui/views/view.cc:2078: // WARNING: we may have been deleted, don't use any View
variables.
Move this after your if.

https://codereview.chromium.org/12529012/diff/42001/ui/views/widget/widget_un...
File ui/views/widget/widget_unittest.cc (right):

https://codereview.chromium.org/12529012/diff/42001/ui/views/widget/widget_un...
ui/views/widget/widget_unittest.cc:1721: WidgetDeleterView() : View() { }
nit: { } -> {}

[varunjain, 2013-04-04 21:02:08]

https://codereview.chromium.org/12529012/diff/42001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/browser_action_view.cc (left):

https://codereview.chromium.org/12529012/diff/42001/chrome/browser/ui/views/b...
chrome/browser/ui/views/browser_action_view.cc:317: // See comments in
MenuButton::Activate() as to why this is needed.
On 2013/04/04 20:36:04, sky wrote:
> Why are you removing this?

Now that we are showing context menu on mouse press for all views, this is not
needed here since it would happen through normal views workflow in
View::ProcessedMousePressed. However, this should not be skipped on OS_WIN.
Adding it back with appropriate conditional.

https://codereview.chromium.org/12529012/diff/42001/ui/views/controls/menu/me...
File ui/views/controls/menu/menu_controller.h (right):

https://codereview.chromium.org/12529012/diff/42001/ui/views/controls/menu/me...
ui/views/controls/menu/menu_controller.h:571: base::TimeDelta menu_start_time_;
On 2013/04/04 20:36:04, sky wrote:
> Why is this a timedeal and not timeticks?

Done.

https://codereview.chromium.org/12529012/diff/42001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/12529012/diff/42001/ui/views/view.cc#newcode2078
ui/views/view.cc:2078: // WARNING: we may have been deleted, don't use any View
variables.
On 2013/04/04 20:36:04, sky wrote:
> Move this after your if.

Done.

https://codereview.chromium.org/12529012/diff/42001/ui/views/widget/widget_un...
File ui/views/widget/widget_unittest.cc (right):

https://codereview.chromium.org/12529012/diff/42001/ui/views/widget/widget_un...
ui/views/widget/widget_unittest.cc:1721: WidgetDeleterView() : View() { }
On 2013/04/04 20:36:04, sky wrote:
> nit: { } -> {}

Done.

[sky, 2013-04-04 22:56:56]

LGTM

[commit-bot: I haz the power, 2013-04-04 23:16:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/12529012/50001

[commit-bot: I haz the power, 2013-04-05 00:31:44]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[varunjain, 2013-04-05 05:36:20]

On 2013/04/05 00:31:44, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

Hi Scott, I had to fix BookmarkBarViewTest that was failing due to this change.
Can you please have a quick look at the last patch. The only un-reviewed file is
bookmark_bar_view_test.cc.
I am putting this in the CQ. If you have comments about the change in
bookmark_bar_view_test.cc, I will address them in a subsequent CL.

[commit-bot: I haz the power, 2013-04-05 05:36:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/12529012/60003

[commit-bot: I haz the power, 2013-04-05 07:43:08]

Change committed as 192525

[tkent, 2013-04-05 09:52:07]

On 2013/04/05 07:43:08, I haz the power (commit-bot) wrote:
> Change committed as 192525

This caused a use-after-free in a test.  Reverting.


http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20...

[ RUN      ] WidgetTest.TestWidgetDeletedInOnMousePressed
Xlib:  extension "RANDR" missing on display ":9.0".
=================================================================
==5643== ERROR: AddressSanitizer: heap-use-after-free on address 0x603600008e78
at pc 0xbfbeb6 bp 0x7fffcafee5b0 sp 0x7fffcafee5a8
READ of size 8 at 0x603600008e78 thread T0
    #0 0xbfbeb5 in views::internal::RootView::OnMousePressed(ui::MouseEvent
const&)
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/views/widget/root_view.cc:445:0
    #1 0xc134b1 in views::Widget::OnMouseEvent(ui::MouseEvent*)
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/views/widget/widget.cc:1203:0
    #2 0x980c79 in views::(anonymous
namespace)::WidgetTest_TestWidgetDeletedInOnMousePressed_Test::TestBody()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/views/widget/widget_unittest.cc:1748:0
    #3 0xd2bba7 in testing::Test::Run()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:2067:0
    #4 0xd2d0b4 in testing::TestInfo::Run()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:2244:0
    #5 0xd2de9c in testing::TestCase::Run()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:2351:0
    #6 0xd3a850 in testing::internal::UnitTestImpl::RunAllTests()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:4177:0
    #7 0xd39e10 in impl
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:2051:0
    #8 0xd39e10 in testing::UnitTest::Run()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:3810:0
    #9 0x9b7079 in base::TestSuite::Run()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../base/test/test_suite.cc:167:0
    #10 0x9b67a8 in main
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/views/run_all_unittests.cc:38:0
    #11 0x7f08d62bac4c in __libc_start_main
/build/buildd/eglibc-2.11.1/csu/libc-start.c:226
    #12 0x43ecb4 in _start ??:0
0x603600008e78 is located 440 bytes inside of 568-byte region
[0x603600008cc0,0x603600008ef8)
freed by thread T0 here:
    #0 0x433592 in operator delete(void*) ??:0
    #1 0xc0afac in operator()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../base/memory/scoped_ptr.h:138:0
    #2 0xc0afac in reset
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../base/memory/scoped_ptr.h:247:0
    #3 0xc0afac in reset
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../base/memory/scoped_ptr.h:368:0
    #4 0xc0afac in DestroyRootView
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/views/widget/widget.cc:1362:0
    #5 0xc0afac in views::Widget::~Widget()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/views/widget/widget.cc:257:0
    #6 0xc0ac5d in views::Widget::~Widget()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/views/widget/widget.cc:251:0
    #7 0x980dcd in views::(anonymous
namespace)::WidgetDeleterView::OnMousePressed(ui::MouseEvent const&)
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/views/widget/widget_unittest.cc:1725:0
    #8 0xbeaf6c in views::View::ProcessMousePressed(ui::MouseEvent const&)
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/views/view.cc:2077:0
    #9 0xbeaaa2 in views::View::OnMouseEvent(ui::MouseEvent*)
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/views/view.cc:941:0
    #10 0xc7e217 in ui::EventTarget::OnEvent(ui::Event*)
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/base/events/event_target.cc:52:0
    #11 0xc7b55e in DispatchEventToSingleHandler
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/base/events/event_dispatcher.cc:151:0
    #12 0xc7b55e in DispatchEvent
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/base/events/event_dispatcher.cc:144:0
    #13 0xc7b55e in ui::EventDispatcher::ProcessEvent(ui::EventTarget*,
ui::Event*)
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/base/events/event_dispatcher.cc:93:0
    #14 0xc7b12c in ui::EventDispatcherDelegate::DispatchEvent(ui::EventTarget*,
ui::Event*)
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/base/events/event_dispatcher.cc:48:0
    #15 0xbfbb22 in DispatchEventToTarget
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/views/widget/root_view.cc:675:0
    #16 0xbfbb22 in views::internal::RootView::OnMousePressed(ui::MouseEvent
const&)
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/views/widget/root_view.cc:436:0
    #17 0xc134b1 in views::Widget::OnMouseEvent(ui::MouseEvent*)
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/views/widget/widget.cc:1203:0
    #18 0x980c79 in views::(anonymous
namespace)::WidgetTest_TestWidgetDeletedInOnMousePressed_Test::TestBody()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/views/widget/widget_unittest.cc:1748:0
    #19 0xd2bba7 in testing::Test::Run()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:2067:0
    #20 0xd2d0b4 in testing::TestInfo::Run()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:2244:0
    #21 0xd2de9c in testing::TestCase::Run()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:2351:0
    #22 0xd3a850 in testing::internal::UnitTestImpl::RunAllTests()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:4177:0
    #23 0xd39e10 in impl
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:2051:0
    #24 0xd39e10 in testing::UnitTest::Run()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:3810:0
    #25 0x9b7079 in base::TestSuite::Run()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../base/test/test_suite.cc:167:0
    #26 0x9b67a8 in main
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/views/run_all_unittests.cc:38:0
    #27 0x7f08d62bac4c in __libc_start_main
/build/buildd/eglibc-2.11.1/csu/libc-start.c:226
previously allocated by thread T0 here:
    #0 0x4333d2 in operator new(unsigned long) ??:0
    #1 0xc14f53 in views::Widget::CreateRootView()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/views/widget/widget.cc:1357:0
    #2 0xc0bba3 in views::Widget::Init(views::Widget::InitParams const&)
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/views/widget/widget.cc:405:0
    #3 0x980a76 in views::(anonymous
namespace)::WidgetTest_TestWidgetDeletedInOnMousePressed_Test::TestBody()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/views/widget/widget_unittest.cc:1738:0
    #4 0xd2bba7 in testing::Test::Run()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:2067:0
    #5 0xd2d0b4 in testing::TestInfo::Run()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:2244:0
    #6 0xd2de9c in testing::TestCase::Run()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:2351:0
    #7 0xd3a850 in testing::internal::UnitTestImpl::RunAllTests()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:4177:0
    #8 0xd39e10 in impl
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:2051:0
    #9 0xd39e10 in testing::UnitTest::Run()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:3810:0
    #10 0x9b7079 in base::TestSuite::Run()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../base/test/test_suite.cc:167:0
    #11 0x9b67a8 in main
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/views/run_all_unittests.cc:38:0
    #12 0x7f08d62bac4c in __libc_start_main
/build/buildd/eglibc-2.11.1/csu/libc-start.c:226

[varunjain, 2013-04-05 17:19:10]

@sky: made some more changes to fix build failures. PTAL.

[sky, 2013-04-05 17:23:37]

https://chromiumcodereview.appspot.com/12529012/diff/84003/chrome/browser/ui/...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc (right):

https://chromiumcodereview.appspot.com/12529012/diff/84003/chrome/browser/ui/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc:85:
base::TimeDelta::FromMilliseconds(250));\
This is fragile, and it adds more time to tests. Could you instead make it so
tests don't use a delay.

https://chromiumcodereview.appspot.com/12529012/diff/84003/ui/views/widget/wi...
File ui/views/widget/widget.h (right):

https://chromiumcodereview.appspot.com/12529012/diff/84003/ui/views/widget/wi...
ui/views/widget/widget.h:855: class WidgetDeletionObserver : public
WidgetObserver {
Move this (and implementation) into its own file.

https://chromiumcodereview.appspot.com/12529012/diff/84003/ui/views/widget/wi...
ui/views/widget/widget.h:858: ~WidgetDeletionObserver();
virtual

https://chromiumcodereview.appspot.com/12529012/diff/84003/ui/views/widget/wi...
ui/views/widget/widget.h:861: bool IsWidgetAlive();
nit: inline this.

https://chromiumcodereview.appspot.com/12529012/diff/84003/ui/views/widget/wi...
ui/views/widget/widget.h:868: Widget* widget_;
nit: newline between 867/868.

[varunjain, 2013-04-05 18:01:44]

https://chromiumcodereview.appspot.com/12529012/diff/84003/chrome/browser/ui/...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc (right):

https://chromiumcodereview.appspot.com/12529012/diff/84003/chrome/browser/ui/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc:85:
base::TimeDelta::FromMilliseconds(250));\
On 2013/04/05 17:23:37, sky wrote:
> This is fragile, and it adds more time to tests. Could you instead make it so
> tests don't use a delay.

Done.

https://chromiumcodereview.appspot.com/12529012/diff/84003/ui/views/widget/wi...
File ui/views/widget/widget.h (right):

https://chromiumcodereview.appspot.com/12529012/diff/84003/ui/views/widget/wi...
ui/views/widget/widget.h:855: class WidgetDeletionObserver : public
WidgetObserver {
On 2013/04/05 17:23:37, sky wrote:
> Move this (and implementation) into its own file.

Done.

https://chromiumcodereview.appspot.com/12529012/diff/84003/ui/views/widget/wi...
ui/views/widget/widget.h:858: ~WidgetDeletionObserver();
On 2013/04/05 17:23:37, sky wrote:
> virtual

Done.

https://chromiumcodereview.appspot.com/12529012/diff/84003/ui/views/widget/wi...
ui/views/widget/widget.h:861: bool IsWidgetAlive();
On 2013/04/05 17:23:37, sky wrote:
> nit: inline this.

Done.

https://chromiumcodereview.appspot.com/12529012/diff/84003/ui/views/widget/wi...
ui/views/widget/widget.h:868: Widget* widget_;
On 2013/04/05 17:23:37, sky wrote:
> nit: newline between 867/868.

Done.

[sky, 2013-04-05 19:41:36]

LGTM

https://chromiumcodereview.appspot.com/12529012/diff/99001/ui/views/controls/...
File ui/views/controls/menu/menu_controller.cc (right):

https://chromiumcodereview.appspot.com/12529012/diff/99001/ui/views/controls/...
ui/views/controls/menu/menu_controller.cc:63: static int
g_context_menu_selection_hold_time_ms = 200;
This isn't global. I think you want context_menu_selection_hod_time_ms.

https://chromiumcodereview.appspot.com/12529012/diff/99001/ui/views/widget/wi...
File ui/views/widget/widget_deletion_observer.h (right):

https://chromiumcodereview.appspot.com/12529012/diff/99001/ui/views/widget/wi...
ui/views/widget/widget_deletion_observer.h:22: bool IsWidgetAlive() { return
widget_; }
I suspect you may need widget_ != NULL for windows.

[varunjain, 2013-04-05 21:58:42]

https://chromiumcodereview.appspot.com/12529012/diff/99001/ui/views/controls/...
File ui/views/controls/menu/menu_controller.cc (right):

https://chromiumcodereview.appspot.com/12529012/diff/99001/ui/views/controls/...
ui/views/controls/menu/menu_controller.cc:63: static int
g_context_menu_selection_hold_time_ms = 200;
On 2013/04/05 19:41:36, sky wrote:
> This isn't global. I think you want context_menu_selection_hod_time_ms.

Done.

https://chromiumcodereview.appspot.com/12529012/diff/99001/ui/views/widget/wi...
File ui/views/widget/widget_deletion_observer.h (right):

https://chromiumcodereview.appspot.com/12529012/diff/99001/ui/views/widget/wi...
ui/views/widget/widget_deletion_observer.h:22: bool IsWidgetAlive() { return
widget_; }
On 2013/04/05 19:41:36, sky wrote:
> I suspect you may need widget_ != NULL for windows.

Done.

[commit-bot: I haz the power, 2013-04-05 21:58:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/12529012/61018

[commit-bot: I haz the power, 2013-04-06 01:46:05]

Change committed as 192688