Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1588093004:
Compute if a layer is drawn without LayerTree hierarchy (Closed)
|
Created:
4 years, 11 months ago by jaydasika Modified:
4 years, 11 months ago CC:
cc-bugs_chromium.org, chromium-reviews, danakj+watch_chromium.org, jbauman+watch_chromium.org, kalyank, piman+watch_chromium.org, sievers+watch_chromium.org, Ian Vollick Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis CL :
* deletes is_hidden
* computes if a layer is drawn using the effect tree
BUG=575413
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/5bd215b33e6343d8ae7e52fb822dd552bf8b59a7
Cr-Commit-Position: refs/heads/master@{#371063}
Patch Set 1 #Patch Set 2 : #
Total comments: 16
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 20
Patch Set 6 : Rebase #Patch Set 7 : #
Total comments: 4
Patch Set 8 : #Patch Set 9 : #
Total comments: 4
Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #
Total comments: 4
Patch Set 13 : #Patch Set 14 : #
Messages
Total messages: 54 (7 generated)
Description was changed from ========== Move hide subtree feature to ui compositor BUG= ========== to ========== Move hide subtree feature to ui compositor BUG= CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Description was changed from ========== Move hide subtree feature to ui compositor BUG= CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== This CL : * moves hide subtree feature to ui compositor. * deletes is_hidden and hide_layer_and_subtree from cc::Layer * computes if a layer is drawn using the effect tree BUG=575413 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
jaydasika@chromium.org changed reviewers: + ajuma@chromium.org, enne@chromium.org, vollick@chromium.org, weiliangc@chromium.org
PTAL
Higher level comment: now that we're using opacity 0 to hide subtrees, whether a layer is drawn purely depends on opacity and on copy requests. Since non-1 opacity and copy requests each cause effect nodes to be created, "drawn-ness" can only change at effect nodes (that is, layers that share the same effect node are either all drawn or all hidden). Should this become purely an effect tree concept then, rather than a property of layers? So instead of having, say, LayerImpl::IsHidden or LayerImpl::IsDrawnFromPropertyTrees, would it make sense to have EffectTree methods telling us these things? https://codereview.chromium.org/1588093004/diff/20001/cc/layers/layer.cc File cc/layers/layer.cc (right): https://codereview.chromium.org/1588093004/diff/20001/cc/layers/layer.cc#newc... cc/layers/layer.cc:1920: // Layers that have screen space opacity are hidden. So they are not drawn. "screen space opacity 0"? https://codereview.chromium.org/1588093004/diff/20001/cc/layers/layer.cc#newc... cc/layers/layer.cc:1925: if (HasPotentiallyRunningOpacityAnimation() || This only checks if the layer itself has an animation. It doesn't handle the case that some ancestor has an opacity animation. For example, say we have A->B->C, where A has an opacity animation and currently has opacity 0. In that case, C's screen space opacity will be 0 right now, and C itself won't have an opacity animation, but C's screen space opacity can still become non-0 during A's opacity animation. https://codereview.chromium.org/1588093004/diff/20001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/1588093004/diff/20001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:1898: // Layers that have screen space opacity are hidden. So they are not drawn. "screen space opacity 0" https://codereview.chromium.org/1588093004/diff/20001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:1903: if (HasPotentiallyRunningOpacityAnimation() || Do we need the animation check on the compositor side? It makes sense on the main thread since we can't predict what the opacity will be on the compositor thread, but on the compositor side we know the current opacity. https://codereview.chromium.org/1588093004/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/1588093004/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common.cc:2403: layer_is_drawn && !(layer->IsHidden() && layer->HasCopyRequest()); This seems almost equivalent to: layer_is_drawn && !layer->IsHidden() except the latter says hidden layers with background filters don't contribute to a drawn surface and the former says they do unless the have a copy request? Do background filters currently affect whether a layer contributes to a drawn surface? https://codereview.chromium.org/1588093004/diff/20001/cc/trees/property_tree_... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/1588093004/diff/20001/cc/trees/property_tree_... cc/trees/property_tree_builder.cc:584: node.data.contributes_to_copy_request = Does this value need to get updated on the compositor side after copy requests are serviced?
https://codereview.chromium.org/1588093004/diff/20001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/1588093004/diff/20001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:1903: if (HasPotentiallyRunningOpacityAnimation() || On 2016/01/15 15:02:46, ajuma wrote: > Do we need the animation check on the compositor side? It makes sense on the > main thread since we can't predict what the opacity will be on the compositor > thread, but on the compositor side we know the current opacity. Won't we need it to handle animating opacity on the pending tree ? https://codereview.chromium.org/1588093004/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/1588093004/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common.cc:2403: layer_is_drawn && !(layer->IsHidden() && layer->HasCopyRequest()); On 2016/01/15 15:02:46, ajuma wrote: > This seems almost equivalent to: > layer_is_drawn && !layer->IsHidden() > > except the latter says hidden layers with background filters don't contribute to > a drawn surface and the former says they do unless the have a copy request? Do > background filters currently affect whether a layer contributes to a drawn > surface? Is we have R->R1->R2, render surfaces R1 and R2 are hidden and R1 has copy request, R1, R2 are drawn, only R2 should contribute to drawn surface (as its target is R1 and it should contribute), R1 shouldn't (as its target is R and it shouldn't contribute). So, layer_is_drawn && !layer->IsHidden() won't work for this case.
https://codereview.chromium.org/1588093004/diff/20001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/1588093004/diff/20001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:1903: if (HasPotentiallyRunningOpacityAnimation() || On 2016/01/15 18:23:05, jaydasika wrote: > On 2016/01/15 15:02:46, ajuma wrote: > > Do we need the animation check on the compositor side? It makes sense on the > > main thread since we can't predict what the opacity will be on the compositor > > thread, but on the compositor side we know the current opacity. > > Won't we need it to handle animating opacity on the pending tree ? Ah, good point. https://codereview.chromium.org/1588093004/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/1588093004/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common.cc:2403: layer_is_drawn && !(layer->IsHidden() && layer->HasCopyRequest()); On 2016/01/15 18:23:05, jaydasika wrote: > On 2016/01/15 15:02:46, ajuma wrote: > > This seems almost equivalent to: > > layer_is_drawn && !layer->IsHidden() > > > > except the latter says hidden layers with background filters don't contribute > to > > a drawn surface and the former says they do unless the have a copy request? Do > > background filters currently affect whether a layer contributes to a drawn > > surface? > > Is we have R->R1->R2, render surfaces R1 and R2 are hidden and R1 has copy > request, R1, R2 are drawn, only R2 should contribute to drawn surface (as its > target is R1 and it should contribute), R1 shouldn't (as its target is R and it > shouldn't contribute). So, layer_is_drawn && !layer->IsHidden() won't work for > this case. Hmm, true. layer_is_drawn (roughly, ignoring background filters and animations which I don't think the current code uses here) amounts to "effect node has screen space opacity non-0 or contributes to copy request". layer->IsHidden() is "effect node has screen space opacity 0". So contributes_to_drawn_surface amounts to "effect node has screen space opacity non-0 or contributes to copy request, and either the effect node has screen space opacity non-0 or the layer doesn't have a copy request". That's pretty confusing :) Could this be made simpler or clearer somehow?
Some high level comments. In my mind how this works is that effect node would have has_copy_request, and that is set with layer->HasCopyRequest(). And to determine whether a layer is drawn to screen, you can look at its screen_space_opacity. Whether a layer contributes to its target is determined by whether effect node's opacity is 0. And to solve edge case where a layer has both copy request and hide_layer_and_subtree, we could add two effect nodes, parent one has opacity 0, and child one has opacity 1 with copy request, and layer will point to child effect node. This should reduce some complexity. WDYT? https://codereview.chromium.org/1588093004/diff/20001/ui/compositor/layer_uni... File ui/compositor/layer_unittest.cc (right): https://codereview.chromium.org/1588093004/diff/20001/ui/compositor/layer_uni... ui/compositor/layer_unittest.cc:842: TEST_F(LayerWithNullDelegateTest, Opacity) { Preferably with more descriptive test name.
On 2016/01/15 19:30:24, weiliangc wrote: > Some high level comments. > > In my mind how this works is that effect node would have has_copy_request, and > that is set with layer->HasCopyRequest(). > > And to determine whether a layer is drawn to screen, you can look at its > screen_space_opacity. > Whether a layer contributes to its target is determined by whether effect node's > opacity is 0. If we have RS1->RS2, RS1->opacity = 0 and RS2->opacity = 1. If we use just the opacity to determine if a layer contributes to its target, we would be wrong in this case as RS2's opacity is 1 but still its not drawn and doesn't contribute to target. > > And to solve edge case where a layer has both copy request and > hide_layer_and_subtree, we could add two effect nodes, parent one has opacity 0, > and child one has opacity 1 with copy request, and layer will point to child > effect node. > > This should reduce some complexity. WDYT? I am not sure if adding 2 effect nodes to a single layer will reduce complexity ? I had a discussion with Ali about it and his opinion was that ideally we wouldn't want to end up with 2 effect nodes with the same owner_id. > > https://codereview.chromium.org/1588093004/diff/20001/ui/compositor/layer_uni... > File ui/compositor/layer_unittest.cc (right): > > https://codereview.chromium.org/1588093004/diff/20001/ui/compositor/layer_uni... > ui/compositor/layer_unittest.cc:842: TEST_F(LayerWithNullDelegateTest, Opacity) > { > Preferably with more descriptive test name.
On 2016/01/15 at 19:49:40, jaydasika wrote: > On 2016/01/15 19:30:24, weiliangc wrote: > > Some high level comments. > > > > In my mind how this works is that effect node would have has_copy_request, and > > that is set with layer->HasCopyRequest(). > > > > And to determine whether a layer is drawn to screen, you can look at its > > screen_space_opacity. > > Whether a layer contributes to its target is determined by whether effect node's > > opacity is 0. > If we have RS1->RS2, RS1->opacity = 0 and RS2->opacity = 1. If we use just the opacity to determine > if a layer contributes to its target, we would be wrong in this case as RS2's opacity is 1 but still > its not drawn and doesn't contribute to target. Well if RS1->RS2->L then L will contributes to RS2 but RS2 won't contribute to RS1 and it's ok? Or if you want to figure out if a layer is drawn, it would be something like: effect_node->screen_space_opacity != 0 || effect_node->has_copy_request || (effect_node->parent()->is_drawn() > > > > And to solve edge case where a layer has both copy request and > > hide_layer_and_subtree, we could add two effect nodes, parent one has opacity 0, > > and child one has opacity 1 with copy request, and layer will point to child > > effect node. > > > > This should reduce some complexity. WDYT? > I am not sure if adding 2 effect nodes to a single layer will reduce complexity ? > I had a discussion with Ali about it and his opinion was that ideally we wouldn't > want to end up with 2 effect nodes with the same owner_id. > > > > https://codereview.chromium.org/1588093004/diff/20001/ui/compositor/layer_uni... > > File ui/compositor/layer_unittest.cc (right): > > > > https://codereview.chromium.org/1588093004/diff/20001/ui/compositor/layer_uni... > > ui/compositor/layer_unittest.cc:842: TEST_F(LayerWithNullDelegateTest, Opacity) > > { > > Preferably with more descriptive test name.
On 2016/01/15 20:11:10, weiliangc wrote: > On 2016/01/15 at 19:49:40, jaydasika wrote: > > On 2016/01/15 19:30:24, weiliangc wrote: > > > Some high level comments. > > > > > > In my mind how this works is that effect node would have has_copy_request, > and > > > that is set with layer->HasCopyRequest(). > > > > > > And to determine whether a layer is drawn to screen, you can look at its > > > screen_space_opacity. > > > Whether a layer contributes to its target is determined by whether effect > node's > > > opacity is 0. > > If we have RS1->RS2, RS1->opacity = 0 and RS2->opacity = 1. If we use just the > opacity to determine > > if a layer contributes to its target, we would be wrong in this case as RS2's > opacity is 1 but still > > its not drawn and doesn't contribute to target. > > Well if RS1->RS2->L then L will contributes to RS2 but RS2 won't contribute to > RS1 and it's ok? > Or if you want to figure out if a layer is drawn, it would be something like: > effect_node->screen_space_opacity != 0 || effect_node->has_copy_request || > (effect_node->parent()->is_drawn() Taking a step back, suppose each effect tree node has a has_copy_request and knows its local opacity. It seems like there's enough information in the tree then to determine what needs to be drawn and what doesn't. If using screen_space_opacity makes things tricky (as it seems to do), could we not just add an is_drawn bool to the effect tree, and update this during EffectTree::UpdateEffects? I think this would be something like: if (effect_node->has_copy_request) effect_node->is_drawn = true; else if (effect_node->opacity == 0) effect_node->is_drawn = false; else effect_node->is_drawn = effect_node->parent->is_drawn
On 2016/01/15 at 20:28:30, ajuma wrote: > On 2016/01/15 20:11:10, weiliangc wrote: > > On 2016/01/15 at 19:49:40, jaydasika wrote: > > > On 2016/01/15 19:30:24, weiliangc wrote: > > > > Some high level comments. > > > > > > > > In my mind how this works is that effect node would have has_copy_request, > > and > > > > that is set with layer->HasCopyRequest(). > > > > > > > > And to determine whether a layer is drawn to screen, you can look at its > > > > screen_space_opacity. > > > > Whether a layer contributes to its target is determined by whether effect > > node's > > > > opacity is 0. > > > If we have RS1->RS2, RS1->opacity = 0 and RS2->opacity = 1. If we use just the > > opacity to determine > > > if a layer contributes to its target, we would be wrong in this case as RS2's > > opacity is 1 but still > > > its not drawn and doesn't contribute to target. > > > > Well if RS1->RS2->L then L will contributes to RS2 but RS2 won't contribute to > > RS1 and it's ok? > > Or if you want to figure out if a layer is drawn, it would be something like: > > effect_node->screen_space_opacity != 0 || effect_node->has_copy_request || > > (effect_node->parent()->is_drawn() > > Taking a step back, suppose each effect tree node has a has_copy_request and knows its local opacity. It seems like there's enough information in the tree then to determine what needs to be drawn and what doesn't. If using screen_space_opacity makes things tricky (as it seems to do), could we not just add an is_drawn bool to the effect tree, and update this during EffectTree::UpdateEffects? I think this would be something like: > if (effect_node->has_copy_request) > effect_node->is_drawn = true; > else if (effect_node->opacity == 0) > effect_node->is_drawn = false; > else > effect_node->is_drawn = effect_node->parent->is_drawn Yeah that sounds reasonable. Mostly trying to describe the reasoning assuming we only have those info, caching a bool to avoid computation for every layer would be nice. Also we'd be able to clear has_copy_request after drawing for the first frame.
On 2016/01/15 at 20:32:52, weiliangc wrote: > On 2016/01/15 at 20:28:30, ajuma wrote: > > On 2016/01/15 20:11:10, weiliangc wrote: > > > On 2016/01/15 at 19:49:40, jaydasika wrote: > > > > On 2016/01/15 19:30:24, weiliangc wrote: > > > > > Some high level comments. > > > > > > > > > > In my mind how this works is that effect node would have has_copy_request, > > > and > > > > > that is set with layer->HasCopyRequest(). > > > > > > > > > > And to determine whether a layer is drawn to screen, you can look at its > > > > > screen_space_opacity. > > > > > Whether a layer contributes to its target is determined by whether effect > > > node's > > > > > opacity is 0. > > > > If we have RS1->RS2, RS1->opacity = 0 and RS2->opacity = 1. If we use just the > > > opacity to determine > > > > if a layer contributes to its target, we would be wrong in this case as RS2's > > > opacity is 1 but still > > > > its not drawn and doesn't contribute to target. > > > > > > Well if RS1->RS2->L then L will contributes to RS2 but RS2 won't contribute to > > > RS1 and it's ok? > > > Or if you want to figure out if a layer is drawn, it would be something like: > > > effect_node->screen_space_opacity != 0 || effect_node->has_copy_request || > > > (effect_node->parent()->is_drawn() > > > > Taking a step back, suppose each effect tree node has a has_copy_request and knows its local opacity. It seems like there's enough information in the tree then to determine what needs to be drawn and what doesn't. If using screen_space_opacity makes things tricky (as it seems to do), could we not just add an is_drawn bool to the effect tree, and update this during EffectTree::UpdateEffects? I think this would be something like: > > if (effect_node->has_copy_request) > > effect_node->is_drawn = true; > > else if (effect_node->opacity == 0) > > effect_node->is_drawn = false; > > else > > effect_node->is_drawn = effect_node->parent->is_drawn > > Yeah that sounds reasonable. Mostly trying to describe the reasoning assuming we only have those info, caching a bool to avoid computation for every layer would be nice. > > Also we'd be able to clear has_copy_request after drawing for the first frame. And if we calculate and cache is_drawn, then no need to do the "two effect node for opacity 0 with copy request" business :)
On 2016/01/15 20:34:45, weiliangc wrote: > On 2016/01/15 at 20:32:52, weiliangc wrote: > > On 2016/01/15 at 20:28:30, ajuma wrote: > > > On 2016/01/15 20:11:10, weiliangc wrote: > > > > On 2016/01/15 at 19:49:40, jaydasika wrote: > > > > > On 2016/01/15 19:30:24, weiliangc wrote: > > > > > > Some high level comments. > > > > > > > > > > > > In my mind how this works is that effect node would have > has_copy_request, > > > > and > > > > > > that is set with layer->HasCopyRequest(). > > > > > > > > > > > > And to determine whether a layer is drawn to screen, you can look at > its > > > > > > screen_space_opacity. > > > > > > Whether a layer contributes to its target is determined by whether > effect > > > > node's > > > > > > opacity is 0. > > > > > If we have RS1->RS2, RS1->opacity = 0 and RS2->opacity = 1. If we use > just the > > > > opacity to determine > > > > > if a layer contributes to its target, we would be wrong in this case as > RS2's > > > > opacity is 1 but still > > > > > its not drawn and doesn't contribute to target. > > > > > > > > Well if RS1->RS2->L then L will contributes to RS2 but RS2 won't > contribute to > > > > RS1 and it's ok? > > > > Or if you want to figure out if a layer is drawn, it would be something > like: > > > > effect_node->screen_space_opacity != 0 || effect_node->has_copy_request || > > > > (effect_node->parent()->is_drawn() > > > > > > Taking a step back, suppose each effect tree node has a has_copy_request and > knows its local opacity. It seems like there's enough information in the tree > then to determine what needs to be drawn and what doesn't. If using > screen_space_opacity makes things tricky (as it seems to do), could we not just > add an is_drawn bool to the effect tree, and update this during > EffectTree::UpdateEffects? I think this would be something like: > > > if (effect_node->has_copy_request) > > > effect_node->is_drawn = true; > > > else if (effect_node->opacity == 0) > > > effect_node->is_drawn = false; > > > else > > > effect_node->is_drawn = effect_node->parent->is_drawn > > > > Yeah that sounds reasonable. Mostly trying to describe the reasoning assuming > we only have those info, caching a bool to avoid computation for every layer > would be nice. > > > > Also we'd be able to clear has_copy_request after drawing for the first frame. > > And if we calculate and cache is_drawn, then no need to do the "two effect node > for opacity 0 with copy request" business :) One more case that Ali mentioned in one of the comments needs to be handled. We need to know if a node is in subtree of node with animating opacity. We will end up with three new bools in effect node : 'is_drawn', 'screen_space_opacity_is_animating' and 'has_copy_request'. Alternatively, We could do it with just two bools: 'force_draw', 'screen_space_opacity_is_animating' While building effect tree we could : if (has_copy_request || has_animated_opacity || parent->screen_space_opacity_is_animating) force_draw = true; screen_space_opacity_is_animating = has_animated_opacity || parent->screen_space_opacity_is_animating else force_draw = parent->force_draw && opacity != 0 In EffectTree::ClearCopyRequests node->force_draw = node->screen_space_opacity_is_animating layer_is_drawn = layer->has_background_filter || effect_node->force_draw || effect_node->screen_space_opacity != 0.f contributes_to_drawn_surface will remain the same: layer_is_drawn && !(layer->HasCopyRequests() && effect_node->screen_space_opacity == 0) How does this look ?
On 2016/01/15 23:05:13, jaydasika wrote:
> On 2016/01/15 20:34:45, weiliangc wrote:
> > On 2016/01/15 at 20:32:52, weiliangc wrote:
> > > On 2016/01/15 at 20:28:30, ajuma wrote:
> > > > On 2016/01/15 20:11:10, weiliangc wrote:
> > > > > On 2016/01/15 at 19:49:40, jaydasika wrote:
> > > > > > On 2016/01/15 19:30:24, weiliangc wrote:
> > > > > > > Some high level comments.
> > > > > > >
> > > > > > > In my mind how this works is that effect node would have
> > has_copy_request,
> > > > > and
> > > > > > > that is set with layer->HasCopyRequest().
> > > > > > >
> > > > > > > And to determine whether a layer is drawn to screen, you can look
at
> > its
> > > > > > > screen_space_opacity.
> > > > > > > Whether a layer contributes to its target is determined by whether
> > effect
> > > > > node's
> > > > > > > opacity is 0.
> > > > > > If we have RS1->RS2, RS1->opacity = 0 and RS2->opacity = 1. If we
use
> > just the
> > > > > opacity to determine
> > > > > > if a layer contributes to its target, we would be wrong in this case
> as
> > RS2's
> > > > > opacity is 1 but still
> > > > > > its not drawn and doesn't contribute to target.
> > > > >
> > > > > Well if RS1->RS2->L then L will contributes to RS2 but RS2 won't
> > contribute to
> > > > > RS1 and it's ok?
> > > > > Or if you want to figure out if a layer is drawn, it would be
something
> > like:
> > > > > effect_node->screen_space_opacity != 0 ||
effect_node->has_copy_request
> ||
> > > > > (effect_node->parent()->is_drawn()
> > > >
> > > > Taking a step back, suppose each effect tree node has a has_copy_request
> and
> > knows its local opacity. It seems like there's enough information in the
tree
> > then to determine what needs to be drawn and what doesn't. If using
> > screen_space_opacity makes things tricky (as it seems to do), could we not
> just
> > add an is_drawn bool to the effect tree, and update this during
> > EffectTree::UpdateEffects? I think this would be something like:
> > > > if (effect_node->has_copy_request)
> > > > effect_node->is_drawn = true;
> > > > else if (effect_node->opacity == 0)
> > > > effect_node->is_drawn = false;
> > > > else
> > > > effect_node->is_drawn = effect_node->parent->is_drawn
> > >
> > > Yeah that sounds reasonable. Mostly trying to describe the reasoning
> assuming
> > we only have those info, caching a bool to avoid computation for every layer
> > would be nice.
> > >
> > > Also we'd be able to clear has_copy_request after drawing for the first
> frame.
> >
> > And if we calculate and cache is_drawn, then no need to do the "two effect
> node
> > for opacity 0 with copy request" business :)
>
> One more case that Ali mentioned in one of the comments needs to be handled.
> We need to know if a node is in subtree of node with animating opacity.
> We will end up with three new bools in effect node :
> 'is_drawn', 'screen_space_opacity_is_animating' and 'has_copy_request'.
>
> Alternatively,
> We could do it with just two bools:
> 'force_draw', 'screen_space_opacity_is_animating'
>
> While building effect tree we could :
> if (has_copy_request || has_animated_opacity ||
> parent->screen_space_opacity_is_animating)
> force_draw = true;
> screen_space_opacity_is_animating = has_animated_opacity ||
> parent->screen_space_opacity_is_animating
> else
> force_draw = parent->force_draw && opacity != 0
>
>
> In EffectTree::ClearCopyRequests
> node->force_draw = node->screen_space_opacity_is_animating
>
> layer_is_drawn = layer->has_background_filter || effect_node->force_draw ||
> effect_node->screen_space_opacity != 0.f
>
> contributes_to_drawn_surface will remain the same:
> layer_is_drawn && !(layer->HasCopyRequests() &&
> effect_node->screen_space_opacity == 0)
>
> How does this look ?
Resending the message as it is weirdly formatted in the previous one.
One more case that Ali mentioned in one of the comments needs to be handled.
We need to know if a node is in subtree of node with animating opacity.
We will end up with three new bools in effect node :
'is_drawn', 'screen_space_opacity_is_animating' and 'has_copy_request'.
Alternatively, we could do it with just two bools:
'force_draw', 'screen_space_opacity_is_animating'
While building effect tree we could :
if (has_copy_request || has_animated_opacity ||
parent->screen_space_opacity_is_animating)
force_draw = true;
screen_space_opacity_is_animating =
has_animated_opacity ||
parent->screen_space_opacity_is_animating
else
force_draw = parent->force_draw && opacity != 0
In EffectTree::ClearCopyRequests
node->force_draw = node->screen_space_opacity_is_animating
layer_is_drawn = layer->has_background_filter || effect_node->force_draw ||
effect_node->screen_space_opacity != 0.f
contributes_to_drawn_surface will remain the same:
layer_is_drawn && !(layer->HasCopyRequests() &&
effect_node->screen_space_opacity == 0)
How does this look ?
On 2016/01/15 23:08:55, jaydasika wrote: > On 2016/01/15 23:05:13, jaydasika wrote: > > On 2016/01/15 20:34:45, weiliangc wrote: > > > On 2016/01/15 at 20:32:52, weiliangc wrote: > > > > On 2016/01/15 at 20:28:30, ajuma wrote: > > > > > On 2016/01/15 20:11:10, weiliangc wrote: > > > > > > On 2016/01/15 at 19:49:40, jaydasika wrote: > > > > > > > On 2016/01/15 19:30:24, weiliangc wrote: > > > > > > > > Some high level comments. > > > > > > > > > > > > > > > > In my mind how this works is that effect node would have > > > has_copy_request, > > > > > > and > > > > > > > > that is set with layer->HasCopyRequest(). > > > > > > > > > > > > > > > > And to determine whether a layer is drawn to screen, you can look > at > > > its > > > > > > > > screen_space_opacity. > > > > > > > > Whether a layer contributes to its target is determined by whether > > > effect > > > > > > node's > > > > > > > > opacity is 0. > > > > > > > If we have RS1->RS2, RS1->opacity = 0 and RS2->opacity = 1. If we > use > > > just the > > > > > > opacity to determine > > > > > > > if a layer contributes to its target, we would be wrong in this case > > as > > > RS2's > > > > > > opacity is 1 but still > > > > > > > its not drawn and doesn't contribute to target. > > > > > > > > > > > > Well if RS1->RS2->L then L will contributes to RS2 but RS2 won't > > > contribute to > > > > > > RS1 and it's ok? > > > > > > Or if you want to figure out if a layer is drawn, it would be > something > > > like: > > > > > > effect_node->screen_space_opacity != 0 || > effect_node->has_copy_request > > || > > > > > > (effect_node->parent()->is_drawn() > > > > > > > > > > Taking a step back, suppose each effect tree node has a has_copy_request > > and > > > knows its local opacity. It seems like there's enough information in the > tree > > > then to determine what needs to be drawn and what doesn't. If using > > > screen_space_opacity makes things tricky (as it seems to do), could we not > > just > > > add an is_drawn bool to the effect tree, and update this during > > > EffectTree::UpdateEffects? I think this would be something like: > > > > > if (effect_node->has_copy_request) > > > > > effect_node->is_drawn = true; > > > > > else if (effect_node->opacity == 0) > > > > > effect_node->is_drawn = false; > > > > > else > > > > > effect_node->is_drawn = effect_node->parent->is_drawn > > > > > > > > Yeah that sounds reasonable. Mostly trying to describe the reasoning > > assuming > > > we only have those info, caching a bool to avoid computation for every layer > > > would be nice. > > > > > > > > Also we'd be able to clear has_copy_request after drawing for the first > > frame. > > > > > > And if we calculate and cache is_drawn, then no need to do the "two effect > > node > > > for opacity 0 with copy request" business :) > > > > One more case that Ali mentioned in one of the comments needs to be handled. > > We need to know if a node is in subtree of node with animating opacity. > > We will end up with three new bools in effect node : > > 'is_drawn', 'screen_space_opacity_is_animating' and 'has_copy_request'. > > > > Alternatively, > > We could do it with just two bools: > > 'force_draw', 'screen_space_opacity_is_animating' > > > > While building effect tree we could : > > if (has_copy_request || has_animated_opacity || > > parent->screen_space_opacity_is_animating) > > force_draw = true; > > screen_space_opacity_is_animating = has_animated_opacity || > > parent->screen_space_opacity_is_animating > > else > > force_draw = parent->force_draw && opacity != 0 > > > > > > In EffectTree::ClearCopyRequests > > node->force_draw = node->screen_space_opacity_is_animating > > > > layer_is_drawn = layer->has_background_filter || effect_node->force_draw || > > effect_node->screen_space_opacity != 0.f > > > > contributes_to_drawn_surface will remain the same: > > layer_is_drawn && !(layer->HasCopyRequests() && > > effect_node->screen_space_opacity == 0) > > > > How does this look ? > > Resending the message as it is weirdly formatted in the previous one. > > One more case that Ali mentioned in one of the comments needs to be handled. > We need to know if a node is in subtree of node with animating opacity. > We will end up with three new bools in effect node : > 'is_drawn', 'screen_space_opacity_is_animating' and 'has_copy_request'. Having three bools seems fine if it makes the logic clearer. > Alternatively, we could do it with just two bools: > 'force_draw', 'screen_space_opacity_is_animating' > > While building effect tree we could : > if (has_copy_request || has_animated_opacity || > parent->screen_space_opacity_is_animating) > force_draw = true; > screen_space_opacity_is_animating = > has_animated_opacity || > parent->screen_space_opacity_is_animating > else > force_draw = parent->force_draw && opacity != 0 > > > In EffectTree::ClearCopyRequests > node->force_draw = node->screen_space_opacity_is_animating > > layer_is_drawn = layer->has_background_filter || effect_node->force_draw || > effect_node->screen_space_opacity != 0.f Any layer that has a background filter has an effect node, so we should be able to take background filters into account when computing on the effect node itself. > contributes_to_drawn_surface will remain the same: > layer_is_drawn && !(layer->HasCopyRequests() && > effect_node->screen_space_opacity == 0) Could we compute this in a clearer way while updating the effect tree? All the information used in this computation seems to be things each effect node should already know. This is really saying "the layer is drawn, and it's not just because it has a copy request", which seem like things that can be computed knowing the node's (local) opacity, whether the node has a copy request, and the is_drawn value inherited from its parent.
On 2016/01/15 23:59:03, ajuma wrote: > On 2016/01/15 23:08:55, jaydasika wrote: > > On 2016/01/15 23:05:13, jaydasika wrote: > > > On 2016/01/15 20:34:45, weiliangc wrote: > > > > On 2016/01/15 at 20:32:52, weiliangc wrote: > > > > > On 2016/01/15 at 20:28:30, ajuma wrote: > > > > > > On 2016/01/15 20:11:10, weiliangc wrote: > > > > > > > On 2016/01/15 at 19:49:40, jaydasika wrote: > > > > > > > > On 2016/01/15 19:30:24, weiliangc wrote: > > > > > > > > > Some high level comments. > > > > > > > > > > > > > > > > > > In my mind how this works is that effect node would have > > > > has_copy_request, > > > > > > > and > > > > > > > > > that is set with layer->HasCopyRequest(). > > > > > > > > > > > > > > > > > > And to determine whether a layer is drawn to screen, you can > look > > at > > > > its > > > > > > > > > screen_space_opacity. > > > > > > > > > Whether a layer contributes to its target is determined by > whether > > > > effect > > > > > > > node's > > > > > > > > > opacity is 0. > > > > > > > > If we have RS1->RS2, RS1->opacity = 0 and RS2->opacity = 1. If we > > use > > > > just the > > > > > > > opacity to determine > > > > > > > > if a layer contributes to its target, we would be wrong in this > case > > > as > > > > RS2's > > > > > > > opacity is 1 but still > > > > > > > > its not drawn and doesn't contribute to target. > > > > > > > > > > > > > > Well if RS1->RS2->L then L will contributes to RS2 but RS2 won't > > > > contribute to > > > > > > > RS1 and it's ok? > > > > > > > Or if you want to figure out if a layer is drawn, it would be > > something > > > > like: > > > > > > > effect_node->screen_space_opacity != 0 || > > effect_node->has_copy_request > > > || > > > > > > > (effect_node->parent()->is_drawn() > > > > > > > > > > > > Taking a step back, suppose each effect tree node has a > has_copy_request > > > and > > > > knows its local opacity. It seems like there's enough information in the > > tree > > > > then to determine what needs to be drawn and what doesn't. If using > > > > screen_space_opacity makes things tricky (as it seems to do), could we not > > > just > > > > add an is_drawn bool to the effect tree, and update this during > > > > EffectTree::UpdateEffects? I think this would be something like: > > > > > > if (effect_node->has_copy_request) > > > > > > effect_node->is_drawn = true; > > > > > > else if (effect_node->opacity == 0) > > > > > > effect_node->is_drawn = false; > > > > > > else > > > > > > effect_node->is_drawn = effect_node->parent->is_drawn > > > > > > > > > > Yeah that sounds reasonable. Mostly trying to describe the reasoning > > > assuming > > > > we only have those info, caching a bool to avoid computation for every > layer > > > > would be nice. > > > > > > > > > > Also we'd be able to clear has_copy_request after drawing for the first > > > frame. > > > > > > > > And if we calculate and cache is_drawn, then no need to do the "two effect > > > node > > > > for opacity 0 with copy request" business :) > > > > > > One more case that Ali mentioned in one of the comments needs to be handled. > > > We need to know if a node is in subtree of node with animating opacity. > > > We will end up with three new bools in effect node : > > > 'is_drawn', 'screen_space_opacity_is_animating' and 'has_copy_request'. > > > > > > Alternatively, > > > We could do it with just two bools: > > > 'force_draw', 'screen_space_opacity_is_animating' > > > > > > While building effect tree we could : > > > if (has_copy_request || has_animated_opacity || > > > parent->screen_space_opacity_is_animating) > > > force_draw = true; > > > screen_space_opacity_is_animating = has_animated_opacity || > > > parent->screen_space_opacity_is_animating > > > else > > > force_draw = parent->force_draw && opacity != 0 > > > > > > > > > In EffectTree::ClearCopyRequests > > > node->force_draw = node->screen_space_opacity_is_animating > > > > > > layer_is_drawn = layer->has_background_filter || effect_node->force_draw || > > > effect_node->screen_space_opacity != 0.f > > > > > > contributes_to_drawn_surface will remain the same: > > > layer_is_drawn && !(layer->HasCopyRequests() && > > > effect_node->screen_space_opacity == 0) > > > > > > How does this look ? > > > > Resending the message as it is weirdly formatted in the previous one. > > > > One more case that Ali mentioned in one of the comments needs to be handled. > > We need to know if a node is in subtree of node with animating opacity. > > We will end up with three new bools in effect node : > > 'is_drawn', 'screen_space_opacity_is_animating' and 'has_copy_request'. > > Having three bools seems fine if it makes the logic clearer. > > > > Alternatively, we could do it with just two bools: > > 'force_draw', 'screen_space_opacity_is_animating' > > > > While building effect tree we could : > > if (has_copy_request || has_animated_opacity || > > parent->screen_space_opacity_is_animating) > > force_draw = true; > > screen_space_opacity_is_animating = > > has_animated_opacity || > > parent->screen_space_opacity_is_animating > > else > > force_draw = parent->force_draw && opacity != 0 > > > > > > In EffectTree::ClearCopyRequests > > node->force_draw = node->screen_space_opacity_is_animating > > > > layer_is_drawn = layer->has_background_filter || effect_node->force_draw || > > effect_node->screen_space_opacity != 0.f > > Any layer that has a background filter has an effect node, so we should be able > to take background filters into account when computing on the effect node > itself. > > > contributes_to_drawn_surface will remain the same: > > layer_is_drawn && !(layer->HasCopyRequests() && > > effect_node->screen_space_opacity == 0) > > Could we compute this in a clearer way while updating the effect tree? All the > information used in this computation seems to be things each effect node should > already know. This is really saying "the layer is drawn, and it's not just > because it has a copy request", which seem like things that can be computed > knowing the node's (local) opacity, whether the node has a copy request, and the > is_drawn value inherited from its parent. If we put both is_drawn and contributes_to_drawn_surface to effect node, we can update both while walking the effect tree. if opacity == 0 is_drawn = false; contributes_to_drawn_surface = false; if has_copy_request is_drawn = true; children.is_drawn = is_drawn; children.contributes_to_drawn_surface = children.contributes_to_drawn_surface; if has_copy_request children.contributes_to_drawn_surface = true;
Moved the computation to effect tree. https://codereview.chromium.org/1588093004/diff/20001/cc/layers/layer.cc File cc/layers/layer.cc (right): https://codereview.chromium.org/1588093004/diff/20001/cc/layers/layer.cc#newc... cc/layers/layer.cc:1920: // Layers that have screen space opacity are hidden. So they are not drawn. On 2016/01/15 15:02:46, ajuma wrote: > "screen space opacity 0"? Done. https://codereview.chromium.org/1588093004/diff/20001/cc/layers/layer.cc#newc... cc/layers/layer.cc:1925: if (HasPotentiallyRunningOpacityAnimation() || On 2016/01/15 15:02:46, ajuma wrote: > This only checks if the layer itself has an animation. It doesn't handle the > case that some ancestor has an opacity animation. For example, say we have > A->B->C, where A has an opacity animation and currently has opacity 0. In that > case, C's screen space opacity will be 0 right now, and C itself won't have an > opacity animation, but C's screen space opacity can still become non-0 during > A's opacity animation. Done. https://codereview.chromium.org/1588093004/diff/20001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/1588093004/diff/20001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:1898: // Layers that have screen space opacity are hidden. So they are not drawn. On 2016/01/15 15:02:46, ajuma wrote: > "screen space opacity 0" Done. https://codereview.chromium.org/1588093004/diff/20001/cc/trees/property_tree_... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/1588093004/diff/20001/cc/trees/property_tree_... cc/trees/property_tree_builder.cc:584: node.data.contributes_to_copy_request = On 2016/01/15 15:02:47, ajuma wrote: > Does this value need to get updated on the compositor side after copy requests > are serviced? Done. https://codereview.chromium.org/1588093004/diff/20001/ui/compositor/layer_uni... File ui/compositor/layer_unittest.cc (right): https://codereview.chromium.org/1588093004/diff/20001/ui/compositor/layer_uni... ui/compositor/layer_unittest.cc:842: TEST_F(LayerWithNullDelegateTest, Opacity) { On 2016/01/15 19:30:24, weiliangc wrote: > Preferably with more descriptive test name. Done.
Thanks for making it simpler. https://codereview.chromium.org/1588093004/diff/80001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/1588093004/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common.cc:2363: bool layer_is_drawn; Initialize this please. https://codereview.chromium.org/1588093004/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common.cc:2389: bool contributes_to_drawn_surface; No need to compute here. Just compute at the location where it is needed. https://codereview.chromium.org/1588093004/diff/80001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1588093004/diff/80001/cc/trees/property_tree.... cc/trees/property_tree.cc:1059: // 2) Nodes that have a background filter. Sorry I probably missed the discussion somewhere. Could you explain again why are layers with background filters drawn? https://codereview.chromium.org/1588093004/diff/80001/cc/trees/property_tree.... cc/trees/property_tree.cc:1061: if (node->data.has_copy_request || If a node has copy request and has opacity 0, it should still be drawn? https://codereview.chromium.org/1588093004/diff/80001/cc/trees/property_tree.... cc/trees/property_tree.cc:1078: UpdateIsDrawn(node, parent_node); In addition to this, could you also set is_drawn correctly while building the effect tree? https://codereview.chromium.org/1588093004/diff/80001/cc/trees/property_tree_... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/1588093004/diff/80001/cc/trees/property_tree_... cc/trees/property_tree_builder.cc:551: const float opacity = layer->opacity(); Don't think we need this cached? https://codereview.chromium.org/1588093004/diff/80001/ui/compositor/layer_uni... File ui/compositor/layer_unittest.cc (right): https://codereview.chromium.org/1588093004/diff/80001/ui/compositor/layer_uni... ui/compositor/layer_unittest.cc:842: TEST_F(LayerWithNullDelegateTest, OpacityOfCCLayer) { Test names usually is description of what behavior is being tested. How about the name "CacheOpacityWhenToggleLayerVisibility"?
https://codereview.chromium.org/1588093004/diff/80001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/1588093004/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common.cc:2363: bool layer_is_drawn; On 2016/01/19 21:03:08, weiliangc wrote: > Initialize this please. Done. https://codereview.chromium.org/1588093004/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common.cc:2389: bool contributes_to_drawn_surface; On 2016/01/19 21:03:08, weiliangc wrote: > No need to compute here. Just compute at the location where it is needed. Done. https://codereview.chromium.org/1588093004/diff/80001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1588093004/diff/80001/cc/trees/property_tree.... cc/trees/property_tree.cc:1059: // 2) Nodes that have a background filter. On 2016/01/19 21:03:08, weiliangc wrote: > Sorry I probably missed the discussion somewhere. Could you explain again why > are layers with background filters drawn? We have unit tests that check that layers with opacity 0 and background filters draw. https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre... We were drawing them before this CL because layer_is_drawn was decided by hide_layer_and_subtree and not by opacity. Now that we have made hide_layer_and_subtree equivalent to opacity=0, we need to handle this separately. https://codereview.chromium.org/1588093004/diff/80001/cc/trees/property_tree.... cc/trees/property_tree.cc:1061: if (node->data.has_copy_request || On 2016/01/19 21:03:08, weiliangc wrote: > If a node has copy request and has opacity 0, it should still be drawn? I think yes, we need to draw the layer for the copy request. https://codereview.chromium.org/1588093004/diff/80001/cc/trees/property_tree.... cc/trees/property_tree.cc:1078: UpdateIsDrawn(node, parent_node); On 2016/01/19 21:03:08, weiliangc wrote: > In addition to this, could you also set is_drawn correctly while building the > effect tree? We call UpdateEffects after building property trees. So its not required. https://codereview.chromium.org/1588093004/diff/80001/cc/trees/property_tree_... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/1588093004/diff/80001/cc/trees/property_tree_... cc/trees/property_tree_builder.cc:551: const float opacity = layer->opacity(); On 2016/01/19 21:03:08, weiliangc wrote: > Don't think we need this cached? Removed https://codereview.chromium.org/1588093004/diff/80001/cc/trees/property_tree_... cc/trees/property_tree_builder.cc:764: property_trees->effect_tree.set_needs_update(true); This was the place I was referring to in a previous comment. Since we set needs_update = true here, we don't have to compute is_drawn while property tree building. https://codereview.chromium.org/1588093004/diff/80001/ui/compositor/layer_uni... File ui/compositor/layer_unittest.cc (right): https://codereview.chromium.org/1588093004/diff/80001/ui/compositor/layer_uni... ui/compositor/layer_unittest.cc:842: TEST_F(LayerWithNullDelegateTest, OpacityOfCCLayer) { On 2016/01/19 21:03:08, weiliangc wrote: > Test names usually is description of what behavior is being tested. How about > the name "CacheOpacityWhenToggleLayerVisibility"? Done.
https://codereview.chromium.org/1588093004/diff/80001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1588093004/diff/80001/cc/trees/property_tree.... cc/trees/property_tree.cc:1061: if (node->data.has_copy_request || On 2016/01/19 at 22:10:36, jaydasika wrote: > On 2016/01/19 21:03:08, weiliangc wrote: > > If a node has copy request and has opacity 0, it should still be drawn? > > I think yes, we need to draw the layer for the copy request. Ah sorry didn't realized it's a "else if". https://codereview.chromium.org/1588093004/diff/80001/cc/trees/property_tree.... cc/trees/property_tree.cc:1078: UpdateIsDrawn(node, parent_node); On 2016/01/19 at 22:10:36, jaydasika wrote: > On 2016/01/19 21:03:08, weiliangc wrote: > > In addition to this, could you also set is_drawn correctly while building the > > effect tree? > > We call UpdateEffects after building property trees. So its not required. That case we don't need to update screen space opacity during tree building either. Mostly I want screen_space_opacity and is_drawn to be updated consistently. Or could you add a comment for this?
https://codereview.chromium.org/1588093004/diff/80001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1588093004/diff/80001/cc/trees/property_tree.... cc/trees/property_tree.cc:1078: UpdateIsDrawn(node, parent_node); On 2016/01/19 22:23:29, weiliangc wrote: > On 2016/01/19 at 22:10:36, jaydasika wrote: > > On 2016/01/19 21:03:08, weiliangc wrote: > > > In addition to this, could you also set is_drawn correctly while building > the > > > effect tree? > > > > We call UpdateEffects after building property trees. So its not required. > > That case we don't need to update screen space opacity during tree building > either. Mostly I want screen_space_opacity and is_drawn to be updated > consistently. Or could you add a comment for this? Both(Removing screen space opacity and adding comment) are already done. I have added comments to where they are. https://codereview.chromium.org/1588093004/diff/80001/cc/trees/property_tree_... File cc/trees/property_tree_builder.cc (left): https://codereview.chromium.org/1588093004/diff/80001/cc/trees/property_tree_... cc/trees/property_tree_builder.cc:591: node.data.screen_space_opacity *= Removed screen space opacity computation from property tree building. https://codereview.chromium.org/1588093004/diff/80001/cc/trees/property_tree_... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/1588093004/diff/80001/cc/trees/property_tree_... cc/trees/property_tree_builder.cc:761: // is_drawn in the effect tree aren't computed during tree building. Comment that we need to update effects.
Just a couple nits, but otherwise lgtm % weiliangc https://codereview.chromium.org/1588093004/diff/120001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1588093004/diff/120001/cc/trees/property_tree... cc/trees/property_tree.cc:1100: contributes_to_drawn_surface && parent_node->data.is_drawn; Nit: I think this might be easier to read as: if (parent_node && !parent_node->data.is_drawn) contributes_to_drawn_surface = false; https://codereview.chromium.org/1588093004/diff/120001/cc/trees/property_tree... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/1588093004/diff/120001/cc/trees/property_tree... cc/trees/property_tree_builder.cc:571: const bool has_copy_request = layer->HasCopyRequest(); If we're only using this once, we probably don't need a variable.
On 2016/01/19 23:57:33, ajuma wrote: > Just a couple nits, but otherwise lgtm % weiliangc > > https://codereview.chromium.org/1588093004/diff/120001/cc/trees/property_tree.cc > File cc/trees/property_tree.cc (right): > > https://codereview.chromium.org/1588093004/diff/120001/cc/trees/property_tree... > cc/trees/property_tree.cc:1100: contributes_to_drawn_surface && > parent_node->data.is_drawn; > Nit: I think this might be easier to read as: > if (parent_node && !parent_node->data.is_drawn) > contributes_to_drawn_surface = false; > > https://codereview.chromium.org/1588093004/diff/120001/cc/trees/property_tree... > File cc/trees/property_tree_builder.cc (right): > > https://codereview.chromium.org/1588093004/diff/120001/cc/trees/property_tree... > cc/trees/property_tree_builder.cc:571: const bool has_copy_request = > layer->HasCopyRequest(); > If we're only using this once, we probably don't need a variable. ui is not the only user of SetHideLayerAndSubtree feature of cc Layer. It is called from many other places in chrome/browser/android/ and content/browser/renderer_host. So, this CL needs to be changed. Since we have multiple clients of SetHideLayerAndSubtree feature, I think we should have it in cc itself. Within cc, we can implement it by setting opacity to 0. How does that sound ?
On 2016/01/20 00:08:14, jaydasika wrote: > On 2016/01/19 23:57:33, ajuma wrote: > > Just a couple nits, but otherwise lgtm % weiliangc > > > > > https://codereview.chromium.org/1588093004/diff/120001/cc/trees/property_tree.cc > > File cc/trees/property_tree.cc (right): > > > > > https://codereview.chromium.org/1588093004/diff/120001/cc/trees/property_tree... > > cc/trees/property_tree.cc:1100: contributes_to_drawn_surface && > > parent_node->data.is_drawn; > > Nit: I think this might be easier to read as: > > if (parent_node && !parent_node->data.is_drawn) > > contributes_to_drawn_surface = false; > > > > > https://codereview.chromium.org/1588093004/diff/120001/cc/trees/property_tree... > > File cc/trees/property_tree_builder.cc (right): > > > > > https://codereview.chromium.org/1588093004/diff/120001/cc/trees/property_tree... > > cc/trees/property_tree_builder.cc:571: const bool has_copy_request = > > layer->HasCopyRequest(); > > If we're only using this once, we probably don't need a variable. > > ui is not the only user of SetHideLayerAndSubtree feature of cc Layer. > It is called from many other places in chrome/browser/android/ and > content/browser/renderer_host. > So, this CL needs to be changed. Since we have multiple clients of > SetHideLayerAndSubtree feature, I think we should have it in cc > itself. Within cc, we can implement it by setting opacity to 0. > How does that sound ? Where would the actual opacity value be cached? Also (something I just realized, which also affects the existing patch): what if a client of cc calls SetHideLayerAndSubtree on something that already has an opacity animation running in cc? Won't the opacity 0 get clobbered by the animation?
On 2016/01/20 at 00:33:39, ajuma wrote: > On 2016/01/20 00:08:14, jaydasika wrote: > > On 2016/01/19 23:57:33, ajuma wrote: > > > Just a couple nits, but otherwise lgtm % weiliangc > > > > > > > > https://codereview.chromium.org/1588093004/diff/120001/cc/trees/property_tree.cc > > > File cc/trees/property_tree.cc (right): > > > > > > > > https://codereview.chromium.org/1588093004/diff/120001/cc/trees/property_tree... > > > cc/trees/property_tree.cc:1100: contributes_to_drawn_surface && > > > parent_node->data.is_drawn; > > > Nit: I think this might be easier to read as: > > > if (parent_node && !parent_node->data.is_drawn) > > > contributes_to_drawn_surface = false; > > > > > > > > https://codereview.chromium.org/1588093004/diff/120001/cc/trees/property_tree... > > > File cc/trees/property_tree_builder.cc (right): > > > > > > > > https://codereview.chromium.org/1588093004/diff/120001/cc/trees/property_tree... > > > cc/trees/property_tree_builder.cc:571: const bool has_copy_request = > > > layer->HasCopyRequest(); > > > If we're only using this once, we probably don't need a variable. > > > > ui is not the only user of SetHideLayerAndSubtree feature of cc Layer. > > It is called from many other places in chrome/browser/android/ and > > content/browser/renderer_host. > > So, this CL needs to be changed. Since we have multiple clients of > > SetHideLayerAndSubtree feature, I think we should have it in cc > > itself. Within cc, we can implement it by setting opacity to 0. > > How does that sound ? > > Where would the actual opacity value be cached? Also (something I just realized, which also affects the existing patch): what if a client of cc calls SetHideLayerAndSubtree on something that already has an opacity animation running in cc? Won't the opacity 0 get clobbered by the animation? If we cache it on cc::Layer, we might run into problem with opacity animation. Changing all the call sites in android compositor is a bit of work, and will need to break into smaller CLs. Sorry should have grep instead of code search before this.
On 2016/01/20 01:46:15, weiliangc wrote:
> On 2016/01/20 at 00:33:39, ajuma wrote:
> > Where would the actual opacity value be cached? Also (something I just
> realized, which also affects the existing patch): what if a client of cc calls
> SetHideLayerAndSubtree on something that already has an opacity animation
> running in cc? Won't the opacity 0 get clobbered by the animation?
>
> If we cache it on cc::Layer, we might run into problem with opacity animation.
I think this is a problem even if the opacity is cached outside cc. Say a
ui::Layer with an opacity animation running in cc is marked as hidden during the
animation. Then the ui::Layer handles this by caching its current opacity value
and making it's cc::Layer have opacity 0. But the animation continues in cc, so
cc could change the opacity from 0 to something else, unhiding the layer.
> Changing all the call sites in android compositor is a bit of work, and will
> need to break into smaller CLs.
>
> Sorry should have grep instead of code search before this.
Yet another idea would be to have a SetHideLayerAndSubtree on cc::Layer that
sets a hidden_ bool on the layer, and then change the opacity getter to take
that bool into account:
float Layer::Opacity() {
if (hidden_)
return 0;
return opacity_;
}
And do the same for LayerImpl. The opacity value in the effect tree would be the
value that Opacity() returns, so I think the rest of this CL would continue to
work as-is.
This would also handle the animation problem, since opacity_ could continue to
change without affecting Opacity() when hidden_ is true.
Description was changed from ========== This CL : * moves hide subtree feature to ui compositor. * deletes is_hidden and hide_layer_and_subtree from cc::Layer * computes if a layer is drawn using the effect tree BUG=575413 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== This CL : * deletes is_hidden * computes if a layer is drawn using the effect tree BUG=575413 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
As Ali suggested, I added back hide_layer_and_subtree to cc::Layer and cc::LayerImpl and made the cc::Layer::opacity() return 0 when hide_layer_and_subtree = true. https://codereview.chromium.org/1588093004/diff/120001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1588093004/diff/120001/cc/trees/property_tree... cc/trees/property_tree.cc:1100: contributes_to_drawn_surface && parent_node->data.is_drawn; On 2016/01/19 23:57:33, ajuma wrote: > Nit: I think this might be easier to read as: > if (parent_node && !parent_node->data.is_drawn) > contributes_to_drawn_surface = false; Done. https://codereview.chromium.org/1588093004/diff/120001/cc/trees/property_tree... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/1588093004/diff/120001/cc/trees/property_tree... cc/trees/property_tree_builder.cc:571: const bool has_copy_request = layer->HasCopyRequest(); On 2016/01/19 23:57:33, ajuma wrote: > If we're only using this once, we probably don't need a variable. Done.
https://codereview.chromium.org/1588093004/diff/150001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/1588093004/diff/150001/cc/layers/layer_impl.c... cc/layers/layer_impl.cc:986: UpdatePropertyTreeOpacity(); Question : Do we need to update property tree opacity here when hide_layer_and_subtree = true ? I think we don't need to as hide_layer_and_subtree is not updated on compositor and so opacity() is going to remain 0 till the next commit.
Approach looks good, but there are a few test failures that need to be sorted out. https://codereview.chromium.org/1588093004/diff/150001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/1588093004/diff/150001/cc/layers/layer_impl.c... cc/layers/layer_impl.cc:986: UpdatePropertyTreeOpacity(); On 2016/01/21 00:53:09, jaydasika wrote: > Question : Do we need to update property tree opacity here when > hide_layer_and_subtree = true ? > > I think we don't need to as hide_layer_and_subtree is not updated on compositor > and so opacity() is going to remain 0 till the next commit. Yes, you're right that Opacity() won't change in this situation, so we can skip the property tree update in that case.
ui::layer may still need to cache opacity? When layer is hidden cc::layer would return opacity 0, but cache opacity might be different?
On 2016/01/21 16:21:46, weiliangc wrote: > ui::layer may still need to cache opacity? When layer is hidden cc::layer would > return opacity 0, but cache opacity might be different? Ah, good point. Then maybe cc::Layer::opacity should still return opacity_ (so external clients see what they were seeing before), but internally we should have a cc::Layer::EffectiveOpacity that takes hide_layer_and_subtree_ into account, and we should use EffectiveOpacity() wherever we currently use opacity().
1) Added Layer(Impl)::EffectiveOpacity for effect tree to use 2) Modified the computation of contributes_to_drawn_surface to address layout test crashes (pages with background filters).
https://codereview.chromium.org/1588093004/diff/150001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1588093004/diff/150001/cc/trees/property_tree... cc/trees/property_tree.cc:1097: node->data.is_drawn && node->data.opacity != 0; What if you just change the above line to: (node->data.is_drawn && node->data.opacity != 0) || node->data.has_background_filters What are the cases that aren't covered by the logic here? https://codereview.chromium.org/1588093004/diff/210001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1588093004/diff/210001/cc/trees/property_tree... cc/trees/property_tree.cc:1095: node->data.has_background_filters) Is this ok even if the parent effect node isn't drawn? e.g. suppose the parent effect node isn't drawn, and suppose the current node has either an opacity animation or background filters. Is it ok to treat this layer as contributing to a drawn surface in that situation? (This seems like a behavior change from what we do now.) Also, if something has opacity 0 and an opacity animation, why does it need to contribute to a drawn surface?
https://codereview.chromium.org/1588093004/diff/210001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1588093004/diff/210001/cc/trees/property_tree... cc/trees/property_tree.cc:1095: node->data.has_background_filters) On 2016/01/21 23:06:20, ajuma wrote: > Is this ok even if the parent effect node isn't drawn? e.g. suppose the parent > effect node isn't drawn, and suppose the current node has either an opacity > animation or background filters. Is it ok to treat this layer as contributing to > a drawn surface in that situation? (This seems like a behavior change from what > we do now.) > Also, if something has opacity 0 and an opacity animation, why does it need to > contribute to a drawn surface? My understanding is contributes_to_drawn_surface = is_drawn && !is_drawn_only_for_copy_request. So, when there are no copy requests, shouldn't they be equal ? (is_drawn is currently true for both the cases you mentioned)
https://codereview.chromium.org/1588093004/diff/210001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1588093004/diff/210001/cc/trees/property_tree... cc/trees/property_tree.cc:1095: node->data.has_background_filters) On 2016/01/21 23:37:09, jaydasika wrote: > On 2016/01/21 23:06:20, ajuma wrote: > > Is this ok even if the parent effect node isn't drawn? e.g. suppose the parent > > effect node isn't drawn, and suppose the current node has either an opacity > > animation or background filters. Is it ok to treat this layer as contributing > to > > a drawn surface in that situation? (This seems like a behavior change from > what > > we do now.) > > > Also, if something has opacity 0 and an opacity animation, why does it need to > > contribute to a drawn surface? > > My understanding is > contributes_to_drawn_surface = is_drawn && !is_drawn_only_for_copy_request. > So, when there are no copy requests, shouldn't they be equal ? > (is_drawn is currently true for both the cases you mentioned) In the current code, if subtree_visible_from_ancestor is false, layer_is_drawn is false regardless of whether the layer has background filters or opacity animations (unless the layer has a copy request). In the new logic, even if the equivalent to subtree_visible_from_ancestor is false (that is, even if the parent node isn't drawn), contributes_to_drawn_surface becomes true if either we have background filters or an opacity animation, even if we don't have a copy request.
On 2016/01/22 00:24:42, ajuma wrote: > https://codereview.chromium.org/1588093004/diff/210001/cc/trees/property_tree.cc > File cc/trees/property_tree.cc (right): > > https://codereview.chromium.org/1588093004/diff/210001/cc/trees/property_tree... > cc/trees/property_tree.cc:1095: node->data.has_background_filters) > On 2016/01/21 23:37:09, jaydasika wrote: > > On 2016/01/21 23:06:20, ajuma wrote: > > > Is this ok even if the parent effect node isn't drawn? e.g. suppose the > parent > > > effect node isn't drawn, and suppose the current node has either an opacity > > > animation or background filters. Is it ok to treat this layer as > contributing > > to > > > a drawn surface in that situation? (This seems like a behavior change from > > what > > > we do now.) > > > > > Also, if something has opacity 0 and an opacity animation, why does it need > to > > > contribute to a drawn surface? > > > > My understanding is > > contributes_to_drawn_surface = is_drawn && !is_drawn_only_for_copy_request. > > So, when there are no copy requests, shouldn't they be equal ? > > (is_drawn is currently true for both the cases you mentioned) > > In the current code, if subtree_visible_from_ancestor is false, layer_is_drawn > is false regardless of whether the layer has background filters or opacity > animations (unless the layer has a copy request). In the new logic, even if the > equivalent to subtree_visible_from_ancestor is false (that is, even if the > parent node isn't drawn), contributes_to_drawn_surface becomes true if either we > have background filters or an opacity animation, even if we don't have a copy > request. What I am probably trying to get at is that we should then change both is_drawn and contributes_to_drawn_surface and not just the latter.
On 2016/01/22 00:32:57, jaydasika wrote: > On 2016/01/22 00:24:42, ajuma wrote: > > > https://codereview.chromium.org/1588093004/diff/210001/cc/trees/property_tree.cc > > File cc/trees/property_tree.cc (right): > > > > > https://codereview.chromium.org/1588093004/diff/210001/cc/trees/property_tree... > > cc/trees/property_tree.cc:1095: node->data.has_background_filters) > > On 2016/01/21 23:37:09, jaydasika wrote: > > > On 2016/01/21 23:06:20, ajuma wrote: > > > > Is this ok even if the parent effect node isn't drawn? e.g. suppose the > > parent > > > > effect node isn't drawn, and suppose the current node has either an > opacity > > > > animation or background filters. Is it ok to treat this layer as > > contributing > > > to > > > > a drawn surface in that situation? (This seems like a behavior change from > > > what > > > > we do now.) > > > > > > > Also, if something has opacity 0 and an opacity animation, why does it > need > > to > > > > contribute to a drawn surface? > > > > > > My understanding is > > > contributes_to_drawn_surface = is_drawn && !is_drawn_only_for_copy_request. > > > So, when there are no copy requests, shouldn't they be equal ? > > > (is_drawn is currently true for both the cases you mentioned) > > > > In the current code, if subtree_visible_from_ancestor is false, layer_is_drawn > > is false regardless of whether the layer has background filters or opacity > > animations (unless the layer has a copy request). In the new logic, even if > the > > equivalent to subtree_visible_from_ancestor is false (that is, even if the > > parent node isn't drawn), contributes_to_drawn_surface becomes true if either > we > > have background filters or an opacity animation, even if we don't have a copy > > request. > > What I am probably trying to get at is that we should then change both is_drawn > and contributes_to_drawn_surface and not just the latter. It'd be really helpful to understand what goes wrong with the contributes_to_drawn_surface logic from patch set 9.
On 2016/01/22 00:41:15, ajuma wrote: > On 2016/01/22 00:32:57, jaydasika wrote: > > On 2016/01/22 00:24:42, ajuma wrote: > > > > > > https://codereview.chromium.org/1588093004/diff/210001/cc/trees/property_tree.cc > > > File cc/trees/property_tree.cc (right): > > > > > > > > > https://codereview.chromium.org/1588093004/diff/210001/cc/trees/property_tree... > > > cc/trees/property_tree.cc:1095: node->data.has_background_filters) > > > On 2016/01/21 23:37:09, jaydasika wrote: > > > > On 2016/01/21 23:06:20, ajuma wrote: > > > > > Is this ok even if the parent effect node isn't drawn? e.g. suppose the > > > parent > > > > > effect node isn't drawn, and suppose the current node has either an > > opacity > > > > > animation or background filters. Is it ok to treat this layer as > > > contributing > > > > to > > > > > a drawn surface in that situation? (This seems like a behavior change > from > > > > what > > > > > we do now.) > > > > > > > > > Also, if something has opacity 0 and an opacity animation, why does it > > need > > > to > > > > > contribute to a drawn surface? > > > > > > > > My understanding is > > > > contributes_to_drawn_surface = is_drawn && > !is_drawn_only_for_copy_request. > > > > So, when there are no copy requests, shouldn't they be equal ? > > > > (is_drawn is currently true for both the cases you mentioned) > > > > > > In the current code, if subtree_visible_from_ancestor is false, > layer_is_drawn > > > is false regardless of whether the layer has background filters or opacity > > > animations (unless the layer has a copy request). In the new logic, even if > > the > > > equivalent to subtree_visible_from_ancestor is false (that is, even if the > > > parent node isn't drawn), contributes_to_drawn_surface becomes true if > either > > we > > > have background filters or an opacity animation, even if we don't have a > copy > > > request. > > > > What I am probably trying to get at is that we should then change both > is_drawn > > and contributes_to_drawn_surface and not just the latter. > > It'd be really helpful to understand what goes wrong with the > contributes_to_drawn_surface logic from patch set 9. There is a layout test that crashes because we don't set contributes_to_drawn_surface true for layer with opacity 0 and background filters
On 2016/01/22 00:46:29, jaydasika wrote: > On 2016/01/22 00:41:15, ajuma wrote: > > On 2016/01/22 00:32:57, jaydasika wrote: > > > On 2016/01/22 00:24:42, ajuma wrote: > > > > > > > > > > https://codereview.chromium.org/1588093004/diff/210001/cc/trees/property_tree.cc > > > > File cc/trees/property_tree.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1588093004/diff/210001/cc/trees/property_tree... > > > > cc/trees/property_tree.cc:1095: node->data.has_background_filters) > > > > On 2016/01/21 23:37:09, jaydasika wrote: > > > > > On 2016/01/21 23:06:20, ajuma wrote: > > > > > > Is this ok even if the parent effect node isn't drawn? e.g. suppose > the > > > > parent > > > > > > effect node isn't drawn, and suppose the current node has either an > > > opacity > > > > > > animation or background filters. Is it ok to treat this layer as > > > > contributing > > > > > to > > > > > > a drawn surface in that situation? (This seems like a behavior change > > from > > > > > what > > > > > > we do now.) > > > > > > > > > > > Also, if something has opacity 0 and an opacity animation, why does it > > > need > > > > to > > > > > > contribute to a drawn surface? > > > > > > > > > > My understanding is > > > > > contributes_to_drawn_surface = is_drawn && > > !is_drawn_only_for_copy_request. > > > > > So, when there are no copy requests, shouldn't they be equal ? > > > > > (is_drawn is currently true for both the cases you mentioned) > > > > > > > > In the current code, if subtree_visible_from_ancestor is false, > > layer_is_drawn > > > > is false regardless of whether the layer has background filters or opacity > > > > animations (unless the layer has a copy request). In the new logic, even > if > > > the > > > > equivalent to subtree_visible_from_ancestor is false (that is, even if the > > > > parent node isn't drawn), contributes_to_drawn_surface becomes true if > > either > > > we > > > > have background filters or an opacity animation, even if we don't have a > > copy > > > > request. > > > > > > What I am probably trying to get at is that we should then change both > > is_drawn > > > and contributes_to_drawn_surface and not just the latter. > > > > It'd be really helpful to understand what goes wrong with the > > contributes_to_drawn_surface logic from patch set 9. > > There is a layout test that crashes because we don't set > contributes_to_drawn_surface true for layer with opacity 0 and background > filters But does it still crash if that logic is changed to something like contributes_to_drawn_surface = node->data.is_drawn && ((node->data.opacity != 0) || node->data.has_background_filters) (and keeping the logic that comes after that sets this to false if the parent isn't drawn)
https://codereview.chromium.org/1588093004/diff/210001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1588093004/diff/210001/cc/trees/property_tree... cc/trees/property_tree.cc:1095: node->data.has_background_filters) On 2016/01/22 00:24:42, ajuma wrote: > On 2016/01/21 23:37:09, jaydasika wrote: > > On 2016/01/21 23:06:20, ajuma wrote: > > > Is this ok even if the parent effect node isn't drawn? e.g. suppose the > parent > > > effect node isn't drawn, and suppose the current node has either an opacity > > > animation or background filters. Is it ok to treat this layer as > contributing > > to > > > a drawn surface in that situation? (This seems like a behavior change from > > what > > > we do now.) > > > > > Also, if something has opacity 0 and an opacity animation, why does it need > to > > > contribute to a drawn surface? > > > > My understanding is > > contributes_to_drawn_surface = is_drawn && !is_drawn_only_for_copy_request. > > So, when there are no copy requests, shouldn't they be equal ? > > (is_drawn is currently true for both the cases you mentioned) > > In the current code, if subtree_visible_from_ancestor is false, layer_is_drawn > is false regardless of whether the layer has background filters or opacity > animations (unless the layer has a copy request). In the new logic, even if the > equivalent to subtree_visible_from_ancestor is false (that is, even if the > parent node isn't drawn), contributes_to_drawn_surface becomes true if either we > have background filters or an opacity animation, even if we don't have a copy > request. Taking a step back, if layer has opacity 0 and hide_layer_and_subtree = false, in the current code is_drawn = true. So if we had L1->L2, L1's opacity = 0 and L2 had background filters, L2 will be drawn.
https://codereview.chromium.org/1588093004/diff/150001/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/1588093004/diff/150001/cc/trees/property_tree... cc/trees/property_tree.cc:1097: node->data.is_drawn && node->data.opacity != 0; On 2016/01/21 23:06:20, ajuma wrote: > What if you just change the above line to: > (node->data.is_drawn && node->data.opacity != 0) || > node->data.has_background_filters > > What are the cases that aren't covered by the logic here? It will work and fix all crashes.
On 2016/01/22 01:01:26, jaydasika wrote: > https://codereview.chromium.org/1588093004/diff/150001/cc/trees/property_tree.cc > File cc/trees/property_tree.cc (right): > > https://codereview.chromium.org/1588093004/diff/150001/cc/trees/property_tree... > cc/trees/property_tree.cc:1097: node->data.is_drawn && node->data.opacity != 0; > On 2016/01/21 23:06:20, ajuma wrote: > > What if you just change the above line to: > > (node->data.is_drawn && node->data.opacity != 0) || > > node->data.has_background_filters > > > > What are the cases that aren't covered by the logic here? > > It will work and fix all crashes. Let's do that then :)
Changed contributes_to_drawn_surface.
On 2016/01/22 16:04:14, jaydasika wrote: > Changed contributes_to_drawn_surface. Thanks a lot for all the investigation and iteration on this! lgtm % weiliangc
Thanks for working this out! LGTM
The CQ bit was checked by jaydasika@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1588093004/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1588093004/250001
Message was sent while issue was closed.
Description was changed from ========== This CL : * deletes is_hidden * computes if a layer is drawn using the effect tree BUG=575413 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== This CL : * deletes is_hidden * computes if a layer is drawn using the effect tree BUG=575413 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #14 (id:250001)
Message was sent while issue was closed.
Description was changed from ========== This CL : * deletes is_hidden * computes if a layer is drawn using the effect tree BUG=575413 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== This CL : * deletes is_hidden * computes if a layer is drawn using the effect tree BUG=575413 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/5bd215b33e6343d8ae7e52fb822dd552bf8b59a7 Cr-Commit-Position: refs/heads/master@{#371063} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/5bd215b33e6343d8ae7e52fb822dd552bf8b59a7 Cr-Commit-Position: refs/heads/master@{#371063}
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:250001) has been created in https://codereview.chromium.org/1621013002/ by thakis@chromium.org. The reason for reverting is: Speculative; might have broken tests on Chrome OS, see http://crbug.com/580806. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
