Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2579693004:
GpuCompositorFrameSink implements cc::mojom::DisplayPrivate (Closed)
|
Created:
4 years ago by Alex Z. Modified:
4 years ago CC:
chromium-reviews, rjkroege, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, cc-bugs_chromium.org, darin (slow to review) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGpuCompositorFrameSink implements cc::mojom::DisplayPrivate
Added DisplayPrivate mojo interface to hide cc::Display from ContextFactory.
The intent of this change is to content mus-ws with mus-gpu for cc::Display
operations.
ColorSpace and OutputIsSecure and so on is passed along from display management
code which resides in Mus-ws. cc::Display lives in mus-gpu and so we need a mojo
interface for cc::Display.
Currently ash talks to cc::Display through ContextFactoryPrivate. This mojo
interface would eventually replace ContextFactoryPrivate or would be used by
ContextFactoryPrivate.
BUG=670364
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
TBR=danakj@chromium.org
Committed: https://crrev.com/dc7e15988c2612f4bdd66e29b238a4eb3fce5cee
Cr-Commit-Position: refs/heads/master@{#440192}
Patch Set 1 #Patch Set 2 : Fixed header path in color_space.typemap #Patch Set 3 : Added //ipc to public_deps in color_space.typemap #
Total comments: 24
Patch Set 4 : Addressed comments #
Total comments: 26
Patch Set 5 : Addressed comments #Patch Set 6 : Removed unrelated chagnes #
Total comments: 13
Patch Set 7 : Fixed a typo #Patch Set 8 : Addresed comments #
Total comments: 7
Patch Set 9 : Rebase and address comments #Patch Set 10 : Changed comments #Patch Set 11 : Rebase #
Total comments: 4
Patch Set 12 : Added deps to ui/gfx/mojo #Patch Set 13 : Address comments #
Total comments: 1
Messages
Total messages: 85 (43 generated)
Description was changed from ========== GpuCompositorFrameSink implements cc::mojom::DisplayPrivate Added DisplayPrivate mojo interface to hide cc::Display from ContextFactory. BUG=670364 ========== to ========== GpuCompositorFrameSink implements cc::mojom::DisplayPrivate Added DisplayPrivate mojo interface to hide cc::Display from ContextFactory. BUG=670364 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by staraz@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_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 staraz@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_trusty_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...) linux_chromium_compile_dbg_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 staraz@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_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
fsamuel@chromium.org changed reviewers: + fsamuel@chromium.org, kylechar@chromium.org, sadrul@chromium.org
drive-by review + added kylechar@ and sadrul@ on how display management stuff will talk to this interface. https://codereview.chromium.org/2579693004/diff/60001/cc/ipc/display_composit... File cc/ipc/display_compositor.mojom (right): https://codereview.chromium.org/2579693004/diff/60001/cc/ipc/display_composit... cc/ipc/display_compositor.mojom:66: SetDisplayColorSpace(gfx.mojom.ColorSpace color_space); +sadrul@, kylechar@ on opinions on how this will interact with display management stuff. https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.cc:46: cc::mojom::DisplayPrivatePtr display_private; I think FrameGenerator probably wants to hold onto display_private in this case. +kylechar@, +sadrul@ https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/server_w... File services/ui/ws/server_window.cc (right): https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/server_w... services/ui/ws/server_window.cc:93: widget, std::move(request), std::move(client), std::move(display_private), I don't think you should be passing in the interfaceptr side of the message pipe. https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/server_w... services/ui/ws/server_window.cc:94: std::move(display_request)); nit: display_private_request https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/server_w... File services/ui/ws/server_window_compositor_frame_sink_manager.cc (right): https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/server_w... services/ui/ws/server_window_compositor_frame_sink_manager.cc:42: cc::mojom::DisplayPrivatePtr display_private, don't pass this in https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/server_w... services/ui/ws/server_window_compositor_frame_sink_manager.cc:43: cc::mojom::DisplayPrivateRequest display_request) { display_private_request https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/server_w... services/ui/ws/server_window_compositor_frame_sink_manager.cc:44: display_private_ = std::move(display_private); don't store this. https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/server_w... services/ui/ws/server_window_compositor_frame_sink_manager.cc:161: std::move(display_request)); nit: display_private_request https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/server_w... File services/ui/ws/server_window_compositor_frame_sink_manager.h (right): https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/server_w... services/ui/ws/server_window_compositor_frame_sink_manager.h:57: cc::mojom::DisplayPrivatePtr display_private, don't pass this in https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/server_w... services/ui/ws/server_window_compositor_frame_sink_manager.h:58: cc::mojom::DisplayPrivateRequest display_request); nit: display_private_request https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/server_w... services/ui/ws/server_window_compositor_frame_sink_manager.h:103: cc::mojom::DisplayPrivateRequest display_request); nit: display_private_request https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/server_w... services/ui/ws/server_window_compositor_frame_sink_manager.h:118: cc::mojom::DisplayPrivatePtr display_private_; Don't store this here. I think FrameGenerator should hold onto this.
The CQ bit was checked by staraz@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...
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.cc:46: cc::mojom::DisplayPrivatePtr display_private; On 2016/12/16 05:29:39, Fady Samuel wrote: > I think FrameGenerator probably wants to hold onto display_private in this case. > +kylechar@, +sadrul@ Done. https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/server_w... File services/ui/ws/server_window.cc (right): https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/server_w... services/ui/ws/server_window.cc:93: widget, std::move(request), std::move(client), std::move(display_private), On 2016/12/16 05:29:39, Fady Samuel wrote: > I don't think you should be passing in the interfaceptr side of the message > pipe. Done. https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/server_w... services/ui/ws/server_window.cc:94: std::move(display_request)); On 2016/12/16 05:29:39, Fady Samuel wrote: > nit: display_private_request Done. https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/server_w... File services/ui/ws/server_window_compositor_frame_sink_manager.cc (right): https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/server_w... services/ui/ws/server_window_compositor_frame_sink_manager.cc:42: cc::mojom::DisplayPrivatePtr display_private, On 2016/12/16 05:29:39, Fady Samuel wrote: > don't pass this in Done. https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/server_w... services/ui/ws/server_window_compositor_frame_sink_manager.cc:43: cc::mojom::DisplayPrivateRequest display_request) { On 2016/12/16 05:29:39, Fady Samuel wrote: > display_private_request Done. https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/server_w... services/ui/ws/server_window_compositor_frame_sink_manager.cc:44: display_private_ = std::move(display_private); On 2016/12/16 05:29:39, Fady Samuel wrote: > don't store this. Done. https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/server_w... services/ui/ws/server_window_compositor_frame_sink_manager.cc:161: std::move(display_request)); On 2016/12/16 05:29:39, Fady Samuel wrote: > nit: display_private_request Done. https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/server_w... File services/ui/ws/server_window_compositor_frame_sink_manager.h (right): https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/server_w... services/ui/ws/server_window_compositor_frame_sink_manager.h:57: cc::mojom::DisplayPrivatePtr display_private, On 2016/12/16 05:29:39, Fady Samuel wrote: > don't pass this in Done. https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/server_w... services/ui/ws/server_window_compositor_frame_sink_manager.h:58: cc::mojom::DisplayPrivateRequest display_request); On 2016/12/16 05:29:39, Fady Samuel wrote: > nit: display_private_request Done. https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/server_w... services/ui/ws/server_window_compositor_frame_sink_manager.h:103: cc::mojom::DisplayPrivateRequest display_request); On 2016/12/16 05:29:39, Fady Samuel wrote: > nit: display_private_request Done. https://codereview.chromium.org/2579693004/diff/60001/services/ui/ws/server_w... services/ui/ws/server_window_compositor_frame_sink_manager.h:118: cc::mojom::DisplayPrivatePtr display_private_; On 2016/12/16 05:29:39, Fady Samuel wrote: > Don't store this here. I think FrameGenerator should hold onto this. Done.
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2579693004/diff/60001/cc/ipc/display_composit... File cc/ipc/display_compositor.mojom (right): https://codereview.chromium.org/2579693004/diff/60001/cc/ipc/display_composit... cc/ipc/display_compositor.mojom:66: SetDisplayColorSpace(gfx.mojom.ColorSpace color_space); On 2016/12/16 05:29:39, Fady Samuel wrote: > +sadrul@, kylechar@ on opinions on how this will interact with display > management stuff. This seems fine. In mustash the WS would call these. In cash the Browser process would eventually call these. https://codereview.chromium.org/2579693004/diff/120001/cc/ipc/display_composi... File cc/ipc/display_compositor.mojom (right): https://codereview.chromium.org/2579693004/diff/120001/cc/ipc/display_composi... cc/ipc/display_compositor.mojom:63: interface DisplayPrivate { Is it DisplayPrivate just to disambiguate it from other things named Display? https://codereview.chromium.org/2579693004/diff/120001/services/ui/surfaces/g... File services/ui/surfaces/gpu_compositor_frame_sink.h (right): https://codereview.chromium.org/2579693004/diff/120001/services/ui/surfaces/g... services/ui/surfaces/gpu_compositor_frame_sink.h:31: class GpuCompositorFrameSink : public cc::CompositorFrameSinkSupportClient, Just thinking out loud here but there is the normal client GpuCompositorFrameSink and then the display client GpuCompositorFrameSink, with a pretty big different in functionality. Would it make sense to have GpuCompositorFrameSink and something else that inherits from it that implements all the cc::Display specific stuff?
From the CL description: > Added DisplayPrivate mojo interface to hide cc::Display from ContextFactory. What ContextFactory are you referring to? (ui::ContextFactory interface does not know about cc::Display right now anyway) https://codereview.chromium.org/2579693004/diff/120001/cc/ipc/display_composi... File cc/ipc/display_compositor.mojom (right): https://codereview.chromium.org/2579693004/diff/120001/cc/ipc/display_composi... cc/ipc/display_compositor.mojom:65: ResizeDisplay(gfx.mojom.Size size); Can you add a comment to mention whether |size| is in DIP or physical pixel space? https://codereview.chromium.org/2579693004/diff/120001/services/ui/surfaces/g... File services/ui/surfaces/gpu_compositor_frame_sink.h (right): https://codereview.chromium.org/2579693004/diff/120001/services/ui/surfaces/g... services/ui/surfaces/gpu_compositor_frame_sink.h:31: class GpuCompositorFrameSink : public cc::CompositorFrameSinkSupportClient, On 2016/12/16 17:08:32, kylechar wrote: > Just thinking out loud here but there is the normal client > GpuCompositorFrameSink and then the display client GpuCompositorFrameSink, with > a pretty big different in functionality. Would it make sense to have > GpuCompositorFrameSink and something else that inherits from it that implements > all the cc::Display specific stuff? +1
https://codereview.chromium.org/2579693004/diff/120001/cc/ipc/display_composi... File cc/ipc/display_compositor.mojom (right): https://codereview.chromium.org/2579693004/diff/120001/cc/ipc/display_composi... cc/ipc/display_compositor.mojom:65: ResizeDisplay(gfx.mojom.Size size); On 2016/12/16 17:12:05, sadrul wrote: > Can you add a comment to mention whether |size| is in DIP or physical pixel > space? Or name it size_in_dip size_in_physical_pixels or the like
https://codereview.chromium.org/2579693004/diff/120001/cc/ipc/display_composi... File cc/ipc/display_compositor.mojom (right): https://codereview.chromium.org/2579693004/diff/120001/cc/ipc/display_composi... cc/ipc/display_compositor.mojom:28: CreateDisplayCompositorFrameSink( Comments to disambiguate CreateDisplayCompositorFrameSink() vs CreateOffscreenCompositorFrameSink() would be helpful.
https://codereview.chromium.org/2579693004/diff/120001/cc/ipc/display_composi... File cc/ipc/display_compositor.mojom (right): https://codereview.chromium.org/2579693004/diff/120001/cc/ipc/display_composi... cc/ipc/display_compositor.mojom:62: // cc::Display. nit: I would not reference GpuCompositorFrameSink here, nor would I reference ContextFactory because those are in different layers of the system. Maybe simply say // See cc/surfaces/display.h Almost move this above the DisplayCompositor interface. This should be near the top of the file. That's simply easier for readability. CreateDisplayCompositorFrameSink uses this interface, and so it would be nice to encounter this interface first in the file. https://codereview.chromium.org/2579693004/diff/120001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2579693004/diff/120001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:50: Display* GetDisplay() { return display_.get(); } Display* display() { return display_.get(); } https://codereview.chromium.org/2579693004/diff/120001/services/ui/surfaces/g... File services/ui/surfaces/gpu_compositor_frame_sink.cc (right): https://codereview.chromium.org/2579693004/diff/120001/services/ui/surfaces/g... services/ui/surfaces/gpu_compositor_frame_sink.cc:17: cc::mojom::MojoCompositorFrameSinkPrivateRequest private_request, compositor_frame_sink_private_request https://codereview.chromium.org/2579693004/diff/120001/services/ui/surfaces/g... services/ui/surfaces/gpu_compositor_frame_sink.cc:19: cc::mojom::DisplayPrivateRequest display_request) display_private_request https://codereview.chromium.org/2579693004/diff/120001/services/ui/surfaces/g... services/ui/surfaces/gpu_compositor_frame_sink.cc:28: private_binding_(this, std::move(private_request)), compositor_frame_sink_private_binding_ https://codereview.chromium.org/2579693004/diff/120001/services/ui/surfaces/g... services/ui/surfaces/gpu_compositor_frame_sink.cc:29: display_binding_(this, std::move(display_request)) { display_private_binding_ https://codereview.chromium.org/2579693004/diff/120001/services/ui/surfaces/g... File services/ui/surfaces/gpu_compositor_frame_sink.h (right): https://codereview.chromium.org/2579693004/diff/120001/services/ui/surfaces/g... services/ui/surfaces/gpu_compositor_frame_sink.h:18: #include "ui/gfx/color_space.h" Forward declaration preferred. https://codereview.chromium.org/2579693004/diff/120001/services/ui/surfaces/g... services/ui/surfaces/gpu_compositor_frame_sink.h:19: #include "ui/gfx/geometry/size.h" Forward declaration preferred. https://codereview.chromium.org/2579693004/diff/120001/services/ui/surfaces/g... services/ui/surfaces/gpu_compositor_frame_sink.h:20: #include "ui/gfx/mojo/color_space.mojom.h" Forward declaration preferred.
btw: can the ui/gfx/ changes be in a separate CL?
So the intent of this change is to content mus-ws with mus-gpu for cc::Display operations. ColorSpace and OutputIsSecure and so on is passed along from display management code which resides in Mus-ws. cc::Display lives in mus-gpu and so we need a mojo interface for cc::Display. Currently ash talks to cc::Display through ContextFactoryPrivate. This mojo interface would eventually replace ContextFactoryPrivate or would be used by ContextFactoryPrivate.
Patchset #4 (id:100001) has been deleted
https://codereview.chromium.org/2579693004/diff/120001/cc/ipc/display_composi... File cc/ipc/display_compositor.mojom (right): https://codereview.chromium.org/2579693004/diff/120001/cc/ipc/display_composi... cc/ipc/display_compositor.mojom:28: CreateDisplayCompositorFrameSink( On 2016/12/16 17:15:12, kylechar wrote: > Comments to disambiguate CreateDisplayCompositorFrameSink() vs > CreateOffscreenCompositorFrameSink() would be helpful. Done. https://codereview.chromium.org/2579693004/diff/120001/cc/ipc/display_composi... cc/ipc/display_compositor.mojom:62: // cc::Display. On 2016/12/16 17:18:18, Fady Samuel wrote: > nit: I would not reference GpuCompositorFrameSink here, nor would I reference > ContextFactory because those are in different layers of the system. Maybe simply > say // See cc/surfaces/display.h > > Almost move this above the DisplayCompositor interface. This should be near the > top of the file. That's simply easier for readability. > CreateDisplayCompositorFrameSink uses this interface, and so it would be nice to > encounter this interface first in the file. Done. https://codereview.chromium.org/2579693004/diff/120001/cc/ipc/display_composi... cc/ipc/display_compositor.mojom:65: ResizeDisplay(gfx.mojom.Size size); On 2016/12/16 17:12:05, sadrul wrote: > Can you add a comment to mention whether |size| is in DIP or physical pixel > space? Done. https://codereview.chromium.org/2579693004/diff/120001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2579693004/diff/120001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:50: Display* GetDisplay() { return display_.get(); } On 2016/12/16 17:18:18, Fady Samuel wrote: > Display* display() { return display_.get(); } Done. https://codereview.chromium.org/2579693004/diff/120001/services/ui/surfaces/g... File services/ui/surfaces/gpu_compositor_frame_sink.cc (right): https://codereview.chromium.org/2579693004/diff/120001/services/ui/surfaces/g... services/ui/surfaces/gpu_compositor_frame_sink.cc:17: cc::mojom::MojoCompositorFrameSinkPrivateRequest private_request, On 2016/12/16 17:18:18, Fady Samuel wrote: > compositor_frame_sink_private_request Done. https://codereview.chromium.org/2579693004/diff/120001/services/ui/surfaces/g... services/ui/surfaces/gpu_compositor_frame_sink.cc:19: cc::mojom::DisplayPrivateRequest display_request) On 2016/12/16 17:18:18, Fady Samuel wrote: > display_private_request Done. https://codereview.chromium.org/2579693004/diff/120001/services/ui/surfaces/g... services/ui/surfaces/gpu_compositor_frame_sink.cc:28: private_binding_(this, std::move(private_request)), On 2016/12/16 17:18:18, Fady Samuel wrote: > compositor_frame_sink_private_binding_ Done. https://codereview.chromium.org/2579693004/diff/120001/services/ui/surfaces/g... services/ui/surfaces/gpu_compositor_frame_sink.cc:29: display_binding_(this, std::move(display_request)) { On 2016/12/16 17:18:19, Fady Samuel wrote: > display_private_binding_ Done. https://codereview.chromium.org/2579693004/diff/120001/services/ui/surfaces/g... File services/ui/surfaces/gpu_compositor_frame_sink.h (right): https://codereview.chromium.org/2579693004/diff/120001/services/ui/surfaces/g... services/ui/surfaces/gpu_compositor_frame_sink.h:18: #include "ui/gfx/color_space.h" On 2016/12/16 17:18:19, Fady Samuel wrote: > Forward declaration preferred. Done. https://codereview.chromium.org/2579693004/diff/120001/services/ui/surfaces/g... services/ui/surfaces/gpu_compositor_frame_sink.h:19: #include "ui/gfx/geometry/size.h" On 2016/12/16 17:18:19, Fady Samuel wrote: > Forward declaration preferred. Done. https://codereview.chromium.org/2579693004/diff/120001/services/ui/surfaces/g... services/ui/surfaces/gpu_compositor_frame_sink.h:20: #include "ui/gfx/mojo/color_space.mojom.h" On 2016/12/16 17:18:19, Fady Samuel wrote: > Forward declaration preferred. Done.
lgtm
Description was changed from ========== GpuCompositorFrameSink implements cc::mojom::DisplayPrivate Added DisplayPrivate mojo interface to hide cc::Display from ContextFactory. BUG=670364 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== GpuCompositorFrameSink implements cc::mojom::DisplayPrivate Added DisplayPrivate mojo interface to hide cc::Display from ContextFactory. The intent of this change is to content mus-ws with mus-gpu for cc::Display operations. ColorSpace and OutputIsSecure and so on is passed along from display management code which resides in Mus-ws. cc::Display lives in mus-gpu and so we need a mojo interface for cc::Display. Currently ash talks to cc::Display through ContextFactoryPrivate. This mojo interface would eventually replace ContextFactoryPrivate or would be used by ContextFactoryPrivate. BUG=670364 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
staraz@chromium.org changed reviewers: + jbauman@chromium.org, msw@chromium.org, tsepez@chromium.org
jbauman@chromium.org: Please review changes in compositor_frame_sink_support.cc msw@chromium.org: Please review changes in services/ui/ws/ tsepez@: Please review changes in display_compositor.mojom
https://codereview.chromium.org/2579693004/diff/160001/cc/ipc/display_composi... File cc/ipc/display_compositor.mojom (right): https://codereview.chromium.org/2579693004/diff/160001/cc/ipc/display_composi... cc/ipc/display_compositor.mojom:16: // See cc//surfaces/display.h nit: path typo https://codereview.chromium.org/2579693004/diff/160001/cc/ipc/display_composi... cc/ipc/display_compositor.mojom:21: SetOutputIsSecure(bool secure); I know its pre-existing, but could you explain what the implications are of something being "secure"?
services/ui/ws/ rubber stamp lgtm with tangential/minor qs https://codereview.chromium.org/2579693004/diff/160001/services/ui/ws/server_... File services/ui/ws/server_window.h (right): https://codereview.chromium.org/2579693004/diff/160001/services/ui/ws/server_... services/ui/ws/server_window.h:62: // TODO(fsamuel): We should not be passing in |gpu_memory_buffer_manager| and tangential q: this TODO seems out of date; should it be removed/updated? https://codereview.chromium.org/2579693004/diff/160001/services/ui/ws/server_... services/ui/ws/server_window.h:69: cc::mojom::MojoCompositorFrameSinkClientPtr client, tangential q: Shouldn't this file #include "cc/ipc/mojo_compositor_frame_sink.mojom.h"? https://codereview.chromium.org/2579693004/diff/160001/services/ui/ws/server_... File services/ui/ws/server_window_compositor_frame_sink_manager.cc (right): https://codereview.chromium.org/2579693004/diff/160001/services/ui/ws/server_... services/ui/ws/server_window_compositor_frame_sink_manager.cc:175: std::move(display_private_request)); q: Should the else case DCHECK(!display_private_request)? (it's ignored)
https://codereview.chromium.org/2579693004/diff/160001/services/ui/ws/server_... File services/ui/ws/server_window.h (right): https://codereview.chromium.org/2579693004/diff/160001/services/ui/ws/server_... services/ui/ws/server_window.h:62: // TODO(fsamuel): We should not be passing in |gpu_memory_buffer_manager| and On 2016/12/19 20:18:11, msw wrote: > tangential q: this TODO seems out of date; should it be removed/updated? Yes, this TODO was addressed a while ago and can be deleted. https://codereview.chromium.org/2579693004/diff/160001/services/ui/ws/server_... services/ui/ws/server_window.h:69: cc::mojom::MojoCompositorFrameSinkClientPtr client, On 2016/12/19 20:18:11, msw wrote: > tangential q: Shouldn't this file #include > "cc/ipc/mojo_compositor_frame_sink.mojom.h"? +1 https://codereview.chromium.org/2579693004/diff/160001/services/ui/ws/server_... File services/ui/ws/server_window_compositor_frame_sink_manager.cc (right): https://codereview.chromium.org/2579693004/diff/160001/services/ui/ws/server_... services/ui/ws/server_window_compositor_frame_sink_manager.cc:175: std::move(display_private_request)); On 2016/12/19 20:18:11, msw wrote: > q: Should the else case DCHECK(!display_private_request)? (it's ignored) Sounds reasonable.
https://codereview.chromium.org/2579693004/diff/160001/cc/ipc/display_composi... File cc/ipc/display_compositor.mojom (right): https://codereview.chromium.org/2579693004/diff/160001/cc/ipc/display_composi... cc/ipc/display_compositor.mojom:16: // See cc//surfaces/display.h On 2016/12/19 20:14:23, Tom Sepez wrote: > nit: path typo Done. https://codereview.chromium.org/2579693004/diff/160001/cc/ipc/display_composi... cc/ipc/display_compositor.mojom:21: SetOutputIsSecure(bool secure); On 2016/12/19 20:14:23, Tom Sepez wrote: > I know its pre-existing, but could you explain what the implications are of > something being "secure"? It indicates if the output content is secure DRM-wise. The value ends up in SurfaceAggregator. SurfaceAggregator only draws the actual content if output is secure, it draws a solid color otherwise.
https://codereview.chromium.org/2579693004/diff/160001/services/ui/ws/server_... File services/ui/ws/server_window.h (right): https://codereview.chromium.org/2579693004/diff/160001/services/ui/ws/server_... services/ui/ws/server_window.h:62: // TODO(fsamuel): We should not be passing in |gpu_memory_buffer_manager| and On 2016/12/19 20:20:18, Fady Samuel wrote: > On 2016/12/19 20:18:11, msw wrote: > > tangential q: this TODO seems out of date; should it be removed/updated? > > Yes, this TODO was addressed a while ago and can be deleted. Done. https://codereview.chromium.org/2579693004/diff/160001/services/ui/ws/server_... services/ui/ws/server_window.h:69: cc::mojom::MojoCompositorFrameSinkClientPtr client, On 2016/12/19 20:20:18, Fady Samuel wrote: > On 2016/12/19 20:18:11, msw wrote: > > tangential q: Shouldn't this file #include > > "cc/ipc/mojo_compositor_frame_sink.mojom.h"? > > +1 Done. https://codereview.chromium.org/2579693004/diff/160001/services/ui/ws/server_... File services/ui/ws/server_window_compositor_frame_sink_manager.cc (right): https://codereview.chromium.org/2579693004/diff/160001/services/ui/ws/server_... services/ui/ws/server_window_compositor_frame_sink_manager.cc:175: std::move(display_private_request)); On 2016/12/19 20:20:18, Fady Samuel wrote: > On 2016/12/19 20:18:11, msw wrote: > > q: Should the else case DCHECK(!display_private_request)? (it's ignored) > > Sounds reasonable. Done.
lgtm
rjkroege@chromium.org changed reviewers: + rjkroege@chromium.org
I dropped by (sorry) to understand display::Display to cc::Display integration. It's not yet clear to me. Any, apologies for the bike-shedding. https://codereview.chromium.org/2579693004/diff/200001/cc/ipc/display_composi... File cc/ipc/display_compositor.mojom (right): https://codereview.chromium.org/2579693004/diff/200001/cc/ipc/display_composi... cc/ipc/display_compositor.mojom:16: // See cc/surfaces/display.h I'd like the .mojom to have more detailed documentation. display.h (at least the version currently used for code search) doesn't separate the private from the public interface so perhaps this could have more stand-alone commentary. Also, what's the reasoning for the name variance between Display::SetColorSpace, Display::SetVisible, Display::Resize and these methods? https://codereview.chromium.org/2579693004/diff/200001/services/ui/surfaces/g... File services/ui/surfaces/gpu_compositor_frame_sink.h (right): https://codereview.chromium.org/2579693004/diff/200001/services/ui/surfaces/g... services/ui/surfaces/gpu_compositor_frame_sink.h:46: cc::mojom::DisplayPrivateRequest display_request); bike-shedding: why not display_private_request?
https://codereview.chromium.org/2579693004/diff/200001/cc/ipc/display_composi... File cc/ipc/display_compositor.mojom (right): https://codereview.chromium.org/2579693004/diff/200001/cc/ipc/display_composi... cc/ipc/display_compositor.mojom:16: // See cc/surfaces/display.h On 2016/12/20 00:51:51, rjkroege wrote: > I'd like the .mojom to have more detailed documentation. display.h (at least the > version currently used for code search) doesn't separate the private from the > public interface so perhaps this could have more stand-alone commentary. > > Also, what's the reasoning for the name variance between Display::SetColorSpace, > Display::SetVisible, Display::Resize and these methods? Currently ash talks to cc::Display through ContextFactoryPrivate. This mojo interface would eventually replace ContextFactoryPrivate or would be used by it. Besides, this is implemented by GpuCompositorFrameSink instead of cc::Display. Hence, I think it's more appropriate to use the names in ContextFactoryPrivate instead of the method names in cc::Display.
https://codereview.chromium.org/2579693004/diff/200001/cc/ipc/display_composi... File cc/ipc/display_compositor.mojom (right): https://codereview.chromium.org/2579693004/diff/200001/cc/ipc/display_composi... cc/ipc/display_compositor.mojom:16: // See cc/surfaces/display.h On 2016/12/20 00:51:51, rjkroege wrote: > I'd like the .mojom to have more detailed documentation. display.h (at least the > version currently used for code search) doesn't separate the private from the > public interface so perhaps this could have more stand-alone commentary. > > Also, what's the reasoning for the name variance between Display::SetColorSpace, > Display::SetVisible, Display::Resize and these methods? Done. https://codereview.chromium.org/2579693004/diff/200001/services/ui/surfaces/g... File services/ui/surfaces/gpu_compositor_frame_sink.h (right): https://codereview.chromium.org/2579693004/diff/200001/services/ui/surfaces/g... services/ui/surfaces/gpu_compositor_frame_sink.h:46: cc::mojom::DisplayPrivateRequest display_request); On 2016/12/20 00:51:51, rjkroege wrote: > bike-shedding: why not display_private_request? Because I missed it while checking all the naming :(. It's display_private_request now.
mojom LGTM
The CQ bit was checked by staraz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org, msw@chromium.org, jbauman@chromium.org Link to the patchset: https://codereview.chromium.org/2579693004/#ps220001 (title: "Rebase and address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
https://codereview.chromium.org/2579693004/diff/200001/cc/ipc/display_composi... File cc/ipc/display_compositor.mojom (right): https://codereview.chromium.org/2579693004/diff/200001/cc/ipc/display_composi... cc/ipc/display_compositor.mojom:16: // See cc/surfaces/display.h On 2016/12/20 15:49:11, StarAZ1 wrote: > On 2016/12/20 00:51:51, rjkroege wrote: > > I'd like the .mojom to have more detailed documentation. display.h (at least > the > > version currently used for code search) doesn't separate the private from the > > public interface so perhaps this could have more stand-alone commentary. > > > > Also, what's the reasoning for the name variance between > Display::SetColorSpace, > > Display::SetVisible, Display::Resize and these methods? > > Done. Given the above, it's not appropriate to reference cc::Display to document these methods. Instead, you should at least refer to ContextFactoryPrivate yes?
https://codereview.chromium.org/2579693004/diff/200001/cc/ipc/display_composi... File cc/ipc/display_compositor.mojom (right): https://codereview.chromium.org/2579693004/diff/200001/cc/ipc/display_composi... cc/ipc/display_compositor.mojom:16: // See cc/surfaces/display.h On 2016/12/20 19:18:08, rjkroege wrote: > On 2016/12/20 15:49:11, StarAZ1 wrote: > > On 2016/12/20 00:51:51, rjkroege wrote: > > > I'd like the .mojom to have more detailed documentation. display.h (at least > > the > > > version currently used for code search) doesn't separate the private from > the > > > public interface so perhaps this could have more stand-alone commentary. > > > > > > Also, what's the reasoning for the name variance between > > Display::SetColorSpace, > > > Display::SetVisible, Display::Resize and these methods? > > > > Done. > > > Given the above, it's not appropriate to reference cc::Display to document these > methods. Instead, you should at least refer to ContextFactoryPrivate yes? Done.
Thanks for improving the comments. lgtm
The CQ bit was checked by staraz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, jbauman@chromium.org, tsepez@chromium.org, fsamuel@chromium.org Link to the patchset: https://codereview.chromium.org/2579693004/#ps240001 (title: "Changed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: 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-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by staraz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rjkroege@chromium.org, msw@chromium.org, jbauman@chromium.org, tsepez@chromium.org, fsamuel@chromium.org Link to the patchset: https://codereview.chromium.org/2579693004/#ps260001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by staraz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
I've seen this failure before, you need to reference //ui/gfx/mojo directly in
internal_interfaces:
mojom("internal_interfaces") {
sources = [
"display_compositor.mojom",
]
public_deps = [
":interfaces",
"//gpu/ipc/common:interfaces",
"//ui/gfx/geometry/mojo",
"//ui/gfx/mojo",
]
}
https://codereview.chromium.org/2579693004/diff/260001/cc/ipc/display_composi...
File cc/ipc/display_compositor.mojom (right):
https://codereview.chromium.org/2579693004/diff/260001/cc/ipc/display_composi...
cc/ipc/display_compositor.mojom:49: // CreateOffscreenCompositorFrameSink is
used by non-privileged clients.
nit: unprivileged
https://codereview.chromium.org/2579693004/diff/260001/services/ui/ws/frame_g...
File services/ui/ws/frame_generator.cc (right):
https://codereview.chromium.org/2579693004/diff/260001/services/ui/ws/frame_g...
services/ui/ws/frame_generator.cc:45: cc::mojom::DisplayPrivatePtr
display_private;
nit: unused variable
The CQ bit was checked by staraz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rjkroege@chromium.org, msw@chromium.org, jbauman@chromium.org, tsepez@chromium.org, fsamuel@chromium.org Link to the patchset: https://codereview.chromium.org/2579693004/#ps280001 (title: "Added deps to ui/gfx/mojo")
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 fsamuel@chromium.org
On 2016/12/21 18:09:10, Fady Samuel wrote:
> I've seen this failure before, you need to reference //ui/gfx/mojo directly in
> internal_interfaces:
>
> mojom("internal_interfaces") {
> sources = [
> "display_compositor.mojom",
> ]
>
> public_deps = [
> ":interfaces",
> "//gpu/ipc/common:interfaces",
> "//ui/gfx/geometry/mojo",
> "//ui/gfx/mojo",
> ]
> }
>
>
https://codereview.chromium.org/2579693004/diff/260001/cc/ipc/display_composi...
> File cc/ipc/display_compositor.mojom (right):
>
>
https://codereview.chromium.org/2579693004/diff/260001/cc/ipc/display_composi...
> cc/ipc/display_compositor.mojom:49: // CreateOffscreenCompositorFrameSink is
> used by non-privileged clients.
> nit: unprivileged
>
>
https://codereview.chromium.org/2579693004/diff/260001/services/ui/ws/frame_g...
> File services/ui/ws/frame_generator.cc (right):
>
>
https://codereview.chromium.org/2579693004/diff/260001/services/ui/ws/frame_g...
> services/ui/ws/frame_generator.cc:45: cc::mojom::DisplayPrivatePtr
> display_private;
> nit: unused variable
Please address other comments as well!
Looks like you're lacking an OWNER for cc/ipc/BUILD.gn. Please TBR=danakj@chromium.org
Description was changed from ========== GpuCompositorFrameSink implements cc::mojom::DisplayPrivate Added DisplayPrivate mojo interface to hide cc::Display from ContextFactory. The intent of this change is to content mus-ws with mus-gpu for cc::Display operations. ColorSpace and OutputIsSecure and so on is passed along from display management code which resides in Mus-ws. cc::Display lives in mus-gpu and so we need a mojo interface for cc::Display. Currently ash talks to cc::Display through ContextFactoryPrivate. This mojo interface would eventually replace ContextFactoryPrivate or would be used by ContextFactoryPrivate. BUG=670364 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== GpuCompositorFrameSink implements cc::mojom::DisplayPrivate Added DisplayPrivate mojo interface to hide cc::Display from ContextFactory. The intent of this change is to content mus-ws with mus-gpu for cc::Display operations. ColorSpace and OutputIsSecure and so on is passed along from display management code which resides in Mus-ws. cc::Display lives in mus-gpu and so we need a mojo interface for cc::Display. Currently ash talks to cc::Display through ContextFactoryPrivate. This mojo interface would eventually replace ContextFactoryPrivate or would be used by ContextFactoryPrivate. BUG=670364 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel TBR=danakj@chromium.org ==========
https://codereview.chromium.org/2579693004/diff/260001/cc/ipc/display_composi... File cc/ipc/display_compositor.mojom (right): https://codereview.chromium.org/2579693004/diff/260001/cc/ipc/display_composi... cc/ipc/display_compositor.mojom:49: // CreateOffscreenCompositorFrameSink is used by non-privileged clients. On 2016/12/21 18:09:09, Fady Samuel wrote: > nit: unprivileged Done. https://codereview.chromium.org/2579693004/diff/260001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2579693004/diff/260001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:45: cc::mojom::DisplayPrivatePtr display_private; On 2016/12/21 18:09:09, Fady Samuel wrote: > nit: unused variable Done.
danakj@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/2579693004/diff/300001/cc/ipc/BUILD.gn File cc/ipc/BUILD.gn (right): https://codereview.chromium.org/2579693004/diff/300001/cc/ipc/BUILD.gn#newcode81 cc/ipc/BUILD.gn:81: "//ui/gfx/mojo", LGTM
The CQ bit was checked by staraz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rjkroege@chromium.org, msw@chromium.org, jbauman@chromium.org, tsepez@chromium.org, fsamuel@chromium.org Link to the patchset: https://codereview.chromium.org/2579693004/#ps300001 (title: "Address comments")
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": 300001, "attempt_start_ts": 1482346044470520,
"parent_rev": "92eedda7c2dc20742e72d9eae5bff3f58946cac4", "commit_rev":
"5bea2c569c456961776bc0d2d3845c28d74548a0"}
Message was sent while issue was closed.
Description was changed from ========== GpuCompositorFrameSink implements cc::mojom::DisplayPrivate Added DisplayPrivate mojo interface to hide cc::Display from ContextFactory. The intent of this change is to content mus-ws with mus-gpu for cc::Display operations. ColorSpace and OutputIsSecure and so on is passed along from display management code which resides in Mus-ws. cc::Display lives in mus-gpu and so we need a mojo interface for cc::Display. Currently ash talks to cc::Display through ContextFactoryPrivate. This mojo interface would eventually replace ContextFactoryPrivate or would be used by ContextFactoryPrivate. BUG=670364 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel TBR=danakj@chromium.org ========== to ========== GpuCompositorFrameSink implements cc::mojom::DisplayPrivate Added DisplayPrivate mojo interface to hide cc::Display from ContextFactory. The intent of this change is to content mus-ws with mus-gpu for cc::Display operations. ColorSpace and OutputIsSecure and so on is passed along from display management code which resides in Mus-ws. cc::Display lives in mus-gpu and so we need a mojo interface for cc::Display. Currently ash talks to cc::Display through ContextFactoryPrivate. This mojo interface would eventually replace ContextFactoryPrivate or would be used by ContextFactoryPrivate. BUG=670364 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel TBR=danakj@chromium.org Review-Url: https://codereview.chromium.org/2579693004 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== GpuCompositorFrameSink implements cc::mojom::DisplayPrivate Added DisplayPrivate mojo interface to hide cc::Display from ContextFactory. The intent of this change is to content mus-ws with mus-gpu for cc::Display operations. ColorSpace and OutputIsSecure and so on is passed along from display management code which resides in Mus-ws. cc::Display lives in mus-gpu and so we need a mojo interface for cc::Display. Currently ash talks to cc::Display through ContextFactoryPrivate. This mojo interface would eventually replace ContextFactoryPrivate or would be used by ContextFactoryPrivate. BUG=670364 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel TBR=danakj@chromium.org Review-Url: https://codereview.chromium.org/2579693004 ========== to ========== GpuCompositorFrameSink implements cc::mojom::DisplayPrivate Added DisplayPrivate mojo interface to hide cc::Display from ContextFactory. The intent of this change is to content mus-ws with mus-gpu for cc::Display operations. ColorSpace and OutputIsSecure and so on is passed along from display management code which resides in Mus-ws. cc::Display lives in mus-gpu and so we need a mojo interface for cc::Display. Currently ash talks to cc::Display through ContextFactoryPrivate. This mojo interface would eventually replace ContextFactoryPrivate or would be used by ContextFactoryPrivate. BUG=670364 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel TBR=danakj@chromium.org Committed: https://crrev.com/dc7e15988c2612f4bdd66e29b238a4eb3fce5cee Cr-Commit-Position: refs/heads/master@{#440192} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/dc7e15988c2612f4bdd66e29b238a4eb3fce5cee Cr-Commit-Position: refs/heads/master@{#440192} |
