Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2133873004:
content: Move Surfaces related code out of RWHVA. (Closed)
|
Created:
4 years, 5 months ago by khushalsagar Modified:
4 years, 4 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, sievers+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, James Su Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncontent: 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
Committed: https://crrev.com/a693aa733d2a91627e174d62988c7ef591d963f2
Cr-Commit-Position: refs/heads/master@{#412356}
Patch Set 1 #
Total comments: 1
Patch Set 2 : .. #
Total comments: 1
Patch Set 3 : .. #
Total comments: 13
Patch Set 4 : Rebase. #Patch Set 5 : Addressed comments. #
Total comments: 23
Patch Set 6 : Rebase. #Patch Set 7 : Addressed comments. #Patch Set 8 : null check DFH webview #Patch Set 9 : remove GetContentLayer #
Total comments: 3
Patch Set 10 : moar null checks #
Total comments: 6
Patch Set 11 : Fix test. #
Total comments: 1
Patch Set 12 : missed overscroll change #Messages
Total messages: 59 (30 generated)
khushalsagar@chromium.org changed reviewers: + khushalsagar@chromium.org, sievers@chromium.org
Only moved out Surfaces related stuff for now. Does this look close to what you had in mind?
On 2016/07/08 23:47:36, Khushal wrote: > Only moved out Surfaces related stuff for now. Does this look close to what you > had in mind? ping. :)
https://codereview.chromium.org/2133873004/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2133873004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_android.cc:1120: void RenderWidgetHostViewAndroid::AttachSurfaceLayer( I was looking at removing the Attach and Detach SurfaceLayer logic here. Looks like it is used to add/remove layers to the cc::Layer for the ViewAndroid owned by the ContentViewCoreImpl as well, which uses it to decide whether its cc::Layer should draw if we have no delegated layers. I'm guessing we would have the RWHVA add its view as a child to a container ViewAndroid, owned by the ContentViewCoreImpl. If the DelegatedFrameHostAndroid has a reference to the ViewAndroid and updates its cc::Layer directly, we still need to notify the container view, for handling the case above. Any suggestions for structuring that? Do you want to add a ViewAndroidOwner interface, that gets notified when any descendant views in a view's hierarchy do/don't have a cc::Layer.
Description was changed from ========== 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 only moves the logic for SurfaceLayers and ties the lifetime of DelegatedFrameHostAndroid to the renderer's OutputSurface. BUG=624666 ========== to ========== 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 ==========
khushalsagar@chromium.org changed reviewers: + dtrainor@chromium.org
I've moved the logic background drawing logic to DelegatedFrameHost, like we discussed. PTAL. +dtrainor if he has any comments too.
khushalsagar@chromium.org changed reviewers: - khushalsagar@chromium.org
https://codereview.chromium.org/2133873004/diff/20001/content/browser/rendere... File content/browser/renderer_host/delegated_frame_host_android.cc (right): https://codereview.chromium.org/2133873004/diff/20001/content/browser/rendere... content/browser/renderer_host/delegated_frame_host_android.cc:52: surface_manager_ = CompositorImpl::GetSurfaceManager(); Was talking to David about this. Should we move these CompositorImpl dependencies to WindowAndroidCompositor in this patch so DelegatedFrameHostAndroid doesn't depend on content? The aim is to finally move this class out so it can be shared with blimp anyway.
khushalsagar@chromium.org changed reviewers: + khushalsagar@chromium.org
friendly pingy! :) Also, if this looks good, should we just move DelegatedFrameHostAndroid to ui/android/ in this patch itself? We can find a more suitable place when the readback logic and frame retention is added to this class.
khushalsagar@chromium.org changed reviewers: + vmpstr@chromium.org - khushalsagar@chromium.org
+vmpstr for DEPS stamp!
+vmpstr for DEPS stamp!
Description was changed from ========== 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 ========== to ========== 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 ==========
Have some comments, but some might be on code you just moved, so they might not relate. https://codereview.chromium.org/2133873004/diff/40001/ui/android/delegated_fr... File ui/android/delegated_frame_host_android.cc (right): https://codereview.chromium.org/2133873004/diff/40001/ui/android/delegated_fr... ui/android/delegated_frame_host_android.cc:64: DelegatedFrameHostAndroid::~DelegatedFrameHostAndroid() { We should make sure to remove all layers we build here like the background_layer_ right? https://codereview.chromium.org/2133873004/diff/40001/ui/android/delegated_fr... ui/android/delegated_frame_host_android.cc:88: DCHECK(!content_layer_); might as well DCHECK(surface_id_.is_null()) also if we're checking content_layer_? https://codereview.chromium.org/2133873004/diff/40001/ui/android/delegated_fr... ui/android/delegated_frame_host_android.cc:90: surface_id_ = surface_id_allocator_->GenerateId(); Not necessarily for this CL, but it would be nice if this was just held internally by the SurfaceLayer and was destroyed/allocated with it. It would be nice if we had a SurfaceLayer that automatically Created/Destroyed the surface with a surface factory and potentially allocated it's own id as well. It looks like the lifecycle of the two is pretty intertwined. https://codereview.chromium.org/2133873004/diff/40001/ui/android/delegated_fr... ui/android/delegated_frame_host_android.cc:97: UpdateContentLayer(std::move(CreateSurfaceLayer())); Do we need to pass in the layer or can we just create it internally? Should these just be the following? CreateContentLayer() DestroyContentLayer() https://codereview.chromium.org/2133873004/diff/40001/ui/android/delegated_fr... ui/android/delegated_frame_host_android.cc:165: surface_layer->SetSurfaceId(surface_id_, 1.f, current_surface_size_); Is there a reason we don't set the new surface id and size on the existing surface layer and build a new one instead? Easier/nicer? https://codereview.chromium.org/2133873004/diff/40001/ui/android/ui_android.gyp File ui/android/ui_android.gyp (right): https://codereview.chromium.org/2133873004/diff/40001/ui/android/ui_android.g... ui/android/ui_android.gyp:17: '../../cc/cc.gyp:cc_surfaces', Don't need gyp changes IIRC
As commented here https://bugs.chromium.org/p/chromium/issues/detail?id=597481#c12, now that ViewAndroid.GetLayer's ancestor(CompositorView) has one SolidColorLayer, why bother adding one here?
khushalsagar@chromium.org changed reviewers: + jbauman@chromium.org
https://codereview.chromium.org/2133873004/diff/40001/ui/android/delegated_fr... File ui/android/delegated_frame_host_android.cc (right): https://codereview.chromium.org/2133873004/diff/40001/ui/android/delegated_fr... ui/android/delegated_frame_host_android.cc:64: DelegatedFrameHostAndroid::~DelegatedFrameHostAndroid() { On 2016/08/04 16:05:46, David Trainor wrote: > We should make sure to remove all layers we build here like the > background_layer_ right? Yup. Done. https://codereview.chromium.org/2133873004/diff/40001/ui/android/delegated_fr... ui/android/delegated_frame_host_android.cc:88: DCHECK(!content_layer_); On 2016/08/04 16:05:46, David Trainor wrote: > might as well DCHECK(surface_id_.is_null()) also if we're checking > content_layer_? Done. https://codereview.chromium.org/2133873004/diff/40001/ui/android/delegated_fr... ui/android/delegated_frame_host_android.cc:90: surface_id_ = surface_id_allocator_->GenerateId(); On 2016/08/04 16:05:46, David Trainor wrote: > Not necessarily for this CL, but it would be nice if this was just held > internally by the SurfaceLayer and was destroyed/allocated with it. It would be > nice if we had a SurfaceLayer that automatically Created/Destroyed the surface > with a surface factory and potentially allocated it's own id as well. It looks > like the lifecycle of the two is pretty intertwined. Looked around the code a little bit, and looks like the lifetime of the 2 doesn't need to be as intertwined. In RenderWidgetHostViewChildFrame, we create the surface id to give to the renderer that's embedding the frame, and the renderer creates the SurfaceLayer. https://codereview.chromium.org/2133873004/diff/40001/ui/android/delegated_fr... ui/android/delegated_frame_host_android.cc:97: UpdateContentLayer(std::move(CreateSurfaceLayer())); On 2016/08/04 16:05:46, David Trainor wrote: > Do we need to pass in the layer or can we just create it internally? Should > these just be the following? > > CreateContentLayer() > DestroyContentLayer() Good idea. Collapsed it into CreateContentLayer and just moved the layer destruction to DestroyDelegatedContent. https://codereview.chromium.org/2133873004/diff/40001/ui/android/delegated_fr... ui/android/delegated_frame_host_android.cc:165: surface_layer->SetSurfaceId(surface_id_, 1.f, current_surface_size_); On 2016/08/04 16:05:46, David Trainor wrote: > Is there a reason we don't set the new surface id and size on the existing > surface layer and build a new one instead? Easier/nicer? Good question. Doesn't look like the Surface Layer needs to be tied to a single Surface id for its lifetime? I'm not sure why we had it here this way. +jbauman since he is the surfaces expert. https://codereview.chromium.org/2133873004/diff/40001/ui/android/ui_android.gyp File ui/android/ui_android.gyp (right): https://codereview.chromium.org/2133873004/diff/40001/ui/android/ui_android.g... ui/android/ui_android.gyp:17: '../../cc/cc.gyp:cc_surfaces', On 2016/08/04 16:05:46, David Trainor wrote: > Don't need gyp changes IIRC YES!!
On 2016/08/09 01:35:05, xing.xu wrote: > As commented here > https://bugs.chromium.org/p/chromium/issues/detail?id=597481#c12, now that > ViewAndroid.GetLayer's ancestor(CompositorView) has one SolidColorLayer, why > bother adding one here? ContentViewCore also has a solid color layer for drawing the background when we have no content. The aim of this change is also to consolidate all the gutter logic for dealing with the Layer for the renderer frame, both when its not available and when we are waiting for a resized frame from the renderer in the DelegatedFrameHostAndroid, similar to the DelegatedFrameHost in aura. We set the CompositorView's root layer to draw in StaticTabSceneLayer::ShouldDrawBackground. I'm not even sure if that's the one handling this case right now. If I hard-code it to false and block orientation change events from reaching the renderer, I still see the fill happening. +dtrainor, do you know what handles this case in the UI right now?
khushalsagar@chromium.org changed reviewers: - jbauman@chromium.org
friendly ping! :) https://codereview.chromium.org/2133873004/diff/40001/ui/android/delegated_fr... File ui/android/delegated_frame_host_android.cc (right): https://codereview.chromium.org/2133873004/diff/40001/ui/android/delegated_fr... ui/android/delegated_frame_host_android.cc:165: surface_layer->SetSurfaceId(surface_id_, 1.f, current_surface_size_); On 2016/08/09 20:54:06, Khushal wrote: > On 2016/08/04 16:05:46, David Trainor wrote: > > Is there a reason we don't set the new surface id and size on the existing > > surface layer and build a new one instead? Easier/nicer? > > Good question. Doesn't look like the Surface Layer needs to be tied to a single > Surface id for its lifetime? I'm not sure why we had it here this way. +jbauman > since he is the surfaces expert. So we can keep a single content layer around and just attach/detach it from the ViewAndroid when the delegated content is destroyed. GetContentLayer() can return null if we are in this state. Either way is good, whatever looks better.
https://codereview.chromium.org/2133873004/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2133873004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:385: delegated_frame_host_->UpdateSize(GetViewSize()); I *think* you are passing DIP here but when you compare the size to the SurfaceLayer in DFH, it'd be pixels for that SurfaceLayer's bounds. https://codereview.chromium.org/2133873004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:408: gfx::Size bounds = delegated_frame_host_->GetContentLayer()->bounds(); I think this should be the same as |texture_size_in_layer_|. https://codereview.chromium.org/2133873004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:428: if (!delegated_frame_host_->GetContentLayer()) I think you can just remove this check. It should be sufficiently covered through the frame_evictor_ and |texture_size_in_layer_| checks below. https://codereview.chromium.org/2133873004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:526: delegated_frame_host_->GetContentLayer()->SetHideLayerAndSubtree(true); Make this DFH::SetVisible() https://codereview.chromium.org/2133873004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:842: DCHECK(delegated_frame_host_->GetContentLayer()); You can move this DCHECK() into DFH. https://codereview.chromium.org/2133873004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1023: delegated_frame_host_->GetContentLayer()->SetContentsOpaque(!enabled); Make this DFH::SetContentsOpaque(). https://codereview.chromium.org/2133873004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1179: gfx::Size RenderWidgetHostViewAndroid::GetViewSize() const { nit: Can you not add this method, since RWHV already has so many confusing bounds methods? I'd just call GetViewBounds().size() if you need this value. https://codereview.chromium.org/2133873004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1549: if (!content_view_core_ || !delegated_frame_host_->GetContentLayer() || I don't understand the layer check here. I'd just drop it... and now you can remove DFH::GetContentLayer() and nobody here needs to touch that layer anymore :) https://codereview.chromium.org/2133873004/diff/80001/ui/android/delegated_fr... File ui/android/delegated_frame_host_android.cc (right): https://codereview.chromium.org/2133873004/diff/80001/ui/android/delegated_fr... ui/android/delegated_frame_host_android.cc:82: gfx::Size texture_size_in_layer = root_pass->output_rect.size(); nit: This name is a bit of an artifact. You can probably call it |surface_size|. https://codereview.chromium.org/2133873004/diff/80001/ui/android/delegated_fr... ui/android/delegated_frame_host_android.cc:186: content_layer_->bounds() != desired_content_size_; See other comment about dip vs. pix. Also, this would probably make the layer drawable while the omnibox is animating. Maybe we just need to compare the |actual drawing surface size + omnibox height (=content translation| to the total container view size in pix without adjust for the translation offset? Also not sure about when the keyboard is up and how that actually works. https://codereview.chromium.org/2133873004/diff/80001/ui/android/delegated_fr... File ui/android/delegated_frame_host_android.h (right): https://codereview.chromium.org/2133873004/diff/80001/ui/android/delegated_fr... ui/android/delegated_frame_host_android.h:58: void UpdateSize(const gfx::Size& desired_content_size); nit: can you call this 'UpdateContainerSize()'?
The CQ bit was checked by khushalsagar@chromium.org to run a CQ dry run
khushalsagar@chromium.org changed reviewers: - khushalsagar@chromium.org
https://codereview.chromium.org/2133873004/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2133873004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:385: delegated_frame_host_->UpdateSize(GetViewSize()); On 2016/08/12 19:33:04, sievers wrote: > I *think* you are passing DIP here but when you compare the size to the > SurfaceLayer in DFH, it'd be pixels for that SurfaceLayer's bounds. You're right. Fixed that. https://codereview.chromium.org/2133873004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:408: gfx::Size bounds = delegated_frame_host_->GetContentLayer()->bounds(); On 2016/08/12 19:33:04, sievers wrote: > I think this should be the same as |texture_size_in_layer_|. Done. https://codereview.chromium.org/2133873004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:428: if (!delegated_frame_host_->GetContentLayer()) On 2016/08/12 19:33:03, sievers wrote: > I think you can just remove this check. It should be sufficiently covered > through the frame_evictor_ and |texture_size_in_layer_| checks below. Should there be a DCHECK at the end then? The DelegatedFrameEvictor and host should be updated together. https://codereview.chromium.org/2133873004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:526: delegated_frame_host_->GetContentLayer()->SetHideLayerAndSubtree(true); On 2016/08/12 19:33:04, sievers wrote: > Make this DFH::SetVisible() Had it update the view's layer directly. https://codereview.chromium.org/2133873004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:842: DCHECK(delegated_frame_host_->GetContentLayer()); On 2016/08/12 19:33:04, sievers wrote: > You can move this DCHECK() into DFH. Done. https://codereview.chromium.org/2133873004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1023: delegated_frame_host_->GetContentLayer()->SetContentsOpaque(!enabled); On 2016/08/12 19:33:04, sievers wrote: > Make this DFH::SetContentsOpaque(). Done. https://codereview.chromium.org/2133873004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1179: gfx::Size RenderWidgetHostViewAndroid::GetViewSize() const { On 2016/08/12 19:33:03, sievers wrote: > nit: Can you not add this method, since RWHV already has so many confusing > bounds methods? I'd just call GetViewBounds().size() if you need this value. Done. https://codereview.chromium.org/2133873004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1229: if (delegated_frame_host_->GetContentLayer() && locks_on_frame_count_ == 0) Btw, why do we have this lock check here? https://codereview.chromium.org/2133873004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1549: if (!content_view_core_ || !delegated_frame_host_->GetContentLayer() || On 2016/08/12 19:33:03, sievers wrote: > I don't understand the layer check here. I'd just drop it... > > and now you can remove DFH::GetContentLayer() and nobody here needs to touch > that layer anymore :) I added a check for DFH::HasDelegatedContent instead, since I think that's what this was doing earlier. Better? https://codereview.chromium.org/2133873004/diff/80001/ui/android/delegated_fr... File ui/android/delegated_frame_host_android.cc (right): https://codereview.chromium.org/2133873004/diff/80001/ui/android/delegated_fr... ui/android/delegated_frame_host_android.cc:82: gfx::Size texture_size_in_layer = root_pass->output_rect.size(); On 2016/08/12 19:33:04, sievers wrote: > nit: This name is a bit of an artifact. You can probably call it |surface_size|. Done. https://codereview.chromium.org/2133873004/diff/80001/ui/android/delegated_fr... ui/android/delegated_frame_host_android.cc:186: content_layer_->bounds() != desired_content_size_; On 2016/08/12 19:33:04, sievers wrote: > See other comment about dip vs. pix. Thanks for pointing that out. > > Also, this would probably make the layer drawable while the omnibox is > animating. Maybe we just need to compare the |actual drawing surface size + > omnibox height (=content translation| to the total container view size in pix > without adjust for the translation offset? Done. Took care of the offset for the top controls. > > Also not sure about when the keyboard is up and how that actually works. The keyboard resizes the android view right now, so that should work correctly. I think someone was planning to make a change to fix that, and have the keyboard overlay on the content area. https://codereview.chromium.org/2133873004/diff/80001/ui/android/delegated_fr... File ui/android/delegated_frame_host_android.h (right): https://codereview.chromium.org/2133873004/diff/80001/ui/android/delegated_fr... ui/android/delegated_frame_host_android.h:58: void UpdateSize(const gfx::Size& desired_content_size); On 2016/08/12 19:33:04, sievers wrote: > nit: can you call this 'UpdateContainerSize()'? Done.
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: 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 khushalsagar@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 checked by khushalsagar@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...
khushalsagar@chromium.org changed reviewers: - khushalsagar@chromium.org
https://codereview.chromium.org/2133873004/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2133873004/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1216: if (locks_on_frame_count_ == 0) Btw, why is there a locks check here?
The CQ bit was checked by khushalsagar@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: 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...)
https://codereview.chromium.org/2133873004/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2133873004/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:330: DCHECK(!delegated_frame_host_); Looks like this is failing because RenderWidgetHostTest.Background does not call Destroy before deleting the view. I had put the DFH destruction there and DCHECKed here since it looked like the contract is to always call Destroy on the view before deleting it, and that's where we doing all the clean up earlier. Is that true?
LGTM w/1 comment https://codereview.chromium.org/2133873004/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2133873004/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1541: if (delegated_frame_host_ && delegated_frame_host_->HasDelegatedContent()) You meant the opposite right? I'm actually not sure why we need this check. I'd think that even if we show background color you'd want the overscroll effect. Maybe just remove it?
Thanks Daniel. I had a couple of doubts still. https://codereview.chromium.org/2133873004/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2133873004/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1216: if (locks_on_frame_count_ == 0) On 2016/08/16 06:22:42, Khushal wrote: > Btw, why is there a locks check here? Pingy! https://codereview.chromium.org/2133873004/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2133873004/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:330: DCHECK(!delegated_frame_host_); On 2016/08/16 17:36:13, Khushal wrote: > Looks like this is failing because RenderWidgetHostTest.Background does not call > Destroy before deleting the view. I had put the DFH destruction there and > DCHECKed here since it looked like the contract is to always call Destroy on the > view before deleting it, and that's where we doing all the clean up earlier. Is > that true? Pingy!
lgtm but wait for sievers
rs lgtm
https://codereview.chromium.org/2133873004/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_unittest.cc (left): https://codereview.chromium.org/2133873004/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_unittest.cc:887: static_cast<RenderWidgetHostViewBase*>(view.release())->Destroy(); I don't know why this is aura only. Destroy should be called on the view before deleting it on android too.
The CQ bit was checked by khushalsagar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org, dtrainor@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2133873004/#ps200001 (title: "Fix test.")
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 khushalsagar@chromium.org
https://codereview.chromium.org/2133873004/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2133873004/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1216: if (locks_on_frame_count_ == 0) On 2016/08/16 18:37:36, Khushal wrote: > On 2016/08/16 06:22:42, Khushal wrote: > > Btw, why is there a locks check here? > > Pingy! I'm about to remove the locking code here as it started scaring me. I already removed the call sites so it's unused now and should always be 0. https://codereview.chromium.org/2133873004/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2133873004/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:330: DCHECK(!delegated_frame_host_); On 2016/08/16 18:37:36, Khushal wrote: > On 2016/08/16 17:36:13, Khushal wrote: > > Looks like this is failing because RenderWidgetHostTest.Background does not > call > > Destroy before deleting the view. I had put the DFH destruction there and > > DCHECKed here since it looked like the contract is to always call Destroy on > the > > view before deleting it, and that's where we doing all the clean up earlier. > Is > > that true? > > Pingy! yes, and in fact 'delete this' is triggered from Destroy(). (I wonder if the destructor could be protected.)
https://codereview.chromium.org/2133873004/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2133873004/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1541: if (delegated_frame_host_ && delegated_frame_host_->HasDelegatedContent()) On 2016/08/16 18:32:59, sievers wrote: > You meant the opposite right? > > I'm actually not sure why we need this check. I'd think that even if we show > background color you'd want the overscroll effect. Maybe just remove it? Done.
The CQ bit was checked by khushalsagar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org, dtrainor@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2133873004/#ps220001 (title: "missed overscroll change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2133873004/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2133873004/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:330: DCHECK(!delegated_frame_host_); On 2016/08/16 20:36:36, sievers wrote: > On 2016/08/16 18:37:36, Khushal wrote: > > On 2016/08/16 17:36:13, Khushal wrote: > > > Looks like this is failing because RenderWidgetHostTest.Background does not > > call > > > Destroy before deleting the view. I had put the DFH destruction there and > > > DCHECKed here since it looked like the contract is to always call Destroy on > > the > > > view before deleting it, and that's where we doing all the clean up earlier. > > Is > > > that true? > > > > Pingy! > > yes, and in fact 'delete this' is triggered from Destroy(). (I wonder if the > destructor could be protected.) I was actually going to ask why its public if it should only be triggered with Destroy.
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://crrev.com/a693aa733d2a91627e174d62988c7ef591d963f2 Cr-Commit-Position: refs/heads/master@{#412356} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/a693aa733d2a91627e174d62988c7ef591d963f2 Cr-Commit-Position: refs/heads/master@{#412356} |
