Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11293124:
Remove top and bottom margins from TrayBubbleView (Closed)
|
Created:
8 years, 1 month ago by bartfab (slow) Modified:
8 years, 1 month ago CC:
chromium-reviews, tfarina, alicet1, msw+watch_chromium.org, ben+watch_chromium.org, miket_OOO Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRemove top and bottom margins from TrayBubbleView
This CL modifies TrayBubbleView so that instead of ~3px margins filled
with a solid color at its top and bottom, the contents extends all the
way to the edges. To prevent the content from drawing over the bubble's
rounded corners, a mask is set on the hosting widget.
Since the masking would not work on Windows non-aura, the previous
behavior is preserved there.
BUG=159682
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167967
Patch Set 1 #
Total comments: 8
Patch Set 2 : Comments addressed. Restored previous behavior on Windows non-aura. #
Total comments: 2
Patch Set 3 : Made TrayBubbleView aura-only, removed #ifdefs. #
Total comments: 2
Patch Set 4 : Nit addressed. #
Total comments: 2
Patch Set 5 : Added TODO owner. #Patch Set 6 : Provide mask layers with device scale information, fixing high-DPI rendering. #
Total comments: 8
Patch Set 7 : Comments addressed. #
Total comments: 6
Patch Set 8 : Comments addressed. #
Total comments: 6
Patch Set 9 : Nits addressed. #Patch Set 10 : Compile fix. #
Total comments: 2
Patch Set 11 : Nit addressed. #
Messages
Total messages: 50 (0 generated)
Hi Steven, Could you please review the system_tray and message_center changes? Hi Mike, Could you please review the tray_bubble_view changes?
This seems reasonable, but I'll wait for MSW's review since I'm unfamiliar with how masks work here. https://codereview.chromium.org/11293124/diff/1/ui/views/bubble/tray_bubble_v... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/11293124/diff/1/ui/views/bubble/tray_bubble_v... ui/views/bubble/tray_bubble_view.cc:133: paint.setStyle(SkPaint::kFill_Style); What are we drawing here? I'm unfamiliar with what a default SkPaint will draw.
Would BubbleDelegateView::set_color() suffice for this functionality? If not, could we improve support there instead?
The problem is that a single color is not sufficient. When a submenu is being opened, two layers get composited on top of each other. They may have different background colors. But the top ~3px of the menu are not part of either layer and not part of the animation. Thus, the top ~3px will always look out of place.
https://codereview.chromium.org/11293124/diff/1/ui/views/bubble/tray_bubble_v... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/11293124/diff/1/ui/views/bubble/tray_bubble_v... ui/views/bubble/tray_bubble_view.cc:133: paint.setStyle(SkPaint::kFill_Style); Since this is a mask layer, only alpha matters. A default SkPaint will draw in a fully opaque color, exactly as needed here.
https://codereview.chromium.org/11293124/diff/1/ui/views/bubble/tray_bubble_v... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/11293124/diff/1/ui/views/bubble/tray_bubble_v... ui/views/bubble/tray_bubble_view.cc:133: paint.setStyle(SkPaint::kFill_Style); On 2012/11/06 23:09:14, bartfab wrote: > Since this is a mask layer, only alpha matters. A default SkPaint will draw in a > fully opaque color, exactly as needed here. I see. Might be worth commenting to that effect (or setting the color explicitly anyway).
Hi Scott, This CL uses layer masking that has existed in Views for a while but that nobody has used before, nobody knows very well and nobody is comfortable reviewing :). Would you mind taking a look at this CL? If you are not comfortable reviewing it either, could you suggest an appropriate reviewer?
https://codereview.chromium.org/11293124/diff/1/ui/views/bubble/tray_bubble_v... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/11293124/diff/1/ui/views/bubble/tray_bubble_v... ui/views/bubble/tray_bubble_view.cc:120: class TrayBubbleContentMask : public ui::Layer, public ui::LayerDelegate { This code won't work on windows non-aura. https://codereview.chromium.org/11293124/diff/1/ui/views/bubble/tray_bubble_v... ui/views/bubble/tray_bubble_view.cc:131: path.addRoundRect(gfx::RectToSkRect(bounds()), radius_, radius_); Bounds() is in the coordinate system of the parent. Don't you want gfx::Rect(bounds().size()) ?
https://chromiumcodereview.appspot.com/11293124/diff/1/ui/views/bubble/tray_b... File ui/views/bubble/tray_bubble_view.cc (right): https://chromiumcodereview.appspot.com/11293124/diff/1/ui/views/bubble/tray_b... ui/views/bubble/tray_bubble_view.cc:120: class TrayBubbleContentMask : public ui::Layer, public ui::LayerDelegate { I modified the CL to preserve the previous implementation when defined(OS_WIN) && !defined(USE_AURA) https://chromiumcodereview.appspot.com/11293124/diff/1/ui/views/bubble/tray_b... ui/views/bubble/tray_bubble_view.cc:131: path.addRoundRect(gfx::RectToSkRect(bounds()), radius_, radius_); Done. bounds() actually worked here because the bounds happen to be set by TrayBubbleView::UpdateBubble() to a rect whose top-left corner is (0,0). But yes, it is safer to use bounds().size() just in case this ever changes. https://chromiumcodereview.appspot.com/11293124/diff/1/ui/views/bubble/tray_b... ui/views/bubble/tray_bubble_view.cc:133: paint.setStyle(SkPaint::kFill_Style); I added an explicit setAlpha() call.
https://codereview.chromium.org/11293124/diff/8001/ui/views/bubble/tray_bubbl... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/11293124/diff/8001/ui/views/bubble/tray_bubbl... ui/views/bubble/tray_bubble_view.cc:332: bubble_content_mask_.reset(new TrayBubbleContentMask(bubble_border_)); Can we instead use ClipPath when painting so that we don't need all the special cases?
https://codereview.chromium.org/11293124/diff/8001/ui/views/bubble/tray_bubbl... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/11293124/diff/8001/ui/views/bubble/tray_bubbl... ui/views/bubble/tray_bubble_view.cc:332: bubble_content_mask_.reset(new TrayBubbleContentMask(bubble_border_)); The problem is that the ash tray context menu contains animated layers. The layer is painted once (creating a texture) and then composited without re-painting. The clip path needs to be enforced while compositing the layer, not when painting it.
Sorry for the questions. Why do we need animated layers in the menu? -Scott On Wed, Nov 7, 2012 at 7:34 AM, <bartfab@chromium.org> wrote: > > https://codereview.chromium.org/11293124/diff/8001/ui/views/bubble/tray_bubbl... > File ui/views/bubble/tray_bubble_view.cc (right): > > https://codereview.chromium.org/11293124/diff/8001/ui/views/bubble/tray_bubbl... > ui/views/bubble/tray_bubble_view.cc:332: bubble_content_mask_.reset(new > TrayBubbleContentMask(bubble_border_)); > The problem is that the ash tray context menu contains animated layers. > The layer is painted once (creating a texture) and then composited > without re-painting. The clip path needs to be enforced while > compositing the layer, not when painting it. > > https://codereview.chromium.org/11293124/
On 2012/11/07 15:34:05, bartfab wrote: > https://codereview.chromium.org/11293124/diff/8001/ui/views/bubble/tray_bubbl... > File ui/views/bubble/tray_bubble_view.cc (right): > > https://codereview.chromium.org/11293124/diff/8001/ui/views/bubble/tray_bubbl... > ui/views/bubble/tray_bubble_view.cc:332: bubble_content_mask_.reset(new > TrayBubbleContentMask(bubble_border_)); > The problem is that the ash tray context menu contains animated layers. The > layer is painted once (creating a texture) and then composited without > re-painting. The clip path needs to be enforced while compositing the layer, not > when painting it. When you say "ash tray context menu" are you referring to the bubble itself (which isn't really a context menu, even though its behavior is menu-like)? or a context menu from a bubble, e.g. right-clicking on a web notification from the message center?
Since we do not have a use case that we need to support on Windows, could we just eliminate the border and use the layer mask #if USE_AURA, with a TODO: implement layer masking or alternate solution on Windows?
+ cc:miket@ FYI
If that's the case, can we exclude compiling the file on windows and get rid of all the ifdefs? -Scott On Wed, Nov 7, 2012 at 2:08 PM, <stevenjb@chromium.org> wrote: > Since we do not have a use case that we need to support on Windows, could we > just eliminate the border and use the layer mask #if USE_AURA, with a TODO: > implement layer masking or alternate solution on Windows? > > > https://chromiumcodereview.appspot.com/11293124/
On 2012/11/07 22:43:57, sky wrote: > If that's the case, can we exclude compiling the file on windows and > get rid of all the ifdefs? > We could safely make this file aura-only for now, since it is only used by code in ui/message_center, which is currently only included in Ash builds. I would definitely remove the #ifdefs, and would still add a comment about TrayBubbleContentMask that it only works on Aura.
The animated layers are used for submenus: When you click on an item in the main context menu, the submenu slides in from the right and gets overlaid on the main menu content.
> When you say "ash tray context menu" are you referring to the bubble itself > (which isn't really a context menu, even though its behavior is menu-like)? or a > context menu from a bubble, e.g. right-clicking on a web notification from the > message center? Sorry if my terminology was confusing: Yes, I meant the bubble that acts as as if it was essentially a menu.
Why does that necessitate using layers? On Thu, Nov 8, 2012 at 1:10 AM, <bartfab@chromium.org> wrote: > The animated layers are used for submenus: When you click on an item in the > main > context menu, the submenu slides in from the right and gets overlaid on the > main > menu content. > > https://chromiumcodereview.appspot.com/11293124/
This is the way it has been implemented by whoever wrote this code in the first place. There may be different ways of course but I expect that yanking out the layers and using e.g. multiple widgets would be a rather large change,
I'm not suggesting using layers. If the code is turning on layers on views it shouldn't be that hard to change. -Scott On Thu, Nov 8, 2012 at 9:13 AM, <bartfab@chromium.org> wrote: > This is the way it has been implemented by whoever wrote this code in the > first > place. There may be different ways of course but I expect that yanking out > the > layers and using e.g. multiple widgets would be a rather large change, > > https://chromiumcodereview.appspot.com/11293124/
I am a bit confused now. Just to get back in sync: The existing code uses layers. It does so because there is a single widget that presents a menu by drawing a number of menu items. When you switch to a submenu, the widget's content is cleared and regenerated, drawing the submenu items instead. It seems that animations between main menu and submenu were an afterthought. Instead of duplicating the whole widget, the approach was chosen to snapshot the old menu into a texture, update the widget and then slide one in/out on top of the other. So the current situation is that the animation is entirely layer-based. It should be possible to make each menu its own widget and then animate these widgets appropriately. But it would be quite some work. The menu that is sliding out needs to maintain its appearance but not e.g. react to hovering or clicking... snapshotting into a texture really seems like the simplest solution and a valid use for layers.
On 2012/11/08 17:28:57, bartfab wrote: > I am a bit confused now. Just to get back in sync: > > The existing code uses layers. It does so because there is a single widget that > presents a menu by drawing a number of menu items. When you switch to a submenu, > the widget's content is cleared and regenerated, drawing the submenu items > instead. It seems that animations between main menu and submenu were an > afterthought. Instead of duplicating the whole widget, the approach was chosen > to snapshot the old menu into a texture, update the widget and then slide one > in/out on top of the other. > > So the current situation is that the animation is entirely layer-based. It > should be possible to make each menu its own widget and then animate these > widgets appropriately. But it would be quite some work. The menu that is sliding > out needs to maintain its appearance but not e.g. react to hovering or > clicking... snapshotting into a texture really seems like the simplest solution > and a valid use for layers. One of the difficulties with sliding in widgets is that visually, it shouldn't look like we are creating a new widget (i.e. the widget, or its shadow, should not change during the slide animation). And doing that would be tricky (trickier than using 'dead' layers already is)
Sadrul set me straight. Go with the ifdef for now. Hopefully not to far in the future we'll get aura working on windows, at which point this will all just work. -Scott On Thu, Nov 8, 2012 at 9:28 AM, <bartfab@chromium.org> wrote: > I am a bit confused now. Just to get back in sync: > > The existing code uses layers. It does so because there is a single widget > that > presents a menu by drawing a number of menu items. When you switch to a > submenu, > the widget's content is cleared and regenerated, drawing the submenu items > instead. It seems that animations between main menu and submenu were an > afterthought. Instead of duplicating the whole widget, the approach was > chosen > to snapshot the old menu into a texture, update the widget and then slide > one > in/out on top of the other. > > So the current situation is that the animation is entirely layer-based. It > should be possible to make each menu its own widget and then animate these > widgets appropriately. But it would be quite some work. The menu that is > sliding > out needs to maintain its appearance but not e.g. react to hovering or > clicking... snapshotting into a texture really seems like the simplest > solution > and a valid use for layers. > > https://chromiumcodereview.appspot.com/11293124/
Sorry, by ifdef I mean not compiling the file at all on windows (non aura). -Scott On Thu, Nov 8, 2012 at 9:42 AM, Scott Violet <sky@chromium.org> wrote: > Sadrul set me straight. Go with the ifdef for now. Hopefully not to > far in the future we'll get aura working on windows, at which point > this will all just work. > > -Scott > > On Thu, Nov 8, 2012 at 9:28 AM, <bartfab@chromium.org> wrote: >> I am a bit confused now. Just to get back in sync: >> >> The existing code uses layers. It does so because there is a single widget >> that >> presents a menu by drawing a number of menu items. When you switch to a >> submenu, >> the widget's content is cleared and regenerated, drawing the submenu items >> instead. It seems that animations between main menu and submenu were an >> afterthought. Instead of duplicating the whole widget, the approach was >> chosen >> to snapshot the old menu into a texture, update the widget and then slide >> one >> in/out on top of the other. >> >> So the current situation is that the animation is entirely layer-based. It >> should be possible to make each menu its own widget and then animate these >> widgets appropriately. But it would be quite some work. The menu that is >> sliding >> out needs to maintain its appearance but not e.g. react to hovering or >> clicking... snapshotting into a texture really seems like the simplest >> solution >> and a valid use for layers. >> >> https://chromiumcodereview.appspot.com/11293124/
On 2012/11/08 17:43:00, sky wrote: > Sorry, by ifdef I mean not compiling the file at all on windows (non aura). This is what the current version of the CL does: The gyp excludes the files when aura=0. Please feel free to review. > > -Scott > > On Thu, Nov 8, 2012 at 9:42 AM, Scott Violet <mailto:sky@chromium.org> wrote: > > Sadrul set me straight. Go with the ifdef for now. Hopefully not to > > far in the future we'll get aura working on windows, at which point > > this will all just work. > > > > -Scott > > > > On Thu, Nov 8, 2012 at 9:28 AM, <mailto:bartfab@chromium.org> wrote: > >> I am a bit confused now. Just to get back in sync: > >> > >> The existing code uses layers. It does so because there is a single widget > >> that > >> presents a menu by drawing a number of menu items. When you switch to a > >> submenu, > >> the widget's content is cleared and regenerated, drawing the submenu items > >> instead. It seems that animations between main menu and submenu were an > >> afterthought. Instead of duplicating the whole widget, the approach was > >> chosen > >> to snapshot the old menu into a texture, update the widget and then slide > >> one > >> in/out on top of the other. > >> > >> So the current situation is that the animation is entirely layer-based. It > >> should be possible to make each menu its own widget and then animate these > >> widgets appropriately. But it would be quite some work. The menu that is > >> sliding > >> out needs to maintain its appearance but not e.g. react to hovering or > >> clicking... snapshotting into a texture really seems like the simplest > >> solution > >> and a valid use for layers. > >> > >> https://chromiumcodereview.appspot.com/11293124/
https://chromiumcodereview.appspot.com/11293124/diff/5002/ui/views/views.gyp File ui/views/views.gyp (right): https://chromiumcodereview.appspot.com/11293124/diff/5002/ui/views/views.gyp#... ui/views/views.gyp:458: ['exclude', 'bubble/tray_bubble_view.cc'], nit: specific files should be listed in a "sources!" section.
https://chromiumcodereview.appspot.com/11293124/diff/5002/ui/views/views.gyp File ui/views/views.gyp (right): https://chromiumcodereview.appspot.com/11293124/diff/5002/ui/views/views.gyp#... ui/views/views.gyp:458: ['exclude', 'bubble/tray_bubble_view.cc'], On 2012/11/08 18:32:54, msw wrote: > nit: specific files should be listed in a "sources!" section. Done.
lgtm
https://chromiumcodereview.appspot.com/11293124/diff/3006/ui/views/bubble/tra... File ui/views/bubble/tray_bubble_view.cc (right): https://chromiumcodereview.appspot.com/11293124/diff/3006/ui/views/bubble/tra... ui/views/bubble/tray_bubble_view.cc:263: // Inform host items (models) that their views are being destroyed. At the time you get here, layer()->parent() still valid and does it still have a reference to bubble_content_mask_?
https://chromiumcodereview.appspot.com/11293124/diff/3006/ui/views/bubble/tra... File ui/views/bubble/tray_bubble_view.cc (right): https://chromiumcodereview.appspot.com/11293124/diff/3006/ui/views/bubble/tra... ui/views/bubble/tray_bubble_view.cc:263: // Inform host items (models) that their views are being destroyed. Yes, this happens. But the Layer class handles it automatically in its destructor: if (layer_mask_back_link_) layer_mask_back_link_->SetMaskLayer(NULL);
Nice, LGTM
Hi Antoine, Could you take a look at the ui/compositor/layer.cc change? As far as I can tell, nobody else is actually using mask layers in Views - hence, nobody noticed that they do not pick up scale settings. Hi Mike, Could you do an owner review for the tray_bubble_view changes when you have time?
https://codereview.chromium.org/11293124/diff/12002/ui/views/bubble/tray_bubb... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/11293124/diff/12002/ui/views/bubble/tray_bubb... ui/views/bubble/tray_bubble_view.cc:125: class TrayBubbleContentMask : public ui::Layer, public ui::LayerDelegate { Include layer.h and layer_delegate.h https://codereview.chromium.org/11293124/diff/12002/ui/views/bubble/tray_bubb... ui/views/bubble/tray_bubble_view.cc:127: TrayBubbleContentMask(views::BubbleBorder* border) nit: |border| is only used for its corner radius, perhaps pass that instead? https://codereview.chromium.org/11293124/diff/12002/ui/views/bubble/tray_bubb... ui/views/bubble/tray_bubble_view.cc:144: virtual void OnDeviceScaleFactorChanged(float device_scale_factor) OVERRIDE { Both super classes have a function with this signature, but Layer::OnDeviceScaleFactorChanged isn't virtual, so perhaps this is okay for now. Still, should we be concerned? Can you point to a similar class that is both a Layer and a LayerDelegate? https://codereview.chromium.org/11293124/diff/12002/ui/views/views.gyp File ui/views/views.gyp (right): https://codereview.chromium.org/11293124/diff/12002/ui/views/views.gyp#newcod... ui/views/views.gyp:462: ['exclude', 'widget/native_widget_aura_window_observer.cc'], optional nit: move these files to sources! too, as bonus-points cleanup.
https://codereview.chromium.org/11293124/diff/12002/ui/views/bubble/tray_bubb... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/11293124/diff/12002/ui/views/bubble/tray_bubb... ui/views/bubble/tray_bubble_view.cc:125: class TrayBubbleContentMask : public ui::Layer, public ui::LayerDelegate { On 2012/11/12 19:15:02, msw wrote: > Include layer.h and layer_delegate.h Done. https://codereview.chromium.org/11293124/diff/12002/ui/views/bubble/tray_bubb... ui/views/bubble/tray_bubble_view.cc:127: TrayBubbleContentMask(views::BubbleBorder* border) On 2012/11/12 19:15:02, msw wrote: > nit: |border| is only used for its corner radius, perhaps pass that instead? Done. https://codereview.chromium.org/11293124/diff/12002/ui/views/bubble/tray_bubb... ui/views/bubble/tray_bubble_view.cc:144: virtual void OnDeviceScaleFactorChanged(float device_scale_factor) OVERRIDE { The implementation of OnDeviceScaleFactorChanged actually was nonsense. I should have left it empty as it was - Layer does the right thing, LayerDelegate does not need to do anything. Regarding your question: Yes, there is precedent for this double inheritance in ui/aura/bench/bench_main.cc: class ColoredLayer : public Layer, public LayerDelegate But if it concerns you, the double inheritance can trivially be avoided. TrayBubbleContentMask really only needs to be a LayerDelegate. I made it a Layer as well purely for convenience so that the Layer always gets torn down together with its delegate. I changed it now so that TrayBubbleContentMask has a Layer member, managing lifetime as effectively and avoiding any double inheritance issues. https://codereview.chromium.org/11293124/diff/12002/ui/views/views.gyp File ui/views/views.gyp (right): https://codereview.chromium.org/11293124/diff/12002/ui/views/views.gyp#newcod... ui/views/views.gyp:462: ['exclude', 'widget/native_widget_aura_window_observer.cc'], On 2012/11/12 19:15:02, msw wrote: > optional nit: move these files to sources! too, as bonus-points cleanup. Done.
tray_bubble_view.cc LGTM with nits and out-of-lining (or opposing consensus). https://codereview.chromium.org/11293124/diff/5005/ui/views/bubble/tray_bubbl... File ui/views/bubble/tray_bubble_view.cc (right): https://codereview.chromium.org/11293124/diff/5005/ui/views/bubble/tray_bubbl... ui/views/bubble/tray_bubble_view.cc:129: TrayBubbleContentMask(int corner_radius) : layer_(ui::LAYER_TEXTURED), nit: I think the line breaking/indents were more correct before. https://codereview.chromium.org/11293124/diff/5005/ui/views/bubble/tray_bubbl... ui/views/bubble/tray_bubble_view.cc:134: virtual ~TrayBubbleContentMask() { Our style guide strongly discourages inline virtuals/dtors/etc. I'd define everything out-of-line except layer() here, but will concede if others disagree. https://codereview.chromium.org/11293124/diff/5005/ui/views/bubble/tray_bubbl... ui/views/bubble/tray_bubble_view.cc:138: ui::Layer& layer() { optional nit: define on one line.
https://chromiumcodereview.appspot.com/11293124/diff/5005/ui/views/bubble/tra... File ui/views/bubble/tray_bubble_view.cc (right): https://chromiumcodereview.appspot.com/11293124/diff/5005/ui/views/bubble/tra... ui/views/bubble/tray_bubble_view.cc:129: TrayBubbleContentMask(int corner_radius) : layer_(ui::LAYER_TEXTURED), On 2012/11/13 17:03:26, msw wrote: > nit: I think the line breaking/indents were more correct before. Done. https://chromiumcodereview.appspot.com/11293124/diff/5005/ui/views/bubble/tra... ui/views/bubble/tray_bubble_view.cc:134: virtual ~TrayBubbleContentMask() { On 2012/11/13 17:03:26, msw wrote: > Our style guide strongly discourages inline virtuals/dtors/etc. I'd define > everything out-of-line except layer() here, but will concede if others disagree. Done. https://chromiumcodereview.appspot.com/11293124/diff/5005/ui/views/bubble/tra... ui/views/bubble/tray_bubble_view.cc:138: ui::Layer& layer() { On 2012/11/13 17:03:26, msw wrote: > optional nit: define on one line. Done.
Sorry, I got a couple more nits. https://chromiumcodereview.appspot.com/11293124/diff/17003/ui/views/bubble/tr... File ui/views/bubble/tray_bubble_view.cc (right): https://chromiumcodereview.appspot.com/11293124/diff/17003/ui/views/bubble/tr... ui/views/bubble/tray_bubble_view.cc:129: TrayBubbleContentMask(int corner_radius); nit: explicit https://chromiumcodereview.appspot.com/11293124/diff/17003/ui/views/bubble/tr... ui/views/bubble/tray_bubble_view.cc:132: ui::Layer& layer() { return layer_; } nit: const ref+function or non-const pointer. https://chromiumcodereview.appspot.com/11293124/diff/17003/ui/views/bubble/tr... ui/views/bubble/tray_bubble_view.cc:147: : layer_(ui::LAYER_TEXTURED), corner_radius_(corner_radius) { nit: one init per line (since they don't fit above).
https://chromiumcodereview.appspot.com/11293124/diff/17003/ui/views/bubble/tr... File ui/views/bubble/tray_bubble_view.cc (right): https://chromiumcodereview.appspot.com/11293124/diff/17003/ui/views/bubble/tr... ui/views/bubble/tray_bubble_view.cc:129: TrayBubbleContentMask(int corner_radius); On 2012/11/13 20:28:04, msw wrote: > nit: explicit Done. https://chromiumcodereview.appspot.com/11293124/diff/17003/ui/views/bubble/tr... ui/views/bubble/tray_bubble_view.cc:132: ui::Layer& layer() { return layer_; } On 2012/11/13 20:28:04, msw wrote: > nit: const ref+function or non-const pointer. Done. https://chromiumcodereview.appspot.com/11293124/diff/17003/ui/views/bubble/tr... ui/views/bubble/tray_bubble_view.cc:147: : layer_(ui::LAYER_TEXTURED), corner_radius_(corner_radius) { On 2012/11/13 20:28:04, msw wrote: > nit: one init per line (since they don't fit above). Done.
LGTM with one last nit (I'm very sorry for the churn). https://chromiumcodereview.appspot.com/11293124/diff/10005/ui/views/bubble/tr... File ui/views/bubble/tray_bubble_view.cc (right): https://chromiumcodereview.appspot.com/11293124/diff/10005/ui/views/bubble/tr... ui/views/bubble/tray_bubble_view.cc:167: float device_scale_factor) { nit: comment like ImageGrid::ImagePainter::OnDeviceScaleFactorChanged: "// Redrawing will take care of scale factor change." (or similar, to indicate why no-op versus calling layer_->OnDeviceScaleFactorChanged is appropriate)
All done. Only waiting for Antoine's review now. https://chromiumcodereview.appspot.com/11293124/diff/10005/ui/views/bubble/tr... File ui/views/bubble/tray_bubble_view.cc (right): https://chromiumcodereview.appspot.com/11293124/diff/10005/ui/views/bubble/tr... ui/views/bubble/tray_bubble_view.cc:167: float device_scale_factor) { On 2012/11/13 21:13:38, msw wrote: > nit: comment like ImageGrid::ImagePainter::OnDeviceScaleFactorChanged: > "// Redrawing will take care of scale factor change." > (or similar, to indicate why no-op versus calling > layer_->OnDeviceScaleFactorChanged is appropriate) Done.
Hi Jonathan, Could you take a look at the ui/compositor/layer.cc change? I had asked Antoine to review but I see that he is OOO right now.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartfab@chromium.org/11293124/13012
Change committed as 167967
On Mon, Nov 12, 2012 at 10:16 AM, <bartfab@chromium.org> wrote: > Hi Antoine, > > Could you take a look at the ui/compositor/layer.cc change? As far as I can > tell, nobody else is actually using mask layers in Views - hence, nobody > noticed > that they do not pick up scale settings. > LGTM for that, but FYI, masks are very expensive. Did you check the performance on device? I would hate for the frame rate to drop when a bubble comes up. Antoine > Hi Mike, > > Could you do an owner review for the tray_bubble_view changes when you have > time? > > https://codereview.chromium.**org/11293124/<https://codereview.chromium.org/1... >
I will try on a Lucas tomorrow. On 2012/11/19 23:03:28, piman wrote: > On Mon, Nov 12, 2012 at 10:16 AM, <mailto:bartfab@chromium.org> wrote: > > > Hi Antoine, > > > > Could you take a look at the ui/compositor/layer.cc change? As far as I can > > tell, nobody else is actually using mask layers in Views - hence, nobody > > noticed > > that they do not pick up scale settings. > > > > LGTM for that, but FYI, masks are very expensive. Did you check the > performance on device? I would hate for the frame rate to drop when a > bubble comes up. > > Antoine > > > > Hi Mike, > > > > Could you do an owner review for the tray_bubble_view changes when you have > > time? > > > > > https://codereview.chromium.**org/11293124/%3Chttps://codereview.chromium.org...> > >
I just tested performance against a build from ~2 weeks ago. There appear to be no regression. The bubble is snappy, an HTML5 benchmark running while the bubble is open reaches the same speeds as it did with the older build. On 2012/11/19 23:18:43, bartfab wrote: > I will try on a Lucas tomorrow. > > On 2012/11/19 23:03:28, piman wrote: > > On Mon, Nov 12, 2012 at 10:16 AM, <mailto:bartfab@chromium.org> wrote: > > > > > Hi Antoine, > > > > > > Could you take a look at the ui/compositor/layer.cc change? As far as I can > > > tell, nobody else is actually using mask layers in Views - hence, nobody > > > noticed > > > that they do not pick up scale settings. > > > > > > > LGTM for that, but FYI, masks are very expensive. Did you check the > > performance on device? I would hate for the frame rate to drop when a > > bubble comes up. > > > > Antoine > > > > > > > Hi Mike, > > > > > > Could you do an owner review for the tray_bubble_view changes when you have > > > time? > > > > > > > > > https://codereview.chromium.**org/11293124/%253Chttps://codereview.chromium.o...> > > >
As a final data point before I let this CL rest in peace: I just verified that there are no performance regressions when the bubble is open and/or being scrolled on Mario either. On 2012/11/20 13:18:41, bartfab wrote: > I just tested performance against a build from ~2 weeks ago. There appear to be > no regression. The bubble is snappy, an HTML5 benchmark running while the bubble > is open reaches the same speeds as it did with the older build. > > On 2012/11/19 23:18:43, bartfab wrote: > > I will try on a Lucas tomorrow. > > > > On 2012/11/19 23:03:28, piman wrote: > > > On Mon, Nov 12, 2012 at 10:16 AM, <mailto:bartfab@chromium.org> wrote: > > > > > > > Hi Antoine, > > > > > > > > Could you take a look at the ui/compositor/layer.cc change? As far as I > can > > > > tell, nobody else is actually using mask layers in Views - hence, nobody > > > > noticed > > > > that they do not pick up scale settings. > > > > > > > > > > LGTM for that, but FYI, masks are very expensive. Did you check the > > > performance on device? I would hate for the frame rate to drop when a > > > bubble comes up. > > > > > > Antoine > > > > > > > > > > Hi Mike, > > > > > > > > Could you do an owner review for the tray_bubble_view changes when you > have > > > > time? > > > > > > > > > > > > > > https://codereview.chromium.**org/11293124/%25253Chttps://codereview.chromium...> > > > > |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
