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

Status: VERIFIED FIXED

(Toolkit :: View Source, defect)

Description

[Gary [:streetwolf52]]

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.

Comment 1

[Gary [:streetwolf52]]

The patch that caused this is https://bugzilla.mozilla.org/show_bug.cgi?id=1165599

Comment 2

[Gary [:streetwolf52]]

I happen to use NotePad++ as my external program.  I suspect other programs will also not format.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 5

[Gary [:streetwolf52]]

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.

Comment 6

[Gary [:streetwolf52]]

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

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: view-source-external-extension

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcc5abe2352a

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Oops, wrong try URL.

Here's the right one:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8748bb546b24

Comment 9

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(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.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: view-source-external-extension

Now using the properties on <browser>.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a731ecbfe666

Comment 12

[Mike Conley (: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?

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(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.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f80d1207c76c

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f80d1207c76c

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: 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.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8664858 [details] [diff] [review]
view-source-external-extension

Fix a regression, taking it.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/14fa7fe64a4a

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6ea3a333b388

Comment 20

[Mohammad Maruf Islam]

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

Comment 21

[Petruta Horea [:phorea], Desktop QA]

Thanks!

Marking as verified based on comment 20.

Comment 22

[Cato Auestad]

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

[Ashickur Rahman]

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.