Closed
Bug 67013
Opened 24 years ago
Closed 24 years ago
js changes mean directory listings get wrong context menu
Categories
(Core :: XUL, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla0.8
People
(Reporter: bbaetz, Assigned: law)
References
()
Details
Attachments
(3 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
While regression testing gopher, I discovered that the javascript changes last
week to cause wrapped xpconnect objects to be printed with their address broke
the context menu for the directory viewer.
nsContextMenu.js at about line 276 did a string equality with the value, and
this now fails. However, even when I change this comparision to just test a
substring, the third conditional (!window._content.HTTPIndex.constructor) fails.
Since I don't know what that conditional is for, I don't know how to fix it. I'm
presuming that it was the same JS change - I haven't tried backing it out to
check.
You can see this by bringing up the context menu. Open link in new window should
be an option, but its not. This was confirmed on IRC, so its not just my hacked
directoryviewer.
Comment 1•24 years ago
|
||
That nsContextMenu.js code is making some bad assumptions and needs to be fixed.
if (window._content.HTTPIndex == "[xpconnect wrapped nsIHTTPIndex]...
--- can be changed to ---
if(window._content.HTTPIndex instanceof Components.interfaces.nsIHTTPIndex...
I'm not clear why this code would want to be checking for the 'constructor'
property. Any JSObject that has Object.proto is into prototype chain would
have that property. My fix to bug 64111 included correcting the oversight that
the JSObject part of xpconnect wrapped natives did not have a __proto__. Now
they do (like most every other JSObject). The DOM conversion to use xpconnect
will require this soon enough anyway. Was this check there only to try to verify
that you were looking at an xpconnect wrapped native?
Assignee: jband → blakeross
Reporter | ||
Comment 2•24 years ago
|
||
Tracing back through bonsai (The file has been moved), this was checked in by
law@netscape.com for bug 32357.
> Was this check there only to try to verify that you were looking at an
> xpconnect wrapped native?
The code in question was just a reasonable attempt to verify that the content
area was a ftp/file directory listing internally generated by the browser rather
than some random web page. I don't recall the exact reasoning but I think it
was to try to avoid being spoofed by a web page with conventional
(non-xpconnect) JS.
There's similar code elsewhere to handle charsets for such directory listings, BTW.
Both of those need to change now. It sounds like the "instanceOf" operator is
the right way to do it.
Reporter | ||
Comment 4•24 years ago
|
||
> There's similar code elsewhere to handle charsets for such directory listings,
BTW.
Where? Isn't everything in ASCII? They're uris, and escaped/unescaped. If
charsets are a problem, I'll test my gopher stuff against it if someone can
point me to something which triggers that code.
So the three tests can be replaced with the single instanceOf test? Patch later
tonight.
http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/navigator.js#1120
That's to properly deal with file names that aren't plain ASCII, or so I believe.
I think the instanceOf test seems to be the right solution. That's what the
bogus code was trying to do.
Comment 7•24 years ago
|
||
I assigned it to you because cvs blame showed you as the person who checked in
the lines to nsContextMenu.js. I now see in the cvs log that you were
'extract'ing it from somewhere else. It's fine with me if you reassign this.
Maybe Bill Law will volunteer to fix the cases - he seems to have a handle on
things.
I'll take it.
Reporter | ||
Comment 9•24 years ago
|
||
I did the patch, then checked my mail...
Attached, and it fixed the problem for ftp, and html still works. I haven't
tested on non-ASCII file names - anyone?
Keywords: patch
Reporter | ||
Comment 10•24 years ago
|
||
Reporter | ||
Comment 11•24 years ago
|
||
Comment 12•24 years ago
|
||
It looks like a strict warning will still be thrown if _content.HTTPIndex is
null; you might want to keep the |"HTTPIndex" in _content| check. Also, why
the window._content/_content inconsistency?
Reporter | ||
Comment 13•24 years ago
|
||
Comment 14•24 years ago
|
||
jband, sr=?
Comment 16•24 years ago
|
||
fix checked in.
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: jrgmorrison → xptoolkit.widgets
You need to log in
before you can comment on or make changes to this bug.
Description
•