New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 662427 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Simplify viewports in Java-side compositor code

Project Member Reported by mdjones@chromium.org, Nov 4 2016

Issue description

The viewports in the Java compositor code are confusing and take a complex path to their destinations. These should be cleaned up and have their responsibilities well defined.

See:

https://docs.google.com/document/d/10NtH_NAczKHqxkEOdyKCNW92xGmcrXqT6IVxi35RIuE/edit#heading=h.xfle5e1h6owf
 
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 11 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fe40aedc7a1c125eb384bd5d52e1874fa5403444

commit fe40aedc7a1c125eb384bd5d52e1874fa5403444
Author: mdjones <mdjones@chromium.org>
Date: Fri Nov 11 01:58:08 2016

Remove visible viewport dependency in CompositorView

BUG= 662427 

Review-Url: https://codereview.chromium.org/2482233002
Cr-Commit-Position: refs/heads/master@{#431456}

[modify] https://crrev.com/fe40aedc7a1c125eb384bd5d52e1874fa5403444/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
[modify] https://crrev.com/fe40aedc7a1c125eb384bd5d52e1874fa5403444/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
[modify] https://crrev.com/fe40aedc7a1c125eb384bd5d52e1874fa5403444/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
[modify] https://crrev.com/fe40aedc7a1c125eb384bd5d52e1874fa5403444/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManager.java
[modify] https://crrev.com/fe40aedc7a1c125eb384bd5d52e1874fa5403444/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerChromeTablet.java
[modify] https://crrev.com/fe40aedc7a1c125eb384bd5d52e1874fa5403444/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutProvider.java
[modify] https://crrev.com/fe40aedc7a1c125eb384bd5d52e1874fa5403444/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutRenderHost.java
[modify] https://crrev.com/fe40aedc7a1c125eb384bd5d52e1874fa5403444/chrome/android/javatests/src/org/chromium/chrome/browser/compositor/layouts/MockLayoutHost.java
[modify] https://crrev.com/fe40aedc7a1c125eb384bd5d52e1874fa5403444/chrome/browser/android/compositor/compositor_view.cc
[modify] https://crrev.com/fe40aedc7a1c125eb384bd5d52e1874fa5403444/chrome/browser/android/compositor/compositor_view.h

Project Member

Comment 2 by bugdroid1@chromium.org, Nov 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/24192fe5a799a1348671885882d24a826e0e1e21

commit 24192fe5a799a1348671885882d24a826e0e1e21
Author: mdjones <mdjones@chromium.org>
Date: Tue Nov 15 04:03:44 2016

Fix visible viewport in LayoutManager

There are corner cases where the browser process determines what the
browser's controls should be doing rather than the renderer. In these
cases the visible viewport needs to include space off-screen to
account for the content offset changing.

BUG=664956,  662427 

Review-Url: https://codereview.chromium.org/2505463002
Cr-Commit-Position: refs/heads/master@{#432091}

[modify] https://crrev.com/24192fe5a799a1348671885882d24a826e0e1e21/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManager.java

Project Member

Comment 3 by bugdroid1@chromium.org, Nov 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b7bb7b7f46fe783dbdeb5d81cd392cd85a9236d1

commit b7bb7b7f46fe783dbdeb5d81cd392cd85a9236d1
Author: mdjones <mdjones@chromium.org>
Date: Tue Nov 22 02:07:03 2016

Remove cached dp viewports in LayoutManager

BUG= 662427 

Review-Url: https://codereview.chromium.org/2499733002
Cr-Commit-Position: refs/heads/master@{#433741}

[modify] https://crrev.com/b7bb7b7f46fe783dbdeb5d81cd392cd85a9236d1/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
[modify] https://crrev.com/b7bb7b7f46fe783dbdeb5d81cd392cd85a9236d1/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
[modify] https://crrev.com/b7bb7b7f46fe783dbdeb5d81cd392cd85a9236d1/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java
[modify] https://crrev.com/b7bb7b7f46fe783dbdeb5d81cd392cd85a9236d1/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManager.java
[modify] https://crrev.com/b7bb7b7f46fe783dbdeb5d81cd392cd85a9236d1/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerChrome.java
[modify] https://crrev.com/b7bb7b7f46fe783dbdeb5d81cd392cd85a9236d1/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java
[modify] https://crrev.com/b7bb7b7f46fe783dbdeb5d81cd392cd85a9236d1/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutProvider.java
[modify] https://crrev.com/b7bb7b7f46fe783dbdeb5d81cd392cd85a9236d1/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/StaticLayout.java
[modify] https://crrev.com/b7bb7b7f46fe783dbdeb5d81cd392cd85a9236d1/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ToolbarSwipeLayout.java
[modify] https://crrev.com/b7bb7b7f46fe783dbdeb5d81cd392cd85a9236d1/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/components/LayoutTab.java
[modify] https://crrev.com/b7bb7b7f46fe783dbdeb5d81cd392cd85a9236d1/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/ContentOffsetProvider.java
[modify] https://crrev.com/b7bb7b7f46fe783dbdeb5d81cd392cd85a9236d1/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java
[modify] https://crrev.com/b7bb7b7f46fe783dbdeb5d81cd392cd85a9236d1/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/SimpleAnimationLayout.java
[modify] https://crrev.com/b7bb7b7f46fe783dbdeb5d81cd392cd85a9236d1/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/StackLayout.java
[modify] https://crrev.com/b7bb7b7f46fe783dbdeb5d81cd392cd85a9236d1/chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/StaticTabSceneLayer.java
[modify] https://crrev.com/b7bb7b7f46fe783dbdeb5d81cd392cd85a9236d1/chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/TabListSceneLayer.java
[modify] https://crrev.com/b7bb7b7f46fe783dbdeb5d81cd392cd85a9236d1/chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ToolbarSceneLayer.java

Project Member

Comment 4 by bugdroid1@chromium.org, Nov 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fd6719290c9dce87292ea653ac370542014030c7

commit fd6719290c9dce87292ea653ac370542014030c7
Author: mdjones <mdjones@chromium.org>
Date: Tue Nov 22 18:16:25 2016

CompositorViewHolder is source of truth for viewports

This change makes the CompositorViewHolder responsible for knowing
what the window and visible viewports are. Any class that needs to
know this information can pull it from the CompositorViewHolder
(a LayoutManagerHost) instead of having it pushed or caching it.
This removes a fair amount of state and complication in
LayoutManager.

BUG= 662427 

Review-Url: https://codereview.chromium.org/2509873003
Cr-Commit-Position: refs/heads/master@{#433916}

[modify] https://crrev.com/fd6719290c9dce87292ea653ac370542014030c7/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
[modify] https://crrev.com/fd6719290c9dce87292ea653ac370542014030c7/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManager.java
[modify] https://crrev.com/fd6719290c9dce87292ea653ac370542014030c7/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerChrome.java
[modify] https://crrev.com/fd6719290c9dce87292ea653ac370542014030c7/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java
[modify] https://crrev.com/fd6719290c9dce87292ea653ac370542014030c7/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerHost.java
[modify] https://crrev.com/fd6719290c9dce87292ea653ac370542014030c7/chrome/android/javatests/src/org/chromium/chrome/browser/compositor/layouts/MockLayoutHost.java

Project Member

Comment 5 by bugdroid1@chromium.org, Nov 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/af2850cc8821a3fec3fa6608b0aeed88a8eb947e

commit af2850cc8821a3fec3fa6608b0aeed88a8eb947e
Author: mariakhomenko <mariakhomenko@chromium.org>
Date: Tue Nov 22 19:12:08 2016

Revert of CompositorViewHolder is source of truth for viewports (patchset #5 id:80001 of https://codereview.chromium.org/2509873003/ )

Reason for revert:
Broke the builder: https://build.chromium.org/p/chromium.android/builders/Android%20arm64%20Builder%20%28dbg%29/builds/11030

Original issue's description:
> CompositorViewHolder is source of truth for viewports
>
> This change makes the CompositorViewHolder responsible for knowing
> what the window and visible viewports are. Any class that needs to
> know this information can pull it from the CompositorViewHolder
> (a LayoutManagerHost) instead of having it pushed or caching it.
> This removes a fair amount of state and complication in
> LayoutManager.
>
> BUG= 662427 
>
> Committed: https://crrev.com/fd6719290c9dce87292ea653ac370542014030c7
> Cr-Commit-Position: refs/heads/master@{#433916}

TBR=aelias@chromium.org,dtrainor@chromium.org,mdjones@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 662427 

Review-Url: https://codereview.chromium.org/2524833002
Cr-Commit-Position: refs/heads/master@{#433937}

[modify] https://crrev.com/af2850cc8821a3fec3fa6608b0aeed88a8eb947e/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
[modify] https://crrev.com/af2850cc8821a3fec3fa6608b0aeed88a8eb947e/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManager.java
[modify] https://crrev.com/af2850cc8821a3fec3fa6608b0aeed88a8eb947e/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerChrome.java
[modify] https://crrev.com/af2850cc8821a3fec3fa6608b0aeed88a8eb947e/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java
[modify] https://crrev.com/af2850cc8821a3fec3fa6608b0aeed88a8eb947e/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerHost.java
[modify] https://crrev.com/af2850cc8821a3fec3fa6608b0aeed88a8eb947e/chrome/android/javatests/src/org/chromium/chrome/browser/compositor/layouts/MockLayoutHost.java

Project Member

Comment 6 by bugdroid1@chromium.org, Nov 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7b2ace3be27bcaa12f371215c435d4cd59c5ff5d

commit 7b2ace3be27bcaa12f371215c435d4cd59c5ff5d
Author: mdjones <mdjones@chromium.org>
Date: Tue Nov 22 20:30:49 2016

CompositorViewHolder is source of truth for viewports

This change makes the CompositorViewHolder responsible for knowing
what the window and visible viewports are. Any class that needs to
know this information can pull it from the CompositorViewHolder
(a LayoutManagerHost) instead of having it pushed or caching it.
This removes a fair amount of state and complication in
LayoutManager.

BUG= 662427 

Committed: https://crrev.com/fd6719290c9dce87292ea653ac370542014030c7
Review-Url: https://codereview.chromium.org/2509873003
Cr-Original-Commit-Position: refs/heads/master@{#433916}
Cr-Commit-Position: refs/heads/master@{#433965}

[modify] https://crrev.com/7b2ace3be27bcaa12f371215c435d4cd59c5ff5d/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
[modify] https://crrev.com/7b2ace3be27bcaa12f371215c435d4cd59c5ff5d/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManager.java
[modify] https://crrev.com/7b2ace3be27bcaa12f371215c435d4cd59c5ff5d/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerChrome.java
[modify] https://crrev.com/7b2ace3be27bcaa12f371215c435d4cd59c5ff5d/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerDocument.java
[modify] https://crrev.com/7b2ace3be27bcaa12f371215c435d4cd59c5ff5d/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerHost.java
[modify] https://crrev.com/7b2ace3be27bcaa12f371215c435d4cd59c5ff5d/chrome/android/javatests/src/org/chromium/chrome/browser/compositor/layouts/MockLayoutHost.java

Project Member

Comment 7 by bugdroid1@chromium.org, Nov 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fdf5564b291c19901f507ad181ad53cbfdac55d2

commit fdf5564b291c19901f507ad181ad53cbfdac55d2
Author: mdjones <mdjones@chromium.org>
Date: Wed Nov 30 21:56:10 2016

Add viewport with assumption that browser controls always show

This change adds a viewport and a new sizing flag to the layouts. Some
layouts require the size of the screen to assume the browser controls
are showing when they would otherwise not be. To use this viewport, a
new sizing flag has been added to clearly identify its use:
USE_PREVIOUS_TOOLBAR_STATE.

As a result, this fixes a sizing bug in SimpleAnimationLayout.

BUG=668643,  662427 

Review-Url: https://codereview.chromium.org/2536553005
Cr-Commit-Position: refs/heads/master@{#435433}

[modify] https://crrev.com/fdf5564b291c19901f507ad181ad53cbfdac55d2/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
[modify] https://crrev.com/fdf5564b291c19901f507ad181ad53cbfdac55d2/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java
[modify] https://crrev.com/fdf5564b291c19901f507ad181ad53cbfdac55d2/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManager.java
[modify] https://crrev.com/fdf5564b291c19901f507ad181ad53cbfdac55d2/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerHost.java
[modify] https://crrev.com/fdf5564b291c19901f507ad181ad53cbfdac55d2/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/ToolbarSwipeLayout.java
[modify] https://crrev.com/fdf5564b291c19901f507ad181ad53cbfdac55d2/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/SimpleAnimationLayout.java
[modify] https://crrev.com/fdf5564b291c19901f507ad181ad53cbfdac55d2/chrome/android/javatests/src/org/chromium/chrome/browser/compositor/layouts/MockLayoutHost.java

Project Member

Comment 8 by bugdroid1@chromium.org, Dec 13 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/acff1ef1dd7e6a0c8f162bdf71f3e5efb9b89811

commit acff1ef1dd7e6a0c8f162bdf71f3e5efb9b89811
Author: aelias <aelias@chromium.org>
Date: Tue Dec 13 06:17:45 2016

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
Cr-Commit-Position: refs/heads/master@{#438080}

[modify] https://crrev.com/acff1ef1dd7e6a0c8f162bdf71f3e5efb9b89811/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/StaticLayout.java
[modify] https://crrev.com/acff1ef1dd7e6a0c8f162bdf71f3e5efb9b89811/chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/StaticTabSceneLayer.java
[modify] https://crrev.com/acff1ef1dd7e6a0c8f162bdf71f3e5efb9b89811/chrome/browser/android/compositor/layer/content_layer.cc
[modify] https://crrev.com/acff1ef1dd7e6a0c8f162bdf71f3e5efb9b89811/chrome/browser/android/compositor/layer/content_layer.h
[modify] https://crrev.com/acff1ef1dd7e6a0c8f162bdf71f3e5efb9b89811/chrome/browser/android/compositor/layer/tab_layer.cc
[modify] https://crrev.com/acff1ef1dd7e6a0c8f162bdf71f3e5efb9b89811/chrome/browser/android/compositor/layer/thumbnail_layer.cc
[modify] https://crrev.com/acff1ef1dd7e6a0c8f162bdf71f3e5efb9b89811/chrome/browser/android/compositor/layer/thumbnail_layer.h
[modify] https://crrev.com/acff1ef1dd7e6a0c8f162bdf71f3e5efb9b89811/chrome/browser/android/compositor/scene_layer/static_tab_scene_layer.cc
[modify] https://crrev.com/acff1ef1dd7e6a0c8f162bdf71f3e5efb9b89811/chrome/browser/android/compositor/scene_layer/static_tab_scene_layer.h

Status: Fixed (was: Assigned)

Sign in to add a comment