Bug 59715 – [GTK] Click counting logic should be shared between WebKit1 and WebKit2

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

[Martin Robinson]

The click counting logic is currently duplicated for both ports. We should push this down into WebCore as a utility class. This has the added benefit of helping put WebKitWebView on a diet.

[Martin Robinson]

Created attachment 91519 [details]
Patch

[WebKit Review Bot]

Attachment 91519 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/gtk/GtkClickCounter.cpp:80:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Xan Lopez]

Comment on attachment 91519 [details]
Patch

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

Great stuff!

> Source/WebCore/platform/gtk/GtkClickCounter.cpp:23
> +#include <gdk/gdk.h>

I suppose this comes for free from gtk.h

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:-339
> -

I actually object to this line being removed! Buurn.

[Martin Robinson]

Committed r85214: <http://trac.webkit.org/changeset/85214>

[WebKit Review Bot]

http://trac.webkit.org/changeset/85214 might have broken GTK Linux 64-bit Debug

[Martin Robinson]

Comment on attachment 91519 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:274
>      WebKitWebViewBasePrivate* priv = webViewBase->priv;
> -
>      priv->page->handleMouseEvent(NativeWebMouseEvent(reinterpret_cast<GdkEvent*>(event), 0 /* currentClickCount */));

This was extra and I can omit it from the patch.

[Carlos Garcia Campos]

Comment on attachment 91519 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:-151
> -    priv->pageClient = PageClientImpl::create(GTK_WIDGET(webkitWebViewBase));

This line should not be removed.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:-268
> -    if (buttonEvent->button == 3) {
> -        // FIXME: [GTK] Add context menu support for Webkit2.
> -        // https://bugs.webkit.org/show_bug.cgi?id=54827
> -        notImplemented();
> -        return FALSE;
> -    }
> -

Why did you remove these lines? we haven't added support for context menus yet.

[Martin Robinson]

Comment on attachment 91519 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:-151
>> -    priv->pageClient = PageClientImpl::create(GTK_WIDGET(webkitWebViewBase));
> 
> This line should not be removed.

Indeed. That was a mistake.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:-268
>> -
> 
> Why did you remove these lines? we haven't added support for context menus yet.

That's correct, but I believe that an early return here prevents detecting double, triple, quadruple, etc third button clicks. If we need to generate context menu events, it should happen later in this method.

[Carlos Garcia Campos]

(In reply to comment #8)
> (From update of attachment 91519 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91519&action=review
> 
> >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:-151
> >> -    priv->pageClient = PageClientImpl::create(GTK_WIDGET(webkitWebViewBase));
> > 
> > This line should not be removed.
> 
> Indeed. That was a mistake.

No problem, I already fixed it.

> >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:-268
> >> -
> > 
> > Why did you remove these lines? we haven't added support for context menus yet.
> 
> That's correct, but I believe that an early return here prevents detecting double, triple, quadruple, etc third button clicks. If we need to generate context menu events, it should happen later in this method.

Patch for bug #54827 would remove those lines anyway.