Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1472063007:
mustash: enable GPU Compositing in renderer (Closed)
|
Created:
5 years ago by Peng Modified:
5 years ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, yusukes+watch_chromium.org, Aaron Boodman, shuchen+watch_chromium.org, nasko+codewatch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, nona+watch_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, darin (slow to review), James Su, ben+mojo_chromium.org, viettrungluu+watch_chromium.org, qsr+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmustash: enable GPU Compositing in renderer.
By using mus::OutputSurface as the cc output surface to allow
renderer to draw on a mus::Window directly.
Known issues:
Input doesn't work.
Renderer position isn't correct.
Resizing is not smooth.
TEST=./out/mandoline_Debug/mojo_runner mojo:example_main --use-mus-in-renderer --use-zero-copy
BUG=551250
TBR=jam@chromium.org
Committed: https://crrev.com/28a5fa2c08f88309e825b46b3b15a345044c712d
Cr-Commit-Position: refs/heads/master@{#362745}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Update #Patch Set 3 : WIP #Patch Set 4 : WIP #Patch Set 5 : Rebase #Patch Set 6 : Update #
Total comments: 16
Patch Set 7 : Address review issues #
Total comments: 2
Patch Set 8 : git cl format #
Total comments: 6
Patch Set 9 : Address review issues #Patch Set 10 : . #Patch Set 11 : . #
Total comments: 1
Patch Set 12 : . #
Total comments: 10
Patch Set 13 : address review issues. #
Total comments: 2
Patch Set 14 : Address issues. #
Total comments: 2
Messages
Total messages: 52 (17 generated)
Description was changed from ========== mustash: GPU Compositing in renderer BUG= ========== to ========== mustash: GPU Compositing in renderer BUG= ==========
penghuang@chromium.org changed reviewers: + fsamuel@chromium.org
https://codereview.chromium.org/1472063007/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_mus.cc (right): https://codereview.chromium.org/1472063007/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mus.cc:52: DoNothing); Looks like in the browser process, this function is called before creating the RenderView. But in the renderer process, the RenderWidgetMusConnection is created after RenderView. I tried using factory.WaitForIncomingResponse() to make sure the RWMusConnection is created first, but factory.WaitForIncomingResponse() never returned. Hi Fady, do you know why? BTW, Stacks create RenderView vs RWHVMus #0 0x7efe12371f7e base::debug::StackTrace::StackTrace() #1 0x7efe0cb0d021 content::RenderViewHostImpl::CreateRenderView() #2 0x7efe0cd99696 content::WebContentsImpl::CreateRenderViewForRenderManager() #3 0x7efe0c6d39be content::RenderFrameHostManager::InitRenderView() #4 0x7efe0c6dabfa content::RenderFrameHostManager::CreateRenderFrame() #5 0x7efe0c6da1bf content::RenderFrameHostManager::CreatePendingRenderFrameHost() #6 0x7efe0c6d2b16 content::RenderFrameHostManager::UpdateStateForNavigate() #7 0x7efe0c6d2020 content::RenderFrameHostManager::Navigate() #8 0x7efe0c6aa57c content::NavigatorImpl::NavigateToEntry() #9 0x7efe0c6ab2ac content::NavigatorImpl::NavigateToPendingEntry() #10 0x7efe0c68d6ea content::NavigationControllerImpl::NavigateToPendingEntryInternal() #11 0x7efe0c68618c content::NavigationControllerImpl::NavigateToPendingEntry() #12 0x7efe0c686719 content::NavigationControllerImpl::LoadEntry() #13 0x7efe0c68879a content::NavigationControllerImpl::LoadURLWithParams() #14 0x7efe14f65fe9 (anonymous namespace)::LoadURLInContents() #15 0x7efe14f64d83 chrome::Navigate() #16 0x7efe14f6b4be chrome::AddTabAt() #17 0x7efe14f67746 chrome::BrowserTabStripModelDelegate::AddTabAt() #18 0x7efe1530e81c BrowserTabStripController::CreateNewTab() ====================== #0 0x7f7ce10d9f7e base::debug::StackTrace::StackTrace() #1 0x7f7cdbc1b43a content::RenderWidgetHostViewMus::RenderWidgetHostViewMus() #2 0x7f7cdbc1d692 content::WebContentsViewMus::CreateViewForWidget() #3 0x7f7cdbb01432 content::WebContentsImpl::CreateRenderWidgetHostViewForRenderManager() #4 0x7f7cdbb015c8 content::WebContentsImpl::CreateRenderViewForRenderManager() #5 0x7f7cdb43b9be content::RenderFrameHostManager::InitRenderView() #6 0x7f7cdb442bfa content::RenderFrameHostManager::CreateRenderFrame() #7 0x7f7cdb4421bf content::RenderFrameHostManager::CreatePendingRenderFrameHost() #8 0x7f7cdb43ab16 content::RenderFrameHostManager::UpdateStateForNavigate() #9 0x7f7cdb43a020 content::RenderFrameHostManager::Navigate() #10 0x7f7cdb41257c content::NavigatorImpl::NavigateToEntry() #11 0x7f7cdb4132ac content::NavigatorImpl::NavigateToPendingEntry() #12 0x7f7cdb3f56ea content::NavigationControllerImpl::NavigateToPendingEntryInternal() #13 0x7f7cdb3ee18c content::NavigationControllerImpl::NavigateToPendingEntry() #14 0x7f7cdb3ee719 content::NavigationControllerImpl::LoadEntry() #15 0x7f7cdb3f079a content::NavigationControllerImpl::LoadURLWithParams() #16 0x7f7ce3ccdfe9 (anonymous namespace)::LoadURLInContents() #17 0x7f7ce3cccd83 chrome::Navigate() #18 0x7f7ce3cd34be chrome::AddTabAt() #19 0x7f7ce3ccf746 chrome::BrowserTabStripModelDelegate::AddTabAt() #20 0x7f7ce407681c BrowserTabStripController::CreateNewTab()
fsamuel@chromium.org changed reviewers: + ben@chromium.org, jam@chromium.org
+ben@ jam@ for thoughts about mojo v.s. chrome IPC ordering on Monday (Don't worry about it now, Happy Thanskgiving!) https://codereview.chromium.org/1472063007/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_mus.cc (right): https://codereview.chromium.org/1472063007/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mus.cc:52: DoNothing); On 2015/11/26 22:13:04, Peng wrote: > Looks like in the browser process, this function is called before creating the > RenderView. But in the renderer process, the RenderWidgetMusConnection is > created after RenderView. > > I tried using factory.WaitForIncomingResponse() to make sure the RWMusConnection > is created first, but factory.WaitForIncomingResponse() never returned. > > > Hi Fady, do you know why? > > > BTW, Stacks create RenderView vs RWHVMus > > #0 0x7efe12371f7e base::debug::StackTrace::StackTrace() > #1 0x7efe0cb0d021 content::RenderViewHostImpl::CreateRenderView() > #2 0x7efe0cd99696 content::WebContentsImpl::CreateRenderViewForRenderManager() > #3 0x7efe0c6d39be content::RenderFrameHostManager::InitRenderView() > #4 0x7efe0c6dabfa content::RenderFrameHostManager::CreateRenderFrame() > #5 0x7efe0c6da1bf > content::RenderFrameHostManager::CreatePendingRenderFrameHost() > #6 0x7efe0c6d2b16 content::RenderFrameHostManager::UpdateStateForNavigate() > #7 0x7efe0c6d2020 content::RenderFrameHostManager::Navigate() > #8 0x7efe0c6aa57c content::NavigatorImpl::NavigateToEntry() > #9 0x7efe0c6ab2ac content::NavigatorImpl::NavigateToPendingEntry() > #10 0x7efe0c68d6ea > content::NavigationControllerImpl::NavigateToPendingEntryInternal() > #11 0x7efe0c68618c content::NavigationControllerImpl::NavigateToPendingEntry() > #12 0x7efe0c686719 content::NavigationControllerImpl::LoadEntry() > #13 0x7efe0c68879a content::NavigationControllerImpl::LoadURLWithParams() > #14 0x7efe14f65fe9 (anonymous namespace)::LoadURLInContents() > #15 0x7efe14f64d83 chrome::Navigate() > #16 0x7efe14f6b4be chrome::AddTabAt() > #17 0x7efe14f67746 chrome::BrowserTabStripModelDelegate::AddTabAt() > #18 0x7efe1530e81c BrowserTabStripController::CreateNewTab() > > ====================== > > #0 0x7f7ce10d9f7e base::debug::StackTrace::StackTrace() > #1 0x7f7cdbc1b43a content::RenderWidgetHostViewMus::RenderWidgetHostViewMus() > #2 0x7f7cdbc1d692 content::WebContentsViewMus::CreateViewForWidget() > #3 0x7f7cdbb01432 > content::WebContentsImpl::CreateRenderWidgetHostViewForRenderManager() > #4 0x7f7cdbb015c8 content::WebContentsImpl::CreateRenderViewForRenderManager() > #5 0x7f7cdb43b9be content::RenderFrameHostManager::InitRenderView() > #6 0x7f7cdb442bfa content::RenderFrameHostManager::CreateRenderFrame() > #7 0x7f7cdb4421bf > content::RenderFrameHostManager::CreatePendingRenderFrameHost() > #8 0x7f7cdb43ab16 content::RenderFrameHostManager::UpdateStateForNavigate() > #9 0x7f7cdb43a020 content::RenderFrameHostManager::Navigate() > #10 0x7f7cdb41257c content::NavigatorImpl::NavigateToEntry() > #11 0x7f7cdb4132ac content::NavigatorImpl::NavigateToPendingEntry() > #12 0x7f7cdb3f56ea > content::NavigationControllerImpl::NavigateToPendingEntryInternal() > #13 0x7f7cdb3ee18c content::NavigationControllerImpl::NavigateToPendingEntry() > #14 0x7f7cdb3ee719 content::NavigationControllerImpl::LoadEntry() > #15 0x7f7cdb3f079a content::NavigationControllerImpl::LoadURLWithParams() > #16 0x7f7ce3ccdfe9 (anonymous namespace)::LoadURLInContents() > #17 0x7f7ce3cccd83 chrome::Navigate() > #18 0x7f7ce3cd34be chrome::AddTabAt() > #19 0x7f7ce3ccf746 chrome::BrowserTabStripModelDelegate::AddTabAt() > #20 0x7f7ce407681c BrowserTabStripController::CreateNewTab() Mojo IPCs and Chrome IPCs can be issued out of order because they use different queues. +ben@, jam@ for thoughts. We could try my alternative solution here in the meantime: https://codereview.chromium.org/1461243002/
On 2015/11/27 00:26:21, Fady Samuel wrote: > +ben@ jam@ for thoughts about mojo v.s. chrome IPC ordering on Monday (Don't > worry about it now, Happy Thanskgiving!) > > https://codereview.chromium.org/1472063007/diff/1/content/browser/renderer_ho... > File content/browser/renderer_host/render_widget_host_view_mus.cc (right): > > https://codereview.chromium.org/1472063007/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/render_widget_host_view_mus.cc:52: DoNothing); > On 2015/11/26 22:13:04, Peng wrote: > > Looks like in the browser process, this function is called before creating the > > RenderView. But in the renderer process, the RenderWidgetMusConnection is > > created after RenderView. > > > > I tried using factory.WaitForIncomingResponse() to make sure the > RWMusConnection > > is created first, but factory.WaitForIncomingResponse() never returned. > > > > > > Hi Fady, do you know why? > > > > > > BTW, Stacks create RenderView vs RWHVMus > > > > #0 0x7efe12371f7e base::debug::StackTrace::StackTrace() > > #1 0x7efe0cb0d021 content::RenderViewHostImpl::CreateRenderView() > > #2 0x7efe0cd99696 content::WebContentsImpl::CreateRenderViewForRenderManager() > > #3 0x7efe0c6d39be content::RenderFrameHostManager::InitRenderView() > > #4 0x7efe0c6dabfa content::RenderFrameHostManager::CreateRenderFrame() > > #5 0x7efe0c6da1bf > > content::RenderFrameHostManager::CreatePendingRenderFrameHost() > > #6 0x7efe0c6d2b16 content::RenderFrameHostManager::UpdateStateForNavigate() > > #7 0x7efe0c6d2020 content::RenderFrameHostManager::Navigate() > > #8 0x7efe0c6aa57c content::NavigatorImpl::NavigateToEntry() > > #9 0x7efe0c6ab2ac content::NavigatorImpl::NavigateToPendingEntry() > > #10 0x7efe0c68d6ea > > content::NavigationControllerImpl::NavigateToPendingEntryInternal() > > #11 0x7efe0c68618c content::NavigationControllerImpl::NavigateToPendingEntry() > > #12 0x7efe0c686719 content::NavigationControllerImpl::LoadEntry() > > #13 0x7efe0c68879a content::NavigationControllerImpl::LoadURLWithParams() > > #14 0x7efe14f65fe9 (anonymous namespace)::LoadURLInContents() > > #15 0x7efe14f64d83 chrome::Navigate() > > #16 0x7efe14f6b4be chrome::AddTabAt() > > #17 0x7efe14f67746 chrome::BrowserTabStripModelDelegate::AddTabAt() > > #18 0x7efe1530e81c BrowserTabStripController::CreateNewTab() > > > > ====================== > > > > #0 0x7f7ce10d9f7e base::debug::StackTrace::StackTrace() > > #1 0x7f7cdbc1b43a content::RenderWidgetHostViewMus::RenderWidgetHostViewMus() > > #2 0x7f7cdbc1d692 content::WebContentsViewMus::CreateViewForWidget() > > #3 0x7f7cdbb01432 > > content::WebContentsImpl::CreateRenderWidgetHostViewForRenderManager() > > #4 0x7f7cdbb015c8 content::WebContentsImpl::CreateRenderViewForRenderManager() > > #5 0x7f7cdb43b9be content::RenderFrameHostManager::InitRenderView() > > #6 0x7f7cdb442bfa content::RenderFrameHostManager::CreateRenderFrame() > > #7 0x7f7cdb4421bf > > content::RenderFrameHostManager::CreatePendingRenderFrameHost() > > #8 0x7f7cdb43ab16 content::RenderFrameHostManager::UpdateStateForNavigate() > > #9 0x7f7cdb43a020 content::RenderFrameHostManager::Navigate() > > #10 0x7f7cdb41257c content::NavigatorImpl::NavigateToEntry() > > #11 0x7f7cdb4132ac content::NavigatorImpl::NavigateToPendingEntry() > > #12 0x7f7cdb3f56ea > > content::NavigationControllerImpl::NavigateToPendingEntryInternal() > > #13 0x7f7cdb3ee18c content::NavigationControllerImpl::NavigateToPendingEntry() > > #14 0x7f7cdb3ee719 content::NavigationControllerImpl::LoadEntry() > > #15 0x7f7cdb3f079a content::NavigationControllerImpl::LoadURLWithParams() > > #16 0x7f7ce3ccdfe9 (anonymous namespace)::LoadURLInContents() > > #17 0x7f7ce3cccd83 chrome::Navigate() > > #18 0x7f7ce3cd34be chrome::AddTabAt() > > #19 0x7f7ce3ccf746 chrome::BrowserTabStripModelDelegate::AddTabAt() > > #20 0x7f7ce407681c BrowserTabStripController::CreateNewTab() > > Mojo IPCs and Chrome IPCs can be issued out of order because they use different > queues. +ben@, jam@ for thoughts. We could try my alternative solution here in > the meantime: https://codereview.chromium.org/1461243002/ Actually, I was looking at WebLayerTreeViewImpl::Initialize, and it needs a GpuPtr from mus anyway. I wonder if we can solve this problem without worrying about ordering as follows: make it possible to create a WindowSurface wrapper a priori and then assign it to the mus::Window when we have it. Basically, introduce a mus::Window::SetSurface (or BindToSurface, I'm indifferent about names) instead of, or in addition to RequestSurface. Add a helper to the mus client lib perhaps, mus::CreateWindowSurface. This creates one end of the MessagePipe, and the other end is passed to mus when the window is ready in RenderWidgetMusConnection::OnEmbed. Let's try that tomorrow, I'm going to need to document all this and the design decisions to date tomorrow.
https://codereview.chromium.org/1472063007/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_mus.cc (right): https://codereview.chromium.org/1472063007/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mus.cc:54: factory.WaitForIncomingResponse(); I think WaitForImcomingResponse() should block current thread until renderer finishes the work, but this function doesn't return for ever, seems deadlock happens inside it (Or I don't use it correctly). Fady, any idea about it?
On 2015/11/27 15:08:03, Peng wrote: > https://codereview.chromium.org/1472063007/diff/1/content/browser/renderer_ho... > File content/browser/renderer_host/render_widget_host_view_mus.cc (right): > > https://codereview.chromium.org/1472063007/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/render_widget_host_view_mus.cc:54: > factory.WaitForIncomingResponse(); > I think WaitForImcomingResponse() should block current thread until renderer > finishes the work, but this function doesn't return for ever, seems deadlock > happens inside it (Or I don't use it correctly). > > Fady, any idea about it? Yea, you need to make RenderWidgetWindowTreeClientFactory a member variable or else the connection is dropped immediately. However, this is not the right solution. The browser should not block on the renderer. I'm working on a solution here: https://codereview.chromium.org/1484533002/
Description was changed from ========== mustash: GPU Compositing in renderer BUG= ========== to ========== mustash: enable GPU Compositing in renderer BUG= ==========
Description was changed from ========== mustash: enable GPU Compositing in renderer BUG= ========== to ========== mustash: enable GPU Compositing in renderer. By using mus::OutputSurface as the cc output surface to allow renderer to draw on a mus::Window directly. TEST=./out/mandoline_Debug/mojo_runner mojo:example_main --use-mus-in-renderer --use-zero-copy BUG= ==========
Description was changed from ========== mustash: enable GPU Compositing in renderer. By using mus::OutputSurface as the cc output surface to allow renderer to draw on a mus::Window directly. TEST=./out/mandoline_Debug/mojo_runner mojo:example_main --use-mus-in-renderer --use-zero-copy BUG= ========== to ========== mustash: enable GPU Compositing in renderer. By using mus::OutputSurface as the cc output surface to allow renderer to draw on a mus::Window directly. Known issues: Input doesn't work. Renderer position isn't correct. Resizing is not smooth. TEST=./out/mandoline_Debug/mojo_runner mojo:example_main --use-mus-in-renderer --use-zero-copy BUG= ==========
So close! Thanks Peng! https://codereview.chromium.org/1472063007/diff/90001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mus.cc (right): https://codereview.chromium.org/1472063007/diff/90001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mus.cc:66: bounds.set_x(10); Please add a TODO(fsamuel): above to figure out position. https://codereview.chromium.org/1472063007/diff/90001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/1472063007/diff/90001/content/renderer/render... content/renderer/render_view_impl.cc:1721: if (MojoShellConnection::Get()->GetApplication()) { I would also put this whole block around #if defined(MOJO_SHELL_CLIENT) ... #endif https://codereview.chromium.org/1472063007/diff/90001/content/renderer/render... File content/renderer/render_widget_mus_connection.cc (right): https://codereview.chromium.org/1472063007/diff/90001/content/renderer/render... content/renderer/render_widget_mus_connection.cc:17: #include "content/renderer/render_view_impl.h" Do you need this? https://codereview.chromium.org/1472063007/diff/90001/content/renderer/render... content/renderer/render_widget_mus_connection.cc:45: DCHECK(!connected_); Just check if you have a root. You don't need this flag. https://codereview.chromium.org/1472063007/diff/90001/content/renderer/render... content/renderer/render_widget_mus_connection.cc:71: RenderWidgetMusConnection* RenderWidgetMusConnection::Get(int routing_id) { Add // static above. https://codereview.chromium.org/1472063007/diff/90001/content/renderer/render... File content/renderer/render_widget_mus_connection.h (left): https://codereview.chromium.org/1472063007/diff/90001/content/renderer/render... content/renderer/render_widget_mus_connection.h:21: RenderWidgetMusConnection( nit: explicit https://codereview.chromium.org/1472063007/diff/90001/content/renderer/render... File content/renderer/render_widget_mus_connection.h (right): https://codereview.chromium.org/1472063007/diff/90001/content/renderer/render... content/renderer/render_widget_mus_connection.h:46: bool connected_; Remove.
https://codereview.chromium.org/1472063007/diff/90001/content/renderer/render... File content/renderer/render_widget_mus_connection.h (right): https://codereview.chromium.org/1472063007/diff/90001/content/renderer/render... content/renderer/render_widget_mus_connection.h:34: static RenderWidgetMusConnection* Get(int routing_id); nit: I usually prefer GetOrCreate for accessors that might create the object on first use.
https://codereview.chromium.org/1472063007/diff/90001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mus.cc (right): https://codereview.chromium.org/1472063007/diff/90001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mus.cc:66: bounds.set_x(10); On 2015/11/30 23:06:54, Fady Samuel wrote: > Please add a TODO(fsamuel): above to figure out position. Done. https://codereview.chromium.org/1472063007/diff/90001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/1472063007/diff/90001/content/renderer/render... content/renderer/render_view_impl.cc:1721: if (MojoShellConnection::Get()->GetApplication()) { On 2015/11/30 23:06:54, Fady Samuel wrote: > I would also put this whole block around > > #if defined(MOJO_SHELL_CLIENT) > ... > > #endif Done. https://codereview.chromium.org/1472063007/diff/90001/content/renderer/render... File content/renderer/render_widget_mus_connection.cc (right): https://codereview.chromium.org/1472063007/diff/90001/content/renderer/render... content/renderer/render_widget_mus_connection.cc:17: #include "content/renderer/render_view_impl.h" On 2015/11/30 23:06:54, Fady Samuel wrote: > Do you need this? Done. https://codereview.chromium.org/1472063007/diff/90001/content/renderer/render... content/renderer/render_widget_mus_connection.cc:45: DCHECK(!connected_); On 2015/11/30 23:06:54, Fady Samuel wrote: > Just check if you have a root. You don't need this flag. Done. https://codereview.chromium.org/1472063007/diff/90001/content/renderer/render... content/renderer/render_widget_mus_connection.cc:71: RenderWidgetMusConnection* RenderWidgetMusConnection::Get(int routing_id) { On 2015/11/30 23:06:54, Fady Samuel wrote: > Add // static above. Done. https://codereview.chromium.org/1472063007/diff/90001/content/renderer/render... File content/renderer/render_widget_mus_connection.h (left): https://codereview.chromium.org/1472063007/diff/90001/content/renderer/render... content/renderer/render_widget_mus_connection.h:21: RenderWidgetMusConnection( On 2015/11/30 23:06:55, Fady Samuel wrote: > nit: explicit Done. https://codereview.chromium.org/1472063007/diff/90001/content/renderer/render... File content/renderer/render_widget_mus_connection.h (right): https://codereview.chromium.org/1472063007/diff/90001/content/renderer/render... content/renderer/render_widget_mus_connection.h:34: static RenderWidgetMusConnection* Get(int routing_id); On 2015/11/30 23:19:19, Fady Samuel wrote: > nit: I usually prefer GetOrCreate for accessors that might create the object on > first use. Done. https://codereview.chromium.org/1472063007/diff/90001/content/renderer/render... content/renderer/render_widget_mus_connection.h:46: bool connected_; On 2015/11/30 23:06:55, Fady Samuel wrote: > Remove. Done.
Please make this bug 551250. LGTM. I'm happy with this modulo the nit. You can pass this on to Ben and John afterward. https://codereview.chromium.org/1472063007/diff/110001/content/renderer/rende... File content/renderer/render_widget_mus_connection.cc (right): https://codereview.chromium.org/1472063007/diff/110001/content/renderer/rende... content/renderer/render_widget_mus_connection.cc:32: int routing_id) nit: Run git cl format
https://codereview.chromium.org/1472063007/diff/110001/content/renderer/rende... File content/renderer/render_widget_mus_connection.cc (right): https://codereview.chromium.org/1472063007/diff/110001/content/renderer/rende... content/renderer/render_widget_mus_connection.cc:32: int routing_id) On 2015/12/01 15:52:27, Fady Samuel wrote: > nit: Run git cl format Done.
Ben & John, PTAL. Thanks.
Description was changed from ========== mustash: enable GPU Compositing in renderer. By using mus::OutputSurface as the cc output surface to allow renderer to draw on a mus::Window directly. Known issues: Input doesn't work. Renderer position isn't correct. Resizing is not smooth. TEST=./out/mandoline_Debug/mojo_runner mojo:example_main --use-mus-in-renderer --use-zero-copy BUG= ========== to ========== mustash: enable GPU Compositing in renderer. By using mus::OutputSurface as the cc output surface to allow renderer to draw on a mus::Window directly. Known issues: Input doesn't work. Renderer position isn't correct. Resizing is not smooth. TEST=./out/mandoline_Debug/mojo_runner mojo:example_main --use-mus-in-renderer --use-zero-copy BUG=551250 ==========
https://codereview.chromium.org/1472063007/diff/130001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_mus.cc (right): https://codereview.chromium.org/1472063007/diff/130001/content/browser/render... content/browser/renderer_host/render_widget_host_view_mus.cc:66: // TODO(fsamuel): figure out position. I suspect you'll end up needing to have an aura::Window in thsi class (e.g. to implement GetNativeView/Window). You should be able to obtain the bounds from that. https://codereview.chromium.org/1472063007/diff/130001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/1472063007/diff/130001/content/renderer/rende... content/renderer/render_view_impl.cc:1719: scoped_ptr<cc::OutputSurface> RenderViewImpl::CreateOutputSurface( why is this overridden in RenderView? What about RenderWidgets? https://codereview.chromium.org/1472063007/diff/130001/content/renderer/rende... File content/renderer/render_widget_mus_connection.h (right): https://codereview.chromium.org/1472063007/diff/130001/content/renderer/rende... content/renderer/render_widget_mus_connection.h:26: void Connect(mojo::InterfaceRequest<mus::mojom::WindowTreeClient> request); more commonly called Bind() in mojo code.
https://codereview.chromium.org/1472063007/diff/130001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_mus.cc (right): https://codereview.chromium.org/1472063007/diff/130001/content/browser/render... content/browser/renderer_host/render_widget_host_view_mus.cc:66: // TODO(fsamuel): figure out position. On 2015/12/01 18:44:14, Ben Goodger (Google) wrote: > I suspect you'll end up needing to have an aura::Window in thsi class (e.g. to > implement GetNativeView/Window). You should be able to obtain the bounds from > that. Talked to fsamuel, and he will take care of it. Acknowledged. https://codereview.chromium.org/1472063007/diff/130001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/1472063007/diff/130001/content/renderer/rende... content/renderer/render_view_impl.cc:1719: scoped_ptr<cc::OutputSurface> RenderViewImpl::CreateOutputSurface( On 2015/12/01 18:44:14, Ben Goodger (Google) wrote: > why is this overridden in RenderView? What about RenderWidgets? RenderWidget is a better place for it. Done https://codereview.chromium.org/1472063007/diff/130001/content/renderer/rende... File content/renderer/render_widget_mus_connection.h (right): https://codereview.chromium.org/1472063007/diff/130001/content/renderer/rende... content/renderer/render_widget_mus_connection.h:26: void Connect(mojo::InterfaceRequest<mus::mojom::WindowTreeClient> request); On 2015/12/01 18:44:14, Ben Goodger (Google) wrote: > more commonly called Bind() in mojo code. Done.
lgtm
On 2015/12/01 20:13:23, Ben Goodger (Google) wrote: > lgtm btw jam sez he doesn't need to look at this so my review is sufficient.
Description was changed from ========== mustash: enable GPU Compositing in renderer. By using mus::OutputSurface as the cc output surface to allow renderer to draw on a mus::Window directly. Known issues: Input doesn't work. Renderer position isn't correct. Resizing is not smooth. TEST=./out/mandoline_Debug/mojo_runner mojo:example_main --use-mus-in-renderer --use-zero-copy BUG=551250 ========== to ========== mustash: enable GPU Compositing in renderer. By using mus::OutputSurface as the cc output surface to allow renderer to draw on a mus::Window directly. Known issues: Input doesn't work. Renderer position isn't correct. Resizing is not smooth. TEST=./out/mandoline_Debug/mojo_runner mojo:example_main --use-mus-in-renderer --use-zero-copy BUG=551250 TBR=jam@chromium.org ==========
The CQ bit was checked by penghuang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org Link to the patchset: https://codereview.chromium.org/1472063007/#ps150001 (title: "Address review issues")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1472063007/150001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1472063007/150001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by penghuang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org, ben@chromium.org Link to the patchset: https://codereview.chromium.org/1472063007/#ps170001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1472063007/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1472063007/170001
https://codereview.chromium.org/1472063007/diff/190001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1472063007/diff/190001/content/renderer/rende... content/renderer/render_widget.cc:1022: if (MojoShellConnection::Get()->GetApplication() && !use_software) { you should null check Get() before dereferencing. You won't have one of these objects when you are not run from the external shell.
On 2015/12/01 22:46:58, Ben Goodger (Google) wrote: > https://codereview.chromium.org/1472063007/diff/190001/content/renderer/rende... > File content/renderer/render_widget.cc (right): > > https://codereview.chromium.org/1472063007/diff/190001/content/renderer/rende... > content/renderer/render_widget.cc:1022: if > (MojoShellConnection::Get()->GetApplication() && !use_software) { > you should null check Get() before dereferencing. You won't have one of these > objects when you are not run from the external shell.
On 2015/12/01 22:47:46, Fady Samuel wrote: > On 2015/12/01 22:46:58, Ben Goodger (Google) wrote: > > > https://codereview.chromium.org/1472063007/diff/190001/content/renderer/rende... > > File content/renderer/render_widget.cc (right): > > > > > https://codereview.chromium.org/1472063007/diff/190001/content/renderer/rende... > > content/renderer/render_widget.cc:1022: if > > (MojoShellConnection::Get()->GetApplication() && !use_software) { > > you should null check Get() before dereferencing. You won't have one of these > > objects when you are not run from the external shell. Actually, this is no longer true. I always create a MojoShellConnection in RendererMain so that I can install a listener before getting a mojo shell handle.
On 2015/12/01 22:48:50, Fady Samuel wrote: > On 2015/12/01 22:47:46, Fady Samuel wrote: > > On 2015/12/01 22:46:58, Ben Goodger (Google) wrote: > > > > > > https://codereview.chromium.org/1472063007/diff/190001/content/renderer/rende... > > > File content/renderer/render_widget.cc (right): > > > > > > > > > https://codereview.chromium.org/1472063007/diff/190001/content/renderer/rende... > > > content/renderer/render_widget.cc:1022: if > > > (MojoShellConnection::Get()->GetApplication() && !use_software) { > > > you should null check Get() before dereferencing. You won't have one of > these > > > objects when you are not run from the external shell. > > Actually, this is no longer true. I always create a MojoShellConnection in > RendererMain so that I can install a listener before getting a mojo shell > handle. OK in this case this object needs to have a "HasApplication()" or "IsBound()" method that returns whether or not the application request has been bound yet, and this code needs to check it otherwise you'll be hitting dchecks in tests.
On 2015/12/01 22:52:04, Ben Goodger (Google) wrote: > On 2015/12/01 22:48:50, Fady Samuel wrote: > > On 2015/12/01 22:47:46, Fady Samuel wrote: > > > On 2015/12/01 22:46:58, Ben Goodger (Google) wrote: > > > > > > > > > > https://codereview.chromium.org/1472063007/diff/190001/content/renderer/rende... > > > > File content/renderer/render_widget.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1472063007/diff/190001/content/renderer/rende... > > > > content/renderer/render_widget.cc:1022: if > > > > (MojoShellConnection::Get()->GetApplication() && !use_software) { > > > > you should null check Get() before dereferencing. You won't have one of > > these > > > > objects when you are not run from the external shell. > > > > Actually, this is no longer true. I always create a MojoShellConnection in > > RendererMain so that I can install a listener before getting a mojo shell > > handle. > > OK in this case this object needs to have a "HasApplication()" or "IsBound()" > method that returns whether or not the application request has been bound yet, > and this code needs to check it otherwise you'll be hitting dchecks in tests. I discussed it with Fady. We think it is better to add a flag to renderer to indicate if the mojo shell will be used. And the MojoShellConnection will only be created, if the command line has the flag. So in here, we can just need check MojoShellConnection::Get(). WDYT?
On 2015/12/02 00:44:48, Peng wrote: > On 2015/12/01 22:52:04, Ben Goodger (Google) wrote: > > On 2015/12/01 22:48:50, Fady Samuel wrote: > > > On 2015/12/01 22:47:46, Fady Samuel wrote: > > > > On 2015/12/01 22:46:58, Ben Goodger (Google) wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1472063007/diff/190001/content/renderer/rende... > > > > > File content/renderer/render_widget.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1472063007/diff/190001/content/renderer/rende... > > > > > content/renderer/render_widget.cc:1022: if > > > > > (MojoShellConnection::Get()->GetApplication() && !use_software) { > > > > > you should null check Get() before dereferencing. You won't have one of > > > these > > > > > objects when you are not run from the external shell. > > > > > > Actually, this is no longer true. I always create a MojoShellConnection in > > > RendererMain so that I can install a listener before getting a mojo shell > > > handle. > > > > OK in this case this object needs to have a "HasApplication()" or "IsBound()" > > method that returns whether or not the application request has been bound yet, > > and this code needs to check it otherwise you'll be hitting dchecks in tests. > > I discussed it with Fady. We think it is better to add a flag to renderer to > indicate if the mojo shell will be used. > And the MojoShellConnection will only be created, if the command line has the > flag. So in here, we can just need check > MojoShellConnection::Get(). WDYT? OK
On 2015/12/02 01:03:18, Ben Goodger (Google) wrote: > On 2015/12/02 00:44:48, Peng wrote: > > On 2015/12/01 22:52:04, Ben Goodger (Google) wrote: > > > On 2015/12/01 22:48:50, Fady Samuel wrote: > > > > On 2015/12/01 22:47:46, Fady Samuel wrote: > > > > > On 2015/12/01 22:46:58, Ben Goodger (Google) wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1472063007/diff/190001/content/renderer/rende... > > > > > > File content/renderer/render_widget.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1472063007/diff/190001/content/renderer/rende... > > > > > > content/renderer/render_widget.cc:1022: if > > > > > > (MojoShellConnection::Get()->GetApplication() && !use_software) { > > > > > > you should null check Get() before dereferencing. You won't have one > of > > > > these > > > > > > objects when you are not run from the external shell. > > > > > > > > Actually, this is no longer true. I always create a MojoShellConnection in > > > > RendererMain so that I can install a listener before getting a mojo shell > > > > handle. > > > > > > OK in this case this object needs to have a "HasApplication()" or > "IsBound()" > > > method that returns whether or not the application request has been bound > yet, > > > and this code needs to check it otherwise you'll be hitting dchecks in > tests. > > > > I discussed it with Fady. We think it is better to add a flag to renderer to > > indicate if the mojo shell will be used. > > And the MojoShellConnection will only be created, if the command line has the > > flag. So in here, we can just need check > > MojoShellConnection::Get(). WDYT? > > OK Done. PTAL. Thanks.
https://codereview.chromium.org/1472063007/diff/210001/content/public/common/... File content/public/common/BUILD.gn (right): https://codereview.chromium.org/1472063007/diff/210001/content/public/common/... content/public/common/BUILD.gn:69: ":mojo_shell_client", Don't put this in public because consumers of this library may not have this defined and so it may end up being inconsistent across files. https://codereview.chromium.org/1472063007/diff/210001/content/public/common/... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/1472063007/diff/210001/content/public/common/... content/public/common/content_switches.cc:988: // Enable the Mojo shell connection in renderers. Move to content/common. https://codereview.chromium.org/1472063007/diff/210001/content/public/common/... File content/public/common/content_switches.h (right): https://codereview.chromium.org/1472063007/diff/210001/content/public/common/... content/public/common/content_switches.h:305: CONTENT_EXPORT extern const char kEnableMojoShellConnection[]; Move this to content/common. https://codereview.chromium.org/1472063007/diff/210001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1472063007/diff/210001/content/renderer/rende... content/renderer/render_widget.cc:1023: MojoShellConnection::Get()->GetApplication() && !use_software) { Actually, you don't need GetApplication anymore I think. In fact, that might be broken in some cases. https://codereview.chromium.org/1472063007/diff/210001/content/renderer/rende... File content/renderer/render_widget_mus_connection.cc (right): https://codereview.chromium.org/1472063007/diff/210001/content/renderer/rende... content/renderer/render_widget_mus_connection.cc:40: // messages until we have an associated RenderWidget. nit: remove this TODO.
lgtm with fady's comments addressed
https://codereview.chromium.org/1472063007/diff/210001/content/public/common/... File content/public/common/BUILD.gn (right): https://codereview.chromium.org/1472063007/diff/210001/content/public/common/... content/public/common/BUILD.gn:69: ":mojo_shell_client", On 2015/12/02 16:20:33, Fady Samuel wrote: > Don't put this in public because consumers of this library may not have this > defined and so it may end up being inconsistent across files. Done. https://codereview.chromium.org/1472063007/diff/210001/content/public/common/... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/1472063007/diff/210001/content/public/common/... content/public/common/content_switches.cc:988: // Enable the Mojo shell connection in renderers. On 2015/12/02 16:20:33, Fady Samuel wrote: > Move to content/common. Done. https://codereview.chromium.org/1472063007/diff/210001/content/public/common/... File content/public/common/content_switches.h (right): https://codereview.chromium.org/1472063007/diff/210001/content/public/common/... content/public/common/content_switches.h:305: CONTENT_EXPORT extern const char kEnableMojoShellConnection[]; On 2015/12/02 16:20:33, Fady Samuel wrote: > Move this to content/common. Removed the #if defined(). Done https://codereview.chromium.org/1472063007/diff/210001/content/renderer/rende... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1472063007/diff/210001/content/renderer/rende... content/renderer/render_widget.cc:1023: MojoShellConnection::Get()->GetApplication() && !use_software) { On 2015/12/02 16:20:33, Fady Samuel wrote: > Actually, you don't need GetApplication anymore I think. In fact, that might be > broken in some cases. Done. https://codereview.chromium.org/1472063007/diff/210001/content/renderer/rende... File content/renderer/render_widget_mus_connection.cc (right): https://codereview.chromium.org/1472063007/diff/210001/content/renderer/rende... content/renderer/render_widget_mus_connection.cc:40: // messages until we have an associated RenderWidget. On 2015/12/02 16:20:33, Fady Samuel wrote: > nit: remove this TODO. Done.
LGTM modulo one comment to address. Thanks! https://codereview.chromium.org/1472063007/diff/230001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1472063007/diff/230001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:126: #include "content/common/mojo/mojo_shell_connection_impl.h" This should also be behind an if defined...
https://codereview.chromium.org/1472063007/diff/230001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/1472063007/diff/230001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:126: #include "content/common/mojo/mojo_shell_connection_impl.h" On 2015/12/02 16:42:11, Fady Samuel wrote: > This should also be behind an if defined... Done.
The CQ bit was checked by penghuang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ben@chromium.org, fsamuel@chromium.org Link to the patchset: https://codereview.chromium.org/1472063007/#ps250001 (title: "Address issues.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1472063007/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1472063007/250001
Message was sent while issue was closed.
Description was changed from ========== mustash: enable GPU Compositing in renderer. By using mus::OutputSurface as the cc output surface to allow renderer to draw on a mus::Window directly. Known issues: Input doesn't work. Renderer position isn't correct. Resizing is not smooth. TEST=./out/mandoline_Debug/mojo_runner mojo:example_main --use-mus-in-renderer --use-zero-copy BUG=551250 TBR=jam@chromium.org ========== to ========== mustash: enable GPU Compositing in renderer. By using mus::OutputSurface as the cc output surface to allow renderer to draw on a mus::Window directly. Known issues: Input doesn't work. Renderer position isn't correct. Resizing is not smooth. TEST=./out/mandoline_Debug/mojo_runner mojo:example_main --use-mus-in-renderer --use-zero-copy BUG=551250 TBR=jam@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #14 (id:250001)
Message was sent while issue was closed.
Description was changed from ========== mustash: enable GPU Compositing in renderer. By using mus::OutputSurface as the cc output surface to allow renderer to draw on a mus::Window directly. Known issues: Input doesn't work. Renderer position isn't correct. Resizing is not smooth. TEST=./out/mandoline_Debug/mojo_runner mojo:example_main --use-mus-in-renderer --use-zero-copy BUG=551250 TBR=jam@chromium.org ========== to ========== mustash: enable GPU Compositing in renderer. By using mus::OutputSurface as the cc output surface to allow renderer to draw on a mus::Window directly. Known issues: Input doesn't work. Renderer position isn't correct. Resizing is not smooth. TEST=./out/mandoline_Debug/mojo_runner mojo:example_main --use-mus-in-renderer --use-zero-copy BUG=551250 TBR=jam@chromium.org Committed: https://crrev.com/28a5fa2c08f88309e825b46b3b15a345044c712d Cr-Commit-Position: refs/heads/master@{#362745} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/28a5fa2c08f88309e825b46b3b15a345044c712d Cr-Commit-Position: refs/heads/master@{#362745}
Message was sent while issue was closed.
https://codereview.chromium.org/1472063007/diff/250001/content/public/common/... File content/public/common/content_switches.h (right): https://codereview.chromium.org/1472063007/diff/250001/content/public/common/... content/public/common/content_switches.h:304: CONTENT_EXPORT extern const char kEnableMojoShellConnection[]; please see the comment 2 lines below, this needs to be put above sorted
Message was sent while issue was closed.
https://codereview.chromium.org/1472063007/diff/250001/content/public/common/... File content/public/common/content_switches.h (right): https://codereview.chromium.org/1472063007/diff/250001/content/public/common/... content/public/common/content_switches.h:304: CONTENT_EXPORT extern const char kEnableMojoShellConnection[]; On 2015/12/10 23:24:56, jam wrote: > please see the comment 2 lines below, this needs to be put above sorted In origin cl, "#if defined(MOJO_SHELL_CLIENT)" is around this switch. But the #ifdef was removed during reviewing, but I forgot moving it to the right position. Sorry about that. |
