Change CompositorVsyncScheduler to use widget proxies

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

(Blocks 1 bug)

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

CompositorVsyncScheduler requests a dispatcher object from nsIWidget, on the compositor thread. We have to change this to use CompositorWidgetProxy instead.
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.
Attachment #8747371 - Flags: review?(mchang)
Attachment #8747371 - Attachment is obsolete: true
Attachment #8747371 - Flags: review?(mchang)
Attachment #8747596 - Flags: review?(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.
Attachment #8747596 - Flags: review?(mchang)
(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).
Remove null checks as a follow-up
Attachment #8748345 - Flags: review?(mchang)
Attachment #8748345 - Flags: review?(mchang) → review+
Attachment #8747596 - Flags: review?(mchang) → review+
https://hg.mozilla.org/mozilla-central/rev/a5086ab88836
https://hg.mozilla.org/mozilla-central/rev/ff82b00b481e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.