Bug 102484 – [EFL] Implementation of pasteboard

• Component: WebKit EFL
• Status: RESOLVED WONTFIX

[Michal Pakula vel Rutka]

EFL port lacks of pasteboard implementation, this bug will handle WebCore and WebKit2 implementation.

[Michal Pakula vel Rutka]

Created attachment 175434 [details]
proposed patch

[WebKit Review Bot]

Attachment 175434 [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/WebKit2/UIProcess/API/efl/ewk_pasteboard.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 24 files


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

[Michal Pakula vel Rutka]

Created attachment 175436 [details]
proposed patch

[Michal Pakula vel Rutka]

This patch is only WebCore and WebKit2 implementation, TestRunner with gardening and Minibrowser implementation will be added later.

[Gyuyoung Kim]

Comment on attachment 175436 [details]
proposed patch

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

> Source/WebKit2/UIProcess/WebContext.h:279
> +    void requestPasteboardDataForType(const String& dataType, String& value);

IMO, getPasteboardDataForType matches to setPasteboardDataForType as mac port.

 -void getPasteboardTypes(const String& pasteboardName, Vector<String>& pasteboardTypes);
 -void setPasteboardTypes(const String& pasteboardName, const Vector<String>& pasteboardTypes);

BTW, don't you need to use Vector instead of String for the type ? Or, can't we share mac port's definitions ?

In WebContext.h,

#if PLATFORM(MAC)
    void getPasteboardTypes(const String& pasteboardName, Vector<String>& pasteboardTypes);
    void getPasteboardPathnamesForType(const String& pasteboardName, const String& pasteboardType, Vector<String>& pathnames);
    void getPasteboardStringForType(const String& pasteboardName, const String& pasteboardType, String&);
    void getPasteboardBufferForType(const String& pasteboardName, const String& pasteboardType, SharedMemory::Handle&, uint64_t& size);
    void pasteboardCopy(const String& fromPasteboard, const String& toPasteboard);
    void getPasteboardChangeCount(const String& pasteboardName, uint64_t& changeCount);
    void getPasteboardUniqueName(String& pasteboardName);
    void getPasteboardColor(const String& pasteboardName, WebCore::Color&);
    void getPasteboardURL(const String& pasteboardName, WTF::String&);
    void addPasteboardTypes(const String& pasteboardName, const Vector<String>& pasteboardTypes);
    void setPasteboardTypes(const String& pasteboardName, const Vector<String>& pasteboardTypes);
    void setPasteboardPathnamesForType(const String& pasteboardName, const String& pasteboardType, const Vector<String>& pathnames);
    void setPasteboardStringForType(const String& pasteboardName, const String& pasteboardType, const String&);
    void setPasteboardBufferForType(const String& pasteboardName, const String& pasteboardType, const SharedMemory::Handle&, uint64_t size);
#endif

> Source/WebKit2/UIProcess/efl/WebPasteboardProxyEfl.h:37
> +class WebPasteboardProxyEfl : public APIObject {

I don't understand why you only support this for EFL port. Is there any EFL specific dependency? If not, IMO, you need to change common WK2 structure.

[Michal Pakula vel Rutka]

Comment on attachment 175436 [details]
proposed patch

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

>> Source/WebKit2/UIProcess/WebContext.h:279
>> +    void requestPasteboardDataForType(const String& dataType, String& value);
> 
> IMO, getPasteboardDataForType matches to setPasteboardDataForType as mac port.
> 
>  -void getPasteboardTypes(const String& pasteboardName, Vector<String>& pasteboardTypes);
>  -void setPasteboardTypes(const String& pasteboardName, const Vector<String>& pasteboardTypes);
> 
> BTW, don't you need to use Vector instead of String for the type ? Or, can't we share mac port's definitions ?
> 
> In WebContext.h,
> 
> #if PLATFORM(MAC)
>     void getPasteboardTypes(const String& pasteboardName, Vector<String>& pasteboardTypes);
>     void getPasteboardPathnamesForType(const String& pasteboardName, const String& pasteboardType, Vector<String>& pathnames);
>     void getPasteboardStringForType(const String& pasteboardName, const String& pasteboardType, String&);
>     void getPasteboardBufferForType(const String& pasteboardName, const String& pasteboardType, SharedMemory::Handle&, uint64_t& size);
>     void pasteboardCopy(const String& fromPasteboard, const String& toPasteboard);
>     void getPasteboardChangeCount(const String& pasteboardName, uint64_t& changeCount);
>     void getPasteboardUniqueName(String& pasteboardName);
>     void getPasteboardColor(const String& pasteboardName, WebCore::Color&);
>     void getPasteboardURL(const String& pasteboardName, WTF::String&);
>     void addPasteboardTypes(const String& pasteboardName, const Vector<String>& pasteboardTypes);
>     void setPasteboardTypes(const String& pasteboardName, const Vector<String>& pasteboardTypes);
>     void setPasteboardPathnamesForType(const String& pasteboardName, const String& pasteboardType, const Vector<String>& pathnames);
>     void setPasteboardStringForType(const String& pasteboardName, const String& pasteboardType, const String&);
>     void setPasteboardBufferForType(const String& pasteboardName, const String& pasteboardType, const SharedMemory::Handle&, uint64_t size);
> #endif

Instead of setPasteboardDataForType, we can use mac's setPasteboardStringForType leaving one of its parameters - pasteboardName - empty for now. Similar instead of requestDataForType we can use getPasteboardStringForType, not using pasteboardName for now.

>> Source/WebKit2/UIProcess/efl/WebPasteboardProxyEfl.h:37
>> +class WebPasteboardProxyEfl : public APIObject {
> 
> I don't understand why you only support this for EFL port. Is there any EFL specific dependency? If not, IMO, you need to change common WK2 structure.

Other ports don't need this type of implementation as all pasteboard job can be, and it is done inside WebKit/WebCore. In EFL port we cannot do that, so I have added this file to delegate copy/paste operations to application. Of course if it is needed I can change it into more common implementation.

[Gyuyoung Kim]

Comment on attachment 175436 [details]
proposed patch

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

>>> Source/WebKit2/UIProcess/efl/WebPasteboardProxyEfl.h:37
>>> +class WebPasteboardProxyEfl : public APIObject {
>> 
>> I don't understand why you only support this for EFL port. Is there any EFL specific dependency? If not, IMO, you need to change common WK2 structure.
> 
> Other ports don't need this type of implementation as all pasteboard job can be, and it is done inside WebKit/WebCore. In EFL port we cannot do that, so I have added this file to delegate copy/paste operations to application. Of course if it is needed I can change it into more common implementation.

>> In EFL port we cannot do that

Could you let me know why EFL can't do that ? Should you use elementary ? If so, we need to check if we can has port specific file to WK2 reviewers.

[Michal Pakula vel Rutka]

Comment on attachment 175436 [details]
proposed patch

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

>>>> Source/WebKit2/UIProcess/efl/WebPasteboardProxyEfl.h:37
>>>> +class WebPasteboardProxyEfl : public APIObject {
>>> 
>>> I don't understand why you only support this for EFL port. Is there any EFL specific dependency? If not, IMO, you need to change common WK2 structure.
>> 
>> Other ports don't need this type of implementation as all pasteboard job can be, and it is done inside WebKit/WebCore. In EFL port we cannot do that, so I have added this file to delegate copy/paste operations to application. Of course if it is needed I can change it into more common implementation.
> 
> 

In EFL copy and paste API is placed in Elementary and as it depends on WebKit (via elm_web) we cannot include it into WebKit. Second thing is that Elementary copy and paste functions - elm_cnp_selection_get/set - need a widget (or Evas_Object with X window i.e. view or Elementary window) to be passed as one of arguments and we cannot provide that kind of object in WebCore with exception of some kind of fake clipboard object. 
The solution is to move the code out to application side as EFL developers assumed, and this file is needed to do it. For now I added it only for EFL and as I wrote earlier I can change it into more common implementation but it will remain unused by other ports.

[Gyuyoung Kim]

(In reply to comment #8)

> In EFL copy and paste API is placed in Elementary and as it depends on WebKit (via elm_web) we cannot include it into WebKit. Second thing is that Elementary copy and paste functions - elm_cnp_selection_get/set - need a widget (or Evas_Object with X window i.e. view or Elementary window) to be passed as one of arguments and we cannot provide that kind of object in WebCore with exception of some kind of fake clipboard object. 
> The solution is to move the code out to application side as EFL developers assumed, and this file is needed to do it. For now I added it only for EFL and as I wrote earlier I can change it into more common implementation but it will remain unused by other ports.

CC'ing Sam, Sam, can EFL port has this port specific files without common file ? EFL port can't support copy & past in WebCore side and WebKit layer because of EFL specific dependency.

Source/WebKit2/UIProcess/efl/WebPasteboardClientEfl.cpp
Source/WebKit2/UIProcess/API/C/efl/WKPasteboardEfl.cpp

[seonae kim]

(In reply to comment #3)
> Created an attachment (id=175436) [details]
> proposed patch

I'm implementing Clipboard API to get/set the data to javascript.
This APIs have similar structure with pasteboard and do you have a plan to change the files to common name not pasteboard?

[Michal Pakula vel Rutka]

(In reply to comment #10)
> (In reply to comment #3)
> > Created an attachment (id=175436) [details] [details]
> > proposed patch
> 
> I'm implementing Clipboard API to get/set the data to javascript.
> This APIs have similar structure with pasteboard and do you have a plan to change the files to common name not pasteboard?

At this moment files names are not definitely set, as I am not sure whether to make WK2 files EFL port-only or available for all ports.
Are you OK with removing the Pasteboard word from methods and change files names to i.e. Clipboard?

[Michal Pakula vel Rutka]

Created attachment 178808 [details]
added WebKit1 stubs

Added WebKit1 stubs, run when copy and paste is called plus some minor changes in WebKit2.

[Michal Pakula vel Rutka]

Created attachment 179749 [details]
added primary selection support

Added primary selection support to enable X11 global selection. Two methods from PasteboardStrategy are now shared between EFL and MAC ports.

[Grzegorz Czajkowski]

Comment on attachment 179749 [details]
added primary selection support

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

> Source/WebCore/ChangeLog:10
> +        to be done on WebKit-using application side.

Please add space around '-'.

> Source/WebCore/platform/efl/PasteboardEfl.cpp:56
> +    platformStrategies()->pasteboardStrategy()->setStringForType(text, selectionFormatText,

Don't you need to add PLATFORM_STRATEGIES guard for it?

> Source/WebKit2/ChangeLog:8
> +        EFL does not provide platform-wide copy and paste support, so we cannot

This comment can be skipped as it's placed in the previous ChangeLogs.

> Source/WebKit2/UIProcess/API/efl/ewk_pasteboard.cpp:53
> +        static_cast<Ewk_Pasteboard_Selection_Type>(selectionType));

Please add extra indent to show that it is one instruction.

> Source/WebKit2/UIProcess/API/efl/ewk_pasteboard.cpp:60
> +        static_cast<Ewk_Pasteboard_Selection_Format>(selectionFormat), static_cast<Ewk_Pasteboard_Selection_Type>(selectionType)));

Ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_pasteboard.cpp:79
> +

Unnecessary empty line.

> Source/WebKit2/UIProcess/API/efl/ewk_pasteboard.h:62
> + * @param data to be copied

Please mention that the data should be freed.

> Source/WebKit2/UIProcess/API/efl/ewk_pasteboard.h:66
> +typedef void (*Ewk_Pasteboard_Data_Set_Cb)(const char *data, Ewk_Pasteboard_Selection_Format selection_format, Ewk_Pasteboard_Selection_Type selection_type);

I am not sure whether data parameter should be passed as const. According to your implementation you are using strdup() so the client is responsible for freeing that memory.

> Source/WebKit2/UIProcess/API/efl/ewk_pasteboard.h:71
> + * This callback will request and return if received a data for given type.

a data -> data

> Source/WebKit2/UIProcess/API/efl/ewk_pasteboard.h:76
> + * @return requested data if at success or @c NULL at failure

requested data if on success or @c NULL on failure

[Michal Pakula vel Rutka]

Comment on attachment 179749 [details]
added primary selection support

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

>> Source/WebCore/platform/efl/PasteboardEfl.cpp:56
>> +    platformStrategies()->pasteboardStrategy()->setStringForType(text, selectionFormatText,
> 
> Don't you need to add PLATFORM_STRATEGIES guard for it?

To be honest I have done it the same way as in PastebaordMac.mm - headers are guarded and code is not. PLATFORM_STRATEGIES is turned on on our port in Platform.h, when turned off WebKit1 does not build as PlatformStrategiesEfl.h is not guarded. As this is EFL specific file I will remove the guards from headers.

>> Source/WebKit2/UIProcess/API/efl/ewk_pasteboard.cpp:53
>> +        static_cast<Ewk_Pasteboard_Selection_Type>(selectionType));
> 
> Please add extra indent to show that it is one instruction.

check-webkit-style enforces 4 spaces indent

>> Source/WebKit2/UIProcess/API/efl/ewk_pasteboard.h:62
>> + * @param data to be copied
> 
> Please mention that the data should be freed.

This is my mistake, I will remove strdup from the code.

>> Source/WebKit2/UIProcess/API/efl/ewk_pasteboard.h:66
>> +typedef void (*Ewk_Pasteboard_Data_Set_Cb)(const char *data, Ewk_Pasteboard_Selection_Format selection_format, Ewk_Pasteboard_Selection_Type selection_type);
> 
> I am not sure whether data parameter should be passed as const. According to your implementation you are using strdup() so the client is responsible for freeing that memory.

the same here

[Michal Pakula vel Rutka]

Created attachment 180131 [details]
fixes after review

[Grzegorz Czajkowski]

Comment on attachment 180131 [details]
fixes after review

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

In my opinion patch itself looks quite good. I understand that this implementation is different from other WebKit ports due to EFL design - copy/paste is delegated to Elementary's widgets.

> Source/WebKit2/UIProcess/API/efl/ewk_pasteboard.h:28
> +

It'd be nice to add some general description in doxygen for this file (@file, @brief).

> Source/WebKit2/UIProcess/API/efl/ewk_pasteboard.h:62
> + * @param data to be copied

You need to duplicate 'data' here. Doxygen generates: paramter parameter_name - parameter description. Please adapt this fix to all doxygen descriptions.

[Grzegorz Czajkowski]

According to EFL policy, to get formal review this patch should contain unit tests as you are adding new API. Those can be added in separate bug.

[Michal Pakula vel Rutka]

(In reply to comment #18)
> According to EFL policy, to get formal review this patch should contain unit tests as you are adding new API. Those can be added in separate bug.

OK I will create a new bug then.

[Michal Pakula vel Rutka]

Created attachment 181928 [details]
fixes

Fixes in documentation according to Grzegorz suggestions also added a possibility for application for passing pointer to its own application data.

[Benjamin Poulain]

Comment on attachment 181928 [details]
fixes

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

A lot of this code is wiring to expose the pasteboard. This is a bit odd and I wonder why should it be done like that.

> Source/WebCore/ChangeLog:10
> +        EFL does not provide platform-wide copy and paste support, so we cannot
> +        implement pasteboard inside WebCore as in other ports. The implementation has
> +        to be done on application side.

I understand that you may have to do the pasteboard support on the UISide, but in the App?
X11 has pretty standard pasteboard support. Can you explain more about this?

> Source/WebCore/ChangeLog:12
> +        No new tests - already covered by existing tests.

How can that be if you are implementing a new feature?
Shouldn't you unskip the existing tests?

> Source/WebCore/platform/Pasteboard.h:70
> +const String selectionFormatText("Text");
> +const String selectionFormatMarkup("Markup");
> +const String selectionTypePrimary("Primary");
> +const String selectionTypeClipboard("Clipboard");

String literals.

> Source/WebCore/platform/Pasteboard.h:134
>      bool m_selectionMode;

This is a poor name for a boolean :)

> Source/WebCore/platform/efl/PasteboardEfl.cpp:54
> -    notImplemented();
> +    platformStrategies()->pasteboardStrategy()->setStringForType(text, selectionFormatText,
> +        m_selectionMode ? selectionTypePrimary : selectionTypeClipboard);

You should reformat this to make it cleaner.
We don't break at 80 characters in WebKit.

> Source/WebCore/platform/efl/PasteboardEfl.cpp:61
> -    notImplemented();
> +    platformStrategies()->pasteboardStrategy()->setStringForType(
> +        createMarkup(selectedRange, 0, AnnotateForInterchange, false, ResolveNonLocalURLs), selectionFormatMarkup,
> +        m_selectionMode ? selectionTypePrimary : selectionTypeClipboard);

ditto.

> Source/WebCore/platform/efl/PasteboardEfl.cpp:102
> +        RefPtr<DocumentFragment> fragment = createFragmentFromMarkup(frame->document(), data, "", DisallowScriptingContent);

"" -> emptyString()

> Source/WebCore/platform/efl/PasteboardEfl.cpp:103
> +        if (fragment)

This is odd, why this branch?

[Gyuyoung Kim]

Comment on attachment 181928 [details]
fixes

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

> Source/WebKit/efl/WebCoreSupport/PlatformStrategiesEfl.cpp:166
> +void PlatformStrategiesEfl::setStringForType(const String&, const String& pasteboardType, const String& pasteboardName)

Remove pasteboardType, pateboardName parameters or use UNUSED_PARAM() macro for them.

> Source/WebKit/efl/WebCoreSupport/PlatformStrategiesEfl.cpp:171
> +String PlatformStrategiesEfl::stringForType(const String& pasteboardType, const String& pasteboardName)

ditto.

> Source/WebKit2/Shared/API/c/efl/WKBaseEfl.h:28
> +typedef const struct OpaqueWKPasteboard* WKPasteboardRef;

I prefer to place variables by alphabetical order, if possible.

> Source/WebKit2/Shared/APIObject.h:135
> +        TypePasteboardProxy,

ditto ?

> Source/WebKit2/UIProcess/API/C/efl/WKPasteboardEfl.cpp:39
> +    WebPasteboardProxyEfl::shared()->initializeClient(client);

Any reason to use *initialize* ? Is setClient(client) appropriate ?

> Source/WebKit2/UIProcess/API/efl/ewk_pasteboard.h:52
> +/* Creates a type name for the Ewk_Pasteboard_Selection_Format.*/

EFL port has used /// instead of /* */ for typedef enum.

> Source/WebKit2/UIProcess/API/efl/ewk_pasteboard.h:64
> +/* Creates a type name for the Ewk_Pasteboard_Selection_Type.*/

ditto.

[Benjamin Poulain]

Comment on attachment 181928 [details]
fixes

Looks like there are enough concerns to r-.

[Michal Pakula vel Rutka]

Comment on attachment 181928 [details]
fixes

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

>> Source/WebCore/ChangeLog:10
>> +        to be done on application side.
> 
> I understand that you may have to do the pasteboard support on the UISide, but in the App?
> X11 has pretty standard pasteboard support. Can you explain more about this?

EFL copy and paste API is placed inside Elementary (http://docs.enlightenment.org/auto/elementary/) which is a toolkit used for developing applications which contains UI widgets like popups, lists, progress bars, etc. We cannot include it into WebKit becauses of one of Elementary's widget - elm_web which is a simple webview based on WebKit and also as I talked with one of EFL developers - Gustavo Sverzut Barbieri - webkit-efl is meant to be elementary independent.
In general Elementary implementation of copy/paste is a wrap on X11: first a data is put into a structure containing data, type, callbacks (http://trac.enlightenment.org/e/browser/trunk/elementary/src/lib/elm_cnp.c#L121) and then passed Ecore where XSetSelectionOwner is called. 
API functions used for copy and paste (elm_cnp_selection_set and elm_cnp_selection_get respectively see: http://docs.enlightenment.org/auto/elementary/group__CopyPaste.html) excepts an Elementary widget as one of arguments. In X11 widget is needed only to get X-window object and an object on which a callback can be called. So to call this API inside WebKit (apart from including Elementary) we will have to create a fake widget or at least a fake object with X-window only for supporting copy and paste API.
The only way then is to (suggested to me by Gustavo) is to delegate it to an application which can take care of calling Elementary clipboard functions, passing proper widgets, etc.

>> Source/WebCore/ChangeLog:12
>> +        No new tests - already covered by existing tests.
> 
> How can that be if you are implementing a new feature?
> Shouldn't you unskip the existing tests?

This patch introduces a new feature only for EFL port, as Pasteboard is already implemented and tests for it has been already written. I have created a separate bug (https://bugs.webkit.org/show_bug.cgi?id=102486) for unskipping tests and implementation for WTR.

>> Source/WebCore/platform/Pasteboard.h:70
>> +const String selectionTypeClipboard("Clipboard");
> 
> String literals.

Done.

>> Source/WebCore/platform/Pasteboard.h:134
>>      bool m_selectionMode;
> 
> This is a poor name for a boolean :)

This was not introduced by me :). If you wish I can fill a new bug for it as Chromium and QT people may be interested in changing it, but not with this patch.

>> Source/WebCore/platform/efl/PasteboardEfl.cpp:54
>> +        m_selectionMode ? selectionTypePrimary : selectionTypeClipboard);
> 
> You should reformat this to make it cleaner.
> We don't break at 80 characters in WebKit.

Done.

>> Source/WebCore/platform/efl/PasteboardEfl.cpp:102
>> +        RefPtr<DocumentFragment> fragment = createFragmentFromMarkup(frame->document(), data, "", DisallowScriptingContent);
> 
> "" -> emptyString()

Done.

>> Source/WebCore/platform/efl/PasteboardEfl.cpp:103
>> +        if (fragment)
> 
> This is odd, why this branch?

I will remove it.

>> Source/WebKit/efl/WebCoreSupport/PlatformStrategiesEfl.cpp:166
>> +void PlatformStrategiesEfl::setStringForType(const String&, const String& pasteboardType, const String& pasteboardName)
> 
> Remove pasteboardType, pateboardName parameters or use UNUSED_PARAM() macro for them.

I will remove them.

>> Source/WebKit2/Shared/API/c/efl/WKBaseEfl.h:28
>> +typedef const struct OpaqueWKPasteboard* WKPasteboardRef;
> 
> I prefer to place variables by alphabetical order, if possible.

Done.

>> Source/WebKit2/Shared/APIObject.h:135
>> +        TypePasteboardProxy,
> 
> ditto ?

I will move it within //Platform specific group

>> Source/WebKit2/UIProcess/API/C/efl/WKPasteboardEfl.cpp:39
>> +    WebPasteboardProxyEfl::shared()->initializeClient(client);
> 
> Any reason to use *initialize* ? Is setClient(client) appropriate ?

setClient will be appropriate, I will change it.

>> Source/WebKit2/UIProcess/API/efl/ewk_pasteboard.h:52
>> +/* Creates a type name for the Ewk_Pasteboard_Selection_Format.*/
> 
> EFL port has used /// instead of /* */ for typedef enum.

Done.

[Michal Pakula vel Rutka]

Created attachment 182731 [details]
fixes after review

[Michal Pakula vel Rutka]

Created attachment 182762 [details]
fixes after review2

emptyString() added

[Benjamin Poulain]

Comment on attachment 182762 [details]
fixes after review2

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

Quick questions:
Why is PasteboardProxy a APIObject? Where is it used for API?
What is your API? What is defined in WKPasteboardEfl.h or ewk_pasteboard.h?

> Source/WebCore/platform/Pasteboard.h:120
> +#if PLATFORM(EFL)
> +    static const char* const selectionFormatText;
> +    static const char* const selectionFormatMarkup;
> +    static const char* const selectionTypePrimary;
> +    static const char* const selectionTypeClipboard;
> +#endif

IMHO, this should be in a EFL header, not in the common header.

> Source/WebCore/platform/efl/PasteboardEfl.cpp:43
> +const char* const Pasteboard::selectionFormatText = "Text";
> +const char* const Pasteboard::selectionFormatMarkup = "Markup";
> +const char* const Pasteboard::selectionTypePrimary = "Primary";
> +const char* const Pasteboard::selectionTypeClipboard = "Clipboard";

What I meant here is that should be using WTF::String literal initialization.

[Michal Pakula vel Rutka]

(In reply to comment #27)
> (From update of attachment 182762 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182762&action=review
> 
> Quick questions:
> Why is PasteboardProxy a APIObject? Where is it used for API?
> What is your API? What is defined in WKPasteboardEfl.h or ewk_pasteboard.h?
> 

EFL port does not export WK API, only ewk_headers are exported (https://trac.webkit.org/browser/trunk/Source/WebKit2/PlatformEfl.cmake#L261), so external applications cannot access it. ewk_pasteboard it is just a wrap over WKPasteboardEfl visible for applications and similar situation is quite common in EFL port i.e WKCookieManager->ewk_cookie_manager.
From application point of view the API is placed in ewk_pasteboard.h, but if WK API will be exported it would be unnecessary as application will set WKPasteboardEfl structure itself.

> > Source/WebCore/platform/Pasteboard.h:120
> > +#if PLATFORM(EFL)
> > +    static const char* const selectionFormatText;
> > +    static const char* const selectionFormatMarkup;
> > +    static const char* const selectionTypePrimary;
> > +    static const char* const selectionTypeClipboard;
> > +#endif
> 
> IMHO, this should be in a EFL header, not in the common header.
> 
> > Source/WebCore/platform/efl/PasteboardEfl.cpp:43
> > +const char* const Pasteboard::selectionFormatText = "Text";
> > +const char* const Pasteboard::selectionFormatMarkup = "Markup";
> > +const char* const Pasteboard::selectionTypePrimary = "Primary";
> > +const char* const Pasteboard::selectionTypeClipboard = "Clipboard";
> 
> What I meant here is that should be using WTF::String literal initialization.

I have created a new header using ASCII literals.

[Michal Pakula vel Rutka]

Created attachment 182970 [details]
added new file for pasteboard constants

[Sam Weinig]

Comment on attachment 182970 [details]
added new file for pasteboard constants

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

Its not clear to me why you can't just make the EFL port dependent on Elementary.

> Source/WebKit2/UIProcess/WebContext.messages.in:47
> +    GetPasteboardStringForType(WTF::String pasteboardName, WTF::String pasteboardType) -> (WTF::String string)

Is there a way you can do this without a sync message.  We would like to remove more sync messages for Mac, and adding this for EFL will make it harder for us to remove it.

[Gyuyoung Kim]

(In reply to comment #30)
> (From update of attachment 182970 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182970&action=review
> 
> Its not clear to me why you can't just make the EFL port dependent on Elementary.

EFL community has wanted to support WebKit EFL as one of elementary module. Look at this article. http://trac.enlightenment.org/e/blog/2011/10/31/elm_web

If we use elementary inside WebKit EFL port, it will make circular dependency. But, I'm not sure if this is correct way personally. Because this policy gives us many inconveniences.

[Michal Pakula vel Rutka]

Comment on attachment 182970 [details]
added new file for pasteboard constants

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

>> Source/WebKit2/UIProcess/WebContext.messages.in:47
>> +    GetPasteboardStringForType(WTF::String pasteboardName, WTF::String pasteboardType) -> (WTF::String string)
> 
> Is there a way you can do this without a sync message.  We would like to remove more sync messages for Mac, and adding this for EFL will make it harder for us to remove it.

I made a test patch and yes, it is possible. When changing to async message I will need to introduce:
- additional internal main loop in WebProcess side started when paste request is called. Similar thing is done in GTK (i.e. gtk_clipboard_wait_for_contents), but EFL does not provide nothing like this and it will have to be written inside WebCore from scratch.
- an API function which will fill WebCore's pasteboard with data and then stop internal loop started, when data from EFL clipboard will come.

This API will cause change in this implementation from currently proposed - EFL only, to common for all ports. I think proposing new WK API used only by one port implemented by all ports is not good. From my point of view current proposed implementation is just better and if it is possible I would rather to stay with sync message. If Mac port wants to get rid of sync messages, please change it and EFL will just restore old getPasteboardStringForType which will not be shared within Mac and EFL ports.

[KyungTae Kim]

Is there any progress on this?

[Michal Pakula vel Rutka]

(In reply to comment #33)
> Is there any progress on this?

I plan to post new patches in the beginning of August.

[KyungTae Kim]

(In reply to comment #34)
> (In reply to comment #33)
> > Is there any progress on this?
> I plan to post new patches in the beginning of August.
When will you post the new patch?
The pasteboard is an important functionality and this blocks other things like drag&drop (bug 109551).

[Michal Pakula vel Rutka]

Created attachment 208734 [details]
new patch

I decided to split this into three patches: pasteboard client in WKContext (bug 119799), EFL implementation in WebCore and WebKit2 (bug 102484) and EFL WK API implementation.

[Michal Pakula vel Rutka]

Created attachment 211564 [details]
rebase after pasteboard changes

[Michal Pakula vel Rutka]

Created attachment 232949 [details]
rebased patch

Rebased patch updated after changes in bug 119799.

[Jinwoo Song]

Hi Michal, recently, Joonhun start to upstream the Tizen pastedboard implementation through https://bugs.webkit.org/show_bug.cgi?id=133782.

This patch conflicts with your implementation but the patch seems to have different implementation way. I'm not sure which is a right approach so please communicate with Joonhun for finding better thing.

[Michal Pakula vel Rutka]

(In reply to comment #39)
> Hi Michal, recently, Joonhun start to upstream the Tizen pastedboard implementation through https://bugs.webkit.org/show_bug.cgi?id=133782.
> 
> This patch conflicts with your implementation but the patch seems to have different implementation way. I'm not sure which is a right approach so please communicate with Joonhun for finding better thing.

This patch and related patches (bug 119799, bug 120209) connect webkit pasteboard with system copy and paste using elementary functions (elm_cnp_selection_set and elm_cnp_selection_get), to allow copy and paste functions work between different applications.

[Michael Catanzaro]

Closing this bug because the EFL port has been removed from trunk.

If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.