Bug 137685 – [GTK] Move click counter logic back to WebKitWebViewBase

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

It was moved to a shared class in platform to be used by both WebKit1 and WebKit2, but it's currently only used by WebKitWebViewBase.

[Carlos Garcia Campos]

Created attachment 239791 [details]
Patch

[Carlos Garcia Campos]

Btw, this is also another file we don't build twice because of the plugin process :-)

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Martin Robinson]

Again, I think we should just move this class to WebKit2. It's useful to separate code into classes.

[Carlos Garcia Campos]

(In reply to comment #4)
> Again, I think we should just move this class to WebKit2. It's useful to separate code into classes.

I don't think it's always helpful, in this case we end up with a very small class (another file to build) with a single method that IMO belongs to the view. In the case of the DND helper is even worse, because having half of the code in the web view and the other half in a separate class makes quite difficult to follow the code (and DND logic is already complex enough). It made sense to me when we needed to share the common code, but that's no longer needed.

[Carlos Garcia Campos]

Created attachment 240104 [details]
Rebased patch to apply on current git master

[Carlos Garcia Campos]

Created attachment 243124 [details]
New patch

Added a private struct

[Martin Robinson]

Comment on attachment 243124 [details]
New patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:768
> +    // For double and triple clicks GDK sends both a normal button press event
> +    // and a specific type (like GDK_2BUTTON_PRESS). If we detect a special press
> +    // coming up, ignore this event as it certainly generated the double or triple
> +    // click. The consequence of not eating this event is two DOM button press events
> +    // are generated.
> +    GUniquePtr<GdkEvent> nextEvent(gdk_event_peek());
> +    if (nextEvent && (nextEvent->any.type == GDK_2BUTTON_PRESS || nextEvent->any.type == GDK_3BUTTON_PRESS))
>          return TRUE;

I think I prefer this method as part of the click counter, since understanding types of click events is what it's good at.

[Carlos Garcia Campos]

(In reply to comment #8)
> Comment on attachment 243124 [details]
> New patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243124&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:768
> > +    // For double and triple clicks GDK sends both a normal button press event
> > +    // and a specific type (like GDK_2BUTTON_PRESS). If we detect a special press
> > +    // coming up, ignore this event as it certainly generated the double or triple
> > +    // click. The consequence of not eating this event is two DOM button press events
> > +    // are generated.
> > +    GUniquePtr<GdkEvent> nextEvent(gdk_event_peek());
> > +    if (nextEvent && (nextEvent->any.type == GDK_2BUTTON_PRESS || nextEvent->any.type == GDK_3BUTTON_PRESS))
> >          return TRUE;
> 
> I think I prefer this method as part of the click counter, since
> understanding types of click events is what it's good at.

it doesn't use anything from the click counter

[Martin Robinson]

Comment on attachment 243124 [details]
New patch

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

>>> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:768
>>>          return TRUE;
>> 
>> I think I prefer this method as part of the click counter, since understanding types of click events is what it's good at.
> 
> it doesn't use anything from the click counter

I just meant that it is involved with counting clicks. Perhaps if it had a better name, it would make more sense. I'm not sure.

[Carlos Garcia Campos]

Committed r177148: <http://trac.webkit.org/changeset/177148>