224977 - remove references to toolkit/content/nsTransferable.js

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Steffen Wilberg]

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.

Comment 1

[Steffen Wilberg]

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.

Comment 2

[Steffen Wilberg]

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

Comment 3

[Steffen Wilberg]

Attachment: patch

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 4

[Steffen Wilberg]

Comment on attachment 136981 [details] [diff] [review]
patch

Pierre, what do you think?

Comment 5

[Steffen Wilberg]

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 6

[Mike Connor [:mconnor]]

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

Comment 7

[Phil Ringnalda (:philor)]

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.

Comment 8

[Phil Ringnalda (:philor)]

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.

Comment 9

[James Ross]

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.)

Comment 10

[Phil Ringnalda (:philor)]

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.

Comment 11

[Phil Ringnalda (:philor)]

Attachment: Compromise v.1

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.

Comment 12

[Asaf Romano (gone)]

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)?

Comment 13

[Mike Connor [:mconnor]]

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 :)

Comment 14

[Mike Connor [:mconnor]]

er, to make nsTransferable.js point to the right place :)

Comment 15

[Phil Ringnalda (:philor)]

Attachment: Lie v.1

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 16

[[e-mail]]

Comment on attachment 242733 [details] [diff] [review]
Lie v.1

Looks OK to me.

Comment 17

[Asaf Romano (gone)]

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

Comment 18

[Mike Connor [:mconnor]]

Right, that's actually what I was looking at.  Use a % line in the toolkit/content/jar.mn to do this, AIUI.

Comment 19

[Phil Ringnalda (:philor)]

Attachment: Lie v.2

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 20

[Asaf Romano (gone)]

Comment on attachment 242835 [details] [diff] [review]
Lie v.2

r=mano on the toolkit and browser parts.

Comment 21

[Mike Connor [:mconnor]]

Can we add a comment here explaining the purpose of the override?

Comment 22

[Phil Ringnalda (:philor)]

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.

Comment 23

[Phil Ringnalda (:philor)]

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.

Comment 24

[Joey Minta]

(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 25

[Scott MacGregor]

Comment on attachment 242835 [details] [diff] [review]
Lie v.2

looks ok to me Phil.

Comment 26

[Phil Ringnalda (:philor)]

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