Rebase the MessageBubble on the new views bubble.

Rebase the MessageBubble on the new views bubble.

Views screen locker password errors and the 3G promo bubble looks good; except:
I filed crosbug.com/23324 for ScreenLockerViews MouseEventRelay.

BUG=98322
TEST=MessageBubbles work as before (screen locker password errors, 3G MobileDataPromo).

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

[msw, 2011-11-19 00:02:06]

Hey guys! I need help testing this code on a CrOS device/VM.

[Emmanuel Saint-loubert-Bié, 2011-11-19 00:09:51]

Hi Michael I will add it tio my next run starting in a few minutes :-)

[alicet1, 2011-11-20 01:46:22]

just looking at bubbles api move only, probably dmitry and saintlou can answer
the chromeos side of changes.

I think you will also need that wm_ipc window type setting that setting level
bubble has. I' ll do the move.

thanx,
alice

http://codereview.chromium.org/8557005/diff/6007/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/message_bubble.cc (left):

http://codereview.chromium.org/8557005/diff/6007/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/message_bubble.cc:201: #if !defined(TOUCH_UI) &&
defined(TOOLKIT_USES_GTK)
is there anything on touch that you need to exclude also in the new code?

http://codereview.chromium.org/8557005/diff/6007/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/message_bubble.cc (right):

http://codereview.chromium.org/8557005/diff/6007/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/message_bubble.cc:63:
views::BubbleDelegateView::CreateBubble(bubble);
I think this bubble wants to be "focusless", check setting level bubble.

I think bubbles need type_bubble to draw the frame. set_use_focusless will give
a gtk popup.

[Dmitry Polukhin, 2011-11-21 06:20:27]

+ Nikita

I remembered one more use case for the bubble - 3G promo. These changes really
need to be tested carefully on real device with and without Aura. But it should
be very easy to get real device if you are in Chrome team.

[msw, 2011-11-21 20:34:43]

Thanks, Dmitry. I have already converted the 3G Promo bubble with my changes to
chrome/browser/chromeos/status/network_menu_button.cc|h

We just tested the password bubble on Alice's device, and I'm going to tweak my
build to show the 3G promo build on some other trigger as needed for testing. Do
you have any tips to trigger that bubble normally? Should it show up any time we
disconnect from wireless and lan and have 3G available?

http://codereview.chromium.org/8557005/diff/6007/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/message_bubble.cc (left):

http://codereview.chromium.org/8557005/diff/6007/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/message_bubble.cc:201: #if !defined(TOUCH_UI) &&
defined(TOOLKIT_USES_GTK)
On 2011/11/20 01:46:22, alicet1 wrote:
> is there anything on touch that you need to exclude also in the new code?

I'm not sure the new bubble needs this mouse button handling hack.
I hope to test this manually once we get a build running on a device.

http://codereview.chromium.org/8557005/diff/6007/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/message_bubble.cc (right):

http://codereview.chromium.org/8557005/diff/6007/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/message_bubble.cc:63:
views::BubbleDelegateView::CreateBubble(bubble);
On 2011/11/20 01:46:22, alicet1 wrote:
> I think this bubble wants to be "focusless", check setting level bubble.
> 
> I think bubbles need type_bubble to draw the frame. set_use_focusless will
give
> a gtk popup.

Done.

I also added some temp code for chromeos::WM_IPC_WINDOW_CHROME_INFO_BUBBLE.
I'll merge when you land http://codereview.chromium.org/8604012/.

[msw, 2011-11-21 23:58:56]

Hey Nikita, please review and help us trigger these bubbles for testing.

[msw, 2011-11-22 20:33:27]

+Oshima... Ping! Can someone please review?

[Dmitry Polukhin, 2011-11-23 12:44:34]

http://codereview.chromium.org/8557005/diff/32001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/message_bubble.cc (right):

http://codereview.chromium.org/8557005/diff/32001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/message_bubble.cc:25: const SkColor kColor =
SK_ColorWHITE;
I would put default color into Bubble class as it was Bubble::kBackgroundColor
to have one default constant.

http://codereview.chromium.org/8557005/diff/32001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/message_bubble.h (right):

http://codereview.chromium.org/8557005/diff/32001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/message_bubble.h:45: static void
ShowBubble(MessageBubble* bubble);
I think there is no sense to make it static and pass this pointer explicit.
Nonvirtual member function without arguments is better.

[Nikita (slow), 2011-11-23 13:30:11]

http://codereview.chromium.org/8557005/diff/32001/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_menu_button.cc (right):

http://codereview.chromium.org/8557005/diff/32001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu_button.cc:247: if
(!mobile_data_bubble_ || mobile_data_bubble_->GetWidget() != widget)
I've checked this patch on Chrome OS.
OnWidgetClosing() is not called when bubble is closed so that leads to a crash
in ~NetworkMenuButton() as mobile_data_bubble_ is not NULL.

I've specifically executed chrome under GDB to check that OnWidgetClosing() is
not called.

[msw, 2011-11-29 00:11:06]

Thanks! comments addressed, please take another look.

http://codereview.chromium.org/8557005/diff/32001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/message_bubble.cc (right):

http://codereview.chromium.org/8557005/diff/32001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/message_bubble.cc:25: const SkColor kColor =
SK_ColorWHITE;
On 2011/11/23 12:44:35, Dmitry Polukhin wrote:
> I would put default color into Bubble class as it was Bubble::kBackgroundColor
> to have one default constant.

Done (moved default constant to BubbleDelegateView class for now, I'll hook up
the NativeTheme/color_helper and cleanup the ctors in a follow-up CL for
crbug.com/105023).

http://codereview.chromium.org/8557005/diff/32001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/message_bubble.h (right):

http://codereview.chromium.org/8557005/diff/32001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/message_bubble.h:45: static void
ShowBubble(MessageBubble* bubble);
On 2011/11/23 12:44:35, Dmitry Polukhin wrote:
> I think there is no sense to make it static and pass this pointer explicit.
> Nonvirtual member function without arguments is better.

Removed, message bubble call sites now call
browser::CreateViewsBubbleAboveLockScreen and views::BubbleDelegateView::Show
directly, as other bubbles do.

http://codereview.chromium.org/8557005/diff/32001/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_menu_button.cc (right):

http://codereview.chromium.org/8557005/diff/32001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu_button.cc:247: if
(!mobile_data_bubble_ || mobile_data_bubble_->GetWidget() != widget)
On 2011/11/23 13:30:12, Nikita Kostylev wrote:
> I've checked this patch on Chrome OS.
> OnWidgetClosing() is not called when bubble is closed so that leads to a crash
> in ~NetworkMenuButton() as mobile_data_bubble_ is not NULL.
> 
> I've specifically executed chrome under GDB to check that OnWidgetClosing() is
> not called.

Fixed (added the observer on bubble construction). Thanks for the catch!

[Dmitry Polukhin, 2011-11-29 12:54:53]

LGTM

http://codereview.chromium.org/8557005/diff/41005/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screen_locker_views.h (right):

http://codereview.chromium.org/8557005/diff/41005/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screen_locker_views.h:14: #include
"ui/base/gtk/gtk_signal.h"
Do we really need this include especially in header?

[Nikita (slow), 2011-11-29 13:08:37]

lgtm

http://codereview.chromium.org/8557005/diff/41005/chrome/browser/ui/views/win...
File chrome/browser/ui/views/window.h (right):

http://codereview.chromium.org/8557005/diff/41005/chrome/browser/ui/views/win...
chrome/browser/ui/views/window.h:31: views::Widget*
CreateViewsBubbleAboveLockScreen(
In general I don't think that every bubble should be created on top of lock
screen. Only those that are created from the lock screen should be placed so.
But you may fix that in upcoming CL if that's needed.

[alicet1, 2011-11-29 19:29:35]

lgtm

[msw, 2011-11-29 19:49:52]

Responses inline; thanks!

http://codereview.chromium.org/8557005/diff/41005/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screen_locker_views.h (right):

http://codereview.chromium.org/8557005/diff/41005/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screen_locker_views.h:14: #include
"ui/base/gtk/gtk_signal.h"
On 2011/11/29 12:54:53, Dmitry Polukhin wrote:
> Do we really need this include especially in header?

Sadly yes; it's needed for CHROMEGTK_CALLBACK_1 (previously pulled in via
MessageBubble's crummy header). Thankfully ScreenLockerViews is headed for the
recycle bin as soon as WebUIScreenLocker is polished and ready (I heard perhaps
in time for M17).

http://codereview.chromium.org/8557005/diff/41005/chrome/browser/ui/views/win...
File chrome/browser/ui/views/window.h (right):

http://codereview.chromium.org/8557005/diff/41005/chrome/browser/ui/views/win...
chrome/browser/ui/views/window.h:31: views::Widget*
CreateViewsBubbleAboveLockScreen(
On 2011/11/29 13:08:37, Nikita Kostylev wrote:
> In general I don't think that every bubble should be created on top of lock
> screen. Only those that are created from the lock screen should be placed so.
> But you may fix that in upcoming CL if that's needed.

Most bubbles use |CreateViewsBubble|, not |CreateViewsBubbleAboveLockScreen|.
MessageBubble is an exception for it's password errors shown on
ScreenLockerViews. Once that's deprecated in favor of the WebUIScreenLocker, the
3G Promo will be the only MessageBubble, and that should warrant some extra
cleanup. More generally, this isn't needed for aura and should eventually be
deprecated along with the legacy WM IPC code it uses. For the time being,
however, this seems unavoidable.

[commit-bot: I haz the power, 2011-11-29 21:26:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/8557005/40020

[commit-bot: I haz the power, 2011-11-29 21:26:53]

Can't apply patch for file chrome/browser/chromeos/login/screen_lock_view.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/chromeos/login/screen_lock_view.cc
Hunk #1 FAILED at 23.
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/login/screen_lock_view.cc.rej

[msw, 2011-11-29 23:55:11]

+Ben for OWNERS: ui/views/bubble/bubble_delegate.*
+Dave/Steven for OWNERS: chrome/browser/chromeos/status/network_menu_button.*

[Ben Goodger (Google), 2011-11-30 15:45:42]

LGTM for UI.

[commit-bot: I haz the power, 2011-11-30 19:55:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/8557005/46002

[commit-bot: I haz the power, 2011-11-30 19:56:08]

Presubmit check for 8557005-46002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/chromeos/status/network_menu_button.cc,chrome/browser/chromeos/status/network_menu_button.h

Presubmit checks took 4.3s to calculate.

[DaveMoore, 2011-11-30 20:10:45]

lgtm for network_menu*

[commit-bot: I haz the power, 2011-11-30 20:14:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/8557005/46002

[stevenjb, 2011-11-30 20:22:15]

status changes LGTN

[stevenjb, 2011-11-30 20:22:33]

LGTM even.

[commit-bot: I haz the power, 2011-11-30 22:43:12]

Try job failure for 8557005-46002 (previous was lost) (retry) on win_rel for
step "compile" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...