Closed
Bug 172817
Opened 22 years ago
Closed 19 years ago
Allow external source viewer/editor
Categories
(Firefox :: Settings UI, enhancement)
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).
Comment 1•22 years ago
|
||
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
Reporter | ||
Comment 2•22 years ago
|
||
*** Bug 174935 has been marked as a duplicate of this bug. ***
Updated•22 years ago
|
Target Milestone: --- → Phoenix0.7
Reporter | ||
Comment 3•22 years ago
|
||
*** 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.
Reporter | ||
Comment 7•21 years ago
|
||
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
Comment 8•21 years ago
|
||
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.
Reporter | ||
Comment 9•21 years ago
|
||
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.
Comment 10•21 years ago
|
||
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...
Comment 11•21 years ago
|
||
What about an internal Source EDITOR ? up to now there is only a viewer!!
Comment 12•21 years ago
|
||
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
Comment 13•21 years ago
|
||
> the external editor (guaranteed one of the simplest requests)
On Windows and Mac, yes. On Linux, not even close.
Comment 14•21 years ago
|
||
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?
Reporter | ||
Comment 15•21 years ago
|
||
> isn't this bug a duplicate of bug #8589
I'd like to see that Bugzilla reorg happen first.
Comment 16•21 years ago
|
||
*** Bug 210517 has been marked as a duplicate of this bug. ***
Comment 17•21 years ago
|
||
It looks like this can be accomplished with the mozex extension - http://mozex.mozdev.org/.
Comment 18•21 years ago
|
||
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.
Comment 19•21 years ago
|
||
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)
Comment 20•21 years ago
|
||
> 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.
Comment 21•21 years ago
|
||
> 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! :)
Comment 22•21 years ago
|
||
> 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.
Comment 23•21 years ago
|
||
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.
Comment 24•21 years ago
|
||
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.
Comment 25•21 years ago
|
||
> 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.
Comment 26•21 years ago
|
||
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?
Comment 27•21 years ago
|
||
that's not enough. you also need to find a terminal emulator.
Comment 29•21 years ago
|
||
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
Comment 30•21 years ago
|
||
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).
Comment 31•21 years ago
|
||
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.
Comment 32•21 years ago
|
||
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?
Comment 34•21 years ago
|
||
This bug is the main reason I still have to use Internet Explorer. See Additional Comment #24 and #32, which express my sentiments exactly.
Updated•21 years ago
|
Keywords: conversion
Comment 35•21 years ago
|
||
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.
Comment 36•21 years ago
|
||
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?)
Comment 37•21 years ago
|
||
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
Comment 38•21 years ago
|
||
> Eliminate View Source editor and use a default system text editor instead
There is no view source editor, thus far... (never should be, imo).
Comment 39•21 years ago
|
||
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)
Comment 40•21 years ago
|
||
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.
Comment 41•21 years ago
|
||
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.
Comment 42•20 years ago
|
||
is this not a duplicate of http://bugzilla.mozilla.org/show_bug.cgi?id=8589
Comment 43•20 years ago
|
||
No, this one is Firefox specific, the one you mention Mozilla Suite specific
Updated•20 years ago
|
Target Milestone: Firefox0.9 → After Firefox 1.0
Comment 44•20 years ago
|
||
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.
Comment 45•20 years ago
|
||
(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).
Comment 46•20 years ago
|
||
already targeted for post 1.0 by ben, minusing blocking0.9?
Flags: blocking0.9? → blocking0.9-
Comment 48•20 years ago
|
||
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.
Reporter | ||
Comment 49•20 years ago
|
||
(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
Comment 50•20 years ago
|
||
*** Bug 250315 has been marked as a duplicate of this bug. ***
Comment 51•20 years ago
|
||
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
Comment 52•20 years ago
|
||
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.
Comment 53•20 years ago
|
||
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.
Comment 54•20 years ago
|
||
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
Comment 55•19 years ago
|
||
is bug 8589 a duplicate of this one or vice verca?
Comment 56•19 years ago
|
||
neither, they're against different apps (Mozilla Suite vs. Firefox)
Comment 57•19 years ago
|
||
Would having an external source viewer eliminate the html re-writing that occurrs with the integrated viewer, or is that filed seperately?
Assignee | ||
Comment 58•19 years ago
|
||
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
Comment 59•19 years ago
|
||
(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.
Assignee | ||
Comment 60•19 years ago
|
||
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
Assignee | ||
Comment 61•19 years ago
|
||
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.
Comment 62•19 years ago
|
||
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</>
Comment 63•19 years ago
|
||
sorry about the .idl, copy and paste error :o (but you know where to go for the next step).
Assignee | ||
Comment 64•19 years ago
|
||
(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.
Comment 65•19 years ago
|
||
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.
Assignee | ||
Comment 66•19 years ago
|
||
(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?
Assignee | ||
Comment 67•19 years ago
|
||
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.
Comment 68•19 years ago
|
||
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...
Assignee | ||
Comment 69•19 years ago
|
||
(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.
Comment 70•19 years ago
|
||
> 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
Assignee | ||
Comment 71•19 years ago
|
||
(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
Comment 72•19 years ago
|
||
> 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.
Assignee | ||
Comment 73•19 years ago
|
||
(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.
Assignee | ||
Comment 74•19 years ago
|
||
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)
Assignee | ||
Updated•19 years ago
|
Flags: blocking-aviary1.1?
Assignee | ||
Comment 75•19 years ago
|
||
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)
Updated•19 years ago
|
Attachment #177442 -
Flags: review?(mconnor)
Comment 76•19 years ago
|
||
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 77•19 years ago
|
||
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-
Assignee | ||
Comment 78•19 years ago
|
||
(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 ?
Comment 79•19 years ago
|
||
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);
Assignee | ||
Comment 80•19 years ago
|
||
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?
Comment 81•19 years ago
|
||
See also bug 271410
Comment 82•19 years ago
|
||
Yeah, I wasn't clear enough, sorry. a) is what I'm looking for.
Assignee | ||
Comment 83•19 years ago
|
||
This should address all the issues raised.
Attachment #177444 -
Attachment is obsolete: true
Attachment #177802 -
Flags: review?(mconnor)
Comment 84•19 years ago
|
||
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?
Assignee | ||
Comment 85•19 years ago
|
||
(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.
Comment 86•19 years ago
|
||
> 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.
Comment 87•19 years ago
|
||
(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.
Assignee | ||
Updated•19 years ago
|
Attachment #177802 -
Flags: review?(mconnor)
Assignee | ||
Comment 88•19 years ago
|
||
(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.
Comment 89•19 years ago
|
||
> 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.
Assignee | ||
Comment 90•19 years ago
|
||
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?
Assignee | ||
Comment 91•19 years ago
|
||
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.
Assignee | ||
Comment 92•19 years ago
|
||
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?
Assignee | ||
Comment 93•19 years ago
|
||
This should address all the issues raised. Again :).
Attachment #177802 -
Attachment is obsolete: true
Attachment #178210 -
Flags: review?(mconnor)
Comment 94•19 years ago
|
||
> 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?
Assignee | ||
Comment 95•19 years ago
|
||
(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.
Comment 96•19 years ago
|
||
> 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.)
Assignee | ||
Comment 97•19 years ago
|
||
Mike, can you review this patch? I'd really like to get this in for 1.1.
Comment 98•19 years ago
|
||
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 :)
Comment 99•19 years ago
|
||
this isn't a blocker, but it should make it with months to spare.
Flags: blocking-aviary1.1? → blocking-aviary1.1-
Assignee | ||
Updated•19 years ago
|
Attachment #178210 -
Flags: review?(mconnor)
Assignee | ||
Comment 100•19 years ago
|
||
Mike, are you still "the man" when it comes to getting reviews or is there someone else who could do it sooner?
Comment 101•19 years ago
|
||
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.
Assignee | ||
Comment 102•19 years ago
|
||
This goes in /toolkit/components/viewsource/content/
Attachment #179233 -
Flags: review?(mconnor)
Assignee | ||
Comment 103•19 years ago
|
||
Attachment #178210 -
Attachment is obsolete: true
Attachment #179234 -
Flags: review?(mconnor)
Comment 104•19 years ago
|
||
*** Bug 293109 has been marked as a duplicate of this bug. ***
Comment 105•19 years ago
|
||
*** Bug 297150 has been marked as a duplicate of this bug. ***
Comment 106•19 years ago
|
||
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.
Comment 107•19 years ago
|
||
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.
Assignee | ||
Comment 108•19 years ago
|
||
Attachment #179234 -
Attachment is obsolete: true
Attachment #185980 -
Flags: review?(mconnor)
Assignee | ||
Comment 109•19 years ago
|
||
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)
Assignee | ||
Updated•19 years ago
|
Attachment #179233 -
Flags: review?(mconnor)
Assignee | ||
Updated•19 years ago
|
Attachment #179234 -
Flags: review?(mconnor)
Comment 110•19 years ago
|
||
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.
Assignee | ||
Comment 111•19 years ago
|
||
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.
Comment 112•19 years ago
|
||
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.
Assignee | ||
Comment 113•19 years ago
|
||
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.
Assignee | ||
Comment 114•19 years ago
|
||
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)
Assignee | ||
Comment 115•19 years ago
|
||
This also has been changed to make use of nsIConverterOutputStream from bug 295047.
Assignee | ||
Updated•19 years ago
|
Attachment #189727 -
Flags: review?(mconnor)
Assignee | ||
Updated•19 years ago
|
Attachment #185980 -
Flags: review?(mconnor)
Assignee | ||
Updated•19 years ago
|
Attachment #185982 -
Attachment is obsolete: true
Attachment #185982 -
Flags: review?(mconnor)
Assignee | ||
Updated•19 years ago
|
Flags: blocking-aviary2.0?
Comment 116•19 years ago
|
||
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.
Assignee | ||
Comment 117•19 years ago
|
||
(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.
Comment 118•19 years ago
|
||
See also bug 309321, "Firefox should use OS default text editor for View Source instead of internal View Source window".
Comment 119•19 years ago
|
||
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 120•19 years ago
|
||
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+
Comment 121•19 years ago
|
||
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
> ...
Updated•19 years ago
|
Flags: blocking-aviary2? → blocking-aviary2+
Updated•19 years ago
|
Flags: blocking0.9-
Flags: blocking-aviary1.5-
Flags: blocking-aviary1.0-
Assignee | ||
Comment 122•19 years ago
|
||
> > 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.
Assignee | ||
Comment 123•19 years ago
|
||
Attachment #189726 -
Attachment is obsolete: true
Assignee | ||
Comment 124•19 years ago
|
||
This goes in toolkit/components/viewsource/content .
Attachment #189727 -
Attachment is obsolete: true
Comment 125•19 years ago
|
||
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
Updated•19 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 126•19 years ago
|
||
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.
Assignee | ||
Comment 127•19 years ago
|
||
(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.
Comment 128•19 years ago
|
||
what if the OS doesn't? (e.g. FC4, Windows)
Assignee | ||
Comment 129•19 years ago
|
||
(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.
Comment 130•19 years ago
|
||
filed Bug 318048
Comment 131•19 years ago
|
||
Jason, why is this using getService() to create the nsIProcess _instance_? That should be a createInstance, no?
Assignee | ||
Comment 132•19 years ago
|
||
(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.
Comment 133•19 years ago
|
||
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.
Comment 134•18 years ago
|
||
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 ;)
Assignee | ||
Comment 135•18 years ago
|
||
This should work fine on all platforms.
Comment 136•18 years ago
|
||
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.
Comment 137•18 years ago
|
||
There's no current plan to implement UI for this.
Comment 138•18 years ago
|
||
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.
Comment 139•18 years ago
|
||
Firefox is about not having every single thing on the preferences menu. If you want a gui, use an extension.
Status: RESOLVED → VERIFIED
Comment 140•18 years ago
|
||
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.
Comment 141•18 years ago
|
||
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.
Comment 142•18 years ago
|
||
(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.
Comment 143•18 years ago
|
||
View Source With extension https://addons.mozilla.org/extensions/moreinfo.php?id=394&application=firefox
Comment 144•18 years ago
|
||
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.
Assignee | ||
Updated•18 years ago
|
Attachment #189726 -
Flags: branch-1.8.1?(mconnor)
Assignee | ||
Updated•18 years ago
|
Attachment #189727 -
Flags: branch-1.8.1?(mconnor)
Comment 145•18 years ago
|
||
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-
Updated•18 years ago
|
Attachment #189727 -
Flags: approval-branch-1.8.1?(mconnor)
Comment 146•18 years ago
|
||
Mike, Why did you ever approve the patch if it's unacceptable?
Comment 147•18 years ago
|
||
(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.
Assignee | ||
Comment 148•18 years ago
|
||
(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)?
Comment 149•18 years ago
|
||
(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 150•18 years ago
|
||
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+
Comment 151•18 years ago
|
||
(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.
Comment 152•18 years ago
|
||
Includes the followups from bug 318073 and bug 319006.
Comment 153•18 years ago
|
||
(In reply to comment #152) > Created an attachment (id=220026) [edit] > Branch rollup Landed this on the 1.8 branch.
Updated•18 years ago
|
Comment 154•18 years ago
|
||
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. :-\
Comment 155•18 years ago
|
||
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?
Comment 156•18 years ago
|
||
(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.
Comment 157•18 years ago
|
||
Why? It seems to be entirely a problem with this patch.
Assignee | ||
Comment 158•18 years ago
|
||
CC me on the new bug you file, too.
Comment 159•18 years ago
|
||
(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".
Comment 160•18 years ago
|
||
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.
Description
•