Closed Bug 170143 Opened 22 years ago Closed 22 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: 22 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: