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)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla0.8

People

(Reporter: bbaetz, Assigned: law)

References

()

Details

Attachments

(3 files)

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.
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
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.
> 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.
Why is this mine? I didn't write that...
Assignee: blakeross → pinkerton
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.
Assignee: pinkerton → law
Keywords: nsbeta1+
Priority: -- → P1
Target Milestone: --- → mozilla0.8
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
Attached patch patch (deleted) — Splinter Review
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?
Attached patch r=blake with these changes (deleted) — Splinter Review
jband, sr=?
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.

Attachment

General

Creator:
Created:
Updated:
Size: