Closed Bug 224977 Opened 17 years ago Closed 14 years ago

remove references to toolkit/content/nsTransferable.js


(Toolkit :: XUL Widgets, defect)

Not set





(Reporter: steffen.wilberg, Assigned: philor)



(1 file, 3 obsolete files)

nsDragAndDrop.js is a 1:1 copy of nsTransferable.js, except that it has
additional functions and the the DUMP_obj() is missing; but nobody needs that:

Files which reference it are:

All these files reference nsDragAndDrop.js as well.
nsTransferable.js is not listed in toolkit/

I can make a patch to remove the references if you want.
The xpfe and the toolkit version of nsTransferable.js are identical.

toolkit's nsDragAndDrop.js is the same as xpfe's nsTransferable.js plus xpfe's
Only DUMP_obj() was left out.
Recent list after the download manager and toolkit changes:

Attached patch patch (obsolete) — Splinter Review
Remove these references. Remove the file from toolkit/content/
As you can see from the patch, every file references nsDragAndDrop.js as well,
and that contains the whole nsTransferable.js, see comment 1.
Comment on attachment 136981 [details] [diff] [review]

Pierre, what do you think?
Attachment #136981 - Flags: review?(p_ch)
Summary: remove toolkit/content/nsTransferable.js and references to it → remove references to toolkit/content/nsTransferable.js
Scott, maybe you can explain why we need both nsTransferable.js and
nsDragAndDrop.js in toolkit.
nsDragAndDrop.js contained the whole nsTransferable.js until you tweaked the
latter a bit on on 01/08/2004 16:10. Also see comment 1.

I think we should remove the duplicate code, either by removing
nsTransferable.js and references to it, because nsDragAndDrop.js already
contains all of it (but Scott's latest addition), or by removing the duplicate
code from nsDragAndDrop.js.
Comment on attachment 136981 [details] [diff] [review]

if you're going to drop this from /toolkit you need to fix the consumers in
/mail as well or you're just getting bustage
Attachment #136981 - Flags: review?(p_ch) → review-
QA Contact: mconnor
The fact that the two copies are currently out of sync (bug 343524) is a nice example of why we need to kill one or the other.
Assignee: p_ch → nobody
Component: General → XUL Widgets
Flags: review-
Product: Firefox → Toolkit
QA Contact: mconnor → xul.widgets
Version: unspecified → Trunk
Assignee: nobody → philringnalda
You don't just have to fix consumers in mail/, you also have to fix consumers in calendar/ and suite/, which is no problem, and extensions/irc/ and extensions/venkman/, which is, since they have to also work with the two separate files in xpfe/.

Approaches that occur to me:

 - #ifdef the extensions (and all the out-of-tree extensions)
 - make the entire content of nsTransferable.js be "// see nsDragAndDrop.js"
 - merge the two files in xpfe/ too, and fix every caller everywhere

Advice or votes from consumers and potential reviewers welcome.
As far as I understand things, ChatZilla and Venkman can be left trying to load chrome://global/content/nsTransferable.js after its removal from toolkit, without breaking anything. (An #ifdef would, for reference, be completely unacceptable.)
Nope, nothing breaks, just JS console spew of "No chrome package registered for chrome://foo/bar.js" - Venkman already does that, since it needs three chrome://communicator/ things and one chrome://browser/. I'd just rather not add to that if we don't have to.
Attachment #136981 - Attachment is obsolete: true
Attached patch Compromise v.1 (obsolete) — Splinter Review
Best compromise I can see between "our" code and extension code:

- replace the broken copy of nsTransferable in toolkit's nsDragAndDrop with an #include nsTransferable.js, so we don't have two copies that can get out of sync, but we still have an nsTransferable.js to quiet console spew from extensions that need to work with older versions

- #include nsTransferable.js in xpfe's nsDragAndDrop.js, so SeaMonkey gets the same thing whether it's xpfe or toolkit

- remove the "don't change this before Mozilla 0.8" warnings

- remove several dozen redundant <script type="application/x-javascript" src="chrome://global/content/nsTransferable.js"/>s

We could save a few bytes of build size, by just removing nsTransferable.js, at the expense of console spew from extensions and the chance of breaking some extension doing the unlikely and only using nsTransferable.js, but I doubt it's worth it.
Attachment #242612 - Flags: second-review?(neil)
Attachment #242612 - Flags: first-review?(mano)
Do note ny opinion here really isn't strong enough yet, so I'm going to wait for neil's review.

I don't think this's worth it. If we do remove it, what would be the first version which cannot be easily supported (you can always use loadSubScript and friends)?
What we want to do here (and I think this syntax is right) is to use chrome manifest bits in toolkit/content/ to make toolkit/content/nsTransferable.js

this means one line of overhead, and automatic syncing :)
er, to make nsTransferable.js point to the right place :)
Attached patch Lie v.1 (obsolete) — Splinter Review
Sure, I can do lying. I wish I could remember why I decided against it, quite a few weeks ago. The only downside I see is that if someone was crazy enough to write their own nsDragAndDrop, and then include nsTransferable.js, we'd stomp on them, but that's really unlikely. Hope it wasn't "Neil didn't like the idea when we talked about it."

- Removes both copies of nsTransferable.js
- copies nsTransferable into xpfe's nsDragAndDrop.js
- unbreaks the nsTransferable in toolkit's nsDragAndDrop.js
- packages nsDragAndDrop.js as chrome://global/content/nsTransferable.js, both to quiet chrome registration messages and to give something to anyone who only uses it for some odd reason
- removes thirty-some redundant <script src>s
Attachment #242612 - Attachment is obsolete: true
Attachment #242733 - Flags: first-review?(neil)
Attachment #242612 - Flags: second-review?(neil)
Attachment #242612 - Flags: first-review?(mano)
Comment on attachment 242733 [details] [diff] [review]
Lie v.1

Looks OK to me.
Attachment #242733 - Flags: first-review?(neil) → first-review+
At least for the new chrome.manifest system, I don't think we have to pack the file twice, see
Right, that's actually what I was looking at.  Use a % line in the toolkit/content/ to do this, AIUI.
Attached patch Lie v.2Splinter Review
Nice, like .htaccess for jars. Only change from the last patch is that toolkit apps don't have to have two copies (too bad for xpfe suite, but suiterunner likes it fine this way).
Attachment #242733 - Attachment is obsolete: true
Attachment #242835 - Flags: first-review?(mano)
Comment on attachment 242835 [details] [diff] [review]
Lie v.2

r=mano on the toolkit and browser parts.
Attachment #242835 - Flags: first-review?(mano) → first-review+
Can we add a comment here explaining the purpose of the override?
Comment on attachment 242835 [details] [diff] [review]
Lie v.2

mscott: I'm mostly looking for MOA for the (trivial) mail bits, though any other review you want to do would be welcome.

jminta: don't forget you promised to "review" the calendar bit, though I'm out of actual review flags to ask for it or get it.
Attachment #242835 - Flags: second-review?(mscott)
And yes, I'll add a comment on checkin - I had one, at one point, and then foolishly decided anyone wanting to take out what looked like useless and bizarre code would check blame first, forgetting how often I've struggled to find where shuffled around code actually came from.
(In reply to comment #22)
> jminta: don't forget you promised to "review" the calendar bit, though I'm out
> of actual review flags to ask for it or get it.
Calendar likes to keep all of its files in sync between trunk and branch, so in an ideal world you'd #ifndef MOZILLA_1_8_BRANCH the nsTransferable line, and pre-process our customizeToolbar.  If you'd rather just file a calendar follow-up to re-sync the file after you commit, that's fine too. r=jminta for the calendar bit.
Comment on attachment 242835 [details] [diff] [review]
Lie v.2

looks ok to me Phil.
Attachment #242835 - Flags: second-review?(mscott) → second-review+
Checked in, with a comment and without the fix to calendar's customizeToolbar.xul, which I'll get in bug 358578

/mozilla/toolkit/content/nsDragAndDrop.js 1.7
/mozilla/toolkit/content/nsTransferable.js, delete
/mozilla/toolkit/content/ 1.26
/mozilla/toolkit/content/customizeToolbar.xul 1.16
/mozilla/toolkit/components/help/content/customizeToolbar.xul 1.5
/mozilla/toolkit/mozapps/downloads/content/downloads.xul 1.21
/mozilla/toolkit/mozapps/extensions/content/extensions.xul 1.53
/mozilla/browser/base/content/web-panels.xul 1.17
/mozilla/browser/base/content/customizeToolbarSheet.xul 1.3
/mozilla/browser/components/bookmarks/content/bookmarksManager.xul 1.34
/mozilla/browser/components/bookmarks/content/bookmarksPanel.xul 1.23
/mozilla/browser/components/history/content/history-panel.xul 1.33
/mozilla/mail/base/content/customizeToolbar.xul 1.11
/mozilla/mail/base/content/messenger.xul 1.64
/mozilla/mail/base/content/messageWindow.xul 1.28
/mozilla/mail/base/content/msgHdrViewOverlay.xul 1.21
/mozilla/mail/extensions/newsblog/content/feed-subscriptions.xul 1.9
/mozilla/mail/components/addrbook/content/addressbook.xul 1.65
/mozilla/mail/components/compose/content/messengercompose.xul 1.95
/mozilla/xpfe/global/ 1.101
/mozilla/xpfe/global/resources/content/nsTransferable.js delete
/mozilla/xpfe/global/resources/content/nsDragAndDrop.js 1.31
/mozilla/editor/ui/composer/content/TextEditorAppShell.xul 1.82
/mozilla/editor/ui/composer/content/editor.xul 1.159
/mozilla/xpfe/components/bookmarks/resources/bm-panel.xul 1.98
/mozilla/xpfe/components/bookmarks/resources/bookmarksManager.xul 1.14
/mozilla/mailnews/addrbook/resources/content/abDirTreeOverlay.xul 1.47
/mozilla/mailnews/addrbook/resources/content/abResultsPaneOverlay.xul 1.21
/mozilla/mailnews/base/resources/content/messenger.xul 1.267
/mozilla/mailnews/base/resources/content/mail3PaneWindowVertLayout.xul 1.115
/mozilla/mailnews/base/resources/content/messageWindow.xul 1.85
/mozilla/mailnews/base/resources/content/msgHdrViewOverlay.xul 1.71
/mozilla/mailnews/compose/resources/content/messengercompose.xul 1.287
/mozilla/suite/browser/viewSourceOverlay.xul 1.29
/mozilla/suite/browser/navigator.xul 1.445
/mozilla/suite/common/search/search-panel.xul 1.79
/mozilla/suite/common/history/historyTreeOverlay.xul 1.42
Closed: 14 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.