[Contextual Search] Remove layers when no longer needed.

chrome/browser/android/compositor/scene_layer/static_tab_scene_layer.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/android/compositor/scene_layer/static_tab_scene_layer.h"
 
 #include "cc/layers/layer.h"
 #include "chrome/browser/android/compositor/layer/content_layer.h"
 #include "chrome/browser/android/compositor/layer_title_cache.h"
 #include "chrome/browser/android/compositor/tab_content_manager.h"
@@ skipping 109 matching lines @@
       filters.Append(cc::FilterOperation::CreateBrightnessFilter(brightness_));
     layer_->SetFilters(filters);
   }
 }
 
 void StaticTabSceneLayer::SetContentSceneLayer(
     JNIEnv* env,
     const JavaParamRef<jobject>& jobj,
     const JavaParamRef<jobject>& jcontent_scene_layer) {
   SceneLayer* content_scene_layer = FromJavaObject(env, jcontent_scene_layer);
-  if (content_scene_layer && content_scene_layer->layer()) {
-    content_scene_layer_ = content_scene_layer->layer();
-    if (content_scene_layer_.get())
-      layer_->AddChild(content_scene_layer_);
-  } else if (content_scene_layer_) {
+  // If |content_scene_layer| is null, or different from the previously added

[David Trainor- moved to gerrit, 2015/12/02 16:34:13]

Would this be easier if we just pulled out the actually new ContentSceneLayer
here and used that?

scoped_refptr<Layer> layer = content_scene_layer ? content_scene_layer->layer()
: nullptr;

if (content_scene_layer_ && content_scene_layer_ != layer) {
  // Remove meh.
}

if (layer) {
  content_scene_layer_ = layer;
  // Add meh.
}

Also, do we want to do this each time we call SetContentSceneLayer?  What if
it's already set?  Even if the layers are equal it looks like we still call
AddChild below, which will remove it and readd it IIUC.

[pedro (no code reviews), 2015/12/02 23:56:14]

As we've discussed offline, the problem is that the content layer is being
re-added to hierarchy on every frame, so if we don't do the same, the content
scene layer will be occluded by the content layer.

For now, since we need to merge this in M48, I'll keep re-adding on every frame.
I put a TODO to revisit this later. We could force the content layer to be added
always at the first position (and only add it once), and then we could add the
content scene layer only once as well, and it will be guaranteed that it will be
placed after the content layer. But I'd rather do that later so we have more
time to properly test the new approach.

+  // layer (which means a new scene layer will replace the old one), then
+  // the layer should be removed from the hierarchy.
+  if (content_scene_layer_ && (!content_scene_layer || (content_scene_layer &&
+      content_scene_layer->layer().get() != content_scene_layer_.get()))) {
     content_scene_layer_->RemoveFromParent();
     content_scene_layer_ = nullptr;
   }
+
+  if (content_scene_layer && content_scene_layer->layer()) {
+    content_scene_layer_ = content_scene_layer->layer();
+    if (content_scene_layer_.get()) {
+      layer_->AddChild(content_scene_layer_);
+    }
+  }
 }
 
 static jlong Init(JNIEnv* env, const JavaParamRef<jobject>& jobj) {
   // This will automatically bind to the Java object and pass ownership there.
   StaticTabSceneLayer* scene_layer = new StaticTabSceneLayer(env, jobj);
   return reinterpret_cast<intptr_t>(scene_layer);
 }
 
 bool RegisterStaticTabSceneLayer(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }
 
 }  // namespace android
 }  // namespace chrome