New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 683732 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Cleanup FrameGenerator, ServerWindowCompositorFrameSink

Project Member Reported by fsam...@chromium.org, Jan 22 2017

Issue description

FrameGenerator and ServerWindowCompositorFrameSink seem to have gotten a bit crufty:

1. FrameGenerator gets continuous BeginFrames but it really doesn't need them. FrameGenerator should only request a BeginFrame if something has changed:

a. Screen resolution.
b. Device scale factor
c. Window manager surface ID has changed.

Once a new CompositorFrame is submitted we should ask Mus-Gpu to stop sending BeginFrames. It's wasteful.

2. We probably shouldn't be creating a FrameGenerator until we have an AcceleratedWidget anyway. It has nothing useful to do until then.

3. FrameGenerator::OnSurfaceCreated isn't receiving all the necessary info. It should get a full SurfaceInfo. If SurfaceInfo has changed then it should request  BeginFrame. It shouldn't get an OnSurfaceCreated for other windows. Taking in a ServerWindow seems silly.

4. delegate_->GetActiveWindow() is confusing and unnecessary once 3 is implemented. OnSurfaceCreated will only be called when the "active window" changes.

5. FrameGenerator likely doesn't need to be a ServerWindowTracker anymore. OnWindowDestroying can go away.

6. ServerWindowCompositorFrameSinkManager::GetLatestSurfaceId is only used by FrameGenerator and FrameGenerator should know about the window manager's SurfaceId from OnSurfaceCreated so this is redundant and can go away.

7. ServerWindowCompositorFrameSinkManager::GetLatestFrameSize is only used in tests and can go away.

8. ServerWindowCompositorFrameSinkManager::window() does not appear to be used.

 
Blocking: 601863
Owner: samans@chromium.org
Status: Assigned (was: Untriaged)
Project Member

Comment 3 by bugdroid1@chromium.org, Jan 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/401c5932118fb24071348b6a6ee5ce1638a7d982

commit 401c5932118fb24071348b6a6ee5ce1638a7d982
Author: samans <samans@chromium.org>
Date: Wed Jan 25 01:30:42 2017

FrameGenerator should receive SurfaceInfo and use it in frame generation

FrameGenerator::OnSurfaceCreated should get a full SurfaceInfo object,
as opposed to a surface id, and use that in frame generation instead of
depending on a window object.

FrameGenerator should not receive begin frames unless the SurfaceInfo
object or device_scale_factor_ changes.

Fixed a bug in SurfaceFactory, where the frame size was not calculated
properly.

BUG= 683732 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2651843002
Cr-Commit-Position: refs/heads/master@{#445901}

[modify] https://crrev.com/401c5932118fb24071348b6a6ee5ce1638a7d982/cc/surfaces/surface_factory.cc
[modify] https://crrev.com/401c5932118fb24071348b6a6ee5ce1638a7d982/cc/surfaces/surface_factory_unittest.cc
[modify] https://crrev.com/401c5932118fb24071348b6a6ee5ce1638a7d982/services/ui/ws/display.cc
[modify] https://crrev.com/401c5932118fb24071348b6a6ee5ce1638a7d982/services/ui/ws/display.h
[modify] https://crrev.com/401c5932118fb24071348b6a6ee5ce1638a7d982/services/ui/ws/frame_generator.cc
[modify] https://crrev.com/401c5932118fb24071348b6a6ee5ce1638a7d982/services/ui/ws/frame_generator.h
[modify] https://crrev.com/401c5932118fb24071348b6a6ee5ce1638a7d982/services/ui/ws/frame_generator_delegate.h
[modify] https://crrev.com/401c5932118fb24071348b6a6ee5ce1638a7d982/services/ui/ws/frame_generator_unittest.cc
[modify] https://crrev.com/401c5932118fb24071348b6a6ee5ce1638a7d982/services/ui/ws/platform_display_default.cc
[modify] https://crrev.com/401c5932118fb24071348b6a6ee5ce1638a7d982/services/ui/ws/platform_display_default.h
[modify] https://crrev.com/401c5932118fb24071348b6a6ee5ce1638a7d982/services/ui/ws/platform_display_delegate.h
[modify] https://crrev.com/401c5932118fb24071348b6a6ee5ce1638a7d982/services/ui/ws/test_utils.cc
[modify] https://crrev.com/401c5932118fb24071348b6a6ee5ce1638a7d982/services/ui/ws/test_utils.h
[modify] https://crrev.com/401c5932118fb24071348b6a6ee5ce1638a7d982/services/ui/ws/window_server.cc

Components: Internals>Compositing
Project Member

Comment 6 by bugdroid1@chromium.org, Feb 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c46bbe24124bdef62daa0e396bb4baa2681fd817

commit c46bbe24124bdef62daa0e396bb4baa2681fd817
Author: samans <samans@chromium.org>
Date: Thu Feb 23 01:30:11 2017

FrameGenerator should not be created until an AcceleratedWidget is available

FrameGenerator does not have anything useful to do until an
AcceleratedWidget is provided. This CL delays the creation of
FrameGenerator until a widget is available.

We also remove the unit test because it's not doing anything useful
right now.

BUG= 683732 

Review-Url: https://codereview.chromium.org/2694043003
Cr-Commit-Position: refs/heads/master@{#452329}

[modify] https://crrev.com/c46bbe24124bdef62daa0e396bb4baa2681fd817/services/ui/ws/BUILD.gn
[modify] https://crrev.com/c46bbe24124bdef62daa0e396bb4baa2681fd817/services/ui/ws/frame_generator.cc
[modify] https://crrev.com/c46bbe24124bdef62daa0e396bb4baa2681fd817/services/ui/ws/frame_generator.h
[delete] https://crrev.com/919b5796526c5c9764ae73ab5f5f50829597556f/services/ui/ws/frame_generator_unittest.cc
[modify] https://crrev.com/c46bbe24124bdef62daa0e396bb4baa2681fd817/services/ui/ws/platform_display_default.cc
[modify] https://crrev.com/c46bbe24124bdef62daa0e396bb4baa2681fd817/services/ui/ws/platform_display_default.h

Project Member

Comment 7 by bugdroid1@chromium.org, Feb 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cfa63cc0384a8ab0d8dbea6e67543d21716485dd

commit cfa63cc0384a8ab0d8dbea6e67543d21716485dd
Author: hajimehoshi <hajimehoshi@chromium.org>
Date: Thu Feb 23 10:07:33 2017

Revert of FrameGenerator should not be created until an AcceleratedWidget is available (patchset #6 id:100001 of https://codereview.chromium.org/2694043003/ )

Reason for revert:
This might cause the crash (crbug/695371)

Original issue's description:
> FrameGenerator should not be created until an AcceleratedWidget is available
>
> FrameGenerator does not have anything useful to do until an
> AcceleratedWidget is provided. This CL delays the creation of
> FrameGenerator until a widget is available.
>
> We also remove the unit test because it's not doing anything useful
> right now.
>
> BUG= 683732 
>
> Review-Url: https://codereview.chromium.org/2694043003
> Cr-Commit-Position: refs/heads/master@{#452329}
> Committed: https://chromium.googlesource.com/chromium/src/+/c46bbe24124bdef62daa0e396bb4baa2681fd817

TBR=fsamuel@chromium.org,sky@chromium.org,samans@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 683732 

Review-Url: https://codereview.chromium.org/2717453002
Cr-Commit-Position: refs/heads/master@{#452446}

[modify] https://crrev.com/cfa63cc0384a8ab0d8dbea6e67543d21716485dd/services/ui/ws/BUILD.gn
[modify] https://crrev.com/cfa63cc0384a8ab0d8dbea6e67543d21716485dd/services/ui/ws/frame_generator.cc
[modify] https://crrev.com/cfa63cc0384a8ab0d8dbea6e67543d21716485dd/services/ui/ws/frame_generator.h
[add] https://crrev.com/cfa63cc0384a8ab0d8dbea6e67543d21716485dd/services/ui/ws/frame_generator_unittest.cc
[modify] https://crrev.com/cfa63cc0384a8ab0d8dbea6e67543d21716485dd/services/ui/ws/platform_display_default.cc
[modify] https://crrev.com/cfa63cc0384a8ab0d8dbea6e67543d21716485dd/services/ui/ws/platform_display_default.h

Project Member

Comment 8 by bugdroid1@chromium.org, Feb 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fa2337ad665fad794a067127e6e4bca8d30764c6

commit fa2337ad665fad794a067127e6e4bca8d30764c6
Author: hajimehoshi <hajimehoshi@chromium.org>
Date: Thu Feb 23 11:08:07 2017

Reland of FrameGenerator should not be created until an AcceleratedWidget is available (patchset #1 id:1 of https://codereview.chromium.org/2717453002/ )

Reason for revert:
This was not the culprit of  crbug.com/695375  as the build bot results said. Sorry for confusion.

https://build.chromium.org/p/chromium.win/buildstatus?builder=Win7%20Tests%20%281%29&number=64018

Original issue's description:
> Revert of FrameGenerator should not be created until an AcceleratedWidget is available (patchset #6 id:100001 of https://codereview.chromium.org/2694043003/ )
>
> Reason for revert:
> This might cause the crash (crbug/695371)
>
> Original issue's description:
> > FrameGenerator should not be created until an AcceleratedWidget is available
> >
> > FrameGenerator does not have anything useful to do until an
> > AcceleratedWidget is provided. This CL delays the creation of
> > FrameGenerator until a widget is available.
> >
> > We also remove the unit test because it's not doing anything useful
> > right now.
> >
> > BUG= 683732 
> >
> > Review-Url: https://codereview.chromium.org/2694043003
> > Cr-Commit-Position: refs/heads/master@{#452329}
> > Committed: https://chromium.googlesource.com/chromium/src/+/c46bbe24124bdef62daa0e396bb4baa2681fd817
>
> TBR=fsamuel@chromium.org,sky@chromium.org,samans@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG= 683732 
>
> Review-Url: https://codereview.chromium.org/2717453002
> Cr-Commit-Position: refs/heads/master@{#452446}
> Committed: https://chromium.googlesource.com/chromium/src/+/cfa63cc0384a8ab0d8dbea6e67543d21716485dd

TBR=fsamuel@chromium.org,sky@chromium.org,samans@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 683732 

Review-Url: https://codereview.chromium.org/2715533006
Cr-Commit-Position: refs/heads/master@{#452457}

[modify] https://crrev.com/fa2337ad665fad794a067127e6e4bca8d30764c6/services/ui/ws/BUILD.gn
[modify] https://crrev.com/fa2337ad665fad794a067127e6e4bca8d30764c6/services/ui/ws/frame_generator.cc
[modify] https://crrev.com/fa2337ad665fad794a067127e6e4bca8d30764c6/services/ui/ws/frame_generator.h
[delete] https://crrev.com/0c9658dcf96d88ad497cd4c43765dfad8b93dd1c/services/ui/ws/frame_generator_unittest.cc
[modify] https://crrev.com/fa2337ad665fad794a067127e6e4bca8d30764c6/services/ui/ws/platform_display_default.cc
[modify] https://crrev.com/fa2337ad665fad794a067127e6e4bca8d30764c6/services/ui/ws/platform_display_default.h

One other nice cleanup (potential bug fix?) that I noticed is FrameGenerator has a FrameGeneratorDelegate whose sole purpose is now just IsInHighContrastMode().

Perhaps, if FrameGenerator instead had a FrameGenerator::SetInHighContrastMode() then the delegate becomes unnecessary.

What's interesting as well, is I think high contrast mode can probably be turned on and off and we don't trigger the generation of a new frame when high contrast mode changes.

If high contrast mode changes, then that should trigger a BeginFrame and then FrameGenerator should pick that up and add the RenderPassDrawQuad with the invert filter.

FrameGeneratorDelegate goes away, so this might actually be a net negative lines of code patch too! Yay!
Before I forget, another change that needs to happen in FrameGenerator, most likely for external window mode, is it needs to be told when the visibility of the PlatformWindow changes (say on minimize).

Currently we always set the cc::Display's visiblity (in GpuDisplayCompositorFrameSink, soon GpuRootCompositorFrameSink) to true. What we should instead to is make visibility another toggle on FrameGenerator that then gets passed along DisplayPrivate.
A couple more observations:

1. We only check if the frame size has changed to allocate a new LocalSurfaceId. We should also check the device scale factor.

2. We read the current frame size from the render_pass_list in FrameGenerator::OnBeginFrame. This seems silly, FrameGenerator already knows the frame size because it generated the CompositorFrame. Maybe we shouldn't be looking at the render_pass_list and instead just look at the root_window_->bounds() directly?

3. I really think we should find a way to delete ServerWindowCompositorFrameSinkManager::GetLatestFrameSize and SetLatestSurfaceInfo. GetLatestFrameSize is only used in tests to ensure a client has drawn. This is a lot of added complexity for a test.
Project Member

Comment 13 by bugdroid1@chromium.org, Feb 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9edf32fa33654215032bae96032066bf7e662a9f

commit 9edf32fa33654215032bae96032066bf7e662a9f
Author: fsamuel <fsamuel@chromium.org>
Date: Thu Feb 23 20:58:30 2017

FrameGenerator: Don't issue BeginFrames if we have nothing to draw

Updating the device scale factor before the window manager's CompositorFrame
is ready caused a premature BeginFrame in FrameGenerator. This would cause
an invalid CompositorFrame to be generated.

BUG= 683732 

Review-Url: https://codereview.chromium.org/2718523002
Cr-Commit-Position: refs/heads/master@{#452623}

[modify] https://crrev.com/9edf32fa33654215032bae96032066bf7e662a9f/services/ui/ws/frame_generator.cc

I think before we make further changes on FrameGenerator, we need to make it mockable in unit tests.

I propose:

1. Rename WindowCompositorFrameSink => ClientCompositorFrameSink.
2. Move ClientCompositorFrameSink to components/display_compositor/public/cpp.
3. FrameGenerator takes in a cc::CompositorFrameSink. By default, a ClientCompositorFrameSink.
4. A unit test plugs a TestCompositorFrameSink into FrameGenerator.

We should unit test device scale factor changes and WindowManager SurfaceInfo changes to start.

We can then get rid of FrameGeneratorDelegate::IsInHighContrastMode, in favor of FrameGenerator::SetInHighContrastMode and then unit test that. 
Owner: staraz@chromium.org
Project Member

Comment 16 by bugdroid1@chromium.org, Mar 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e4ff7160c06948baf8dd3d3141c00c8cee40fc3b

commit e4ff7160c06948baf8dd3d3141c00c8cee40fc3b
Author: staraz <staraz@chromium.org>
Date: Wed Mar 08 01:07:45 2017

Rename WindowCompositorFrameSink To ClientCompositorFrameSink

This name is more natural since it's the CompositorFrameSink running on the
client side.

BUG= 683732 

Review-Url: https://codereview.chromium.org/2732283002
Cr-Commit-Position: refs/heads/master@{#455316}

[modify] https://crrev.com/e4ff7160c06948baf8dd3d3141c00c8cee40fc3b/content/browser/compositor/mus_browser_compositor_output_surface.cc
[modify] https://crrev.com/e4ff7160c06948baf8dd3d3141c00c8cee40fc3b/content/browser/compositor/mus_browser_compositor_output_surface.h
[modify] https://crrev.com/e4ff7160c06948baf8dd3d3141c00c8cee40fc3b/content/renderer/mus/renderer_window_tree_client.cc
[modify] https://crrev.com/e4ff7160c06948baf8dd3d3141c00c8cee40fc3b/content/renderer/mus/renderer_window_tree_client.h
[modify] https://crrev.com/e4ff7160c06948baf8dd3d3141c00c8cee40fc3b/services/ui/public/cpp/BUILD.gn
[modify] https://crrev.com/e4ff7160c06948baf8dd3d3141c00c8cee40fc3b/services/ui/public/cpp/OWNERS
[rename] https://crrev.com/e4ff7160c06948baf8dd3d3141c00c8cee40fc3b/services/ui/public/cpp/client_compositor_frame_sink.cc
[rename] https://crrev.com/e4ff7160c06948baf8dd3d3141c00c8cee40fc3b/services/ui/public/cpp/client_compositor_frame_sink.h
[modify] https://crrev.com/e4ff7160c06948baf8dd3d3141c00c8cee40fc3b/ui/aura/mus/DEPS
[modify] https://crrev.com/e4ff7160c06948baf8dd3d3141c00c8cee40fc3b/ui/aura/mus/window_port_mus.cc
[modify] https://crrev.com/e4ff7160c06948baf8dd3d3141c00c8cee40fc3b/ui/aura/mus/window_port_mus.h

Project Member

Comment 17 by bugdroid1@chromium.org, Mar 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/72df4334a57fdd13b8dbbc7cd48895fcd79b3a63

commit 72df4334a57fdd13b8dbbc7cd48895fcd79b3a63
Author: staraz <staraz@chromium.org>
Date: Sat Mar 11 17:48:41 2017

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}

[modify] https://crrev.com/72df4334a57fdd13b8dbbc7cd48895fcd79b3a63/services/ui/ws/BUILD.gn
[add] https://crrev.com/72df4334a57fdd13b8dbbc7cd48895fcd79b3a63/services/ui/ws/display_client_compositor_frame_sink.cc
[add] https://crrev.com/72df4334a57fdd13b8dbbc7cd48895fcd79b3a63/services/ui/ws/display_client_compositor_frame_sink.h
[modify] https://crrev.com/72df4334a57fdd13b8dbbc7cd48895fcd79b3a63/services/ui/ws/frame_generator.cc
[modify] https://crrev.com/72df4334a57fdd13b8dbbc7cd48895fcd79b3a63/services/ui/ws/frame_generator.h
[modify] https://crrev.com/72df4334a57fdd13b8dbbc7cd48895fcd79b3a63/services/ui/ws/platform_display_default.cc
[modify] https://crrev.com/72df4334a57fdd13b8dbbc7cd48895fcd79b3a63/services/ui/ws/platform_display_default.h

Sweet! Now that FrameGenerator uses a cc::CompositorFrameSink, it ought to be fairly straightforward to write unit tests for FrameGenerator. In particular, a mock CompositorFrameSink could also dispense a test BeginFrameSource.

We can manually tick BeginFrames in unit tests. We can receive CompositorFrames in the mock and verify that the right CompositorFrames have been generated.

Some useful test ideas:

1. We should verify that changes the device scale factor triggers a BeginFrame but only if there is a valid SurfaceInfo set.
2. Changing the SurfaceInfo should trigger a BeginFrame.

We should verify that a CompositorFrame refers to the window manager's surface via referenced_surfaces.

Changes that need to happen and ultimately need tests:

1. Changing high contrast mode should trigger a BeginFrame. We should verify that an appropriate RenderPass is created as a result.
2. We should track window visibility. A hidden window should not get BeginFrames.

Eventually to support mus-gpu restarts, we should detect when we lose a CompositorFrameSink and request a new one. That can happen later... maybe that can be a separate bug?
Status: Started (was: Assigned)
I think we can modify TestCompositorFrameSink::DidReceiveCompositorFrameAck() to keep 
track of the number of CompositorFrames received, but how do we know if a 
FrameGenerator is requesting BeginFrames?
We don't necessarily need to use TestCompositorFrameSink if it doesn't give us the hooks we need for FrameGenerator. We can write a new CompositorFrameSink for FrameGenerator testing if necessary. 

Take a look at what we did in compositor_frame_sink_support_unittest:

https://cs.chromium.org/chromium/src/cc/surfaces/compositor_frame_sink_support_unittest.cc?type=cs&q=compositor_frame_sink_support_unittest.cc&l=161

We dispense a FakeExternalBeginFrameSource. We can then fire off BeginFrames as we do here:

https://cs.chromium.org/chromium/src/cc/surfaces/compositor_frame_sink_support_unittest.cc?type=cs&l=471
I think I can use a simpler TestCompositorFrameSink along with FakeExternalBeginFrameSource. 

Once we manually tick the FakeExternalBeginFrameSource, CFS should only receives a CompositorFrame if SurfaceInfo has changed OR (device scale factor has changed AND FrameGeneraotr has a valid SurfaceInfo).

fsamuel@: any thoughts?
Well, you could either reuse TestCompositorFrameSink or write your own test version. TestCompositorFrameSink is actually kind of complicated, so maybe writing a simpler one is easier. Not sure. Use your judgement.
For what it's worth.. Usually our test fixtures are complicated because they let tests use them with minimal work, and I prefer adding things to a single test fixture to adding many test fixtures that do slightly different things but are not as discoverable/shareable. But ya, use your judgement.
#18: 
Currently, FrameGenerator calls delegate_->IsInHighContrastMode() to know if it's in
high contrast mode. Do you want to add a FrameGenerator::OnHighContrastModeSet() so 
WindowServer calls that to trigger a BeginFrame in SetHighContrastMode()?
(There's also a TODO saying setting high contrast mode doesn't seem like a window 
server concept: 
https://cs.chromium.org/chromium/src/services/ui/ws/window_server.cc?l=284)

Also, FrameGenerator early returns in OnBeginFrame(0 if the root_window_ is not 
visible. Do you want that check to move to SetNeedsBeginFrames()?


staraz@: FrameGenerator::SetInHighConstrastMode(bool) yea.

It should behave like SetDeviceScaleFactor.

I like the idea of checking visibility in SetNeedsBeginFrames! Thanks.

Furthermore, changing visibility should trigger a BeginFrame.
Project Member

Comment 26 by bugdroid1@chromium.org, Mar 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8e89706698b46b5391e000fbd7175fd63cdf1a64

commit 8e89706698b46b5391e000fbd7175fd63cdf1a64
Author: staraz <staraz@chromium.org>
Date: Sat Mar 18 04:12:39 2017

Add FrameGenerator Unit Tests

Added FrameGeneratorTest and several mock classes to facilitate unit tests for
FrameGenerator.

Also added some explanatory comments to DisplayClientCompositorFrameSink.

BUG= 683732 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2755573002
Cr-Commit-Position: refs/heads/master@{#457954}

[modify] https://crrev.com/8e89706698b46b5391e000fbd7175fd63cdf1a64/services/ui/ws/BUILD.gn
[modify] https://crrev.com/8e89706698b46b5391e000fbd7175fd63cdf1a64/services/ui/ws/display_client_compositor_frame_sink.h
[modify] https://crrev.com/8e89706698b46b5391e000fbd7175fd63cdf1a64/services/ui/ws/frame_generator.cc
[add] https://crrev.com/8e89706698b46b5391e000fbd7175fd63cdf1a64/services/ui/ws/frame_generator_unittest.cc

Project Member

Comment 27 by bugdroid1@chromium.org, Mar 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0855d091dfde14cb00275864c1534651441f7180

commit 0855d091dfde14cb00275864c1534651441f7180
Author: staraz <staraz@chromium.org>
Date: Tue Mar 21 15:23:10 2017

[mus]Add FrameGenerator::SetHighContrastMode(bool)

Added FrameGenerator::high_contrast_mode_enabled_ flag. The flags of all
FrameGenerators are updated by WindowServer when high contrast mode is toggled
by AccessibilityManager for the current active user.

Toggling high contrast mode triggers a BeginFrame for the FrameGenerator.

FrameGeneratorTest.SetHighContrastMode turns on high contrast mode and verifies
that the CompositorFrame received from FrameGenerator has a InvertFilter with
invert amount of 1.0.

BUG= 683732 

TBR=msw@chromium.org

Review-Url: https://codereview.chromium.org/2759933007
Cr-Commit-Position: refs/heads/master@{#458421}

[modify] https://crrev.com/0855d091dfde14cb00275864c1534651441f7180/services/ui/ws/display.cc
[modify] https://crrev.com/0855d091dfde14cb00275864c1534651441f7180/services/ui/ws/display.h
[modify] https://crrev.com/0855d091dfde14cb00275864c1534651441f7180/services/ui/ws/display_manager.cc
[modify] https://crrev.com/0855d091dfde14cb00275864c1534651441f7180/services/ui/ws/display_manager.h
[modify] https://crrev.com/0855d091dfde14cb00275864c1534651441f7180/services/ui/ws/frame_generator.cc
[modify] https://crrev.com/0855d091dfde14cb00275864c1534651441f7180/services/ui/ws/frame_generator.h
[delete] https://crrev.com/a3d6db6921ab508bc66d04a8141b872325a7b5c2/services/ui/ws/frame_generator_delegate.h
[modify] https://crrev.com/0855d091dfde14cb00275864c1534651441f7180/services/ui/ws/frame_generator_unittest.cc
[modify] https://crrev.com/0855d091dfde14cb00275864c1534651441f7180/services/ui/ws/platform_display_default.cc
[modify] https://crrev.com/0855d091dfde14cb00275864c1534651441f7180/services/ui/ws/platform_display_default.h
[modify] https://crrev.com/0855d091dfde14cb00275864c1534651441f7180/services/ui/ws/platform_display_delegate.h
[modify] https://crrev.com/0855d091dfde14cb00275864c1534651441f7180/services/ui/ws/test_utils.cc
[modify] https://crrev.com/0855d091dfde14cb00275864c1534651441f7180/services/ui/ws/test_utils.h
[modify] https://crrev.com/0855d091dfde14cb00275864c1534651441f7180/services/ui/ws/window_server.cc
[modify] https://crrev.com/0855d091dfde14cb00275864c1534651441f7180/services/ui/ws/window_server.h

Project Member

Comment 28 by bugdroid1@chromium.org, Mar 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/394caf0ffa05d6c7ada7560a3379f4807215bfb4

commit 394caf0ffa05d6c7ada7560a3379f4807215bfb4
Author: staraz <staraz@chromium.org>
Date: Thu Mar 23 18:39:27 2017

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}

[modify] https://crrev.com/394caf0ffa05d6c7ada7560a3379f4807215bfb4/services/ui/ws/frame_generator.cc
[modify] https://crrev.com/394caf0ffa05d6c7ada7560a3379f4807215bfb4/services/ui/ws/frame_generator.h
[modify] https://crrev.com/394caf0ffa05d6c7ada7560a3379f4807215bfb4/services/ui/ws/frame_generator_unittest.cc
[modify] https://crrev.com/394caf0ffa05d6c7ada7560a3379f4807215bfb4/services/ui/ws/platform_display_default.cc
[modify] https://crrev.com/394caf0ffa05d6c7ada7560a3379f4807215bfb4/services/ui/ws/platform_display_default.h

Status: Fixed (was: Started)
I'm pretty happy with the cleanups that have been done since I filed this bug. FrameGenerator is finally (fairly) easy to understand and well tested. Thanks Saman! Thanks Alex! I'm marking as FIXED!
Blocking: -601863
Components: -Internals>MUS Internals>Services>WindowService

Sign in to add a comment