Closed Bug 1204365 Opened 5 years ago Closed 5 years ago

External source editor does not format file correctly (view_source.editor.external).

Categories

(Toolkit :: View Source, defect)

41 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla44
Tracking Status
firefox40 --- unaffected
firefox41 --- wontfix
firefox42 --- verified
firefox43 --- verified
firefox44 --- verified

People

(Reporter: streetwolf52, Assigned: jryans)

References

Details

(Keywords: regression, Whiteboard: [testday-20151016])

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20150913103008

Steps to reproduce:

Set user_pref("view_source.editor.external", true);
Set user_pref("view_source.editor.path", "Some external program"); 

Use the context menu and select 'View Page Source'


Actual results:

The resulting file often has no file extension therefore the editor does not recognize the source and does not apply any formatting to it.


Expected results:

Files created from the 'View Page Source' command should have a proper file extension.

Some pages do get an extension.  It appears that the source is not being interpreted when written to my temp folder.
The patch that caused this is https://bugzilla.mozilla.org/show_bug.cgi?id=1165599
Component: Untriaged → View Source
Depends on: 1165599
Keywords: regression
Product: Firefox → Toolkit
I happen to use NotePad++ as my external program.  I suspect other programs will also not format.
Summary: view_source.editor.external → External source editor does not format file correctly.
Summary: External source editor does not format file correctly. → External source editor does not format file correctly (view_source.editor.external).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(jryans)
Version: 43 Branch → 41 Branch
I'll investigate the complexity in restoring the file extension here.  It might be too late to fix in 41, but we'll see.

I think it's more accurate to say this is another regression from bug 1025146, which broke external view source entirely.  In bug 1165599, basic support for external view source was restored.
Assignee: nobody → jryans
Blocks: 1025146
No longer blocks: 1165599
Status: NEW → ASSIGNED
Flags: needinfo?(jryans)
Just to confirm, is the problem only about HTML URLs?  You can also view source on JS and CSS files (even though it's a bit redundant since you already see the source in the browser), and these appear to have the correct extensions for me.

Also, could you provide some of the URLs you were trying?  This way I can check them in my own testing.
Flags: needinfo?(garyshap)
http://www.softexia.com/  Displayed in Notepad++ as a plain text file, no formatting.
http://www.weather.com/weather/today/l/07746:4:US  Same.
about:support  Same.

http://forums.newsbin.com/viewforum.php?f=44  Formatted and extension appears to be .php
http://forums.mozillazine.org/viewforum.php?f=23  Same.
http://www.freeware-guide.com/html/updates.html  Formatted and extension is html.
Flags: needinfo?(garyshap)
http://www.cnn.com/  Displays as US<BS followed by 16 NUL.   No extension assumed to be a txt file.
Comment on attachment 8662158 [details] [diff] [review]
view-source-external-extension

Review of attachment 8662158 [details] [diff] [review]:
-----------------------------------------------------------------

The data you're trying to get is actually already available on the <xul:browser> node (both remote and non-remote support this), as follows:

browser.characterSet
browser.documentContentType
browser.contentTitle

And you can use PrivateBrowsingUtils.isBrowserPrivate(browser) to get the last bit.

Which means that this method can probably safely be made synchronous.
Attachment #8662158 - Flags: review?(mconley) → review-
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #9)
> The data you're trying to get is actually already available on the
> <xul:browser> node (both remote and non-remote support this), as follows:
> 
> browser.characterSet
> browser.documentContentType
> browser.contentTitle
> 
> And you can use PrivateBrowsingUtils.isBrowserPrivate(browser) to get the
> last bit.
> 
> Which means that this method can probably safely be made synchronous.

That does seem quite a bit simpler! :) I'll make another attempt with this.
Attached patch view-source-external-extension (obsolete) — Splinter Review
Now using the properties on <browser>.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a731ecbfe666
Attachment #8662158 - Attachment is obsolete: true
Attachment #8662665 - Flags: review?(mconley)
Comment on attachment 8662665 [details] [diff] [review]
view-source-external-extension

Review of attachment 8662665 [details] [diff] [review]:
-----------------------------------------------------------------

The code looks good! I'll admit, however, that I haven't tested this. Assuming you've tested this yourself, and the tests pass, r=me. Thanks!

::: toolkit/components/viewsource/content/viewSourceUtils.js
@@ +246,5 @@
>        };
> +      if (aDocument) {
> +        try {
> +          data.isPrivate =
> +            aDocument.defaultView

We could probably just do:

data.isPrivate = PrivateBrowsingUtils.isWindowPrivate(aDocument.defaultView);

Can I ask what the try/catch is for? What case is this to handle?
Attachment #8662665 - Flags: review?(mconley) → review+
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #12)
> Comment on attachment 8662665 [details] [diff] [review]
> view-source-external-extension
> 
> Review of attachment 8662665 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The code looks good! I'll admit, however, that I haven't tested this.
> Assuming you've tested this yourself, and the tests pass, r=me. Thanks!

Yes, I've done some tests locally and does recover the file name and extension correctly.

> ::: toolkit/components/viewsource/content/viewSourceUtils.js
> @@ +246,5 @@
> >        };
> > +      if (aDocument) {
> > +        try {
> > +          data.isPrivate =
> > +            aDocument.defaultView
> 
> We could probably just do:
> 
> data.isPrivate = PrivateBrowsingUtils.isWindowPrivate(aDocument.defaultView);

Ah yes, I'll do that.

> Can I ask what the try/catch is for? What case is this to handle?

No idea, I was just moving a block from another place in the file.  But I've noticed the same calls were made elsewhere without the try / catch, so it seems redundant to me.  I'll remove it.
https://hg.mozilla.org/mozilla-central/rev/f80d1207c76c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Approval Request Comment
[Feature/regressing bug #]: Bug 1025146 which added some view source support for e10s
[User impact if declined]: Some pages will have no file extension if you use external view source (which opens in a program you choose), causing the content to possibly be misunderstood by your editor.  This feature is not on by default, several prefs are required to use it at all.
[Describe test coverage new/current, TreeHerder]: On m-c, tested locally.
[Risks and why]: Low, non-default feature.
[String/UUID change made/needed]: None.
Attachment #8662665 - Attachment is obsolete: true
Attachment #8664858 - Flags: review+
Attachment #8664858 - Flags: approval-mozilla-beta?
Attachment #8664858 - Flags: approval-mozilla-aurora?
Comment on attachment 8664858 [details] [diff] [review]
view-source-external-extension

Fix a regression, taking it.
Attachment #8664858 - Flags: approval-mozilla-beta?
Attachment #8664858 - Flags: approval-mozilla-beta+
Attachment #8664858 - Flags: approval-mozilla-aurora?
Attachment #8664858 - Flags: approval-mozilla-aurora+
QA Whiteboard: [good first verify]
I have reproduced this bug with Firefox Nightly 43.0a1 (Build ID:20150913030204) on windows 8.1 64-bit with the instructions from comment 0 .

Verified as fixed with Firefox Nightly 44.0a1(Build ID:20151015030233)

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0
Whiteboard: [testday-20151016]
Thanks!

Marking as verified based on comment 20.
I have not been able to reproduce this bug on Firefox 42 Beta. I tried all the URLs listed in this task and all opened in Sublime Text on Windows with a proper extension.

Verified as fixed on Firefox Beta 42.0 (Build ID: 20151026170526)

Mozilla/5.0 (Windows NT 10.0; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0

(First QA so no rights to set status)
I have successfully reproduce the bug in Firefox : 43.0a1; Build ID : 20150913030204; User Agent : Mozilla/5.0 (X11; Linux i686; rv:43.0) Gecko/20100101 Firefox/43.0 .

The fix works for me in Firefox : 43.0b1; Build ID :20151103023037; User Agent : Mozilla/5.0 (X11; Linux i686; rv:43.0) Gecko/20100101 Firefox/43.0. 

The fix also works for me in Firefox : 42.0; Build ID :20151029151421; User Agent : Mozilla/5.0 (X11; Linux i686; rv:42.0) Gecko/20100101 Firefox/42.0


Based on comment 20 , I am marking this bug as verified.
Status: RESOLVED → VERIFIED
QA Whiteboard: [good first verify] → [good first verify] [bugday-20151104]
You need to log in before you can comment on or make changes to this bug.