Closed
Bug 700361
Opened 13 years ago
Closed 13 years ago
View Source line numbers are busted for plain-text documents
Categories
(Toolkit :: View Source, defect)
Toolkit
View Source
Tracking
()
RESOLVED
FIXED
mozilla11
Tracking | Status | |
---|---|---|
firefox10 | + | unaffected |
People
(Reporter: jruderman, Assigned: hsivonen)
References
Details
(Keywords: regression)
Attachments
(2 files)
2.22 KB,
patch
|
Gavin
:
review+
christian
:
approval-mozilla-aurora-
|
Details | Diff | Splinter Review |
2.52 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
1. data:text/plain,1
2. View Source
3. Click somewhere
Result: status bar says "Line NaN, Col 2"
This also affects stylesheets, e.g.
1. http://www.squarefree.com/
2. View Source
3. Click link to stylesheet
4. Click somewhere
Maybe findLocation's assumptions are no longer valid?
http://hg.mozilla.org/mozilla-central/annotate/42e8b5f1357c/toolkit/components/viewsource/content/viewSource.js#l576
Assignee | ||
Comment 1•13 years ago
|
||
That's interesting. I didn't realize that in the old implementation, text/plain went through the View Source highlighter, too.
Assignee | ||
Comment 2•13 years ago
|
||
I have no clue how to write a test case for this. I know very, very little about front end testing.
Updated•13 years ago
|
Comment 3•13 years ago
|
||
Comment on attachment 572751 [details] [diff] [review]
Make viewSource.js assume a pre without an id starts on line 1
You could write a browser-chrome test that opens view source for various types of documents, and then checks the status bar line numbers as you move the selection in the document.
https://developer.mozilla.org/en/Browser_chrome_tests
(you can ping me if you have other questions)
Attachment #572751 -
Flags: review?(gavin.sharp) → review+
Comment 4•13 years ago
|
||
Looks like Geoff is working on some tests already, in bug 702448.
Assignee | ||
Comment 5•13 years ago
|
||
OK. Let's deal with the tests in the other bug. Thanks for the review.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b534bab26edf
Flags: in-testsuite?
Target Milestone: --- → mozilla11
Comment 6•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 7•13 years ago
|
||
This needs to land on aurora too, right? I see the same breakage there.
tracking-firefox10:
--- → ?
Assignee | ||
Comment 8•13 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #7)
> This needs to land on aurora too, right? I see the same breakage there.
There are multiple View Source regressions that affect Aurora. Of those, bug 700034 hasn't landed on trunk yet.
Once bug 700034 has landed (and no new regressions have been found by then), the plan is to ask the release drivers if they prefer landing all the regression fixes on Aurora or if the new View Source should be disabled on Aurora. I think it doesn't make sense to make that call in several pieces.
Updated•13 years ago
|
Assignee | ||
Comment 9•13 years ago
|
||
Comment on attachment 572751 [details] [diff] [review]
Make viewSource.js assume a pre without an id starts on line 1
(This comment has been posted on all the bugs mentioned in this comment, except bug 710142, so that the release drivers see it regardless of the order in which they process approval requests.)
The new View Source implementation landed before Firefox 10 moved to Aurora. Afterwards, a bunch of regressions were identified. Many of the regression fixes didn't land before Firefox 10 moved to Aurora but they have now been fixed on trunk except for bug 710142.
To avoid shipping with regressions, we either need to land all the regression fixes on Aurora for Firefox 10 (followed by a fix for bug 710142 in Beta if it doesn't make it before Dec 20th) or switch back to the old View Source implementation on Aurora. The new View Source implementation provides much better diagnostics for Web developers than the old View Source implementation.
So I'd like to ask the release drivers to either approve bug 535530, bug 699356, bug 699365, bug 700034, bug 700361, bug 703965, bug 704667 and bug 705473 plus bug 695640, which is a non-regression tweak, or to approve bug 710175 for reverting to the old View Source implementation for Firefox 10.
Attachment #572751 -
Flags: approval-mozilla-aurora?
Comment 10•13 years ago
|
||
Comment on attachment 572751 [details] [diff] [review]
Make viewSource.js assume a pre without an id starts on line 1
[triage comment]
We decided to back out the new view source parser (bug 710175) rather than take this fixup for Firefox 10.
Denying for Aurora.
Attachment #572751 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Assignee | ||
Comment 11•13 years ago
|
||
Firefox 10 no longer affected due to bug 710175 landing.
Comment 12•13 years ago
|
||
Attachment #598738 -
Flags: review?(hsivonen)
Updated•13 years ago
|
Attachment #598738 -
Flags: review?(hsivonen) → review?(dao)
Comment 13•13 years ago
|
||
Comment on attachment 598738 [details] [diff] [review]
test
Use of runningPlainText seems confusing, I think I would prefer nesting like (pseudo-code):
openViewSourceWindow("data:text/html,etc", function() {
checkViewSource();
closeViewSourceWindow(function () {
executeSoon(function () {
openViewSourceWindow("data:text/plain,etc", function () {
checkViewSource();
closeViewSourceWindow(aWindow, finish);
});
});
});
});
You could also implement an openAndCloseViewSourceWindow(url, checkFunction, callback); helper to simplify this, so that you could write:
openAndCloseViewSourceWindow("text", checkViewSource, function () {
openAndCloseViewSourceWindow("html", checkViewSource, finish);
});
r=me with those options considered - you can also just land this as-is if you want.
Attachment #598738 -
Flags: review?(dao) → review+
Comment 14•13 years ago
|
||
Flags: in-testsuite? → in-testsuite+
Comment 15•13 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•