Bug 61734 – [GTK] Support smart replace for the pasteboard

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

[Martin Robinson]

There are some pasteboard tests that fail because GTK+ does not have a hook for enabling and disabling smart replace.

[Martin Robinson]

Created attachment 95496 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 95496 [details]
Patch

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

toggleSmartInsertDelete seems odd to me but maybe it's gtk-style.

> Source/WebKit/gtk/WebCoreSupport/DragClientGtk.cpp:106
> -    GRefPtr<GtkTargetList> targetList(PasteboardHelper::defaultPasteboardHelper()->targetListForDataObject(dataObject.get()));
> +    GRefPtr<GtkTargetList> targetList(PasteboardHelper::defaultPasteboardHelper()->targetListForDataObject(dataObject.get(), false));

Can we instead set the default value be false?  This is a classic case of enum being superior to bool.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:617
> +    if (client->smartInsertDeleteEnabled() != enabled)
> +        client->toggleSmartInsertDelete();

If we had setSmartInsertDeleteEnabled, then we shouldn't need to have an if statement here.

> Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:429
> +void EditorClient::toggleSmartInsertDelete()
> +{
> +    m_smartInsertDeleteEnabled = !m_smartInsertDeleteEnabled;
> +}

We normally add setSmartInsertDeleteEnabled(bool).

[Martin Robinson]

Created attachment 95923 [details]
Patch

[Martin Robinson]

(In reply to comment #2)
> (From update of attachment 95496 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95496&action=review

> toggleSmartInsertDelete seems odd to me but maybe it's gtk-style.

Nope, this was just copied from the Mac port. I've converted it to setSmartInsertDeleteEanbled(bool);

> > Source/WebKit/gtk/WebCoreSupport/DragClientGtk.cpp:106
> > -    GRefPtr<GtkTargetList> targetList(PasteboardHelper::defaultPasteboardHelper()->targetListForDataObject(dataObject.get()));
> > +    GRefPtr<GtkTargetList> targetList(PasteboardHelper::defaultPasteboardHelper()->targetListForDataObject(dataObject.get(), false));
> 
> Can we instead set the default value be false?  This is a classic case of enum being superior to bool.

I've changed the two methods in this class to take an enum. I've uploaded a new patch, in case you had any comments on the naming. If you have no objections, I'll land the new version.

> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:617
> > +    if (client->smartInsertDeleteEnabled() != enabled)
> > +        client->toggleSmartInsertDelete();
> 
> If we had setSmartInsertDeleteEnabled, then we shouldn't need to have an if statement here.

We have setSmartInsertDeleteEnabled now so I've corrected this.

> > Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:429
> > +void EditorClient::toggleSmartInsertDelete()
> > +{
> > +    m_smartInsertDeleteEnabled = !m_smartInsertDeleteEnabled;
> > +}
> We normally add setSmartInsertDeleteEnabled(bool).

Fixed.

[Ryosuke Niwa]

Comment on attachment 95923 [details]
Patch

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

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:65
> +                                                                     PasteboardHelper::DoNotIncludeSmartPaste);

I don't think we indent deep like this.  We normally do:
helper->writeClipboardContents(clipboard, canSmartCopyOrDelete ? PasteboardHelper::IncludeSmartPaste :
    PasteboardHelper::DoNotIncludeSmartPaste);

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:616
> +    WebKit::EditorClient* client = static_cast<WebKit::EditorClient*>(core(webView)->editorClient());
> +    client->setSmartInsertDeleteEnabled(enabled);

Can webView or client ever be null?

[Martin Robinson]

Comment on attachment 95923 [details]
Patch

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

>> Source/WebCore/platform/gtk/PasteboardGtk.cpp:65
>> +                                                                     PasteboardHelper::DoNotIncludeSmartPaste);
> 
> I don't think we indent deep like this.  We normally do:
> helper->writeClipboardContents(clipboard, canSmartCopyOrDelete ? PasteboardHelper::IncludeSmartPaste :
>     PasteboardHelper::DoNotIncludeSmartPaste);

In WebKitGTK+ we typically line them up instead of using a 4-space indent. If it's okay with you, I'll just make this one line.

>> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:616
>> +    client->setSmartInsertDeleteEnabled(enabled);
> 
> Can webView or client ever be null?

The WebView could potentially be null if DumpRenderTree calls this incorrectly. I'll add:

g_return_if_fail(webView);

[Martin Robinson]

Committed r88066: <http://trac.webkit.org/changeset/88066>