Closed Bug 769603 Opened 8 years ago Closed 7 years ago

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

Categories

(SeaMonkey :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.13

People

(Reporter: philip.chee, Assigned: ewong)

References

Details

Attachments

(2 files, 4 obsolete files)

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
Sorry guys, should've known that this will break comm-central...
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Blocks: 769882
Attachment #638079 - Flags: review?(neil)
Comment on attachment 638079 [details] [diff] [review]
Call init after creating nsITransferable. (v1)

There appear to be three transferables in controller.js ...
Attachment #638079 - Attachment is obsolete: true
Attachment #638079 - Flags: review?(neil)
Attachment #638171 - Flags: review?(neil)
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.
(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.)
(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 on attachment 638171 [details] [diff] [review]
Call init method after creating nsITransferable. (v2)

As per comment #8, just use init(null) everywhere please.
Attachment #638171 - Flags: review?(neil) → review-
Attachment #638171 - Attachment is obsolete: true
Attachment #638947 - Flags: review?(neil)
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.
Attachment #638947 - Flags: review?(neil) → review+
Attachment #638947 - Attachment is obsolete: true
Attachment #639087 - Flags: review?(neil)
Attachment #639087 - Flags: review?(neil) → review+
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/d13f980cee1e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
V.Fixed, per bug 770687 comment 2.
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Summary: Call the Init method after creating nsITransferable ( Port bug 722872) → Call the Init method after creating nsITransferable. (Port bug 722872 to SeaMonkey)
Target Milestone: --- → seamonkey2.13
Ftr, what about
/suite/common/downloads/downloadmanager.js
    * line 373 -- .createInstance(Components.interfaces.nsITransferable);
which was not fixed?
(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.
(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?
>> which was not fixed?
> Oversight?
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
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.
Attachment #667301 - Flags: review?(neil) → review+
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/5adf56e7b7ce
Status: ASSIGNED → RESOLVED
Closed: 8 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.