769882 - Futureproof clipboard.copyString() by adding second parameter.

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Edmund Wong (:ewong)]

+++ This bug was initially created as a clone of Bug #769603 +++

>Although the new second parameter to clipboard.copyString() is optional we >should fix this as well to future proof our code.

>http://mxr.mozilla.org/comm-central/search?string=copyString%28&find=%2Fsuite%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

Comment 1

[Edmund Wong (:ewong)]

Attachment: Added second parameter to future proof clipboard.copyString(). (v1)

Comment 2

[Philip Chee]

Hmm. Any reason you use this.ownerDocument sometimes?

> -    CopyString(emailAddressNode.getAttribute("emailAddress"));
> +    CopyString(emailAddressNode.getAttribute("emailAddress"), document);
...
> -    CopyString(url);
> +    CopyString(url, document);

But then you ignore the second parameter in:
>  function CopyString(aString)
>  {
>    Components.classes["@mozilla.org/widget/clipboardhelper;1"]
>              .getService(Components.interfaces.nsIClipboardHelper)
> -            .copyString(aString);
> +            .copyString(aString, document);

Comment 3

[Edmund Wong (:ewong)]

(In reply to Philip Chee from comment #2)
> Hmm. Any reason you use this.ownerDocument sometimes?

I thought those needed 'this.ownerDocument'.  In hindsight, I think
document was fine.

> 
> > -    CopyString(emailAddressNode.getAttribute("emailAddress"));
> > +    CopyString(emailAddressNode.getAttribute("emailAddress"), document);
> ...
> > -    CopyString(url);
> > +    CopyString(url, document);
> 
> But then you ignore the second parameter in:

Error on my part.  I blindly added the 'document' parameter to CopyString
thinking it was copyString().   

Will fix that.

Comment 4

[[e-mail]]

Comment on attachment 638082 [details] [diff] [review]
Added second parameter to future proof clipboard.copyString(). (v1)

Your uses of ownerDocument appear to be entirely incorrect. You could possibly use them in .xml files, although I'm not always sure we bother, but you don't want to use it otherwise.

Currently we use the chrome document everywhere, this doesn't affect us at the moment because we don't support private browsing yet, but there are some places where I am concerned that we should be using the content document.

Comment 5

[Edmund Wong (:ewong)]

Attachment: Futureproof clipboard.copyString() by adding second parameter.(v2)

Comment 6

[[e-mail]]

Comment on attachment 638170 [details] [diff] [review]
Futureproof clipboard.copyString() by adding second parameter.(v2)

>+    clipboard.copyString(this.mediaURL, this.ownerDocument);
Oops ;-)
[I still don't have an update as to whether nsContextMenu.js should use document or this.target.ownerDocument]

Comment 7

[Edmund Wong (:ewong)]

Attachment: Futureproof clipboard.copyString() by adding second parameter.(v3)

Comment 8

[[e-mail]]

Comment on attachment 638251 [details] [diff] [review]
Futureproof clipboard.copyString() by adding second parameter.(v3)

OK, so clarification from ehsan is that we should use the document that the string corresponds to, if any.

>diff --git a/suite/browser/pageinfo/pageInfo.js b/suite/browser/pageinfo/pageInfo.js
>-  gClipboardHelper.copyString(text.join("\n"));
>+  gClipboardHelper.copyString(text.join("\n"), document);
So this needs to be the page's document (gDocument)...

>diff --git a/suite/common/nsContextMenu.js b/suite/common/nsContextMenu.js
and both of these need to use the content document (this.target.ownerDocument).

r=me with those three fixed.

Comment 9

[Edmund Wong (:ewong)]

Attachment: Futureproof clipboard.copyString() by adding second parameter.(v4)

Comment 10

[Edmund Wong (:ewong)]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/3b54d4b9a065

Comment 11

[(no longer active)]

Thanks a lot, Edmund, for taking care of this.  Highly appreciated.  :-)