Last Comment Bug 776550 - Provide API to start Compositor Parent with external thread/messageloop
: Provide API to start Compositor Parent with external thread/messageloop
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: ARM Linux
: -- normal (vote)
: mozilla17
Assigned To: Oleg Romashin (:romaxa)
:
:
Mentors:
Depends on:
Blocks: 763234
  Show dependency treegraph
 
Reported: 2012-07-23 08:34 PDT by Oleg Romashin (:romaxa)
Modified: 2012-07-26 05:10 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Provide API to start Compositor Parent with external thread/messageloop (4.80 KB, patch)
2012-07-23 08:35 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Provide API to start Compositor Parent with external thread/messageloop (5.79 KB, patch)
2012-07-24 20:19 PDT, Oleg Romashin (:romaxa)
cjones.bugs: review+
Details | Diff | Splinter Review

Description Oleg Romashin (:romaxa) 2012-07-23 08:34:01 PDT
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) 2012-07-23 08:35:28 PDT
Created attachment 644942 [details] [diff] [review]
Provide API to start Compositor Parent with external thread/messageloop
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-23 15:07:21 PDT
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) 2012-07-23 18:39:13 PDT
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 2012-07-24 01:14:01 PDT
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) 2012-07-24 20:19:01 PDT
Created attachment 645633 [details] [diff] [review]
Provide API to start Compositor Parent with external thread/messageloop
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-25 02:35:10 PDT
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] 2012-07-25 11:30:26 PDT
I don't think I am entitled to provide a proper review, but it looks good to me with Chris's remarks.
Comment 9 Ed Morley [:emorley] 2012-07-26 05:10:14 PDT
https://hg.mozilla.org/mozilla-central/rev/4224b265d940

Note You need to log in before you can comment on or make changes to this bug.