Closed Bug 1113725 Opened 5 years ago Closed 5 years ago

Rename VsyncDispatcher to CompositorVsyncDispatcher

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: mchang, Assigned: mchang)

References

Details

Attachments

(1 file, 2 obsolete files)

Since we will have two VsyncDispatchers: One for the refresh driver and one for the Compositors, let's rename all the VsyncDispatchers to the CompositorVsyncDispatcher to make it easier to review 1092978.
Useful to implement the refresh driver in silk in bug 1092978. Just a global rename, no logic changes.
Attachment #8539394 - Flags: review?(bugmail.mozilla)
Comment on attachment 8539394 [details] [diff] [review]
Rename VsyncDispatcher to CompositorVsyncDispatcher

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

::: gfx/thebes/VsyncSource.h
@@ +34,5 @@
>        virtual void DisableVsync() = 0;
>        virtual bool IsVsyncEnabled() = 0;
>  
>      private:
> +      nsTArray<nsRefPtr<mozilla::CompositorVsyncDispatcher>> mCompositorVsyncDispatchers;

Are you going to store the refresh driver vsync dispatchers in a separate array? If so what's the point of having them both extend from the VsyncDispatcher base class?

::: widget/nsBaseWidget.h
@@ +139,5 @@
>                                            LayersBackend aBackendHint = mozilla::layers::LayersBackend::LAYERS_NONE,
>                                            LayerManagerPersistence aPersistence = LAYER_MANAGER_CURRENT,
>                                            bool* aAllowRetaining = nullptr);
>  
> +  CompositorVsyncDispatcher*        GetCompositorVsyncDispatcher() MOZ_OVERRIDE;

nit: reduce gap to one space
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> Comment on attachment 8539394 [details] [diff] [review]
> Rename VsyncDispatcher to CompositorVsyncDispatcher
> 
> Review of attachment 8539394 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/VsyncSource.h
> @@ +34,5 @@
> >        virtual void DisableVsync() = 0;
> >        virtual bool IsVsyncEnabled() = 0;
> >  
> >      private:
> > +      nsTArray<nsRefPtr<mozilla::CompositorVsyncDispatcher>> mCompositorVsyncDispatchers;
> 
> Are you going to store the refresh driver vsync dispatchers in a separate
> array? If so what's the point of having them both extend from the
> VsyncDispatcher base class?

No, each display should only have 1 RefreshDriverVsyncDispatcher that's associated with that display, so it won't be in the array.
Updated from comment 2.
Attachment #8539394 - Attachment is obsolete: true
Attachment #8539394 - Flags: review?(bugmail.mozilla)
Attachment #8539416 - Flags: review?(bugmail.mozilla)
(In reply to Mason Chang [:mchang] from comment #3)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> > Are you going to store the refresh driver vsync dispatchers in a separate
> > array? If so what's the point of having them both extend from the
> > VsyncDispatcher base class?
> 
> No, each display should only have 1 RefreshDriverVsyncDispatcher that's
> associated with that display, so it won't be in the array.

My question stands, then. What's the point of the VsyncDispatcher base class? Will you ever use VsyncDispatcher* or nsRefPtr<VsyncDispatcher> anywhere?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5)
> (In reply to Mason Chang [:mchang] from comment #3)
> > (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> > > Are you going to store the refresh driver vsync dispatchers in a separate
> > > array? If so what's the point of having them both extend from the
> > > VsyncDispatcher base class?
> > 
> > No, each display should only have 1 RefreshDriverVsyncDispatcher that's
> > associated with that display, so it won't be in the array.
> 
> My question stands, then. What's the point of the VsyncDispatcher base
> class? Will you ever use VsyncDispatcher* or nsRefPtr<VsyncDispatcher>
> anywhere?

Hmm, you're right, we don't explicitly need a base VsyncDispatcher class then, reworking.
Attachment #8539416 - Attachment is obsolete: true
Attachment #8539416 - Flags: review?(bugmail.mozilla)
Attachment #8539444 - Flags: review?(bugmail.mozilla)
Attachment #8539444 - Flags: review?(bugmail.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/a736be6898d4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.