Closed Bug 710142 Opened 12 years ago Closed 12 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)

defect
Not set
normal

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
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: nobody → hsivonen
Status: NEW → ASSIGNED
Attachment #581326 - Flags: review?(gavin.sharp)
How did your patch in bug 700034 break this?
(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 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.
Hrm, or is this.file always set in that case? This code is confusing.
(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.
(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 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+
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.
https://hg.mozilla.org/mozilla-central/rev/687289854e56
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.