Last Comment Bug 246620 - Add line numbers to View Source
: Add line numbers to View Source
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: View Source (show other bugs)
: unspecified
: All All
: -- enhancement with 60 votes (vote)
: mozilla12
Assigned To: Geoff Lankow (:darktrojan)
:
Mentors:
: 239211 455944 481992 (view as bug list)
Depends on: 469762 706394 713479 808401 166235 482921
Blocks: 455888
  Show dependency treegraph
 
Reported: 2004-06-13 22:43 PDT by Hiro
Modified: 2012-11-04 09:38 PST (History)
43 users (show)
mbeltzner: blocking‑firefox2-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Screenshot of line numbers in HomeSite HTML editor (18.39 KB, image/png)
2004-07-18 08:33 PDT, Ross Shannon
no flags Details
Extension to show line numbers in status bar and implement goto line number function (1.03 KB, application/x-xpinstall)
2004-12-28 12:10 PST, johann.petrak@gmail.com
no flags Details
patch v1 (8.22 KB, patch)
2010-03-16 19:57 PDT, Geoff Lankow (:darktrojan)
no flags Details | Diff | Splinter Review
patch v2 (14.88 KB, patch)
2010-04-23 01:39 PDT, Geoff Lankow (:darktrojan)
no flags Details | Diff | Splinter Review
HTML file with a comment (133 bytes, text/html)
2010-04-25 12:38 PDT, Curtis Bartley [:cbartley]
no flags Details
patch v3 (18.25 KB, patch)
2010-04-26 03:50 PDT, Geoff Lankow (:darktrojan)
bzbarsky: feedback-
Details | Diff | Splinter Review
patch v4 (22.74 KB, patch)
2010-05-20 19:02 PDT, Geoff Lankow (:darktrojan)
no flags Details | Diff | Splinter Review
Simple view-source test (3.33 KB, patch)
2010-05-30 19:49 PDT, Curtis Bartley [:cbartley]
bzbarsky: review+
Details | Diff | Splinter Review
patch v5 (23.33 KB, patch)
2010-09-16 04:43 PDT, Geoff Lankow (:darktrojan)
no flags Details | Diff | Splinter Review
more detailed tests (7.37 KB, patch)
2010-09-16 04:47 PDT, Geoff Lankow (:darktrojan)
no flags Details | Diff | Splinter Review
patch, v6 (1.99 KB, patch)
2011-11-01 18:12 PDT, Geoff Lankow (:darktrojan)
ehsan: review+
Details | Diff | Splinter Review
fixed-up test (3.24 KB, patch)
2011-11-01 18:36 PDT, Geoff Lankow (:darktrojan)
ehsan: review-
Details | Diff | Splinter Review
tests (3.34 KB, patch)
2011-11-29 23:27 PST, Geoff Lankow (:darktrojan)
ehsan: review+
philringnalda: checkin+
Details | Diff | Splinter Review
fix for broken reftests (13.32 KB, patch)
2011-12-26 03:07 PST, Geoff Lankow (:darktrojan)
bugs: review+
Details | Diff | Splinter Review

Description Hiro 2004-06-13 22:43:13 PDT
The status line is shown by view source by correction of bug15364 at mozilla.
I want the same function also at Firefox.

There is a problem of bug 194380 in present mozilla view sourcede.
When adding a function by Firefox, cautions are required also for this problem.

Windows XP
Mozilla/5.0 (Windows; U; Windows NT 5.1; ja-JP; rv:1.7) Gecko/20040613
Firefox/0.8.0+
Comment 1 Ross Shannon 2004-07-18 08:33:36 PDT
Created attachment 153586 [details]
Screenshot of line numbers in HomeSite HTML editor

Firstly, the status bar is very useful, mainly for displaying the Column
number. Go to Line, while useful functionality, is not as useful as just
scrolling through the file with line numbers displayed in parallel.

When tracking down validation errors oftentimes all the context you have to go
on is the line# and column# the validator gives back. If your site uses
includes or any sort of programming, these numbers will not correspond to the
numbers in your own local source file, so you need to be able to find the line
in the online source (do a view source on the generated page) and then find the
corresponding text in the local copy.

The Mozilla suite's implementation of this is not as good as it could be. There
should be a column running down the left side of the window listing the line
numbers. This makes word-wrap line breaks obvious. I'm attaching an image of
how this is represented in Homesite 4.5.

The column can either be fixed width with a gutter as above, or can be sized
dynamically based on the length of the source file. I'm not a Mozilla hacker,
but to get the required width of the line# column you can get the number of
lines of source code, which will be, say, 350. Then you know it needs to be 3
digits wide.
Comment 2 johann.petrak@gmail.com 2004-09-21 15:10:55 PDT
This looks like a DUPE of bug 207656 
Comment 3 Jerry Baker 2004-09-21 16:47:30 PDT
That bug looks like something to put the current line number in the status bar.
That is a useless feature when looking for a particular line.
Comment 4 Sebastien Celles 2004-12-28 04:28:04 PST
Having current cursor position (column and line) is very usefull when validating
an html page !
Comment 5 Jerry Baker 2004-12-28 06:13:42 PST
So when a validator says there is an error on line 394, how fast can you find
that by scrolling, clicking, and looking at the status bar? How many times do
you have to do that to find the right line?
Comment 6 johann.petrak@gmail.com 2004-12-28 12:10:24 PST
Created attachment 169741 [details]
Extension to show line numbers in status bar and implement goto line number function

This is an extension by Chris Neale (http://cdn.mozdev.org/) that shows the
line and column number of where the cursor is in the view source window in the
status bar of the window and also  adds the edit->goto line menu item that lets
you jump to a certain line number directly. Maybe it is helpful for those that
are desperate, want to improve on it or want to use it as a basis for a patch
for this bug.
Comment 7 Jerry Baker 2004-12-28 17:03:45 PST
All that is really needed is something that says something like:

foreach(line) {
  $i++
  print i . ". $";
  }

Forgive the pseudo-Perl ... it's my native tongue.
Comment 8 Jason Barnabe (np) 2005-04-06 20:38:42 PDT
I agree with comment 5; displaying the character in the status bar is not the
ultimate solution (and it's coverted by bug 207656 anyway). I think the solution
is to have line numbers to display in the left margin and ensure that
copy-pasting the source won't also copy-paste those numbers. To do this, I
believe the following would work:

a) Change the code here
http://lxr.mozilla.org/seamonkey/source/parser/htmlparser/src/nsViewSourceHTML.cpp#1194
to output a new pre element at every line break in the source (I don't know C++)
b) Put in a CSS counter to show a number at the start of every pre element.
Comment 9 Max Nokhrin 2005-07-18 14:19:47 PDT
I would suggest something different (additional) to what others are saying. I
wound really like to have an option in the View menu (checkbox) to Show Line
Numbers. If checked, this would add a vertical bar along the left side of the
screen with line numbers in it. Similar to VC or pretty much any other decent
text/code editor. Of course, a Goto Line... feature would also be great.
Comment 10 qbxk 2005-08-15 06:45:29 PDT
here here - line numbers next to the lines !
Comment 11 nate parsons 2005-12-01 13:06:42 PST
This bug is absolutely critical to resolve.  Firefox view source is like a complete joke without view line numbers.
Comment 12 Alex 2006-01-11 22:06:04 PST
I am using the excellent HTML Validator plugin to view HTML errors and warnings, and my biggest complaint is the lack of line numbers. The HTML Validator plugin author says that this is something mozilla/Firefox core needs to fix.
Comment 13 eric.caron 2006-07-05 16:59:57 PDT
I know this bug has been neglected, but does anyone know anyone who can either A) fix this or B) mark it as a 2.0-blocker?
Comment 14 Jerry Baker 2006-07-05 17:08:40 PDT
Can only nominate.
Comment 15 Mike Beltzner [:beltzner, not reading bugmail] 2006-07-06 07:00:31 PDT
We're not blocking the release of Firefox 2 for this. It sounds like a worthwhile addition, though.
Comment 16 Max Nokhrin 2006-07-06 07:02:07 PDT
Yes, would be great to have it in :)
Comment 17 jsnod 2006-07-06 09:44:28 PDT
The newest beta of the HTML Validator plugin has support for line numbers in view source, although it's a little buggy at the moment.
http://users.skynet.be/mgueury/mozilla/
Comment 18 Tae Kim 2006-10-16 04:35:08 PDT
Is this a dupe of bug 239221? (Or maybe dupe the other once since the last comment was at the end of 2004?) I'm not sure what the original reporter is talking about but it looks like there is a status bar for view source for the trunk build.

As for implementing line numbers in the left column, it looks likes nsViewSourceHTML.cpp already adds a <pre id="line%"> tag at intervals defined by NS_VIEWSOURCE_TOKENS_PER_BLOCK in order to fix bug 86355.

Can't I just output the tag for every line instead of intervals and just add a CSS before counter the pre tag at viewsource.css? (As suggested in a previous comment) I can then add a View -> Line Numbers menu option to enable or disable the css counter similar to the wrap lines implemention.

It would also simply the go to line feature without all the treewalker business.

Any reasons why the pre tag can't be outputted for every line?
Comment 19 Tae Kim 2006-10-16 04:38:16 PDT
Sorry, I meant bug 239211 not 239221.
Comment 20 Adam Guthrie 2006-10-21 19:38:44 PDT
No, bug 239211 is the SeaMonkey equivalent of this bug.
Comment 21 Matthias Versen [:Matti] 2008-09-18 14:56:25 PDT
*** Bug 455944 has been marked as a duplicate of this bug. ***
Comment 22 Ray Kiddy 2009-01-22 14:10:04 PST
*** Bug 239211 has been marked as a duplicate of this bug. ***
Comment 23 Leith Bade 2009-01-28 16:59:22 PST
Why is this still not implemented? We are now at 3.1!
Comment 24 Phil Ringnalda (:philor) 2009-03-06 20:35:57 PST
*** Bug 481992 has been marked as a duplicate of this bug. ***
Comment 25 Marat Tanalin | tanalin.com 2009-03-09 15:09:39 PDT
It would be really useful for any web developer to have ability to easily determine what exact line is line that Error Console referring to. "Go to Line" feature is not the same. Very not the same.
Comment 26 Lars Pohlmann 2010-01-12 03:35:40 PST
5 1/2 years? Is it sooo hard to fix?
Comment 27 Marat Tanalin | tanalin.com 2010-01-12 04:16:05 PST
Just notice for developers: even IE8 already shows line numbers in View Source.
Comment 28 Geoff Lankow (:darktrojan) 2010-03-16 19:57:34 PDT
Created attachment 432985 [details] [diff] [review]
patch v1

This does the donkey work of actually adding numbers into the HTML (each line number in a <span class="linenumber">). There's still some issues with how best to use the span - the current CSS is a bit of a hack, but it works.

(This patch also fixes an earlier mistake where I assigned a PRInt32 to a PRBool. Oops.)
Comment 29 Curtis Bartley [:cbartley] 2010-04-07 10:19:16 PDT
(In reply to comment #28)
> Created an attachment (id=432985) [details]
> patch v1
> 
> This does the donkey work of actually adding numbers into the HTML (each line
> number in a <span class="linenumber">). There's still some issues with how best
> to use the span - the current CSS is a bit of a hack, but it works.
> 
> (This patch also fixes an earlier mistake where I assigned a PRInt32 to a
> PRBool. Oops.)

Note that you can use CSS counters to generate the linenumbers for you, for example:

  body {
    counter-reset: line; 
  }
  .linenumber:before {
    content: counter(line) ": ";
    counter-increment: line;
  }

I haven't looked at the C++ code in detail yet, but I see that with your patch there is both a "mLineNumber" and a "myLineNumber".

(Also, mrbkap's feedback is likely to be more useful than mine since he can actually approve your patch for landing.)
Comment 30 Geoff Lankow (:darktrojan) 2010-04-23 01:39:27 PDT
Created attachment 440990 [details] [diff] [review]
patch v2

I *think* the only remaining piece is to make the space for line numbers smaller or larger as appropriate. Is there a way to know the total number of lines when I'm in BuildModel (ie. before we go through and count them all, so I can set an attribute on the body tag)?
Comment 31 Curtis Bartley [:cbartley] 2010-04-23 16:55:44 PDT
(In reply to comment #30)
> Created an attachment (id=440990) [details]
> patch v2
> 
> I *think* the only remaining piece is to make the space for line numbers
> smaller or larger as appropriate. Is there a way to know the total number of
> lines when I'm in BuildModel (ie. before we go through and count them all, so I
> can set an attribute on the body tag)?

I personally think just assigning a fixed width like you're currently doing is fine.
Comment 32 Curtis Bartley [:cbartley] 2010-04-25 12:38:36 PDT
Created attachment 441390 [details]
HTML file with a comment

Geoff: I discovered a bug with how the line numbers are displayed in the presence of a multi-line HTML comment.  This is with the v2 patch.
Comment 33 Geoff Lankow (:darktrojan) 2010-04-26 03:50:26 PDT
Created attachment 441458 [details] [diff] [review]
patch v3

Good point. And fixed.
Comment 34 Boris Zbarsky [:bz] 2010-04-26 06:36:47 PDT
I didn't look at the code too carefully yet (though there are various formatting and string usage issues there as far as I can tell).  General questions:

1) Have you measured the memory consumption of view-source on large files with
   and without this patch?  How does this patch affect it?
2) Have you measured the performance of view-source on large files with and
   without this patch?  How does this patch affect it?
Comment 35 Boris Zbarsky [:bz] 2010-05-05 22:38:17 PDT
Comment on attachment 441458 [details] [diff] [review]
patch v3

pending answers to my questions.
Comment 36 Geoff Lankow (:darktrojan) 2010-05-10 17:39:03 PDT
I don't really have the right tools to test these sorts of things (and I don't have jemalloc), but memory usage seems to be up about 10-15%. 

StartNewPreBlock seems to use a lot of time. Called every 40 lines the overall time was up 50-90% on some pages. :( If it's not called at all the overall time is up less than 10%.
Comment 37 Curtis Bartley [:cbartley] 2010-05-10 19:28:44 PDT
(In reply to comment #36)
> I don't really have the right tools to test these sorts of things (and I don't
> have jemalloc), but memory usage seems to be up about 10-15%. 

You might look at a simple count of elements in the view source document, both with line numbers and without.  E.g. "javascript:alert(document.body.getElementsByTagName("*").length)".  As I recall, the performance difference I saw for view-source linkification pretty closely correlated to the number of elements created.

> StartNewPreBlock seems to use a lot of time. Called every 40 lines the overall
> time was up 50-90% on some pages. :( If it's not called at all the overall time
> is up less than 10%.

That seems surprising.  (Alas, I have no other insight to offer.)
Comment 38 Geoff Lankow (:darktrojan) 2010-05-20 19:02:56 PDT
Created attachment 446641 [details] [diff] [review]
patch v4

Fixes multiple-line tags in error and a few other bits and pieces.
Comment 39 Curtis Bartley [:cbartley] 2010-05-30 19:49:57 PDT
Created attachment 448324 [details] [diff] [review]
Simple view-source test

This is a dirt-simple mochitest for view-source.  It verifies that the source displayed by view-source actually matches the true source.  It doesn't test formatting issues or the display of line numbers in the main patch.  Notably it won't detect the multi-line comment bug in the v3 patch.  It does provide some confidence that changes don't break existing view-source functionality.

Right now the test uses itself as the test source.  It would benefit from better test source files.

Note that the test file can be run directly in the browser with minor modifications (e.g. firefox file://.../view-source.html).
Comment 40 Geoff Lankow (:darktrojan) 2010-05-31 03:22:41 PDT
(In reply to comment #39)
> Created an attachment (id=448324) [details]
> Simple view-source test

Very nice. FYI the URL appearing at the beginning is due to the title tag in the view-source page.

>+nsresult CViewSourceHTML::StartLine(void){
>+  //if (mLineNumber % 40 == 0) {
>+  //  StartNewPreBlock();
>+  //}
I think it best if StartNewPreBlock is called every 50 lines, it seems to be a good tradeoff between initial speed and UI responsiveness.

I also think that the line numbers column can be narrowed to 4 characters instead of 5. It's highly unlikely that people would use view source for a 10,000 line page.
Comment 41 Curtis Bartley [:cbartley] 2010-05-31 08:52:10 PDT
(In reply to comment #40)

> I also think that the line numbers column can be narrowed to 4 characters
> instead of 5. It's highly unlikely that people would use view source for a
> 10,000 line page.

Actually I think this is moderately common.  I know a lot of work went in to Firebug to handle long files.  On the other hand, I'm not sure that we need to look pretty when handling really long files.  For example, if the main content gets bumped over by an extra character going from line 9999 to 10000, that may not be such a big deal.
Comment 42 Boris Zbarsky [:bz] 2010-06-01 08:25:03 PDT
Jeff, it's likely to be at least a week (two is more likely) until I get to this review...
Comment 43 Geoff Lankow (:darktrojan) 2010-06-29 16:21:30 PDT
(In reply to comment #42)
> Jeff, it's likely to be at least a week (two is more likely) until I get to
> this review...

Is two weeks up yet Boris? :)
Comment 44 Boris Zbarsky [:bz] 2010-06-30 14:00:18 PDT
Yes, but people keep dumping emergencies on me.  :(  And my sincere apologies for mis-spelling your name....

Current time estimate is ... sometime.  If no one else dumps 1.5MB "must review now" crap on me, should be within a week.
Comment 45 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-08-11 01:59:45 PDT
Comment on attachment 446641 [details] [diff] [review]
patch v4

(same as bug 536503 - can't ask for approval until review is completed)
Comment 46 Geoff Lankow (:darktrojan) 2010-09-16 04:43:56 PDT
Created attachment 475811 [details] [diff] [review]
patch v5
Comment 47 Geoff Lankow (:darktrojan) 2010-09-16 04:47:58 PDT
Created attachment 475812 [details] [diff] [review]
more detailed tests

I've added these tests to make sure the patch does as it's supposed to. Kept as a separate patch for clarity's sake.
Comment 48 Boris Zbarsky [:bz] 2011-04-22 22:23:57 PDT
Ok.  So I really suck at getting to this review, mostly because I keep not finding the time to page this code back in.  :(  Geoff, I'm really sorry about that, and about my complete lack of communication here.

And now Henri is in the process of removing this whole file and rewriting view-source on top of the HTML5 parser...

Henri, would you be willing to effectively roll these patches into your rewrite or do something like this as a followup or something?
Comment 49 Henri Sivonen (:hsivonen) 2011-04-25 23:41:18 PDT
(In reply to comment #48)
> Henri, would you be willing to effectively roll these patches into your rewrite
> or do something like this as a followup or something?

As a follow-up would work. The rewrite is now big enough that I have worries about it getting it reviewed and landed, so I don't want new features added before the rewrite has landed.

It seems to me that the patch here regresses bug 86355. Am I missing something?
Comment 50 Boris Zbarsky [:bz] 2011-04-26 06:33:14 PDT
The patch still does the multiple <pre> block thing.

It also puts each line into a separate block, which breaks bidi stuff even more than bug 86355 did...
Comment 51 Geoff Lankow (:darktrojan) 2011-04-26 15:58:56 PDT
Thanks for the update Boris. I'll have another look at this once the rewrite is done.
Comment 52 Boris Zbarsky [:bz] 2011-07-01 13:55:06 PDT
Comment on attachment 448324 [details] [diff] [review]
Simple view-source test

r=me
Comment 53 :Ehsan Akhgari 2011-09-16 12:09:08 PDT
Can the reviewed test be landed?
Comment 54 Geoff Lankow (:darktrojan) 2011-11-01 18:12:34 PDT
Created attachment 571210 [details] [diff] [review]
patch, v6

This is effectively the same CSS as the previously reviewed patch, the other changes are unnecessary now.
Comment 55 Geoff Lankow (:darktrojan) 2011-11-01 18:36:09 PDT
Created attachment 571213 [details] [diff] [review]
fixed-up test

I've made a few slight changes to Curtis's test.
Comment 56 Henri Sivonen (:hsivonen) 2011-11-02 07:33:23 PDT
Comment on attachment 571213 [details] [diff] [review]
fixed-up test

Drive-by comment:
>+        return xhr.responseText;

If you are only reading responseText, please set xhr.responseType = "text" to make the test run faster.
Comment 57 :Ehsan Akhgari 2011-11-29 11:39:36 PST
Comment on attachment 571213 [details] [diff] [review]
fixed-up test

The test has two problems:

1. It doesn't use SimpleTest.waitForExplicitFinish/finish, so it effectively doesn't test anything.
2. It loads the same test in an iframe, which could potentially cause _another_ instance of the test to run.  I suggest that you add a test HTML file and test the view-source protocol on that URL instead.

Also, please address comment 56 in the next version of the test.  Thanks!
Comment 58 Geoff Lankow (:darktrojan) 2011-11-29 23:27:15 PST
Created attachment 577870 [details] [diff] [review]
tests

(In reply to Ehsan Akhgari [:ehsan] from comment #57)
> 2. It loads the same test in an iframe, which could potentially cause
> _another_ instance of the test to run.  I suggest that you add a test HTML
> file and test the view-source protocol on that URL instead.

It's loading the source of the test in the iframe, not the test itself.
Comment 59 :Ehsan Akhgari 2011-12-21 14:44:00 PST
Comment on attachment 577870 [details] [diff] [review]
tests

Review of attachment 577870 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thanks!
Comment 60 Geoff Lankow (:darktrojan) 2011-12-25 15:44:58 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/4730eb3ec77a
https://hg.mozilla.org/integration/mozilla-inbound/rev/389a18921ea5

Okay, I've pushed this. It doesn't currently work for non-HTML documents, so I'll file a followup and hopefully have it fixed by the end of this cycle.
Comment 61 Geoff Lankow (:darktrojan) 2011-12-25 16:31:48 PST
Backed out, reftest bustage. :(
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f29daaecbcc
Comment 62 Bill Gianopoulos [:WG9s] 2011-12-25 18:19:57 PST
Looks like the reference images need to be updated to include the line numbers.
Comment 63 Phil Ringnalda (:philor) 2011-12-25 20:57:12 PST
Comment on attachment 577870 [details] [diff] [review]
tests

https://hg.mozilla.org/mozilla-central/rev/389a18921ea5
Comment 64 Geoff Lankow (:darktrojan) 2011-12-26 03:07:22 PST
Created attachment 584319 [details] [diff] [review]
fix for broken reftests
Comment 65 Olli Pettay [:smaug] 2011-12-27 02:56:01 PST
Comment on attachment 584319 [details] [diff] [review]
fix for broken reftests

Please add a comment why empty id attributes.
And, if possible, remove =""

Note You need to log in before you can comment on or make changes to this bug.