Closed Bug 636666 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Widget: Android, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: blassey, Assigned: blassey)

References

Details

Attachments

(1 file, 1 obsolete file)

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
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → blassey.bugs
Attachment #515122 - Flags: review?(doug.turner)
Duplicate of this bug: 637353
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.
Attachment #515122 - Flags: review?(doug.turner) → review-
(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
Attached patch patchSplinter Review
Attachment #515122 - Attachment is obsolete: true
Attachment #515712 - Flags: review?(doug.turner)
brad, what about the other flavors, what do you think the content api should look like when image and file is added?
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 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).
Attachment #515712 - Flags: review?(doug.turner) → review+
tracking-fennec: ? → 2.0+
pushed http://hg.mozilla.org/mozilla-central/rev/d53314c913a8
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 638479
Verified fix on Mozilla/5.0 (Android; Linux armv71; rv:2.0b13pre) Gecko/20110303 Firefox/4.0b13pre Fennec/4.0b6pre
Status: RESOLVED → VERIFIED
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.