Closed
Bug 946646
Opened 11 years ago
Closed 10 years ago
Implement nsClipboard with text support only for B2G
Categories
(Firefox OS Graveyard :: Gaia::Keyboard, defect)
Tracking
(feature-b2g:2.0)
People
(Reporter: janjongboom, Assigned: janjongboom)
References
Details
Attachments
(1 file, 3 obsolete files)
9.22 KB,
patch
|
janjongboom
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•11 years ago
|
||
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 2•11 years ago
|
||
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 3•11 years ago
|
||
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+
Assignee | ||
Comment 4•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #8343656 -
Flags: feedback?(xyuan) → feedback+
Comment 5•11 years ago
|
||
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•11 years ago
|
||
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)
Assignee | ||
Comment 8•11 years ago
|
||
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)
Comment 10•11 years ago
|
||
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•11 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•11 years ago
|
||
Note that it seems like the Android keyboard code is broken in similar ways. Not sure why that's the case.
Comment 13•11 years ago
|
||
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
Assignee | ||
Comment 14•11 years ago
|
||
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)
Assignee | ||
Comment 15•11 years ago
|
||
Hmm, so I tracked it down to PContent.ipdl..
Assignee | ||
Comment 16•11 years ago
|
||
Alright, I give up. I can't make a nice IPDL binding.
Comment 17•11 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)
Assignee | ||
Comment 18•11 years ago
|
||
(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•11 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)
Assignee | ||
Comment 20•11 years ago
|
||
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•11 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 | ||
Comment 22•11 years ago
|
||
Follow up in bug 952456. Added an error message as well.
Assignee | ||
Comment 23•11 years ago
|
||
Try (including tests from bug 948065): https://tbpl.mozilla.org/?tree=Try&rev=8ae806f502b4
Attachment #8343656 -
Attachment is obsolete: true
Attachment #8350585 -
Flags: review+
Assignee | ||
Comment 24•10 years ago
|
||
Let's try again... Don't know if the previous try run was OK.. https://tbpl.mozilla.org/?tree=Try&rev=a69681b36fa0
Assignee | ||
Comment 25•10 years ago
|
||
So the try seems fine now, let's do a full try run: https://tbpl.mozilla.org/?tree=Try&rev=54f614f896b0
Assignee | ||
Comment 26•10 years ago
|
||
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•10 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.
Assignee | ||
Comment 28•10 years ago
|
||
Thanks Ehsan, I reopened the bug. Will look into it next week.
Assignee | ||
Comment 29•10 years ago
|
||
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
Comment 30•10 years ago
|
||
I retriggered the failed tests to have a try :-)
Comment 31•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/d0b883c071e0
Keywords: checkin-needed
Comment 32•10 years ago
|
||
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 34•10 years ago
|
||
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)
Comment 35•10 years ago
|
||
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 - ************************************************************
Assignee | ||
Comment 36•10 years ago
|
||
Hmm, I can do IPC copy/paste on the device. I'll investigate tomorrow.
Comment 37•10 years ago
|
||
(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•10 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
Assignee | ||
Comment 39•10 years ago
|
||
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•10 years ago
|
||
After rebase the patch, it passes the test cases on try server. https://tbpl.mozilla.org/?tree=Try&rev=02ee72de2d5e
Comment 41•10 years ago
|
||
Jan, Could you rebase your patch?
Updated•10 years ago
|
Flags: needinfo?(janjongboom)
Assignee | ||
Comment 42•10 years ago
|
||
Rebased patch that was submitted to Try.
Attachment #8350585 -
Attachment is obsolete: true
Flags: needinfo?(janjongboom)
Assignee | ||
Updated•10 years ago
|
Attachment #8404606 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 43•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/65114a1d9011
Keywords: checkin-needed
Comment 44•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/65114a1d9011
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.5 S1 (9may)
Updated•10 years ago
|
feature-b2g: --- → 2.0
You need to log in
before you can comment on or make changes to this bug.
Description
•