Bug 58921 – [chromium] linux chromium doesn't set click count for mouse up events in WebInputEventFactory.cpp.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

WebKit/chromium/src/gtk/WebInputEventFactory.cpp doesn't set clickCount for mouse up events.

We probably want to have similar behavior as the win implementation.

I will upload a patch soon.

Created attachment 90248 [details]
Set click count for mouse up events, using similar algorithm as win implementation.

The reason why we need such a fix is that the Pepper API passes this information to plugins, and some consumers (e.g., Flash) need it to detect double-clicks.

[Tony Chang]

Comment on attachment 90248 [details]
Set click count for mouse up events, using similar algorithm as win implementation.

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

> Source/WebKit/chromium/ChangeLog:6
> +        [chromium] linux chromium doesn't set click count for mouse up events.
> +        https://bugs.webkit.org/show_bug.cgi?id=58921

Please explain what this is breaking (ppapi, flash, whatever).

> Source/WebKit/chromium/src/gtk/WebInputEventFactory.cpp:492
> +    if (shouldForgetPreviousClick(event->window, event->time, event->x, event->y))
> +        resetClickCountState();

Why do we need to reset here?

Created attachment 90256 [details]
Revise the ChangeLog

Comment on attachment 90248 [details]
Set click count for mouse up events, using similar algorithm as win implementation.

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

>> Source/WebKit/chromium/ChangeLog:6
>> +        https://bugs.webkit.org/show_bug.cgi?id=58921
> 
> Please explain what this is breaking (ppapi, flash, whatever).

Done.

>> Source/WebKit/chromium/src/gtk/WebInputEventFactory.cpp:492
>> +        resetClickCountState();
> 
> Why do we need to reset here?

I am not sure whether this is very reasonable, but I tried to follow the behavior of the win implementation.

[Tony Chang]

Comment on attachment 90256 [details]
Revise the ChangeLog

I think this patch is correct, but it would be nice to add a layout test for this.  The benefit of a layout test is that it would ensure that we get consistent results across platforms.  Specifically, you can use window.eventSender.leapForward and mouseDown/Up to simulate mouse events.  You might need to add eventSender.clickCount() for this.  This could be a follow up patch.

[Tony Chang]

(+avi since I think he touched this code a bunch in the past)

Hi, Tony.

Thanks for your review!
I will work on a layout test in a follow up patch.

Do you mean I should wait for avi's feedback before committing the patch?

[Tony Chang]

(In reply to comment #9)
> Do you mean I should wait for avi's feedback before committing the patch?

I think it's fine to commit now.  We can revert if avi has additional feedback.

Please make sure the layout tests pass before committing.

Thanks!

[Avi Drissman]

(In reply to comment #9)
> Do you mean I should wait for avi's feedback before committing the patch?

The change looks good to me.

Hi, Tony.

Would you please help to get this patch committed? Thanks!

[WebKit Commit Bot]

Comment on attachment 90256 [details]
Revise the ChangeLog

Clearing flags on attachment: 90256

Committed r84427: <http://trac.webkit.org/changeset/84427>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.