Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2369643002:
Fix OOPIFs on Android (Closed)
|
Created:
4 years, 3 months ago by enne (OOO) Modified:
4 years, 2 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, lfg, nona+watch_chromium.org, shuchen+watch_chromium.org, James Su, yusukes+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix OOPIFs on Android
RenderWidgetHostViewAndroid / DelegatedFrameHostAndroid don't
participate correctly in unified begin frame. They listen to
vsync messages from the window android themselves and then
send begin frames to the renderer.
This patch hooks up RWHVA into the surface client namespace hierarchy in
surface manager by registering its client id and its association with
the CompositorImpl's client id. Because RenderWidgetHostViewChildFrame
attaches itself to the RWHVAndroid's client id, this allows child frames
to get the begin frame sources that they need to drive themselves.
In the future, RWHVAs will no longer be WindowAndroidObservers
and will instead get their begin frame source via the surface manager.
However, thorny issues around begin frame source pausing need to be
addressed first before this can be removed. So, this patch is only
a halfway step to make sure that begin frames flow from
CompositorImpl -> RWHVAndroid -> RWHVChildFrame even if
RWHVAndroid ignores it and uses its own.
OOPIFs were broken when begin frame scheduling was turned on in
https://codereview.chromium.org/1939253002. Because the child frames
weren't hooked up to the surface hierarchy, they never got a begin frame
source, never ticked, and thus never rendered at all.
BUG=621835
Committed: https://crrev.com/a487a27cc88695723d102e43592b14a5958ad4e3
Cr-Commit-Position: refs/heads/master@{#422193}
Patch Set 1 #
Total comments: 2
Patch Set 2 : sievers review #Patch Set 3 : Always unregister hierarchy #Patch Set 4 : Rebase #Patch Set 5 : Add conditional for WebView shutdown #
Messages
Total messages: 36 (22 generated)
enne@chromium.org changed reviewers: + sievers@chromium.org, tedchoc@chromium.org
https://codereview.chromium.org/2369643002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2369643002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_android.cc:1308: content_view_core_->GetWindowAndroid()->GetCompositor(); Can I assume that there's always a compositor at this point? It's a little weird to me that RenderWidgetHostViewAndroid never gets OnAttachCompositor messages, but maybe it starts observing after the compositor is attached to the window.
Description was changed from ========== Fix OOPIFs on Android RenderWidgetHostViewAndroid / DelegatedFrameHostAndroid don't participate correctly in unified begin frame. They listen to vsync messages from the window android themselves and then send begin frames to the renderer. This patch hooks up RWHVA into the surface client namespace hierarchy in surface manager by registering its client id and its association with the CompositorImpl's client id. Because RenderWidgetHostViewChildFrame attaches itself to the RWHVAndroid's client id, this allows child frames to get the begin frame sources that they need to drive themselves. In the future, they will no longer be WindowAndroidObservers and will instead get their begin frame source via the surface manager. However, thorny issues around begin frame source pausing need to be addressed first before this can be removed. So, this patch is only a halfway step. OOPIFs were broken when begin frame scheduling was turned on in https://codereview.chromium.org/1939253002. Because the child frames weren't hooked up to the surface hierarchy, they never got a begin frame source, never ticked, and thus never rendered at all. BUG=621835 ========== to ========== Fix OOPIFs on Android RenderWidgetHostViewAndroid / DelegatedFrameHostAndroid don't participate correctly in unified begin frame. They listen to vsync messages from the window android themselves and then send begin frames to the renderer. This patch hooks up RWHVA into the surface client namespace hierarchy in surface manager by registering its client id and its association with the CompositorImpl's client id. Because RenderWidgetHostViewChildFrame attaches itself to the RWHVAndroid's client id, this allows child frames to get the begin frame sources that they need to drive themselves. In the future, RWHVAs will no longer be WindowAndroidObservers and will instead get their begin frame source via the surface manager. However, thorny issues around begin frame source pausing need to be addressed first before this can be removed. So, this patch is only a halfway step. OOPIFs were broken when begin frame scheduling was turned on in https://codereview.chromium.org/1939253002. Because the child frames weren't hooked up to the surface hierarchy, they never got a begin frame source, never ticked, and thus never rendered at all. BUG=621835 ==========
Description was changed from ========== Fix OOPIFs on Android RenderWidgetHostViewAndroid / DelegatedFrameHostAndroid don't participate correctly in unified begin frame. They listen to vsync messages from the window android themselves and then send begin frames to the renderer. This patch hooks up RWHVA into the surface client namespace hierarchy in surface manager by registering its client id and its association with the CompositorImpl's client id. Because RenderWidgetHostViewChildFrame attaches itself to the RWHVAndroid's client id, this allows child frames to get the begin frame sources that they need to drive themselves. In the future, RWHVAs will no longer be WindowAndroidObservers and will instead get their begin frame source via the surface manager. However, thorny issues around begin frame source pausing need to be addressed first before this can be removed. So, this patch is only a halfway step. OOPIFs were broken when begin frame scheduling was turned on in https://codereview.chromium.org/1939253002. Because the child frames weren't hooked up to the surface hierarchy, they never got a begin frame source, never ticked, and thus never rendered at all. BUG=621835 ========== to ========== Fix OOPIFs on Android RenderWidgetHostViewAndroid / DelegatedFrameHostAndroid don't participate correctly in unified begin frame. They listen to vsync messages from the window android themselves and then send begin frames to the renderer. This patch hooks up RWHVA into the surface client namespace hierarchy in surface manager by registering its client id and its association with the CompositorImpl's client id. Because RenderWidgetHostViewChildFrame attaches itself to the RWHVAndroid's client id, this allows child frames to get the begin frame sources that they need to drive themselves. In the future, RWHVAs will no longer be WindowAndroidObservers and will instead get their begin frame source via the surface manager. However, thorny issues around begin frame source pausing need to be addressed first before this can be removed. So, this patch is only a halfway step to make sure that begin frames flow from CompositorImpl -> RWHVAndroid -> RWHVChildFrame even if RWHVAndroid uses its own. OOPIFs were broken when begin frame scheduling was turned on in https://codereview.chromium.org/1939253002. Because the child frames weren't hooked up to the surface hierarchy, they never got a begin frame source, never ticked, and thus never rendered at all. BUG=621835 ==========
Description was changed from ========== Fix OOPIFs on Android RenderWidgetHostViewAndroid / DelegatedFrameHostAndroid don't participate correctly in unified begin frame. They listen to vsync messages from the window android themselves and then send begin frames to the renderer. This patch hooks up RWHVA into the surface client namespace hierarchy in surface manager by registering its client id and its association with the CompositorImpl's client id. Because RenderWidgetHostViewChildFrame attaches itself to the RWHVAndroid's client id, this allows child frames to get the begin frame sources that they need to drive themselves. In the future, RWHVAs will no longer be WindowAndroidObservers and will instead get their begin frame source via the surface manager. However, thorny issues around begin frame source pausing need to be addressed first before this can be removed. So, this patch is only a halfway step to make sure that begin frames flow from CompositorImpl -> RWHVAndroid -> RWHVChildFrame even if RWHVAndroid uses its own. OOPIFs were broken when begin frame scheduling was turned on in https://codereview.chromium.org/1939253002. Because the child frames weren't hooked up to the surface hierarchy, they never got a begin frame source, never ticked, and thus never rendered at all. BUG=621835 ========== to ========== Fix OOPIFs on Android RenderWidgetHostViewAndroid / DelegatedFrameHostAndroid don't participate correctly in unified begin frame. They listen to vsync messages from the window android themselves and then send begin frames to the renderer. This patch hooks up RWHVA into the surface client namespace hierarchy in surface manager by registering its client id and its association with the CompositorImpl's client id. Because RenderWidgetHostViewChildFrame attaches itself to the RWHVAndroid's client id, this allows child frames to get the begin frame sources that they need to drive themselves. In the future, RWHVAs will no longer be WindowAndroidObservers and will instead get their begin frame source via the surface manager. However, thorny issues around begin frame source pausing need to be addressed first before this can be removed. So, this patch is only a halfway step to make sure that begin frames flow from CompositorImpl -> RWHVAndroid -> RWHVChildFrame even if RWHVAndroid ignores it and uses its own. OOPIFs were broken when begin frame scheduling was turned on in https://codereview.chromium.org/1939253002. Because the child frames weren't hooked up to the surface hierarchy, they never got a begin frame source, never ticked, and thus never rendered at all. BUG=621835 ==========
https://codereview.chromium.org/2369643002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2369643002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_android.cc:1308: content_view_core_->GetWindowAndroid()->GetCompositor(); On 2016/09/23 22:58:09, enne wrote: > Can I assume that there's always a compositor at this point? It's a little weird > to me that RenderWidgetHostViewAndroid never gets OnAttachCompositor messages, > but maybe it starts observing after the compositor is attached to the window. I don't trust the logic here with 'observing' but maybe you can sidestep it since you only care about making sure that you catch all cases of attaching to a different browser compositor. So instead of here, I'd register the namespace in the RWHVA constructor (null-check for compositor because of webview). Then I'd simply unregister the namespace in OnDetachCompositor() and register it in OnAttachCompositor(). It's possible that it will cause some redundant calls. I assume that's fine for unregister. But not sure about registering twice. Maybe you need to track the current compositor SurfaceClientId here?
On 2016/09/23 at 23:16:09, sievers wrote: > I don't trust the logic here with 'observing' but maybe you can sidestep it since you only care about making sure that you catch all cases of attaching to a different browser compositor. What can I do to test this? I was unable to get OnAttachCompositor working properly. Here's a patch though that does what you ask. I made DelegatedFrameHostAndroid be robust to multiple calls, as SurfaceManager is not.
Description was changed from ========== Fix OOPIFs on Android RenderWidgetHostViewAndroid / DelegatedFrameHostAndroid don't participate correctly in unified begin frame. They listen to vsync messages from the window android themselves and then send begin frames to the renderer. This patch hooks up RWHVA into the surface client namespace hierarchy in surface manager by registering its client id and its association with the CompositorImpl's client id. Because RenderWidgetHostViewChildFrame attaches itself to the RWHVAndroid's client id, this allows child frames to get the begin frame sources that they need to drive themselves. In the future, RWHVAs will no longer be WindowAndroidObservers and will instead get their begin frame source via the surface manager. However, thorny issues around begin frame source pausing need to be addressed first before this can be removed. So, this patch is only a halfway step to make sure that begin frames flow from CompositorImpl -> RWHVAndroid -> RWHVChildFrame even if RWHVAndroid ignores it and uses its own. OOPIFs were broken when begin frame scheduling was turned on in https://codereview.chromium.org/1939253002. Because the child frames weren't hooked up to the surface hierarchy, they never got a begin frame source, never ticked, and thus never rendered at all. BUG=621835 ========== to ========== Fix OOPIFs on Android RenderWidgetHostViewAndroid / DelegatedFrameHostAndroid don't participate correctly in unified begin frame. They listen to vsync messages from the window android themselves and then send begin frames to the renderer. This patch hooks up RWHVA into the surface client namespace hierarchy in surface manager by registering its client id and its association with the CompositorImpl's client id. Because RenderWidgetHostViewChildFrame attaches itself to the RWHVAndroid's client id, this allows child frames to get the begin frame sources that they need to drive themselves. In the future, RWHVAs will no longer be WindowAndroidObservers and will instead get their begin frame source via the surface manager. However, thorny issues around begin frame source pausing need to be addressed first before this can be removed. So, this patch is only a halfway step to make sure that begin frames flow from CompositorImpl -> RWHVAndroid -> RWHVChildFrame even if RWHVAndroid ignores it and uses its own. OOPIFs were broken when begin frame scheduling was turned on in https://codereview.chromium.org/1939253002. Because the child frames weren't hooked up to the surface hierarchy, they never got a begin frame source, never ticked, and thus never rendered at all. BUG=621835 ==========
sievers@chromium.org changed reviewers: + yusufo@chromium.org
On 2016/09/24 00:44:14, enne wrote: > On 2016/09/23 at 23:16:09, sievers wrote: > > > I don't trust the logic here with 'observing' but maybe you can sidestep it > since you only care about making sure that you catch all cases of attaching to a > different browser compositor. > > What can I do to test this? I was unable to get OnAttachCompositor working > properly. > > Here's a patch though that does what you ask. I made DelegatedFrameHostAndroid > be robust to multiple calls, as SurfaceManager is not. lgtm. +yusuf for how to test browser compositor reparenting. I'm guessing you need an activity with a custom tab that you then add to Chrome's tab stack.
On 2016/09/26 19:43:19, sievers wrote: > On 2016/09/24 00:44:14, enne wrote: > > On 2016/09/23 at 23:16:09, sievers wrote: > > > > > I don't trust the logic here with 'observing' but maybe you can sidestep it > > since you only care about making sure that you catch all cases of attaching to > a > > different browser compositor. > > > > What can I do to test this? I was unable to get OnAttachCompositor working > > properly. > > > > Here's a patch though that does what you ask. I made > DelegatedFrameHostAndroid > > be robust to multiple calls, as SurfaceManager is not. > > lgtm. > > +yusuf for how to test browser compositor reparenting. > > I'm guessing you need an activity with a custom tab that you then add to > Chrome's tab stack. lgtm (ui owners rs), and indeed that is how you would go about testing reparenting. And easy way to get CCTs: 1.) Build custom_tabs_client_example_apk 2.) Install CustomTabsClientExample.apk 3.) Launch Chrome Custom Tabs Example (looks like a Chameleon) 4.) Change package dropdown to org.chromium.chrome (assuming you built ChromePublic.apk) 5.) "Connect to service" -> "Launch URL in a Chrome Custom Tab" 6.) (Now in a CCT), click three dot menu "Open in Chrome" That will move a tab and renderer from one activity to another.
Ok. I see OnAttachCompositor and OnAttachedToWindow calls happening in RenderWidgetHostViewAndroid. I don't see any OnDetachCompositor or OnDetachedFromWindow calls. Is that expected?
Ok, nevermind. I see some now. I think this looks like it works just fine there. OOPIFs appear, nothing crashes or dchecks, and everything registers and deregisters as it should.
enne@chromium.org changed reviewers: + sadrul@chromium.org
enne@chromium.org changed reviewers: + aelias@chromium.org - sadrul@chromium.org
+aelias for content/browser/renderer_host OWNERS
The CQ bit was checked by enne@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by enne@chromium.org
The CQ bit was checked by enne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2369643002/#ps60001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by enne@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by enne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, aelias@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2369643002/#ps80001 (title: "Add conditional for WebView shutdown")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix OOPIFs on Android RenderWidgetHostViewAndroid / DelegatedFrameHostAndroid don't participate correctly in unified begin frame. They listen to vsync messages from the window android themselves and then send begin frames to the renderer. This patch hooks up RWHVA into the surface client namespace hierarchy in surface manager by registering its client id and its association with the CompositorImpl's client id. Because RenderWidgetHostViewChildFrame attaches itself to the RWHVAndroid's client id, this allows child frames to get the begin frame sources that they need to drive themselves. In the future, RWHVAs will no longer be WindowAndroidObservers and will instead get their begin frame source via the surface manager. However, thorny issues around begin frame source pausing need to be addressed first before this can be removed. So, this patch is only a halfway step to make sure that begin frames flow from CompositorImpl -> RWHVAndroid -> RWHVChildFrame even if RWHVAndroid ignores it and uses its own. OOPIFs were broken when begin frame scheduling was turned on in https://codereview.chromium.org/1939253002. Because the child frames weren't hooked up to the surface hierarchy, they never got a begin frame source, never ticked, and thus never rendered at all. BUG=621835 ========== to ========== Fix OOPIFs on Android RenderWidgetHostViewAndroid / DelegatedFrameHostAndroid don't participate correctly in unified begin frame. They listen to vsync messages from the window android themselves and then send begin frames to the renderer. This patch hooks up RWHVA into the surface client namespace hierarchy in surface manager by registering its client id and its association with the CompositorImpl's client id. Because RenderWidgetHostViewChildFrame attaches itself to the RWHVAndroid's client id, this allows child frames to get the begin frame sources that they need to drive themselves. In the future, RWHVAs will no longer be WindowAndroidObservers and will instead get their begin frame source via the surface manager. However, thorny issues around begin frame source pausing need to be addressed first before this can be removed. So, this patch is only a halfway step to make sure that begin frames flow from CompositorImpl -> RWHVAndroid -> RWHVChildFrame even if RWHVAndroid ignores it and uses its own. OOPIFs were broken when begin frame scheduling was turned on in https://codereview.chromium.org/1939253002. Because the child frames weren't hooked up to the surface hierarchy, they never got a begin frame source, never ticked, and thus never rendered at all. BUG=621835 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix OOPIFs on Android RenderWidgetHostViewAndroid / DelegatedFrameHostAndroid don't participate correctly in unified begin frame. They listen to vsync messages from the window android themselves and then send begin frames to the renderer. This patch hooks up RWHVA into the surface client namespace hierarchy in surface manager by registering its client id and its association with the CompositorImpl's client id. Because RenderWidgetHostViewChildFrame attaches itself to the RWHVAndroid's client id, this allows child frames to get the begin frame sources that they need to drive themselves. In the future, RWHVAs will no longer be WindowAndroidObservers and will instead get their begin frame source via the surface manager. However, thorny issues around begin frame source pausing need to be addressed first before this can be removed. So, this patch is only a halfway step to make sure that begin frames flow from CompositorImpl -> RWHVAndroid -> RWHVChildFrame even if RWHVAndroid ignores it and uses its own. OOPIFs were broken when begin frame scheduling was turned on in https://codereview.chromium.org/1939253002. Because the child frames weren't hooked up to the surface hierarchy, they never got a begin frame source, never ticked, and thus never rendered at all. BUG=621835 ========== to ========== Fix OOPIFs on Android RenderWidgetHostViewAndroid / DelegatedFrameHostAndroid don't participate correctly in unified begin frame. They listen to vsync messages from the window android themselves and then send begin frames to the renderer. This patch hooks up RWHVA into the surface client namespace hierarchy in surface manager by registering its client id and its association with the CompositorImpl's client id. Because RenderWidgetHostViewChildFrame attaches itself to the RWHVAndroid's client id, this allows child frames to get the begin frame sources that they need to drive themselves. In the future, RWHVAs will no longer be WindowAndroidObservers and will instead get their begin frame source via the surface manager. However, thorny issues around begin frame source pausing need to be addressed first before this can be removed. So, this patch is only a halfway step to make sure that begin frames flow from CompositorImpl -> RWHVAndroid -> RWHVChildFrame even if RWHVAndroid ignores it and uses its own. OOPIFs were broken when begin frame scheduling was turned on in https://codereview.chromium.org/1939253002. Because the child frames weren't hooked up to the surface hierarchy, they never got a begin frame source, never ticked, and thus never rendered at all. BUG=621835 Committed: https://crrev.com/a487a27cc88695723d102e43592b14a5958ad4e3 Cr-Commit-Position: refs/heads/master@{#422193} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/a487a27cc88695723d102e43592b14a5958ad4e3 Cr-Commit-Position: refs/heads/master@{#422193} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
