Closed Bug 60606 Opened 24 years ago Closed 24 years ago

Remove MailNews dependency on navigator.js

Categories

(SeaMonkey :: MailNews: Message Display, defect, P2)

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9

People

(Reporter: bugzilla, Assigned: bugzilla)

References

Details

Attachments

(6 files)

In bug 43239, putterman said: "Yes, we use it. We use it in the message pane when someone right clicks on html mail. right now it works. I probably should have tried making it more generic when I implemented it but I was trying to make feature complete and I could make it work the way it was. If we break it out then I could change our code to use the new version. As far as I know we use nsContextMenus.js and navigator.js (the big win would be not importing all of navigator.js)." I don't think mail needs navigator.js anymore. I assume it was pulling it in for the 2-3 functions that navigator.js had for context menus (open in new window, save link) but I've since moved these into contentAreaClick.js, which mail pulls in anyways for other reasons. ~20 minutes of usage of MailNews without navigator.js didn't turn up any problems, but I would, of course, like to be more sure than that. putterman, any insight on why else mail might need navigator.js?
Attached patch patch (deleted) — Splinter Review
cc alec for [s]r this actually needs to be checked in with my patch for bug 59707; it's part of it, but I forgot to include it there.
Status: NEW → ASSIGNED
Priority: P3 → P2
Target Milestone: --- → mozilla0.9
more great work, blake. Please do as extensive testing as you can on this - this might be a tricky extraction. Esp try: - opening composer - saving attachments - opening new message windows - sidebar stuff (open/close/remove/add) - view message source - others? All these things seem like they could be depending on functions in navigator.js
Alec: sure, I'll pound on it. All of the things you mentioned work fine, but I'll do some more testing.
Thanks for doing this. That was the only reason that I knew of for including navigator.js. It's possible, of course, that something else snuck in but I was using it solely for the message pane context menus.
Blake: can you take QA contact for this? I don't think any (or many) of us in QA will have the insight to test this dependency.
I'd prefer if someone else would (especially if they could test on multiple platforms), since I'm already doing testing. Verifying this only entails using MailNews as dogfood (which mail qa should be doing anyways) and thoroughly testing the context menus.
Attached patch patch (deleted) — Splinter Review
Please update your navigator.js, I've done most of the whitespace changes you've done there... Could you copy over the adjusted BrowserAddBookmark code? Changing the name is good though :-) Why are you moving these lines to sidebarOverlay.xul? navigator.xul needs them, no need to introduce extra dependancies on sidebarOverlay.xul, imo: + <script type="text/javascript" src="chrome://communicator/content/contentAreaClick.js"/> + <script type="text/javascript" src="chrome://communicator/content/contentAreaDD.js"/> + <script type="text/javascript" src="chrome://global/content/nsDragAndDrop.js"/> Ah, I think I see why... It's because they all have the sidebarOverlay ... Still, it's bad design to put the "includes" in there. You're hiding them, and making removing the sidebar harder.
I have updated my navigator.js, otherwise the diff would be much longer than it is. Those are just some arbitrary whitespace fixes I have in my file. I'll try updating again... Yes, all the files contain sidebarOverlay.xul. It seems Bad to me that all of those files are each including two or three files that could be factored into a least common denominator, which needs those files anyways. And I also hate the idea that every client who wants dragging and click handling in their sidebar has to (remember to) include those files. There's something wrong with our architecture when everyone that wants a sidebar has to do extra work so the sidebar has browser functionality. Those should be inherent characteristics of sidebar panels... ...which is why, in the ideal world of the future, all this will be encapsulated into the currently sparse <browser/>, which will get rid of all the contentAreaXXX sort-of hacks that files must currently include to have a genuine, fully functional content area. Places that need a context menu, dragging, click handling, and so forth will simply need to add a <browser/> to get all of this functionality (the message pane and sidebar panels will use this). This ensures identical functionality and consistency... ...but until this (<browser/>) is working, as I feel is the best solution, I guess I'll leave it the way it is (since it's not required to fix this bug anyways). The sidebar isn't very modular anyways (it's got lots of hooks), however, and removing it currently isn't an option. I'm not really saying that's a reason to add to that problem. But the way it is now (without my patch) doesn't help that modularization either, because then the sidebar wouldn't have all this functionality if it was broken off. So, new patch coming.
Attached patch patch (deleted) — Splinter Review
r=jag
alec, can you sr?
nice job, blake. sr=alecf
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Taking..
QA Contact: esther → stephend
VERIFIED FIXED on NT 2000120720, Linux 2000120721 and Mac 2000120708. No doubt this is a very good checkin.
Status: RESOLVED → VERIFIED
did this fix bug 39229 as well?
*** Bug 39229 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: