253905 - the function goToLine in viewSource.js (Page Source) can not go to the last line

Status: RESOLVED FIXED

(Core Graveyard :: View Source, defect)

Description

[mgueury]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.1) Gecko/20040725
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.1) Gecko/20040725

The function goToLine of the viewSource.js can not go to the last
line.

I was trying to use this function for writting a new browser extension.
But I am not able to jump to the last line of a file:

The reason is because of this code:

         if (curLine == line && !("range" in result)) {
 -  489           result.range = document.createRange();
 -  490           result.range.setStart(textNode, curPos);
    491 
    492           //
    493           // This will always be overridden later, except when we look for
    494           // the very last line in the file (this is the only line that does
    495           // not end with \n).
    496           //
 -  497           result.range.setEndAfter(pre.lastChild);
    498 
 -  499         } else if (curLine == line + 1) {
 -  500           result.range.setEnd(textNode, curPos - 1);
 -  501           found = true;
 -  502           break;
    503         }

Line 499, there is a condition with curLine=line+1
Unhappily if you try to go to the last line, there is no line +1 and it does
not work.



Reproducible: Always
Steps to Reproduce:
1. call the function goToLine( 16 ) when viewing www.google.com
  ( 16 ) is the last line

Actual Results:  
Nothing - the line is not found

Expected Results:  
Jump to the line 16

Comment 1

[mgueury]

To correct the problem, one possibility is to check at the end
of the search that the searched line is the last line.

I tried to correct the viewSource.js (the one of 171 that I have).
I will attach the file.

By doing the following
1) added a variable lastNode 
2) test at the end if 
   - it is a search
   - with curLine = serched line
   - and that is not found 
   - and where the lastNode exist
   then set then end of the range

[...]
  var found = false;
  var lastNode = null;
  for (var textNode = treewalker.firstChild();
       textNode && !found;
[...]
    }    
    lastNode = textNode;
  }

  // The searched line is the last line
  if( pre && curLine == line && !found && lastNode ) {
    // Set the range end.
    result.range.setEnd(lastNode, lastNode.data.length);
    found = true;
  }
    
  return found;
}
[...]

Tested with that change. And it works. I have no access to CVS of Mozilla :-)
Can you double-check this ?

Thanks

Comment 2

[mgueury]

Attachment: Potential fix

Comment 3

[Frank Wein [:mcsmurf]]

Can you please make a patch against this file here
http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/viewsource.js?raw=1
(this is from trunk) and attach it? I tried to create a patch, but wasn't sure
what changes were from you and what changes were from 1.7.1->trunk. You can get
diff from http://gnuwin32.sourceforge.net/packages/diffutils.htm if you don't
have it.

Comment 4

[rbs]

-> regression

The code was heavily tested, and it used to work. In fact, it still works in my
copy of Netscape 7.1, but not in the latest Mozilla build. Maybe something has
changed somewhere in the back-end. Anyway, making the front-end more robust is
welcomed.

Comment 5

[mgueury]

Here is the output from diff:

432d431
<   var lastNode = null;
506,507c505
<     }
<     lastNode = textNode;
---
>     }
510,516d507
<   // The searched line is the last line
<   if( pre && curLine == line && !found && lastNode ) {
<     // Set the range end.
<     result.range.setEnd(lastNode, lastNode.data.length);
<     found = true;
<   }
<

Comment 6

[Frank Wein [:mcsmurf]]

Can you create a version with diff -u please? It seems patch doesn't quite like
the normal diff version (without the -u).

Comment 7

[mgueury]

Here it is.

--- viewsource.js2      2004-08-07 20:49:46.981737600 +0200
+++ viewsource.js1      2004-08-07 20:49:18.560870400 +0200
@@ -429,6 +429,7 @@
   var firstCol = 1;

   var found = false;
+  var lastNode = null;
   for (var textNode = treewalker.firstChild();
        textNode && !found;
        textNode = treewalker.nextNode()) {
@@ -502,9 +503,17 @@
           break;
         }
       }
-    }
+    }
+    lastNode = textNode;
   }

+  // The searched line is the last line
+  if( pre && curLine == line && !found && lastNode ) {
+    // Set the range end.
+    result.range.setEnd(lastNode, lastNode.data.length);
+    found = true;
+  }
+
   return found;
 }

Comment 8

[rbs]

Attachment: real fix

This is the real fix. It was a regression from when we started displaying the
line:col on the status bar. The code (in m1.4 / Nav7.1) used to check the
range. Since then, the |found| variable was introduced, and it missed precisely
the case of the last line. 

------- the other patch duplicates that -----
+  // The searched line is the last line
+  if( pre && curLine == line && !found && lastNode ) {

->   |pre| always true.
->   |curLine == line && !found && lastNode|, that's equivalent to last line
test in the loop...

+  }
+

Comment 9

[rbs]

Comment on attachment 155754 [details] [diff] [review]
real fix

Will also request approval-aviary & approval1.7.3.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 155754 [details] [diff] [review]
real fix

Yeah, makes sense.  r+sr=bzbarsky.

Comment 11

[rbs]

Comment on attachment 155754 [details] [diff] [review]
real fix

[Bad timing, the tree has just closed for 1.8a3...]

drivers, this is a regression from m1.4/Nav7.1. Goto Line (Ctrl+L) in
view-source cannot go to the last line due to an incomplete test condition. The
fix is a one-liner that brings the code in parity to what is in m1.4/Nav7.1.

Comment 12

[Mike Kaply [:mkaply]]

Comment on attachment 155754 [details] [diff] [review]
real fix

a=mkaply for all

Comment 13

[rbs]

Fixed on the aviary1.0 branch and the 1.7.3 branch.

However, I am having troubles fixing the bug on the trunk. I get the message:
cvs [diff aborted]: connect to cvs.mozilla.org:2401 failed: Connection timed out

Somebody knows what is going on? I cannot access anything, including the
aviary1.0 branch and the 1.7.3 branch. I checked in there a couple of days ago
immediately after I got the approval.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Somebody knows what is going on?

pserver access finally got turned off yesterday.  That may be causing trouble
for you...  Google groups doesn't seem to have the message yet; it said:

  The pserver protocol has been disabled for cvs.mozilla.org; access is now via
  ssh only.

  If you need assistance establishing adding ssh to your cvs account, or if you
  need assistance getting checkins landed, please contact or
  [e-mail]

  Information is up (and will be updated as time goes on) at:
  http://www.mozilla.org/cvs-ssh-faq.html

Comment 15

[mgueury]

Being quite novice, is aviary and Firefox, the same thing ?
I just wished to say that I checkout Firefox yesterday from CVS.
And the problem is in Firefox too.

Forget this message, if it is stupid.

Comment 16

[rbs]

> Being quite novice, is aviary and Firefox, the same thing ?

aviary is a branch of the Firefox trunk -- it is what is going to become Firefox
1.0. [similar as m1.7 is a branch of the seamonkey trunk and m1.7 gave Netscape7.2].

I haven't yet checked in the fix on the trunk [Firefox & Seamonkey]. So if you
checkout from the CVS trunk, you will still see the problem. I am not yet able
to checkin the fix. I am having all sort of problems with the newish CVS over
ssh because I use samba, and the CVS admins haven't yet resolved those problems.

Comment 17

[rbs]

Finally fixed on the trunk.

Comment 18

[mgueury]

Can you fix it too for Firefox ? Aviary ? How does it work to get it 
fixed there ? 

I was looking the source of viewsource.js in Firefox 1.0 PR
today. There is another bug in Firefox 1.0 PR only. I will log it.

Thanks by advance.

Ps: this is for this: http://users.skynet.be/mgueury/mozilla/

Comment 19

[mgueury]

Sorry forget my update. It is working in Firefox 1.0 PR.
But there is another problem. bug 259363. But this is fixed.