Bug 103058 - It's confusing that return values of 'bool Node::dispatchEvent(...)' and 'bool Node::dispatchMouseEvent(..)' have the opposite meanings.
Summary: It's confusing that return values of 'bool Node::dispatchEvent(...)' and 'boo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hayato Ito
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-22 05:10 PST by Hayato Ito
Modified: 2013-01-29 00:38 PST (History)
3 users (show)

See Also:


Attachments
Make them return the same meaning bool. (10.41 KB, patch)
2012-11-22 05:27 PST, Hayato Ito
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hayato Ito 2012-11-22 05:10:25 PST
Every return values from 'bool dispatchXXXEvent(...)' member functions should have the same meanings to avoid confusions.
Comment 1 Hayato Ito 2012-11-22 05:11:11 PST
I think we should use enum, or struct, as a return value, but that might be a goal in the long term.
Comment 2 Hayato Ito 2012-11-22 05:27:31 PST
Created attachment 175653 [details]
Make them return the same meaning bool.
Comment 3 WebKit Review Bot 2012-11-24 07:12:42 PST
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>
Comment 4 WebKit Review Bot 2012-11-24 07:12:45 PST
All reviewed patches have been landed.  Closing bug.
Comment 5 Min Qin 2013-01-27 13:18:13 PST
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.
Comment 6 Hayato Ito 2013-01-27 18:22:19 PST
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.
Comment 7 Min Qin 2013-01-27 18:46:49 PST
(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.
Comment 8 Hayato Ito 2013-01-27 19:27:19 PST
(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.
Comment 9 Hayato Ito 2013-01-27 21:27:32 PST
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.
Comment 10 Hayato Ito 2013-01-27 21:32:10 PST
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.
Comment 11 Min Qin 2013-01-27 21:56:42 PST
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.
Comment 12 Hayato Ito 2013-01-27 23:41:25 PST
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.
Comment 13 Min Qin 2013-01-28 07:27:14 PST
probably in webkit_unittests should be fine
Comment 14 Hayato Ito 2013-01-29 00:38:02 PST
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