Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
664173002:
Prevent recreation of UI resource holder when created with resource ID (Closed)
|
Created:
6 years, 2 months ago by Changwan Ryu Modified:
6 years, 1 month ago CC:
chromium-reviews, cc-bugs_chromium.org, David Trainor- moved to gerrit, clholgat, Jaekyun Seok (inactive) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionPrevent recreation of UI resource holder if created with resource ID
Previously, UI resource holder was recreated whenever SetLayerTree was
called. This was problematic as the resource might be lost depending
on the timing of calling SetLayerTree. With this CL, we no longer
recreate UI resource holder when it was created with a resource ID.
BUG=405232
Committed: https://crrev.com/8a51976d3223ba3d004d3515832b98b15f014b05
Cr-Commit-Position: refs/heads/master@{#305104}
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Patch Set 3 : added NeedsCreation #
Total comments: 2
Patch Set 4 : fixed not to recreate when not necessary #
Total comments: 4
Patch Set 5 : addressed danakj@'s comments #
Total comments: 2
Patch Set 6 : adding SetPersistentUIResourceID #
Total comments: 1
Patch Set 7 : added unit test, going back to patch #5 #
Total comments: 6
Patch Set 8 : #Patch Set 9 : removed NeedsRecreation #
Total comments: 4
Patch Set 10 : verify DrawsContent() #
Total comments: 4
Patch Set 11 : reuse test_layer #
Messages
Total messages: 34 (4 generated)
changwan@chromium.org changed reviewers: + jamesr@chromium.org
danakj@chromium.org changed reviewers: + awoloszyn@chromium.org
danakj@chromium.org changed reviewers: + danakj@chromium.org
Andrew, wanna give this a first pass?
https://codereview.chromium.org/664173002/diff/20001/cc/layers/ui_resource_la... File cc/layers/ui_resource_layer.cc (right): https://codereview.chromium.org/664173002/diff/20001/cc/layers/ui_resource_la... cc/layers/ui_resource_layer.cc:115: UIResourceId ui_resource_id = 0; If someone calls SetBitmap with an empty bitmap, we will destroy the ScopedUIResourceHolder that contains it but retain its id. This will then be converted to a SharedUIResourceHolder that is now pointing to a resource_id that no longer exists. I think we need something a bit more explicit rather than trying to infer that this was a SharedUIResource. https://codereview.chromium.org/664173002/diff/20001/cc/layers/ui_resource_la... cc/layers/ui_resource_layer.cc:119: ui_resource_holder_ = nullptr; Something like // ui_resource_holder_ = nullptr; (remove this) if (layer_tree_host() && !bitmap_.empty()) { //unchanged } else if (ui_resource_holder_.get() && ui_resource_holder_->NeedsRecreation()) { ui_resource_holder_ = nullptr; } This was if it was a ScopedUIResource it will get cleaned and if it as a SharedUIResource, it will get retained.
https://codereview.chromium.org/664173002/diff/20001/cc/layers/ui_resource_la... File cc/layers/ui_resource_layer.cc (right): https://codereview.chromium.org/664173002/diff/20001/cc/layers/ui_resource_la... cc/layers/ui_resource_layer.cc:115: UIResourceId ui_resource_id = 0; On 2014/10/21 14:29:41, awoloszyn wrote: > If someone calls SetBitmap with an empty bitmap, we will destroy the > ScopedUIResourceHolder that contains it but retain its id. This will then be > converted to a SharedUIResourceHolder that is now pointing to a resource_id that > no longer exists. > > I think we need something a bit more explicit rather than trying to infer that > this was a SharedUIResource. Good idea. I overlooked it. Done. https://codereview.chromium.org/664173002/diff/20001/cc/layers/ui_resource_la... cc/layers/ui_resource_layer.cc:119: ui_resource_holder_ = nullptr; On 2014/10/21 14:29:41, awoloszyn wrote: > Something like > // ui_resource_holder_ = nullptr; (remove this) > if (layer_tree_host() && !bitmap_.empty()) { > //unchanged > } else if (ui_resource_holder_.get() && ui_resource_holder_->NeedsRecreation()) > { > ui_resource_holder_ = nullptr; > } > > This was if it was a ScopedUIResource it will get cleaned and if it as a > SharedUIResource, it will get retained. Done.
On 2014/10/21 17:28:57, Changwan Ryu wrote: > https://codereview.chromium.org/664173002/diff/20001/cc/layers/ui_resource_la... > File cc/layers/ui_resource_layer.cc (right): > > https://codereview.chromium.org/664173002/diff/20001/cc/layers/ui_resource_la... > cc/layers/ui_resource_layer.cc:115: UIResourceId ui_resource_id = 0; > On 2014/10/21 14:29:41, awoloszyn wrote: > > If someone calls SetBitmap with an empty bitmap, we will destroy the > > ScopedUIResourceHolder that contains it but retain its id. This will then be > > converted to a SharedUIResourceHolder that is now pointing to a resource_id > that > > no longer exists. > > > > I think we need something a bit more explicit rather than trying to infer that > > this was a SharedUIResource. > > Good idea. I overlooked it. Done. > > https://codereview.chromium.org/664173002/diff/20001/cc/layers/ui_resource_la... > cc/layers/ui_resource_layer.cc:119: ui_resource_holder_ = nullptr; > On 2014/10/21 14:29:41, awoloszyn wrote: > > Something like > > // ui_resource_holder_ = nullptr; (remove this) > > if (layer_tree_host() && !bitmap_.empty()) { > > //unchanged > > } else if (ui_resource_holder_.get() && > ui_resource_holder_->NeedsRecreation()) > > { > > ui_resource_holder_ = nullptr; > > } > > > > This was if it was a ScopedUIResource it will get cleaned and if it as a > > SharedUIResource, it will get retained. > > Done. PTAL. Andrew, just curious, are you going to go for the first pass only, in which case I'll need to rope in others?
I'll take a look when he's happy. On Thu, Oct 23, 2014 at 6:28 PM, <changwan@chromium.org> wrote: > On 2014/10/21 17:28:57, Changwan Ryu wrote: > > https://codereview.chromium.org/664173002/diff/20001/cc/ > layers/ui_resource_layer.cc > >> File cc/layers/ui_resource_layer.cc (right): >> > > > https://codereview.chromium.org/664173002/diff/20001/cc/ > layers/ui_resource_layer.cc#newcode115 > >> cc/layers/ui_resource_layer.cc:115: UIResourceId ui_resource_id = 0; >> On 2014/10/21 14:29:41, awoloszyn wrote: >> > If someone calls SetBitmap with an empty bitmap, we will destroy the >> > ScopedUIResourceHolder that contains it but retain its id. This will >> then be >> > converted to a SharedUIResourceHolder that is now pointing to a >> resource_id >> that >> > no longer exists. >> > >> > I think we need something a bit more explicit rather than trying to >> infer >> > that > >> > this was a SharedUIResource. >> > > Good idea. I overlooked it. Done. >> > > > https://codereview.chromium.org/664173002/diff/20001/cc/ > layers/ui_resource_layer.cc#newcode119 > >> cc/layers/ui_resource_layer.cc:119: ui_resource_holder_ = nullptr; >> On 2014/10/21 14:29:41, awoloszyn wrote: >> > Something like >> > // ui_resource_holder_ = nullptr; (remove this) >> > if (layer_tree_host() && !bitmap_.empty()) { >> > //unchanged >> > } else if (ui_resource_holder_.get() && >> ui_resource_holder_->NeedsRecreation()) >> > { >> > ui_resource_holder_ = nullptr; >> > } >> > >> > This was if it was a ScopedUIResource it will get cleaned and if it as a >> > SharedUIResource, it will get retained. >> > > Done. >> > > PTAL. Andrew, just curious, are you going to go for the first pass only, in > which case I'll need to rope in others? > > > https://codereview.chromium.org/664173002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/664173002/diff/40001/cc/layers/ui_resource_la... File cc/layers/ui_resource_layer.cc (right): https://codereview.chromium.org/664173002/diff/40001/cc/layers/ui_resource_la... cc/layers/ui_resource_layer.cc:126: ui_resource_holder_ = We should be able to do nothing in this case, since SharedUIResourceHolder just contains an id (integer), destroying and re-creating it seem un-necessary.
PTAL https://codereview.chromium.org/664173002/diff/40001/cc/layers/ui_resource_la... File cc/layers/ui_resource_layer.cc (right): https://codereview.chromium.org/664173002/diff/40001/cc/layers/ui_resource_la... cc/layers/ui_resource_layer.cc:126: ui_resource_holder_ = On 2014/10/24 14:15:13, awoloszyn wrote: > We should be able to do nothing in this case, since SharedUIResourceHolder just > contains an id (integer), destroying and re-creating it seem un-necessary. Done.
lgtm Will still have to wait for Dana, since I am not owner though.
Does the patch description still describe this change? "With this CL, we recreate UI resource holder with the existing resource ID, but only when bitmap is not set, in order to avoid any side effect." I don't see anything about bitmaps being set in this CL. https://codereview.chromium.org/664173002/diff/60001/cc/layers/ui_resource_la... File cc/layers/ui_resource_layer.cc (right): https://codereview.chromium.org/664173002/diff/60001/cc/layers/ui_resource_la... cc/layers/ui_resource_layer.cc:124: } else if (ui_resource_holder_.get() && no .get() needed https://codereview.chromium.org/664173002/diff/60001/cc/layers/ui_resource_la... File cc/layers/ui_resource_layer.h (right): https://codereview.chromium.org/664173002/diff/60001/cc/layers/ui_resource_la... cc/layers/ui_resource_layer.h:46: virtual bool NeedsRecreation(); why not pure virtual?
https://codereview.chromium.org/664173002/diff/60001/cc/layers/ui_resource_la... File cc/layers/ui_resource_layer.cc (right): https://codereview.chromium.org/664173002/diff/60001/cc/layers/ui_resource_la... cc/layers/ui_resource_layer.cc:124: } else if (ui_resource_holder_.get() && On 2014/10/27 18:47:08, danakj wrote: > no .get() needed Done. https://codereview.chromium.org/664173002/diff/60001/cc/layers/ui_resource_la... File cc/layers/ui_resource_layer.h (right): https://codereview.chromium.org/664173002/diff/60001/cc/layers/ui_resource_la... cc/layers/ui_resource_layer.h:46: virtual bool NeedsRecreation(); On 2014/10/27 18:47:08, danakj wrote: > why not pure virtual? Done.
On 2014/10/28 00:39:18, Changwan Ryu wrote: > https://codereview.chromium.org/664173002/diff/60001/cc/layers/ui_resource_la... > File cc/layers/ui_resource_layer.cc (right): > > https://codereview.chromium.org/664173002/diff/60001/cc/layers/ui_resource_la... > cc/layers/ui_resource_layer.cc:124: } else if (ui_resource_holder_.get() && > On 2014/10/27 18:47:08, danakj wrote: > > no .get() needed > > Done. > > https://codereview.chromium.org/664173002/diff/60001/cc/layers/ui_resource_la... > File cc/layers/ui_resource_layer.h (right): > > https://codereview.chromium.org/664173002/diff/60001/cc/layers/ui_resource_la... > cc/layers/ui_resource_layer.h:46: virtual bool NeedsRecreation(); > On 2014/10/27 18:47:08, danakj wrote: > > why not pure virtual? > > Done. Ping. Could you take another look?
https://codereview.chromium.org/664173002/diff/80001/cc/layers/ui_resource_la... File cc/layers/ui_resource_layer.cc (right): https://codereview.chromium.org/664173002/diff/80001/cc/layers/ui_resource_la... cc/layers/ui_resource_layer.cc:122: } else if (ui_resource_holder_ && !ui_resource_holder_->NeedsRecreation()) { So when you attach a layer to a LayerTreeHost its resource ID namespace becomes that of the LTH. Does it make sense to call UIResourceLayer::SetUIResourceId() when it's not attached to a LayerTreeHost? Why does this need to be done? Without a LTH it has no namespace for that uiresourceid.
https://codereview.chromium.org/664173002/diff/80001/cc/layers/ui_resource_la... File cc/layers/ui_resource_layer.cc (right): https://codereview.chromium.org/664173002/diff/80001/cc/layers/ui_resource_la... cc/layers/ui_resource_layer.cc:122: } else if (ui_resource_holder_ && !ui_resource_holder_->NeedsRecreation()) { On 2014/10/31 14:23:35, danakj wrote: > So when you attach a layer to a LayerTreeHost its resource ID namespace becomes > that of the LTH. > > Does it make sense to call UIResourceLayer::SetUIResourceId() when it's not > attached to a LayerTreeHost? Why does this need to be done? Without a LTH it has > no namespace for that uiresourceid. Hmm.. Let me give you more background: Because we aren't in control of the timing of when SetLayerTreeHost would be called, we simply do not know when to set UI resource ID. Previously, we did this by overriding SetLayerTreeHost in an inheriting class, but we are trying to switching from inheritance to containment model. (See the bug for more details.) If UI resource ID should be tied to a resource namespace (which is specific to LTH), probably what we should do is try to get some callback when SetLayerTreeHost is called. Do you have any suggestion of where we can do it?
On 2014/11/06 00:27:27, Changwan Ryu wrote: > https://codereview.chromium.org/664173002/diff/80001/cc/layers/ui_resource_la... > File cc/layers/ui_resource_layer.cc (right): > > https://codereview.chromium.org/664173002/diff/80001/cc/layers/ui_resource_la... > cc/layers/ui_resource_layer.cc:122: } else if (ui_resource_holder_ && > !ui_resource_holder_->NeedsRecreation()) { > On 2014/10/31 14:23:35, danakj wrote: > > So when you attach a layer to a LayerTreeHost its resource ID namespace > becomes > > that of the LTH. > > > > Does it make sense to call UIResourceLayer::SetUIResourceId() when it's not > > attached to a LayerTreeHost? Why does this need to be done? Without a LTH it > has > > no namespace for that uiresourceid. > > Hmm.. Let me give you more background: > > Because we aren't in control of the timing of when SetLayerTreeHost would be > called, we simply do not know when to set UI resource ID. > > Previously, we did this by overriding SetLayerTreeHost in an inheriting class, > but we are trying to switching from inheritance to containment model. (See the > bug for more details.) > > If UI resource ID should be tied to a resource namespace (which is specific to > LTH), probably what we should do is try to get some callback when > SetLayerTreeHost is called. Do you have any suggestion of where we can do it? Never mind. After reading your comment I realized that this can be achieved in some other code piece. Thank you for your careful review so far. Let me abandon this.
On 2014/11/06 02:01:42, Changwan Ryu wrote: > On 2014/11/06 00:27:27, Changwan Ryu wrote: > > > https://codereview.chromium.org/664173002/diff/80001/cc/layers/ui_resource_la... > > File cc/layers/ui_resource_layer.cc (right): > > > > > https://codereview.chromium.org/664173002/diff/80001/cc/layers/ui_resource_la... > > cc/layers/ui_resource_layer.cc:122: } else if (ui_resource_holder_ && > > !ui_resource_holder_->NeedsRecreation()) { > > On 2014/10/31 14:23:35, danakj wrote: > > > So when you attach a layer to a LayerTreeHost its resource ID namespace > > becomes > > > that of the LTH. > > > > > > Does it make sense to call UIResourceLayer::SetUIResourceId() when it's not > > > attached to a LayerTreeHost? Why does this need to be done? Without a LTH it > > has > > > no namespace for that uiresourceid. > > > > Hmm.. Let me give you more background: > > > > Because we aren't in control of the timing of when SetLayerTreeHost would be > > called, we simply do not know when to set UI resource ID. > > > > Previously, we did this by overriding SetLayerTreeHost in an inheriting class, > > but we are trying to switching from inheritance to containment model. (See the > > bug for more details.) > > > > If UI resource ID should be tied to a resource namespace (which is specific to > > LTH), probably what we should do is try to get some callback when > > SetLayerTreeHost is called. Do you have any suggestion of where we can do it? > > Never mind. After reading your comment I realized that this can be achieved in > some other code piece. Thank you for your careful review so far. Let me abandon > this. Sorry for reviving this CL. I tried several approaches but none of them looked good. Probably resource IDs may not make sense when LTH isn't there, but I find it very difficult to use UIResourceLayer without this feature. So I'd like to make a separate function called SetPersistentUIResourceId(). Please check my rationale below: In our use cases, namespace does not change, and we want to show / hide / add / remove subtrees that has UI resources associated with them. In order to achieve this, we build subtrees, assign resource IDs first and attach / detach them to/from main tree freely. But when you attach a subtree to the main tree for the first time, SetLayerTreeHost() will be called implicitly, and all the IDs should be assigned to the correct values. With the current logic, there are some ways to work around this issue, but both are very painful. 1) Pass around LayerTreeHost whenever you’re building a subtree, and update it when it changes, so that LayerTreeHost won’t be updated. --> Passing around LayerTreeHost and update them is a painful process. 2) Whenever the subtree is attached (or its parent or grand parent is) to another tree, reassign the IDs. --> It is difficult to tell the timing that one's ancestor is attached to another tree. Both content/browser/android/edge_effect.cc and edge_effect_l.cc implement SetParent() following this approach, but they cannot reassign resource IDs if parent is attached to a new tree. Thanks.
https://codereview.chromium.org/664173002/diff/100001/cc/layers/ui_resource_l... File cc/layers/ui_resource_layer.h (right): https://codereview.chromium.org/664173002/diff/100001/cc/layers/ui_resource_l... cc/layers/ui_resource_layer.h:31: // Use this if |resource_id| should be preserved even when LayerTreeHost This doesn't make sense. If you moved it from one LayerTreeHost to another one, the ID would be completely invalid. I think that I prefer your previous patch then, with a comment on SetUIResourceID() that if you use that method you are responsible for updating the id if the layer moves between compositors. And can you add a unit test to show that this works as intended?
On 2014/11/19 15:56:04, danakj wrote: > https://codereview.chromium.org/664173002/diff/100001/cc/layers/ui_resource_l... > File cc/layers/ui_resource_layer.h (right): > > https://codereview.chromium.org/664173002/diff/100001/cc/layers/ui_resource_l... > cc/layers/ui_resource_layer.h:31: // Use this if |resource_id| should be > preserved even when LayerTreeHost > This doesn't make sense. If you moved it from one LayerTreeHost to another one, > the ID would be completely invalid. > > I think that I prefer your previous patch then, with a comment on > SetUIResourceID() that if you use that method you are responsible for updating > the id if the layer moves between compositors. > > And can you add a unit test to show that this works as intended? Thanks for the quick response! I've added a comment and modified a unit test to test it. Please take another look.
https://codereview.chromium.org/664173002/diff/120001/cc/layers/ui_resource_l... File cc/layers/ui_resource_layer.cc (right): https://codereview.chromium.org/664173002/diff/120001/cc/layers/ui_resource_l... cc/layers/ui_resource_layer.cc:122: } else if (ui_resource_holder_ && !ui_resource_holder_->NeedsRecreation()) { I think we could possibly make this simpler to follow. What if we move the clearing of ui_resource_holder_ to the Set* methods? If you have a bitmap in SetBitmap, then you'll make a new ScopedUIResourceHolder here. If you don't have a bitmap in SetBitmap, then you'll have nothing to clear here. Likewise, if you have a resource_id in SetUIResourceID then you don't want to clear it here, and if you didn't have a resource_id, then there's nothing to destroy here. Do you even need the NeedsRecreation bit then? https://codereview.chromium.org/664173002/diff/120001/cc/layers/ui_resource_l... File cc/layers/ui_resource_layer.h (right): https://codereview.chromium.org/664173002/diff/120001/cc/layers/ui_resource_l... cc/layers/ui_resource_layer.h:35: UIResourceId GetUIResourceId(); If it's just for testing, you could expose it with a subclass in the test code.
https://codereview.chromium.org/664173002/diff/120001/cc/layers/ui_resource_l... File cc/layers/ui_resource_layer.cc (right): https://codereview.chromium.org/664173002/diff/120001/cc/layers/ui_resource_l... cc/layers/ui_resource_layer.cc:122: } else if (ui_resource_holder_ && !ui_resource_holder_->NeedsRecreation()) { On 2014/11/20 16:38:08, danakj wrote: > I think we could possibly make this simpler to follow. What if we move the > clearing of ui_resource_holder_ to the Set* methods? > > If you have a bitmap in SetBitmap, then you'll make a new ScopedUIResourceHolder > here. If you don't have a bitmap in SetBitmap, then you'll have nothing to clear > here. > > Likewise, if you have a resource_id in SetUIResourceID then you don't want to > clear it here, and if you didn't have a resource_id, then there's nothing to > destroy here. > > Do you even need the NeedsRecreation bit then? I'm not sure I'm following you correctly, but I'm concerned about SetLayerTreeHost() logic then. When layer tree host changes, we set empty resource holder only when 1. layer tree host is invalid or 2. bitmap is empty but previously it was scoped resource holder. otherwise we will create a new scoped resource holder. But it seems that the condition #2 cannot be detected without NeedsRecreation bit. Could you explain a bit more about SetLayerTreeHost workflow? https://codereview.chromium.org/664173002/diff/120001/cc/layers/ui_resource_l... File cc/layers/ui_resource_layer.h (right): https://codereview.chromium.org/664173002/diff/120001/cc/layers/ui_resource_l... cc/layers/ui_resource_layer.h:35: UIResourceId GetUIResourceId(); On 2014/11/20 16:38:08, danakj wrote: > If it's just for testing, you could expose it with a subclass in the test code. Done.
https://codereview.chromium.org/664173002/diff/120001/cc/layers/ui_resource_l... File cc/layers/ui_resource_layer.cc (right): https://codereview.chromium.org/664173002/diff/120001/cc/layers/ui_resource_l... cc/layers/ui_resource_layer.cc:122: } else if (ui_resource_holder_ && !ui_resource_holder_->NeedsRecreation()) { On 2014/11/20 20:52:26, Changwan Ryu wrote: > On 2014/11/20 16:38:08, danakj wrote: > > I think we could possibly make this simpler to follow. What if we move the > > clearing of ui_resource_holder_ to the Set* methods? > > > > If you have a bitmap in SetBitmap, then you'll make a new > ScopedUIResourceHolder > > here. If you don't have a bitmap in SetBitmap, then you'll have nothing to > clear > > here. > > > > Likewise, if you have a resource_id in SetUIResourceID then you don't want to > > clear it here, and if you didn't have a resource_id, then there's nothing to > > destroy here. > > > > Do you even need the NeedsRecreation bit then? > > I'm not sure I'm following you correctly, but I'm concerned about > SetLayerTreeHost() logic then. > > When layer tree host changes, we set empty resource holder only when > 1. layer tree host is invalid or > 2. bitmap is empty but previously it was scoped resource holder. > > otherwise we will create a new scoped resource holder. > > But it seems that the condition #2 cannot be detected without NeedsRecreation > bit. > > Could you explain a bit more about SetLayerTreeHost workflow? You only have a bitmap_ != null if you called SetBitmap and have a ScopedUIResource as a result. In all other cases you either don't want to clear the ui_resource_holder_, or it is already empty, I think?
Thanks for the quick response. PTAL https://codereview.chromium.org/664173002/diff/120001/cc/layers/ui_resource_l... File cc/layers/ui_resource_layer.cc (right): https://codereview.chromium.org/664173002/diff/120001/cc/layers/ui_resource_l... cc/layers/ui_resource_layer.cc:122: } else if (ui_resource_holder_ && !ui_resource_holder_->NeedsRecreation()) { On 2014/11/20 20:56:04, danakj wrote: > On 2014/11/20 20:52:26, Changwan Ryu wrote: > > On 2014/11/20 16:38:08, danakj wrote: > > > I think we could possibly make this simpler to follow. What if we move the > > > clearing of ui_resource_holder_ to the Set* methods? > > > > > > If you have a bitmap in SetBitmap, then you'll make a new > > ScopedUIResourceHolder > > > here. If you don't have a bitmap in SetBitmap, then you'll have nothing to > > clear > > > here. > > > > > > Likewise, if you have a resource_id in SetUIResourceID then you don't want > to > > > clear it here, and if you didn't have a resource_id, then there's nothing to > > > destroy here. > > > > > > Do you even need the NeedsRecreation bit then? > > > > I'm not sure I'm following you correctly, but I'm concerned about > > SetLayerTreeHost() logic then. > > > > When layer tree host changes, we set empty resource holder only when > > 1. layer tree host is invalid or > > 2. bitmap is empty but previously it was scoped resource holder. > > > > otherwise we will create a new scoped resource holder. > > > > But it seems that the condition #2 cannot be detected without NeedsRecreation > > bit. > > > > Could you explain a bit more about SetLayerTreeHost workflow? > > You only have a bitmap_ != null if you called SetBitmap and have a > ScopedUIResource as a result. In all other cases you either don't want to clear > the ui_resource_holder_, or it is already empty, I think? How about now?
Thanks, yes that's what I was thinking of. LGTM https://codereview.chromium.org/664173002/diff/160001/cc/layers/ui_resource_l... File cc/layers/ui_resource_layer_unittest.cc (right): https://codereview.chromium.org/664173002/diff/160001/cc/layers/ui_resource_l... cc/layers/ui_resource_layer_unittest.cc:129: EXPECT_EQ(new_resource->id(), new_test_layer->GetUIResourceId()); can you verify DrawsContent() as well like in the other cases?
https://codereview.chromium.org/664173002/diff/160001/cc/layers/ui_resource_l... File cc/layers/ui_resource_layer_unittest.cc (right): https://codereview.chromium.org/664173002/diff/160001/cc/layers/ui_resource_l... cc/layers/ui_resource_layer_unittest.cc:99: test_layer->SetIsDrawable(true); You could make this test_layer your new type, so you don't need to duplicate these Set calls if you want.
PTAL https://codereview.chromium.org/664173002/diff/160001/cc/layers/ui_resource_l... File cc/layers/ui_resource_layer_unittest.cc (right): https://codereview.chromium.org/664173002/diff/160001/cc/layers/ui_resource_l... cc/layers/ui_resource_layer_unittest.cc:99: test_layer->SetIsDrawable(true); On 2014/11/20 21:48:46, danakj wrote: > You could make this test_layer your new type, so you don't need to duplicate > these Set calls if you want. Done. https://codereview.chromium.org/664173002/diff/160001/cc/layers/ui_resource_l... cc/layers/ui_resource_layer_unittest.cc:129: EXPECT_EQ(new_resource->id(), new_test_layer->GetUIResourceId()); On 2014/11/20 21:32:48, danakj wrote: > can you verify DrawsContent() as well like in the other cases? Done.
Yep LGTM w/ a couple nits if you like for the test https://codereview.chromium.org/664173002/diff/180001/cc/layers/ui_resource_l... File cc/layers/ui_resource_layer_unittest.cc (right): https://codereview.chromium.org/664173002/diff/180001/cc/layers/ui_resource_l... cc/layers/ui_resource_layer_unittest.cc:122: scoped_refptr<TestUIResourceLayer> new_test_layer = it's a nit, but you can reuse the test_layer here by doing layer_tree_host_->SetRootLayer(nullptr); make shared resource SetUIResourceID layer_tree_host_->SetRootLayer(test_layer); https://codereview.chromium.org/664173002/diff/180001/cc/layers/ui_resource_l... cc/layers/ui_resource_layer_unittest.cc:124: scoped_ptr<ScopedUIResource> new_resource = ScopedUIResource::Create( s/new/shared/
https://codereview.chromium.org/664173002/diff/180001/cc/layers/ui_resource_l... File cc/layers/ui_resource_layer_unittest.cc (right): https://codereview.chromium.org/664173002/diff/180001/cc/layers/ui_resource_l... cc/layers/ui_resource_layer_unittest.cc:122: scoped_refptr<TestUIResourceLayer> new_test_layer = On 2014/11/20 22:07:26, danakj wrote: > it's a nit, but you can reuse the test_layer here by doing > > layer_tree_host_->SetRootLayer(nullptr); > > make shared resource > SetUIResourceID > > layer_tree_host_->SetRootLayer(test_layer); Done. https://codereview.chromium.org/664173002/diff/180001/cc/layers/ui_resource_l... cc/layers/ui_resource_layer_unittest.cc:124: scoped_ptr<ScopedUIResource> new_resource = ScopedUIResource::Create( On 2014/11/20 22:07:26, danakj wrote: > s/new/shared/ Done.
The CQ bit was checked by changwan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/664173002/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/8a51976d3223ba3d004d3515832b98b15f014b05 Cr-Commit-Position: refs/heads/master@{#305104} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
