Futureproof clipboard.copyString() by adding second parameter.

RESOLVED FIXED

Status

SeaMonkey
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ewong, Assigned: ewong)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
+++ 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
(Assignee)

Updated

5 years ago
Summary: Call the Init method after creating nsITransferable ( Port bug 722872) → Futureproof clipboard.copyString() by adding second parameter.
(Assignee)

Comment 1

5 years ago
Created attachment 638082 [details] [diff] [review]
Added second parameter to future proof clipboard.copyString(). (v1)
Attachment #638082 - Flags: review?(neil)

Comment 2

5 years ago
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);
(Assignee)

Comment 3

5 years ago
(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

5 years ago
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-
(Assignee)

Comment 5

5 years ago
Created attachment 638170 [details] [diff] [review]
Futureproof clipboard.copyString() by adding second parameter.(v2)
Attachment #638082 - Attachment is obsolete: true
Attachment #638170 - Flags: review?(neil)

Comment 6

5 years ago
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]
(Assignee)

Comment 7

5 years ago
Created attachment 638251 [details] [diff] [review]
Futureproof clipboard.copyString() by adding second parameter.(v3)
Attachment #638170 - Attachment is obsolete: true
Attachment #638170 - Flags: review?(neil)
Attachment #638251 - Flags: review?(neil)

Comment 8

5 years ago
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+
(Assignee)

Comment 9

5 years ago
Created attachment 638942 [details] [diff] [review]
Futureproof clipboard.copyString() by adding second parameter.(v4)
Attachment #638251 - Attachment is obsolete: true
Attachment #638942 - Flags: review+
(Assignee)

Comment 10

5 years ago
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/3b54d4b9a065
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 11

5 years ago
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.