FrameGenerator should not be created until an AcceleratedWidget is available

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

[Saman Sami, 2017-02-13 21:21:57]

Description was changed from

==========
FrameGeneartor should not be created until an AcceleratedWidget is available

BUG=683732
==========

to

==========
FrameGenerator should not be created until an AcceleratedWidget is available

BUG=683732
==========

[Saman Sami, 2017-02-13 21:22:01]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 21:22:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-02-13 21:26:20]

samans@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2017-02-13 21:31:20]

Hooray for code cleanup!

https://codereview.chromium.org/2694043003/diff/1/services/ui/ws/platform_dis...
File services/ui/ws/platform_display_default.cc (right):

https://codereview.chromium.org/2694043003/diff/1/services/ui/ws/platform_dis...
services/ui/ws/platform_display_default.cc:40: root_window_ =
init_params.root_window;
It seems like this should just be in the initializer list. Then you don't need
to initialize it elsewhere.

[Saman Sami, 2017-02-13 21:39:34]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[Saman Sami, 2017-02-13 21:39:52]

samans@chromium.org changed reviewers:
+ sky@chromium.org

[Saman Sami, 2017-02-13 21:40:03]

sky@: Please review all files.

[commit-bot: I haz the power, 2017-02-13 21:40:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-13 22:03:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-13 22:03:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Saman Sami, 2017-02-13 22:19:59]

Description was changed from

==========
FrameGenerator should not be created until an AcceleratedWidget is available

BUG=683732
==========

to

==========
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 FrameGenerator
until a widget is available.

BUG=683732
==========

[Saman Sami, 2017-02-13 22:20:04]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 22:20:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-02-13 22:22:55]

Description was changed from

==========
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 FrameGenerator
until a widget is available.

BUG=683732
==========

to

==========
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.

BUG=683732
==========

[Saman Sami, 2017-02-13 22:35:50]

Description was changed from

==========
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.

BUG=683732
==========

to

==========
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
==========

[sky, 2017-02-13 23:21:08]

https://codereview.chromium.org/2694043003/diff/40001/services/ui/ws/frame_ge...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2694043003/diff/40001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:49: FrameGenerator::~FrameGenerator() {
Style guide says order of declaration and definition should be the same, so move
this before SetDeviceScaleFactor.

https://codereview.chromium.org/2694043003/diff/40001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:50: compositor_frame_sink_.reset();
Is there a reason you need to explicitly destroy this?

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_));
Use MakeUnique (see threads on chromium-dev for details).

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_));
I'm inclined to pass in the initial scale factor. Less error prone that way.

[commit-bot: I haz the power, 2017-02-14 00:00:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-14 00:00:39]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2017-02-17 14:33:02]

This is generally a good cleanup and I'm supportive of wrapping it up and
landing, for what it's worth :-)

[Saman Sami, 2017-02-17 16:29:22]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 16:29:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-02-17 16:31:50]

PTAL

https://codereview.chromium.org/2694043003/diff/40001/services/ui/ws/frame_ge...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2694043003/diff/40001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:49: FrameGenerator::~FrameGenerator() {
On 2017/02/13 23:21:07, sky wrote:
> Style guide says order of declaration and definition should be the same, so
move
> this before SetDeviceScaleFactor.

Done.

https://codereview.chromium.org/2694043003/diff/40001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:50: compositor_frame_sink_.reset();
On 2017/02/13 23:21:07, sky wrote:
> Is there a reason you need to explicitly destroy this?

I don't think so.

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/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'"

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/13 23:21:07, sky wrote:
> I'm inclined to pass in the initial scale factor. Less error prone that way.

Done.

[commit-bot: I haz the power, 2017-02-17 17:55:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 17:55:53]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Saman Sami, 2017-02-17 18:08:45]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 18:09:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 20:34:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 20:34:41]

Dry run: This issue passed the CQ dry run.

[sky, 2017-02-17 20:40:51]

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?

[Saman Sami, 2017-02-22 23:29:30]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[Saman Sami, 2017-02-22 23:29:39]

PTAL

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 20:40:51, sky wrote:
> 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?

I don't believe there is a good reason. I'll fix it.

[commit-bot: I haz the power, 2017-02-22 23:30:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Fady Samuel, 2017-02-22 23:40:01]

https://codereview.chromium.org/2694043003/diff/80001/services/ui/ws/frame_ge...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2694043003/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:76: frame_size =
frame.render_pass_list[0]->output_rect.size();
This is wrong actually. You should grab the last item in the render_pass_list,
not the first.

[Saman Sami, 2017-02-22 23:43:51]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[Saman Sami, 2017-02-22 23:44:23]

PTAL

https://codereview.chromium.org/2694043003/diff/80001/services/ui/ws/frame_ge...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2694043003/diff/80001/services/ui/ws/frame_ge...
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 wrote:
> This is wrong actually. You should grab the last item in the render_pass_list,
> not the first.

Done.

[commit-bot: I haz the power, 2017-02-22 23:45:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Fady Samuel, 2017-02-22 23:45:28]

lgtm

[sky, 2017-02-23 01:00:32]

LGTM - thanks

[Saman Sami, 2017-02-23 01:04:19]

The CQ bit was unchecked by samans@chromium.org

[Saman Sami, 2017-02-23 01:04:32]

The CQ bit was checked by samans@chromium.org

[commit-bot: I haz the power, 2017-02-23 01:05:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 01:30:15]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487811872159260,
"parent_rev": "919b5796526c5c9764ae73ab5f5f50829597556f", "commit_rev":
"c46bbe24124bdef62daa0e396bb4baa2681fd817"}

[commit-bot: I haz the power, 2017-02-23 01:31:24]

Description was changed from

==========
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
==========

to

==========
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/+/c46bbe24124bdef62daa0e396bb4...
==========

[commit-bot: I haz the power, 2017-02-23 01:31:27]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/c46bbe24124bdef62daa0e396bb4...

[hajimehoshi, 2017-02-23 10:06:31]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/2717453002/ by hajimehoshi@chromium.org.

The reason for reverting is: This might cause the crash (crbug/695371).

[Marc Treib, 2017-02-23 10:20:56]

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).