573735 - support waitForClipboard events in e10s

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 1

[Martijn Wargers (dead)]

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

Comment 2

[Martijn Wargers (dead)]

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

Comment 3

[Martijn Wargers (dead)]

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.

Comment 4

[Martijn Wargers (dead)]

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?

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: port waitForClipboard to SpecialPowers (WIP)

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.

Comment 6

[Josh Matthews [:jdm]]

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?

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: port waitForClipboard to SpecialPowers (1.0)

updated with function name change.

Comment 9

[Josh Matthews [:jdm]]

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?

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

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 11

[(not currently active) Ted Mielczarek]

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?

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 13

[(not currently active) Ted Mielczarek]

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.

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

actually this patch allows waitForClipboard to work in fennec (e10s).  So we can close this bug when this patch lands.

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c8f6b1f00b8

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7c8f6b1f00b8