Allow message center and related bubbles to render on Windows.

Allow message center and related bubbles to render on Windows.

SetPaintToLayer causes Windows to render only black squares since
there is no compositor there.

BUG=168605
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177742

[dewittj, 2013-01-11 23:48:30]

Please take a look at this.  The work is done by #ifdefs, which are sub-optimal
in many cases, but seem to work OK here.  If you have any objections or ideas on
how to structure the code differently (i.e. without the ifdefs), let me know.

[Jun Mukai, 2013-01-12 01:32:57]

lgtm with a nit

https://codereview.chromium.org/11836003/diff/15003/ui/views/bubble/tray_bubb...
File ui/views/bubble/tray_bubble_view.cc (right):

https://codereview.chromium.org/11836003/diff/15003/ui/views/bubble/tray_bubb...
ui/views/bubble/tray_bubble_view.cc:283: new
TrayBubbleContentMask(bubble_border_->GetBorderCornerRadius() - 1));
fix the indent

[dewittj, 2013-01-12 01:38:10]

https://codereview.chromium.org/11836003/diff/15003/ui/views/bubble/tray_bubb...
File ui/views/bubble/tray_bubble_view.cc (right):

https://codereview.chromium.org/11836003/diff/15003/ui/views/bubble/tray_bubb...
ui/views/bubble/tray_bubble_view.cc:283: new
TrayBubbleContentMask(bubble_border_->GetBorderCornerRadius() - 1));
On 2013/01/12 01:32:57, Jun Mukai wrote:
> fix the indent

Done.

[dewittj, 2013-01-12 02:01:30]

fyi mukai,

Had to change another line to fix the Win bot, I somehow left out the change of
superclass in the cc file: SlideOutView->MessageViewBase.

[Jun Mukai, 2013-01-12 02:04:44]

On 2013/01/12 02:01:30, dewittj wrote:
> fyi mukai,
> 
> Had to change another line to fix the Win bot, I somehow left out the change
of
> superclass in the cc file: SlideOutView->MessageViewBase.

lgtm++

[commit-bot: I haz the power, 2013-01-14 17:49:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/11836003/19019

[commit-bot: I haz the power, 2013-01-14 17:49:36]

Presubmit check for 11836003-19019 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ui/views/bubble

[dewittj, 2013-01-14 18:07:38]

msw, could you do an OWNERS review for ui/views/bubble? Thanks.

[msw, 2013-01-14 19:33:59]

Is there another way to check for [the lack of] layer support?
Does layer() return NULL or can you check for a compositor?
These functions should ideally no-op without a compositor.

[sky, 2013-01-14 22:06:09]

I believe we create the Layer even if the compositor isn't supported.
I do agree with you though that we should find a way to avoid these
ifdefs.

  -Scott

On Mon, Jan 14, 2013 at 11:33 AM,  <msw@chromium.org> wrote:
> Is there another way to check for [the lack of] layer support?
> Does layer() return NULL or can you check for a compositor?
> These functions should ideally no-op without a compositor.
>
> https://codereview.chromium.org/11836003/

[dewittj, 2013-01-15 18:19:24]

Is there a good way to detect if the compositor isn't supported, before the view
is attached to a widget?

On 2013/01/14 22:06:09, sky wrote:
> I believe we create the Layer even if the compositor isn't supported.
> I do agree with you though that we should find a way to avoid these
> ifdefs.
> 
>   -Scott
> 
> On Mon, Jan 14, 2013 at 11:33 AM,  <mailto:msw@chromium.org> wrote:
> > Is there another way to check for [the lack of] layer support?
> > Does layer() return NULL or can you check for a compositor?
> > These functions should ideally no-op without a compositor.
> >
> > https://codereview.chromium.org/11836003/

[sky, 2013-01-15 18:34:13]

Not sure, but I would think it possible to plumb something through.

  -Scott

On Tue, Jan 15, 2013 at 10:19 AM,  <dewittj@chromium.org> wrote:
> Is there a good way to detect if the compositor isn't supported, before the
> view
> is attached to a widget?
>
>
> On 2013/01/14 22:06:09, sky wrote:
>>
>> I believe we create the Layer even if the compositor isn't supported.
>> I do agree with you though that we should find a way to avoid these
>> ifdefs.
>
>
>>    -Scott
>
>
>> On Mon, Jan 14, 2013 at 11:33 AM,  <mailto:msw@chromium.org> wrote:
>> > Is there another way to check for [the lack of] layer support?
>> > Does layer() return NULL or can you check for a compositor?
>> > These functions should ideally no-op without a compositor.
>> >
>> > https://codereview.chromium.org/11836003/
>
>
>
>
> https://codereview.chromium.org/11836003/

[dewittj, 2013-01-17 01:47:58]

Instead of ifdefs, I used the function get_use_acceleration_when_possible(). 
This function essentially returns a boolean based on an ifdef done in
ui/views/view.h, so I assume that's the most straightforward way to determine if
the functionality is there.

Unfortunately this function is currently only used by tests, and the boolean
that backs it is only used within view.cc, leading me to believe that this might
be an unintended use case for this function.  Please let me know.

As far as SlideOutView,  I have made an Impl class which handles the
transformation of the view.  It has an implementation for Aura using layers, and
a stub for other platforms that doesn't do any rendering but that will still
allow the onSlideOut event to process.

sadrul: slide_out_view*, gyp

[msw, 2013-01-17 02:28:30]

I think get_use_acceleration_when_possible() is reasonable for now
(renaming/formalizing that or making the layer operations no-op outside of Aura
is out-of-scope here), thanks for addressing that.

I'm not a fan of the impl abstraction here, it doesn't seem necessary (or to add
sufficient benefit) for its complexity.

https://codereview.chromium.org/11836003/diff/44001/ui/views/controls/slide_o...
File ui/views/controls/slide_out_view.cc (right):

https://codereview.chromium.org/11836003/diff/44001/ui/views/controls/slide_o...
ui/views/controls/slide_out_view.cc:16: if
(get_use_acceleration_when_possible()) {
nit: remove unnecessary braces.

https://codereview.chromium.org/11836003/diff/44001/ui/views/controls/slide_o...
File ui/views/controls/slide_out_view.h (right):

https://codereview.chromium.org/11836003/diff/44001/ui/views/controls/slide_o...
ui/views/controls/slide_out_view.h:20: SLIDE_RIGHT
nit: use a comma after trailing enum entries.

https://codereview.chromium.org/11836003/diff/44001/ui/views/controls/slide_o...
ui/views/controls/slide_out_view.h:23: class Impl {
nit: comment.

https://codereview.chromium.org/11836003/diff/44001/ui/views/controls/slide_o...
ui/views/controls/slide_out_view.h:28: virtual ~Impl() {}
nit: list the destructor above other functions.

https://codereview.chromium.org/11836003/diff/44001/ui/views/controls/slide_o...
File ui/views/controls/slide_out_view_impl.h (right):

https://codereview.chromium.org/11836003/diff/44001/ui/views/controls/slide_o...
ui/views/controls/slide_out_view_impl.h:21: // Used when the scroll amount
changes, this method should transform |view|
nit: These comments should go with the pure virtual decls.
Instead, comment here: // Overridden from SlideOutView::Impl:

https://codereview.chromium.org/11836003/diff/44001/ui/views/controls/slide_o...
File ui/views/controls/slide_out_view_impl_aura.cc (right):

https://codereview.chromium.org/11836003/diff/44001/ui/views/controls/slide_o...
ui/views/controls/slide_out_view_impl_aura.cc:24: gfx::Transform transform;
Perhaps it's just me, but I'd rather see these implementations left in
SlideOutView wrapped with if(get_use_acceleration_when_possible()), avoiding the
impl interface/files/classes.

[sadrul, 2013-01-17 03:27:22]

https://codereview.chromium.org/11836003/diff/44001/ui/views/controls/slide_o...
File ui/views/controls/slide_out_view_impl_aura.cc (right):

https://codereview.chromium.org/11836003/diff/44001/ui/views/controls/slide_o...
ui/views/controls/slide_out_view_impl_aura.cc:24: gfx::Transform transform;
On 2013/01/17 02:28:30, msw wrote:
> Perhaps it's just me, but I'd rather see these implementations left in
> SlideOutView wrapped with if(get_use_acceleration_when_possible()), avoiding
the
> impl interface/files/classes.

I agree. That would be simpler.

[dewittj, 2013-01-17 21:25:18]

Thanks for the input.  PTAL, I followed your suggestion about the Impl class and
axed it.

https://codereview.chromium.org/11836003/diff/44001/ui/views/controls/slide_o...
File ui/views/controls/slide_out_view.cc (right):

https://codereview.chromium.org/11836003/diff/44001/ui/views/controls/slide_o...
ui/views/controls/slide_out_view.cc:16: if
(get_use_acceleration_when_possible()) {
On 2013/01/17 02:28:30, msw wrote:
> nit: remove unnecessary braces.

Gone.

https://codereview.chromium.org/11836003/diff/44001/ui/views/controls/slide_o...
File ui/views/controls/slide_out_view.h (right):

https://codereview.chromium.org/11836003/diff/44001/ui/views/controls/slide_o...
ui/views/controls/slide_out_view.h:20: SLIDE_RIGHT
On 2013/01/17 02:28:30, msw wrote:
> nit: use a comma after trailing enum entries.

Done.

https://codereview.chromium.org/11836003/diff/44001/ui/views/controls/slide_o...
ui/views/controls/slide_out_view.h:23: class Impl {
On 2013/01/17 02:28:30, msw wrote:
> nit: comment.

Gone.

https://codereview.chromium.org/11836003/diff/44001/ui/views/controls/slide_o...
ui/views/controls/slide_out_view.h:28: virtual ~Impl() {}
On 2013/01/17 02:28:30, msw wrote:
> nit: list the destructor above other functions.

Gone.

https://codereview.chromium.org/11836003/diff/44001/ui/views/controls/slide_o...
File ui/views/controls/slide_out_view_impl.h (right):

https://codereview.chromium.org/11836003/diff/44001/ui/views/controls/slide_o...
ui/views/controls/slide_out_view_impl.h:21: // Used when the scroll amount
changes, this method should transform |view|
On 2013/01/17 02:28:30, msw wrote:
> nit: These comments should go with the pure virtual decls.
> Instead, comment here: // Overridden from SlideOutView::Impl:

Gone.

https://codereview.chromium.org/11836003/diff/44001/ui/views/controls/slide_o...
File ui/views/controls/slide_out_view_impl_aura.cc (right):

https://codereview.chromium.org/11836003/diff/44001/ui/views/controls/slide_o...
ui/views/controls/slide_out_view_impl_aura.cc:24: gfx::Transform transform;
On 2013/01/17 03:27:22, sadrul wrote:
> On 2013/01/17 02:28:30, msw wrote:
> > Perhaps it's just me, but I'd rather see these implementations left in
> > SlideOutView wrapped with if(get_use_acceleration_when_possible()), avoiding
> the
> > impl interface/files/classes.
> 
> I agree. That would be simpler.

Done.

[msw, 2013-01-17 22:45:39]

LGTM, thanks!

https://codereview.chromium.org/11836003/diff/51001/ui/views/controls/slide_o...
File ui/views/controls/slide_out_view.cc (right):

https://codereview.chromium.org/11836003/diff/51001/ui/views/controls/slide_o...
ui/views/controls/slide_out_view.cc:15: // Only use accelerated compositing when
it is available on the platform.
nit: this comment seems redundant, but it's okay if you find it helpful.

[dewittj, 2013-01-18 17:50:55]

ping sadrul slide_out_view*, thanks.

[sadrul, 2013-01-18 17:53:11]

LGTM

https://codereview.chromium.org/11836003/diff/51001/ui/views/controls/slide_o...
File ui/views/controls/slide_out_view.cc (right):

https://codereview.chromium.org/11836003/diff/51001/ui/views/controls/slide_o...
ui/views/controls/slide_out_view.cc:72: // Restore the layer state.
Move the comment inside the if.

You could also early return.

[dewittj, 2013-01-18 18:30:48]

https://codereview.chromium.org/11836003/diff/51001/ui/views/controls/slide_o...
File ui/views/controls/slide_out_view.cc (right):

https://codereview.chromium.org/11836003/diff/51001/ui/views/controls/slide_o...
ui/views/controls/slide_out_view.cc:15: // Only use accelerated compositing when
it is available on the platform.
On 2013/01/17 22:45:39, msw wrote:
> nit: this comment seems redundant, but it's okay if you find it helpful.

Changed it to be more helpful.

https://codereview.chromium.org/11836003/diff/51001/ui/views/controls/slide_o...
ui/views/controls/slide_out_view.cc:72: // Restore the layer state.
On 2013/01/18 17:53:11, sadrul wrote:
> Move the comment inside the if.
> 
> You could also early return.

Done.

[commit-bot: I haz the power, 2013-01-18 18:31:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/11836003/60001

[commit-bot: I haz the power, 2013-01-18 20:41:11]

Change committed as 177742