Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2875753002:
Implement aura::WindowPortMus::CreateCompositorFrameSink() (Closed)
|
Created:
3 years, 7 months ago by Peng Modified:
3 years, 6 months ago CC:
Aaron Boodman, abarth-chromium, chromium-reviews, darin (slow to review), kalyank, qsr+mojo_chromium.org, rjkroege, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement aura::WindowPortMus::CreateCompositorFrameSink()
This CL implements aura::WindowPortMus::CreateCompositorFrameSink().
Exo will use it in Mushrome and Mustash to create a frame sink
and use it to submit frames.
BUG=720600
Review-Url: https://codereview.chromium.org/2875753002
Cr-Original-Commit-Position: refs/heads/master@{#478287}
Committed: https://chromium.googlesource.com/chromium/src/+/810bd6f44ea3be0e7ff4522190cf2e4924769e17
Review-Url: https://codereview.chromium.org/2875753002
Cr-Commit-Position: refs/heads/master@{#478355}
Committed: https://chromium.googlesource.com/chromium/src/+/7d3b44d7d03614dd15fda5e81df050b0e618ad2b
Patch Set 1 #Patch Set 2 : WIP #Patch Set 3 : WIP #Patch Set 4 : WIP #Patch Set 5 : WIP #Patch Set 6 : WIP #
Total comments: 9
Patch Set 7 : WIP #Patch Set 8 : Rebase #
Total comments: 20
Patch Set 9 : Address review issues #Patch Set 10 : Rebase #Patch Set 11 : Rebase #Patch Set 12 : WIP #
Total comments: 12
Patch Set 13 : Address review issues #Patch Set 14 : Address review issues #Patch Set 15 : Address review issues #
Total comments: 3
Patch Set 16 : Rebase #Patch Set 17 : Fix a problem #Patch Set 18 : Rebase #Patch Set 19 : Fix a build error #Patch Set 20 : Fix unittests #
Total comments: 9
Patch Set 21 : Address review issues. #
Total comments: 18
Patch Set 22 : Update #
Total comments: 19
Patch Set 23 : Address review issues #
Total comments: 14
Patch Set 24 : Address review issues. #
Total comments: 6
Patch Set 25 : Address review issues #Patch Set 26 : Rebase #
Total comments: 4
Patch Set 27 : Address the review issue #Patch Set 28 : Fix build problem #Messages
Total messages: 125 (69 generated)
Description was changed from ========== WIP WIP BUG= ========== to ========== Implement aura::WindowPortMus::CreateCompositorFrameSink() BUG=720600 ==========
The CQ bit was checked by penghuang@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...
Description was changed from ========== Implement aura::WindowPortMus::CreateCompositorFrameSink() BUG=720600 ========== to ========== Implement aura::WindowPortMus::CreateCompositorFrameSink() This CL implements aura::WindowPortMus::CreateCompositorFrameSink(). Exo will use it in Mushrome and Mustash to create a frame sink and use it to submit frames. BUG=720600 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
penghuang@chromium.org changed reviewers: + fsamuel@chromium.org
Hi Fady, PTAL. Thanks.
I'm not sure I understand this control flow. I suspect this code breaks surface synchronization too. https://codereview.chromium.org/2875753002/diff/100001/ui/aura/mus/mus_contex... File ui/aura/mus/mus_context_factory.cc (right): https://codereview.chromium.org/2875753002/diff/100001/ui/aura/mus/mus_contex... ui/aura/mus/mus_context_factory.cc:53: if (!shared_main_thread_context_provider_) { Maybe this can be a separate CL? https://codereview.chromium.org/2875753002/diff/100001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2875753002/diff/100001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:354: if (!ref_factory_) what calls this? https://codereview.chromium.org/2875753002/diff/100001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:614: void WindowPortMus::OnSurfaceChanged(const cc::LocalSurfaceId& local_surface_id, I'm confused about when this happens then...
https://codereview.chromium.org/2875753002/diff/100001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2875753002/diff/100001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:564: window_tree_client_->GetFrameSinkId(server_id()); Why do we need this? https://codereview.chromium.org/2875753002/diff/100001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:573: frame_sink->SetSurfaceChangedCallback( What is the purpose of this callback?
https://codereview.chromium.org/2875753002/diff/100001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2875753002/diff/100001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:354: if (!ref_factory_) On 2017/05/16 10:26:41, Fady Samuel wrote: > what calls this? window_->layer()->SetShowPrimarySurface(surface_info, ref_factory_); It needs a SurfaceReferenceFactory. https://codereview.chromium.org/2875753002/diff/100001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:564: window_tree_client_->GetFrameSinkId(server_id()); On 2017/05/16 10:28:03, Fady Samuel wrote: > Why do we need this? Right now, WS doesn't send frame sink id to client for a local window. So this CL addes a new mojo IPC GetFrameSinkId() to will request the WS send the frame sink id to client via OnFrameSinkIdAllocated() method. And then the WindowPortMus::SetFrameSinkIdFromServer() will be called, and then we can set aura::Window's ui::Layer's Primary surface with the frame sink id and local surface id. https://codereview.chromium.org/2875753002/diff/100001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:573: frame_sink->SetSurfaceChangedCallback( On 2017/05/16 10:28:03, Fady Samuel wrote: > What is the purpose of this callback? The ClientCompositorFrameSink will generate new local surface id, when the frame size is changed. So we need this callback for being notified for the new size and local surface id. https://codereview.chromium.org/2875753002/diff/100001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:614: void WindowPortMus::OnSurfaceChanged(const cc::LocalSurfaceId& local_surface_id, On 2017/05/16 10:26:42, Fady Samuel wrote: > I'm confused about when this happens then... See line 103 at https://codereview.chromium.org/2875753002/diff/100001/services/ui/public/cpp...
https://codereview.chromium.org/2875753002/diff/140001/services/ui/public/cpp... File services/ui/public/cpp/client_compositor_frame_sink.h (right): https://codereview.chromium.org/2875753002/diff/140001/services/ui/public/cpp... services/ui/public/cpp/client_compositor_frame_sink.h:40: void SetSurfaceChangedCallback(const SurfaceChangedCallback& callback); This is fundamentally not compatible with surface synchronization. Please rely on the window server's OnWindowSurfaceChanged message. https://codereview.chromium.org/2875753002/diff/140001/services/ui/public/int... File services/ui/public/interfaces/window_tree.mojom (right): https://codereview.chromium.org/2875753002/diff/140001/services/ui/public/int... services/ui/public/interfaces/window_tree.mojom:321: GetFrameSinkId(uint32 window_id); Who calls this? The embedder? And then the embeddee gets OnFrameSInkIdAllocated? https://codereview.chromium.org/2875753002/diff/140001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2875753002/diff/140001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:37: class StubSurfaceReferenceFactory : public cc::SurfaceReferenceFactory { Remove this. https://codereview.chromium.org/2875753002/diff/140001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:327: UpdateClientSurfaceEmbedder(); Undo this change. Always use ClientSurfaceEmbedder. https://codereview.chromium.org/2875753002/diff/140001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:553: base::Bind(&WindowPortMus::OnSurfaceChanged, weak_factory_.GetWeakPtr())); This is fundamentally not compatible with surface synchronization. Rely on the message from the window server instead? https://codereview.chromium.org/2875753002/diff/140001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:567: if (embeds_surface && !window_tree_client_->enable_surface_synchronization_) Undo this? https://codereview.chromium.org/2875753002/diff/140001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:593: void WindowPortMus::OnSurfaceChanged(const cc::LocalSurfaceId& local_surface_id, Remove this. https://codereview.chromium.org/2875753002/diff/140001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.h (right): https://codereview.chromium.org/2875753002/diff/140001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.h:268: void OnSurfaceChanged(const cc::LocalSurfaceId& local_surface_id, Delete this. https://codereview.chromium.org/2875753002/diff/140001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.h:292: scoped_refptr<cc::SurfaceReferenceFactory> ref_factory_; Delete this. https://codereview.chromium.org/2875753002/diff/140001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.h:294: base::WeakPtrFactory<WindowPortMus> weak_factory_; Delete this.
https://codereview.chromium.org/2875753002/diff/140001/services/ui/public/cpp... File services/ui/public/cpp/client_compositor_frame_sink.h (right): https://codereview.chromium.org/2875753002/diff/140001/services/ui/public/cpp... services/ui/public/cpp/client_compositor_frame_sink.h:40: void SetSurfaceChangedCallback(const SurfaceChangedCallback& callback); On 2017/05/18 16:48:50, Fady Samuel wrote: > This is fundamentally not compatible with surface synchronization. Please rely > on the window server's OnWindowSurfaceChanged message. Done. https://codereview.chromium.org/2875753002/diff/140001/services/ui/public/int... File services/ui/public/interfaces/window_tree.mojom (right): https://codereview.chromium.org/2875753002/diff/140001/services/ui/public/int... services/ui/public/interfaces/window_tree.mojom:321: GetFrameSinkId(uint32 window_id); On 2017/05/18 16:48:51, Fady Samuel wrote: > Who calls this? The embedder? And then the embeddee gets OnFrameSInkIdAllocated? Removed, since the SurfaceInfo will be send to client via OnWindowSurfaceChanged(). https://codereview.chromium.org/2875753002/diff/140001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2875753002/diff/140001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:37: class StubSurfaceReferenceFactory : public cc::SurfaceReferenceFactory { On 2017/05/18 16:48:51, Fady Samuel wrote: > Remove this. Done. https://codereview.chromium.org/2875753002/diff/140001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:327: UpdateClientSurfaceEmbedder(); On 2017/05/18 16:48:51, Fady Samuel wrote: > Undo this change. Always use ClientSurfaceEmbedder. Done. https://codereview.chromium.org/2875753002/diff/140001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:553: base::Bind(&WindowPortMus::OnSurfaceChanged, weak_factory_.GetWeakPtr())); On 2017/05/18 16:48:51, Fady Samuel wrote: > This is fundamentally not compatible with surface synchronization. Rely on the > message from the window server instead? Done. https://codereview.chromium.org/2875753002/diff/140001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:567: if (embeds_surface && !window_tree_client_->enable_surface_synchronization_) On 2017/05/18 16:48:51, Fady Samuel wrote: > Undo this? Done. https://codereview.chromium.org/2875753002/diff/140001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:593: void WindowPortMus::OnSurfaceChanged(const cc::LocalSurfaceId& local_surface_id, On 2017/05/18 16:48:51, Fady Samuel wrote: > Remove this. Done. https://codereview.chromium.org/2875753002/diff/140001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.h (right): https://codereview.chromium.org/2875753002/diff/140001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.h:268: void OnSurfaceChanged(const cc::LocalSurfaceId& local_surface_id, On 2017/05/18 16:48:51, Fady Samuel wrote: > Delete this. Done. https://codereview.chromium.org/2875753002/diff/140001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.h:292: scoped_refptr<cc::SurfaceReferenceFactory> ref_factory_; On 2017/05/18 16:48:51, Fady Samuel wrote: > Delete this. Done. https://codereview.chromium.org/2875753002/diff/140001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.h:294: base::WeakPtrFactory<WindowPortMus> weak_factory_; On 2017/05/18 16:48:51, Fady Samuel wrote: > Delete this. Done.
The CQ bit was checked by penghuang@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...
https://codereview.chromium.org/2875753002/diff/220001/services/ui/ws/window_... File services/ui/ws/window_server.cc (right): https://codereview.chromium.org/2875753002/diff/220001/services/ui/ws/window_... services/ui/ws/window_server.cc:857: HandleTemporaryReferenceForNewSurface(surface_info.id(), window); This breaks right? If the embedder is top most (Ash?) then this code fails? https://codereview.chromium.org/2875753002/diff/220001/services/ui/ws/window_... File services/ui/ws/window_tree.cc (right): https://codereview.chromium.org/2875753002/diff/220001/services/ui/ws/window_... services/ui/ws/window_tree.cc:965: if (!IsWindowKnown(window, &client_window_id)) Well, I think the conditions below are still valid if the window has a parent, right? https://codereview.chromium.org/2875753002/diff/220001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2875753002/diff/220001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:499: WindowPortMus::CreateCompositorFrameSink() { This is super confusing. We have a RequestCompositorFrameSink, and a CreateCompositorFrameSink. Could we simply use RequestCompositorFrameSink with two optional parameters? https://codereview.chromium.org/2875753002/diff/220001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:509: const bool enable_surface_synchronization = false; Surface synchronization is actually a requirement here. Each CompositorFrameSink is a separate messagepipe but you want to update all of them atomically, so this is incorrect.
https://codereview.chromium.org/2875753002/diff/220001/services/ui/ws/window_... File services/ui/ws/window_server.cc (right): https://codereview.chromium.org/2875753002/diff/220001/services/ui/ws/window_... services/ui/ws/window_server.cc:857: HandleTemporaryReferenceForNewSurface(surface_info.id(), window); On 2017/05/18 20:02:01, Fady Samuel wrote: > This breaks right? If the embedder is top most (Ash?) then this code fails? I tested it with mushrome, mustash, and mustash + enable_surface_sync. I didn't see any problem. I don't understand why it breaks. Could you please explain it? https://codereview.chromium.org/2875753002/diff/220001/services/ui/ws/window_... File services/ui/ws/window_tree.cc (right): https://codereview.chromium.org/2875753002/diff/220001/services/ui/ws/window_... services/ui/ws/window_tree.cc:965: if (!IsWindowKnown(window, &client_window_id)) On 2017/05/18 20:02:01, Fady Samuel wrote: > Well, I think the conditions below are still valid if the window has a parent, > right? With this change, the client can start submitting frames before adding it to a parent window. And exo does it. https://codereview.chromium.org/2875753002/diff/220001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2875753002/diff/220001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:499: WindowPortMus::CreateCompositorFrameSink() { On 2017/05/18 20:02:02, Fady Samuel wrote: > This is super confusing. We have a RequestCompositorFrameSink, and a > CreateCompositorFrameSink. Could we simply use RequestCompositorFrameSink with > two optional parameters? Done https://codereview.chromium.org/2875753002/diff/220001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:509: const bool enable_surface_synchronization = false; On 2017/05/18 20:02:02, Fady Samuel wrote: > Surface synchronization is actually a requirement here. Each CompositorFrameSink > is a separate messagepipe but you want to update all of them atomically, so this > is incorrect. As my understanding, when the the CompositorFrameSink allocates a new local surface id, and sends a frame with the id, the viz will notify ws a new surface is created, and then WindowTreeClient::OnWindowSurfaceChanged() will be called, and then we will use the new surface id for submitting up level compositor frame. So I don't see why we need enable surface synchronization. thought?
https://codereview.chromium.org/2875753002/diff/220001/services/ui/ws/window_... File services/ui/ws/window_server.cc (right): https://codereview.chromium.org/2875753002/diff/220001/services/ui/ws/window_... services/ui/ws/window_server.cc:857: HandleTemporaryReferenceForNewSurface(surface_info.id(), window); On 2017/05/18 20:50:59, Peng wrote: > On 2017/05/18 20:02:01, Fady Samuel wrote: > > This breaks right? If the embedder is top most (Ash?) then this code fails? > > I tested it with mushrome, mustash, and mustash + enable_surface_sync. I didn't > see any problem. I don't understand why it breaks. Could you please explain it? Do we assign an owner to the temporary reference? Is ClaimTemporaryReference getting called? https://codereview.chromium.org/2875753002/diff/220001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2875753002/diff/220001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:509: const bool enable_surface_synchronization = false; On 2017/05/18 20:50:59, Peng wrote: > On 2017/05/18 20:02:02, Fady Samuel wrote: > > Surface synchronization is actually a requirement here. Each > CompositorFrameSink > > is a separate messagepipe but you want to update all of them atomically, so > this > > is incorrect. > > As my understanding, when the the CompositorFrameSink allocates a new local > surface id, and sends a frame with the id, the viz will notify ws a new surface > is created, and then WindowTreeClient::OnWindowSurfaceChanged() will be called, > and then we will use the new surface id for submitting up level compositor > frame. So I don't see why we need enable surface synchronization. thought? Without surface synchronization, there is no guarantee that all the CompositorFrames from all the exo subsurfaces will be received and aggregated in time for the next display frame.
https://codereview.chromium.org/2875753002/diff/220001/services/ui/ws/window_... File services/ui/ws/window_server.cc (right): https://codereview.chromium.org/2875753002/diff/220001/services/ui/ws/window_... services/ui/ws/window_server.cc:857: HandleTemporaryReferenceForNewSurface(surface_info.id(), window); On 2017/05/18 20:56:22, Fady Samuel wrote: > On 2017/05/18 20:50:59, Peng wrote: > > On 2017/05/18 20:02:01, Fady Samuel wrote: > > > This breaks right? If the embedder is top most (Ash?) then this code fails? > > > > I tested it with mushrome, mustash, and mustash + enable_surface_sync. I > didn't > > see any problem. I don't understand why it breaks. Could you please explain > it? > > Do we assign an owner to the temporary reference? Is ClaimTemporaryReference > getting called? Done. https://codereview.chromium.org/2875753002/diff/220001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2875753002/diff/220001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:509: const bool enable_surface_synchronization = false; On 2017/05/18 20:56:22, Fady Samuel wrote: > On 2017/05/18 20:50:59, Peng wrote: > > On 2017/05/18 20:02:02, Fady Samuel wrote: > > > Surface synchronization is actually a requirement here. Each > > CompositorFrameSink > > > is a separate messagepipe but you want to update all of them atomically, so > > this > > > is incorrect. > > > > As my understanding, when the the CompositorFrameSink allocates a new local > > surface id, and sends a frame with the id, the viz will notify ws a new > surface > > is created, and then WindowTreeClient::OnWindowSurfaceChanged() will be > called, > > and then we will use the new surface id for submitting up level compositor > > frame. So I don't see why we need enable surface synchronization. thought? > > > Without surface synchronization, there is no guarantee that all the > CompositorFrames from all the exo subsurfaces will be received and aggregated in > time for the next display frame. Done.
Patchset #15 (id:280001) has been deleted
https://codereview.chromium.org/2875753002/diff/300001/services/ui/ws/window_... File services/ui/ws/window_server.cc (right): https://codereview.chromium.org/2875753002/diff/300001/services/ui/ws/window_... services/ui/ws/window_server.cc:865: window_tree_id = window->parent()->id().client_id; So we agreed to undo this change?
https://codereview.chromium.org/2875753002/diff/300001/services/ui/ws/window_... File services/ui/ws/window_server.cc (right): https://codereview.chromium.org/2875753002/diff/300001/services/ui/ws/window_... services/ui/ws/window_server.cc:865: window_tree_id = window->parent()->id().client_id; On 2017/05/25 20:58:03, Fady Samuel wrote: > So we agreed to undo this change? The origin code will not send the new surface id to the window tree client, if the window is not parented. We need this change, so the client can know the surface id, and use it later.
OK this seems reasonable once the bots are happy.
The CQ bit was checked by penghuang@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 penghuang@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 penghuang@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...
https://codereview.chromium.org/2875753002/diff/300001/services/ui/ws/window_... File services/ui/ws/window_server.cc (right): https://codereview.chromium.org/2875753002/diff/300001/services/ui/ws/window_... services/ui/ws/window_server.cc:865: window_tree_id = window->parent()->id().client_id; On 2017/05/25 23:10:39, Peng wrote: > On 2017/05/25 20:58:03, Fady Samuel wrote: > > So we agreed to undo this change? > > The origin code will not send the new surface id to the window tree client, if > the window is not parented. We need this change, so the client can know the > surface id, and use it later. I just realized, we should never use the parent window's client id. It is because, for an embedded window, the window's id is always allocated within parent window tree, so we should just use the window's client id. But for a "local window"'s parent is an embedded window, we cannot use parent window's client id. It will notify a wrong tree.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by penghuang@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 penghuang@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_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by penghuang@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by penghuang@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Hi Fady, I fixed build issues. PTAL. Thanks.
The CQ bit was checked by penghuang@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...
Looking good. Please rebase. https://codereview.chromium.org/2875753002/diff/400001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2875753002/diff/400001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:479: DCHECK_NE(window_mus_type(), WindowMusType::TOP_LEVEL_IN_WM); I think you need to rebase...there's another type I added recently.
sadrul@chromium.org changed reviewers: + sadrul@chromium.org
https://codereview.chromium.org/2875753002/diff/400001/ui/aura/window.cc File ui/aura/window.cc (right): https://codereview.chromium.org/2875753002/diff/400001/ui/aura/window.cc#newc... ui/aura/window.cc:996: nullptr, these extra params don't seem necessary? Can WindowPortMus just get the gmb-manager from aura::Env itself? (and just remove the context_provider param since it's always null?)
https://codereview.chromium.org/2875753002/diff/400001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2875753002/diff/400001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:479: DCHECK_NE(window_mus_type(), WindowMusType::TOP_LEVEL_IN_WM); On 2017/05/26 20:19:28, Fady Samuel wrote: > I think you need to rebase...there's another type I added recently. As offline discussion, we don't need add another DCHECK here. https://codereview.chromium.org/2875753002/diff/400001/ui/aura/window.cc File ui/aura/window.cc (right): https://codereview.chromium.org/2875753002/diff/400001/ui/aura/window.cc#newc... ui/aura/window.cc:996: nullptr, On 2017/05/26 20:22:25, sadrul wrote: > these extra params don't seem necessary? Can WindowPortMus just get the > gmb-manager from aura::Env itself? (and just remove the context_provider param > since it's always null?) I added this two args, because we want to get rid of WindowPortMus::RequestCompositorFrameSink(). See [1] for detail. [1] https://codereview.chromium.org/2875753002/diff2/220001:240001/ui/aura/mus/wi...
penghuang@chromium.org changed reviewers: + reveman@chromium.org, sky@chromium.org
reveman@chromium.org: Please review changes in //components/exo sky@chromium.org: Please review changes in //services/ui Hi David and Scott, PTAL. Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2875753002/diff/400001/services/ui/ws/window_... File services/ui/ws/window_server.cc (right): https://codereview.chromium.org/2875753002/diff/400001/services/ui/ws/window_... services/ui/ws/window_server.cc:859: // We always use the client_id of window's id (even for embedded window), How about, 'We always use the owner of the window...' https://codereview.chromium.org/2875753002/diff/400001/services/ui/ws/window_... File services/ui/ws/window_tree.cc (right): https://codereview.chromium.org/2875753002/diff/400001/services/ui/ws/window_... services/ui/ws/window_tree.cc:965: if (!IsWindowKnown(window, &client_window_id)) This means for embed points you send the change to both the owner and the embedded window. Is that expected? I would think you would only send it to one based on who originated the message.
https://codereview.chromium.org/2875753002/diff/400001/ui/aura/window.cc File ui/aura/window.cc (right): https://codereview.chromium.org/2875753002/diff/400001/ui/aura/window.cc#newc... ui/aura/window.cc:996: nullptr, On 2017/05/26 20:41:32, Peng wrote: > On 2017/05/26 20:22:25, sadrul wrote: > > these extra params don't seem necessary? Can WindowPortMus just get the > > gmb-manager from aura::Env itself? (and just remove the context_provider param > > since it's always null?) > > I added this two args, because we want to get rid of > WindowPortMus::RequestCompositorFrameSink(). See [1] for detail. > > [1] > https://codereview.chromium.org/2875753002/diff2/220001:240001/ui/aura/mus/wi... I see. I guess you need the ContextProvider param, but you still should not need the gmb-manager param. (i.e. using Env::context_factory() to get the gmb-manager should still work just OK)
Patchset #21 (id:420001) has been deleted
Patchset #21 (id:440001) has been deleted
https://codereview.chromium.org/2875753002/diff/460001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2875753002/diff/460001/components/exo/surface... components/exo/surface.cc:789: window_->SetBounds(gfx::Rect(window_->bounds().origin(), content_size_)); Why? https://codereview.chromium.org/2875753002/diff/460001/services/ui/ws/window_... File services/ui/ws/window_server.cc (right): https://codereview.chromium.org/2875753002/diff/460001/services/ui/ws/window_... services/ui/ws/window_server.cc:873: WindowTree* window_tree = GetTreeWithId(window->id().client_id); I think this is a reasonable change along with the unit test, maybe this can be a separate CL? https://codereview.chromium.org/2875753002/diff/460001/ui/aura/mus/mus_contex... File ui/aura/mus/mus_context_factory.cc (right): https://codereview.chromium.org/2875753002/diff/460001/ui/aura/mus/mus_contex... ui/aura/mus/mus_context_factory.cc:42: window_port->CreateCompositorFrameSinkWithContextAndBufferManager( This is a really long name? Can we just keep the name CreateCompositorFrameSink or RequestCompositorFrameSink? https://codereview.chromium.org/2875753002/diff/460001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2875753002/diff/460001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:111: bool generate_local_surface_id = window_mus_type() == WindowMusType::LOCAL; The parent should allocate the LocalSurfaceId. Let's not do this? https://codereview.chromium.org/2875753002/diff/460001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:297: void WindowPortMus::SetSurfaceInfoFromServer( This doesn't match our discussion. Let's keep the code path for local and embedded windows the same. https://codereview.chromium.org/2875753002/diff/460001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:556: void WindowPortMus::OnSurfaceChangedForLocalWindow( Please don't special case local windows? https://codereview.chromium.org/2875753002/diff/460001/ui/aura/mus/window_tre... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2875753002/diff/460001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:1385: window->SetSurfaceInfoFromServer(surface_info); Please restore this.
https://codereview.chromium.org/2875753002/diff/460001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2875753002/diff/460001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:111: bool generate_local_surface_id = window_mus_type() == WindowMusType::LOCAL; On 2017/05/30 18:36:42, Fady Samuel wrote: > The parent should allocate the LocalSurfaceId. Let's not do this? One comment here. Currently the client that initiates a bounds change on a window does *not* get OnWindowBoundsChanged(). We shouldn't change that as it'll mean the client will start getting unexpected OnWindowBoundsChanged(). The only way the client could deal with that is if we added extra information to OnWindowBoundsChanged() so that the client could know it initiated the change. I think this would effectively mean we need a OnWindowBoundsChangeCompleted() function is called in this case instead of OnChangeCompleted().
https://codereview.chromium.org/2875753002/diff/400001/services/ui/ws/window_... File services/ui/ws/window_server.cc (right): https://codereview.chromium.org/2875753002/diff/400001/services/ui/ws/window_... services/ui/ws/window_server.cc:859: // We always use the client_id of window's id (even for embedded window), On 2017/05/26 23:14:18, sky wrote: > How about, 'We always use the owner of the window...' Done. https://codereview.chromium.org/2875753002/diff/400001/services/ui/ws/window_... File services/ui/ws/window_tree.cc (right): https://codereview.chromium.org/2875753002/diff/400001/services/ui/ws/window_... services/ui/ws/window_tree.cc:965: if (!IsWindowKnown(window, &client_window_id)) On 2017/05/26 23:14:18, sky wrote: > This means for embed points you send the change to both the owner and the > embedded window. Is that expected? I would think you would only send it to one > based on who originated the message. For the embed points, we only call this function on parent tree. The function will not be called on the child tree. So the child tree will not be notified. https://codereview.chromium.org/2875753002/diff/460001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2875753002/diff/460001/components/exo/surface... components/exo/surface.cc:789: window_->SetBounds(gfx::Rect(window_->bounds().origin(), content_size_)); On 2017/05/30 18:36:41, Fady Samuel wrote: > Why? ClientSurfaceEmbedder will read window's size and set it to an extra layer. See [1] For cash, we will set the frame's size to window_->layer() in WindowPortLocal. And aura::Window will observe the window->layer()'s size changes and update itself. [1] https://cs.chromium.org/chromium/src/ui/aura/mus/client_surface_embedder.cc?q... https://codereview.chromium.org/2875753002/diff/460001/services/ui/ws/window_... File services/ui/ws/window_server.cc (right): https://codereview.chromium.org/2875753002/diff/460001/services/ui/ws/window_... services/ui/ws/window_server.cc:873: WindowTree* window_tree = GetTreeWithId(window->id().client_id); On 2017/05/30 18:36:41, Fady Samuel wrote: > I think this is a reasonable change along with the unit test, maybe this can be > a separate CL? The changes in services/ui/ws/window_tree_client_unittest.cc verify it I think. Please check. https://codereview.chromium.org/2875753002/diff/460001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2875753002/diff/460001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:111: bool generate_local_surface_id = window_mus_type() == WindowMusType::LOCAL; On 2017/05/30 18:36:42, Fady Samuel wrote: > The parent should allocate the LocalSurfaceId. Let's not do this? For a local window, actually we don't have the concept parent and child. I think you mean we should generate local surface id in WindowPortMus instead of ClientCompositorFrameSink, right? https://codereview.chromium.org/2875753002/diff/460001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:297: void WindowPortMus::SetSurfaceInfoFromServer( On 2017/05/30 18:36:41, Fady Samuel wrote: > This doesn't match our discussion. Let's keep the code path for local and > embedded windows the same. I thought to generate in ClientCompositorFrameSink may not work with surface synchronization. But after looking it carefully, I think it should work fine. So I thought it is better to avoid the extra OnWindowBoundsChanged IPC, and with the extra IPC, the exo cannot submit new frames immediately after resizing, it has to want the extra IPC. It makes code difficult to write. So why not make code simpler and more efficient?
https://codereview.chromium.org/2875753002/diff/460001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2875753002/diff/460001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:297: void WindowPortMus::SetSurfaceInfoFromServer( On 2017/05/30 19:46:28, Peng wrote: > On 2017/05/30 18:36:41, Fady Samuel wrote: > > This doesn't match our discussion. Let's keep the code path for local and > > embedded windows the same. > > I thought to generate in ClientCompositorFrameSink may not work with surface > synchronization. But after looking it carefully, I think it should work fine. So > I thought it is better to avoid the extra OnWindowBoundsChanged IPC, and with > the extra IPC, the exo cannot submit new frames immediately after resizing, it > has to want the extra IPC. It makes code difficult to write. So why not make > code simpler and more efficient? This introduces a special case for local windows so I'd argue this is introducing more complexity. Let's have fewer code paths.
https://codereview.chromium.org/2875753002/diff/460001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2875753002/diff/460001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:297: void WindowPortMus::SetSurfaceInfoFromServer( On 2017/05/30 19:46:28, Peng wrote: > On 2017/05/30 18:36:41, Fady Samuel wrote: > > This doesn't match our discussion. Let's keep the code path for local and > > embedded windows the same. > > I thought to generate in ClientCompositorFrameSink may not work with surface > synchronization. But after looking it carefully, I think it should work fine. So > I thought it is better to avoid the extra OnWindowBoundsChanged IPC, and with > the extra IPC, the exo cannot submit new frames immediately after resizing, it > has to want the extra IPC. It makes code difficult to write. So why not make > code simpler and more efficient? This introduces a special case for local windows so I'd argue this is introducing more complexity. Let's have fewer code paths.
https://codereview.chromium.org/2875753002/diff/460001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.cc (left): https://codereview.chromium.org/2875753002/diff/460001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:292: void WindowPortMus::SetPrimarySurfaceInfo(const cc::SurfaceInfo& surface_info) { Please don't delete this either.
Patchset #22 (id:480001) has been deleted
Patchset #22 (id:500001) has been deleted
https://codereview.chromium.org/2875753002/diff/460001/ui/aura/mus/mus_contex... File ui/aura/mus/mus_context_factory.cc (right): https://codereview.chromium.org/2875753002/diff/460001/ui/aura/mus/mus_contex... ui/aura/mus/mus_context_factory.cc:42: window_port->CreateCompositorFrameSinkWithContextAndBufferManager( On 2017/05/30 18:36:41, Fady Samuel wrote: > This is a really long name? Can we just keep the name CreateCompositorFrameSink > or RequestCompositorFrameSink? Done. https://codereview.chromium.org/2875753002/diff/460001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2875753002/diff/460001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:111: bool generate_local_surface_id = window_mus_type() == WindowMusType::LOCAL; On 2017/05/30 19:46:28, Peng wrote: > On 2017/05/30 18:36:42, Fady Samuel wrote: > > The parent should allocate the LocalSurfaceId. Let's not do this? > > For a local window, actually we don't have the concept parent and child. > I think you mean we should generate local surface id in WindowPortMus instead of > ClientCompositorFrameSink, right? Done. By Allocating local surface id in WindowPortMus. https://codereview.chromium.org/2875753002/diff/460001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:556: void WindowPortMus::OnSurfaceChangedForLocalWindow( On 2017/05/30 18:36:42, Fady Samuel wrote: > Please don't special case local windows? Done. By allocating local_surface_id in GetOrAllocateLocalSurfaceId(). https://codereview.chromium.org/2875753002/diff/460001/ui/aura/mus/window_tre... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2875753002/diff/460001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:1385: window->SetSurfaceInfoFromServer(surface_info); On 2017/05/30 18:36:42, Fady Samuel wrote: > Please restore this. As we discussed this offline. The SetPrimarySurfaceInfo is not used anymore. So it is better to have this change.
fsamuel@google.com changed reviewers: + fsamuel@google.com
not lgtm Let's focus on the newly discussed approach: 1. exo submits a single CompositorFrame 2. Each exo app has a WindowTreeHost and ash/browser embeds that. https://codereview.chromium.org/2875753002/diff/520001/ui/aura/mus/window_mus.h File ui/aura/mus/window_mus.h (right): https://codereview.chromium.org/2875753002/diff/520001/ui/aura/mus/window_mus... ui/aura/mus/window_mus.h:91: virtual void SetSurfaceInfoFromServer( restore name: SetFallbackSurfaceInfo...
On 2017/05/31 20:42:45, fsamuel wrote: > not lgtm > > Let's focus on the newly discussed approach: > > 1. exo submits a single CompositorFrame > 2. Each exo app has a WindowTreeHost and ash/browser embeds that. I am still not sure if this will work with mushrome (because in mushrome, we do not allow embedding a WindowTreeHost in another). It is entirely possible that exo aura windows expect to live in the same window-tree as the rest of the windows in ash, and [2] would break those assumptions. We may need to live with something like this CL until we sort out the details.
On 2017/05/31 20:52:50, sadrul wrote: > On 2017/05/31 20:42:45, fsamuel wrote: > > not lgtm > > > > Let's focus on the newly discussed approach: > > > > 1. exo submits a single CompositorFrame > > 2. Each exo app has a WindowTreeHost and ash/browser embeds that. > > I am still not sure if this will work with mushrome (because in mushrome, we do > not allow embedding a WindowTreeHost in another). It is entirely possible that > exo aura windows expect to live in the same window-tree as the rest of the > windows in ash, and [2] would break those assumptions. > > We may need to live with something like this CL until we sort out the details. Actually, the more I think about it, the more I am convinced we cannot do [2] for mushrome. exo does a fair amount of event sniffing from the ash window-tree, and [2] would break that.
On 2017/05/31 20:55:59, sadrul wrote: > On 2017/05/31 20:52:50, sadrul wrote: > > On 2017/05/31 20:42:45, fsamuel wrote: > > > not lgtm > > > > > > Let's focus on the newly discussed approach: > > > > > > 1. exo submits a single CompositorFrame > > > 2. Each exo app has a WindowTreeHost and ash/browser embeds that. > > > > I am still not sure if this will work with mushrome (because in mushrome, we > do > > not allow embedding a WindowTreeHost in another). It is entirely possible that > > exo aura windows expect to live in the same window-tree as the rest of the > > windows in ash, and [2] would break those assumptions. > > > > We may need to live with something like this CL until we sort out the details. > > Actually, the more I think about it, the more I am convinced we cannot do [2] > for mushrome. exo does a fair amount of event sniffing from the ash window-tree, > and [2] would break that. How about we fix that then instead?
On 2017/05/31 21:03:42, Fady Samuel wrote: > On 2017/05/31 20:55:59, sadrul wrote: > > On 2017/05/31 20:52:50, sadrul wrote: > > > On 2017/05/31 20:42:45, fsamuel wrote: > > > > not lgtm > > > > > > > > Let's focus on the newly discussed approach: > > > > > > > > 1. exo submits a single CompositorFrame > > > > 2. Each exo app has a WindowTreeHost and ash/browser embeds that. > > > > > > I am still not sure if this will work with mushrome (because in mushrome, we > > do > > > not allow embedding a WindowTreeHost in another). It is entirely possible > that > > > exo aura windows expect to live in the same window-tree as the rest of the > > > windows in ash, and [2] would break those assumptions. > > > > > > We may need to live with something like this CL until we sort out the > details. > > > > Actually, the more I think about it, the more I am convinced we cannot do [2] > > for mushrome. exo does a fair amount of event sniffing from the ash > window-tree, > > and [2] would break that. > > How about we fix that then instead? As discussed offline, that is the plan. But that requires a fair amount of work, and until then, it would make sense to me to have a solution we can use.
fsamuel@chromium.org changed reviewers: + kylechar@chromium.org
+kylechar for opinion on surface references.
On 2017/06/01 17:22:04, Fady Samuel wrote: > +kylechar for opinion on surface references. Hi Fady, As our discussion yesterday, we will still need create CFS from an local aura::Window for the exo shell surface. So we still need this CL. Could we land it? Thanks.
https://codereview.chromium.org/2875753002/diff/520001/services/ui/ws/window_... File services/ui/ws/window_server.cc (right): https://codereview.chromium.org/2875753002/diff/520001/services/ui/ws/window_... services/ui/ws/window_server.cc:869: HandleTemporaryReferenceForNewSurface(surface_info.id(), window); There is a heuristic here to find the embedder where it looks for the first window with a different client_id. That breaks if the client_id doesn't change for the embedder, so would need to be fixed somehow?
https://codereview.chromium.org/2875753002/diff/520001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2875753002/diff/520001/components/exo/surface... components/exo/surface.cc:789: window_->SetBounds(gfx::Rect(window_->bounds().origin(), content_size_)); Please explain why we need this in a comment? https://codereview.chromium.org/2875753002/diff/520001/components/viz/client/... File components/viz/client/client_compositor_frame_sink.cc (right): https://codereview.chromium.org/2875753002/diff/520001/components/viz/client/... components/viz/client/client_compositor_frame_sink.cc:35: ClientCompositorFrameSink::GetWeakPtr() { DCHECK_CALLED_ON_VALID_THREAD too? https://codereview.chromium.org/2875753002/diff/520001/ui/aura/mus/window_mus.h File ui/aura/mus/window_mus.h (right): https://codereview.chromium.org/2875753002/diff/520001/ui/aura/mus/window_mus... ui/aura/mus/window_mus.h:91: virtual void SetSurfaceInfoFromServer( On 2017/05/31 20:42:44, fsamuel wrote: > restore name: SetFallbackSurfaceInfo... Please restore name https://codereview.chromium.org/2875753002/diff/520001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2875753002/diff/520001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:280: return local_surface_id_; Do we need this change? https://codereview.chromium.org/2875753002/diff/520001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:297: frame_sink_for_local_window_->SetLocalSurfaceId(local_surface_id_); Just call this compositor_frame_sink_. No point in mentioning this is for local windows. Let's always keep a weak ptr for consistency? https://codereview.chromium.org/2875753002/diff/520001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:312: UpdatePrimarySurfaceInfo(); Do we need this change? https://codereview.chromium.org/2875753002/diff/520001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:515: DCHECK_EQ(window_mus_type(), WindowMusType::LOCAL); Can we just have a single code path? Let's not special case local windows. https://codereview.chromium.org/2875753002/diff/520001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:528: // mus, so don't implement it now. Just say this is only used by WindowPortLocal in unit tests.
https://codereview.chromium.org/2875753002/diff/520001/services/ui/ws/window_... File services/ui/ws/window_server.cc (right): https://codereview.chromium.org/2875753002/diff/520001/services/ui/ws/window_... services/ui/ws/window_server.cc:869: HandleTemporaryReferenceForNewSurface(surface_info.id(), window); On 2017/06/07 17:18:48, kylechar wrote: > There is a heuristic here to find the embedder where it looks for the first > window with a different client_id. That breaks if the client_id doesn't change > for the embedder, so would need to be fixed somehow? It looks like the heuristic is broken anyways. I'll take a closer look but I don't think there is anything you need to here.
https://codereview.chromium.org/2875753002/diff/520001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2875753002/diff/520001/components/exo/surface... components/exo/surface.cc:789: window_->SetBounds(gfx::Rect(window_->bounds().origin(), content_size_)); On 2017/06/07 17:36:26, Fady Samuel wrote: > Please explain why we need this in a comment? Done. https://codereview.chromium.org/2875753002/diff/520001/components/viz/client/... File components/viz/client/client_compositor_frame_sink.cc (right): https://codereview.chromium.org/2875753002/diff/520001/components/viz/client/... components/viz/client/client_compositor_frame_sink.cc:35: ClientCompositorFrameSink::GetWeakPtr() { On 2017/06/07 17:36:26, Fady Samuel wrote: > DCHECK_CALLED_ON_VALID_THREAD too? Done. https://codereview.chromium.org/2875753002/diff/520001/ui/aura/mus/window_mus.h File ui/aura/mus/window_mus.h (right): https://codereview.chromium.org/2875753002/diff/520001/ui/aura/mus/window_mus... ui/aura/mus/window_mus.h:91: virtual void SetSurfaceInfoFromServer( On 2017/06/07 17:36:26, Fady Samuel wrote: > On 2017/05/31 20:42:44, fsamuel wrote: > > restore name: SetFallbackSurfaceInfo... > > Please restore name Done. https://codereview.chromium.org/2875753002/diff/520001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2875753002/diff/520001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:280: return local_surface_id_; On 2017/06/07 17:36:27, Fady Samuel wrote: > Do we need this change? We don't want to generate local surface id, if a local window doesn't have associated CFS. So WindowTreeClient::ScheduleInFlightBoundsChange() will not send a unused local surface id to WS and not set synchronizing_with_child_on_next_frame_ to true. https://codereview.chromium.org/2875753002/diff/520001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:297: frame_sink_for_local_window_->SetLocalSurfaceId(local_surface_id_); On 2017/06/07 17:36:27, Fady Samuel wrote: > Just call this compositor_frame_sink_. No point in mentioning this is for local > windows. Let's always keep a weak ptr for consistency? Done. https://codereview.chromium.org/2875753002/diff/520001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:312: UpdatePrimarySurfaceInfo(); On 2017/06/07 17:36:27, Fady Samuel wrote: > Do we need this change? Yes. Because we don't know frame sink id for a local window. We only can get the frame sink id here. And we like to call UpdatePrimarySurfaceInfo here, becasue the CFS may already generate a frame. https://codereview.chromium.org/2875753002/diff/520001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:515: DCHECK_EQ(window_mus_type(), WindowMusType::LOCAL); On 2017/06/07 17:36:26, Fady Samuel wrote: > Can we just have a single code path? Let's not special case local windows. We need make sure this function is used on a embedded window in embeddor. https://codereview.chromium.org/2875753002/diff/520001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:528: // mus, so don't implement it now. On 2017/06/07 17:36:27, Fady Samuel wrote: > Just say this is only used by WindowPortLocal in unit tests. Done.
https://codereview.chromium.org/2875753002/diff/540001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2875753002/diff/540001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:297: Remove this change above? https://codereview.chromium.org/2875753002/diff/540001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:324: DCHECK_EQ(window_mus_type(), WindowMusType::LOCAL); This seems like an unnecessary check. https://codereview.chromium.org/2875753002/diff/540001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:337: // fallback. For embedded window, the primary SurfaceInfo is created by the For embedded windows https://codereview.chromium.org/2875753002/diff/540001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:562: window_mus_type() != WindowMusType::LOCAL) {..} https://codereview.chromium.org/2875753002/diff/540001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:580: window_mus_type() != WindowMusType::LOCAL) {..} https://codereview.chromium.org/2875753002/diff/540001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.h (right): https://codereview.chromium.org/2875753002/diff/540001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.h:291: base::WeakPtr<viz::ClientCompositorFrameSink> compositor_frame_sink_; local_compositor_frame_sink_? https://codereview.chromium.org/2875753002/diff/540001/ui/aura/mus/window_tre... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2875753002/diff/540001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:703: } else if (window->window_mus_type() == WindowMusType::LOCAL) { if it has a CompositorFrameSink then do this...
https://codereview.chromium.org/2875753002/diff/540001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2875753002/diff/540001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:297: On 2017/06/07 20:36:11, Fady Samuel wrote: > Remove this change above? Done. https://codereview.chromium.org/2875753002/diff/540001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:324: DCHECK_EQ(window_mus_type(), WindowMusType::LOCAL); On 2017/06/07 20:36:11, Fady Samuel wrote: > This seems like an unnecessary check. Done. https://codereview.chromium.org/2875753002/diff/540001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:337: // fallback. For embedded window, the primary SurfaceInfo is created by the On 2017/06/07 20:36:11, Fady Samuel wrote: > For embedded windows Done. Move this comment back to window_tree_client.cc https://codereview.chromium.org/2875753002/diff/540001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:562: window_mus_type() != WindowMusType::LOCAL) On 2017/06/07 20:36:11, Fady Samuel wrote: > {..} Done. https://codereview.chromium.org/2875753002/diff/540001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:580: window_mus_type() != WindowMusType::LOCAL) On 2017/06/07 20:36:11, Fady Samuel wrote: > {..} Done. https://codereview.chromium.org/2875753002/diff/540001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.h (right): https://codereview.chromium.org/2875753002/diff/540001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.h:291: base::WeakPtr<viz::ClientCompositorFrameSink> compositor_frame_sink_; On 2017/06/07 20:36:11, Fady Samuel wrote: > local_compositor_frame_sink_? Done. https://codereview.chromium.org/2875753002/diff/540001/ui/aura/mus/window_tre... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2875753002/diff/540001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:703: } else if (window->window_mus_type() == WindowMusType::LOCAL) { On 2017/06/07 20:36:11, Fady Samuel wrote: > if it has a CompositorFrameSink then do this... Done.
The CQ bit was checked by penghuang@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.
LGTM + a few minor comments. https://codereview.chromium.org/2875753002/diff/560001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2875753002/diff/560001/components/exo/surface... components/exo/surface.cc:810: window_->SetBounds(gfx::Rect(window_->bounds().origin(), content_size_)); Move this to ClientSurfaceEmbedder::UpdateSizeAndGutters? https://codereview.chromium.org/2875753002/diff/560001/services/ui/ws/window_... File services/ui/ws/window_server.cc (right): https://codereview.chromium.org/2875753002/diff/560001/services/ui/ws/window_... services/ui/ws/window_server.cc:889: // We always use the owner of the window's id (even for embedded window), for an https://codereview.chromium.org/2875753002/diff/560001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.h (right): https://codereview.chromium.org/2875753002/diff/560001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.h:292: base::WeakPtr<viz::ClientCompositorFrameSink> local_compositor_frame_sink_; Add a comment about when we'd use this?
https://codereview.chromium.org/2875753002/diff/560001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2875753002/diff/560001/components/exo/surface... components/exo/surface.cc:810: window_->SetBounds(gfx::Rect(window_->bounds().origin(), content_size_)); On 2017/06/08 17:46:23, Fady Samuel wrote: > Move this to ClientSurfaceEmbedder::UpdateSizeAndGutters? After reading ClientSurfaceEmbedder::UpdateSizeAndGutters(). We have to fix the issue in exo. See the inline comment. https://codereview.chromium.org/2875753002/diff/560001/services/ui/ws/window_... File services/ui/ws/window_server.cc (right): https://codereview.chromium.org/2875753002/diff/560001/services/ui/ws/window_... services/ui/ws/window_server.cc:889: // We always use the owner of the window's id (even for embedded window), On 2017/06/08 17:46:24, Fady Samuel wrote: > for an Done. https://codereview.chromium.org/2875753002/diff/560001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.h (right): https://codereview.chromium.org/2875753002/diff/560001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.h:292: base::WeakPtr<viz::ClientCompositorFrameSink> local_compositor_frame_sink_; On 2017/06/08 17:46:24, Fady Samuel wrote: > Add a comment about when we'd use this? Done.
Hi David & Scott, PTAL. Thanks.
The CQ bit was checked by penghuang@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm
The CQ bit was checked by penghuang@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.
https://codereview.chromium.org/2875753002/diff/600001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2875753002/diff/600001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:542: static_cast<viz::ClientCompositorFrameSink*>(frame_sink.get()); Why do you need the static_cast here?
https://codereview.chromium.org/2875753002/diff/600001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2875753002/diff/600001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:542: static_cast<viz::ClientCompositorFrameSink*>(frame_sink.get()); On 2017/06/08 21:18:49, sky wrote: > Why do you need the static_cast here? frame_sink is a cc:CFS. It doesn't have the GetWeakPtr method.
https://codereview.chromium.org/2875753002/diff/600001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2875753002/diff/600001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:542: static_cast<viz::ClientCompositorFrameSink*>(frame_sink.get()); On 2017/06/08 22:14:59, Peng wrote: > On 2017/06/08 21:18:49, sky wrote: > > Why do you need the static_cast here? > > frame_sink is a cc:CFS. It doesn't have the GetWeakPtr method. Maybe create a "RequestCompositorFrameSinkInternal" that returns a viz::CientCompositorFrameSink that both methods use instead?
https://codereview.chromium.org/2875753002/diff/600001/ui/aura/mus/window_por... File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2875753002/diff/600001/ui/aura/mus/window_por... ui/aura/mus/window_port_mus.cc:542: static_cast<viz::ClientCompositorFrameSink*>(frame_sink.get()); On 2017/06/08 22:16:45, Fady Samuel wrote: > On 2017/06/08 22:14:59, Peng wrote: > > On 2017/06/08 21:18:49, sky wrote: > > > Why do you need the static_cast here? > > > > frame_sink is a cc:CFS. It doesn't have the GetWeakPtr method. > > Maybe create a "RequestCompositorFrameSinkInternal" that returns a > viz::CientCompositorFrameSink that both methods use instead? Done. By changing return type of RequestCompositorFrameSink.
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM
The CQ bit was unchecked by penghuang@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, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2875753002/#ps620001 (title: "Address the review issue")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 620001, "attempt_start_ts": 1497021726470530,
"parent_rev": "f91641dfb18f5f4232e13dde01a9b586c2f15666", "commit_rev":
"e9036c2ba33b4c3f087dac37b238c79792aa8923"}
CQ is committing da patch.
Bot data: {"patchset_id": 620001, "attempt_start_ts": 1497021726470530,
"parent_rev": "01567ac93460e8f152106a74aa91afe13b818e0e", "commit_rev":
"810bd6f44ea3be0e7ff4522190cf2e4924769e17"}
Message was sent while issue was closed.
Description was changed from ========== Implement aura::WindowPortMus::CreateCompositorFrameSink() This CL implements aura::WindowPortMus::CreateCompositorFrameSink(). Exo will use it in Mushrome and Mustash to create a frame sink and use it to submit frames. BUG=720600 ========== to ========== Implement aura::WindowPortMus::CreateCompositorFrameSink() This CL implements aura::WindowPortMus::CreateCompositorFrameSink(). Exo will use it in Mushrome and Mustash to create a frame sink and use it to submit frames. BUG=720600 Review-Url: https://codereview.chromium.org/2875753002 Cr-Commit-Position: refs/heads/master@{#478287} Committed: https://chromium.googlesource.com/chromium/src/+/810bd6f44ea3be0e7ff4522190cf... ==========
Message was sent while issue was closed.
Committed patchset #27 (id:620001) as https://chromium.googlesource.com/chromium/src/+/810bd6f44ea3be0e7ff4522190cf...
Message was sent while issue was closed.
A revert of this CL (patchset #27 id:620001) has been created in https://codereview.chromium.org/2934523002/ by rdevlin.cronin@chromium.org. The reason for reverting is: Broke chromeos compile. https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.chromiumos%2FCh... [1661/3845] CXX obj/ui/aura/aura/window_port_mus.o FAILED: obj/ui/aura/aura/window_port_mus.o /b/c/goma_client/gomacc i686-pc-linux-gnu-g++ -B/b/c/b/ChromiumOS_x86_generic_Compile/.cros_cache/chrome-sdk/tarballs/x86-generic+9588.0.0+target_toolchain/usr/x86_64-pc-linux-gnu/i686-pc-linux-gnu/binutils-bin/2.25.51-gold -MMD -MF obj/ui/aura/aura/window_port_mus.o.d -DAURA_IMPLEMENTATION -DV8_DEPRECATION_WARNINGS -DUSE_UDEV -DUSE_ASH=1 -DUSE_AURA=1 -DUSE_NSS_CERTS=1 -DUSE_OZONE=1 -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DCHROMIUM_BUILD -DFIELDTRIAL_TESTING_ENABLED -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_FORTIFY_SOURCE=2 -DOS_CHROMEOS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DUSE_EGL -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION -DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE -DUCHAR_TYPE=uint16_t -DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS -DSK_HAS_PNG_LIBRARY -DSK_HAS_WEBP_LIBRARY -DSK_HAS_JPEG_LIBRARY -DSK_SUPPORT_GPU=1 -DGOOGLE_PROTOBUF_NO_RTTI -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DHAVE_PTHREAD -DMESA_EGL_NO_X11_HEADERS -I../.. -Igen -I../../third_party/libwebp -I../../third_party/khronos -I../../gpu -I../../third_party/ced/src -I../../third_party/icu/source/common -I../../third_party/icu/source/i18n -I../../skia/config -I../../skia/ext -I../../third_party/skia/include/c -I../../third_party/skia/include/config -I../../third_party/skia/include/core -I../../third_party/skia/include/effects -I../../third_party/skia/include/encode -I../../third_party/skia/include/images -I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops -I../../third_party/skia/include/pdf -I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports -I../../third_party/skia/include/utils -I../../third_party/skia/third_party/vulkan -I../../third_party/skia/include/gpu -I../../third_party/skia/src/gpu -I../../third_party/skia/src/sksl -I../../third_party/protobuf/src -Igen/protoc_out -I../../third_party/protobuf/src -I../../third_party/boringssl/src/include -I/b/c/b/ChromiumOS_x86_generic_Compile/.cros_cache/chrome-sdk/tarballs/x86-generic+9588.0.0+sysroot_chromeos-base_chromeos-chrome.tar.xz/usr/include/nss -I/b/c/b/ChromiumOS_x86_generic_Compile/.cros_cache/chrome-sdk/tarballs/x86-generic+9588.0.0+sysroot_chromeos-base_chromeos-chrome.tar.xz/usr/include/nspr -I../../third_party/mesa/src/include -fno-strict-aliasing -Wno-builtin-macro-redefined -D__DATE__= -D__TIME__= -D__TIMESTAMP__= -funwind-tables -fPIC -pipe -B../../third_party/binutils/Linux_x64/Release/bin -pthread -m32 -msse2 -mfpmath=sse -mmmx -Wall -Werror -Wno-unused-local-typedefs -Wno-maybe-uninitialized -Wno-missing-field-initializers -Wno-unused-parameter -O2 -fno-ident -fdata-sections -ffunction-sections -fno-omit-frame-pointer -g2 -gsplit-dwarf --sysroot=../../../.cros_cache/chrome-sdk/tarballs/x86-generic+9588.0.0+sysroot_chromeos-base_chromeos-chrome.tar.xz -fvisibility=hidden -std=gnu++11 -Wno-narrowing -fno-rtti -fno-exceptions -fvisibility-inlines-hidden -march=i686 -pipe -march=i686 -pipe -pipe -march=i686 -mfpmath=sse -mmmx -msse -msse2 -msse3 -D__google_stl_debug_vector=1 -c ../../ui/aura/mus/window_port_mus.cc -o obj/ui/aura/aura/window_port_mus.o ../../ui/aura/mus/window_port_mus.cc: In member function 'virtual std::unique_ptr<cc::CompositorFrameSink> aura::WindowPortMus::CreateCompositorFrameSink()': ../../ui/aura/mus/window_port_mus.cc:542:10: error: cannot bind 'std::unique_ptr<viz::ClientCompositorFrameSink>' lvalue to 'std::unique_ptr<viz::ClientCompositorFrameSink>&&' return frame_sink; ^ In file included from /b/c/b/ChromiumOS_x86_generic_Compile/.cros_cache/chrome-sdk/tarballs/x86-generic+9588.0.0+target_toolchain/usr/x86_64-pc-linux-gnu/i686-pc-linux-gnu/gcc-bin/4.9.x/../../../../lib/gcc/i686-pc-linux-gnu/4.9.x/include/g++-v4/memory:81:0, from ../../base/sequence_checker_impl.h:8, from ../../base/sequence_checker.h:10, from ../../base/memory/ref_counted.h:19, from ../../base/memory/weak_ptr.h:79, from ../../components/viz/client/client_compositor_frame_sink.h:9, from ../../ui/aura/mus/window_port_mus.h:16, from ../../ui/aura/mus/window_port_mus.cc:5: /b/c/b/ChromiumOS_x86_generic_Compile/.cros_cache/chrome-sdk/tarballs/x86-generic+9588.0.0+target_toolchain/usr/x86_64-pc-linux-gnu/i686-pc-linux-gnu/gcc-bin/4.9.x/../../../../lib/gcc/i686-pc-linux-gnu/4.9.x/include/g++-v4/bits/unique_ptr.h:220:2: note: initializing argument 1 of 'std::unique_ptr<_Tp, _Dp>::unique_ptr(std::unique_ptr<_Up, _Ep>&&) [with _Up = viz::ClientCompositorFrameSink; _Ep = std::default_delete<viz::ClientCompositorFrameSink>; <template-parameter-2-3> = void; _Tp = cc::CompositorFrameSink; _Dp = std::default_delete<cc::CompositorFrameSink>]' unique_ptr(unique_ptr<_Up, _Ep>&& __u) noexcept ^ ../../ui/aura/mus/window_port_mus.cc:543:1: error: control reaches end of non-void function [-Werror=return-type] }.
Message was sent while issue was closed.
Description was changed from ========== Implement aura::WindowPortMus::CreateCompositorFrameSink() This CL implements aura::WindowPortMus::CreateCompositorFrameSink(). Exo will use it in Mushrome and Mustash to create a frame sink and use it to submit frames. BUG=720600 Review-Url: https://codereview.chromium.org/2875753002 Cr-Commit-Position: refs/heads/master@{#478287} Committed: https://chromium.googlesource.com/chromium/src/+/810bd6f44ea3be0e7ff4522190cf... ========== to ========== Implement aura::WindowPortMus::CreateCompositorFrameSink() This CL implements aura::WindowPortMus::CreateCompositorFrameSink(). Exo will use it in Mushrome and Mustash to create a frame sink and use it to submit frames. BUG=720600 Review-Url: https://codereview.chromium.org/2875753002 Cr-Commit-Position: refs/heads/master@{#478287} Committed: https://chromium.googlesource.com/chromium/src/+/810bd6f44ea3be0e7ff4522190cf... ==========
The CQ bit was checked by penghuang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, fsamuel@chromium.org, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2875753002/#ps640001 (title: "Fix build problem")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 640001, "attempt_start_ts": 1497029111244780,
"parent_rev": "86dd703c1540e436d342e4450f95c887cb4af8fb", "commit_rev":
"7d3b44d7d03614dd15fda5e81df050b0e618ad2b"}
Message was sent while issue was closed.
Description was changed from ========== Implement aura::WindowPortMus::CreateCompositorFrameSink() This CL implements aura::WindowPortMus::CreateCompositorFrameSink(). Exo will use it in Mushrome and Mustash to create a frame sink and use it to submit frames. BUG=720600 Review-Url: https://codereview.chromium.org/2875753002 Cr-Commit-Position: refs/heads/master@{#478287} Committed: https://chromium.googlesource.com/chromium/src/+/810bd6f44ea3be0e7ff4522190cf... ========== to ========== Implement aura::WindowPortMus::CreateCompositorFrameSink() This CL implements aura::WindowPortMus::CreateCompositorFrameSink(). Exo will use it in Mushrome and Mustash to create a frame sink and use it to submit frames. BUG=720600 Review-Url: https://codereview.chromium.org/2875753002 Cr-Original-Commit-Position: refs/heads/master@{#478287} Committed: https://chromium.googlesource.com/chromium/src/+/810bd6f44ea3be0e7ff4522190cf... Review-Url: https://codereview.chromium.org/2875753002 Cr-Commit-Position: refs/heads/master@{#478355} Committed: https://chromium.googlesource.com/chromium/src/+/7d3b44d7d03614dd15fda5e81df0... ==========
Message was sent while issue was closed.
Committed patchset #28 (id:640001) as https://chromium.googlesource.com/chromium/src/+/7d3b44d7d03614dd15fda5e81df0... |
