The default bug view has changed. See this FAQ.

Provide API to start Compositor Parent with external thread/messageloop

RESOLVED FIXED in mozilla17

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: romaxa, Assigned: romaxa)

Tracking

Trunk
mozilla17
ARM
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
For Native toolkit OMTC embedding I need to startup Compositor Parent with external created ThreadID message loop
It was possible before, but was a bit broken after bug 763234 landed.
(Assignee)

Comment 1

5 years ago
Created attachment 644942 [details] [diff] [review]
Provide API to start Compositor Parent with external thread/messageloop
Attachment #644942 - Flags: review?(nsilva)
Attachment #644942 - Flags: review?(jones.chris.g)
Comment on attachment 644942 [details] [diff] [review]
Provide API to start Compositor Parent with external thread/messageloop

>--- a/gfx/layers/ipc/CompositorParent.cpp
>+++ b/gfx/layers/ipc/CompositorParent.cpp

> // FIXME/bug 774386: we're assuming that there's only one
> // CompositorParent, but that's not always true.  This assumption only
> // affects CrossProcessCompositorParent below.
> static CompositorParent* sCurrentCompositor;
> static Thread* sCompositorThread = nsnull;
>+static MessageLoop* sCompositorLoop = nsnull;
>+static PlatformThreadId sCompositorThreadID = nsnull;

Can you not use a Thread in your implementation?  I don't really
want to duplicate this stuff.
Attachment #644942 - Flags: review?(nsilva)
Attachment #644942 - Flags: review?(jones.chris.g)
(Assignee)

Comment 3

5 years ago
How can I get base::Thread from main thread? the only way I've found it is 
sMainThreadID = PlatformThread::CurrentId();
(Assignee)

Updated

5 years ago
Attachment #644942 - Flags: review?(jones.chris.g)
Comment on attachment 644942 [details] [diff] [review]
Provide API to start Compositor Parent with external thread/messageloop

>--- a/gfx/layers/ipc/CompositorParent.cpp
>+++ b/gfx/layers/ipc/CompositorParent.cpp

> // FIXME/bug 774386: we're assuming that there's only one
> // CompositorParent, but that's not always true.  This assumption only
> // affects CrossProcessCompositorParent below.
> static CompositorParent* sCurrentCompositor;
> static Thread* sCompositorThread = nsnull;
>+static MessageLoop* sCompositorLoop = nsnull;
>+static PlatformThreadId sCompositorThreadID = nsnull;
> 

Please add a comment describing why we have two copies of these,
and when each copy is used.

>+void CompositorParent::StartUpWithThread(MessageLoop* aMsgLoop, PlatformThreadId aThreadID)
>+{

MOZ_ASSERT(!sCompositorThread)

> void CompositorParent::StartUp()
> {


MOZ_ASSERT(!sCompositorLoop)

> void CompositorParent::ShutDown()
> {
>   DestroyThread();

You need to clear out the duplicate thread state in
DestroyThread(), too.

>--- a/gfx/layers/ipc/CompositorParent.h
>+++ b/gfx/layers/ipc/CompositorParent.h

>+  /**
>+   * Setup external message loop and threadID for Compositor.

"thread ID" is two words.  Please extend this comment to say when this
interface should be used, and that only one of
StartUp()/StartUpWithExistingThread() may be used.

>+ */ + static void StartUpWithThread(MessageLoop* aMsgLoop,
>PlatformThreadId aThreadID);

Let's call this |StartUpWithExistingThread()|.

Would like to see the next patch.
Attachment #644942 - Flags: review?(jones.chris.g)
(Assignee)

Comment 5

5 years ago
Created attachment 645633 [details] [diff] [review]
Provide API to start Compositor Parent with external thread/messageloop
Attachment #644942 - Attachment is obsolete: true
Attachment #645633 - Flags: review?(jones.chris.g)
Comment on attachment 645633 [details] [diff] [review]
Provide API to start Compositor Parent with external thread/messageloop

>diff --git a/gfx/layers/ipc/CompositorParent.cpp b/gfx/layers/ipc/CompositorParent.cpp

>+// It would be nice to have API for building base::Thread from existing linux thread
>+// When/If that would be possible, we can create API to start compositor from external base::Thread
>+// and remove these extra variables

Let's say

 When ContentParent::StartUp() is called, we use the Thread global.
 When StartUpWithExistingThread() is used, we have to use the two
 duplicated globals, because there's no API to make a Thread from an
 existing thread.

r=me with comment change
Attachment #645633 - Flags: review?(jones.chris.g) → review+
I don't think I am entitled to provide a proper review, but it looks good to me with Chris's remarks.
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4224b265d940
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/4224b265d940
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.