Closed Bug 170143 Opened 23 years ago Closed 23 years ago

Page Info has no information in Media, Forms, or Links tabs [WAS: pageInfo media tab hits JS error; can no longer [].concat(document.images)]

Categories

(SeaMonkey :: Page Info, defect)

Product:
SeaMonkey
Component:
Page Info
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: jrgmorrison, Assigned: db48x)

References

Details

Attachments

(1 file, 10 obsolete files)

change linkRev to gStrings.linkRev
38.14 KB, patch
jag+mozilla
: review+
bzbarsky
: superreview+
asa
: approval+
Details | Diff | Splinter Review
In grabAllMedia() of pageInfo.js, it builds up a list of the media elements by array concat'ing together various collections like document.images and getElementsByTagName('embed'). e.g., var theList = [1,2,3]; theList = theList.concat(document.images); // wants concat the flatten '.images' // onto the end of the list With the fix for bug 169795, this no longer works, and the checkin comment suggests that this was intentional ("Fix Array.prototype.concat to special-case and flatten Array arguments (including |this|) only, not any object with array-like .length "). Is that correct brendan? For what it's worth, the above example in IE makes theList into (1,2,3,[object]). But this does break with Nav4.x and prior mozilla behaviour. So I guess this bug has two resolutions: either fix the pageInfo.js to deal with the new concat, or 'grandfather' some special DOM collections. [Actually, grandfathering may not be enough, since some of what pageInfo.js wants to have automagically unrolled are generic nsIDOMNodeList's returned from DOM APIs].
ECMA says Array, not object-with-length-property-containing-uint32-value, in its special-case language for flattening an array argument to Array.prototype.concat, and standards rule. I don't see a way to fix this JS behavior without regressing the ECMA compliance bug. So I think the page-info JS code has to change. /be
It's possible that compatibility constrains us more fiercely than ECMA purity, though. If lots of pages counted on the js_HasLengthProperty condition for the special flattening case, instead of the is-of-Array-class predicate, then we may have to revert. /be
If the goal is to treat a DOM nodeList as a JS Array, calling Array methods on it, you could use Array.prototype.concat.apply or .call, and likewise for other methods than concat. What is the goal of the page-info code here? I hope no dummy array containing [1,2,3] is created and then discarded immediately. /be
Sorry. My example of the pageInfo usage was too brief. It's more like (still abbreviated): function grabAllMedia(aWindow, aDocument) { var theList = []; theList = theList.concat(aDocument.getElementsByTagName("embed")); theList = theList.concat(aDocument.getElementsByTagName("object")); theList = theList.concat(aDocument.images); return(theList); } But it looks like that code will need to change given comments above. Given that Opera and IE on win32 both do no unroll 'document.images' when concat'ing an array (example below), then I doubt there is much cross-browser JS that depends on this convenience. <html> <head> <script> window.onload = function () { var theList = [0, 1, 2, 3, 4, 5]; var theList2 = document.images; var theList3 = theList.concat(theList2); var str = theList.length + "/" + theList2.length + "/" + theList3.length + "\n"; for (var i = 0; i < theList3.length; ++i) { str += theList3[i] + "\n"; } alert(str); } </script> </head> <body> <img src="http://www.mozilla.org/images/mozilla-banner.gif"> <img src="http://www.mozilla.org/newlayout/l.gif"> </body> </html>
The ECMA-compliant form of that onload function is: window.onload = function () { var theList = [0, 1, 2, 3, 4, 5]; var theList2 = document.images; var theList3 = Array.prototype.concat.call(theList, theList2); var str = theList.length + "/" + theList2.length + "/" + theList3.length + "\n"; for (var i = 0; i < theList3.length; ++i) { str += theList3[i] + "\n"; } alert(str); } /be
It's really no problem to change it; I was planning to make it use a TreeWalker anyway. It's the same with the links tab, the forms tab too I bet. The cool thing about using a tree walker is that I can combine two steps into one. Rather than calling grabAllMedia and then processing the list in a seperate pass in makeMedia Tab, I can forgo actually building the array and just add rows to the treeview. Currently I build the array and then copy it element by element into the treeview (which builds it's own array.) And of course, I keep the first arrays around for the lifetime of the window since they're global.
*** Bug 170252 has been marked as a duplicate of this bug. ***
I need someone to test it for me, any volunteers?
Daniel: Is any binary (win32) build available for testing?
Keywords: mozilla1.2
Attached patch should apply cleanly :P (obsolete) — — Splinter Review
Attachment #100726 - Attachment is obsolete: true
*** Bug 140899 has been marked as a duplicate of this bug. ***
*** Bug 171085 has been marked as a duplicate of this bug. ***
Setting All/All per comment 12.
OS: Windows 2000 → All
Hardware: PC → All
*** Bug 171222 has been marked as a duplicate of this bug. ***
Adam: since this is just a change to js, you can just drop the new files into a nightly to test it.
Status: NEW → ASSIGNED
*** Bug 171551 has been marked as a duplicate of this bug. ***
*** Bug 171664 has been marked as a duplicate of this bug. ***
*** Bug 171672 has been marked as a duplicate of this bug. ***
Jag, can you help get this patch reviewed? /be
*** Bug 171975 has been marked as a duplicate of this bug. ***
Comment on attachment 100732 [details] [diff] [review] should apply cleanly :P Can you do a new version of this patch? Remove all dump()s, and remove all the code you commented out and is now dead. Also, you get a bunch of localized strings and store them in global vars, then later you locally get that value again from the stringbundle instead of using the global one. I guess I would prefer just keeping a global reference to the stringbundle element and always getting the string, but if you must store them globally, then I suggest you do it for all of them so you won't need a global reference to the stringbundle, and put them all in one global object, e.g.: var gStrings = {}; function onLoadPageInfo() { var theBundle = document.getElementById("pageinfobundle"); gStrings.unknown = theBundle.getString("unknown"); gStrings.notSet = theBundle.getString("notset"); gStrings.emptyString = theBundle.getString("emptystring"); ... } Or you could do something fancy like: var localizedStrings = ["unknown", "notset", "emptystring", ...]; for (var i in localizedStrings) { var s = localizedStrings[i]; gStrings[s] = theBundle.getString(s); } though you then want to make sure all those identifiers use the interCaps format in the .properties file, otherwise you'll have to access them like gStrings.emptystring; >\ No newline at end of file Tsk tsk. So, when you get your new patch I'll take another look.
Attachment #100732 - Flags: needs-work+
Attached patch improved (obsolete) — — Splinter Review
Attachment #100732 - Attachment is obsolete: true
jag: I played around with your ideas for the strings, but it won't really work any better. I cannot get rid of the global reference to the string bundle, because I have to call getFormattedString on it a number of places. I could hide it inside an object along with all the strings, but that doesn't actually get rid of it, just makes it harder to use. Also, it should be noted that the only strings I made global are the ones which I would otherwise have had to put inside a grab*() function. These functions are called once for each element node in the dom of the page (courtesy of nsIDOMTreeWalker), so loading them from the .properties file there didn't seem like a good idea. I would like to know if you see anything else I can do to optimize these functions, they're pretty simple but maybe I missed something. Oh, and I need to know if storing a dom node in my tree view will have any undesired effects. These nodes have always been kept around (for the life of the window), but since the view object is used outside of my code, I'd just like to know.
May I humbly suggest adding something like "Page Info has no information in Media, Forms, or Links tabs" to the summary. I queried on "page info"n didn't find this bug, and then filed a dup. (sorry for the spam)
Resummarizing as djk suggested -
Summary: pageInfo media tab hits JS error; can no longer [].concat(document.images) → Page Info has no information in Media, Forms, or Links tabs [WAS: pageInfo media tab hits JS error; can no longer [].concat(document.images)]
Attachment #101370 - Attachment is obsolete: true
I have 2 nodelists, and I try to use var theList3 = Array.prototype.concat.call(theList, theList2); var str = theList.length + "/" + theList2.length + "/" + theList3.length + "\n"; It still always tells me that theList3.length == 2, even though it should be more.
Ok, I know that I really shouldn't change this patch since people have already started looking at it, but I think it's important. Basically, the changes in the previous patches altered the balance for where time is spent in the code. Now, rather than the most expensive parts corresponding to where rows are added to the trees, the most expensive part is where it iterates over the iterator, simply because it has to iterate over the entire dom tree and not just a subset (just the links or just the images or just the forms.) I originaly thought this would be ok, but after playing with it, I'd prefer to do it differently. Now, rather than building each tab only when the user clicks on it, all three tabs are built at once, when the user clicks on any of them. This means that when the user wants to view all three of them, we only iterate over the tree once instead of three times. There is no improvement if the user only wants to view one of the tree tabs, but there shouldn't be any real loss either, at least not one that I could measure. This patch also fixes a slight regression that apparently slipped in with bug 158245 and caused the image preview pane to break with anything other than an image or image input field. Should have been caught on the review, but oh well. So, Jag, can I expect r= before the freeze?
Attachment #101652 - Attachment is obsolete: true
Quick skim comments: > + // loop through the list of column names that ...... Linebreak this to respect the 80-char authoritay. > this.data[row][colidx] || ""; This pattern appears in a number of places (getCellText, setCellText, handleCopy, etc)... wouldn't this throw an exception if "row" is out of bounds such that this.data[row] is null/undefined? Is that what we want? > + if (action == "copy") > + var data = this.handleCopy(row) > + this.tree.treeBody.parentNode.setAttribute("copybuffer", data); Should that third line not be inside the if()? > +var theBundle = {}; Wouldn't it make more sense to pre-initialize this to null? Or do things throw exceptions then? > + if (formView.rowCount || linkView.rowCount || imageView.rowCount) What if there just happen to be no forms, images, or links on the page? (Yeah, I know; stupid case, but still.) Wouldn't it be possible to have an "initialized" either as a member on those objects or even as a global? > + makeTab(tree, view, filter, aWindow.frames[i].document, aWindow.frames[i]); // recurse through the frames What am I missing? Where did "tree", "view", and "filter" come from? (In particular, I see no uses of "filter" anywhere else in the patch.) For that matter, makeTab is nowhere to be seen either. Does this just break on framed pages? > + while (iterator.nextNode()) > + ; // it'll never be executed anyway... Perhaps add one more line of comment saying that grabAll will never accept a node? > + var s = new Date(); > + var e = new Date(); I assume you will be removing these? ;) > + switch (elem.nodeName.toLowerCase()) What about XML? > + break semicolon? > + // should this test use regexes to be a little more lenient wrt whitespace? Yes. It should. Note that "stylesheet alternate" and "foo bar baz alternate stylesheet" are also valid values. Please spin off a separate bug on that. > + linkView.addRow([elem.rel || elem.rev, elem.href, gStrings.linktext, elem.target]); Why bother determining a nice "linktext" if you're not going to bother using it here? > + else if (item.src != null) else after return. I know that's how it was before, but please fix.
Attached patch address review (obsolete) — — Splinter Review
Thanks bz, I appreciate the quick response. >> + if (formView.rowCount || linkView.rowCount || imageView.rowCount) > >What if there just happen to be no forms, images, or links on the page? (Yeah, >I know; stupid case, but still.) Wouldn't it be possible to have an >"initialized" either as a member on those objects or even as a global? good idea. >> + makeTab(tree, view, filter, aWindow.frames[i].document, >>aWindow.frames[i]); // recurse through the frames > >What am I missing? Where did "tree", "view", and "filter" come from? (In >particular, I see no uses of "filter" anywhere else in the patch.) For that >matter, makeTab is nowhere to be seen either. Does this just break on framed >pages? you aren't missing anything, those arguments were originally there. Just forgot to delete them with this last patch. >> + var s = new Date(); >> + var e = new Date(); > >I assume you will be removing these? ;) I suppose that's a reasonable request :) >> + linkView.addRow([elem.rel || elem.rev, elem.href, gStrings.linktext, >>elem.target]); > >Why bother determining a nice "linktext" if you're not going to bother using it >here? oops, that's what happens when you move a bunch of things. >> + switch (elem.nodeName.toLowerCase()) > >What about XML? XML elements will always show up in page info, and always have. It needs to be fixed, but I'm not entirely clear on how to go about it. Anyway, it's a different bug.
Attachment #101909 - Attachment is obsolete: true
good thing I decided to test it one more time :P
Attachment #102019 - Attachment is obsolete: true
aaronl, re: comment #27, ECMA-262 Edition 3 requires Array.prototype.concat to treat non-Arrays as singletons, even if they have length properties with valid array lengths in them. See http://bugzilla.mozilla.org/show_bug.cgi?id=169795#c3 step 4 from ECMA-262 Edition 3 15.4.4.4. /be
Comment on attachment 102026 [details] [diff] [review] I was forgetting to clear the field list in onFormSelect() > Oh, and I need to know if storing a dom node in my tree view will have > any undesired effects. These nodes have always been kept around (for > the life of the window), but since the view object is used outside of > my code, I'd just like to know. It sounds to me like there could be a problem if the view (and thus the DOM nodes) are kept around for a long time. I would have to look more deeply into this, for which I don't really have time right now. Perhaps you could ask someone else? >Index: xpfe/browser/resources/content/pageInfo.js >=================================================================== >RCS file: /cvsroot/mozilla/xpfe/browser/resources/content/pageInfo.js,v >retrieving revision 1.39 >diff -u -r1.39 pageInfo.js >--- xpfe/browser/resources/content/pageInfo.js 30 Aug 2002 14:50:16 -0000 1.39 >+++ xpfe/browser/resources/content/pageInfo.js 7 Oct 2002 18:38:05 -0000 >+ imageView.rowCountChanged(0, imageView.rowCount); >+ imageView.selection.select(0); >+ linkView.initialized = 1; Shouldn't this be imageView.initialized? >+ //var e = new Date(); >+ //var t = e - s; >+ //dump("took "+t+"ms.\n"); Don't comment out, just remove. Rinse, repeat for all other cases. Don't consider this "and the rest is okay", I just spotted these two things while looking at the patch. I'm okay with these changes being made to pageInfo, but it'll need a thorough code level review and someone answering your question (and a fix if the answer is "it's bad") before this goes in.
ok, I'll make those two changes (pretty sure there isn't any other commented out code, but I'll double check.) Who should I ask about keeping dom nodes around?
Also, Brendan, in comment #5 you give the "ECMA compliant form" of my old function. I was under the impression that that was the way you had to do it if you wanted it to flatten 'arrays'. Is that wrong, or was there some other problem with my old code that made it noncompliant?
Well, jst's on vacation.... The issue with keeping domnodes around is that the JS objects corresponding to those domnodes are holding references to the document. So as long as you're holding onto the nodes the entire document is kept alive. Once you release the nodes, it should all go away properly, however... jkeiser? sicking? Any other bad side effects you can think of? I feel that that's not a big issue, myself... if we wanted to be really slick, we could treat the page info window like IE treats the Find dialog -- tear it down if the document goes away. Though that would be hard and perhaps a little surprising on the UI front....
In comment #5, I was going from memory, and from SpiderMonkey code prior to the recent fix to match ECMA. I do agree ECMA limited utility here. I'm not sure it was intended. Cc'ing waldemar. /be
Attached patch fix problems noticed by jag (obsolete) — — Splinter Review
well, since no one else has any comments... I'd better at least put this here so that I don't lose track.
Attachment #102026 - Attachment is obsolete: true
> + this.copycol = copycol; Add a comment before this one saying what that member does? > + // loop through the list of column names that ... Again, 80 chars. > this.data[row][colidx] || ""; See what I said in comment 29 near the beginning > + return (row < 0) ? "" : (this.data[row][this.copycol] || ""); Same issue > + gStrings.unknown = theBundle.getString("unknown"); As a separate patch, perhaps, consider just creating an array of strings, looping over it, and doing: gStrings[arr[i]] = theBundle.getString(arr[i]); for each string. In any case, I wouldn't hold up this patch for this (but if you're willing to do it, go for it). > + var url = ("src" in item && item.src) || ("code" in item && item.code) || ("data" in item && item.data) || ("href" in item && item.href) || gStrings.unknown; // it better have at least one of those... This could use some 80-column love too. > + mimeType = ("type" in item && item.type) || ("codeType" in item && item.codeType) || ("contentType" in item && item.contentType) || gStrings.unknown; And this. The rest looks good. Fix the nits and tell me something useful about the [row][col] stuff and sr=bzbarsky. ;)
> + return (row < 0) ? "" : (this.data[row][this.copycol] || ""); I don't think there is a problem if copycol is negative since there is at least one case already where this happens (and I don't recall anything bad happening there), but just to be on the safe side I added a check.
Attached patch fix problems noted by bz (obsolete) — — Splinter Review
Attachment #102877 - Attachment is obsolete: true
Attachment #103113 - Attachment is obsolete: true
thanks BZ
Attachment #103120 - Attachment is obsolete: true
Comment on attachment 103126 [details] [diff] [review] change linkRev to gStrings.linkRev r/sr=bzbarsky
Attachment #103126 - Flags: superreview+
Comment on attachment 103126 [details] [diff] [review] change linkRev to gStrings.linkRev r=bz, sr=jag bz, db48x: > gStrings[arr[i]] = theBundle.getString(arr[i]); > > for each string. If you do this (in another patch), make sure to correctly interCaps the property names.
Attachment #103126 - Flags: review+
*** Bug 175058 has been marked as a duplicate of this bug. ***
Comment on attachment 103126 [details] [diff] [review] change linkRev to gStrings.linkRev a=asa for checkin to 1.2 (on behalf of drivers).
Attachment #103126 - Flags: approval+
Checked in to the trunk.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
*** Bug 175208 has been marked as a duplicate of this bug. ***
*** Bug 175273 has been marked as a duplicate of this bug. ***
It still shows nothing on some pages, until you do a "shift-reload"... as an example, the following URL http://www.forbes.com/work/newswire/2002/10/17/rtr754920.html has empty panes for everything when I first hit it. After reload, too. After shift-reload, I get full information. While for this page: http://www.miami.com/mld/miamiherald/4306482.htm I can't ever seem to get the info. Or can it be a timing problem? build 2002101804 - WinNT4 Does this exist as a separate bug?
Same problem here with Philipp links, but seems to work great elsewhere. Mozilla 1.2b Mozilla/5.0 (Macintosh; U; PPC; en-US; rv:1.2b) Gecko/20021018 Mac8.6
*** Bug 175369 has been marked as a duplicate of this bug. ***
*** Bug 175366 has been marked as a duplicate of this bug. ***
Phillip: those look like bug 175447
*** Bug 175693 has been marked as a duplicate of this bug. ***
Verified on the netscape trunk build (2002-10-17-08-TRUNK)
Status: RESOLVED → VERIFIED
*** Bug 175970 has been marked as a duplicate of this bug. ***
*** Bug 176198 has been marked as a duplicate of this bug. ***
*** Bug 176265 has been marked as a duplicate of this bug. ***
*** Bug 176664 has been marked as a duplicate of this bug. ***
*** Bug 176789 has been marked as a duplicate of this bug. ***
*** Bug 177323 has been marked as a duplicate of this bug. ***
*** Bug 177631 has been marked as a duplicate of this bug. ***
*** Bug 180313 has been marked as a duplicate of this bug. ***
when press ctrl+tab to change tabs en page info cant see noting, but if press short cut ctrl+l or ctrl+f or use the mouse no have any problem
Product: Browser → Seamonkey
Depends on: 523573
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: