Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2738923002:
Add DisplayClientCompositorFrameSink (Closed)
DescriptionAdd DisplayClientCompositorFrameSink class.
DisplayClientCompositorFrameSink can be understood as a
ClientCompositorFrameSink that also has a DisplayPrivate interface
pointer.
This is part of the effort to make FrameGenerator mockable and unit
test-able.
DisplayClientCompositorFrameSink will be used by FrameGenerator in the
future where FrameGenerator takes a cc::CompositorFrameSink*
(DisplayClientCompositorFramesink by default) upon creation. We can then
unit test FrameGenerator with a TestCompositorFrameSink.
BUG=683732
Review-Url: https://codereview.chromium.org/2738923002
Cr-Commit-Position: refs/heads/master@{#456282}
Committed: https://chromium.googlesource.com/chromium/src/+/72df4334a57fdd13b8dbbc7cd48895fcd79b3a63
Patch Set 1 #Patch Set 2 : Implement DisplayClientCompositorFrameSink #
Total comments: 2
Patch Set 3 : Moved DisplayClientCompositorFrameSink to services/ui/ws #Patch Set 4 : Fixed includes #Patch Set 5 : Crashing mojo #
Total comments: 12
Patch Set 6 : Addressed comments #Patch Set 7 : SetLocalSurfaceId before submitting CompositorFrame #Patch Set 8 : Updated BeginFrame requests logic and other fixes from fsamuel@ #
Total comments: 23
Patch Set 9 : clean up #
Total comments: 16
Patch Set 10 : addressed comments #
Total comments: 20
Patch Set 11 : addressed nits #
Messages
Total messages: 39 (21 generated)
Description was changed from ========== WIP: Add DisplayClientCompositorFrameSink BUG=683732 ========== to ========== Add DisplayClientCompositorFrameSink class. DisplayClientCompositorFrameSink can be understood as a ClientCompositorFrameSink that also has a DisplayPrivate interface pointer. This is part of the effort to make FrameGenerator mockable and unit test-able. DisplayClientCompositorFrameSink will be used by FrameGenerator in the future where FrameGenerator takes a cc::CompositorFrameSink* (DisplayClientCompositorFramesink by default) upon creation. We can then unit test FrameGenerator with a TestCompositorFrameSink. BUG=683732 ==========
Description was changed from ========== Add DisplayClientCompositorFrameSink class. DisplayClientCompositorFrameSink can be understood as a ClientCompositorFrameSink that also has a DisplayPrivate interface pointer. This is part of the effort to make FrameGenerator mockable and unit test-able. DisplayClientCompositorFrameSink will be used by FrameGenerator in the future where FrameGenerator takes a cc::CompositorFrameSink* (DisplayClientCompositorFramesink by default) upon creation. We can then unit test FrameGenerator with a TestCompositorFrameSink. BUG=683732 ========== to ========== Add DisplayClientCompositorFrameSink class. DisplayClientCompositorFrameSink can be understood as a ClientCompositorFrameSink that also has a DisplayPrivate interface pointer. This is part of the effort to make FrameGenerator mockable and unit test-able. DisplayClientCompositorFrameSink will be used by FrameGenerator in the future where FrameGenerator takes a cc::CompositorFrameSink* (DisplayClientCompositorFramesink by default) upon creation. We can then unit test FrameGenerator with a TestCompositorFrameSink. BUG=683732 ==========
staraz@chromium.org changed reviewers: + fsamuel@chromium.org, msw@chromium.org
fsamuel@: Please review DisplayClientCompositorFrameSink. msw@: Please review BUILD.gn.
This looks good so far. Please move DisplayClientCompositorFrameSink into services/ui/ws. Please update FrameGenerator to use it. https://codereview.chromium.org/2738923002/diff/20001/services/ui/public/cpp/... File services/ui/public/cpp/display_client_compositor_frame_sink.h (right): https://codereview.chromium.org/2738923002/diff/20001/services/ui/public/cpp/... services/ui/public/cpp/display_client_compositor_frame_sink.h:24: class DisplayClientCompositorFrameSink Put this in the window server: services/ui/ws as this should not be used by external clients.
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
https://codereview.chromium.org/2738923002/diff/20001/services/ui/public/cpp/... File services/ui/public/cpp/display_client_compositor_frame_sink.h (right): https://codereview.chromium.org/2738923002/diff/20001/services/ui/public/cpp/... services/ui/public/cpp/display_client_compositor_frame_sink.h:24: class DisplayClientCompositorFrameSink On 2017/03/09 18:50:10, Fady Samuel wrote: > Put this in the window server: services/ui/ws as this should not be used by > external clients. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng 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_...)
https://codereview.chromium.org/2738923002/diff/80001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator.cc (left): https://codereview.chromium.org/2738923002/diff/80001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.cc:63: compositor_frame_sink_->SetNeedsBeginFrame(true); What you want instead is to add an observer on the BeginFrameSource. https://codereview.chromium.org/2738923002/diff/80001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2738923002/diff/80001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.cc:45: compositor_frame_sink_->BindToClient(this); Do this in the constructor. https://codereview.chromium.org/2738923002/diff/80001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.cc:55: compositor_frame_sink_->BindToClient(this); Add an observer on the begin_frame_source instead. https://codereview.chromium.org/2738923002/diff/80001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.cc:61: compositor_frame_sink_->BindToClient(this); Do this in the constructor. https://codereview.chromium.org/2738923002/diff/80001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.cc:107: compositor_frame_sink_->DetachFromClient(); Don't DetachFromClient here.
https://codereview.chromium.org/2738923002/diff/80001/services/ui/ws/platform... File services/ui/ws/platform_display_default.cc (right): https://codereview.chromium.org/2738923002/diff/80001/services/ui/ws/platform... services/ui/ws/platform_display_default.cc:253: cc::mojom::MojoCompositorFrameSinkAssociatedPtrInfo compositor_frame_sink; These need to be member variables in DisplayClientCompositorFrameSink.
https://codereview.chromium.org/2738923002/diff/80001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator.cc (left): https://codereview.chromium.org/2738923002/diff/80001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.cc:63: compositor_frame_sink_->SetNeedsBeginFrame(true); On 2017/03/09 21:25:18, Fady Samuel wrote: > What you want instead is to add an observer on the BeginFrameSource. Done. https://codereview.chromium.org/2738923002/diff/80001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2738923002/diff/80001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.cc:45: compositor_frame_sink_->BindToClient(this); On 2017/03/09 21:25:19, Fady Samuel wrote: > Do this in the constructor. Done. https://codereview.chromium.org/2738923002/diff/80001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.cc:55: compositor_frame_sink_->BindToClient(this); On 2017/03/09 21:25:19, Fady Samuel wrote: > Add an observer on the begin_frame_source instead. Done. https://codereview.chromium.org/2738923002/diff/80001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.cc:61: compositor_frame_sink_->BindToClient(this); On 2017/03/09 21:25:19, Fady Samuel wrote: > Do this in the constructor. Done. https://codereview.chromium.org/2738923002/diff/80001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.cc:107: compositor_frame_sink_->DetachFromClient(); On 2017/03/09 21:25:18, Fady Samuel wrote: > Don't DetachFromClient here. Done. https://codereview.chromium.org/2738923002/diff/80001/services/ui/ws/platform... File services/ui/ws/platform_display_default.cc (right): https://codereview.chromium.org/2738923002/diff/80001/services/ui/ws/platform... services/ui/ws/platform_display_default.cc:253: cc::mojom::MojoCompositorFrameSinkAssociatedPtrInfo compositor_frame_sink; On 2017/03/09 21:27:48, Fady Samuel wrote: > These need to be member variables in DisplayClientCompositorFrameSink. Done. mash chrome still crashes on startup with stack trace: https://paste.googleplex.com/6651048328429568
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 #8 (id:90007) has been deleted
Dry run: Try jobs failed on following builders: 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 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/2738923002/diff/150001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:29: gfx::AcceleratedWidget widget, Remove this? https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:35: DCHECK_NE(gfx::kNullAcceleratedWidget, widget); Remove this? https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.h (right): https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/frame_g... services/ui/ws/frame_generator.h:20: #include "services/ui/ws/ids.h" I think this can go away? https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/frame_g... services/ui/ws/frame_generator.h:21: #include "services/ui/ws/server_window_delegate.h" I think this can go away? https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/frame_g... services/ui/ws/frame_generator.h:22: #include "services/ui/ws/server_window_tracker.h" I think this can go away? https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/frame_g... services/ui/ws/frame_generator.h:35: class FrameGeneratorTest; Delete this? https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/frame_g... services/ui/ws/frame_generator.h:49: gfx::AcceleratedWidget widget, Remove this? https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/frame_g... services/ui/ws/frame_generator.h:61: friend class ui::ws::test::FrameGeneratorTest; Delete this. https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/platfor... File services/ui/ws/platform_display_default.cc (right): https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/platfor... services/ui/ws/platform_display_default.cc:262: mojo::MakeRequest(&display_private); Let's cut down the number of local variables here because this is hard to read, pass MakeRequest directly. Thanks! https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/platfor... services/ui/ws/platform_display_default.cc:264: widget_, std::move(sink_request), std::move(compositor_frame_sink_client), mojo::MakeRequest(&compositor_frame_sink) https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/platfor... services/ui/ws/platform_display_default.cc:265: std::move(display_request)); MakeRequest(&display_private) https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/platfor... services/ui/ws/platform_display_default.cc:271: std::move(compositor_frame_sink_client_request)); mojo::MakeRequest(&compositor_frame_sink_client)?
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:29: gfx::AcceleratedWidget widget, On 2017/03/10 18:37:57, Fady Samuel wrote: > Remove this? Done. https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:35: DCHECK_NE(gfx::kNullAcceleratedWidget, widget); On 2017/03/10 18:37:57, Fady Samuel wrote: > Remove this? Done. https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.h (right): https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/frame_g... services/ui/ws/frame_generator.h:20: #include "services/ui/ws/ids.h" On 2017/03/10 18:37:57, Fady Samuel wrote: > I think this can go away? Done. https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/frame_g... services/ui/ws/frame_generator.h:21: #include "services/ui/ws/server_window_delegate.h" On 2017/03/10 18:37:57, Fady Samuel wrote: > I think this can go away? Done. https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/frame_g... services/ui/ws/frame_generator.h:22: #include "services/ui/ws/server_window_tracker.h" On 2017/03/10 18:37:57, Fady Samuel wrote: > I think this can go away? Done. https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/frame_g... services/ui/ws/frame_generator.h:35: class FrameGeneratorTest; On 2017/03/10 18:37:57, Fady Samuel wrote: > Delete this? Done. https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/frame_g... services/ui/ws/frame_generator.h:49: gfx::AcceleratedWidget widget, On 2017/03/10 18:37:57, Fady Samuel wrote: > Remove this? Done. https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/frame_g... services/ui/ws/frame_generator.h:61: friend class ui::ws::test::FrameGeneratorTest; On 2017/03/10 18:37:57, Fady Samuel wrote: > Delete this. Done. https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/platfor... File services/ui/ws/platform_display_default.cc (right): https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/platfor... services/ui/ws/platform_display_default.cc:264: widget_, std::move(sink_request), std::move(compositor_frame_sink_client), On 2017/03/10 18:37:57, Fady Samuel wrote: > mojo::MakeRequest(&compositor_frame_sink) Done. https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/platfor... services/ui/ws/platform_display_default.cc:265: std::move(display_request)); On 2017/03/10 18:37:57, Fady Samuel wrote: > MakeRequest(&display_private) Done. https://codereview.chromium.org/2738923002/diff/150001/services/ui/ws/platfor... services/ui/ws/platform_display_default.cc:271: std::move(compositor_frame_sink_client_request)); On 2017/03/10 18:37:57, Fady Samuel wrote: > mojo::MakeRequest(&compositor_frame_sink_client)? Chrome mash window shows no content on startup once I change this to mojo::MakeRequest(&compositor_frame_sink_client); I wonder if it has anything to do with order or evaluation?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
looks good once my remaining comments are addressed. https://codereview.chromium.org/2738923002/diff/170001/services/ui/ws/display... File services/ui/ws/display_client_compositor_frame_sink.cc (right): https://codereview.chromium.org/2738923002/diff/170001/services/ui/ws/display... services/ui/ws/display_client_compositor_frame_sink.cc:77: const cc::BeginFrameArgs& begin_frame_args) { DCHECK(thread_checker_); DCHECK(thread_checker_->CalledOnValidThread()); https://codereview.chromium.org/2738923002/diff/170001/services/ui/ws/display... services/ui/ws/display_client_compositor_frame_sink.cc:97: bool needs_begin_frames) { DCHECK(thread_checker_); DCHECK(thread_checker_->CalledOnValidThread()); https://codereview.chromium.org/2738923002/diff/170001/services/ui/ws/display... File services/ui/ws/display_client_compositor_frame_sink.h (right): https://codereview.chromium.org/2738923002/diff/170001/services/ui/ws/display... services/ui/ws/display_client_compositor_frame_sink.h:33: compositor_frame_sink_info, nit: remove "info" https://codereview.chromium.org/2738923002/diff/170001/services/ui/ws/display... services/ui/ws/display_client_compositor_frame_sink.h:34: cc::mojom::DisplayPrivateAssociatedPtr display_private_info, nit: remove info https://codereview.chromium.org/2738923002/diff/170001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2738923002/diff/170001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:10: #include "base/containers/adapters.h" nit: remove this? https://codereview.chromium.org/2738923002/diff/170001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:19: #include "services/ui/ws/server_window_compositor_frame_sink_manager.h" nit: remove this? https://codereview.chromium.org/2738923002/diff/170001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:20: #include "services/ui/ws/server_window_delegate.h" nit: remove this? https://codereview.chromium.org/2738923002/diff/170001/services/ui/ws/platfor... File services/ui/ws/platform_display_default.h (right): https://codereview.chromium.org/2738923002/diff/170001/services/ui/ws/platfor... services/ui/ws/platform_display_default.h:21: class LocatedEvent; Revert?
https://codereview.chromium.org/2738923002/diff/170001/services/ui/ws/display... File services/ui/ws/display_client_compositor_frame_sink.cc (right): https://codereview.chromium.org/2738923002/diff/170001/services/ui/ws/display... services/ui/ws/display_client_compositor_frame_sink.cc:77: const cc::BeginFrameArgs& begin_frame_args) { On 2017/03/10 19:17:39, Fady Samuel wrote: > DCHECK(thread_checker_); > DCHECK(thread_checker_->CalledOnValidThread()); Done. https://codereview.chromium.org/2738923002/diff/170001/services/ui/ws/display... services/ui/ws/display_client_compositor_frame_sink.cc:97: bool needs_begin_frames) { On 2017/03/10 19:17:40, Fady Samuel wrote: > DCHECK(thread_checker_); > DCHECK(thread_checker_->CalledOnValidThread()); Done. https://codereview.chromium.org/2738923002/diff/170001/services/ui/ws/display... File services/ui/ws/display_client_compositor_frame_sink.h (right): https://codereview.chromium.org/2738923002/diff/170001/services/ui/ws/display... services/ui/ws/display_client_compositor_frame_sink.h:33: compositor_frame_sink_info, On 2017/03/10 19:17:40, Fady Samuel wrote: > nit: remove "info" Done. https://codereview.chromium.org/2738923002/diff/170001/services/ui/ws/display... services/ui/ws/display_client_compositor_frame_sink.h:34: cc::mojom::DisplayPrivateAssociatedPtr display_private_info, On 2017/03/10 19:17:40, Fady Samuel wrote: > nit: remove info Done. https://codereview.chromium.org/2738923002/diff/170001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2738923002/diff/170001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:10: #include "base/containers/adapters.h" On 2017/03/10 19:17:40, Fady Samuel wrote: > nit: remove this? Done. https://codereview.chromium.org/2738923002/diff/170001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:19: #include "services/ui/ws/server_window_compositor_frame_sink_manager.h" On 2017/03/10 19:17:40, Fady Samuel wrote: > nit: remove this? Done. https://codereview.chromium.org/2738923002/diff/170001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:20: #include "services/ui/ws/server_window_delegate.h" On 2017/03/10 19:17:40, Fady Samuel wrote: > nit: remove this? Done. https://codereview.chromium.org/2738923002/diff/170001/services/ui/ws/platfor... File services/ui/ws/platform_display_default.h (right): https://codereview.chromium.org/2738923002/diff/170001/services/ui/ws/platfor... services/ui/ws/platform_display_default.h:21: class LocatedEvent; On 2017/03/10 19:17:40, Fady Samuel wrote: > Revert? This forward declaration is needed for UpdateEventRootLocation() after one of the includes in frame_generator.h is removed.
LGTM
Fady should probably be an owner here; I have far less familiarity in this area. Rubber stamp lgtm with minor comments. https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/display... File services/ui/ws/display_client_compositor_frame_sink.cc (right): https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/display... services/ui/ws/display_client_compositor_frame_sink.cc:31: thread_checker_.reset(new base::ThreadChecker()); nit: = base::MakeUnique https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/display... services/ui/ws/display_client_compositor_frame_sink.cc:47: DCHECK(thread_checker_); nit: these are not really needed right before the CalledOnValidThread DCHECKs. https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/display... services/ui/ws/display_client_compositor_frame_sink.cc:107: // TODO(eseckler): Pass on the ack to compositor_frame_sink_. optional nit: CC eseckler@ and use vertical bars: |compositor_frame_sink_| https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/display... File services/ui/ws/display_client_compositor_frame_sink.h (right): https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/display... services/ui/ws/display_client_compositor_frame_sink.h:15: #include "ui/gfx/geometry/rect.h" optional nit: explicit include not needed (transitive okay for subclass overrides), though you then might need this in the cc file. https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/display... services/ui/ws/display_client_compositor_frame_sink.h:16: #include "ui/gfx/native_widget_types.h" nit: not needed? https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/display... services/ui/ws/display_client_compositor_frame_sink.h:25: class DisplayClientCompositorFrameSink Add an explanatory class comment. https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/display... services/ui/ws/display_client_compositor_frame_sink.h:38: // cc::CompositorFrameSink implementation. nit: be consistent about trailing period/colon https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/display... services/ui/ws/display_client_compositor_frame_sink.h:55: gfx::Size last_submitted_frame_size_; nit: include size.h? https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:190: begin_frame_source_->AddObserver(this); Should this null-check |begin_frame_source_|? ditto below. https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.h (right): https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/frame_g... services/ui/ws/frame_generator.h:79: void SetNeedsBeginFrame(bool needs_begin_frame); nit: comment
https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:190: begin_frame_source_->AddObserver(this); On 2017/03/10 20:08:41, msw wrote: > Should this null-check |begin_frame_source_|? ditto below. I'd argue no because FrameGenerator gets a BeginFrameSource on BindToClient which is called in the constructor. Maybe a DCHECK?
https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:190: begin_frame_source_->AddObserver(this); On 2017/03/10 20:17:58, Fady Samuel wrote: > On 2017/03/10 20:08:41, msw wrote: > > Should this null-check |begin_frame_source_|? ditto below. > > I'd argue no because FrameGenerator gets a BeginFrameSource on BindToClient > which is called in the constructor. Maybe a DCHECK? I asked because of similar checks in FrameGenerator::SetBeginFrameSource
https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:190: begin_frame_source_->AddObserver(this); On 2017/03/10 20:23:29, msw wrote: > On 2017/03/10 20:17:58, Fady Samuel wrote: > > On 2017/03/10 20:08:41, msw wrote: > > > Should this null-check |begin_frame_source_|? ditto below. > > > > I'd argue no because FrameGenerator gets a BeginFrameSource on BindToClient > > which is called in the constructor. Maybe a DCHECK? > > I asked because of similar checks in FrameGenerator::SetBeginFrameSource begin_frame_source_ is null the first time SetBeginFrameSource is called. Though I can't think of a case where begin_frame_source_ is null but observing_begin_frame_source_ is true (i.e. you could say that checking begin_frame_source_ in SetBeginFrameSource() is redundant), I think we can leave it there so the code is consistent with the other SetBeginFrameSource() overrides.
The CQ bit was checked by fsamuel@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/2738923002/diff/190001/services/ui/ws/display... File services/ui/ws/display_client_compositor_frame_sink.cc (right): https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/display... services/ui/ws/display_client_compositor_frame_sink.cc:31: thread_checker_.reset(new base::ThreadChecker()); On 2017/03/10 20:08:40, msw wrote: > nit: = base::MakeUnique Done. https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/display... services/ui/ws/display_client_compositor_frame_sink.cc:47: DCHECK(thread_checker_); On 2017/03/10 20:08:40, msw wrote: > nit: these are not really needed right before the CalledOnValidThread DCHECKs. Done. These are there because I copied them from ClientCompositorFrameSink. https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/display... File services/ui/ws/display_client_compositor_frame_sink.h (right): https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/display... services/ui/ws/display_client_compositor_frame_sink.h:15: #include "ui/gfx/geometry/rect.h" On 2017/03/10 20:08:40, msw wrote: > optional nit: explicit include not needed (transitive okay for subclass > overrides), though you then might need this in the cc file. Done. https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/display... services/ui/ws/display_client_compositor_frame_sink.h:16: #include "ui/gfx/native_widget_types.h" On 2017/03/10 20:08:40, msw wrote: > nit: not needed? Done. https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/display... services/ui/ws/display_client_compositor_frame_sink.h:38: // cc::CompositorFrameSink implementation. On 2017/03/10 20:08:40, msw wrote: > nit: be consistent about trailing period/colon Done. https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/display... services/ui/ws/display_client_compositor_frame_sink.h:55: gfx::Size last_submitted_frame_size_; On 2017/03/10 20:08:40, msw wrote: > nit: include size.h? Done. But why do we need to include size.h but not rect.h? https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.h (right): https://codereview.chromium.org/2738923002/diff/190001/services/ui/ws/frame_g... services/ui/ws/frame_generator.h:79: void SetNeedsBeginFrame(bool needs_begin_frame); On 2017/03/10 20:08:41, msw wrote: > nit: comment Done.
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 Link to the patchset: https://codereview.chromium.org/2738923002/#ps210001 (title: "addressed nits")
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": 210001, "attempt_start_ts": 1489204353342420,
"parent_rev": "60792c5afb8cb1eb84db0a874c95286e529eb850", "commit_rev":
"72df4334a57fdd13b8dbbc7cd48895fcd79b3a63"}
Message was sent while issue was closed.
Description was changed from ========== Add DisplayClientCompositorFrameSink class. DisplayClientCompositorFrameSink can be understood as a ClientCompositorFrameSink that also has a DisplayPrivate interface pointer. This is part of the effort to make FrameGenerator mockable and unit test-able. DisplayClientCompositorFrameSink will be used by FrameGenerator in the future where FrameGenerator takes a cc::CompositorFrameSink* (DisplayClientCompositorFramesink by default) upon creation. We can then unit test FrameGenerator with a TestCompositorFrameSink. BUG=683732 ========== to ========== Add DisplayClientCompositorFrameSink class. DisplayClientCompositorFrameSink can be understood as a ClientCompositorFrameSink that also has a DisplayPrivate interface pointer. This is part of the effort to make FrameGenerator mockable and unit test-able. DisplayClientCompositorFrameSink will be used by FrameGenerator in the future where FrameGenerator takes a cc::CompositorFrameSink* (DisplayClientCompositorFramesink by default) upon creation. We can then unit test FrameGenerator with a TestCompositorFrameSink. BUG=683732 Review-Url: https://codereview.chromium.org/2738923002 Cr-Commit-Position: refs/heads/master@{#456282} Committed: https://chromium.googlesource.com/chromium/src/+/72df4334a57fdd13b8dbbc7cd488... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:210001) as https://chromium.googlesource.com/chromium/src/+/72df4334a57fdd13b8dbbc7cd488... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
