Support Windows native textfield, combobox, etc. in new bubbles.

Support Windows native textfield, combobox, etc. in new bubbles.
Should allow us to fully convert all bubbles (bookmark, etc.)
Use separate border and contents widgets (like old bubble).
Supports transparency/opacity with fading, etc.
Remove BubbleView remnants; update tests.

BUG=98312
TEST=No views_examples bubble problems.

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

[msw, 2011-10-21 20:52:32]

Hey Ben, I wrote this as a learning exercise, it's not ready for full review,
but I'd like advice. It may be feasible to fully convert bubbles, using native
controls as needed. This eliminates the need to support two versions of the
bookmark (etc.) bubbles, and lets us sunset the old bubble code before pure
views controls are fully baked. Do you think I should pursue this path?

[Ben Goodger (Google), 2011-10-25 15:29:36]

In the sense that you're using a separate border widget to host the multi-alpha
bg, yes this is reasonable (this is what the current code also does).

The border widget should be necessary only on windows.

http://codereview.chromium.org/8368006/diff/1/views/bubble/bubble_delegate.cc
File views/bubble/bubble_delegate.cc (right):

http://codereview.chromium.org/8368006/diff/1/views/bubble/bubble_delegate.cc...
views/bubble/bubble_delegate.cc:57: // Create a widget to host the content with
transparency off.
pull the border widget init into a separate function to make this init easier to
read.

http://codereview.chromium.org/8368006/diff/1/views/bubble/bubble_delegate.h
File views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/8368006/diff/1/views/bubble/bubble_delegate.h#...
views/bubble/bubble_delegate.h:111: };
DISALLOW_COPY_..

[msw, 2011-10-27 02:01:45]

PTAL; thanks!

http://codereview.chromium.org/8368006/diff/1/views/bubble/bubble_delegate.cc
File views/bubble/bubble_delegate.cc (right):

http://codereview.chromium.org/8368006/diff/1/views/bubble/bubble_delegate.cc...
views/bubble/bubble_delegate.cc:57: // Create a widget to host the content with
transparency off.
On 2011/10/25 15:29:36, Ben Goodger (Google) wrote:
> pull the border widget init into a separate function to make this init easier
to
> read.

Done (sort of, let me know what you think).

http://codereview.chromium.org/8368006/diff/1/views/bubble/bubble_delegate.h
File views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/8368006/diff/1/views/bubble/bubble_delegate.h#...
views/bubble/bubble_delegate.h:111: };
On 2011/10/25 15:29:36, Ben Goodger (Google) wrote:
> DISALLOW_COPY_..

Done.

[Ben Goodger (Google), 2011-10-27 15:37:00]

http://codereview.chromium.org/8368006/diff/11001/views/bubble/bubble_delegat...
File views/bubble/bubble_delegate.cc (right):

http://codereview.chromium.org/8368006/diff/11001/views/bubble/bubble_delegat...
views/bubble/bubble_delegate.cc:41: : bubble_(bubble) {}
nit: 4-space indent instead of 2.

http://codereview.chromium.org/8368006/diff/11001/views/bubble/bubble_delegat...
views/bubble/bubble_delegate.cc:106: Widget* border_widget =
CreateBorderWidget(bubble_delegate, parent_widget);
Move the border initialization into a separate function to keep the reading flow
here smooth.

http://codereview.chromium.org/8368006/diff/11001/views/bubble/bubble_delegat...
views/bubble/bubble_delegate.cc:162: GetWindowLong(hwnd, GWL_EXSTYLE) |
WS_EX_LAYERED);
Question: Can we do this in NativeWidgetWin::SetOpacity() when setting an
opacity value other than 1.0?

http://codereview.chromium.org/8368006/diff/11001/views/bubble/bubble_delegat...
views/bubble/bubble_delegate.cc:195: GetWindowLong(hwnd, GWL_EXSTYLE) &
~WS_EX_LAYERED);
Again, in NativeWidgetWin::SetOpacity() when setting the value to 1.0?

http://codereview.chromium.org/8368006/diff/11001/views/bubble/bubble_delegat...
views/bubble/bubble_delegate.cc:208: SetLayeredWindowAttributes(hwnd, 0,
opacity, LWA_ALPHA);
Here, can we call SetOpacity() on GetWidget()?

[msw, 2011-10-28 17:10:18]

PTAL; sorry for adding some extra cleanup; I can split that out if you'd prefer.

http://codereview.chromium.org/8368006/diff/11001/views/bubble/bubble_delegat...
File views/bubble/bubble_delegate.cc (right):

http://codereview.chromium.org/8368006/diff/11001/views/bubble/bubble_delegat...
views/bubble/bubble_delegate.cc:41: : bubble_(bubble) {}
On 2011/10/27 15:37:00, Ben Goodger (Google) wrote:
> nit: 4-space indent instead of 2.

Done.

http://codereview.chromium.org/8368006/diff/11001/views/bubble/bubble_delegat...
views/bubble/bubble_delegate.cc:106: Widget* border_widget =
CreateBorderWidget(bubble_delegate, parent_widget);
On 2011/10/27 15:37:00, Ben Goodger (Google) wrote:
> Move the border initialization into a separate function to keep the reading
flow
> here smooth.

Only 3 lines were initializing the border, but I split up some functionality;
let me know if I missed your point.

http://codereview.chromium.org/8368006/diff/11001/views/bubble/bubble_delegat...
views/bubble/bubble_delegate.cc:162: GetWindowLong(hwnd, GWL_EXSTYLE) |
WS_EX_LAYERED);
On 2011/10/27 15:37:00, Ben Goodger (Google) wrote:
> Question: Can we do this in NativeWidgetWin::SetOpacity() when setting an
> opacity value other than 1.0?

Moved into AnimationProgressed. We don't want to toggle the layered window style
on other widgets using per-pixel alpha (like the bubble's border widget) when
they're set to full opacity.

http://codereview.chromium.org/8368006/diff/11001/views/bubble/bubble_delegat...
views/bubble/bubble_delegate.cc:195: GetWindowLong(hwnd, GWL_EXSTYLE) &
~WS_EX_LAYERED);
On 2011/10/27 15:37:00, Ben Goodger (Google) wrote:
> Again, in NativeWidgetWin::SetOpacity() when setting the value to 1.0?

Ditto to the StartFade comment.

http://codereview.chromium.org/8368006/diff/11001/views/bubble/bubble_delegat...
views/bubble/bubble_delegate.cc:208: SetLayeredWindowAttributes(hwnd, 0,
opacity, LWA_ALPHA);
On 2011/10/27 15:37:00, Ben Goodger (Google) wrote:
> Here, can we call SetOpacity() on GetWidget()?

Nope, as you pointed out, we need to use SetLayeredWindowAttributes instead of
SetOpacity's underlying UpdateLayeredWindow in order to host child native
Windows controls.

[Ben Goodger (Google), 2011-10-31 17:43:02]

http://codereview.chromium.org/8368006/diff/19005/views/examples/bubble_examp...
File views/examples/bubble_example.cc (right):

http://codereview.chromium.org/8368006/diff/19005/views/examples/bubble_examp...
views/examples/bubble_example.cc:93: DCHECK(bubble);
Looking at your code, CreateBubble can never return NULL, so why bother?

[msw, 2011-10-31 17:47:37]

http://codereview.chromium.org/8368006/diff/19005/views/examples/bubble_examp...
File views/examples/bubble_example.cc (right):

http://codereview.chromium.org/8368006/diff/19005/views/examples/bubble_examp...
views/examples/bubble_example.cc:93: DCHECK(bubble);
On 2011/10/31 17:43:02, Ben Goodger (Google) wrote:
> Looking at your code, CreateBubble can never return NULL, so why bother?

To silence a clang error for the otherwise unused variable :)

[Ben Goodger (Google), 2011-10-31 17:48:29]

http://codereview.chromium.org/8368006/diff/19005/views/examples/bubble_examp...
File views/examples/bubble_example.cc (right):

http://codereview.chromium.org/8368006/diff/19005/views/examples/bubble_examp...
views/examples/bubble_example.cc:93: DCHECK(bubble);
Why not nix the variable then?

[msw, 2011-10-31 17:56:33]

http://codereview.chromium.org/8368006/diff/19005/views/examples/bubble_examp...
File views/examples/bubble_example.cc (right):

http://codereview.chromium.org/8368006/diff/19005/views/examples/bubble_examp...
views/examples/bubble_example.cc:93: DCHECK(bubble);
On 2011/10/31 17:48:29, Ben Goodger (Google) wrote:
> Why not nix the variable then?

Done. I thought it might help fill out the example, but yeah, it's useless here.

[alicet1, 2011-10-31 18:48:31]

http://codereview.chromium.org/8368006/diff/19005/views/bubble/bubble_delegate.h
File views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/8368006/diff/19005/views/bubble/bubble_delegat...
views/bubble/bubble_delegate.h:36: static Widget*
CreateBubble(BubbleDelegateView* bubble_delegate,
hmm. I'm wondering what to do with the return widget -- the caller will have to
call Show() on the delegate view, and not on the widget alone. To get to the
content's widget, it will have to call delegate->GetWidget(), as opposed to if
we return a Widget* here, to get to the delegate, they will need
static_cast<...>(widget->widget_delegate())->Show(), or hold on to it somehow.

what do you think?

http://codereview.chromium.org/8368006/diff/19005/views/bubble/bubble_delegat...
views/bubble/bubble_delegate.h:113: // The widget hosting the border for this
bubble (only used on Windows).
this is non-aura windows only, no?

http://codereview.chromium.org/8368006/diff/19005/views/bubble/bubble_delegat...
File views/bubble/bubble_delegate_unittest.cc (right):

http://codereview.chromium.org/8368006/diff/19005/views/bubble/bubble_delegat...
views/bubble/bubble_delegate_unittest.cc:36:
EXPECT_EQ(bubble_delegate->GetArrowLocation(), border->arrow_location());
this tests one more line than the one above, I think it's because you moved this
from bubble_view_unittest.cc to here. should be able to combine CreateDelegate
and CreateArrowBubble here, it's not checking a lot more.

http://codereview.chromium.org/8368006/diff/19005/views/bubble/bubble_frame_v...
File views/bubble/bubble_frame_view_unittest.cc (right):

http://codereview.chromium.org/8368006/diff/19005/views/bubble/bubble_frame_v...
views/bubble/bubble_frame_view_unittest.cc:58: EXPECT_EQ(HTCLIENT,
bubble_fame_view->NonClientHitTest(kPtInBound));
s/fame/frame/

http://codereview.chromium.org/8368006/diff/19005/views/window/client_view.cc
File views/window/client_view.cc (left):

http://codereview.chromium.org/8368006/diff/19005/views/window/client_view.cc...
views/window/client_view.cc:40: BubbleView* ClientView::AsBubbleView() {
yeah, I was wondering why they are still here, thanks for the cleanup.

[msw, 2011-10-31 19:31:17]

http://codereview.chromium.org/8368006/diff/19005/views/bubble/bubble_delegate.h
File views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/8368006/diff/19005/views/bubble/bubble_delegat...
views/bubble/bubble_delegate.h:36: static Widget*
CreateBubble(BubbleDelegateView* bubble_delegate,
On 2011/10/31 18:48:31, alicet1 wrote:
> hmm. I'm wondering what to do with the return widget -- the caller will have
to
> call Show() on the delegate view, and not on the widget alone. To get to the
> content's widget, it will have to call delegate->GetWidget(), as opposed to if
> we return a Widget* here, to get to the delegate, they will need
> static_cast<...>(widget->widget_delegate())->Show(), or hold on to it somehow.
> 
> what do you think?

I'm not sure what you mean. Users can cache a pointer to the delegate if
desired, as in the example.

We considered a views::Bubble last week for holding delegate/widgets ptrs and
exposing show/fade/close/etc, but I think we agreed that hanging onto the
delegate pointer seemed sufficient. Any new thoughts?

http://codereview.chromium.org/8368006/diff/19005/views/bubble/bubble_delegat...
views/bubble/bubble_delegate.h:113: // The widget hosting the border for this
bubble (only used on Windows).
On 2011/10/31 18:48:31, alicet1 wrote:
> this is non-aura windows only, no?

Comment updated.

http://codereview.chromium.org/8368006/diff/19005/views/bubble/bubble_delegat...
File views/bubble/bubble_delegate_unittest.cc (right):

http://codereview.chromium.org/8368006/diff/19005/views/bubble/bubble_delegat...
views/bubble/bubble_delegate_unittest.cc:36:
EXPECT_EQ(bubble_delegate->GetArrowLocation(), border->arrow_location());
On 2011/10/31 18:48:31, alicet1 wrote:
> this tests one more line than the one above, I think it's because you moved
this
> from bubble_view_unittest.cc to here. should be able to combine CreateDelegate
> and CreateArrowBubble here, it's not checking a lot more.

Done.

http://codereview.chromium.org/8368006/diff/19005/views/bubble/bubble_frame_v...
File views/bubble/bubble_frame_view_unittest.cc (right):

http://codereview.chromium.org/8368006/diff/19005/views/bubble/bubble_frame_v...
views/bubble/bubble_frame_view_unittest.cc:58: EXPECT_EQ(HTCLIENT,
bubble_fame_view->NonClientHitTest(kPtInBound));
On 2011/10/31 18:48:31, alicet1 wrote:
> s/fame/frame/

Done.

http://codereview.chromium.org/8368006/diff/19005/views/window/client_view.cc
File views/window/client_view.cc (left):

http://codereview.chromium.org/8368006/diff/19005/views/window/client_view.cc...
views/window/client_view.cc:40: BubbleView* ClientView::AsBubbleView() {
On 2011/10/31 18:48:31, alicet1 wrote:
> yeah, I was wondering why they are still here, thanks for the cleanup.

:)

[alicet1, 2011-10-31 23:26:59]

http://codereview.chromium.org/8368006/diff/19005/views/bubble/bubble_delegate.h
File views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/8368006/diff/19005/views/bubble/bubble_delegat...
views/bubble/bubble_delegate.h:36: static Widget*
CreateBubble(BubbleDelegateView* bubble_delegate,
On 2011/10/31 19:31:17, msw wrote:
> On 2011/10/31 18:48:31, alicet1 wrote:
> > hmm. I'm wondering what to do with the return widget -- the caller will have
> to
> > call Show() on the delegate view, and not on the widget alone. To get to the
> > content's widget, it will have to call delegate->GetWidget(), as opposed to
if
> > we return a Widget* here, to get to the delegate, they will need
> > static_cast<...>(widget->widget_delegate())->Show(), or hold on to it
somehow.
> > 
> > what do you think?
> 
> I'm not sure what you mean. Users can cache a pointer to the delegate if
> desired, as in the example.
> 
> We considered a views::Bubble last week for holding delegate/widgets ptrs and
> exposing show/fade/close/etc, but I think we agreed that hanging onto the
> delegate pointer seemed sufficient. Any new thoughts?

no, what I mean is what if this method is
static void CreateBubble(....);

I'm not sure if it needs to return a widget at all. Callers will have to cache
the delegate anyway. delegate->GetWidget() will always get it back for them, no?

[msw, 2011-11-01 00:27:36]

http://codereview.chromium.org/8368006/diff/19005/views/bubble/bubble_delegate.h
File views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/8368006/diff/19005/views/bubble/bubble_delegat...
views/bubble/bubble_delegate.h:36: static Widget*
CreateBubble(BubbleDelegateView* bubble_delegate,
On 2011/10/31 23:26:59, alicet1 wrote:
> On 2011/10/31 19:31:17, msw wrote:
> > On 2011/10/31 18:48:31, alicet1 wrote:
> > > hmm. I'm wondering what to do with the return widget -- the caller will
have
> > to
> > > call Show() on the delegate view, and not on the widget alone. To get to
the
> > > content's widget, it will have to call delegate->GetWidget(), as opposed
to
> if
> > > we return a Widget* here, to get to the delegate, they will need
> > > static_cast<...>(widget->widget_delegate())->Show(), or hold on to it
> somehow.
> > > 
> > > what do you think?
> > 
> > I'm not sure what you mean. Users can cache a pointer to the delegate if
> > desired, as in the example.
> > 
> > We considered a views::Bubble last week for holding delegate/widgets ptrs
and
> > exposing show/fade/close/etc, but I think we agreed that hanging onto the
> > delegate pointer seemed sufficient. Any new thoughts?
> 
> no, what I mean is what if this method is
> static void CreateBubble(....);
> 
> I'm not sure if it needs to return a widget at all. Callers will have to cache
> the delegate anyway. delegate->GetWidget() will always get it back for them,
no?

Done; I guess that's reasonable. It'd be nice to make the bubble workflow a
little less awkward, but we gotta get cracking on converting these bubbles.

[alicet1, 2011-11-01 17:00:40]

LGTM

looking good!

[msw, 2011-11-01 19:00:00]

Ben, PTAL; thanks!

[Ben Goodger (Google), 2011-11-01 19:24:54]

Sorry for not getting to this sooner. It is a convention of our CreateFoo()
methods to create something and return it. As a result I think void
CreateBubble(..) is awkward, and it was better the way it was. It was
consistent with Widget::CreateWindow() for example.

-Ben

On Mon, Oct 31, 2011 at 5:27 PM, <msw@chromium.org> wrote:

>
> http://codereview.chromium.**org/8368006/diff/19005/views/**
>
bubble/bubble_delegate.h<http://codereview.chromium.org/8368006/diff/19005/views/bubble/bubble_delegate.h>
> File views/bubble/bubble_delegate.h (right):
>
> http://codereview.chromium.**org/8368006/diff/19005/views/**
>
bubble/bubble_delegate.h#**newcode36<http://codereview.chromium.org/8368006/diff/19005/views/bubble/bubble_delegate.h#newcode36>
> views/bubble/bubble_delegate.**h:36: static Widget*
> CreateBubble(**BubbleDelegateView* bubble_delegate,
> On 2011/10/31 23:26:59, alicet1 wrote:
>
>> On 2011/10/31 19:31:17, msw wrote:
>> > On 2011/10/31 18:48:31, alicet1 wrote:
>> > > hmm. I'm wondering what to do with the return widget -- the caller
>>
> will have
>
>> > to
>> > > call Show() on the delegate view, and not on the widget alone. To
>>
> get to the
>
>> > > content's widget, it will have to call delegate->GetWidget(), as
>>
> opposed to
>
>> if
>> > > we return a Widget* here, to get to the delegate, they will need
>> > > static_cast<...>(widget->**widget_delegate())->Show(), or hold on to
>>
> it
>
>> somehow.
>> > >
>> > > what do you think?
>> >
>> > I'm not sure what you mean. Users can cache a pointer to the
>>
> delegate if
>
>> > desired, as in the example.
>> >
>> > We considered a views::Bubble last week for holding delegate/widgets
>>
> ptrs and
>
>> > exposing show/fade/close/etc, but I think we agreed that hanging
>>
> onto the
>
>> > delegate pointer seemed sufficient. Any new thoughts?
>>
>
>  no, what I mean is what if this method is
>> static void CreateBubble(....);
>>
>
>  I'm not sure if it needs to return a widget at all. Callers will have
>>
> to cache
>
>> the delegate anyway. delegate->GetWidget() will always get it back for
>>
> them, no?
>
> Done; I guess that's reasonable. It'd be nice to make the bubble
> workflow a little less awkward, but we gotta get cracking on converting
> these bubbles.
>
>
http://codereview.chromium.**org/8368006/<http://codereview.chromium.org/8368...
>

[Ben Goodger (Google), 2011-11-01 19:24:54]

http://codereview.chromium.org/8368006/diff/24004/views/bubble/bubble_delegate.h
File views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/8368006/diff/24004/views/bubble/bubble_delegat...
views/bubble/bubble_delegate.h:35: // Create and intitialize the bubble
Widget(s) with proper bounds.
initialize

[msw, 2011-11-01 19:47:11]

Reverted to return Widget* from CreateBubble.

http://codereview.chromium.org/8368006/diff/24004/views/bubble/bubble_delegate.h
File views/bubble/bubble_delegate.h (right):

http://codereview.chromium.org/8368006/diff/24004/views/bubble/bubble_delegat...
views/bubble/bubble_delegate.h:35: // Create and intitialize the bubble
Widget(s) with proper bounds.
On 2011/11/01 19:24:54, Ben Goodger (Google) wrote:
> initialize

Done.

[Ben Goodger (Google), 2011-11-01 19:51:43]

lgtm

[commit-bot: I haz the power, 2011-11-01 19:53:10]

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

[commit-bot: I haz the power, 2011-11-01 21:06:02]

Change committed as 108167