Avoid dispatching gesture events of unknown types

• Component: New Bugs
• Status: RESOLVED FIXED

[Sadrul Habib Chowdhury]

Avoid dispatching gesture events of unknown types

[Sadrul Habib Chowdhury]

Created attachment 156235 [details]
Patch

[Adam Barth]

Comment on attachment 156235 [details]
Patch

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

> Source/WebCore/dom/GestureEvent.cpp:63
>          ASSERT_NOT_REACHED();
> +        return 0;

Does that means this ASSERT isn't valid?

[Sadrul Habib Chowdhury]

Comment on attachment 156235 [details]
Patch

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

>> Source/WebCore/dom/GestureEvent.cpp:63
>> +        return 0;
> 
> Does that means this ASSERT isn't valid?

WebCore does not currently have event-types corresponding to pinch, long-press etc that are there in Platform gesture-events. Ideally, if one of these events is dispatched into WebCore, then we would want to process them appropriately. So the ASSERT here would be useful for development purposes. However, the ASSERT becomes a no-op on release builds. So it is necessary to guard against that too.

Would you rather remove the ASSERT and just return 0?

[Adam Barth]

Either the case can occur or it can't.  If the case can occur, then ASSERT_NOT_REACHED is inappropriate because that line of code can be reached.  If the case cannot occur, then the return is not needed.

If you want protection in a release build, we can use CRASH() rather than ASSERT_NOT_REACHED.

[Sadrul Habib Chowdhury]

Created attachment 156241 [details]
Patch

[Adam Barth]

Comment on attachment 156241 [details]
Patch

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

> Source/WebCore/dom/GestureEvent.cpp:63
> +        return 0;

There's no point in returning zero after a crash :)

[Sadrul Habib Chowdhury]

Created attachment 156242 [details]
Patch

[Sadrul Habib Chowdhury]

(In reply to comment #6)
> (From update of attachment 156241 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156241&action=review
> 
> > Source/WebCore/dom/GestureEvent.cpp:63
> > +        return 0;
> 
> There's no point in returning zero after a crash :)

Indeed. :) Sorry, I misunderstood. We do not want to trigger a crash in release build. I was more trying to trigger a crash in a debug build with the ASSERT (so that we know some gesture events are reaching WebCore and we need to add support for those), but handle gracefully in a release build. I have changed to remove the ASSERT instead, and always return 0.

[Adam Barth]

Comment on attachment 156242 [details]
Patch

Ok.

[WebKit Review Bot]

Comment on attachment 156242 [details]
Patch

Clearing flags on attachment: 156242

Committed r124633: <http://trac.webkit.org/changeset/124633>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Allan Sandfeld Jensen]

(In reply to comment #3)
> (From update of attachment 156235 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156235&action=review
> 
> >> Source/WebCore/dom/GestureEvent.cpp:63
> >> +        return 0;
> > 
> > Does that means this ASSERT isn't valid?
> 
> WebCore does not currently have event-types corresponding to pinch, long-press etc that are there in Platform gesture-events. Ideally, if one of these events is dispatched into WebCore, then we would want to process them appropriately. So the ASSERT here would be useful for development purposes. However, the ASSERT becomes a no-op on release builds. So it is necessary to guard against that too.
> 
> Would you rather remove the ASSERT and just return 0?

I think assert and returning 0 would be correct. Types that have not been specified should not be send into webcore and should be caught in debug mode, but the return was missing for release build to also act correctly.

[Adam Barth]

> I think assert and returning 0 would be correct. Types that have not been specified should not be send into webcore and should be caught in debug mode, but the return was missing for release build to also act correctly.

Can the case occur or can the case not occur?