636666 - paste option doesn't appear in context menu for text boxes in content on android

Status: VERIFIED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

with the patch in bug 611741 applied, copy, copy all and select all all work, but paste never shows up as a menu option. Presumably this is because hasDataMatchingFlavors isn't working in content for android.

https://mxr.mozilla.org/mozilla-central/source/widget/src/android/nsClipboard.cpp#123

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 515122 [details] [diff] [review]
patch


>+bool
>+ContentParent::RecvSetClipboardText(const nsString& text, const PRInt32& whichClipboard)
>+{
>+    nsIClipboard* clipboard;
>+    nsresult rv = CallGetService(kCClipboardCID, &clipboard);

nsCOMPtr's please:
  nsCOMPtr<nsIClipboard> clipboard(do_GetService(kCClipboardCID, &rv));




>+    if(NS_SUCCEEDED(rv)) {


just early return on failure to avoid this indentation.


>+        
>+        nsCOMPtr<nsISupports> nsisupportsDataWrapper =
>+            do_QueryInterface(dataWrapper);
>+
>+        rv = trans->SetTransferData(kUnicodeMime, nsisupportsDataWrapper,
>+                                            text.Length() * sizeof(PRUnichar));

I do not understand the need to QI back to nsISupports here.  What am I missing?

>+        NS_RELEASE(clipboard);

remove.
 
>+    nsIClipboard* clipboard;
>+    nsresult rv = CallGetService(kCClipboardCID, &clipboard);
>+    if(NS_SUCCEEDED(rv)) {

same comment from abolve.


>+        nsresult rv  = trans->GetTransferData(kUnicodeMime, getter_AddRefs(tmp),&len);

space between the comma and the amp.  Also, please use a different variable name - |tmp| is vague.

>+bool
>+ContentParent::RecvEmptyClipboard()
>+{
>+    nsIClipboard* clipboard;
>+    nsresult rv = CallGetService(kCClipboardCID, &clipboard);
>+    if(NS_SUCCEEDED(rv)) {

same comments from above -- nsCOMPtr and early return;

>+
>+static const char* textFlavors[] = { kUnicodeMime };
>+

prefix textFlavors[] with s, so sTextFlavors.  Maybe sClipboardFlavors and move it to the top of this file with the new #include statements.

>+bool
>+ContentParent::RecvClipboardHasText(PRBool* hasText)
>+{
>+    nsIClipboard* clipboard;
>+    nsresult rv = CallGetService(kCClipboardCID, &clipboard);
>+    if(NS_SUCCEEDED(rv)) {

Same.

>+        clipboard->HasDataMatchingFlavors(textFlavors, 1, 
>+                     nsIClipboard::kGlobalClipboard, hasText);


indention.


>     virtual bool RecvReadPermissions(InfallibleTArray<IPC::Permission>* aPermissions);
>+    virtual bool RecvSetClipboardText(const nsString& text, const PRInt32& whichClipboard);
>+    virtual bool RecvGetClipboardText(const PRInt32& whichClipboard, nsString* text);
>+    virtual bool RecvEmptyClipboard();
>+    virtual bool RecvClipboardHasText(PRBool* hasText);
> 
>     virtual bool RecvStartVisitedQuery(const IPC::URI& uri);
> 
>diff --git a/dom/ipc/PContent.ipdl b/dom/ipc/PContent.ipdl
>--- a/dom/ipc/PContent.ipdl
>+++ b/dom/ipc/PContent.ipdl
>@@ -179,6 +179,13 @@ parent:
>     // nsIPermissionManager messages
>     sync ReadPermissions() returns (Permission[] permissions);
> 
>+    SetClipboardText(nsString text, PRInt32 whichClipboard);
>+    sync GetClipboardText(PRInt32 whichClipboard)
>+        returns (nsString text);
>+    EmptyClipboard();
>+    sync ClipboardHasText()
>+        returns (PRBool hasText);
>+
> both:
>      AsyncMessage(nsString aMessage, nsString aJSON);


Explain why we don't care about any other flavor?  What about image flavors, or even drop files which use clipboard (i think?).  please get cjones or bsmedberg to consider these.  Maybe we don't need anything else -- and we can adjust later for these other flavors.  However, we do want to ensure that we aren't going to break anything in the future (as best we can).

r-.  please post another patch, and lets talk to ^^ on irc or here to figure out if this is the best approach.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #3)

> 
> 
> >+        
> >+        nsCOMPtr<nsISupports> nsisupportsDataWrapper =
> >+            do_QueryInterface(dataWrapper);
> >+
> >+        rv = trans->SetTransferData(kUnicodeMime, nsisupportsDataWrapper,
> >+                                            text.Length() * sizeof(PRUnichar));
> 
> I do not understand the need to QI back to nsISupports here.  What am I
> missing?
without the QI, I get:
error: conversion from 'nsCOMPtr<nsISupportsString>' to non-scalar type 'nsCOMPtr<nsISupports>' requested

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 6

[Doug Turner (:dougt)]

brad, what about the other flavors, what do you think the content api should look like when image and file is added?

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

if and when we have a platform clipboard implementation that supports other content types and needs this remoting interface we should add other getters and setters (like Get/SetClipboardImage, Get/SetClipboardFile, etc)

Comment 8

[Doug Turner (:dougt)]

Comment on attachment 515712 [details] [diff] [review]
patch


-  AndroidBridge::Bridge()->SetClipboardText(buffer);
+
+
+  if (XRE_GetProcessType() == GeckoProcessType_Default) {
+    if (AndroidBridge::Bridge())
+      AndroidBridge::Bridge()->Se

Two ws lines not needed


Please comment up the PContent about why this is required for android (and other implementations that don't have direct access to the clipboard).

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/d53314c913a8

Comment 10

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Android; Linux armv71; rv:2.0b13pre) Gecko/20110303 Firefox/4.0b13pre Fennec/4.0b6pre