Closed Bug 573735 Opened 10 years ago Closed 8 years ago

support waitForClipboard events in e10s

Categories

(Testing :: Mochitest, defect)

x86
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla10

People

(Reporter: jmaher, Assigned: jmaher)

References

Details

(Whiteboard: [specialpowers][inbound])

Attachments

(1 file, 1 obsolete file)

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#374

We need to fill in the code that handles this when running in IPC mode.
Tests that are probably failing because of this are:
/editor/libeditor/html/tests/test_bug599322.html 
/editor/libeditor/html/tests/test_bug551704.html 
/editor/libeditor/html/tests/test_bug410986.html 
/editor/libeditor/text/tests/test_bug596001.html 
/editor/libeditor/text/tests/test_bug600570.html
Joel, do you know how this code should be added in SimpleTest.js? I thought it might be similar to the focus code, but I can't make sens of that (does that code even work?)
Probably, also editor/libeditor/html/tests/test_bug478725.html is suffering from something similar, although in this case, the doc.execCommand("copy", false, null); call doesn't seem to work.
Actually, the tests I mentioned in comment 1 might just work now in Maemo and Android. (they don't work on Windows Fennec, ctrl-c doesn't work there).
Wasn't this basically fixed by bug 590349?
Blocks: 682858
ok, this wasn't as hard as I thought it would be.  This works locally for me and doesn't seem hacky at all.  Going to run it through try and wait for some initial feedback before asking for formal review.
Attachment #565324 - Flags: feedback?(josh)
Whiteboard: [specialpowers]
Comment on attachment 565324 [details] [diff] [review]
port waitForClipboard to SpecialPowers (WIP)

s/get_xferData/getTransferData/, but otherwise it's looking good. Is the plan to get e10s support working in this bug or another one?
Attachment #565324 - Flags: feedback?(josh) → feedback+
Josh, thanks for the feedback.  With this patch, we support specialpowers, so this is half the battle.  I think we will get this in first and then work on any further actions items in this bug.
updated with function name change.
Assignee: nobody → jmaher
Attachment #565324 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #565917 - Flags: review?(josh)
Comment on attachment 565917 [details] [diff] [review]
port waitForClipboard to SpecialPowers (1.0)

I see nothing here that offends me, but ted should probably look at it too. One question though - what's changed that you're able to remove the |aData &&| from the checks in various places?
Attachment #565917 - Flags: review?(ted.mielczarek)
Attachment #565917 - Flags: review?(josh)
Attachment #565917 - Flags: review+
for the aData stuff, I now have clipboardCopyString() which returns a String ('' if None).  This way I ensure that I am always comparing a string.
Comment on attachment 565917 [details] [diff] [review]
port waitForClipboard to SpecialPowers (1.0)

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

Looks good, just a few nits.

Also, please remember to document your API additions on MDN:
https://developer.mozilla.org/en/SpecialPowers

::: testing/mochitest/tests/SimpleTest/SimpleTest.js
@@ +602,5 @@
>                                         aSuccessFn, aFailureFn, aFlavor) {
>      if (ipcMode) {
>        //TODO: support waitForClipboard via events to chrome
>        dump("E10S_TODO: bug 573735 addresses adding support for this");
>        return;

This comment is outdated now, right? Just remove it.

@@ +626,5 @@
>              failureFn();
>              return;
>          }
>  
> +        data = SpecialPowers.getTransferData(flavor);

Can we call this "getClipboardData"? I know this is internal clipboard terminology, but it's not very clear.

::: testing/mochitest/tests/SimpleTest/specialpowersAPI.js
@@ +644,5 @@
> +    this._cb = Components.classes["@mozilla.org/widget/clipboard;1"].
> +                          getService(Components.interfaces.nsIClipboard);
> +
> +    return this._cb;
> +  },

Does it make sense to expose this as a public API? Content pages won't be able to do anything useful with the resulting clipboard object, right?

@@ +671,1 @@
>  };

This all works fine from a content process? We don't need to do any of this in the chrome process?
Attachment #565917 - Flags: review?(ted.mielczarek) → review+
 - good catch on the leftover ipc code.
 - yes getTransferData -> getClipboardData is good.
 - exposing this._cb, no need that I know of to make it public, I will change that
 - we need the same api for all our tests;  Not sure what the question is getting at about needing to do this in chrome process or not.
It was an honest question. If you run these tests in Fennec do they pass now? I really don't know how the clipboard code functions in e10s.
actually this patch allows waitForClipboard to work in fennec (e10s).  So we can close this bug when this patch lands.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c8f6b1f00b8
Whiteboard: [specialpowers] → [specialpowers][inbound]
https://hg.mozilla.org/mozilla-central/rev/7c8f6b1f00b8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.