The default bug view has changed. See this FAQ.

Add line numbers to View Source

RESOLVED FIXED in mozilla12

Status

()

Toolkit
View Source
--
enhancement
RESOLVED FIXED
13 years ago
5 years ago

People

(Reporter: Hiro, Assigned: darktrojan)

Tracking

(Depends on: 4 bugs, Blocks: 1 bug)

unspecified
mozilla12
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox2 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 8 obsolete attachments)

18.39 KB, image/png
Details
1.03 KB, application/x-xpinstall
Details
133 bytes, text/html
Details
1.99 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
3.34 KB, patch
Ehsan
: review+
philor
: checkin+
Details | Diff | Splinter Review
13.32 KB, patch
smaug
: review+
Details | Diff | Splinter Review
(Reporter)

Description

13 years ago
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

13 years ago
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.
This looks like a DUPE of bug 207656 

Comment 3

13 years ago
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

12 years ago
Having current cursor position (column and line) is very usefull when validating
an html page !

Comment 5

12 years ago
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?
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

12 years ago
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

12 years ago
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

12 years ago
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

12 years ago
here here - line numbers next to the lines !

Comment 11

12 years ago
This bug is absolutely critical to resolve.  Firefox view source is like a complete joke without view line numbers.

Comment 12

11 years ago
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

11 years ago
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

11 years ago
Can only nominate.
Flags: blocking-firefox2?
Assignee: bugs → nobody
We're not blocking the release of Firefox 2 for this. It sounds like a worthwhile addition, though.
Flags: blocking-firefox2? → blocking-firefox2-

Comment 16

11 years ago
Yes, would be great to have it in :)

Comment 17

11 years ago
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

11 years ago
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

11 years ago
Sorry, I meant bug 239211 not 239221.

Comment 20

11 years ago
No, bug 239211 is the SeaMonkey equivalent of this bug.
Product: Firefox → Toolkit
Duplicate of this bug: 455944
Blocks: 455888
Depends on: 469762

Updated

8 years ago
Duplicate of this bug: 239211

Comment 23

8 years ago
Why is this still not implemented? We are now at 3.1!
Duplicate of this bug: 481992
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

7 years ago
5 1/2 years? Is it sooo hard to fix?
Just notice for developers: even IE8 already shows line numbers in View Source.
(Assignee)

Comment 28

7 years ago
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.)
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Attachment #432985 - Flags: feedback?(mrbkap)
(Assignee)

Updated

7 years ago
Depends on: 166235
No longer depends on: 469762
(Assignee)

Updated

7 years ago
Depends on: 469762
(Assignee)

Updated

7 years ago
Attachment #432985 - Flags: feedback?(cab)
(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.)
(Assignee)

Comment 30

7 years ago
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)?
Attachment #432985 - Attachment is obsolete: true
Attachment #432985 - Flags: feedback?(mrbkap)
Attachment #432985 - Flags: feedback?(cab)
(Assignee)

Updated

7 years ago
Attachment #440990 - Flags: feedback?(mrbkap)
Attachment #440990 - Flags: feedback?(bzbarsky)
(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.
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.
(Assignee)

Comment 33

7 years ago
Created attachment 441458 [details] [diff] [review]
patch v3

Good point. And fixed.
Attachment #440990 - Attachment is obsolete: true
Attachment #441458 - Flags: feedback?(mrbkap)
Attachment #441458 - Flags: feedback?(bzbarsky)
Attachment #440990 - Flags: feedback?(mrbkap)
Attachment #440990 - Flags: feedback?(bzbarsky)
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 on attachment 441458 [details] [diff] [review]
patch v3

pending answers to my questions.
Attachment #441458 - Flags: feedback?(bzbarsky) → feedback-
(Assignee)

Comment 36

7 years ago
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%.

Updated

7 years ago
Attachment #441458 - Flags: feedback?(mrbkap)
(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.)
(Assignee)

Comment 38

7 years ago
Created attachment 446641 [details] [diff] [review]
patch v4

Fixes multiple-line tags in error and a few other bits and pieces.
Attachment #441458 - Attachment is obsolete: true
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).
(Assignee)

Comment 40

7 years ago
(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.
(Assignee)

Updated

7 years ago
Attachment #446641 - Flags: review?(bzbarsky)
(Assignee)

Updated

7 years ago
Attachment #448324 - Flags: review?(bzbarsky)
(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.
Jeff, it's likely to be at least a week (two is more likely) until I get to this review...
(Assignee)

Comment 43

7 years ago
(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? :)
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.
(Assignee)

Updated

7 years ago
Attachment #446641 - Flags: approval2.0?
Comment on attachment 446641 [details] [diff] [review]
patch v4

(same as bug 536503 - can't ask for approval until review is completed)
Attachment #446641 - Flags: approval2.0?
(Assignee)

Comment 46

7 years ago
Created attachment 475811 [details] [diff] [review]
patch v5
Attachment #446641 - Attachment is obsolete: true
Attachment #475811 - Flags: review?(bzbarsky)
Attachment #446641 - Flags: review?(bzbarsky)
(Assignee)

Comment 47

7 years ago
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.
Attachment #475812 - Flags: review?(bzbarsky)
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?
(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?
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...
(Assignee)

Updated

6 years ago
Depends on: 482921
(Assignee)

Comment 51

6 years ago
Thanks for the update Boris. I'll have another look at this once the rewrite is done.
Comment on attachment 448324 [details] [diff] [review]
Simple view-source test

r=me
Attachment #448324 - Flags: review?(bzbarsky) → review+
Attachment #475812 - Flags: review?(bzbarsky)
Attachment #475811 - Flags: review?(bzbarsky)
Can the reviewed test be landed?
(Assignee)

Comment 54

6 years ago
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.
Attachment #475811 - Attachment is obsolete: true
Attachment #571210 - Flags: review?(ehsan)
(Assignee)

Updated

6 years ago
Attachment #475812 - Attachment is obsolete: true
(Assignee)

Comment 55

6 years ago
Created attachment 571213 [details] [diff] [review]
fixed-up test

I've made a few slight changes to Curtis's test.
Attachment #448324 - Attachment is obsolete: true
Attachment #571213 - Flags: review?(ehsan)
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.

Updated

5 years ago
Attachment #571210 - Flags: review?(ehsan) → review+
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!
Attachment #571213 - Flags: review?(ehsan) → review-
(Assignee)

Comment 58

5 years ago
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.
Attachment #571213 - Attachment is obsolete: true
Attachment #577870 - Flags: review?(ehsan)
Depends on: 706394
Comment on attachment 577870 [details] [diff] [review]
tests

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

Looks great, thanks!
Attachment #577870 - Flags: review?(ehsan) → review+
(Assignee)

Comment 60

5 years ago
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.
Target Milestone: --- → mozilla12
(Assignee)

Updated

5 years ago
Depends on: 713479
(Assignee)

Comment 61

5 years ago
Backed out, reftest bustage. :(
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f29daaecbcc
Looks like the reference images need to be updated to include the line numbers.
Comment on attachment 577870 [details] [diff] [review]
tests

https://hg.mozilla.org/mozilla-central/rev/389a18921ea5
Attachment #577870 - Flags: checkin+
(Assignee)

Comment 64

5 years ago
Created attachment 584319 [details] [diff] [review]
fix for broken reftests
Attachment #584319 - Flags: review?(ehsan)
Comment on attachment 584319 [details] [diff] [review]
fix for broken reftests

Please add a comment why empty id attributes.
And, if possible, remove =""
Attachment #584319 - Flags: review?(ehsan) → review+
(Assignee)

Comment 66

5 years ago
Take 2
https://hg.mozilla.org/integration/mozilla-inbound/rev/02b3a12aea70
https://hg.mozilla.org/integration/mozilla-inbound/rev/39d88d1cd7d0
https://hg.mozilla.org/mozilla-central/rev/02b3a12aea70
https://hg.mozilla.org/mozilla-central/rev/39d88d1cd7d0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Summary: Add line numbers to View Source for Firefox → Add line numbers to View Source

Updated

5 years ago
Depends on: 808401
You need to log in before you can comment on or make changes to this bug.