Decouple BrowserCompositorOutputSurface from BeginFrameSource.

content/browser/compositor/software_browser_compositor_output_surface.cc

Index: content/browser/compositor/software_browser_compositor_output_surface.cc
diff --git a/content/browser/compositor/software_browser_compositor_output_surface.cc b/content/browser/compositor/software_browser_compositor_output_surface.cc
index 4e6fd5f83a548d2b678b64c6310fd7e89be60462..85cd4eee16c04c0ecadc73099c239ffe6ec5e61b 100644
--- a/content/browser/compositor/software_browser_compositor_output_surface.cc
+++ b/content/browser/compositor/software_browser_compositor_output_surface.cc
@@ -23,12 +23,10 @@ namespace content {
 
 SoftwareBrowserCompositorOutputSurface::SoftwareBrowserCompositorOutputSurface(
     std::unique_ptr<cc::SoftwareOutputDevice> software_device,
-    const scoped_refptr<ui::CompositorVSyncManager>& vsync_manager,
-    cc::SyntheticBeginFrameSource* begin_frame_source,
+    const UpdateVSyncParametersCallback& update_vsync_parameters_callback,
     scoped_refptr<base::SingleThreadTaskRunner> task_runner)
     : BrowserCompositorOutputSurface(std::move(software_device),
-                                     vsync_manager,
-                                     begin_frame_source),
+                                     update_vsync_parameters_callback),
       task_runner_(std::move(task_runner)),
       weak_factory_(this) {}
 
@@ -81,9 +79,7 @@ void SoftwareBrowserCompositorOutputSurface::SwapBuffers(
 
   gfx::VSyncProvider* vsync_provider = software_device()->GetVSyncProvider();
   if (vsync_provider) {

[danakj, 2016/12/02 21:44:14]

nit: can remove {}

[stanisc, 2016/12/02 22:44:54]

Done.

-    vsync_provider->GetVSyncParameters(base::Bind(
-        &BrowserCompositorOutputSurface::OnUpdateVSyncParametersFromGpu,
-        weak_factory_.GetWeakPtr()));
+    vsync_provider->GetVSyncParameters(update_vsync_parameters_callback_);

[danakj, 2016/12/02 21:44:14]

It looks like this one has good reason to check the weakptr. "We provide the
strong guarantee that the callback will not be called once the instance of this
class is destroyed." And I don't see anything else cancelling the callback if
this class is destroyed for instance?

[stanisc, 2016/12/02 22:25:20]

I thought the comment "We provide the strong guarantee that the callback will
not be called once the instance of this class is destroyed." is for the instance
of VSyncProvider class. All existing implementation of VSyncProvider look like
they invoke callback synchronously, but I agree the assumption about lifetime of
the compositor vs. the callback cannot be guaranteed in this case.

I am bit confused. I'd prefer to go back to check the WeakPtr<ui::Compositor>
inside the callback implementation in GPTF. Another alternative is to make the
callback function a member of ui::Compositor which as I understand would prevent
it from being called if Compositor gets deleted. Would that be better?

[danakj, 2016/12/02 22:35:10]

OH I read this backwards, I thought it says it doesn't guarantee. But it does,
and the VsyncProvider is owned by the SoftwareOutputDevice owned by this, so ya
it's fine. LGTM :)

[stanisc, 2016/12/02 22:44:54]

Thanks! One last change.
I decided it would be better to eliminate the callback function from GPTF
because it is now redundant and just point back to
ui::CompositorSetDisplayVSyncParameters.

   }
 
   task_runner_->PostTask(