Bug 87428 (LinkUI)

No UI HTML <link> element

VERIFIED FIXED in mozilla1.0

Status

SeaMonkey
General
P3
normal
VERIFIED FIXED
16 years ago
11 years ago

People

(Reporter: gerv, Assigned: Eric Hodel)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Hixie-P2] SPEC: http://www.bath.ac.uk/%7Epy8ieh/internet/discussion/linkspec.txt, URL)

Attachments

(33 attachments, 2 obsolete attachments)

14.81 KB, application/x-xpinstall
Details
20.90 KB, application/x-xpinstall
Details
5.27 KB, application/octet-stream
Details
15.30 KB, application/x-xpinstall
Details
36.18 KB, patch
Details | Diff | Splinter Review
4.49 KB, text/plain
Details
11.79 KB, patch
Details | Diff | Splinter Review
1.48 KB, text/xml
Details
15.97 KB, application/x-xpinstall
Details
32.90 KB, patch
Details | Diff | Splinter Review
1.48 KB, application/xhtml+xml
Details
680 bytes, patch
Details | Diff | Splinter Review
36.10 KB, patch
Details | Diff | Splinter Review
31.75 KB, patch
Details | Diff | Splinter Review
19.60 KB, application/x-xpinstall
Details
16.17 KB, patch
Details | Diff | Splinter Review
2.45 KB, application/octet-stream
Details
21.04 KB, application/x-xpinstall
Details
40.00 KB, patch
Details | Diff | Splinter Review
21.33 KB, application/x-xpinstall
Details
4.44 KB, patch
Details | Diff | Splinter Review
5.87 KB, patch
Details | Diff | Splinter Review
24.01 KB, application/x-xpinstall
Details
80.63 KB, patch
Details | Diff | Splinter Review
13.29 KB, application/zip
Details
59.18 KB, patch
Details | Diff | Splinter Review
2.91 KB, application/octet-stream
Details
58.60 KB, patch
Details | Diff | Splinter Review
58.67 KB, patch
Details | Diff | Splinter Review
58.75 KB, patch
Details | Diff | Splinter Review
60.90 KB, patch
Details | Diff | Splinter Review
3.99 KB, text/plain
Details
70.06 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

16 years ago
Continuation bug from bug 2800, which was getting unwieldy.

Current implementation:
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=39798

GNU Make manual testcase:
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=39630

Note: bug 2800 had 60 votes.

Newsgroup discussion of the current implementation has been started in 
netscape.public.mozilla.ui. Let's thrash this out there :-)

Gerv
(Reporter)

Updated

16 years ago
Blocks: 7954, 62399, 68419
Status: NEW → ASSIGNED
Keywords: mozilla1.0
Priority: -- → P3
Whiteboard: [Hixie-P2] SPEC: http://www.bath.ac.uk/%7Epy8ieh/internet/discussion/linkspec.txt
Target Milestone: --- → mozilla1.0
(Reporter)

Comment 1

16 years ago
*** Bug 2800 has been marked as a duplicate of this bug. ***

Updated

16 years ago
Blocks: 59118
(Assignee)

Comment 2

16 years ago
Current issues for this bug:
  UI design (to be discussed in n.p.m.ui), this includes which rel types get
displayed where.
  Display of non-HTML4 rel types (n.p.m.ui)
  Tooltips
  Localization
  Icons
  Implementation of keyboard shortcuts (which keyboard shortcuts is for n.p.m.ui)

Lets please keep traffic on this bug to actual implementation details.  UI is
for the newsgroups because its mostly subjective.
(Assignee)

Comment 3

16 years ago
Created attachment 39828 [details]
Skin support added w/icons
(Assignee)

Comment 4

16 years ago
This is the latest latest implementation.  Skin support has been added.  Your 
best bet is to install from a clean Mozilla.

Comment 5

16 years ago
ack! un-cc myself.

Comment 6

16 years ago
Created attachment 39879 [details]
Same as above, only doesn't assume /bin directory (works on Mac)

Comment 7

16 years ago
Created attachment 39892 [details]
This is a zip of some icons for classic.  Bookmarks don't have a hover state, so I didn't make them, but I have yet to see a disabled bookmark, so I made disabled icons just in case.

Comment 8

16 years ago
Has anyone been able to install from the attachments? I managed to do it from
http://segment7.net/mozilla/linktoolbar/index.html but none of the attachments
upgrade it.I think I'm missing a step.

Comment 9

16 years ago
Oops, I screwed up and misunderstood the install script.  The right thing to do
is to save the original attachment to a local file ending in .xpi and then open
it in mozilla.  Just clicking on the link fails with a -207 error in the install
log (on mac build 2001062008).

Comment 10

16 years ago
Peter, I had to add the following lines to installed-chrome.txt before the 
toolbar worked:

content,install,url,jar:resource:/chrome/linktoolbar.jar!/content/linktoolbar/
locale,install,url,jar:resource:/chrome/linktoolbar.jar!/content/en-us/linktoolb
ar/

Does that help?

Comment 11

16 years ago
Just a quick warning: we backed out the wallet toolbar because it would appear 
and disappear which drove people nuts.  If you support autoshow (it's not 
autohiding, since the functionality is that it shows up whenever it feels like 
annoying users, not disappears as a convenience to users), you can not use that 
as your default.  I would suggest having another discussion specifically about 
this, but you will need to run a user study on it.  The bug for wallet should 
be easy to find.

Comment 12

16 years ago
The current toolbar does not handle multiple links for Top, Next, etc. or 
contents. Also, the title doesn't show up for unrecognized link types.
I've fixed the title for unrecognized link types and added some other improvements.

I intentionally didn't support multiple links for certain link types.  Currently
the last occurance of the same link type is used.  I've changed that to use the
first occurance instead.  Somewhat related, links that have the same REL, HREF,
MEDIA, and HREFLANG are treated properly, i.e. only one will be used.  They can
differ by title, which is intentional.  Only the first link encountered will be
used.

Likewise, in most cases there is only one Table of Contents, Index, and
Glossary.  So I have changed Indices and Glossaries to Index and Glossary
respectively and made them menuitems instead of menus.

I mentioned in bug 2800 that I made it easy to switch the toolbar items between
button, menuitem, and menu.  A planned improvement will make it even
simpler...just change the XUL.  In other words, maybe we can defer the debate on
this until the UI reviewers come around, which will be after we think the
toolbar is working good enough.

Tooltips are working again.  I broke them when I did my rewrite.  

I'll attach a new JAR or XPI with these and other changes within 24 hours.
Created attachment 40168 [details]
Tim's revised XPI (changes, bugs below)
Created attachment 40170 [details] [diff] [review]
patch against the previous XPI to get to Tim's revised XPI (comments below)
OK, there's my latest XPI.  Either wait until Eric incorporates all or some of
the changes on his Link Toolbar page or read the previous discussion on how to
install this XPI from the attachment (just clicking on the attachment may work
for you).

I also added a patch of the changes I made.  The diff was made against the
contents of linktoolbar.jar in the previously attached XPI, the one labeled
"Same as above, only doesn't assume /bin directory (works on Mac)".  Here's the
command I used to make the patch

  $ diff -ruN linktoolbar-old linktoolbar

On your end either 'patch <linktoolbar.patch' or 'patch -p1 <linktoolbar.patch'
should work depending on whether you extracted the contents of linktoolbar.jar
into a directory called linktoolbar.


Changes:

* skin improvements:
    - fixed Modern horkage, but see bugs below
    - removed most hard coded styles and use XUL classes properly
    - toolbar items look like they do in personal toolbar
    - More menu looks like a bookmark folder
    - created disabled versions of home, bookmark-folder and bookmark-item
        icons
* handle equivalent links as duplicates.  Links are equivalent if
    they match on REL, HREF, MEDIA, and HREFLANG attributes
* tooltips work again and labels in the Miscellaneous menu should work
    properly now
* added toolbar separators, but see bugs
* the More menu is disabled if it contains no active items.
* added Search menuitem to More menu.  Uses REL="Search".
* renamed:
    Authors => Author
    Contents => Table of Contents
    SubSections => Subsections
    Indices => Index              (menuitem instead of menu)
    Glossaries => Glossary        (ditto)
    Alternates => Other Versions

    The idea is that plural nouns are for menus and singular nouns for
    menuitems.


Known bugs:

* Modern theme uses Classic skin for linktoolbar.  Help needed.
* toolbar separators displayed without vertical bar (border).  Help needed.
* toolbar only gets updated when the document has completely loaded.  It
    should update incrementally  as the document is loaded.
* browser is unresponsive for 2 seconds or more while updating the toolbar
  on the GNU make Manual testcase
* HTTP LINK header ignored
* Drag & Drop should work like the Personal Toolbar
Created attachment 40212 [details]
Description of link types to support.
I've attached a document which classifies the various types of links we should
support, and what they do.

Tim, is there any possibility of your adding:
1) autogenerating toolbar initialization from XUL
2) Changing the parsing so that if you have one "contents"/"toc" link, say, you
get a single menuitem, but if you have multiple items, the menu cascades?

If not, I'd be willing to work on both of these changes, but I don't have a CVS
installation I can use right now and can't generate diffs, so I'm trying to
manually avoid conflicts with other people working on the code.
Am I the only one to find the various "standards" confusing, ambiguous, and
sometimes contradictory on the meanings of the link types?  Of course, the only
standard to date is the HTML recommendation.  The rest are just drafts or
people's recommendations.

We're at least in agreement on the navigation UI: [site homepage], Up, First,
Previous, Next, Last.  But the specs offer little help for mapping REL values to
these.  I'm frustrated because the most important type -- a link to the current
site's homepage -- has /no/ clear and obvious corresponding REL value.  The
obvious choice, Home, is deprecated.  The next likely candidate, Start, suffers
from the vagueness of W3C-speak.  Falling back to the IETF draft we learn that
Start is overloaded to mean either the site homepage *or* as a synonym of First.
 Ditto that for Top.

I suggest we build a Link Toolbar that makes sense to the user.  The goal is to
make it easier to navigate.  We should be careful to comply with the HTML 4.01
recommendation and should avoid behaving badly with the pages of early adopters
of LINK types.  We can use the other drafts and recommendations for guidance
/when/ they actually provide guidance.

That said, thank you, Chris, for putting together that summary on link types. 
Now we need an unambigous mapping of REL attribute values to our link types. 
I'll kick start it by asking someone to suggest the proper newsgroup for this
discussion, and by providing the current implementation:

    switch (relAttribute) {
      case "home":
      case "start":
      case "top":
      case "origin":
        return "top";

      case "up":
      case "parent":
        return "up";

      case "begin":
      case "first":
        return "first";

      case "next":
      case "child":
        return "next";

      case "prev":
      case "previous":
        return "prev";

      case "end":
      case "last":
        return "last";

      case "author":
      case "made":
        return "author";

      case "contents":
      case "toc":
        return "toc";

      default:
        return relAttribute;
    }
> Tim, is there any possibility of your adding:
> 1) autogenerating toolbar initialization from XUL

Yes.

> 2) Changing the parsing so that if you have one "contents"/"toc" link,
> say, you get a single menuitem, but if you have multiple items, the
> menu cascades?

I would rather not, because I'm opposed to that feature.  I explain why in
n.p.m.ui.  I believe Eric liked the idea or, as you say, you might do it yourself.

> [...] I don't have a CVS installation I can use right now and can't
> generate diffs, so I'm trying to manually avoid conflicts with
> other people working on the code

I know what you mean.  However, you don't need CVS to make a patch. Assuming you
have the standard GNU tools just do:

  $ diff -u old-file new-file >file.patch              # or
  $ diff -ruN old-directory new-directory >file.patch

Comment 21

16 years ago
If you care to study my now-nine-months-old working implementation of the link
toolbar:

http://bugzilla.mozilla.org/showattachment.cgi?attach_id=14701

you will see just such a switch statement, which I hope may be of some guidance
in the new work.

Comment 22

16 years ago
Do I alone notice the irony that this work is nearing completion just in time to
miss another Netscape release?
(Reporter)

Comment 23

16 years ago
> Am I the only one to find the various "standards" confusing, ambiguous, and
> sometimes contradictory on the meanings of the link types? 

No. I just added LINK support to Bugzilla (bug 87818) and couldn't work out 
whether to have returning to the Buglist as "Home", "Contents" or something 
else. Also, there seems no official value to represent "Last" in the HTML 4 
spec, as Christopher Hoess points out in his spec.

> I'm frustrated because the most important type -- a link to the current
> site's homepage -- has /no/ clear and obvious corresponding REL value.

I suggest we recognise "Root" or "Origin", which I believe the Links draft 
suggests. But yes, this is a horrible situation, and needs to be addressed in 
the draft. Who is drafting it?

Gerv

Comment 24

16 years ago
At http://www.w3.org/TR/REC-CSS2/about.html , the "More" menu is selectable,
with the sub-options "Table of Contents", "Index", and the "Miscellaneous" menu
not disabled. But there doesn't seem to be a way of seeing what is in the
Miscellaneous menu, nothing pops up when I mouseover it.

Using the latest attachment with Mozilla 0.9.1.



Hmm...at <http://www.w3.org/TR/REC-CSS2/about.html> I see the following in the
Misc menu:

CSS-properties: properties
biblioentry: HTML40

Which reminds me that I need to go find Chris' code that hid certain types
(CSS-properties shouldn't show up).

Anyhow, it works for me.  Can you check your JavaScript console (Tasks -> Tools
-> JavaScript Console) for error messages coming from linkToolbarOverlay.js?

Comment 26

16 years ago
Actually this CSS-Properties is the pointer to an additional index (of CCS
properties). So it should show up under Misc. I would also vote for displaying
multiple index, toc etc entries in some low level menu instead of displaying
only the first (or last).

So that it would make sense to change the W3C site to use a second rel=index
with a TITLE "CSS-Properties".

Comment 27

16 years ago
Okay, so sometimes the Misc menu works (on the CSS2 page), sometimes it doesn't.
When it doesn't I get no visual feedback from the More menu at all, i.e. when I
scroll down the menu, the menu item I am over does not get highlighted. This
seems to be different in the first window I open from other windows (I think).
So perhaps it's an XUL bug?

Also, when I hover over a button which is disabled (eg First, Previous...) I get
a "hand" cursor (the same cursor as for a link). This should be the normal cursor.
Created attachment 40260 [details] [diff] [review]
patch to above XPI to get localization working
> So perhaps it's an XUL bug?

I wouldn't totally discount that I may be doing something wrong.  I assume those
times when it doesn't work, there's no messages in the JavaScript Console or you
would have said something about them, right?  ;-)

> Also, when I hover over a button which is disabled...this should be the
> normal cursor.

This is fixed in the patch I just attached for localization support.

Comment 30

16 years ago
Given many feel the specs are ambiguous, I'd say "ask the w3c working group".

Now I've heard (as in, I don't _know_ but I've been told) that the HTML working
group is a relatively closed list. This would suggest someone with membership
needs to raise the issue.

Who at Netscape has HTML working group membership? Are they cc:'d on this bug?
Can they find the time to help?

You should try to touch base with them to make sure they know about the
problems, and to make sure if its going to be addressed in some future XHTML
draft we remain as forward compatible as possible.

Speaking of which, if one is going to introduce terms for REL types, consider
using the -moz- (-moz-foobar) style used in CSS for vendor specific extensions?
(Assignee)

Updated

16 years ago
Depends on: 87818
(Assignee)

Updated

16 years ago
No longer depends on: 87818
(Assignee)

Updated

16 years ago
Blocks: 87818
(Reporter)

Updated

16 years ago
No longer blocks: 87818
(Reporter)

Comment 31

16 years ago
*** Bug 57399 has been marked as a duplicate of this bug. ***

Updated

16 years ago
Blocks: 57399
Gervase, please don't try and make support for REL on anchor elements an
essential part of this bug.  We already have enough on our plate.  Supporting
anchors is not without its problems or opponents.  It was correctly a
seperate enhancement, bug 57399.
No longer blocks: 57399
(Reporter)

Comment 33

16 years ago
Tim,

That bug was duped against this one because this bug fixes that one.

mpt (important UI design guy) says, over in the other bug:

"So could this bug be fixed just by treating A REL/REV in the same way we treat 
LINK REL/REV -- showing those links in the Links Bar, in addition to their usual 
rendering?"

the answer to which is blatantly "yes". The current code does it, and the only 
issue you have raised with it is a performance one. There is a comment in the 
newsgroup which, if implemented, will double the speed straight off the bat - 
and I'm sure that's only the start. 

If you have conceptual problems with this, then again they need to be hashed out 
in the newsgroup - but I just can't see why you want to differentiate between <A 
rel="next" href="next.html">Next</a> and <LINK rel="next" href="next.html" />. 
These should surely trigger the same browser navigation UI.

If drbrain, to whom this bug is assigned, rips <A> support out of the links 
toolbar, we'll reopen the other bug. 

Gerv
Following up to various commentary:

CSS-Properties should indeed be shown; why they aren't using rel="appendix" for 
it, I don't know, but the w3c pages do seem to have a few link types that have 
never appeared in any specification or draft.

Re: clarification from the HTML working group: we could ask the question on www-
html as well, which some members of the WG read.  Both Hixie and dbaron are W3C 
members and could bring it to the WG for us; I'd suggest Hixie, as he seems to be 
following the newsgroup discussion.  Perhaps we should even consider writing our 
own draft of what the various values should indicate and see if it could get 
published as a W3C Note.  (Hixie would know more about the feasibility of this.)  
Any such draft would have to allow leeway for continued extension: people will 
always be using "rel" values that we don't recognize for human-readable 
documents, and we'll have to continue to support this.  Given that even the W3C 
seems to feel comfortable making up new values, I don't think we really need to 
worry about marking values as vendor-defined.

I still think that we should not be ignoring "rel" values in the toolbar just 
because they're in anchors rather than links.  I see this toolbar as being a 
navigation instrument for the entire document, and rel="index" is rel="index", 
regardless of where it is located in the document.  Given Tim T.'s example (in 
the NG) of a Slashdot page where this significantly affects page load, I do agree 
that we need some performance help.  Our first step should be to make the 
suggested change to the scope of the getElementByTagName calls (see NG) so that 
links are only searched for in the head and anchors only in the body; once we see 
what the performance is for that case, we can discuss other options (DOM 
mutation, XBL, as discussed in NG.)
> There is a comment in the newsgroup which, if implemented, will double the
> speed straight off the bat

No, it doesn't.  Here's a benchmark:

  document.getElementsByTagName("LINK")
  document.getElementsByTagName("A")
   LINK  |   A
  ----------------
   97 ms | 1340 ms
   95 ms | 1329 ms
  116 ms | 1366 ms
   96 ms | 1353 ms
   96 ms | 1378 ms


  HEAD.getElementsByTagName("LINK")
  BODY.getElementsByTagName("A")
   LINK  |   A
  ----------------
   49 ms | 1339 ms
   49 ms | 1388 ms
   49 ms | 1359 ms
   48 ms | 1357 ms
   69 ms | 1322 ms


So it retrieves LINK elements on average 50 milliseconds faster.  That's good,
and it's the correct behavior.  I'll switch over to using the correct approach
for both link and anchor elements.

Here's the sourcecode for the test so you can verify the results:

function testRefresh() {
  profile("iterateLinkElements", iterateLinkElements);
  profile("iterateAnchorElements", iterateAnchorElements);
}

function iterateLinkElements() {
  var document = window._content.document;
  //var nodeList = document.getElementsByTagName("LINK");

  var node = document.firstChild.firstChild;  // document -> HTML
  while (node.tagName != "HEAD") {
    node = node.nextSibling;
  }
  var nodeList = node.getElementsByTagName("LINK");

  iterateNodeList(nodeList);
}

function iterateAnchorElements() {
  var document = window._content.document;
  //var nodeList = document.getElementsByTagName("A");

  var node = document.firstChild.firstChild;  // document -> HTML
  while (node.tagName != "BODY") {
    node = node.nextSibling;
  }
  var nodeList = node.getElementsByTagName("A");

  iterateNodeList(nodeList);
}

function iterateNodeList(nodeList) {
  for (var i = 0; i < nodeList.length; i++)
    nodeList.item(i);
}

function profile(name, func) {
  ddump(name + " begin");
  var start = new Date();

  func();

  var finish = new Date();
  ddump(name + " end");
  ddump(name + ": completed in " + (finish.getTime() - start.getTime()) + " ms")}

function ddump(msg) {
  dump("LinkToolbar::" + msg + "\n");
}


To run the benchmark yourself:

  1. add the above code to linkToolbarOverlay.js
  2. modify linkToolbarOverlay.xul to call testRefresh() instead of refresh()
  3. comment/uncomment the relevant lines for building the nodeList using
     either document.getElementsByTagName or node.getElementsByTagName


At this point I'm satisfied there isn't much performance tuning we can do from
the JavaScript side of things.  Feel free to provide working code that proves me
wrong :)

I did some research last night looking for ways to improve performance using
available APIs.  One candidate is the DOM2 Traversal API, specifically
NodeIterator with a NodeFilter.  Another is the DOM2 Event API, specifically
support for the NodeInsertedIntoDocument Mutation Event.  Neither of these are
working yet.  You can read more on the mozilla DOM pages:

<http://www.mozilla.org/docs/dom/>

The mozilla extension getElementsByAttribute doesn't exist on the content
document DOM implementation, only the XUL document DOM implementation.
(Reporter)

Comment 36

16 years ago
I won't be able to get to this for a few days... but when I do, I'll need to 
know which page you used for testing :-)

Gerv
(Assignee)

Comment 37

16 years ago
Changing title of this bug.

Tim's enhancement of adding support for rel= on anchor elements as well as title
elements is a worthwhile and useful enhancement and kills two bugs with one patch.

We should not waste time building a second UI for these elements, they are
highly useful and important to increasing the meaning of links between documents.

Why are we not using document.links?  This is a collection of all anchor and
area elements in the document, and if someone accidentally puts a rel= on an
area link all the better (not having rel= on area elements is an oversight in
HTML4, IMHO.  If we want to be semantic we can add a test.)
Summary: No UI for HTML2 LINK element → No UI HTML "rel=" attribute
Why on earth do we want to go through all the <a> elements as well??? That's 
what the links sidebar is for. Let's not get carried away here. As the original
reporter, I'm putting this back to what I originally asked for, wrote a spec 
for, and got discussion about.

If we want to add <a rel=> (or <a rev=>, <area>, xlink:role, etc.) support then
that should be an enhancement added AFTER this bug is fixed.

Don't succumb to feature creep!
Summary: No UI HTML "rel=" attribute → No UI HTML <link> element
Regarding the rel attributes -- don't expect the HTML WG to give you anything
useful to go on. Do what you think is best. Of course, if you really want me to
ask, send me the exact question I should ask and I'll look into it...
Gervase, I got those times running the benchmark against the GNU make manual
testcase.  However, you can use any heavily linked document you want, because
there's no point in comparing your times to my times, unless you want to see who
has the faster computer :)

Eric Hodel wrote:
> Tim's enhancement of adding support for rel= on anchor elements

Actually, it was Chris Hoess' enhancement, I just carried it over in my rewrite
because I thought it was neat.
(Reporter)

Comment 41

16 years ago
> Why on earth do we want to go through all the <a> elements as well??? 

> That's what the links sidebar is for. 

So what you are saying is that no-one will use <A rel="foo">, because the Links 
sidebar isn't in default Mozilla.

Are you really saying that <A rel="Next" href="next.html">Next</a> should have 
different UI for the rel attribute to <LINK rel="Next" href="next.html/> ?

> If we want to add <a rel=> (or <a rev=>, <area>, xlink:role, etc.) support 
> then that should be an enhancement added AFTER this bug is fixed.

Why, if the support is there now? Removing it would be more complex than leaving 
it in!

Gerv
Oh, come on.  Disabling REL on anchors is as "complex" as commenting out one line.
Ian actually managed to convince me last night on IRC that there's a reason to
have different UI for <link rel="foo"> and <a rel="foo">.  The first is a page
link; the relationship denoted in the link element applies for the whole page. 
The second is a context link; the relationship denoted applies to the immediate
context of the anchor element.  Looking carefully at the W3C's homepage, it
appears that they're actually making this distinction.  If you look at the links
brought up by the toolbar (current version, both a and link) on that page, the
"Miscellaneous" section has a bunch of links marked "details: news archive" and
one "details: press release".  Examining the source, these are attached to the
anchors at the end of each of the little blurbs on a W3C announcement.  It seems
more clear if we assume that each of these "details" relationships applies to
the little blurb of text rather than the entire page-it doesn't make sense to
have five links into different points of the news archive if it's "details" for
the entire page, but it is meaningful when viewed in context.

I agree that I'd like a better UI than the Links Sidebar.  (No insult meant to
Eric's design, but I think a lot of people will turn off the sidebar to save
screen space and never see anything that appears there.)  However, thanks to
Hixie's evangelism, I do feel that that is better brainstormed out in bug 57399.
 Anyway, the matter of turning the code on/off is, as Tim T. pointed out, quite
simple, so enabling or disabling it is no big deal.
(Reporter)

Comment 44

16 years ago
OK, if in an ideal world, <A rel="foo"> is different to <LINK rel="foo"> then we 
need to ask ourselves the following questions:

1) Is no UI better than the current UI? That is, is having these links 
"unflagged", as it were, better than having them in the Misc menu of the Links 
Toolbar?
2) If different UI is planned, what would it look like and how on earth can we 
implement it (particularly in the case of the example given)?
3) Why don't the W3C say in what context they expect the two sorts to be used? 
;-) It very much affects the sort of UI you are providing, as we have found out.

Gerv
Just to clarify, the spec says <link> elements are document links, whereas <a>
links are links from certain contextually-sensitive parts of the document. Nowhere 
does it suggest that <a rel="next"> should be taken and used in some other part
of the UI, and in fact I see no reason to do that. There are probably logical 
things we can do with rel="" links on <a>, but IMHO putting them on the <link>
toolbar is not one of them.

Comment 46

16 years ago
There's already a UI for <a rel="foo">: all those elements appear in the page,
underlined, in blue type.  There is NO currently visible UI for the <link>
element because it doesn't correspond to anything in the content.

Comment 47

16 years ago
Just noticed that, as it is (I'm using the version from attachment 40168 [details], "Tim's
Revised XPI") only works when the page is served as text/html.  If you have
XHTML and serve it as text/xml, the toolbar doesn't pick anything up (I'd guess
it isn't even activated).

Comment 48

16 years ago
Created attachment 40698 [details]
XHTML w/ LINK elements testcase
(Assignee)

Comment 49

16 years ago
That testcase is invalid, Mozilla is behaving exactly as it is supposed to.  If
you want to serve XHTML as XML and get Mozilla to treat an XHTML file like an
HTML file, you need to use the MIME type "application/xhtml+xml"
Created attachment 40716 [details]
Hot XPI Action !!!
Created attachment 40717 [details] [diff] [review]
patch covering changes for Hot XPI Action !!! (requires previous patch be applied first)
Latest XPI with lots-o-good-stuff.  Grab it.  Love it.  Hate it.


Changes:

* incorporated Neil's Classic icons  [Eric]
* renamed Author => Authors  [Eric]
* generate link handling objects automatically from XUL,
    i.e. no more code like:

    registerItemForType(new LinkToolbarButton("next"))

* Miscellaneous menu removed.  Unknown link types appear
    below the More menu in a submenu for that link type.
* refreshing is avoided when the toolbar is hidden via
    "View -> Show/Hide -> Link Toolbar" (but see bugs below)
* minor refactorings to improve readability and reduce duplication
* added a few icons for Modern theme (but see bugs below)
* added testRefresh() and related functions for performance testing


Known bugs:

* attempting to switch themes causes the "You have selected 
    a theme that was designed for an earlier version of 
    Mozilla..." error dialog, no theme change
* lack First icons, and hover and active icons in Classic
* lack appropriate icons in Modern
* toolbar is refreshed when collapsed via the twisty.  Should 
    avoid refresh when collapsed, then perform an immediate 
    refresh when exposed just like for hiding the toolbar
* toolbar separators displayed without vertical bar (border).  Help needed.
* disabled toolbar items shift (probably border, margin, or padding)
* toolbar only gets updated when the document has completely loaded.  It
    should update incrementally  as the document is loaded.
* browser is unresponsive for 2 seconds or more while updating the toolbar
    on the GNU make Manual testcase
* HTTP LINK header ignored  
* Drag & Drop should work like the Personal Toolbar


Regarding appending unknown link types to the More menu:

To preempt the "bug" reports, I intentionally didn't implement the "switcheroo"
menu/menuitem as described by fantasai and others.  That is, append a menuitem
to the More menu for link types that have only one link, and a menu for link
types with more than one link.  If you disagree agree, feel free to duke it out
wherever you think is appropriate, but consider saving the debate till this bug
is resolved and you can submit your preferred implementation as an enhancement.

If you need to vent, send me a private email full of flamage...I am full of
love.  ;-)


Hackers:

The patch requires source from the previous XPI + the patch to get localization
working.  If you're patching, you'll also need to grab the XPI to get the new
images. FYI, I'm about to move a bunch of functions around in
linkToolbarOverlay.js to make the code easier to follow. I avoided doing it this
time around since such changes make it difficult to tell what's changed when
using patch files. If you're doing something that you think might cause big
collisions with my changes, drop me an email.
Regarding icons for Modern theme:

Chris Hoess made a complete set of icons (normal, disabled, hover) from the
original icons I scrounged together.  In the Classic skin these have been
replaced with Neil's icons.

Someone (Chris?) made a few Modern icons, but until Modern has a cohesive set,
the icons Chris made are more than adequate.  Unless someone comes forward with
other icons soon, I'll grab Chris' from an older JAR and incorporate them into
the Modern skin.

Comment 54

16 years ago
Created attachment 40731 [details]
Revised XHTML+LINK testcase (changed mime type)

Comment 55

16 years ago
Thanks for the correction on the MIME type.  I fixed my script, noticed it
didn't help at all, and then changed my local server to use
application/xhtml+xml for .xhtml and saw that my test case still doesn't trigger
the toolbar.  This is using the newer, "Hot XPI Action!" version of the toolbar.
 I've created a new attachment which is identical except for the mime type
(which may have been a bit overkill, but at least it's small).
The link toolbar isn't showing up because getElementsByTagName("HEAD") is
returning null.  Changing to getElementsByTagName("head") and
getElementsByTagName("link") the link toolbar shows up for the XHTML test case
and all other test cases.

My explanation is that HTML is case insensitive, whereas XML is case sensitive.
 Using lowercase "head" and "link" will work because XHTML requires lowercase
tag names.
(Reporter)

Comment 57

16 years ago
Review

This is extremely well-written _code_, and the current toolbar looks very good. 
A couple of things:

Comments. Can we have some, please? :-) Documentation is definitely one of the 
1.0 criteria, and a new person coming to this code has no help in understanding 
it. Each function should say what it does, at the very least, and there should 
be a block at the top explaining how it all fits together. This is the one 
glaring problem with it. The rest is nits.

    // FIXME: we really want to use this:
    // <snip>
It should say in the comments why you can't. If the function doesn't, and will 
never exist, stop saying it. If there's a bug number, quote it.

function getHeadElement() {
  return window._content.document.getElementsByTagName("HEAD").item(0);
}
The point of getting the HEAD is to avoid searching the entire document tree. If 
you search the entire tree to get the HEAD, it rather defeats the object. If 
there is no document.head object or somesuch, try writing a small bit of code 
that searches the immediate children of the <HTML> element (which according to 
your code, is document.firstChild.firstChild. Like in your 
testIterateAnchorElements function.

How much of the CSS file is pinched from other CSS files? If we are using a lot 
of the same CSS as the Personal Toolbar we should be including its CSS files to 
avoid duplication, and only putting stuff unique to us in our CSS file.

Can it be made to install somewhere more sensible (chrome/content/navigator and 
parallel friends) or would it need to be part of the main build for that?

I am going to keep making the point about making unused items on the More menu 
disappear, of course. And the one about not having single-item submenus (which 
is now an issue with the new unrecognised types.) :-)

On a related note, is anyone seeing what is probably a XUL bug, where when you 
open the Sections menu on the Make testcase, which has too many items to fit and 
so requires menu scroll arrows, the root menu also acquires scroll arrows which 
don't go away until you reload?

For those who want to test a multipage version of the GNU make manual, it's 
here:
http://www.gnu.org/manual/make/html_node/make_toc.html

Fix the above, and you will find it easy to acquire r=gerv ;-)

Gerv
gervase.markham@univ.ox.ac.uk wrote:
> This is extremely well-written _code_, and the current toolbar looks very good.

Thanks :)


> Comments. Can we have some, please? :-)

If what you just said above is correct, then it shouldn't need comments...at
least not many. I'm from the XP/Refactoring/Wiki crowd. We tend to
TreatCommentsWithSuspicion:

<http://c2.com/cgi/wiki?TreatCommentsWithSuspicion>

The most sinister of them all are comments that merely say what the function
does. These add no value.

If there's a section of code that you find difficult to understand, let me know
so I can refactor it to make it clearer. Or better yet, refactor it yourself :)


> // FIXME: we really want to use this:
> // <snip>
> It should say in the comments why you can't. If the function
> doesn't, and will never exist, stop saying it. If there's a
> bug number, quote it.

You'll have to give me a specific example and explain your problem with that
specific example, because I don't understand what you're trying to say.


> function getHeadElement() {
> return window._content.document.getElementsByTagName("HEAD").item(0);
> }
> The point of getting the HEAD is to avoid searching the entire document tree. If
> you search the entire tree to get the HEAD, it rather defeats the object. If
> there is no document.head object or somesuch, try writing a small bit of code
> that searches the immediate children of the <HTML> element (which according to
> your code, is document.firstChild.firstChild. Like in your
> testIterateAnchorElements function.

I appreciate your good intentions, but before you make comments about code
performance, please do some profiling yourself. Your comment about
getElementsByTagName searching the entire document to retrieve the first
occurance is wrong.

At first, I did implement a specialized method that walked the document tree.
Problem is, I'm not sure you can make the assumption that HEAD is always
document.firstChild.firstChild. A comment or processing instruction could be
the docment.firstChild or HTML.firstChild. Using
getElementsByTagName("head").item(0) will always work (mozilla synthesizes the
HEAD element if it doesn't exist) and it only adds about 50ms to the call. In
fact, as I started handling the special cases in the tree walker version, it
became apparent that there might not be a performance benefit at all.


> How much of the CSS file is pinched from other CSS files?

The majority of the CSS is for specifying the button icons. A few attachments
ago I removed most of the copy-paste-reuse in the CSS and added appropriate
CLASS attributes in the XUL.

What does remain are styles for disabled bookmarks, because until now there
never were any. These have to get approved, because they specifiy color and
border properties, which is verboten in derived CSS files (more on derived CSS
files below). FYI, there /is/ a comment in the CSS explaining this :)

I imagine that at some time in the future the disabled bookmark styles should
just get folded into
skin/[classic|modern]/communicator/bookmarks/bookmarksToolbar.css.


> If we are using a lot of the same CSS as the Personal Toolbar we
> should be including its CSS files to avoid duplication, and only
> putting stuff unique to us in our CSS file.

Nope, definately *must not* do that...here's why:

    "Your CSS file should never import another skin file if your
    file is loaded by a XUL overlay"
    -- <http://www.mozilla.org/xpfe/skins.html>

When I came on board, that actually was being done. As I learned more about
XUL, I learned it was a no-no. When I removed it, suddenly the Modern horkage
was fixed. Funny that :)

The correct way to do it is to put the proper CLASS attributes in the XUL
overlay, which I did. An overlay inherits all the styles for that which it is
overlaying.


> Can it be made to install somewhere more sensible (chrome/content/navigator
> and parallel friends) or would it need to be part of the main build for
> that?

I have no idea how the review/acceptance process actually works. I've been
imagining the following scenario:

1. develop seperate XPI until complete
2. have UI review. If accepted, proceed to 3., else go back to 1.
3. submit a patch to the main build tree that incorporates the link
toolbar into the base chrome packages
4. patch review. If accepted, proceed to 5., else go back to 3.
5. done

Someone please post how it really works, and who the reviewer(s) will be. I've
been expecting it to be r=hixie, r=mpt, the maintainer(s) of the chrome
packages, etc..

Once we add support for LINK HTTP (and META http-equiv) headers, I think it will
be Good Enough(TM). Oh, and consistent Modern icons.


> I am going to keep making the point about making unused items on the More
> menu disappear, of course. And the one about not having single-item
> submenus (which is now an issue with the new unrecognised types.) :-)

You're free to keep making your point, so long as I'm free to continue ignoring
it :)


> On a related note, is anyone seeing what is probably a XUL bug, where
> when you open the Sections menu on the Make testcase, which has too
> many items to fit and so requires menu scroll arrows, the root menu
> also acquires scroll arrows which don't go away until you reload?

Yes. You can reproduce it by filling up a submenu in a top level personal
toolbar bookmark folder with enough bookmarks, i.e.

Personal Toolbar:
Home  Bookmarks | + my-folder
                    + subfolder
                        bookmark1
                        bookmark2
                        ...
                        bookmarkN

Knowing that it wasn't the link toolbar was good enough for me, so I stopped
there and didn't search/file a bug. I know this makes me a bad citizen :)
Created attachment 40774 [details] [diff] [review]
patch to above XPI to fix XHTML bug
(Reporter)

Comment 60

16 years ago
> If what you just said above is correct, then it shouldn't need comments...at
> least not many.

You do not comment to make unclear code more clear, this is true. You comment so 
someone who wanted to make a small addition to the Links toolbar (e.g. to make 
it support <A rel="foo"> ;-) can come along and work out what's going on 
quickly. It's a large file and it took me 15 minutes just to work out how it 
fitted together. Multiply that by the number of people who may one day need to 
understand it.

I can't believe I'm having to justify comments. Are you seriously suggesting 
that uncommented code is in some way clearer than commented code?

> You'll have to give me a specific example and explain your problem with that
> specific example, because I don't understand what you're trying to say.

I'm referring to a particular comment in the code. Read the comment, and what I 
say will become clear :-)

> Your comment about
> getElementsByTagName searching the entire document to retrieve the first
> occurance is wrong.

Why? As the code is written, it will do a getElementsByTagName() on the whole 
document, because it can't know if you want the first, the second, the xth or 
the forty-second. Then, you ask for the first one, and it gives it to you and 
throws the rest away.

Either that, or there is some way for getElementsByTagName() to know, when it's 
being called, that subsequently you are going to only ask for the first one. 
Which would make it telepathic.

There may or may not be a measurable performance benefit; but I stand by my 
assertion that it does check the whole document.

> Someone please post how it really works, and who the reviewer(s) will be. 

It could work that way if you wanted it to. In this case, UI review can happen 
now. Summon mpt! As for who reviews it, the answer is "whoever's nearest". I've 
started already; but you are free to ask anyone you want, and anyone is free to 
review it and make comments, if you see what I mean.

> Once we add support for LINK HTTP (and META http-equiv) headers, I think it 
> will be Good Enough(TM). Oh, and consistent Modern icons.

I agree. In fact, it's Good Enough(TM) now if you fix the icons. META should be 
easy, right? head.getElementsByTagName("meta") etc.

One last point - currently you merely lookup language tags in your dictionary, 
instead of implementing Hixie's hreflang spec (see the URL box) which copes with 
x-minbari-warrior-caste and fr-CH-slang or whatever. I've implemented code that 
does this for the Properties (metadata) window, in metadata.js. Feel free to 
leverage it.

Gerv
Created attachment 40779 [details] [diff] [review]
patch to reorganize code and add comments for easier readability
> You do not comment to make unclear code more clear, this is true. You comment so 
> someone who wanted to make a small addition to the Links toolbar (e.g. to make 
> it support <A rel="foo"> ;-) can come along and work out what's going on 
> quickly. It's a large file and it took me 15 minutes just to work out how it 
> fitted together.

Yes, I'm aware of that.  See the recent patch and the paragraph above labeled
"Hackers:" from a few comments ago.  Breaking up linkToolbarOverlay.js into a
few files and moving around some functions should go a long way towards making
it easier to digest.

> Are you seriously suggesting 
> that uncommented code is in some way clearer than commented code?

Yes.

> Why? As the code is written, it will do a getElementsByTagName() on the whole 
> document, because it can't know if you want the first, the second, the xth or 
> the forty-second. Then, you ask for the first one, and it gives it to you and 
> throws the rest away.
> 
> Either that, or there is some way for getElementsByTagName() to know, when it's 
> being called, that subsequently you are going to only ask for the first one. 
> Which would make it telepathic.
> 
> There may or may not be a measurable performance benefit; but I stand by my 
> assertion that it does check the whole document.

getElementsByTagName returns a NodeList.  Unlike returning an Array, there's no
reason that the NodeList returned has to contain all the matched elements in
some internal data structure.  Instead, it could defer scanning the document
until you start requesting nodes.  When you request a node via NodeList.item(n)
it only needs to scan the document upto and including the n-th occurance of the
matched element.  Only something like NodeList.length would require a full scan
of the document [1].

I'm fairly certain that the NodeList is implemented using an optimization
similar to what I described above.  That would explain why I observed little
overhead for getElementsByTagName("head").item(0)...no telepathy required.

[1] or something like NodeList.item(n), where n >= NodeList.length

Comment 63

16 years ago
>On a related note, is anyone seeing what is probably a XUL bug, where when 
>you open the Sections menu on the Make testcase, which has too many items to 
>fit and so requires menu scroll arrows, the root menu also acquires scroll 
>arrows which don't go away until you reload?

Yes, I see that on Windows 2000 with the Bookmarks menu, my Imported IE 
favourites is longer than the screen and it does what you describe. Very likely a 
XUL bug.
Created attachment 40805 [details] [diff] [review]
patch to revert back to spaces for indentation (oops!)
Created attachment 40806 [details]
All Your XPI Are Belong To Us
Created attachment 40807 [details] [diff] [review]
patch covering changes for All Your XPI Are Belong To Us (requires previous patch be applied first)
Release Every ZIG !!

I brought back the HTTP header parsing code originally written by Eric, Chris,
Jeffrey Baker, or someone else.  It works for META http-equiv="link", but not
for real HTTP headers.  Getting access to the HTTP headers require some
XPConnect voodoo.  Therefore getting real LINK headers to work will require one
or more of:

a) an "aha" moment on my part
b) someone responding to my n.p.m.xpcom post with example code
c) someone else diving in and making linktoolbar/content/linktoolbar/http.js
     work for HTTP headers

Not handling HTTP headers is a blocker.  Not being able to switch themes after
linktoolbar.xpi is installed is a blocker.  The "2 second delay" bug /isn't/ a
blocker, because that's only for handling anchors in BODY...a feature that will
eventually be disabled.  The remaining bugs aren't blockers...though it would be
nice to have icons for First in Classic (Neil, are you there?  :-) ).


Changes:

* META http-equiv="link" works (but real HTTP headers still don't)
* Modern has cohesive set of icons
* toolbar seperators display properly


Known bugs:
    
* attempting to switch themes causes the "You have selected
    a theme that was designed for an earlier version of 
    Mozilla..." error dialog, no theme change 
* HTTP LINK header ignored  
* language name lookup doesn't use hixie's algorithm for non-standard
    languages
* lack First icons, and hover and active icons in Classic
* toolbar is refreshed when collapsed via the twisty.  Should
    avoid refresh when collapsed, then perform an immediate 
    refresh when exposed just like for hiding the toolbar
* disabled toolbar items shift around in Classic (probably border,
    margin, or padding)
* toolbar only gets updated when the document has completely loaded.  It
    should update incrementally  as the document is loaded.
* browser is unresponsive for 2 seconds or more while updating the toolbar
    on the GNU make Manual testcase
* Drag & Drop should work like the Personal Toolbar
(Reporter)

Comment 68

16 years ago
> Not handling HTTP headers is a blocker. 

I disagree. It's an RFE on the current code. It shouldn't stop this getting 
checked in.

>  Not being able to switch themes after
> linktoolbar.xpi is installed is a blocker.  

I wouldn't worry too much; that may well go away when it's no longer an XPI. :-)

> though it would be
> nice to have icons for First in Classic (Neil, are you there?  :-) ).

I'd say that this _is_ a blocker; I doubt you'll pass UI review without the UI 
being all there. Speaking of which, I'll ping mpt.

Gerv

Comment 69

16 years ago
>> Not handling HTTP headers is a blocker. 

>I disagree. It's an RFE on the current code. It shouldn't stop this getting 
>checked in.

I'm with Gerv.  Can anyone point to a site that uses links via HTTP headers?  I
don't think it is a blocker.  Nevertheless I'll be happy to take a crack at
updating the header parsing code from lo these many months ago, and post here if
I have success.

Comment 70

16 years ago
the bug lordpixel and others mentioned is filed and should be fixed eventually, 
if you need help finding it, ask on #mozillazine. This bug has to be one of the 
fastest growing and most actively developed bugs this week. i haven't checked 
the classic icons lately but i'd expect the first/last buttons to resemble 
mplayer.exe/mplay32.exe's icons.  [don't confuse that app w/ mediaplayer2/wmp 
(mplayer2) or activemovie(amovie)]

As for menu depth, 2 levels is perfectly acceptable (the menubar is not a 
level). Having a combined menu used to be acceptable but we're moving away from 
that -- actually we're both moving away from and towards that.  people are 
trying to combine Edit+Search while they are trying to split view>apply {foo}. 
And people are working to split Window-list from Tasks (yes that's right 
we're growing menu length, shrinking menu length, increasing top level menu 
count and decreasing the same). Personally I would prefer a shorter view menu 
(view is like the context menu, it's getting huge), which means combining 
view>select {foo}> and view>select {bar}> into view>select 
bazs>{foo-list}|{bar-list}.

But i haven't provided code so my voice shouldn't count for much.
If classic icons are a blocker, I will change classic to use the same icons as
modern (except for the More menu).  Debating whether HTTP headers is a blocker
will be kind of moot if Jeff resolves it.

Do we have someone capable of incorporating the linktoolbar into the main
chrome?  I have a CVS build tree, but my build alway segfaults (probably my
system libraries are out of whack), making it difficult to test my
changes.

Could someone add the "review" keyword?

Comment 72

16 years ago
I don't see how we can get a handle on the current HTTP channel.  It used to be
that navigator.js had a reference to it, and it got passed to us.  We could
implement nsIHttpNotify in js or cpp, but I'm not up for it right now.  This is
how the cookie permission prompter works.
I'll see if I can wrap my head around nsIHttpNotify.

Comment 74

16 years ago
Created attachment 40850 [details]
The 6 missing icons

Comment 75

16 years ago
I don't think the "first" icon is missing in the classic skin. I think its just 
named start.gif and start-disabled.gif instead (leastways, it is in that last 
XPI).

That means the missing icons for Classic are the 6 "hover" icons, and modern is 
missing bookmark-icon-disabled.gif. I attached my attempt at doing those icons in 
the appropriate style. (its a zip file containing 7 gifs).

As far as licensing goes - any IP _I've_ created w.r.t. to these 7 icons I place 
in the public domain. Feel free to copy and relicense as needed. Anything anyone 
else owns remains theirs, naturally.
Excellent.  Thanks for making the rest of the icons, and for locating the First
icons...ugh...me dumb.

As for Modern, there actually is a disabled bookmark item icon (called
bookmark-item-dis.gif) that I've been using.  I don't know what it was used for
upto now.  Hope you don't mind if I leave it unchanged.  I'll toss your version
into the linktoolbar modern skin folder just in case the "official" one disappears.

I should have a new XPI with these icons and a some other changes (sorry, no
HTTP headers) in a couple of hours.
Created attachment 40856 [details]
I am full of XPI
Created attachment 40857 [details] [diff] [review]
patch covering changes for I am full of XPI (requires previous patches)
The good news: this XPI is the first I feel is Good Enough (TM).  I wimped out
on HTTP headers and noone came to my rescue on n.p.m.xpcom (thanks for giving it
a look, though, Jeff).  I'm in agreement that HTTP header support can be a
seperate bug.

The bad news: this is the last XPI you'll be seeing from me for a week or so. 
OK, maybe that's good news for some :)

I'll be on vacation Jul 4-8 and I need to take care of "life stuff" in the
intervening time.  I'll still be checking how things are going till around Tue,
Jul 3.  Now's your chance to make all those changes I didn't want to make.

Changing toolbar items between button, menu, and menuitem only requires changes
to the XUL.  Ditto for adding new top level menus.  Unknown link types are hard
coded to append to the More menu.  It would require some additional code to
support class="menubutton-dual" i.e. the button with dropdown menu.

BTW, you may need to remove chrome/user-skins.rdf inside your mozilla profile to
get the "them switching" error to go away.  Before reporting that it's still a
bug, try installing the XPI into a clean mozilla tree with a fresh profile.


Changes:

* added lordpixel's hover icons and Neil's rediscovered First button.
* fixed "theme switching" error
* added "Document" menubutton for ToC through Index
* display HREF for tooltip when title absent
* disabled code for handling anchors with REL
* more refactoring

Known bugs:

* HTTP LINK header ignored   
* language name lookup doesn't use hixie's algorithm for non-standard
    languages
* toolbar is refreshed when collapsed via the twisty.  Should
    avoid refresh when collapsed, then perform an immediate 
    refresh when exposed just like for hiding the toolbar
* disabled toolbar items shift around in Classic (probably border,
    margin, or padding)
* toolbar only gets updated when the document has completely loaded.  It
    should update incrementally  as the document is loaded.
* Drag & Drop should work like the Personal Toolbar
erg...it's getting late.  That should have been "theme switching" and "clean
mozilla install".  And that bit about adding in all sorts of features I
disagreed with was sarcasm ;>
(Assignee)

Comment 81

16 years ago
I tried to get the HTTP Headers via nsIHTTPChannel.idl XPCOM magic some time
ago, but I would always crash.  I eventually ran out of time trying to fix it. 
I'll try to resurrect my past work and see if I can get it to run properly this
time (I think the problem was Mozilla's not mine).  I should have the code
buried in my CVS repository...

This should be easy to get into a standard location, but not from an XPI.  The
task should be as simple as:
Move the files to the appropriate location,
Adjust the paths on the files
Add the new files to the JAR Manifests
Build and test, fix as appropriate.

I'm not sure what is horked with Modern skin switching.  I duplicated skin
information from modern.jar's contents.rdf, but apprarently I horked it somehow :(
Unless the code you have in CVS is more recent, I already brought back the HTTP
code from a previous JAR/XPI.  The code I snagged I placed into
content/linktoolbar/http.js.  I twiddled with it a bit to get META http-equiv
working, but it's mostly the same code.

The Modern skin switching isn't horked anymore.  By comparing it with other
files, I found a contents.rdf file that was missing a skinVersion="1" attribute
on one of the elements.  Adding that attribute fixed it.  As I mentioned, I had
to install the XPI into a clean mozilla install with a clean profile, but I
later found you can also get it fixed by removing
~/.mozilla/[your-profile]/chrome/user-skins.rdf from an existing profile.  You
can also install the new XPI over an already installed version, but I had to
save the file to my harddrive and open the file up with "File -> Open File". 
Other people mentioned having to do this above to get the XPI to install.

Lastly, I forgot to mention the following change that was also done with the
latest XPI:

* position "View -> Show/Hide -> LinkToolbar" between "Personal Toolbar"
  and "Status Bar" menuitems

Now, back to paying bills :-<

Comment 83

16 years ago
Adding review and patch keyword per Tim's 2001-07-01 11:40 comment to get
reviewers attention. However, this is not always enough to get the necessary
reviews. (Especially with code from outside Netscape) The owner of this bug and
the patch supplier should contact the individuals they would like to review the
fix directly as well.
Keywords: patch, review
Thanks.  You should probably remove the "patch" keyword.  The attached patches
are only for changes to the XPI.  We haven't yet had a patch that can be applied
against the build tree.  At this point only a UI review is in order, I guess.

Comment 85

16 years ago
I thought about not putting it in but the description for the patch keyword is
"Tracks contributions from the net community. Fixes/patches found by individuals
without direct code check in privileges." This patch is from the net community,
and the more keywords the better! :-)

While the fix is just an XPI, I'd imagine the check-in/longterm solution will be
to include the code directly in the build, so better to let everyone (who wants
to) know.

Comment 86

16 years ago
Is it possible 'for the rest of us' to install the latest version (on MacOS 9.1)
by just clicking on those attached files in a certain row or do we have to apply
the diff-patches manually as well (no idea how this had to be done).

Can someone make a current full-installer package?
For me only the version from
<http://segment7.net/mozilla/linktoolbar/index.html> works and AFAIKS this is
really outdated ...

Greeting, Michi

Comment 87

16 years ago
You know, the question of "what icon to use for Top" would be answered if bug
32087 gets resolved.

Comment 88

16 years ago
I just installed "I am full of XPI". I'm seeing a problem with the table of
contents link. http://www.w3.org/TR/html4/ has it on all the pages, and toc
shows up, but doesn't move anywhere. Mac 0.9.2.

Comment 89

16 years ago
Never mind. It seems to have fixed itdelf after a restart. 

I'm doing it with another skin and it still looks good (Given the disabled icons
look off.) Great job Tim.

Comment 90

16 years ago
Finally I managed to install "I am full of XPI" on the Mac 
(short howto: download with another Browser, rename the resulting file
'showattachment.cgi' to 'showattachment.xpi' and drop it into an open Mozilla
window).

Well done! One bug and one RfE:

1. Please have a look at the 'More' menu on <http://michael.nahrath.de/dnet/>!
<link rel="shortcut icon" ...> breaks all following links (shurely this is
related to bug 32087).

2. RfE: Using the 'link' element with e-mail adresses (mailto:) is rather common
(typically but not exclusively with the 'rev' attribute). It would be very nice
if the icon for these links would change to a mail symbol.
 
More ideas for this feature: An icon for ftp: or file downloads in general and
an icon for content that is meant to be printed.

Comment 91

16 years ago
Michael Nahrath, many thanks for explainign how to install the last XPI. It
finally worked for me too. And it doesn't break Modern 3!

Bug report: Is it just me or can anyone else repro: I have the following link
elements:
<link rel="alternate stylesheet" type="text/css" href="styles/print.css"
title="Print" />
<link rev="made" href="mailto:webmaster@domain.com" />
<link rel="start" href="" title="Start Page" />
<link rev="prev" href="" title="Previous Page" />
<link rel="next" href="" title="Next Page" />
<link rel="contents" href="" title="Contents" />
<link rel="copyright" href="" title="Copyright" />
<link rel="section" href="" title="Section" />
<link rel="help" href="" title="Help" />
<link rel="search" href="" title="Search" />
<link rel="alternate" hreflang="de" href="" title="Seite auf D" />
<link rel="alternate" hreflang="fr" href="" title="Page en f" />
<link rel="alternate" hreflang="en" href="" title="Page in E" />

Bug: the "prev" doesn't show.

Comment 92

16 years ago
| <link rev="prev" href="" title="Previous Page" />
| <link rel="next" href="" title="Next Page" />
| 
| Bug: the "prev" doesn't show.

At least it is not applied to the predefined arrow button. This is not a bug but
correct behaviour. The 'rev' attribute ist opposed to the 'rel' attribute, but
not in the way you seem to have understood it.

You want to use <link rel="prev" href="" title="Previous Page" /> in this case
as well. 
This means "the referenced document is related to to this document in the way
that it is the previous document to this".

The REVerse relationship was "this document is related to the referenced
document in the way that it is the previous document to the other".

So <link rev="prev" href="" title="Previous Page" /> was aequivalent 
to <link rel="next" href="" title="Next page" />. 
You could even combine them: 
<link rel="next" rev="prev" href="" title="..." />

See http://www.htmlhelp.com/reference/html40/head/link.html for further explanation.

P.S.: The programmers intended to use this Bug mainly for technical
implementation details, not for UI, styling and authoring issues. 
Please open a new thread or post to the existing one in the newsgroup
netscape.public.mozilla.ui if you would like more discussion upon the rel/rev thing!

Comment 93

16 years ago
Discovered how to install this on a multiuser linux install:

Become root and cd to your mozilla install directory, then:

export LD_LIBRARY_PATH=`pwd`
export MOZILLA_FIVE_HOME=`pwd`
cd chrome
unzip linktoolbar.xpi (or whatever you saved the attachment as)
rm install.js
cat >>installed-chrome.txt <<EOF
content,install,url,jar:resource:/chrome/linktoolbar.jar!/content/linktoolbar/
locale,install,url,jar:resource:/chrome/linktoolbar.jar!/locale/en-US/linktoolbar/
skin,install,url,jar:resource:/chrome/linktoolbar.jar!/skin/modern/linktoolbar/
skin,install,url,jar:resource:/chrome/linktoolbar.jar!/skin/classic/linktoolbar/
EOF
cd ..
./regchrome

Next time you run mozilla you should see the link toolbar in place :)

Also, for anyone interested in hacking on this, it's also possible to work with
an unzipped copy by making the following changes:
1) make a directory linktoolbar and unzip linktoolbar.jar inside it.
2) Change the four lines added to installed-chrome by removing the string "jar:"
and the string ".jar!" on each line.
3) Run regchrome again (or make these changes before running it the first time).

From what I've seen so far I'm extremely impressed with the quality of this
patch and I hope it gets approved very soon :) Great work guys!

Comment 94

16 years ago
The implementation really is first quality.  I used to advocate the magically
appearing and disappearing link toolbar, but now I think I prefer the permanent
toolbar.  It's in your face will remind authors about the LINK element.
Created attachment 41662 [details]
XPI Strikes Back
Created attachment 41663 [details] [diff] [review]
patch covering changes for XPI Strikes Back
Hi.  As if it weren't obvious, I'm back from vacation.  Thanks for the kudos on
the Link Toolbar :)   It wouldn't be as good as it is without the work of
several people who have contributed to this bug.

Most everyone can ignore the patches. Unless you've been modifying the code
yourself and need to merge in my changes you don't need/want to use the patch.
Just follow the procedures posted above for installing the XPI.

The attached XPI, XPI Strikes Back, has a couple of changes.  Michael uncovered
a couple of bugs with <http://michael.nahrath.de/dnet/>.  These are fixed. 
While I was at it, I fixed a couple of small bugs that never made it into the
Known Bugs list.

Changes:
* ignore REL attributes containing "icon" as a word.  Whitespace and
    hyphens are considered word delimiters, so the following are all
    ignored:
  - rel="icon"
  - rel="shortcut icon"
  - rel="desktop-icon"
* fixed bug with unknown link types only being added to first transient
    submenu in the More menu
* other small bug fixes


Known Bugs: unchanged from previous list

Comment 98

16 years ago
Security question: Can someone verify that if I do <link rel="next"
href="javascript:document.forms[0].submit()">, the javascript will be executed
in the document's context (and not, say, the context of the chrome where it
would become privileged)?

Also, how hard would it be to make the href of the link appear in the status bar
when you mouseover the buttons/menuitems? For document-specified links, it's
important to be able to see where you're going.

(Not to detract in any way from the wonderfulness of the implementation as is,
though :) )
All URIs are opened with LoadURI(...), so it runs in which ever context LoadURI
has it run.  I tested a page with <link rel="next"
href="javascript:document.forms[0].submit()"> and a minimal form and it works as
expected.  If it were running in the chrome context instead of the content
document context, I would expect "document" to refer to linkToolbarOverlay.xul
and to get a JS error from "document.forms[0].submit()".

Comment 100

16 years ago
Another of my sites shows two more bugs: <http://fakemail.de/>

<link rev="Author" href="mailto:webmaster@fakemail.de" title="..."> is not
displayed at all. 
Please note, that using the 'rev'-attribute with the value 'author' and a
'mailto:'-URL is rather common authoring practice!

Maybe rev="made" is more widely spread (and _is_ displayed in the 'Document'
menu) but in principle this should make no difference. None of both is named in
any valid spec. 

Second issue:
I use two rel="home" links in the same document (or one for "home" and one for
"start") with different href values and different title values: One for my
personal homepage and one for the homepage of the specific project.
 
The current LinkToolbar joins the first of them with the 'Top' button and
ignores all others completely. IMHO it should at least show the others (or all)
in the 'Document' menu.

Comment 101

16 years ago
Regarding <link rev="Author" href="mailto:webmaster@fakemail.de" title="...">

Remember, REV indicates a reversal in the relationship between document and
link.  What this actually says is that the document is the author of the person
at the email address, rather than the other way around.  What you should be
using in this case is

<link rel="Author" href="mailto:webmaster@fakemail.de" title="...">

which is both accurate and displayed by the toolbar.

Comment 102

16 years ago
I see the logical weakness of <link rev="author" ...>.

Thank you for the formulation of your explanation. This is the first time
undersand why rev="made" may be sensible.

Anyway I believe that it is not a browser's job to decide if the author's
provided content is illogical and should thus not be rendered. As long as the
used markup is valid in a formal way it should try to show the content.

Comment 103

16 years ago
One definite bug in the implementation (at least, as of "I am full of XPI" -
haven't tested Strikes Back but it wasn't mentioned in the changelog) is that
the target attribute of <link> is ignored. This is a valid and useful attribute
for <link>, see http://www.w3.org/TR/html401/struct/links.html#edef-LINK

Comment 104

16 years ago
I believe rev="made" is popular because the text broswer lynx supports it and has 
for years. I used to use it in all my pages and I've seen reference to it in 
guides to writing HTML. Due to widespread use (compared to the <link> element in 
general, unfortunately) I think its a must to support this idiom. It never made 
sense it was rev to me either though...
I'm working on a draft document I hope to submit as a W3C note, which I'll
submit here for public comment first.  (For values of "here" probably equal to a
bug filed as a dependency of this bug.)  I'd recommend that we support
rev="made" as equivalent to rel="made" and rel="author", but not rev="author";
"author" is a clearer value than "made", and that way we can handle the instance
where the author is represented by a web page or other resource rather than a
mailto: link cleanly (i.e., such a page can have rev="author" pointing to
documents creating by the person the page describes).

While I agree that we should support "rev" links in general, that shouldn't be
part of this patch, as I don't think it's possible to adequately differentiate
between the two in this UI; this should be spun off in a separate bug.  Probably
the first-order solution would be to add cascading menus of both "rel" and "rev"
links in the "Go" menu (although I'd rather wait until the dust stops falling
from mpt's imminent rearrangement of the menus); if someone comes up with a neat
UI implementation for "rev" and we get his OK on it, we can do that instead or
as well.
Michael Nahrath wrote:
> I use two rel="home" links in the same document...One for my
> personal homepage and one for the homepage of the specific project...The
> current LinkToolbar joins the first of them with the 'Top' button and
> ignores all others completely.

My initial reason for contributing code was a strong disagreement with having
everything in the toolbar be a menu or a menubutton.  The resulting UI is
cumbersome and silly.  So I implemented a UI that assumed certain link types
should have one instance; specifically the link types that make the most sense
as singular items.  None of the alternatives, including stuffing additional
occurances into the Document or More menus, seemed acceptable.

  There is little disagreement that the current UI is good for the common case.
 I ask you to consider waiting until this bug is resolved and file a seperate
RFE for supporting multiple instances of all link types.   I expect much debate
over the proper UI for multiple instances of Top, Up, First, Next, et al.  It
would be a shame to hold up acceptance of the current UI over that debate.

Comment 107

16 years ago
I have to agree with Tim T.  The current implementation is quite good for the common case.  I'd love to see it reviewed in time to make the next milestone release.  We can argue UI and enhancements later.  Let's just get it in.

Do we have a working spec that defines the meaning/usage of the various rel types?  I see some disagreement among people with conflicting ideas of what "rel=foo" means.  I wish W3C had spelled this out in the specs long ago.  If we have one, should it be submitted to W3C?

Comment 108

16 years ago
I agree that the implementation is good enough to become part of the next
Milestone and should get into the tree as soon as possible. 
Implementing the not-so-common cases may be done later (shurely I will find a
lot more ;-)


About specs: There are several!

I listed all of them (and all current implementations) at
<http://www.subotnik.net/html/link.html>

Why don't you think that
<http://www.w3.org/TR/html4/struct/links.html#edef-LINK> is enough (it even
explains the difference betwheen rel and rev)?

Most common link types are named in
<http://www.ics.uci.edu/~ejw/authoring/draft-ietf-html-relrev-00.txt> but this
Internet Draft expired long ago.

Fantasai collected all link types and specifications (except the new XHTML-Mod)
at <http://fantasai.tripod.com/qref/Appendix/LinkTypes/alphindex.html>

Can somebody find out why rel="up" was in the HTML3.0 draft but did not make it
into HTML3.2? There must have been reasons and before bothering the W3C with new
suggestions one should find out why they disliked it long time ago.


About rev="made":

kelon@pobox.com wrote:
> Remember, REV indicates a reversal in the relationship between document and
> link.  What this actually says is that the document is the author of the 
> person at the email address, rather than the other way around.

Exchange "author" by "made" and you get a sensible sentence:

[...] What this actually says is that the document is _made_ of the
person at the email address, rather than the other way around.

A bit tricky to use a passive verb as a link type, which turns the logic of
direction around by grammar...
Help Wanted.

If you have the latest Mozilla source checked out from CVS, can successfully
build a working browser, and are willing to try out patches to the build tree,
then please send me an email (please don't post to this bug).

As I mentioned before, the browsers I build from source all segfault.  I'm going
to attempt to make a patch to the build tree for the Link Toolbar, but I'd like
to test these out instead of posting a bunch of broken patches to this bug.
Michael: the reason I'm working on Yet Another Spec is because the various 
specs conflict.  I'm hoping that by combining them and smoothing out the 
inconsistencies, we'll have a clear plan for what link types we should 
support.  When I finish the first draft (I'm trying, but don't hold your 
breath), I'm going to put it into bugzilla and invite comment (especially by 
yourself, and other people who have been giving thought to these issues); once 
we've reached a consensus on it, I was going to take it to www-html for 
exposure outside of Mozilla.  Once this is wrapped up, Hixie/dbaron can help me 
submit for possible publication as a W3C Note; I realize that won't carry any 
real weight as a standard, but it will help make our implementation public 
[Note: our implementation should be adjusted to the Note, not the other way 
around!] and maybe provoke some interest from other browser developers.
(Reporter)

Comment 111

16 years ago
Yes, for goodness sake, let's get this in the tree and worry about all the other
issues later. It's already of extremely high quality. Henceforth, all comments
of the form "it doesn't do X" (as opposed to "it does X incorrectly according to
spec/standard Y", and even then it would have to be a serious bug) should be
ignored. Checking it in doesn't mean we stop working on it. At least, I hope not
:-) 

Tim: to get this checked in, you need to do the following:
a) Do a CVS checkout, or download a tarball of the Mozilla source and convert it
to a CVS sandbox. Instructions on http://www.mozilla.org .
b) Create a patch using cvs diff -u which, when applied to a Mozilla tree, adds
the Links toolbar, with all the files in sane places within the current
directory structure. If there are completely new files, cvs add them so that cvs
diff will work (after telling us where you are putting them so we can complain
if it's the wrong place ;-).
c) Post the patch, and ask for review. I could do that, and I'm sure there are
several other people who could as well. Given the quality of the code and the
fact that I've half-reviewed it once, I don't expect much trouble here.
d) Get super-review; probably from blake@netscape.com, CCing
reviewers@mozilla.org. See http://www.mozilla.org/hacking/reviewers.html for the
procedure.
e) Ask someone, e.g. me, to check the patch in.
f) Party. :-)

If you are working in a CVS tree but want to use a nightly build for testing
(i.e. you don't want to have to build Mozilla yourself) you might find
http://www.gerv.net/software/patch-manager.html helpful for keeping the two in
sync as you test.

[Oops - mid air collision. Some of this is redundant. I volunteer to help test
patches. :-) . ]

Gerv

Comment 112

16 years ago
Gerv: the current version of the linktoolbar contains a number of gifs in the
skin/ directories. Is there a mozilla-standard way to handle these in posted
patches? (I assume that cvs diff doesn't work too happily with these, even after
passing -kb to cvs add...)
> If there are completely new files, cvs add them

Unless Tim has checkin priveleges, he can't cvs add files.

It may be easier to post a diff for existing files and a tarball of the new
files....  (a little harder to review, granted).
The '-N' flag to diff will result in a patch that creates new files when
applied.  This only works for non-binary files.  A tarball containing the images
will still be required.  No, I don't have CVS checkin privledges, which is OK by me.

Comment 115

16 years ago
personally i like zip's (application/zip) for new binary files, anyone building 
mozilla proper has zip support.

-N is rather useless if you don't have a cvs account unless you forge your 
CVS/Entries record to include files you added.
-kb is very important to remember for graphics ...
(Reporter)

Comment 116

16 years ago
As I read the cvs man page, "cvs add" fixes your local repository so that cvs 
diff works with new files. But, you are right, binary files complicate things.

Tim: re: the images. Probably what you'll have to do it create 1 big patch, and 
one zip to be unzipped in a Mozilla source dir that will put the new files in 
place, including the images. If you are adding images or other stuff to skins, 
add the image files to both skins in the zip.

Does that make sense?

Gerv

Comment 117

16 years ago
I am about to attach a patch (against XPI strikes back) which provides an
auto-hiding link toolbar *as an option*. The default is the current behavior of
displaying the toolbar all the time. I will post shortly on n.p.m.ui in order to
provide a convenient place for flamewars on the desirability and UI of this
option; please use this bug for technical issues only.

Behavior-wise, I am 100% happy with this implementation, which isn't bad for an
evening's work with essentially no prior knowledge of XUL. Elegance-wise, there
are a couple of things I'm unsure about:

1) The CSS that does the auto-hiding is in the skins. This is the only place
that contains CSS, so I didn't really have any other option.
2) I used oncreate on my new <menupopup> to restore the state of this option
which is persist()ed in the "hidden" attribute of the toolbar. This works, but
happens every time the popup is opened. My initial thought was that this is very
bad because it should be a one-time initialization, but then it occurred to me
that the current method means that for most browsing sessions (in which this
popup is never opened) the initialization is never performed at all. So it
actually *saves* us a miniscule amount of time (it really is miniscule, too -
completely unnoticeable when you pop up the menu).
3) I set the "hidden" attribute on the toolbar to "maybe" in this case. This
appears to work (including persisting to false), but I'm not sure it's supported
to just throw an arbitrary value into an existing attribute.
4) Similarly, I set a new attribute "hasitems" to true or false on the toolbar
element to allow matching based on whether there are any items in the toolbar.
Is it permissible to just add an arbitrary, undefined attribute to an object?
The only other possible issue with this patch is that I had to move away from
goToggleToolbar and remove the broadcaster that was being used, because I
couldn't figure out a way to fit a broadcaster model to this situation, and
goToggleToolbar didn't do what I wanted it to. It turns out that goToggleToolbar
seems to do a lot of redundant work, and my replacement (which is handling a
more complicated case) is still less lines of code :)

Comment 118

16 years ago
Created attachment 42027 [details] [diff] [review]
Patch against XPI Strikes Back providing autohide option

Comment 119

16 years ago
Created attachment 42038 [details]
XPI of autohide option

Comment 120

16 years ago
Sorry for spam... the just-attached XPI is my attempt at putting everything back
together into an xpi. It's 100% untested because I don't have the capability to
install xpis - use at own risk (but post back here if it worked!)
I installed the XPI with autoshow option (I agree with timeless@mac.com that
autohide is  a misnomer ;-)  ) and it works for me.  Nice work.  I'll merge it
into my stuff and try and incorporate it into the source tree patch I'm working
on.  Reducing lines of code is a Good Thing (TM)
Handling TARGET attributes isn't as straightforward as, say, making a call like
LoadURI(uri, target).  Getting TARGET working is low on my list right now.  If
someone would like to puzzle it out and submit a patch, that would be nice.

Comment 123

16 years ago
Yeah, I noticed that nsIWebNavigation was inexplicably missing a loadURI variant
that had a target parameter. I'm thinking of either asking around on IRC or
filing a bug asking for one, because it seems really odd not to have that basic
functionality.

I didn't exactly reduce lines of code per se, because goToggleToolbar still
exists in the code - we just aren't using it. All I meant was that my
replacement was less lines of code than if I'd cut'n'pasted the whole source to
goToggleToolbar into the code.

Thanks for taking a look at this :) (Btw, for anyone that's interested, I've
received confirmation that the autohide (ok, autoshow, but I called it autohide
in the attachment) XPI works okay.
OK, I think I found the necessary info to handle window targets in the
nsIDocShell API:

nsIDocShell::loadURI
<http://lxr.mozilla.org/seamonkey/source/docshell/base/nsIDocShell.idl#50>

nsIDocShell::InternalLoad
<http://lxr.mozilla.org/seamonkey/source/docshell/base/nsIDocShell.idl#95>

nsIDocShell::createLoadInfo
<http://lxr.mozilla.org/seamonkey/source/docshell/base/nsIDocShell.idl#125>

Of course, having a convienience function, loadURI(uri, target), in
nsIWebNavigation would still be a valid RFE.
Tim, nsIDocShell::loadURI and nsIDocShell::InternalLoad are noscript. 
InternalLoad is so for good reason -- it really should not be used from a
script.

I'm talking to Rick Potts right now about a function on nsIWebNavigation that
will be pretty much like nsIDocShell::loadURI.  I need it to do loads from
cache, but it would likely work for this as well....  nsIDocShellLoadInfo
objects include a target, as you noticed.
(Assignee)

Comment 126

16 years ago
The CSS for the autoshow option should be placed in a new file:
content/linktoolbar/LinkToolbarOverlay.css (or whatever)

The style rule governs content and does not vary from skin to skin.  For
precedence see sidebarOverlay.css (lxr is your friend ;)

Comment 127

16 years ago
I've filed bug 90722 as a request for a scriptable loadURI method that accepts a
target parameter.

Tim, would you like to implement Eric's suggestion (which I thoroughly agree
with) as part of your merge into the standard build process, or would you like
me to provide the (fairly trivial) updated patch to do this? It occurs to me
that it would probably be just as hard to integrate a patch if I *did* provide
one as for you to just make the change based on Eric's comments.
Stuart, the easiest thing right now would be to just submit patches against the
XPI like we've been doing.

The patch against the source tree (which I'm about to submit) is based off of
XPI Strikes Back.  I'm going to concentrate on getting it reviewed and hopefully
accepted.  If changes to the source tree patch are required to get it accepted,
I'll try and fold in the newer features as well.
Created attachment 42239 [details] [diff] [review]
patch to mozilla source implementing Link Toolbar as of XPI Strikes Back
Created attachment 42240 [details]
ZIP archive of binary files to accompany above source tree patch
I've attached a patch to the mozilla source tree and a zip containing the new
binary files.  Both files assumes that the source is checked out into a
directory named 'mozilla'.  All new files are in mozilla/extensions/linktoolbar/
or a subdirectory thereof.  When it comes time, a cvs add should be relatively
painless.

Because the patch modifies configure.in you'll need to rerun autoconf, per the
Detailed Unix Build instructions (assuming you're building on *nix).

Gervase, who volunteered to test the patch, replied that the linktoolbar should
be added to xpfe/components instead of extensions.  I'll let him post his own
comment instead of trying to summarize his position myself.  I see it belonging
in either location.  Pragmatically speaking, the patch works.  The JAR files
generated in mozilla/chrome are essentially identical regardless of whether the
Link Toolbar is an extension or a component.

Further testing is appreciated.  Please post your results here, including which
platform you are on.
(Reporter)

Comment 132

16 years ago
extensions/ is for things which are optional in the build. <LINK> support is a
key part of our HTML 2 story and should not be optional. If you look at the
things currently in extensions/ (venkman, chatzilla, transformiix, xmlterm) you
can see that it's not appropriate for the links toolbar to be there.

xpfe/components already contains things like the personal toolbar and so on;
it's the logical place for the links toolbar. The Links Toolbar should be placed
in components, and the code altered so that it works like the Personal Toolbar -
using static rather than dynamic overlays, and tying into the current components
build process. The skin bits should be put in the relevant bits of the skin
directories.

Gerv
I disagree on a couple of counts:

1. LINK support is an optional part of the HTML spec.  Having the implementation
be an optional part of the build is congruous with that.  This is really just a
pedantic argument, because the Link Toolbar is included in the build by default.
 You have to explicitly disable it.

2. You only listed example extensions that support your argument.  Some other
things found in extensions that don't support your position are cookie and wallet.

I'm not opposed to putting the Link Toolbar in xpfe/components.  However, I
disagree that extensions is any less appropriate than components.  The work's
been done.  The patch works.

Comment 134

16 years ago
FWIW, I agree with Gerv about the location. I'd also be very disappointed to see
this go in without the autohide option - not just because I wrote it, but
because a *lot* of people besides myself seem to want to use it as their
preferred mode. This should be trivial if your work is based on "strikes back",
because you can probably just replace the files my patch modifies (excluding the
skin file patches which should go in a new css file in content anyway)

If there's anything I can do to help at this point, let me know (perhaps in
private mail to avoid cluttering up the bug).
Whadya mean "cluttering up the bug"?  We're talking implementation details fer
chrissakes  ;)

I'm confident that the autoshow feature will make it into the implementation. 
Either as a revision to this patch, or as a seperate patch once the toolbar is
in CVS.  Also, as you mentioned in n.p.m.ui, mpt has some reservations about the
depth of View -> Show/Hide -> Link Toolbar -> ....  Although I'm confident that
autoshow will make it in eventually, I'm not so confident that the current
implementation won't hold back the Link Toolbar as of XPI Strikes Back, when we
had concensus that it was "Good Enough".

If you'd like to help, you could work on making a patch to Gerv's satisfaction.
   I wouldn't mind that at all.  That should at least gaurantee r=Gerv. 
Meanwhile, looks like I'll have to go fishing for another reviewer for the
current patch :)

Comment 136

16 years ago
> 1. LINK support is an optional part of the HTML spec.  Having the implementation
> be an optional part of the build is congruous with that.  

Having UI for forward-backward-reload is not even part of any HTML spec. 
But you would not call it "optional" for a browser.

> 2. You only listed example extensions that support your argument.  Some other
> things found in extensions that don't support your position are cookie and 
> wallet.

Where is "Personal Toolbar" and "Status Bar"?

"Site Navigator Bar" should be in the same location.

Comment 137

16 years ago
I can imagine, depending on who does your review, that some people might object
to the UI on the grounds of *not* having autoshow... anyhow...

In terms of location in the build, I'd like to see this put in the "right" place
now. Arguing where the "right" place is is valid, but arguing that it should go
in extensions because the patch exists and works isn't a valid argument - that
logic has led to many messes that always end up never getting fixed. My personal
suspicion is that this is how wallet and cookie ended up in extensions and never
got moved...

I can (just about) live with a checkin without autoshow so long as somebody can
reasonably guarantee that the autoshow will get r/sr fairly quickly afterwards,
and won't be held up unreasonably hoping for an ideal UI that will never come.
But I can't live with putting it in a wrong location, and I've yet to be
persuaded that extensions isn't a wrong location.

Comment 138

16 years ago
> Also, as you mentioned in n.p.m.ui, mpt has some reservations about the
> depth of View -> Show/Hide -> Link Toolbar -> ....  Although I'm confident
> that autoshow will make it in eventually, I'm not so confident that the
> current implementation won't hold back the Link Toolbar as of XPI Strikes
> Back, when we had concensus that it was "Good Enough".

If I can get mpt's personal guarantee that he won't oppose the checking in of
the current autoshow implementation including UI (I don't know if I can, but I
can try ;) ) would you consider adding the autoshow component to the initial
patch?

I can't guarantee that I'll have time this weekend to work on providing a build
system patch (ie, don't stop working on it yourself assuming that I will). But I
will try to find time and have a go at it - although any patch I produce will
probably include autoshow unless someone persuades me otherwise.
(Reporter)

Comment 139

16 years ago
Tim: wallet in Mozilla is a byword for doing everything that it's possible to 
do, wrongly. And the cookie manager is part of wallet.

> The work's been done.  The patch works.

From this logic comes many evil hacks. If you like, I could drag brendan or 
someone over here to tell you to put it in components and integrate it properly 
(removing dynamic overlaying), or we could just skip that step and do it anyway. 
:-) And you could take the opportunity to integrate auto-hide, although I would 
say that this is definitely _not_ required for initial checkin. Mozilla is 
allowed to go through times when bits of it are half-finished - that's what 
development is.

And what are we going to call it in the UI? I like "Site Navigation Bar", but 
something a bit shorter might be better. We definitely want something better 
than "Link Toolbar" - your average user will have no clue what that is.

Gerv

Comment 140

16 years ago
> And what are we going to call it in the UI? I like "Site Navigation Bar", but
> something a bit shorter might be better. We definitely want something better
> than "Link Toolbar" - your average user will have no clue what that is.

I don't like 'Link Toolbar' either - IE escapees might confuse it with IE's
Links Bar (Bill's name for the Personal Toolbar). I'm a bit worried that 'Site
Navigation Bar' might be confused with the regular Navigation Toolbar. How about
'Site Links', 'Page Links' or 'Document Links' or something?
> If you like, I could drag brendan or someone over here to tell you to put
> it in components and integrate it properly (removing dynamic overlaying)

What I would like, if you could, is for you to drag some module reviewers into
here to review the patch and draw their own conclusions.  An objective second
(and third, fourth, etc.) opinion would certainly be helpful.

It's certainly ironic if, as you say, wallet is the bad seed of extensions,
because I used it as a code example to help me figure out how to incorporate the
link toolbar into the build.
The name for this toolbar should include the word "Toolbar" just like
"Navigation Toolbar" and "Personal Toolbar".

I'll throw the name "Advanced Navigation Toolbar" into the fray. For people
worried about long menu labels, here are examples of some other long-ish menu
labels in comparison

     View -> Advanced Navigation Toolbar
     File -> Open Web Location... Ctrl+Shift+L
   Search -> Find in This Page... Ctrl+F
    Tasks -> Privacy and Security
Bookmarks -> Manage Bookmarks... Ctrl+B

And, of course, there are the generated menu items in "Go" and "Bookmarks" menus
that can be very long.

Unless there's a limit stated in a UI guideline, I recommend prioritizing
clarity over brevity, taking into consideration that sometimes brevity aids clarity.

Comment 143

16 years ago
"Site Navigation Toolbar" -- both shorter and more descriptive than "Advanced...".
(Reporter)

Comment 144

16 years ago
> What I would like, if you could, is for you to drag some module reviewers into
> here to review the patch and draw their own conclusions.  An objective second
> (and third, fourth, etc.) opinion would certainly be helpful.

I can't offer anyone's time to review this apart from my own, which I have been
offering and continue to offer. I'm not sure what you mean by an "objective
second opinion" - do you think I have some vested interest (stock options
perhaps) in the components/ directory? The people who probably have the final
say where it goes are blake and ben - perhaps you should ask them to pop over
here and give a ruling.

> The name for this toolbar should include the word "Toolbar" just like
> "Navigation Toolbar" and "Personal Toolbar".

Semi-counter-example: Status Bar. How is a "toolbar" different from a "bar"?
Good question. 

I think we should avoid the word "Advanced"; there is nothing particularly
Advanced (advanced compared to what?) about <LINK>-based navigation, and it's a
word that I think will scare users. 

What's it really _for_? It's for navigating between related pages. "Page
Navigation Bar"? Regarding possible confusion, we should also think whether,
given that it contains Print and Search, Reload and Stop buttons (which are not
directly to do with Navigation), whether "Navigation Toolbar" is the right name
for the main toolbar.

Oh, and how about "Only When Supported" or "When Supported" rather than "Only As
Needed"?

Gerv
> I'm not sure what you mean by an "objective second opinion" - do you think
> I have some vested interest (stock options perhaps) in the components/
> directory?

I was contrasting your offer to bring someone in to prove me wrong versus my
suggestion to bring someone in to review the patch.  I don't attribute malice to
any of your actions or opinions.  I just disagree with some.

I readily admit I may be wrong.  I'm just would like a second opinion.  A moment
ago you were willing to try and bring someone in to prove your point.  I assumed
you could apply these same connections to attempt and attract another reviewer.

> The people who probably have the final say where it goes are blake
> and ben - perhaps you should ask them to pop over here and give a ruling.

Would you mind trying?  I'm sure you have a greater chance of attracting their
attention than I do.

In the meantime, it would be helpful if someone could test the current patch. 
Assuming that I need to rework it for extensions, I still need to know that I at
least got things right with the first patch.  Undoing the changes made by the
current patch is as simple as:

$ rm -rf configure.in extensions/makefile.win extensions/linktoolbar
$ cvs update configure.in extensions/makefile.win
$ autoconf
$ gmake -f client.mk build

Updated

16 years ago
Blocks: 90966

Comment 146

16 years ago
I've got this installed locally and it looks to run rather well.  One thing I've
noticed is that I can right-click the items in the Document and More menus.  The
item executes but the menu doesn't close.  The Bookmarks menu on the Personal
Toolbar doesn't do this, I wonder why these menus do?

Comment 147

16 years ago
>I don't like 'Link Toolbar' either - IE escapees might confuse it with IE's
>Links Bar (Bill's name for the Personal Toolbar). I'm a bit worried that 'Site
>Navigation Bar' might be confused with the regular Navigation Toolbar. How about
>'Site Links', 'Page Links' or 'Document Links' or something?


how about:

Site-Provided ToolbarSite-Specific Toolbar
Site-Customized Navigation Bar
Remote Site's Navigation Bar
Extra Site Navigation Toolbar
Quick Links Toolbar
or some variation thereof?

Comment 148

16 years ago
First comments on the latest patch: sounds good all in all. I haven't tested the
patch but I began looking at the code. Here are a few points that I would raise,
but then again who am I to say whether they are really bad or just a matter of
style...
1) What struck me is that in the functions defining an object, for example,
function LinkToolbarHandler(), sometimes you define member functions using
"this.functionName = function()", and sometimes you define functions (are they
member functions? I don't think so) without using this.functionName, like
function getLinkType(relAttribute). I am still wondering how a call to "function
fctName()"  {} inside another function works. IMHO, it would all be much clearer
if you used LinkToolbarHandler.prototype = { } instead of the confusing
definitions you have at the moment. I trust the current code works, but it just
seems evil. 

2) Sounds like overkill to me. I'd remove the |else return false| entirely.
+ if (findWord("icon", this.rel.toLowerCase()))
+      return true;
+    else
+      return false;
+
+    return false;

3)+    if (!this.rel) return true;
+    if (this.isStylesheet()) return true;
+    if (this.isIcon()) return true;
This could be written |if( !this.rel || this.isStyleSheet() || this.isIcon() )
return true|, but then again it's just a matter of style.
The efficiency should be exactly the same.

Ok this is just an initial code-only review, I'll comment more as I read the
patch further.
> 1) What struck me is that in the functions defining an object...sometimes
> you define member functions using "this.functionName = function()", and
> sometimes you define functions...like function getLinkType(relAttribute).
> I am still wondering how a call to "function fctName()"  {} inside another
> function works.

Think of it as the difference between instance methods and static methods in a
class based language.  That's not how it works in JavaScript, but that's the
basis for how I wrote the functions.  Functions defined as "function fctName()"
don't need access to any "this.foo" members, so they're sort of like static
methods.  Defined that way, the function is only in scope in the containing
function/class, so it's also a form of encapsulation.

> IMHO, it would all be much clearer if you used
> LinkToolbarHandler.prototype = { } instead of the confusing definitions you
> have at the moment.  I trust the current code works, but it just
> seems evil.

Well, coming from a Java programming background, the Class.prototype = { } sytax
seems evil ;-)

However, when in Rome, do as the Romans do.  For that reason, I've considered
rewriting to use the syntax you mention.


> 2) Sounds like overkill to me. I'd remove the |else return false| entirely.
> + if (findWord("icon", this.rel.toLowerCase()))
> +      return true;
> +    else
> +      return false;
> +
> +    return false;

Yup, that's an oversight.  I was actually changing it from a guard conditional
idiom to a regular if-else idiom, but forgot to remove the last return statement.


> 3)+    if (!this.rel) return true;
> +    if (this.isStylesheet()) return true;
> +    if (this.isIcon()) return true;
> This could be written |if( !this.rel || this.isStyleSheet() || this.isIcon() )
> return true|, but then again it's just a matter of style.
> The efficiency should be exactly the same.

Agreed.  This is partly a style issue, but also a holdover from when I
erroneously convinced myself that JavaScript didn't short-circuit conditionals.
 I say "partly a style issue" because I like to keep unrelated gauard
conditionals in seperate if statements.  I'll rewrite it as:

  if (!this.rel) return true;
  if (this.isStylesheet() || this.isIcon()) return true;


Thanks for the review!
What's the status of this feature getting in?
(Reporter)

Comment 151

16 years ago
Tim requested super-review some time ago, but seems to have been ignored. I
suggest he request it again, and if a response is not received within 48 hours
from the selected super-reviewer, to take it up with staff@mozilla.org.

Gerv

Comment 152

16 years ago
There was also the issue of whether it should be in extensions/ or components/ -
I don't know whether any resolution was ever reached on this, but (last I
remember hearing) the only actual existing patch placed it in extensions, which
some people thought was wrong. Was this ever resolved?
(Reporter)

Comment 153

16 years ago
That's a very good point. It should be in components. If this is to get
super-review, a patch needs to appear which does this. It'll probably change
enough that it needs reviewing again, as well.

We'll get there eventually :-)

Gerv

Comment 154

16 years ago
I really wish I had the time and knowledge to do this. I said I'd "try to find
time" months ago (with a warning not to depend on me) and needless to say, I
couldn't (life is hectic and shows no sign of slowing down).

Surely all it would take for someone who knows what they're doing would be to
change a few directories, add a few overlay tags, and make sure that the whole
thing compiles and works afterwards?

Updated

16 years ago
Blocks: 96683

Updated

16 years ago
No longer blocks: 96683

Comment 155

16 years ago
I really hope this patch isn't going to end up bitrotting *again* due to such
trivial issues.

*please*, if there's anyone out there with the time or ability to work on this,
consider doing the small amount of work necessary to move the most recent
attached patch from extensions/ to components/. I'll still snap it up the moment
I get a chance to do so, if nobody already has, but I still don't see that
happening in the forseeable future. So consider this pitiful grovelling as
bug-advocacy: I know that the second-last thing this bug needs is more comments,
but the *very* last thing it needs is to be abandoned to bitrot again for the
third time.
The extensions to components issue is in stalemate.  I could spend the time to
move things to components to satisfy Gerv, but that wouldn't garauntee an sr=. 
Gervase and I have both requested second opinions and have heard nothing.  To be
honest, I'm not inclined to spend any more of my time coding until this bug
receives some arbitration or reviewer attention.  There's also the possibility
that the previous bitrot and current neglect means this feature isn't desired
except by our small group.

When I get a chance this week I'll follow Gerv's suggestion for escalating the
review of this bug.  In the meantime, if someone wants, they can advocate this
bug now if they're as confident as I am that extensions is at least an equally
appropriate location for the link toolbar.  No coding necessary.  In that case,
we still need an r= and an sr=.
(Reporter)

Comment 157

16 years ago
Created attachment 47932 [details] [diff] [review]
Gerv's patch v.1
(Reporter)

Comment 158

16 years ago
If you want something doing...

The patch I just attached is all the files, with the locations changed to the
correct place. I've cvs added them there; they can be removed if the
super-reviewer decides they should be elsewhere.

I haven't included the binary images; they haven't changed, although they've
moved to the relevant skin directories (toolbar subdir in the case of modern) if
anyone wants to try this patch out. I've also removed quite a lot of debugging
dump() statements and other debugging code which should not go into the tree.
Lastly, I changed a few style nits to fit in with the tree style. I've tested it
a bit and it does still seem to work.

I can't review my own patch, but drbrain and tim wrote most of it. Hmm. Never
mind - could one of you review it, please, and make sure it's still the same
patch? :-)

Gerv
(Reporter)

Comment 159

16 years ago
Come on, people - if you are all so keen to see this in the tree, you could at
least review my patch.

Gerv
(Reporter)

Comment 160

16 years ago
More specifically: drbrain or tim, could you please either review this patch or
comment that you aren't going to?

Thanks :-)

Gerv
Are they allowed to review the patch, since it contains their code?  Anyway, I
talked to db48x on IRC last night and agreed to exchange his review of this
patch for my review of his (52730).  Adding him to cc to make sure it happens...
(Reporter)

Comment 162

16 years ago
> Are they allowed to review the patch, since it contains their code? 

Well, I reviewed that bit. They need to review the changes I made.

> Anyway, I
> talked to db48x on IRC last night and agreed to exchange his review of this
> patch for my review of his (52730).  Adding him to cc to make sure it happens..

OK. Whatever. Just as long as someone reviews it ;-)

Gerv

Comment 163

16 years ago
Brought to you by The Reviewers Corp Inc S.A. (TM)
Thanks to Boris Zbarsky for helping me out very much :-)
*************languageDictionary.js***************
1.
+ if (this.getDictionary()[languageCode])
+   return this.getDictionary()[languageCode];
should be (for performance and style reasons)
var language = this.getDictionary()[languageCode];
if (language) return language; else return "";

2. 
+ if (this.dictionary == null)
should be (for style reasons)
if (! this.dictionary)

***********linkToolbarHandler.js************
1.
+ for (var i = 0; i < nodeList.length; i++)
should be (for performance reasons)
var length = nodeList.length;
for(var i = 0; i < length; i++)

2. In function this.handle():
+ var element = new LinkElementDecorator(element);
Use another var name than |element|. Very confusing.

3. 
+ * XXX: would rather add some methods to Element.prototype instead of
+ * using a decorator, but Element.prototype is no longer exposed 
+ * since the XPCDOM landing, see bug 83433
Bug is fixed. Should we re-do the patch to accomodate this?

4.
+  function getText(element) {
+    return condenseWhitespace(getTextRecursive(element));
+  }
and related functions:
A (much?) more efficient way would be to use the Range object which does
everything automatically for you. I'm not sure myself how to do it exactly, but
it should be easy. I can do that if you so wish.

5.
+  this.enableParentMenuButton = function() {
+    if(this.getParentMenuButton())
+      this.getParentMenuButton().removeAttribute("disabled");
+  }
cache this.getParentMenuButton()

6. 
In this.displayLink and this.isAlreadyAdded: something's really wrong. You
assign "true" to getKnownLinks()[linkElement.href]
then in isAlreadyAdded() you attempt to use
getKnownLinks()[linkElement.href].media and .hreflang. What's the point? Is 
this on purpose? Maybe add comments to the code if it's on purpose? Should it be
getKnownLinks()[linkElement.href] = linkElement; ?
Please also cache this.getKnownLinks()[linkElement.href] since a call to that
isn't exactly cheap.

*************http.js**************
1. global var ioService is initialized in getHTTPHeaders() but overriden with 
another instance of nsIIOService directly in
doGetHTTPHeaders(). What gives? Why the commented out code in doGetHTTPHeaders?

2. badly named http.js file (in our opinion, a too generic name)

3. Rewrite doGetHTTPHeaders(). Bad indentation and excessive unconditional
|return|. If possible do less inside the try {}, and more outside. In that
function you sometimes return false and sometimes return void, that is inconsistent.

4. in function parseLinkHeader(content)
Undeclared variables: contentlen, state, starthref, endhref, startparam,
endparam, startparamval, endparamval, curchar

5. comment that some functions in there are not used (getHTTPHeaders, ...)

6. +  // XXX: cleanup
+  var ioService = 
Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces["nsIIOService"]);
+   var base = ioService.newURI(window._content.document.base 
+      ? window._content.document.base
+      : window._content.document.URL, null);
Reuse the ioService global var.

************linkToolbarOverlay.js**************
1.
+  for (var i = 0; i < metas.length; i++) {
should be
var length = metas.length;
for(var i = 0; i < length; i++)

2.
+  for (var i = 0; i < metas.length; i++) {
+    if (metas.item(i).hasAttribute("http-equiv"))
+      if (metas.item(i).getAttribute("http-equiv").toLowerCase() == "link")
+        links.push(parseLinkHeader(metas.item(i).getAttribute("content")));
should be (again, in our opinion)
for (var i = 0; i < length; i++) {
  var currentMeta = metas[i];
  if (currentMeta.hasAttribute("http-equiv") &&
currentMeta.getAttribute("http-equip").toLowerCase() == "link")
    links.push(parseLinkHeader(currentMeta.getAttribute("content")));
}

3. 
+  function clicked(event) {
+    if (event.target.nodeName == "menubutton") return;
+    if (!event.target.getAttribute("href")) return;
+    loadURI(event.target.getAttribute("href"));
should be (imho)
function clicked(event) {
  if (event.target.nodeName == "menubutton") return;
  if(event.target.hasAttribute("href")) {
    loadURI(event.target.getAttribute("href"))
  }
  return;
}

**************linkToolbarOverlay.xul*************
1. <script src="..."> should refer to full chrome URL's, not relative url's

2. No need for language="javascript" in the <script>'s. We don't use that
attribute in Mozilla.

3. Investigate use of observers instead of addEventListener(load). I'm not sure
at all about this one. It's just a thought I wrote down, maybe one of you knows
more about them.

4. Won't <toolbox id="navigator-toolbox"> interfere with the navigator.xul one?
Is it on purpose?

LinkToolbarOverlay.css is diffed twice, as well as all the gifs.
Navigator.xul and linkToolbarOverlay.dtd changes look good.

We'll look again when we get a new patch :-)
Thanks for doing all this work guys, it's very appreciated!
I can't get this to run right, mozilla keeps crashing in docshell.dll. (the
mozilla window opens, with the link toolbar showing, but it crashes as it tries
to load a document)

Looking at the patch, the modifications to the jar.mn files show that you're
putting all the files in with the regular chrome files, but the zip file of
images extracts to extentions/linktoolbar/. Personally, I'd prefer all the files
to go under components/linktoolbar/[content|skin], with a seperate jar.mn. You
can still have the files end up in comm.jar/modern.jar/classic.jar and so on if
you like, but it makes rebuilding just this project much easier. 

Also, the diff is kind of unorganized; some of the files (like
languageDictionary.js) are diffed from the directory they're in, whereas some
files (like the jar.mn files) are diffed from the mozilla/ directory. It'd make
it much much easier if they were all diffed from the same point in the tree.

Other than those nits and the things Fabian brought up, I think this is pretty
close. (I doubt the crash is truely your fault)
Optimizations should be benchmarked to ensure enough (or any) improvement is
made .  I disagree on adding temporary variables merely as a style improvement.
 Avoiding the use of temporary or cache variables improves readability and
maintainability ("Refactoring", Fowler).  They should be avoided unless needed
for a demonstrated optimization.

Can we get the "unoptimized" code in the tree and accept optimizations as
patches.  I write "unoptimized" because I've already done some optimization,
backed by benchmarks, that shaved off a couple hundred milliseconds.  BTW, how
many milliseconds per page load is considered a signifigant performance
improvement to justify less concise code?

I've seen examples of conflicting style in the tree, so I'm suspicious whenever
someone cites style improvements.  Is there a style document, or  a particular
section of code to emulate?  Or  are these a matter of personal preference?  The
one convincing style suggestion so far is switching to "Class.prototype = { ...
}" for class declarations.  There is overwhelming evidence that this is the
proper style.


> *************languageDictionary.js***************
[...]
> 2. In function this.handle():
> + var element = new LinkElementDecorator(element);
> Use another var name than |element|. Very confusing.

Would renaming it to "htmlElement" make it clear?


> 3.
[...]
> Bug is fixed. Should we re-do the patch to accomodate this?

It can always be patched seperately once it's in the tree, yes?


> 6.
> In this.displayLink and this.isAlreadyAdded: something's really wrong.

Agreed.  For now, we can remove the following lines, since the conditions
are always false: 

// FIXME: media="screen all" should be equivalent to media="all screen"
if (!match(this.getKnownLinks()[linkElement.href].media, linkElement.media))
return false;
if (!match(this.getKnownLinks()[linkElement.href].hreflang, linkElement.href))
return false;

That will eliminate the confusion.  The intended functionality can be added
later , and even this botched attempt would have been incomplete.


> *************http.js**************
[snip]

Agreed.  http.js is a mess and only META HTTP-EQUIV works.  The code might as
well be removed until it can be done completely and properly.


> ************linkToolbarOverlay.js**************
[...]
> 3.
> +  function clicked(event) {
> +    if (event.target.nodeName == "menubutton") return;
> +    if (!event.target.getAttribute("href")) return;
> +    loadURI(event.target.getAttribute("href"));
> should be (imho)
> function clicked(event) {
>   if (event.target.nodeName == "menubutton") return;
>   if(event.target.hasAttribute("href")) {
>     loadURI(event.target.getAttribute("href"))
>   }
>   return;
> }

The conditionals are examples of a programming idiom called a guard clause. 
These take the form of simple conditionals at the top of a function that guard
entry to the rest of the function.  Anyone familiar with the idiom will
understand the intent.  It makes the rest of the method body easier to read. 
Normally I follow the guard  clauses with a blank line, which unfortunately I
neglected to do here.


> **************linkToolbarOverlay.xul*************
[...]
> 4. Won't <toolbox id="navigator-toolbox"> interfere with the navigator.xul one?
> Is it on purpose?

It's on purpose.  It is how XUL overlaying is done.
> > *************languageDictionary.js***************
> [...]
> > 2. In function this.handle():
> > + var element = new LinkElementDecorator(element);
> > Use another var name than |element|. Very confusing.
> 
> Would renaming it to "htmlElement" make it clear?

Erg...brainfart.  How about "linkElement"?  I had used that originally, but
found it redundant with the class name "LinkElementDecorator".
(Reporter)

Comment 167

16 years ago
> Looking at the patch, the modifications to the jar.mn files show that you're
> putting all the files in with the regular chrome files, but the zip file of
> images extracts to extentions/linktoolbar/.

Yes, that's my fault - I didn't accompany the patch with a correctly-zipped zip
of the images. Would you like me to do that?

> Also, the diff is kind of unorganized; some of the files (like
> languageDictionary.js) are diffed from the directory they're in, whereas some
> files (like the jar.mn files) are diffed from the mozilla/ directory. It'd 
> make it much much easier if they were all diffed from the same point in the 
> tree.

I'm not quite sure why that has happened - they are all diffed from the mozilla/
directory. Ah, I see - the ones that are new files, and have been cvs added, are
the ones that appear to be diffed from the directory they are in. This must be a
quirk of CVS. Does it present problems applying the patch?

Gerv
(Reporter)

Comment 168

16 years ago
Created attachment 48698 [details] [diff] [review]
Patch v.2
(Reporter)

Updated

16 years ago
Attachment #47932 - Attachment is obsolete: true
(Reporter)

Comment 169

16 years ago
Created attachment 48699 [details]
Images for Gerv's patch; unzip in mozilla directory
>> Looking at the patch, the modifications to the jar.mn files show that you're
>> putting all the files in with the regular chrome files, but the zip file of
>> images extracts to extentions/linktoolbar/.

>Yes, that's my fault - I didn't accompany the patch with a correctly-zipped zip
>of the images. Would you like me to do that?

Yea, that'd be great, thanks.

>> Also, the diff is kind of unorganized; some of the files (like
>> languageDictionary.js) are diffed from the directory they're in, whereas some
>> files (like the jar.mn files) are diffed from the mozilla/ directory. It'd 
>> make it much much easier if they were all diffed from the same point in the 
>> tree.

>I'm not quite sure why that has happened - they are all diffed from the mozilla/
>directory. Ah, I see - the ones that are new files, and have been cvs added, are
>the ones that appear to be diffed from the directory they are in. This must be a
>quirk of CVS. Does it present problems applying the patch?

That really is weird. It does make it difficult to apply, because all the files
that were added end up in mozilla/. It's not really more than a nuisance if you
expect it to happen.
Comment on attachment 48698 [details] [diff] [review]
Patch v.2

Comments on Gerv's patch v.2

> Index: xpfe/browser/resources/content/linkToolbarHandler.js

>+  this.handle = function(linkElement) {
>+    // XXX: if you're going to re-enable handling of anchor elements,
>+    //    you'll want to change this to AnchorElementDecorator
>+    var linkElement = new LinkElementDecorator(element);
>+
>+    if (linkElement.isIgnored()) return;
>+
>+    var relAttributes = linkElement.rel.split(" ");
>+    for (var i = 0; i < relAttributes.length; i++) {
>+      var linkType = getLinkType(relAttributes[i]);
>+      this.getItemForLinkType(linkType).displayLink(linkElement);
>+    }
>+  }

That function is now broken.  What Fabian and I meant by our comment about the variable name
is that the function argument should be named something different from the variable you assign
the new LinkElementDecorator to.  The changes you've made don't really help that and break the
function (since |new LinkElementDecorator(element);| will now happily fail).

> Index: xpfe/browser/resources/content/linkToolbarOverlay.js

>+        links.push(parseLinkHeader(metas.item(i).getAttribute("content")));

Since you removed http.js, parseLinkHeader() is no longer present.  Please put http.js back
(with a better name like http-link.js maybe) or remove this call.

>Index: xpfe/browser/jar.mn

>+        content/navigator/http.js                      (resources/content/http.js)

Again, be consistent in your removal or not of http.js
Attachment #48698 - Flags: needs-work+
> This must be a quirk of CVS. Does it present problems applying the patch?

It should not be a problem if one sets POSIXLY_CORRECT=1 when making the diff
and when applying the patch.

You should keep http.js in, because if there are 3 places the information can
come from, supporting 2 of the three is better than just 1 of the three. I think
the only complaint was that the name doesn't suggest who uses it, or what it
does. The name could indicate that it's required to support the http protocol,
for all someone unfamiliar with mozilla knows. On the other hand, if you were to
stick everything in your own subdirectory of xpfe/components/, you could pretty
much call it whatever you want, but that's just my opinion.

Also, I keep thinking that the language dictionary stuff has been done before,
perhaps there's a scriptable object out there that we could reuse? If not then
one should be created. On a different note, are getDictionary, createDictionary
etc closures? Just curious as to how JS does that.

Can the four regular expressions in findWord() be replaced by a single
expression? Maybe /\W$word\W/?
http.js has several problems, six of which were outlined by Fabian just a few
comments ago.  To his list, I'll add that it's not object oriented, as the rest
of the code is.  It will take more work then just renaming the file.  Work that
can be done later, under a seperate bug for adding support for HTTP LINK headers.

Comment 175

16 years ago
Comment on attachment 48698 [details] [diff] [review]
Patch v.2

>Optimizations should be benchmarked to ensure enough (or any) improvement is 
>made.
That sounds like a good argument to me. In my comment I suggested using
var length = array.length; for(var i = 0; i < length; i++)
because Boris told me that JS doesn't inline array.length in a for loop, because the array could grow in the loop. So for a large array, caching the length would avoid a lot of (unnecessary, since the length is constant) calls to array.length.
I agree that caching an expression that comes back often just for a matter of style is not good. We should probably cache expressions only in loops or functions called very often. I hope you see what I mean.

>Can we get the "unoptimized" code in the tree and accept optimizations as
>patches.
Sounds good to me, but if this going to add overhead to the page load time (I haven't tested the patch so I can't tell), let's try to quickly work on optim after it's checked in :-)

>> 3.
>[...]
>> Bug is fixed. Should we re-do the patch to accomodate this?
>It can always be patched seperately once it's in the tree, yes?
Yes. But as usual let's not forget it.

>Agreed.  http.js is a mess and only META HTTP-EQUIV works.  The code might as
>well be removed until it can be done completely and properly.
OK.

>The conditionals are examples of a programming idiom called a guard clause. >[...]
>Normally I follow the guard  clauses with a blank line, which unfortunately I
>neglected to do here.
You didn't, I removed the blank line when commenting on it.

The rest looks fine. Address Boris' new comments and we'll almost be there!
Contradicting myself somewhat about having optimizations be a seperate bug I did
a benchmark.  Using a test document containing 127 REL links (the GNU make
manual test case modified to have LINK elements instead of anchors) I tested the
following optimization:

    var length = nodeList.length;
    for (var i = 0; i < length; i++) {
        /* ... */
    }

Average Page Load (milliseconds)
Before     After
 936ms     926ms

I loaded the test case 6 times, and through out the first page load.  The
optimization saves an average of 10 milliseconds in this case.

Things to consider:

1. My machine: 750 MHz Athlon, 256 MB RAM, Linux
2. 127 REL links is a large number of links
3. Is 10 milliseconds a signifigant performance improvement

I'm being a bit pedantic.  In this case, the optimization is simple, the use of
a temporary hardly obscures the code, and doing it demonstratably saves time.

One thing to note, nodeList is /not/ an array.  It's an XPCOM wrapped C++ object
implementing the DOM NodeList interface.  I suspect the implementation caches
its length so all we're paying for here is the method invocations.


Side Note:

In LinkToolbarHandler.js, this code clears out all the link toolbar buttons and
menuitems.  It loops using an associative array:

  this.clearAllItems = function() {
    for (var linkType in this.items)
      this.items[linkType].clear();
    this.hasItems = false;
  }

This may be another candidate for optimization, though doing so will be more
complex than just adding a temporary.

Comment 177

16 years ago
>One thing to note, nodeList is /not/ an array.  It's an XPCOM wrapped C++ object
>implementing the DOM NodeList interface.  I suspect the implementation caches
>its length so all we're paying for here is the method invocations.
As far as I know, a NodeList, as defined by the W3C, is live, i.e. its length
can vary during the execution of the program. This is why I said the length of
the NodeList is recomputed each time (though it's probably just a call to get a
data member of the objct).
Maybe someone knows exactly what's going on?
(Reporter)

Comment 178

16 years ago
Created attachment 48807 [details] [diff] [review]
Patch v.3
(Reporter)

Comment 179

16 years ago
Patch v.3 attached. Setting POSIXLY_CORRECT=1 didn't seem to have any effect on
the diff produced. 

Any further comments should be of the form: "This really can't go into the tree
as it is, because the following code is evil, bad and wrong, or doesn't work at
all: ..."

Gerv
Comment on attachment 48807 [details] [diff] [review]
Patch v.3

Alright.  This really can't go into the tree as it is, because the following
code doesn't work at all (except the overall file location thing, which
may be just evil):

>Index: xpfe/browser/resources/content/linkToolbarOverlay.xul

>+<?xml-stylesheet href="chrome://linktoolbar/skin/linkToolbarOverlay.css" type="text/css"?>

You mean href="chrome://navigator/skin/toolbar/linkToolbarOverlay.css" right?
Either that, or fix your jar.mn files....  Which seem to be inconsistent,
by the way.  More on this later.

>+  <script type="application/x-javascript" src="chrome://navigator/content/LinkToolbarHandler.js" />
>+  <script type="application/x-javascript" src="chrome://navigator/content/LinkToolbarItem.js" />
>+  <script type="application/x-javascript" src="chrome://navigator/content/LanguageDictionary.js" />

The first "l" in the filename should be lowercased in all of those.
Otherwise the scripts don't get loaded and bad things happen.

>Index: xpfe/browser/jar.mn

>+        content/navigator/languageDictionary.js        (resources/content/languageDictionary.js)
>+        content/navigator/linkToolbarHandler.js        (resources/content/linkToolbarHandler.js)
>+        content/navigator/linkToolbarItem.js           (resources/content/linkToolbarItem.js)
>+        content/navigator/linkToolbarOverlay.js        (resources/content/linkToolbarOverlay.js)
>+        content/navigator/linkToolbarOverlay.xul       (resources/content/linkToolbarOverlay.xul)
>+        locale/en-US/navigator/linkToolbar.dtd              (resources/locale/en-US/linkToolbar.dtd)

So.  Question.  Wasn't this stuff going to move to xpfe/components/linktoolbar ?
It seems to be living in xpfe/browser/resources/content at the moment with
Gerv's current patch...

Don't you want to put this stuff in components/ and edit components/jar.mn
to include it in comm.jar?

>Index: themes/classic/jar.mn

>+    skin/classic/navigator/linkToolbarOverlay.css                        (navigator/linkToolbarOverlay.css)

>Index: themes/modern/jar.mn

>+  skin/modern/navigator/toolbar/linkToolbarOverlay.css             (navigator/toolbar/linkToolbarOverlay.css)

These put the css file in different places in the jar.  Please fix that.
The path inside the jar should be the same for the CSS file and the 
images in both themes.

> Index: themes/modern/navigator/toolbar/linkToolbarOverlay.css

All the image urls in this file are broken with the current file locations.
Once you have the jar.mn's fixed, please update the chrome:// urls in here.
Attachment #48807 - Flags: needs-work+

Comment 181

16 years ago
Gerv, when you do get this into the tree please file a bug on me for the
autohide support, and I'll update my patch to the new locations.


Also, FWIW: Anyone testing this who's ever had the xpi installed in the same
installation directory (on Linux at least): I've found that regchrome will *NOT*
uninstall the xpi even if you remove the relevant lines from
installed-chrome.txt first. The only way to do it is either to edit all-*.rdf
and remove the linktoolbar sections, or (at your own risk) blow away all-*.rdf
completely (as well as removing the lines from installed-chrome.txt) and run
regchrome again.
(Reporter)

Comment 182

16 years ago
bz: fair comments :-( I think I probably wasn't testing the current version, but
an old version - this is why it all still seemed to work.

>Index: xpfe/browser/jar.mn

> So.  Question.  Wasn't this stuff going to move to 
> xpfe/components/linktoolbar ?
> It seems to be living in xpfe/browser/resources/content at the moment with
> Gerv's current patch...

Hmm. I appear to have misremembered the substance of the argument about this.
However, I would now argue (and perhaps this is a change from my previous
position; I'm not afraid to admit it when I'm wrong :-) that its closest
counterpart in the tree currently is the Personal Toolbar, and so it should live
wherever that lives - which is in navigator/, where I put it. This was what was
going through my head as I was making the patch, at any rate.

The links toolbar is chrome specific to the browser window, so should be
somewhere in xpfe/browser, certainly. 

> These put the css file in different places in the jar.  Please fix that.
> The path inside the jar should be the same for the CSS file and the 
> images in both themes.

If you examine the current versions of jar.mn for the two themes, you will see I
am merely continuing a trend. All the toolbar stuff in the Modern theme seems to
be in the toolbar directory; but Classic doesn't have a toolbar directory. Are
you merely offended by the aesthetics, or do you think something will break?

> > Index: themes/modern/navigator/toolbar/linkToolbarOverlay.css
> 
> All the image urls in this file are broken with the current file locations.
> Once you have the jar.mn's fixed, please update the chrome:// urls in here.

Serves me right for not testing with Modern.

I fixed the other stuff - thanks.

Gerv 
> >+  <script type="application/x-javascript"
src="chrome://navigator/content/LinkToolbarHandler.js" />
> >+  <script type="application/x-javascript"
src="chrome://navigator/content/LinkToolbarItem.js" />
> >+  <script type="application/x-javascript"
src="chrome://navigator/content/LanguageDictionary.js" />
>
> The first "l" in the filename should be lowercased in all of those.
> Otherwise the scripts don't get loaded and bad things happen.

For what it's worth, the files themselves should have been left with leading
uppercase characters.  The filenames, not the URLs that should be corrected.  I
explained the convention several weeks back when someone previously commented on
them.  The filename matches the classname that's defined within that file, a
convention used by all Java programmers.  So, for example, the class
LanguageDictionary is defined in LanguageDictionary.js.
> All the toolbar stuff in the Modern theme seems to
> be in the toolbar directory; but Classic doesn't have a toolbar directory.
> Are you merely offended by the aesthetics, or do you think something will
> break?

I looked at this.  All the stuff in the toolbar dir in modern.jar is images that
are only used by and referenced by modern.  The CSS files do _not_ live in the
toolbar dir.

Your patch v3 put the link toolbar CSS file in different locations inside
modern.jar and classic.jar.  Thus different chrome:// urls would be necessary to
reference that same CSS file, which we can't do.

So as far as I can tell, you need to put the CSS in the same place in both jars.
 The images should follow the theme's convention -- inside toolbar/ in modern
and just on their own in classic.  Then have the two different CSS files refer
to the images with different chrome:// urls

> For what it's worth, the files themselves should have been left with leading
> uppercase characters.

That would be fine too.  As long as the names are consistent.  :)

Gerv, attach a new patch once you have all the chrome:// paths straightened out?






(Reporter)

Comment 185

16 years ago
When in Rome, do what the Romans do. I can't see another example of leading
uppercase in Mozilla js, so lowercase it is.

Does this next patch pass muster?

Gerv
(Reporter)

Comment 186

16 years ago
Created attachment 48902 [details] [diff] [review]
Patch v.4
Comment on attachment 48902 [details] [diff] [review]
Patch v.4

>Index: themes/modern/navigator/linkToolbarOverlay.css
>+.button-toolbar.bookmark-item[disabled="true"]:hover:active
>+{
>+  color               : GrayText;
>+  text-decoration     : none;
>+  list-style-image    : url("chrome://communicator/skin/toolbar/bookmarks/bookmark-item-dis.gif");
>+  list-style-image    : url("chrome://communicator/skin/toolbar/bookmarks/bookmark-item-dis.gif");

>+.menuitem-iconic.bookmark-item[disabled="true"]
>+{
>+  list-style-image    : url("chrome://communicator/skin/toolbar/bookmarks/bookmark-item-dis.gif");

You mean communicator/skin/bookmarks, not communicator/skin/toolbar/bookmarks,
right?
(Reporter)

Comment 188

16 years ago
Drat - I messed up the search and replace. I'll fix it after lunch.

Gerv
(Reporter)

Comment 189

16 years ago
Created attachment 48917 [details] [diff] [review]
Patch v.5
OK.  More comments on file locations:

> +  list-style-image    : 
> url("chrome://communicator/skin/bookmarks/bookmark-folder-disabled.gif");

OK.
> Index: themes/modern/communicator/bookmarks/bookmark-folder-disabled.gif

Fine.

in classic/jar.mn 
> +    skin/classic/navigator/bookmark-folder-disabled.gif
>                   (navigator/bookmark-folder-disabled.gif)

er?  That's not where that file lives according to the rest of the patch...

same for modern/jar.mn

Same for bookmark-item-disabled.gif

Please attach an updated image archive; the seem to have moved...

Also, please _do_ make the one perf change that has been proved to have a
measurable benefit:

"1.
+  for (var i = 0; i < metas.length; i++) {
should be
var length = metas.length;
for(var i = 0; i < length; i++)"

10ms off load time is a definite improvement.  Hyatt did a complete style system
rewrite to shave off about 150ms....

Other perf stuff should be handled in a bug filed immediately after this is
checked in.



(Reporter)

Comment 191

16 years ago
Created attachment 48968 [details] [diff] [review]
Patch v.6
(Reporter)

Comment 192

16 years ago
<sigh> Where's the documentation on this stuff?

I've changed what you said. I hope your review is comprehensive. I'm building
now, as well as testing doing a file copy, to see if there's anything else. But,
in the mean time, is it OK now? :-)

Gerv
OK.  I've been testing this now that it actually works.  Looks good.  One thing:

There is a themes/modern/communicator/bookmarks/bookmark-item-dis.gif
We are adding a themes/modern/communicator/bookmarks/bookmark-item-disabled.gif
The linkToolbarOverlay.css for modern only references bookmark-item-dis.gif

Is there a reason to add bookmark-item-disabled.gif?  (It's needed for classic, 
but modern seems to have it in place already.)

Sort that out and r=bzbarsky, assuming you and Tim are happy with it.

Comment 194

16 years ago
Comment on attachment 48968 [details] [diff] [review]
Patch v.6

Patch v.6 doesn't contain the optimization
var length = nodelist.length;
for(var i = 0; i < length; i++)
I'd really like to see that one in.
Once that is done, I can, yes baby, say r=fabian :)
Excellent work!
This patch can't be checked in as-is for security reasons:

As a test, gerv says that a <link> to about:cache loads. It shouldn't. Ditto for
file urls. (Although javascript:window.open("about:cache") doesn't, for some
reason. This may just be hitting js security checks which are more strict, I
guess.)

CheckLoadUri needs to be called, and quick tests with gerv on irc show that the
following untested pseudocode should work:

var ssm = Compoents.classes[scriptsecuritymanager_contractid].getService().
		QueryInterface(Components.interfaces.nsIScriptSecurityManager);
try {
	ssm.CheckLoadUriStr(sourceURL, dest, 0);
	loadURI(dest);
} catch (e) {}

mstoltz? jesse? does that sound right?
Test case: http://www.hixie.ch/tests/evil/security/001.html
One other code comment... I've been browsing with this and just ran into a
problem with:

+function getLinkElements() {
+  return getHeadElement().getElementsByTagName("link");
+}

for XML documents getHeadElement() can well return null.  Then this code throws
an exception.  So please check that the head element exists before calling its
methods.
Should we even be doing anything with documents that aren't HTML?  Afterall, a
LINK element in your XML document might not have anything to do with the HTML
LINK element.
For XML, to be safe, you'd need to be looking at localName == "link" in the
XHTML namespace. 

Comment 200

16 years ago
Will this patch bitrot with the <toolbarbutton> changes?
The copyright headers need to be changed to the MPL/GPL/LGPL triple license
before this patch can be accepted:

<http://www.mozilla.org/MPL/relicensing-faq.html>

Portions are copyright Eric Hodel.  So, Eric, do we have your permission to
relicense the code?  Please say "Yes"  :)
(Assignee)

Comment 202

16 years ago
You have my permission to relicense to MPL/GPL/LGPL.

(I prefer BSDL, but...)
(Reporter)

Comment 203

16 years ago
Mine also, obviously. If no-one else claims they contributed to this code, then
we are away.

Gerv
> for XML documents getHeadElement() can well return null

Same thing for image documents... as well as text/css or text/javascript content
which we show in the browser without creating a <head>

We _do_ have a <head> for viewsource content loaded in the main browser, but
that could change.

So please make sure that you check that the head element is non-null

Comment 205

16 years ago
I give my permission to incorporate my auto-hide patch under the tri-license.
(it doesn't seem like this will be in the initial version anyway, so I'll end up
adding it myself, but it can't hurt to give extra permission)

How about making getHeadElement() return

{getElementsByTagName: function() {return []}}

if there is no head element? :)

Comment 206

16 years ago
Do you need permission to relicense the images I made for the classic theme a 
while back? (I don't even know if you're still using them)
FWIW, Eric was the only person who copyrighted his contributed code.  I assume
it means he's the only one who needs to grant permission, though I'm certainly
unqualified to make that judgement.  The FAQ lists relicensing@mozilla.org as
the email address to contact for granting permission, so perhaps they accept
questions as well.  They'll be more qualified to answer than the rest of us.


> How about making getHeadElement() return
> 
> {getElementsByTagName: function() {return []}}

I'm not even sure what that code does.  Does it temporarily redefine
getElementsByTagName?  If that is so, it can't return an empty array, it must
return an empty nodeList.  Near as I can tell, there's nothing in the DOM API
for constructing your own nodeList.  Besides, it'd be nasty hack, which I
suspect is what you meant by the smiley emoticon.

I also noticed, in addition to checking if getHeadElement returns null,
getHeadElement should check if getElementsByTagName returns an empty nodeList
before accessing item(0), i.e.:

  function getHeadElement() {
    var nodeList = window._content.document.getElementsByTagName("head");
    return nodeList.length == 1 ? nodeList.item(0) : null;
  }

The code works without the check, but it's bad form to request an index that's
known to be out of bounds in some cases.  Also, we shouldn't be doing anything
with documents containing multiple HEAD elements, hence the explicit check for
length == 1.
(Reporter)

Comment 208

16 years ago
I'm going to wait until the toolbarbutton dust settles. Yes, the patch will need
to be fixed. Comments on that are welcome.

Could someone expand on Heikki's last comment, which I don't understand?

> I assume it means he's the only one who needs to grant permission

It doesn't mean that. People automatically have copyright on their contributions.

> Image permissions

Yes. If you could click the link to send the preformatted mail on the FAQ page,
that would he helpful.

> relicensing@mozilla.org

That would be me :-)


Gerv

> Could someone expand on Heikki's last comment, which I don't understand?

Sure. For XML, in which random nodes could be called <link>, you need to check
the following for a node in an xml doc:

1)  the namespaceURI property is the XHTML namespace URI
    (http://www.w3.org/1999/xhtml)
2)  the localName property is "link"
Last time I checked plain text and images were wrapped in HTML and therefore
have an HTML DOM tree.
> Last time I checked plain text and images were wrapped in HTML and therefore
> have an HTML DOM tree.

Yep.  And that tree has no <head> element.  The document looks like this:

<html><body><p><img/></p></body></html>

or 

<html><body><pre>text</pre></body></html>

Comment 212

16 years ago
The example just given DOES have a head element, even though it is not explicit,
it is implied.  For example, this document has a head and body element, and also
validates as HTML 4.01 strict:

<title>A document</title>
<p>hi

That's it!  document.getElementsByTagName('head') will return a valid node list
with one item.

Comment 213

16 years ago
Tim: As I understand Javascript (and I could be wrong, because I hadn't tested
that particular code) it would return an Object with only one property -
getElementsByTagName - with a value that is a function that returns the empty
list. It was taking advantage of the fact that JavaScript is more-or-less
untyped, and the only thing that ever gets called on the result of
getHeadElement is getElementsByTagName(). And you're right about the reason it
wouldn't work - I didn't realize that it returned a NodeList rather than an
array.

And yes, it would be far too hacky to actually do that in practice :)
> For example, this document has a head and body element

Jeffrey, there is a subtle difference here.  The HTML you provide goes through
the parser when we load it in Mozilla and the parser creates and inserts HEAD
and BODY nodes.  Image documents and the like are created explicitly.  There is
never any HTML source involved -- the appropriate code creates nodes by hand and
inserts them in the document.  As a result those documents do _not_ have a HEAD
node in the DOM.
Apparently, <head> is an instance of HTMLHeadElement even when it appears in an
XHTML document parsed as XML. I'm working on a version that doesn't
differentiate between HTML and XHTML but relies on instanceof HTMLHeadElement.
Created attachment 50279 [details]
linkToolbarOverlay.js that works with XHTML parsed as XML
I rewrote doRefresh() to access linkToolbarHandler.handle() directly.

This implementation works with text/html and XHTML parsed as XML. It does
minimal tree walking. (But I haven't benchmarked it.)

Thanks to jag and bz.
Created attachment 50283 [details]
linkToolbarOverlay.js that works with XHTML parsed as XML v.2
Comment on attachment 50279 [details]
linkToolbarOverlay.js that works with XHTML parsed as XML

The first version had a bug in it. :-(
Attachment #50279 - Attachment is obsolete: true
(Reporter)

Comment 220

16 years ago
Created attachment 51333 [details] [diff] [review]
Patch v.7
(Reporter)

Comment 221

16 years ago
OK, so the new patch has the menubuttons eliminated, the security problems
fixed, Heikki's XML document change, the relicensing changes, and smells minty
fresh too :-)

Looking for re-review (as far as I remember, the big changes are in
linkToolbarOverlay.js and linkToolbarOverlay.xul; there are tiny ones in
linkToolbarHandler.js).

Let's go, people, before they change XUL again!

Gerv

Comment 222

16 years ago
window.content.location.href is currently not a secure way to get the source 
URL.  A page could set a getter on window.location to make it return the object 
{'href':'chrome://navigator/content/navigator.xul'}.  I don't know what the 
right way is, or if there even is one at this point.  See bug 36946.

You should also make sure that a link pointing to a data: URL yields a page 
with the same principal as the linking page.  To check whether data urls 
inherit principal correctly, make sure
- the data: page gets a security error when it tries to access
Components.classes.
- the data: page can open a URL at the site that linked to it, and then read
data in that window.
- the data: page can't open a URL at *another* site and do the same thing.
(Reporter)

Comment 223

16 years ago
I talked this problem through with Mitch; he says that he will look into it
early next week, but that it shouldn't hold up getting it checked into the tree
(as long as it's not on the 0.9.4 branch ;-). 

He mentioned something about using XPConnect to get the Document object and
getting the current URL from there...

Review! Review! :-)

Gerv
> He mentioned something about using XPConnect to get the Document object

Nope.  No good.  All the idl-ized document interfaces we have are the DOM ones -- 
there are no scriptable internal interfaces.  This means that your only options 
for getting the URL as nsIDOMNSDocument::location.href or nsIDOMHTMLDocument::URL
The latter suffers from the problem that it's only defined for HTML documents.

The proper solution, of course, is to idl-ize nsIDocument and not expose it to 
unpriveleged scripts... But that's not happening near-term.

Gerv, I'll try to take a look at this patch this weekend (most likely tomorrow 
morning).
+  var ssm = 
+ Components.classes["@mozilla.org/scriptsecuritymanager;1"].getService().
+          QueryInterface(Components.interfaces.nsIScriptSecurityManager);

This should probably be wrapped in a try/catch... at the very least, the QI can
fail.

I got a warning for line 262 in LinkToolbarItem.js -- "Anonymous function does
not always return a value"

Other thank those, looks good to me.  Fix those and get review from the security
people.
(Reporter)

Comment 226

16 years ago
I'll put it inside the try/catch that I use the returned value in on the next
line - easy.

The security people are fine with this at the moment; I did some tests over the
phone with Mitch.

Gerv
Attachment #51333 - Flags: review+
Comment on attachment 51333 [details] [diff] [review]
Patch v.7

Gerv says he'll add 'return true;' to all the functions
that need it to quiet that warning.  With that, r=bzbarsky
One can also use

  var ifrq = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor);
  var uri = ifrq.getInterface(Components.interfaces.nsIWebNavication).currentURI;

to get at the current URI if you have full XPConnect priveleges. This is not
100% bullet proof either, but it's less likely to be wrong than using location.href
Please use the method that jst just mentioned for getting the document URL. It's
a bit safer.

Comment 230

16 years ago
Comment on attachment 51333 [details] [diff] [review]
Patch v.7

* Brackets and spacing in modern's linkToolbar.css are not using the conventions used in other files throughout the modern theme.  Please make it look like other files.

* You are setting type attribute on some menuitems (in linkToolbarItem.js and linkToolbarOverlay.xul).  This attribute is reserved for XUL, so please qualify it with another namespace (perhaps rdf?).

* The function names in linkToolbarOverlay.js are somewhat generic ("clicked", "clear", "doRefresh"), and should be qualified by putting them on an object prototype encapsulating link Toolbar functionality

* Many of the new Javascript files create fake js "classes" by adding functions to the object inside its constructor.  This should be done more efficiently using object prototypes.

* On this line:
  +    switch(document.getElementById("link-" + linkType).tagName) {
  ...you should use localName instead of tagName.
  
* The CSS styles you added for disabled bookmark-items in linkToolbarOverlay.css should be moved into bookmarksToolbar.css

* In linkToolbarOverlay.css, GrayText is not the right color for disabled text in the Modern theme.  Use #9399AB instead.

* In linkToolbarOverlay.css, you don't need to specify text-decoration or color in the rules for toolbarbutton.bookmark-item[disabled="true"][container="true"], etc, since they will pick that up from the rule above.

* Do not import chrome://global/skin/toolbar.css from linkToolbarOverlay.xul

* Your image file names aren't using the naming conventions prevalent in the Modern theme. Rename all the buttons to say -hov instead of -hover and -dis instead of -disabled.

* In the modern theme, please move all the button images into navigator/btn1.

* Rename linkToolbarOverlay.css to linkToolbar.css to be consistent with personalToolbar.css.
Attachment #51333 - Flags: needs-work+
(Reporter)

Comment 231

16 years ago
Grr. :-)

OK, I've done all of this except:

* The function names in linkToolbarOverlay.js are somewhat generic...
and
* Many of the new Javascript files create fake js "classes"..

which are beyond my JS-fu. If Hewitt requires either of these for super-review,
one of the original patch authors is going to have to step in here. The
alternative is this patch sitting here for _another_ milestone. :-(

Gerv

(Reporter)

Comment 232

16 years ago
Hewitt reformatted the relevant files (thanks, hewitt!) and says: sr=hewitt.

Checked in.

My apologies to Eric and Tim; in my rush, I forgot to credit them in my checkin
comment. However, as their names are all over the source, those in the know will
know. :-) Congratulations to all involved. Any problems with this feature should
be filed in a _separate_, short, manageable bug, probably CCing the three of us.

We have a commitment to make it hidden by default for 0.9.5; if someone can come
up with working autoshow code really quickly (in _another_ bug ;-) we might take
that, otherwise it'll just be turned off by default, which would be a shame. Who
has autoshow code? :-)

Gerv
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Reporter)

Comment 233

16 years ago
OK, so I suck even more. Thanks and well done to Chris, too - and anyone else I
may have forgotten. 

Everyone over to bug 102832!

<hides in corner.>

Gerv
Depends on: 102899
Depends on: 102886

Comment 234

16 years ago
Removing dependancy on bug 102899. This bug is fixed. A fixed bug cannot depend 
on on an open bug. Bug 102899 is an enhancement related to the functionality 
introduced by the fixing of this bug.
No longer depends on: 102886, 102899

Comment 235

16 years ago
Sorry, I got bug 102899 and bug 102832 mixed up. In any case, this bug is fixed 
can't depend on an open bug. Bug 102899 is a problem with the current 
implementation. It doesn't block this bug.

Apologies for the spam everyone. The laws of probability state that one of 
these days I'll make a useful contribution to Bugzilla!
Depends on: 102896
Depends on: 102915
Depends on: 102832
Depends on: 102895
Depends on: 102897
(Reporter)

Updated

16 years ago
No longer depends on: 102896
No longer depends on: 102895

Comment 236

16 years ago
Removing dependancies (again). Bug 103053 is now used to track Link Toolbar 
improvements.
No longer depends on: 102832, 102897, 102915

Comment 237

16 years ago
This checkin is being blamed for the large degradation in performance yesterday.
See <news://news.mozilla.org/3BBBFBD0.8CA40B45@netscape.com>. If this is really
the case, I'm going to lobby for this being backed out until the performance
ramifications are understood.

Comment 238

16 years ago
It's already been backed out and I'm working on a fix that would at least
eliminate the performance penalty when the toolbar is disabled. See bug 103082
for more details.

Comment 239

16 years ago
Now that I had a chance to look at it as a live implmentation from a UI design
perspective I find this toolbar very unsuitable to aid navigation because it
looks like part of the chrome rather then part of the page. This is really a
geek feature for now and very confusing looking to a consumer. It seems very
confusing to have two essentailly to similar looking layers of navigation and is
sure to get confused  with history (which is already over the head for most of
Netscape's customers)
I believe that a much better implimentaton would be to think about how to make
it visually part of the page rather then part of the chrome (like e.g a frame
the drops down or a sidebar panel aka sitemap).

Comment 240

16 years ago
German, I agree with your statement about the UI, but why don't we get the 
backend working and not be backing things out every three seconds 
before we worry about what percentage of the population can use it. We 
will need the same backend code in a sidebar panel or a toolbar.

Comment 241

16 years ago
German, this is exactly how a UI for 'link rel="..."' and 'link rev="..."'
should look like.
Please go back to the very start and read the basic article
<http://www.euronet.nl/~tekelenb/WWW/LINK/> if you don't understand the benefit
of a standardisized site navigation!

Comment 242

16 years ago
This bug is _DONE_. We have a UI for the <link> element, if you dislike 
the UI, then please file a new bug for that (against this component) and 
have it block http://bugzilla.mozilla.org/show_bug.cgi?id=103053
Status: RESOLVED → VERIFIED

Comment 243

16 years ago
German, an RFE has been filed to move the toolbar into the frame.

Updated

16 years ago
Blocks: 103053

Comment 244

16 years ago
*** Bug 68419 has been marked as a duplicate of this bug. ***

Comment 245

16 years ago
Shouldn't links in this form also be supported by the toolbar? 

<a rel="next" href="foo.html" title="Next document">Next</a>

ie. a link in the body rather then the head of the document.

Also the language is displayed as English if the link has hreflang="en" but not
hreflang="en-gb", which is valid for UK English.

(Reporter)

Comment 246

16 years ago
This is not the correct bug to make these comments in :-) Both of these issues
are known. The language one depends on switching over to use the decent
language-parsing code I wrote for Page Info, and the <a href=> one was (as you
will find out if you read this bug) removed for performance and semantic
reasons. I personally am in favour of putting it in, but it is a perf. hit.

Gerv
> and the <a href=> one was (as you will find out if you read this bug)
> removed for performance and semantic reasons. I personally am in favour
> of putting it in, but it is a perf. hit.

Someone could create a seperate bug for this enhancement.  At a minimum it would
be marked WONTFIX for perf reasons, providing a convienient target for the dupes
that are bound to come in.   However, bug 102992 may solve the perf problems and
make the enhancement possible, in which case we would resume the debate on
semantics in the new bug.

I'm opposed to the enhancement, so I'm only providing friendly advice...someone
else will have to create the bug if they want it ;-)

Updated

15 years ago
Component: User Interface Design → Browser-General
Product: Browser → Seamonkey

Updated

13 years ago
Alias: LinkUI
You need to log in before you can comment on or make changes to this bug.