Implement nsClipboard with text support only for B2G

RESOLVED FIXED in 2.0 S1 (9may)

Status

Firefox OS
Gaia::Keyboard
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: janjongboom, Assigned: janjongboom)

Tracking

unspecified
2.0 S1 (9may)
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.0)

Details

Attachments

(1 attachment, 3 obsolete attachments)

At the moment it's not possible to use the clipboard on B2G because we don't have an implementation. We'll need it for copy/paste.
(Assignee)

Updated

4 years ago
Blocks: 938045
Created attachment 8342992 [details] [diff] [review]
clipboard_b2g.patch

So here is my take on a clipboard for B2G... Because there is no native context I buffer it all in a nsAutoString on the object itself.

And remember it's my first C++ patch of size so be nice. :-)
Assignee: nobody → janjongboom
Attachment #8342992 - Flags: review?(xyuan)
Attachment #8342992 - Flags: feedback?(fabrice)
Comment on attachment 8342992 [details] [diff] [review]
clipboard_b2g.patch

Review of attachment 8342992 [details] [diff] [review]:
-----------------------------------------------------------------

r=me.

It look great for me. Most parts of the code are the same with that for android platform (see dom/widget/android/nsClipboard.cpp) except that the clipboard text is stored by a member variable of nsClipboard.

If the code styles nits are fixed, it will be better ;-)

::: widget/gonk/nsClipboard.h
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef NS_CLIPBOARD_H
> +#define NS_CLIPBOARD_H

The name of the include guards is suggested to be something like |nsClipbard_h__| for Mozilla.

You can refer to the full explanation from the wiki: https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style
"""
Include guards are named by determining the fully-qualified include path, then substituting _ for / and . and - in it. For example, nsINode.h's guard is nsINode_h, and Element.h's guard is mozilla_dom_Element_h (because its include path is mozilla/dom/Element.h).
"""

@@ +9,5 @@
> +#include "nsIClipboard.h"
> +
> +class nsClipboard MOZ_FINAL : public nsIClipboard
> +{
> +  nsAutoString _clipboard;

Typically, for C++ member variable, we use the prefix "m" rather than "_". So the variable name is suggested to be mClipboard.
Attachment #8342992 - Flags: review?(xyuan) → review+
Comment on attachment 8342992 [details] [diff] [review]
clipboard_b2g.patch

Review of attachment 8342992 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly nits to fix, thanks Jan!

We also need tests here before landing (or at least check if we can enable for b2g some existing clipboard tests).

::: widget/gonk/nsClipboard.cpp
@@ +22,5 @@
> +nsClipboard::SetData(nsITransferable *aTransferable,
> +                     nsIClipboardOwner *anOwner, int32_t aWhichClipboard)
> +{
> +  if (aWhichClipboard != kGlobalClipboard)
> +    return NS_ERROR_NOT_IMPLEMENTED;

nit: if() { .... } even for single line blocks.

@@ +28,5 @@
> +  nsCOMPtr<nsISupports> tmp;
> +  uint32_t len;
> +  nsresult rv  = aTransferable->GetTransferData(kUnicodeMime, getter_AddRefs(tmp),
> +                                                &len);
> +  NS_ENSURE_SUCCESS(rv, rv);

NS_ENSURE_SUCCESS is deprecated: see https://groups.google.com/forum/#!searchin/mozilla.dev.platform/NS_ENSURE_SUCCESS/mozilla.dev.platform/1clMLuuhtWQ/8MxLDZN28Y4J

@@ +84,5 @@
> +NS_IMETHODIMP
> +nsClipboard::EmptyClipboard(int32_t aWhichClipboard)
> +{
> +  if (aWhichClipboard != kGlobalClipboard)
> +    return NS_ERROR_NOT_IMPLEMENTED;

nit: if() { .... } even for single line blocks.

@@ +101,5 @@
> +                                    bool *aHasText)
> +{
> +  *aHasText = false;
> +  if (aWhichClipboard != kGlobalClipboard)
> +    return NS_ERROR_NOT_IMPLEMENTED;

nit: if() { .... } even for single line blocks.
Attachment #8342992 - Flags: feedback?(fabrice) → feedback+
Created attachment 8343656 [details] [diff] [review]
clipboard_b2g.patch v2

Updated patch, makes sense now?

Fabrice, can you give me some pointers on enabling tests for clipboard on b2g?
Attachment #8342992 - Attachment is obsolete: true
Attachment #8343656 - Flags: review+
Attachment #8343656 - Flags: feedback?(xyuan)
Flags: needinfo?(fabrice)
Attachment #8343656 - Flags: feedback?(xyuan) → feedback+
I made a search and it seems there is no existing test available, but you can create one under widget/tests like this
http://mxr.mozilla.org/mozilla-central/source/widget/tests/test_bug565392.html?force=1
Yep, I'm surprised that we don't have more tests already, but that can't hurt to make sure we run the one Yuan pointed to on b2g.
Flags: needinfo?(fabrice)
Duplicate of this bug: 938040

Updated

4 years ago
Blocks: 747798
So I checked this out and gerard_majax thought we run the tests here against B2G desktop and not against gonk, which makes sense because I think we have some tests regarding clipboard already enabled for all platforms.

The problem is that in b2g desktop we use the OS clipboard (you can copy something in OSX and then paste it in B2G desktop) and not the one from /gonk. So I don't know how to test this...

Any idea?
Flags: needinfo?(xyuan)
Flags: needinfo?(fabrice)
We can test it on phone device or try server.
Flags: needinfo?(xyuan)
mochitests run on ICS emulator, which use gonk. If the clipboard ones are disabled there, you should start by re-enabling and doing an emulator try run.
Flags: needinfo?(fabrice)

Comment 11

4 years ago
Comment on attachment 8343656 [details] [diff] [review]
clipboard_b2g.patch v2

This is assuming that the clipboard only accepts plain text, which is not correct.  The clipboard implementation needs to be able to handle multiple mime types and should not make any assumptions about what mime types are available in either the GetData or SetData functions.
Attachment #8343656 - Flags: review-

Comment 12

4 years ago
Note that it seems like the Android keyboard code is broken in similar ways.  Not sure why that's the case.
I'm struggling with this, so let's say I write the SetData method like this:

NS_IMETHODIMP
nsClipboard::SetData(nsITransferable *aTransferable,
                     nsIClipboardOwner *anOwner, int32_t aWhichClipboard)
{
  if (aWhichClipboard != kGlobalClipboard) {
    return NS_ERROR_NOT_IMPLEMENTED;
  }
  
  char* mime;
  nsCOMPtr<nsISupports> data;
  uint32_t length;
  nsresult rv = aTransferable->GetAnyTransferData(&mime, getter_AddRefs(data), &length);
  LOG("====== Got mime %s %d\n", mime, length);
  if (NS_WARN_IF(NS_FAILED(rv))) {
    return rv;
  }

  if (XRE_GetProcessType() == GeckoProcessType_Default) {
    mMime = mime;
    mData = data;
    mLength = length;
  } else {
    // This is important!
    LOGW("PROCESSTYPE is not default");
  }

  return NS_OK;
}

Then it works but only within one process. In Android we do some forwarding via: 

    ContentChild::GetSingleton()->SendSetClipboardText(buffer, isPrivateData,
                                                       aWhichClipboard);

I don't get:

* Where the definition of this function is, it gets autogenerated in the ipc/ directory, but based on what info no clue...
* How |SetClipboardText| maps back to a real function...
Flags: needinfo?(ehsan)
Hmm, so I tracked it down to PContent.ipdl..
Alright, I give up. I can't make a nice IPDL binding.

Comment 17

4 years ago
The "SetClipboardText" message and friends are declared here: <http://mxr.mozilla.org/mozilla-central/source/dom/ipc/PContent.ipdl#431>  The IPDL compiler generates C++ code which sends and receives these messages.  If you're curious about the generated code, you can find it in objdir/ipc/ipdl/PContent{Parent,Child}.cpp.

For the purposes of this bug, you should probably add new IPDL messages for transferring other types of clipboard data between the child and parent process.

Also, reading the implementation of ContentParent::RecvSetClipboardText() for example, it seems like that function is not actually implementing a clipboard, it's just creating the clipboard widget service in the parent process and asks it to deal with the request.  So, that won't work for gonk anyways.  We need to actually implement a clipboard which lives somewhere in the parent process.
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #17)
> Also, reading the implementation of ContentParent::RecvSetClipboardText()
> for example, it seems like that function is not actually implementing a
> clipboard, it's just creating the clipboard widget service in the parent
> process and asks it to deal with the request.  So, that won't work for gonk
> anyways.  We need to actually implement a clipboard which lives somewhere in
> the parent process.
From what I understand is that every process has it's own clipboard service, and if we're a client process it will IPC it up to the parent process and that holds the actual value. That is also what I see on the device, because with the IPC enabled the clipboard works nice between all different apps.

I'm wondering though how I would serialize the nsITransferable / nsISupports. Any clue?
Flags: needinfo?(ehsan)

Comment 19

4 years ago
(In reply to Jan Jongboom [:janjongboom] from comment #18)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #17)
> > Also, reading the implementation of ContentParent::RecvSetClipboardText()
> > for example, it seems like that function is not actually implementing a
> > clipboard, it's just creating the clipboard widget service in the parent
> > process and asks it to deal with the request.  So, that won't work for gonk
> > anyways.  We need to actually implement a clipboard which lives somewhere in
> > the parent process.
> From what I understand is that every process has it's own clipboard service,
> and if we're a client process it will IPC it up to the parent process and
> that holds the actual value. That is also what I see on the device, because
> with the IPC enabled the clipboard works nice between all different apps.

Yes, I think that's the basic architecture that we want.  Please note that the clipboard service is just a Gecko component which _usually_ talks to the OS provided clipboard APIs.  It's just that on b2g those APIs do not exist, and therefore we need to manage the clipboard ourselves.  One key requirement is that each app should have access to the same clipboard, so the parent process is the most natural place to keep that data.

> I'm wondering though how I would serialize the nsITransferable /
> nsISupports. Any clue?

Each nsITransferable basically holds a key/value map for different data flavors.  Each flavor has a mimetype, and the type of the data associated with that mimetype depends on the mimetype itself.  For example, for "text/plain" the data type is an nsISupportsCString, for "text/unicode" it's an nsISupportsString and so on.  The list of mimetypes which we currently support can be found at <http://mxr.mozilla.org/mozilla-central/source/widget/nsITransferable.idl#14>.

So in order to implement this serializartion, you need to roll out your own code which does the serialization depending on the flavor.  So, you would first call FlavorsTransferableCanExport to get a list of the flavors that the transferable provides, and then serialize the data that the transferable has for that flavor based on that.  You can look at our existing nsClipboard implementations for examples of how we do this on other platforms.
Flags: needinfo?(ehsan)
Can we land the current patch with just text support, and leave the bug open to add all mime types?
Flags: needinfo?(ehsan)

Comment 21

4 years ago
(In reply to Jan Jongboom [:janjongboom] from comment #20)
> Can we land the current patch with just text support, and leave the bug open
> to add all mime types?

Sure, please file a follow-up bug though.  Also, please modify the patch to log something useful when we encounter a non-plaintext mime type so that people don't wonder why the clipboard service works only some of the time.

Thanks!
Flags: needinfo?(ehsan)
Summary: Implement nsClipboard for B2G → Implement nsClipboard with text support only for B2G
(Assignee)

Updated

4 years ago
Blocks: 952456
Follow up in bug 952456. Added an error message as well.
Created attachment 8350585 [details] [diff] [review]
946646_v3.patch

Try (including tests from bug 948065): https://tbpl.mozilla.org/?tree=Try&rev=8ae806f502b4
Attachment #8343656 - Attachment is obsolete: true
Attachment #8350585 - Flags: review+
Let's try again... Don't know if the previous try run was OK.. https://tbpl.mozilla.org/?tree=Try&rev=a69681b36fa0
So the try seems fine now, let's do a full try run: https://tbpl.mozilla.org/?tree=Try&rev=54f614f896b0
So try run failed on OSX because the clipboard test didn't work there. Ah well, that's another bug for that test anyway. So here is the try run without the test: https://tbpl.mozilla.org/?tree=Try&rev=0ec307c25dce

Comment 27

4 years ago
Your test assumes that the clipboard is synchronous.  I suspect that's what's wrong with it.  Since bug 948065 is closed, please either reopen that one or fix the test and land it together with this patch.
Thanks Ehsan, I reopened the bug. Will look into it next week.
Sheriff, the try is green except for some intermittents and https://tbpl.mozilla.org/php/getParsedLog.php?id=32519714&tree=Try. I don't know if the last one is also some kind of intermittent error. If so, please land. Otherwise, please don't :p
Keywords: checkin-needed
I retriggered the failed tests to have a try :-)
Unfortunately, the mochitest-9 failures on the Try push were real. They showed up on b-i as well. Backed out.
https://hg.mozilla.org/integration/b2g-inbound/rev/ae5d2e7db377

https://tbpl.mozilla.org/php/getParsedLog.php?id=32584569&tree=B2g-Inbound
Any clue?
Flags: needinfo?(xyuan)
The following mochitest failed on B2G emulator:

source/layout/forms/test/test_bug549170.html

http://mxr.mozilla.org/mozilla-central/source/layout/forms/test/test_bug549170.html?force=1#58

This test will paste the text from the clipboard.

On B2G emulator, it seems the test is running out of process, and the clipboard service is called through IPC.

There is some problem with the IPC part and makes the test fail.
Flags: needinfo?(xyuan)
This is the error throw from test_bug549170.html:

09:11:36     INFO -  [Parent 663] WARNING: pipe error (65): Connection reset by peer: file ../../../gecko/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 446
09:11:36     INFO -  ************************************************************
09:11:36     INFO -  * Call to xpconnect wrapped JSObject produced this error:  *
09:11:36     INFO -  [Exception... "Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIMessageSender.sendAsyncMessage]"  nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame :: chrome://specialpowers/content/SpecialPowersObserver.js :: SpecialPowersObserver.prototype._sendAsyncMessage :: line 89"  data: no]
09:11:36     INFO -  ************************************************************
Hmm, I can do IPC copy/paste on the device. I'll investigate tomorrow.
(In reply to Yuan Xulei [:yxl] from comment #35)
> This is the error throw from test_bug549170.html:
> 
> 09:11:36     INFO -  [Parent 663] WARNING: pipe error (65): Connection reset
> by peer: file
> ../../../gecko/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 446

From Bug 946720 Comment 37, it seems that the above error could happen from an application's main thread abort.

Comment 38

4 years ago
Hi guys, is there any update on this?
The ticket blocks a focus area item bug#747798 Copy'n'paste support for v2.0
Hmm, not looked into it. It worked previously, only test failure. If there's someone with time on their hands should be easy to land.

Comment 40

4 years ago
After rebase the patch, it passes the test cases on try server. 
https://tbpl.mozilla.org/?tree=Try&rev=02ee72de2d5e

Comment 41

4 years ago
Jan,
Could you rebase your patch?

Updated

4 years ago
Flags: needinfo?(janjongboom)
Created attachment 8404606 [details] [diff] [review]
bug946646-rebased.patch

Rebased patch that was submitted to Try.
Attachment #8350585 - Attachment is obsolete: true
Flags: needinfo?(janjongboom)
(Assignee)

Updated

4 years ago
Attachment #8404606 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/65114a1d9011
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.5 S1 (9may)

Updated

4 years ago
feature-b2g: --- → 2.0

Updated

4 years ago
No longer blocks: 747798

Updated

4 years ago
Blocks: 1029465
Depends on: 1194497
You need to log in before you can comment on or make changes to this bug.