769603 - Call the Init method after creating nsITransferable. (Port bug 722872 to SeaMonkey)

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Philip Chee]

Bug 722872 - Part 1: Add nsITransferable::Init(nsILoadContext*), enforce that it's called in debug builds, and add nsIDOMDocument* arguments to nsIClipboardHelper methods.

From MailNews Core Bug 769588:
> I have already done a minor port here to fix bustage:
> 
> http://hg.mozilla.org/comm-central/rev/4e4cd1fb04c3
> 
> We need to fix the rest of the instances in mailnews and mail
> 
> http://mxr.mozilla.org/comm-central/search?string=nsITransferable&find=mail&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central
> 
> Should be reasonable simple to do, attachment 636714 [details] [diff] [review]
> has plenty of examples as well.

For Suite the query is:
http://mxr.mozilla.org/comm-central/search?string=nsITransferable&find=%2Fsuite%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

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

[(no longer active)]

Sorry guys, should've known that this will break comm-central...

Comment 2

[Justin Wood (:Callek)]

Also of note is the bustage in chatzilla and inspector:

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

Comment 3

[Edmund Wong (:ewong)]

Attachment: Call init after creating nsITransferable. (v1)

Comment 4

[[e-mail]]

Comment on attachment 638079 [details] [diff] [review]
Call init after creating nsITransferable. (v1)

There appear to be three transferables in controller.js ...

Comment 5

[Edmund Wong (:ewong)]

Attachment: Call init method after creating nsITransferable. (v2)

Comment 6

[[e-mail]]

Comment on attachment 638171 [details] [diff] [review]
Call init method after creating nsITransferable. (v2)

OK, so I've been studying the core patch and it looks like we don't actually need to pass a load context most of the time. But I guess I should ask ehsan exactly when a load context is needed.

>+    trans.init(window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                     .getInterface(Components.interfaces.nsIWebNavigation)
>+                     .QueryInterface(Components.interfaces.nsILoadContext));
>     trans.addDataFlavor("text/unicode");
>     // If available, use selection clipboard, otherwise global one
>     if (clipboard.supportsSelectionClipboard())
>       clipboard.getData(trans, clipboard.kSelectionClipboard);
>     else
>       clipboard.getData(trans, clipboard.kGlobalClipboard);
Core doesn't seem to need to pass a load context when getting data.

>diff --git a/suite/common/downloads/tests/chrome/test_multi_select.xul b/suite/common/downloads/tests/chrome/test_multi_select.xul
Core seems to pass one for tests.

>diff --git a/suite/common/history/controller.js b/suite/common/history/controller.js
Core doesn't seem to pass one for non-browser windows.

Comment 7

[(no longer active)]

(In reply to comment #6)
> Comment on attachment 638171 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=638171
> Call init method after creating nsITransferable. (v2)
> 
> OK, so I've been studying the core patch and it looks like we don't actually
> need to pass a load context most of the time. But I guess I should ask ehsan
> exactly when a load context is needed.

The load context is only needed when setting the data on a transferable object which will be used to set clipboard data.  Transferable objects created to read information from the clipboard or to deal with things like drag/drop do not need a load context (although it would be nice if the caller could pass them in when available, as I don't generally expect the callers to have to know about this distinction.)

Comment 8

[[e-mail]]

(In reply to Ehsan Akhgari from comment #7)
> (In reply to comment #6)
> > OK, so I've been studying the core patch and it looks like we don't actually
> > need to pass a load context most of the time. But I guess I should ask ehsan
> > exactly when a load context is needed.
> The load context is only needed when setting the data on a transferable
> object which will be used to set clipboard data.
Ah, well we don't ever do this, so we might as well pass null everywhere.

Comment 9

[[e-mail]]

Comment on attachment 638171 [details] [diff] [review]
Call init method after creating nsITransferable. (v2)

As per comment #8, just use init(null) everywhere please.

Comment 10

[Edmund Wong (:ewong)]

Attachment: Call init after creating nsITransferable. (v3)

Comment 11

[[e-mail]]

Comment on attachment 638947 [details] [diff] [review]
Call init after creating nsITransferable. (v3)

>diff --git a/suite/common/history/controller.js b/suite/common/history/controller.js
>       function addData(type, data) {
>+        xferable.init(null);
Oops, this one is in the wrong place, it goes after the createInstance like the others do. r=me with that fixed.

Comment 12

[Edmund Wong (:ewong)]

Attachment: Call init method after creating nsITransferable. (v4)

Comment 13

[Edmund Wong (:ewong)]

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

Comment 14

[Serge Gautherie (:sgautherie)]

V.Fixed, per bug 770687 comment 2.

Comment 15

[Serge Gautherie (:sgautherie)]

Ftr, what about
/suite/common/downloads/downloadmanager.js
    * line 373 -- .createInstance(Components.interfaces.nsITransferable);
which was not fixed?

Comment 16

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to Justin Wood (:Callek) from comment #2)
> Also of note is the bustage in chatzilla and inspector:
> 
> http://mxr.mozilla.org/comm-central/
> search?string=nsITransferable&find=%2Fextensions%2F&findi=&filter=^[^\0]*%24&
> hitlimit=&tree=comm-central

Those inspector hits are in dead code.

Comment 17

[[e-mail]]

(In reply to Serge Gautherie from comment #15)
> Ftr, what about
> /suite/common/downloads/downloadmanager.js
>     * line 373 -- .createInstance(Components.interfaces.nsITransferable);
> which was not fixed?

Oversight?

Comment 18

[Philip Chee]

>> which was not fixed?
> Oversight?

Comment 19

[Edmund Wong (:ewong)]

Attachment: Call init after creating nsITransferable for suite/downloads/downloadmanager.js (v1)

Comment 20

[[e-mail]]

Comment on attachment 667301 [details] [diff] [review]
Call init after creating nsITransferable for suite/downloads/downloadmanager.js (v1)

> function handlePaste() {
>   let trans = Components.classes["@mozilla.org/widget/transferable;1"]
>                         .createInstance(Components.interfaces.nsITransferable);
>   let flavors = ["text/x-moz-url", "text/unicode"];
>+  trans.init(null);
>   flavors.forEach(trans.addDataFlavor);
This will work, but it looks really odd to put the init on this particular line. It would probably look best something like this:

let trans = Components.classes["@mozilla.org/widget/transferable;1"]
                      .createInstance(Components.interfaces.nsITransferable);
trans.init(null);

let flavors = ["text/x-moz-url", "text/unicode"];
flavors.forEach(trans.addDataFlavor);

r=me with that nit fixed.

Comment 21

[Edmund Wong (:ewong)]

Attachment: Call init after creating nsITransferable for suite/downloads/downloadmanager.js (v2)

Fixed nit.

Comment 22

[Edmund Wong (:ewong)]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/5adf56e7b7ce