[webkit-reviews] review granted: [Bug 56734] AX: showContextMenu not working in WK2 : [Attachment 86299] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 56734: AX: showContextMenu not working in WK2
https://bugs.webkit.org/show_bug.cgi?id=56734

Attachment 86299: patch
https://bugs.webkit.org/attachment.cgi?id=86299&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=86299&action=review

> Source/WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:2338
> +    // In WK2 use the available crome client.

Typo, "chrome".

> Source/WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:2339
> +    // In WK1, the popup menu can be triggered directly.

Can’t we put this code into the chrome client for WebKit1 also? Is it important
to keep the code here? It seems awkward to have different code for the two and
I don’t see why.

> Source/WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:2341
> +	   PlatformMouseEvent mouseEvent(clickPoint, clickPoint, RightButton,
MouseEventPressed, 1, false, false, false, false, WTF::currentTime());

Should just be able to call currentTime rather than WTF::currentTime.

> Source/WebCore/accessibility/mac/AccessibilityObjectWrapper.mm:2344
> +	   bool handled =
frameView->frame()->eventHandler()->sendContextMenuEvent(mouseEvent);
> +	   if (handled)
> +	       frameView->frame()->page()->chrome()->showContextMenu();

Any need to null check the frame or the page?