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

VERIFIED FIXED in Firefox 42

Status

()

VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: streetwolf, Assigned: jryans)

Tracking

({regression})

41 Branch
mozilla44
regression
Points:
---

Firefox Tracking Flags

(firefox40 unaffected, firefox41 wontfix, firefox42 verified, firefox43 verified, firefox44 verified)

Details

(Whiteboard: [testday-20151016])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
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
(Reporter)

Comment 2

3 years ago
I happen to use NotePad++ as my external program.  I suspect other programs will also not format.
(Reporter)

Updated

3 years ago
Summary: view_source.editor.external → External source editor does not format file correctly.
(Reporter)

Updated

3 years ago
Summary: External source editor does not format file correctly. → External source editor does not format file correctly (view_source.editor.external).
Status: UNCONFIRMED → NEW
status-firefox40: --- → unaffected
status-firefox41: --- → affected
status-firefox42: --- → affected
status-firefox43: --- → affected
Ever confirmed: true
Flags: needinfo?(jryans)
Version: 43 Branch → 41 Branch
(Assignee)

Comment 3

3 years ago
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)
(Assignee)

Comment 4

3 years ago
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)
(Reporter)

Comment 5

3 years ago
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)
(Reporter)

Comment 6

3 years ago
http://www.cnn.com/  Displays as US<BS followed by 16 NUL.   No extension assumed to be a txt file.
(Assignee)

Comment 7

3 years ago
Created attachment 8662158 [details] [diff] [review]
view-source-external-extension

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcc5abe2352a
Attachment #8662158 - Flags: review?(mconley)
(Assignee)

Comment 8

3 years ago
Oops, wrong try URL.

Here's the right one:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8748bb546b24
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-
(Assignee)

Comment 10

3 years ago
(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.
(Assignee)

Comment 11

3 years ago
Created attachment 8662665 [details] [diff] [review]
view-source-external-extension

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+
(Assignee)

Comment 13

3 years ago
(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
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
(Assignee)

Updated

3 years ago
status-firefox41: affected → wontfix
(Assignee)

Comment 16

3 years ago
Created attachment 8664858 [details] [diff] [review]
view-source-external-extension

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.
status-firefox44: fixed → verified

Comment 22

3 years ago
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)

Comment 23

3 years ago
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]
status-firefox42: fixed → verified
status-firefox43: fixed → verified
You need to log in before you can comment on or make changes to this bug.