946646 - Implement nsClipboard with text support only for B2G

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

Description

[Jan Jongboom [:janjongboom] (inactive)]

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.

Comment 1

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: 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. :-)

Comment 2

[Yuan Xulei [:yxl] (Leave from May 18)]

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.

Comment 3

[[:fabrice] Fabrice Desré]

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.

Comment 4

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: clipboard_b2g.patch v2

Updated patch, makes sense now?

Fabrice, can you give me some pointers on enabling tests for clipboard on b2g?

Comment 5

[Yuan Xulei [:yxl] (Leave from May 18)]

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

Comment 6

[[:fabrice] Fabrice Desré]

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.

Comment 8

[Jan Jongboom [:janjongboom] (inactive)]

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?

Comment 9

[Yuan Xulei [:yxl] (Leave from May 18)]

We can test it on phone device or try server.

Comment 10

[[:fabrice] Fabrice Desré]

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.

Comment 11

[(no longer active)]

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.

Comment 12

[(no longer active)]

Note that it seems like the Android keyboard code is broken in similar ways.  Not sure why that's the case.

Comment 13

[Yuan Xulei [:yxl] (Leave from May 18)]

Found some tests for this:

http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_copypaste.xhtml
http://mxr.mozilla.org/mozilla-central/source/content/base/test/copypaste.js

Comment 14

[Jan Jongboom [:janjongboom] (inactive)]

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...

Comment 15

[Jan Jongboom [:janjongboom] (inactive)]

Hmm, so I tracked it down to PContent.ipdl..

Comment 16

[Jan Jongboom [:janjongboom] (inactive)]

Alright, I give up. I can't make a nice IPDL binding.

Comment 17

[(no longer active)]

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.

Comment 18

[Jan Jongboom [:janjongboom] (inactive)]

(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?

Comment 19

[(no longer active)]

(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.

Comment 20

[Jan Jongboom [:janjongboom] (inactive)]

Can we land the current patch with just text support, and leave the bug open to add all mime types?

Comment 21

[(no longer active)]

(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!

Comment 22

[Jan Jongboom [:janjongboom] (inactive)]

Follow up in bug 952456. Added an error message as well.

Comment 23

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: 946646_v3.patch

Try (including tests from bug 948065): https://tbpl.mozilla.org/?tree=Try&rev=8ae806f502b4

Comment 24

[Jan Jongboom [:janjongboom] (inactive)]

Let's try again... Don't know if the previous try run was OK.. https://tbpl.mozilla.org/?tree=Try&rev=a69681b36fa0

Comment 25

[Jan Jongboom [:janjongboom] (inactive)]

So the try seems fine now, let's do a full try run: https://tbpl.mozilla.org/?tree=Try&rev=54f614f896b0

Comment 26

[Jan Jongboom [:janjongboom] (inactive)]

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

[(no longer active)]

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.

Comment 28

[Jan Jongboom [:janjongboom] (inactive)]

Thanks Ehsan, I reopened the bug. Will look into it next week.

Comment 29

[Jan Jongboom [:janjongboom] (inactive)]

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

Comment 30

[Yuan Xulei [:yxl] (Leave from May 18)]

I retriggered the failed tests to have a try :-)

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/d0b883c071e0

Comment 32

[Ryan VanderMeulen [:RyanVM]]

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

Comment 33

[Jan Jongboom [:janjongboom] (inactive)]

Any clue?

Comment 34

[Yuan Xulei [:yxl] (Leave from May 18)]

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.

Comment 35

[Yuan Xulei [:yxl] (Leave from May 18)]

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 -  ************************************************************

Comment 36

[Jan Jongboom [:janjongboom] (inactive)]

Hmm, I can do IPC copy/paste on the device. I'll investigate tomorrow.

Comment 37

[Sotaro Ikeda [:sotaro]]

(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

[howie [:howie]]

Hi guys, is there any update on this?
The ticket blocks a focus area item bug#747798 Copy'n'paste support for v2.0

Comment 39

[Jan Jongboom [:janjongboom] (inactive)]

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

[StevenLee[:slee]]

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

Comment 41

[StevenLee[:slee]]

Jan,
Could you rebase your patch?

Comment 42

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: bug946646-rebased.patch

Rebased patch that was submitted to Try.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/65114a1d9011

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/65114a1d9011