Summary: | [GTK] Support smart replace for the pasteboard | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Martin Robinson <mrobinson> | ||||||
Component: | WebKitGTK | Assignee: | Martin Robinson <mrobinson> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | gustavo, rniwa, xan.lopez | ||||||
Priority: | P3 | Keywords: | Gtk | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | Linux | ||||||||
Bug Depends on: | 61690 | ||||||||
Bug Blocks: | 61661 | ||||||||
Attachments: |
|
Description
Martin Robinson
2011-05-30 10:12:17 PDT
Created attachment 95496 [details]
Patch
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). Created attachment 95923 [details]
Patch
(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. 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? 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); Committed r88066: <http://trac.webkit.org/changeset/88066> |