|
|
Created:
7 years, 11 months ago by dewittj Modified:
7 years, 11 months ago Reviewers:
msw, Jun Mukai, sadrul CC:
chromium-reviews, tfarina, alicet1, msw+watch_chromium.org, sky Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAllow 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
Patch Set 1 #Patch Set 2 : Cut out SlideOutView when not using Aura. #
Total comments: 2
Patch Set 3 : Fix an indentation nit. #Patch Set 4 : Add back a missing hunk. #Patch Set 5 : Remove ifdefs. #
Total comments: 13
Patch Set 6 : Take out Impl, address nits. #
Total comments: 4
Patch Set 7 : Move comment and early return instead of big blocks. #
Created: 7 years, 11 months ago
Messages
Total messages: 22 (0 generated)
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.
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
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.
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.
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++
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dewittj@chromium.org/11836003/19019
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
msw, could you do an OWNERS review for ui/views/bubble? Thanks.
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.
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/
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/
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/
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
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.
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.
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.
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.
ping sadrul slide_out_view*, thanks.
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.
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.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dewittj@chromium.org/11836003/60001
Message was sent while issue was closed.
Change committed as 177742 |