Bug 103058 – It's confusing that return values of 'bool Node::dispatchEvent(...)' and 'bool Node::dispatchMouseEvent(..)' have the opposite meanings.

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

[Hayato Ito]

Every return values from 'bool dispatchXXXEvent(...)' member functions should have the same meanings to avoid confusions.

[Hayato Ito]

I think we should use enum, or struct, as a return value, but that might be a goal in the long term.

[Hayato Ito]

Created attachment 175653 [details]
Make them return the same meaning bool.

[WebKit Review Bot]

Comment on attachment 175653 [details]
Make them return the same meaning bool.

Clearing flags on attachment: 175653

Committed r135650: <http://trac.webkit.org/changeset/135650>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Min Qin]

I am having a hard time understanding this patch and it doesn't look correct to me.


after this patch, MouseEventDispatchMediator::dispatchEvent will return a true instead of false in case the event was not canceled. I am ok with that, but better add some comments in the header to indicate that.

This patch also tries to change the return value of EventHandler::dispatchMouseEvent(). However, this doesn't seem to cover all the paths in which swallowEvent is changed. For example, in some code paths there is swallowEvent = true and that is not changed. However, this patch will return !swallowEvent instead, isn't that changing the behavior of those cases?

What confuses me most is handleMousePressEvent(). Why we change the return value to be !swallowEvent?  What should be the return value of handleMousePressEvent()? Can you add a comment in the header? If this is to indicate whether the event was canceled or not, why not change the return value of handleMouseReleaseEvent? 

I think we need some documentation in eventhandler. And we definitely need some test here to see what is being changed and whether that is correct.

[Hayato Ito]

Thank you for the comment.

(In reply to comment #5)
> I am having a hard time understanding this patch and it doesn't look correct to me.
> 
> 
> after this patch, MouseEventDispatchMediator::dispatchEvent will return a true instead of false in case the event was not canceled. I am ok with that, but better add some comments in the header to indicate that.
> 
> This patch also tries to change the return value of EventHandler::dispatchMouseEvent(). However, this doesn't seem to cover all the paths in which swallowEvent is changed. For example, in some code paths there is swallowEvent = true and that is not changed. However, this patch will return !swallowEvent instead, isn't that changing the behavior of those cases?
> 
> What confuses me most is handleMousePressEvent(). Why we change the return value to be !swallowEvent?  What should be the return value of handleMousePressEvent()? Can you add a comment in the header? If this is to indicate whether the event was canceled or not, why not change the return value of handleMouseReleaseEvent? 

That was on my rader when I wrote this patch, but I gave up changing all at once. That requires a lot of changes. I don't know the historical reason, but dispatchXXXEvent and handleXXXEvent use bool as opposite meaning as far as I remember. This patch attacked only dispatchXXXEvent, without touching handleXXEvent.

I think using 'bool' in both cases as a return value is the root cause of confusion. Actually I tried to introduce enum, but I was not confident whether using enum here is good or not since we have to change a lot of code.

> 
> I think we need some documentation in eventhandler. And we definitely need some test here to see what is being changed and whether that is correct.

Agreed. We have to resolve the current inconsistency usage of bool. That requires further patches.

[Min Qin]

(In reply to comment #6)
> Thank you for the comment.
> 
> (In reply to comment #5)
> > I am having a hard time understanding this patch and it doesn't look correct to me.
> > 
> > 
> > after this patch, MouseEventDispatchMediator::dispatchEvent will return a true instead of false in case the event was not canceled. I am ok with that, but better add some comments in the header to indicate that.
> > 
> > This patch also tries to change the return value of EventHandler::dispatchMouseEvent(). However, this doesn't seem to cover all the paths in which swallowEvent is changed. For example, in some code paths there is swallowEvent = true and that is not changed. However, this patch will return !swallowEvent instead, isn't that changing the behavior of those cases?
> > 
> > What confuses me most is handleMousePressEvent(). Why we change the return value to be !swallowEvent?  What should be the return value of handleMousePressEvent()? Can you add a comment in the header? If this is to indicate whether the event was canceled or not, why not change the return value of handleMouseReleaseEvent? 
> 
> That was on my rader when I wrote this patch, but I gave up changing all at once. That requires a lot of changes. I don't know the historical reason, but dispatchXXXEvent and handleXXXEvent use bool as opposite meaning as far as I remember. This patch attacked only dispatchXXXEvent, without touching handleXXEvent.


then we should not return !swallowevent in handlemousepressevent. that changes the previous behavior.

> 
> I think using 'bool' in both cases as a return value is the root cause of confusion. Actually I tried to introduce enum, but I was not confident whether using enum here is good or not since we have to change a lot of code.
> 
> > 
> > I think we need some documentation in eventhandler. And we definitely need some test here to see what is being changed and whether that is correct.
> 
> Agreed. We have to resolve the current inconsistency usage of bool. That requires further patches.

[Hayato Ito]

(In reply to comment #7)
> (In reply to comment #6)
> > Thank you for the comment.
> > 
> > (In reply to comment #5)
> > > I am having a hard time understanding this patch and it doesn't look correct to me.
> > > 
> > > 
> > > after this patch, MouseEventDispatchMediator::dispatchEvent will return a true instead of false in case the event was not canceled. I am ok with that, but better add some comments in the header to indicate that.
> > > 
> > > This patch also tries to change the return value of EventHandler::dispatchMouseEvent(). However, this doesn't seem to cover all the paths in which swallowEvent is changed. For example, in some code paths there is swallowEvent = true and that is not changed. However, this patch will return !swallowEvent instead, isn't that changing the behavior of those cases?
> > > 
> > > What confuses me most is handleMousePressEvent(). Why we change the return value to be !swallowEvent?  What should be the return value of handleMousePressEvent()? Can you add a comment in the header? If this is to indicate whether the event was canceled or not, why not change the return value of handleMouseReleaseEvent? 
> > 
> > That was on my rader when I wrote this patch, but I gave up changing all at once. That requires a lot of changes. I don't know the historical reason, but dispatchXXXEvent and handleXXXEvent use bool as opposite meaning as far as I remember. This patch attacked only dispatchXXXEvent, without touching handleXXEvent.
> 
> 
> then we should not return !swallowevent in handlemousepressevent. that changes the previous behavior.

I understand that. That might not be my intention. Let me investigate the current code base and codebase when this patch was landed further.

In current code base, the only client which uses the return value of the EventHandler::handleMousePressed(const PlatformMouseEvent&) looks EventHandler::handleGensutureTap().
Is that right?


> 
> > 
> > I think using 'bool' in both cases as a return value is the root cause of confusion. Actually I tried to introduce enum, but I was not confident whether using enum here is good or not since we have to change a lot of code.
> > 
> > > 
> > > I think we need some documentation in eventhandler. And we definitely need some test here to see what is being changed and whether that is correct.
> > 
> > Agreed. We have to resolve the current inconsistency usage of bool. That requires further patches.

[Hayato Ito]

I investigated. This change of line (!swallowEvent)  in handleMousePressEvent seems unnecessary as you mentioned.
The bad part is there isn't any test which catch that.

Let me revert the line in another patch. I'll add a layout test too.

Thank you for reporting. I missed that.


(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > Thank you for the comment.
> > > 
> > > (In reply to comment #5)
> > > > I am having a hard time understanding this patch and it doesn't look correct to me.
> > > > 
> > > > 
> > > > after this patch, MouseEventDispatchMediator::dispatchEvent will return a true instead of false in case the event was not canceled. I am ok with that, but better add some comments in the header to indicate that.
> > > > 
> > > > This patch also tries to change the return value of EventHandler::dispatchMouseEvent(). However, this doesn't seem to cover all the paths in which swallowEvent is changed. For example, in some code paths there is swallowEvent = true and that is not changed. However, this patch will return !swallowEvent instead, isn't that changing the behavior of those cases?
> > > > 
> > > > What confuses me most is handleMousePressEvent(). Why we change the return value to be !swallowEvent?  What should be the return value of handleMousePressEvent()? Can you add a comment in the header? If this is to indicate whether the event was canceled or not, why not change the return value of handleMouseReleaseEvent? 
> > > 
> > > That was on my rader when I wrote this patch, but I gave up changing all at once. That requires a lot of changes. I don't know the historical reason, but dispatchXXXEvent and handleXXXEvent use bool as opposite meaning as far as I remember. This patch attacked only dispatchXXXEvent, without touching handleXXEvent.
> > 
> > 
> > then we should not return !swallowevent in handlemousepressevent. that changes the previous behavior.
> 
> I understand that. That might not be my intention. Let me investigate the current code base and codebase when this patch was landed further.
> 
> In current code base, the only client which uses the return value of the EventHandler::handleMousePressed(const PlatformMouseEvent&) looks EventHandler::handleGensutureTap().
> Is that right?
> 
> 
> > 
> > > 
> > > I think using 'bool' in both cases as a return value is the root cause of confusion. Actually I tried to introduce enum, but I was not confident whether using enum here is good or not since we have to change a lot of code.
> > > 
> > > > 
> > > > I think we need some documentation in eventhandler. And we definitely need some test here to see what is being changed and whether that is correct.
> > > 
> > > Agreed. We have to resolve the current inconsistency usage of bool. That requires further patches.

[Hayato Ito]

Comment on attachment 175653 [details]
Make them return the same meaning bool.

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

> Source/WebCore/page/EventHandler.cpp:1691
> +    return !swallowEvent;

I'll try to revert only this line. If you find any other things, please let me know.

[Min Qin]

Thanks, this line should only affect HandleGestureTap()

(In reply to comment #9)
> I investigated. This change of line (!swallowEvent)  in handleMousePressEvent seems unnecessary as you mentioned.
> The bad part is there isn't any test which catch that.
> 
> Let me revert the line in another patch. I'll add a layout test too.
> 
> Thank you for reporting. I missed that.
> 
> 
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #6)
> > > > Thank you for the comment.
> > > > 
> > > > (In reply to comment #5)
> > > > > I am having a hard time understanding this patch and it doesn't look correct to me.
> > > > > 
> > > > > 
> > > > > after this patch, MouseEventDispatchMediator::dispatchEvent will return a true instead of false in case the event was not canceled. I am ok with that, but better add some comments in the header to indicate that.
> > > > > 
> > > > > This patch also tries to change the return value of EventHandler::dispatchMouseEvent(). However, this doesn't seem to cover all the paths in which swallowEvent is changed. For example, in some code paths there is swallowEvent = true and that is not changed. However, this patch will return !swallowEvent instead, isn't that changing the behavior of those cases?
> > > > > 
> > > > > What confuses me most is handleMousePressEvent(). Why we change the return value to be !swallowEvent?  What should be the return value of handleMousePressEvent()? Can you add a comment in the header? If this is to indicate whether the event was canceled or not, why not change the return value of handleMouseReleaseEvent? 
> > > > 
> > > > That was on my rader when I wrote this patch, but I gave up changing all at once. That requires a lot of changes. I don't know the historical reason, but dispatchXXXEvent and handleXXXEvent use bool as opposite meaning as far as I remember. This patch attacked only dispatchXXXEvent, without touching handleXXEvent.
> > > 
> > > 
> > > then we should not return !swallowevent in handlemousepressevent. that changes the previous behavior.
> > 
> > I understand that. That might not be my intention. Let me investigate the current code base and codebase when this patch was landed further.
> > 
> > In current code base, the only client which uses the return value of the EventHandler::handleMousePressed(const PlatformMouseEvent&) looks EventHandler::handleGensutureTap().
> > Is that right?
> > 
> > 
> > > 
> > > > 
> > > > I think using 'bool' in both cases as a return value is the root cause of confusion. Actually I tried to introduce enum, but I was not confident whether using enum here is good or not since we have to change a lot of code.
> > > > 
> > > > > 
> > > > > I think we need some documentation in eventhandler. And we definitely need some test here to see what is being changed and whether that is correct.
> > > > 
> > > > Agreed. We have to resolve the current inconsistency usage of bool. That requires further patches.

[Hayato Ito]

Thank you.

I've tried to add a Layout test so we can catch the regresson in the future, but I could not find how to detect it from the LayoutTest layer.

I referred to LayoutTests/fast/events/touch/gesture/gesture-click.html, which hits EventHandler:handleGestureTap(), but I couldn't find any side effects caused by defaultPrevented value which is set in EventHandler:handleGestureTap.

I guess you have some ideas. I appreciate your feedbacks.

(In reply to comment #11)
> Thanks, this line should only affect HandleGestureTap()
> 
> (In reply to comment #9)
> > I investigated. This change of line (!swallowEvent)  in handleMousePressEvent seems unnecessary as you mentioned.
> > The bad part is there isn't any test which catch that.
> > 
> > Let me revert the line in another patch. I'll add a layout test too.
> > 
> > Thank you for reporting. I missed that.
> > 
> > 
> > (In reply to comment #8)
> > > (In reply to comment #7)
> > > > (In reply to comment #6)
> > > > > Thank you for the comment.
> > > > > 
> > > > > (In reply to comment #5)
> > > > > > I am having a hard time understanding this patch and it doesn't look correct to me.
> > > > > > 
> > > > > > 
> > > > > > after this patch, MouseEventDispatchMediator::dispatchEvent will return a true instead of false in case the event was not canceled. I am ok with that, but better add some comments in the header to indicate that.
> > > > > > 
> > > > > > This patch also tries to change the return value of EventHandler::dispatchMouseEvent(). However, this doesn't seem to cover all the paths in which swallowEvent is changed. For example, in some code paths there is swallowEvent = true and that is not changed. However, this patch will return !swallowEvent instead, isn't that changing the behavior of those cases?
> > > > > > 
> > > > > > What confuses me most is handleMousePressEvent(). Why we change the return value to be !swallowEvent?  What should be the return value of handleMousePressEvent()? Can you add a comment in the header? If this is to indicate whether the event was canceled or not, why not change the return value of handleMouseReleaseEvent? 
> > > > > 
> > > > > That was on my rader when I wrote this patch, but I gave up changing all at once. That requires a lot of changes. I don't know the historical reason, but dispatchXXXEvent and handleXXXEvent use bool as opposite meaning as far as I remember. This patch attacked only dispatchXXXEvent, without touching handleXXEvent.
> > > > 
> > > > 
> > > > then we should not return !swallowevent in handlemousepressevent. that changes the previous behavior.
> > > 
> > > I understand that. That might not be my intention. Let me investigate the current code base and codebase when this patch was landed further.
> > > 
> > > In current code base, the only client which uses the return value of the EventHandler::handleMousePressed(const PlatformMouseEvent&) looks EventHandler::handleGensutureTap().
> > > Is that right?
> > > 
> > > 
> > > > 
> > > > > 
> > > > > I think using 'bool' in both cases as a return value is the root cause of confusion. Actually I tried to introduce enum, but I was not confident whether using enum here is good or not since we have to change a lot of code.
> > > > > 
> > > > > > 
> > > > > > I think we need some documentation in eventhandler. And we definitely need some test here to see what is being changed and whether that is correct.
> > > > > 
> > > > > Agreed. We have to resolve the current inconsistency usage of bool. That requires further patches.

[Min Qin]

probably in webkit_unittests should be fine

[Hayato Ito]

Thank you for suggestion. I've never used webkit_unittests.

I'd like to avoid using webkit_unittests here since this is rather a web facing feature and should be tested by LayoutTest for all ports.

Let me file another bug until we can find a way to test that.


(In reply to comment #13)
> probably in webkit_unittests should be fine