Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2484793002:
Remove access to WebContents in RWHVA::SynchronousFrameMetadata() (Closed)
|
Created:
4 years, 1 month ago by Jinsuk Kim Modified:
4 years, 1 month ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, nona+watch_chromium.org, darin-cc_chromium.org, James Su Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove access to WebContents in RWHVA::SynchronousFrameMetadata()
This CL helps resolve layering violation done in
RenderWidgetHostViewAndroid by obtaining DevToolsAgentHost
instance using RenderFrameHost instead.
BUG=626765
Committed: https://crrev.com/16b5a22bcbb4fc166b893ded0ca8116e8632e6d0
Cr-Commit-Position: refs/heads/master@{#431061}
Patch Set 1 #
Total comments: 4
Patch Set 2 : case-by-case #
Total comments: 8
Patch Set 3 : comments #
Total comments: 4
Patch Set 4 : moved to RFDTAH #
Total comments: 8
Patch Set 5 : comments #
Messages
Total messages: 29 (13 generated)
Patchset #1 (id:1) has been deleted
jinsukkim@chromium.org changed reviewers: + boliu@chromium.org
I don't think we need a blanket RWHVADelegate, and can deal with them case by case https://codereview.chromium.org/2484793002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2484793002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:607: content_view_core_->ForceUpdateImeAdapter(GetNativeImeAdapter()); I think this can move to WebContentsViewAndroid, maybe? https://codereview.chromium.org/2484793002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1792: this, content_view_core_->GetWebContents()); you missed this one. along with the DevToolsAgentHost one, these two are specifically for synchronous compositor (ie webview), which is very different from how chrome rendering works So SynchronousCompositorHost is in the same layer as RWHVA, and should not access WebContents either. There is already SynchronousCompositorClient interface to talk up to the content embedder, perhaps there is a way to expand that interface to delegate for content as well, but without expanding the scope of the actual public interface, perhaps a subclass + composition pattern?
Description was changed from ========== Do not access WebContents directly from RenderWidgetHostViewAndroid renderer_host files should only call upwards in the layering via delegate interfaces. Added a delegate interface for RWHVA to avoid such layering violation. BUG=626765 ========== to ========== Do not access WebContents directly from RenderWidgetHostViewAndroid renderer_host files should only call upwards in the layering via delegate interfaces. Updated the way to access the necessary info (SynchronousCompositorClient, etc) for RWHVA to avoid such layering violation. BUG=626765 ==========
PTAL. Note that SynchronousCompositor still has access to WebContents - but it's not the scope of this CL. https://codereview.chromium.org/2484793002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2484793002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:607: content_view_core_->ForceUpdateImeAdapter(GetNativeImeAdapter()); On 2016/11/07 18:44:36, boliu wrote: > I think this can move to WebContentsViewAndroid, maybe? Done. https://codereview.chromium.org/2484793002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1792: this, content_view_core_->GetWebContents()); On 2016/11/07 18:44:36, boliu wrote: > you missed this one. along with the DevToolsAgentHost one, these two are > specifically for synchronous compositor (ie webview), which is very different > from how chrome rendering works > > So SynchronousCompositorHost is in the same layer as RWHVA, and should not > access WebContents either. There is already SynchronousCompositorClient > interface to talk up to the content embedder, perhaps there is a way to expand > that interface to delegate for content as well, but without expanding the scope > of the actual public interface, perhaps a subclass + composition pattern? Modified so that SynchronousCompositorClient is passed to RWVHA - then Host can be created without WebContents. With just one new public method in DevToolAgentHost (which I belive is acceptable), the other can do without WebContents too.
can you break this up? there are 3 distinct things here, the Focus change, SyncCompositorClient change, and DevTools change https://codereview.chromium.org/2484793002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2484793002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1189: if (!frame_host) can merge this with the HasFor check https://codereview.chromium.org/2484793002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2484793002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_android.cc:99: static_cast<WebContentsImpl*>(web_contents_)->GetWebContentsAndroid(); I think WCVA could just hold SCClient directly, and then don't need to involve WebContentsAndroid? You could even move the SynchronousCompositor::SetClientForWebContents implementation here, which "sorta" fixes the SCH dependency on WebContents. https://codereview.chromium.org/2484793002/diff/40001/content/public/browser/... File content/public/browser/devtools_agent_host.h (right): https://codereview.chromium.org/2484793002/diff/40001/content/public/browser/... content/public/browser/devtools_agent_host.h:67: // TODO(dgozman): this is a temporary measure until we can inspect says here accessor for RFH is temporary, perhaps we don't want to add more to this dependency.. https://codereview.chromium.org/2484793002/diff/40001/content/public/browser/... content/public/browser/devtools_agent_host.h:75: static bool HasFor(RenderFrameHost* frame_host); parameter overloading is not allowed by style guide, need to rename this something else.. plus this doesn't need to be on the public interface
Description was changed from ========== Do not access WebContents directly from RenderWidgetHostViewAndroid renderer_host files should only call upwards in the layering via delegate interfaces. Updated the way to access the necessary info (SynchronousCompositorClient, etc) for RWHVA to avoid such layering violation. BUG=626765 ========== to ========== Remove access to WebContents in RWHVA::SynchronousFrameMetadata() This CL helps resolve layering violation done in RenderWidgetHostViewAndroid by obtaining DevToolsAgentHost instance using RenderFrameHost instead. BUG=626765 ==========
https://codereview.chromium.org/2484793002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2484793002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1189: if (!frame_host) On 2016/11/08 00:13:55, boliu wrote: > can merge this with the HasFor check Done. https://codereview.chromium.org/2484793002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2484793002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_view_android.cc:99: static_cast<WebContentsImpl*>(web_contents_)->GetWebContentsAndroid(); On 2016/11/08 00:13:55, boliu wrote: > I think WCVA could just hold SCClient directly, and then don't need to involve > WebContentsAndroid? > > You could even move the SynchronousCompositor::SetClientForWebContents > implementation here, which "sorta" fixes the SCH dependency on WebContents. Good idea. Addressed in the new patch. https://codereview.chromium.org/2484793002/diff/40001/content/public/browser/... File content/public/browser/devtools_agent_host.h (right): https://codereview.chromium.org/2484793002/diff/40001/content/public/browser/... content/public/browser/devtools_agent_host.h:67: // TODO(dgozman): this is a temporary measure until we can inspect On 2016/11/08 00:13:55, boliu wrote: > says here accessor for RFH is temporary, perhaps we don't want to add more to > this dependency.. Granted, I just dgozman would take care of my addition too when he comes back to clear them up all. https://codereview.chromium.org/2484793002/diff/40001/content/public/browser/... content/public/browser/devtools_agent_host.h:75: static bool HasFor(RenderFrameHost* frame_host); On 2016/11/08 00:13:55, boliu wrote: > parameter overloading is not allowed by style guide, need to rename this > something else.. > > plus this doesn't need to be on the public interface Moved to RenderFrameDevToolsAgentHost.
boliu@chromium.org changed reviewers: + dgozman@chromium.org
+dgozman then Is it ok to add more dependency on GetOrCreateFor(RenderFrameHost*)? There's a comment that it's supposed to be temporary?
https://codereview.chromium.org/2484793002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2484793002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1194: RenderFrameHost* frame_host = RenderViewHost::From(host_)->GetMainFrame(); What about making RFDTAH::SynchronousSwapCompositorFrame function static, and moving all this code there? Then we avoid exposing more public functions and just issue instrumentation signals.
https://codereview.chromium.org/2484793002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2484793002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1194: RenderFrameHost* frame_host = RenderViewHost::From(host_)->GetMainFrame(); On 2016/11/08 18:41:08, dgozman wrote: > What about making RFDTAH::SynchronousSwapCompositorFrame function static, and > moving all this code there? Then we avoid exposing more public functions and > just issue instrumentation signals. If I get it right, I need to pass |content_view_core_| for RFDTAH to get WebContents from it. This will work but I will have to come back and make another change to remove content_view_core which will be going away. How about adding GetAgentHost(RenderFrameHost*) (or FindAgentHost public)? It won't depend on GetOrCreateFor(RenderFrameHost *).
https://codereview.chromium.org/2484793002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2484793002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1194: RenderFrameHost* frame_host = RenderViewHost::From(host_)->GetMainFrame(); On 2016/11/08 22:21:12, Jinsuk Kim wrote: > On 2016/11/08 18:41:08, dgozman wrote: > > What about making RFDTAH::SynchronousSwapCompositorFrame function static, and > > moving all this code there? Then we avoid exposing more public functions and > > just issue instrumentation signals. > > If I get it right, I need to pass |content_view_core_| for RFDTAH to get > WebContents from it. This will work but I will have to come back and make > another change to remove content_view_core which will be going away. > > How about adding GetAgentHost(RenderFrameHost*) (or FindAgentHost public)? It > won't depend on GetOrCreateFor(RenderFrameHost *). I suggest to add RenderFrameDevToolsAgentHost::SynchronousSwapCompositorFrame(RenderFrameHost*, cc::CompositorFrameMetadata) and call it retrieving RFH as you do here.
https://codereview.chromium.org/2484793002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2484793002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1194: RenderFrameHost* frame_host = RenderViewHost::From(host_)->GetMainFrame(); On 2016/11/08 22:47:29, dgozman wrote: > On 2016/11/08 22:21:12, Jinsuk Kim wrote: > > On 2016/11/08 18:41:08, dgozman wrote: > > > What about making RFDTAH::SynchronousSwapCompositorFrame function static, > and > > > moving all this code there? Then we avoid exposing more public functions and > > > just issue instrumentation signals. > > > > If I get it right, I need to pass |content_view_core_| for RFDTAH to get > > WebContents from it. This will work but I will have to come back and make > > another change to remove content_view_core which will be going away. > > > > How about adding GetAgentHost(RenderFrameHost*) (or FindAgentHost public)? It > > won't depend on GetOrCreateFor(RenderFrameHost *). > > I suggest to add > RenderFrameDevToolsAgentHost::SynchronousSwapCompositorFrame(RenderFrameHost*, > cc::CompositorFrameMetadata) > and call it retrieving RFH as you do here. Now I got it. Thanks! Done. I named the static method 'Signal'SynchronousSwapCompositorFrame, which in turn calls SynchronousSwapCompositorFrame which does the actual job.
lgtm https://codereview.chromium.org/2484793002/diff/80001/content/browser/devtool... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2484793002/diff/80001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:1057: cc::CompositorFrameMetadata* frame_metadata) { Take a class, not pointer to it. https://codereview.chromium.org/2484793002/diff/80001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:1066: base::Passed(frame_metadata))); base::Passed(std::move(frame_metadata)) https://codereview.chromium.org/2484793002/diff/80001/content/browser/devtool... File content/browser/devtools/render_frame_devtools_agent_host.h (right): https://codereview.chromium.org/2484793002/diff/80001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.h:79: void SynchronousSwapCompositorFrame( Move this to private section please. https://codereview.chromium.org/2484793002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2484793002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1198: &frame_metadata); std::move(frame_metadata)
https://codereview.chromium.org/2484793002/diff/80001/content/browser/devtool... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2484793002/diff/80001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:1057: cc::CompositorFrameMetadata* frame_metadata) { On 2016/11/08 23:36:01, dgozman wrote: > Take a class, not pointer to it. Done. https://codereview.chromium.org/2484793002/diff/80001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:1066: base::Passed(frame_metadata))); On 2016/11/08 23:36:00, dgozman wrote: > base::Passed(std::move(frame_metadata)) Done. https://codereview.chromium.org/2484793002/diff/80001/content/browser/devtool... File content/browser/devtools/render_frame_devtools_agent_host.h (right): https://codereview.chromium.org/2484793002/diff/80001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.h:79: void SynchronousSwapCompositorFrame( On 2016/11/08 23:36:01, dgozman wrote: > Move this to private section please. Done. https://codereview.chromium.org/2484793002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2484793002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1198: &frame_metadata); On 2016/11/08 23:36:01, dgozman wrote: > std::move(frame_metadata) Done.
lgtm
The CQ bit was checked by jinsukkim@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 jinsukkim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2484793002/#ps100001 (title: "comments")
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 ========== Remove access to WebContents in RWHVA::SynchronousFrameMetadata() This CL helps resolve layering violation done in RenderWidgetHostViewAndroid by obtaining DevToolsAgentHost instance using RenderFrameHost instead. BUG=626765 ========== to ========== Remove access to WebContents in RWHVA::SynchronousFrameMetadata() This CL helps resolve layering violation done in RenderWidgetHostViewAndroid by obtaining DevToolsAgentHost instance using RenderFrameHost instead. BUG=626765 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Remove access to WebContents in RWHVA::SynchronousFrameMetadata() This CL helps resolve layering violation done in RenderWidgetHostViewAndroid by obtaining DevToolsAgentHost instance using RenderFrameHost instead. BUG=626765 ========== to ========== Remove access to WebContents in RWHVA::SynchronousFrameMetadata() This CL helps resolve layering violation done in RenderWidgetHostViewAndroid by obtaining DevToolsAgentHost instance using RenderFrameHost instead. BUG=626765 Committed: https://crrev.com/16b5a22bcbb4fc166b893ded0ca8116e8632e6d0 Cr-Commit-Position: refs/heads/master@{#431061} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/16b5a22bcbb4fc166b893ded0ca8116e8632e6d0 Cr-Commit-Position: refs/heads/master@{#431061} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
