Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2763143002:
Remove FrameGenerator::root_window_ (Closed)
DescriptionRemove FrameGenerator::root_window_
FrameGenerator should be as simple as possible.
PlatformDisplayDefault implements ServerWindowObserver and notify its
FrameGenerator when the visibility or bounds of the root window changes.
FrameGenerator updates window visibility and bounds when it gets notified and
requests BeginFrames accordingly.
BUG=683732
Review-Url: https://codereview.chromium.org/2763143002
Cr-Commit-Position: refs/heads/master@{#459157}
Committed: https://chromium.googlesource.com/chromium/src/+/394caf0ffa05d6c7ada7560a3379f4807215bfb4
Patch Set 1 #
Total comments: 16
Patch Set 2 : Rebased and addressed comments #
Total comments: 7
Patch Set 3 : Addressed comments #
Total comments: 18
Patch Set 4 : Addressed comments #
Total comments: 6
Patch Set 5 : Addressed nits #
Total comments: 2
Patch Set 6 : PlatformDisplayDefault does not implement ServerWindowObserver #
Total comments: 4
Patch Set 7 : Addressed comments #
Total comments: 10
Patch Set 8 : Addressed comments #
Messages
Total messages: 53 (27 generated)
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
staraz@chromium.org changed reviewers: + fsamuel@chromium.org
PTAL
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.
Ohh sorry I forgot to send these out.. https://codereview.chromium.org/2763143002/diff/1/services/ui/ws/frame_genera... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2763143002/diff/1/services/ui/ws/frame_genera... services/ui/ws/frame_generator.cc:39: if (window_manager_surface_info_.is_valid()) Once you move this condition to SetNeedsBeginFrame then you can delete this. https://codereview.chromium.org/2763143002/diff/1/services/ui/ws/frame_genera... services/ui/ws/frame_generator.cc:48: if (window_manager_surface_info_.is_valid()) Once you move this condition to SetNeedsBeginFrame then you can delete this. https://codereview.chromium.org/2763143002/diff/1/services/ui/ws/frame_genera... services/ui/ws/frame_generator.cc:64: if (window_manager_surface_info_.is_valid()) Once you move this condition to SetNeedsBeginFrame then you can delete this. https://codereview.chromium.org/2763143002/diff/1/services/ui/ws/frame_genera... services/ui/ws/frame_generator.cc:73: if (visible && window_manager_surface_info_.is_valid()) SetNeedsBeginFrame(visible)? https://codereview.chromium.org/2763143002/diff/1/services/ui/ws/frame_genera... services/ui/ws/frame_generator.cc:74: SetNeedsBeginFrame(true); You know, maybe we should move window_manager_surface_info_ into SetNeedsBeginFrame(..)? https://codereview.chromium.org/2763143002/diff/1/services/ui/ws/frame_genera... services/ui/ws/frame_generator.cc:83: SetNeedsBeginFrame(true); SetNeedsBeginFrame(true); https://codereview.chromium.org/2763143002/diff/1/services/ui/ws/frame_genera... services/ui/ws/frame_generator.cc:210: if (!is_window_visible_ || needs_begin_frame == observing_begin_frames_) This isn't quite right... needs_begin_frame &= is_window_visible_; if (needs_begin_frame == observing_begin_frames_) ... https://codereview.chromium.org/2763143002/diff/1/services/ui/ws/frame_genera... File services/ui/ws/frame_generator.h (right): https://codereview.chromium.org/2763143002/diff/1/services/ui/ws/frame_genera... services/ui/ws/frame_generator.h:37: bool visible, Hmm, we don't do this for device_scale_factor... Maybe just call these AFTER creating FrameGenerator for the sake of consistency?
https://codereview.chromium.org/2763143002/diff/1/services/ui/ws/frame_genera... File services/ui/ws/frame_generator.h (right): https://codereview.chromium.org/2763143002/diff/1/services/ui/ws/frame_genera... services/ui/ws/frame_generator.h:37: bool visible, On 2017/03/22 03:38:39, Fady Samuel wrote: > Hmm, we don't do this for device_scale_factor... Maybe just call these AFTER > creating FrameGenerator for the sake of consistency? I did it this way to preserve the same behavior. Changing visibility or window bounds triggers BeginFrames. Initializing them in the constructor makes sure that they stay consistent with the root window without requesting more BeginFrames than it currently does. Setting them after creation (like what we do with device scale factor) could result in FrameGenerator getting 2 more BeginFrames. Are we OK with that?
On 2017/03/22 13:17:18, Alex Z. wrote: > https://codereview.chromium.org/2763143002/diff/1/services/ui/ws/frame_genera... > File services/ui/ws/frame_generator.h (right): > > https://codereview.chromium.org/2763143002/diff/1/services/ui/ws/frame_genera... > services/ui/ws/frame_generator.h:37: bool visible, > On 2017/03/22 03:38:39, Fady Samuel wrote: > > Hmm, we don't do this for device_scale_factor... Maybe just call these AFTER > > creating FrameGenerator for the sake of consistency? > > I did it this way to preserve the same behavior. Changing visibility or window > bounds triggers BeginFrames. Initializing them in the > constructor makes sure that they stay consistent with the root window > without requesting more BeginFrames than it currently does. > > Setting them after creation (like what we do with device scale factor) > could result in FrameGenerator getting 2 more BeginFrames. Are we OK > with that? Actually, it shouldn't. We request a BeginFrame once, right? If we've already requested a BeginFrame, we don't request it again... SetNeedsBeginFrame(true); SetNeedsBeginFrame(true); will only get us a single BeginFrame. BeginFrames will not tick between those two calls.
staraz@chromium.org changed reviewers: + eseckler@chromium.org
eseckler@: Please review changes in frame_generator_unittest.cc::BeginFrameWhileInvisible. https://codereview.chromium.org/2763143002/diff/1/services/ui/ws/frame_genera... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2763143002/diff/1/services/ui/ws/frame_genera... services/ui/ws/frame_generator.cc:39: if (window_manager_surface_info_.is_valid()) On 2017/03/22 03:38:38, Fady Samuel wrote: > Once you move this condition to SetNeedsBeginFrame then you can delete this. Done. https://codereview.chromium.org/2763143002/diff/1/services/ui/ws/frame_genera... services/ui/ws/frame_generator.cc:48: if (window_manager_surface_info_.is_valid()) On 2017/03/22 03:38:39, Fady Samuel wrote: > Once you move this condition to SetNeedsBeginFrame then you can delete this. Done. https://codereview.chromium.org/2763143002/diff/1/services/ui/ws/frame_genera... services/ui/ws/frame_generator.cc:64: if (window_manager_surface_info_.is_valid()) On 2017/03/22 03:38:39, Fady Samuel wrote: > Once you move this condition to SetNeedsBeginFrame then you can delete this. Done. https://codereview.chromium.org/2763143002/diff/1/services/ui/ws/frame_genera... services/ui/ws/frame_generator.cc:73: if (visible && window_manager_surface_info_.is_valid()) On 2017/03/22 03:38:38, Fady Samuel wrote: > SetNeedsBeginFrame(visible)? Done. https://codereview.chromium.org/2763143002/diff/1/services/ui/ws/frame_genera... services/ui/ws/frame_generator.cc:74: SetNeedsBeginFrame(true); On 2017/03/22 03:38:38, Fady Samuel wrote: > You know, maybe we should move window_manager_surface_info_ into > SetNeedsBeginFrame(..)? Done. https://codereview.chromium.org/2763143002/diff/1/services/ui/ws/frame_genera... services/ui/ws/frame_generator.cc:210: if (!is_window_visible_ || needs_begin_frame == observing_begin_frames_) On 2017/03/22 03:38:39, Fady Samuel wrote: > This isn't quite right... > > needs_begin_frame &= is_window_visible_; > if (needs_begin_frame == observing_begin_frames_) > ... Done. https://codereview.chromium.org/2763143002/diff/1/services/ui/ws/frame_genera... File services/ui/ws/frame_generator.h (right): https://codereview.chromium.org/2763143002/diff/1/services/ui/ws/frame_genera... services/ui/ws/frame_generator.h:37: bool visible, On 2017/03/22 03:38:39, Fady Samuel wrote: > Hmm, we don't do this for device_scale_factor... Maybe just call these AFTER > creating FrameGenerator for the sake of consistency? Done.
On 2017/03/22 14:56:44, Alex Z. wrote: > eseckler@: Please review changes in > frame_generator_unittest.cc::BeginFrameWhileInvisible. Those changes lgtm :)
https://codereview.chromium.org/2763143002/diff/20001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator.cc (left): https://codereview.chromium.org/2763143002/diff/20001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.cc:39: SetNeedsBeginFrame(true); alignment. https://codereview.chromium.org/2763143002/diff/20001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.cc:48: SetNeedsBeginFrame(true); This alignment is wrong. https://codereview.chromium.org/2763143002/diff/20001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2763143002/diff/20001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.cc:211: if (!window_manager_surface_info_.is_valid()) This seems wrong. https://codereview.chromium.org/2763143002/diff/20001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.cc:214: needs_begin_frame &= is_window_visible_; Maybe: needs_begin_frame &= is_window_visible_ && window_manager_surface_info_.is_valid();
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
https://codereview.chromium.org/2763143002/diff/20001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator.cc (left): https://codereview.chromium.org/2763143002/diff/20001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.cc:39: SetNeedsBeginFrame(true); On 2017/03/22 15:32:27, Fady Samuel wrote: > alignment. Done. https://codereview.chromium.org/2763143002/diff/20001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.cc:48: SetNeedsBeginFrame(true); On 2017/03/22 15:32:27, Fady Samuel wrote: > This alignment is wrong. Done. https://codereview.chromium.org/2763143002/diff/20001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2763143002/diff/20001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.cc:214: needs_begin_frame &= is_window_visible_; On 2017/03/22 15:32:27, Fady Samuel wrote: > Maybe: > > needs_begin_frame &= is_window_visible_ && > window_manager_surface_info_.is_valid(); 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: This issue passed the CQ dry run.
https://codereview.chromium.org/2763143002/diff/40001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator.h (right): https://codereview.chromium.org/2763143002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:11: #include "base/timer/timer.h" nit: Is this necessary? https://codereview.chromium.org/2763143002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:12: #include "cc/ipc/display_compositor.mojom.h" nit: is this necessary? https://codereview.chromium.org/2763143002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:15: #include "cc/surfaces/frame_sink_id.h" nit: is this necessary? https://codereview.chromium.org/2763143002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:16: #include "cc/surfaces/local_surface_id_allocator.h" nit: is this necessary? https://codereview.chromium.org/2763143002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:18: #include "cc/surfaces/surface_reference.h" nit: is this necessary? https://codereview.chromium.org/2763143002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:19: #include "services/ui/public/interfaces/window_tree_constants.mojom.h" nit: Is this necessary? https://codereview.chromium.org/2763143002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:82: bool is_window_visible_; is_window_visible_ = false; https://codereview.chromium.org/2763143002/diff/40001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2763143002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator_unittest.cc:161: frame_generator_->OnWindowVisibilityChanged(kVisibility); OnWindowVisibilityChanged(true); is more readable. https://codereview.chromium.org/2763143002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator_unittest.cc:162: frame_generator_->OnWindowBoundsChanged(kArbitraryRect); gfx::Rect(1, 2) is more readable.
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
https://codereview.chromium.org/2763143002/diff/40001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator.h (right): https://codereview.chromium.org/2763143002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:11: #include "base/timer/timer.h" On 2017/03/22 18:19:40, Fady Samuel wrote: > nit: Is this necessary? Done. https://codereview.chromium.org/2763143002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:12: #include "cc/ipc/display_compositor.mojom.h" On 2017/03/22 18:19:40, Fady Samuel wrote: > nit: is this necessary? Done. https://codereview.chromium.org/2763143002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:15: #include "cc/surfaces/frame_sink_id.h" On 2017/03/22 18:19:40, Fady Samuel wrote: > nit: is this necessary? Done. https://codereview.chromium.org/2763143002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:16: #include "cc/surfaces/local_surface_id_allocator.h" On 2017/03/22 18:19:40, Fady Samuel wrote: > nit: is this necessary? Done. https://codereview.chromium.org/2763143002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:18: #include "cc/surfaces/surface_reference.h" On 2017/03/22 18:19:40, Fady Samuel wrote: > nit: is this necessary? Done. https://codereview.chromium.org/2763143002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:19: #include "services/ui/public/interfaces/window_tree_constants.mojom.h" On 2017/03/22 18:19:40, Fady Samuel wrote: > nit: Is this necessary? Done. https://codereview.chromium.org/2763143002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:82: bool is_window_visible_; On 2017/03/22 18:19:40, Fady Samuel wrote: > is_window_visible_ = false; Done. https://codereview.chromium.org/2763143002/diff/40001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2763143002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator_unittest.cc:161: frame_generator_->OnWindowVisibilityChanged(kVisibility); On 2017/03/22 18:19:40, Fady Samuel wrote: > OnWindowVisibilityChanged(true); is more readable. Done. https://codereview.chromium.org/2763143002/diff/40001/services/ui/ws/frame_ge... services/ui/ws/frame_generator_unittest.cc:162: frame_generator_->OnWindowBoundsChanged(kArbitraryRect); On 2017/03/22 18:19:40, Fady Samuel wrote: > gfx::Rect(1, 2) is more readable. Done.
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 these nits are addressed. https://codereview.chromium.org/2763143002/diff/60001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator.h (right): https://codereview.chromium.org/2763143002/diff/60001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:17: #include "ui/gfx/native_widget_types.h" nit: You probably don't need this either. https://codereview.chromium.org/2763143002/diff/60001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:32: FrameGenerator( nit explicit https://codereview.chromium.org/2763143002/diff/60001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:79: ; remove this.
https://codereview.chromium.org/2763143002/diff/60001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator.h (right): https://codereview.chromium.org/2763143002/diff/60001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:17: #include "ui/gfx/native_widget_types.h" On 2017/03/22 19:02:19, Fady Samuel wrote: > nit: You probably don't need this either. Done. https://codereview.chromium.org/2763143002/diff/60001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:32: FrameGenerator( On 2017/03/22 19:02:19, Fady Samuel wrote: > nit explicit Done. https://codereview.chromium.org/2763143002/diff/60001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:79: ; On 2017/03/22 19:02:19, Fady Samuel wrote: > remove this. Done.
staraz@chromium.org changed reviewers: + msw@chromium.org
msw@: Please review changes in platform_display_default*.
lgtm
https://codereview.chromium.org/2763143002/diff/80001/services/ui/ws/platform... File services/ui/ws/platform_display_default.cc (right): https://codereview.chromium.org/2763143002/diff/80001/services/ui/ws/platform... services/ui/ws/platform_display_default.cc:273: void PlatformDisplayDefault::OnWindowBoundsChanged( Why not make FrameGenerator observe the ServerWindow directly? (it seems odd to make PlatformDisplayDefault forward these calls)
Description was changed from ========== Remove FrameGenerator::root_window_ FrameGenerator should be as simple as possible. PlatformDisplayDefault implements ServerWindowObserver and notify its FrameGenerator when the visibility or bounds of the root window changes. FrameGenerator updates window visibility and bounds when it gets notified and requests BeginFrames accordingly. BUG=683732 ========== to ========== Remove FrameGenerator::root_window_ FrameGenerator should be as simple as possible. PlatformDisplayDefault implements ServerWindowObserver and notify its FrameGenerator when the visibility or bounds of the root window changes. FrameGenerator updates window visibility and bounds when it gets notified and requests BeginFrames accordingly. BUG=683732 ==========
https://codereview.chromium.org/2763143002/diff/80001/services/ui/ws/platform... File services/ui/ws/platform_display_default.cc (right): https://codereview.chromium.org/2763143002/diff/80001/services/ui/ws/platform... services/ui/ws/platform_display_default.cc:273: void PlatformDisplayDefault::OnWindowBoundsChanged( On 2017/03/22 19:36:52, msw wrote: > Why not make FrameGenerator observe the ServerWindow directly? > (it seems odd to make PlatformDisplayDefault forward these calls) We wanted to make FrameGenerator as simple as possible. After some offline discussion, we decided to update bounds in PlatformDisplayDefault::UpdateViewportMetrics() and remove FrameGenerator::is_window_visible_.
lgtm
https://codereview.chromium.org/2763143002/diff/100001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.h (right): https://codereview.chromium.org/2763143002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator.h:42: void OnWindowBoundsChanged(const gfx::Rect& bounds); This should just be a size. The origin isn't important in FrameGenerator. https://codereview.chromium.org/2763143002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator.h:76: gfx::Rect bounds_; gfx::Size pixel_size_ maybe?
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
https://codereview.chromium.org/2763143002/diff/100001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.h (right): https://codereview.chromium.org/2763143002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator.h:42: void OnWindowBoundsChanged(const gfx::Rect& bounds); On 2017/03/22 20:58:48, kylechar wrote: > This should just be a size. The origin isn't important in FrameGenerator. Done. https://codereview.chromium.org/2763143002/diff/100001/services/ui/ws/frame_g... services/ui/ws/frame_generator.h:76: gfx::Rect bounds_; On 2017/03/22 20:58:48, kylechar wrote: > gfx::Size pixel_size_ maybe? Done.
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/2763143002/diff/120001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2763143002/diff/120001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:61: void FrameGenerator::OnWindowBoundsChanged(const gfx::Size& pixel_size) { nit: BoundsChange isn't quite right now. It's Size change...
kylechar@chromium.org changed reviewers: + kylechar@chromium.org
A few nits. https://codereview.chromium.org/2763143002/diff/120001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2763143002/diff/120001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:131: const gfx::Rect bounds(pixel_size_.width(), pixel_size_.height()); There is a gfx::Rect constructor that takes a gfx::Size, that will be cleaner. https://codereview.chromium.org/2763143002/diff/120001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:173: const gfx::Rect boundsat_origin( Accidental refactor? https://codereview.chromium.org/2763143002/diff/120001/services/ui/ws/platfor... File services/ui/ws/platform_display_default.cc (right): https://codereview.chromium.org/2763143002/diff/120001/services/ui/ws/platfor... services/ui/ws/platform_display_default.cc:137: frame_generator_->OnWindowBoundsChanged(bounds.size()); optional: metrics.pixel_size, or move this down into the if block below and use metrics_.pixel_size; https://codereview.chromium.org/2763143002/diff/120001/services/ui/ws/platfor... services/ui/ws/platform_display_default.cc:262: frame_generator_->OnWindowBoundsChanged(root_window_->bounds().size()); metrics_.pixel_size here?
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_...)
https://codereview.chromium.org/2763143002/diff/120001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2763143002/diff/120001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:61: void FrameGenerator::OnWindowBoundsChanged(const gfx::Size& pixel_size) { On 2017/03/22 21:21:37, Fady Samuel wrote: > nit: BoundsChange isn't quite right now. It's Size change... Done. https://codereview.chromium.org/2763143002/diff/120001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:131: const gfx::Rect bounds(pixel_size_.width(), pixel_size_.height()); On 2017/03/22 21:27:36, kylechar wrote: > There is a gfx::Rect constructor that takes a gfx::Size, that will be cleaner. Done. https://codereview.chromium.org/2763143002/diff/120001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:173: const gfx::Rect boundsat_origin( On 2017/03/22 21:27:36, kylechar wrote: > Accidental refactor? Done. https://codereview.chromium.org/2763143002/diff/120001/services/ui/ws/platfor... File services/ui/ws/platform_display_default.cc (right): https://codereview.chromium.org/2763143002/diff/120001/services/ui/ws/platfor... services/ui/ws/platform_display_default.cc:137: frame_generator_->OnWindowBoundsChanged(bounds.size()); On 2017/03/22 21:27:36, kylechar wrote: > optional: metrics.pixel_size, or move this down into the if block below and use > metrics_.pixel_size; Done. https://codereview.chromium.org/2763143002/diff/120001/services/ui/ws/platfor... services/ui/ws/platform_display_default.cc:262: frame_generator_->OnWindowBoundsChanged(root_window_->bounds().size()); On 2017/03/22 21:27:36, kylechar wrote: > metrics_.pixel_size here? Done.
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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by staraz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eseckler@chromium.org, fsamuel@chromium.org Link to the patchset: https://codereview.chromium.org/2763143002/#ps140001 (title: "Addressed 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": 140001, "attempt_start_ts": 1490293916743300,
"parent_rev": "e6dd14cf70705132b492d376d34acc841dec88cc", "commit_rev":
"394caf0ffa05d6c7ada7560a3379f4807215bfb4"}
Message was sent while issue was closed.
Description was changed from ========== Remove FrameGenerator::root_window_ FrameGenerator should be as simple as possible. PlatformDisplayDefault implements ServerWindowObserver and notify its FrameGenerator when the visibility or bounds of the root window changes. FrameGenerator updates window visibility and bounds when it gets notified and requests BeginFrames accordingly. BUG=683732 ========== to ========== Remove FrameGenerator::root_window_ FrameGenerator should be as simple as possible. PlatformDisplayDefault implements ServerWindowObserver and notify its FrameGenerator when the visibility or bounds of the root window changes. FrameGenerator updates window visibility and bounds when it gets notified and requests BeginFrames accordingly. BUG=683732 Review-Url: https://codereview.chromium.org/2763143002 Cr-Commit-Position: refs/heads/master@{#459157} Committed: https://chromium.googlesource.com/chromium/src/+/394caf0ffa05d6c7ada7560a3379... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/394caf0ffa05d6c7ada7560a3379... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
