44120 - need Idle routine in embedding api.

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect, P1)

Description

[Doug Turner (:dougt)]

Another important point is to make sure to call Repeater::DoIdlers() and
  Repeater::DoRepeaters(), and PR_Sleep() in your event loop. It won't work
  without this. Look at the code in the PowerPlant sample for how to do this.
  This code was updated monday so get a fresh copy.


Adam, we need to add this to the embedding API.  I would like an |Idle| routine 
so that users don't have to now about the details of nspr 
event handling.

Comment 1

[Conrad Carlen]

Adam, check out <http://lxr.mozilla.org/seamonkey/source/widget/src/mac/

nsMacMessagePump.cpp#413> Notice the diff in how DoRepeaters() and how DoIdlers() 

is called and that they need to get the OS Event. I'm not sure if there is this 

distinction (or even this mechanism) on Windows.

Comment 2

[Judson Valeski]

dougt, should this be part of 43990?

Comment 3

[Doug Turner (:dougt)]

actually it should be fixed with 44678

Comment 4

[Adam Lock]

Do we have something that can reasonably be called an API for this feature or
are we expecting embedders on certain platforms to have to include some C++ classes?

I'm not very familiar with the problems of handing events on the Mac or with
GTK, but would it be possible to boil all this stuff down into a single function
call that the programmer could insert into their event handling loop?

Comment 5

[lchiang]

per email with Jud, changing nsbeta3+ to nsbeta3- on all "embed" keyword bugs
since embedding changes will not be made in the mn6 branch. If you feel this bug
fix needs to go into mn6 branch, please list the reasons/user impact/risk and
nominate for rtm. Thanks.

Comment 6

[Ed]

Please change severity to critical on Windows NT platform, this bug at least affect
web page redirection (e.g. http://www.dmg.com) and continuous scrolling. Have very
bad impact and need to be fixed ASAP.

Comment 7

[Adam Lock]

Ed, are you sure the problems you are seeing are to do with this bug? AFAIK 
Win32 is the one platform that _doesn't_ need the idle routine because on Win32 
timer events are done with WM_TIMER messages that the app's message pump will 
dispatch correctly when it's doing its own messages.

Here is an example redirect page that redirects fine from the winEmbed sample:

http://www.as.ua.edu/math/icad-brl.htm

Comment 8

[Adam Lock]

Attachment: Draft idle loop APIs

Comment 9

[Adam Lock]

I've attached a couple of API methods for doing idle event processing and a 
Win32 implementation of each them.

The first method, NS_DoIdleEmbeddingStuff() fires timers and so on. Embedders 
should call this when they are idle.

The second method, NS_HandleEmbeddingEvent() is for platforms like the Mac where 
events must be dispatched manually.

The Win32 implementation dispatches Javascript timers for the first method and 
nothing for the second.

Conrad & Doug, can I have feedback as to if this is the right way I should be 
proceeding? If so I'd like to get one platform working and checked in and others 
can follow.

Comment 10

[Doug Turner (:dougt)]

As for NS_HandleEmbeddingEvent() on the mac, I have a patch sitting in bug 
http://bugzilla.mozilla.org/show_bug.cgi?id=44678 which will make/allow the mac 
able to process nspr events in it eventloop.  You should take a look at it... 
maybe run some benchmarks as simon fraser suggests to ensure that I did not slow 
things down.  I did alot of testing with this patch, and nothing seamed to 
break... simon and I agreed that it was a cleaner way to do things, but the 
patch needs some perf evaluation.

Comment 11

[Adam Lock]

Attachment: Updated header adds protection for unimplemented platforms

Comment 12

[Adam Lock]

Doug, I've updated the code so that the functions won't be available on 
unsupported platforms. The only supported platform so far is Win32 while I 
figure out what's right for the other platforms. 

Since I am totally unfamiliar with Mac events (& my CodeWarrior is still on 
order) I need to know is if this approach is the correct way to go. Are the 
functions suitable abstractions for all platforms or is the Mac or Linux likely 
to require something different?

If the former then I'd like to checkin this code as-is since it should compile 
to nothing on everything except Win32.

Comment 13

[Conrad Carlen (not reading bugmail)]

As far as the Mac goes: NS_HandleEmbeddingEvent() could use or do the equiv. of
nsMacMessageSink::DispatchOSEvent(). This is what my embedding sample uses.
Another thing the Mac needs to do at idle time is call nsRepeater::DoRepeaters()
and nsRepeater::DoIdlers() and PR_Sleep(). See
http://lxr.mozilla.org/seamonkey/source/embedding/browser/powerplant/source/CBrowserApp.cp#290
I could fill in the Mac specifics in the API if you want.

Comment 14

[Doug Turner (:dougt)]

For linux, gtk specifically, we already have a callback which the OS calls
during idle time.  In this callback we currently just call processPendingEvents
on the UI event queue.  We could replace or add this embedding idle call to this
callback.  We will have to play around with where exactly to place this idle
call.  It mayh be necessary to create a seperate callback at a lower priority. 
Nonetheless, I think that your API will be a nice fit for linux.

Also, maybe embedding should also be abstracting the nspr/xpcom event queue
processing?  Any thoughts? 

Also, I noticed in the last patch we are going to implement all the different
platforms in the same file.  Although it is more a style based nit, I would
rather see each of the platform inplementions in different files.


  

Comment 15

[Adam Lock]

Attachment: A few more tweaks, made file naming OS-specific

Comment 16

[Paul Wyskoczka]

Updating QA Contact

Comment 17

[Adam Lock]

The new routines have been checked in and work on the Win32 platform. They 
compile to nothing on other platforms. winEmbed has been updated to demonstrate 
how they are used.

Doug, Conrad feel free to take a stab at doing the Linux/Mac versions.

Comment 18

[Chris Waterson]

Adam, will this fix bug 58701?

Comment 19

[Chris Waterson]

Hey adam, I notice that with your recent changes, winEmbed now pegs the CPU when
idle (on Win2K, anyway). Is that what you expected?

Comment 20

[Adam Lock]

Attachment: Revised message loop for winEmbed

Comment 21

[Adam Lock]

The latest patch fixes the 100% CPU usage by reorganizing things a bit and 
putting in a call to WaitMessage().

I'm investigating whether the call to WaitMessage may reintroduce stalling - it 
doesn't seem to on my box but I'm not convinced yet. I may have have to write 
some code to periodically post something to the queue (e.g. WM_TIMER) to kick 
the loop through another cycle.

Comment 22

[Judson Valeski]

adam, what's your stalling concern. I don't see what's wrong w/ WaitMessage().

Comment 23

[Adam Lock]

Basically I think that an app could be waiting a long time for message to 
arrive, especially on an unattended machine. In that situation the timers will 
not be processed.

The fix is to kick off a timer which will ensure that periodic WM_TIMER messages 
will arrive to wake up the message processing loop.

Comment 24

[Judson Valeski]

ick. good point. Can you specify a timeout on WaitMessage()?

Comment 25

[Adam Lock]

WaitMessage() doesn't have a timeout but it might be possible to do something 
clever with MsgWaitForMultipleObjects() - a method that does have a timeout 
value

Comment 26

[Adam Lock]

Attachment: Message loop using MsgWaitForMultipleObjects to guarantee idle processing even when there are no messages

Comment 27

[Adam Lock]

I replaced WaitMessage() with a call to MsgWaitForMultipleObjects(). The call 
waits for a new message to arrive, the event object to fire or a 100ms timeout 
to occur. The event object is just a fake to keep the function happy.

Comment 28

[Adam Lock]

Marking future. Win32 stuff is in, other platforms will get support as and when 
a demand arises (with accompanying volunteers) to provide it.

Comment 29

[Michael Dunn]

Moving API bug ownership to David Epstein.

Comment 30

[Michael Dunn]

Correction: Changing QA contact for the Embed API bugs to David Epstein.

Comment 31

[Adam Lock]

Closing this bug. Open new bugs for other platforms.

Comment 32

[David Epstein]

NS_HandleEmbeddingEvent() and NS_DoIdleEmbeddingStuff() are used in WinEmbed
(called in WinEmbed.cpp), declared in nsEmbedAPI.h

Comment 33

[Alex Weyers]

David - 

Just out of curiosity, why is NS_HandleEmbeddingEvent called in WinEmbed.cpp 
if, in fact, the method is superfluous (on Windows)?

Comment 34

[Judson Valeski]

winembed is old and crusty. it's not even built anymore AFAIK.

Comment 35

[Dan M]

*I* still like winEmbed. It's easier and nicer and reminds me of simpler days.
It works (mostly) great, though it does have some problems. From the current
design intention, it looks like we should remove the call to
NS_HandleEmbeddingEvent from winEmbed's event loop.