Closed Bug 224977 Opened 17 years ago Closed 14 years ago
remove references to toolkit/content/ns
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: http://lxr.mozilla.org/mozilla/search?string=DUMP_obj Files which reference it are: mozilla/browser/base/content/web-panels.xul mozilla/browser/components/bookmarks/content/bookmarksManager.xul mozilla/browser/components/bookmarks/content/bookmarksPanel.xul mozilla/browser/components/downloads/content/downloadPanel.xul mozilla/browser/components/history/content/history-panel.xul mozilla/toolkit/content/customizeToolbar.xul All these files reference nsDragAndDrop.js as well. nsTransferable.js is not listed in toolkit/jar.mn. 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 nsDragAndDrop.js. Only DUMP_obj() was left out.
Recent list after the download manager and toolkit jar.mn changes: mozilla/browser/base/content/web-panels.xul mozilla/browser/components/bookmarks/content/bookmarksManager.xul mozilla/browser/components/bookmarks/content/bookmarksPanel.xul mozilla/browser/components/history/content/history-panel.xul mozilla/toolkit/content/customizeToolbar.xul mozilla/toolkit/content/jar.mn mozilla/toolkit/mozapps/downloads/content/downloads.xul
Remove these references. Remove the file from toolkit/content/jar.mn. 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] patch 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] patch 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-
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
Product: Firefox → Toolkit
QA Contact: mconnor → xul.widgets
Version: unspecified → Trunk
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
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/jar.mn 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 :)
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
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 http://developer.mozilla.org/en/docs/Chrome_Registration#override
Right, that's actually what I was looking at. Use a % line in the toolkit/content/jar.mn to do this, AIUI.
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).
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/jar.mn 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/jar.mn 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
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.