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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 1 week ago by Saman Sami
Modified:
1 month 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 Review-Url: https://codereview.chromium.org/2694043003 Cr-Commit-Position: refs/heads/master@{#452329} Committed: https://chromium.googlesource.com/chromium/src/+/c46bbe24124bdef62daa0e396bb4baa2681fd817

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: 10

Patch Set 4 : c #

Patch Set 5 : Use MakeUnique #

Total comments: 2

Patch Set 6 : Fix frame size #

Unified diffs Side-by-side diffs Delta from patch set Stats (+42 lines, -169 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 4 5 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 4 2 chunks +3 lines, -1 line 0 comments Download
M services/ui/ws/platform_display_default.cc View 1 2 3 4 4 chunks +10 lines, -7 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 49 (32 generated)
Saman Sami
1 month, 1 week 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 ...
1 month, 1 week ago (2017-02-13 21:31:20 UTC) #6
Saman Sami
sky@: Please review all files.
1 month, 1 week 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 ...
1 month, 1 week 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, ...
1 month, 1 week 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: > ...
1 month, 1 week ago (2017-02-17 16:31:50 UTC) #24
sky
https://codereview.chromium.org/2694043003/diff/40001/services/ui/ws/platform_display_default.cc File services/ui/ws/platform_display_default.cc (right): https://codereview.chromium.org/2694043003/diff/40001/services/ui/ws/platform_display_default.cc#newcode250 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 ...
1 month, 1 week ago (2017-02-17 20:40:51 UTC) #31
Saman Sami
PTAL https://codereview.chromium.org/2694043003/diff/40001/services/ui/ws/platform_display_default.cc File services/ui/ws/platform_display_default.cc (right): https://codereview.chromium.org/2694043003/diff/40001/services/ui/ws/platform_display_default.cc#newcode250 services/ui/ws/platform_display_default.cc:250: frame_generator_.reset(new FrameGenerator(this, root_window_, widget_)); On 2017/02/17 20:40:51, sky ...
1 month ago (2017-02-22 23:29:39 UTC) #33
Fady Samuel
https://codereview.chromium.org/2694043003/diff/80001/services/ui/ws/frame_generator.cc File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2694043003/diff/80001/services/ui/ws/frame_generator.cc#newcode76 services/ui/ws/frame_generator.cc:76: frame_size = frame.render_pass_list[0]->output_rect.size(); This is wrong actually. You should ...
1 month ago (2017-02-22 23:40:01 UTC) #35
Fady Samuel
1 month ago (2017-02-22 23:40:02 UTC) #36
Saman Sami
PTAL https://codereview.chromium.org/2694043003/diff/80001/services/ui/ws/frame_generator.cc File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2694043003/diff/80001/services/ui/ws/frame_generator.cc#newcode76 services/ui/ws/frame_generator.cc:76: frame_size = frame.render_pass_list[0]->output_rect.size(); On 2017/02/22 23:40:01, Fady Samuel ...
1 month ago (2017-02-22 23:44:23 UTC) #38
Fady Samuel
lgtm
1 month ago (2017-02-22 23:45:28 UTC) #40
sky
LGTM - thanks
1 month ago (2017-02-23 01:00:32 UTC) #41
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2694043003/100001
1 month ago (2017-02-23 01:05:28 UTC) #44
commit-bot: I haz the power
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/c46bbe24124bdef62daa0e396bb4baa2681fd817
1 month ago (2017-02-23 01:31:27 UTC) #47
hajimehoshi
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2717453002/ by hajimehoshi@chromium.org. ...
1 month ago (2017-02-23 10:06:31 UTC) #48
Marc Treib
1 month ago (2017-02-23 10:20:56 UTC) #49
Message was sent while issue was closed.
On 2017/02/23 10:06:31, hajimehoshi wrote:
> A revert of this CL (patchset #6 id:100001) has been created in
> https://codereview.chromium.org/2717453002/ by
mailto:hajimehoshi@chromium.org.
> 
> The reason for reverting is: This might cause the crash (crbug/695371).

Correct bug: crbug.com/695375

This is suspected of causing the failures in
RenderWidgetHostViewGuestTest.VisibilityTest (not with very high confidence, but
nothing else in the blame list looks related).
Sign in to reply to this message.

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