Bug 137685 - [GTK] Move click counter logic back to WebKitWebViewBase
Summary: [GTK] Move click counter logic back to WebKitWebViewBase
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2014-10-14 03:52 PDT by Carlos Garcia Campos
Modified: 2014-12-11 09:07 PST (History)
10 users (show)

See Also:


Attachments
Patch (13.49 KB, patch)
2014-10-14 03:54 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Rebased patch to apply on current git master (13.00 KB, patch)
2014-10-20 03:26 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
New patch (12.35 KB, patch)
2014-12-11 08:39 PST, Carlos Garcia Campos
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2014-10-14 03:52:12 PDT
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.
Comment 1 Carlos Garcia Campos 2014-10-14 03:54:54 PDT
Created attachment 239791 [details]
Patch
Comment 2 Carlos Garcia Campos 2014-10-14 03:55:49 PDT
Btw, this is also another file we don't build twice because of the plugin process :-)
Comment 3 WebKit Commit Bot 2014-10-14 03:57:05 PDT
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
Comment 4 Martin Robinson 2014-10-14 09:03:33 PDT
Again, I think we should just move this class to WebKit2. It's useful to separate code into classes.
Comment 5 Carlos Garcia Campos 2014-10-14 23:23:23 PDT
(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.
Comment 6 Carlos Garcia Campos 2014-10-20 03:26:43 PDT
Created attachment 240104 [details]
Rebased patch to apply on current git master
Comment 7 Carlos Garcia Campos 2014-12-11 08:39:46 PST
Created attachment 243124 [details]
New patch

Added a private struct
Comment 8 Martin Robinson 2014-12-11 08:42:36 PST
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.
Comment 9 Carlos Garcia Campos 2014-12-11 08:51:48 PST
(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
Comment 10 Martin Robinson 2014-12-11 08:57:12 PST
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.
Comment 11 Carlos Garcia Campos 2014-12-11 09:07:10 PST
Committed r177148: <http://trac.webkit.org/changeset/177148>