Replace ChromiumOS BalloonView Menu2 with MenuItemView.

Replace ChromiumOS BalloonView Menu2 with MenuItemView.

BUG=chromium-os:13887
TEST=none


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

[rhashimoto, 2011-06-10 17:13:14]

Hi Oshima-san -

I'm not sure how to test this change.  I have seen what I thought were
notifications (slide-up windows at the bottom right for loss of network, I
think) but I didn't see an Options button on them and they didn't seem to have a
context menu.

How should I generate a notification for testing?

Thanks,
Roy

[oshima, 2011-06-10 23:19:04]

LGTM

menu button appears only when you hover mouse over on notification. you can use
http://www.corp.google.com/~oshima/notification/notify.html to test.

[rhashimoto, 2011-06-13 22:21:22]

Hi Oshima-san -

Thanks for the notifications test page!  So my menu code crashes because the
NativeWindow argument to MenuItemView::RunMenu()

source->GetWidget()->GetNativeWindow()

is NULL.  If I use instead

source->GetWidget()->GetTopLevelWidget()->GetNativeWindow()

then it is not NULL, but in a debug build I get a GLib assertion:

[3472:3472:690339174:FATAL:browser_main.cc(962)] GLib-GObject: g_object_ref:
assertion `G_IS_OBJECT (object)' failed

Unfortunately the stack trace at the assertion is confused.  It happens
somewhere below the call to gtk_grab_add() in
views::NativeWidgetGtk::SetMouseCapture() to show the menu.

With a release build the menu seems to operate as expected.  Do you have any
insight on what is happening or suggestions on how to debug it?

Thanks,
Roy

[rhashimoto, 2011-06-14 20:25:59]

Patch 2 uses top level widget and fixes build on TOT.

Roy

[oshima, 2011-06-18 00:37:41]

I couldn't reproduce the issue you had. Can you try again?

- oshima

On Tue, Jun 14, 2011 at 1:25 PM, <rhashimoto@chromium.org> wrote:

> Patch 2 uses top level widget and fixes build on TOT.
>
>
> Roy
>
>
http://codereview.chromium.**org/7003121/<http://codereview.chromium.org/7003...
>

[rhashimoto, 2011-06-20 16:58:55]

On 2011/06/18 00:37:41, oshima wrote:
> I couldn't reproduce the issue you had. Can you try again?

I can still reproduce the crash on TOT on Cr-48.  It does require a debug build.

Roy

[rhashimoto, 2011-06-28 16:57:07]

Hi Oshima-san -

This CL still produces a crash for me on Cr-48 ToT when built debug (release
build seems to work).  Do you have any advice on analyzing this?

Thanks,
Roy

[oshima, 2011-06-28 17:23:59]

Can you run debug build on desktop and see if you can reproduce it? You can
use the same test page that I sent.
(It behaves a bit weird under non chromeos wm, but you should be able to
interact with)

- oshima

On Tue, Jun 28, 2011 at 9:57 AM, <rhashimoto@chromium.org> wrote:

> Hi Oshima-san -
>
> This CL still produces a crash for me on Cr-48 ToT when built debug
> (release
> build seems to work).  Do you have any advice on analyzing this?
>
> Thanks,
> Roy
>
>
http://codereview.chromium.**org/7003121/<http://codereview.chromium.org/7003...
>

[rhashimoto, 2011-06-28 20:29:16]

On 2011/06/28 17:23:59, oshima wrote:
> Can you run debug build on desktop and see if you can reproduce it? You can
> use the same test page that I sent.
> (It behaves a bit weird under non chromeos wm, but you should be able to
> interact with)

It crashes on linux with a reasonable stack trace:

#0  base::debug::BreakDebugger () at base/debug/debugger_posix.cc:182
#1  0x00007ffff2b59fc0 in logging::LogMessage::~LogMessage (
    this=0x7fffffff85c0, __in_chrg=<value optimized out>)
    at base/logging.cc:613
#2  0x00007ffff1c7bf55 in (anonymous namespace)::GLibLogHandler(const gchar *,
<anonymous enum>, const gchar *, gpointer) (
    log_domain=0x7fffee0f87f2 "GLib-GObject", log_level=G_LOG_LEVEL_CRITICAL, 
    message=0x7fffda982dc0 "g_object_ref: assertion `G_IS_OBJECT (object)'
failed", userdata=0x0) at chrome/browser/browser_main.cc:998
#3  0x00007fffeda25fb9 in IA__g_logv (log_domain=<value optimized out>, 
    log_level=<value optimized out>, format=<value optimized out>, 
    args1=0x7fffffff9240)
    at /tmp/glib2.0.0xzuTt/glib2.0-2.24.1/glib/gmessages.c:519
#4  0x00007fffeda263d3 in IA__g_log (log_domain=0x7fffffff7ed0 "", 
    log_level=75, format=0x0)
    at /tmp/glib2.0.0xzuTt/glib2.0-2.24.1/glib/gmessages.c:569
#5  0x00007fffee0d36f2 in IA__g_object_ref (_object=<value optimized out>)
    at /tmp/glib2.0.0xzuTt/glib2.0-2.24.1/gobject/gobject.c:2396
#6  0x00007fffefcc222b in synth_crossing (widget=0x0, 
    type=<value optimized out>, window=0x7fffdb8c5060, 
    mode=GDK_CROSSING_GTK_GRAB, detail=GDK_NOTIFY_NONLINEAR_VIRTUAL)
    at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:8940
#7  0x00007fffefcc25f8 in _gtk_widget_synthesize_crossing (
    from=<value optimized out>, to=<value optimized out>, 
    mode=GDK_CROSSING_GTK_GRAB)
    at /build/buildd/gtk+2.0-2.20.1/gtk/gtkwidget.c:9150
#8  0x00007fffefba40fd in gtk_grab_notify_foreach (child=0x7fffdb8c2640, 
    data=<value optimized out>)
    at /build/buildd/gtk+2.0-2.20.1/gtk/gtkmain.c:1763
#9  0x00007fffefb630b8 in gtk_fixed_forall (container=<value optimized out>, 
    include_internals=<value optimized out>, 
    callback=0x7fffefba3fc0 <gtk_grab_notify_foreach>, 
    callback_data=0x7fffffff9500)
    at /build/buildd/gtk+2.0-2.20.1/gtk/gtkfixed.c:452
#10 0x00007fffefba4112 in gtk_grab_notify_foreach (child=0x7fffda98f740, 
    data=<value optimized out>)
    at /build/buildd/gtk+2.0-2.20.1/gtk/gtkmain.c:1756
#11 0x00007fffefb630b8 in gtk_fixed_forall (container=<value optimized out>, 
    include_internals=<value optimized out>, 
---Type <return> to continue, or q <return> to quit---
    callback=0x7fffefba3fc0 <gtk_grab_notify_foreach>, 
    callback_data=0x7fffffff9500)
    at /build/buildd/gtk+2.0-2.20.1/gtk/gtkfixed.c:452
#12 0x00007fffefba4112 in gtk_grab_notify_foreach (child=0x7fffda98f0c0, 
    data=<value optimized out>)
    at /build/buildd/gtk+2.0-2.20.1/gtk/gtkmain.c:1756
#13 0x00007fffefba4112 in gtk_grab_notify_foreach (child=0x7fffdb8c56c0, 
    data=<value optimized out>)
    at /build/buildd/gtk+2.0-2.20.1/gtk/gtkmain.c:1756
#14 0x00007fffefba41fb in gtk_grab_notify (group=0x7fffde5cb400, 
    old_grab_widget=<value optimized out>, 
    new_grab_widget=<value optimized out>, from_grab=<value optimized out>)
    at /build/buildd/gtk+2.0-2.20.1/gtk/gtkmain.c:1815
#15 0x00007ffff4d8bbaa in views::NativeWidgetGtk::SetMouseCapture (
    this=0x7fffdb15fe70) at views/widget/native_widget_gtk.cc:877
#16 0x00007ffff4dad083 in views::MenuHostGtk::StartCapturing (
    this=0x7fffdb15fe70) at views/controls/menu/menu_host_gtk.cc:46
#17 0x00007ffff4dac531 in views::MenuHost::ShowMenuHost (this=0x7fffde50b960, 
    do_capture=true) at views/controls/menu/menu_host.cc:55
#18 0x00007ffff4dac4a6 in views::MenuHost::InitMenuHost (this=0x7fffde50b960, 
    parent=0x7fffdb8c56c0, bounds=..., contents_view=0x7fffdb440f00, 
    do_capture=true) at views/controls/menu/menu_host.cc:45
[...]

The window argument passed to synth_crossing() (#6) seems to be empty:

(gdb) print *window
$3 = {parent_instance = {g_type_instance = {g_class = 0x0}, ref_count = 0, 
    qdata = 0x0}}

When synth_crossing() refs the window to create an event, this triggers the
debug assertion.  Perhaps this is a GTK bug?

[rhashimoto, 2011-06-28 22:41:16]

On 2011/06/28 20:29:16, rhashimoto wrote:
> The window argument passed to synth_crossing() (#6) seems to be empty:
> 
> (gdb) print *window
> $3 = {parent_instance = {g_type_instance = {g_class = 0x0}, ref_count = 0, 
>     qdata = 0x0}}
> 
> When synth_crossing() refs the window to create an event, this triggers the
> debug assertion.  Perhaps this is a GTK bug?

Seems to be an issue with feedback from the fake events generated by the grab. 
I think those events are to move the focus from wherever the pointer was to the
the widget that has the grab.  NotificationPanel::OnMouseLeave is getting the
fake event and it eventually causes another queued window object to be
destroyed.  When that destroyed window gets to the front of the queue it
triggers the assertion.

[oshima, 2011-06-29 01:12:05]

I still couldn't reproduce this. I think i'm still missing something.
let's work together tomorrow and nail it down.

- oshima

On Tue, Jun 28, 2011 at 3:41 PM, <rhashimoto@chromium.org> wrote:

> On 2011/06/28 20:29:16, rhashimoto wrote:
>
>> The window argument passed to synth_crossing() (#6) seems to be empty:
>>
>
>  (gdb) print *window
>> $3 = {parent_instance = {g_type_instance = {g_class = 0x0}, ref_count = 0,
>>     qdata = 0x0}}
>>
>
>  When synth_crossing() refs the window to create an event, this triggers
>> the
>> debug assertion.  Perhaps this is a GTK bug?
>>
>
> Seems to be an issue with feedback from the fake events generated by the
> grab.
> I think those events are to move the focus from wherever the pointer was to
> the
> the widget that has the grab.  NotificationPanel::**OnMouseLeave is
> getting the
> fake event and it eventually causes another queued window object to be
> destroyed.  When that destroyed window gets to the front of the queue it
> triggers the assertion.
>
>
>
http://codereview.chromium.**org/7003121/<http://codereview.chromium.org/7003...
>

[oshima, 2011-07-06 18:10:36]

http://codereview.chromium.org/7003121/diff/4001/chrome/browser/chromeos/noti...
File chrome/browser/chromeos/notifications/balloon_view.cc (right):

http://codereview.chromium.org/7003121/diff/4001/chrome/browser/chromeos/noti...
chrome/browser/chromeos/notifications/balloon_view.cc:123:
menu_model_adapter.BuildMenu(&menu);
I felt this API a bit wired because you create menu by passing adapter, but then
call adapter.BuildMenu with the menu. I guess this is necessary to migrate from
menu2 to views menu, but I think we can clean this up once migration is
completed. do you have any plan to clean this up?

[rhashimoto, 2011-07-06 18:27:33]

http://codereview.chromium.org/7003121/diff/4001/chrome/browser/chromeos/noti...
File chrome/browser/chromeos/notifications/balloon_view.cc (right):

http://codereview.chromium.org/7003121/diff/4001/chrome/browser/chromeos/noti...
chrome/browser/chromeos/notifications/balloon_view.cc:123:
menu_model_adapter.BuildMenu(&menu);
On 2011/07/06 18:10:36, oshima wrote:
> I felt this API a bit wired because you create menu by passing adapter, but
then
> call adapter.BuildMenu with the menu. I guess this is necessary to migrate
from
> menu2 to views menu, but I think we can clean this up once migration is
> completed. do you have any plan to clean this up?

I agree that it is a little strange.  The model serves two purposes - defining
the structure of the menu (items and submenus) and defining the item behavior
(enabled/disabled, font, execution, etc.).  MenuItemView only calls its delegate
to get item behavior and publishes an API to change the structure.  It's
probably better to pull at least the menu structuring directly into
MenuItemView.

I don't have any immediate plans to do this but I can put it on my list. 
pkasting has also requested tweaks to the MenuItemView API.

[rhashimoto, 2011-07-07 17:42:01]

OK to commit this?  I know there was an LGTM, but there was one change and a lot
of discussion since then so I just want to be sure.

Thanks,
Roy

[oshima, 2011-07-07 22:06:34]

yes OK to commit.

- oshima

On Thu, Jul 7, 2011 at 10:42 AM, <rhashimoto@chromium.org> wrote:

> OK to commit this?  I know there was an LGTM, but there was one change and
> a lot
> of discussion since then so I just want to be sure.
>
> Thanks,
> Roy
>
>
>
http://codereview.chromium.**org/7003121/<http://codereview.chromium.org/7003...
>

[commit-bot: I haz the power, 2011-07-08 16:44:04]

Change committed as 91862