Chromium Code Reviews
Help | Chromium Project | Sign in
(9)

Issue 2694043003: FrameGenerator should not be created until an AcceleratedWidget is available

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 days, 22 hours ago by Saman Sami
Modified:
2 days, 23 hours ago
Reviewers:
Fady Samuel, sky
CC:
chromium-reviews, rjkroege
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

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

Patch Set 1 #

Total comments: 1

Patch Set 2 : Move server_window_ to initializer list #

Patch Set 3 : Remove frame_generator_unittest.cc #

Total comments: 9

Patch Set 4 : c #

Unified diffs Side-by-side diffs Delta from patch set Stats (+40 lines, -168 lines) Patch
M services/ui/ws/BUILD.gn View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download
M services/ui/ws/frame_generator.h View 1 2 3 1 chunk +3 lines, -4 lines 0 comments Download
M services/ui/ws/frame_generator.cc View 1 2 3 3 chunks +26 lines, -37 lines 0 comments Download
D services/ui/ws/frame_generator_unittest.cc View 1 2 1 chunk +0 lines, -119 lines 0 comments Download
M services/ui/ws/platform_display_default.h View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M services/ui/ws/platform_display_default.cc View 1 2 3 4 chunks +9 lines, -7 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 31 (24 generated)
Saman Sami
6 days, 22 hours ago (2017-02-13 21:26:44 UTC) #5
Fady Samuel
Hooray for code cleanup! https://codereview.chromium.org/2694043003/diff/1/services/ui/ws/platform_display_default.cc File services/ui/ws/platform_display_default.cc (right): https://codereview.chromium.org/2694043003/diff/1/services/ui/ws/platform_display_default.cc#newcode40 services/ui/ws/platform_display_default.cc:40: root_window_ = init_params.root_window; It seems ...
6 days, 22 hours ago (2017-02-13 21:31:20 UTC) #6
Saman Sami
sky@: Please review all files.
6 days, 22 hours ago (2017-02-13 21:40:03 UTC) #9
sky
https://codereview.chromium.org/2694043003/diff/40001/services/ui/ws/frame_generator.cc File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2694043003/diff/40001/services/ui/ws/frame_generator.cc#newcode49 services/ui/ws/frame_generator.cc:49: FrameGenerator::~FrameGenerator() { Style guide says order of declaration and ...
6 days, 20 hours ago (2017-02-13 23:21:08 UTC) #18
Fady Samuel
This is generally a good cleanup and I'm supportive of wrapping it up and landing, ...
3 days, 5 hours ago (2017-02-17 14:33:02 UTC) #21
Saman Sami
PTAL https://codereview.chromium.org/2694043003/diff/40001/services/ui/ws/frame_generator.cc File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2694043003/diff/40001/services/ui/ws/frame_generator.cc#newcode49 services/ui/ws/frame_generator.cc:49: FrameGenerator::~FrameGenerator() { On 2017/02/13 23:21:07, sky wrote: > ...
3 days, 3 hours ago (2017-02-17 16:31:50 UTC) #24
sky
2 days, 23 hours ago (2017-02-17 20:40:51 UTC) #31
https://codereview.chromium.org/2694043003/diff/40001/services/ui/ws/platform...
File services/ui/ws/platform_display_default.cc (right):

https://codereview.chromium.org/2694043003/diff/40001/services/ui/ws/platform...
services/ui/ws/platform_display_default.cc:250: frame_generator_.reset(new
FrameGenerator(this, root_window_, widget_));
On 2017/02/17 16:31:50, Saman Sami wrote:
> On 2017/02/13 23:21:08, sky wrote:
> > Use MakeUnique (see threads on chromium-dev for details).
> 
> I tried and I got "error: cannot cast 'ui::ws::PlatformDisplayDefault' to its
> private base class 'ui::ws::FrameGeneratorDelegate'"

That's because this class privately implements FrameGeneratorDelegate, which the
style guide says not to do. Is there a reason the inheritance is private and not
public?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f8e48bd