Bug 146574 – [GTK][Wayland] Implement clipboard support

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

[nick]

copy/paste address bar from/to  another application works.

copy/paste page contents from/to  another application don't works.

copy/paste on same tab works.

copy/paste between tabs don't works.

there is no log output.

webkitgtk-2.8.3  gtk-3.16.4 wayland-1.8.1 weston-1.8.0

wayland has protokol level clipboard and drag&drop support.

thanks.

[nick]

forgot : epiphany-3.16.2

[Michael Catanzaro]

(In reply to comment #0)
> copy/paste address bar from/to  another application works.
> 
> copy/paste page contents from/to  another application don't works.

Hm, webkitgtk-2.8.3 indicates the web process is running under XWayland, and gtk-3.16.4 indicates you probably have mutter 3.16.x, which I don't believe knew how to copy/paste between X and Wayland applications. We need to find out if this is still a problem with 2.9.4 compiled with ENABLE_WAYLAND_TARGET=ON (the new default setting) and running under the latest mutter 3.17.x.

[nick]

Great! i compiled webkitgtk-2.9.4 successfully into my libX11'less partition.

i tested it only on weston(drm-backend) via MiniBrowser.

So there aren't xwayland and mutter here.

copy/paste between apps still doesn't work.

copy/paste between page and address bar didn't work too.

copy/paste in page scope works.

[nick]

just an additional info: i noticed that MiniBrowser and weston-terminal(a native 

weston app) maintains two different clipboard when they running.

copy/paste between two weston-terminal instances works.

But copy/paste between two MiniBrowser instances doesn't work.

[nick]

a webkitsetting exists in MiniBrowser's settings menu.

it is named as "JavaScript can access clipboard"

[nick]

page context(so webkit) doesn't respect 'global clipboard object'; creates and

maintains its own.But 'address bar'(so gtk) know and respect it.

[Michael Catanzaro]

I'll look into this in 3-4 weeks, unless Zan beats me to it. I have to implement Wayland copy/paste for Chromium anyway. I'd prefer to do Chromium first, but I have no deadline there, while this bug is starting to look relatively urgent....

[Michael Gratton]

*** Bug 150316 has been marked as a duplicate of this bug. ***

[Michael Catanzaro]

(In reply to comment #7)
> I'll look into this in 3-4 weeks, unless Zan beats me to it. I have to
> implement Wayland copy/paste for Chromium anyway. I'd prefer to do Chromium
> first, but I have no deadline there, while this bug is starting to look
> relatively urgent....

FWIW at the time we were planning to ship Fedora 23 with Wayland instead of X11, but that transition got delayed until Fedora 24.

[Bastien Nocera]

This is a problem for me using epiphany under Wayland.

[Claudio Saavedra]

I implemented this for the WPE backend. If someone is in a hurry, feel free to use the code in https://github.com/WebKitForWayland/webkit/pull/14 . Otherwise I might be able to look into this at some point, no promises though :)

[Michael Catanzaro]

*** Bug 156497 has been marked as a duplicate of this bug. ***

[Michael Catanzaro]

By the way, some background on this: with help from Carlos Garnacho, we determined it's impossible to access the clipboard from the web process in Wayland, because the web process doesn't have a mutter window. Instead we decided to implement a clipboard proxy to the UI process, like Apple already has for its ports, and like what Claudio recently did for WPE. We can probably reuse most of Claudio's code, except ours of course needs to properly support the same set of MIME types that we currently do.

[Torben Andresen]

Ist there any progress on this? It's really annoying that i have to use a second browser for C&P.

[Michael Catanzaro]

(In reply to comment #14)
> Ist there any progress on this?

Nope.

> It's really annoying that i have to use a
> second browser for C&P.

Yup.

[Michael Catanzaro]

(In reply to comment #14)
> Ist there any progress on this?

I've been investigating and have some code written, but I need more time. No estimate as to when this might be finished.

[Michael Catanzaro]

(In reply to comment #16)
> I've been investigating and have some code written, but I need more time. No
> estimate as to when this might be finished.

In case anyone else wants to look into this, the information flow needs to be:

Pasteboard (WebCore, web process) -> PasteboardStrategy/WebPlatformStrategies (WebCore/WebKit2, web process) -> WebPasteboardProxy (WebKit2 web process) -> PlatformPasteboard (WebCore UI process)

Most of the code currently in the Pasteboard or PasteboardHelper classes needs to move to PlatformPasteboard (in the UI process). GtkClipboard must not be accessed anywhere except PlatformPasteboard. There is example code for a different WebKit port in comment #11.

[Carlos Garnacho]

Created attachment 288269 [details]
Patch, implement clipboard as a PlatformPasteboard

Initial patch to get things rolling. It implements PlatformPasteboard for GTK+, and adds the necessary plumbing so Pasteboard in the webprocess side forwards clipboard contents/requests to the UI process. It does so by reusing as much of the previous code as it was possible, more concretely DataObjectGtk is used on both sides to keep a local representation of the clibpoard contents, and PasteboardHelper usage has simply been shuffled from PasteboardGtk.cpp to PlatformPasteboardGtk.cpp.

[Michael Catanzaro]

Comment on attachment 288269 [details]
Patch, implement clipboard as a PlatformPasteboard

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

Thanks very much.

Since you're now our expert on this code, any thoughts on lightly-related bug #144547 would be appreciated.

> Source/WebCore/ChangeLog:3
> +        [GTK] Manage selections through PlatformPasteboard

The first line should be the title of the bug.

If you want to rename the bug, then make sure to mention that this implements clipboard support for Wayland somewhere in the changelog.

> Source/WebCore/platform/Pasteboard.h:145
> +    enum PasteboardType { None, Primary, Selection };

Hm, this seems like very confusing terminology. In GTK+ parlance the default clipboard is CLIPBOARD and the primary selection clipboard is PRIMARY. At first I thought your Primary was GTK+'s CLIPBOARD, and your Selection was GTK+'s PRIMARY, but I see you are using Primary as expected and Selection to mean CLIPBOARD. It's really confusing because primary is of course short for "primary selection," so when we talk about the selection clipboard we're normally talking about primary, right? I would stick to GTK+ terminology here and rename Selection to Clipboard. Yeah it's a bit confusing for Clipboard to be a PasteboardType, but I don't see any way around it.

Also, use enum class rather than old style enums, so it gets scoped nicely.

> Source/WebCore/platform/Pasteboard.h:229
> +    String pasteboardName(void);

This function should be const, so that it can be called from other const member functions (and to ensure it can't modify member variables). Also, unlike in C, in C++ functions with no parameters declared are always void by default, so we don't include that. So the declaration should be:

String pasteboardName() const;

> Source/WebCore/platform/Pasteboard.h:230
> +    void writeToClipboard(bool includeSmartPaste = false);

We don't like to use boolean parameters since it's harder to read; instead, you can do something like this:

enum class ShouldIncludeSmartPaste { No, Yes };

void writeToClipboard(ShouldIncludeSmartPaste = ShouldIncludeSmartPaste::No);

Then instead of calling writeToClipboard(true), you call writeToClipboard(ShouldIncludeSmartPaste::Yes), which is easier to read. Also consider whether having the default argument is really worth it; I'd probably just pass the tag everywhere to be explicit about it.

> Source/WebCore/platform/Pasteboard.h:231
> +    void readFromClipboard(void);

void readFromClipboard();

> Source/WebCore/platform/PasteboardStrategy.h:77
> +#if PLATFORM(GTK)
> +    virtual void clearSelection(String selection);
> +    virtual void claimSelection(String selection);
> +    virtual Vector<String> getTypes(String selection);
> +    virtual void setString(String selection, String pasteboardType, String buffer);
> +    virtual String getString(String selection, String pasteboardType);
> +#endif // PLATFORM(GTK)

As discussed on IRC, these need to be pure virtual. I'm not sure how you were able to build it like this.

I think using "selection" to refer to clipboard names is perhaps confusing. What do think about using "pasteboardName" instead of "selection"?

Lastly, I think "buffer" is a confusing name for a String object, since actually the buffer is encapsulated by the String. What do you think about "pasteboardContents" instead of "buffer"?

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:43
> +    return std::make_unique<Pasteboard>(Selection);

(When you convert this to an enum class, you'll have to write PasteboardType::Selection and PasteboardType::Primary instead of Selection and Primary on their own.)

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:87
>      ASSERT(m_dataObject);

You can get rid of this assert. It's safe to assume a create function only fails if you pass an invalid parameter, and it doesn't take a parameter anymore.

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:122
> +String Pasteboard::pasteboardName(void)

String Pasteboard::pasteboardName() const

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:128
> +    return emptyString();

Our Strings actually have a separate null state. I think we would normally return a default-constructed String here:

return String {};

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:136
> +    String selection = pasteboardName();
> +
> +    ASSERT(!selection.isEmpty());

I would remove the blank line here, since the assert goes with the declaration above it.

Perhaps "pasteboardName" would be a better name than "selection" for this variable.

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:170
> +    for (auto it : types) {

Careful, you're copying into "it" here. You want auto& it

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:306
>  bool Pasteboard::canSmartReplace()

This function should be const. Since you're modifying it anyway, go ahead and fix it:

bool Pasteboard::canSmartReplace() const

> Source/WebKit2/UIProcess/gtk/WebPasteboardProxyGtk.cpp:2
> + * Copyright (C) 2016 Red Hat.

Don't you normally write Red Hat Inc?

> Source/WebKit2/UIProcess/gtk/WebPasteboardProxyGtk.cpp:39
> +    static HashMap<String, std::unique_ptr<WebCore::PlatformPasteboard> > pasteboardMap;

Since you have a "using namespace WebCore" statement up above, you don't need this extra scoping here. Also, you don't need the extra space between the > > anymore, that's been valid since C++11 and we're using C++14 now. Also, we want to avoid static objects being destroyed at exit time since that tends to cause bugs, so please leak it with NeverDestroyed (you'll need to #include <wtf/NeverDestroyed.h>. It will look like this:

static NeverDestroyed<HashMap<String, std::unique_ptr<PlatformPasteboard>>> pasteboardMap;

[Michael Catanzaro]

Hm, I now see these warnings when closing an Epiphany tab:

pure virtual method called
terminate called without an active exception

That looks pretty bad; I've never seen errors like these before. I wonder if we're slicing off the derived class portion of an object somewhere.

[Michael Catanzaro]

Comment on attachment 288269 [details]
Patch, implement clipboard as a PlatformPasteboard

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

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:147
> +    if (m_dataObject->hasText())
> +        strategy.setString(selection, "Text", m_dataObject->text());
> +    if (m_dataObject->hasMarkup())
> +        strategy.setString(selection, "text/html", m_dataObject->markup());
> +    if (m_dataObject->hasURL())
> +        strategy.setString(selection, "URL", m_dataObject->url());
> +    if (m_dataObject->hasURIList())
> +        strategy.setString(selection, "Files", m_dataObject->uriList());

I agree with you that it'd be better to use enums rather than strings for the second parameter here. Carlos Garcia gave you a tip on IRC on how to do this, was it enough?

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:168
> +    Vector<String> types;
> +    types = strategy.getTypes(selection);

This should be done in one line.

> Source/WebCore/platform/gtk/PlatformPasteboardGtk.cpp:39
> +    : m_clipboard(gtk_clipboard_get(gdk_atom_intern(pasteboardName.utf8().data(), true)))

Nit: we normally use gboolean TRUE when calling GObject functions.

[Michael Catanzaro]

Comment on attachment 288269 [details]
Patch, implement clipboard as a PlatformPasteboard

(r- until we figure out what's causing "pure virtual method called")

[Michael Catanzaro]

(In reply to comment #20)
> Hm, I now see these warnings when closing an Epiphany tab:
> 
> pure virtual method called
> terminate called without an active exception
> 
> That looks pretty bad; I've never seen errors like these before. I wonder if
> we're slicing off the derived class portion of an object somewhere.

I rebooted my computer and the errors are gone. Very strange....

[Michael Catanzaro]

(In reply to comment #23)
> I rebooted my computer and the errors are gone. Very strange....

I'm not confident these were related to your patch. Let's assume otherwise. We can debug when they appear again.

[Carlos Garcia Campos]

Comment on attachment 288269 [details]
Patch, implement clipboard as a PlatformPasteboard

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

What about WebEditorClient::updateGlobalSelection()? it's currently protected by X11 guards, could we remove those guards now?

>> Source/WebCore/platform/Pasteboard.h:145
>> +    enum PasteboardType { None, Primary, Selection };
> 
> Hm, this seems like very confusing terminology. In GTK+ parlance the default clipboard is CLIPBOARD and the primary selection clipboard is PRIMARY. At first I thought your Primary was GTK+'s CLIPBOARD, and your Selection was GTK+'s PRIMARY, but I see you are using Primary as expected and Selection to mean CLIPBOARD. It's really confusing because primary is of course short for "primary selection," so when we talk about the selection clipboard we're normally talking about primary, right? I would stick to GTK+ terminology here and rename Selection to Clipboard. Yeah it's a bit confusing for Clipboard to be a PasteboardType, but I don't see any way around it.
> 
> Also, use enum class rather than old style enums, so it gets scoped nicely.

Yes, also PasteboardType is redundant inside a PasteboardClass and even more if we use an emu class, so use just Type.

>> Source/WebCore/platform/Pasteboard.h:229
>> +    String pasteboardName(void);
> 
> This function should be const, so that it can be called from other const member functions (and to ensure it can't modify member variables). Also, unlike in C, in C++ functions with no parameters declared are always void by default, so we don't include that. So the declaration should be:
> 
> String pasteboardName() const;

No, it should be:

const String& name() const;

again pasteboardName sounds redundant and it should return a const reference.

>> Source/WebCore/platform/Pasteboard.h:230
>> +    void writeToClipboard(bool includeSmartPaste = false);
> 
> We don't like to use boolean parameters since it's harder to read; instead, you can do something like this:
> 
> enum class ShouldIncludeSmartPaste { No, Yes };
> 
> void writeToClipboard(ShouldIncludeSmartPaste = ShouldIncludeSmartPaste::No);
> 
> Then instead of calling writeToClipboard(true), you call writeToClipboard(ShouldIncludeSmartPaste::Yes), which is easier to read. Also consider whether having the default argument is really worth it; I'd probably just pass the tag everywhere to be explicit about it.

We already have an enum SmartPasteInclusion in PasteboardHelper

> Source/WebCore/platform/PasteboardStrategy.h:74
> +    virtual Vector<String> getTypes(String selection);

This should return the vector as an out parameter. Or leave it this way but rename it as types()

>> Source/WebCore/platform/PasteboardStrategy.h:77
>> +#endif // PLATFORM(GTK)
> 
> As discussed on IRC, these need to be pure virtual. I'm not sure how you were able to build it like this.
> 
> I think using "selection" to refer to clipboard names is perhaps confusing. What do think about using "pasteboardName" instead of "selection"?
> 
> Lastly, I think "buffer" is a confusing name for a String object, since actually the buffer is encapsulated by the String. What do you think about "pasteboardContents" instead of "buffer"?

Yes, they should be pure virtual, and all string parameters should be const String&. The pasteboardType parameter is also confusing, we have PasteboardType enum referring to the clipboard name, but here it refers to the content types. I wonder if we could use DataObjectGtk directly, making it serializable.

> Source/WebCore/platform/PlatformPasteboard.h:86
> +    WEBCORE_EXPORT void clearSelection();
> +    WEBCORE_EXPORT void claimSelection();

I'm not sure I understand these. Claim is what writes, and clear is donde before, why do we need the clear?

>> Source/WebCore/platform/gtk/PasteboardGtk.cpp:122
>> +String Pasteboard::pasteboardName(void)
> 
> String Pasteboard::pasteboardName() const

const String& Pasteboard::name() const

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:127
> +    if (m_pasteboardType == Primary)
> +        return "PRIMARY";
> +    if (m_pasteboardType == Selection)
> +        return "CLIPBOARD";

To be able to return a const String& you could define PRIMARY and CLIPBOARD as:

static NeverDestroyed<const String> primaryName(ASCIILiteral("PRIMARY"));
static NeverDestroyed<const String> clipboardName(ASCIILiteral("CLIPBOARD"));

Then use a switch instead of ifs to handle enum cases

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:134
> +    String selection = pasteboardName();

and here you could use const String& selection = name();

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:157
> +    strategy.claimSelection(selection);

So, to write something to the clipboard we need, one IPC message to clear the clipboard, one or more to set data contents, and one to claim the selection. If we serialize DataObjectGtk, this could be just one message:

WriteToClipboard(WebCore::DataObjectGtk);

no?

>> Source/WebCore/platform/gtk/PasteboardGtk.cpp:170
>> +    for (auto it : types) {
> 
> Careful, you're copying into "it" here. You want auto& it

Use a name for the iterator, since in this case it's not an iterator what you get, but the actual value.

for (auto& type : types)

for example

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:175
> +    }

Reading is even worse, because GetString and GetTypes messages are synchronous. So, again we could have s single message that returns a DataObjectGtk

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:205
> -    if (m_gtkClipboard)
> -        PasteboardHelper::singleton().writeClipboardContents(m_gtkClipboard, (smartReplaceOption == CanSmartReplace) ? PasteboardHelper::IncludeSmartPaste : PasteboardHelper::DoNotIncludeSmartPaste);
> +    if (m_pasteboardType != None)
> +        writeToClipboard(smartReplaceOption == CanSmartReplace);

It's a good time to change this and do the check in writeToClipboard, returning early for None type.

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:314
> +    types = strategy.getTypes(pasteboardName());

Now that I see this again, the name() method is a private function, and the value is known at construction time. The type is only used to check whether it's none or not and to return the name, whcih is always the same for the same instance. So, what about having m_name only? Then we only need to check isEmpty() or isNull() instead of type == None, and use m_name everywhere instead of name();

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:329
> -    if (m_gtkClipboard)
> -        PasteboardHelper::singleton().getClipboardContents(m_gtkClipboard);
> +    if (m_pasteboardType != None)
> +        readFromClipboard();

Same her,e let's move the check to readFromClipboard()

> Source/WebCore/platform/gtk/PlatformPasteboardGtk.cpp:53
> +void PlatformPasteboard::claimSelection()
> +{
> +    PasteboardHelper::singleton().writeClipboardContents(m_clipboard);
> +}

I think it's clearer if this method is called writeClipboardContents instead of claim. As I suggested before, if we receive a DataObjectGtk here, we could only need to set the DataObjectGtk of this clipboard from the given one.

>> Source/WebKit2/UIProcess/gtk/WebPasteboardProxyGtk.cpp:39
>> +    static HashMap<String, std::unique_ptr<WebCore::PlatformPasteboard> > pasteboardMap;
> 
> Since you have a "using namespace WebCore" statement up above, you don't need this extra scoping here. Also, you don't need the extra space between the > > anymore, that's been valid since C++11 and we're using C++14 now. Also, we want to avoid static objects being destroyed at exit time since that tends to cause bugs, so please leak it with NeverDestroyed (you'll need to #include <wtf/NeverDestroyed.h>. It will look like this:
> 
> static NeverDestroyed<HashMap<String, std::unique_ptr<PlatformPasteboard>>> pasteboardMap;

Do we even want this? This is already done by GTK+. Here we heap allocate a PlatformPasteboard which only state is a GtkClipboard, obtained with gtk_clipboard_get(). That returns a permanent object owned by GTK+. So, we could just create a stack allocated PlatformPasteboard object to use the clipboard for the given name, since this table is already maintained by GTK+.

> Source/WebKit2/UIProcess/gtk/WebPasteboardProxyGtk.cpp:51
> +    PlatformPasteboard* pasteboard = lookupPlatformPasteboard(selection);
> +    pasteboard->clearSelection();

Then this would be just:

PlatformPasteboard(selection).clearSelection();

[Carlos Garcia Campos]

DataObjectGtk is already serializable, we send it over IPC as part of DragData. We just need to add ArgumentCoder<DataObjectGTK>::encode/decode methdos

[Michael Catanzaro]

(In reply to comment #25)
> > Source/WebCore/platform/PasteboardStrategy.h:74
> > +    virtual Vector<String> getTypes(String selection);
> 
> This should return the vector as an out parameter. Or leave it this way but
> rename it as types()

Leave it this way but rename to types(). Using an out parameter was a pre-C++1 workaround for the lack of language guarantee that the return value would be moved rather than copied.

[Michael Catanzaro]

(In reply to comment #22)
> Comment on attachment 288269 [details]
> Patch, implement clipboard as a PlatformPasteboard
> 
> (r- until we figure out what's causing "pure virtual method called")

It's related to bug #161605, not your patch.

[Jana Saout]

I'm getting complaints at link time that the virtual table of WebCore::PasteboardStrategy cannot be found.

The five added method methods in Source/WebCore/platform/PasteboardStrategy.h should be declared with '= 0' to fix this since it's only an interface.

[Carlos Garnacho]

Created attachment 288563 [details]
Patch, implement clipboard as a PlatformPasteboard

This patch applies the suggestions from Michael Catanzaro, and the suggestions from Carlos García on IRC to allow serialization of DataObjectGtk, thus simplifying the IPC into two setSelection/getSelection requests, this makes the IPC parts much easier to follow.

[Carlos Garnacho]

Created attachment 288564 [details]
Patch, implement clipboard as a PlatformPasteboard

This patch applies most of the remaining suggestions (except the ones rendered obsolete by the switch to serializing DataObjectGtk) in comment #25, which I didn't notice.

[Carlos Garnacho]

(In reply to comment #25)
> Comment on attachment 288269 [details]
> Patch, implement clipboard as a PlatformPasteboard
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288269&action=review
> 
> What about WebEditorClient::updateGlobalSelection()? it's currently
> protected by X11 guards, could we remove those guards now?

I need to investigate that. Having an implementation of primary selection is dependent on the compositor, and I'm not sure how well gtk+ will fare with gtk_clipboard_wait_for_contents() on it if the compositor doesn't offer an implementation. An option might be teaching PasteboardHelper to do gtk_clipboard_request* so stalls are not possible.

With gnome-shell/mutter it should just work though.

> > We don't like to use boolean parameters since it's harder to read; instead, you can do something like this:
> > 
> > enum class ShouldIncludeSmartPaste { No, Yes };
> > 
> > void writeToClipboard(ShouldIncludeSmartPaste = ShouldIncludeSmartPaste::No);
> > 
> > Then instead of calling writeToClipboard(true), you call writeToClipboard(ShouldIncludeSmartPaste::Yes), which is easier to read. Also consider whether having the default argument is really worth it; I'd probably just pass the tag everywhere to be explicit about it.
> 
> We already have an enum SmartPasteInclusion in PasteboardHelper

I think this is the only unaddressed comment. IMHO it's PasteboardHelper's enum which should change to using the one defined here, since this one is exposed in shared layers. If you think this modification belongs to the patch at hand I'll do that.

[Carlos Garcia Campos]

Comment on attachment 288564 [details]
Patch, implement clipboard as a PlatformPasteboard

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

This looks much better now, I still have a few comments, tough.

> Source/WebCore/platform/Pasteboard.h:235
> +    Type m_pasteboardType;
> +    const String m_name;

I don't think we need to keep both type and name. If we keep the type, then we could simply use the name() method, instead of keeping this member. What I meant in my previous revision is that we can only keep m_name only, initialized on construction, because m_type is only used to get the name.

> Source/WebCore/platform/PasteboardStrategy.h:42
> +#if PLATFORM(GTK)
> +class DataObjectGtk;
> +#endif

Leave an empty line between non-platform specific declarations and this.

> Source/WebCore/platform/PasteboardStrategy.h:76
> +    virtual void setSelection(const String& pasteboardName, WebCore::DataObjectGtk*) = 0;
> +    virtual RefPtr<WebCore::DataObjectGtk> getSelection(const String& pasteboardName) = 0;

I prefer to follow the pasteboard names, writeToClipboard/ReadFromClipboard. No need for WebCore::, since we already inside WebCore namespace.

> Source/WebCore/platform/PlatformPasteboard.h:46
> +#if PLATFORM(GTK)
> +class DataObjectGtk;
> +#endif

Move this below the declarations

> Source/WebCore/platform/PlatformPasteboard.h:89
> +    WEBCORE_EXPORT DataObjectGtk* getSelection();

This should never return nullptr either, so return a reference instead of a pointer.

> Source/WebCore/platform/gtk/DataObjectGtk.cpp:164
> +    auto addResult = objectMap.add(clipboard, nullptr);
> +    addResult.iterator->value = dataObject;

Since we are going to replace the DataObjectGtk associated to the clipboard, I think we can stop doing this. We should probably pass a DataObjectGTK to the PasteboardHelper write method, and read method should return a new DataObjectGtk too.

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:87
> +    , m_pasteboardType(type)
> +    , m_name(name())

So, here we could initialize the name using a helper function that can be static instead of name()

m_name(pateboardNameForType(type))

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:123
> +const String& Pasteboard::name() const

And then this wouldn't return a const String& anymore, so we could return directly ASCIILiteral("PRIMARY"), ASCIILiteral("CLIPBOARD") or String(). Now it's better to use a null string, so that it matches the constructor not taking a type.

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:140
> +    if (m_name.isEmpty())

here we would check isNull instead

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:143
> +    DataObjectGtk* dataObject = m_dataObject.get();

We don't need a local variable for this, you can use m_dataObject-> directly

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:147
> +    strategy.setSelection(m_name, dataObject);

strategy is used only once here, so we don't need a local variable for that either:

platformStrategies()->pasteboardStrategy()->setSelection(m_name, m_dataObject.get());

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:152
> +    if (m_name.isEmpty())

and here isNull() too.

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:156
> +    RefPtr<DataObjectGtk> dataObject = strategy.getSelection(m_name);
> +    m_dataObject = dataObject.get();

m_dataObject = WTFMove(platformStrategies()->pasteboardStrategy()->getSelection(m_name));

> Source/WebCore/platform/gtk/PlatformPasteboardGtk.cpp:37
> +void PlatformPasteboard::setSelection(DataObjectGtk* dataObject)

This should receive a reference instead of a pointer

> Source/WebCore/platform/gtk/PlatformPasteboardGtk.cpp:40
> +    DataObjectGtk::setForClipboard(m_clipboard, dataObject);
> +    PasteboardHelper::singleton().writeClipboardContents(m_clipboard);

And I think we could simply pass the dataObject to PasteboardHelper::writeClipboardContents() so that we don't need the clipboard/dataobjects map.

> Source/WebCore/platform/gtk/PlatformPasteboardGtk.cpp:46
> +    PasteboardHelper::singleton().getClipboardContents(m_clipboard);
> +    return DataObjectGtk::forClipboard(m_clipboard);

And PasteboardHelper::getClipboardContents(9), that should probably be renamed as readClipboardContents(), should return the newly created DataObjectGtk

> Source/WebKit2/Shared/gtk/PasteboardContentGtk.cpp:20
> +#include "PasteboardContentGtk.h"

PasteboardContentGtk sounds like if there was a PasteboardContent base class and this is the Gtk implementation. Since this is already inside a gtk dir and only used inide GTK ifdefs, we don't need to use Gtk in the name.

> Source/WebKit2/Shared/gtk/PasteboardContentGtk.cpp:23
> +#include "DataObjectGtk.h"

This should be included as <WebCore/DataObjectGtk.h>

> Source/WebKit2/Shared/gtk/PasteboardContentGtk.cpp:32
> +PasteboardContentGtk::PasteboardContentGtk()
> +{
> +}

Use = default in the header.

> Source/WebKit2/Shared/gtk/PasteboardContentGtk.cpp:43
> +    encoder << static_cast<bool>(dataObject);
> +    if (dataObject)
> +        IPC::encode(encoder, dataObject.get());

We should never allow to encode a PasteboardContent without a dataObject. The default constructor is only required to the decode method, were we need to pass a ref to be filled.

> Source/WebKit2/Shared/gtk/PasteboardContentGtk.cpp:53
> +    bool hasPasteboardContent;
> +    if (!decoder.decode(hasPasteboardContent))
> +        return false;
> +
> +    if (!hasPasteboardContent)
> +        return true;

So, this is not needed either.

> Source/WebKit2/UIProcess/WebPasteboardProxy.h:45
> +#if PLATFORM(GTK)
> +class PasteboardContentGtk;
> +#endif

Move this below.

> Source/WebKit2/UIProcess/WebPasteboardProxy.h:90
> +    void setSelection(const String& pasteboardName, const WebKit::PasteboardContentGtk&);
> +    void getSelection(const String& pasteboardName, WebKit::PasteboardContentGtk&);

No need for WebKit:: inside WebKit namespace

> Source/WebKit2/UIProcess/gtk/WebPasteboardProxyGtk.cpp:29
> +#include <Shared/gtk/PasteboardContentGtk.h>

This should be "PasteboardContentGtk.h" include the directory to the include dir list if needed in the makefile.

> Source/WebKit2/UIProcess/gtk/WebPasteboardProxyGtk.cpp:39
> +void WebPasteboardProxy::setSelection(const String& pasteboardName, const WebKit::PasteboardContentGtk& pasteboardContent)

No need for WebKit::

> Source/WebKit2/UIProcess/gtk/WebPasteboardProxyGtk.cpp:44
> +void WebPasteboardProxy::getSelection(const String& pasteboardName, WebKit::PasteboardContentGtk& pasteboardContent)

Ditto.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:349
> +void WebPlatformStrategies::setSelection(const String& pasteboardName, WebCore::DataObjectGtk* dataObject)

This should receive a reference instead of a pointer.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:359
> +    return WTFMove(pasteboardContent.dataObject);

I'm don't think WTFMove() is needed in this case.

[Carlos Garcia Campos]

(In reply to comment #32)
> (In reply to comment #25)
> > Comment on attachment 288269 [details]
> > Patch, implement clipboard as a PlatformPasteboard
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=288269&action=review
> > 
> > What about WebEditorClient::updateGlobalSelection()? it's currently
> > protected by X11 guards, could we remove those guards now?
> 
> I need to investigate that. Having an implementation of primary selection is
> dependent on the compositor, and I'm not sure how well gtk+ will fare with
> gtk_clipboard_wait_for_contents() on it if the compositor doesn't offer an
> implementation. An option might be teaching PasteboardHelper to do
> gtk_clipboard_request* so stalls are not possible.
> 
> With gnome-shell/mutter it should just work though.

My point is that there's nothing X11 specific in there, and guards are compile time, so when building X11 and wayland at the same time that code will be used in wayland anyway.

> > > We don't like to use boolean parameters since it's harder to read; instead, you can do something like this:
> > > 
> > > enum class ShouldIncludeSmartPaste { No, Yes };
> > > 
> > > void writeToClipboard(ShouldIncludeSmartPaste = ShouldIncludeSmartPaste::No);
> > > 
> > > Then instead of calling writeToClipboard(true), you call writeToClipboard(ShouldIncludeSmartPaste::Yes), which is easier to read. Also consider whether having the default argument is really worth it; I'd probably just pass the tag everywhere to be explicit about it.
> > 
> > We already have an enum SmartPasteInclusion in PasteboardHelper
> 
> I think this is the only unaddressed comment. IMHO it's PasteboardHelper's
> enum which should change to using the one defined here, since this one is
> exposed in shared layers. If you think this modification belongs to the
> patch at hand I'll do that.

It's ok.

[Carlos Garcia Campos]

Comment on attachment 288564 [details]
Patch, implement clipboard as a PlatformPasteboard

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

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:219
> -    if (m_gtkClipboard)
> -        PasteboardHelper::singleton().writeClipboardContents(m_gtkClipboard, pasteboardContent.canSmartCopyOrDelete ? PasteboardHelper::IncludeSmartPaste : PasteboardHelper::DoNotIncludeSmartPaste, pasteboardContent.callback.get());
> +    writeToClipboard(pasteboardContent.canSmartCopyOrDelete ? ShouldIncludeSmartPaste::Yes : ShouldIncludeSmartPaste::No);

Wait, this one is breaking the callback used for global selection, since we are now ignoring pasteboardContent.callback. We need to rework all that, I don't remember why we needed to colapse the selection when cleared.

[Carlos Garnacho]

Created attachment 288814 [details]
Patch, implement clipboard as a PlatformPasteboard

[Carlos Garcia Campos]

Comment on attachment 288814 [details]
Patch, implement clipboard as a PlatformPasteboard

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

This looks really good now, I only have a few more comments

> Source/WebCore/platform/gtk/DataObjectGtk.cpp:142
> +    m_hasSmartPaste = false;

This set when writing to the clipboard based on whether SmartReplaceOption passed is CanSmartReplace. So I think it's more accurate to call this canSmartPaste

> Source/WebCore/platform/gtk/DataObjectGtk.h:79
> +    bool m_hasSmartPaste;

bool m_hasSmartPaste { false };

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:197
> -    if (m_gtkClipboard)
> -        PasteboardHelper::singleton().writeClipboardContents(m_gtkClipboard, *m_dataObject, pasteboardContent.canSmartCopyOrDelete ? PasteboardHelper::IncludeSmartPaste : PasteboardHelper::DoNotIncludeSmartPaste, pasteboardContent.callback.get());
> +    writeToClipboard();

We should pass the smart paste in this case too depending on pasteboardContent.canSmartCopyOrDelete

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:260
> -    return m_gtkClipboard && PasteboardHelper::singleton().clipboardContentSupportsSmartReplace(m_gtkClipboard);
> +    readFromClipboard();
> +    return !m_dataObject->unknownTypeData("application/vnd.webkitgtk.smartpaste").isEmpty();

I don't think this is correct, we shouldn't mix canSmartPaste with unknown types. I guess we could make canSmartPaste bidirectional, and in case of reading, set it also using gtk_clipboard_wait_is_target_available as we did before.

> Source/WebCore/platform/gtk/PasteboardHelper.cpp:292
> -    GRefPtr<GtkTargetList> list = targetListForDataObject(dataObject, includeSmartPaste);
> +    GRefPtr<GtkTargetList> list = targetListForDataObject(dataObject, dataObject.hasSmartPaste() ? IncludeSmartPaste : DoNotIncludeSmartPaste);

I think we should remove smart paste parameter from targetListForDataObject, since it already received the data object.

> Source/WebKit2/Shared/gtk/PasteboardContent.h:33
> +    explicit PasteboardContent(RefPtr<WebCore::DataObjectGtk>);

I think this could receive a const reference too

[Carlos Garnacho]

Created attachment 288825 [details]
Patch, implement clipboard as a PlatformPasteboard

Thanks for the review! I applied all suggestions, and removed the "smart paste" checks from using unknownDataTypes, now canSmartReplace() is set both ways on the DataObjectGtk.

[Carlos Garcia Campos]

Comment on attachment 288825 [details]
Patch, implement clipboard as a PlatformPasteboard

Excellent!

[WebKit Commit Bot]

Comment on attachment 288825 [details]
Patch, implement clipboard as a PlatformPasteboard

Clearing flags on attachment: 288825

Committed r205909: <http://trac.webkit.org/changeset/205909>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jeremy Huddleston Sequoia]

Build failure regression tracked in:
    https://bugs.webkit.org/show_bug.cgi?id=162261

[Michael Catanzaro]

It broke layout test editing/pasteboard/image-in-iframe, see bug #163185.