Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
251193004:
Animate the OverviewButtonTray (Closed)
|
Created:
6 years, 7 months ago by jonross Modified:
6 years, 7 months ago CC:
chromium-reviews, kalyank, sadrul, ben+ash_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAnimate the OverviewButtonTray
Take the animations from TrayItemView and port them to TrayBackgroundView
so that the outer trays also animate their visibility.
TEST=OverviewButtonTray
TEST=SystemTrayTest
TEST=WebNotificationTrayTest
BUG=363714
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269378
Patch Set 1 #
Total comments: 8
Patch Set 2 : Reimplement animation using compositor layers #
Total comments: 10
Patch Set 3 : #Patch Set 4 : Animate Container of the tray items #
Total comments: 14
Patch Set 5 : #
Total comments: 4
Patch Set 6 : #
Total comments: 8
Patch Set 7 : #
Total comments: 16
Patch Set 8 : #Patch Set 9 : #
Messages
Total messages: 22 (0 generated)
I wouldn't list those tests as they specifically disable animations, so they're not really verifying the animation. Also, it looks like you might be able to do this using compositor animations, since the actual animation is implemented using a layer transform. Can you try to do it this way? You can see many examples if you search for ui::ScopedLayerAnimationSettings: https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&typ.... Doing a compositor animation is not only faster but also means that from a code perspective things can be treated as if they've immediately updated to the final positions. https://codereview.chromium.org/251193004/diff/1/ash/system/tray/tray_backgro... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/251193004/diff/1/ash/system/tray/tray_backgro... ash/system/tray/tray_background_view.cc:44: bool animations_enabled = true; nit: newline before end of namespace. https://codereview.chromium.org/251193004/diff/1/ash/system/tray/tray_backgro... ash/system/tray/tray_background_view.cc:343: AnimationProgressed(animation_.get()); Isn't AnimationProgressed supposed to be called automatically from the animation? https://codereview.chromium.org/251193004/diff/1/ash/system/tray/tray_backgro... ash/system/tray/tray_background_view.cc:472: animations_enabled = false; I realize this pattern is copied from elsewhere, but long-term we should probably have a single global for all animations similar to (or perhaps making more generic) this: https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/scop.... Maybe a TODO? The idea being that this code wouldn't have to have a special case for no animations, it would just be the case that gfx::Animations complete instantly in tests unless otherwise requested. https://codereview.chromium.org/251193004/diff/1/ash/system/tray/tray_backgro... ash/system/tray/tray_background_view.cc:480: static_cast<double>(height()) / 2, 0.)); nit: The static_cast is unnecessary for this sort of thing, the implicit cast from height() * 0.5 is fine. https://codereview.chromium.org/251193004/diff/1/ash/system/tray/tray_backgro... ash/system/tray/tray_background_view.cc:483: static_cast<double>(width() / 2), 0.), 0); ditto. https://codereview.chromium.org/251193004/diff/1/ash/system/tray/tray_backgro... ash/system/tray/tray_background_view.cc:487: layer()->SetTransform(transform); The animation is done as a layer transform? Would it work to do all of the animations as ScopedLayerAnimations? I.e. just use https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/scop.... https://codereview.chromium.org/251193004/diff/1/ash/system/tray/tray_backgro... ash/system/tray/tray_background_view.cc:497: AnimationEnded(animation); Isn't this not guaranteed to give the correct result since in AnimationEnded you base whether or not to hide on the current value. Presumably a canceled animation should immediately update to end state?
I've reimplemented the animation using compositor layers. I still need to make the outer container animate its change in size. However I wanted a first look at my approach. https://codereview.chromium.org/251193004/diff/1/ash/system/tray/tray_backgro... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/251193004/diff/1/ash/system/tray/tray_backgro... ash/system/tray/tray_background_view.cc:44: bool animations_enabled = true; On 2014/04/29 14:18:22, flackr wrote: > nit: newline before end of namespace. Done.
https://codereview.chromium.org/251193004/diff/40001/ash/system/tray/tray_bac... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/251193004/diff/40001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:329: void TrayBackgroundView::SetVisible(bool set_visible) { s/set_visible/visible to match the variable name in definition in .h file and overridden method. https://codereview.chromium.org/251193004/diff/40001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:334: views::View::SetVisible(set_visible); I believe you should be able to SetVisible(false) in the scope of the ScopedLayerAnimationSettings, and then it will automagically keep the layer visible until the animation is complete and you won't need the animation observer. https://codereview.chromium.org/251193004/diff/40001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:354: shelf_alignment_ == SHELF_ALIGNMENT_TOP) { nit: bad indent, should line up with above line. https://codereview.chromium.org/251193004/diff/40001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:359: transform.Scale(0.01f, 0.01f); Use constants for non 0 or 1 values. https://codereview.chromium.org/251193004/diff/40001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:434: // transform to be the offscreen position for the new alignment. It'd probably make more sense to set the correct transform to transition in from in Show before the scope of the ScopedLayerAnimationSettings object.
https://codereview.chromium.org/251193004/diff/40001/ash/system/tray/tray_bac... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/251193004/diff/40001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:329: void TrayBackgroundView::SetVisible(bool set_visible) { On 2014/04/30 16:06:40, flackr wrote: > s/set_visible/visible to match the variable name in definition in .h file and > overridden method. Done. https://codereview.chromium.org/251193004/diff/40001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:334: views::View::SetVisible(set_visible); On 2014/04/30 16:06:40, flackr wrote: > I believe you should be able to SetVisible(false) in the scope of the > ScopedLayerAnimationSettings, and then it will automagically keep the layer > visible until the animation is complete and you won't need the animation > observer. In order to user layer()->SetVisibility(false) I would need to detach the layer from the tree. This is typically done when a window is about to be destroyed. Other areas use the pattern of an animation listener. For this change we will stick with this pattern. https://codereview.chromium.org/251193004/diff/40001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:354: shelf_alignment_ == SHELF_ALIGNMENT_TOP) { On 2014/04/30 16:06:40, flackr wrote: > nit: bad indent, should line up with above line. Done. https://codereview.chromium.org/251193004/diff/40001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:359: transform.Scale(0.01f, 0.01f); On 2014/04/30 16:06:40, flackr wrote: > Use constants for non 0 or 1 values. Done. https://codereview.chromium.org/251193004/diff/40001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:434: // transform to be the offscreen position for the new alignment. On 2014/04/30 16:06:40, flackr wrote: > It'd probably make more sense to set the correct transform to transition in from > in Show before the scope of the ScopedLayerAnimationSettings object. Done.
Animation for both the tray items, as well as their layout within their parent container. Ready for review.
https://codereview.chromium.org/251193004/diff/70001/ash/system/status_area_w... File ash/system/status_area_widget_delegate.cc (right): https://codereview.chromium.org/251193004/diff/70001/ash/system/status_area_w... ash/system/status_area_widget_delegate.cc:158: StatusAreaWidgetDelegate::SetupAnimationForLayer(ui::Layer* layer) { Use anonymous namespace, though it'd probably be better to subclass ScopedLayerAnimationSettings to override these. https://codereview.chromium.org/251193004/diff/70001/ash/system/tray/tray_bac... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/251193004/diff/70001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:46: const int kAnimationDurationForVisibility = 250; Does this need to match the status area widget duration? If so, you should share a constant. https://codereview.chromium.org/251193004/diff/70001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:340: HideTransformation(); Given visible != this->visible(), if !this->visible() then visible is true, merge with following if statement. https://codereview.chromium.org/251193004/diff/70001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:344: views::View::SetVisible(visible); Comment that SetVisible(false) is deferred until after the animation completes in OnImplicitAnimationsCompleted. I'm not sure if this will be problematic in terms of visibility calculations from other classes but I defer to someone else for this. https://codereview.chromium.org/251193004/diff/70001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:356: layer()->GetAnimator()->SchedulePauseForProperties( Why is the show delayed? Add comment as to why. https://codereview.chromium.org/251193004/diff/70001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:366: layer()->SetOpacity(0.0f); Perhaps also layer()->SetVisible(false); https://codereview.chromium.org/251193004/diff/70001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:473: if (layer()->opacity() == 0.0f) It might be more robust to compare the layer's visibility to the view's visibility and change the view if they don't match.
https://codereview.chromium.org/251193004/diff/70001/ash/system/status_area_w... File ash/system/status_area_widget_delegate.cc (right): https://codereview.chromium.org/251193004/diff/70001/ash/system/status_area_w... ash/system/status_area_widget_delegate.cc:158: StatusAreaWidgetDelegate::SetupAnimationForLayer(ui::Layer* layer) { On 2014/05/06 15:42:11, flackr wrote: > Use anonymous namespace, though it'd probably be better to subclass > ScopedLayerAnimationSettings to override these. Done. https://codereview.chromium.org/251193004/diff/70001/ash/system/tray/tray_bac... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/251193004/diff/70001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:46: const int kAnimationDurationForVisibility = 250; On 2014/05/06 15:42:11, flackr wrote: > Does this need to match the status area widget duration? If so, you should share > a constant. They currently match in the spec. However the spec is due for tweaking once they can test the implementation. So they are not guaranteed to remain the same. https://codereview.chromium.org/251193004/diff/70001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:340: HideTransformation(); On 2014/05/06 15:42:11, flackr wrote: > Given visible != this->visible(), if !this->visible() then visible is true, > merge with following if statement. Done. https://codereview.chromium.org/251193004/diff/70001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:344: views::View::SetVisible(visible); On 2014/05/06 15:42:11, flackr wrote: > Comment that SetVisible(false) is deferred until after the animation completes > in OnImplicitAnimationsCompleted. > > I'm not sure if this will be problematic in terms of visibility calculations > from other classes but I defer to someone else for this. Done. https://codereview.chromium.org/251193004/diff/70001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:356: layer()->GetAnimator()->SchedulePauseForProperties( On 2014/05/06 15:42:11, flackr wrote: > Why is the show delayed? Add comment as to why. Done. https://codereview.chromium.org/251193004/diff/70001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:366: layer()->SetOpacity(0.0f); On 2014/05/06 15:42:11, flackr wrote: > Perhaps also layer()->SetVisible(false); Done. https://codereview.chromium.org/251193004/diff/70001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:473: if (layer()->opacity() == 0.0f) On 2014/05/06 15:42:11, flackr wrote: > It might be more robust to compare the layer's visibility to the view's > visibility and change the view if they don't match. Moved the listener to only be added for the !visible case. Since it is the only one for which we have an action.
LGTM with nits. https://codereview.chromium.org/251193004/diff/80001/ash/system/status_area_w... File ash/system/status_area_widget_delegate.cc (right): https://codereview.chromium.org/251193004/diff/80001/ash/system/status_area_w... ash/system/status_area_widget_delegate.cc:34: : ui::ScopedLayerAnimationSettings(layer->GetAnimator()) { Separate out implementation: http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in... https://codereview.chromium.org/251193004/diff/80001/ash/system/tray/tray_bac... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/251193004/diff/80001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:47: const int kShowAnimationDelay = 100; It might be worth briefly describing these parameters.
Hi Oshima, Could you review an animation for the status area on touchview? https://codereview.chromium.org/251193004/diff/80001/ash/system/status_area_w... File ash/system/status_area_widget_delegate.cc (right): https://codereview.chromium.org/251193004/diff/80001/ash/system/status_area_w... ash/system/status_area_widget_delegate.cc:34: : ui::ScopedLayerAnimationSettings(layer->GetAnimator()) { On 2014/05/07 21:32:26, flackr wrote: > Separate out implementation: > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in... Done. https://codereview.chromium.org/251193004/diff/80001/ash/system/tray/tray_bac... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/251193004/diff/80001/ash/system/tray/tray_bac... ash/system/tray/tray_background_view.cc:47: const int kShowAnimationDelay = 100; On 2014/05/07 21:32:26, flackr wrote: > It might be worth briefly describing these parameters. Done.
https://codereview.chromium.org/251193004/diff/100001/ash/system/status_area_... File ash/system/status_area_widget_delegate.cc (right): https://codereview.chromium.org/251193004/diff/100001/ash/system/status_area_... ash/system/status_area_widget_delegate.cc:42: : ui::ScopedLayerAnimationSettings(layer->GetAnimator()) { nit: I think this should align with the S since it's at the same lexical? depth https://codereview.chromium.org/251193004/diff/100001/ash/system/tray/tray_ba... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/251193004/diff/100001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.cc:47: // Animation constant based on visual design. During of opacity animation Did you mean duration of opacity ...? nit: "Animation constant based on visual design." doesn't really add anything to what the variable means, I'd remove it, just keep the description of what the duration is of. Same for next comment. https://codereview.chromium.org/251193004/diff/100001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.cc:49: const int kAnimationDurationForVisibility = 250; nit: This should have units in the name, i.e. kAnimationDurationForVisibilityMs https://codereview.chromium.org/251193004/diff/100001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.cc:54: const int kShowAnimationDelay = 100; Ditto.
https://codereview.chromium.org/251193004/diff/100001/ash/system/status_area_... File ash/system/status_area_widget_delegate.cc (right): https://codereview.chromium.org/251193004/diff/100001/ash/system/status_area_... ash/system/status_area_widget_delegate.cc:42: : ui::ScopedLayerAnimationSettings(layer->GetAnimator()) { On 2014/05/08 17:09:49, flackr wrote: > nit: I think this should align with the S since it's at the same lexical? depth Done. https://codereview.chromium.org/251193004/diff/100001/ash/system/tray/tray_ba... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/251193004/diff/100001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.cc:47: // Animation constant based on visual design. During of opacity animation On 2014/05/08 17:09:49, flackr wrote: > Did you mean duration of opacity ...? > > nit: "Animation constant based on visual design." doesn't really add anything to > what the variable means, I'd remove it, just keep the description of what the > duration is of. Same for next comment. Done. https://codereview.chromium.org/251193004/diff/100001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.cc:49: const int kAnimationDurationForVisibility = 250; On 2014/05/08 17:09:49, flackr wrote: > nit: This should have units in the name, i.e. kAnimationDurationForVisibilityMs Done. https://codereview.chromium.org/251193004/diff/100001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.cc:54: const int kShowAnimationDelay = 100; On 2014/05/08 17:09:49, flackr wrote: > Ditto. Done.
https://codereview.chromium.org/251193004/diff/120001/ash/system/status_area_... File ash/system/status_area_widget_delegate.cc (right): https://codereview.chromium.org/251193004/diff/120001/ash/system/status_area_... ash/system/status_area_widget_delegate.cc:33: StatusAreaWidgetDelegateAnimationSettings(ui::Layer* layer); explicit https://codereview.chromium.org/251193004/diff/120001/ash/system/status_area_... ash/system/status_area_widget_delegate.cc:34: virtual ~StatusAreaWidgetDelegateAnimationSettings(); just inline empty dtor. You may also inline ctor as it's in .cc, but it's up to you. https://codereview.chromium.org/251193004/diff/120001/ash/system/status_area_... ash/system/status_area_widget_delegate.cc:47: SetTweenType(gfx::Tween::EASE_IN_OUT); indent https://codereview.chromium.org/251193004/diff/120001/ash/system/tray/tray_ba... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/251193004/diff/120001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.cc:346: HideTransformation(); If you need this, don't you also need to set the opacity/visibility here as well? https://codereview.chromium.org/251193004/diff/120001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.cc:368: layer()->SetOpacity(1.0f); don't you have to set layer()->SetVisible(true); ? https://codereview.chromium.org/251193004/diff/120001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.cc:488: void TrayBackgroundView::HideTransformation() { Set/ApplyHideTransformation ?
https://codereview.chromium.org/251193004/diff/120001/ash/system/status_area_... File ash/system/status_area_widget_delegate.cc (right): https://codereview.chromium.org/251193004/diff/120001/ash/system/status_area_... ash/system/status_area_widget_delegate.cc:34: virtual ~StatusAreaWidgetDelegateAnimationSettings(); On 2014/05/08 17:38:42, oshima wrote: > just inline empty dtor. You may also inline ctor as it's in .cc, but it's up to > you. Oh, even though it's virtual extending ScopedLayerAnimationSettings? http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...
https://codereview.chromium.org/251193004/diff/120001/ash/system/tray/tray_ba... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/251193004/diff/120001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.cc:346: HideTransformation(); On 2014/05/08 17:38:42, oshima wrote: > If you need this, don't you also need to set the opacity/visibility here as > well? No, the setting of position is because the tray can be rotated while the button is hidden. So the position it animated to could be invalid for the animation to visibility. The rotation changes won't affect opacity, which will be off. https://codereview.chromium.org/251193004/diff/120001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.cc:368: layer()->SetOpacity(1.0f); On 2014/05/08 17:38:42, oshima wrote: > don't you have to set layer()->SetVisible(true); ? The setting of the view to visible at line 350 turns the layer visibility back on.
https://codereview.chromium.org/251193004/diff/120001/ash/system/status_area_... File ash/system/status_area_widget_delegate.cc (right): https://codereview.chromium.org/251193004/diff/120001/ash/system/status_area_... ash/system/status_area_widget_delegate.cc:34: virtual ~StatusAreaWidgetDelegateAnimationSettings(); On 2014/05/08 17:43:14, flackr wrote: > On 2014/05/08 17:38:42, oshima wrote: > > just inline empty dtor. You may also inline ctor as it's in .cc, but it's up > to > > you. > > Oh, even though it's virtual extending ScopedLayerAnimationSettings? > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in... Notice the word "in headers" :)
https://codereview.chromium.org/251193004/diff/120001/ash/system/status_area_... File ash/system/status_area_widget_delegate.cc (right): https://codereview.chromium.org/251193004/diff/120001/ash/system/status_area_... ash/system/status_area_widget_delegate.cc:34: virtual ~StatusAreaWidgetDelegateAnimationSettings(); On 2014/05/08 18:13:45, oshima wrote: > On 2014/05/08 17:43:14, flackr wrote: > > On 2014/05/08 17:38:42, oshima wrote: > > > just inline empty dtor. You may also inline ctor as it's in .cc, but it's up > > to > > > you. > > > > Oh, even though it's virtual extending ScopedLayerAnimationSettings? > > > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in... > > Notice the word "in headers" :) Done. https://codereview.chromium.org/251193004/diff/120001/ash/system/status_area_... ash/system/status_area_widget_delegate.cc:47: SetTweenType(gfx::Tween::EASE_IN_OUT); On 2014/05/08 17:38:42, oshima wrote: > indent Done.
lgtm https://codereview.chromium.org/251193004/diff/120001/ash/system/tray/tray_ba... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/251193004/diff/120001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.cc:346: HideTransformation(); On 2014/05/08 17:50:07, jonross wrote: > On 2014/05/08 17:38:42, oshima wrote: > > If you need this, don't you also need to set the opacity/visibility here as > > well? > > No, the setting of position is because the tray can be rotated while the button > is hidden. So the position it animated to could be invalid for the animation to > visibility. > > The rotation changes won't affect opacity, which will be off. I see, make sense. Can you update the description a bit like "the self can change while hidden." https://codereview.chromium.org/251193004/diff/120001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.cc:368: layer()->SetOpacity(1.0f); On 2014/05/08 17:50:07, jonross wrote: > On 2014/05/08 17:38:42, oshima wrote: > > don't you have to set layer()->SetVisible(true); ? > > The setting of the view to visible at line 350 turns the layer visibility back > on. can you document it? The code that looks to cause opacity / visibility inconsistency may confuse us later.
https://codereview.chromium.org/251193004/diff/120001/ash/system/tray/tray_ba... File ash/system/tray/tray_background_view.cc (right): https://codereview.chromium.org/251193004/diff/120001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.cc:346: HideTransformation(); On 2014/05/08 18:30:54, oshima wrote: > On 2014/05/08 17:50:07, jonross wrote: > > On 2014/05/08 17:38:42, oshima wrote: > > > If you need this, don't you also need to set the opacity/visibility here as > > > well? > > > > No, the setting of position is because the tray can be rotated while the > button > > is hidden. So the position it animated to could be invalid for the animation > to > > visibility. > > > > The rotation changes won't affect opacity, which will be off. > > I see, make sense. Can you update the description a bit like "the self can > change while hidden." Done. https://codereview.chromium.org/251193004/diff/120001/ash/system/tray/tray_ba... ash/system/tray/tray_background_view.cc:368: layer()->SetOpacity(1.0f); On 2014/05/08 18:30:54, oshima wrote: > On 2014/05/08 17:50:07, jonross wrote: > > On 2014/05/08 17:38:42, oshima wrote: > > > don't you have to set layer()->SetVisible(true); ? > > > > The setting of the view to visible at line 350 turns the layer visibility back > > on. > > can you document it? The code that looks to cause opacity / visibility > inconsistency may confuse us later. Done.
The CQ bit was checked by jonross@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jonross@chromium.org/251193004/160001
Message was sent while issue was closed.
Change committed as 269378 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
