Closed
Bug 52154
Opened 23 years ago
Closed 23 years ago
Make gui for view source coloring pref (was make a pref...)
Categories
(SeaMonkey :: UI Design, defect, P3)
SeaMonkey
UI Design
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.9
People
(Reporter: akkzilla, Assigned: gerv)
References
Details
(Whiteboard: [se-radar])
Attachments
(12 files)
3.85 KB,
patch
|
Details | Diff | Splinter Review | |
3.94 KB,
patch
|
Details | Diff | Splinter Review | |
3.82 KB,
patch
|
Details | Diff | Splinter Review | |
22.47 KB,
image/png
|
Details | |
494 bytes,
patch
|
Details | Diff | Splinter Review | |
1.36 KB,
patch
|
Details | Diff | Splinter Review | |
573 bytes,
patch
|
Details | Diff | Splinter Review | |
573 bytes,
patch
|
Details | Diff | Splinter Review | |
542 bytes,
patch
|
Details | Diff | Splinter Review | |
541 bytes,
patch
|
Details | Diff | Splinter Review | |
5.15 KB,
patch
|
Details | Diff | Splinter Review | |
929 bytes,
patch
|
Details | Diff | Splinter Review |
Gervase Markham wrote: > In http://bugzilla.mozilla.org/show_bug.cgi?id=49187 you say: > > "Source coloring can be switched back on by #defining > VIEW_SOURCE_COLORING in nsViewSource.HTML. It is currently off since that > substantially speeds up the display of the source window." > > Will this become a user choice? Vidur asked if someone in the editor team would be willing to do this, so I'll step up to the plate. Any suggestions for the name of the pref?
Reporter | ||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Comment 1•23 years ago
|
||
taking for qa. this would be tres nifty (since it's avail in 4.x). any hope of getting this in for 6.0?
Assignee | ||
Comment 2•23 years ago
|
||
Nominating for nsbeta3, based on Akkana's email estimate of "trivial - less than an hour's work". Also, seeing as we had source highlighting before, this is a regression. Pref: "Syntax highlighting when Viewing Source" for the UI, dunno the namespace for the internal name so can't comment. Gerv
Keywords: nsbeta3,
regression
Comment 3•23 years ago
|
||
moving to m19, this can wait till after b3 push
Whiteboard: se-radar → [nsbeta3-]se-radar
Target Milestone: --- → M19
Comment 5•23 years ago
|
||
If this is trivial to impliment, then why not make it nsbeta3+? I might assign this one to myself. If this isn't trivial, then turn syntax hilighting back on by default. Since when did maximum speed become more important then readability when viewing source?
Comment 6•23 years ago
|
||
jce2: If you want to do it, then you do not need nsbeta3+. Unfortunately, Netscape is currently focussing on issues that are considerably more important than a pref to change how pretty View Source is. Pleeeease do it... ;-)
Comment 7•23 years ago
|
||
I'm taking this one. I'll land it in two steps. Step one will be the back end stuff, making it so you can add "browser.view_source_syntaxs_hilight=true" to your prefs file manually. Step 2 will be adding the preference to the pref panel. I need UI advice (for example, what pane should it go into?). I'm about to attach a patch for step 1.
Assignee: akkana → jce2
Status: ASSIGNED → NEW
Comment 8•23 years ago
|
||
Ok, that would be user_pref("browser.view_source_syntax_hilight", true); Same difference. :P
Comment 9•23 years ago
|
||
Comment 10•23 years ago
|
||
Adding keywords "patch" and "review". I've tested this patch on my build and it exhibits the following behaviour: Preference not in prefs.js = No syntax hilighting. preference set to true = syntax hilighting. preference set to false = No syntax hilighting. So checking this in is very low risk, and won't affect the default behaviour. Please review it and check it in so people who download nightly builds can turn syntax hilighting back on. :)
Assignee | ||
Comment 11•23 years ago
|
||
How about browser.view_source.hilight_syntax ? Of course, I think it should be "highlight", but I'm English :-) Gerv
Comment 12•23 years ago
|
||
How about a better pref name: browser.view_source.syntax_hilight or something. I can imagine that we may come up with different view source prefs in future.
Reporter | ||
Comment 13•23 years ago
|
||
Staying on the cc list (thanks for taking it!) and will look at the patch. Me, I'd prefer browser.view_source.syntax_highlight (it's long anyway, why not take the extra 2-char hit and spell it right?) but the important thing is to get it checked in.
Reporter | ||
Comment 14•23 years ago
|
||
The patch looks fine (though I suggest we switch to using NS_LITERAL_STRING as long as we happen to be in that code) and I've sent mail asking for approval to check this in. (We can change the name of the pref, or not, but it doesn't affect the validity of the patch.)
Comment 15•23 years ago
|
||
Comment 16•23 years ago
|
||
Ok, now the pref is called "browser.view_source.syntax_highlight" Adding "approval" keyword.
Keywords: approval
Comment 17•23 years ago
|
||
I sent comments by mail, echoing akkana's plea for NS_LITERAL_STRING and NS_WITH SERVICE, adding emphasis. I also whined about non-conforming if/brace style. If the substantive items above (NS_LITERAL_STRING, NS_WITH_SERVICE) get fixed, then a=brendan@mozilla.org. /be
Comment 18•23 years ago
|
||
Comment 19•23 years ago
|
||
I've now got a r=akkana and an a=brendan for the patch for step one. Please check it in. I find it a bit disturbing that I haven't gotten any UI suggestions for part two (as in what preference pane it should go into). Adding relnote3 keyword in case step two doesn't make it in before nsbeta3. The Relnote in that case should be the following: "Syntax highlighting in the view source window is turned off by default for better performance. To turn syntax highlighting on, add the line "user_pref("browser.view_source.syntax_highlight", true)" to your prefs.js file."
Keywords: relnote3
Comment 20•23 years ago
|
||
*** Bug 52509 has been marked as a duplicate of this bug. ***
Comment 21•23 years ago
|
||
Akkana: Now that we have a r= and an a=, can you check this in for me?
Reporter | ||
Comment 22•23 years ago
|
||
Definitely. I haven't been able to today, due to the tree being closed, but I expect I should be able to check it in tomorrow.
Reporter | ||
Comment 23•23 years ago
|
||
The patch is in! Thanks again for the contribution.
Comment 24•23 years ago
|
||
Comment 25•23 years ago
|
||
Comment 26•23 years ago
|
||
Comment 27•23 years ago
|
||
The last two attachments are the patch for step two of fixing this bug. This will create a checkbox in the "advanced" preferences pane. I'm asking for another a=, since I (assume I) have a r=jag. Once this has been checked in, and confirmed to work on all platforms, then this bug can be considered fixed.
Comment 28•23 years ago
|
||
Waterson, please give me an a=? If we don't get this checked in, then people are going to bitch about no syntax highlighting faster then we can tell them to alter their prefs.js file.
Comment 29•23 years ago
|
||
Oh shit. Replace: pref="true" preftype="bool" prefstring="view_source.syntax_highlight" with pref="true" preftype="bool" prefstring="browser.view_source.syntax_highlight" in pref-advanced.xul
Comment 30•23 years ago
|
||
IMHO the pref doesn't fit under "enable features that help interpret webpages" :-)
Comment 31•23 years ago
|
||
Ok, then what panel DOES this belong under? The advanced pane seemed to be the best place to put "misc" stuff. If we aren't getting this pref into nsbeta3, then we should have syntax highlighting enabled by default (add a line to prefs.js setting that preference to true).
Comment 32•23 years ago
|
||
How about the `Colors' panel?
Comment 33•23 years ago
|
||
bug 26593 (unable to ctrl-C from view source) will probably show up again when this pref is on.
Comment 34•23 years ago
|
||
No it won't. Coloured source uses HTML, not XML now.
Comment 35•23 years ago
|
||
BTW, please see http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey for checkin rules (you'll need to get module owner review and super review approval if you want to check this in).
Comment 36•23 years ago
|
||
Hmm, yep. I suggest putting the pref(s) in the Colors panel, under the bit which talks about whether to allow documents to specify their own colors. Ideally, the same colors as are used for HTML source in the source window should be used when showing un-stylesheeted XML source in the browser itself.
Comment 37•23 years ago
|
||
OK, *LAST* call for comments. Should this pref be in the "Colours" panel or not?
Comment 38•23 years ago
|
||
the Colors panel is fine with me. (or, Advanced main panel as a runner-up: since viewing the source might be considered a form a advanced usage wrt endusers.)
Comment 40•23 years ago
|
||
I really think that this should be in under "advanced" after more thought. Colours is really only talking about the colors of links, and other color selections. If we were allowing the user to choose the syntax highlighting colors, then it would be more appropriate to go under colours, but since it is only an on/off switch, I think that it should go under advanced, where there are a lot of other assorted on/off switches. Besides, the average user would want syntax highlighting on. Only an "advanced" person would really need blazing speed in the syntax highlighting window. Is there someone at Netscape that can make a good final UI decision about this?
Comment 41•23 years ago
|
||
Well, let's see. You can cc the UI lead at Netscape, wait a couple weeks, and get a flimsy answer. Or, you can ask Matthew and get a good, solid answer within a matter of days... Your choice.
Comment 42•23 years ago
|
||
mpt (I assume that was the "matthew" that blake was talking about), could you explain further why you originally said that the pref should go under "colors" and not "advanced", given my comments that this is basically an "on/off" switch, and should be grouped with a bunch of other on/off switches (which would be the advanced folder).
Comment 43•23 years ago
|
||
Actually, I don't think this should be a pref at all. If you're the sort of person who would be affected by any slowness of syntax highlighting (i.e. a Web developer or similar geek), you are going to be the sort of person who has a computer fast enough to handle it, and/or you are going to be the sort of person who farms out HTML source viewing to an external text editor anyway. When I suggested the Colors panel, I hadn't read the bug properly (sorry), and I was thinking of controls for changing the actual colors, hidden behind a disclosure triangle: | Category: Colors & Effects ::::::::::::::::::::::::::: | | +-------------------+ | | |=General===========| [::] _Backgrounds [@@] _Links | | |=Display===========| [##] _Text [OO] _Recent links | | | Languages | [@@] _Headings [**] _Highlighted links | | | Fonts | [OO] _Quoted text | | |::Colors:&:Effects:| St_yle for quoted text: [Italic :^] | | | Styles | | | | Multimedia | [/] Allow documents to use _other colors | | | Filters | [/] _Underline links | | | Scripts | | | | Privacy/Security | > Source code options----------------------- | | | | | | | | | | +-------------------+ :::::::::::::::::::::::::::::::::::::::::::: | | Category: Colors & Effects ::::::::::::::::::::::::::: | | +-------------------+ | | |=General===========| [::] _Backgrounds [@@] _Links | | |=Display===========| [##] _Text [OO] _Recent links | | | Languages | [@@] _Headings [**] _Highlighted links | | | Fonts | [OO] _Quoted text | | |::Colors:&:Effects:| St_yle for quoted text: [Italic :^] | | | Styles | | | | Multimedia | [/] Allow documents to use _other colors | | | Filters | [/] _Underline links | | | Scripts | | | | Privacy/Security | V Source code options----------------------- | | | | [%%] Tag_s [**] _Errors | | | | | | +-------------------+ :::::::::::::::::::::::::::::::::::::::::::: | But for now, if it's just a checkbox, I still think it should go in the Colors panel, not the Advanced panel. Four reasons. 1. Hopefully it will stop being a checkbox and start being a pair of colorpicker widgets (as shown above) pretty soon. Then it would absolutely belong in the Colors panel, and having to remember which prefs category it was in (depending on which version of Mozilla you were using) would be a pain. 2. The source code colors are (or should be) used whenever the user views an XML document which contains even the slightest error (bug 52422), which has no stylesheet, or whose stylesheet is missing. Once XML becomes widespread one might imagine that these situations are going to happen fairly often, which would mean that this pref would hardly be `advanced'. 3. The background, text, and URL colors of source code are (or should be) set by the other controls in the Colors panel. To have some colors set by controls in one panel, and other colors determined by a checkbox in another panel, would be a bit annoying. 4. In the future, the prefs will not have an `Advanced' category at all. So if you put stuff there now, you'll only have to move it later. The argument that since this is a checkbox it should go in a panel with other checkboxes seems ... curious, given that many other prefs panels are mixtures of various kinds of controls. Anyway, the Colors panel already has checkboxes in it. Finally, I don't understand why the pref has a name starting with `browser.', given that it is (or should be) also used when viewing source code of e-mail messages in the mailer app, and source code of documents in the editor app.
Comment 44•23 years ago
|
||
if it's changed, how about ui.view_source.syntax_highlight? kinda parallel to the ui.key.accelKey and ui.key.menuAccess pref...
Comment 45•23 years ago
|
||
>If you're the sort of person who would be affected by any slowness of syntax
>highlighting (i.e. a Web developer or similar geek), you are going to be the
>sort of person who has a computer fast enough to handle it, and/or you are
>going to be the sort of person who farms out HTML source viewing to an
>external text editor anyway.
You shouldn't assume that all geeks have fast computers. Also, Mozilla
shouldn't discourage curious people with slow computers from playing with HTML.
Again, mpt, I think you're being over-zealous in trying to simplify prefs.
(btw, there are more than two colors in the view-source window.)
Comment 46•23 years ago
|
||
My two cents: $.01 - I like Matthew's UI design. $.02 - I agree that, for now, this pref (though it should exist) should not have a UI (until everything Matthew mentioned is implemented). We shouldn't have a UI for every prefable thing in the world. If we have this as an option in the UI, then users are probably going to wonder why we didn't just put the coloring on, and not make it an option. Then we'll have to explain that putting the colors on makes the window load slower, which makes us look..well...incredibly stupid (`they can't even figure out how to load a window fast if it has some colored text'). But anyways..
Comment 47•23 years ago
|
||
Exactly. If you have a performance problem with something as basic as coloring text, then work on speeding up the coloring code -- don't just pref your way out of that responsibility. And the argument that I'm trying to over-simplify prefs `again' doesn't really hold water here. Even Ben Bucksch (my chief detractor in that particular debate) agrees that the `Advanced' category of prefs should go away.
OS: Linux → All
Hardware: PC → All
Comment 48•23 years ago
|
||
>don't just pref your way out of that responsibility. Agreed, but there isn't time to fix the performance issue, so the next best options are to not color at all or to add a pref. Coloring suffers from a performance issue, so it shouldn't be forced on users. Also, if we decide to leave coloring out of NS6, we can temporarily work around bug 49030 and bug 44186 by not parsing the code that is displayed by view-source.
Comment 49•23 years ago
|
||
> there isn't time to fix the performance issue We have *months* to fix the performance issue. If by chance you are referring to the Netscape branch, then you would seem to be under the impression that Web developers would rather use Netscape than use Mozilla, and in that case I have a bridge I'd like to sell you. > so the next best options are to not color at all or to add a pref. You missed one option: to put up with it until the performance issue is fixed (like we're putting up with all the other performance problems in Mozilla right now). > Coloring suffers from a performance issue, so it shouldn't be forced on users. Interpreting HTML in the browser window as anything other than a string of ASCII characters causes a performance hit, yet we `force' such a rendering on all users. Showing headers of an e-mail message as a pretty header panel causes a performance hit, yet we `force' that on all users too. > Also, if we decide to leave coloring out of NS6, Neither you nor I are members of Netscape's PDT, so the use of `we' is rather amusing. Anyway, I would guess the chance of such a patch getting rtm++ed at this stage would be remote. > we can temporarily work around > bug 49030 and bug 44186 by not parsing the code that is displayed by > view-source. Ditto for the patch to implement that workaround.
Comment 50•23 years ago
|
||
*** Bug 56264 has been marked as a duplicate of this bug. ***
Comment 51•23 years ago
|
||
rtm-, too late for UI changes on the branch and seems like a true P3.
Whiteboard: [nsbeta3-]se-radar → [nsbeta3-]se-radar[rtm-]
Assignee | ||
Updated•23 years ago
|
Whiteboard: [nsbeta3-]se-radar[rtm-] → [nsbeta3-]se-radar[rtm-] relnote-user
Updated•23 years ago
|
Target Milestone: M19 → mozilla0.9
Comment 53•23 years ago
|
||
*** Bug 61608 has been marked as a duplicate of this bug. ***
Comment 54•23 years ago
|
||
Updating summary to indicate that the pref is there, but there isn't a checkbox in the preferences window yet. Btw, what's the bug for the performance problems, so the pref can eventually be changed to default to "on"?
Summary: Make a pref for view source coloring → Make gui for view source coloring pref (was make a pref...)
Comment 55•23 years ago
|
||
from what i can tell [after grepping the files in defaults/pref/], the pref for browser.view_source.syntax_highlight isn't in any of the files yet. so, i guess this means one would need (1) know about it and (2) add it in manually [eg, your prefs.js] and set it to true. apologies for my pendantic-ness! but i guess my remaining question is, why doesn't this appear in all.js? thx again!
Reporter | ||
Comment 56•23 years ago
|
||
Yes, we need to get a default value for the pref checked in. (I could swear that I made the review contingent upon that, but don't seem to have said so here; if I forgot that, mea culpa, but it's a necessary part of adding a new pref.)
Comment 57•23 years ago
|
||
Clearing out all notations in the Status Whiteboard except "se-radar" because I don't know what that is for. Are we going to expand this bug to cover definition of colours in syntax highlighting, background colours, etc, or do we still only need a single checkbox? And was the default pref ever added to the all.js file?
Whiteboard: [nsbeta3-]se-radar[rtm-] relnote-user → se-radar
Comment 58•23 years ago
|
||
nope, the pref still isn't in the defaults/pref/all.js file. what needs to get done for that to happen, and could it get done soon? thx!
Comment 59•23 years ago
|
||
Comment 60•23 years ago
|
||
actually the pref should be set as: pref("browser.view_source.syntax_highlight", false); to match what's in nsViewSourceHTML.cpp, right...?
Comment 61•23 years ago
|
||
Comment 62•23 years ago
|
||
Well you could use pref("browser.view_source.syntax_hilight", false) in all.js if you actually wanted it to do something ... [second patch does that even though i didn't fix the comment]
Comment 63•23 years ago
|
||
Comment 64•23 years ago
|
||
Comment 65•23 years ago
|
||
would this also enable blinking if tags are not closed appropriately ... as it was a NS 4.X behavior?! This was very useful and some kind of HTML debugging.
Comment 66•23 years ago
|
||
Noting that there really isn't a consensus on what should be done, which is why this bug is just sitting here at the moment. Notes: 1) This bug should NOT be related to the request for a UI that specifies the colours used in the syntax hilighting 2) This bug is also not related to making items flash in the syntax highlighting that aren't well formed. Personal preference: I would like to add a checkbox to the "debug" section of the prefs and be done with it.
Whiteboard: se-radar → There is significant disagreement on what should be done.
Comment 67•23 years ago
|
||
jce: make it so. if someone later wants to move it out of debug they can.
Keywords: relnote3
Whiteboard: There is significant disagreement on what should be done. → [se-radar] There is significant disagreement on what should be done.
Comment 68•23 years ago
|
||
Is there a bug actually filed on the perf problem rather than band-aiding this with a pref?
Comment 69•23 years ago
|
||
*** Bug 71801 has been marked as a duplicate of this bug. ***
Comment 70•23 years ago
|
||
Re markush@world-direct.com's comment from December: I find the tag-blinking in NS4 very annoying, since the it makes it nearly impossible to read the part of the html that's blinking.
Comment 71•23 years ago
|
||
blinking is just on errors,which is a very convenient feature for finding erros.
Comment 72•23 years ago
|
||
With the new cache in the delay in the display of highlighted HTML is not as noticable because previously you had the overhead of fetching the source from the network and highlighting it. On most reasonably sized HTML pages the delay in displaying the source is not as noticable. Some work should be done on making the highlighting more effecient but it's probably fast enough to enable it by default (but have a pref in the ui). We need something in view source to make it better than the IE default of opening in notepad and highlighting is useful.
Assignee | ||
Comment 73•23 years ago
|
||
Right. Enough discussion :-) What I plan to do here (unless Jason wants to take the job off me, and he's quite welcome to) is: a) change the name of the pref to "view_source.syntax_highlight". It's not a browser-only pref and view source is probably big enough and cross-product enough to stand on its own. If there is a spec for pref names, point me to it. b) Add a single checkbox to Preferences | Appearance | Colours for it. This will default to "on". Later, this will be expanded to allow colour selection etc., when anyone gets around to implementing it, according to mpt's UI spec. c) Get a default pref of "true" checked into all.js. I think that if performance is poor, we shouldn't hide it behind having a sucky view-source window. Any anyway, isn't someone rewriting it? I will begin implementing this strategy in 48 hours unless anyone objects before then :-) Gerv
Assignee | ||
Comment 74•23 years ago
|
||
Assignee | ||
Comment 75•23 years ago
|
||
OK, patch attached implementing my plan from before. Looking for r= from akkana? Gerv
Assignee: jce2 → gervase.markham
Status: ASSIGNED → NEW
Keywords: regression → mozilla1.0
Whiteboard: [se-radar] There is significant disagreement on what should be done. → [se-radar]
Reporter | ||
Comment 76•23 years ago
|
||
I like the plan as stated; the code changes look fine to me; with the default value for the pref, the view source window comes up with syntax highlighting and doesn't take TOO long. This quickie review is unfortunately all I have time for right now (I've been called out of town on a family emergency and I'd planned to leave five minutes ago) but it looks good and I don't want to block it, so r=akkana. If anyone else sees anything objectionable, please speak up.
Comment 77•23 years ago
|
||
Looks good to me! Looking forward to using this. sr=hewitt
Comment 78•23 years ago
|
||
[Registering interest on coloring w.r.t. MathML where the tag-to-content ratio is large. Usually, single characters are surrounded by a deep hierarchy of tags. Coloring greatly helps to wade through the markups.] The observed perf problem that has been lingering is now being worked out in bug 74486.
Blocks: 74486
Fix landed.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
![]() |
||
Updated•23 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
![]() |
||
Comment 80•23 years ago
|
||
Er... looking at the patch: + <checkbox id="syntaxHighlight" label="&syntaxHighlight.label;" accesskey="&syntaxHighlight.accesskey;" + pref="true" preftype="bool" prefstring="browser.view_source.syntax_highlight" prefattribute="checked"/> And +pref("view_source.syntax_highlight", true); And +thePrefsService->GetBoolPref("view_source.syntax_highlight", &syntaxHighlight); Which of these pref names is not like the other two? Needless to say this does not work. Reopening.
![]() |
||
Comment 81•23 years ago
|
||
r=blake, r=stephend, I can't believe that was missed in the 1st place. Thanks, Boris. Fix checked in, for real this time.
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 83•23 years ago
|
||
<shuffles feet> Er... not sure how that happened. Ahem. Gerv
Comment 84•23 years ago
|
||
Thanks for finishing this. I had patches that would place the checkbox in debug, but for some inexplict reason, it wasn't working for me! My project at work is getting close to shipping, which is why I keep lacking time to get my bugs fixed. :-(
Comment 85•23 years ago
|
||
I want this UI removed. I agree with mpt and blake, this feature should NOT need a UI. A pref as an excuse for slow syntax highlighting covers the issue that the syntax highlighting system is slow. If we must have any sort of UI, could it be accessible to the view source window? (maybe as a context menu item or something). Please, before adding pref *UI* to navigator/xpapps components (in this case, Navigator), please get me or someone like me (e.g. mpt, blake, etc) to comment and review.
Assignee | ||
Comment 86•23 years ago
|
||
This bug has had significant UI input from mpt. However, if you want the UI removed, you're the boss - just back out the XUL changes. I believe that should do the trick, but I can't be certain as my net access is currently via Windows CE 1.0. Turning it on by default was the other main thing - I think we are agreed we need to keep that... Gerv
![]() |
||
Comment 87•23 years ago
|
||
I believe Doron at some point had a way to turn this on and off as part of the menus he is making for the view source window. Doron, you want to readd that to your menus? Then the UI in prefs can be backed out if Ben decides to do so.
Comment 88•23 years ago
|
||
yes, i had a checked/unchecked menuitem which did this. I think I still have the code backed up.
Comment 89•23 years ago
|
||
The "Additional Comments From Matthew Thomas (mpt) 2000-10-02 16:40" sum up the weirdness of this checkbox. Since coloring is expected, users will find it weird that there is a pref for it at all. Unfortunately, until recently, the slowness that was at the basis of the pref was on nobody's plate. (Style resolution is expensive. The existing code uses the style='.' attribute which is the slowest way to style a page. The style='.' is _parsed_ and this court-circuits the efforts at speeding the matching style resolution algorithm. Worse: coloring is done by setting this attribute on every tag.) Hopefully the improvements provided by the patch in bug 74486 might help performance-wise so that the pref could be removed/hidden. What is also nice with that patch is that colors are not hard-coded anymore. So while awaiting point 1 in mpt's comments [if this ever gets implemented], users will be able to hand-edit the viewsource.css file to set different colors.
Comment 90•23 years ago
|
||
Gerv, mpt later stated that using prefs to attempt to hide a performance problem is not a good thing. I'd prefer to see the patch that rbs refers to evaluated and checked in rather than weighing down an already heavy preferences dialog with options that will make no sense to a majority of people (even advanced users like myself... who often have the attitudes like "fix the speed problem" or "open notepad like IE")
Comment 91•23 years ago
|
||
I have checked-in the patch.
Assignee | ||
Comment 92•23 years ago
|
||
rbs' patch is in. We need to remove that checkbox and replace it with a way of changing the View Source colours - or do we think that's so trivial that it doesn't need UI? As long as we choose them carefully in the first place... Gerv
![]() |
||
Comment 93•23 years ago
|
||
see bug 62678 for color choices and the like for the view source window.
Comment 94•23 years ago
|
||
It just occurred to me that something was left out when jce2 initially implemented viewsource in html. I am mentioning it here for the record in case someone wants to fix it. It has to do with error reporting. The XML version outputs <error>...</error> [see CViewSourceHTML::WriteTagWithError()]. In the HTML version, this has to become <span class="error">...</span>.
![]() |
||
Comment 95•23 years ago
|
||
bug 76412 filed on the error reporting thing.
Comment 96•23 years ago
|
||
vrfy'ing a couple of things: * coloring in view source is present [has been for a while]. * the frontend pref is in the Colors pref panel [for better or for worse :)] if anyone wants the the frontend pref pulled out or moved elsewhere, i'd prefer a separate bug rather than reopening this...
Status: RESOLVED → VERIFIED
Comment 97•22 years ago
|
||
I have filed bug 129612 to remove this checkbox from the UI (possibly to have it moved to Debug UI, or somewhere far less conspicuous).
Updated•19 years ago
|
Product: Core → Mozilla Application Suite
You need to log in
before you can comment on or make changes to this bug.
Description
•