New widget factories & methods to support desktop notifications, GTK stubs....

Refactor widget methods to support desktop notifications, including GTK stubs.

BUG=none
TEST=none

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

[Ben Goodger (Google), 2009-10-22 21:18:36]

http://codereview.chromium.org/307030/diff/1/4
File views/widget/widget.h (right):

http://codereview.chromium.org/307030/diff/1/4#newcode51
Line 51: static Widget* CreateLayeredPopupWidget(bool delete_on_destroy);
Hrm... I don't like using the term Layered here since it relates to a
windows-specific feature.

http://codereview.chromium.org/307030/diff/1/4#newcode53
Line 53: // Creates a popup widget for the current platform (not
layered/transparent).
Remove reference to transparent.

"Creates a transient popup widget."

http://codereview.chromium.org/307030/diff/1/4#newcode82
Line 82: virtual void SetInFrontOf(Widget* widget) = 0;
MoveAbove reads better to me.

http://codereview.chromium.org/307030/diff/1/2
File views/widget/widget_win.cc (right):

http://codereview.chromium.org/307030/diff/1/2#newcode1173
Line 1173: WS_EX_TRANSPARENT |
can you remove the extra WS_EX_TRANSPARENT?

http://codereview.chromium.org/307030/diff/1/2#newcode1184
Line 1184: l10n_util::GetExtendedTooltipStyles());
I think all three methods here could be combined into:

CreatePopupWidget(bool transparent, bool accepts_events, bool
delete_on_destroy);

transparent here corresponds to WS_EX_LAYERED.
accepts_events corresponds to WS_EX_TRANSPARENT (don't ask).

[John Gregg, 2009-10-23 00:39:59]

New code posted addressing comments.

http://codereview.chromium.org/307030/diff/1/4
File views/widget/widget.h (right):

http://codereview.chromium.org/307030/diff/1/4#newcode51
Line 51: static Widget* CreateLayeredPopupWidget(bool delete_on_destroy);
On 2009/10/22 21:18:36, Ben Goodger wrote:
> Hrm... I don't like using the term Layered here since it relates to a
> windows-specific feature.

gone with the method-merger.

http://codereview.chromium.org/307030/diff/1/4#newcode53
Line 53: // Creates a popup widget for the current platform (not
layered/transparent).
On 2009/10/22 21:18:36, Ben Goodger wrote:
> Remove reference to transparent.
> 
> "Creates a transient popup widget."

gone

http://codereview.chromium.org/307030/diff/1/4#newcode82
Line 82: virtual void SetInFrontOf(Widget* widget) = 0;
On 2009/10/22 21:18:36, Ben Goodger wrote:
> MoveAbove reads better to me.

MoveAbove it is.

http://codereview.chromium.org/307030/diff/1/2
File views/widget/widget_win.cc (right):

http://codereview.chromium.org/307030/diff/1/2#newcode1184
Line 1184: l10n_util::GetExtendedTooltipStyles());
On 2009/10/22 21:18:36, Ben Goodger wrote:
> I think all three methods here could be combined into:
> 
> CreatePopupWidget(bool transparent, bool accepts_events, bool
> delete_on_destroy);
> 
> transparent here corresponds to WS_EX_LAYERED.
> accepts_events corresponds to WS_EX_TRANSPARENT (don't ask).
> 

I like it.  I'm pretty sure it should be (!accepts_events) that implies
WS_EX_TRANSPARENT, anyway that's how i'll write it.

[Ben Goodger (Google), 2009-10-26 18:10:13]

LGTM with one change.

http://codereview.chromium.org/307030/diff/4015/3006
File views/widget/widget_win.cc (right):

http://codereview.chromium.org/307030/diff/4015/3006#newcode141
Line 141: WidgetWin* other = static_cast<WidgetWin*>(widget);
How about just other->GetNativeView()... then you don't need to do this.