Closed Bug 769882 Opened 8 years ago Closed 8 years ago

Futureproof clipboard.copyString() by adding second parameter.

Categories

(SeaMonkey :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ewong, Assigned: ewong)

References

Details

Attachments

(1 file, 3 obsolete files)

+++ 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
Summary: Call the Init method after creating nsITransferable ( Port bug 722872) → Futureproof clipboard.copyString() by adding second parameter.
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);
(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 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.
Attachment #638082 - Flags: review?(neil) → review-
Attachment #638082 - Attachment is obsolete: true
Attachment #638170 - Flags: review?(neil)
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]
Attachment #638170 - Attachment is obsolete: true
Attachment #638170 - Flags: review?(neil)
Attachment #638251 - Flags: review?(neil)
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.
Attachment #638251 - Flags: review?(neil) → review+
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/3b54d4b9a065
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Thanks a lot, Edmund, for taking care of this.  Highly appreciated.  :-)
You need to log in before you can comment on or make changes to this bug.