104383 - Ability to go to specific line number in View Source window

Status: RESOLVED FIXED

(Core Graveyard :: View Source, enhancement)

Description

[Steve Chapel]

The View Source window needs the capability to go to a specific line number in
the source.

Here is how such a feature might work:

1. The user selects Edit | Go to Line #...
2. The user types a line number in a field and clicks the Go button or presses
the Enter key
3. The line is displayed and highlighted, with the window scrolled all the way
to the left side so the beginning of the line can be seen, and perhaps the
dialog is dismissed

Comment 1

[Tim Powell]

Great idea. I couldn't find another request for this, so confirming. Also see
bug 15364.

Comment 2

[Blake Ross]

doron, I'll love you if you do this.

Comment 3

[Doron Rosenberg (IBM)]

ui wise we can have a input box in the menubar. There is a
window.scrollByLines() function
(http://developer.netscape.com/evangelism/docs/technotes/xref/window-object/)
which might work.  bz even suggest if we have a view-source:url#45 to jump to
line 45.

Will look at it after sleep

Comment 4

[Doron Rosenberg (IBM)]

I have this working, we need to decide for how to do the UI (I currently have it
in the menubar).

Comment 5

[timeless]

Notepad [w2k] uses Edit>Goto which is fine by me, assuming we have no Search 
menu.

Comment 6

[Doron Rosenberg (IBM)]

i need to bring back the search menu for viewsource  since the naviagor search
remocal was futured.

can you take a screenshot of the noteapd goto dialog? win98 goto has none

Comment 7

[timeless]

Attachment: [notepad w2k Goto Dialog] it's really very very simple...

Comment 8

[Neil Marshall]

In the windows 2k notepad the shortcut for goto line is ctrl-G and when the
dialog opens up, the line number displayed in the textbox is the line number
that you are currently on.

Comment 9

[Doron Rosenberg (IBM)]

0.9.7 it is

Comment 10

[Doron Rosenberg (IBM)]

future. I have this working, but it depends on 110506 getting fully fixed by me.

Comment 11

[sairuh (rarely reading bugmail)]

mass moving open bugs pertaining to view source to [e-mail] as qa contact.

to find all bugspam pertaining to this, set your search string to
"ItsSharkeysNight".

Comment 12

[Doron Rosenberg (IBM)]

do we want to call this "jump to line" or goto?

Have this working, attaching what I have in a sec

Comment 13

[Doron Rosenberg (IBM)]

Attachment: patch which needs work

How do we want to call it? Go to or jump to line? Patch hardcodes some text
still.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> + * The Original Code is Jumo To Line Code.

Who's "Jumo" ?  :)

Why is the little jump-to-line dialog a dialog=no?  Why is it even a separate
dialog, instead of being a textfield in the viewsource window (like the typein
field in the JS console)?

Also, I thought the idea was to pass a default line number to the view-source
window and have it scroll to that line number onload... are we still planning to
do that?

Comment 15

[Tim Powell]

I've always seen this called Go to. I believe Win2K's Edit | Go To... menu 
makes sense. HomeSite 5 uses Edit | Go to Position... and gives you the option 
to indicate both the line number and the character position on the line. Of 
course it's an editor and not a viewer. One nice thing about the HomeSite 
dialog is that it gives you the valid range:

Choose a line number (1-789)
[                           ]

Comment 16

[Doron Rosenberg (IBM)]

I copied the find code :)  For the default line number, that isn't yet done as I
need to modify the js console, don't worry.

If I could figure form js the number of lines, that would be cool and solve some
issues.  I think I have a rough idea of a possible way, but need to do more
research on that.  

Comment 17

[Mike Brittain]

I would say that if there was a way to display line numbers (see bug 15364),
this functionality would not necessarily be needed.  I would say that line
numbers should be added to the source viewer before the "go to line" is added.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Doron, how does this patch deal with the "wrap long lines" mode?

Mike, getting this to work is a _lot_ easier than displaying line numbers.  We
have two proposed fixes for this bug, none for that one yet...

Comment 19

[Mike Brittain]

I don't do any development on Mozilla, so I certainly can't argue what would be
the easiest solution.

If "jump to line" is added (without a display of line numbers on the left), how
does the user identify the line once the window has scrolled to it.  Is it
supposed to be obvious that it is the line at the top of the source window
(assuming that the window scrolls to the top of the indicated line)?

Does the line become highlighted?  There's no cursor to indicate the line in the
manner that most editors might.

What happens if you "jump to" the last line of the source, or a line near the
end?  The scrollbar for the viewer only scrolls down enough for the last line to
appear at the bottom of the window.  On my screen, that leaves close to 40
lines.  If I were to "jump to" line 396 of 400 in a particular file, will I only
see the last 5 lines in the window (with blank space below)?  Or will there be
some other way to indicate the line that I was searching for?

I'm not trying to argue, I am just pointing out why I was thinking that
displaying the line numbers on the left might help make the "jump to" feature a
little more usable.

Comment 20

[Doron Rosenberg (IBM)]

I haven't had time to log into linux and check, and won't till next week
probably.  As for wrapping, it doesn't know that lines were wrapped, so it will
scroll to the nth line displayed.

As for line numbers, venkman does it, need to look at that code

Comment 21

[Tim Powell]

*** Bug 160209 has been marked as a duplicate of this bug. ***

Comment 22

[Doron Rosenberg (IBM)]

setting to 1.2alpha

btw, I am very inclined to replace the new->brower/mail/etc with just a "New
Browser" menuitem, or even removing that totally

As for wrapping, I'm using window.scrollto, so it has no idea about wrapping, so
this might cause some issues when I get a jsconsole -> viewsource to linenumber
patch (which I had, but lost).

Comment 23

[Boris Zbarsky [:bzbarsky]]

> As for wrapping, I'm using window.scrollto, so it has no idea about wrapping

Then maybe it's the wrong thing to use?

If the line numbers in view source are going to have no resemblance to the line
numbers in the JS console, what's the point of this patch?

Comment 24

[Doron Rosenberg (IBM)]

I could always turn off wrapping :)

reseting, as this is a valid concern.

Comment 25

[Doron Rosenberg (IBM)]

so, would turning off wrapping if the caller is js console be evil?

Comment 26

[Boris Zbarsky [:bzbarsky]]

Somewhat, yes.  There's also the approach taken in bug 79612.  We _could_ try to
minimize the speed hit there...

Comment 27

[Doron Rosenberg (IBM)]

it's not worth it for this feature to hack c++

Comment 28

[Boris Zbarsky [:bzbarsky]]

That's a matter of your opinion, not fact.... I think it's perfectly worth it.

Comment 29

[Kai Lahmann (is there, where MNG is)]

current patch doesn't select that line, but it's a start

Comment 30

[Roger Perttu]

Now using Mozilla 1.2

Javascript Console gives me this (just an example):
Error: xxx is not defined
Source File: http://localhost/file.asp      Line: 86

I can click the link and the "Source of" window opens the file at the top.

IMHO it would be a great start if the "Source of" window had the ability to move
to line x and then highlight it (is there a bug for that). Leave the Goto
Menu/Dialog stuff for later.

(Ctrl-A, Ctrl-C, "open editor", Ctrl-V, Ctrl-G, is not fun)

Comment 31

[Boris Zbarsky [:bzbarsky]]

> is there a bug for that

Yes.  This is the bug for that.

Comment 32

[R.K.Aa.]

*** Bug 185572 has been marked as a duplicate of this bug. ***

Comment 33

[Turbo]

... lot of bugs were marked as duplicate of this one, but there is NO RESULT. There is still no "GoToLine" or "JumToLine" in ViewSource ... so is there finally a possibility to add thes SIMPLY feature?

Comment 34

[Boris Zbarsky [:bzbarsky]]

If it's so simple, add it.  Maybe it's not as simple as you think, eh?

Comment 35

[Turbo]

It was totally useless comentary because Mozilla-team solve this problem
since Mozilla 0.9 and now we have 1.3a ...

Comment 36

[Boris Zbarsky [:bzbarsky]]

Where exactly have we solved this problem?

Comment 37

[Doron Rosenberg (IBM)]

future. to make this really work, we need line numbers, which would mean slowing
down viewsource.  Might have an idea how to speed viewsource a bit to hide the
perf hit of line numbers, but I don't have time now to do that.

Comment 38

[Christian Schmidt]

Attachment: Patch that implements highlighting of a specified line - also works with line wrapping

This patch finds the specified line the hard way by counting newlines.

When the line (e.g. line 34) is found, it is enclosed in a <span
class="selected-line" id="line-34"> element. If necessary, span elements that
span several lines including the specified are split into two.

The line can be styled by inserting some suitable CSS in /res/viewsource.css,
e.g.

.selected-line {
 border: 1px dotted black;
}

Currently it only searches for \n. I am not sure if \r should be handled as a
newline as well.

When syntax highlighting is on, the performance isn't impressing. With syntax
highlighting turned on, however, there is no noticeable delay.

The UI is still pretty rudimentary. It still needs localization and some kind
of feedback when the line number entered is out of range. Currently the line
number is entered in a regular JS prompt(). Personally I prefer a popup dialog,
but another possibility is a fixed textfield in the viewsource window as
suggested in comment 14.

Comments?

Comment 39

[Boris Zbarsky [:bzbarsky]]

> I am not sure if \r should be handled as a newline as well.

\r is not a valid character in the DOM; there will never be \r in #text nodes
unless someone sticks it in there via DOM methods (as in, the parser will never
produce such text nodes).

Would it possibly be faster to select all the #text nodes in the document with a
treewalker and then stop iterating when you hit the n-th newline?

Also, what sort of speed are we talking about here in the highlighted case? 
Would we be better off just using the patch in bug 79612 (perf hit opening view
source every time) or the patch in this bug (per hit on every traversal to a
line)?  (This is a question about what we think the primary usage patter of such
a function would be, really...)

Thanks for picking this one up, Christian!

Comment 40

[Christian Schmidt]

Attachment: Updated patch with more optimizations

>Would it possibly be faster to select all the #text nodes in the document with

>a treewalker and then stop iterating when you hit the n-th newline?

I doesn't seem so. Infact is seems considerably (> +50%) slower. And it doesn't
make the code easier to read either, because we still need to know which pre
and span (if any) each text node is in (disclaimer: I have never worked with
the treewalker before so I might have used it in a less-than-optimal way).


>Also, what sort of speed are we talking about here in the highlighted case? 

I have changed the code so it now uses offsetHeight-calculations to count the
number of lines when word wrap is off. Also the number of lines in each pre
element is now cached, so when the user jumps to a line for the second time it
is usually much faster.

The following results show approximately how long time it takes to jump to line
2000 of a document on my old Pentium Celeron 300 MHz:

word wrap OFF, syntax highlighting OFF	     < 1 sec
word wrap OFF, syntax highlighting ON	     < 1 sec
word wrap ON, syntax highlighting OFF	     ~ 3 sec
word wrap ON, syntax highlighting ON	     ~15 sec


>Would we be better off just using the patch in bug 79612 (perf hit opening
view
>source every time) or the patch in this bug (per hit on every traversal to a
>line)?  (This is a question about what we think the primary usage patter of
>such a function would be, really...)

Personally I'd use the go-to-line feature a lot, but I usually have word
wrapping turned off, so I would probably prefer the patch in this bug. Judging
from the number of votes and duplicates, this feature (surprisingly) doesn't
seem that popular.

However, I think the best solution would be a combination of the two patches.
If the patch in bug 79612 is changed, so it doesn't insert nodes for every line
but just one for every, say, 50 lines (perhaps it could just add an id
attribute on each pre element indicating the line number of the last line in
the pre), then the perf hit on window open probably wouldn't be that big. The
patch in this bug can then find and highlight the exact line by counting
newlines from the nearest of these target nodes. Unfortunately I don't really
hack C++, so I can't change the patch to bug 79612 myself.

Comment 41

[Boris Zbarsky [:bzbarsky]]

> (perhaps it could just add an id attribute on each pre element indicating the
> line number of the last line in the pre)

That is an interesting thought, now...  Do you build Mozilla, by any chance?  If
I changed the patch in bug 79612 to do that, would you be able to test?

Comment 42

[Christian Schmidt]

Yes, I build (on Linux). I tried applying the existing patch, but it seems to
have bitrotted. But if you update and change it, I will try again and work from
there.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated patch....

This adds an id to all but the first "pre" element (I could add it to the first
one too, if you really want, of course).  The id has the form "lineXXX" where
XXX is the line number of the first line in that pre.  So:

<pre>
Text
</pre>
<pre id="line2">
text2
text3
</pre>
<pre id="line4">
...

I've not done any perf testing on how long it takes to open viewsource with or
without this patch; if someone has the time to do that, it would be much
appreciated.  I _think_ this one shouldn't really affect perf much...

Comment 44

[Boris Zbarsky [:bzbarsky]]

Oh, and I did first line instead of last, because it's a _lot_ easier to do in
the current setup... if having the last line number would be much much better,
I'll try to look into doing that instead...

Comment 45

[Christian Schmidt]

Attachment: Patch v 1.2

It now highlights the line in < 1 sec, independently of word wrapping and
syntax highlighting.

>Oh, and I did first line instead of last, because it's a _lot_ easier to do
That's fine with me.

Now, I am not sure how to move this further (this is my first real Mozilla
hacking):

- is regular JS prompt() and alert() calls the way to go, should I create a XUL
dialog for entering the line number, or should the UI be totally different?

- element.scrollIntoView() doesn't do what I expect (I can't find any
authorative information about how it is supposed to react). When called on an
span element containing a lot of lines down in a pre element, only the top of
the pre element is scrolled into view, leaving the span element way below the
viewport. This is particularly a problem when viewing source of documents
containing a lot of inline Javascript, because a <script> block appears not to
be split into seperate pre elements. Is this a bug in scrollIntoView()?

Comment 46

[Christian :Biesinger (don't email me, ping me on IRC)]

> is regular JS prompt() and alert() calls the way to go

Not really, you should use nsIPromptService. see
http://lxr.mozilla.org/seamonkey/source/embedding/components/windowwatcher/public/nsIPromptService.idl

Comment 47

[Boris Zbarsky [:bzbarsky]]

> element.scrollIntoView() doesn't do what I expect

One possibility here is to use <a id="line-xxx" name="line-xxx"> </a> to enclose
the line, to scroll to it by calling loadURI("#line-xxx") on the content
webnavigation, and to style it by using the :target selector.... 

That said, is the problem that scrollIntoView is just scrolling to the top of
the parent span that the span you added is in or something?

Regular prompt() or alert() should not be called from chrome.  You could use the
ones on nsIWindowWatcher instead....

I think just having a line number toolbar thingy would be best, myself.... but
I'm not a good person to ask about UI.  In any case, it's be good to get the
code itself working and landed; that can happen even without real UI for it
(note that it would be trivial to at least make links from the JS console scroll
to the right line, per patch in bug 79612).

Two more questions:

1) Why the relative positioning instead of just using a negative margin?

2) What page are you using as your testcase for how long the "go to line" takes?
 Could you possibly test it on http://www.w3.org/TR/DOM-Level-2-HTML/html.html
just so we have a baseline for a fairly large heterogeneous page?

Comment 48

[Boris Zbarsky [:bzbarsky]]

Er, biesi is right.  nsIPromptService, not nsIWindowWatcher.

I just thought of something else.  Sicking, could the splitting and such done in
this patch be more easily done using surroundContents on a range or something?

Comment 49

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

it looks like you should be able to just set the start and endpoint of a range
and then use the range to do all the splitting and cloning. However you will not
be able to call .surroundContents because that doesn't like splitting
non-textnodes (i.e splitting a parent <span>). This is a very stupid limitation
in .surroundContents and I poked them about it before the spec went into
recommendation, but aparently it is by design (don't ask me why though, it
doesn't make sense to me)

http://www.w3.org/TR/DOM-Level-2-Traversal-Range/ranges.html#Level-2-Range-Surrounding

However what you still can do is to use the range and call
selectedLine.appendChild(range.extractContents());
range.insertNode(selectedLine);

That should split any spans as necessary.

Comment 50

[Christian Schmidt]

Attachment: Patch v. 1.3

>> element.scrollIntoView() doesn't do what I expect
>
>That said, is the problem that scrollIntoView is just scrolling to the top of
>the parent span that the span you added is in or something?

It scrolls to the top of the parent pre. This is a problem, because a pre
containing Javascript can easily contain more than a screenful of lines.


>One possibility here is to use <a id="line-xxx" name="line-xxx">

This appears to behave exactly like scrollIntoView().

I found out that scrollIntoView() is actually broken for dynamically created
elements (bug 186149), and this is probably also affecting <a id="line-xxx"
name="line-xxx">. However, using setTimeout('element.scrollIntoView()', 1)
appears to be working for now.


>1) Why the relative positioning instead of just using a negative margin?

I was tired :-) Now fixed.


>2) What page are you using as your testcase for how long the "go to line"
>takes? Could you possibly test it on
>http://www.w3.org/TR/DOM-Level-2-HTML/html.html

It works in < 1 sec even on a long page like this. However, you have to wait
for the line to display before searching (not the whole document, just the line
your are looking for). Especially with syntax highlighting turned on, this may
take quite a while. But once it is loaded, "go to line" works "instantly".


>However what you still can do is to use the range and call
>selectedLine.appendChild(range.extractContents());
>range.insertNode(selectedLine);

Wow, that definately made the code a lot simpler. Also, it now makes sense to
use a treewalker to find the text nodes, making the code even more simple.
However, due to bug 135922 I cannot use range.insertNode(), so instead I use a
slightly more complicated workaround.


>Not really, you should use nsIPromptService.

Done. Also, the strings are now localizable.


>I think just having a line number toolbar thingy would be best, myself.... but

>I'm not a good person to ask about UI.  In any case, it's be good to get the
>code itself working and landed; that can happen even without real UI for it

This patch uses the dialog approach. I can easily change it to a toolbar thingy
or completely remove the UI (who decides this?), but at least for testing some
kind of UI is probably necessary.


>(note that it would be trivial to at least make links from the JS console
>scroll to the right line, per patch in bug 79612).

It's not completely trivial (for me, at least :-), because the "go to line"
function has to wait until the line has been displayed (loaded?). That could
probably be done using window.setInterval(), but I don't know how to detect
when the page has finished displaying (I cannot find an appropriate onload
event).

Comment 51

[Boris Zbarsky [:bzbarsky]]

Assigning to Christian, since he's doing all the work... ;)

> I found out that scrollIntoView() is actually broken for dynamically created
> elements (bug 186149), and this is probably also affecting <a id="line-xxx"
> name="line-xxx">

Yep.  Try the patch in that bug?  If that makes the <a name="xxx"> approach
work, I'd sorta like to go with it, since if/when we implement smart scrolling
behavior on resize we'd be more likely to keep the right place...  That, and it
makes the code simpler again -- no need for keeping track of the class.  If one
approach works and the other does not, of course, we use the one that works.  ;)

> However, due to bug 135922 I cannot use range.insertNode()

Try the patch in that bug too?  I've always loved how the use of DOM in our UI
forces our DOM impl to suck less.... ;)

I hope that all the methods involved keep the selected line properly
highlighted, btw.... (I'm not too familiar with ranges, as you can tell.... ;)).
 In particular, using the "pre" as the thing to append to instead of using the
range start container in the case when the container is not a #text node looks
odd... (but I've not really put much thought about what the range looks like at
that point yet).

> but I don't know how to detect when the page has finished displaying

Hm...  You _may_ be able to attach an onload event to _content.  That has to be
done after you start the load, however...  Perhaps a better way is to, before
you start the load,
getBrowser().webNavigation.GetInterface(Components.interfaces.nsIWebProgress),
then use a small nsIWebProgressListener impl to watch for the STATE_STOP state
change?  The set an interval and keep trying to scroll and cancel when you
manage to scroll or the stop event comes in....

I haven't really done a thorough nitpicking review yet, since this is sort of
in-progress so far.. but there are some places where things don't really line
up; dunno if those are tabs or just bad whitespacing.

This is looking really good, btw.  Thanks a ton for working on it.

Comment 52

[Christian Schmidt]

>> I found out that scrollIntoView() is actually broken for dynamically created
>> elements (bug 186149), [...]
>
>Yep.  Try the patch in that bug?

>> However, due to bug 135922 I cannot use range.insertNode()
>
>Try the patch in that bug too?  I've always loved how the use of DOM in our UI
>forces our DOM impl to suck less.... ;)

Boris, you rock! Both your patches work perfectly and made the patch for this
bug even cleaner.


>Hm...  You _may_ be able to attach an onload event to _content.  That has to be
>done after you start the load, however...  Perhaps a better way is to, before
>you start the load,
>getBrowser().webNavigation.GetInterface(Components.interfaces.nsIWebProgress),
>then use a small nsIWebProgressListener impl to watch for the STATE_STOP state
>change?  The set an interval and keep trying to scroll and cancel when you
>manage to scroll or the stop event comes in....

Appearently STATE_STOP is reached way before the whole document is displayed and
accessible via the DOM API. If I understand nsViewSourceHTML.cpp correctly it
just adds elements to an existing document, so waiting for onload/STATE_STOP
wont work.

Is it possible for nsViewSourceHTML.cpp to trigger an event, or perhaps close
the element with a magic element, e.g. <span id="the-end">? Or is there another
way to detect when nsViewSourceHTML.cpp is done? Or did I miss something about
nsIWebProgressListener?

Comment 53

[Boris Zbarsky [:bzbarsky]]

OK, maybe we should just do this the stupid way.  ;)  Sometime before you
actually start the load (say at the top of onLoadViewSource() or viewSource(), do:

getBrowser().addEventListener("load", foo, true);

and add

function foo (event) {
  // we are loaded
}

(with a better function name and all).

Just make sure to use a capturing listener, since load events don't bubble...
(and yes, I should have thought of this before suggesting the other stuff; I've
been working on necko/docshell too much lately.... ;) )

Comment 54

[Christian Schmidt]

Attachment: Patch v. 1.4

Now I managed to get hold of the onload event (using
getBrowser().addEventListener).  Included in the patch is now also a change to
the JS console that makes the links to the view source window also go to the
line in question.  The line number is supplied via window.arguments as
recommended in bug 79612. The argument must be of type "number" to work.

I changed window._content.document.getElementById('viewsource') to
window._content.document.body, because when view source is called without a
page descripter (e.g. from the JS console) the body element for some reason
does not have id="viewsource".	I don't know why this is the case, but using
document.body is probably just as good as using
document.getElementById("viewsource").

I jump to the line using webNavigation.loadURI(), but I looks as if I have to
supply an absolute URL instead of just |#line-1234|.  I don't know a way to
find this absolute URL except by doing simple string searches and
concatenations (I tried getBrowser().webNavigation.currentURI.resolve(...) and
ioService.newURI(...) without luck).

Comment 55

[Boris Zbarsky [:bzbarsky]]

Gack.  You're right; loadURI does not take an anchor name.... I should consider
fixing that or adding a scrollToAnchor on that API....

Anyway, that works for now.  I'm not sure why there is no "viewsource" id --
nsViewSourceHTML.cpp always adds it to the <body>...

Please give global variables names like gLineFound?

If you use the :target pseudo-class instead of a class, you shouldn't need to
keep track of the selected line globally, right?

This is looking really excellent. ;)

Comment 56

[Christian Schmidt]

Attachment: Patch v. 1.5

I now use the :target selector, so selectedLine no longer have to be global.

The global variables have been prefixed with |g|.

Comment 57

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 120593 [details] [diff] [review]
Updated patch....

rbs, would you sr the back end part of this?

Comment 58

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 121923 [details] [diff] [review]
Patch v. 1.5

+    textNodes: for (var textNode = treewalker.firstChild();
+	  textNode != null;
+	  textNode = treewalker.nextNode()) {	 

Indent the sub-clauses of the if to line up?

No need for a new patch with that change till after Neil's review.  sr=me, and
looking forward to having this in the tree.  ;)

Comment 59

[rbs]

Comment on attachment 121923 [details] [diff] [review]
Patch v. 1.5

+  var selectedLine = window._content.document.getElementById('line-' + line);


always null here due to 'line-'

Comment 60

[Boris Zbarsky [:bzbarsky]]

rbs, that's not looking for the pre blocks.  See

+          selectedLine.id = 'line-' + line;

Comment 61

[rbs]

Might be best to settle on a uniform nomenclature then. It is confusion otherwise.
Other comments comming up...

Comment 62

[rbs]

Comment on attachment 121923 [details] [diff] [review]
Patch v. 1.5

+      if (typeof(arg) == "number" && !isNaN(arg)) {
+	 window.setTimeout(waitForLine, 200, arg);
+	 getBrowser().addEventListener("load", function() { contentLoaded =
true; }, true);
+      }
+    }
[...]
+function waitForLine(line) {
+  var found = goToLine(line);
+  //
+  // If the line was not found and the page has not finished 
+  // loading, then try again after a short while.
+  //
+  if (!found && !contentLoaded) {
+    window.setTimeout(waitForLine, 200, line);
+  }
+}

Do you need all these genuflections?

BTW, I experienced similar troubles with View Selection Source where 
I had to wait until the DOM is built before highligthing the selected
area. (It is until you set out to do these stuff that difficulties
show up :-) It would have save you some hassle to look over there.

Your code can be simplified in these ways:

1) to wait until the view-source DOM is there, you can for
example do the following (i.e., no need for setTimeout):
   disable_the_goto_line_menu_item_by_default();
   window.document.getElementById("appcontent")
	 .addEventListener("load", waitForLine, true);

function waitForLine() {
  removeListener(waitForLine) ...
  enable_the_goto_line_menu_item();
}


2) no much point for CSS rules with :target. Just re-use the selection
infrastructure to select the line (like in View Selection Source).

function gotoLine() {
  // get the selection controller
  var contentWindow = getBrowser().contentDocument.defaultView;
  var selection = contentWindow.getSelection();

  // figure out the range of interest, say |myRange|
  // (it is possible to avoid modifying the view-source DOM by
  // defining the parameters of range appropriately)
  myRange = ...
  if (invalid)
    return, of course

  selection.removeAllRanges();
  selection.addRange(myRange); // this will automatically scroll
			       // the selection/line into view...
			       // (i.e. no need to re-load)

  // =======
  // FYI, the default behavior of the selection is to scroll at the
  // end of the selection. For wrapped lines, to ensure that the
  // selection is scrolled at the beginning, you might want to override
  // the default behavior with this little voodoo: (this is what
  // View Selection Source does, BTW)
  try {
    getBrowser().docShell
   .QueryInterface(Components.interfaces.nsIInterfaceRequestor)
   .getInterface(Components.interfaces.nsISelectionDisplay)
   .QueryInterface(Components.interfaces.nsISelectionController)
   .scrollSelectionIntoView(
     Components.interfaces.nsISelectionController.SELECTION_NORMAL,
     Components.interfaces.nsISelectionController.SELECTION_ANCHOR_REGION,
     true);
  }
  catch(e) { }
}

Comment 63

[rbs]

Comment on attachment 120593 [details] [diff] [review]
Updated patch....

r+sr=rbs. (With the freeze, I wouldn't have bothered doing the cleanup since it
makes the patch unnecessarily big when requesting the a=)

Comment 64

[Christian Schmidt]

>1) to wait until the view-source DOM is there, you can for
>example do the following (i.e., no need for setTimeout):

The thing is that the line may be accessible before the whole document has
loaded and the load event occurs.  setTimeout allows us to highlight the line
almost as soon as it has been loaded.

Thanks for the review. An updated patch coming up later today.

Comment 65

[[e-mail]]

Comment on attachment 121923 [details] [diff] [review]
Patch v. 1.5

>+        getBrowser().addEventListener("load", function() { gContentLoaded = true; }, true);
I agree that you should be trying to find the line here.

>+  var viewSourceBundle = document.getElementById('viewSourceBundle');
Pity you forgot to include the bundle in the XUL...

>+  for (;;) {
I'm doubtful you want this in a loop...

>+    if (isNaN(line)) {
>+      promptService.alert(window,
>+          viewSourceBundle.getString("invalidInputTitle"),
>+          viewSourceBundle.getString("invalidInputText"));
>+    } else {
>+      break;
>+    }
if (!isNan(line))
  break;

promptService.alert(...);

IMHO looks neater. Actually, you might want to check for
if (line > 0)
  break;

>+  if (selectedLine == null) {
Do we do this or !selectedLine? (Can't remember "house" style)

>+    for (var lbound = 0, ubound = viewsource.childNodes.length - 1; ; ) {
>+      var middle = lbound + Math.ceil((ubound - lbound) / 2);
var middle = Math.ceil((ubound + lbound) / 2);

Aside: I prefer to have ubound start at .length and adjust the code accordingly
e.g. middle = (ubound + lbound) >> 1

>+    textNodes:
A label! Is this to be the first JS label in Mozilla? (OK I haven't searched
the rest of the code).

>+      // Count newlines;
Try splitting the data into lines, that gives you an instant count.
i.e. var lineArray = textNode.data.split(/\n/);
(you might need to adjust for a trailing newline though)

>+          selectedLine = window._content.document.createElement('SPAN');
Interesting this. When word wrap is on the border looks awful. I tried using
-moz-outline which outlines each row of the wrapped line. I also tried using a
DIV instead of a SPAN, which forces a 100% width block which looks good for
short and wrapped lines, but not for long unwrapped lines (i.e. more than 100%
width). You could of course try
pre[wrap] :target { display: block; } (or whatever).
Or you could forget the border and put both a DIV and a SPAN in...

>+  if (selectedLine != null) {
>+    selectedLine.className = 'selected-line';
A leftover from some code that you might need to resurrect?

>+    getBrowser().webNavigation.loadURI(viewSrcUrl, loadFlags, null, null, null);
Note that getBrowser() also has a .loadURI method which passes some default
values to .webNavigation.loadURI

>+<!ENTITY goToLineCmd.label "Go to line ...">
No space before the ...

Comment 66

[Christian Schmidt]

neil, thanks for the review.  I have a few clarifying questions:

>>+   getBrowser().addEventListener("load", function() { gContentLoaded = true;
}, true);
>I agree that you should be trying to find the line here.

I am not sure I understand you correctly. Do you suggest that I should wait for
the whole DOM to load before looking for the line? This will make looking for a
line in the beginning of the document take significantly longer - especially
when syntax highlighting is on.


>>+  for (;;) {
>I'm doubtful you want this in a loop...

If I get you right, you think that I should just show the alert(invalidInput)
and then terminate without prompting for a line again? Notepad, UltraEdit and
others keep prompting for a line until a valid integer has been entered.


>Interesting this. When word wrap is on the border looks awful.

I will probably just use a selection as suggested by rbs to be in sync with View
Selection Source (if people agreee).

Comment 67

[Boris Zbarsky [:bzbarsky]]

Yeah, using the selection sounds fine.

Comment 68

[rbs]

re: comment #64
> >1) to wait until the view-source DOM is there, you can for
> >example do the following (i.e., no need for setTimeout):
>
> The thing is that the line may be accessible before the whole document has
> loaded and the load event occurs.  setTimeout allows us to highlight the line
> almost as soon as it has been loaded.

With the selection approach, you wouldn't be using a view-source(url#line)
anymore. Hence there is no (re)load. Rather, the gotoLine kicks in after the
user mouses clumsily to Edit -> Goto Line -> input. By then, a rather long time
would have elapsed and the single load (first time) would have completed in most
cases.

[In an earlier implementation of View Selection Source, it used a setTimeout. It
was not worth the trouble. Plus the sr didn't like it. Also, optionally
disabling the menu item (if you deem necessary) provides a safeguard until that
first load is done. As you experiment with the selection, you will get a clearer
picture of where things stand.]

Comment 69

[Boris Zbarsky [:bzbarsky]]

> Rather, the gotoLine kicks in after the user mouses clumsily to Edit -> Goto
> Line -> input.

Except in the common case of source being opened from the JS console.  Then the
gotoLine() is called programmatically.  We could indeed delay that till the
onload, but if there is a JS error on line 143 of a 5000-line HTML file, it'd be
good to scroll to it asap after the user clicks the link in the JS console.

Comment 70

[rbs]

Adding dependency to bug 79612 which I hadn't read before and which is the
scenario that you describe. So, the fix is intended to subsume two cases:

Normal:     view-source(url)  +  Edit -> Goto line -> input
JS Console: view-source(url#line), jump to line.

====
From my recollection, I couldn't get a meaningful document pointer from the
script until the document onload was fired (paint supression/zombie, perhaps).
There is a difference between:

   /* this one tells that the chrome is ready */
+ getBrowser().addEventListener("load", function() { gContentLoaded = true; },
true);

   /* this one tells that the content document is ready, same as <body onload> */
   window.document.getElementById("appcontent")
	 .addEventListener("load", waitForLine, true);

Are you really observing a jump to the line asap with the other one, while the
document is still sluggishly loading?

Comment 71

[Boris Zbarsky [:bzbarsky]]

> There is a difference between:

There is?  The first sets a capturing load listener on the XUL <browser> node. 
The second sets a load listener on the parent of said node.  Both will fire at
the same point -- when the onload() of the document loading withing the browser
fires.

I'm assuming that Christian tested the scrolling to line while the document is
loading, of course.

Comment 72

[rbs]

> There is? 

Indeed. It took me a while to discover & get what I wanted because of that
difference.

In fact, come to think of it, if there wasn't a difference the setTimeout would
be meaningless. Since <body onload> would mean that the DOM is there, and so
gotoLine will succeed. Whereas <chrome onload> means that the chrome is done and
is about to load the document, and so attaching the waitLine for the document
means something.

This brings to what I was most wondering about: whether what is exposed to the
script is indeed a meaningful at that stage, which is why I am curious to know
how the testing goes.

Comment 73

[Christian Schmidt]

> This brings to what I was most wondering about: whether what is exposed to the
> script is indeed a meaningful at that stage, which is why I am curious to know
> how the testing goes.

I do not know whether the DOM exposed to the script is meaningful during load,
but if the text nodes are added sequentially in the order they will appear in
the tree, then I assume that if the script thinks it has found the line then it
in fact is the right line (because text nodes cannot be inserted before it in
the tree). But I am just guessing.

Judging by my testing it looks as if the setTimeout version always (?) succeeds
in finding the line, but sometimes the scrollSelectionIntoView fails, i.e. the
line is highlighted but the window does not scroll at all. Perhaps the text is
somehow present in the DOM a short while before it is displayed on the screen.

If people prefer the conservative approach that waits for the whole document to
load before looking for the line, I will use that.

Comment 74

[rbs]

> the setTimeout version always (?) succeeds in finding the line

Indeed, since you are looping. But it could be that the loop is succeeding at
the very end, i.e., when the document has already loaded anyway.

Another thing to double-check is that the line that is seemingly found may
actually belong to the old document (the so-called "zombie" document). I am a
bit cautious in this muddy area because it took me a while to get out of it.

Once you are happy with a reliable version, feel free to show us.

Comment 75

[Christian Schmidt]

Attachment: Patch v. 1.6

This patch waits for the load event before looking for the line. This appears
to work in all cases contrary to the setTimeout solution that sometimes failed
to scroll the line into view.

The specified line is highlighted using a selection. If the line happens to be
blank, the selection is invisible. This might change if the linefeed characters
are made visible in a selection as proposed in bug 156175. I don't know if we
need a workaround for this, e.g. reverting to highlighting via CSS, but since
empty lines in general aren't of much interest this probably isn't a big issue.


It looks like there is a problem with scrollSelectionIntoView being off by one
line, so I filed bug 205006.

Comment 76

[rbs]

Comment on attachment 122940 [details] [diff] [review]
Patch v. 1.6

Looking good to me, just a few sanity check comments:

+  //	 arg[3] - Line number to go to.
   //
   if ("arguments" in window) {
     var arg;
@@ -78,6 +104,16 @@
       }
     }
     //
+    // Wait for the specified line to load.
+    //
+    if (window.arguments.length >= 4) {
+      arg = window.arguments[3];
+
+      if (typeof(arg) == "number" && arg > 0) {
+	 gGoToLine = arg;
+      }
+    }
+    //

This is for bug 79612, view-source(url#line), right? I don't see the parsing of
#line. Needs to hook properly now?

+  getBrowser().addEventListener(
+      "load",
+      function() {
+	 if (gGoToLine) {
+	   goToLine(gGoToLine);
+	   gGoToLine = false;
+	 }

You mean |if (gGoToLine > 0)| and |gGoToLine = -1| since it isn't a boolean.

Comment 77

[Christian Schmidt]

> This is for bug 79612, view-source(url#line), right? I don't see the parsing
> of #line.

It is indeed a hook for the JS console, but the line number is sent from the JS
console (consoleBindings.xml) via window.arguments instead of url#line (as
suggested in bug 79612), i.e. no parsing needs to be done:

window.openDialog(
       "chrome://navigator/content/viewSource.xul", "_blank", 
        "scrollbars,resizable,chrome,dialog=no", url, null, null, line);

Comment 78

[rbs]

Attachment: v1.7 - correct remaining issues from using the selection

I applied the patch for experimentation and could fix/work-around the remaining
issues:

1) If the selected line happens to be blank, the selection is invisible.
-> show a blinking caret on the line...

2) scrollSelectionIntoView off by one line
-> use a "hint" to orientate the selection. This one was subtle because range
indices are delicate. With your code, the selection start's offset is after the
"\n" at the end of the previous line. By using an interline hint ("hintright"),
I forced the needed line to come into view.

Also,
-> moved the bundle to the viewsource overlay
-> placed the Goto line menu item before the Finds. It looked akward to be
in-between them. (I guess some UI people might comment later on the wordings
anyway.)

Back to you. Good job. It works great and the ice on the cake is that it kills
bug 79612, as well as CSS errors (when the line number is given properly).

Re: bz, you might want to sync the line's id in DUMP_TO_FILE. Also, it looked
like you could have set id="line1" on the first <pre> to save the special-case
in the JS.

Comment 79

[Boris Zbarsky [:bzbarsky]]

Yeah, I should sync the DUMP_TO_FILE stuff, and it looks like I need to add some
IF_FREE() calls as well (the body token is the only one we do it for now)... 
those are not real leaks, since it's an arena, but still....

Oh, and yes; I'll add an id to the first <pre>.

Comment 80

[rbs]

Want to drive things in, then? It is low risk with high reward. Click on a
warning/error in the JavaScript console and it jumps to the precise line in the
source and highlights it... JavaScript'ers in particular will like this a lot.

Comment 81

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch for back end updated to comment 78

Comment 82

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 123360 [details] [diff] [review]
Patch for back end updated to comment 78

I was wrong about IF_FREE.  Parser nodes free their tokens, so we should NOT,
in fact, be freeing them.  

This is the same as the previous patch except I added id="line1" to the first
<pre> and fixed the DUMP_TO_FILE sections.

I agree that it would be great to take this for 1.4, drivers willing (I agree
that this is quite low-risk).  What we need is r= on the front end section from
a front-end person (eg Neil).

Comment 83

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 123251 [details] [diff] [review]
v1.7 - correct remaining issues from using the selection

sr=me on this (modulo whatever you guys decide to do with that first pre).

Neil, would you review?

Comment 84

[rbs]

Comment on attachment 123360 [details] [diff] [review]
Patch for back end updated to comment 78

r+sr=rbs on this updated back-end.

Comment 85

[[e-mail]]

Comment on attachment 123251 [details] [diff] [review]
v1.7 - correct remaining issues from using the selection

>+        locale/en-US/navigator/viewSource.properties        (resources/locale/en-US/viewSource.properties)
Sadly the file itself is missing from the patch :-/

>+  //
>+  // Disable "go to line" while reloading due to e.g. change of charset
>+  // or toggling of syntax highlighting.
>+  //
>+  getBrowser().addEventListener(
>+      "unload",
>+      function() {
>+        document.getElementById('cmd_goToLine').setAttribute('disabled', 'true');
>+      },
>+      true);
>+
>+  getBrowser().addEventListener(
>+      "load",
>+      function() {
>+        if (gGoToLine) {
>+          goToLine(gGoToLine);
>+          gGoToLine = 0;
>+        }
>+        document.getElementById('cmd_goToLine').removeAttribute('disabled');
>+      },
>+      true);
I'm not a fan of big inline functions...

>+      arg = window.arguments[3];
>+
>+      if (typeof(arg) == "number" && arg > 0) {
>+        gGoToLine = arg;
>+      }
Probably neater to do the parseInt here instead of in consoleBindings.xml i.e.
arg = parseInt(window.arguments[3]);
if (arg > 0) {
  gGoToLine = arg;
}
Actaully, gGoToLine = parseInt(window.arguments[3]); would work becuause your
load handler already checks for > 0.

>+    promptService.alert(window,
>+        viewSourceBundle.getString("invalidInputTitle"),
>+        viewSourceBundle.getString("invalidInputText"));
The subtle difference is that Notepad et al don't close the prompt for the
input while displaying the invalid input alert... in fact, Notepad doesn't
close it for the out of range alert either, so I think you should be consistent
too, whichever way around, either always prompt after an error or never prompt
after an error.

>+  gLastLineFound = line;
>+  var found = goToLine(line);
You haven't found the line yet... please set gLastLineFound in goToLine()
instead.

>+      if (curLine == line) {// && !range) {
Still setting the end of the range every time?

>+        if (!range) {
>+          range = document.createRange();
>+          range.setStart(textNode, curPos);
>+        }
>+
>+        //
>+        // This will always be overridden later, except when we look for
>+        // the very last line in the file (this is the only line that does 
>+        // not end with \n).
>+        //
>+        range.setEndAfter(pre.lastChild);

>+  var selection = window._content.getSelection();
>+  selection.removeAllRanges();
I think you should use
if (!range)
  return false;
before here (also change the last return to return true;).

>+          var url = this.getAttribute("url");
>+          var line = this.hasAttribute("line") ? parseInt(this.getAttribute("line")) : null;  
>           window.openDialog(
>             "chrome://navigator/content/viewSource.xul", "_blank", 
>-            "scrollbars,resizable,chrome,dialog=no", this.getAttribute("url"));
>+            "scrollbars,resizable,chrome,dialog=no", url, null, null, line);
With the parseInt in viewsource.js you can just use two inline getAttribute
calls instead now.

Comment 86

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 123360 [details] [diff] [review]
Patch for back end updated to comment 78

Requesting 1.4 approval for the back end part.	This is a very safe change that
just adds line number information to the markup view-source generates; this
should enable front-end code to be added (in the other patch, or via an XPI)
that will enable users to "go to line" in view-source.

Comment 87

[Christian Schmidt]

Attachment: v1.8 - updated to comment 85

> The subtle difference is that Notepad et al don't close the prompt for the
> input while displaying the invalid input alert... [...]  so I think you
should 
> be consistent too, whichever way around, either always prompt after an error
> or never prompt after an error.
I decided to always prompt after an error. This allows the user to see and
possible change what he typed.

Comment 88

[rbs]

+  // the first line in the pre element is number 123 (except the first 
+  // pre that does not have an id attribute).

Now that the first pre isn't a special-case, the "except" bit is not necessary 
anymore, you might want update the comment at check-in.

Comment 89

[[e-mail]]

Comment on attachment 123452 [details] [diff] [review]
v1.8 - updated to comment 85

>+  if (gGoToLine) {
I think gGoToLine > 0 would be better here.
>+    goToLine(gGoToLine);
>+    gGoToLine = 0;
>+  }

>+  var selection = window._content.getSelection();
>+  selection.removeAllRanges();
>+
>+  if (!range) {
>+    return false;
>+  }
I think you should swap these around, otherwise the previous selection will be
lost if the line was not found.

Comment 90

[Christian Schmidt]

Attachment: v1.9 that addresses comment 88 and 89

I am not sure whether this update needs new reviews.

I do not have CVS access so I need help getting this checked in when it's time 
- thanks.

Comment 91

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 123502 [details] [diff] [review]
v1.9 that addresses comment 88 and 89

Neil, one last look, please?  ;)

sr=me

Comment 92

[Asa Dotzler [:asa]]

Comment on attachment 123360 [details] [diff] [review]
Patch for back end updated to comment 78

a=asa (on behalf of drivers) for checkin to 1.4

Comment 93

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 123360 [details] [diff] [review]
Patch for back end updated to comment 78

Back end patch checked in.

Comment 94

[Asa Dotzler [:asa]]

Comment on attachment 123502 [details] [diff] [review]
v1.9 that addresses comment 88 and 89

a=asa (on behalf of drivers) for checkin to 1.4

Comment 95

[Boris Zbarsky [:bzbarsky]]

Patch checked in.  Christian, thank you once again (I just noticed that this
plays perfectly with the debug-only CSS error reporting, so life got that much
happier).

Comment 96

[rbs]

*** Bug 239211 has been marked as a duplicate of this bug. ***