Closed Bug 52730 Opened 24 years ago Closed 23 years ago

db48x's Page Info improvement patch

Categories

(SeaMonkey :: Page Info, defect, P1)

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.9

People

(Reporter: bugzilla, Assigned: db48x)

References

Details

(Keywords: helpwanted, meta)

Attachments

(4 files, 25 obsolete files)

63.63 KB, patch
Details | Diff | Splinter Review
7.86 KB, image/png
Details
8.34 KB, image/png
Details
26.84 KB, application/x-zip-compressed
Details
wrt 4.x, mozilla is missing the following items in the Page Info window:

* File MIME Type
* Source
* Local cache file
* Content length
* Expires
* Charset
* Security (already filed as bug 49697)
doubtful this'll get fixed on time, so adding helpwanted.
Depends on: 49697
Keywords: 4xp, helpwanted
Suggested Info window spec, for making Mozilla the browser of choice for Web 
developers: <http://critique.net.nz/project/mozilla/general/component/info/>
OS: Windows 98 → All
several items listed/tracked here, so adding meta kw.

mpt: woo! that's a very thorough spec, lotsa info. :)
Keywords: meta
Depends on: 53503
Depends on: 41443
Depends on: 53641
Summary: missing items in Page Info → missing items in and for Page Info
Depends on: 55569
Depends on: 55713
Matt's spec is definitely well suited for targetting web developers!
Depends on: 60428
only a few problems I see with mpt's proposal. My views at <a
href="http://db48x.hypermart.net/pageinfo.html">http://db48x.hypermart.net/pageinfo.html
</a>. 

It's big, but probably implementable.
Note that I moved the document in my previous comment to
http://db48x.hypermart.net/pageinfo/

It's also been updated. Any comments?
In a blue sky world, DOM View would also be present here if Source View was.  =)
I am going to try working on this one :) 
Hey, that's a great idea. I've already seen one page that does something like
that, but can't remeber the URL. It was by someone in irc. Perhaps he'd like do
something like that.
Blocks: 68410
Depends on: 68823
Depends on: 69295
Depends on: 67518
Depends on: 72740
Depends on: 66037
Depends on: 73847
New bug filed -- #73992. Will this work with the current page info spec?
Depends on: dublinCore
Ok, I'm back. What's left of my real life got up and bit me. I'd just like to 
ask: Who broke my XUL?!?

arrg.

Anyway, I do still want this in .9

db48x
as for bug 73992, see my comments there.
Thanks sairuh for keeping these dependencies up to date! My spec is incomplete 
(especially with regard to non-HTML data, and security info), so I'll take this 
bug and update the spec with the dependencies in mind. Then it can be 
implemented by people who are less busy than Ben is. :-)
Assignee: ben → mpt
Component: XP Apps: GUI Features → User Interface: Design Feedback
Summary: missing items in and for Page Info → Page Info design tracking bug (missing items)
Depends on: 74760
Depends on: 74729
Depends on: 74668
Depends on: 76099
Depends on: 76170
Ok, I've got it into shape for .9, but unfortunatly I can't make a patch for it.
The files are at www.db48x.net/pageinfo. You'll need pageinfo.xul/.js,
imagepreview.xul/.js, and pageinfo.dtd. I'd like to get some reviews, etc.

This patch probably fixes 3/4 of the bugs this one depends on, if not more.

db48x
first comment - on the TriStateColumnSort stuff, you shouldn't put a seperate
handler in each treecell... do something like this:

<treerow onclick="TriStateColumnSort(event.target.getAttribute('sortcolumn');">
  <treecell sortcolumn="foo"...../>


etc...
 that saves us from creating 5x the number of event handlers than we need to
Can I be <gerv@gerv.net> please? :-) Do you still want me to look at/review 
this? How much has it changed since I last had it?

Gerv
hehe, sure, I can change the email address.

Yea, I'd like as many people to look at it as possible. Less chance of me
missing bugs that way. I think there were a number of changes since you last had
it. I know I had to add a number of entities for the stuff I insert from
javascript, though that's not working correcly yet. (they show as
&pageInfo.unknown; instead of Unknown. Silly DOM.)
a few changes as in a better image preview and some stuff like that.

Also note that the diff I just posted is now slightly out of date, I fixed a few
small things, like the image preview not updating, js warnings, etc.
Probably best if you post a zip of all the files to your website every time you 
fix something. Then people can download it, copy the files into a working 
Mozilla and comment on the results :-)

Gerv
Good idea gerv. db48x.net/pageinfo/pageinfo.zip
Depends on: 76516
No longer depends on: 76516
*** Bug 76516 has been marked as a duplicate of this bug. ***
Whiteboard: patch
Copying the files from db48x.net over those in a 2001-04-20 Linux Mozilla
installation causes Mozilla to segfault :-(

Gerv
I know, I was trying this yesterday, and I can't get it to start up with the
chrome unjarred. I'd be willing to bet it's a bug.

db48x
Depends on: 76926
Well, given that it doesn't appear with anyone else's chrome apart from yours...

Also, note that some relevant tab control syntax changes just landed. That might 
be it.
http://bugzilla.mozilla.org/show_bug.cgi?id=70810

Gerv
yea, I've already accounted for those changes, but at any rate I don't believe
it has anything to do with my files, because I haven't copied them into the
chrome directories yet. All I've done is unjar the chrome, and modify a line in
installed_chrome.txt

db48x
Depends on: 77408
r=ksosez
Ksosez, I am sorry, but I must question your review. Usually, when reviewing
large patches, the reviewer has lots of nits and points. This one is almost 1000
lines o' code...

I will take a shot at reviewing this last patch, if you Daniel before can
confirm that it is the final patch (before reviews).
What's the purpose of this?

-  <keyset id="dialogKeys"/>
+  <keyset id="dialogkeys"/>
Ksosez: yea, surely I'm not that perfect!

Hwaara: it's mostly final. The last tab, the one that's supposed to show the 
frames, doesn't work right, and needs more work. I may end up leaving it out and 
filing another bug, or doing another patch on this bug. There was one other 
relativly major thing that I was going to do, but I can't remeber off hand what 
it was, and I don't have my list with me (doh!). Even though this isn't the 
final final version, I'd like to get the review if I still can, in order to 
catch things as early as possible.

blake: Good catch. I wasn't able to apply the last XUL patch that went through, 
so I had to do it by hand. Don't think it made much of a difference though.

Thanks for all the help guys!

db48x
whoops. both db48x.net/pageinfo and db48x.hypermart.net/pageinfo have the
updated files now :P

db48x
status update, for those who are interested...

These are fixed:
 bug 53503 - correctly show mime type, cache info, etc
 bug 53641 - not much we can do, I don't thing, but it shows the urls and stuff
 bug 60428 - shows all META tags and their contents
 bug 68823 - shows all META tags, will show LONGDESC etc for images
 bug 73992 - again, shows all META tags
 bug 74760 - never should have been a bug
 bug 76926 - fixed, silly error

These won't be:
 bug 66037 - probably won't "fix"
 bug 72740 - wontfix :P
 bug 74729 - wontfix, imho

These aren't yet done, but should be: (if not now, then later)
 bug 55713 - not done
 bug 67518 - not done
 bug 69295 - researching dnd
 bug 76099 - cool, but later
 bug 76170 - high priority
 bug 77480 - qaneeded

and these are 'cantfix':
 bug 73847 - I won't be able to fix this
 bug 74668 - should be checked in real soon now

db48x


Yeah your right your not that perfect..I withdraw the r= so i can take you apart
piece by piece :)
Depends on: 74530
Ksosez: doh, what have I done!
Depends on: 78123
Depends on: 80556
Depends on: 81202
Depends on: 81328
No longer depends on: 76170
Ok, the files at http://db48x.net/pageinfo/ are the ones I'm submitting for
review. I'd really like to get these into .9.1, and I think there's enough time
to do it.

It's pretty much complete as far as features go, though I've left off the frames
tab that I wanted to include. It just didn't happen in the limited amount of
time I have available for working on mozilla.

I'm sure there are a number of polish things that I've missed, though I made a
good attempt to scrutinize everything. 

I realize that getting this thing "installed" correctly is a pain. I have to do
it over again every few days as I update my copy of mozilla. The best way to do
it is to build without jarred chrome, then just drop the files in. Turn off the
XUL cache or restart moz and you're on your way. Starting from a nightly is a
little more difficult, you have to unjar the chrome and then edit
installed-chrome.txt to get it going. All the nitty-gritty details are on my
little webpage, though admittedly the explanation could use some work.

Anyway, I'm actively seeking review.
Attached file compressed copy of the latest version (obsolete) —
No longer depends on: 66037
Depends on: 81657
Keywords: review
Daniel,

You need to start using diffs for changes because this makes things very hard
otherwise. You may think other people aren't changing these files, but that's
not true - I checked into to pageInfo.dtd and pageInfo.xul yesterday.

pageinfo.properties is inconsistently-named. It should have a capital I, and it
should have a license header. The "unknown" property needs a clearer name, or
you need to add a comment that it's used in multiple places.

pageInfo.css is new - where does it live? Is it part of Classic or Modern?

" * The Original Code is the Mozilla." -> "The original code is Mozilla" (I
think; but definitely something more gramatically correct.) You should list
yourself as Original Author: rather than a Contributor in those files you wrote
from scratch.
   
- The Original Code is the Netscape security libraries.
Did you really mean this in imagePreview.js? Also, .js header comments are
supposed to have *s down the side. See http://www.mozilla.org/MPL/ for
pre-formatted boiler-plate suitable for file headers.

Recently, as mentioned above, I updated the checked-in versions of pageInfo.dtd
and .xul to use the brandShortName entity rather than hard-coding Mozilla. You
need to incorporate these changes - but, better still, you need to remove
mainWindow.title and friends from your DTD and include a DTD which contains them
(to avoid duplication.) See the new pageInfo.xul for an example of how to do
this.

In general, you should avoid excessive width in files. Although the 80-column
limit seems not to be enforced hard in Mozilla, 123 columns seems excessive. Try
aligning tag attributes in the .xul.

If other tabs are added by overlay, does this mean they haven't changed? You
haven't included any files for them.

Why is the frames stuff disabled? You need to explain the problem or, better
yet, reference a bug number in the comments. In general, blocks of commented-out
code need removing or, at the least, an explanation for their presence.

> var defaultLinkText = "doh!";
Er... no :-) Please replace this with something more sensible that will explain
to the user what is going on. If this can only not change through a coding
mistake, it should still say something like (No link text) or (None).

You need to dump all the dump statements. 

> pageURL.init(nsIStandardURL.URLTYPE_STANDARD, 80, url, null);
80 is the default port (check the definition of nsIStandardURL using LXR.) If
you aren't requesting this URL remotely, I don't suppose it matters what value
you set.

"Skipping" has two `p's, and "concatenate" is spelt like that.

concatAA and concatAS could do with clear names. How about "joinArrays" (or
"concatArrays") and "push"?

the makeARow function may well become more efficient if you create the entire
row before adding it to the tree. I'm not sure about this, but it's worth a go.
Something like:
function makeARow(parent, array)
{
  var treeitem = document.createElement("treeitem");
  var treerow = treeitem.appendChild(document.createElement("treerow"));

  for (var i = 0; i < array.length; i++)
  {
    var treecell = treerow.appendChild(document.createElement("treecell"));
    treecell.setAttribute("label", array[i]);
  }

  return parent.appendChild(treeitem);
}

I think the Javascript MIME type is now application/x-javascript .

Is there any reason for a separate imagePreview.js? There seems to be a good
opportunity to share code (a lot looks duplicated) if you use pageInfo.js in the
Image Preview window as well. It's not compulsory to have one .js file per .xul
file.

// a few things I'll need
This is never a helpful comment :-) Either enhance it or remove it.

Just fix that (shouldn't take long), and r=gerv. 

No - just kidding ;-) Sort that lot out, attach some new diffs plus new files
and a note of where they live, and I'll actually test the thing for you and
review it again :-) I won't have a chance when the week starts and you'll
probably miss the 0.9.1 boat, so you may want to do this first thing in the
morning when you get up...

Gerv
Ok, a few notes:

I couldn't make a single patch due to command line length restrictions, so there
are two seperate ones. This is very annoying to be sure.

Also note that the patches don't include several of the files, because they're
new. You'll have to copy them into your tree by hand before you remake the chrome.

A list of files changed, and where they go:
pageInfo.xul - included in the patch
pageInfo.js - include in the patch
imagePreview.xul - put it in xpfe/browser/resources/content/
imagePreview.js - goes in xpfe/browser/resources/content/
pageInfo.dtd - patched
pageInfo.properties - xpfe/browser/resources/locale/en-US/
pageInfo.css - goes in both themes/classic/navigator/ and themes/modern/navigator/

I fixed the liscenses, note that I had accidentally put one in pageInfo.dtd
where there wasn't one before, and had left it off pageInfo.properties. Gerv
hadn't caught this.

Regarding overlays: The only overlay currently in the tree is the security tab.
I'm not doing any work on it, so I haven't included it here.

I just realized that I didn't take out the dump statements - I'd meant to.

regarding function names: I really have a bad imagination, especially when it
comes to thinking up names. I suppose I'll fix them, though I haven't yet.

Also, that simple improvement to makeARow() is awesome! Something like a 20%
decrease in the time it takes to fill in the link tab on slashdot. Also see bug
81328.
> command line length restrictions, 

Which platform are you on that doesn't allow diff -u 
xpfe/browser/resources/content/foo xpfe/browser/resources/content/bar ?
If you are on Unix, see http://www.gerv.net/software/patch-manager.html .

> imagePreview.js - goes in xpfe/browser/resources/content/

This should be rolled into pageInfo.js and the common code factored out into 
helper functions.

> I fixed the liscenses, note that I had accidentally put one in pageInfo.dtd
> where there wasn't one before, and had left it off pageInfo.properties. 

If there wasn't one there before, you should add it. If the original author was 
a Netscape employee, use the NPL, otherwise the MPL.

I'll try and have a look at this tonight, but no promises. You should actively 
seek review from others. (Note: don't ask reviewers@mozilla.org for a 
super-review until you've got a review.)

Gerv
Gerv: he was trying to diff _all_ of the files at once, not just one at a time.

you should be able to just do
./mozilla% cvs diff -u xpfe/browser/ themes/ > patchfile
or just concattenate the diff outputs.
you could just do cvs ./mozilla% diff -u > patchfile
but that would take a bit longer ...

If someone from UID feels the need to create a spec, they should file a new bug 
blocking this one.
Assignee: mpt → db48x
Component: User Interface Design → XP Apps: GUI Features
Keywords: patch
Whiteboard: patch
Yea, I had to list all the files individually because my tree was out of date.
Didn't want to undo anyone else's work. Windows has a 128 character limit on the
command line, which sucks.

>This should be rolled into pageInfo.js and the common code factored out into 
>helper functions.

I suppose I can do this.

Also, did you notice that the image preview doesn't scroll when it gets larger
than will fit? Any idea how to correct that?
The following is split into "must fix", "should fix" and "could fix" - none of
the "should fix" items are necessary to get my review, but they are all
desireable.

Must Fix
--------

You've lost a ">" character in pageInfo.dtd which has broken it. Also, you've
included brand.dtd in pageInfo.xul and not imagePreview.xul, and this breaks
imagePreview.xul. You also need to include navigator.dtd for the mainWindow.*
properties, rather than copying them into pageInfo.dtd.

pageInfo.css makes Mozilla segfault on loading the XUL with 100%
reproducibility. Removing it from the XUL files makes things work. (And
everything seems to work fine, too - what does it do?) You need to work out what
you are doing that our CSS engine doesn't like :-) Also, does imagePreview.xul
actually need pageInfo.css?

The default size should be a bit wider - URLs are truncated far too often. The
other option is not to fully resolve them.

Why is imagePreview.xul a separate XUL file when it's not a separate window? I
assume this generates the bottom part of the Image tab, which is presented in a
different way to the UI of e.g. the General tab. Please make the UI consistent
with the General tab, because the two share a lot of fields and so should look
similar. If you want to keep imagePreview.xul separate, you need to say why it
has to be.

The grippy on the Images tab doesn't seem to work. It changes arrow state but
doesn't collapse the window.

I know the HTTP standard spells "Referrer" wrong, but that's no excuse for you
to do it :-)

If you go to a long Links tab, this "stretches" the content window. And there's
no scrollbar. Then, if you go to the Forms tab and click on the grippy, the
entire bottom pane vanishes irretrievably. :-(

There's something very confused about the order of the Method and Action columns
in the Forms tab XUL - compare the treecols with the treecells.

The word "Unknown" seems to come up a lot. It makes you seem incompetent ;-)
Perhaps you want something like "default" or "not set" in some cases. For
example, Form Target's "Unknown" should be "None (opens in same window)". 

In JS files, you should probably align the comments with the relevant code. You
seem to have several out by one space.

If we are still aiming for 0.9.1 (and it's looking a bit unlikely ;-), if stuff
isn't working (e.g. Referrer) it should be removed for the moment. You are
unlikely to get a chance to fix it, and some mozilla.org code consumers are
shipping this code.

You've missed a few "skiping"s ;-) And Dumps need to be removed, not commented
out (in general.)

http://bugzilla.mozilla.org
- - - - - - - - - - - - - -
The Names in the Links tab have whitespace issues. Please investigate.

On the Links tab, "form submission" URLs are not fully resolved, whereas
standard ones are.

JS files seem to be appearing on the list in the Images tab. Looking at the
properties file, perhaps they are "supposed" to be there. If this is the case,
the tab needs a new name! The same applies if you are listing objects and
applets here.  

http://www.w3.org
- - - - - - - - -
The Security tab has a single button "View" which doesn't do anything. What's
going on there?

The General tab has most fields blank, instead of "Unknown" or whatever. Why is
that?

With lots of links, the Links tab doesn't acquire a scrollbar. There's also a
strange spacing bug whereby Name and subsequent fields start being shifted
progressively more to the right as you go down, after you reach number 20 or so.
Same for the Images tab. 

Check the whitespace in pageInfo.js - there are a few issues.

Should Fix
----------

Performance is still pretty terrible. Remember that performance improvement? You
can do more of the same. For starters, try:

function makeARow(parent, array)
{
  var treeitem = document.createElement("treeitem");
  var treerow = document.createElement("treerow");

  for (var i = 0; i < array.length; i++)
  {
    var treecell = document.createElement("treecell");
    treecell.setAttribute("label", array[i]);
    treerow.appendChild(treecell);
  }
  
  treeitem.appendChild(treerow);
  return parent.appendChild(treeitem);
}

More generally, if we support it, you should create a dummy row in the XUL for
each place where you want rows and give the treeitem an "id". Then,
getElementById() and call cloneNode(true) on it, to do a deep cloning. Then, set
the labels. This is almost certainly more efficient than doing createElement()s
all the time.

You are also doing totally the wrong thing with concat :-( If you allocate a new
array and copy every time, you end up copying all the previous elements each
time you concat, which is really inefficient, particularly with concatASing one
thing onto the end of a very long string. Because each new Array is allocated,
it'll also chew through memory and the garbage collector will have to run,
slowing you down even more. 

For example, on the Links tab, you have an order n^2 (at least) algorithm
because every time you call ConcatAS() to add a new thing to the end of the
list, you are re-copying everything in the list before it into a new array. So,
8 links takes 4 times as long as 4 links, and 16 links takes 4 times as long
again. :-(

You have several choices. The first is to make the grabAll* commands return an
array of arrays, and then iterate over both dimensions. The second is to
pre-allocate an Array of the entire size and then copy all the smaller arrays
(things) in one by one. You could continually pass the target array as a param
to the copy function. Actually, both those ideas suck. Does Javascript not have
a Vector type of some sort? Anyway, this really needs fixing.

Another performance issue is the stringbundle.getString() function. This is
expensive. Cache the values you get - don't call getString inside a loop.

Image Tab: What other types of image are there than "image"? If you mean "plain,
boring, standard image", how about something like "normal". Type "image" seems a
bit, well, obvious, on an Image tab.

Automatically select the first form when you open the Forms tab. Same for the
Images tab.

You really need to de-dupe the Images list. 

Can you not ask the cache for the size if it's not in any headers?

Could Fix
---------

Can you get the URL text strings' ellipses (...) to appear in the middle rather
than at the ends?

There's no need to prefix all your DTD entries with "pageInfo.". You can if you
want, I suppose... Please make the justification consistent in this file.

You don't normally list yourself as the Original Developer and also as a
Contributor. Also, if you are the initial developer, you probably don't need the
bit about "Portions created by Netscape".

On links, you could probably detect "link rel: stylesheet" and just say
"stylesheet".

Other Stuff
-----------

You can probably get around the 128-char limit by either using a batch file, or
doing each diff separately and then just concatenating the files. If you could
do the diffs from the top-level Mozilla directory (i.e. cvs diff -u
xpfe/browser/resources/content/foo ) that would make my life easier.

Have a look at all that stuff. I think if you fix some of those perf issues,
things should perk up considerably ;-) Give me a shout when you've fixed this
lot, and I'll take another look.

Gerv
Filed bug 82059, `Page Info design tracking bug (missing items)'. Please
resummarize this bug to reflect what you've taken it over for. Thanks.
<shrug>

Gerv
Summary: Page Info design tracking bug (missing items) → db48x's Page Info improvement patch
Blocks: 82059
remember, you can use cvs diff <dirname>
such as
cvs diff xpfe/browser
db48x: You need to abolish concatAA and concatAS and use the Array.concat method 
which belongs to Javascript arrays instead. This should produce a performance 
improvement of an order of magnitude, I would guess, on long Links tabs.

Gerv
Ok, same deal as last time, use a diff from the archive (either pageInfo-u.diff
or pageInfo-wu.diff, it doesn't make much of a difference), then put
imagePreview.* into mozilla/xpfe/browser/resources/content/navigator/, and
pageinfo.css into both mozilla/themes/classic/navigator/ and
mozilla/themes/modern/navigator/, then remake your chrome. Chill, then serve. Enjoy.
I won't be able to review this until next week at the earliest. Just so you know 
:-) If you want to ask someone else, feel free.

Gerv
Ok, in that case I'll go ahead and post comments to most of your points above
here. I was just going to email it to you, just to save space.

---
>pageInfo.css makes Mozilla segfault on loading the XUL with 100%
>reproducibility. Removing it from the XUL files makes things work. (And
>everything seems to work fine, too - what does it do?) You need to work out what
>you are doing that our CSS engine doesn't like :-) Also, does imagePreview.xul
>actually need pageInfo.css?

hrm. seems to work ok for me. Could you do try leaving the files in, but
deleting one or the other of the rules that're in it? That'd be better than just
omitting the file. And yes, imagePreview.xul does need pageInfo.css. I didn't
feel like adding yet another file, especially when the contents of
'imagePreview.css' would just be a subset of pageInfo.css.

>The default size should be a bit wider - URLs are truncated far too often. The
>other option is not to fully resolve them.

easily enough done. Actually, now that you mention it, the default height won't
fit on a 640x480 screen, so I shortened it some.

>Why is imagePreview.xul a separate XUL file when it's not a separate window? I
>assume this generates the bottom part of the Image tab, which is presented in a
>different way to the UI of e.g. the General tab. Please make the UI consistent
>with the General tab, because the two share a lot of fields and so should look
>similar. If you want to keep imagePreview.xul separate, you need to say why it
>has to be.

I don't know that it absolutely _has_ to be, and in fact I don't particularly
care for it to stay that way. however, I know of no other way to keep it from
overflowing it's own little box than to put it in an iframe, as I've done here.
Any suggestions are welcome. See also bug 70751.

>The grippy on the Images tab doesn't seem to work. It changes arrow state but
>doesn't collapse the window.

Ok, this is weird. The splitters work correctly for me, but they're drawn
incorrectly. The grippy part is drawn vertically, and the entire splitter is a
thick as the grippy is tall. It still collapses the right way though. See
http://db48x.net/pageinfo/badsplitters.gif to see what I mean.

>I know the HTTP standard spells "Referrer" wrong, but that's no excuse for you
>to do it :-)

Doh.

>If you go to a long Links tab, this "stretches" the content window. And there's
>no scrollbar. Then, if you go to the Forms tab and click on the grippy, the
>entire bottom pane vanishes irretrievably. :-(

This happens because you aren't loading pageInfo.css. Sorry.

>There's something very confused about the order of the Method and Action columns
>in the Forms tab XUL - compare the treecols with the treecells.

They're correct on every form I've tested on. Is there a particular form you
were looking at? (I don't particularly like the attribute names, they're
practically synonymous :P)

>The word "Unknown" seems to come up a lot. It makes you seem incompetent ;-)
>Perhaps you want something like "default" or "not set" in some cases. For
>example, Form Target's "Unknown" should be "None (opens in same window)". 

ok, I'll see about that.

>If we are still aiming for 0.9.1 (and it's looking a bit unlikely ;-), if stuff
>isn't working (e.g. Referrer) it should be removed for the moment. You are
>unlikely to get a chance to fix it, and some mozilla.org code consumers are
>shipping this code.

yea, it is looking less and less likely. In fact, due to the rather unexpeced
shortening of the release cycle, I'll likely set it to .9.3


>http://bugzilla.mozilla.org
>- - - - - - - - - - - - - -
>The Names in the Links tab have whitespace issues. Please investigate.

I see what you mean. It's not taking out whitespace from the source that is
taken out when it's rendered.

>On the Links tab, "form submission" URLs are not fully resolved, whereas
>standard ones are.

I'm not doing anything to the data in either case. Apparently the urls are
already resolved by the time they get into the dom for an href, but not for a
form action.

>JS files seem to be appearing on the list in the Images tab. Looking at the
>properties file, perhaps they are "supposed" to be there. If this is the case,
>the tab needs a new name! The same applies if you are listing objects and
>applets here.  

oops, fixed. Also, 'local script' isn't really what I meant for js not read from
a seperate file. 'embeded script' is what I was thinking orginally, what do you
think?

>http://www.w3.org
>- - - - - - - - -
>The Security tab has a single button "View" which doesn't do anything. What's
>going on there?

Dunno. Somebody Else's Problem. It's added via overlay. In any case, I'm not
seeing it any more.

>The General tab has most fields blank, instead of "Unknown" or whatever. Why is
>that?

It was generating an exception at some point. I honestly don't remember what
fixed it, I didn't make a note of it. It was pretty minor, though it had a
rather noticable effect.

>With lots of links, the Links tab doesn't acquire a scrollbar. There's also a
>strange spacing bug whereby Name and subsequent fields start being shifted
>progressively more to the right as you go down, after you reach number 20 or so.
>Same for the Images tab. 

Two problems, first off you've left out the css file, and secondly, I really
haven't got a clue why it does that. No one else does either. See also
http://db48x.net/pageinfo/treebug1.gif, and http://db48x.net/pageinfo/treebug2.gif

Again, you really do need that pageInfo.css.

>Check the whitespace in pageInfo.js - there are a few issues.

I'll look really closely at this one, but could it be that you're seeing
whitespace left over from previous revisions? That patch was -w, should the next
one just be plain -u?

>Should Fix
>----------

>More generally, if we support it, you should create a dummy row in the XUL for
>each place where you want rows and give the treeitem an "id". Then,
>getElementById() and call cloneNode(true) on it, to do a deep cloning. Then, set
>the labels. This is almost certainly more efficient than doing createElement()s
>all the time.

Ok, I can't get this to work. Using slashdot as a test, it normally displays all
the links in about 13-14 seconds. I get two slightly different results depending
on exactly how I use the cloning, one only takes 4-5 seconds, but shows an empty
tree, the other takes closer to 20 seconds and crashes the browser just after
I'm done populating the tree. The second way to do it is really the correct way
to do it.

>You have several choices. The first is to make the grabAll* commands return an
>array of arrays, and then iterate over both dimensions. The second is to
>pre-allocate an Array of the entire size and then copy all the smaller arrays
>(things) in one by one. You could continually pass the target array as a param
>to the copy function. Actually, both those ideas suck. Does Javascript not have
>a Vector type of some sort? Anyway, this really needs fixing.

Well, it does have an Array type, and the Array type does have it's own
(presumably efficient) concat() member function. It adds to the size of the
array, which is really what I want. However, if you pass it a DOMCollection (or
whatever it was that getElementsByTagName() returns), it doesn't work, it just
returns the original array. That's why I had to try writing my own.

>Image Tab: What other types of image are there than "image"? If you mean "plain,
>boring, standard image", how about something like "normal". Type "image" seems a
>bit, well, obvious, on an Image tab.

s/image/media/, in this case

>Automatically select the first form when you open the Forms tab. Same for the
>Images tab.

easy enough.

>You really need to de-dupe the Images list. 

Is there some kind of hash table like thing i can use for this? I can't think of
any other fast way of doing it. I'd also like to do the same thing for the links
tab, though I expect less duplication there.

>Can you not ask the cache for the size if it's not in any headers?

That's where I'm getting all the information currently.

>Can you get the URL text strings' ellipses (...) to appear in the middle rather
>than at the ends?

sure, as soon as bug 50833 is fixed, it has r= now.
---

Also, I just found bug 42976, which may explain why the cloneNode() thing isn't
working. Wish I'd found that a week and a half ago :P

Anway, I'll see what I can rustle up as far a a review goes.
oh, silly me. cloneNode() does work as intended, but it's not any faster. It
still takes 13 seconds on slashdot. Oh well, I'll keep looking.
>I only have email access at the moment; feel free to paste these into the
>bug. I will do more investigating into some of the issues when I get back
>to my machine.
>
> > I don't know that it absolutely _has_ to be, and in fact I don't
> particularly
> > care for it to stay that way. however, I know of no other way to keep
> it from
> > overflowing it's own little box than to put it in an iframe, as I've
> done here.
> > Any suggestions are welcome. See also bug 70751.
>
>You can keep it in an iframe without it being a separate XUL file; add the
>HTML namespace to your <window> tag and then use <html:iframe> and then
>insert the contents of the current imagePreview.xul inside that.

Oh, silly me. I guess it's been so long since frames originally came out that it
seems natural to put their contents in another file, whether you need to or not.
I'll see what I can do with it, probably have something tonight or tommorrwo.

> > Again, you really do need that pageInfo.css.
>
>Sure. I'll test using it next time :-)

cool :)

>
> > >Check the whitespace in pageInfo.js - there are a few issues.
> >
> > I'll look really closely at this one, but could it be that you're seeing
> > whitespace left over from previous revisions? That patch was -w, should
> the next
> > one just be plain -u?
>
>You certainly should never attach -w patches for review without a straight
>-u, because the straight -u one is the changes that are actually going to
>get checked in.
>

Ok, I've done that this time, attached both a -wu and a -u. Guess it depends on
who you ask.

>Nice one mate - stick at it :-)
>
>Gerv
ah, the joy of a shortened release cycle.
Priority: P3 → P2
Target Milestone: --- → mozilla0.9.3
hrm. more spam, I'm apologise
Status: NEW → ASSIGNED
Priority: P2 → P1
OK, you have a window where I'm available to do Moz work and on a fast Net
connection. It lasts until Wednesday night. Any chance of something to look at?
:-)

Gerv
yea, look at the last attachment
Depends on: 88537
I will review this patch.
Yeah, sorry, I dropped the ball. Apologies.

Gerv
db48x:

Sorry for the delay. I thought boberb was going to sort you out. 

I've just downloaded your latest zip. Some questions:
- it appears from the patch that you have <html:iframe>d the image preview
window, which is great. However, surely this means that we no longer need
imagePreview.xul, and that imagePreview.js can be rolled into pageInfo.js (with
quite a bit of code sharing)?
- You'll need to tell us where to put pageInfo.css. Again, if necessary. ;-)
- There are merge conflict marks in your diff, under themes/modern/jar.mn and
themes/classic/jar.mn. I seriously doubt you mean to add about 400 lines of
stuff to both of these files; I suggest you investigate what has happened.
- If you are going to reformat xpfe/browser/jar.mn, please at least fix the
indentation.
- in pageInfo.dtd (and hence in your XUL), you should probably use the standard
DTD conventions for differentiating between different sorts of entity. These
are:
fooBar.label  for any labels
fooBar.accesskey for any accesskeys
fooBar.commandkey (I think) for hotkeys

The code is undoubtedly still sound; if you could clear up these organisational
points I'm sure a review can't be far off. Warning, though: I'm going away for a
week on Tuesday.

Gerv
Depends on: 91173
Arrg! you have to do this while my email isn't working, right? fun fun fun.

well, I'll see about a patch for those things, even though I suppose you've
already gone.
No longer depends on: 91173
I'm back, and available until Friday :-)

Gerv
Didn't make the 0.9.3 train.
Target Milestone: mozilla0.9.3 → mozilla0.9.4
Depends on: 91173
yea, it didn't make it. At any rate, the current code can't be checked in as is.
I have to mash it all through a sieve and refix bug 88153. Yay. At least gerv
will get his wish, namely that I nuke imagePreview.xul and imagePreview.js.

db48x
*** Bug 90408 has been marked as a duplicate of this bug. ***
Depends on: 94146
Bug 90408 [RFE] "view page info" add all headers, add reload, force 

was made duplicate. So I'll copy here my proposal:

Also I whould like to have there [Reload] and [Shift+Reload] ability for all 
components of page without page reloading - for external JS, java applets,
 css files, images (and BG images) - all which is embeded but you can't open it 
in new window to press Shift+Reload on it.
*** Bug 96955 has been marked as a duplicate of this bug. ***
see RFE bug 96994
Depends on: 96994
Depends on: 97479
looks like this has missed 0.9.4.  -> 0.9.5
Target Milestone: mozilla0.9.4 → mozilla0.9.5
db48x: ping?

Gerv
Sorry, db48x and I didn't have much luck testing the patch on my computer. It
seems like we could never get the CVS to merge his patches correctly. Probably
due to my inability at CVS (Sometimes I want to slit CVS's throaght). Anyway,
sorry db48x and I will be keeping an eye on this since its one hell of a patch.
So are you guys abandoning this?

Gerv
No, I haven't abandoned it. I've just been rewriting part of it in my spare
time. Never did figure out why it wouldn't apply correctly to netdemon's tree,
it worked fine for me in a clean tree. Sorry for the lack of communication, that
was my fault.
*** Bug 103124 has been marked as a duplicate of this bug. ***
0.9.5 is out the door. bumping TM up by one.
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Does this fix any of the keyboard accessibility problems with page info?
yay, I'm back. As soon as update my tree I'll post the latest and greatest,
which I believe is all finished except for one "minor" problem that I'll have to
discuss with sicking. 
Attachment #41000 - Attachment is obsolete: true
the new attachement form is great, but it should have a checkbox for
needs-work, because this patch needs one little tiny bit of work still. There's
some typo or some weird mojo happeneing on the links tab that my tired eyeballs
can't find.

anyway, this one uses outliners finally, I'll also update the dependancies.

gerv, fabian, anyone, could I get an r=? perhaps you could find the bug at the
same time. it would be so cool if you did.

db48x

PS: the readme has the locations for all the files, in case you'd forgotten.
Attachment #29823 - Attachment is obsolete: true
Attachment #31125 - Attachment is obsolete: true
Attachment #32574 - Attachment is obsolete: true
Attachment #35076 - Attachment is obsolete: true
Attachment #35302 - Attachment is obsolete: true
Attachment #35362 - Attachment is obsolete: true
Attachment #38026 - Attachment is obsolete: true
Attachment #57461 - Flags: needs-work+
Depends on: 81328
Attached file minor update (obsolete) —
timeless pointed out that the media tab didn't include images inlcuded as <link
rel="icon" href="...">, so this fixes that. (and the readme is in the zip this
time)
Attachment #57461 - Attachment is obsolete: true
Attachment #57462 - Attachment is obsolete: true
Overall comments.  Please use localName, not nodeName when you test the
node name.  getAttribute() will return null if no attribute is present.
So getAttribute("foo").toLowerCase() is bad, bad, bad.
	
> if("arguments" in window && window.arguments[0])

Please check the argument array length before accessing particular
elements of it.  This will prevent strict JS warnings.

> var cacheentrydescriptor = httpcachesession.openCacheEntry(url, 1, true);
> //open for READ, in blocking mode

use nsICache::ACCESS_READ for clarity?

>  var formview = new
> pageInfoOutlinerView(["form-number","form-name","form-action","form-method"]);
>  var formpreview = document.getElementById("formpreview");
>  var fieldview = new
>pageInfoOutlinerView(["field-number","field-label","field-field","field-type","field-value"]);
  
>  if(formoutliner.outlinerBoxObject.view)
>  {
>    return;
>  }

Would it make more sense to return _before_ creating the
pageInfoOutlinerView objects if they are not needed?

>     document.getElementById("formname").setAttribute("value", (ftp + " " +
> form.name + fts) || thebundle.getString("formUntitled"));

The "||" will never do anything since that string will always include at
least " ".  This is bad for localization, by the way.  Please use a
single entry in the stringbundle with a %s in it that you replace with
the name (see
http://lxr.mozilla.org/seamonkey/source/intl/strres/public/nsIStringBundle.idl#75)

> if(elem.nodeName.toLowerCase() == "label") continue; // ARRR! I wish
> these weren't included...

This is not needed.  .elements does not include labels

>      fieldview.addRow([index, "", elem.name, elem.type, (elem.value !=
> undefined) ? elem.value : ((elem.checked == true) ? checked : unchecked)]);

This will fail for buttons.  In particular, for a <button> "value" will
be "".  Use getValueText here.

> if (childNode.nodeName == "IMG")

This will have case-sensitivity issues.  Especially in XHTML.  Please
uppercase the name before comparing or something.

> function getAltText(node)

Why is this function looking at "title" in preference to "alt"?
Why is this function doing tree-walking?  All the places it's used that
seems unneeded.

>  for (i = 0; i < inputlist.length; i++)
>   if (inputlist[i].type.toLowerCase() == "submit")
>      thelist = concatAS(thelist, inputlist[i]);

[in grabAllLinks].  Should you not include type="image" inputs as well?
Also, what about <button type="submit"> ?

>       case "a":
>         if (!(linktext = getValueText(elem)))
>           linktext = getAltText(elem);

[in makeLinkTab].  Why are we using getAltText here?

The link* property strings have inconsistent capitalization, in my
opinion. That is, all of linkAnchor, linkArea, linkSubmission, linkRel,
linkStylesheet, linkRev should be capitalized or none should be.

>  // should this test use regexes to be a little more lenient wrt whitespace?

Yes.  Especially since those properties are case-insensitive.  If
nothing else, make sure your string compare is case-insensitive.

> function openURL(target)

is this used?

> <!ENTITY  imagesTab             "Media">

That seems a little inconsistent.. :)  rename those entities to mediaTab
and the like (as appropriate).

>  else
>  {
>    //preview.setAttribute("collapsed", "true");
>  }

Is that in or out?  :)  At least make the comment clearer as to why it's
there but commented out

>   //var mimetype = item.contentType || unknown;   // XXX: /me wishes

You can actually get this info for <object> and <embed>, as well as
<link>, though not yet for images.  Both <object> and embed have "type"
and <embed> has codeType as well.

> function makeARow(parent, array)

This is not used is it?  If not, kill it.

Fix these and attach an updated patch?
> getAttribute() will return null if no attribute is present.
> So getAttribute("foo").toLowerCase() is bad, bad, bad.

Are you sure about this? the spec says that "" should be returned if the 
attribute doesn't exist, so it should be ok.
sicking, see bug 37750.  Basically, we deliberately broke compat with the spec
here in favor of compat with IE.  See bug 106822 and bug 106733, which were
caused by the change.
I can tell you up front that I don't have the cycles to review this. :-)

Gerv
Addresses all the issues raised by bz and fabian, both here and in irc (ok,
everything but the toLowerCase() stuff.) Includes a diff, now that cvs-mirror
is back (I didn't bother with a diff -w, the changes are extensive enough that
it's probably easier to apply first and then review the changed files than to
browse the diff.)
Attachment #57490 - Attachment is obsolete: true
Per Hakan's advice, ccing Jan.  Jan, could you review the outliner stuff in this
patch?
Keywords: mozilla0.9.7
patch looks good, here are some nits ...
 
 
+  if (formlist.length > 0)
+  {
+    for (var i = 0; i < formlist.length; i++)
+    {
+      var elem = formlist[i];
+      formview.addRow([++formindex, elem.name, elem.method, elem.action]);
 
I think the first condition is useless.
 
 
+  var thebundle = document.getElementById("pageinfobundle");
+  var linkoutliner = document.getElementById("linkoutliner");
+  var linkview = new pageInfoOutlinerView(["link-number","link-name","link-add
+  if(linkoutliner.outlinerBoxObject.view)
+  {
+    return;
+  }
 
It would be more efficient to bail out before creating a new view.
 
 
+//******** Image Stuff
+function makeMediaTab()
+{
+  var thebundle = document.getElementById("pageinfobundle");
+  var imageoutliner = document.getElementById("imageoutliner");
+  var imageview = new pageInfoOutlinerView(["image-number","image-address","im
-    treeChildren.appendChild(treeItem);
+  if(imageoutliner.outlinerBoxObject.view)
+  {
+    return;
 
Same as above.
 
 
+function pageInfoOutlinerView(columnids)
+{                            
+  this.columnids = columnids;
+  this.colcount = columnids.length;
 
I don't see any use of this.colcount, it can be removed.
 
 
+  addRow: function(row)
+  {
+    var oldrowcount = this.rowcount;
+    this.rowcount = this.data.push(row);
+    this.outliner.rowCountChanged(oldrowcount, 1);
+  },
 
And finally the most important one...
I don't think that calling rowCountChanged() in addRow() is a good idea.
rowCountChanged() call invalidating methods (e.g. repainting of regions)
so it is more efficient to add all rows and then call rowCountChanged()
in one shot. 

other than that r=varga
Attached file latest and greatest (obsolete) —
varga: thanks for looking at this, your comments (especially the
rowCountChanged stuff) shaved another 20% (very rough measurement) off the
worst performing tab of all, the links tab. It used to take 15-20 seconds to
show that tab on slashdot, now with the outliners it's down to 4-5 seconds.
Yay.

I also fixed one or two other very small bugs that had gone unnoticed by me
until now.
Attachment #57557 - Attachment is obsolete: true
fixes everything Fabian brought up in irc (I think), and replaces my sucky
concatenation function with Array.concat(), which never seemed to work
before... That seems to have saved another 25-30% on our favorite informal
benchmark.

Also, application/x-zip-compressed should be added to the list of mime-types in
this form...

PS: if this gets submitted twice, ignore the previous one :P
Attachment #57606 - Attachment is obsolete: true
Comment on attachment 57706 [details]
ready for sr= perhaps? or are Fabian and bz even done with it yet?

Is it just me or does this patch still use concatAA and concatAS? Also did you
make sure you corrected all the places calling the page info dialog? (i don't
see it in the diff).
Also you didn't change the parameter names in grabAll* functions:
thewindow->awindow, thedocument->adocument.
Is this attachment the one you meant to attach?
Ah, yes. I did make a mistake with that attachment. The copy of pageInfo.js
included in the zip is the incorrect version, but I did make the diff with the
correct copy, there is no concatAA() function in it. On the other hand, I
apparently did not change the varible names in those three functions, so neither
the diff nor the .js file have the better names. I'll post another copy of
pageInfo.js shortly.

Jan Varga: Could I ask you to take another look at this for me? Specifically,
theres a problem with the forms tab; the onFormSelect() function bombs out with
a js error that there is no function 'clear()' on the OutlinerView object stored
in the varible 'fieldview'. I haven't been able to figure out why, because I
specifically create a PageInfoOutlinerView and assign it as the view for that
particular outliner in makeFormTab(). It's only after I retrieve the view in
onFormSelect() that there are problems with it. I was wondering if you knew of
anything in particular that could be going on, some quirk in the outliner that I
don't know of. I also just need another pair of eyeballs, it could just be some
silly typo or something.

Thanks
db48x
Oh, and the patch to correct all the places that open the page info dialog was
attached to bug 83017 as attachment 57555 [details] [diff] [review]. Should I make those changes part of
the patch for this bug?
Fixes a small aesthetic bug that made the <textbox>es appear 1px vertically
offset from the <text> labels next to them, and fixes bug 85905.
Attachment #57706 - Attachment is obsolete: true
Depends on: 85905
Ok, tried this in action and looked at the XUL a bit.  Another partial
review (more thourough review of the XUL still to come).

All tabs:

1) <text class="label"> is deprecated.  Please use <label>

2) In either case, <label> / <text class="label"> and <textbox> don't
   seem to be aligning quite right vertically.  I'm not sure what the
   best solution is.  Can you dynamically toggle the <label> value?  If
   so, just drop <textbox> completely?

General Tab:

3) "render mode" needs more space between the ":" and the following
   text.

Form Tab:

4)

> var fieldview = new pageInfoOutlinerView(["field-number","field-label",
                                          "field-field","field-type",
					  "field-value"]);
> formpreview.outlinerBoxObject.view = fieldview;

This is a problem.  The view property of the outlinerBoxObject is an
nsIOutlinerView.  This interface has no clear() method and has no
addRow()/addRows()/rowCountChanged() methods.  Right after these two
statements are executed, alert shows the following

fieldview.clear == function { ... }
formpreview.outlinerBoxObject.view == undefined

and similarly for the row functions.  This was causing the form tab breakage.

Looking over the code, it seems that the creation of the "fieldview"
view should just move to onFormSelect().  As it is, you're clearing and
rebuilding it in there every time.  So I doubt that just creating it in
there would be any worse perf-wise.  Besides, working is better perf
than non-working.  :)  Jan, do you have any better ideas on this?
   
Media Tab:

5)  Preview does not handle relative urls

cloneNode will give you a node with the same src or whatever, but the
base is different.  If there is a way to set the base url on the preview
pane (or on the whole XUL document?) to the base url of the page, that
would be best.

6)  Preview does not handle <link rel="icon"> (test on
    http://www.mozilla.org).  You may want to create "img" nodes for
    <link rel="icon">

7)  Changing the image being previewed completely reflows the tab.  Any
    way to avoid this?

8)  Gray box with info about the image does not have a decent right
    margin (unlike its left margin).  Looks pretty ugly....

9)  item.width and item.height can be undefined and trigger strict
    warnings
1,2) I fixed this in the previous revision, it was just that the margins on the
text element and the textbox element were mismatched by 1px. Naturally, changing
<text class="label"/> into <label/> made this fix mismatch them by 1px the other
direction, I've removed that bit from the css file now.

3) Fixed by adding a narrow (.5em) column between them.

4) I think that'll work ok, but I'll have to see why none of my onSelect
functions are even being called first. I'm also not clear on why I should have
to do it that way. If I assign a pageInfoOutlinerView to .view, I should get a
pageInfoOutlinerView back when I reference .view (but maybe that's just my opinion.)

5) eww, let me work on this. 

6) easily fixed.

7) hrm, doesn't look like it. why exacly does it cause a complete reflow? at
most I would expect the reflow to be contained completely within that vbox
there, the one with the scrollbars, simply because it has overflow: auto; on it,
so changing the size of the content doesn't change the size of the box.

8) I don't see that, it has a margin of 1em all the way around it...

9) also easily fixe^D^D^D^Dworked around.


let me catch some shuteye, then I'll see what I can do about 4 and 5. I must
have broken it somewhere, though I don't get any warnings or anything in the js
console. Maybe venkman can straighten me out.
Comment on attachment 58060 [details]
a few small tweaks, all the correct files

parameter names still need to be changed to awindow and adocument. 
Also you should be aware that document.compatMode is only available for HTML
documents, so you should set the textbox value to "Strict" for all other kind
of documents (XML, XUL, ...). Just do something like this:
var mode = thebundle.getString("GeneralStrictMode");
if (thedocument.compatMode && thedocument.compatMode == "BackCompat") {
  mode = thebundle.getString("generalQuirksMode");
}
Apart from that it looks very very good. The last step for me is to actually
test it ;-) I'll do that later today.
Fabian, you mean:

if ("compatMode" in thedocument && thedocument.compatMode == "BackCompat")

right?  :)  Let's not have strict warnings on non-HTML docs.
Depends on: 110981
Depends on: 110973
Attached file ok, I think this is it (obsolete) —
fixes the last known bug in the forms tab, cleans up a few things. Fabian, bz:
is it time for sr= yet?
Attachment #58060 - Attachment is obsolete: true
Comment on attachment 58858 [details]
ok, I think this is it

1) There's a blank line between the xmlns and the windowtype line.
2) What happens when a tab is selected when the dialog is launched? Is the
function to populate the tab called in that case?
3) onselect attribute should be on <outlinerbody > not on <outliner>

bbaetz said he'd look at the networking code later on.

We are soo close :-)
1) The vertical alignment is still broken for label/textbox. The
   textboxes are about 2px lower here (modern theme).  In classic it
   seems to be better (1px or no offset at all).

2) The window title says "Frame Info" even when it's plain old page info

3) For file:/// urls there is no hostname so you just get a ":" there.
   Looks pretty ugly.

4) For forms with no name you get "Form :" -- looks pretty ugly.
   Maybe default name to some string like "[No Name]" or something?  Or
   only put that text in if the form name is nonempty?

5) <link rel="icon"> images are still not rendering in the image pane
   (try www.mozilla.org).  This is because the "href" attribute of
   <link> is a relative link (unlike the "src" attribute of <img>).  You
   may have to just get the base url from the document (falling back to
   the document url) and create the fully qualified uri yourself....  I
   _know_ we have code to do this, but I don't know whether it's
   scriptable.

6) Changing form being viewed changes the sizes of the two panes of
   the forms tab (try this bug page for example)

7) Resizing the window horizontally will expand the tabbox
   horizontally but only up to a point.  Past about 700px the tabbox will
   no longer grow.  Looks ugly and means that in the image tab I _always_
   have scrollbars in the preview area, since the gray data rectangle is
   wider than the 700px limit I'm hitting.  This is the cause of my
   comments about the margin of that grey box -- the margin was off the
   right edge of the visible area.

8) Removing columns from the outliners (using the doohickey in the top
   right) does not repaint them.  Removing a column then clicking on the
   rows or scrolling will repaint things more or less correctly.
I can help with 1), db48x: ping me on irc
2) is because the patch for bug 83017 has not been checked in yet. Maybe we
should just merge the two patches.
4) is easy enough to fix
7) is wierd because it works in the current page info. Maybe missing some flex's?
8): we're probably missing a reflow somewhere. I hope some outliner guy will
find what's wrong.
Hrm. I guess I have seen parts of this before ;-).
I gave a quick glance on the outlinerview. I didn't get what columnids is doing.
That could use a comment.
getCellText looks like it's prone to be slow. But as I don't know what it's 
supposed to do, I can't suggest an impovement. A datastructure that let's you
do 
return this.data[row][this.colidx[column]];
would definitly be faster here, but may be harder to maintain. Might be worth
a try.
Of course, I'd prefer to have stuff like while( vs. while ( done. Follow the 
coding style of the surrounding files.

Axel alias Pike
8) has been recently fixed. See bug 110033.
Fabian:
1) I guess that can be changed, if need be...

2) correct, the <tab/> elements have onselect="" handlers on them calling (for 
example) makeFormTab();

3) Easily fixed, it wasn't mentioned in any of the documentation I read. file 
another bug?

bz:
1) is easy, but I need you to be absolutely sure you have pageInfo.css 
installed in the correct place? Maybe you just have an outdated version or 
something? If so I'll have to update my tree and look at it when I get back. 
From your description, it may involve seperate css files for each theme. fun.

2) we could fairly easily merge in the changes to bug 83017

3,4) are probably a regression from a previous patch, I'll fix it. I think I 
just typed over the part where it puts (unnamed form) or what ever it had 
before. it's probably still in the properties file, same with the page title.

5) correct, I remeber seeing some code in the tree I can copy. should modify it 
some now that Node.baseURL or whatever it was works. simplifies things a bit.

6) I thought this was only happening in my old nightly that I use when my build 
crashes. Does the media tab exhibit the same behaviour?

7) I can play around with. I've never noticed it before. Perhaps we should just 
limit the width of the image to the width of the pane - margin, scaling as 
needed? I don't think there's an easy way to do that in css. could use a 
percentage width, but that wouldn't help the height much. maybe we could modify 
<image/> to take an attribute kinda like crop="scale" or something? Anyway, 
I'll look at it.

8) good, I was pretty sure I wasn't doing any thing wrong here. I'll double 
check on an updated build just to be sure.

Pike:
columnids is just a list of names that are given to the columns. unfortunaly, 
when the outliner needs to ask us a question about a given cell, it gives us 
the row number and the column name, as defined by the id="" attribute on the 
actual xul <outlinercol/>. Very much a pain. The only places it's used I just 
loop over it to find the index into the array for the column number. not 
particularly fast, but not terribly slow either O(n), and with just 4 or 5 
columns, it won't bother us. It just seemed like more of a bother to make a 
hash of "id"=>column index, so I didn't. Do you think I should? I will add 
comments to that effect in the code.

I'm still on vacation, but I'll check back,
db48x
I put the css files in themes/modern/navigator and themes/classic/navigator 
(that's where your jar.mn changes seem to look for them).  Is that incorrect?

I'd leave bug 83017 as a separate issue.  This patch is already pretty big....

As far as pane resizing, yes I've seen the media tab do the same thing.  It's 
more reproducible in the forms tab for me.
Do not know how to look at your patch untill it checked in...sorry...

But just want to bring again issue about 'user should be able to "reload"
every part of page independently'.
 I thinks it is easy to make "reload" batton in Page Info dialog,
 wich whould reload or shift+reload(no cache) selected images/htmls(without
images)/flash/applets/etc.
  Also (but not so argent as ^^^^^^^) It whould be just great to have "Save"
button, 
 which whould save selected objects to users HDD.
I like the patch, but I just have two points(I didn't look at the code).  If you
do page info for this Bug, it reports it's last modified time as the beginning
of time(or else pretty close I am not sure what the exact beginning of time is).
 Unless the page is actually 'correctly' saying that it was last modified at the
beginning of time, there should instead be an error message something to the
effect of "Last Modification not reported" or just "(not reported)" or
"(unknown)".   Also, Render Mode should have a shorter answer.  You aren't
saying "This document is of the type text/html" so you shouldn't have "This
document was rendered in compatibility mode."  Shorten it to something like
"Compatibility mode" and "Standards compliance mode".  My normal Page Info
window size cuts off the end of the sentence which is where the important part
is.  Otherwise, great work and it is a great improvement over the previous Page
Info window if for no other reason than you can now highlight text in the window
:). 

Also, I put pageInfo.css in chrome://skin/modern/navigator/.  I am assuming this
is correct since it works??
afaik chrome://skin/modern/navigator/ is not a correct url.
[chrome://package/portion/pathto/file] chrome://navigator/skin/pageInfo.css
package==navigator
portion==skin|content|locale|locale-region

however, chrome urls do eventually map to a file system (jars are zips and 
behave like file systems) where skin/modern/navigator is probably right.

nc4 says:
Location:
                 http://bugzilla.mozilla.org/show_bug.cgi?id=52730
   Last Modified:
                 Unknown
   Last Modified:
                 Unknown
         Expires:
                 No date given
        Charset:
                 Unknown

so "Unknown" is 4xp.
Another thing, if a page has a java applet in it than it breaks any tab which
the applet is supposed to add information to.  The page at
Debug->Verification->Applets has a blank media tab even though it should have
the mozilla banner, the site icon, and the applet.  The main splash page at
java.sun.com has a blank media tab as well as a blank links tab since there are
links in the applets.  I am using Build 2001112408 for Win2K.
Depends on: 112297
0.9.6 is long gone. -> 0.9.7
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Daniel, how about getting a patch that applies to the current trunk and check it
in as is. The requirements set by Gerv way earlier are by far reached and it
works in 99% of the cases, if not more. The code has been spanked more than
enough already.
I vote for getting it in for 0.9.7.
yea, sorry about the delay, had a bit of a cold.

anyway, I'll attach a patch shortly with the lastest, gotta check out just to be
sure.
Attached file final patch - one known bug (obsolete) —
The only bug this has that I know of is that images specified via a <link> tag
(the ones that show up in the url bar) don't show up in the media tab. They're
listed, but no image shows in the preview pane. Also, items included via
<embed> tags don't show up either, but I'm not sure that's a bug.

so, is this good enough to check in for 0.9.7? There really isn't much else to
do with it, though I would like to be able to sort the columns in the various
lists. I'll gladly do that in a different bug though.
Attachment #58858 - Attachment is obsolete: true
Comment on attachment 61704 [details]
final patch - one known bug

Take out the dump of link tab construction time.  Fix the string warning about
milliseconds.  After that, r=bzbarsky
Attachment #61704 - Flags: review+
Attached file /me sighs (obsolete) —
You would think I'd at least remember to check for errant dump()s first :P
Attachment #61704 - Attachment is obsolete: true
Comment on attachment 61709 [details]
/me sighs

One nit that should very easily be fixed:
The license you use is not the tri-license. In pageInfo.js, you even remove the
new, correct tri-license with the old dual-license.
There are other coding nits that don't affect functionality that I'll tell you
about in a follow-up bug.
r=fabian with the license fix. I tell you, this is a nice piece of js.
Attached file license fix (obsolete) —
I just now updated the licenses, the one in pageInfo.js had gotten cropped in a
copy-paste (the begin/end license block bits were missing, everything else was
the same)
Attachment #61709 - Attachment is obsolete: true
The readme.txt says to put the pageInfo.css under themes/classic and themes/modern
but the jar.mn says they should be under themes/classic/navigator and
themes/modern/navigator. Not a biggie, but it caused me a build problem until I
realized...
Ah, yes, that was my mistake. pageInfo.css should go in themes/classic/navigator
and themes/modern/navigator.
Have you looked at using <dialog> instead of <window class="dialog">?

Also, whoever checks this in should make sure the right stuff happens for the
Mac classic skin.

CC'ing some people who I feel are more appropriate for sr'ing this code.
Yes, we did, but we pretty much decided that it's not really a dialog because 
it's not asking the user for information. What exactly does <dialog> do that 
<window> doesn't? I wasn't really able to tell.
We really don't need to use <dialog> here, but you should also remove the
dialogOverlay reference from pageInfo.xul, since that should not be needed at
all for this window.
Argh, need to change the outliner syntax to use <outlinercols> to group
<outlinercol>. (bug 113477). Afraid there needs to be a patch update to address
this comment and hewitt's :-(
Arrg. I'll have to do that when I get home.
*** Bug 116226 has been marked as a duplicate of this bug. ***
Depends on: 77408
I'll be back from vacation soon.
Target Milestone: mozilla0.9.7 → mozilla0.9.8
*** Bug 117137 has been marked as a duplicate of this bug. ***
Depends on: 117027
What are the plans on this patch? Do you still plan to land it for 0.9.8?  It's
been around for quite some time, and I look forward to see it in the nightlies.
Attached file fix xul syntax, remove dead overlay (obsolete) —
and that should do it.
Attachment #61752 - Attachment is obsolete: true
I can't review this till Jan 7 or (more likely) Jan 8.  Håkan, Fabian, would you 
take a look?
I vote for landing this sometime next week - when Fabian, bzbarsky and I have
had a look. I'll check it out next weekend [when I get back from vacation].
fwiw, I just now fixed the readme file so that it shows the correct paths for
pageinfo.css, but I'm not going to bother attaching it.
Comment on attachment 63215 [details]
fix xul syntax, remove dead overlay

Assuming everything still works with the new xul syntax and the removed
overlay, my r= still holds.
Attachment #63215 - Flags: review+
Shouldn't pageInfo.xul be tri-licensed? 

r=hwaara
Yeah please fix the stoopid license please. Please. Grr. This HAS to end someday :-)
Oh, I just noticed pageInfo.properties has the wrong license as well. Darn.
I left the licenses the same as in the existing files, figuring that they were
still the old ones because permission from contributors hadn't been obtained.
It's no trouble to change them though...
*** Bug 118590 has been marked as a duplicate of this bug. ***
For tester's convenience I have attached a patch that includes also the new
files. In other words, you only need to apply this patch to help test this. The
patch also includes the new line in mozilla/themes/modern/jar.mn and
mozilla/themes/classic/jar.mn. Now I'm going to test it:-)
With this patch applied my build dies with:

+++ making chrome /home/andre/devel/cvs/mozilla.pageinfo/themes/classic  =>
../../dist/bin/chrome/classic.jar
error: file './navigator/pageInfo.css' doesn't exist at
../../config/make-jars.pl line 207, <STDIN> line 257.
make[4]: *** [libs] Error 2
make[4]: Leaving directory `/home/andre/devel/cvs/mozilla.pageinfo/themes/classic'
make[3]: *** [libs] Error 2
make[3]: Leaving directory `/home/andre/devel/cvs/mozilla.pageinfo/themes'
make[2]: *** [tier_9] Error 2
make[2]: Leaving directory `/home/andre/devel/cvs/mozilla.pageinfo'
make[1]: *** [default] Error 2
make[1]: Leaving directory `/home/andre/devel/cvs/mozilla.pageinfo'

Is is perhaps so that pageInfo.css should be copied to
themes/<skin>/navigator/pageInfo.css instead of themes/<skin>/pageInfo.css as
the readme says? Or are these instructions in the readme wrong:

"then add the following line to the end of mozilla/themes/classic/jar.mn:
    skin/classic/navigator/pageInfo.css              (navigator/pageInfo.css)"

Should the "navigator" part be removed there?



and likewise with mozilla/themes/modern/jar.mn:
  skin/modern/navigator/pageInfo.css                              
(navigator/pageInfo.css)
The README is wrong.  See comment #126
This is the complete patch with pageInfo.css in the right directory this time.
Attachment #64743 - Attachment is obsolete: true
db48x, I just noticed the pageInfo.css file has <!-- --> for the license blocks.

CSS uses /* */ comment syntax, not HTML syntax.
Either I did a misstake with my version of the patch or this Page Info patch is
not quiet working yet. In my linux build with this patch applied all the fields
in Page Info are blank and the fields are also shifted too much to the right. I
will attach a screenshot that shows this.
bother, I goofed somewhere. the comment block at the top of pageInfo.js is
messed up. Add */ to the end of the first line (the modeline), then remove the
first /* from the end of the license block.

As for the css file, it doesn't seem to matter either way. You are correct that
<!-- --> is not technically a valid comment in css.

Also, check your copy of the css file, if it's applied correctly then the
textfields on the general tab won't have boxes around them.
oh, and now I notice a message in n.p.m.xpfe about outliner syntax changes. I
only have a nightly on this computer, and it's a few days old, so maybe if you
have a newer build you're also seeing that.

this thing really really needs sr= so it doesn't keep bitrotting!

I'll be able to produce a patch in the morning. Meanwhile, there are two changes
you may have to make: first, change all <outlinerbody>s to <outlinerchildren>,
and second, move the onselect="" properties from the <outlinerbody>s to the
<outliner>s themselves.
the outliner syntax changes have not landed yet, but they will soon :-(
have you asked hewitt for sr= yet? I'm sorry I can't test the new patches, only
review the code, cause I don't have a tree anymore. I so wish we were done with
this already.
I fixed the comments in pageInfo.js and pageInfo.css and rebuilt, and now it's
working nicely. One visual bug, which is similar to what bzbarsky already
pointed out as item 4) in that a : will be where the page title should be if
there is no page title (such as in about:blank). Just to be over clear I will
attach a screenshot of that.

Anyone want me to attach the entire patch which is now working and has been
cleaned from windows line feeds and had comments fixed?
Attached file /me thwaps himself (obsolete) —
dunno why the page title bit was still messed up, but I have to maintain two
copies of everything because this has taken so long.

As for the css, it truely does not matter what kind of comments you use. There
must have been something else messed up about the copy in your tree. Who knows.
Attachment #63215 - Attachment is obsolete: true
> As for the css, it truely does not matter what kind of comments you use.

Yes, it does.  Using HTML comments is invalid CSS and will lead to some rules
(usually the one right after the HTML comment) being ignored.

Please use CSS /* */ comments.

Andre, a patch that is ready to check in (with correct line endings) is a good
thing to have around.  :)
bz: I certainly agree that it is invalid. However, as there are currently no ill
effects (all the rules get parsed and used), I'm not sure that now is the best
time to worry about it, unless not doing so will hold up an sr. I'd rather just
do it in another bug, if that's ok.
Don't worry about it.  I'll fix the comments before checking this in once it has
sr.
Nice work, Daniel! Just a few small nitpicks:

Don't load chrome://navigator/skin from pageInfo.xul. Just load pageInfo.css,
and then import chrome://communicator/skin/ from pageInfo.css. We generally
import one css file per xul file if possible, and import all other sheets from
the skin css.

In some places you have variables like somevar and other places like someVar. 
Can we try to use uppercase letters at word boundaries consistently?

Is there a good reason why you need to throw an exception for the rowCount
setter in pageInfoOutlinerView?

Attachment #64873 - Attachment is obsolete: true
Comment on attachment 65243 [details]
fix Hewitt's nits, except for the varible names

It is really annoying when people break our general code conventions, so I'm
hesitant to let those var names go.  Try to watch for that in the future.

sr=hewitt
Attachment #65243 - Flags: superreview+
I'm going to cancel out hewitt's sr and insist that you fix the variable names,
either make them all use interCaps, or make them all lowercase. Since the
original style was interCaps, it would make sense for you to use interCaps too.
Another reason for choosing interCaps, or some other form of clearly separating
words in composed variable names is readability; compare "cacheentrydescriptor"
with "cacheEntryDescriptor" or "cache_entry_descriptor".

Along similar lines of reasoning, either use

/*
*
*
*/

or

/*
 *
 *
 */

with again the latter preferred.

The same goes for placement of braces: you're mixing same-line style (old code)
with next-line style (new code). Please make it so that it's consistent, either way.

And again for space between keyword and open parenthesis:

+  for (i = 0; i < length; i++)

+  if (outliner.outlinerBoxObject.selection.count == 1)

+    if(linklist[i].rel.toLowerCase() == "icon")

I would prefer the former style, but choose whichever style you want, as long as
you're consistent with yourself and the prevailing (original) style in the file.

+    var ft = null;
+    if (form.name)
+      ft = thebundle.getFormattedString("formTitle", [form.name]);
+    else
+      ft = thebundle.getString("formUntitled");
+
+    document.getElementById("formname").value = ft ||
thebundle.getString("formUntitled");

No need for the || on that last line, that's guaranteed by the |else| right
above it. Also no need to initialize |ft| to |null| here.

I'll take a closer look at the code itself later today, but I wanted to get the
above list of nits to you before you check in.
Attached file fixes jag's nits, one other weird bug (obsolete) —
I had said I'd change the varible names to get the sr. Also fixed the other
minor syntax variances, thanks for pointing them out.

The other weird/interesting bug happened because getCellText() on my
outlinerview was being called for columns 0-4, even though I only have 4
columns. As a result it was showing undefined in the "column" underneath where
the little colpicker widget goes. (though it was being cropped to "u...").
Nothing like Mozilla to keep you on your toes, this was a recent regression.

Anything else?
Attachment #65243 - Attachment is obsolete: true
It was unclear (from comment 162) that you were in fact going to fix the
variable names. Thanks for doing so.

sr=jag
0.9.9 it is
Target Milestone: mozilla0.9.8 → mozilla0.9.9
I just applied the patch and checked it out (planning to check it in if it
worked as advertised).  I keep getting:

###!!! ASSERTION: NS_ENSURE_TRUE(aContent) failed: 'aContent', file
nsFrameManager.cpp, line 607
###!!! Break: at file nsFrameManager.cpp, line 607
###!!! ASSERTION: NS_ENSURE_TRUE(aContent) failed: 'aContent', file
nsFrameManager.cpp, line 607
###!!! Break: at file nsFrameManager.cpp, line 607
JavaScript error: 
chrome://navigator/content/pageInfo.js line 848: this.outliner has no properties

Dunno whether that's an outliner bug or whatnot, but the upshot is that the
outliners are empty, hence no lists of images/forms/links.

Daniel? Jan?  Thoughts on what's up?
hmm, I took quick look and I found out that there is something wrong.

<outlinerbody flex="1" onselect="onFormSelect();"/>
this is obsolete, should be just:
<outlinerchildren/>

and it seems it could be implemented using outliner content model.
hmm, ignore me. let's get it in finally!
anyway, please fix it to use latest syntax.
this changes the <outlinerbody>s to <outlinerchildren>, moves the event
handlers to the <outliner>s, and adds a check to make sure the view actually
has data in it before asking for the contents of a cell. Apparently the
onselect handler is being called now even when there are zero rows in the
outliner, the check wasn't necessary before.
Attachment #65366 - Attachment is obsolete: true
Attached file one of these days...
I forgot to copy the updated xul file into the tree before I diffed.
Attachment #66250 - Attachment is obsolete: true
r=bzbarsky for the update to the new outliner syntax, tested on all the deps of
this bug, and checked into trunk (the outliner syntax change was basically a
search-and-replace).

yay! finnally checked in. Thank you bz :)

(and this should be my last bit of spam for the night)
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Wow, I think I've never seen so many bugs get fixed at once! Great work db48x!
Congratulations, db48x, for seeing this major improvement through to completion!
 It joins a whole bunch of other good improvements and bug fixes that have been
checked in over the last couple of weeks, which I think are "turning the corner"
for Mozilla in making it an excellent browser.  I've been trying Mozilla builds
since a very early stage in the development process.  The earliest builds were
unusable... after a couple of years of development they became somewhat
tolerable... finally, a few months ago, they were sufficiently stable and usable
that I made it my default browser, but many rough edges still existed.  But you
developers have been doing a great job at smoothing them over, while adding some
neat new features.  (Now, if only you could get the View Source feature to work
correctly for dynamically-generated pages...)  I think it's time for everybody
to start evangelizing Mozilla... not necessarily to the non-geek general public
yet (until 1.0, it's still in the testing stage and not quite a consumer
product), but to computer enthusiasts who tried earlier Mozilla builds or
Netscape 6.x releases, found lots of problems, and have since been filling the
newsgroups and mailing lists with "Mozilla sucks!" messages.  It's time they
tried Mozilla again.... they'll be surprised at how it's come along.
Before I finally move my vote elsewhere, allow me to abuse your inboxes just
once to say three cheers for Mr. Brooks and his Sisyphean patience.

Hip hip hooray!
Hip hip hooray!
Hip hip hooray!
mass moving resolved fixed bugs pertaining to page info, view source, find in
page and open web location to pmac@netscape.com.

to find all bugspam pertaining to this, set your search string to
"SunsComingUpLikeABigBaldHead".
QA Contact: sairuh → pmac
No longer depends on: 96994
Daniel: 

Every time I saw email in my mailbox from this bug and read it and saw that
there was still more you had to do for this bug, I though "Poor Daniel". I
wanted so much for you to finally get this checked in since you have been
working on this forever. Finally, you got it in and I'm so happy for you! Keep
up the good work! You have really stuck it out on this one.

YAAAAAAAAAAAAAAAYYYYYYYYYYYYYYYYYYYYYYYYYYY!!!!!!!!!!!!!!!!!!!!

And we need some smileys!

:) :) :) :) :) :) :) :) :) :)
Component: XP Apps: GUI Features → Page Info
It's great !!! (I got nightly version)

But still I whould ask for some features, wich was mentioned in bug #90408
These features could be implemented as context menu or separate dialog buttons.
1) Copy one or more selected rows to clipboard/selection
  (this this to copy URLs  and Data
 http://cad.ntu-kpi.kiev.ua/avhdl/ - here meta description/keywords are longer
then screen size, so one even can't read it all)

2) For "Media" section - do not select fist line by default
  (on same URL - it is JavaApplet, which is not so safe to run by default :)

3) For "Media" please add "Reload" and "Force Reload" (shift+reload)
   (it is Very essential for JS, CSS and Java debugging/developing)

4) In "General" - add list "HTTP response headers"
   (actually those headers may conflict with META data in HTML, so it whould be
just great to have them both listed in same place (and hilight if they difer -
see Content-type on same URL))
   Also ... It is just interesting to be able to see "Server:" header. ;)

If you whould ask about priorities... (3) is the one I really need.
malvin@cad.ntu-kpi.kiev.ua, please file separate bugs, one per issue, in the
page info component.
Verified (2002-02-02-22-trunk)
Status: RESOLVED → VERIFIED
Filed bug 131211 covering the 'server:' header thing.
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: