The checkbox shouldn't have existed from the start (except in Debug ui, maybe) and is becoming less necessary as speed improvements are checked in.

How about removing the pref in addition to removing the checkbox?

Current testing shows that view source _with_ highlighting is about 2-5 times slower than view source without (depending on page). This is in case we need some sort of hard data here. :) I agree that this pref is better off in debug prefs. I also think we still need this... a typical "big" page takes about 10 seconds on my machine (p3-733). That's a long time of 100% CPU usage.

I think it's the "100% CPU", not the slowness, that's the problem. See bug 129640, "Incremental rendering is busted on fast connections". (The cache is a very fast connection.) bz: how big was the page you tested, and were you testing a 3/6 or later build? I view-sourced http://www.w3.org/TR/REC-html32 (122kB) on 400MHz, and it took 5 seconds.

Try http://www.toronto.com (make sure you have "wrap long lines" _off_ in view source). I'm testing with a 3/7 build with all the view source perf stuff that I've checked into the tree in it. Similar issues can be observed on long documents converted into HTML from MSWord and the like. As a note, the "100%" cpu problem should also be better now that I've made the view-source parser interruptible.... Except on bidi pages or pages with no linebreaks we should no longer freeze up the UI....

--> change qa to pmac@netscape.com

so we agree that it should remain turned on by default, right? There is no space in the debug preference ui for an additional checkbox, so we might have to kick it out and perf testers will have to manually change it (or hack a menuitem in the viewsource xul to change the pref).

taking

removes the groupbox and the entities from the dtd

Comment on attachment 84037 [details] [diff] [review] the patch r=neil@parkwaycc.co.uk

Why not remove the pref, too? To put it differently, who do you expect will turn it off?

I suspect this pref will become a lot more attractive this summer when I fix view-source to actually report the correct source when highlighting is off.... (the highlighted version will continue to show the crap it shows now).

If the pref is not removed, it should be moved to the View menu of View Source. It doesn't fit with the other items in Appearance -> Colors prefs, and more people use prefs than view source.

Comment on attachment 84037 [details] [diff] [review] the patch please readd to view menu in viewsource

This is separate from the patch to remove the option from preferences.

> + PageLoader.LoadPage(PageLoader.currentDescriptor, pageLoaderIface.DISPLAY_NORMAL); I'm pretty unconvinced that this'll do the right thing wrt to cache and such... Also, what happens if I have multiple "view source" windows open at once and I toggle the menuitem in one of them. Should they all change, or just the one I toggled?

> > + PageLoader.LoadPage(PageLoader.currentDescriptor, pageLoaderIface.DISPLAY_NORMAL); > I'm pretty unconvinced that this'll do the right thing wrt to cache and such... Actually it seems to be the only way to do the right thing, everything else gives me the POSTDATA prompt. I use DISPLAY_NORMAL because the currentDescriptor is already for a view-source: URL. > Also, what happens if I have multiple "view source" windows open at once and I toggle the menuitem in one of them. Should they all change, or just the one I toggled? Well, wrap long lines only changes the current and new windows...

Comment on attachment 90428 [details] [diff] [review] Add View/Syntax Highlighting menuitem to source window >+ document.getElementById("cmd_highlightSyntax").setAttribute("checked", gPrefs.getBoolPref("view_source.syntax_highlight")); What if getBoolPref throws an exception (eg the pref is just not set)? >+ var pageLoaderIface = Components.interfaces.nsIWebPageDescriptor; const, not var. >+ var PageLoader = getBrowser().webNavigation.QueryInterface(pageLoaderIface); Do we need a getPageLoader() function in this file yet? > <menuitem id="menu_wrapLongLines" label="&menu_wrapLongLines.title;" accesskey="&menu_wrapLongLines.accesskey;" type="checkbox" command="cmd_wrapLongLines"/> >+ <menuitem label="&menu_highlightSyntax.label;" accesskey="&menu_highlightSyntax.accesskey;" type="checkbox" command="cmd_highlightSyntax"/> 80-column lines, please? I know the existing code does not do that; you could fix that too. ;)

Comment on attachment 91093 [details] [diff] [review] Addressed bz's comments r/sr=bzbarsky

Comment on attachment 91093 [details] [diff] [review] Addressed bz's comments r=doron, sr=bz

Comment on attachment 84037 [details] [diff] [review] the patch doron, I assume this bit can be resurrected now? If so, can you or rossi find sr/a= for it as I'm going on holiday soon.

blake can sr, he loves this bug

Comment on attachment 91093 [details] [diff] [review] Addressed bz's comments a=asa (on behalf of drivers) for checkin to 1.1

Comment on attachment 91093 [details] [diff] [review] Addressed bz's comments checked in, leaving open for checkbox removal

Comment on attachment 84037 [details] [diff] [review] the patch sr=blake All that's left is to check this in, right?

yup

Comment on attachment 84037 [details] [diff] [review] the patch a=asa (on behalf of drivers) for checkin to 1.1

I noted that the initial state of the checkbox isn't reflected properly. i.e., when you launch view-source for the first time (on a profile for which the pref has never been created), the syntax_highlight checkbox doesn't appear as checked. Unlike wrap_long_lines which defaults to false when the pref doesn't exist, syntax_highlight defaults to true. This distinction isn't enforced in the patch.

Not true, syntax_highlight defaults to false if the default pref doesn't exist. But in current releases, it does exist, and it is set to true.

This is fixed, right?

re: comment 29 It might have been in the window of "View Selection Source" that I saw what I reported in comment 28. It is still there, but seems now covered in bug 160924.