1269037 - Change CompositorVsyncScheduler to use widget proxies

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

CompositorVsyncScheduler requests a dispatcher object from nsIWidget, on the compositor thread. We have to change this to use CompositorWidgetProxy instead.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, CompositorWidgetProxy changes

This patch changes CompositorVsyncScheduler to go through CompositorWidgetProxy instead of nsIWidget.

We'll need another patch to fix WinCompositorWidgetProxy (from bug 1265975), and something else later to actually introduce an IPC version.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, CompositorWidgetProxy changes, v2

Comment 3

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8747596 [details] [diff] [review]
part 1, CompositorWidgetProxy changes, v2

Review of attachment 8747596 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/CompositorBridgeParent.cpp
@@ +346,5 @@
>  {
>    MOZ_ASSERT(!mIsObservingVsync);
>    MOZ_ASSERT(!mVsyncObserver);
>    // The CompositorVsyncDispatcher is cleaned up before this in the nsBaseWidget, which stops vsync listeners
>    mCompositorBridgeParent = nullptr;

Don't we still want to null this out during clean up?

::: widget/CompositorWidgetProxy.cpp
@@ +206,5 @@
> +already_AddRefed<CompositorVsyncDispatcher>
> +CompositorWidgetProxyWrapper::GetCompositorVsyncDispatcher()
> +{
> +  if (!mWidget) {
> +    return nullptr;

We should probably crash here or this should never happen. If this were true, the compositor would never tick for this widget.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Mason Chang [:mchang] from comment #3)
> Comment on attachment 8747596 [details] [diff] [review]
> part 1, CompositorWidgetProxy changes, v2
> 
> Review of attachment 8747596 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/CompositorBridgeParent.cpp
> @@ +346,5 @@
> >  {
> >    MOZ_ASSERT(!mIsObservingVsync);
> >    MOZ_ASSERT(!mVsyncObserver);
> >    // The CompositorVsyncDispatcher is cleaned up before this in the nsBaseWidget, which stops vsync listeners
> >    mCompositorBridgeParent = nullptr;
> 
> Don't we still want to null this out during clean up?

There's no reason to null it, is there? This is the destructor.

> ::: widget/CompositorWidgetProxy.cpp
> @@ +206,5 @@
> > +already_AddRefed<CompositorVsyncDispatcher>
> > +CompositorWidgetProxyWrapper::GetCompositorVsyncDispatcher()
> > +{
> > +  if (!mWidget) {
> > +    return nullptr;
> 
> We should probably crash here or this should never happen. If this were
> true, the compositor would never tick for this widget.

This check is in all the wrapper methods. Currently mWidget can't be null but that might change in the future (for example if the widget needs to revoke its proxy and make a new one).

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, follow-up

Remove null checks as a follow-up

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5086ab88836
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff82b00b481e

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a5086ab88836
https://hg.mozilla.org/mozilla-central/rev/ff82b00b481e