global error bubble view move to views/bubble api.

global error bubble view move to views/bubble api.

BUG=98323
TEST=None


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

[alicet1, 2011-11-16 00:53:47]

mike can you take a pass?

thanx,
alice

[msw, 2011-11-16 01:19:44]

Looks pretty decent so far, I suspect you'll need to update all the clients in
this CL too.

http://codereview.chromium.org/8570014/diff/2001/chrome/browser/ui/views/glob...
File chrome/browser/ui/views/global_error_bubble_view.cc (right):

http://codereview.chromium.org/8570014/diff/2001/chrome/browser/ui/views/glob...
chrome/browser/ui/views/global_error_bubble_view.cc:58:
message_label->SizeToFit(kBubbleViewWidth);
It looks like that const is the max bubble width.
This should be okay, but please test shorter errors, just in case.

http://codereview.chromium.org/8570014/diff/2001/chrome/browser/ui/views/glob...
chrome/browser/ui/views/global_error_bubble_view.cc:76: // Add Top insets.
This comment doesn't quite capture your intent...

http://codereview.chromium.org/8570014/diff/2001/chrome/browser/ui/views/glob...
chrome/browser/ui/views/global_error_bubble_view.cc:77: layout->SetInsets(0, 0,
2, -6);
Make these local consts.

http://codereview.chromium.org/8570014/diff/2001/chrome/browser/ui/views/glob...
chrome/browser/ui/views/global_error_bubble_view.cc:151: // TODO(sail): Enabling
fade causes the window to be disabled for some
Check if Fade works now.

http://codereview.chromium.org/8570014/diff/2001/chrome/browser/ui/views/glob...
chrome/browser/ui/views/global_error_bubble_view.cc:153:
views::BubbleDelegateView::CreateBubble(bubble_view)->Show();
You need to call Show on the delegate instead of the widget for the bubble to
work correctly on windows (for now... it's the whole two-widget thing...)

[msw, 2011-11-16 01:29:25]

Oh, I see, ShowBubbleView is the only client constructing these delegates, so
disregard my general comment.

[alicet1, 2011-11-17 00:31:42]

checked on windows, offset and fade in looks ok. mike for another look over, ben
for approval.

thanx,
alice

http://codereview.chromium.org/8570014/diff/2001/chrome/browser/ui/views/glob...
File chrome/browser/ui/views/global_error_bubble_view.cc (right):

http://codereview.chromium.org/8570014/diff/2001/chrome/browser/ui/views/glob...
chrome/browser/ui/views/global_error_bubble_view.cc:58:
message_label->SizeToFit(kBubbleViewWidth);
On 2011/11/16 01:19:44, msw wrote:
> It looks like that const is the max bubble width.
> This should be okay, but please test shorter errors, just in case.

seems to be a min, actually, if you look at the original GetPreferredSize?

http://codereview.chromium.org/8570014/diff/2001/chrome/browser/ui/views/glob...
chrome/browser/ui/views/global_error_bubble_view.cc:58:
message_label->SizeToFit(kBubbleViewWidth);
On 2011/11/16 01:19:44, msw wrote:
> It looks like that const is the max bubble width.
> This should be okay, but please test shorter errors, just in case.

Done.

http://codereview.chromium.org/8570014/diff/2001/chrome/browser/ui/views/glob...
chrome/browser/ui/views/global_error_bubble_view.cc:76: // Add Top insets.
On 2011/11/16 01:19:44, msw wrote:
> This comment doesn't quite capture your intent...

Done.

http://codereview.chromium.org/8570014/diff/2001/chrome/browser/ui/views/glob...
chrome/browser/ui/views/global_error_bubble_view.cc:76: // Add Top insets.
On 2011/11/16 01:19:44, msw wrote:
> This comment doesn't quite capture your intent...

Done.

http://codereview.chromium.org/8570014/diff/2001/chrome/browser/ui/views/glob...
chrome/browser/ui/views/global_error_bubble_view.cc:77: layout->SetInsets(0, 0,
2, -6);
On 2011/11/16 01:19:44, msw wrote:
> Make these local consts.

Done.

http://codereview.chromium.org/8570014/diff/2001/chrome/browser/ui/views/glob...
chrome/browser/ui/views/global_error_bubble_view.cc:77: layout->SetInsets(0, 0,
2, -6);
On 2011/11/16 01:19:44, msw wrote:
> Make these local consts.

Done.

http://codereview.chromium.org/8570014/diff/2001/chrome/browser/ui/views/glob...
chrome/browser/ui/views/global_error_bubble_view.cc:151: // TODO(sail): Enabling
fade causes the window to be disabled for some
On 2011/11/16 01:19:44, msw wrote:
> Check if Fade works now.

fade "works", and by that I mean: the bubble will fade in and be activated, will
close on deactivation or esc.

http://codereview.chromium.org/8570014/diff/2001/chrome/browser/ui/views/glob...
chrome/browser/ui/views/global_error_bubble_view.cc:153:
views::BubbleDelegateView::CreateBubble(bubble_view)->Show();
On 2011/11/16 01:19:44, msw wrote:
> You need to call Show on the delegate instead of the widget for the bubble to
> work correctly on windows (for now... it's the whole two-widget thing...)

oy.

[msw, 2011-11-17 01:00:26]

LGTM with nits; thanks!

http://codereview.chromium.org/8570014/diff/2001/chrome/browser/ui/views/glob...
File chrome/browser/ui/views/global_error_bubble_view.cc (right):

http://codereview.chromium.org/8570014/diff/2001/chrome/browser/ui/views/glob...
chrome/browser/ui/views/global_error_bubble_view.cc:151: // TODO(sail): Enabling
fade causes the window to be disabled for some
On 2011/11/17 00:31:42, alicet1 wrote:
> On 2011/11/16 01:19:44, msw wrote:
> > Check if Fade works now.
> 
> fade "works", and by that I mean: the bubble will fade in and be activated,
will
> close on deactivation or esc.

Great!

http://codereview.chromium.org/8570014/diff/6001/chrome/browser/ui/views/glob...
File chrome/browser/ui/views/global_error_bubble_view.cc (right):

http://codereview.chromium.org/8570014/diff/6001/chrome/browser/ui/views/glob...
chrome/browser/ui/views/global_error_bubble_view.cc:61:
message_label->SizeToFit(kMaxBubbleViewWidth);
Is this a max or min? Your earlier comment indicated that it's a min width.
Can this be combined with the SizeToFit call at the end?

http://codereview.chromium.org/8570014/diff/6001/chrome/browser/ui/views/glob...
File chrome/browser/ui/views/global_error_bubble_view.h (right):

http://codereview.chromium.org/8570014/diff/6001/chrome/browser/ui/views/glob...
chrome/browser/ui/views/global_error_bubble_view.h:16: class Label;
You can remove this forward decl.

[alicet1, 2011-11-17 15:13:46]

thanks mike. 

ben?

thanx,
alice

http://codereview.chromium.org/8570014/diff/6001/chrome/browser/ui/views/glob...
File chrome/browser/ui/views/global_error_bubble_view.cc (right):

http://codereview.chromium.org/8570014/diff/6001/chrome/browser/ui/views/glob...
chrome/browser/ui/views/global_error_bubble_view.cc:61:
message_label->SizeToFit(kMaxBubbleViewWidth);
On 2011/11/17 01:00:27, msw wrote:
> Is this a max or min? Your earlier comment indicated that it's a min width.
> Can this be combined with the SizeToFit call at the end?

This is the preferred max width of the message. if the button is normal, we want
to use this as the max bubble width. if the button is long, the adjustment in
L120 will size the label again.

http://codereview.chromium.org/8570014/diff/6001/chrome/browser/ui/views/glob...
File chrome/browser/ui/views/global_error_bubble_view.h (right):

http://codereview.chromium.org/8570014/diff/6001/chrome/browser/ui/views/glob...
chrome/browser/ui/views/global_error_bubble_view.h:16: class Label;
On 2011/11/17 01:00:27, msw wrote:
> You can remove this forward decl.

Done.

[Ben Goodger (Google), 2011-11-17 20:54:12]

lgtm

[commit-bot: I haz the power, 2011-11-18 01:32:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alicet@chromium.org/8570014/10001

[commit-bot: I haz the power, 2011-11-18 04:05:26]

Change committed as 110646