Simplify content_layer attachment and viewporting logic.

Simplify content_layer attachment and viewporting logic.

I noticed a lot of complex logic in content_layer.cc and
static_tab_scene_layer.cc that appears to do nothing useful anymore.  I
simplified it by:
1) Always detaching and reattaching the live and static sublayers
instead of trying to preserve the existing structure.  (There shouldn't
be any performance cost to this.)
2) Following a much simpler policy for positioning and clipping.  I
compared the behavior of the old and new logic in a variety of device
rotation, tab switcher and URL-bar-hiding scenarios, and I haven't
yet observed any regression.

BUG=662427
Committed: https://crrev.com/acff1ef1dd7e6a0c8f162bdf71f3e5efb9b89811
Cr-Commit-Position: refs/heads/master@{#438080}

[aelias_OOO_until_Jul13, 2016-12-09 23:46:21]

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

[aelias_OOO_until_Jul13, 2016-12-09 23:47:45]

Hi changwan@, PTAL.  I fixed a bug in ClearClip as compared to
https://codereview.chromium.org/2499863002/, so it's ready for review.

[aelias_OOO_until_Jul13, 2016-12-09 23:48:40]

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

[commit-bot: I haz the power, 2016-12-09 23:49:39]

Dry run: 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-12-10 01:51:28]

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

[commit-bot: I haz the power, 2016-12-10 01:51:28]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[aelias_OOO_until_Jul13, 2016-12-10 02:33:20]

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

[commit-bot: I haz the power, 2016-12-10 02:33:56]

Dry run: 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-12-10 03:14:50]

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

[commit-bot: I haz the power, 2016-12-10 03:14:51]

Dry run: This issue passed the CQ dry run.

[Changwan Ryu, 2016-12-12 07:38:04]

https://codereview.chromium.org/2566753004/diff/20001/chrome/browser/android/...
File chrome/browser/android/compositor/layer/content_layer.h (right):

https://codereview.chromium.org/2566753004/diff/20001/chrome/browser/android/...
chrome/browser/android/compositor/layer/content_layer.h:38: bool should_clip,
optional: how about removing this parameter and check clip.IsEmpty()?

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

https://codereview.chromium.org/2566753004/diff/20001/chrome/browser/android/...
chrome/browser/android/compositor/scene_layer/static_tab_scene_layer.cc:78: x +=
content_viewport_x;
Probably content_viewport_x is always 0 here. If that is the case, we don't need
to have this line. (Otherwise, I think we may need a comment explaining why
we're only adding viewport_x.)

https://codereview.chromium.org/2566753004/diff/20001/chrome/browser/android/...
chrome/browser/android/compositor/scene_layer/static_tab_scene_layer.cc:79: y +=
content_offset_y;
This is a great reduction. But now we're passing lots of parameters that aren't
used. How about removing the following parameters?

(content_viewport_x)
content_viewport_y
content_viewport_width
content_viewport_height
width
height

[aelias_OOO_until_Jul13, 2016-12-13 02:15:58]

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

[aelias_OOO_until_Jul13, 2016-12-13 02:16:05]

https://codereview.chromium.org/2566753004/diff/20001/chrome/browser/android/...
File chrome/browser/android/compositor/layer/content_layer.h (right):

https://codereview.chromium.org/2566753004/diff/20001/chrome/browser/android/...
chrome/browser/android/compositor/layer/content_layer.h:38: bool should_clip,
On 2016/12/12 at 07:38:04, Changwan Ryu wrote:
> optional: how about removing this parameter and check clip.IsEmpty()?

I considered that, but that would create a weird semantic inversion that might
lead to bugs.  I think an empty clip should result in nothing being shown, not
everything being shown.

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

https://codereview.chromium.org/2566753004/diff/20001/chrome/browser/android/...
chrome/browser/android/compositor/scene_layer/static_tab_scene_layer.cc:78: x +=
content_viewport_x;
On 2016/12/12 at 07:38:04, Changwan Ryu wrote:
> Probably content_viewport_x is always 0 here. If that is the case, we don't
need to have this line. (Otherwise, I think we may need a comment explaining why
we're only adding viewport_x.)

OK, removed this addition as you're right it doesn't do anything and is
inconsistent now.

https://codereview.chromium.org/2566753004/diff/20001/chrome/browser/android/...
chrome/browser/android/compositor/scene_layer/static_tab_scene_layer.cc:79: y +=
content_offset_y;
On 2016/12/12 at 07:38:04, Changwan Ryu wrote:
> This is a great reduction. But now we're passing lots of parameters that
aren't used. How about removing the following parameters?
> 
> (content_viewport_x)
> content_viewport_y
> content_viewport_width
> content_viewport_height
> width
> height

Done.  I also removed content_offset_y and added it to "y" in Java.

[commit-bot: I haz the power, 2016-12-13 02:16:29]

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-12-13 02:18:37]

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

[commit-bot: I haz the power, 2016-12-13 02:18:59]

Dry run: 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-12-13 03:09:47]

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

[commit-bot: I haz the power, 2016-12-13 03:09:48]

Dry run: This issue passed the CQ dry run.

[Changwan Ryu, 2016-12-13 05:53:09]

lgtm

[Changwan Ryu, 2016-12-13 05:53:24]

The CQ bit was checked by changwan@chromium.org

[commit-bot: I haz the power, 2016-12-13 05:53:46]

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

[Changwan Ryu, 2016-12-13 05:55:41]

The CQ bit was unchecked by changwan@chromium.org

[Changwan Ryu, 2016-12-13 06:12:27]

The CQ bit was checked by changwan@chromium.org

[commit-bot: I haz the power, 2016-12-13 06:12:53]

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-12-13 06:17:51]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481609547936050,
"parent_rev": "c65374e1a267e0a26d87f2d9bda6edfa4728b152", "commit_rev":
"fbcf5f4c01198dbb4de7c1ebe9dd8a264fbce21b"}

[commit-bot: I haz the power, 2016-12-13 06:18:16]

Description was changed from

==========
Simplify content_layer attachment and viewporting logic.

I noticed a lot of complex logic in content_layer.cc and
static_tab_scene_layer.cc that appears to do nothing useful anymore.  I
simplified it by:
1) Always detaching and reattaching the live and static sublayers
instead of trying to preserve the existing structure.  (There shouldn't
be any performance cost to this.)
2) Following a much simpler policy for positioning and clipping.  I
compared the behavior of the old and new logic in a variety of device
rotation, tab switcher and URL-bar-hiding scenarios, and I haven't
yet observed any regression.

BUG=662427
==========

to

==========
Simplify content_layer attachment and viewporting logic.

I noticed a lot of complex logic in content_layer.cc and
static_tab_scene_layer.cc that appears to do nothing useful anymore.  I
simplified it by:
1) Always detaching and reattaching the live and static sublayers
instead of trying to preserve the existing structure.  (There shouldn't
be any performance cost to this.)
2) Following a much simpler policy for positioning and clipping.  I
compared the behavior of the old and new logic in a variety of device
rotation, tab switcher and URL-bar-hiding scenarios, and I haven't
yet observed any regression.

BUG=662427

Review-Url: https://codereview.chromium.org/2566753004
==========

[commit-bot: I haz the power, 2016-12-13 06:18:17]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-13 06:20:21]

Description was changed from

==========
Simplify content_layer attachment and viewporting logic.

I noticed a lot of complex logic in content_layer.cc and
static_tab_scene_layer.cc that appears to do nothing useful anymore.  I
simplified it by:
1) Always detaching and reattaching the live and static sublayers
instead of trying to preserve the existing structure.  (There shouldn't
be any performance cost to this.)
2) Following a much simpler policy for positioning and clipping.  I
compared the behavior of the old and new logic in a variety of device
rotation, tab switcher and URL-bar-hiding scenarios, and I haven't
yet observed any regression.

BUG=662427

Review-Url: https://codereview.chromium.org/2566753004
==========

to

==========
Simplify content_layer attachment and viewporting logic.

I noticed a lot of complex logic in content_layer.cc and
static_tab_scene_layer.cc that appears to do nothing useful anymore.  I
simplified it by:
1) Always detaching and reattaching the live and static sublayers
instead of trying to preserve the existing structure.  (There shouldn't
be any performance cost to this.)
2) Following a much simpler policy for positioning and clipping.  I
compared the behavior of the old and new logic in a variety of device
rotation, tab switcher and URL-bar-hiding scenarios, and I haven't
yet observed any regression.

BUG=662427

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

[commit-bot: I haz the power, 2016-12-13 06:20:23]

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