Closed
Bug 710142
Opened 13 years ago
Closed 13 years ago
Following line-numbered links from the Error Console doesn't work with an external View Source viewer/editor
Categories
(Toolkit :: View Source, defect)
Toolkit
View Source
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: hsivonen, Assigned: hsivonen)
References
Details
Attachments
(1 file)
Follow-up from bug 700034: (In reply to Alice0775 White from bug 700034 comment 10) > (In reply to neil@parkwaycc.co.uk from bug 700034 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
Assignee | ||
Comment 1•13 years ago
|
||
While fixing what I broke, I also fixed the file: URL case that was broken and the destroy function which already had a null dereference problem with this.webShell.
Assignee | ||
Comment 3•13 years ago
|
||
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #2) > How did your patch in bug 700034 break this? I failed to realize that the nsIWebProgressListener implementation was used both for listening to nsIDocShell progress in some cases and for listening nsIWebBrowserPersist progress in others. In the latter case, this.webShell is null, which I didn't take into account.
Comment 4•13 years ago
|
||
Comment on attachment 581326 [details] [diff] [review] Fix the nsIWebBrowserPersist case that I broke and the file: URL case that was already broken >diff --git a/toolkit/components/viewsource/content/viewSourceUtils.js b/toolkit/components/viewsource/content/viewSourceUtils.js > onStateChange: function(aProgress, aRequest, aFlag, aStatus) { > // once it's done loading... > if ((aFlag & this.mnsIWebProgressListener.STATE_STOP) && aStatus == 0) { >+ if (!this.webShell) { >+ // We aren't waiting for the parser. Instead, we are waiting for >+ // an nsIWebBrowserPersist. >+ this.onContentLoaded(); >+ return 0; >+ } Shouldn't you just call gViewSourceUtils.handleCallBack(this.callBack, false, this.data); directly? AFAICT onContentLoaded() is just going to throw when it tries to access this.webShell without a null check, and relying on the exception for control flow is just gross.
Assignee | ||
Comment 6•13 years ago
|
||
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #4) > Shouldn't you just call gViewSourceUtils.handleCallBack(this.callBack, > false, this.data); directly? AFAICT onContentLoaded() is just going to throw > when it tries to access this.webShell without a null check, and relying on > the exception for control flow is just gross. It's gross, yeah, but I'm only restoring the flow that was there before I touched the file in the first place. Compare with http://hg.mozilla.org/mozilla-central/file/2cc5ad2cf917/toolkit/components/viewsource/content/viewSourceUtils.js Since I'm not really familiar with this code, I'm afraid to change it more.
Assignee | ||
Comment 7•13 years ago
|
||
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #5) > Hrm, or is this.file always set in that case? This code is confusing. It seems that this.file is set when this.webShell is null.
Comment 8•13 years ago
|
||
Comment on attachment 581326 [details] [diff] [review] Fix the nsIWebBrowserPersist case that I broke and the file: URL case that was already broken Can you make sure we get test coverage for these code paths? I think you should file a followup to refactor this code (or at the very least add some comments); it's very confusing to have these two types of transfers using similar code with slight differences.
Attachment #581326 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 9•13 years ago
|
||
Thanks for the r+. Landed with added comment (see below): https://hg.mozilla.org/integration/mozilla-inbound/rev/687289854e56 (In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #8) > Can you make sure we get test coverage for these code paths? I'm not sure how to do this, since there's a dependency on an external executable. Filed bug 712227. > I think you should file a followup to refactor this code Filed bug 712229. > (or at the very least add some comments); I added a comment when landing.
Comment 10•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/687289854e56
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in
before you can comment on or make changes to this bug.
Description
•