Make TabLayer recycling sticky to tab_id.

Make TabLayer recycling sticky to tab_id.

TabListSceneLayer is the parent of all currently visible phone tab
switcher CC layers.  It prefers to reuse existing layers instead of
recreating them from scratch.

However, the old logic was vector-based and had no guaranteed
correlation to tab ids, so when a layer in the middle became covered and
invisible, all layers after it were suddenly repurposed to display a
different tab than on the previous frame (and the last layer in the list
was destroyed, not the layer that became invisible).  The thumbnail
subsystems have problems handling this switchover perfectly and
sometimes flicker white or black for one frame, and this is also
inefficient.

This patch replaces the vector of TabLayers with a map sorted by id, so
that when a TabLayer becomes invisible, that one is removed without
disturbing any of the other layers.

BUG=638068
Committed: https://crrev.com/f859be05a8db1f6301119cd9dfd58d7ddd75ee22
Cr-Commit-Position: refs/heads/master@{#413244}

[aelias_OOO_until_Jul13, 2016-08-18 22:29:14]

The CQ bit was checked by aelias@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 22:30:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aelias_OOO_until_Jul13, 2016-08-18 23:08:06]

The CQ bit was checked by aelias@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 23:09:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aelias_OOO_until_Jul13, 2016-08-18 23:09:35]

The CQ bit was unchecked by aelias@chromium.org

[aelias_OOO_until_Jul13, 2016-08-18 23:09:42]

The CQ bit was checked by aelias@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 23:10:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aelias_OOO_until_Jul13, 2016-08-18 23:10:29]

aelias@chromium.org changed reviewers:
+ changwan@chromium.org

[aelias_OOO_until_Jul13, 2016-08-18 23:10:39]

Hi changwan@, PTAL.

[commit-bot: I haz the power, 2016-08-19 00:09:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-19 00:09:49]

Dry run: This issue passed the CQ dry run.

[Changwan Ryu, 2016-08-19 06:55:37]

lgtm w/ nits

nice, this improves readability as well!

https://codereview.chromium.org/2253423003/diff/20001/chrome/browser/android/...
File chrome/browser/android/compositor/scene_layer/tab_list_scene_layer.cc
(right):

https://codereview.chromium.org/2253423003/diff/20001/chrome/browser/android/...
chrome/browser/android/compositor/scene_layer/tab_list_scene_layer.cc:38:
tab.second->layer()->RemoveFromParent();
I assume that we're doing this because ClearUnusedLayers() does not remove layer
from parent. Is this the case?
I think either is fine, but please add some comment if you want to keep the code
here.

https://codereview.chromium.org/2253423003/diff/20001/chrome/browser/android/...
chrome/browser/android/compositor/scene_layer/tab_list_scene_layer.cc:137:
incognito, resource_manager_, layer_title_cache_, tab_content_manager_);
needs 4 space indentation

https://codereview.chromium.org/2253423003/diff/20001/chrome/browser/android/...
chrome/browser/android/compositor/scene_layer/tab_list_scene_layer.cc:191: for
(auto tab : tab_map_)
may not be a big deal, but I prefer the following:

for (auto it = tab_map_.cbegin(); it != tab_map_.cend(); ) {
  if (visible_tabs_this_frame_.find(it->first) ==
      visible_tabs_this_frame_.end())
    it = tab_map_.erase(it);
  else
    ++it;
}

BTW, I prefer merging this into FinishBuildingFrame() since both functions are
quite simple and ClearUnusedLayers() is not called elsewhere.

[aelias_OOO_until_Jul13, 2016-08-19 20:38:29]

https://codereview.chromium.org/2253423003/diff/20001/chrome/browser/android/...
File chrome/browser/android/compositor/scene_layer/tab_list_scene_layer.cc
(right):

https://codereview.chromium.org/2253423003/diff/20001/chrome/browser/android/...
chrome/browser/android/compositor/scene_layer/tab_list_scene_layer.cc:38:
tab.second->layer()->RemoveFromParent();
On 2016/08/19 at 06:55:37, Changwan Ryu wrote:
> I assume that we're doing this because ClearUnusedLayers() does not remove
layer from parent. Is this the case?
> I think either is fine, but please add some comment if you want to keep the
code here.

This is actually important to get z-order correctness among used layers.  I
added a comment.

https://codereview.chromium.org/2253423003/diff/20001/chrome/browser/android/...
chrome/browser/android/compositor/scene_layer/tab_list_scene_layer.cc:137:
incognito, resource_manager_, layer_title_cache_, tab_content_manager_);
On 2016/08/19 at 06:55:37, Changwan Ryu wrote:
> needs 4 space indentation

Done.

https://codereview.chromium.org/2253423003/diff/20001/chrome/browser/android/...
chrome/browser/android/compositor/scene_layer/tab_list_scene_layer.cc:191: for
(auto tab : tab_map_)
On 2016/08/19 at 06:55:37, Changwan Ryu wrote:
> may not be a big deal, but I prefer the following:

Done.

[aelias_OOO_until_Jul13, 2016-08-19 20:39:37]

The CQ bit was checked by aelias@chromium.org

[aelias_OOO_until_Jul13, 2016-08-19 20:39:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
changwan@chromium.org
Link to the patchset: https://codereview.chromium.org/2253423003/#ps40001
(title: "Code review comments")

[commit-bot: I haz the power, 2016-08-19 20:40:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 21:30:40]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-19 21:41:20]

Description was changed from

==========
Make TabLayer recycling sticky to tab_id.

TabListSceneLayer is the parent of all currently visible phone tab
switcher CC layers.  It prefers to reuse existing layers instead of
recreating them from scratch.

However, the old logic was vector-based and had no guaranteed
correlation to tab ids, so when a layer in the middle became covered and
invisible, all layers after it were suddenly repurposed to display a
different tab than on the previous frame (and the last layer in the list
was destroyed, not the layer that became invisible).  The thumbnail
subsystems have problems handling this switchover perfectly and
sometimes flicker white or black for one frame, and this is also
inefficient.

This patch replaces the vector of TabLayers with a map sorted by id, so
that when a TabLayer becomes invisible, that one is removed without
disturbing any of the other layers.

BUG=638068
==========

to

==========
Make TabLayer recycling sticky to tab_id.

TabListSceneLayer is the parent of all currently visible phone tab
switcher CC layers.  It prefers to reuse existing layers instead of
recreating them from scratch.

However, the old logic was vector-based and had no guaranteed
correlation to tab ids, so when a layer in the middle became covered and
invisible, all layers after it were suddenly repurposed to display a
different tab than on the previous frame (and the last layer in the list
was destroyed, not the layer that became invisible).  The thumbnail
subsystems have problems handling this switchover perfectly and
sometimes flicker white or black for one frame, and this is also
inefficient.

This patch replaces the vector of TabLayers with a map sorted by id, so
that when a TabLayer becomes invisible, that one is removed without
disturbing any of the other layers.

BUG=638068

Committed: https://crrev.com/f859be05a8db1f6301119cd9dfd58d7ddd75ee22
Cr-Commit-Position: refs/heads/master@{#413244}
==========

[commit-bot: I haz the power, 2016-08-19 21:41:21]

Patchset 3 (id:??) landed as
https://crrev.com/f859be05a8db1f6301119cd9dfd58d7ddd75ee22
Cr-Commit-Position: refs/heads/master@{#413244}