746800 - Provide Embedding API init with OMTC rendering

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Embedding: APIs, defect)

Description

[Oleg Romashin (:romaxa)]

Attachment: gecko embedding with OMTC rendering

OMTC would make big sense in mozilla embedding (in process)
with OMTC embedders could startup native application, run gecko initialization in Child thread, and USE OMTC for rendering Mozilla Layers in Native Toolkit UI widget structure.

The only difference comparing to traditional OMTC, is that CompositorParent should live inside Main NativeUIToolkit thread, because that thread usually owns GL context X windows et.c.

Comment 1

[Oleg Romashin (:romaxa)]

Sequence of embedding initialization is next:
1) Start Native toolkit Main loop (create application UI)
2) Call MozAppEmbedHelper->CreateEventLoop() - will start chromium message loop in application main thread
2) Create child thread (using native toolkit primitives like QThread)
3) Start Gecko in child thread
4) call MozAppEmbedHelper->SetupCompositor(nsIWebBrowser) from gecko thread, which will make PCompositor using MessageLoop created in 2)
5) PCompositorParent on SheduleComposite will call MozAppEmbedHelper hooks which will redirect and generate native application Invalidate signal
6) Native Application paint will call MozAppEmbedHelper Paint which supposed to call PCompositorParent->Composite

Comment 2

[Oleg Romashin (:romaxa)]

Attachment: XRE EmbedHelper API

Comment 3

[Oleg Romashin (:romaxa)]

Attachment: Provide Compositor API for running in cusom message loop/thread

Comment 4

[Oleg Romashin (:romaxa)]

Attachment: Widget SetupCompositor API

This patch basically allow to setup UI-toolkit paint Methods/Signals for CompositorParent and would allow to make Gecko Embedding works with OMTC

Comment 5

[Oleg Romashin (:romaxa)]

Attachment: Widget SetupCompositor API

Fixed lookup for Compositor signals in parent widget.
also wondering would it be possible to make CompositorParent hooks interface more generic and use "class CompositorSignals" for android and embedding? and remove android ifdefs from CompositorParent

Comment 6

[Oleg Romashin (:romaxa)]

hmm, sounds like in my case it would be easier to provide with embedHelper custom nsIWidget implementation just for embeding, also as ajuma suggested probably it make sense to create CompositorParent subclass et.c

Comment 7

[Ali Juma [:ajuma]]

Comment on attachment 617166 [details] [diff] [review]
Provide Compositor API for running in cusom message loop/thread

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

This seems reasonable, but I'd like to look it over again once the comments are addressed.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +90,5 @@
> +CompositorParent::CompositorLoop()
> +{
> +  if (mCompositorThread)
> +    return mCompositorThread->message_loop();
> +  return mCompositorLoop;

Since we're initializing mCompositorLoop to mCompositorThread->message_loop(), we should be able to return mCompositorLoop unconditionally here.

@@ +98,5 @@
> +CompositorParent::CompositorThreadID()
> +{
> +  if (mCompositorThread)
> +    return mCompositorThread->thread_id();
> +  return mThreadID;

Since we're initializing mThreadID to mCompositorThread->thread_id(), we should be able to return mThreadID unconditionally here.

::: gfx/layers/ipc/CompositorParent.h
@@ +90,5 @@
>  {
>    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(CompositorParent)
>  public:
>    CompositorParent(nsIWidget* aWidget, base::Thread* aCompositorThread);
> +  CompositorParent(nsIWidget* aWidget, MessageLoop* aMsgLoop, PlatformThreadId aThreadID);

If we don't really need mCompositorThread anymore (see comment below), we can get rid of the first constructor.

@@ +122,5 @@
>    void ResumeComposition();
>  
>    void Composite();
>    void ScheduleComposition();
> +  void NotifyInvalidation(int time);

This should have a better name. How about calling this 'ScheduleTask', make it also take a pointer to the task, and then use it whenever we have a task to schedule on our message loop (e.g. in SchedulePause and ScheduleResume)? Also, add a comment saying that 'time' is in milliseconds.

@@ +164,5 @@
>    // after a layers update has it set. It is cleared after that first composition.
>    bool mLayersUpdated;
>  
> +  MessageLoop* mCompositorLoop;
> +  PlatformThreadId mThreadID;

With these two added, we don't seem to have a need for mCompositorThread anymore.

Comment 8

[Oleg Romashin (:romaxa)]

Attachment: OMTC embedding API's

Ok, here is basic OMTC embeding helper implementation which provide API for setting up OMTC embedding
I added simply re-implemented nsPuppetWidget stub here, because it is impossible to create nsIWidget using frozen gecko API's (nsAString_internal is part of nsIWidget interface), also this way I can reuse nsBaseWidget implementation.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 618723 [details] [diff] [review]
OMTC embedding API's

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

::: embedding/embedhelper/MozEmbedWidget.h
@@ +41,5 @@
> +/**
> + * This "puppet widget" isn't really a platform widget.  It's intended
> + * to be used in widgetless rendering contexts, such as sandboxed
> + * content processes.  If any "real" widgetry is needed, the request
> + * is forwarded to and/or data received from elsewhere.

This comment probably isn't right...

Why does this widget implementation live outside 'widget'? It should probably be in widget.

Maybe I don't understand why you can't use nsPuppetWidget here.

Comment 10

[Oleg Romashin (:romaxa)]

> 
> Why does this widget implementation live outside 'widget'? It should
> probably be in widget.

Yep, that make sense

> Maybe I don't understand why you can't use nsPuppetWidget here.
Puppet widget basically work in child process, and has bunch of stuff related to ContentChild/TabChild... I decided it is better to keep standalone implementation instead of mixing tonns of if's.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 618723 [details] [diff] [review]
OMTC embedding API's

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

::: embedding/embedhelper/MozEmbedWidget.cpp
@@ +183,5 @@
> +
> +NS_IMETHODIMP
> +MozEmbedWidget::Resize(PRInt32 aWidth,
> +                     PRInt32 aHeight,
> +                     bool    aRepaint)

Fix indent

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 618723 [details] [diff] [review]
OMTC embedding API's

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

The idea seems reasonable.

::: embedding/embedhelper/EmbedCompositorParent.h
@@ +55,5 @@
> +public:
> +  EmbedCompositorParent(nsIWidget* aWidget, MessageLoop* aMsgLoop, PlatformThreadId aThreadID);
> +  virtual ~EmbedCompositorParent();
> +
> +  virtual void ScheduleTask(CancelableTask*, int);

Why are you making this public?

@@ +59,5 @@
> +  virtual void ScheduleTask(CancelableTask*, int);
> +
> +  void SetupSignals(mozilla::embedhelper::CompositorSignals* aSignals) { mCompositorSignals = aSignals; }
> +
> +  virtual void Paint();

You'd better document this.

::: embedding/embedhelper/MozAppEmbedHelper.cpp
@@ +102,5 @@
> +    {
> +        // This is a lexical scope for the MessageLoop below.  We want it
> +        // to go out of scope before NS_LogTerm() so that we don't get
> +        // spurious warnings about XPCOM objects being destroyed from a
> +        // static context.

This comment doesn't make sense here

@@ +111,5 @@
> +            sMessageLoop = uiMessageLoop.current();
> +            sMainThreadID = PlatformThread::CurrentId();
> +            // Run the UI event loop on the main thread.
> +            uiMessageLoop.MessageLoop::Run();
> +        }

You don't need any of these nested scopes

Comment 13

[Benjamin Smedberg]

Comment on attachment 617164 [details] [diff] [review]
XRE EmbedHelper API

It's totally unclear from this patch what this singleton object is *for*. There's no documentation in any of the headers.

Comment 14

[Oleg Romashin (:romaxa)]

Attachment: Embed helper OMTC widget/compositing API

Comment 15

[Oleg Romashin (:romaxa)]

Attachment: XRE EmbedHelper API

Added description of EmbedHelper object and Singleton method purpose
Added Destroy method

Comment 16

[Oleg Romashin (:romaxa)]

Benjamin any comments on last patch? is it ok to go?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 619147 [details] [diff] [review]
XRE EmbedHelper API

Like I said, I'm not comfortable reviewing this.

Comment 18

[Benjamin Smedberg]

It is not clear to me that we should accept this code into the main Mozilla repository. I don't think that this is a core feature that we would be building into our platform by default, and apart from the linkage issues with libxul I would put this into its own repository pretty much permanently. I think that it would be better to just link your code into libxul the way seamonkey does, or maintain a fork of the -central code for the time being with your patch series in it.

As for this particular patch, I really dislike having a class on which we hang random stuff called EmbedHelper.

Comment 19

[Oleg Romashin (:romaxa)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #18)
> It is not clear to me that we should accept this code into the main Mozilla
> repository. I don't think that this is a core feature that we would be
> building into our platform by default

I work on building Native Toolkit client which is using gecko as rendering engine and Web Runtime platform.
I need to have that setup available as part of releases/mozilla-release in nearest future in order to not bother with manual merging and receive security updates for XulRuntime engine without headaches. 

>, and apart from the linkage issues
> with libxul I would put this into its own repository pretty much
> permanently. I think that it would be better to just link your code into
> libxul the way seamonkey does, or maintain a fork of the -central code for
> the time being with your patch series in it.
I already maintain separate patch queue with my own builds.
target of this API to be working on top of official XULRuntime builds.
IMPORTANT: So I can trace regressions and figure out faster where something got broken... otherwise bisecting usually take much longer. also it would be easier to report bugs with small test example which works with official XULRUNNER build, so everyone could reproduce/test problem without using special config and setting up build environment.

> 
> As for this particular patch, I really dislike having a class on which we
> hang random stuff called EmbedHelper.

Goal it to make some sort of old style embedding (similar to GtkMozEmbed) but without Platform/Toolkit dependency and ability to build own widget view with own interactions mouse/touch/input events et.c.
Additional functionality supposed to be implemented using External APIS, extensions/components style

Everything is mostly good, but I need to organize rendering part for that embedding.
It is not good to use mozilla/widget/* for that because each embedding need it's own behavior of mouse/input interactions, scrolling, touch et.c
So it is better to use LayerManager API's directly and especially use OMTC compositor setup which is just perfect for embedding

it is quite hard to use basic set of LayerManager API's outside of external linkage, so my idea was to expose this EmbedHelper object with API for:
1) setup OMTC chromium event loop which would help to integrate LayersParent into Native Client rendering context
2) Expose basic set of Layers API which is enough for basic software and accelerated rendering

This problem could be also solved by making Layers API/or some part of it accessible in external linkage but that would require lot more work, and more work for keeping it working in future.

Comment 20

[Oleg Romashin (:romaxa)]

We do build native Android client as official product, and having AndroidBridge integrated as part of libxul
So why we cannot add more generic bridge to libxul which would allow community build native client such like this one: http://snowshoe.qtlabs.org.br, make it possible to work on top of official Mozilla desktop xulrunner builds.

Comment 21

[Oleg Romashin (:romaxa)]

> It is not clear to me that we should accept this code into the main Mozilla
> repository. I don't think that this is a core feature that we would be building into our platform by default
Why we cannot integrate code which not part of official mozilla products? we have xulrunner, libxul.so, why I cannot integrate small piece of code which would help me to use xulrunner builds for embedding?
> I think that it would be better to just link your code into libxul the way seamonkey does

Seamonkey is pretty much final end user product.
and here I'm trying to provide "easy for start" embedding API which would simplify to use official Mozilla engine builds for building other products, experimental projects, native HTML5 app launchers integrated into native toolkit framework et.c.

Comment 22

[Oleg Romashin (:romaxa)]

Brendan do you have any though about this, provide some more obvious reasons why Gecko could not expose embedding API's which would work out of the box?

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #18)
> It is not clear to me that we should accept this code into the main Mozilla
> repository.
If we ever want to get any sane embedding API (and I think we really should have a good embedding API),
we need to get this kind of stuff to m-c

Comment 24

[Jed Leahy]

@Benjamin/Mozilla,

Is there ever going to be any substantive feedback to Oleg on this? TY.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment 18 seems fairly substantive to me, fwiw....

Comment 26

[Jed Leahy]

Right, and nothing since.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Perhaps nothing has changed since?

Comment 28

[Jed Leahy]

Missing the point completely but nvm, lets not spam the thread with pointless dialogue shall we >.>

Comment 29

[Benjamin Smedberg]

Jed, I'm not sure what your relationship is to this project, but here is a copy of an email I sent to Oleg a while back:

Dear Oleg,

Regarding your patches in bug 746800 and 713681, and your work to the OMTC embedding in general: I don't want to discourage you from developing a new embedding API based on gecko. But I'm concerned that the approach you are taking to try to get your embedding to work with a "stock" build of gecko/XULRunner is not a path for success. One of the primary reasons we removed the prior embedding APIs (gtkmozembed and the ActiveX control) was that they represented a significant chunk of code for which there were no tests or testing framework. In addition, they imposed a cost on other more important refactoring and improvements which were taking place in the code.

The "EmbedHelper" patch in particular in bug 746800 is to work around limitation of internal linkage, where you cannot use various layers classes outside of the libxul binary. Rather than trying to create these poorly-named helper classes, why not just build your own code into libxul where it can use internal linkage directly? This would likely be the better engineering choice, and is similar to techniques that seamonkey and thunderbird are using today.

I think that the path for long-term success is for you (us?) to build a community around this code.

* Keep building this code on a branch (using hg/git) and producing regular releases to get people interested and excited about helping.
* Develop a test framework which would allow you to keep track of regressions and make it possible to consider landing this into the main tree.
* As you have been doing, keep landing obvious bugfixes and refactoring which would help make your code easier to integrate.
* Work with the layout/graphics teams to make sure that you're aware of design decisions which affect your architecture.

I think the hardest part of this process will be deciding on things like https://bug713681.bugzilla.mozilla.org/attachment.cgi?id=613631 where a fair bit of in-tree code needs to change. I do think we should probably take *some* refactoring there to aid your project, although it's not clear to me what the best solution is, which is why I asked for cjones' feedback on that patch.

I don't want to speak for Oleg, but I think he is working on other projects now and this bug should probably be marked INCOMPLETE or WONTFIX, but I'll let him comment on that.

Comment 30

[Jed Leahy]

Thank-you for the comprehensive response & apologies for being so presumptuous on this.
I'll got sit in the corner now.... ;-)

Comment 31

[Oleg Romashin (:romaxa)]

Ok, I see Benjamin's point.

I would need to refactor some bit's of my embedding approach, and will do it inside branch.

Also after checking everything once again, I guess I've found more optimal solution which would help to optimize and simplify thread/ipc-communication implementation on client side (using mozilla chromium-ipdl) and would help to cover OOP and OMTC solution with the same API's.

Recently I've been working on building Windows rt application wrapper for these API's, and tried to make it run on Nemo sdk environment.

Yes, I don't have so much time for this implementation as I had before, but I'll try to restore existing functionality withing new setup as soon as possible, so other people could take that skeleton and participate development.

what to do with this bug right now, I'm not sure, probably incomplete solution would be right way to close this bug for now.

Comment 32

[Jed Leahy]

Thanks for the info/update Oleg, appreciated.

Comment 33

[Oleg Romashin (:romaxa)]

Comment on attachment 619126 [details] [diff] [review]
Embed helper OMTC widget/compositing API

this is obsolete for now

Comment 34

[Jed Leahy]

(In reply to Oleg Romashin (:romaxa) from comment #33)
> Comment on attachment 619126 [details] [diff] [review]
> Embed helper OMTC widget/compositing API
> 
> this is obsolete for now

Can you point to what succeeded it? TIA.

Comment 35

[Jed Leahy]

So since this ticket is now superseded by IPCLiteAPI, is there a new one filed for IPCLiteAPI?

Comment 36

[Benjamin Smedberg]

Marking a bunch of bugs in the "Embedding: APIs" component INCOMPLETE in preparation to archive that component. If I have done this incorrectly, please reopen the bugs and move them to a more correct component as we don't have "embedding" APIs any more.