Closed
Bug 60606
Opened 24 years ago
Closed 24 years ago
Remove MailNews dependency on navigator.js
Categories
(SeaMonkey :: MailNews: Message Display, defect, P2)
SeaMonkey
MailNews: Message Display
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.9
People
(Reporter: bugzilla, Assigned: bugzilla)
References
Details
Attachments
(6 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Comment 1•24 years ago
|
||
Assignee | ||
Comment 2•24 years ago
|
||
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
Comment 3•24 years ago
|
||
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
Assignee | ||
Comment 4•24 years ago
|
||
Alec: sure, I'll pound on it. All of the things you mentioned work fine, but
I'll do some more testing.
Comment 5•24 years ago
|
||
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.
Assignee | ||
Comment 7•24 years ago
|
||
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.
Assignee | ||
Comment 8•24 years ago
|
||
Assignee | ||
Comment 9•24 years ago
|
||
Comment 10•24 years ago
|
||
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.
Assignee | ||
Comment 11•24 years ago
|
||
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.
Assignee | ||
Comment 12•24 years ago
|
||
Assignee | ||
Comment 13•24 years ago
|
||
Comment 14•24 years ago
|
||
r=jag
Assignee | ||
Comment 15•24 years ago
|
||
Assignee | ||
Comment 16•24 years ago
|
||
alec, can you sr?
Comment 17•24 years ago
|
||
nice job, blake. sr=alecf
Assignee | ||
Comment 18•24 years ago
|
||
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
Comment 21•23 years ago
|
||
did this fix bug 39229 as well?
Comment 22•21 years ago
|
||
*** Bug 39229 has been marked as a duplicate of this bug. ***
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•