159336 - View Source with an external program

Status: RESOLVED INCOMPLETE

(Camino Graveyard :: Preferences, enhancement)

Description

[Stephane Moureau]

Would it be possible to add an option in the preferences to allow the View
Source menu command to open the source in an external program (e.g. BBEdit)
rather than in a new tab inside Chimera.

Comment 1

[saari (gone)]

seems like that might be more useful for actual web developers that would be
viewing source. Pink, what do you think?

Comment 2

[Mike Pinkerton (not reading bugmail)]

max and i had a related discussion about this a few days ago. i personally would
like to just punt on the whole issue of what we do for view source and open it
up in another application (TextEdit, etc). others may disagree. i don't think a
pref for what app to use would be appropriate for chimera.

comments?

Comment 3

[Simon Fraser [no longer active]]

Some people use View Source a lot (site authors). I think a pref might be
appropriate (View source -> new window/new tab/helper app).

Comment 4

[Kathleen :Brade]

I don't think we should have an explicit preference just for this; can it be
part of helper apps or something?  Do most people really want this functionality?

I think it's fine as is :)

Comment 5

[Will Budreau]

The current "open in tab behaviour" surprised me

Concerns with current behaviour
- Other browsers usually pop view-source up in a seperate window
- with multiple tabs open, where does the user expecct view-source to open up?
- Current behaviour does not allow side-by-side viewing of source with page

Concern with ONLY handling by apps: TextEdit (presumably the default) does not
do color formatting.

Comment 6

[Adam]

How about opening the view source window, pressing Command+A to select all the text, 
then Navigator->Services->BBEdit->Open Selection? (Not that it actually works, but it's 
supposed to, and will someday, so that's all that should matter.)

View source lets me do just that - *view* the source of the current loaded document. If I 
want to copy a URL from source (e.g., snag the URL to that QuickTime movie so I can 
pass it off to Interarchy - via the useless Services, of course), I definitely don't want to 
have to wait for BBEdit or TextEdit or MozillaViewSourceApplicationSuite to load just for 
that.

This bug is describing an Edit Source feature - something I'm not sure is appropriate for 
Chimera.

Comment 7

[Thomas F. O'Connell]

FWIW, i notice that this bug is targeted for 0.6, and i'd just like to provide a
reference here to bug 153884, which i find to be relevant.

to me, as a web developer who frequently needs to view, but not always edit,
source that is dynamically generated, i would be very satisfied with a way (a
pref works) to specify how to open source. i like the Services suggestion, too.

still, i think that of more immediate concern even than opening in a new
application is a proper semantic description of opening source in Chimera itself
(i.e., one that does not reload the source).

Comment 8

[kaldari]

I have to agree with Mr. O'Connell. I would much rather see bug 153884 fixed
before this one. This bug is mere luxury, while bug 153884 is a serious headache
for web developers. (Sorry for the advocacy comment. I'll shut up now.)

Comment 9

[Torben]

FWIW, related Mozilla bugs are bug 8589 and bug 69329.

Re comment 4: I think you could change this in NS 4x by chosing a helper
application for netscape/source.

Comment 10

[Michael Scheel]

In the process of working on this, I had to attend to some items in Bug 153884
(view source caching/hitting server), in order to resolve getting the actual
source code of the visible window/frame, instead of re-downloading it or getting
it from the cache.  Anyway, I'll repost my comments here:

I have a version that does this, sort of correctly.  I didn't actually try
to get it to show up in the tab/window, since I was actually attempting to do
view-source-in-external-app (see Bug 159336).  It currently saves the current
window (or frame) to disk (in a hardcoded location) and calls BBEdit (also
hardcoded location)  I know, I'm a terrible person, but I'm still making this
work correctly.  Anyway, the way I found to do it, in case it inspires someone,
is to make a function in CHBrowserView called saveDocumentSource, that just
needs to know whether it's a frame or not, and a fileName:(NSString *)

I'll post the whole thing on my website later this weekend, I hope, if I clean
it up, but the bare ideas of saveDocumentSource look like:
nsCOMPtr<nsIDOMWindow> domWindow;
_webBrowser->GetContentDOMWindow(getter_AddRefs(domWindow));
nsCOMPtr<nsIDOMDocument> domDocument;
domWindow->GetDocument(getter_AddRefs(domDocument));
nsCOMPtr<nsIWebBrowserPersist> webPersist(do_CreateInstance(kPersistContractID));
(set up a bunch of flags and an nsILocalFile)
webPersist->SaveDocument(domDocument, saveFile, nsnull, "text/html",
encodingFlags, 80);

This correctly dumps the window contents, without caching or hitting the server
again, to disk, or changing any of the HTML (assuming you set the webPersist
flags and encodingFlags correctly, which I think I have, but need to check more)

I don't know how to get this into the real Chimera source, or whatever, but I
have my own Navigator.app that I'll keep tweaking to make this correctly
configurable (pick your own app, turn it on/off, auto-gen the filename, that
kind of stuff, shouldn't be too hard ;)  and I'll release that at:
http://www.osxtreme.net/software/Chimera_Custom/
Look for version 1.3 when I post it.  It'll have the pre-built .app and the
.tgz with the source code.  (relevant to this feature is BrowserWindowController
and CHBrowserView, look for "saveDocumentSource")

Comment 11

[Simon Fraser [no longer active]]

There's also a way to prevent view source from hitting the server by using
nsIWebPageDescriptor. I'm not sure if that ends up using a similar code path.
This is discussed in bug 153884.

Comment 12

[Michael Scheel]

I took a brief look at nsIWebPageDescriptor, I'm not sure if that's a better way
to implement this or not, it'll take more digging.  For the time being, this
seems to work very well, tho - it pulls from the window, not the cache or the
server, like it 'should'.

Among other features, which you can read about and download from
http://www.osxtreme.net/software/Chimera_Custom/

        -Viewing source internally respects the same tab-or-window choice
                as Preferences->Navigation->Tabbed Browser, what to do on
                command-click
        -Viewing source externally can be selected in Custom preference
                as well as what program to use.  If it fails to save or
                has no program, it will pop up a dialog, and ask whether
                to cancel or just view internally (in tab or window).
                Note this source is pulled directly from the open window,
                so it doesn't get it from the cache or the webserver.

I'll try to turn this into a patch instead of a tarball of specific source
trees, but if anyone has pointers on how to do that, or if I should even bother,
please let me know.  I'd like to contribute to Chimera, if someone would accept
my help.

(crossposted to Bug 153884)

Comment 13

[Simon Fraser [no longer active]]

Assuming you pulled source from CVS:

cd mozilla/chimera
cvs diff -u 12 > ~/chimeradiff.patch

Some files are binary, and won't show up in the patch (somme .strings files,
parts of .nib files etc.). When you changed those, you should attach a .tar.gz
of them, and also describe in the bug what you changed.

Comment 14

[Michael Scheel]

Attachment: Patch to implement this feature

This patch supplies the source to view source externally, and have view-source
internally respect the cmd-click user preference.  An additional attachment has
the binary Localized.strings file and a patch.txt explaining how to apply and
use these.

Comment 15

[Michael Scheel]

Attachment: Localizable.strings and patch.txt for my patch

Here's the Localizable.strings and patch.txt instructions.

Comment 16

[Michael Scheel]

This is covered in patch.txt, but I'd like some input: This requires two
preferences (in the org.mozilla.navigator.plist), one to enable the feature, and
another to give the path to the External application.  It's easily added by
hand, but that's clearly not the final solution, assuming this gets approved.

What's the best way to make this kind of change?  I could have my own
PreferencePane (I already do, for all my random change ideas), but that's a pain
to submit because it requires changes to the ProjectBuilder files; also, that
involves more UI changes than would be made final, anyway.  I could patch, say,
the WebFeatures pane, but since that involves binary .nib files, my patch/.tgz
would get out of date as other changes went in.  As it is, I've left it to
manual .plist tweaking, but I'm open to better ideas.

Comment 17

[Stephane Moureau]

Michael, perhaps this can be integrated in the "Extended panel" (EP) that can be
easily extended. The extended panel would be 'simply' a GUI to third party
module. See bug 184394.

All new extensions would be added to that pane.
I'm thinking to a pane like in MSIE Preferences:
On the left part of the EP a list of the new extensions (icons with title under
it) with a scroll bar.
Clicking on one in the left part will show all its settings in the right part of
the EP, the currently selected extensions options.
That would be a great way to add new features and functionnalities without
cluttering the interface. All pro-features (user_pref, new options) that some
users want and will probably not be included in Chimera (must be simple), even
if some are already accessible via user.js, can be added there.
See (old) Eudora Esoteric add-ons, you can configure very weird settings.

Comment 18

[Simon Fraser [no longer active]]

Thanks for the patch! With a brief look, it needs quite a bit of cleanup. I'm
not sure how we want to deal with this feature, but this is a good starting point.

Comment 19

[Michael Scheel]

Great.  I'm happy to assist with cleanup, if that will help; or I'll sit here
patiently.  Not sure if you mean things like function names,
placement/organization, whitespace, comments, other, or the ever popular All of
the Above, but I'll happily rip my changes out and put them back in correctly in
the name of progress ;)

Comment 20

[Michael Scheel]

Attachment: Sample preference pane

If you don't want to edit the .plist by hand, or just want to see my example of
how this could look in a preference pane:

This is my Custom preference pane.  See the .txt file in the _bundle.tgz file
attached, basically you need to build Navigator.app and then apply the .patch
and unpack the .tgz in order to recognize and include my Custom PreferencePane
binary.  You can then go to Preferences->Custom (and ignore my other
preferences, sorry) and select "View source in external application" and then
click "Pick external viewer" to pick an application.  BBEdit is recommended. 
TextEdit will try to render the HTML, so it's not very useful other than to see
that this feature works.

(This is the Custom preference pane from my Chimera Custom build, referenced
above)

Comment 21

[Brad Kemper]

I no longer seem to have the ability to view the source within Chimera for
certain pages. When I "view page source", it immediately starts downloading,
then opens it in Internet Explorer (my default browser (sorry)). 

For instance, that is what happens on this page:
http://bugzilla.mozilla.org/buglist.cgi?product=Chimera&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&bugidtype=include&changedin=1&chfieldfrom=&chfieldto=Now&chfieldvalue=&cmdtype=doit

But it does not happen that way on this page:
http://bugzilla.mozilla.org/show_bug.cgi?id=159336
(the page you are looking at now).

According to the new download manager, it's destination (for the source
downloaded in the first example) is:
~/Documents/In Basket/bugs-2002-12-31-1.html
(my download folder is "~/Documents/In Basket/")

By the way, when it opens in IE, it looks like the normal rendered html page,
except it has this at the top:

--thisrandomstring Content-Type: text/html 

Please stand by ...
--thisrandomstring Content-Type: text/html Set-Cookie:
LASTORDER=bugs.delta_ts%2C%20bugs.bug_status%2C%20bugs.priority%2C%20map_assigned_to.login_name%2C%20bugs.bug_id
; path=/; expires=Sun, 30-Jun-2029 00:00:00 GMT Set-Cookie:
BUGLIST=172259:186698:187047:181229:184394:153884:187155:187164:184994:178607:175029:183401:184273:155218:187169:186591:184407:185916:186965:186964:185985:176911:178063:187193:168564:168627:182583:183313:187231:186628:187234:186757:187233:159336:184756:187261:186401:187271:181243
; path=/; expires=Sun, 30-Jun-2029 00:00:00 GMT 

Comment 22

[sairuh (rarely reading bugmail)]

re comment 21: it's prolly due to a known issue in mozilla --see bug 76816 (or
bug 84542).

Comment 23

[Jasper]

News please, seems to be assigned and patch is available but never was reviewed
or checked in. This bug still is an issue for web developers.

Comment 24

[Torben]

AppleScripts for doing this is available at
http://www.webgraph.com/usability/applescripts/

Would be nice to have this built into Camino (and Mozilla) though.

Comment 25

[Bruce Davidson]

Jasper, the current patch won't apply to the source tree as its gone significant
change since then (not least changing "chimera" to "camino"). It may be possible
to rework the patch.

Comment 26

[Jasper]

Bruce Ithink it's worth while looking if you can make the patch so that it
applies again.

1) I really don't know where we should place source view preferences.
2) just a simple pop-up menu should do fine:

Where would like to open source view:
Camino
-------
Select...

And adding two options for setting better prefs when opeing source view in
camino self.

(•) open source in a new tab
(•) open source in a new window

We should detirmine what would be the best place to put all this and whether we
really want this. I'd like to see this happen, as a designer point of view. And
I really don't think it's bloat.

Comment 27

[Bruce Davidson]

Marking the patch as obsolete and removing the [patch] on the summary - the
patch doesn't come close to applying to the current source tree.

Comment 28

[Samuel Sidler (old account; do not CC)]

This is on the list for 1.0. Has anyone got time to look at it? 

Comment 29

[Torben]

Attachment: Patch that applies

I've updated Michael's patch so it applies on the current tree. To test this
follow the instructions in patch.txt in attachment id=110339 (don't use the
supplied Localizable.strings, it's very outdated, apply the changes manually).
Tested and working with TextWrangler, TextEdit and M$ Word (the two later try
to display as html). 

If this is what we want, I'll clean up the patch if necessary and attach an
updated Localizable.strings. Someone else will have to update the preference
pane though,  I know nothing about this.

Comment 30

[Torben]

(In reply to comment #29)
> the two later try to display as html). 

This is fixed by saving the source as text/plain and not html.

Comment 31

[Simon Fraser [no longer active]]

I don't think we want this for 1.0.

Comment 32

[Jasper]

Bump, there are many of use that would love this patch. If not for the branch
lets' only check it in for the trunk. I'd really appreciate it.

Comment 33

[Simon Fraser [no longer active]]

Patch still needs work, not going to make 1.0. I'm not even sure if we want this.

Comment 34

[Samuel Sidler (old account; do not CC)]

For what it's worth, FF just landed a patch on the trunk that does it. (bug 172817)

Comment 35

[Torben]

Attachment: Updated patch w/o UI

This patch reuses the FF user.js-preferences, no UI supported. In case of failure, Camino will use the internal viewer (and print an error message in the console). Also removed debug-code and fixed some potential crashes. Not requesting reviews until we decide if we really want this.

Comment 36

[Chris Lawson (gone)]

Torben, how does your patch interact with that from bug 188604 (which looks fairly likely to happen for 1.1)?

cl

Comment 37

[Torben]

(In reply to comment #36)
> Torben, how does your patch interact with that from bug 188604

The only interaction is that the view externaly preferences will override the view in tab/window preference. And there is some changes in the surrounding code so this patch won't apply after the patch for bug 188604 lands, I'll make a new patch as soon as this happens, _if_ we actually want this.

Comment 38

[Torben]

Attachment: Updated patch

Asking for sr to decide whether we want this or not.

Comment 39

[Samuel Sidler (old account; do not CC)]

I think it would be good to have this as a hidden pref (as I believe the patch currently implements). I don't see it needing UI however.

Comment 40

[Håkan Waara]

Comment on attachment 215801 [details] [diff] [review]
Updated patch

>+  // first check if we have an application to call at all
>+  NSString* extApp  = [[PreferenceManager sharedInstance] getStringPref:"view_source.editor.path" withSuccess:NULL];
>+  if (!extApp) {
>+    NSLog(@"Cannot view source with external application: no application specified");
>+    return NO;
>+  }

I think it'd make sense to just use the default text editor app, rather than just bail.

Comment 41

[Mike Pinkerton (not reading bugmail)]

seems like storing a path is a silly thing to do. shouldn't we be storing an base64-encoded FSRef or alias?

Comment 42

[Torben]

(In reply to comment #40)
> >+  // first check if we have an application to call at all
[...]
> I think it'd make sense to just use the default text editor app, rather than
> just bail.

Maybe, the main problem I see is that the default on OS X is TextEdit, which doesn't treat .html-files as plain text (see comment #29) :-( Oh, and pretend I never wrote comment #30, that just makes things worse, the correct thing (as done in the last patches) is passing nsnull. In addition we need to figure out what the default app is...

Both of this is easily "fixed" by always adding a .txt-extension to the temp-file but then decent text editors wouldn't apply colour coding etc :-( And we would have to copy local files to the temp-dir.

IMHO this is a feature for advanced users that shouldn't have any problems giving a correct path to their preferred app.

BTW, we don't just bail, we fall back to the internal viewer, respecting the tab/window pref.

(In reply to comment #41)
> seems like storing a path is a silly thing to do. shouldn't we be storing
> an base64-encoded FSRef or alias?

Why? Both NSWorkspace and NSFileManager use paths, to use FSRefs this would have to be rewritten to use Launch Services, which AFAICT would be a lot more code.

Comment 43

[Simon Fraser [no longer active]]

We should store the app's bundle identifier.

Comment 44

[Torben]

(In reply to comment #43)
> We should store the app's bundle identifier.

You mean in user.js/prefs.js? If so, is there any easy way for the user to finding this for a random program? We'll also lose compability with the firefox-prefs...

Comment 45

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #44)
> (In reply to comment #43)
> > We should store the app's bundle identifier.
> 
> You mean in user.js/prefs.js? If so, is there any easy way for the user to
> finding this for a random program? 

Look for the app's plist?  The bundle identifier, IMO, makes more sense, and is more Mac-like, than storing a hard path.  Mac users like to move and rename their apps (and some Adobe & friends apps include the version in the appname, so when you upgrade the app, the pref would break.

It would be a bit difficult to automatically supply the bundle identifier easily for a hidden pref, but again, this is a hidden pref, so presumably the people that want this feature could figure out the bundle identifier and how to enter it.  It's no harder than manually entering a full path, too--probably considerably easier, actually.

> We'll also lose compability with the firefox-prefs...

I don't think that's very important at all; it's a "nice to have" when it makes sense, but it doesn't always make sense given the xp nature of Fx.

Comment 46

[Håkan Waara]

I really think almost anything is a better default than using our half-working view source parser. 

The Camino spirit is to defer these things to the OS default, just like we'll do with RSS feeds. :) We just need a way to get around what Torben observed in comment 42. 

Would changing the type/creator to TEXT/ttxt fix that? Use MIME types?

Comment 47

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

InternetConfig has a nice pref for a default editor app; it's a shame IC is all but dead and this never made it into LaunchServices.

The problem with doing the right thing by simply letting the OS decide what to do is that for common document types, there are lots of reader and editor apps and ending up with the right one is still too much of a crapshoot--yes, there's the hierarchy of LS bindings, but for our purposes it's (often) going to come down to the old HFS "last installed app" unless the user has specifically done a global override for the extension.  

For some file extensions (html, js, css) the OS might even end up sending the file back to the browser by default.  I imagine many people keep at least html assigned to a browser by default and rely on their apps being well-behaved and setting a proper creator code to ensure pages they're creating/editing end up opening in the editor and the rest be browser-viewable.  (MIME types are going to have the same infinitely-recursive issue; the default handler for text/html is likely going to be the browser.)

And then there are the issues with TextEdit being brain-dead (foo.html with type/creator TEXT/ttxt still causes TextEdit to open the file and render the HTML) and proper extensions needed for syntax-highlighting in some editors and so forth....

Without a "default editor" type pref in LS (and/or a pseudo-protocol), I think the best thing we can do is target an app explicitly.  And the most Mac-like way to do that is to store the bundle identifier in the pref and use that to find the app; that should protect us from users/app upgrades moving or renaming the app, which would break paths.  

I hereby promise to document the bundle identifier as the pref value (and how to find it...look for the editor app's .plist) on the Hidden Prefs page when the pref appears in a release.  (As I said before, I don't think discovering that is going to be too complicated for the target audience for this pref...and if it turns out to be, this is the perfect sort of thing to end up in a 3rd-party "Web Developer" prefPane...expose this pref, spawn a window to display the JS/CSS console, etc.)

Comment 48

[Torben]

Attachment: Patch with appbundle

Preferences renamed to camino.viewsource.external and camino.viewsource.app, the last now uses the bundle identifier.

Comment 49

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 224366 [details] [diff] [review]
Patch with appbundle

I tested this patch and have a few comments:

1) It doesn't apply on the trunk; the BWC.h portion fails.

2) After applying that part manually, the patch works pretty well; when the app can't be found, it falls back to the internal impl, which is nice.  I assume it does this for the other error conditions that are NSLog()ed, but I didn't check.  

3) Given that this pref and both of the states in camino.viewsouce_in_tab are mutually exclusive, it makes more sense to make the "destination" pref a unified 3-way pref, something like camino.viewsource.location (window, tab, app).  (Currently this pref takes precedence over the tab/window one, which is correct IMO, but if a user has set both at some point, it might be confusing, and a 3-way pref is clear.)

4) When someone's set the camino.viewsource.app pref to org.mozilla.camino, we get the source back and attempt to render it.  It's not the end of the world, but if we fell back to our own parser to begin with in that case, it would be better.  

Also, if we could prefill our bundle identifier in the .app pref, people would have a "sample" to see what to change; might be more confusing, so just a thought.

5) There are some icky behaviors that will mostly be solved by bug 159337 and bug 309132 (this patch actually handles the latter better than Camino now), but right now this patch crashes when viewing source of about:config. Until bug 159337 lands, it would be good to see if we can prevent this crash.

6) xhtml (like www.caminobrowser.org home page) gets turned into HTML using the patch (xml declaration and closing / in singleton tags removed, at a glance) while the internal viewer preserves the proper source (straight xml seems to be fine, though).

r- based on those behavioral issues.

Comment 50

[Håkan Waara]

(In reply to comment #49)
> Also, if we could prefill our bundle identifier in the .app pref, people would
> have a "sample" to see what to change; might be more confusing, so just a
> thought.

How about prefilling it to some text app, so people actually notice that this can now be customized? I wonder if we can force TextEdit to show the source as raw text instead of trying to present it as a web page... perhaps with some apple event?

Comment 51

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #50)
> How about prefilling it to some text app, so people actually notice that this
> can now be customized? 

Sure, that would make more sense if...

> I wonder if we can force TextEdit to show the source as
> raw text instead of trying to present it as a web page... perhaps with some
> apple event?

That's been the crux of this bug of late.  No one's figured out a way to make TextEdit do that if the user's pref is set to "render html" (which is the default).  I don't see anything in the AppleScript dictionary, but TextEdit's source is in the AppKit examples, so maybe someone will find some hint there.

Comment 52

[Torben]

> I tested this patch and have a few comments:

> 2) After applying that part manually, the patch works pretty well; when the 
> app can't be found, it falls back to the internal impl, which is nice.  I 
> assume it does this for the other error conditions that are NSLog()ed, but I 
> didn't check.  

Yes, that's what should happen (and does in all tests I've been able to make up).

> 3) Given that this pref and both of the states in camino.viewsouce_in_tab are
> mutually exclusive, it makes more sense to make the "destination" pref a
> unified 3-way pref, something like camino.viewsource.location (window, tab,
> app). 

Maybe, but what if the user wants to chose whether to open a window or tab if the external application fails (this is a rather obscure scenario though)?

> 4) When someone's set the camino.viewsource.app pref to org.mozilla.camino, 
> we get the source back and attempt to render it.  It's not the end of the 
> world, but if we fell back to our own parser to begin with in that case, it 
> would be better.  

Easy to do, but I really don't like special casing any stupid mistakes a user can make (this is a feature for advanced users that should know better).

> Also, if we could prefill our bundle identifier in the .app pref, people 
> would have a "sample" to see what to change; might be more confusing, so just 
> a thought.

As Håkan said a text editor would make more sense, if only Apple had included a decent one with OS X. Maybe we should bundle one with Camino ;-)

> 5) There are some icky behaviors that will mostly be solved by bug 159337 and
> bug 309132 (this patch actually handles the latter better than Camino now), 
> but right now this patch crashes when viewing source of about:config. Until 
> bug 159337 lands, it would be good to see if we can prevent this crash.

:-/ I thought I had tested all "strange" pages, obviously I forgot some.

> 6) xhtml (like www.caminobrowser.org home page) gets turned into HTML using 
> the patch (xml declaration and closing / in singleton tags removed, at a 
> glance) while the internal viewer preserves the proper source (straight xml 
> seems to be fine, though).

An other thing I didn't test (I tested xml-pages though). The problem seems to be that nsIWebBrowserPersist::SaveDocument saves the parsed DOM not the raw source. SaveURI might work better, I'll try to understand what Firefox does (might take some time, I hate Java...).

Comment 53

[Torben]

> > 5) There are some icky behaviors that will mostly be solved by bug 159337 and
> > bug 309132 (this patch actually handles the latter better than Camino now), 
> > but right now this patch crashes when viewing source of about:config. Until 
> > bug 159337 lands, it would be good to see if we can prevent this crash.
> 
> :-/ I thought I had tested all "strange" pages, obviously I forgot some.

Just tested, about:config doesn't crash for me (neither do any other of the abouts). Did you get a chrash log or any messages in the console?

Comment 54

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Crash log from viewing source in about:config on the trunk

(In reply to comment #53)
> Just tested, about:config doesn't crash for me (neither do any other of the
> abouts). Did you get a chrash log or any messages in the console?

Torben, sorry for the delay; I had to un-bitrot your patch again because I didn't grab the console messages or crash log when I initially tried this.

Console says:
###!!! ASSERTION: What kind of weird script element is this?: 'script', file /Users/smokey/Camino/dev/trunk/mozilla/content/base/src/nsHTMLContentSerializer.cpp, line 768
###!!! ASSERTION: You can't dereference a NULL nsCOMPtr with operator->().: 'mRawPtr != 0', file ../../../dist/include/xpcom/nsCOMPtr.h, line 849

Crash log attached.  It's possible I bungled the un-bitrotting, or it might be an OS version thing, or ...?

Comment 55

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Also, "View Frame Source" in the context menu doesn't work correctly with this patch; my external editor loads the source of the frameset page, not of the frame in which I invoked the CM.  (See bug 332102 comment 8 for a simple testcase.)