remove references to toolkit/content/nsTransferable.js

RESOLVED FIXED

Status

()

Toolkit
XUL Widgets
RESOLVED FIXED
15 years ago
11 years ago

People

(Reporter: Steffen Wilberg, Assigned: philor)

Tracking

Trunk
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

15 years ago
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.
(Reporter)

Comment 1

15 years ago
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.
(Reporter)

Comment 2

14 years ago
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
(Reporter)

Comment 3

14 years ago
Created attachment 136981 [details] [diff] [review]
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.
(Reporter)

Comment 4

14 years ago
Comment on attachment 136981 [details] [diff] [review]
patch

Pierre, what do you think?
Attachment #136981 - Flags: review?(p_ch)
(Reporter)

Updated

14 years ago
Summary: remove toolkit/content/nsTransferable.js and references to it → remove references to toolkit/content/nsTransferable.js
(Reporter)

Comment 5

14 years ago
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-

Updated

14 years ago
QA Contact: mconnor
(Assignee)

Comment 7

12 years ago
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)

Updated

12 years ago
Assignee: nobody → philringnalda
(Assignee)

Comment 8

12 years ago
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

12 years ago
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.)
(Assignee)

Comment 10

12 years ago
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.
(Assignee)

Updated

12 years ago
Attachment #136981 - Attachment is obsolete: true
(Assignee)

Comment 11

12 years ago
Created attachment 242612 [details] [diff] [review]
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.
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/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 :)
(Assignee)

Comment 15

12 years ago
Created attachment 242733 [details] [diff] [review]
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
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 16

12 years ago
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.
(Assignee)

Comment 19

12 years ago
Created attachment 242835 [details] [diff] [review]
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).
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?
(Assignee)

Comment 22

12 years ago
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)
(Assignee)

Comment 23

12 years ago
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

12 years ago
(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

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

looks ok to me Phil.
Attachment #242835 - Flags: second-review?(mscott) → second-review+
(Assignee)

Comment 26

12 years ago
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
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

11 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.