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

Issue 624666 link

Starred by 3 users

Issue metadata

Status: WontFix
Owner:
Closed: Dec 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Sharing Code from RenderWidgetHostViewAndroid for Blimp.

Project Member Reported by khushals...@chromium.org, Jun 30 2016

Issue description

There are some components in the RenderWidgetHostViewAndroid that should potentially be shared with BlimpHostView or equivalent class in Blimp.

- Consuming Delegated Compositor Frames: This involves the logic for building SurfaceLayers from the CompositorFrame. It needs accessing the cc::SurfaceManager and creating the cc::SurfaceIdAllocator, both of which live in CompositorImpl.

- BeginFrameScheduling: This includes the logic for throttling compositor frames. If the renderers are going to receive notifications from the browser for when to start an impl frame, we want to do the same for the Blimp Compositor.

- Input Handling: There is some logic for handling web gesture events. I'm not sure if this is absolutely necessary but would be great if it can be shared.
 
Cc: klo...@chromium.org
You might want to be able to do readbacks for thumbnails and tab switching also (with ETC1 compression).

You could use sk picture and raster but it seems like it might get slow esp. in the tab stack and/or when you have a lot of layers and images.
Let's start simple and just pull the Surfaces-related stuff out (and deal with readback, locking and frame retention afterwards):

https://docs.google.com/document/d/1bwPCOEPOZw-Nk3QAv1c8dgv06NwWe-aO6LjU6nUQQOE/edit
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 2 2016

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

commit e0835e17db7ecd698cac1e87eeba474f6210e496
Author: sievers <sievers@chromium.org>
Date: Sat Jul 02 00:41:22 2016

Android: Remove some dead code

And collapse a function in preparing for moving out and sharing
the code that handles Surfaces (SubmitCompositorFrame()).

BUG= 624666 

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

[modify] https://crrev.com/e0835e17db7ecd698cac1e87eeba474f6210e496/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/e0835e17db7ecd698cac1e87eeba474f6210e496/content/browser/renderer_host/render_widget_host_view_android.h

Thanks, this looks great. Does the DelegatedFrameHostAndroid need to remain in content/, or can we move it to ui/ instead?
We should be able to move DFHAndroid to somewhere lower since what I described only depends on cc (it needs to get CompositorImpl's surface manager but we can expose that through ui::WindowAndroidCompositor or something else).

We'll have to see about readback logic though since that depends on GLHelper, which used to be in content but now has moved to a component (components/display_compositor).

There are multiple variations in which this could work. In the one extreme this 'submit compositor frame' and related logic could actually be part of cc::Surfaces (or a convenience helper right above since it's just what ties the incoming CompositorFrame to the SurfaceLayer in the host). That probably works for the little core logic Android has but looking at DelegatedFrameHost for desktop in comparison, it has more ui compositor specific things that would make it more awkward there (extra layers of indirection needed). (Not that DFHAndroid necessarily needs to be in the same place as the desktop version but it would be nice if we'd have the option to at least share the core logic in the same place/layer.)

In the other direction - other than being in content/browser/compositor - it could be a component right below (one that blimp and RWHVA can depend on, such as components/delegated_frame_host). It might eventually have to depend on components/display_compositor for GLHelper, which doesn't seem worse than content/browser/compositor depending on it the way it does today. Or even better, maybe GLHelper should just be its own component.
Owner: khushals...@chromium.org
Status: Assigned (was: Untriaged)
Assigning to Khushal for dealing with the right side of the diagram (DFHAndroid).
I'll look into ViewAndroid and having that own a layer etc.
Project Member

Comment 8 by bugdroid1@chromium.org, Jul 11 2016

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

commit e6343711183c73be584725b1b0b7f70a60dd2a5b
Author: sievers <sievers@chromium.org>
Date: Mon Jul 11 22:35:30 2016

Android: Extend ViewAndroid

This moves cc::Layer ownership from
RenderWidgetHostViewAndroid and ContentViewCore to
ViewAndroid.

Also, ContentViewCore changes from 'is a' ViewAndroid to
'has a' ViewAndroid.

RenderWidgetHostViewAndroid also gets its own ViewAndroid now.
For getting to the WindowAndroid or ViewAndroidDelegate it
will ask its parent.

BUG= 624666 , 598880 ,581521, 626765 
TBR=dtrainor@chromium.org
NOTRY=True

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

[modify] https://crrev.com/e6343711183c73be584725b1b0b7f70a60dd2a5b/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java
[modify] https://crrev.com/e6343711183c73be584725b1b0b7f70a60dd2a5b/chrome/browser/android/bottombar/overlay_panel_content.cc
[modify] https://crrev.com/e6343711183c73be584725b1b0b7f70a60dd2a5b/chrome/browser/android/compositor/layer/overlay_panel_layer.cc
[modify] https://crrev.com/e6343711183c73be584725b1b0b7f70a60dd2a5b/chrome/browser/android/tab_android.cc
[modify] https://crrev.com/e6343711183c73be584725b1b0b7f70a60dd2a5b/content/browser/android/content_view_core_impl.cc
[modify] https://crrev.com/e6343711183c73be584725b1b0b7f70a60dd2a5b/content/browser/android/content_view_core_impl.h
[modify] https://crrev.com/e6343711183c73be584725b1b0b7f70a60dd2a5b/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/e6343711183c73be584725b1b0b7f70a60dd2a5b/content/browser/renderer_host/render_widget_host_view_android.h
[modify] https://crrev.com/e6343711183c73be584725b1b0b7f70a60dd2a5b/content/browser/web_contents/web_contents_view_android.cc
[modify] https://crrev.com/e6343711183c73be584725b1b0b7f70a60dd2a5b/content/public/browser/android/content_view_core.h
[modify] https://crrev.com/e6343711183c73be584725b1b0b7f70a60dd2a5b/ui/android/BUILD.gn
[add] https://crrev.com/e6343711183c73be584725b1b0b7f70a60dd2a5b/ui/android/view_android.cc
[modify] https://crrev.com/e6343711183c73be584725b1b0b7f70a60dd2a5b/ui/android/view_android.h

Project Member

Comment 9 by bugdroid1@chromium.org, Aug 16 2016

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

commit a693aa733d2a91627e174d62988c7ef591d963f2
Author: khushalsagar <khushalsagar@google.com>
Date: Tue Aug 16 22:18:06 2016

content: Move Surfaces related code out of RWHVA.

Move logic for managing cc::Surfaces for CompositorFrames and building
Surface Layers to DelegatedFrameHostAndroid from
RenderWidgetHostViewAndroid. This patch moves the logic for SurfaceLayers
and for drawing the background when there is no content from the
renderer.

BUG= 624666 ,  618987 

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

[modify] https://crrev.com/a693aa733d2a91627e174d62988c7ef591d963f2/content/browser/android/content_view_core_impl.cc
[modify] https://crrev.com/a693aa733d2a91627e174d62988c7ef591d963f2/content/browser/android/content_view_core_impl.h
[modify] https://crrev.com/a693aa733d2a91627e174d62988c7ef591d963f2/content/browser/renderer_host/render_widget_host_unittest.cc
[modify] https://crrev.com/a693aa733d2a91627e174d62988c7ef591d963f2/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/a693aa733d2a91627e174d62988c7ef591d963f2/content/browser/renderer_host/render_widget_host_view_android.h
[modify] https://crrev.com/a693aa733d2a91627e174d62988c7ef591d963f2/ui/android/BUILD.gn
[modify] https://crrev.com/a693aa733d2a91627e174d62988c7ef591d963f2/ui/android/DEPS
[add] https://crrev.com/a693aa733d2a91627e174d62988c7ef591d963f2/ui/android/delegated_frame_host_android.cc
[add] https://crrev.com/a693aa733d2a91627e174d62988c7ef591d963f2/ui/android/delegated_frame_host_android.h

Blocking: -622848
I'll leave this bug open since we still have to deal with frame retention and readbacks. But it doesn't block Tab for 0.6 anymore.
Status: WontFix (was: Assigned)

Sign in to add a comment