[Ozone] Adding a software rendering implementation of SurfaceFactoryOzone

[Ozone] Adding a software rendering implementation of SurfaceFactoryOzone

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229575

[rjkroege, 2013-10-16 19:03:36]

https://codereview.chromium.org/26849004/diff/4001/ui/gfx/ozone/impl/software...
File ui/gfx/ozone/impl/software_surface_factory_ozone.cc (right):

https://codereview.chromium.org/26849004/diff/4001/ui/gfx/ozone/impl/software...
ui/gfx/ozone/impl/software_surface_factory_ozone.cc:153: SoftwareSurfaceOzone*
surface = CreateSurface(controller_.get());
who tracks ownership of surface? it would seem to leak here

https://codereview.chromium.org/26849004/diff/4001/ui/gfx/ozone/impl/software...
ui/gfx/ozone/impl/software_surface_factory_ozone.cc:192: if
(!controller_->SchedulePageFlip())
The comments imply that we must always return false because otherwise, we have
called WaitForPageFlipEvent from the UI thread which you say is forbidden. Can
you clarify?

[dnicoara, 2013-10-16 20:27:29]

https://codereview.chromium.org/26849004/diff/4001/ui/gfx/ozone/impl/software...
File ui/gfx/ozone/impl/software_surface_factory_ozone.cc (right):

https://codereview.chromium.org/26849004/diff/4001/ui/gfx/ozone/impl/software...
ui/gfx/ozone/impl/software_surface_factory_ozone.cc:153: SoftwareSurfaceOzone*
surface = CreateSurface(controller_.get());
On 2013/10/16 19:03:36, rjkroege wrote:
> who tracks ownership of surface? it would seem to leak here

You are right, there is one case where it would leak. I also added comment to
indicate that the controller takes ownership of the surface.

https://codereview.chromium.org/26849004/diff/4001/ui/gfx/ozone/impl/software...
ui/gfx/ozone/impl/software_surface_factory_ozone.cc:192: if
(!controller_->SchedulePageFlip())
On 2013/10/16 19:03:36, rjkroege wrote:
> The comments imply that we must always return false because otherwise, we have
> called WaitForPageFlipEvent from the UI thread which you say is forbidden. Can
> you clarify?

I have updated the comments. Hopefully they are clearer now.

[rjkroege, 2013-10-16 21:42:13]

https://codereview.chromium.org/26849004/diff/8001/ui/gfx/ozone/impl/software...
File ui/gfx/ozone/impl/software_surface_factory_ozone.cc (right):

https://codereview.chromium.org/26849004/diff/8001/ui/gfx/ozone/impl/software...
ui/gfx/ozone/impl/software_surface_factory_ozone.cc:163: // controller takes
ownership of the surface.
In this kind of complicated ownership transferring, please use the appropriate
kind of scoped_ptr  (note the Pass operator)

https://codereview.chromium.org/26849004/diff/8001/ui/gfx/ozone/impl/software...
ui/gfx/ozone/impl/software_surface_factory_ozone.cc:205: // provide any means to
use any of the above solutions.
You still say that WaitForPageFlipEvent needs threaded compositing. So this
function is safe to call on the UI thread but blocks? So this is currently a
performance problem only?

Maybe the comment needs to be even more clarified.

[dnicoara, 2013-10-17 14:26:19]

The SSO unittests need 1 change due to the scoped_ptr change. However it is in
the CQ now. I'll updated and include it once it commits.

https://codereview.chromium.org/26849004/diff/8001/ui/gfx/ozone/impl/software...
File ui/gfx/ozone/impl/software_surface_factory_ozone.cc (right):

https://codereview.chromium.org/26849004/diff/8001/ui/gfx/ozone/impl/software...
ui/gfx/ozone/impl/software_surface_factory_ozone.cc:163: // controller takes
ownership of the surface.
On 2013/10/16 21:42:13, rjkroege wrote:
> In this kind of complicated ownership transferring, please use the appropriate
> kind of scoped_ptr  (note the Pass operator)

Done. Thank you for the tip.

https://codereview.chromium.org/26849004/diff/8001/ui/gfx/ozone/impl/software...
ui/gfx/ozone/impl/software_surface_factory_ozone.cc:205: // provide any means to
use any of the above solutions.
On 2013/10/16 21:42:13, rjkroege wrote:
> You still say that WaitForPageFlipEvent needs threaded compositing. So this
> function is safe to call on the UI thread but blocks? So this is currently a
> performance problem only?
> 
> Maybe the comment needs to be even more clarified.
> 

Oh, I see where the lack of clarity is. Added more comments to fix that.

[rjkroege, 2013-10-17 16:52:49]

lgtm

[commit-bot: I haz the power, 2013-10-17 17:05:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dnicoara@chromium.org/26849004/24001

[commit-bot: I haz the power, 2013-10-18 01:01:21]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-10-18 02:09:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dnicoara@chromium.org/26849004/24001

[commit-bot: I haz the power, 2013-10-18 18:12:58]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2013-10-18 22:53:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dnicoara@chromium.org/26849004/24001

[commit-bot: I haz the power, 2013-10-19 10:18:32]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-10-19 13:34:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dnicoara@chromium.org/26849004/24001

[commit-bot: I haz the power, 2013-10-19 15:59:37]

Change committed as 229575