The default bug view has changed. See this FAQ.

Make CompositorParent usable by Embedlite

RESOLVED FIXED in mozilla32

Status

()

Core
Graphics: Layers
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: tatiana, Assigned: tatiana)

Tracking

Trunk
mozilla32
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
In bug 984786 CompositoParent become final class and I cannot use it for Embedlite anymore.
In order to connect some embedding callbacks I need CompositorParent be extendable class with protected methods or allow to setup Interface listener.
(Assignee)

Comment 1

3 years ago
Created attachment 8429803 [details] [diff] [review]
Make it back to non-final with protected methods
Attachment #8429803 - Flags: review?(dholbert)

Comment 2

3 years ago
Comment on attachment 8429803 [details] [diff] [review]
Make it back to non-final with protected methods

Change the comment 
"// Private destructor, to discourage deletion outside of Release():"
since the dtor isn't private anymore.

Comment 3

3 years ago
But do you really need non-private dtor?
Removing MOZ_FINAL is certainly ok.
(Assignee)

Comment 4

3 years ago
If I make dtor non-private, then I see build errors, even on constructor in derived class:
In file included from /embedding/embedlite/embedthread/EmbedLiteCompositorParent.cpp:8:0:
../../dist/include/mozilla/layers/CompositorParent.h: In destructor 'virtual mozilla::embedlite::EmbedLiteCompositorParent::~EmbedLiteCompositorParent()':
../../dist/include/mozilla/layers/CompositorParent.h:247:11: error: 'mozilla::layers::CompositorParent::~CompositorParent()' is private
/embedding/embedlite/embedthread/EmbedLiteCompositorParent.h:33:32: error: within this context
../../dist/include/mozilla/layers/CompositorParent.h: In constructor 'mozilla::embedlite::EmbedLiteCompositorParent::EmbedLiteCompositorParent(nsIWidget*, bool, int, int, uint32_t)':
../../dist/include/mozilla/layers/CompositorParent.h:247:11: error: 'mozilla::layers::CompositorParent::~CompositorParent()' is private
/embedding/embedlite/embedthread/EmbedLiteCompositorParent.cpp:43:25: error: within this context

Here is the source of that derived class:
https://github.com/tmeshkova/gecko-dev/blob/embedlite_compositorReworkProtectedVersion/embedding/embedlite/embedthread/EmbedLiteCompositorParent.h [.cpp]
(Assignee)

Comment 5

3 years ago
Created attachment 8430093 [details] [diff] [review]
Interface Listener version proposal

Here is another option which allow us to use CompositorParent in embedded setup, but require some methods to be public and extra interface setup.
Comment on attachment 8429803 [details] [diff] [review]
Make it back to non-final with protected methods

>+++ b/gfx/layers/ipc/CompositorParent.h
>-class CompositorParent MOZ_FINAL : public PCompositorParent,
>-                                   public ShadowLayersManager
>+class CompositorParent : public PCompositorParent,
>+                         public ShadowLayersManager
> {
>   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(CompositorParent)
> 
>@@ -242,7 +242,7 @@ public:
>    */
>   static bool IsInCompositorThread();
> 
>-private:
>+protected:
>   // Private destructor, to discourage deletion outside of Release():
>   virtual ~CompositorParent();

Please update the comment to say "Protected destructor". (not "Private")

Looks fine to me, with that.
Attachment #8429803 - Flags: review?(dholbert) → review+
(In reply to Olli Pettay [:smaug] (vacation-ish May 26-30) from comment #3)
> But do you really need non-private dtor?

It does need to be protected (not private), yeah -- otherwise, instances of the derived class could never be destructed (because their destructor has to call the base class's destructor). This is true even if they're destructed via the base class's Release() method.

The important thing from the perspective of bug 984786 is that the destructor be *non-public* (whether that means private or protected), so that we can't just arbitrarily declare instances of the class on the stack.

HOWEVER: In order for the derived class to get the same safeguard, it needs to **also** declare a private or protected destructor. If it doesn't, then we'll be able to declare instances of the derived class on the stack (accidentally), which is evil since it's a refcounted class and should only be destroyed via ::Release(). (not by going out of scope on the stack)

It looks like the derived class does *not* currently have a protected destructor, based on the link in comment 4. Tatiana: could you add one (to your copy of EmbedLiteCompositorParent.h), as part of fixing this?
Assignee: nobody → tanya.meshkova
Flags: needinfo?(tanya.meshkova)
(In reply to Tatiana Meshkova (:tatiana) from comment #5)
> Created attachment 8430093 [details] [diff] [review]
> Interface Listener version proposal
> 
> Here is another option which allow us to use CompositorParent in embedded
> setup, but require some methods to be public and extra interface setup.

(I don't know enough about this interface to make a judgement-call about whether this alternative is preferable.

I'm OK r+'ing the first version, since CompositorParent was originally subclassable and I'm the one who made it no-longer-subclassable, largely because there were no subclasses. I'm happy to have that reverted [safely, per comment 7] under the assumption that someone who knows more about this code will be looking at the actual subclass itself before *that* lands in mozilla-central.)
(Assignee)

Comment 9

3 years ago
Created attachment 8430186 [details] [diff] [review]
Make it back to non-final with protected methods

Fixed comment, added protected dtor to derived class
https://github.com/tmeshkova/gecko-dev/blob/embedlite_compositorReworkProtectedVersion/embedding/embedlite/embedthread/EmbedLiteCompositorParent.h#L35
Attachment #8429803 - Attachment is obsolete: true
Attachment #8430093 - Attachment is obsolete: true
Attachment #8430186 - Flags: review+
Flags: needinfo?(tanya.meshkova)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Is there a recent Try link for this by chance? :)
Keywords: checkin-needed
(Assignee)

Comment 11

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=dd95d26df9c9
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc0110ce4594

In the future, a "does this still build?" Try push probably would have sufficed :)
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fc0110ce4594
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.