Prevent recreation of UI resource holder when created with resource ID

cc/layers/ui_resource_layer.cc

Index: cc/layers/ui_resource_layer.cc
diff --git a/cc/layers/ui_resource_layer.cc b/cc/layers/ui_resource_layer.cc
index 944cf7d54844c7016feb32eacfc401fb590146ab..8d6f8590dedada5b52dbeb8aa7ae1a8f4cfa0014 100644
--- a/cc/layers/ui_resource_layer.cc
+++ b/cc/layers/ui_resource_layer.cc
@@ -25,6 +25,8 @@ class ScopedUIResourceHolder : public UIResourceLayer::UIResourceHolder {
   }
   UIResourceId id() override { return resource_->id(); }
 
+  bool NeedsRecreation() override { return false; }
+
  private:
   ScopedUIResourceHolder(LayerTreeHost* host, const SkBitmap& skbitmap) {
     resource_ = ScopedUIResource::Create(host, UIResourceBitmap(skbitmap));
@@ -41,6 +43,8 @@ class SharedUIResourceHolder : public UIResourceLayer::UIResourceHolder {
 
   UIResourceId id() override { return id_; }
 
+  bool NeedsRecreation() override { return true; }
+
  private:
   explicit SharedUIResourceHolder(UIResourceId id) : id_(id) {}
 
@@ -112,10 +116,11 @@ void UIResourceLayer::SetLayerTreeHost(LayerTreeHost* host) {
 }
 
 void UIResourceLayer::RecreateUIResourceHolder() {
-  ui_resource_holder_ = nullptr;
   if (layer_tree_host() && !bitmap_.empty()) {
     ui_resource_holder_ =
         ScopedUIResourceHolder::Create(layer_tree_host(), bitmap_);
+  } else if (ui_resource_holder_ && !ui_resource_holder_->NeedsRecreation()) {

[danakj, 2014/11/20 16:38:08]

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?

[Changwan Ryu, 2014/11/20 20:52:26]

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?

[danakj, 2014/11/20 20:56:04]

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?

[Changwan Ryu, 2014/11/20 21:29:40]

How about now?

+    ui_resource_holder_ = nullptr;
   }
   UpdateDrawsContent(HasDrawableContent());
 }
@@ -140,6 +145,12 @@ void UIResourceLayer::SetUIResourceId(UIResourceId resource_id) {
   SetNeedsCommit();
 }
 
+UIResourceId UIResourceLayer::GetUIResourceId() {
+  if (ui_resource_holder_)
+    return ui_resource_holder_->id();
+  return 0;
+}
+
 bool UIResourceLayer::HasDrawableContent() const {
   return ui_resource_holder_ && ui_resource_holder_->id() &&
          Layer::HasDrawableContent();