Bug 98890 – EventHandler::handleGestureLongPress breaks compilation if CONTEXT_MENUS isn't set

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Luciano Wolf]

If CONTEXT_MENU isn't enabled there will be a call to "sendContextMenuEventForGesture(gestureEvent);" that is wrapped with "#if ENABLE(CONTEXT_MENU)" and the code won't compile.

[Luciano Wolf]

Created attachment 167984 [details]
Proposed patch

Enable EventHandler::handleGestureLongPress only if CONTEXT_MENU is set

[Build Bot]

Comment on attachment 167984 [details]
Proposed patch

Attachment 167984 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14238708

[WebKit Review Bot]

Comment on attachment 167984 [details]
Proposed patch

Attachment 167984 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14248495

[Early Warning System Bot]

Comment on attachment 167984 [details]
Proposed patch

Attachment 167984 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14245514

[Early Warning System Bot]

Comment on attachment 167984 [details]
Proposed patch

Attachment 167984 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14252183

[Peter Beverloo (cr-android ews)]

Comment on attachment 167984 [details]
Proposed patch

Attachment 167984 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14245517

[Caio Marcelo de Oliveira Filho]

Comment on attachment 167984 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167984&action=review

> Source/WebCore/page/EventHandler.cpp:2573
> +#if ENABLE(CONTEXT_MENU)

This ifdef is in the wrong place. Shouldn't it be protecting the sendContextMenuEventForGesture() call?

[Luciano Wolf]

Created attachment 167997 [details]
Another proposed patch

[Luciano Wolf]

(In reply to comment #7)
> (From update of attachment 167984 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167984&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:2573
> > +#if ENABLE(CONTEXT_MENU)
> 
> This ifdef is in the wrong place. Shouldn't it be protecting the sendContextMenuEventForGesture() call?

But in this case we've to return something. If you're not ANDROID the only line remaining is this sendContextMenuEventForGesture(). Should we just return "false" (after moving ENABLE(CONTEXT_MENUS) to the proper place)?

[Caio Marcelo de Oliveira Filho]

Comment on attachment 167984 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167984&action=review

>>> Source/WebCore/page/EventHandler.cpp:2573
>>> +#if ENABLE(CONTEXT_MENU)
>> 
>> This ifdef is in the wrong place. Shouldn't it be protecting the sendContextMenuEventForGesture() call?
> 
> But in this case we've to return something. If you're not ANDROID the only line remaining is this sendContextMenuEventForGesture(). Should we just return "false" (after moving ENABLE(CONTEXT_MENUS) to the proper place)?

Yes. But sendContextMenuEventForGesture() is only declared if we have CONTEXT_MENUS (see EventHandler.h), so you need to guard it.

My understanding from the event handling is that if it's not defined you can simply return false here. So add an else case for the #if ENABLE(CONTEXT_MENUS) returning false should be enough.

[Luciano Wolf]

Created attachment 168010 [details]
Another proposed patch

[Caio Marcelo de Oliveira Filho]

Comment on attachment 168010 [details]
Another proposed patch

Let's wait the EWS just in case :-)

[Luciano Wolf]

Created attachment 168020 [details]
Patch for landing

[Luciano Wolf]

Created attachment 168022 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 168022 [details]
Patch for landing

Rejecting attachment 168022 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
it2/ChangeLog
Auto-merging Source/WebKit2/win/WebKit2.def
Failed to merge in the changes.
Patch failed at 0001 Needs internal API to return distributed nodes for InsertionPoint

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.

Full output: http://queues.webkit.org/results/14260093

[Caio Marcelo de Oliveira Filho]

Committed r130930: <http://trac.webkit.org/changeset/130930>