383560 - clean up cocoa child view event handling code

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Josh Aas]

Attachment: fix v1.0

This patch does a bunch of event handling cleanup without changing any behavior.

1. Comments are more clear, standardized.
2. Common event-related code patterns are standardized.
3. Better event conversion method names for clarity.
4. Native-to-gecko event conversion re-factored for clarity.
5. Got rid of some duplicate code.
6. Moved some methods for more logical organization.

Comment 1

[Josh Aas]

BTW, there is a bunch more cleanup we can do but this patch is already big and I don't want to make it bigger.

Comment 2

[Stuart Morgan]

Comment on attachment 267542 [details] [diff] [review]
fix v1.0

>+- (void) convertCocoaMouseEvent:(NSEvent*)aMouseEvent toGeckoEvent:(nsInputEvent*)outGeckoEvent;

If it's possible to fold the actual creation of the gecko event into this method and have it be something like
(nsInputEvent*)geckoEventForCocoaMouseEvent:(NSEvent*)aMouseEvent

>-  [(ChildView*)mView convertEvent:nil toGeckoEvent:&geckoEvent];
>-
>-  // Use our own coordinates in the gecko event.
>-  // Convert event from gecko global coords to gecko view coords.
>-  NSPoint localPoint = NSMakePoint(aMouseGlobalX, aMouseGlobalY);
>-  FlipCocoaScreenCoordinate(localPoint);
>-  localPoint = [[mView window] convertScreenToBase:localPoint];
>-  localPoint = [mView convertPoint:localPoint fromView:nil];
>-  geckoEvent.refPoint.x = NS_STATIC_CAST(nscoord, localPoint.x);
>-  geckoEvent.refPoint.y = NS_STATIC_CAST(nscoord, localPoint.y);
>+  [(ChildView*)mView convertCocoaMouseEvent:nil toGeckoEvent:&geckoEvent];

|convertCocoaMouseEvent:nil ...| is a no-op now; why is it there, and how is this not changing behavior?

>+- (void) genericCocoaEvent:(NSEvent*)inEvent toGeckoEvent:(nsInputEvent*)outGeckoEvent

Why doesn't this method use the 'convert' prefix?

>+  if (!aMouseEvent || !outGeckoEvent)
>+    return;
...
>+  if (!aKeyEvent || !outGeckoEvent)
>+    return;

Enforcing a non-nil aMouseEvent/aKeyEvent is new, so again it seems like this isn't behavior-preserving.

It seems like outGeckoEvent probably be an assert, rather than just a check.

Comment 3

[Josh Aas]

Attachment: fix v1.1

Thanks for the nice catch. I didn't intend to do that at all.

I don't want to fold gecko event creation into those methods right now because of scoping/lifetime issues. Maybe I would in a later patch.

Comment 4

[Josh Aas]

Another thing I forgot to mention that is hard to see in the patch...

-    if (!outGeckoEvent->isControl && !outGeckoEvent->isMeta)
-      outGeckoEvent->isControl = outGeckoEvent->isAlt = outGeckoEvent->isMeta = 0;

I completely removed this. First of all, isControl and isMeta are obviously already 0, so setting those is unnecessary. Secondly, it doesn't make any sense to set "isAlt" to false just because meta and control are false. I looked back in cvs blame to see why that was done and it looks like a mistake to me. Jinglepants just copied it over from Carbon where the logic is actually slightly different anyway (though it still looks buggy in Carbon to me).

Comment 5

[Colin Barrett [:cbarrett]]

Comment on attachment 267604 [details] [diff] [review]
fix v1.1

r=me, although I'd like to see the gecko events being created by the methods eventually.

I didn't notice anything else that might not be behavior preserving.

Comment 6

[Josh Aas]

Attachment: window fix v1.0

Just putting some window cleanup on this bug as well.

Comment 7

[Stuart Morgan]

Comment on attachment 267718 [details] [diff] [review]
window fix v1.0

>+  if (gRollupListener != nsnull && gRollupWidget != nsnull)

I'd prefer to see this changed to |if (gRollupListener && gRollupWidget)| while you are touching it, but r=me either way.

Comment 8

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 267718 [details] [diff] [review]
window fix v1.0

sr=pink

Comment 9

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 267604 [details] [diff] [review]
fix v1.1

sr=pink

Comment 10

[Josh Aas]

checked in "window fix v1.0"

Comment 11

[Josh Aas]

checked in "fix v1.0"