Closed Bug 1269037 Opened 8 years ago Closed 8 years ago

Change CompositorVsyncScheduler to use widget proxies

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: