110851 - COMify mac event sink API

Status: VERIFIED FIXED

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

Description

[Mike Pinkerton (not reading bugmail)]

Currently, the API to sending events into gecko on mac is nsMacEventSink which
is not an XPCOM api. In addition, this API is effectually a singleton object
(though that's not documented anywhere) and duplicates the functionality that an
embedding app has already performed with regards to event dispatching.

The current proposal is to create nsIEventSink which has the following methods
on it:
- a generic, native event dispatch (taking void*, EventRecord* on carbon,
NSEvent* on Cocoa)
- a method to process an apple event
- a method to process a drag/drop event
- a method to process any relevant timers (maybe)

This api will be implemented by nsMacWindow, accessible by QI from whatever
toplevel widget the embedding app holds on to. We should make mozilla use this
mechanism as well.

The only problem is that some places in our code rely on the
WindowPtr-to-nsIWidget functionality that nsMacMessageSink provides (the
NavServices stuff is one example). We'll have to find a way to move this
functionality, maybe to another service or tie it into an existing service that
embedding applications don't require (windowMediator?).

Comment 1

[Conrad Carlen (not reading bugmail)]

Sounds good.

> - a generic, native event dispatch (taking void*, EventRecord* on carbon,
NSEvent* on Cocoa)
I'd like to see these 3: EventRef (Carbon), EventRecord* (Classic), and NSEvent*
(Cocoa), and no void*

> - a method to process any relevant timers (maybe)

Isn't this just an Idle() method? Maybe this should be on a singleton service
whereas nsIEventSink is window-based.

Comment 2

[Mike Pinkerton (not reading bugmail)]

Attachment: remove nsMacMessageSink, replace with nsIEventSink

patch removes nsMacMessageSink entirely and replaces with a nsIEventSink on
each window. To map WindowPtr->nsIWidget, we now use properties on the root
control of each window. This patches both embedding and standalone, but I
haven't tried to compile the embedding changes yet (long story).

Comment 3

[Conrad Carlen (not reading bugmail)]

Looks good. A few things about nsIEventSink.idl

+  boolean DispatchEvent ( in voidPtr anEvent ) ;

For the anEvent param, it would be good to have some doxygen style comments
about  what this parameter is under different environments. Or, a conditionally
compiled type-def.

+  boolean DispatchMenuEvent ( in voidPtr anEvent, in long aNativeResult ) ;

Since this is only useful internally (to mozilla), maybe there should be
nsIEventSinkInternal and this should go there.

+  boolean DragEvent ( in unsigned long aMessage, in short aMouseGlobalX, in
short aMouseGlobalY,
+                       in unsigned short aKeyModifiers ) ;

Since this handles dragging and dropping, should it be renamed "DragDropEvent?"

Comment 4

[Mike Pinkerton (not reading bugmail)]

i agree about more header comments needed. I also want to wait and see how our
carbon-event story pans out wrt these interfaces before changing too much up
front. Obviously, we'll need both a EventRecord* and an EventRef version while
we're in transition. Once we need both, we can add both. Maybe I'll go back and
change the voidPtr to an EventRecord& for the time being since that's all we're
currently using.

i'll factor out DispatchMenuEvent into a separate interface. New patch will
follow when I'm done testing the embedding stuff.

I think it's ok to keep it "DragEvent" since it's a general term (it's the "Drag
Manager", not the "Drag&Drop Manager").

Comment 5

[saari (gone)]

is nsIEventSink XP?
If not I'd suggest nsIMacEventSink

Comment 6

[Mike Pinkerton (not reading bugmail)]

the more i think about it, i think we should keep everything generic for the
time being. there's nothing (yet) in this file that makes it mac-specific so I
don't think we want to change the name. There's nothing to stop some other
platform from adopting this interface.

Comment 7

[Mike Pinkerton (not reading bugmail)]

Attachment: this patch builds

this new patch adds some comments and actually builds in embedding.

Comment 8

[Simon Fraser [no longer active]]

Comments:

 interface nsIEventSink : nsISupports
 {
...
+  boolean DispatchEvent ( in voidPtr anEvent ) ;

IDL methods should have initial lowercase. They get uppercaseed in the C++ 
headers.

+                                 nsCOMPtr<nsIWidget> topWidget;
+                                 nsToolkit::GetTopWidget ( whichWindow, 
getter_AddRefs(topWidget) );
+                                 nsCOMPtr<nsPIWidgetMac> macWindow ( 
do_QueryInterface(topWidget) );
+                                 if ( macWindow )

You do this 3 times, so maybe factor into a helper method.

+  nsCOMPtr<nsIEventSink> sink;
+  nsToolkit::GetWindowEventSink ( aWindow, getter_AddRefs(sink) );
...
+  nsCOMPtr<nsIEventSink> sink;
+  GetWindowEventSink ( aWindow, getter_AddRefs(sink) );

Any reason for the lack of "nsToolkit::" the second time?

+  OSStatus scpStatus = ::SetControlProperty ( rootControl, 'MOSS', 'GEKO', 
sizeof(nsIWidget*), &temp );

Should probably use properties on the WindowPtr for this.

+    
+    // we don't want activate events for this window
+    ::ChangeWindowAttributes ( mWindowPtr, kWindowNoActivatesAttribute, 
kWindowNoAttributes );

Should not be in this diff.

+       if ( GetRootControl(aWindow, &rootControl) == noErr ) {
+         nsIWidget* topLevelWidget = nsnull;
+         ::GetControlProperty ( rootControl, 'MOSS', 'GEKO', sizeof(nsIWidget*), 
nsnull, (void*)&topLevelWidget);

Fix to use properties on the window.

+  nsCOMPtr<nsIWidget> topWidget;
+  GetTopWidget ( aWindow, getter_AddRefs(topWidget) );
+       nsCOMPtr<nsIEventSink> sink ( do_QueryInterface(topWidget) );
+       if ( sink ) {
+    *outSink = sink;
+    NS_ADDREF(*outSink);

Tab/space mess.

+void
+CEmbedEventAttachment::GetTopWidget ( WindowPtr aWindow, nsIWidget** outWidget )
+{
+  *outWidget = nsnull;
+  ControlHandle rootControl = nsnull;
+  PRBool handled = PR_FALSE;
+       if ( GetRootControl(aWindow, &rootControl) == noErr ) {
+         nsIWidget* topLevelWidget = nsnull;
+         ::GetControlProperty ( rootControl, 'MOSS', 'GEKO', sizeof(nsIWidget*), 
nsnull, (void*)&topLevelWidget);
+    if ( topLevelWidget ) {
+      *outWidget = topLevelWidget;
+      NS_ADDREF(*outWidget);
+    }
+  }
+}

Does this code really have to be in 2 places, on the outside of the embedding 
firewall? This seems wrong. Since the code that sets up the control properties 
(soon to be window properties) is inside of Gecko, the code that gets the 
property should be inside also, so that this knowledge is hidden from embedders.
pink says this will go away, but I'd like to see a bug #.

Comment 9

[Mike Pinkerton (not reading bugmail)]

patch coming to fix the small things and use the window property. smfr and i
already talked about the GetWindowEventSink code being in 2 places and agreed
that the embedding usage will go away when we fix other problems with window
creation notification for embedding, beyond the scope of this bug.

Comment 10

[Conrad Carlen (not reading bugmail)]

I hope you're still planning on creating nsIEventSinkInternal and putting
+  boolean DispatchMenuEvent ( in voidPtr anEvent, in long aNativeResult ) ;
on it.

Also, as an idl style guideline, see
http://lxr.mozilla.org/mozilla/source/embedding/browser/webBrowser/nsIEmbeddingSiteWindow.idl

Its status should be UNDER_REVIEW.

Comment 11

[Mike Pinkerton (not reading bugmail)]

Attachment: final patch

final patch corrects issues, separates out HandleMenuEvent into a private
interface, merged with the tip, uses window properties instead of control
properties.

Comment 12

[Mike Pinkerton (not reading bugmail)]

needing r/sr.

Comment 13

[Conrad Carlen (not reading bugmail)]

(1)
@@ -94,4 +98,9 @@
   static void AppInBackground ( ) ;
   static bool IsAppInForeground ( ) ;
+
+  // utility routines for getting the toplevel widget and event sink
+  // stashed in the root control.

This comment isn't right anymore.

(2)
 Boolean CEmbedEventAttachment::IsAlienGeckoWindow(WindowPtr inMacWindow)
 {
+    // it's an 'alien' window if there's no LWindow object and there is
+    // an nsIEventSink stashed in the window's root control.
+    nsCOMPtr<nsIEventSink> sink;
+    GetWindowEventSink ( inMacWindow, getter_AddRefs(sink) );    
     return (inMacWindow &&
-            !LWindow::FetchWindowObject(inMacWindow) &&
-
        mMessageSink.IsRaptorWindow(inMacWindow));
+            !LWindow::FetchWindowObject(inMacWindow) && sink);
 }

Since GetWindowEventSink is the most expensive part of this, it should only be
called if we haven't returned due to one of the first two conditions. Break it
up into 2 or more statements.

(3)
I would make nsPIEventSinkStandalone descend from nsIEventSink since the two are
so clearly the same thing. We don't often use inheritance in ifaces but there
are  cases where it makes sense. In this case, the PI version is just for hiding
from embeddors. That's what was done with nsIProfileInternal : nsIProfile -
though I admit, that's my own example ;-)

Since I could be wrong about (3) and the other 2 points were minor, r=ccarlen.

Comment 14

[Conrad Carlen (not reading bugmail)]

Comment on attachment 59434 [details] [diff] [review]
final patch

r=ccarlen

Comment 15

[Mike Pinkerton (not reading bugmail)]

Attachment: fixes conrad's comments

(hopefully) final patch fixing (1) and (2) from conrad.

needing r/sr.

Comment 16

[Conrad Carlen (not reading bugmail)]

Comment on attachment 59528 [details] [diff] [review]
fixes conrad's comments

r=ccarlen

Comment 17

[Simon Fraser [no longer active]]

Comment on attachment 59528 [details] [diff] [review]
fixes conrad's comments

sr=sfraser

Comment 18

[Mike Pinkerton (not reading bugmail)]

fixed.

Comment 19

[Michael Dunn]

Clean up verification of dated code change bus