Closed Bug 172817 Opened 22 years ago Closed 19 years ago

Allow external source viewer/editor

Categories

(Firefox :: Settings UI, enhancement)

2.0 Branch
enhancement
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 2 alpha2

People

(Reporter: djst, Assigned: jason.barnabe)

References

(Depends on 2 open bugs)

Details

(Keywords: conversion, fixed1.8.1)

Attachments

(3 files, 12 obsolete files)

5.05 KB, patch
Details | Diff | Splinter Review
10.75 KB, patch
Details | Diff | Splinter Review
16.19 KB, patch
Details | Diff | Splinter Review
I know this has been filed as a RFE for Mozilla, but I guess this could be
implemented in Phoenix anyway.

It would probably be a pref under the Advanced secion. Let's keep it simple: It
would send the cached filename as an argument to the program you specify. It
could just be a text field in the Preferences section, with the label "External
source viewer (leave blank for built-in viewer)" or something similar.

Why should this be implemented?

The main reason (I think) is that you often want to edit the source directly,
for example when viewing the source of a local html file.

If adding UI is not wanted (which I respect), I could live with this being a
hidden user.js pref. That's pretty much as it is in IE (only they have the
TweakUI app to change this using UI).
I thought we had this reported. not finding so I guess it didn't make it into
bugzilla. This has, however, already been planned. 
Status: UNCONFIRMED → NEW
Ever confirmed: true
*** Bug 174935 has been marked as a duplicate of this bug. ***
Target Milestone: --- → Phoenix0.7
*** Bug 186503 has been marked as a duplicate of this bug. ***
> The main reason (I think) is that you often want to edit
> the source directly, for example when viewing the source
> of a local html file.

I find this very usefull with IE/Notepad to make quick corrections (e.g. typos)
to HTML files on our servers - those which I have read/write access too.  In
this case, its not a "local" file.

Adding my vote for this...
Shouldn't this bug be called "allow external source _editor_" ? A viewer is
already there and it works perfectly as a viewer. It does syntax highlighting
and allows you to view only the selection which is extremely useful. 

What is in Mozilla and Phoenix lacks of is an editor. A way to modify the
current page in an external editor (be it a text or a wysiwyg editor) should
indeed be there (and accessible from the source viewer as well).
benlu@mindless.org:
No, this is is not the case. The viewer currently in place, whilst good for some
people, is not that grand for others: for instance:
1) yes, there is syntax highlighting: the colors are not the ones i am used to,
and i am not comfortable with them.
2) Copying from the source embeds html in whatever i copy! if i am viewing a
section of a page i developed, that is spewed out from a script, or a code
snippet online, i get extra html tags that i don't want. These things would be
acceptable from the browser itself, but from the viewer? no.
3) People who view page sources are, more often than not, using that as a basis
to develop from. Thus a read-only viewer is not appropriate.

I think i speak for many people when i say that i would just like to be able to
choose where the html is piped out to! All it needs is the creation of a
temporary file that can be passed on the commandline to another program. The
process is quite simple, and a must-have for any decent browser. I have even
created a fake "notepad.exe" to enable this with IE. But at least i can hack
into the process with IE.

To the Phoenix team: please don't take this as a dis on your viewer. It does
what it was designed to do, and that's fine. I want more from a viewer / editor,
and the request has been noted as valid (i see target milestone has been set @
0.7... long wait, but at least it's in there)
As for the current builds -- things are really shaping up nicely. Keep up the
good work, guys.
Updating summary. This is what I ment in the first place, the ability to select
any text editor to view and edit the source. Just like IE does it.
Summary: allow external source viewer → allow external source viewer/editor
One comment on the description, and this is probably understood by all anyway,
but just to make sure, we want to be able to view/edit the cached file if it's
external/served and directly open any local ("file://"). I want to edit web
pages on my hard drive, but edit the cache for anything else. For some reason,
Mozilla always opens the cache, even if it is a local file.
Yes, that's what I wanted from the beginning, too.
Any local file (on your hard drive or in the LAN) should be opened directly, all
other files (http://, etc.) should be opened from the cache.
Isn't this what the nice "edit page" menuitem in the File menu is for (in
Mozilla; Phonix has it off).  That could be trivially hooked up to an external
editor instead of Composer...
What about an internal Source EDITOR ? up to now there is only a viewer!!
I think that creating an internal editor would be 
(a) a waste of time -- everyone has their own personal favourite editor, and
nothing can pull a person away from it (just have a gander at the VI/EMACS
debates that fly around  ^_^)
(b) would be more bloat in what is supposed to be a light browser.
I even would go so far as to say that the viewer is superfluous. I still don't
get why the external editor (guaranteed one of the simplest requests) has been
left on hold for so long whilst more complex requests have been processed. Not
that i'm complaining at the development of
phoe^H^H^H^Hfireb^H^H^H^H^Hlite-mozilla-browser-thingy -- it is my favourite
borwser and has been from phoenix 0.1. I just wish that this last niggle would
be resolved -- at the moment, to check the output from one of my asp pages, i
have to cut 'n paste from the editor into a "dumb" text editor like notepad or
VIM (i lyke VIM -- it's crunchy!) to avoid html tags from being transferred from
the viewer to InterDev (in other words, the html code is being represented in
html, and i get the double-escaped code on a cut 'n paste).
At maximum, the request requres two preference variables -- path to the editor
executable, and output directory where the current user has permission to write.
Spawning the expecutable should be a piece of cake, and all we have to do is
pass the name of the dump of the html file on the commandline to the editor. It
really is simple.
Let's not cruft up a beautifu browser with an internal editor. Please!

-d
> the external editor (guaranteed one of the simplest requests)

On Windows and Mac, yes.  On Linux, not even close.
isn't this bug a duplicate of bug #8589, the reason being that mozilla and
firebird will be "merging" or firebird becoming the default browser in the future?
> isn't this bug a duplicate of bug #8589

I'd like to see that Bugzilla reorg happen first.
*** Bug 210517 has been marked as a duplicate of this bug. ***
It looks like this can be accomplished with the mozex extension -
http://mozex.mozdev.org/.
At the risk of being flamed for useless comments, i would like to point out that
the mozex extension does this via an extra sub-menu off the right-click menu: it
doesn't actually direct the browser to use another app as a source editor. This
is no more convenient than saving the file and opening it; ctrl-U also doesn't
obey the mozex directives.
My two cents

In the interests of simplicity it makes sense to eliminate the internal "View
Source" viewer in favor of the
dump-text-to-temp-file-and-fire-up-default-text-editor approach.

Also in the interests of simplicity file:// URL's should be handled this same way

The ability to edit file://-URL-source in place has its appeal but is not really
"View Source" - perhaps a File | Edit command needs to be invented for this
purpose, which starts up the default text editor on the current URL (grayed out
for all but a few protocols)
> In the interests of simplicity it makes sense to eliminate the internal "View
> Source" viewer in favor of the
> dump-text-to-temp-file-and-fire-up-default-text-editor approach.

Simplicity of what, exactly?  Note that there is no "default text editor" on
Linux, so you would have to deal with that somehow.
> Simplicity of what, exactly?

Simplicity of Firebird code.  No reason to reinvent the wheel with a custom
viewer when there are so many good text viewers/editors

> Note that there is no "default text editor" on
> Linux, so you would have to deal with that somehow.

Sure there is - just use the EDITOR environment variable, if it exists.  If it
doesn't, use (insert editor name here and let the flamewar begin! :)
> just use the EDITOR environment variable, if it exists.

It almost never does (in my experience, at least), so we have to take the other
option somewhat seriously...

Finally, there are some things the built-in view-source viewer can do that an
external editor cannot do easily (eg automatically use the character encoding of
the page being viewed).  Something else to keep in mind when deciding to remove it.
There is a good precedent from the UNIX world to use vi unless the user has set
a preferred editor.  emacs fans could set their EDITOR environment variable.  I
think the choice of which editor to use is best left to the OS, and not kept as
a Firebird pref.

The following article is probably old but illustrates my point:

http://unix.about.com/library/glossary/bldef-var-environment.htm

Many Unix applications launch a text editor. Typically, it will be the default
Unix text editor, vi. However, many applications will launch the editor
specified in the EDITOR environment variable if it is set. For example,

Bourne, Korn, Bash and Z-Shell
$ EDITOR="/usr/local/bin/emacs"
$ export EDITOR

C-Shell and TC-Shell
% setenv EDITOR "/usr/local/bin/emacs"

will tell applications to use the emacs text editor instead of vi.
I believe someone should make an extension for Thunderbird that allows one to
edit the page source as in the mozex extension, but the button should be under
the Edit or maybe the File menu.  Like Edit->Page Source.  If Unix users (or
anyone else) does not like this feature, then they shouldn't install the
extension.  The feature would however make it much easier for web designers and
others to view and edit the source code of pages on the internet and especially
on their harddrive.

When someone makes this extension I believe this bug should be marked as resolved.
> There is a good precedent from the UNIX world to use vi

This made sense at the time, when it was installed pretty much everywhere....
nowadays I keep running into Linux installs without it for some reason.
Re: 25
Well, we could do this:

1) Check the EDITOR (or VISUAL ?) environment variable - if it exists, use that
2) If EDITOR isn't set, check if vi exists - if it exists, use vi
3) If EDITOR isn't set and vi doesn't exist:
Have the user browse to the editor they want to use - then, set the EDITOR
environment variable to that path (just for this session?  Perhaps display an
informative message saying they can avoid this wizard by setting EDITOR in their
login script.)  Then all further View Source's during the current login session
should be able to pick up on the EDITOR environment variable.

Could the built-in viewer be easily made an Extension?
that's not enough. you also need to find a terminal emulator.
taking QA contact, sorry about the bugspam
QA Contact: asa → mconnor
mozex does this, so we can port the source for this into FB or use it as a base
to write a new impl.

UI is needed to make this a viable addition, where do we put it?

Example of possible UI

(o)  Use Default Viewer
( )  Use External Application
      [ C:\foo\edit.exe  ] [Browse]

Trying for auto-discovery of a default text editor seems painful enough on
Linux, and some people may want to use a different app for the View Source than
they would in other places.

cc-ing Ben on this.  Ben, is there a specific place you would want to put this
UI in?  I can do the patch for the functionality, but I'm not sure where the UI
would/should go.
Assignee: blake → bugs
I just looked at the mozex source... it redownloads the file via the persistence
object, which will break pretty horribly on pages that are the results of POST
operations.  See the various "View source does not work on POST pages" bugs we
had a few years back.

The persistence object may be able to make use of cache keys now; if so, you may
want to use that... in any case, lots of testing would be needed (esp. for
no-store POST pages).
I also don't know that we want this UI as part of the standard install. Maybe as
part of the developer extension. But even then, I don't know valuable that is
since you generally have the site you're coding on stored locally anyway (or at
least I do, and have the files open in an editor while testing with a browser),
so loading it in an editor from the browser seems like a non-essential component. 
Ben, this is a lot more useful for site maintenance than design.  I had an
internal portal site at IBM that I sometimes needed to update links and do other
tidying things.  Since I also used the site, someone could IM or email and I'd
open the site, verify the problem, and a quick right click and I made the edit.
 Very easy and desirable, but I agree that this belongs more in the Developer
extension, rather than in the core product.

Do we have a tracking bug for developer extension features or should we have a
component added to Bugzilla for this type of thing?
Not soon. 
Target Milestone: Firebird0.7 → Firebird0.9
This bug is the main reason I still have to use Internet Explorer. See
Additional Comment #24 and #32, which express my sentiments exactly.
Keywords: conversion
To echo #34 and before:

I am not a developer, and I use this functionality often with IE. Like many
people, I have remotely virtually hosted sites where I do not have the sources
locally, or at least don't have a complete development environment locally.

I just want to be able to look at a page, see a problem, open the page in an
editor, fix the problem, and save the page back to the server. The lack of this
functionality, (or in my case the kluge I was forced to adopt instead) is IMHO a
barrier to Firebird adoption by people like me who aren't serious developers,
but who are early adopters of technology. My 2 cents. Thanks for listening.

Getting this functionality as an extension would be fine.
EDITOR (should check VISUAL first, BTW, then EDITOR second) is a standard for
specifying an editor to run in the current terminal.  Mozilla and Firebird need
to run an app that brings up its own window (something like gvim or emacs), or
else run something like xterm -e $VISUAL (but how many users like bare xterm? 
Most would prefer a different terminal client, but which one?  Is xterm always
installed everywhere when X is?)

That's not an argument against implementing this feature; just keep in mind that
running $EDITOR would probably break badly in most cases.  Maybe xterm -e
{$VISUAL|$EDITOR|vi} is the best solution, and make it easy for users to specify
something else in their prefs (not as an env variable).

(Is the "default text editor" on windows and mac the app that's assigned to
handle .txt files?)
I think this is now several independent bugs:

172817 (a)
Allow editing-in-place of locally hosted websites

172817 (b)
Allow use of a user-specified text editor instead of the View Source

172817 (c)
Eliminate View Source editor and use a default system text editor instead
> Eliminate View Source editor and use a default system text editor instead

There is no view source editor, thus far... (never should be, imo).
Reword as:
172817 (c)
Eliminate Firebird's View Source viewer and use a default system text editor instead

I believe the consensus is that (a) should be an extension, (b) has a fair
amount of support and (alas) (c) is likely to be rejected.  Which is a pity,
because I personally think the View Source viewer is a reinvention of the wheel.

But perhaps the best thing to do is:

refile (a) (b) and (c)
Close out (a) with a recommendation to interested parties to write an extension
Assign (b) to someone
Make (c) dependent on (b)
It's interesting to find some people couldn't switch to Mozilla/Firebird from MS
IE because they like view-source to use notepad when to me that's exactly the
opposite. One of reasons I don't like about MS IE is that it brings up Notepad
(OK. I can change it to something else) when I want to view source. Needless to
say, I do want to have a choice and I'm all for making it possible for
Mozilla/Firebird users to specify a program of their choice for view-source (and
possibly editing-in-place). 

However, I'm strongly against removing the default internal source viewer. I
know there are some bugs to fix (like 'post' data, not showing the 'raw' data
but 'processed/parsed' data ), but that can't justify removing it altogether.  

What's not been taken into account here is that  the internal source viewer has
about the same ability as Mozilla/Firebird's  web page rendering area when it
comes to rendering non-ASCII text. Although gvim on my Linux box supports a lot
of scripts in many different encodings, it's not yet as capable as
Mozilla/Firebird.  Even if your external editor supports many different
encodings and many different scripts, you still have to set the encoding
manually once it's brough up unless there's a way to specify the 'encoding' in
the command line. Suppose it's possible, you still have a problem because
'character encoding' names are far from standardized so that what Mozilla calls
'A' can be called 'B' by your editor. The internal source viewer doesn't have
any of this problem, which is why I find it extremly inconvenient to use MS IE
that brings up Notepad filled with 'garbage'. 

In summary, I'm all for 'b' in comment #39 but is against  'c' in comment #39.
Some people here just need to realize that a viewer is not an editor.
"Replacing" the source viewer with an external editor is not an option by
comment #22. 

Furthermore, it would make "View selection source" look weird (what to do with
that? Create a new partial text file?)

I think this bug is really about allowing an external editor, then "Edit page"
and not "View source". Both of them exists in IE and in Mozilla anyway (though
Mozilla only allows you to edit a page in Composer). 

Both could be included in a web developer extension, I don't mind as long as
they are not completely removed or strangely mixed.
Blocks: majorbugs
Blocks: 232849
No longer blocks: 232849
Depends on: 232849
Flags: blocking0.9?
is this not a duplicate of http://bugzilla.mozilla.org/show_bug.cgi?id=8589
No, this one is Firefox specific, the one you mention Mozilla Suite specific
Target Milestone: Firefox0.9 → After Firefox 1.0
From the comments in this bug one would get the impression that there is 
nothing that can be used in Linux. How difficult is 'xterm -e less <page 
source>' (or even 'xterm -hold -e more <page source>') as a default viewer? 
 
This is kind of secondary anyway - what's being asked for is a way to specify 
an external viewer or editor, not to replace it, not yet anyway. The best 
suggestion so far is Boris's comment #10. That way it could be hooked up to 
EDITOR or xterm -e vim or (even better) kvim (which actually makes vim sane 
and usable, but I digress). I assume in Windows there's a registry entry for 
HTML editors (doesn't Fx take it over for some strange reason?) Or it could be 
left to the user to decide in the UI somewhere since we need a mozex-like 
helper-app facility built-in anyway. 
 
Once we've implemented a way to specify an external editor we could look into 
specifying the viewer as well (and maybe even removing it altogether...). One 
problem I just thought of is a keyboard shortcut for the editor - Ctrl+E would 
be logical (and consistent with Mozilla) but that opens up the Download 
ManagEr (!) for some reason. Alt+U might be the next best choice. 
(In reply to comment #44)
> I assume in Windows there's a registry entry for HTML editors (doesn't Fx
> take it over for some strange reason?)

I think that's been fixed (after 0.8).

> One problem I just thought of is a keyboard shortcut for the editor - Ctrl+E
> would be logical (and consistent with Mozilla) but that opens up the Download 
> ManagEr (!) for some reason.

DL manager is Ctrl+Y because of dataloss issues (E is between R-Reload and
W-Close).  I'm not sure if that would make it unacceptable for editing source or
not.

There's a bug somewhere to make View Source an optional extension, but I don't
remember it now and don't have time to look.  A search for "source" and reporter
 /.*netdemonz.*/ would return it, I think (if he did file it as I remember).
already targeted for post 1.0 by ben, minusing blocking0.9?
Flags: blocking0.9? → blocking0.9-
Flags: blocking1.0?
feature. -ing. 
Flags: blocking1.0? → blocking1.0-
I'm really failing to see why all this debate is happening about removing the
default internal view source editor.  If some people like it, just leave it
there!  But please, do add an option to view page source with an external
viewer.  I see no reason to use the system default.  Let people specify the
executable they wish to use.  Should be simple to implement.

As for the web page encoding issue, that's strange, because many times when I've
been using the Mozex extension to view source with Mozilla, it has returned
garbage (probably bad encoding or compression).  However when I tried to view
the same page with IE, it opened a local cache in Notepad and displayed fine.  I
feel that if the local (decompressed/decoded?) cache file was opened instead of
re-downloading the page, this should be OK.

Finally, I hope whoever implements this doesn't forget to make Firefox open the
actual page, not cache, when viewing source of a local file.
(In reply to comment #48)
> I'm really failing to see why all this debate is happening about removing the
> default internal view source editor.

Me too. This bug is about allowing an external (text) editor to view the source
instead of the internal source viewer. I am the reporter for this bug and I
don't want it to turn into a debate about the internal viewer. 

If someone wants to remove the internal viewer, file a separate bug for that and
let that bug depend on this one.
Summary: allow external source viewer/editor → Allow external source viewer/editor
*** Bug 250315 has been marked as a duplicate of this bug. ***
I need to see the page source in MY editor.
Why not make this configurable?
IE lets me configure the page source editor (with a reg. hack).

At the LEAST, let me set the tab stops to something other then 8 in YOUR page 
source editor. Then I can finally ditch the outdated IE.

Until then I will be running both.

Thanks for a great browser.

Mark,
SendJunkMailHere@Resultsp.com


The Seamonkey bug for External Program for View Source (ctrl-U) is, as noted 
earlier, bug 8589.
The Seamonkey bug for External Program for Edit Page (ctrl-E) is bug 35268.

Reporter apparently (per comment 49) prefers the first option, but I agree with 
comment 40 and 41 that implementing the second is a more useful feature overall. 
An external program for View|Source is fine, but Edit Page is also useful (more 
useful, I would say), and having an editor set up shouldn't override the useful 
features the internal viewer provides.

The MozEdit extension for FF (http://mozedit.mozdev.org/) apparently provides a 
Edit Page menu capability but (?) hardwires you into using the MozEdit editor.  
(Correct me if I'm wrong -- I haven't installed it.)


Interesting discussion about determining the editor under Linux.  However, I 
would argue for having a FF preference to specify the editor; I'm sure there are 
Linux users who have their preferred text editor set up but still would prefer 
to edit the page with, say, NVU, in response to Edit Page.  The default for this 
pref could certainly be intelligent about initializing to the VISUAL or EDITOR 
setting, and if neither is set -- just disabled the Edit Page option until the 
user sets the pref.

Note that Windows has a "default HTML editor" setting as part of the IE 
installation; it would be best if FF defaulted to that for Edit Page.
I'm curious to know whether OS X has a similar setting.
(See bug 35268 comment 23 for how to locate the default HTML editor.)

If the user somehow has indicated she wants to Edit Page with the program 
specified in the environment (VISUAL or Windows' Default HTML editor or 
whatever), then FF should be able to keep track of when the systemwide setting 
changes.
In Mozilla, Edit Page actually changes the original source. This is not what I
want. I want a page editor to display the actual HTML source not what the editor
thinks it should be. I know the people behind Edit Page put a lot of effort into
it, but if a user doesn't want it then the user should be given a choice of what
he or she can use.
Launchy allows external editors. many are auto detected. if not you can add them
via the launchy.xml file
http://gemal.dk/mozilla/launchy.html
is bug 8589 a duplicate of this one or vice verca?
neither, they're against different apps (Mozilla Suite vs. Firefox)
Would having an external source viewer eliminate the html re-writing that
occurrs with the integrated viewer, or is that filed seperately?
Attached patch first try (obsolete) — Splinter Review
This patch allows the user to define the preference "view_source.path". If it
is not defined, View Source opens normally. If it is defined, it will open the
source in the application at the path provided. If it's a local file, it will
open it normally, otherwise, it uses nsIWebPageDescriptor to grab the source,
outputs a file to the system temp directory, and then opens it.

Issues with this patch:

1. For some reason, removing the call to open viewSource.xul and the two alerts
makes getting the source not work. Ideas?
2. Should the filename sniffing use nsHeaderSniffer?
3. It doesn't work with "View Partial Source". Should it?
4. What happens if the view source path isn't defined, isn't executable, isn't
accessible, etc.?
5. Only tried in on Windows XP.

Feedback most welcome.
Assignee: bugs → jason_barnabe
Status: NEW → ASSIGNED
(In reply to comment #58)
> 3. It doesn't work with "View Partial Source". Should it?

The one implementation of View Partial Source as an IE browser help object that
I've seen used a BHO-specific viewer to show the source (not Notepad or the
like); I wouldn't worry about working View Partial Source into this.
Attached patch second try (obsolete) — Splinter Review
More improvements...

It now uses the contentType to make the temp filename instead of just using the
remote filename.

It displays an alert if the editor path doesn't exist or isn't executable. I'm
guessing this should be localizable.

The previous patch didn't work with extra long files. Now it does.

The problem with nsIWebPageDescriptor continues, but there's a better
workaround now. Instead of still opening the view source window, it just calls
loadPage twice. It also only needs one pointless alert instead of two. I'd
really like some guidance on what's going on here; I think it's the major issue
with this patch.
Attachment #174535 - Attachment is obsolete: true
As far as I can tell, nsIWebPageDescriptor.loadPage does its thing
asynchronously, and there's no way to specify a callback function or anything.
I'm basically stuck  with doing it this way.

Would instead downloading the file using nsIWebBrowserPersist be suitable? I'm
unsure if it grabs the file from the cache or downloads it every time.
try webshell.QueryInterface(Components.interfaces.nsIWebProgress.idl)

personally my house style says that a variable in js should be interCaps, and
only Classes should be InitialCaps. however i'm not a firefox developer. and
you're always free to ignore me, further if you don't want my comments in your
bugs, just tell me and i won't touch them again</>
sorry about the .idl, copy and paste error :o (but you know where to go for the
next step).
(In reply to comment #62)
> try webshell.QueryInterface(Components.interfaces.nsIWebProgress.idl)

I knew about that interface but how would I use that in the case? I found this
example
http://kb.mozillazine.org/Dev_:_Extensions_:_Example_Code_:_Progress_Listeners
which assumes I have a browser element to call addProgressListener with, but I
only have a webshell and no addProgressListener in sight.

> personally my house style says that a variable in js should be interCaps, and
> only Classes should be InitialCaps. however i'm not a firefox developer. and
> you're always free to ignore me, further if you don't want my comments in your
> bugs, just tell me and i won't touch them again</>

I totally agree, but that's how it was in other areas of the code. I plan on
making all caps sane, with the exception of the function name which will match
the style of the other functios in that file.
The webshell can be QI'd to nsIWebProgress (in Moz 1.8 alpha and later), and
that interface provides the addProgressListener method that you want.
(In reply to comment #65)
> The webshell can be QI'd to nsIWebProgress (in Moz 1.8 alpha and later), and
> that interface provides the addProgressListener method that you want.
Ah, so it can. Thanks. The XUL Planet docs on webshell don't mention it (because
it's new?)

Anyway, I set everything to run on NOTIFY_STATE_DOCUMENT but the source I'm
getting is missing all element names, attribute names, values, and linebreaks.
For example, the Firefox Start Page comes up as

<><><==><>Mozilla Firefox Start Page</><><!--
body,td,a,p,.h{font-family:arial,sans-serif;}
body {font-size: small;color: #333;margin: 0 20px 2em 20px;line-height:
140%;background: #fff url(/images/firefox/grgrad.gif) repeat-x;}a:link { color:
#039; }a:visited { color: #666; }a:hover { color: #333; }a:active { color: #000;
}img,table { border: 0;}#sf{ width: 100%; }#frame {width: 564px;margin: 0
auto;}// -->
</><=><!--
function sf(){document.f.q.focus();}
// -->
</></><=><><==><====><><><===></></><><><==><><><====></><==></><><====></><><====></></></></></><><><====><><=><===></><=><==><><><===></><=><==><><=><=></><=>&</><=><><!--
function qs(el) {if (window.RegExp && window.encodeURIComponent) {var
qe=encodeURIComponent(document.f.q.value);if (el.href.indexOf("q=")!=-1)
{el.href=el.href.replace(new RegExp("q=[^&$]*"),"q="+qe);} else
{el.href+="&q="+qe;}}return 1;}
// -->
</><===><><><=><>Web</>&&&&<====>Images</>&&&&<====>Groups</>&&&&<====>News</>&&&&</></></></></></></><===><><><===><===><===><=====><><=>Search:
<====><=> the web</><====><=>pages from
Canada</></><><===></><=><=>&&<=>Advanced
Search</><>&&<=>Preferences</><>&</></><>&</></></></></></></><=><===></><><===></></></></></><><><==><><=><===></><==>&</><=><===></><><===></></></></></><><><===><><><><====>
<><> <===> </><> <=>Welcome to <=>Firefox 1.0</>, the new, easy-to-use browser
from Mozilla.</> </></></> <> <====><> <=><=><=>Firefox Help & Add-ons</></></>
<=><=><=>About Mozilla</></></> <=><=><=>CDs & Merchandise</></></> <=><=><=>Get
Involved</></></> </></> </><><===></></></></></></></></></></>

The same thing happened in my other patches when I only called the load method
once. Again, if I call loadPage twice it all works fine the second time. Could
this be a bug with nsIWebPageDescriptor?
Boris, I saw that you reviewed the patch for bug 40867, which implemented
nsIWebPageDescriptor.loadPage. Can you shed some light into why on first load,
it's outputting the source I posted in comment 66 instead of the real source?
Thanks.
I can tell you why it's getting it "wrong".  I have no idea why it would ever be
"right", however...

The problem I see is that your "grab the text" code is just wrong.  If syntax
highlighting is on, the <pre> can have textnodes not only children, but also
grandchildren (and grand-grandchildren); take a look at the view-source DOM in
DOM inspector.  Is there a reason you're not just getting the .textContent of
each <pre> instead of rolling your own?

That said, this approach in general seems very inefficient (reparses the data,
builds a DOM, just to reserialize to a file, etc).  Darin, perhaps we should
really look into exposing a better api for doing this (getting data from cache)
somehow?  I've never been that fond of the nsIWebPageDescriptor dodge...
(In reply to comment #68)
> I can tell you why it's getting it "wrong".  I have no idea why it would ever be
> "right", however...

I don't know if you read that part, but as I said at the bottom of comment 66,
if I fire loadPage twice, it works "right" the second time.

> Is there a reason you're not just getting the .textContent of
> each <pre> instead of rolling your own?

Because I've never heard of .textContent. I'll certainly look into it now.
> I don't know if you read that part, but as I said at the bottom of comment 66,
> if I fire loadPage twice, it works "right" the second time.

I read that part, yes.  _That_ sounds like a bug in something (presumably
docshell, but I'm not sure)...  Whatever else, the output should be consistent.
 Might be worth filing a separate bug on it.

> Because I've never heard of .textContent. I'll certainly look into it now.

http://www.w3.org/TR/2004/REC-DOM-Level-3-Core-20040407/core.html#Node3-textContent
(In reply to comment #70)
>  Might be worth filing a separate bug on it.
Bug 285828 filed.
(In reply to comment #70)
>
http://www.w3.org/TR/2004/REC-DOM-Level-3-Core-20040407/core.html#Node3-textContent
I tried it, and it works on first load, but it loses the source's whitespace.
http://www.mts.net/~jbarnabe/textContent.html
> I tried it, and it works on first load, but it loses the source's whitespace.

Hmm... that sounds like a bug, though the spec is pretty vague here.  File it,
please, and cc me?

I guess you have to stick with the "roll your own" method for now, then, but it
needs to recurse down the tree.
(In reply to comment #72)
> > I tried it, and it works on first load, but it loses the source's whitespace.
> 
> Hmm... that sounds like a bug, though the spec is pretty vague here.  File it,
> please, and cc me?
Bug 285830 filed.

Thanks for your help.
Attached patch third try (obsolete) — Splinter Review
Once the textContent bug is fixed or the "better api" is made we'll make use of
them, but this is good for now.
Attachment #175262 - Attachment is obsolete: true
Attachment #177442 - Flags: review?(mconnor)
Flags: blocking-aviary1.1?
Attached patch fourth try (obsolete) — Splinter Review
Forgot to do a little clean-up.

As a side note, it's not that fun to e-mail a couple hundred people every time
you make a mistake.
Attachment #177442 - Attachment is obsolete: true
Attachment #177444 - Flags: review?(mconnor)
Attachment #177442 - Flags: review?(mconnor)
Won't that screw up non-ascii text?  You can't send non-ascii (or more
precisely, non-ISO-8859-1, though only ascii is guaranteed to work) data to the
write() method of an nsIOutputStream from JS without it getting mangled.
Comment on attachment 177444 [details] [diff] [review]
fourth try

bz's comment is enough for the r- here.  But I'm going to whack a few other
things about this patch that aren't right.

- don't use alert() in chrome.	If you have to throw a dialog, use the
promptservice.
- in choosing between throwing a error dialog vs. doing something useful... do
something useful.  
In this case, if the path is horked, just use internal view source.  Or, prompt
the user for a new path.  
- the alert in writeNodeTextContent should either be a throw or a dump,
depending on how severe the error case is.  
If its the former, you need to catch the exception.
- if view source with an external editor isn't going to refetch, why would the
internal view Source?  behaviour shouldn't 
differ based solely on viewer.
- ditch the extra spacing after and before brackets.  Follow the existing style
please.

Basically, I'd like to see a patch addressing non-ASCII text properly, that
uses the internal viewer as a fallback.  
As an alternative, giving the user the opportunity to edit the path is
acceptable, but they need to be able to choose, 
from that dialog, to go back to using the internal viewer.  I don't really want
to add UI/localization impact for a hidden 
feature though, so I'd prefer to go with the former for now.

I think we may want to implement two prefs here, actually:

view_source.editor.external (as a boolean).
view_source.editor.path (as a string).

This would allow us to a) let extensions leverage the external editor path even
if the default for view source is to use 
the internal.  Also, if at some point we were to provide UI for this (i.e. if
we ever have a true "developers' package) 
its a lot easier to write the UI without having to munge prefs.
Attachment #177444 - Flags: review?(mconnor) → review-
(In reply to comment #77)
> (From update of attachment 177444 [details] [diff] [review] [edit])
> - if view source with an external editor isn't going to refetch, why would the
> internal view Source?  behaviour shouldn't 
> differ based solely on viewer.
Do you mean that my external viewer code should refetch in case of a failure of
loadPage, just like the internal viewer does here
http://lxr.mozilla.org/seamonkey/source/toolkit/components/viewsource/content/viewSource.js#165
?
It should do that, yes.

I want to see something like this, where openInExternalEditor will return false
on any failure, and true on success.  Also, having an openInExternalEditor
allows other consumers (i.e. extensions) to define an external editor and allow
the choice somehow (i.e. adding a context menu).

I'm also starting to wonder if this has value outside of /browser and whether we
should make this an accessible method from /toolkit.

var loadInExternalEditor = false;
if (getBoolPref("view_source.editor.external", false))
  loadInExternalEditor = openInExternalEditor(args);

if (!loadInExternalEditor)
  openInInternalViewer(args);
In my head, your answer to my question conflicts with the rest of what you
wrote, but maybe that's not you meant. Let me clarify.

If the external editor is defined, do you want

a) Attempt to load without refetching. If it can't load without refetching, then
refetch and open in the external editor. If a failure occurs at any point, then
just use the internal view source. (Refetching is not in itself a failure.)

b) Attempt to load without refetching. If it can't load without refetching, then
pass it on to the internal editor. If any other error occurs, then also pass it
on to the internal editor. (Refetching is in itself a failure.)


Another problem is that loadPage runs asynchronously. Wouldn't I have use a
callback function instead of returning a boolean?
See also bug 271410
Yeah, I wasn't clear enough, sorry.  a) is what I'm looking for.
Attached patch fifth try (obsolete) — Splinter Review
This should address all the issues raised.
Attachment #177444 - Attachment is obsolete: true
Attachment #177802 - Flags: review?(mconnor)
It doesn't address the encoding issue (largely because
nsIScriptableUnicodeConverter is broken-by-design, looks like.....)

Also, please don't use nsHeaderSniffer.  It just needs to go away (and there are
bugs on this).  It's pretty buggy with dynamically generated pages.

Is there a reason you assume all URIs are file:// or http:// ?

And is there a reason you're falling back from LoadPage exceptions to some other
(buggy!) method of fetching the data at all?  The only way LoadPage will throw
is if there is actually a serious error of some sort...

One other thing... was this tested on post results that have expired from cache
(eg clear cache after loading the post page)?  What happens if the user cancels
the ensuing dialog with this patch?
(In reply to comment #84)
> It doesn't address the encoding issue (largely because
> nsIScriptableUnicodeConverter is broken-by-design, looks like.....)
What's broken and how do I fix it?

> Also, please don't use nsHeaderSniffer.
Alternatives?

> Is there a reason you assume all URIs are file:// or http:// ?
I only assume that a URL that came in without a scheme is http. How am I
assuming *everything* is http?

> And is there a reason you're falling back from LoadPage exceptions to some other
> (buggy!) method of fetching the data at all?  The only way LoadPage will throw
> is if there is actually a serious error of some sort...
It's the same thing that happens in the internal viewer.
http://lxr.mozilla.org/seamonkey/source/toolkit/components/viewsource/content/viewSource.js#165

> 
> One other thing... was this tested on post results that have expired from cache
> (eg clear cache after loading the post page)?  What happens if the user cancels
> the ensuing dialog with this patch?
It opens the external editor with an empty file. I'll look into it.
> What's broken and how do I fix it?

It's using "string" when it wants "byte array".... Simplest example is that if
the charset of the page is, say, UTF-16 and the string is ASCII you'll lose all
but the first character.

> Alternatives?

What are you trying to do, exactly?  Just download data to a known location?  If
so, just save it using nsIWebBrowserPersist...  If you're trying to get a
content type, and have a document object, just use the content type on the
document... and if you don't have a document, just guess based on the file
extension in the URI.  The content type returned by nsHeaderSniffer is pretty
unreliable due to server bugs.

> I only assume that a URL that came in without a scheme is http.

You assume something needs http: prepended if:

1)  newURI on the string fails
2)  The URI is not a file:// URI and loadPage() throws.

I'm not sure what kind of strings you're expecting to see in this method, so I'm
not sure how valid the first assuption is.  But the second seems pretty wrong.

> It's the same thing that happens in the internal viewer.

Ah... That's for the case when the caller passes in some bogus thing for the
page descriptor, I guess.  OK.
(In reply to comment #84)

> One other thing... was this tested on post results that have expired from cache
> (eg clear cache after loading the post page)?  What happens if the user cancels
> the ensuing dialog with this patch?

Does this happen much in reality?  One of most useful things about loading
source from cache is that you can see the source of a POST result, re-getting
the source is very likely to get something different.
Attachment #177802 - Flags: review?(mconnor)
(In reply to comment #86)
> It's using "string" when it wants "byte array".... Simplest example is that if
> the charset of the page is, say, UTF-16 and the string is ASCII you'll lose all
> but the first character.
I'll try it out on UTF-16 pages. This patch had fixed things for UTF-8, at least.

> What are you trying to do, exactly?  Just download data to a known location?  If
> so, just save it using nsIWebBrowserPersist...  If you're trying to get a
> content type, and have a document object, just use the content type on the
> document... and if you don't have a document, just guess based on the file
> extension in the URI.  The content type returned by nsHeaderSniffer is pretty
> unreliable due to server bugs.
I was indeed trying to get the content type from the server. I'll drop that part
if header sniffer sucks so bad. But what if there's no extension on the file?
text/html? text/plain? The only reason I think it's important that we get the
right extension is that many editors provide syntax highlighting and stuff for
certain extensions.

> You assume something needs http: prepended if:
> 
> 1)  newURI on the string fails
> 2)  The URI is not a file:// URI and loadPage() throws.
> 
> I'm not sure what kind of strings you're expecting to see in this method, so I'm
> not sure how valid the first assuption is.  But the second seems pretty wrong.
1) handles someone typing in view-source://google.com into the address bar. The
view-source: part is stripped
http://lxr.mozilla.org/seamonkey/source/browser/base/content/browser.js#1677, so
I assume http.
2) is a mistake; I forgot to update it. The first argument is supposed to be uri.

> Ah... That's for the case when the caller passes in some bogus thing for the
> page descriptor, I guess.  OK.
I'm under the impression that loadPage will throw if the page isn't in cache.
Examples would be your post-data-clear-cache scenario and a view-source: URL.
> I'm under the impression that loadPage will throw if the page isn't in cache.

I think the only way loadPage will maybe throw on valid input (other than
obvious things like out of memory conditions, etc) is if all three of the
following are true:

1)  The data is not in the cache
2)  The page is a POST result
3)  The user cancels the dialog

Even then, I'm not sure that you won't just get an error status on your progress
listener and a successful execution of loadPage.
If that's true, in that scenario we have no way of getting the source anyway.
There's nothing useful we can do, so we should either display an error or do
nothing. I'm thinking the latter because the user knows that he cancelled the
action. Mike?
I did some testing... The only time I could get loadPage to throw is if the
document isn't currently loaded (a view-source: URL). Clearing cache or not,
post or get, doesn't matter. So basically I have to keep the saveURI bit there
so view-source: works and figure something out for cache-cleared-post-data.
Boris, if I have a valid page descriptor and I pass it to
nsIWebBrowserPersist.saveURI, does it always save it from cache if it can? If
so, couldn't I just call that in all cases and let it handle itself?
Attached patch sixth try (obsolete) — Splinter Review
This should address all the issues raised. Again :).
Attachment #177802 - Attachment is obsolete: true
Attachment #178210 - Flags: review?(mconnor)
> The only time I could get loadPage to throw is if the document isn't currently
> loaded (a view-source: URL).

It can also throw if a bogus page descriptor is passed in, in general...

> if I have a valid page descriptor and I pass it to nsIWebBrowserPersist.saveURI

That takes a URI (and cache key), not a page descriptor, no?
(In reply to comment #94)
> That takes a URI (and cache key), not a page descriptor, no?
saveURI ends up calling this code
http://lxr.mozilla.org/seamonkey/source/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp#1173
, which says cache keys can be page descriptors sometimes. I'm unsure of the
relationship.

I'm just thinking that if saveURI always tries to load from cache first, and if
that fails, it refetches, then we could just use that in all cases.
> which says cache keys can be page descriptors sometimes

Ugh... I wonder how well that works...

> I'm just thinking that if saveURI always tries to load from cache first

Probably depends on the exact flags you use.  See documentation, or what there
is of it?

If you do decide to use that, make sure to check what happens for POST pages...
(eg, will we end up just doing a GET on the URI if the page is not in cache? 
That's not desirable here.)
Mike, can you review this patch? I'd really like to get this in for 1.1.
Comment on attachment 178210 [details] [diff] [review]
sixth try

I don't have time to do an in-depth review of anything for the next few weeks,
but here's a few things that jumped out in skimming the patch.

I do like the way this is all set up, although a lot of this may belong in
toolkit so other view source consumers can take advantage of this.  Probably a
new file (viewSourceUtils.js?) or somesuch.

>+function BrowserViewSourceOfURL(aUrl, aPageDescriptor, aDocument)
>+{
>+  var prefs = Components.classes["@mozilla.org/preferences-service;1"].getService(nsCI.nsIPrefBranch);
>+  var loadInExternalViewer = prefs.getBoolPref("view_source.editor.external");

just use the getBoolPref helper function for this, since the call can fail, and
we don't need to duplicate code.

var loadInExternalViewer = getBoolPref("view_source.editor.external", false);

>+      if (aPageDescriptor == null) {
>+        // without a page descriptor, loadPage has no chance of working. download the file.
>+        var file = getTemporaryFile(uri, aDocument, contentType);
>+        viewSourceProgressListener.file = file;
>+
>+        var webBrowserPersist = Components.classes["@mozilla.org/embedding/browser/nsWebBrowserPersist;1"].createInstance(nsCI.nsIWebBrowserPersist);
>+        // the default setting is to not decode. we need to decode.
>+        webBrowserPersist.persistFlags = nsCI.nsIWebBrowserPersist.PERSIST_FLAGS_REPLACE_EXISTING_FILES;
>+        webBrowserPersist.progressListener = viewSourceProgressListener;
>+        webBrowserPersist.saveURI(uri, null, null, null, null, file);
>+      } else {
>+        // we'll use nsIWebPageDescriptor to get the source because it may not have to refetch the file from the server
>+        var webShell = Components.classes["@mozilla.org/webshell;1"].createInstance();
>+        viewSourceProgressListener.webShell = webShell;
>+        var progress = webShell.QueryInterface(nsCI.nsIWebProgress);
>+        progress.addProgressListener(viewSourceProgressListener, nsCI.nsIWebProgress.NOTIFY_STATE_DOCUMENT);
>+        var pageLoader = webShell.QueryInterface(nsCI.nsIWebPageDescriptor);    
>+        pageLoader.loadPage(aPageDescriptor, nsCI.nsIWebPageDescriptor.DISPLAY_AS_SOURCE);

there's some really long lines here, please break them properly, I know we're
not at 80 cols, but its a lot higher than those.

>+// Returns nsiProcess of the external view source editor or null

nsIProcess

>+function getExternalViewSourceEditor()
>+{
>+  var editor = null;
>+  var viewSourceAppPath = null;
>+  var prefs = Components.classes["@mozilla.org/preferences-service;1"].getService(nsCI.nsIPrefBranch);
>+  var prefPath = prefs.getCharPref("view_source.editor.path");

this can throw, make sure to catch the failure and return editor (null) at this
point.

somewhere in the patch there's two blank lines in a row, please fix that too :)
this isn't a blocker, but it should make it with months to spare.
Flags: blocking-aviary1.1? → blocking-aviary1.1-
Attachment #178210 - Flags: review?(mconnor)
Mike, are you still "the man" when it comes to getting reviews or is there
someone else who could do it sooner?
I don't know what's on ben's radar, you can try mailing him if you're really
impatient.  The patch is in pretty good shape so he may be able to get to it,
but I know his time is even more limited than mine right now.

If nothing else, I'll have time around mid-April.
Attached patch viewSourceUtils.js (obsolete) — Splinter Review
This goes in /toolkit/components/viewsource/content/
Attachment #179233 - Flags: review?(mconnor)
Attached patch move the code to the toolkit (obsolete) — Splinter Review
Attachment #178210 - Attachment is obsolete: true
Attachment #179234 - Flags: review?(mconnor)
*** Bug 293109 has been marked as a duplicate of this bug. ***
No longer blocks: majorbugs
*** Bug 297150 has been marked as a duplicate of this bug. ***
Is there a need for two prefs?  What about just using view_source.editor.path
and comparing it to "".  Probably goes against what we have elsewhere as far as
prefs.
For the app itself, its not especially useful, although I'm fairly sure the bool
pref is a faster check.

The goal was to provide a pref that can be leveraged by an extension to support
_both_, not an either/or, without having to use an extension branch pref.
Attached patch unbitrotted patch (obsolete) — Splinter Review
Attachment #179234 - Attachment is obsolete: true
Attachment #185980 - Flags: review?(mconnor)
Attached patch viewSourceUtils.js (obsolete) — Splinter Review
The textContent whitespace bug was fixed, so this has been changed to use it.
Attachment #179233 - Attachment is obsolete: true
Attachment #185982 - Flags: review?(mconnor)
Attachment #179233 - Flags: review?(mconnor)
Attachment #179234 - Flags: review?(mconnor)
Jason: I like the looks of this, but one suggestion - perhaps a future
enhancement; personally, I'd like to know if a file has to be re-downloaded.  If
I'm viewing the source of the response to a POST, for example.  Perhaps a dialog
could be popped up informing the user if the page couldn't be located in cache,
and ask whether the user would like to re-download the file or cancel the operation.
If it can't grab a GET without going to the server, there is no prompt, just
like reloading the results of a GET. If it can't grab a POST without going to
the server, it'll prompt you just like it prompts you if you reload the results
of a POST.
Hmm, even repeated requests for non-POSTs can cause a difference in the source.
 It seems to be pretending you just requested the file be displayed again. 
Shame that it can't differentiate between that and viewing source, which isn't
quite the same - really, you're asking for the source of what you're viewing
right now.
See comment #77, showing an error vs. doing something useful.

There are few situations where you would even notice the difference. You'd have
to visit a GET page that loaded differently every time, clear the cache, then
view the source. It might even still work without re-downloading in that
situation, I don't quite remember because I wrote this 4 months ago.

In any case, this is the same behaviour as internal view source.
Attached patch unbitrotted again (obsolete) — Splinter Review
One thing to note: the external editor will only work from the View Source menu
option/keyboard shortcut. It won't get launched from a view-source: URL or
clicking on an exception in the Javascript Console.
Attachment #185980 - Attachment is obsolete: true
Attachment #189726 - Flags: review?(mconnor)
Attached patch viewSourceUtils.js, unbitrotted (obsolete) — Splinter Review
This also has been changed to make use of nsIConverterOutputStream from bug
295047.
Attachment #189727 - Flags: review?(mconnor)
Attachment #185980 - Flags: review?(mconnor)
Attachment #185982 - Attachment is obsolete: true
Attachment #185982 - Flags: review?(mconnor)
Flags: blocking-aviary2.0?
Three things:

1.) This would be perfect:

(o)  Use Default Viewer
( )  Use External Application
      [ C:\foo\edit.exe  ] [Browse]

Where Default Viewer means the one that is part of Firefox now.  This totally
sidesteps the "System" Default Viewer issue that was brought up earlier.  A user
who cares about this issue probably has a very specific external viewer in mind.

2a.) I see that there is a patch, but I don't have the directory you mentioned
to put it in:

/toolkit/components/viewsource/content/

2b.) And would this directory belong in the Firefox install or the user directory?

2c.) And I don't know where to find contentAreaUtils.js which the comments in
the jsp file indicate is necessary.

2d.) What build of Firefox is this supposed to work with?

3.) I have a Windows XP system and a Fedora Core 4 Linux system to test this on.
 I'm willing to spend a an hour or two testing this if I could have a little
help installing it.
(In reply to comment #116)
> 1.) This would be perfect:
> 
> (o)  Use Default Viewer
> ( )  Use External Application
>       [ C:\foo\edit.exe  ] [Browse]

This bug deals with adding the functionality, not the UI, for this feature.

>  I'm willing to spend a an hour or two testing this if I could have a little
> help installing it.

You need to be able to compile your own Firefox. If you need help with that,
e-mail me directly.
See also bug 309321, "Firefox should use OS default text editor for View Source
instead of internal View Source window".
Blocks: 309321
Comment on attachment 189726 [details] [diff] [review]
unbitrotted again

>-function BrowserViewSourceOfURL(url, charset, pageCookie)
>+function BrowserViewSourceOfURL(aURL, aPageDescriptor, aDocument)
> {
>-  // try to open a view-source window while inheriting the charset (if any)
>-  openDialog("chrome://global/content/viewSource.xul",
>-             "_blank",
>-             "scrollbars,resizable,chrome,dialog=no",
>-             url, charset, pageCookie);
>+  if (getBoolPref("view_source.editor.external", false)) {
>+    openInExternalEditor(aURL, aPageDescriptor, aDocument, internalViewerFallback);
>+  }
>+  else {
>+    openInInternalViewer(aURL, aPageDescriptor, aDocument);
>+  }
> }

Let's change the function name here, since we're changing the signature.  Also I'm thinking we want to wrap this all in a gViewSourceUtils prototype like we just did with findbar.  More in the second part of this review.
Attachment #189726 - Flags: review?(mconnor) → review+
Comment on attachment 189727 [details] [diff] [review]
viewSourceUtils.js, unbitrotted

ok, so I think we want to wrap this all like we've done in http://lxr.mozilla.org/mozilla/source/toolkit/components/typeaheadfind/content/findBar.js for the same reasons as we did it there.

>function openInInternalViewer(aUrl, aPageDescriptor, aDocument)
>{
>  // try to open a view-source window while inheriting the charset (if any)
>  var charset = null;
>  if (aDocument != null) {
>    charset = "charset=" + aDocument.characterSet;
>  }

nit: extra brackets.  Also, lets use aURL instead of aUrl.

>
>// aCallBack is a function accepting two arguments - result (true=success) and a data object
>function openInExternalEditor(aUrl, aPageDescriptor, aDocument, aCallBack)
>{
>  var data = {url: aUrl, pageDescriptor: aPageDescriptor, doc: aDocument};
>
>  try {
>    var editor = getExternalViewSourceEditor();    
>    if (editor == null) {
>      if (aCallBack != null) {
>        aCallBack(false, data);
>      }
>      return;
>    }

ok, let's default to using the internal viewer as a callback, and people can explicitly pass null or another callback if they don't want this behaviour.

>    // make a uri
>    var ios = Components.classes["@mozilla.org/network/io-service;1"]
>                        .getService(Components.interfaces.nsIIOService);
>    var charset = aDocument == null ? null : aDocument.characterSet;

var charset = aDocument ? aDocument.characterSet : null;

>    var uri;
>    try {
>      uri = ios.newURI(aUrl, charset, null);
>    } catch (ex) {
>      // this is a view-source: url with the "view-source:" part stripped. let's assume it's http.
>      uri = ios.newURI("http:" + aUrl, charset, null);
>    }
>    data.uri = uri;

shouldn't this insert http:// ?

>    var path;
>    var contentType = aDocument == null ? null : aDocument.contentType;
>    if (uri.scheme == "file") {    
>      // it's a local file; we can open it directly
>      path = uri.QueryInterface(Components.interfaces.nsIFileURL).file.path;
>      this.editor.run(false, [path], 1);
>      if (aCallBack != null) {
>        aCallBack(true, data);
>      }

nit: brackets

>    } else {
>      // set up the progress listener with what we know so far
>      viewSourceProgressListener.editor = editor;
>      viewSourceProgressListener.callBack = aCallBack;
>      viewSourceProgressListener.data = data;      
>      if (aPageDescriptor == null) {
>        // without a page descriptor, loadPage has no chance of working. download the file.
>        var file = getTemporaryFile(uri, aDocument, contentType);
>        viewSourceProgressListener.file = file;
>
>        var webBrowserPersist = Components
>
>                                .classes["@mozilla.org/embedding/browser/nsWebBrowserPersist;1"]
>                                .createInstance(Components.interfaces.nsIWebBrowserPersist);

nit: weird spacing here.  Its probably useful to create a const/var for Components.interfaces.nsIWebBrowserPersist and friends


>  onStateChange: function(aProgress, aRequest, aFlag, aStatus) {
>    if ((aFlag & Components.interfaces.nsIWebProgressListener.STATE_STOP) && aStatus == 0) {
>      try {
>        if (this.file == null) {
>          // it's not saved to file yet, it's in the webshell
>          
>          // get a temporary filename
>          this.file = getTemporaryFile(this.data.uri, this.data.doc, this.data.doc.contentType);
>  
>          // we have to convert from the source charset.
>          var webNavigation = this.webShell.QueryInterface(nsIWebNavigation);
>          var foStream = Components.classes["@mozilla.org/network/file-output-stream;1"]
>                                   .createInstance(Components.interfaces.nsIFileOutputStream);
>          foStream.init(this.file, 0x02 | 0x08 | 0x20, 0664, 0); // write | create | truncate                                   

trailing whitespace, can we also get a more verbose comment here explaining where the values are coming from, not just what they are?


>        if (this.callBack != null) {
>          this.callBack(true, this.data);
>        }

nit: brackets

r=me, pending rearch as an object (gViewSourceUtils) so we don't collide with extensions nearly as easily.
Attachment #189727 - Flags: review?(mconnor) → review+
I think all this code should have been placed in an attachment instead of a comment.  It makes the message list very difficult to read through, especially when the reader has absolutely no idea what it means!

PS. External source viewer: a MUST have for developers.  Vote!

> (From update of attachment 189727 [details] [diff] [review] [edit])
> ok, so I think we want to wrap this all like we've done in
> ...
Flags: blocking-aviary2? → blocking-aviary2+
Flags: blocking0.9-
Flags: blocking-aviary1.5-
Flags: blocking-aviary1.0-
> >    var uri;
> >    try {
> >      uri = ios.newURI(aUrl, charset, null);
> >    } catch (ex) {
> >      // this is a view-source: url with the "view-source:" part stripped. let's assume it's http.
> >      uri = ios.newURI("http:" + aUrl, charset, null);
> >    }
> >    data.uri = uri;
> 
> shouldn't this insert http:// ?
Actually, bug 262915 made it so the view-source: protocol always uses the current window, so this code never gets hit. I've removed it.

Other than that, all changes you've asked for have been made in the upcoming patches.
Attachment #189726 - Attachment is obsolete: true
This goes in toolkit/components/viewsource/content .
Attachment #189727 - Attachment is obsolete: true
mozilla/browser/app/profile/firefox.js; new revision: 1.90;
mozilla/browser/base/content/browser.js; new revision: 1.532;
mozilla/browser/base/content/global-scripts.inc; new revision: 1.4;
mozilla/toolkit/components/viewsource/jar.mn; new revision: 1.8;
mozilla/toolkit/components/viewsource/content/viewSourceUtils.js; initial revision: 1.1
Target Milestone: Future → Firefox1.6-
Version: unspecified → Trunk
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
am I reading this right that the temporary files are never deleted? might want to consider using http://lxr.mozilla.org/seamonkey/source/uriloader/exthandler/nsIExternalHelperAppService.idl#82 (I guess nsIDownloader would delete the file too early)

 
as for the prepending of http to the string, nsIURIFixup might be a more general solution, although it probably doesn't matter much.
(In reply to comment #126)
> am I reading this right that the temporary files are never deleted?
Never deleted by us. Since it's in the system temp directory, the OS will delete them whenever it sees fit.

> as for the prepending of http to the string
As I said, the patch for bug 262915 removed the only case we would have had to do this.
what if the OS doesn't? (e.g. FC4, Windows)
(In reply to comment #128)
> what if the OS doesn't?
Then the file will stay there forever. If you think this is a problem, feel free to file a new bug.
Jason, why is this using getService() to create the nsIProcess _instance_?  That should be a createInstance, no?
(In reply to comment #131)
> Jason, why is this using getService() to create the nsIProcess _instance_? 
> That should be a createInstance, no?

I see you've filed bug 318073, so we'll talk there.
How can access to cache using nsIWebPageDescriptor to save DOM document?
Actually the 'Save as Web Page, complete' refetches from net bypassing cache.
Should be useful (and complete) to allow to save also the DOM representation.
I've tried to use the document object queried from Components.interfaces.nsIWebNavigation but the result is wrong.
So if I read correctly this does not work on linux at all? That sucks.

Would be nice if I could use /usr/bin/gedit ;)

This should work fine on all platforms.
Perhaps this isn't the best place to ask this but I haven't been getting much joy anywhere else; when/how is the UI for this going to be implemented in FF?  I downloaded the latest Deer Park nightly and whilst the implementation works great when I set the prefs correctly, there doesn't seem to be a UI for it.
There's no current plan to implement UI for this.
Isn't that a bit ridiculous?  A popular feature that's been worked **** and implemented well, and yet the large majority of people who might want to use it won't know about it, only those who research what prefs to change.  And adding the UI to the options dialog would be relatively simple.
Firefox is about not having every single thing on the preferences menu.  If you want a gui, use an extension.
Status: RESOLVED → VERIFIED
I don't think it's ridiculous at all.  The standard for inclusion in the primary UI has always been based on utility to the general userbase, not how hard someone worked on it.  I don't think the vast majority of our userbase doesn't care about editing source.  If we had a cohesive set of developer tools, this would fit right in.
I think should be easy to add a simple configuration interface like my ViewSourceWith.
Maybe a more simple UI from which select only one editor.
(In reply to comment #140)
> I don't think it's ridiculous at all.  The standard for inclusion in the
> primary UI has always been based on utility to the general userbase, not how
> hard someone worked on it.

And I've never understood that standard.  Why on earth can't you hide stuff like this away in an 'Advanced | Super advanced' section or something, the general userbase would never go there.  I know you'll say that that's equivalent to about:config, but it's not; hand-editing prefs is often more complicated and time-consuming.
Isn't that extension re-inventing the wheel?  It seems to be implementing its own custom source viewing function instead of making use of this well-implemented patch.  I suspect it also has problems that other similar extensions have had, such as re-downloading webpages instead of using cache.
Attachment #189726 - Flags: branch-1.8.1?(mconnor)
Attachment #189727 - Flags: branch-1.8.1?(mconnor)
Comment on attachment 189726 [details] [diff] [review]
unbitrotted again

hmm, so this patch, as written, breaks the pseudo-API because it changes the args to BrowserViewSourceOfURL.  Can you rename that function and provide a wrapper to support the old function, please?  (Ugly, yes, but we're trying to stick to our own rules here)
Attachment #189726 - Flags: approval-branch-1.8.1?(mconnor) → approval-branch-1.8.1-
Attachment #189727 - Flags: approval-branch-1.8.1?(mconnor)
Mike,

Why did you ever approve the patch if it's unacceptable?
(In reply to comment #146)
> Why did you ever approve the patch if it's unacceptable?

It's only "unacceptable" for the 1.8 branch, where API stability is important.

(In reply to comment #145)
> Can you rename that function and provide a
> wrapper to support the old function, please?

I don't think I can. The new function requires a document and that's not one of the arguments to the old function. How about leaving the old function as is (always open internal View Source)?
(In reply to comment #148)
> I don't think I can. The new function requires a document and that's not one of
> the arguments to the old function. How about leaving the old function as is
> (always open internal View Source)?

Can you make it an optional parameter by placing it last in the param list? And then make it behave as it did before if called the old way?
Comment on attachment 189726 [details] [diff] [review]
unbitrotted again

Given how much other churn there is in browser.js now, I think we're safe to not jump through hoops here
Attachment #189726 - Flags: approval-branch-1.8.1- → approval-branch-1.8.1+
Depends on: 318073
(In reply to comment #135)
> This should work fine on all platforms.

That turns out not to be the case: bug 322865 and bug 307463 mean it doesn't work at all for Macs.
Depends on: 307463, 322865
Attached patch Branch rollupSplinter Review
Includes the followups from bug 318073 and bug 319006.
(In reply to comment #152)
> Created an attachment (id=220026) [edit]
> Branch rollup

Landed this on the 1.8 branch.
Keywords: fixed1.8.1
Target Milestone: Firefox 2 → Firefox 2 alpha2
Version: Trunk → 2.0 Branch
I hate to say it, but I think there's a problem with the patch.  I think the bug needs to be reopened.

I've juut come across a webpage, http://forums.cpanel.net/showthread.php?t=52311&highlight=pureftp

... that, when I go for View source, it opens in the internal viewer.  I have my FF beta setup to view with notepad, and it does for every other page i've tried it with, but it consistently uses the internal viewer for that one.  It does display the source, so it's getting the content, but not supplying it to the correct viewer.  :-\
Those forums do set a no-cache pragma for their pages, but surely FF should just download the content and then paste it into whatever viewer the user has set up rather than the internal one?
(In reply to comment #154)
> I hate to say it, but I think there's a problem with the patch.

Please file a new bug.
Why?  It seems to be entirely a problem with this patch.
CC me on the new bug you file, too.
(In reply to comment #157)
> Why?  It seems to be entirely a problem with this patch.

Because each bug should be about a specific issue. "external view source editor doesn't work on this page" is not the same issue as "implement external view source editor".
sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs,
filter on "beltznerLovesGoats" to get rid of this mass change
QA Contact: mconnor → preferences
You need to log in before you can comment on or make changes to this bug.