Last Comment Bug 700034 - [HTML5] view_source.editor.path no longer works with Cset cd9add22f090.
: [HTML5] view_source.editor.path no longer works with Cset cd9add22f090.
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: HTML: Parser (show other bugs)
: Other Branch
: x86_64 Windows 7
: -- normal (vote)
: mozilla11
Assigned To: Henri Sivonen (:hsivonen)
:
Mentors:
Depends on:
Blocks: 482921
  Show dependency treegraph
 
Reported: 2011-11-05 08:08 PDT by Gary [:streetwolf]
Modified: 2012-02-02 03:19 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
unaffected


Attachments
Wait for DOMContentLoaded after STATE_STOP (7.34 KB, patch)
2011-11-28 08:40 PST, Henri Sivonen (:hsivonen)
no flags Details | Diff | Review
Wait for DOMContentLoaded after STATE_STOP, v2 (7.43 KB, patch)
2011-11-29 05:21 PST, Henri Sivonen (:hsivonen)
gavin.sharp: review+
christian: approval‑mozilla‑aurora-
Details | Diff | Review

Description Gary [:streetwolf] 2011-11-05 08:08:21 PDT
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111101 Firefox/10.0a1
Build ID: 20111101073309

Steps to reproduce:

In Prefs I have:

view_source.editor.external boolean true

view_source.editor.path string C:\Program Files (x86)\Notepad++\notepad++.exe


Actual results:

The default source editor displayed when I clicked on 'View Page Source.'


Expected results:

Notepad++.exe should be displayed as the source editor.

Problem looks like it might be caused by https://bugzilla.mozilla.org/show_bug.cgi?id=482921
Comment 2 Alice0775 White 2011-11-05 08:56:33 PDT
Regrtesion window(m-i),
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/746e7c151de2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111101 Firefox/10.0a1 ID:20111101015315
Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/d9cc2539a85d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111031 Firefox/10.0a1 ID:20111101043415
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=746e7c151de2&tochange=d9cc2539a85d
Comment 3 Henri Sivonen (:hsivonen) 2011-11-28 08:40:24 PST
Created attachment 577281 [details] [diff] [review]
Wait for DOMContentLoaded after STATE_STOP
Comment 4 Henri Sivonen (:hsivonen) 2011-11-29 05:21:32 PST
Created attachment 577562 [details] [diff] [review]
Wait for DOMContentLoaded after STATE_STOP, v2
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-02 08:43:50 PST
Why did the view-source rewrite affect this code?
Comment 6 Henri Sivonen (:hsivonen) 2011-12-05 03:52:59 PST
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #5)
> Why did the view-source rewrite affect this code?

When not executing scripts (that could block the parser), the old parser always finished parsing when OnStopRequest for the stream fired. View Source does not execute scripts, so the whole View Source DOM was ready after the parser had processed OnStopRequest (on the main thread).

The new parser always proxies stream data to the parser thread, so it always finishes asynchronously: OnStopRequest sends a runnable to the parser thread and then some time later, the main thread gets another event back that makes the main thread see the finished parse.
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-09 12:32:11 PST
Comment on attachment 577562 [details] [diff] [review]
Wait for DOMContentLoaded after STATE_STOP, v2

You could use this.onContentLoaded.bind(this) instead of the closure.
Comment 8 Henri Sivonen (:hsivonen) 2011-12-12 00:57:14 PST
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #7)
> Comment on attachment 577562 [details] [diff] [review]
> Wait for DOMContentLoaded after STATE_STOP, v2
> 
> You could use this.onContentLoaded.bind(this) instead of the closure.

Thanks. Landed with this change:
https://hg.mozilla.org/mozilla-central/rev/15fb7f28f213
Comment 9 neil@parkwaycc.co.uk 2011-12-12 01:29:09 PST
Doesn't this change break the case when you click on a link in the Error Console when you have an external editor defined?
Comment 10 Alice0775 White 2011-12-12 01:54:24 PST
(In reply to neil@parkwaycc.co.uk from comment #9)
> Doesn't this change break the case when you click on a link in the Error
> Console when you have an external editor defined?

I got an error message when I click link in the Error Console. And nothing happens...

Error: this.webShell is null
Source File: chrome://global/content/viewSourceUtils.js
Line: 193
Comment 11 Henri Sivonen (:hsivonen) 2011-12-13 00:34:08 PST
(In reply to Alice0775 White from comment #10)
> (In reply to neil@parkwaycc.co.uk from comment #9)
> > Doesn't this change break the case when you click on a link in the Error
> > Console when you have an external editor defined?
> 
> I got an error message when I click link in the Error Console. And nothing
> happens...
> 
> Error: this.webShell is null
> Source File: chrome://global/content/viewSourceUtils.js
> Line: 193

Thanks. Sigh. Filed as bug 710142.
Comment 12 Henri Sivonen (:hsivonen) 2011-12-13 05:06:03 PST
Comment on attachment 577562 [details] [diff] [review]
Wait for DOMContentLoaded after STATE_STOP, v2

(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.
Comment 13 Davide Ficano 2011-12-13 05:29:00 PST
Regressions affect also my extension ViewSourceWith, I've already patched it adding the listener and luckly the code works also on previous Firefox versions.

I suspect also other extensions should contain regression...
Comment 14 christian 2011-12-13 14:40:13 PST
Comment on attachment 577562 [details] [diff] [review]
Wait for DOMContentLoaded after STATE_STOP, v2

[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.
Comment 15 Henri Sivonen (:hsivonen) 2011-12-14 01:03:35 PST
Firefox 10 no longer affected due to bug 710175 landing.

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