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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 2 days ago by Saman Sami
Modified:
6 hours, 19 minutes ago
Reviewers:
Fady Samuel, sky
CC:
chromium-reviews, rjkroege (slow)
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 week, 2 days 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 week, 2 days ago (2017-02-13 21:31:20 UTC) #6
Saman Sami
sky@: Please review all files.
1 week, 2 days 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 week, 2 days 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, ...
6 days, 2 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: > ...
6 days 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 ...
5 days, 19 hours 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 ...
17 hours, 10 minutes 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 ...
16 hours, 59 minutes ago (2017-02-22 23:40:01 UTC) #35
Fady Samuel
16 hours, 59 minutes 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 ...
16 hours, 55 minutes ago (2017-02-22 23:44:23 UTC) #38
Fady Samuel
lgtm
16 hours, 54 minutes ago (2017-02-22 23:45:28 UTC) #40
sky
LGTM - thanks
15 hours, 39 minutes 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
15 hours, 34 minutes 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
15 hours, 8 minutes 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. ...
6 hours, 33 minutes ago (2017-02-23 10:06:31 UTC) #48
Marc Treib
6 hours, 19 minutes 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 f8e48bd