776550 - Provide API to start Compositor Parent with external thread/messageloop

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Oleg Romashin (:romaxa)]

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.

Comment 1

[Oleg Romashin (:romaxa)]

Attachment: Provide API to start Compositor Parent with external thread/messageloop

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 3

[Oleg Romashin (:romaxa)]

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

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 5

[Oleg Romashin (:romaxa)]

Attachment: Provide API to start Compositor Parent with external thread/messageloop

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 7

[Nicolas Silva [:nical]]

I don't think I am entitled to provide a proper review, but it looks good to me with Chris's remarks.

Comment 8

[Oleg Romashin (:romaxa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4224b265d940

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4224b265d940